WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 6 months ago

Last modified 5 months ago

#46894 closed enhancement (fixed)

Personal Data Export Report: Table of contents for easier navigation

Reported by: birgire Owned by: xkon
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-screenshots has-patch has-unit-tests commit has-dev-note
Focuses: ui Cc:

Description

The personal data export reports can contain lot of groups and items, so having a table of contents and back to top links would help navigate very long reports.

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

See e.g. the screenshot below https://core.trac.wordpress.org/ticket/39165#comment:23.

We could explore similar setup for these reports.

Attachments (15)

toc.jpg (40.7 KB) - added by birgire 16 months ago.
46894.diff (2.5 KB) - added by xkon 16 months ago.
Screen Shot 2019-04-14 at 2.11.58 PM.png (87.1 KB) - added by garrett-eclipse 16 months ago.
No Data w/ TOC
Screen Shot 2019-04-14 at 2.13.29 PM.png (217.5 KB) - added by garrett-eclipse 16 months ago.
One Group w/ TOC
46894.2.diff (2.7 KB) - added by xkon 7 months ago.
46894.2_preview.png (224.2 KB) - added by xkon 7 months ago.
46894.3.diff (2.7 KB) - added by xkon 7 months ago.
46894.4.diff (5.9 KB) - added by garrett-eclipse 7 months ago.
Refreshed patch to esc_attr, and fix unit tests
46894.4.alt.diff (8.3 KB) - added by garrett-eclipse 7 months ago.
Alternate patch to ensure uniqueness by using group_id
46894.5.diff (9.0 KB) - added by xkon 6 months ago.
46894.5_preview.jpg (191.7 KB) - added by xkon 6 months ago.
46894.6.diff (28.4 KB) - added by garrett-eclipse 6 months ago.
Updated patch to include Group Counts on the TOC items, suppress Return to Top when only About group and introduce Unit Test coverage for these additions.
Screen Shot 2020-01-31 at 12.18.19 PM.png (13.6 KB) - added by garrett-eclipse 6 months ago.
Group Count in TOC
46894.7.diff (20.3 KB) - added by garrett-eclipse 6 months ago.
Replace patch 6 as it unintentionally had code from some other tickets.
46894.8.diff (21.4 KB) - added by xkon 6 months ago.

Download all attachments as: .zip

Change History (36)

@birgire
16 months ago

#1 @birgire
16 months ago

toc.jpg is a mockup suggestion showing a table of contents and "Return to table of contents" links.

There could also be some minimum number of groups (e.g. 3-5) to display the table of contents.

#2 @birgire
16 months ago

  • Focuses ui added
  • Keywords has-screenshots added

#3 @garrett-eclipse
16 months ago

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

Thanks @birgire

I like the concept here, this could be very useful on larger more data heavy sites. And I agree it would make sense to have a minimum threshold as a TOC when there's only 1 or 2 items wouldn't have much worth.

Cheers

@xkon
16 months ago

#4 @xkon
16 months ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to xkon
  • Status changed from new to assigned

46894.diff is a first pass on adding a TOC and the return links as well. It uses the Group Labels to create the needed anchor IDs to link to and a standardized toc id to get back to top.

This doesn't add the "limiting" of groups to create the TOC as I'm leaning on the side of "let's have it always for consistency". It's not going to be an issue in my opinion to have it there even if you only have the "default" groups.

#5 @garrett-eclipse
16 months ago

  • Keywords needs-refresh added

Thanks for the initial patch @xkon

It applies nicely and introduces the TOC as expected.

Some thoughts;

  1. Should we use sanitize_title or esc_url on the anchors? Some plugins will have special characters, and many languages will as well.
  2. Should we enable plugins to provide a $group_toc_label so they can provide a shortened version of their Group Label. In many cases the label used by plugins can be quite long which wouldn't be as appropriate for the TOC, so wondering if we allow for a group_toc_label and fall back to the group_label otherwise and for back-compat.
  3. When there's no data there's no poitn of a TOC for just the 'About' section. Also when there's just one more section in most browsers there's not enough content to make it worth anchors as no scroll action is possible. Am wondering if we just start at '>1' to enable the functionality otherwise it feels broken as it doesn't do anything.

*I would normally agree be consistent, but when there's link on the screen and they don't appear to cause an action they feel broken.

All the best,
Cheers

#6 @garrett-eclipse
16 months ago

P.S. Now that grunt test completed it's showing there's 4 phpunit issues due to the email contents being changed without their corresponding tests being updated.

There were 4 failures:

1) Tests_Privacy_WpPrivacyGeneratePersonalDataExportFile::test_contents
Failed asserting that '<!DOCTYPE html>\n
<html>\n
<head>\n
<meta http-equiv='Content-Type' content='text/html; charset=UTF-8' />\n
<style type='text/css'>body { color: black; font-family: Arial, sans-serif; font-size: 11pt; margin: 15px auto; width: 860px; }table { background: #f0f0f0; border: 1px solid #ddd; margin-bottom: 20px; width: 100%; }th { padding: 5px; text-align: left; width: 20%; }td { padding: 5px; }tr:nth-child(odd) { background-color: #fafafa; }#table_of_contents { padding: 10px 0 10px 0; }.return_to_table_of_contents { text-align:right; }</style><title>Personal Data Export for export-requester@example.com</title></head>\n
<body>\n
<h1>Personal Data Export</h1><div id="table_of_contents"><a href="#About">About</a></div><h2 id="About">About</h2><div><table><tbody><tr><th>Report generated for</th><td>export-requester@example.com</td></tr><tr><th>For site</th><td>Test Blog</td></tr><tr><th>At URL</th><td><a href="http://example.org">http://example.org</a></td></tr><tr><th>On</th><td>2019-04-14 21:17:52</td></tr></tbody></table><div class="return_to_table_of_contents"><a href="#table_of_contents">Return to table of contents</a></div></div></body>\n
</html>\n
' contains "<h2>About</h2>".

/Users/garretthyder/WordPress/46894-export-toc/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportFile.php:264

2) Tests_Privacy_WpPrivacyGeneratePersonalDataExportGroupHtml::test_group_html_generation_single_data_item
Failed asserting that '<h2 id="Test_Data_Group">Test Data Group</h2><div><table><tbody><tr><th>Field 1 Name</th><td>Field 1 Value</td></tr><tr><th>Field 2 Name</th><td>Field 2 Value</td></tr></tbody></table><div class="return_to_table_of_contents"><a href="#table_of_contents">Return to table of contents</a></div></div>' contains "<h2>Test Data Group</h2>".

/Users/garretthyder/WordPress/46894-export-toc/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportGroupHtml.php:45

3) Tests_Privacy_WpPrivacyGeneratePersonalDataExportGroupHtml::test_group_html_generation_multiple_data_items
Failed asserting that '<h2 id="Test_Data_Group">Test Data Group</h2><div><table><tbody><tr><th>Field 1 Name</th><td>Field 1 Value</td></tr><tr><th>Field 2 Name</th><td>Field 2 Value</td></tr></tbody></table><table><tbody><tr><th>Field 1 Name</th><td>Another Field 1 Value</td></tr><tr><th>Field 2 Name</th><td>Another Field 2 Value</td></tr></tbody></table><div class="return_to_table_of_contents"><a href="#table_of_contents">Return to table of contents</a></div></div>' contains "<h2>Test Data Group</h2>".

/Users/garretthyder/WordPress/46894-export-toc/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportGroupHtml.php:83

4) Tests_Privacy_WpPrivacyGeneratePersonalDataExportGroupHtml::test_group_labels_escaped
Failed asserting that '<h2 id="&lt;div&gt;Escape_HTML_in_group_lavels&lt;/div&gt;">&lt;div&gt;Escape HTML in group lavels&lt;/div&gt;</h2><div><div class="return_to_table_of_contents"><a href="#table_of_contents">Return to table of contents</a></div></div>' contains "<h2>&lt;div&gt;Escape HTML in group lavels&lt;/div&gt;</h2>".

/Users/garretthyder/WordPress/46894-export-toc/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportGroupHtml.php:139

#7 @birgire
16 months ago

  • Keywords needs-design-feedback added

@garrett-eclipse @xkon thank you for looking into this.

It would be interesting to explore other TOC design options as well.

Here are few that come to mind:

  • Floating TOC.
  • Sidebar with TOC.
  • Special mobile TOC (hamburger)
  • The one like in screenshot toc.jpg
  • Some other options ...

So I'm tagging it with needs-design-feedback and look forward to suggestions.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


10 months ago

#9 @karmatosed
10 months ago

  • Keywords needs-design added; needs-design-feedback removed

Just swapping in 'needs design' as this looks like it needs design over just feedback.

#10 @garrett-eclipse
10 months ago

  • Milestone changed from Future Release to 5.4

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


7 months ago

@xkon
7 months ago

@xkon
7 months ago

#12 @xkon
7 months ago

  • Keywords needs-refresh removed

46894.2.diff refreshes the patch to be properly applied and adjusts the TOC as seen on 46894.2_preview.png it also uses sanitize_title_with_dashes() and will show the TOC only if there are more than 1 groups on the page to fix the issues from comment 5.

@garrett-eclipse, I don't think that we need to provide a "group_toc_label" as mentioned in the same comment. Plugins / Themes should be already "branding" their groups by adding the plugin name also to avoid conflicts or data that can't be easily be attached to specific plugins. If they are not doing it then we have to communicate it better :).

@karmatosed, do we really need to re-design this "at this point" :D? I'm asking as it's just a utility page & a simple means to an end for whoever wants to take a look at their data. If you really think that we should re-design this (and there's no time at the moment for that) would it be OK to at least get this ticket in for 5.4?

We can then open up a fresh ticket for a full redesign for a future release (we need to see if accessibility is ok on this page also that's why I'm proposing a fresh re-design on a different ticket to address all possible issues in 1 go).

#13 @karmatosed
7 months ago

  • Keywords needs-design removed

Thanks for the context @xkon, I think this case we can swap it back but it could do with iterations maybe after 5.4. Looking at the screenshots (thanks for uploading those), I would say it is absolutely fine to get in.

Let's remove the design keywords so it can get shipped for now.

@xkon
7 months ago

#14 @xkon
7 months ago

  • Keywords commit added

Thanks @karmatosed! Marking this for commit and I'll open up a fresh ticket for the re-designs & further checks asap :).

46894.3.diff changes sanitize_title_with_dashes to sanitize_title to benefit from remove_accents also.

@garrett-eclipse
7 months ago

Refreshed patch to esc_attr, and fix unit tests

@garrett-eclipse
7 months ago

Alternate patch to ensure uniqueness by using group_id

#15 @garrett-eclipse
7 months ago

  • Keywords needs-testing dev-feedback added; commit removed

Thanks @xkon for starting this. I've refreshed you patch in 46894.4.diff to address the following;

  1. Make 'Table of Contents' translatable.
  2. Use Escape functions on the output and sanitize_title_width_dashes to lowercase and replace spaces with '-'.

https://developer.wordpress.org/themes/theme-security/data-sanitization-escaping/

  1. Fixed the Unit Tests to introduce the id attr we added on sections.

*These tests are what drove me to switch to esc_attr( sanitize_title() ) for lowercase and dashes

I also uploaded an experiment in 46894.4.alt.diff as there's potential that two plugins could use the same Group Label, say two gallery plugins with 'Images' groups. @xkon what are your thoughts? it passes the group_id along as it's more likely to be unique and appends it to the group label. We could just use the group_id, but if no slug is provided when registering the data it'll us a numeric so I didn't want to leave html ids with just numbers. I'm on the fence of this so uploaded as an alt for feedback.
*Mainly on the fence as it ends up with id's like about-about and test-group-data-test-group-data

@xkon
6 months ago

@xkon
6 months ago

#16 @xkon
6 months ago

  • Keywords commit added; needs-testing dev-feedback removed

Thanks @garrett-eclipse good idea on the group_id as well. 46894.5.diff makes a refresh on the 46894.4.alt.diff as the patch couldn't apply cleanly (due to JSON commit).

Tested with extra data from WooCommerce, bbPress & BuddyPress as well for the group_ids, works fine for me & preview at 46894.5_preview.jpg. I'm marking this for commit, I think we're good here :-).

Last edited 6 months ago by xkon (previous) (diff)

@garrett-eclipse
6 months ago

Updated patch to include Group Counts on the TOC items, suppress Return to Top when only About group and introduce Unit Test coverage for these additions.

#17 @garrett-eclipse
6 months ago

  • Keywords needs-testing has-unit-tests added; commit removed

Thanks @xkon I did some additional testing and everything works nicely. Tried with a couple plugins as well as applied the patches for Community Events Location (#43921) and Sessions Tokens (#45889) which added to the TOC properly.

The only question/improvement that came to mind while reviewing is the group count. For Session Tokens, Comments and other groups there can be multiple items. These groups have a group count applied beside the title which was introduced in Changeset#46209 / Ticket#46895.

In 46894.6.diff I've added the following;

  • Group Count to TOC groups with multiple items
  • Unit Tests for Group Count and TOC
  • Suppressed Return To Top when only 1 group (About) (This passes a $groups_count param on the wp_privacy_generate_personal_data_export_group_html function so updated relevant Unit Tests).

Can you take a look and make sure you're happy with the addition of Group Count to TOC and suppression of Return to Top on single group exports please @xkon. I removed commit as there was several changes so just another set of eyes would make me happy to move it back to commit.
Thanks

@garrett-eclipse
6 months ago

Replace patch 6 as it unintentionally had code from some other tickets.

@xkon
6 months ago

#18 @xkon
6 months ago

  • Keywords commit added; needs-testing removed

46894.8.diff has some minor CS fixes.

Tests fine on my end and group count work as expected, tested again with WooCommerce, bbPress & BuddyPress as well for the extra groups :-).

Thanks, @garrett-eclipse!

This ticket was mentioned in Slack in #core by francina. View the logs.


6 months ago

#20 @SergeyBiryukov
6 months ago

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

In 47278:

Privacy: Add a table of contents to Personal Data Export report for easier navigation.

Props xkon, garrett-eclipse, birgire, karmatosed.
Fixes #46894.

Note: See TracTickets for help on using tickets.