#44234 closed enhancement (fixed)
Add missing unit tests for erasing personal data by username or email address
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.1 | Priority: | low |
Severity: | normal | Version: | 4.9.6 |
Component: | Privacy | Keywords: | has-patch has-unit-tests |
Focuses: | administration | Cc: |
Attachments (3)
Change History (28)
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
7 years ago
#3
@
7 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
@
7 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.
7 years ago
This ticket was mentioned in Slack in #core by pbiron. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
7 years ago
#9
@
7 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.
7 years ago
This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.
7 years ago
#12
@
7 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 theWP_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.
7 years ago
#14
@
7 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.
7 years ago
#19
@
6 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.
#23
@
6 years ago
- Keywords needs-refresh removed
https://travis-ci.org/desrosj/wordpress-develop/builds/477630506 is the refreshed patch passing on Travis.
Marking as lower priority.