The privacy data erase fulfillment email should be in the user's language

[desrosj]

When using the new admin tools, the email sent to the user when their data has successfully been erased is currently sent in the language of the user fulfilling the request. It should be sent in the language of the user who's data is being erased (or the site's default locale if they are not a registered user).

Related: #43985, #44084.

[desrosj]

Unit tests are blocked by the class being introduced in #44234. I can write them once that is committed.

[audrasjb]

Moving this one to 5.1 along with #44234.

[birgire]

Did the following test with the patch in 44721.diff​ applied, with the setup:

• Registered user that has languages set to Danish.
• Site language is English.
• Admin user with English.

Test steps:

• The admin user adds a Data Erasure Request for the registered user above.
• The registered user received an email in Danish. See the screenshot in danish-erase-request.jpg​ is from Gmail
• The registered user clicks the confirmation link within the email and sees the confirmation in confirmation.jpg​. It was in English. There was another ticket #44550 addressing this, as this should be in Danish here.
• The admin user receives the confirmation email in English as expected. See admin-email.jpg​.
• The admin erases personal data.
• The registered user receives an fulfillment email in English. But I think this should be in Danish. See user-fullfilment-email.jpg​.

[garrett-eclipse]

Thanks @birgire

I'm finding the same with French setup. Everything looks good aside from the confirm page and fulfillment email. I did the same text with export as well as erasure and in that workflow it's the same where we want the confirm page and fulfillment email in the user locale. I created #46056 for the fulfillment email and we already have #44550 for the confirm page as you mentioned.

@desrosj this is good, and we'll deal with the other non-locale page/email in future.

Cheers

[garrett-eclipse]

As @swissspidy mentioned unit tests aren't required here. And @birgire as well as myself has tested this so am moving forward.

[desrosj]

I am also seeing the incorrect results @birgire detailed above for the fulfillment email, which this ticket is meant to fix. Since this is the 11th hour before the 5.1 beta 2 deadline, let's punt this so we can commit it more confidently.

[birgire]

44721-2.diff​ updates 44721.diff​ with the correct $request variable.

That fixed the issue during my testing.

[garrett-eclipse]

Thanks @birgire that makes sense why it worked initially but not during testing as #44708 was committed in 5.1 and it renamed $request_data to $request.

[garrett-eclipse]

En français, c'est bien

Subject:
[Privacy Locale Emails] Demande d’effacement effectuée
Message:
Bonjour, Votre demande d’effacement de vos données personnelles sur Privacy Locale Emails a bien été réalisée. Si vous avez d’autres questions ou demandes, veuillez contacter l’administrateur du site. Pour plus d’informations, vous pouvez aussi lire notre politique de confidentialité : Cordialement, L’équipe de Privacy Locale Emails ​http://localhost:8888/privacy-locale

I got around to testing your patch @birgire and it works nicely for the erasure fulfillment email. The export fulfillment email is still broken but that's to be dealt with in #46056 which I'll update now.

@desrosj want to take the last swing? I've marked as commit as it feels good to go aside from unit tests but those have been previously mentioned as not required.
Reference - ​https://wordpress.slack.com/archives/C9695RJBW/p1548088806258300

[desrosj]

44721.2.diff​ has the associated unit tests. @garrett-eclipse as agreed to review. I have sent him the updated .mo files for testing.

[desrosj]

44721.3.diff​ fixed a mistake I had in the last two tests.

[birgire]

Great, thanks for the tests @desrosj

They run successfully on my test install.

From a quick skimming I noticed that in the tests in 44721.3.diff​, there's the 'erase_personal_data' action name instead of the usual 'remove_personal_data' one.

Example:

$request_id = wp_create_user_request( 'export-user-not-registered@example.com', 'erase_personal_data' );

and perhaps in this context, the email could be like erasure-user-not-registered@example.com.

I always have to remind myself that the erase_ prefix is sometimes remove_ :-)

PS: I noticed that there's no erasure action check in _wp_privacy_send_erasure_fulfillment_notification(), so I think any kind of a user request, would go through with _wp_privacy_send_erasure_fulfillment_notification(). Even this one:

$request_id = wp_create_user_request( 'export-user-not-registered@example.com', 'any-kind-of-action-here' );

might go through?

This could be investigated in another ticket if needed.

[garrett-eclipse]

Updated unit tests to address issues and a failing test.

[garrett-eclipse]

Nice catch @birgire you're correct that the supported privacy_action_request_type for erasure is remove_personal_data as defined in _wp_privacy_action_request_types here;
​https://github.com/WordPress/wordpress-develop/blob/5.1.1/src/wp-includes/user.php#L2842-L2855

It does look like any call to wp_create_user_request doesn't take into account this check allowing the any-action-name scenario. It would probably make the most sense to move the check from _wp_personal_data_handle_actions into wp_create_user_request so as to check all user_request creation calls. The check can be found here;
​https://github.com/WordPress/wordpress-develop/blob/5.1.1/src/wp-admin/includes/user.php#L691-L698
*This is the only instance of checking against _wp_privacy_action_request_types and throwing an error. There is one other check but it's just in an if conditional in _wp_privacy_account_request_confirmed_message.

I agree this deviates from the issue this ticket seeks to address so opened another here to investigate further;
#46536
*There are also two other pre-existing unit tests which were using the invalid request action name, to get my patch on the above ticket passing tests I had to address them. I made a note on that ticket that it'll need a refresh once this ticket is merged or vice versa.

That being said I uploaded [44721.4.diff] to update @desrosj unit tests to account for @birgire recommendations.

• As I addressed the erase_personal_data cases in tests on #46536 I focused on only fixing the ones directly related to @desrosj patch.
• I also updated the email to erase-user-not-registered@example.com to match what's found in test_should_send_fulfillment_email_in_site_locale as @birgire noted.

I also found the tests were failing initially and struggled until I realized that's why you supplied the .mo files seperately. It's unfortunate that svn doesn't support binary files as I tried to include them in the patch to avoid issues with unit tests. @desrosj will those .mo files be committed to trunk in time or how do we ensure they're submitted with the patch to avoid unit test failures.

Also I had to adjust test_should_send_fulfillment_email_in_user_locale_when_both_have_different_locales_than_site in order to get past the following error;

1) Tests_Privacy_WpPrivacySendErasureFulfillmentNotification::test_should_send_fulfillment_email_in_user_locale_when_both_have_different_locales_than_site
Failed asserting that '[Test Blog] Solicitud de borrado completada' contains "Erasure Request Fulfilled".
/Users/garretthyder/WordPress/44721-privacy-data-erase-fullfullment-email-language/tests/phpunit/tests/privacy/wpPrivacySendErasureFulfillmentNotification.php:385

@desrosj can you take a look at this failing test, I feel it's due to en_US not being a locale of WP but the default that's causing issues here but am unsure. Here's the original failing test case;

/**
 * The function should respect the user locale settings when the site is not en_US and both the
 * administrator and the user use different locales.
 *
 * @ticket 44721
 * @group l10n
 */
public function test_should_send_fulfillment_email_in_user_locale_when_both_have_different_locales_than_site() {
	update_option( 'WPLANG', 'es_ES' );
	switch_to_locale( 'es_ES' );

	update_user_meta( self::$admin_user->ID, 'locale', 'de_DE' );
	update_user_meta( self::$request_user->ID, 'locale', 'en_US' );

	wp_set_current_user( self::$admin_user->ID );

	_wp_privacy_send_erasure_fulfillment_notification( self::$request_id );
	$mailer = tests_retrieve_phpmailer_instance();

	$this->assertContains( 'Erasure Request Fulfilled', $mailer->get_sent()->subject );
}

*Manual test of the same situation there's no issue and you receive the en_US version so seems to just be the test case but would love second eyes to be sure.

Thanks @birgire for the ever-vigilant eyes. Also @desrosj thank you for the initial unit tests. Mind giving it another once over and we can move it back into 'commit'.

[birgire]

Thanks @garrett-eclipse

I tested 44721.4.diff​ on Travis here:

​https://travis-ci.com/birgire/wordpress-develop/builds/104831970

and it seems to pass all core tests successfully.

[garrett-eclipse]

Add missing @since to new tests

[garrett-eclipse]

Thanks @birgire I appreciate the tests, you'll have to show me that Travis CI setup.
Minor update in 44721.5.diff to add missing @since tags to the new unit tests.
Moving this forward.

[garrett-eclipse]

I've brought #46056 inline with this patch to provide similar unit tests as found here for it's personal data export email. Once this is committed it will need a refresh.

[SergeyBiryukov]

In 45039:

Privacy: Ensure the privacy data erase fulfillment email is sent in the locale of the user whose data is being erased (or the site's default locale if they are not a registered user) when the administrator fulfilling the request uses a different locale.

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

[garrett-eclipse]

Thanks for the commit @SergeyBiryukov