WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 8 weeks ago

#46895 closed enhancement (fixed)

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:
PR Number:

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 7 months ago.
46895.diff (3.6 KB) - added by birgire 3 months ago.
group_display_count.jpg (51.9 KB) - added by birgire 3 months ago.
46895-2.diff (5.0 KB) - added by birgire 3 months ago.
46895-3.diff (5.7 KB) - added by birgire 3 months ago.
46895.4.diff (3.5 KB) - added by garrett-eclipse 2 months 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 2 months 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 2 months ago.
The export html didn't render in Trac as I wanted so this will better illustrate the export and counts

Change History (23)

#1 @birgire
7 months ago

  • Description modified (diff)

@birgire
7 months ago

#2 @birgire
7 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
7 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
3 months ago

#4 @birgire
3 months 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
3 months ago

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

#6 @SergeyBiryukov
3 months ago

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

@birgire
3 months ago

#7 @birgire
3 months 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
3 months ago

#8 follow-up: @birgire
3 months 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
2 months 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.


2 months ago

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


2 months ago

#12 in reply to: ↑ 9 ; follow-up: @pputzer
2 months 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
2 months ago

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

@garrett-eclipse
2 months 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
2 months 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
2 months ago

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

#14 @birgire
2 months ago

Thanks for your feedback @garrett-eclipse and @pputzer

I like the simplifications in 46895.4.diff, the screenshot and HTML file are also helpful. Thanks Garret

It seems to work as expected for the Comments and Media sections.

The Privacy test group run successfully.

So this looks good to me.

#15 @SergeyBiryukov
8 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 46209:

Privacy: Display group items count in the personal data export file if there's more than one item in the group.

Props birgire, garrett-eclipse, pputzer.
Fixes #46895.

Note: See TracTickets for help on using tickets.