Skip to content

Add commands for managing signups on multisite#489

Merged
swissspidy merged 17 commits intowp-cli:mainfrom
ernilambar:feat-signup
May 4, 2024
Merged

Add commands for managing signups on multisite#489
swissspidy merged 17 commits intowp-cli:mainfrom
ernilambar:feat-signup

Conversation

@ernilambar
Copy link
Member

@ernilambar ernilambar commented Mar 15, 2024 •
edited
Loading

Adds signup command and subcommands

  • signup list
  • signup activate
  • signup get
  • signup delete

Command details: https://github.com/ernilambar/entity-command/tree/feat-signup#wp-signup

Related: wp-cli/wp-cli#1911

PR inspired from https://github.com/trepmal/wp-cli-unconfirmed-users

Highlights:

  • Separate Signup Fetcher is used here
  • Better signup validation using $this->fetcher->get_check()
  • Command class extends CommandWithDBObject which provides several helpful methods

@ernilambar ernilambar marked this pull request as ready for review March 15, 2024 10:44
@ernilambar ernilambar requested a review from a team as a code owner March 15, 2024 10:44
@ernilambar ernilambar mentioned this pull request Mar 15, 2024
Closed
tyrann0us
tyrann0us suggested changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@tyrann0us tyrann0us left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you very much for working on this! I left some nit-picky comments that try aligning wording as close to existing commands as possible.
If you agree with my comments in README.md, please also apply them to src/Signup_Command.php. Thank you!
Also, I would have expected that there would be WordPress APIs (PHP functions) for fetching and deleting signups, but I couldn't find anything. 🤷🏽

@ernilambar
Copy link
Member Author

ernilambar commented Mar 19, 2024

Note:

tyrann0us
tyrann0us reviewed Mar 19, 2024
View reviewed changes
Copy link
Contributor

@tyrann0us tyrann0us left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for incorporating all the changes. 🙏🏽
Would it be possible to add tests for failed activating and deleting signups to ensure the presence of the "Failed (activating|deleting) signup 123" message?

Fetcher should be probably moved to https://github.com/wp-cli/wp-cli/tree/main/php/WP_CLI/Fetchers

Does it make sense to create a separate PR there, have it merged, and then continue here?

tyrann0us
tyrann0us approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@tyrann0us tyrann0us left a comment

Choose a reason for hiding this comment

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

I left another round of nit-picky wording-related comments. However, these don't stop me from approving.
Thank you for working on this! It's a small feature, but we have desperately wanted it at times. 💚

@tyrann0us
Copy link
Contributor

tyrann0us commented Mar 26, 2024

Hi @danielbachhuber, to continue our conversation from wp-cli/wp-cli#1911, would you consider this PR ready? Thanks!

danielbachhuber
danielbachhuber requested changes Apr 7, 2024
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 start! I left some comments.

@ernilambar ernilambar mentioned this pull request Apr 9, 2024
Merged
@swissspidy
Copy link
Member

swissspidy commented Apr 26, 2024

@ernilambar I just merged wp-cli/wp-cli#5926 now

To make the PR work now, update composer.json here to require "wp-cli/wp-cli": "^2.11"

@swissspidy
Copy link
Member

swissspidy commented Apr 26, 2024

I don't understand the test failures, they look unrelated 🤔

@ernilambar
Copy link
Member Author

ernilambar commented Apr 26, 2024

I don't understand the test failures, they look unrelated 🤔

Yah, there are no changes related to taxonomies here.

@swissspidy
Copy link
Member

swissspidy commented Apr 26, 2024 •
edited
Loading

Ah, when I try locally I get this:

$ WP_VERSION=trunk composer behat features/taxonomy.feature:7
> run-behat-tests 'features/taxonomy.feature:7'
-----

--- Failed hooks:

    BeforeSuite # WP_CLI\Tests\Context\FeatureContext::prepare()
      $ wp cli info
      
      Error: Callable "Signup_Command" does not exist, and cannot be registered as `wp user signup`.
      cwd: 
      run time: 0.16830706596375
      exit status: 1 (RuntimeException)

Edit: just needed to run composer update

@swissspidy
Copy link
Member

swissspidy commented Apr 26, 2024

As just discussed on the Zoom call, it's because of wp-cli/wp-cli#5928

We'll fix this separately.

@swissspidy
Copy link
Member

swissspidy commented Apr 26, 2024

wp-cli/wp-cli#5928 has been reverted in the meantime

@johnbillion
Copy link
Contributor

johnbillion commented Apr 26, 2024

See also wp-cli/ideas#99

@swissspidy swissspidy self-requested a review April 26, 2024 12:10
swissspidy
swissspidy reviewed Apr 26, 2024
View reviewed changes
swissspidy
swissspidy reviewed Apr 26, 2024
View reviewed changes
johnbillion
johnbillion reviewed Apr 26, 2024
View reviewed changes
danielbachhuber
danielbachhuber previously requested changes Apr 26, 2024
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.

@ernilambar Can you:

  1. Credit the original package in the PR description?
  2. Highlight any differences between this implementation and the original package?

danielbachhuber
danielbachhuber reviewed Apr 27, 2024
View reviewed changes
@danielbachhuber danielbachhuber changed the title Add signup command class Add commands for managing signups on multisite Apr 27, 2024
@swissspidy swissspidy dismissed danielbachhuber’s stale review May 2, 2024 13:32

Feedback has been addressed

@swissspidy swissspidy added command:user-signup Related to 'user signup' command command:user-signup-activate Related to 'user signup activate' command command:user-signup-delete Related to 'user signup delete' command command:user-signup-get Related to 'user signup get' command command:user-signup-list Related to 'user signup list' command labels May 2, 2024
@swissspidy swissspidy merged commit 3648d0e into wp-cli:main May 4, 2024
@swissspidy swissspidy added this to the 2.8.0 milestone May 4, 2024
@ernilambar ernilambar mentioned this pull request Jun 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy swissspidy approved these changes

@danielbachhuber danielbachhuber Awaiting requested review from danielbachhuber

+2 more reviewers

@johnbillion johnbillion johnbillion left review comments

@tyrann0us tyrann0us tyrann0us approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

command:user-signup Related to 'user signup' command command:user-signup-activate Related to 'user signup activate' command command:user-signup-delete Related to 'user signup delete' command command:user-signup-get Related to 'user signup get' command command:user-signup-list Related to 'user signup list' command

Projects

None yet

Milestone

2.8.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ernilambar @tyrann0us @swissspidy @johnbillion @danielbachhuber