WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 5 weeks ago

#44707 closed defect (bug) (fixed)

The user should be able to create additional requests when previous duplicates are complete or archived

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

Description

Hello,

Currently the wp_create_user_request function has a check for duplicate requests of any post_status which blocks an admin from making a second request with the same email_address.

This is great however this limits admins from making another request after their initial request is completed or archived (future feature) and forces them to remove the original request before being able to make another.

It would be nice to soften this block so additional requests using the email_address can be done when the original request is completed/old as there may be new information added for the email_address since the previous request.

Many admins also need to leave requests rather than remove for logging and auditing needs so forcing the removal before another request can be made conflicts with their legal requirements in some areas.

So in short I'm suggesting updating the $requests_query post_status argument to be an array of states that should actually block the duplicates such as array( 'request-pending', 'request-confirmed' ). I didn't include 'request-failed' or 'request-completed' as these states I feel should support duplicates as they're no longer active requests.

In this way admins can retain a log of their requests but still be able to make additional requests for users if all their previous requests had finished.

Thanks

Attachments (8)

Screen Shot 2018-08-02 at 9.45.18 AM.png (11.7 KB) - added by garrett-eclipse 9 months ago.
Duplicate Request Error
44707.patch (1.5 KB) - added by cc0a 9 months ago.
44707.2.patch (1.5 KB) - added by cc0a 9 months ago.
44707.3.patch (1.5 KB) - added by cc0a 9 months ago.
44707.diff (7.5 KB) - added by desrosj 8 months ago.
44707-2.diff (8.2 KB) - added by birgire 7 months ago.
44707-3.diff (8.2 KB) - added by birgire 3 months ago.
44707.2.diff (9.7 KB) - added by desrosj 5 weeks ago.

Download all attachments as: .zip

Change History (38)

@garrett-eclipse
9 months ago

Duplicate Request Error

@cc0a
9 months ago

#1 @cc0a
9 months ago

Hi @garrett-eclipse, I went ahead and implemented 'request_pending => $email_address', 'request_confirmed => $email_address' in the array as suggested, and I removed the catch-all entry 'title => $email_address' regarding email addresses. This will block only repeat $email_address queries related to request_pending and request_confirmed. Thanks!!

#2 @garrett-eclipse
9 months ago

  • Keywords needs-refresh reporter-feedback needs-unit-tests added

Thanks @cc0a

I took a look at your patch and it's currently invalid as neither request_pending or request_confirmed are WP_Query arguments. You'll want to restore the title argument as it was before and instead update the post_status from 'any' to be an array of the two suggested stati.

Something more like this;

<?php
// Check for duplicates.
$requests_query = new WP_Query( array(
        'post_type'     => 'user_request',
        'post_name__in' => array( $action_name ),  // Action name stored in post_name column.
        'title'         => $email_address, // Email address stored in post_title column.
        'post_status'   => array( 'request_pending', 'request_confirmed' ),
        'fields'        => 'ids',
) );

Hope that clarifies,
Thanks

@cc0a
9 months ago

#3 @cc0a
9 months ago

@garrett-eclipse I understand now! Thanks for helping me with this patch! I went ahead and updated/uploaded it.

#4 @garrett-eclipse
9 months ago

  • Keywords reporter-feedback removed

Thanks @cc0a,

Sorry looks like I got the user request stati incorrect, they should use dashes instead of underscores as seen in their register_post_status calls;
https://github.com/WordPress/WordPress/blob/aab929b8d619bde14495a97cdc1eb7bdf1f1d487/wp-includes/post.php#L318-L352

My bad, updated snippet;

<?php
// Check for duplicates.
$requests_query = new WP_Query( array(
        'post_type'     => 'user_request',
        'post_name__in' => array( $action_name ),  // Action name stored in post_name column.
        'title'         => $email_address, // Email address stored in post_title column.
        'post_status'   => array( 'request-pending', 'request-confirmed' ),
        'fields'        => 'ids',
) );

With that change this is good for some unit tests. I haven't written much unit tests so will defer to @desrosj for help but here's the guide;
https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/

The tests should attempt to duplicate identical requests of the different stati and confirm that request-failed and request-confirmed are allowed duplicate requests while request-pending and request-confirmed block duplicates.

Thanks again

Last edited 9 months ago by garrett-eclipse (previous) (diff)

@cc0a
9 months ago

#5 @cc0a
9 months ago

No worries at all @garrett-eclipse learning is never a bad thing right? :) Revised patch uploaded. Thanks again!!!

#6 @garrett-eclipse
9 months ago

  • Keywords has-patch needs-testing added; needs-refresh removed
  • Milestone changed from Awaiting Review to 4.9.9

Nice, thanks @cc0a that looks good.

I've updated the ticket to 4.9.9 to see if we can get this in the next release.

If you want to look into the Unit Tests go right ahead, or if you're not up to that the ticket will get raised in the next Privacy meeting. Feel free to join us on Slack in #core-privacy

Cheers

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


9 months ago

#8 @cc0a
9 months ago

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

I'll be looking into running some unit tests on my patch this weekend (8/10 - 8/12).

#9 @cc0a
8 months ago

I'm currently working on running PHPUnit tests for this patch...having some trouble configuring PHPStorm. Hopefully I'll have everything resolved in a couple days.

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


8 months ago

#11 @itowhid06
8 months ago

  • Keywords has-patch added

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


8 months ago

#13 @desrosj
8 months ago

Related: #44222.

@desrosj
8 months ago

#14 @desrosj
8 months ago

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

The entire wp_create_user_request() function was missing unit tests. 44707.diff adds thorough tests for the function. @cc0a are you able to test and review the tests?

#15 @desrosj
8 months ago

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

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


8 months ago

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


7 months ago

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


7 months ago

@birgire
7 months ago

#19 @birgire
7 months ago

44707.diff looks good.

Just some minor suggestions in 44707-2.diff:

  • Introduces self::$non_registered_user_email as it's used in few places and to help with the distinction (registered vs non-registered) in the test methods.
  • Changes the "unregistered" in descriptions to "non-registered". I just started wondering about the meaning of the former word, if it could also mean to "de-register"? Please redo this if I'm far out here :-)
  • Renames self::$email to self::$registered_user_email.
  • Fixes the inline documentation for the definition of self::$user_id.
  • Adds inline comments when testing duplicated requests.

#20 @pento
7 months ago

  • Milestone changed from 4.9.9 to Future Release

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


7 months ago

#22 @garrett-eclipse
3 months ago

  • Keywords needs-refresh added; needs-testing removed

Thanks @birgire & @desrosj

This applies nicely, unit tests pass and manual testing for both the erasure and export requests are successful and properly block only when there's an existing request with pending or confirmed status.

I'm only adding a refresh to indicate the versions will need to be updated in the tests to that of the milestone this enters.

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 birgire. View the logs.


3 months ago

#25 @garrett-eclipse
3 months ago

  • Milestone changed from Future Release to 5.2

Hi @birgire after discussing with @desrosj on the weekend lets slot this into 5.2 so you can update versions to 5.2.0

@birgire
3 months ago

#26 @birgire
3 months ago

  • Keywords needs-refresh removed

The 44707-3.diff patch updates @since to 5.2.0 and adds the :: prefix to the function in @covers.

The test runs successfully for the privacy group.

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


8 weeks ago

#28 @garrett-eclipse
7 weeks ago

  • Keywords commit added

Thanks @birgire, this applies cleanly with both manual and unit tests passing. Marking for commit.

@desrosj
5 weeks ago

#29 @desrosj
5 weeks ago

44707.2.diff adds two unit tests to ensure the email and key are properly sanitized and changes the duplicate request error from A request for this email address already exists. to An incomplete request for this email address already exists. to better describe the reason for failure.

#30 @desrosj
5 weeks ago

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

In 44906:

Privacy: Allow new requests to be created after a user’s existing one has been completed.

When dealing with personal data exports and erasure requests, it is important to have a log of all the requests for a specific person. This is often required to confirm when and how many times requests were completed and fulfilled properly.

This change allows a new request to be created after a previous data request has reached completed status (request-completed) instead of requiring admins to delete or re-initiate the existing request. The latter approach removes the historical log of requests for that user when creating a new request.

Full unit tests for the wp_create_user_request() function are also included.

Props garrett-eclipse, cc0a, birgire, desrosj.
Fixes #44707.

Note: See TracTickets for help on using tickets.