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

[garrett-eclipse]

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

[garrett-eclipse]

Duplicate Request Error

[cc0a]

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!!

[garrett-eclipse]

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]

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

[garrett-eclipse]

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

[cc0a]

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

[garrett-eclipse]

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

[cc0a]

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

[cc0a]

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.

[desrosj]

Related: #44222.

[desrosj]

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?

[birgire]

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.

[garrett-eclipse]

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.

[garrett-eclipse]

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]

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.

[garrett-eclipse]

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

[desrosj]

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.

[desrosj]

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.

[garrett-eclipse]

Dropping privacy focus as it's already in the Privacy component.