Privacy: The user request email should be sent in the user language

[Chouby]

When using the new admin tools, "Remove personal data" or "Export personal data", the user request email is sent in the admin language. It should be sent in the suser language.

[desrosj]

Hi @Chouby, thanks for the ticket!

Good catch. I had issues applying your patch so I refreshed it. I gave this some testing and all looks good to me.

[allendav]

Apologies, but how exactly does a user set their locale?

[Chouby]

Replying to allendav:

Apologies, but how exactly does a user set their locale?

This can be done in Users > Your Profile. But of course plugins can add other ways.

[desrosj]

43985.2.diff​ is a refresh, and also adds locale switching to _wp_privacy_send_erasure_fulfillment_notification(), which was added after this ticket was created (#43973).

[iandunn]

Fall back to site locale, simplify code w/ helper functions

[iandunn]

I think the fallback locale for visitors should be the site's default locale, rather than the current admin's locale. For example, the admin could be a native Spanish speaker who also speaks English, and is moderating an English site. For their convenience, they set Spanish as their user locale. The visitor to an English site is more likely to speak English than Spanish, though.

43985.3.diff​ makes that change, and also simplifies the code by relying on the fact that get_user_locale() falls back to get_locale(), and that there's a helper function for is_locale_switched().

That tests well for me, but since we're in RC, can I ask a few of you to test it as well, before I commit it to trunk? Then we can ask another committer review it for backport to 4.9 branch.

If someone has time to write unit tests, that'd be great too (that's true of the 4.9 tickets in general).

[ocean90]

is_locale_switched() shouldn't be used here, instead use the return value of switch_to_locale() to check if the specific request for switching the locale was successful and not any other locale switch. It's similar to ms_is_switched(), see #38253.

[iandunn]

Track if locale switch succeeded

[iandunn]

Ah, good catch, thanks! 43985.4.diff​ addresses that.

[lbenicio]

add unit tests to 43985.4

[desrosj]

@lbenicio Thanks for the patch adding tests. It seems they are in the wrong place, though. All unit tests should be in the tests/phpunit directory. There is a guide for ​Writing PHPUnit Tests in the handbook. tests/phpunit/user.php seems like the appropriate place for this ticket's tests.

[desrosj]

Working on figuring out why, but wanted to document the issue that I am currently having with this patch. I have tested this on fresh installs of trunk using both PHP 7.1 and PHP 7.2 using VVV.

I have 3 test users:

• One (the admin) with en_US.
• One (contributor) with ar.
• One (author) with de_DE.

Submitting requests for each user only results in the confirmation email being sent to the administrator when the site default language is en_US. When the site is changed to de_DE as the default, no users receive emails.

When using var_dump() on the PHPMailer object in wp_mail(), it shows [ErrorInfo] => Could not instantiate mail function., and when using var_dump() on the $mail_error_data variable, it shows phpmailer_exception_code as 2.

[coreymckrill]

I just created #44084 for the related issue of translating the personal data export file.

[birgire]

43985.4.3.diff​ is a skeleton for the wp_send_user_request() unit tests.

The test_function_should_send_user_request_email_in_user_language() still fails, because there are no es_ES translations for e.g. the [%1$s] Confirm Action: %2$s string in /tests/phpunit/data/language/es_ES.mo.

I guess there are few ways to handle that in the test, like adding it to the es_ES.mo/es_ES.po test files or use a gettext filter that checks the user locale before overriding. I also tried to filter the subject with an existing es_ES string, but that didn't work for the translated string.

[desrosj]

This needs more time. 4.9.6 RC2 is happening in a few minutes, punting.

[desrosj]

Moving to the new Privacy component.

[desrosj]

Marking privacy bugs as introduced in 4.9.6.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[desrosj]

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

[swissspidy]

Minor nitpick: core usually uses $switched_locale without a plural

[desrosj]

I am no longer experiencing the issues with wp_mail() that I detailed above.

43985.5.diff​ combines 43985.4.diff​ and 43985.4.3.diff​ and makes a few changes/additions.

• Changes @since tags to 4.9.9.
• Adds the string being tested to /tests/phpunit/data/language/es_ES.mo (updated .mo file incoming separately).
• Adds missing visibility to setUp() and tearDown() methods.
• Adds missing @package tag to the test class file docblock.
• Fixes a few typos in method docblocks and method names.
• Added tests for a user export request where the email does not belong to a registered user.
• Removed the dynamic aspect of the modify_email_subject() and modify_email_content() methods (the email does not need to be added to the filtered subject to test that it is filterable).

[desrosj]

Updated tests/phpunit/data/languages/es_ES.mo file with new test string.

[desrosj]

Changes $switch_locales to singular for consistency.

[garrett-eclipse]

Hey @desrosj checking 43985.6.diff I found that it's completely missing the Unit tests (wpSendUserRequest.php) that were present in 43985.5.diff.

As well the wp_mail returns a bool only but the wp_send_user_request indicates it returns WP_Error|bool;
* @return WP_Error|bool Will return true/false based on the success of sending the email, or a WP_Error object.
Should the return approach be updated to match more what's done in wp_privacy_send_personal_data_export_email here;
​https://github.com/WordPress/WordPress/blob/d9df5dec117ca01211c02b90cb88e015e697d68e/wp-admin/includes/file.php#L2248
*Althought maybe instead of just standard 'error' the error code could be more specific.

Cheers

[garrett-eclipse]

As well looking at the switch_to_locale, if the request is for an email which doesn't associate to a valid user then the user_id on the request is 0. In this case get_user_locale will return the locale of the current user (admin taking the action). So I am wondering is it the desire to use the admin user locale when the request user isn't a real WP_User or in that case should it default to the site locale? If so a check should be done for $request->user_id == 0 and either supply false to get_user_locale or simply call get_locale in that case.

[desrosj]

Ah, I forgot to do svn add on wpSendUserRequest.php test file when I created 43985.6.diff​.

wp_send_user_request() can return a WP_Error at the very beginning of the function if an invalid request ID is passed. The wp_mail() function will return true/false, though. I do like making the functions consistent, so I have updated the function to return a WP_Error on failure instead of false.

I could not reproduce the issue you described above with the email sending in the current administrator's locale when the user is not registered. I wrote some unit tests for each scenario that should have failed if what you described was true, but they all passed. I dug a little deeper and figured out the problem. get_user_locale() does call wp_get_current_user() when the passed user ID is 0, but it performs an explicit check for a 0 integer. The user ID property of the WP_User_Request class is being stored as a string. Since the inline documentation for the user ID property says it is an int, this is definitely a bug. I included that fix in the patch as well.

In summary, 43985.7.diff​ contains:

• Changed wp_send_user_request() to return a WP_Error when the email does not send successfully to match the behavior in wp_privacy_send_personal_data_export_email().
• Updated the WP_Error codes in wp_get_user_request_data(), wp_privacy_send_personal_data_export_email(), and wp_send_user_request() to be more descriptive and consistent when requests are invalid and emails fail.
• Guarantee the WP_User_Request->user_id property is in fact an integer.
• Updated wp_send_user_request() to ensure the email is sent in the requested user's locale (or the site's default locale if they are not a registered user) when the administrator creating the request uses a different locale. Added all necessary tests.
• Added a string to the de_DE.po file in order to have two non-default locales to ensure the correct behaviors.

I also created #44721 for the fulfillment email and pulled that change out of 43985.7.diff​ to prevent this patch/commit from ballooning (that function currently has no tests, so needs more attention).

[desrosj]

Additional changed .mo file to update.

[desrosj]

I pulled the change to WP_User_Request into its own ticket after discussing with @jorbin (see #44723).

43985.8.diff​ has that change pulled out.

The unit tests pass when run as a group (phpunit --group privacy or phpunit --group 43985), but when you run them in conjunction with the l10n tests, there is one failure. The localeSwitcher.phpseems to be causing the issue, but haven't figured out why and need to step away for the day.

[earnjam]

I worked on this with @desrosj this afternoon. After a little more investigation, I can get the tests to pass by adding some set_current_screen() calls. Seems $current_screen isn't getting changed between tests, which throws off this block in WP_Locale_Switcher->switch_to_locale() and preventing the locale from getting switched correctly.

<?php
public function switch_to_locale( $locale ) {
        $current_locale = is_admin() ? get_user_locale() : get_locale();
        if ( $current_locale === $locale ) {
                return false;
        }

The last 3 tests in Tests_Locale_Switcher all run set_current_screen() calls, which are what is tripping up these later tests when you pair the User group with the l10n group on the tests.

Need to dig in to the specific screen settings here a bit more to figure out why they're failing. Don't want to set that to get them to pass and mask a bigger underlying problem.

[earnjam]

Ok, I think I figured out the issue.

get_locale() tries to use the global $locale value first. If it's not set, it works down a list checking other things like WPLANG constant or the WPLANG option, eventually settling on en_US if there is no other choice.

When it figures out the locale, it updates the global $locale for future use. This global was getting set to en_US early on and then persisting across tests, meaning that any check of get_locale() was returning that en_US value.

switch_to_locale() only does something if the locale passed to it is different than the value of get_locale() outside of the admin. Inside the admin, it only switches if it's different than the locale of the current user.

So on our case, we had 2 globals throwing everything off here.

1. $current_screen->id was always set to front because of the tests at the end of Tests_Locale_Switcher setting it and these tests not resetting it to anything else. This caused is_admin() to be false for all our tests, making switch_locale() always determine whether to change based of the return of get_locale() and not necessarily the current user.
2. The $locale global was always set to en_US and not being updated, causing get_locale() to return the wrong locale after it was supposed to have been switched using the WPLANG option.

Basically we just need to add unset( $GLOBALS['locale'] and set_current_screen( 'dashboard' ) to the setUp() function to make sure those are consistent across tests.

[earnjam]

Hurray for passing unit tests

[desrosj]

Thanks @earnjam! Your patch looked good, but the type casting on the author ID had made it back in to the patch. 43985.10.diff​ removes that change so it can get more attention in #44723.

[desrosj]

@swissspidy was kind enough to give me a review on this! He recommended using restore_previous_locale() instead of restore_current_locale(). The latter empties the history of locale switching and restores the original locale. If a plugin has performed additional locale switches before the locale is changed for the email, that history would be lost. restore_previous_locale() only undoes the most recent switch.

The test class was also causing a widget test to fail. This was caused by the set_current_screen() calls. I added some screen related global variable unset()s to the tearDown() method, moved the unset() in the setUp() to tearDown() to ensure it is unset after the last test method runs, and consolidated some repeated code into the tearDown() method.

He also asked for the user creation to be moved into wpSetUpBeforeClass(). The test class now only creates two users total.

The passing build can be seen on my fork: ​https://travis-ci.org/desrosj/wordpress-develop/builds/413276594

[birgire]

This seems to be in good progress.

The latest patch 43985.11.diff​ runs successfully on my install.

Here are few things from reviewing the latest 43985.11.diff​:

• There are currently no restore_previous_locale(). Was that intentionally?

• There is some discrepancy between test method descriptions and test method function names

Here is "site has a default locale" vs site_has_non_default_locale:

/**
 * The function should respect the user locale settings when the site has a default locale and the administrator
 * uses the site default.
 *
 * @ticket 43985
 */
public function test_should_send_user_request_email_in_user_locale_when_site_has_non_default_locale() {

It looks like the site is not in default locale (en_US) here:

/**
 * The function should respect the user locale settings when the site has a default locale, the administrator
 * has a different locale, and the user uses the site's default.
 *
 * @ticket 43985
 */
public function test_should_send_user_request_email_in_user_locale_when_admin_has_different_locale_than_site() {
	update_option( 'WPLANG', 'es_ES' );
	switch_to_locale( 'es_ES' );

Same here:

/**
 * The function should respect the user locale settings when the site has a default locale and both the
 * administrator and the user use different locales.
 *
 * @ticket 43985
 */
public function test_should_send_user_request_email_in_user_locale_when_admin_and_site_have_different_locales() {
	update_option( 'WPLANG', 'es_ES' );
	switch_to_locale( 'es_ES' );

• A typo in 'erase.request.from.unergistered.user@example.com', should be 'erase.request.from.unregistered.user@example.com'.
• There are two wp_create_user_request() and two wp_set_current_user() in test_should_send_user_request_email_in_user_locale_when_admin_has_different_locale_than_site().
• Are the global $taxnow and $typenow (implicitly) set in the test methods? It was always empty during my test runs.
• Should the manual remove_filter be avoided in test methods if possible and rely on the built-in _restore_hooks() during tear down? Sometimes it's nice though to see the scope of the filtering, i.e. when add_filter is added and when remove_filter. I'm just mentioning it here for future reference.
• Should some of the tests be marked with @group l10n?

[desrosj]

Replying to birgire:

Thanks for the thorough review!

• There are currently no restore_previous_locale(). Was that intentionally?

I had been experimenting with making the tests faster. Removing that had seemed to make the tests run a bit faster. I added restore_previous_locale() to prevent the class from adversely affecting another one down the road.

• There is some discrepancy between test method descriptions and test method function names

I fixed the discrepancies and updated some of the inline documentation. I was mixing up too many defaults. There is the core default language (en_US), and the site default language, which is selected on the user screen and switches the user to whichever language is selected for the site. They should match and be more clear now.

• There are two wp_create_user_request() and two wp_set_current_user() in test_should_send_user_request_email_in_user_locale_when_admin_has_different_locale_than_site().

Fixed! Copy paste error. This function's new name is test_should_send_user_request_email_in_user_locale_when_admin_and_site_have_different_locales()

• Are the global $taxnow and $typenow (implicitly) set in the test methods? It was always empty during my test runs.

I unset() the three globals that can be altered in set_current_screen(), but I can see those two being a little overcautious. We can remove those unset()s if needed.

• Should the manual remove_filter be avoided in test methods if possible and rely on the built-in _restore_hooks() during tear down? Sometimes it's nice though to see the scope of the filtering, i.e. when add_filter is added and when remove_filter. I'm just mentioning it here for future reference.

I don't know that there is a standard for this. I removed remove_filter() calls that were not necessary for the remainder of the test method to succeed it in the interest of brevity. But, like the previous item, don't feel strongly that this must be changed.

• Should some of the tests be marked with @group l10n?

Good idea! Added that tag to the appropriate tests.

The other things you mentioned were fixed as well in 43985.12.diff​

[birgire]

Thanks for the update @desrosj - the descriptions looks good and informative.

[SergeyBiryukov]

In 43568:

Privacy: Ensure the user request email is sent in the requested user's locale (or the site's default locale if they are not a registered user) when the administrator creating the request uses a different locale.

Props desrosj, Chouby, iandunn, lbenicio, birgire, earnjam, swissspidy, garrett-eclipse.
Fixes #43985.

[SergeyBiryukov]

Reopening for 4.9.9.

[SergeyBiryukov]

In 43614:

Privacy: Ensure the user request email is sent in the requested user's locale (or the site's default locale if they are not a registered user) when the administrator creating the request uses a different locale.

Props desrosj, Chouby, iandunn, lbenicio, birgire, earnjam, swissspidy, garrett-eclipse.
Merges [43568] to the 4.9 branch.
Fixes #43985.

[pento]

[43614] needs to be reverted from the 4.9 branch. This ticket can then be moved to the 5.0.1 milestone.

[SergeyBiryukov]

In 43705:

Privacy: Revert [43614] from the 4.9 branch.

This change is out of the 4.9.x scope, and will be reintroduced in 5.0.x.

See #43985.

[garrett-eclipse]

Hi @pento,

Sorry if I've missed something here but it looks like commit 43705 by @SergeyBiryukov reverted the change from 4.9.x, but I don't see it being added to 5.1 so should this be closed as fixed? Did I miss the commit into 5.1 branch?

Thanks

[SergeyBiryukov]

Replying to garrett-eclipse:

Did I miss the commit into 5.1 branch?

[43568] was the commit to trunk, which is now 5.1.

[garrett-eclipse]

Thanks @SergeyBiryukov got confused by the branch switcheroo there. I see this in trunk, appreciate the clarity

[SergeyBiryukov]

No worries, it was indeed confusing :)