Make WordPress Core

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#43890 closed enhancement (fixed)

Allow Admin to Skip e-mail confirmation for Export/Anonymization

Reported by: xkon's profile xkon Owned by: xkon's profile xkon
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Privacy Keywords: needs-dev-note
Focuses: Cc:

Description

We thought that it might be good to allow admins the option to avoid a user's e-mail confirmation.

We've talked a bit about it with @allendav and @mikejolley and 1 way was adding a drop down menu for the Admin to select if they want to skip it or not.

Attachments (9)

43890_preview.gif (302.1 KB) - added by xkon 6 years ago.
43890.diff (7.5 KB) - added by xkon 6 years ago.
43890.2.diff (7.8 KB) - added by xkon 6 years ago.
refresh
43890.3.diff (9.1 KB) - added by xkon 5 years ago.
refresh
43890.4-before.jpg (62.7 KB) - added by xkon 4 years ago.
43890.4-after.jpg (69.7 KB) - added by xkon 4 years ago.
43890.4.diff (8.2 KB) - added by xkon 4 years ago.
43890.5.diff (9.8 KB) - added by xkon 4 years ago.
phpunit tests
43890-retested.gif (4.0 MB) - added by hellofromTonya 4 years ago.
Retested "Export Personal Data" using Email Log plugin to capture email. Works as expected.

Change History (65)

@xkon
6 years ago

@xkon
6 years ago

#1 @xkon
6 years ago

This first patch gives a try on skipping confirmation e-mails completely.

As far as my tests go on the Admin side everything works as supposed to but do take a good look please as I'm not sure if there's anything else hooked up that I missed :D .

Patch 43890.diff :

  • Adds a dropdown menu with 2 options With Confirmation / Without Confirmation
  • Add a new post_status of request-skipped - Confirmation Skipped
  • Adds new buttons on the Next Steps column that allow you to download / erase data from scratch
  • Avoids the request-confirmed update after every action to keep the same status

as seen in 43890_preview.gif

This ticket was mentioned in Slack in #gdpr-compliance by xkon. View the logs.


6 years ago

#3 @xkon
6 years ago

  • Keywords has-patch added

#4 @azaozz
6 years ago

I'm thinking this would be good for v2 perhaps? Also, why add a "user request" at all, admins can export any data at any time and as many times as they want. We can add the entry forms for the email address on the Tools => Privacy screen (that was the original intention anyway).

#5 @xkon
6 years ago

Yes v2 sounds good as it still needs stuff fixing and a full check at this point will be not worth the time.

On the Tools -> Privacy screen you mention. That page was named 'Tools' when it was hosting everything under it, there's nothing in there at the moment except the Privacy Page settings (see #43894 suggestion) but I think that keeping them in the actual Export/Erasure lists makes more sense as that's what they are about and you can easily find it (+ you can keep the logs and such as well in your list since they are kept there for the time being).

#6 @TZ Media
6 years ago

For our clients, we'll need the possibility to skip the confirmation completely. They have data in multiple systems and have confirmed the request before a request is entered into WordPress. So they need a way to export all user data for a given email address without any confirmation emails.

#7 @xkon
6 years ago

Well if we decide 'were to put' (I still think that under the respective tools is better) these 2 actions all the base code from the patch above works as I've tested. We can skip adding them to the tables as @azaozz mentions and just use the functions solely for an instant export/anonymization.

If you think it will absolutely help for v1 ( note that nobody says how soon a next version will be - that's what I gathered from the last core chat ) I can squeeze some time and maybe rework on it today if we can catch the deadline as well of course.

#8 @TZ Media
6 years ago

If I remember correctly I can still hook into the confirmation request email before it is sent and change the email address to prevent it from being sent to the user.

I would definitely like to see it in 4.9.6, but it is not the highest priority for me.

Instant export would also be great.

BTW: I'd also need an "instant erase" without confirmation email (again because confirmation happened already outside of WordPress. Should this go into a separate ticket? I can use the same workaround as above for now with our clients, though.

#9 @iandunn
6 years ago

Related: #44066

#10 @desrosj
6 years ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

#11 @iprg
6 years ago

#44066 was marked as a duplicate.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


6 years ago

#13 @allendav
6 years ago

I also think we should also consider giving a box for the admin to give a reason for skipping confirmation - that will be useful in the logs eventually

#14 @allendav
6 years ago

Idea: instead of a dropdown, perhaps the no-confirmation flow could be started with a link next to the "primary" confirmation-flow button

#15 @desrosj
6 years ago

  • Keywords needs-design ui-feedback added

#16 @desrosj
6 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 #core-privacy by desrosj. View the logs.


6 years ago

#18 @wesselvandenberg
6 years ago

  • Keywords changed from has-patch, needs-design, ui-feedback to has-patch needs-design ui-feedback

Hey, we at Wordcamp Nijmegen saw that it needs design. We got a few questions; Why does this need a email conformation and why do we need an admin for this?

The main question here is:
What risk is mitigated with the double conformation?

Is it an option to let users export their personal data trough a button on their profile page?

@xkon
6 years ago

refresh

#19 @xkon
6 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to xkon
  • Status changed from new to assigned

43890.2.diff is simply a refresh on the previous patch.

We would still like a feedback on ui and design of course whenever possible ( cc @melchoyce ) just to have it fully ready for a future release :) .

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


6 years ago

#21 @karmatosed
5 years ago

  • Keywords needs-design-feedback added; needs-design ui-feedback removed

This seems to have a design, so I am going to remove that keyword and make sure it has the feedback one.

@xkon
5 years ago

refresh

#22 @xkon
5 years ago

@garrett-eclipse , could you take a look at 43890.3.diff as well? I don't think I've missed something and if all good we can mark this for commit.

Last edited 5 years ago by xkon (previous) (diff)

#23 @birgire
5 years ago

Thanks for the refresh @xkon

I noticed one thing in 43890.3.diff, it changes the signature of the wp_create_user_request() function from:

wp_create_user_request( $email_address = '', $action_name = '', $request_data = array() ) {

to:

wp_create_user_request( $email_address = '', $action_name = '', $request_confirmation = '', $request_data = array() ) {

To preserve backward support, I would rather suggest:

wp_create_user_request( $email_address = '', $action_name = '', $request_data = array(), $request_confirmation = '' ) {

#24 @birgire
5 years ago

Few random thoughts:

It looks like $request_confirmation is binary, what about using true/false instead of 'yes' vs 'no' or '' strings ?

Personally, I like the former better:

wp_create_user_request( ..., true );

wp_create_user_request( ..., 'yes' );

In that case one might use:

$request_confirmation      = ( 'yes' === $_POST['request_confirmation'] );

instead of

$request_confirmation      = sanitize_text_field( $_POST['request_confirmation'] );

Another thing, how should we handle missing $_POST['request_confirmation'] ?

Should it trigger an invalid action error?

Since this is a binary option, would a checkbox be suitable here, instead of a dropdown?

It looks like unit tests will need an update.

Cheers

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


5 years ago

#26 @karmatosed
5 years ago

I tried to test this but couldn't get it working, would it be possible to either have some steps (just to check I am not missing anything) or a screencast?

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


5 years ago

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


5 years ago

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


5 years ago

#30 @davidbaumwald
5 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 5.3 to Future Release

This ticket still needs some work. I'm adding a 2nd-opinion tag to hopefully have some more input and movement toward inclusion in a Future Release.

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


4 years ago

#32 @estelaris
4 years ago

  • Keywords needs-screenshots added

As discussed in the design meeting, we would like to request a new screenshot to be able to provide design feedback

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


4 years ago

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


4 years ago

#35 @paaljoachim
4 years ago

It would be helpful with a screenshot so we can see where it is at right now.
@davidbaumwald can you add a screenshot?

@xkon
4 years ago

@xkon
4 years ago

@xkon
4 years ago

#36 @xkon
4 years ago

  • Keywords has-screenshots needs-unit-tests added; needs-screenshots removed
  • Milestone changed from Future Release to 5.7

43890.4.diff is a refresh of the previous versions that applies cleanly now & adjusts code taking into account the tickets comments.

It adds an extra Checkbox (checked by default) instead of a dropdown to allow the Admins to skip the confirmation e-mails while a request is added. If the confirmation is skipped the requests are added as Completed directly.

The new form takes a bit of space now pushing the list table further down the page, but I chose to use the same setup as we do in Settings pages. I'll wait for a design feedback on this in case we can make it a bit ore compact :) .

Since the wp_create_user_request() has an extra param now we'll have to check phpunit tests also as @birgire mentioned in a previous reply. I'll work on those on a later update.

43890.4-before.jpg & 43890.4-after.jpg are a comparison of the UI change of this patch.

Marking this for 5.7 as well as I still believe that it would be a helpful addition for cases that emails don't need to be communicated.

@xkon
4 years ago

phpunit tests

#37 @xkon
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

43890.5.diff continues on previous patch and adds unit tests for each $send_confirmation_email case.

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


4 years ago

#39 @xkon
4 years ago

  • Keywords needs-testing added; dev-feedback needs-design-feedback 2nd-opinion removed

Removing design-feedback as discussed in #design & adding needs-testing for a final check before marking for commit.

To test:

  • Apply the latest patch: 43890.5.diff
  • Navigate to Tools -> Export Personal Data
  • Input your username and request an export with the Confirmation option checked. Result you should receive the email and the request should be added with a pending status.
  • Remove your username.
  • Add your username again with the Confirmation un-checked. A request should be added with a completed status and no emails should be sent to the user (all confirmations are skipped).

In a similar way the same steps can be tested under Tools -> Erase Personal Data.

#40 @paaljoachim
4 years ago

Hey @xkon

I just tested the patch. Going to Tools -> Export Personal Data screen.
It think it is a very good idea to add a checkbox option here.

I am following the testing instructions above.

Test 1:
In the Username or email address I added the default localhost:8889 admin username. Checkbox is checked. Then clicked the Send Request button.

Result is a "Confirmation request initiated successfully."
Below I see a Pending message.

Hovered over the Requester e-mail address: test@… and clicked "Complete request"
Then under Next steps: clicked Remove request.

---

Test 2.
Again added the default username: admin.
Unchecked the checkbox.
Clicked Sent Request.

Result is that it skipped a "Confirmation request initiated successfully." and went straight to "Completed".

The patch works well in the Export Personal Data screen.


I also did the same test within the Erase Personal Data screen, and it looks to work identical.


In localhost:8889 I made a new profile with my hotmail address. I am not sure if I will get an e-mail or not. If/when I get an e-mail I can update this trac ticket.

#41 @xkon
4 years ago

  • Keywords commit needs-dev-note added; needs-testing removed

Thanks a lot for testing @paaljoachim ! Marking for commit :) .

#42 @paaljoachim
4 years ago

Happy to help, @xkon :)

This ticket was mentioned in Slack in #core-css by paaljoachim. View the logs.


4 years ago

#44 @antpb
4 years ago

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

In 50159:

Privacy: Allow Admin to Skip e-mail confirmation for Export.

This adds a form option to skip the admin email alert when exporting personal data.

Props xkon, azaozz, TZ-Media, iandunn, desrosj, iprg, allendav, wesselvandenberg, karmatosed, birgire, davidbaumwald, estelaris, paaljoachim, hellofromTonya.
Fixes #43890.

#45 @antpb
4 years ago

In 50160:

Coding Standards: Fix spacing in test_pending_status_with_false_send_confirmation_email test.

Follow-up to [50159] adjusts alignment of the $request_data value.

See #43890.

#46 @SergeyBiryukov
4 years ago

In 50165:

Docs: Update documentation for wp_create_user_request() per the documentation standards.

Add a @since note for the $send_confirmation_email parameter.

Follow-up to [50159].

See #43890.

#48 @SergeyBiryukov
4 years ago

In 50230:

Privacy: Rename the $send_confirmation_email parameter of wp_create_user_request() to $status, for clarity.

Follow-up to [50159], [50165].

Props xkon, TimothyBlynJacobs.
Fixes #52430. See #43890.

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


4 years ago

#50 @monikarao
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have Tested the Patch and it's working as expected:

Case 1- Input your username and request an export with the Confirmation option checked. Result you should receive the email and the request should be added with a pending status.

Result: Success message "Confirmation request initiated successfully." but I didn't receive any email in my inbox.
https://a.cl.ly/mXu15LZg

Case 2 - Add your username again with the Confirmation un-checked. A request should be added with a completed status and no emails should be sent to the user (all confirmations are skipped).

Result : https://a.cl.ly/Jru1qJLy

Note: I see one issue that Box automatically gets checked after creating a new request.

Please check this video - https://a.cl.ly/2NuE0Lq0

#51 @johnbillion
4 years ago

  • Keywords needs-testing added; has-patch has-screenshots has-unit-tests commit removed

#52 @paaljoachim
4 years ago

Hey @johnbillion

I tested the patch here: https://core.trac.wordpress.org/ticket/43890#comment:40
It sounds like it needs another round of testing.

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


4 years ago

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


4 years ago

@hellofromTonya
4 years ago

Retested "Export Personal Data" using Email Log plugin to capture email. Works as expected.

#55 @hellofromTonya
4 years ago

  • Keywords needs-testing removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Retested functionality for "Export Personal Data". Used the Email Log plugin to capture the email.

Results: Works as expected.

  • Email is captured in Email Log when the "Confirmation email" checkbox is checked
  • Email is not sent when the "Confirmation email" checkbox is not checked

Marking testing as done. And reclosing the ticket.

What about the checkbox state on reload?

@monikarao noted that the checkbox rechecks after pressing "Send Request":

Note: I see one issue that Box automatically gets checked after creating a new request.

This happens as the page is sent and then reloaded after processing the request. The default state of the checkbox is enabled. It does not "remember" the last state.

@xkon is the intent to have the state of the checkbox persist?

#56 @xkon
4 years ago

A request required a confirmation up to now, so I left the checkbox to be reset and not keep it's state intentionally.

I didn't want to change the logic that requests where working "by default" and create further unwanted confusion since the option is also new.

The current behavior of the commit is working as expected and in future time if there are tickets/requests in Trac to also keep the state we can look into that as well of course, IMHO though it should always "reset" as it does now.

Note: See TracTickets for help on using tickets.