Personal data export request status changed to completed by admin download

[knutsp]

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

[garrett-eclipse]

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

[desrosj]

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).

[JoshuaWold]

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?

[desrosj]

@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!

[JoshuaWold]

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.

[garrett-eclipse]

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

[garrett-eclipse]

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.

[birgire]

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

[birgire]

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]

Updated long comment to break onto multiple lines

[garrett-eclipse]

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.

[pento]

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.