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

team: allow specifying role #262

Closed
wants to merge 1 commit into from

Conversation

GiedriusS
Copy link
Contributor

Permit specifying the role when adding a user to a team. Tested locally. The role must be "manager", "observer", or "responder".

type AddUserToTeamOptions struct {
TeamID string `json:"-"`
UserID string `json:"-"`
Role string `json:"role,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on your comment in the PR, there are only a handful of values that are accepted. Instead of this being a string, should we make it a named string type so that we can have a set of constants that people can also use?

type UserRole string

const (
  UserRoleManager UserRole = "manager"
  // ...
)

The User struct also has a Role field that's a string. I'm not sure if it makes sense for these to be different. 🤔

To be clear, this comment isn't a request to change but looking for your thoughts / feedback.

@@ -84,8 +91,8 @@ func (c *Client) RemoveUserFromTeam(teamID, userID string) error {
}

// AddUserToTeam adds a user to a team.
func (c *Client) AddUserToTeam(teamID, userID string) error {
_, err := c.put("/teams/"+teamID+"/users/"+userID, nil, nil)
func (c *Client) AddUserToTeam(options AddUserToTeamOptions) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This modification makes this a breaking change, which will add some delays in merging.

How would you feel about creating a new method, and update AddUserToTeam() to call that one and indicate the other should be used instead? Then we can file an issue to clean this up in v2.0.0.

@theckman theckman added open question there is an open question on the issue / PR breaking change This PR contains a breaking change labels Feb 20, 2021
theckman added a commit that referenced this pull request Mar 8, 2021
@theckman
Copy link
Collaborator

theckman commented Mar 8, 2021

I've taken inspiration from this PR, and added an option struct with role support to #308 on the new method that also accepts the context.Context. #308 will accomplish the same thing as this PR, without the breaking change, and so I'm going to close this PR in favor of that one.

Thank you for this contribution and the inspiration.

@theckman theckman closed this Mar 8, 2021
@theckman theckman removed the open question there is an open question on the issue / PR label Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR contains a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants