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

BREAKING CHANGE(cli): add user management commands, remove old ones #106

Merged
merged 6 commits into from
Nov 21, 2019

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Nov 19, 2019

Ok! This was a long one.

Refactors the zapier invite command into:

  • users:add
  • users:remove
  • users:get
  • users:links

Refactors the zapier collaborate command into:

  • team:add
  • team:remove
  • team:get

As is, the code is fairly repetitive. We're under a little bit of a deadline to get this out, so rather than spend too much time here drying it up, I wanted to get it out the door. In particular, we can probably clean up reused code between the :add commands and how we split api calls between subscribers and collaborators.

For the review, pay particular attention to:

  • the help text, ensuring it's clear and not copy-pasted wrong
  • the UX of each command. I tried to stay with args and flags instead of interactive, but on one command it was pretty unavoidable. I'm not too bothered though, I honestly really like the interactive experiences.

Also, it would be great to be able to call this.exit() without it looking like an error. The only way I could think to do that is to throw an error with a specific message that we handle at the top-level. I don't love using errors for control flow though. In any case, a simple return from our perform() is perfectly serviceable.

@xavdid xavdid marked this pull request as ready for review November 20, 2019 06:30
@xavdid xavdid requested a review from eliangcs as a code owner November 20, 2019 06:30
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

The only blocker here is the broken zapier users:links command. I didn't see anything major that needs to be changed in the help text and UX. Good work!

We can leave this in a separate PR, but are you planning to remove zapier invite and zapier collaborate? I think we should because they can be confusing along with the new commands you added.

packages/cli/src/oclif/commands/users/get.js Outdated Show resolved Hide resolved
packages/cli/src/oclif/ZapierBaseCommand.js Outdated Show resolved Hide resolved
packages/cli/src/utils/api.js Outdated Show resolved Hide resolved
@xavdid xavdid merged commit 544b2f1 into master Nov 21, 2019
@xavdid xavdid deleted the add-users branch November 21, 2019 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants