WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 3 months ago

#46303 closed enhancement (fixed)

Update wp_privacy_send_personal_data_export_email to provide the same filters as _wp_privacy_send_erasure_fulfillment_notification

Reported by: garrett-eclipse Owned by: garrett-eclipse
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests needs-dev-note commit
Focuses: Cc:
PR Number:

Description

Hello,

With the Erasure Complete/Fulfilllment email the subject is filtered using the user_erasure_complete_email_subject filter.
https://github.com/WordPress/WordPress/blob/5.1/wp-includes/user.php#L3170-L3189

However with the Export Complete email the subject is unfiltered;
https://github.com/WordPress/WordPress/blob/5.1/wp-admin/includes/file.php#L2250

We should be consistent and apply the same type of filter on the Export Complete email found in wp_privacy_send_personal_data_export_email.

Cheers

Attachments (5)

46303.patch (1.1 KB) - added by thakkarhardik 10 months ago.
Filter added for the Export Complete email subject
46303.2.patch (1.1 KB) - added by thakkarhardik 10 months ago.
Here's the patch with updated root path
46303.3.diff (5.6 KB) - added by garrett-eclipse 9 months ago.
Updated patch to setup $email_data and include wp_privacy_personal_data_email_to and other updates
46303.4.diff (9.6 KB) - added by garrett-eclipse 3 months ago.
Refreshed patch to apply cleanly in new privacy-tools.php, updated versions and CS issues. Introduced unit tests.
46303.5.diff (9.7 KB) - added by garrett-eclipse 3 months ago.
Adjusted the phpdoc to codequote the $email_data variable.

Download all attachments as: .zip

Change History (23)

@thakkarhardik
10 months ago

Filter added for the Export Complete email subject

#1 @thakkarhardik
10 months ago

  • Keywords has-patch added; needs-patch removed

#2 follow-up: @garrett-eclipse
10 months ago

  • Keywords needs-refresh added; good-first-bug has-patch removed

Thanks @thakkarhardik, greatly appreciate the patch, a good start here. I've got a few minor notes below for you.

  1. Attempting to apply the patch I'm greeted with error, and have to manually correct the path;
    Running "patch:46303" (patch) task
    (Stripping trailing CRs from patch.)
    can't find file to patch at input line 5
    Perhaps you used the wrong -p or --strip option?
    The text leading up to this was:
    --------------------------
    |Index: includes/file.php 
    

This just means you created your patch from the wp-admin directory, please 'ensure that you are in the root directory created by your SVN checkout'.
Creating a Patch - https://make.wordpress.org/core/handbook/tutorials/working-with-patches/#creating-a-patch

  1. Comparing this to the user_erasure_complete_email_subject filter I really like the position (prior to the email content setup, and the use of $email_data to provide the filter with the request, user_email, privacy_policy_url and other useful data.

Here's the snippet I'm speaking of - https://github.com/WordPress/WordPress/blob/5.1/wp-includes/user.php#L3156-L3189

Let me know if you have any questions and thanks again for the contribution.
Cheers

#3 @thakkarhardik
10 months ago

Hi @garrett-eclipse , thanks for pointing that out. Really appreciate that. Next time I will take care of it.

@thakkarhardik
10 months ago

Here's the patch with updated root path

#4 @thakkarhardik
10 months ago

  • Keywords has-patch added; needs-refresh removed

#5 in reply to: ↑ 2 @garrett-eclipse
10 months ago

Thanks for correcting the path @thakkarhardik. Please see below for my other feedback to take into account;

garrett-eclipse:

  1. Comparing this to the user_erasure_complete_email_subject filter I really like the position (prior to the email content setup, and the use of $email_data to provide the filter with the request, user_email, privacy_policy_url and other useful data.

Here's the snippet I'm speaking of - https://github.com/WordPress/WordPress/blob/5.1/wp-includes/user.php#L3156-L3189

Let me know if you have any questions,
Cheers

#6 @garrett-eclipse
9 months ago

  • Summary changed from Create user_erasure_complete_email_subject to match user_erasure_complete_email_subject for privacy export emails to Create wp_privacy_personal_data_email_subject to match user_erasure_complete_email_subject for privacy export emails

#7 @birgire
9 months ago

Thanks for the patch @thakkarhardik

In addition to what @garrett-eclipse said, we will also need to add @since 5.x.y in the filter's doc block. See e.g. the other filter's doc block:

https://github.com/WordPress/WordPress/blob/5.1/wp-includes/user.php#L3170-L3189

I also wonder about the site name, as it's also an available parameter to the other filter and used in the email subject.

@garrett-eclipse
9 months ago

Updated patch to setup $email_data and include wp_privacy_personal_data_email_to and other updates

#8 @garrett-eclipse
9 months ago

  • Component changed from Administration to Privacy
  • Focuses privacy removed
  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to garrett-eclipse
  • Status changed from new to assigned
  • Summary changed from Create wp_privacy_personal_data_email_subject to match user_erasure_complete_email_subject for privacy export emails to Update wp_privacy_send_personal_data_export_email to provide the same filters as _wp_privacy_send_erasure_fulfillment_notification

Thanks for the refresh @thakkarhardik and the feedback @birgire while working through this and comparing with the _wp_privacy_send_erasure_fulfillment_notification function I found several inconsistencies and decided to address them in 46303.3.diff.

  1. I've introduced the wp_privacy_personal_data_email_to filter to match user_erasure_fulfillment_email_to.
  2. I've setup the $email_data array and used it in the new subject filter as well as the content filter.
  3. Introducing the wp_privacy_personal_data_email_subject that @thakkarhardik worked on I added the $site_name and $email_data.

I've moved this to the 5.3.0 milestone and versioned as such. Please test when you have a chance and we'll work on unit tests.

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


5 months ago

#10 @rconde
3 months ago

Any info on when wp_privacy_personal_data_email_subject is going to be released?
Thanks

@garrett-eclipse
3 months ago

Refreshed patch to apply cleanly in new privacy-tools.php, updated versions and CS issues. Introduced unit tests.

#11 follow-up: @garrett-eclipse
3 months ago

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

Hi @rconde thanks for the ping, I'm hoping to land this in 5.3.0.

I've refreshed the patch in 46303.4.diff so it applies cleanly now that everything lives in privacy-tools.php. Updated version numbers and addressed some CS issues.

Also introduced unit tests;

  • Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_email_address_of_recipient_should_be_filterable - Tests new wp_privacy_personal_data_email_to filter.
  • Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_email_subject_should_be_filterable - Tests new wp_privacy_personal_data_email_subject filter.
  • Tests_Privacy_WpPrivacySendPersonalDataExportEmail::test_email_content_should_be_filterable_using_email_data - Tests addition of $email_data to the wp_privacy_personal_data_email_content filter.
  • These are all found in the wpPrivacySendPersonalDataExportEmail.php test suite.

@birgire would you mind reviewing my unit tests and review/test the patch in general.

@rconde / @thakkarhardik would appreciate any review/testing you're able to do on the patch.

Thank you
P.S. Marking with needs-dev-note as this patch introduces two new filters (wp_privacy_personal_data_email_to & wp_privacy_personal_data_email_subject) and adds $email_data to the wp_privacy_personal_data_email_content filter.

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


3 months ago

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


3 months ago

#14 in reply to: ↑ 11 @pputzer
3 months ago

Replying to garrett-eclipse:

Hi @rconde thanks for the ping, I'm hoping to land this in 5.3.0.

I've refreshed the patch in 46303.4.diff so it applies cleanly now that everything lives in privacy-tools.php. Updated version numbers and addressed some CS issues.

Patch looks good to me.

#15 @garrett-eclipse
3 months ago

  • Keywords commit added
  • Status changed from assigned to accepted

Thanks @pputzer this still applied nicely to trunk with all unit tests passing, moving to commit.

#16 @birgire
3 months ago

  • Keywords needs-testing removed

I tested this successfully with a request from user1@example.com and then with a plugin:

<?php
Plugin Name: Test 46303

add_filter( 'wp_privacy_personal_data_email_to', function($user_email) {
	return 'user2@example.com';
} );

add_filter( 'wp_privacy_personal_data_email_subject',function($subject ) {
	return 'Test';
} );

Steps to test:

  • A request is made for user1@example.com.
  • A verification link is sent to user1@example.com.
  • user1 clicks on verification link in email.
  • Admin sends the export link (clicks the button).
  • The export download link is sent to user2@example.com emailbox and not user1@example.com emailbox as expected.

The privacy test group runs successfully.

One should though use the email filter with great care, to avoid sending the data export link to wrong emails!

ps:

Just a minor regarding PHPdoc, maybe we should replace:

* @since 5.3.0 Introduced the $email_data array.

with

 * @since 5.3.0 Introduced the `$email_data` array.

for the changelog display on https://developer.wordpress.org/reference/

It took me few seconds though to notice in the test filter, that $site_url of $email_data are not value generated within the string as it's single quote but not double quote :-)

@garrett-eclipse
3 months ago

Adjusted the phpdoc to codequote the $email_data variable.

#17 @garrett-eclipse
3 months ago

Thanks @birgire I appreciate the testing there. Looks good, I did make that tweak to the phpdoc as you suggested in 46303.5.diff

#18 @SergeyBiryukov
3 months ago

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

In 46265:

Privacy: Introduce wp_privacy_personal_data_email_to and wp_privacy_personal_data_email_subject filters.

Pass email data to the wp_privacy_personal_data_email_content filter.

Props garrett-eclipse, thakkarhardik, birgire.
Fixes #46303.

Note: See TracTickets for help on using tickets.