-
Notifications
You must be signed in to change notification settings - Fork 91
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.
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
Warn when deleting multisite user with no blog roles #408
Conversation
There was a problem hiding this 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?
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.'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
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.
Yes, the database setup/install-package-test step all went fine. I've updated the tests with |
@danielbachhuber I've updated the PR to try and address the issues you pointed out. Thanks for suggestions. |
@MannyAdumbire Yes in Behat tests you must pass |
@MannyAdumbire Ah, yeah that will cause the tests to hang. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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. ;)
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I'm all set as far as changes.
|
There was a problem hiding this 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!
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?