WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 10 hours ago

#46895 reviewing enhancement

Personal Data Export Report: A way to display the group count

Reported by: birgire Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-screenshots has-patch has-unit-tests commit
Focuses: Cc:

Description (last modified by birgire)

It would be informative to know the number of items in a given ("countable") group.

This could be an option for each group, because not all groups are "countable".

The Comments and Media groups could e.g. benefit from this.

Similar was explored in the early stages of the Site Health admin page #39165.

See e.g. the screenshot for that ticket.

Attachments (8)

group-count.jpg (22.4 KB) - added by birgire 5 months ago.
46895.diff (3.6 KB) - added by birgire 5 weeks ago.
group_display_count.jpg (51.9 KB) - added by birgire 5 weeks ago.
46895-2.diff (5.0 KB) - added by birgire 5 weeks ago.
46895-3.diff (5.7 KB) - added by birgire 4 weeks ago.
46895.4.diff (3.5 KB) - added by garrett-eclipse 10 hours ago.
Updated patch to base count display on count > 1 removing boolean and need for dev-notes
index 4.html (8.4 KB) - added by garrett-eclipse 10 hours ago.
Attached example export with multiple comments, one media and multiple custom erasers this one from WP Mail Logging
34c4823fb10820f7454ed8cba6ad6d50.gif (9.5 MB) - added by garrett-eclipse 10 hours ago.
The export html didn't render in Trac as I wanted so this will better illustrate the export and counts

Change History (21)

#1 @birgire
5 months ago

  • Description modified (diff)

@birgire
5 months ago

#2 @birgire
5 months ago

  • Keywords has-screenshots added

group-count.jpg is an example showing the number of comments within the core Comments group.

#3 @garrett-eclipse
5 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Thanks @birgire this would make sense for repeating groups of items such as Comments.

@birgire
5 weeks ago

#4 @birgire
5 weeks ago

  • Keywords has-patch added; needs-patch removed

46895.diff is a first pass.

It adds a support for the group_display_count boolean key, that determines if the corresponding group count is displayed.

It's displayed for the Comments and Media groups.

#5 @birgire
5 weeks ago

ps: It was helpful to have the implementation of the group_description as a comparison :-)

#6 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Future Release to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@birgire
5 weeks ago

#7 @birgire
5 weeks ago

  • Keywords has-unit-tests added

46895-2.diff uses instead the group_count group key and introduces two test cases:

  • test_group_html_generation_when_group_count_displayed()
  • test_group_html_generation_when_group_count_not_displayed()

in the Tests_Privacy_WpPrivacyGeneratePersonalDataExportGroupHtml class.

@birgire
4 weeks ago

#8 follow-up: @birgire
4 weeks ago

I think there's no need to display the group count for a single item.

Patch updated in 46895-3.diff with a corresponding test case.

#9 in reply to: ↑ 8 ; follow-up: @garrett-eclipse
9 days ago

Thanks @birgire testing this and running the unit tests everything went well, the patch still applies cleanly and all the unit tests run properly.

I also agree with your statement "there's no need to display the group count for a single item" and it begs the question if the group_count (previously group_display_count) boolean is even necessary here. Can we just not display the count if more than one for all groups. I don't believe there's a case where a group has multiple items and the count wouldn't be useful. And by requiring a boolean set would mean a dev-note would be required and the third-party tools that use the exporter will need to be aware of the boolean and update their implementation to utilize it, by simply adding count when >1 all third-party tools automatically are accounted for.

If there are cases where group count isn't desired on multi-item groups another option is a disable boolean which would still require a dev note but then third-party tools will get the count unless they opt-out.

Sidenote - If the boolean makes more sense than just using count>1 I would suggest going back to a more descriptive one like in the initial diff when it was group_display_count as reading $group_count = (bool) $export_datum['group_count']; while I was reviewing had me double-check as on first glance it seemed like the actual group count of items was being cast as a boolean.

Let me know your thoughts on dropping the boolean, I feel it simplifies things and removes the need to educate plugins, etc. via a dev-note.

All the best

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


3 days ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


23 hours ago

#12 in reply to: ↑ 9 ; follow-up: @pputzer
21 hours ago

Replying to garrett-eclipse:

I also agree with your statement "there's no need to display the group count for a single item" and it begs the question if the group_count (previously group_display_count) boolean is even necessary here. Can we just not display the count if more than one for all groups. I don't believe there's a case where a group has multiple items and the count wouldn't be useful. And by requiring a boolean set would mean a dev-note would be required and the third-party tools that use the exporter will need to be aware of the boolean and update their implementation to utilize it, by simply adding count when >1 all third-party tools automatically are accounted for.

I think it might be useful to see at a glance which groups could have more items (but don't), so if we decide to go with the explicit enabling property, I think "Comments (1)" should be displayed.

As for the other approach, automatically adding the group count if item count > 1 sounds very elegant architecturally. I was hesitant for a moment for any BC issues, but since this would only affect the HTML output, it should not have an effect on plugins that add to existing Core groups in their data exporters.

@garrett-eclipse
10 hours ago

Updated patch to base count display on count > 1 removing boolean and need for dev-notes

@garrett-eclipse
10 hours ago

Attached example export with multiple comments, one media and multiple custom erasers this one from WP Mail Logging

#13 in reply to: ↑ 12 @garrett-eclipse
10 hours ago

  • Keywords commit added

Thanks @pputzer I've refreshed the patch 46895.4.diff to use the count of items to toggle display over-relying on a boolean. This means plugins need no change as illustrated in example export https://core.trac.wordpress.org/attachment/ticket/46895/index%204.html which should multiple Comments, a single Media and multiple mails form WP Mail Logging as an example to illustrate that 3rd parties are supported without requiring a change.

Moving this forward to commit as I've also addressed unit test issues and removed the now unneeded one that did a check on the boolean.

@birgire any final thoughts, would love you to test/review.

@garrett-eclipse
10 hours ago

The export html didn't render in Trac as I wanted so this will better illustrate the export and counts

Note: See TracTickets for help on using tickets.