#43481 closed enhancement (fixed)
Add tabs and placeholders to privacy tools page in wp-admin
Reported by: | xkon | Owned by: | |
---|---|---|---|
Milestone: | 4.9.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Privacy | Keywords: | gdpr has-patch fixed-major |
Focuses: | ui, administration | Cc: |
Description
I wanted to make this ticket separate from the others, as this will only be focusing on the ui/ux aspect of the new pages only so we can have a clean conversation without actual 'tools' + actions distractions.
I took the liberty of creating a first pass using elements that already exist in the Admin area trying to have a simple process of using Steps like menus etc.
I will wait for #43435 , @allendav 's patch specifically as he's creating a new one to get in first and then I'll jump over that code to implement this so we can continue from there.
Attachments (21)
Change History (79)
#2
@
7 years ago
- Keywords 2nd-opinion added
Thoughts in general and 'what went wrong':
Accordiong (gif preview): this was the first thinking of how we should go with this that seemd playful, a good workflow and by adding the accordion it meant 1 extra click for the 'deletion' tool that might have saved some trouble to well not-so-bright users.
After some chats though this was leading in a way that we had to put in extra time to make it a11y ready etc and I don't think it's worth the time for now that we're trying to get as much as possible done. In a future update we can always come back and re-design everything or add whatever we like, we're trying to 'save' time now though so we can deal with other tickets and have time for Docs etc etc.
Rows: this is a plain UI but as you can see the Informational big box is nowhere to be found. That box is supposed to have some basic information of what this new screen is helpful links and other stuff that users HAVE to get aquainted with to understand the whole Privacy/GDPR implementation. So personally I don't like it.
Boxes: this is by far the simplest way to go, a11y ready, everything visible on 'common' screens all content & tools is accesible straight away and in general it follows with the idea of the Admin either way. -I'm personally voting this at the moment.-
Page-Tabs: these are either staying as tabs or we are going to pull everything in 1 page. The 2nd tab is supposed to host ( as you can see in the 1st gif ) all of the information that is incoming from the plugins + core that concern Privacy. Think that at some sites this information might be a loooong list, that's why the thinking of having it on an extra tab.
---
Notes: There are patches already for all versions but I'm waiting for the final desision as they will be put over #43435 so we can make a starting-clean commit and continue from there as with that ticket 2-3 will close together as well.
The contnet that you see now is subject to change, according to what is decided as final Tools and what we are actually going to show as information etc. But that doesn't matter as they are easy changes, so pay no attention to the content, just the general idea of how the page should / would look.
--
Waiting for further input either here or in #gdpr-compliance so I can refine and polish the UI and make it responsive as well before I implement it.
#3
@
7 years ago
43481_first_pass.patch creates a UI like privacy_rows.jpg to be extended by @fclaussen as it was discussed on slack.
Note: reminder that #43435 has to be committed first and then we'll rework this to get it in as well.
#4
@
7 years ago
Added a reviewed UI proposal.
Added a requests tab with what I think the requests table should look like. Or something similar to that.
#5
@
7 years ago
43481.1.gif is a preview of what we've came up with for now. We're not going to push it further for the time being as patch-on-patch will become a mess from one point on.
@azaozz this will be re-worked and finalized a bit to be implemented when there's a commit of the Privacy Page ticket ( #43435 ) as now we're actually duplicating some stuff to able to have parallel patches ready :) .
Tell me if we're splitting the js/css please into extra 'new' files or using existing ones or even keeping everything self-contained in the page for the time being so I know what I'll have to do to make this ready for commit as well asap as I didn't understand much from the core chat yesterday about releases and what can be done :) . Ty!
#6
@
7 years ago
43481.2.diff adds a grid layout for the tools view
#7
@
7 years ago
43481.3.diff is an attempt to move everything into the existing privacy.php
as have to use existing files.
There is a little jQuery
snippet on the original Privacy page just for the means to un-check the Tools->Privacy menu option so users won't be getting confused as we're actually using the same page but with different content. I'm not sure if that's can be done any other way since the menu automatically linters whatever is bound to privacy.php
.
Also I've added the
if ( preg_match( '#/wp-admin/about.php$#', $referer ) || preg_match( '#/wp-admin/credits.php$#', $referer ) || preg_match( '#/wp-admin/freedoms.php$#', $referer ) ) {
so if you go to the privacy from any of the About section pages you see the original content.
#8
@
7 years ago
43481.4.diff gets the UI down to basic and uses native admin UI elements of what we have now and has some placeholders extra for the ease of use in the future.
After the meeting today we discussed that it might be a good idea to get this in so everybody can easily adjust the UI when a ticket-patch requires it so we can shape things as we go as well.
The Tools tab has the code from #43435 implemented as well as some extra holders for extra things.
The Requests tab is the tables for the requests ( I just kept what @fclaussen did on previous patches here )
The Privacy Policy Information tab is just a holder ( we might as well change it ) for the plugins & core to show all the information that have on Privacy notices.
It uses wp-admin/js/xfn.j
s and wp-admin/css/forms.cssforms.css
as it was discussed in the chat & other tickets.
@azaozz do you think we can commit this asap?
#9
@
7 years ago
- Summary changed from Privacy Policy Tools - Admin Page UI to Add tabs and placeholders to privacy tools page in wp-admin
Clarifying the focus of this ticket as "Add tabs and placeholders to privacy tools page in wp-admin" per ticket scrub discussion in Making WordPress gdpr-compliance chat today. This is the UI into which all privacy tools (personal data export, etc) will be added by their own separate tickets.
#10
@
7 years ago
Under the Privacy Policy Information tab, instead of "Core" could we say "WordPress"? "Core" won't mean much to people besides WordPress developers. :P
#11
@
7 years ago
Under the Requests tab, do we really have the intention of providing two flows - one for deletion and one for anonymization? I think not (for simplicity's sake and to meet the law's expectations) so how about we have just have one flow.
And... could we (kinda) match the GDPR terms and call the table "Erasure" or "Personal Data Erasure" requests. https://gdpr-info.eu/art-17-gdpr/
#12
@
7 years ago
A few more things from a review of the code:
- there are inconsistent indents in src/wp-admin/css/forms.css for the body.privacy-php .privacy-information .tab-content rules
- there is inconsistent whitespace (e.g. around this) in src/wp-admin/js/xfn.js - codesniffer/wp standards could help
- in src/wp-admin/privacy.php "front-static-pages" is a boo boo from my old patch - lets use "privacy-policy-page" or some such instead
- also, in privacy.php, I think instead of _e and esc_attr_e in the requests headings, table cells and buttons you want esc_html_e, no?
- and, still in privacy.php, same thing (esc_html_e instead of _e) for the privacy policy information tabs and headings
#13
@
7 years ago
43481.5.diff fixes @allendav comments, thanks for noticing! ( some stuff where broken from copy pasting - oh well ).
For the Flow you mention, we are still missing an extra table for exports in there, no :D ? I wouldn't mind having everything in 1 list since the buttons are changing per 'action', but that is to be done when the actual code is behind it I guess.
As for the titles and _e
etc you mentioned, all those tabs are going to be populated automatically ( especially on the privacy tab ), so even though I changed them atm as they are static the code will eventually change again in the future.
#14
@
7 years ago
Thank you @xkon ! I think we are almost there!
I tested it out and looked at it - I noticed you changed it to "WordPress Core" - i really recommend just "WordPress" to be consistent with the rest of the WordPress admin UI.
For the Flow you mention, we are still missing an extra table for exports in there, no :D
Hmmm - good point - #43546 will deliver that. How about replacing the redundant Anonymize flow with "Export Personal Data" or some such for now? I'll noodle on that ticket's UI tomorrow but we can run with your placeholder UI I think for now.
As for the titles and _e etc you mentioned...
I still see a few esc_attr_e in the table headings (e.g. ID, Full Name, etc) that should be esc_html_e I think - those won't be changing, so we should probably fix those now before they accidentally survive too long :P
And one last thing I missed previously... the 'type' key in the requests - with that uppercase first letter there it looks like it is a user facing string (that missed translation) - how about using a constant instead or at least an all lowercase string? I know these are dummy values for the moment :)
#15
@
7 years ago
@allendav I'll pass it again on the next patch :)
What do you think of this:
At the moment we are in the situation of 3 main tabs.
Since we have the tables on the 2nd tab, I think that this leaves nothing else for the 1st ( there's only the page creation there ).. Maybe we could merge those two ?
We could just end up with 2 tabs:
Main 'Tools' Tab again that has the export / anonymize tools in the main area, and the Create Page block on the right side (above the information) to match the Publish Page as we normally find there in other admin areas.
Should we try that? The 1st page seems like it has no other reason at the moment part from creating the page and it's totally empty, there's no point. ( see ui_2_tabs.jpg below )
We have to consider though that there might not all actions be from requests so the Admins might still need a straight e-mail input to manually export/delete.
#16
@
7 years ago
This seems to be in a good progress.
Here are few additional suggestions after reviewing 43481.5.diff:
- Use
esc_url( $edit_href )
andesc_url( $view_href )
when using in HTML. - Use the
striped
class on the request tables, to get alternating background for each row. - Change
include()
torequire_once
without brackets. - Change require_once() to
require_once
without brackets. - Use
.hide()
instead of.css( 'display', 'none' )
inxfn.js
. - When the page loads, the non-active tabs are hidden with JS, could we do that via CSS instead to avoid the flickering?
- Should HTML tags in translation strings be avoided here? See e.g. #41645, #39898, #35988.
Also wondering in general about:
- The plugin tabs when there are many plugins, as some sites have 100+ plugins. The new "System Information" page in progress uses e.g. table-of-contents links.
- The case when there are hundreds of requests - pagination?
- If it would make sense to have the row actions like in the list tables? They show up when we hover over each row. Many large blue action buttons take a lot of visual attention.
- Is there a need for a "state" column, to show what has been done to that request?
- Allow the user to order requests by date, email or full name?
- If a plugin wants to link to the plugin information tab? Would it make sense to have it deep-linkable?
Hope it helps.
#17
@
7 years ago
For: 43481.6.diff
OK! Let me go through the list because I have like 10 different .diff(s) now locally that are not even published here and it is starting to get a bit messy.
From @allendav :
- Change
WordPress Core' to
WordPress`: fixed - Replace
Anonymize
Flow withExport
: I merged Erasure / Anonymization but this can be renamed again ( needs chat ). esc_attr_e
: fixedtype key
: turned to lowercase (this might change with according to what other tickets in the future want to get over it)
---
From @birgire :
esc_url()
: fixed on both$edit_href
,$view_href
striped
class: fixedinclude()
&require_once()
: I didn't touch these as that's how all admin pages have them at the moment..hide()
inxfn.js
: fixedjs flickering issue
: fixed, I passed everything in CSS on a default stateTOC instead of tabs on information
: fixed, I left the idea of 'tabed' content instead of a full list underneath the link index as we don't know how much information plugins will output yet
Extra things from @birgire for chat:
Row actions instead of buttons
: yes we could do that and it would make the tables a lot simpler imho as well
State column
: I think not as when you export or delete the request will probably be removed as well?
Order + Pagination
: we could do that but let's get this in and move with other tickets as well first imho
---
Extra things added:
Tables responsive
: since the tables where too big for small screens I just added anoverflow-x
for now and we can see in the future how to get that properly managedManual E-mail Export / Erasure boxes
: Admins might as well need to export or anonymize / delete data without having a request so that option should be given somewhere. For now I just placed some extra boxes on the side as they don't take up much space.No requests
: If there are no requests now the table will just display a message instaed of just removing the whole box. Since all websites will pretty much start 'empty' it woudln't look nice to just have boxes on the Right side with the main content showing nothing.
---
Any extra input is always more than welcome but let's try to finalize this and don't go over minor-things for now as all of these are always subject to change. Don't forget it's meant to just be a base so we can work on the other tickets as well so the other tickets can actually alter / add / remove things from the UI as well as we proceed with them.
(Hopefully I didn't forget something again ! :D )
#18
@
7 years ago
I like the sidebar setup in 43481.6.jpg, probably because it looks familiar ;-)
Also thanks @xkon for all your screenshots/animations, they are really helpful here.
#19
@
7 years ago
@birgire no problem at all, I always prefer to have a visual as well instead having to apply patches all the time just for quicker reviews etc.
Since you went on with the idea of the 'hidden - on hover' actions should we change the whole table into how Pages/Posts tables are working? That would also help on the responsive. What do you think ?
#20
@
7 years ago
yes I think it's good to reuse what's possible from current admin tables, whether it's like the post/page tables or the plugin table.
All of these tables extend the WP_List_Table
class that provides the general structure, or do you mean just to take the CSS class names from it?
#21
follow-up:
↓ 27
@
7 years ago
WP_List_Table
can definitely be something for discussion. Although this ticket was supposed to be just the base or let's say ui frame so everybody could just then start clicking and altering things on ( so the less code /css /js etc footprint possible in this one ). My point is that we can easily have an extra tickets after of 'Let's convert the tables to that' etc imho instead of moving forward with this on an enldess discussion pretty much :D as we don't yet know what even goes where as a final view.
The System Information
patches are ok to have them on a rolling mode as it concerns only 1 specific thing and everything is packed in 1 screen.
Most of the gdpr tickets are somewhat connected with eachother so the smaller the footprint they have the easier we can go through them in my opinion at least now that it's a start.
#22
@
7 years ago
Nice work @xkon :)
so the smaller the footprint they have the easier we can go through them in my opinion at least now that it's a start.
this, +1000
I vote we land this and iterate on it (including what goes on what tab, how many tabs, etc) in follow on tickets
#23
@
7 years ago
yes I agree, it's better keep the scope as contained as possible and inline with the ui/ux focus of the ticket. Any possible future WP_List_Table
implementation would be better served in another ticket, as it would most likely block this one.
#24
@
7 years ago
- Keywords has-patch added; 2nd-opinion removed
Sweet and thanks for everything!
Just a ping at @azaozz for a final look and a shot to commit 43481.6.diff when possible so we can continue fresh with the others.
#28
@
7 years ago
Note to reviewers, especially designers:
The export personal data flow steps are envisioned here: https://core.trac.wordpress.org/ticket/43546#comment:3
The erase personal data flow steps are envisioned here: https://core.trac.wordpress.org/ticket/43602#comment:3
Although the flows are very similar, the results couldn't be any more different, so the ideal UI will make it easy to NOT accidentally work with the wrong flow.
Lastly, the separate manage privacy policy page settings flow is already committed and can be seen on trunk today under Tools > Privacy
#29
follow-up:
↓ 30
@
7 years ago
Hey @allendav, looking at 43481-unified-ux-idea.gif, it looks like you could accidentally delete requests for data export? In general, I'm finding this idea very confusing.
#30
in reply to:
↑ 29
;
follow-up:
↓ 32
@
7 years ago
Replying to melchoyce:
Hey @allendav, looking at 43481-unified-ux-idea.gif, it looks like you could accidentally delete requests for data export? In general, I'm finding this idea very confusing.
In what way? Because of the bulk action? We don't need to have the bulk action for delete.
The idea behind deleting a request is if a request goes months and months without the user confirming the request - to give the site owner a way of "tidying things up."
We could add a prompt when deleting an individual request, i.e. an "You are about to delete a request. Are you sure?" If we keep bulk delete we could do that as well, i.e. an "You are about to delete 5 requests. Are you sure?"
#31
@
7 years ago
Does https://core.trac.wordpress.org/ticket/43442#comment:6 now supersede 43481.7.gif?
#32
in reply to:
↑ 30
@
7 years ago
Replying to allendav:
Replying to melchoyce:
Hey @allendav, looking at 43481-unified-ux-idea.gif, it looks like you could accidentally delete requests for data export? In general, I'm finding this idea very confusing.
In what way? Because of the bulk action? We don't need to have the bulk action for delete.
All this new stuff is really confusing and complicated. It kind of has to be — issues around privacy and data retention are complicated. But that means we need to make sure the tools we're providing are as clear and uncomplicated as we can make them.
The idea behind deleting a request is if a request goes months and months without the user confirming the request - to give the site owner a way of "tidying things up."
This isn't clear from the interface — when I see that "delete" bulk action, I think, "okay, yes, I am deleting this person's data." I had no idea that you're removing the request, not deleting the data itself. We need to be really careful about how we present this option, because it can go a couple ways:
- Site owner goes, "Okay, I'll delete their data. Done!" and then they delete the request, but not the data.
- Site owner goes, "I'll delete all of the peoples' data. Done! Wait, did I also just delete the export requests?? Oh no, oh no, how do I fix this??"
- Site owner looks at this interface, goes "I have no idea what this is," and nopes out.
We could add a prompt when deleting an individual request, i.e. an "You are about to delete a request. Are you sure?" If we keep bulk delete we could do that as well, i.e. an "You are about to delete 5 requests. Are you sure?"
I don't think that clarifies what exactly you're deleting, since it could be perceived as "deleting the data this person asked me to delete."
I need to clarify — is this replacing, or in addition to, the list of folks who have already requested either data export or anonymization? The more I look at this the more I'm starting to think it's in addition to, as in — this is the manual list, versus an automatic list that comes in somewhere that you then need to act upon. Am I correct or incorrect here?
#33
follow-up:
↓ 34
@
7 years ago
So as I figured from #43546 since new designs where added, we're splitting up the tools into different screens as well? ( I got confused a bit .. again :D ).
Either way, shall we close this? I don't see a point of this being open anymore if everything will be handled 'with' the patches and not as a UI first.
#34
in reply to:
↑ 33
@
7 years ago
Replying to xkon:
So as I figured from #43546 since new designs where added, we're splitting up the tools into different screens as well? ( I got confused a bit .. again :D ).
Either way, shall we close this? I don't see a point of this being open anymore if everything will be handled 'with' the patches and not as a UI first.
Yeah, our tickets have gotten a bit muddled. Let's keep this one open for this reason:
As many people know, @mikejolley and I have been working on a side repo with @melchoyce 's designs. I'm attaching to THIS ticket the request UX for both export and erasure. The patch has gotten big, so we will deliver the actual export and erasure underlying bits under tickets #43546 (for export) and #43602 (for erasure)
With the latest patch on this ticket, you will see two new entries under the wp-admin tools menu. One for export requests and one for erasure requests. Clicking on either will bring you to a new UI where you can enter an email address for a user and start a new request confirmation flow. Requests will show a status of "Pending" while we wait for the user to read their email and click the link.
The user clicks on the confirmation link in the email and then you should see "Confirmed"
You can also "Resend" from the bulk action menu and you can "Delete" requests (e.g. if a user never gets around to confirming a request and the admin wants to clean up.)
Please try both export and erasure requests.
Then, in #43546 and #43602 we'll hook up the "Next Steps" column and under-the-email actions per @melchoyce 's design.
Sound good? Feedback welcome
#37
follow-up:
↓ 46
@
7 years ago
Now that we have CPTs for the privacy requests, thinking we should store the actual requests in them too and stop using the options and user_meta tables. Posts are the most powerful "storage object" in WP and can organize and store data in many different ways: post_content, post_excerpt, post_content_filtered, etc. can all be used for... anything :)
On top of that there is "unlimited" post_meta that can be accessed by post_id or by meta_name. For example we can store unconfirmed requests in post meta and use something like _wp_pending_privacy_request
where value will be the hash. Then we will be able to query the post meta table and get an array with all keys with that name, and compare the hash.
Few other TODOs that we were discussing in Slack at some point:
- Requests (new, pending, completed) should not be deletable. This is important for keeping audit log. The only exception may be be requests that haven't been confirmed by email, but then we can auto-delete them when the link in the email expires (and they become unusable).
- Where to add actions and filters so plugins can easily extend this functionality.
#41
in reply to:
↑ 39
@
7 years ago
Replying to pento:
Thanks for fixing this, completely forgot about it...
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
7 years ago
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
7 years ago
#46
in reply to:
↑ 37
;
follow-up:
↓ 47
@
7 years ago
Replying to azaozz:
Few other TODOs that we were discussing in Slack at some point:
- Requests (new, pending, completed) should not be deletable. This is important for keeping audit log. The only exception may be be requests that haven't been confirmed by email, but then we can auto-delete them when the link in the email expires (and they become unusable).
I think the Admins given their 'power' should do whatever they like. We already are underway going extra chats of keeping proper logs in a different screen so they might take of these logs on v2.
For example I might send a wrong e-mail as an Admin for a request that has no place in my table at the moment. I should be able to just remove it since there are no notes etc to keep me from remembering.
Also ticket #43890 after recent discussions adds the ability to just add requests avoiding confirmation. This can be used for a full manual export / anonymization without implicating the user. And that might be something that an Admin want to do as an action and then actually remove it for any purpose.
- Where to add actions and filters so plugins can easily extend this functionality.
Do we need more for the time being for v1? Since plugins can already hook etc for the actions I think there's no need to push it for extra things at the moment ( if that's the case ). We can re-open something like this in the future for v2, just to be able to clear our list for the time being and start somewhat 'fresh' :) .
#47
in reply to:
↑ 46
;
follow-up:
↓ 48
@
7 years ago
Replying to xkon:
I think the Admins given their 'power' should do whatever they like.
Right, but that doesn't mean to give them the tools so they prevent proper "bookkeeping" :)
We already are underway going extra chats of keeping proper logs in a different screen so they might take of these logs on v2.
Hmm, not sure what a "proper log" is and why would the list-tables be "improper logs", but thinking duplicate logs don't belong in core. Plugins can implement as many logs as they want, and I'm sure some people will like having many of them :)
For example I might send a wrong e-mail as an Admin for a request that has no place in my table at the moment. I should be able to just remove it since there are no notes etc to keep me from remembering.
Yeah, failed confirmation requests can be "garbage collected" after few days/weeks.
Still think that log entries should not be deletable by default.
Also ticket #43890 after recent discussions adds the ability to just add requests avoiding confirmation. This can be used for a full manual export
Yeah, that is a possible enhancement, however I don't see why a request should be added at all. Admins can export any data at any time and as many times as they want. It's up to them :)
#48
in reply to:
↑ 47
@
7 years ago
Replying to azaozz:
Hmm, not sure what a "proper log" is and why would the list-tables be "improper logs", but thinking duplicate logs don't belong in core. Plugins can implement as many logs as they want, and I'm sure some people will like having many of them :)
Logs afaic are meant to keep track of everything even if the main data are the same as the 'time' would be different, not have single entries.
To give an easy example as I've mentioned in a different comment:
For the time being (it might get fixed in a future release) if there's a user already in the list I can't add another request for the same user.
So if a user asks me for 3 exports within 2 months or I want to do manual exports ever 2 days, I only have 1 request 'logged'.
A more correct way imho would be to have '1 entry per request' to call it a log as for the time being you only know 'that you exported some data' not how many times/when you did that.
Since some might 'charge' or whatever else they could think of for exports etc it will make a huge difference to see 5 exports of xkon
within 1 month than just have xkon
on the list without any extra information, if that makes sense :).
#49
@
7 years ago
43481-mailto-links.diff adds a mailto link to the requester's email-address in WP_Privacy_Data_Removal_Requests_Table
and WP_Privacy_Data_Export_Requests_Table
.
This could be helpful when we need to write emails to requesters.
Similar mailto links are on email-addresses in the Users and Comments admin list tables.
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
7 years ago
#51
follow-up:
↓ 52
@
7 years ago
Splitting this up so we can finalize it after today's bug scrubs, so please see the following:
@azaozz :
Todo: Requests (new, pending, completed) should not be deletable. #43912 ( and related logging #43797 )
Todo: Add actions and filters so plugins can easily extend export/erasure functionality. #43910
@birgire : Add mailto on export/erasure tables. #43911
--
@desrosj I think everything else is as it should be here. And this has to go first before all other @allendav 's patches afaic as it was the first commit that everything else based on ( if he can confirm also ).
privacy tools preview first