WordPress.org

Make WordPress Core

Opened 2 weeks ago

Last modified 3 days ago

#43985 accepted defect (bug)

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

Reported by: Chouby Owned by: desrosj
Milestone: 4.9.8 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch gdpr needs-testing needs-unit-tests
Focuses: administration Cc:

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 (8)

43985.patch (1016 bytes) - added by Chouby 2 weeks ago.
43985.diff (1.1 KB) - added by desrosj 2 weeks ago.
43985.2.diff (1.4 KB) - added by desrosj 13 days ago.
43985.3.diff (1.5 KB) - added by iandunn 12 days ago.
Fall back to site locale, simplify code w/ helper functions
43985.4.diff (1.5 KB) - added by iandunn 12 days ago.
Track if locale switch succeeded
43985.4.2.diff (1.7 KB) - added by lbenicio 10 days ago.
add unit tests to 43985.4
43438.4.3.diff (8.4 KB) - added by birgire 9 days ago.
43985.4.3.diff (8.4 KB) - added by birgire 9 days ago.

Download all attachments as: .zip

Change History (33)

@Chouby
2 weeks ago

#1 @Chouby
2 weeks ago

  • Keywords has-patch added

#2 @TZ Media
2 weeks ago

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

#3 @desrosj
2 weeks 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
2 weeks ago

#4 follow-up: @allendav
2 weeks ago

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

#5 in reply to: ↑ 4 @Chouby
2 weeks 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
13 days ago

#6 @desrosj
13 days 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
13 days ago

  • Milestone changed from Awaiting Review to 4.9.6

#8 @desrosj
13 days ago

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

@iandunn
12 days ago

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

#9 @iandunn
12 days 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
12 days 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
12 days ago

Track if locale switch succeeded

#11 @iandunn
12 days ago

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

@lbenicio
10 days ago

add unit tests to 43985.4

#12 @desrosj
10 days 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.


10 days ago

#14 @desrosj
10 days 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.


10 days ago

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


10 days ago

#17 @coreymckrill
9 days ago

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

@birgire
9 days ago

@birgire
9 days ago

#18 @birgire
9 days 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.


9 days ago

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


9 days ago

#21 @desrosj
8 days 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.


8 days ago

#23 @desrosj
7 days ago

  • Component changed from Mail to Privacy

Moving to the new Privacy component.

#24 @desrosj
6 days ago

  • Version changed from trunk to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#25 @desrosj
3 days ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

Note: See TracTickets for help on using tickets.