Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#44234 closed enhancement (fixed)

Add missing unit tests for erasing personal data by username or email address

Reported by: desrosj's profile desrosj Owned by: desrosj's profile desrosj
Milestone: 5.1 Priority: low
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests
Focuses: administration Cc:

Description

This ticket picks up where #43546 left off to add missing tests for the functionality introduced and to keep when the enhancement and tests were committed clear.

See #43602.

Attachments (3)

44234.diff (6.3 KB) - added by desrosj 6 years ago.
44234-2.diff (8.7 KB) - added by birgire 6 years ago.
44234.2.diff (8.7 KB) - added by desrosj 5 years ago.
Refreshed patch with 5.1.0.

Download all attachments as: .zip

Change History (28)

#1 @desrosj
6 years ago

  • Priority changed from normal to low

Marking as lower priority.

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


6 years ago

#3 @audrasjb
6 years ago

  • Owner set to audrasjb
  • Status changed from new to assigned

Hi,

As I said during today's bug scrub, I'll try to write the unit tests related to this ticket :)

Cheers,

Jb

#4 @ocean90
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

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


6 years ago

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


6 years ago

#7 @pbiron
6 years ago

  • Milestone changed from 4.9.8 to 4.9.9

4.9.8 has hit RC, moving to 4.9.9.

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


6 years ago

@desrosj
6 years ago

#9 @desrosj
6 years ago

  • Focuses administration added
  • Keywords has-patch has-unit-tests needs-testing added; needs-patch removed
  • Owner changed from audrasjb to desrosj

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


6 years ago

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


6 years ago

@birgire
6 years ago

#12 @birgire
6 years ago

@desrosj 44234.diff looks good.

44234-2.diff contains few suggestions:

  • Adds test cases for the filters:
    • user_erasure_fulfillment_email_to,
    • user_erasure_complete_email_subject,
    • user_confirmed_action_email_content.

I only used the first input argument of those filter, so it's open for improvements to add the other arguments.

  • Fixes the request type from 'earase_personal_data' to 'remove_personal_data'. It's defined as the latter in the WP_Privacy_Data_Removal_Requests_Table class. But in general I would have preferred the 'earase_personal_data' name of that action type :-)
  • Adds :: for the global function in the @covers annotation for _wp_privacy_send_erasure_fulfillment_notification.
  • Uses "send an email" instead of "send emails" in inline method descriptions, to make it clear that it only sends a single email.
  • The unit tests are a useful documentation, so what about changing the order of methods; show first the methods that explain how it should work, then after that the methods for failing/error? This is applied in the patch.
  • Adds inline comments within methods:
    • test_should_send_email_only_once(),
    • test_should_not_send_email_when_request_not_completed(),
    • test_should_not_send_email_when_not_wp_user_request().
  • Adjusted the description of the test_should_not_send_email_when_not_wp_user_request() method.
  • Changes 'erase-my-data@local.dev' to 'erase-my-data@local.test' as the .dev domain is no longer suitable for local.
  • The $mailer->get_sent()->to is:
    [0] => Array
       (
    		[0] => erase-my-data@local.test
    		[1] =>
    	)
    )
    

so it seems like $mailer->get_sent()->to[0][0] should be used instead of $mailer->get_sent()->to[0].
I used $mailer->get_recipient( 'to' )->address instead, as it is a wrapper for $mailer->get_sent()->to.

  • Moves
    wp_update_post(
    	array(
    		'ID'          => self::$request_id,
    		'post_status' => 'request-completed',
    	)
    );
    

into the wpSetUpBeforeClass() method to reduce code duplications, as I was also starting to use it in the new methods.

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


6 years ago

#14 @desrosj
6 years ago

  • Keywords commit added; needs-testing removed

I only used the first input argument of those filter, so it's open for improvements to add the other arguments.

I think tests for filters should just confirm that a filtered value effects the outcome of the function, so this should be sufficient.

The unit tests are a useful documentation, so what about changing the order of methods; show first the methods that explain how it should work, then after that the methods for failing/error?

I think this is just preference, but fine with your changes. I usually organize my tests in a top down manner, testing the scenarios from the first line of the function to the last.

All your changes look good, though. I think this one is ready.

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


5 years ago

#16 @pento
5 years ago

  • Milestone changed from 4.9.9 to 5.0.1

#17 @pento
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#18 @pento
5 years ago

  • Milestone changed from 5.0.2 to 5.0.3

#19 @audrasjb
5 years ago

  • Keywords needs-refresh added

5.0.3 is going to be released in few days. We are currently sorting the remaining tickets in the milestone. The ticket is sorted in milestone 5.0.3, but requires a @since refresh and a commit/backport to the associated branch.

#20 @desrosj
5 years ago

Let's punt this to 5.1. It's not necessary for 5.0.3.

#21 @desrosj
5 years ago

  • Keywords commit removed

#22 @desrosj
5 years ago

  • Milestone changed from 5.0.3 to 5.1

@desrosj
5 years ago

Refreshed patch with 5.1.0.

#23 @desrosj
5 years ago

  • Keywords needs-refresh removed

#24 @desrosj
5 years ago

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

In 44535:

Privacy: Add unit tests for privacy erasure fulfillment notifications.

Adds appropriate unit tests for _wp_privacy_send_erasure_fulfillment_notification().

Props birgire, desrosj.
Fixes #44234.

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


5 years ago

Note: See TracTickets for help on using tickets.