WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 12 months ago

Last modified 12 months ago

#43985 closed defect (bug) (fixed)

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

Reported by: Chouby Owned by: desrosj
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.9.6
Component: I18N Keywords: has-patch needs-testing has-unit-tests
Focuses: administration, privacy Cc:
PR Number:

Description

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.

Attachments (18)

43985.patch (1016 bytes) - added by Chouby 19 months ago.
43985.diff (1.1 KB) - added by desrosj 19 months ago.
43985.2.diff (1.4 KB) - added by desrosj 19 months ago.
43985.3.diff (1.5 KB) - added by iandunn 19 months ago.
Fall back to site locale, simplify code w/ helper functions
43985.4.diff (1.5 KB) - added by iandunn 19 months ago.
Track if locale switch succeeded
43985.4.2.diff (1.7 KB) - added by lbenicio 19 months ago.
add unit tests to 43985.4
43438.4.3.diff (8.4 KB) - added by birgire 19 months ago.
43985.4.3.diff (8.4 KB) - added by birgire 19 months ago.
es_ES.mo (853 bytes) - added by desrosj 17 months ago.
Updated tests/phpunit/data/languages/es_ES.mo file with new test string.
43985.5.diff (11.8 KB) - added by desrosj 17 months ago.
43985.6.diff (3.1 KB) - added by desrosj 17 months ago.
Changes $switch_locales to singular for consistency.
43985.7.diff (21.8 KB) - added by desrosj 16 months ago.
de_DE.mo (697 bytes) - added by desrosj 16 months ago.
Additional changed .mo file to update.
43985.8.diff (21.7 KB) - added by desrosj 16 months ago.
43985.9.diff (22.7 KB) - added by earnjam 16 months ago.
Hurray for passing unit tests
43985.10.diff (21.8 KB) - added by desrosj 16 months ago.
43985.11.diff (20.9 KB) - added by desrosj 16 months ago.
43985.12.diff (20.9 KB) - added by desrosj 16 months ago.

Download all attachments as: .zip

Change History (75)

@Chouby
19 months ago

#1 @Chouby
19 months ago

  • Keywords has-patch added

#2 @TZ Media
19 months ago

  • Keywords gdpr needs-testing needs-unit-tests added

#3 @desrosj
19 months ago

  • Keywords commit added; needs-testing needs-unit-tests removed

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.

@desrosj
19 months ago

#4 follow-up: @allendav
19 months ago

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

#5 in reply to: ↑ 4 @Chouby
19 months ago

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
19 months ago

#6 @desrosj
19 months ago

  • Keywords needs-testing added; commit removed

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

#7 @desrosj
19 months ago

  • Milestone changed from Awaiting Review to 4.9.6

#8 @desrosj
19 months ago

  • Owner set to desrosj
  • Status changed from new to accepted

@iandunn
19 months ago

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

#9 @iandunn
19 months ago

  • Component changed from General to Mail
  • Focuses administration added
  • Keywords needs-unit-tests added

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

#10 @ocean90
19 months ago

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
19 months ago

Track if locale switch succeeded

#11 @iandunn
19 months ago

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

@lbenicio
19 months ago

add unit tests to 43985.4

#12 @desrosj
19 months ago

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

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


19 months ago

#14 @desrosj
19 months ago

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.

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


19 months ago

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


19 months ago

#17 @coreymckrill
19 months ago

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

@birgire
19 months ago

@birgire
19 months ago

#18 @birgire
19 months ago

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.

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


19 months ago

This ticket was mentioned in Slack in #core-committers by azaozz. View the logs.


19 months ago

#21 @desrosj
19 months ago

  • Milestone changed from 4.9.6 to 4.9.7

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

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


19 months ago

#23 @desrosj
19 months ago

  • Component changed from Mail to Privacy

Moving to the new Privacy component.

#24 @desrosj
19 months ago

  • Version changed from trunk to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#25 @desrosj
19 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

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


17 months ago

#27 @desrosj
17 months 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 by pbiron. View the logs.


17 months ago

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


17 months ago

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


17 months ago

#31 @swissspidy
17 months ago

  • Keywords changed from has-patch, needs-testing, needs-unit-tests to has-patch needs-testing needs-unit-tests

Minor nitpick: core usually uses $switched_locale without a plural

#32 @desrosj
17 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from 4.9.8 to 4.9.9

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
17 months ago

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

@desrosj
17 months ago

@desrosj
17 months ago

Changes $switch_locales to singular for consistency.

#33 @garrett-eclipse
16 months ago

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

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

#34 @garrett-eclipse
16 months ago

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.

#35 @desrosj
16 months ago

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

Last edited 16 months ago by desrosj (previous) (diff)

@desrosj
16 months ago

@desrosj
16 months ago

Additional changed .mo file to update.

#36 @desrosj
16 months ago

  • Component changed from Privacy to I18N
  • Focuses privacy added
  • Keywords has-unit-tests added; needs-refresh needs-unit-tests removed

#37 @desrosj
16 months ago

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.

@desrosj
16 months ago

#38 @earnjam
16 months ago

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.

Last edited 16 months ago by earnjam (previous) (diff)

#39 @earnjam
16 months ago

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
16 months ago

Hurray for passing unit tests

@desrosj
16 months ago

#40 @desrosj
16 months ago

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.

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


16 months ago

#42 @desrosj
16 months ago

@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

@desrosj
16 months ago

#43 follow-up: @birgire
16 months ago

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.
Version 0, edited 16 months ago by birgire (next)

#44 in reply to: ↑ 43 @desrosj
16 months ago

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

@desrosj
16 months ago

#45 @birgire
16 months ago

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

#46 @SergeyBiryukov
16 months ago

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

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.

#47 @SergeyBiryukov
16 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9.9.

#48 @SergeyBiryukov
15 months ago

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

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.

#49 @pento
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#50 @SergeyBiryukov
14 months ago

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.

#51 @SergeyBiryukov
14 months ago

  • Milestone changed from 4.9.9 to 5.0.1

#52 @pento
12 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#53 @pento
12 months ago

  • Keywords fixed-major removed
  • Milestone changed from 5.0.2 to 5.1
  • Resolution set to fixed
  • Status changed from reopened to closed

#54 follow-up: @garrett-eclipse
12 months ago

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

#55 in reply to: ↑ 54 @SergeyBiryukov
12 months ago

Replying to garrett-eclipse:

Did I miss the commit into 5.1 branch?

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

#56 @garrett-eclipse
12 months ago

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

#57 @SergeyBiryukov
12 months ago

No worries, it was indeed confusing :)

Note: See TracTickets for help on using tickets.