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

feat: add CLI command to manage users #833

Merged
merged 66 commits into from
Sep 19, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 14, 2023

Description
Supersedes #579
Closes #566

Add command shield:user to manage users:

            shield:user create -n newusername -e [email protected]
            shield:user activate -n username
            shield:user activate -e [email protected]
            shield:user deactivate -n username
            shield:user deactivate -e [email protected]
            shield:user changename -n username --new-name newusername
            shield:user changename -e [email protected] --new-name newusername
            shield:user changeemail -n username --new-email [email protected]
            shield:user changeemail -e [email protected] --new-email [email protected]
            shield:user delete -i 123
            shield:user delete -n username
            shield:user delete -e [email protected]
            shield:user password -n username
            shield:user password -e [email protected]
            shield:user list
            shield:user list -n username -e [email protected]
            shield:user addgroup -n username -g mygroup
            shield:user addgroup -e [email protected] -g mygroup
            shield:user removegroup -n username -g mygroup
            shield:user removegroup -e [email protected] -g mygroup

TODO

  • remove exit()
  • fix if conditions
  • fix hard-coded table names
  • fix hard-coded validation rules

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis marked this pull request as draft September 14, 2023 06:30
@kenjis kenjis added the new feature PRs for new features label Sep 14, 2023
@kenjis kenjis force-pushed the feat-user-command branch 3 times, most recently from 0d08c19 to 8ead772 Compare September 16, 2023 04:58
@kenjis kenjis mentioned this pull request Sep 16, 2023
@kenjis kenjis force-pushed the feat-user-command branch 2 times, most recently from 6bb4aba to da466af Compare September 16, 2023 07:09
@kenjis kenjis marked this pull request as ready for review September 16, 2023 07:10
src/Commands/User.php Outdated Show resolved Hide resolved
src/Commands/User.php Outdated Show resolved Hide resolved
$action = $params[0] ?? null;

if ($action === null || ! in_array($action, $this->validActions, true)) {
$this->write(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If not valid, display a form to select valid items.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is too big. If you want to add more features, you should send another PR.

I am more concerned about the current features working properly than usability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am more concerned about the current features working properly than usability.

Ok I get it, I haven't exactly checked out this PR yet. I will try to continue with this in mind.

$this->removegroup($group, $username, $email);
break;
}
} catch (RuntimeException $e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Throw exception! If not valid, display a form to select valid items.


class User extends BaseCommand
{
private static ?InputOutput $io = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

$io I think it is short, and it is not understandable.
https://phpmd.org/rules/naming.html#shortvariable

if ($password !== $passwordConfirm) {
$this->write("The passwords don't match", 'red');

throw new RuntimeException("The passwords don't match");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not good. Do not remove the user from the process. Display new form to re-enter data.

src/Commands/User.php Outdated Show resolved Hide resolved
if ($user === null) {
$this->write("User doesn't exist", 'red');

throw new RuntimeException("User doesn't exist");
Copy link
Collaborator

Choose a reason for hiding this comment

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

In all cases, I disagree with throw.

Copy link
Member Author

Choose a reason for hiding this comment

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

The exception will be caught in run() method.
It is the replacement of exit().

src/Controllers/RegisterController.php Outdated Show resolved Hide resolved
@kenjis kenjis marked this pull request as draft September 16, 2023 09:05
@kenjis kenjis marked this pull request as ready for review September 16, 2023 09:28
@kenjis
Copy link
Member Author

kenjis commented Sep 16, 2023

@robertogerola @cijagani Try this, if you can.

@kenjis kenjis changed the title feat: spark shield:user command feat: add CLI command to manage users Sep 16, 2023
Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to review, and I probably won't be able to for a few days.
It seems that people are waiting for this PR. You can merge.

@kenjis kenjis merged commit fc0926b into codeigniter4:develop Sep 19, 2023
29 checks passed
@kenjis kenjis deleted the feat-user-command branch September 19, 2023 20:09
@kenjis
Copy link
Member Author

kenjis commented Sep 19, 2023

Okay. The test code has been improved, so there should be no major bugs.
Testing this feature is welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature PRs for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI command to manage users
4 participants