WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 2 months ago

Last modified 2 months ago

#44644 closed defect (bug) (fixed)

Personal data export request status changed to completed by admin download

Reported by: knutsp Owned by: pento
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.7
Component: Privacy Keywords: has-patch has-unit-tests commit
Focuses: administration Cc:

Description

Steps to reproduce:

  1. Create an export request (for yourself, an email will be sent to user, do not confirm)
  2. Click download action on the request, now in waiting status
  3. Refresh page

Result: Request is marked as completed, and "Next step" is "Remove"

Expected: No change, since it was done only to inspect the export file, since the status was waiting.

As it is, if just downloading the export file, no longer possible to:

  1. Email the file to the requester/user
  2. See if the requester/user later approves the request

Attachments (3)

44644.diff (1.5 KB) - added by garrett-eclipse 3 months ago.
Patch to address the issue of admin downloads triggering the request-completed.
before-after-patching.jpg (41.2 KB) - added by birgire 2 months ago.
Screenshots taken before and after patching. Here we've reloaded the page after downloading personal data.
44644.2.diff (1.8 KB) - added by garrett-eclipse 2 months ago.
Updated long comment to break onto multiple lines

Download all attachments as: .zip

Change History (25)

#1 @knutsp
11 months ago

  • Keywords needs-patch 2nd-opinion added
  • Type changed from enhancement to defect (bug)

#2 @SergeyBiryukov
11 months ago

  • Summary changed from Peronal data export request status changed to completed by admin download to Personal data export request status changed to completed by admin download

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


11 months ago

#4 @garrett-eclipse
11 months ago

  • Milestone changed from Awaiting Review to 4.9.9

Discussing this in Slack and reviewing the issue some more this is quite a frustrating limitation as in some cases it requires you making a second confirmation request to restart the workflow.

In the code it seems both the download and email actions trigger the same ajax workflow but the email has $send_as_email set to true. We should probably move the _wp_privacy_completed_request() call found in wp_privacy_process_personal_data_export_page within the conditional if ( $send_as_email ) here;
https://github.com/WordPress/WordPress/blob/d9df5dec117ca01211c02b90cb88e015e697d68e/wp-admin/includes/file.php#L2369
*This would only trigger the complete when the email is sent

As well to support the case where admin wants to download the export and craft their own email we should introduce a 'Mark Completed' list action to the rows that are currently in Confirmed state so they can still push the request through the workflow but avoid the autogenerated email.

I've marked this for 4.9.9 as it would be nice to rectify this asap

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


11 months ago

#6 @desrosj
10 months ago

  • Keywords ux-feedback ui-feedback added; 2nd-opinion removed

I think these suggestions make sense. An administrator downloading an export file does not necessarily mean the request should be completed. I also like a way to manually mark a request Completed.

@joshuawold would love some feedback here from a UX standpoint, and your suggestions for UI.

Related: #44233 (Unit tests for the changes here should go in the test class introduced in #44233).

#7 @JoshuaWold
10 months ago

Happy to give feedback! @desrosj is there a way for me to see the current state and know what I should be giving feedback on?

#8 @desrosj
10 months ago

@JoshuaWold The behavior that changes are being proposed to is present in any trunk or 4.9.8 install. To reproduce:

  1. Go to the Tools > Export Personal Data page.
  2. Create a data export request for a user.
  3. Before clicking the confirmation link in the email, hover over the request in the table and click the "Download Personal Data" action link.
  4. Refresh the page.

You'll see that the request is marked as completed and the button at the end of the row becomes "Remove request".

In summary, the suggested flow changes so far are:

  1. Do not mark the request as completed when the administrator clicks the download link. This would leave the request waiting for the user to click the confirmation link in the email. When they do, the request would be confirmed, and the "Email Data" button would appear.
  2. Introduce a way to manually mark a request as completed for instances where an administrator downloads the user's data and emails it separately. I was thinking of another action link near the download link, and maybe a bulk action.

If you could evaluate the current flow and give any feedback on (or expand upon) the suggested changes, that would be great!

#9 @desrosj
10 months ago

  • Focuses administration added; privacy removed

#10 @JoshuaWold
9 months ago

Thank you @desrosj, that's really helpful!

Do not mark the request as completed when the administrator clicks the download link. This would leave the request waiting for the user to click the confirmation link in the email. When they do, the request would be confirmed, and the "Email Data" button would appear.

I agree with that suggestion. In this case I'm guessing we'd just indefinitely allow them to "Resend email" if the user doesn't click the confirmation link.

Introduce a way to manually mark a request as completed for instances where an administrator downloads the user's data and emails it separately. I was thinking of another action link near the download link, and maybe a bulk action.

I like the idea of giving them a way to complete the request. I just want to make sure I understand the user need here. What is the typical scenario for this kind of a request to happen? Does a user ask for it and then the admin needs to supply it?

Either way, having "Mark as completed" as part of the bulk actions makes sense. Especially since I'm assuming we could move it back to "Pending" if the admin made a mistake or something changed. At the moment (unless there's something I'm missing about the admin needs here, and they will be doing this action often) I don't think adding another button, apart from the bulk actions, makes sense; seems like it'd be making it more busy.

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


9 months ago

#12 @pento
8 months ago

  • Milestone changed from 4.9.9 to Future Release

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


8 months ago

#14 @garrett-eclipse
5 months ago

  • Keywords needs-refresh added; ux-feedback ui-feedback removed

@garrett-eclipse
3 months ago

Patch to address the issue of admin downloads triggering the request-completed.

#15 @garrett-eclipse
3 months ago

  • Keywords has-patch needs-testing has-unit-tests added; needs-patch needs-refresh removed
  • Milestone changed from Future Release to 5.2
  • Owner set to garrett-eclipse
  • Status changed from new to assigned

Hello,

Thank you for the initial report @knutsp and I greatly appreciate the discussion and feedback @JoshuaWold and @desrosj.

In 44644.diff I address the bug at hand ensuring admin downloads don't modify the request state. To accomplish this I moved the _wp_privacy_completed_request call into the $send_as_email check to ensure it's only triggered by the user via the email flow. I've also modified the associated comment to indicate the change.

Testing this has so far worked nicely allowing me to download the export via admin multiple times throughout the process without affected the request state. Along with additional user testing we might want to setup unit tests for the wp_privacy_process_personal_data_export_page function which check the _wp_privacy_completed_request call and change of request status is done when in $send_as_email.

I've also created #46619 to branch off the enhancement request for introducing an admin action for 'Mark as Completed' either via bulk action, a button/link or both. @JoshuaWold I also answered your questions via that ticket. Happy to continue the conversation there.

*After running grunt test I identified a unit testing issue which I've addressed in this patch. I struckthrough my above comment on unit tests because the below issue brought to light the pre-existing tests in data_export_page_status_transitions, and with my adjustment it provides the coverage of the two cases I was thinking would need unit tests (admin vs user as defined by $send_as_email.
The error prior to my unit test adjustments;

1) Tests_Privacy_WpPrivacyProcessPersonalDataExportPage::test_request_status_transitions_correctly with data set #1 ('request-completed', 'last', 'last', 'last', false, 'last')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'request-completed'
+'request-pending'

/WordPress/44644-admin-download-completes-request/tests/phpunit/tests/privacy/wpPrivacyProcessPersonalDataExportPage.php:597

Moving this into 5.2 for an easy win. Let me know if anyone runs into issues.

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


3 months ago

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


3 months ago

@birgire
2 months ago

Screenshots taken before and after patching. Here we've reloaded the page after downloading personal data.

#18 @birgire
2 months ago

Thanks for the patch @garrett-eclipse

I tested the patch 44644.diff successfully as seen in before-after-patching.jpg.

ps: I would suggest dividing the long comment in 44644.diff into tvo lines.

@garrett-eclipse
2 months ago

Updated long comment to break onto multiple lines

#19 @garrett-eclipse
2 months ago

  • Keywords commit added; needs-testing removed
  • Status changed from assigned to accepted

Thanks for testing @birgire I appreciate the review.

I've uploaded 44644.2.diff to break the long comment onto multiple lines.

I think that wraps it up, marking for commit.

#20 @pento
2 months ago

  • Owner changed from garrett-eclipse to pento

#21 @pento
2 months ago

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

In 45148:

Privacy: Only mark a personal data export as complete when the user downloads it.

An admin may download an export to check that it's all correct, but this action shouldn't mark the request as complete.

Props garrett-eclipse, JoshuaWold, birgire.
Fixes #44644.

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


2 months ago

Note: See TracTickets for help on using tickets.