Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#43967 closed enhancement (fixed)

Admin emails after email confirmation don't work for data privacy requests

Reported by: garrett-eclipse's profile garrett-eclipse Owned by: iandunn's profile iandunn
Milestone: 4.9.8 Priority: low
Severity: normal Version: 5.1
Component: Privacy Keywords: has-unit-tests has-patch commit
Focuses: ui, administration Cc:

Description

Hello,

While testing the GDPR requests I found that the admin emails aren't being triggered once the user has confirmed their email.

To reproduce;

  • Submit an Export or Removal request
  • From the confirmation email click the confirmation link
  • You'll note the message displayed indicate an admin email was generated but as the admin you won't receive one and if you had Email Log or another plugin in place you'll note one was never spawned.

So currently the admin users won't know about a request confirmation until they login to the request panels.

Please setup admin emails for both the export and removal requests once a user has confirmed their email. It would be nice if this email either has a link for the next action or at least links to the appropriate request panel and indicates for which user the request was confirmed.

Thank you

Attachments (8)

Screen Shot 2018-05-04 at 1.30.51 PM.png (29.2 KB) - added by garrett-eclipse 6 years ago.
Confirmation Page w/ note about admin getting notified
43967.diff (3.5 KB) - added by desrosj 6 years ago.
First pass at an email to the site admin when a request is confirmed.
43967.2.diff (5.2 KB) - added by iandunn 6 years ago.
Modularize function, Multisite tweaks, remove "sent to..."
43967.3.diff (5.4 KB) - added by desrosj 6 years ago.
43967.4.diff (6.6 KB) - added by birgire 6 years ago.
43967.5.diff (6.8 KB) - added by desrosj 6 years ago.
43967.6.diff (7.7 KB) - added by desrosj 6 years ago.
43967.7.diff (7.8 KB) - added by desrosj 6 years ago.

Download all attachments as: .zip

Change History (53)

@garrett-eclipse
6 years ago

Confirmation Page w/ note about admin getting notified

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


6 years ago

#2 @xkon
6 years ago

  • Keywords gdpr added

#3 @allendav
6 years ago

I don't think there are any admin emails generated. @mikejolley can confirm. In that case, this is a good idea for an enhancement.

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


6 years ago

#5 follow-up: @jeffr0
6 years ago

WordPress notifies site admins of pending updates and comments in moderation with notification bubbles. Perhaps this can be extended to show a notification bubble for confirmed requests and take site admins to the appropriate page (Data Removal or Export).

As it stands, without an email or a notification in WordPress, the site admin has no idea a user confirmed a request unless they manually visit the Data Removal or Export page.

#6 in reply to: ↑ 5 ; follow-up: @azaozz
6 years ago

  • Focuses ui added
  • Milestone changed from Awaiting Review to 4.9.6
  • Priority changed from normal to high

Replying to jeffr0:

Yep, I agree. Ideally this will work similarly to the comments moderation queue. We should send an email to the admin that a request has been confirmed (for each confirmed request), and should show the bubble with number of confirmed requests in the main menu.

As Export personal data and Remove personal data are sub-menu items, each will need their own bubble with a number, plus the top menu item, Tools will need a bubble will the total number.

Hoping we can get this done before the RC. If not, this should be marked "4.9.7 early".

Last edited 6 years ago by azaozz (previous) (diff)

#7 in reply to: ↑ 6 @desrosj
6 years ago

  • Keywords needs-patch added

I like the email for 4.9.6, but I think the numbers need a bit more exploration. Displaying a number for the Tools menu item when it is closed is not something I like. If the tools were under a top level Privacy menu, I might like that more.

Additionally, real estate is already limited due to the long sub menu item names. I would love some UI/UX feedback on that part of this.

@desrosj
6 years ago

First pass at an email to the site admin when a request is confirmed.

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


6 years ago

#9 @pbarthmaier
6 years ago

~As a user I get the confirmation and the indication that Admin has been notified, but I don't receive and Admin notification.~

![Confirmation of Request](https://monosnap.com/image/3Wg6cKBGwTgPgxMyjxx4cAYNKNoAm5.png)

Added in the patch from 43967.diff to local and Admin Email worked!
![Alt text](https://monosnap.com/image/WaRH00z2WSNmJcDn2J6Cfh9xECjIej.png)

Last edited 6 years ago by pbarthmaier (previous) (diff)

#10 @pbarthmaier
6 years ago

Also, and perhaps off topic, tried to add more info about which request was being honored, to improve clarity:

	/**
	 * Return request confirmation message HTML.
	 *
	 * @since 4.9.6
	 * @access private
	 *
	 * @return string $message The confirmation message.
	 */
function _wp_privacy_account_request_confirmed_message( $message, $request_id ) {
	$request = wp_get_user_request_data( $request_id );
	$description  = wp_user_request_action_description( $request->action_name );

	if ( $request && in_array( $request->action_name, _wp_privacy_action_request_types(), true ) ) {
		$message = '<p class="message">' . __( "{$description} Action has been confirmed." ) . '</p>';
		$message .= __( 'The site administrator has been notified and will fulfill your request as soon as possible.' );
		$message .= print_r( $request );
	}

	return $message;
}

which yields:

![Alt text](https://monosnap.com/image/b1hU8JCB3n9yfPy65jBpRSjGoJiXIk.png)

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


6 years ago

@iandunn
6 years ago

Modularize function, Multisite tweaks, remove "sent to..."

#12 @iandunn
6 years ago

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

It seems like a good goal might be email notifications for 4.9.6, then add the wp-admin notifications in a future release? I feel a bit leery about adding too much stuff this late in the process.

43967.2.diff makes these changes:

  • Modularizes the email functionality into a separate function. A side-effect of this is that it can now be triggered multiple times, which isn't great, but I can't think of any serious problem that creates. It seems like the benefits of smaller, more maintainable and testable functions outweigh the drawbacks. Having it as a separate callback might also be useful for unhooking while batch-processing Multisite requests in the future.
  • Use the individual site name/URL, rather than the network name, since the export will only contain data related to a single site.
  • Send the email to the network admin, since single site admins won't have the capabilities to process the request by default. I added a filter in case devs want to send the notifications to a DPO, single site admins, etc.
  • Removed the "this email was sent to..." string, see #44030.

I'd like to get another set of eyes on all this, and some other testers, before considering it ready for commit.

@desrosj
6 years ago

#13 @desrosj
6 years ago

@iandunn 43967.2.diff looks great. Great improvement.

I was able to break it in one way, though.

  1. Create a confirmation request.
  2. Click the link to confirm the request.
  3. Admin receives an email.
  4. Click the link to confirm the request again.
  5. Admin receives an email.
  6. Can be repeated.

43967.3.diff adds a post meta flag when the admin is notified to prevent any further notifications. Some considerations:

  • Should this meta value be removed when a request is re-initiated?
  • Should it use the status of the request instead? (I don't think so, because it will be completed before the email function runs)

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


6 years ago

#15 @iandunn
6 years ago

In 43211:

Privacy: Notify admin via email when a request is confirmed.

Previously the admin didn't have any way to know if a pending request was ready to be processed, aside from manually checking the Export/Erase pages. Sending them an email is a much more convenient option.

Props garrett-eclipse, desrosj, iandunn.
See #43967.

#16 @iandunn
6 years ago

  • Keywords ui-feedback fixed-major added; has-patch removed
  • Priority changed from high to normal
  • Type changed from defect (bug) to enhancement

r43211 introduces email notifications, which I think satisfies the most urgent need. That needs to be backported to 4.9.

The idea of adding upgrade-style bubbles can be explored for a future release. I think @desrosj makes some good points about needing design feedback, though.

#17 @SergeyBiryukov
6 years ago

In 43215:

Privacy: Notify admin via email when a request is confirmed.

Previously the admin didn't have any way to know if a pending request was ready to be processed, aside from manually checking the Export/Erase pages. Sending them an email is a much more convenient option.

Props garrett-eclipse, desrosj, iandunn.
Merges [43211] to the 4.9 branch.
See #43967.

#18 @SergeyBiryukov
6 years ago

Let's create a new ticket to address comment:5, and close this one.

#19 @iandunn
6 years ago

  • Component changed from General to Administration
  • Keywords ui-feedback fixed-major removed
  • Owner set to iandunn
  • Status changed from new to accepted

Looks like #44000 covers the notification bubbles, so this just needs unit tests and can then be closed.

#20 @azaozz
6 years ago

  • Priority changed from normal to low

Set priority to low for tickets that only need unit tests.

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


6 years ago

@birgire
6 years ago

#22 @birgire
6 years ago

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

43967.4.diff adds test cases for _wp_privacy_send_request_confirmation_notification().

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


6 years ago

@desrosj
6 years ago

#24 @desrosj
6 years ago

In 43967.5.diff:

  • Replace 4.9.7 with 4.9.6.
  • Add @return tags to modify_email_content(), modify_email_address().
  • Remove $result variables for wp_privacy_account_request_confirmed() calls (the function does not return a value, so $result is never used).
  • Added a check for the presence of the _wp_user_request_confirmed_timestamp meta key.

Wondering if we should make this class more generic and include the tests for the other notification functions.

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


6 years ago

#26 @desrosj
6 years ago

  • Component changed from Administration to Privacy

Moving to the new Privacy component.

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


6 years ago

#28 @casiepa
6 years ago

  • Milestone changed from 4.9.6 to 4.9.8

Moving enhancement to 4.9.8

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


6 years ago

#30 @desrosj
6 years ago

  • Milestone changed from 4.9.8 to 4.9.7

This only needs unit tests, so let's leave it in 4.9.7.

#31 follow-up: @garrett-eclipse
6 years ago

Reviewing @desrosj unit tests some questions;

  • Should the tests check the user_request_action_confirmed action works properly?
  • Should the tests check that _wp_admin_notified is updated on the request post?
  • Should the tests be duplicated for erasure requests?
  • Should the tests try and confirm a completed request?
  • Should any tests try to trigger the user_request_action_confirmed by loading the login page with confirmaction?
  • Should any tests be done when the request doesn’t exist or was removed somehow?

@desrosj
6 years ago

#32 in reply to: ↑ 31 @desrosj
6 years ago

43967.6.diff has the changes noted below.

  • Should the tests check the user_request_action_confirmed action works properly?

I don't know that I have seen any tests before verifying that an action hook fires successfully (except the tests for the hooks API).

  • Should the tests check that _wp_admin_notified is updated on the request post?

Added tests for this.

  • Should the tests be duplicated for erasure requests?
  • Should the tests try and confirm a completed request?

See #44234 for erasure request related tests.

  • Should any tests try to trigger the user_request_action_confirmed by loading the login page with confirmaction?

I couldn't find any tests for anything similar. May be worth investigating tests for the wp-login.php page separately.

  • Should any tests be done when the request doesn’t exist or was removed somehow?

Added assertions for the early returns in the function (an invalid post ID is provided, or an ID that is not of the user_request post type).

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


6 years ago

#34 @desrosj
6 years ago

Noting this here so it is not left out. After #44099 is committed, tests should be added to check that the email subject includes the action name.

#35 @desrosj
6 years ago

  • Summary changed from Admin emails after email confirmation don't work for GDPR requests to Admin emails after email confirmation don't work for data privacy requests

#36 @pbiron
6 years ago

  • Keywords has-patch added

#37 @ocean90
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

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


6 years ago

#39 @pbiron
6 years ago

Are these unit tests ready to land in 4.9.8?

#40 @desrosj
6 years ago

Gave this another review. They look good to me. In 43967.7.diff I updated the @since tags to reflect 4.9.8 and added public visibility to a few functions that were missing it.

@desrosj
6 years ago

#41 @desrosj
6 years ago

I also set this up on my wordpress-develop fork in Travis to show the tests passing.

#42 @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.

#43 @desrosj
6 years ago

  • Keywords commit added

#44 @SergeyBiryukov
6 years ago

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

In 43499:

Privacy: Add unit tests for _wp_privacy_send_request_confirmation_notification(), introduced in [43211].

Props birgire, desrosj, garrett-eclipse.
Fixes #43967.

#45 @SergeyBiryukov
6 years ago

In 43500:

Privacy: Add unit tests for _wp_privacy_send_request_confirmation_notification(), introduced in [43211].

Props birgire, desrosj, garrett-eclipse.
Merges [43499] to the 4.9 branch.
Fixes #43967.

Note: See TracTickets for help on using tickets.