Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn when deleting multisite user with no blog roles #408

Merged
merged 5 commits into from
Jun 9, 2023
Merged

Warn when deleting multisite user with no blog roles #408

merged 5 commits into from
Jun 9, 2023

Conversation

MannyAdumbire
Copy link
Contributor

@MannyAdumbire MannyAdumbire commented Jun 4, 2023 •
edited by danielbachhuber
Loading

Fixes #403

I have no clue what functional tests should look like for this scenario.
I'm new to Behat and my attempt hangs when doing composer behat -- features/user.feature . Any suggestions?
@MannyAdumbire MannyAdumbire requested a review from a team as a code owner June 4, 2023 22:15
@MannyAdumbire MannyAdumbire changed the title Show error when multisite user to delete has no blog roles(#407) Jun 4, 2023
@MannyAdumbire MannyAdumbire changed the title Warn when deleting multisite user with no blog roles(#407) Jun 5, 2023
danielbachhuber
danielbachhuber reviewed Jun 6, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm new to Behat and my attempt hangs when doing composer behat -- features/user.feature . Any suggestions?

To confirm, you've created a database for the test suite to connect to?

What happens when you run composer behat -- features/user.feature --format pretty?

I have no clue what functional tests should look like for this scenario.

It seems like you made a pretty good start! Do you have some specific questions we could help with?
features/user.feature Outdated Show resolved Hide resolved
src/User_Command.php Outdated
$user_id = $user->ID;

if ( $network ) {
$result = wpmu_delete_user( $user_id );
$message = "Deleted user {$user_id}.";
} else if (is_multisite() && empty($user->roles)) {
$message = "No roles found for user {$user_id} on " . get_site_url($blog_id) . ', no users deleted.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use home_url() like below?

Also, there will be some PHPCS warnings on your current code.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My computer is running php PHP 8.2.4 and I'm getting a single error that prevents phpcs from working properly.
I had to do phpcs --standard=WP_CLI_CS User_Command.php -d error_reporting="E_ALL&~E_DEPRECATED to actaully get accurate( I hope ) report from running phpcs.
@MannyAdumbire
Copy link
Contributor Author

I'm new to Behat and my attempt hangs when doing composer behat -- features/user.feature . Any suggestions?

To confirm, you've created a database for the test suite to connect to?

What happens when you run composer behat -- features/user.feature --format pretty?

Yes, the database setup/install-package-test step all went fine.
Maybe issue was getting hung because of missing --yes?
image
image

I've updated the tests with --yes and they now pass.. at least locally :D
@MannyAdumbire
Copy link
Contributor Author

@danielbachhuber I've updated the PR to try and address the issues you pointed out. Thanks for suggestions.
@wojsmol
Copy link
Contributor

wojsmol commented Jun 7, 2023

@MannyAdumbire Yes in Behat tests you must pass --yes because tests are run in non interactive mode.
@danielbachhuber
Copy link
Member

Maybe issue was getting hung because of missing --yes?

@MannyAdumbire Ah, yeah that will cause the tests to hang.
danielbachhuber
danielbachhuber reviewed Jun 7, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor nit, then should be good if the tests pass.
features/user.feature Outdated
And save STDOUT as {BOB_ID}

When I run `wp user delete bobjones --yes`
Then STDOUT should not be empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a Then STDOUT should contain:, and also have a And STDERR should be empty?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!
I was going based off current test
https://github.com/wp-cli/entity-command/blob/1f013e1b097a6af7a1ae85ea797f076486a7c749/features/user.feature#L70-71
but makes sense. hope this works. ;)
MannyAdumbire
MannyAdumbire commented Jun 8, 2023
View reviewed changes
features/user.feature Outdated
When I run `wp user delete bobjones --yes`
Then STDOUT should be:
"""
Success: Removed user {BOB_ID} from https://example.com.
Copy link
Contributor Author

@MannyAdumbire MannyAdumbire Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielbachhuber Is this Then more specific than necessary? I'm now noticing example that I could have copied
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entity-command/features/user.feature

Line 345 in 1f013e1

When I run `wp user delete testsubscriber --yes`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this Then more specific than necessary? I'm now noticing example that I could have copied

@MannyAdumbire I don't have strong feelings one way or the other. A little less specific would be fine.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! I made it less specific then for consistency with the other step.
@MannyAdumbire
Copy link
Contributor Author

MannyAdumbire commented Jun 8, 2023 •
edited
Loading

I'm all set as far as changes.
Behat tests passing locally.
Seeing one phpcs error, but I don't think it relates to this PR.

-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------
 36 | ERROR | Classes declared by a theme/plugin should start with the theme/plugin prefix. Found: "User_Command".
    |       | (WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedClassFound)
-------------------------------------------------------------------------------------------------------------------------
@danielbachhuber danielbachhuber changed the title Warn when deleting multisite user with no blog roles(#408) Jun 9, 2023
@danielbachhuber danielbachhuber added this to the 2.5.2 milestone Jun 9, 2023
@danielbachhuber danielbachhuber self-requested a review June 9, 2023 12:25
danielbachhuber
danielbachhuber approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this, @MannyAdumbire — thanks again!
@danielbachhuber danielbachhuber merged commit 5c14109 into wp-cli:main Jun 9, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:user command:user-delete
3 participants
@MannyAdumbire @wojsmol @danielbachhuber