Make WordPress Core

Opened 5 months ago

Last modified 13 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
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 (5)

group-count.jpg (22.4 KB) - added by birgire 5 months ago.
46895.diff (3.6 KB) - added by birgire 4 weeks ago.
group_display_count.jpg (51.9 KB) - added by birgire 4 weeks ago.
46895-2.diff (5.0 KB) - added by birgire 4 weeks ago.
46895-3.diff (5.7 KB) - added by birgire 4 weeks ago.

Download all attachments as: .zip

Change History (15)

#1 @birgire
5 months ago

  • Description modified (diff)

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.

4 weeks ago

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

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

#6 @SergeyBiryukov
4 weeks ago

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

4 weeks ago

#7 @birgire
4 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.

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 @garrett-eclipse
7 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.

13 hours ago

Note: See TracTickets for help on using tickets.