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.

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

Add wp-cli command to delete guest authors, with option to reassign their posts. #588

Open
wants to merge 314 commits into
base: develop
Choose a base branch
from

Conversation

smistephen
Copy link
Contributor

Currently, there is no way to delete guest authors from the command line. This command fills that need.

It is activated using delete-guest-authors, and passing in the path to a CSV file containing a list of two columns: the ID to delete, and the login to assign the posts to (or "false" to not reassign).

The command runs dry by default, showing the end user the guest authors it would have attempted to delete, and the user login it would have attempted to transfer the posts to.

Passing --no-dry-run, and confirming that you really do want to delete guest authors, actively modifies the database.

Rebecca Hum and others added 30 commits May 29, 2017 19:48
Script was stopping after no co-authors was find. Now script will
display an error message and continue to the next post.
Script was stopping after no co-authors was find. Now script will
display an error message and continue to the next post.
Resolves Automattic#417

* Removed duplicate left join for optimization
…r-posts

Fixed WP CLI create-terms-for-posts if no co-authors found
…uthor

Replace hardcoded 'author' with $this->$coauthor_taxonomy
Fixes warning: `Warning: sprintf(): Too few arguments in /path/wp-content/plugins/co-authors-plus/php/class-coauthors-guest-authors.php on line 487`

Warning surfaces when deleting a guest author that is mapped to a WP user
Move parenthesis to fix esc_html and sprintf
@smistephen
Copy link
Contributor Author

Perfect

@smistephen
Copy link
Contributor Author

Looking at #550, it seems like it could be its own independent PR. Am I reading it right?

Copy link
Contributor

@sboisvert sboisvert left a comment

Choose a reason for hiding this comment

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

A few questions :)

php/class-wp-cli.php Show resolved Hide resolved
php/class-wp-cli.php Outdated Show resolved Hide resolved
php/class-wp-cli.php Show resolved Hide resolved
php/class-wp-cli.php Outdated Show resolved Hide resolved
This commit adds additional information on success and failure, telling the end user not only the ID of the guest author that was deleted, but also (if applicable) the login their posts were assigned to.
@sboisvert
Copy link
Contributor

sboisvert commented Aug 14, 2018 via email

This commit utilizes the CAP API during dry runs to pull out information about the guest authors about to be deleted, to make sure they're the right ones.
This commit utilizes the CAP API to pull out information about the guest authors about to be deleted, to make sure they're the right ones.
Copy link
Contributor

@philipjohn philipjohn left a comment

Choose a reason for hiding this comment

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

This is looking good - I've left some comment.

One overall question - how can we check this code is going to work as expected in production?

php/class-wp-cli.php Outdated Show resolved Hide resolved
php/class-wp-cli.php Outdated Show resolved Hide resolved
php/class-wp-cli.php Outdated Show resolved Hide resolved
@smistephen
Copy link
Contributor Author

smistephen commented Aug 15, 2018

One overall question - how can we check this code is going to work as expected in production?

Checks I've performed so far:

  • run phpunit (passes)
  • run phpcs (passes)
  • look for performance problems and security vulnerabilities (nothing obvious)
  • test function return values and variable existence (added appropriate error handling code)
  • test on a sample WordPress instance (seems to work as promised)
  • try to break it with faulty data (seems to fail gracefully as predicted)

And I'm always open to more opportunities 👍

Allow for remapping to user named "false".

The CAP delete function takes, as its second parameter, either a string containing the login of the user you wish to reassign the deleted guest author's posts to, or a boolean false stating that you do wish to reassign.

But what if you want to reassign to a user named false? Before that was not possible.

Now, the code specifically looks for if the instance has a user named false, and if so, allows it to be reassigned to. If not, it assumes the boolean was intended.
@smistephen
Copy link
Contributor Author

Hooray for @philipjohn pulling the gremlin out of Travis, builds pass now, as they should 👍

@philipjohn
Copy link
Contributor

run phpunit (passes)

Which unit tests did you look at when trying to assess whether this code works as intended?

try to break it with faulty data (seems to fail gracefully as predicted)

That's a good idea! What kind of faulty data did you use here?

@smistephen
Copy link
Contributor Author

Which unit tests did you look at when trying to assess whether this code works as intended?

I examined the test_delete_* tests (https://github.com/Automattic/Co-Authors-Plus/blob/master/tests/test-coauthors-guest-authors.php#L722), as well as running the entire unit test suite locally before creating the PR.

That's a good idea! What kind of faulty data did you use here?

I used:

  • non-existent guest authors
  • malformed guest author IDs (various strings containing characters and punctuation)
  • non-existent assignees
  • malformed assignees (same as the IDs)

As well as:

  • filepaths that should work (testFile.csv, ~/testFile.csv, /home/steve/testFile.csv)
  • filepaths that shouldn't work (../../../../mwahahahaha.csv) work
  • files that don't exist
  • files that do exist but can't be read (I set the file's permissions to 111 to test this).

php/class-wp-cli.php Outdated Show resolved Hide resolved
@chrissy-dev
Copy link

@rebeccahum I appreciate this is an old PR but is there a reason this PR never got merged? Happy to help get this over the line if possible.

@rebeccahum
Copy link
Contributor

@chrissy-dev I think this just slipped the radar. First step would be to resolve any merge conflicts and test if the current PR still works please!

@GaryJones
Copy link
Contributor

Overlaps with #696.

@GaryJones GaryJones added this to the 3.6 milestone Jul 27, 2023
@GaryJones GaryJones changed the base branch from main to develop August 25, 2023 15:23
@alecgeatches alecgeatches modified the milestones: 3.6, 3.7 Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.