WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 4 months ago

#44707 assigned defect (bug)

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

Reported by: garrett-eclipse Owned by: desrosj
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests needs-testing
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 (6)

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

Download all attachments as: .zip

Change History (27)

@garrett-eclipse
6 months ago

Duplicate Request Error

@cc0a
6 months ago

#1 @cc0a
6 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
6 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
6 months ago

#3 @cc0a
6 months ago

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

#4 @garrett-eclipse
6 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 6 months ago by garrett-eclipse (previous) (diff)

@cc0a
6 months ago

#5 @cc0a
6 months ago

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

#6 @garrett-eclipse
6 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.


6 months ago

#8 @cc0a
6 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
5 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.


5 months ago

#11 @itowhid06
5 months ago

  • Keywords has-patch added

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


5 months ago

#13 @desrosj
5 months ago

Related: #44222.

@desrosj
5 months ago

#14 @desrosj
5 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
5 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.


5 months ago

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


4 months ago

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


4 months ago

@birgire
4 months ago

#19 @birgire
4 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
4 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.


4 months ago

Note: See TracTickets for help on using tickets.