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

CLI-80: user-blocks commands with user_id #201

Closed
wants to merge 1 commit into from

Conversation

mhsu-auth0
Copy link
Contributor

@mhsu-auth0 mhsu-auth0 commented Mar 26, 2021


func userBlocksCmd(cli *cli) *cobra.Command {
cmd := &cobra.Command{
Use: "user-blocks",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put it under users

Copy link
Contributor

Choose a reason for hiding this comment

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

As a subcommand: auth0 users blocks


var (
userID = Argument{
Name: "user_id",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Name: "user_id",
Name: "User ID",

var (
userID = Argument{
Name: "user_id",
Help: "user_id of the user.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Help: "user_id of the user.",
Help: "Id of the user.",


func listUserBlocksByUserIdCmd(cli *cli) *cobra.Command {
var inputs struct {
user_id string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use snake_case

}

cmd := &cobra.Command{
Use: "listByUserId",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use list. Entire command should be auth0 users blocks list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can list by user_id or list by email / phonenumber / user_identifier. It's a different API call, what would a good way to name the handle the two different input types?

Copy link
Contributor

Choose a reason for hiding this comment

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

The command should accept any of these as a single argument, and then disambiguate. It's the easiest for the user, that remains unaware (as they should) of those implementation details.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. auth0 apis show accepts an API id or the audience.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the end user, there's no difference.

cmd := &cobra.Command{
Use: "listByUserId",
Args: cobra.MaximumNArgs(1),
Short: "List user-blocks by user_id",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not include the API2 entities as-is, instead describe what the command does in the context of a single user.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assume the user knows nothing about API2.

Use: "listByUserId",
Args: cobra.MaximumNArgs(1),
Short: "List user-blocks by user_id",
Long: `List user-blocks by user_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.


func deleteUserBlocksByUserIdCmd(cli *cli) *cobra.Command {
var inputs struct {
user_id string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use snake_case.

}

cmd := &cobra.Command{
Use: "deleteByUserId",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please usedelete. Entire command should be auth0 users blocks delete.

cmd := &cobra.Command{
Use: "deleteByUserId",
Args: cobra.MaximumNArgs(1),
Short: "Delete all user-blocks by user_id",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not include the API2 entities as-is, instead describe what the command does in the context of a single user.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assume the user knows nothing about API2.

Use: "deleteByUserId",
Args: cobra.MaximumNArgs(1),
Short: "Delete all user-blocks by user_id",
Long: `Delete all user-blocks by user_id:
Copy link
Contributor

@Widcket Widcket Mar 27, 2021

Choose a reason for hiding this comment

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

Same as above.

}

func (r *Renderer) UserBlocksList(userBlocks []*management.UserBlock) {
resource := "userBlocks"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resource := "userBlocks"
resource := "user blocks"


if len(userBlocks) == 0 {
r.EmptyState(resource)
r.Infof("No blocks for user.")
Copy link
Contributor

Choose a reason for hiding this comment

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

r.EmptyState(resource) and r.Infof("No blocks for user.") do the same thing, please remove the last one.

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Please refactor to fit in with the rest of the CLI. Please use the apps, apis, and rules commands as a reference.

@mhsu-auth0 mhsu-auth0 requested a review from Widcket March 29, 2021 23:45
@mhsu-auth0 mhsu-auth0 closed this Mar 29, 2021
@mhsu-auth0
Copy link
Contributor Author

moving to branch on main repo

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