[GH-13278] Port the "user activate" subcommand from Mattermost CLI to mmctl #67
Conversation
hanzei
added
2: Dev Review
lindy65
added
QA Review Done
and removed
3: QA Review
commands/user.go
Outdated
| if len(args) < 1 { | ||
| return errors.New("Expected at least one argument. See help text for details.") | ||
| } |
There was a problem hiding this comment.
This check can be replaced using Args: cobra.MinimumNArgs(1), in the command struct
commands/user.go
Outdated
| if user == nil { | ||
| return fmt.Errorf("Can't find user '%v'", userArg) | ||
| } |
There was a problem hiding this comment.
I'd prefer this check to be made right after getting the user, and for the changeUserActiveStatus function to trust the arguments so it's simpler and the errors are handled right where they occur. 3/5
There was a problem hiding this comment.
If this is the only workflow that is expected (changeUsersActiveStatus->changeUserActiveStatus) this work. If at any moment we decide to expose or use in other place the "changeUserActiveStatus" function, it may result in a panic. That is why I generally prefer to check things just before using them.
Nevertheless, if we want to enforce that changeUserActiveStatus to trust the arguments and expect a panic if someone or something is using it wrong, we can make the change.
There was a problem hiding this comment.
For me is the other way around, if we retrieve something but don't check that we got a value, that could cause a panic. Now we are doing changeUsersActiveStatus -> changeUserActiveStatus, but if we decide to do something else with those users before (or even after) going through the function that checks the nils, we would be operating with usafe values.
Checking something as part of getting it is safe, as it doesn't matter where you use it, you know that it has already been checked, hence the rest of the functions can trust that value (or check it for double safety if the functions are exposed as part of an API).
commands/user_test.go
Outdated
| s.Require().Equal(fmt.Errorf("Unable to change activation status of user: %v", emailArg).Error(), printer.GetErrorLines()[0]) | ||
| }) | ||
|
|
||
| s.Run("Activate several users with unexistent ones and failure ones", func() { |
There was a problem hiding this comment.
| s.Run("Activate several users with unexistent ones and failure ones", func() { | |
| s.Run("Activate several users with unexistent ones and failed ones", func() { |
commands/user.go
Outdated
| return fmt.Errorf("Unable to change activation status of user: %v", userArg) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func userDeactivateCmdF(c client.Client, cmd *cobra.Command, args []string) error { | ||
| if len(args) < 1 { |
There was a problem hiding this comment.
It doesn't belong specifically to this ticket, but it would be great if you could apply the Args: cobra.MinimumNArgs(1) to this command as well
| if _, response := c.DeleteUser(user.Id); response.Error != nil { | ||
| printer.PrintError("Unable to deactivate user " + userArgs[i] + ". Error: " + response.Error.Error()) | ||
| } | ||
| func changeUserActiveStatus(c client.Client, user *model.User, userArg string, activate bool) error { |
There was a problem hiding this comment.
I don't think we are using c.DeleteUser anywhere else. If so, would you mind removing it from the Client and updating the mocks?
…d remove deleteuser
|
This issue has been automatically labelled "stale" because it hasn't had recent activity. /cc @jasonblais @hanzei |
There was a problem hiding this comment.
Sorry for the late review! LGTM, thanks a lot for the changes @larkox!!! 🎊
mgdelacroix
added
4: Reviews Complete
Summary:
Port the "user activate" subcommand from Mattermost CLI to mmctl
Link:
Fixes mattermost/mattermost#13278