WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 28 hours ago

#44721 assigned defect (bug)

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

Reported by: desrosj Owned by: desrosj
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.9.6
Component: I18N Keywords: has-patch has-unit-tests commit
Focuses: privacy Cc:

Description

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.

Attachments (12)

44721.diff (831 bytes) - added by desrosj 7 months ago.
danish-erase-request.jpg (40.3 KB) - added by birgire 2 months ago.
confirmation.jpg (19.8 KB) - added by birgire 2 months ago.
admin-email.jpg (30.1 KB) - added by birgire 2 months ago.
user-fullfilment-email.jpg (23.8 KB) - added by birgire 2 months ago.
44721-2.diff (878 bytes) - added by birgire 5 weeks ago.
44721.2.diff (9.2 KB) - added by desrosj 5 days ago.
es_ES.mo (939 bytes) - added by desrosj 5 days ago.
de_DE.mo (777 bytes) - added by desrosj 5 days ago.
44721.3.diff (9.5 KB) - added by desrosj 5 days ago.
44721.4.diff (9.5 KB) - added by garrett-eclipse 4 days ago.
Updated unit tests to address issues and a failing test.
44721.5.diff (9.6 KB) - added by garrett-eclipse 29 hours ago.
Add missing @since to new tests

Download all attachments as: .zip

Change History (36)

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


8 months ago

@desrosj
7 months ago

#2 @desrosj
7 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

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

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


6 months ago

#4 @pento
5 months ago

  • Milestone changed from 4.9.9 to 5.0.1

#5 @pento
3 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#6 @pento
3 months ago

  • Milestone changed from 5.0.2 to 5.0.3

#7 @audrasjb
3 months ago

  • Milestone changed from 5.0.3 to 5.1

Moving this one to 5.1 along with #44234.

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


2 months ago

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


2 months ago

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


2 months ago

@birgire
2 months ago

@birgire
2 months ago

#11 @birgire
2 months ago

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.

#12 @garrett-eclipse
2 months ago

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

#13 @garrett-eclipse
2 months ago

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

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

#14 @desrosj
2 months ago

  • Keywords needs-unit-tests needs-testing added; commit removed
  • Milestone changed from 5.1 to 5.2

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
5 weeks ago

#15 @birgire
5 weeks ago

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

That fixed the issue during my testing.

#16 @garrett-eclipse
4 weeks ago

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.

#17 @garrett-eclipse
4 weeks ago

  • Keywords commit added; needs-testing removed

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
5 days ago

#18 @desrosj
5 days ago

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

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
5 days ago

@desrosj
5 days ago

@desrosj
5 days ago

#19 @desrosj
5 days ago

  • Keywords needs-testing added; commit removed

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

#20 @birgire
5 days ago

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
4 days ago

Updated unit tests to address issues and a failing test.

#21 @garrett-eclipse
4 days ago

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

#22 @birgire
39 hours ago

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
29 hours ago

Add missing @since to new tests

#23 @garrett-eclipse
29 hours ago

  • Keywords commit added; needs-testing removed

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.

#24 @garrett-eclipse
28 hours ago

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.

Note: See TracTickets for help on using tickets.