WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 days ago

#44266 accepted enhancement

Add per-request-row delete row action for privacy actions

Reported by: allendav Owned by: garrett-eclipse
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch
Focuses: ui Cc:

Description

Right now the only way to delete a privacy request (e.g. ones created while doing periodic testing) is to use the bulk-delete flow. It could simplify things greatly to add a per-request-row Delete action below the email address.

Attachments (17)

44266.diff (5.8 KB) - added by xkon 9 months ago.
44266_preview.gif (845.0 KB) - added by xkon 9 months ago.
44266.2.diff (6.7 KB) - added by garrett-eclipse 9 months ago.
Refresh of initial patch
3057f33c547ab2e5ba8c03a035eef87d.gif (530.1 KB) - added by garrett-eclipse 9 months ago.
Export request screencast of the two new actions and their notices
e3242fe2f2569cd51b63a12c6950e248.gif (511.2 KB) - added by garrett-eclipse 9 months ago.
Screencast of the erasure row actions and notices
44266.3.diff (6.7 KB) - added by garrett-eclipse 9 months ago.
Added missed periods to make notices and aria labels sentences.
44266.4.diff (7.0 KB) - added by xkon 9 months ago.
Screen Shot 2020-02-07 at 10.52.53 AM.png (7.7 KB) - added by garrett-eclipse 9 months ago.
Concept: Export Row Actions shortened to match other screens
Screen Shot 2020-02-07 at 10.53.21 AM.png (9.6 KB) - added by garrett-eclipse 9 months ago.
Concept: Reduce erasure row actions for brevity
44266.5.diff (10.6 KB) - added by garrett-eclipse 2 weeks ago.
Refresh to improve verbiage and address the URI issue by using sendback passthrough.
Screen Shot 2020-10-07 at 10.16.08 PM.png (17.1 KB) - added by garrett-eclipse 2 weeks ago.
Erasure Row Actions
Screen Shot 2020-10-07 at 10.16.24 PM.png (20.2 KB) - added by garrett-eclipse 2 weeks ago.
Export Row Actions
Screen Shot 2020-10-07 at 10.12.57 PM.png (17.2 KB) - added by garrett-eclipse 2 weeks ago.
Resent request notice with email.
Screen Shot 2020-10-07 at 10.13.01 PM.png (11.2 KB) - added by garrett-eclipse 2 weeks ago.
Deleted notice
44266.6.diff (12.0 KB) - added by garrett-eclipse 2 weeks ago.
After testing found on removal requests we suppress actions. Updated so we only suppress confirm and erasure actions on confirmed requests. Delete and download will always be available.
44266-erasure-Oct19202-0702.gif (8.1 MB) - added by hellofromTonya 5 days ago.
Screen capture: Tools > Erase Personal Data with patch 44266.6 applied. Browser: Firefox 81.0.2 on Mac
44266.7.diff (12.2 KB) - added by garrett-eclipse 4 days ago.
Minor refresh to provide a message when all items failed either to delete or resend.

Change History (48)

#1 @allendav
2 years ago

  • Keywords ux-feedback added

#2 @allendav
2 years ago

Should also consider how this could work with an Archive flow #44222

#3 @desrosj
2 years ago

  • Milestone changed from Awaiting Review to Future Release

#4 @dejliglama
2 years ago

We should have in mind that the request for data export and erasure is a matter of documentation (logging), and should not be deleted lightly.

#5 @desrosj
2 years ago

  • Keywords gdpr removed

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

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


11 months ago

#7 @michaelarestad
11 months ago

  • Keywords changed from needs-patch, ux-feedback to needs-patch ux-feedback

Can you please add some testing instructions and a screenshot of existing UI?

#8 @xkon
9 months ago

  • Owner set to xkon
  • Status changed from new to assigned

@xkon
9 months ago

@xkon
9 months ago

#9 @xkon
9 months ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Future Release to 5.4

44266.diff introduces 2 row actions for a Resend Confirmation Email as well as Delete Request on both Export/Erase personal data list tables.

I'll mark this for 5.4 as well in case we can have some extra eyes on testing :-).

#10 @xkon
9 months ago

  • Keywords 2nd-opinion added

#11 @xkon
9 months ago

  • Keywords has-screenshots added; ux-feedback removed

@garrett-eclipse
9 months ago

Refresh of initial patch

@garrett-eclipse
9 months ago

Export request screencast of the two new actions and their notices

@garrett-eclipse
9 months ago

Screencast of the erasure row actions and notices

#12 @garrett-eclipse
9 months ago

  • Keywords 2nd-opinion removed

Thanks @xkon I appreciate the initial patch here, it worked very nicely.

I've refreshed in 44266.2.diff to update the following;

  1. Updated the verbiage and classes to match #44674.
  2. Updated the translator comments to match the placeholder.
  3. Fixed some formatting for CS.
  4. Updated the delete action for the single request to match the resend by providing the email in the notice.
  5. Updated the resend multiple to keep it's plural form to support russian/arabic and other languages with multiple plural forms.

Uploaded screens to show the actions in action on both request tables.

If you can retest I think this can move forward. I was initially hesitant to use the bulk action here but it works nicely and when the list tables are updated in #47488 we can revisit how the row actions are handled.

@garrett-eclipse
9 months ago

Added missed periods to make notices and aria labels sentences.

@xkon
9 months ago

#13 @xkon
9 months ago

Thanks for the update @garrett-eclipse! Uploaded 44266.4.diff for some minor CS fixes (spaces instead tabs on some instances /shrug editors.. :D).

There is a small hiccup here, but not sure what would be the best approach to deal with it. When clicking the row actions you end up with a full URL on your browser depending on the action taken.

As an example, you click on "resend confirmation request" your URL should now have ?action=resend&request_id%5B0%5D=42&_wpnonce=8e47d5d5eb appended. This leads into a situation that if you hit a refresh page a confirmation is re-sent unintentionally.

We'll have to address this somehow before committing. I believe that we had the same issue for some other action in the past but couldn't find it at the moment.

#14 @birgire
9 months ago

Hi, it's very handy and makes it much easier to delete each request,

but I wonder if it would be benificial to look into the Trash feature, of the list table, for requests.

i.e. changing: "Delete Request" into "Trash Request"

and similar for bulk: "Delete Requests" into "Trash Requests"

to avoid accidental removal.

I think the older ticket had two forms on the same page interfering and it was solved by adding a action attribute to the other one, if I recall correctly. Not sure that would solve this one here.

I guess one could look into redirects like when posts are trashed from the list table, like when:

https://wp.test/wp-admin/post.php?post=157&action=trash&_wpnonce=c7dc10c655

is redirected to

https://wp.test/wp-admin/edit.php?post_type=post&ids=157

#15 @garrett-eclipse
9 months ago

  • Keywords needs-refresh added

Thanks for the refresh @xkon and flagging the nonce issue.

@birgire is correct the other issue #44047 was resolved by forcing an action onto the form to ensure its request didn't pull the nonce into its submission causing the invalid link. I don't believe this will apply here so will do some testing and see if I can come up with a solution before beta. *Marked for needs-refresh as we'll need to address this issue before finalizing.

@birgire as to your mention of the trash feature, that's accounted for in #44222 which I just milestoned to 5.5 as I'd like to work on that for next release.

@garrett-eclipse
9 months ago

Concept: Export Row Actions shortened to match other screens

@garrett-eclipse
9 months ago

Concept: Reduce erasure row actions for brevity

#16 @garrett-eclipse
9 months ago

@xkon / @birgire reviewing the other row-actions in core they in most cases use single words to make them more concise.
These new actions are quite verbose with 2-3 words per action forcing this to always be on two+ lines.

What are your thoughts on just going with Download | Resend | Delete or for the erasure Force Erase | Resend | Delete?

I uploaded some screens to illustrate.

#17 @xkon
9 months ago

  • Milestone changed from 5.4 to Future Release

We already changed the naming of the Bulk Actions on #44674 to Delete Requests and Resend Confirmation Requests to make it clear.

Unfortunately, since the tables show the e-mails it was misunderstood that the accounts might be getting deleted instead of just the requests. This is the reason why we also went in the way of including "big action titles" here following 44674 to always be clear about what the action is and avoid miscommunication.

I know that the general rule is the simplicity with "Download / Trash / Resend" etc but I believe that we can make an exception here as it is actually hard to distinguish the action, especially under the Erasures tables.

Also due to the 5.4 beta being close, I'm marking this for a future release so we can also take care of the URI issue here.

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


7 weeks ago

@garrett-eclipse
2 weeks ago

Refresh to improve verbiage and address the URI issue by using sendback passthrough.

@garrett-eclipse
2 weeks ago

Resent request notice with email.

#19 @garrett-eclipse
2 weeks ago

  • Focuses ui added
  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 5.6
  • Owner changed from xkon to garrett-eclipse
  • Status changed from assigned to accepted

Good point @xkon, I've refreshed in 44266.5.diff and feel it's pretty well there so am moving to 5.6 for extra eyes. Please take a minute to test if you can.

I've gone with the following verbiage which is clear enough but not overly verbose;
Download Data | Resend Confirmation | Delete Request
Force Erasure | Resent Confirmation | Delete Request

As to the URI and leaving param remnants during requests I adopted the posts/comments approach to using a sendback and redirect. To handle the redirect there's now a new handle_bulk_messages function that takes the info params from the redirect to display the message. This strips the wpnonce and other undesired items.

One drawback of the sendback method was the split mean passing the request id in order for us to lookup the email, but on delete actions the request is gone so we can't do the lookup. The only workaround is to pass the email via the param and I'd prefer not to have email in URL as that's saved to history etc. So instead for delete we only provide the count and not the email. The resent we do provide email as it's more important to know who was resent to than who you deleted.

Please test and review and let's move this forward.

P.S. One issue you may note, especially locally testing, is failed request emails aren't reported. I'm looking to address that via #44081.

@garrett-eclipse
2 weeks ago

After testing found on removal requests we suppress actions. Updated so we only suppress confirm and erasure actions on confirmed requests. Delete and download will always be available.

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


10 days ago

#21 @hellofromTonya
10 days ago

📣 Call for testers 📣

This ticket is ready for commit once it gets some testing. Calling on our testers to help get this over the finish line to land in 5.6 Beta 1 (which is next week, ie 20th).

@hellofromTonya
5 days ago

Screen capture: Tools > Erase Personal Data with patch 44266.6 applied. Browser: Firefox 81.0.2 on Mac

#22 @hellofromTonya
5 days ago

@garrett-eclipse Testing patch 42266.6 applied in both Firefox and Chrome:

  • I did not get an admin notice after clicking on the Resend Confirmation link or via Bulk actions.
  • I did get an admin notice for the delete action via the Delete Request link and bulk actions.

Is this the expected behavior? Should there be a resend notice?

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


5 days ago

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


4 days ago

#25 @garrett-eclipse
4 days ago

There should be a resend notice, but unfortunately I believe you're testing on local where emails won't successfully send so they aren't reported. And when there's a failure it's not reported currently so we have #44081 to fix that.

If you have a hosted server w/ mail you can test there, hopefully successfully.

#26 @carike
4 days ago

  • Keywords needs-testing has-screenshots removed

Tested on a single-site installation.
The remove link only appears at the end of the process, but it does remove the request from the list (checked both export and erasure).
Also, just a note, I needed to refresh the page before the link appeared. I'm assuming that this is intended behaviour / that you did not want to use AJAX.
The user did receive the confirmation e-mails.

Removing "has screenshots" tag as the screenshots are no longer representative of what the patch looks like.

#27 @carike
4 days ago

Resend confirmation request via the bulk action did send an e-mail with a new link on a live server.

#28 @hellofromTonya
4 days ago

  • Keywords commit added

Thanks @carike for testing in live site and confirming it works well. Marking this one for commit.

@garrett-eclipse
4 days ago

Minor refresh to provide a message when all items failed either to delete or resend.

#29 @garrett-eclipse
4 days ago

Thanks @carike and @hellofromTonya after thinking on your issue with local mail failures I made a minor mod in 44266.7.diff to provide messaging for that failure. We'll greatly improve that in #44081 which is ready to land as a bug during beta.

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


4 days ago

#31 @helen
4 days ago

  • Keywords commit removed
  • Milestone changed from 5.6 to Future Release

Late punt, not a fan of adding one-click delete without trash. See #44222.

Note: See TracTickets for help on using tickets.