[MM-19066] Added `user list` command #34

njkevlani · 2019-11-01T03:27:41Z

Fixes mattermost/mattermost#12965

mgdelacroix

Thanks @njkevlani!! It looks very good, a few changes noted.

Would you be open to add some unit tests for these PR? You can take inspiration on the search channel command test

commands/user.go

Co-Authored-By: Miguel de la Cruz <mgdelacroix@gmail.com>

njkevlani · 2019-11-10T03:31:34Z

Would you be open to add some unit tests for these PR? You can take inspiration on the search channel command test

Sure, will do that.

To report an error if there are any positional args

…19066

njkevlani · 2019-11-17T12:06:15Z

@mgdelacroix

Tests added. Let me know if this seems fine :)

mgdelacroix

Thanks for the addition of the tests @njkevlani!!

commands/user_test.go

Co-Authored-By: Miguel de la Cruz <mgdelacroix@gmail.com>

* Replace printer.Flush() with printer.Clean() * Added printer.Flush() in first test case as well * Added one more mock call when --all flag is passed

commands/user_test.go

mattermod · 2019-11-29T00:01:51Z

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

hanzei · 2019-11-29T01:46:07Z

@njkevlani Would you please merge master into the branch, when you have the time?

…19066

njkevlani · 2019-11-29T02:57:43Z

@hanzei master branch is merged into this branch.

mgdelacroix

Thanks for the changes @njkevlani, LGTM 👍

njkevlani · 2019-12-05T10:38:01Z

ping @mkraft

mkraft · 2019-12-10T11:25:59Z

commands/user.go

+	showAll, _ := command.Flags().GetBool("all")
+
+	tpl := `{{.Id}}: {{.Username}} ({{.Email}})`
+	for users, _ := c.GetUsers(page, perPage, ""); users != nil && len(users) > 0; users, _ = c.GetUsers(page, perPage, "") {


What's the reason we're using the blank identifier for these GetUsers errors?

There is no particular reason behind that. I guess we can handle that response like following:

if response.Error != nil { printer.PrintError("Unable list all users.\n " + response.Error.Error()) }

Let me know if this seems fine or it can be improved :)

@njkevlani would you be open to help with @mkraft's question above? Thanks!

Hey @jasonblais,

Actually I have suggested what should be changed in my last comment, but haven't got any confirmation on that. Can you please check that out?

I also have the same question. We should check the error and bubble that above.

And this logic still uses the page number if showAll is set. What we should probably do is:

if showAll {
page = 0
}
And then loop through c.GetUsers until it runs out of users.

Will commit & push soon.

Okay, I'll check for error and bubble that up.

if response.Error != nil { return errors.Wrap(response.Error, "Failed to fetch users") }

- Sort import - Typo fix - Utilized `EqualError` rather than using `Equal` - Added one more test

https://github.com/mattermost/mmctl/pull/34/files#r369056415

agnivade · 2020-01-22T07:18:42Z

We are nearly there. Just one outstanding comment to clarify. And please fix the merge conflicts. Thanks !

njkevlani · 2020-01-22T11:54:35Z

We are nearly there. Just one outstanding comment to clarify.

Does this reply answer the question in linked comment?

agnivade · 2020-01-22T15:45:22Z

That doesn't link to a comment for me. :( Feel free to just paste the comment.

agnivade

Sorry, perhaps there is some miscommunication amongst us. I meant why do we need to call the GetUsers method twice. Does something like this work ?

for {
	users, res := c.GetUsers(page, perPage, "")
	if res.Error != nil {
		return errors.Wrap(...)
	}
	if len(users) == 0 {
		break
	}
	for _, user := range users {
		printer.PrintT(tpl, user)
	}

	if !showAll {
		break
	}
	page++	
}

check for error in server response outside of loop

njkevlani · 2020-01-22T18:22:05Z

Sorry, perhaps there is some miscommunication amongst us. I meant why do we need to call the GetUsers method twice. Does something like this work ?

for {
	users, res := c.GetUsers(page, perPage, "")
	if res.Error != nil {
		return errors.Wrap(...)
	}
	if len(users) == 0 {
		break
	}
	for _, user := range users {
		printer.PrintT(tpl, user)
	}

	if !showAll {
		break
	}
	page++	
}

Yes, this would work fine. I guess the difference in this approach any my previous code is that,

In this approach: for { ... } loop is used. Initialization / assignment occurs inside loop body
In my previous approach: for init; condition; post { ... } initialization / assignment were part of loop,

Anyway, I have made changes in code. Let me know if all look good.

agnivade

Thanks for your patience on this @njkevlani !

njkevlani · 2020-01-23T05:03:35Z

Thanks for feedback on this, got to learn some things :) @agnivade

…19066 � Conflicts: � client/client.go � commands/user_test.go � mocks/client_mock.go

njkevlani · 2020-01-27T05:19:27Z

@mgdelacroix any updates?

mgdelacroix · 2020-01-27T05:50:22Z

@njkevlani thanks a lot for all the changes you've implemented! Can you please run make docs and update the PR to fix the CI?

njkevlani · 2020-01-27T05:56:14Z

@njkevlani thanks a lot for all the changes you've implemented! Can you please run make docs and update the PR to fix the CI?

Done :)

mkraft

LGTM

mattermod · 2020-02-06T15:12:02Z

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

mattermod · 2020-02-06T15:12:03Z

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

mattermod · 2020-02-06T15:35:06Z

Trying to auto merge this PR.

mattermod · 2020-02-06T15:35:10Z

Pull Request successfully merged
SHA: daf8cb8

njkevlani added 2 commits November 1, 2019 08:37

Added getUsers function to client

e58545a

Added user list command

2e74125

hanzei requested a review from mgdelacroix November 1, 2019 06:20

hanzei assigned mgdelacroix Nov 1, 2019

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 1, 2019

mgdelacroix requested changes Nov 9, 2019

View reviewed changes

commands/user.go Show resolved Hide resolved

commands/user.go Outdated Show resolved Hide resolved

mgdelacroix requested a review from mkraft November 9, 2019 21:12

Logic

44e5690

Co-Authored-By: Miguel de la Cruz <mgdelacroix@gmail.com>

njkevlani added 4 commits November 11, 2019 20:23

Added Args: cobra.NoArgs

87ca704

To report an error if there are any positional args

Merge branch 'master' of https://github.com/mattermost/mmctl into MM-…

12bdab4

…19066

Fixed Mock Client for GetUsers

f57e198

Added tests for listUserCmdF

be22d8b

mgdelacroix requested changes Nov 17, 2019

View reviewed changes

commands/user_test.go Outdated Show resolved Hide resolved

commands/user_test.go Outdated Show resolved Hide resolved

njkevlani and others added 2 commits November 18, 2019 00:56

Fixed typo

3b5c9ce

Co-Authored-By: Miguel de la Cruz <mgdelacroix@gmail.com>

Corrections in test

3d60f93

* Replace printer.Flush() with printer.Clean() * Added printer.Flush() in first test case as well * Added one more mock call when --all flag is passed

mgdelacroix requested changes Nov 17, 2019

View reviewed changes

commands/user_test.go Outdated Show resolved Hide resolved

commands/user_test.go Outdated Show resolved Hide resolved

commands/user_test.go Show resolved Hide resolved

Move printer.Clear() in the beginning of each case

a1585ad

mattermod added the Lifecycle/1:stale label Nov 29, 2019

hanzei requested a review from mgdelacroix November 29, 2019 01:45

Merge branch 'master' of https://github.com/mattermost/mmctl into MM-…

9b3b217

…19066

mgdelacroix approved these changes Nov 29, 2019

View reviewed changes

hanzei removed the Lifecycle/1:stale label Nov 29, 2019

mkraft reviewed Dec 10, 2019

View reviewed changes

njkevlani added 3 commits January 11, 2020 02:21

Improvements in test

d444ed3

- Sort import - Typo fix - Utilized `EqualError` rather than using `Equal` - Added one more test

Utilize same variable for checking err in flag

da8f2fb

Change text for page arg in user list cmd

4fb676c

https://github.com/mattermost/mmctl/pull/34/files#r369056415

njkevlani requested a review from agnivade January 22, 2020 17:37

agnivade requested changes Jan 22, 2020

View reviewed changes

Changes in server call

42d4f23

check for error in server response outside of loop

agnivade approved these changes Jan 23, 2020

View reviewed changes

Merge branch 'master' of https://github.com/mattermost/mmctl into MM-…

079c221

…19066 � Conflicts: � client/client.go � commands/user_test.go � mocks/client_mock.go

mgdelacroix requested a review from mkraft January 27, 2020 05:45

Added docs for user list

8c15257

mkraft approved these changes Feb 6, 2020

View reviewed changes

mgdelacroix added 2 commits February 6, 2020 15:59

Merge remote-tracking branch 'upstream/master' into MM-19066

c01efca

Fix linter

8bc03b2

mgdelacroix added 4: Reviews Complete All reviewers have approved the pull request AutoMerge used by Mattermod to merge PR automatically and removed 2: Dev Review Requires review by a core committer labels Feb 6, 2020

mattermod merged commit daf8cb8 into mattermost:master Feb 6, 2020

mattermod removed the AutoMerge used by Mattermod to merge PR automatically label Feb 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-19066] Added `user list` command #34

[MM-19066] Added `user list` command #34

njkevlani commented Nov 1, 2019

mgdelacroix left a comment

njkevlani commented Nov 10, 2019

njkevlani commented Nov 17, 2019

mgdelacroix left a comment

mattermod commented Nov 29, 2019

hanzei commented Nov 29, 2019

njkevlani commented Nov 29, 2019

mgdelacroix left a comment

njkevlani commented Dec 5, 2019

mkraft Dec 10, 2019

njkevlani Dec 11, 2019

jasonblais Jan 7, 2020

njkevlani Jan 7, 2020

agnivade Jan 7, 2020

njkevlani Jan 8, 2020

njkevlani Jan 8, 2020

agnivade commented Jan 22, 2020

njkevlani commented Jan 22, 2020

agnivade commented Jan 22, 2020

agnivade left a comment

njkevlani commented Jan 22, 2020

agnivade left a comment

njkevlani commented Jan 23, 2020

njkevlani commented Jan 27, 2020

mgdelacroix commented Jan 27, 2020 •

edited

njkevlani commented Jan 27, 2020

mkraft left a comment

mattermod commented Feb 6, 2020

mattermod commented Feb 6, 2020

mattermod commented Feb 6, 2020

mattermod commented Feb 6, 2020

[MM-19066] Added user list command #34

[MM-19066] Added user list command #34

Conversation

njkevlani commented Nov 1, 2019

mgdelacroix left a comment

Choose a reason for hiding this comment

njkevlani commented Nov 10, 2019

njkevlani commented Nov 17, 2019

mgdelacroix left a comment

Choose a reason for hiding this comment

mattermod commented Nov 29, 2019

hanzei commented Nov 29, 2019

njkevlani commented Nov 29, 2019

mgdelacroix left a comment

Choose a reason for hiding this comment

njkevlani commented Dec 5, 2019

mkraft Dec 10, 2019

Choose a reason for hiding this comment

njkevlani Dec 11, 2019

Choose a reason for hiding this comment

jasonblais Jan 7, 2020

Choose a reason for hiding this comment

njkevlani Jan 7, 2020

Choose a reason for hiding this comment

agnivade Jan 7, 2020

Choose a reason for hiding this comment

njkevlani Jan 8, 2020

Choose a reason for hiding this comment

njkevlani Jan 8, 2020

Choose a reason for hiding this comment

agnivade commented Jan 22, 2020

njkevlani commented Jan 22, 2020

agnivade commented Jan 22, 2020

agnivade left a comment

Choose a reason for hiding this comment

njkevlani commented Jan 22, 2020

agnivade left a comment

Choose a reason for hiding this comment

njkevlani commented Jan 23, 2020

njkevlani commented Jan 27, 2020

mgdelacroix commented Jan 27, 2020 • edited

njkevlani commented Jan 27, 2020

mkraft left a comment

Choose a reason for hiding this comment

mattermod commented Feb 6, 2020

mattermod commented Feb 6, 2020

mattermod commented Feb 6, 2020

mattermod commented Feb 6, 2020

[MM-19066] Added `user list` command #34

[MM-19066] Added `user list` command #34

mgdelacroix commented Jan 27, 2020 •

edited