#46894 closed enhancement (fixed)
Personal Data Export Report: Table of contents for easier navigation
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (36)
#3
@
6 years 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
#4
@
6 years 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
@
5 years ago
- Keywords needs-refresh added
Thanks for the initial patch @xkon
It applies nicely and introduces the TOC as expected.
Some thoughts;
- Should we use sanitize_title or esc_url on the anchors? Some plugins will have special characters, and many languages will as well.
- 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.
- 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
@
5 years 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="<div>Escape_HTML_in_group_lavels</div>"><div>Escape HTML in group lavels</div></h2><div><div class="return_to_table_of_contents"><a href="#table_of_contents">Return to table of contents</a></div></div>' contains "<h2><div>Escape HTML in group lavels</div></h2>".
/Users/garretthyder/WordPress/46894-export-toc/tests/phpunit/tests/privacy/wpPrivacyGeneratePersonalDataExportGroupHtml.php:139
#7
@
5 years 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.
5 years ago
#9
@
5 years ago
- Keywords needs-design added; needs-design-feedback removed
Just swapping in 'needs design' as this looks like it needs design over just feedback.
This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.
5 years ago
#12
@
5 years 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
@
5 years 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.
#14
@
5 years 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.
#15
@
5 years 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;
- Make 'Table of Contents' translatable.
- 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/
- 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
#16
@
5 years 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 :-).
@
5 years 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
@
5 years 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_htmlfunction 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
#18
@
5 years 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!
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.