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

Spark cli user command #579

Closed
wants to merge 11 commits into from
Closed

Spark cli user command #579

wants to merge 11 commits into from

Conversation

robertogerola
Copy link
Contributor

@robertogerola robertogerola commented Jan 2, 2023

Supersedes #567,#568

With the User command is possible by cli to run these actions on users :
'create', 'activate', 'deactivate', 'changename', 'changeemail', 'delete', 'password', 'list', 'addgroup', 'removegroup'

Available options :
'-i' => 'User id'
'-u' => 'User name'
'-e' => 'User email'
'-n' => 'New username'
'-m' => 'New email'
'-g' => 'Group name'

1. create a new user

./spark shield:user create -u newuser -e [email protected]

2. activate a user

by username : ./spark shield:user activate -u username
or
by email : ./spark shield:user activate -e [email protected]

3. deactivate a user

by username : ./spark shield:user deactivate -u username
or
by email : ./spark shield:user deactivate -e [email protected]

4. change user name

by username : ./spark shield:user changename -u username -n newusername
or
by email : ./spark shield:user changename -e [email protected] -n newusername

5. change user email

by username : ./spark shield:user changeemail -u username -m [email protected]
or
by email : ./spark shield:user changeemail -e [email protected] -m [email protected]

6. delete a user

by user id : ./spark shield:user delete -i 123
or
by username : ./spark shield:user delete -u user
or
by email : ./spark shield:user delete -e [email protected]

7. change a user password

by username : ./spark shield:user password -u username
or
by email : ./spark shield:user password -e [email protected]

8. list users

List all users : ./spark shield:user list
or filter by username and/or email ./spark shield:user list -u user -e [email protected]

9. add a user to a group

by username : ./spark shield:user addgroup -u username -g mygroup
or
by email : ./spark shield:user addgroup -e [email protected] -g mygroup

10. remove a user from a group

by username : ./spark shield:user removegroup -u username -g mygroup
or
by email : ./spark shield:user removegroup -e [email protected] -g mygroup

All the options are optional and if the username or email are not passed on command line, a prompt will ask the user for the necessary information to complete the required task. The data inserted are validated and every operation asks for a confirmation too proceed.

@datamweb
Copy link
Collaborator

datamweb commented Jan 2, 2023

@robertogerola Thanks, can you write the php unit test for this PR?

see :
https://github.com/codeigniter4/CodeIgniter4/tree/develop/tests#resources

Also add descriptions to documents.

@datamweb datamweb added tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. new feature PRs for new features waiting for info Issues or pull requests that need further clarification from the author labels Jan 2, 2023
@kenjis
Copy link
Member

kenjis commented Jan 5, 2023

@codeigniter4/core-team
Is it okay the option like -nu newusername.
Generally an option is made of - and a single letter (e.g., -n), or -- and word(s) (e.g. --namespace).

@paulbalandan
Copy link
Member

Personally, i would choose short options with single letters and -, or long options with word(s) and --. If I'm not mistaken, from posix?, -nu would mean the short option -n followed by the value u, OR a combination of short options -n and -u.

@robertogerola
Copy link
Contributor Author

Hello, just made the changes : -n for new username and -m for new email.
Regarding the request to write the unit tests, I'd need some guidance, please.
The command actions heavily rely on user input and to test these I have to simulate via MockBuilder the user input, right ?
I think it would require some effort to write the tests and due to the user input interaction I think I have to modify the code in several points and make it more complex.
Where can I find some examples how to use MockBuilder to simulate to user cli input ? I found something in the setup unit test command, but it is not very clear to me how to use it, because I have many requests of users input.
Many thanks !

@kenjis
Copy link
Member

kenjis commented Jan 5, 2023

The command actions heavily rely on user input and to test these I have to simulate via MockBuilder the user input, right ?

Is there any command that does not need user input?
CI 4.2 does not provide a way to write tests for methods that require user input, such as CLI::prompt(), CLI::wait(), and CLI::input(). So it is difficult to write such tests.

@MGatner
Copy link
Member

MGatner commented Jan 5, 2023

Thanks for the command argument change, I think that is the correct way to handle it.

@datamweb
Copy link
Collaborator

datamweb commented Jan 6, 2023

CI 4.2 does not provide a way to write tests for methods that require user input, such as CLI::prompt(), CLI::wait(), and CLI::input(). So it is difficult to write such tests.

I remember someone raised this issue, but I couldn't find PR/IS CI4 threads on GitHub.
Do you mean that we can advance this PR without testing?

@kenjis
Copy link
Member

kenjis commented Jan 6, 2023

CI 4.3 provide a way to write tests for methods that require user input.
It seems difficult to write test for this PR without CI 4.3.

Do you mean that we can advance this PR without testing?

It is not good to have no test at all, so if there are commands without user input,
it is not difficult to write tests for them.

We use mocking the test target because there is no other way.

$command = $this->getMockBuilder(Setup::class)
->setConstructorArgs([Services::logger(), Services::commands()])
->onlyMethods(['cliPrompt'])
->getMock();
$command
->method('cliPrompt')
->willReturn('y');

But I don't think it is good practice. Because mock is not a true product code.

@datamweb datamweb removed the waiting for info Issues or pull requests that need further clarification from the author label Mar 31, 2023
@kenjis
Copy link
Member

kenjis commented Apr 1, 2023

CI 4.3 was released, so you can write tests for CLI input.
https://codeigniter4.github.io/CodeIgniter4/testing/overview.html#testing-cli-input

I think we can skip the tests for CI 4.2.

@kenjis
Copy link
Member

kenjis commented Sep 16, 2023

CI 4.3 was released, so you can write tests for CLI input.
https://codeigniter4.github.io/CodeIgniter4/testing/overview.html#testing-cli-input

It was wrong. 4.3 is not enough to write tests for this command.
I rewrote the command to improve testability. See #833

@kenjis
Copy link
Member

kenjis commented Sep 19, 2023

Closed by #833

@kenjis kenjis closed this Sep 19, 2023
@robertogerola robertogerola deleted the user-command branch September 19, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs needed Pull requests needing documentation write-ups and/or revisions. new feature PRs for new features tests needed Pull requests that need tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants