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

Fixes updateUserGroup cannot clear the description and the default channels #1082

Merged

Conversation

jmatsu
Copy link
Contributor

@jmatsu jmatsu commented Jun 28, 2022

Fix: #1081

Expected

We can clear the description and the default channels of usergroups by sending empty strings.

Actual

updateUserGroup ignores empty strings so this never sends empty strings to the server.

Examples

The followings are examples using curl. ref: https://api.slack.com/methods/usergroups.update

Set Up

curl -F token=... -F usergroup=SJ6JQHVBN --url 'https://slack.com/api/usergroups.update' -F description="this is a description" -F handle="zz__test_usergroup" -F name="this is a name" -F channels=C92MKCRPU
{"usergroup":{"id":"SJ6JQHVBN","name":"this is a name","description":"this is a description","handle":"zz__test_usergroup","prefs":{"channels":["C92MKCRPU"],"groups":[]}}, ...}

Clear the default channels

curl -F token=... -F usergroup=SJ6JQHVBN --url 'https://slack.com/api/usergroups.update' -F description="this is a description" -F handle="zz__test_usergroup" -F name="this is a name" -F channels=
{"usergroup":{"id":"SJ6JQHVBN","name":"this is a name","description":"this is a description","handle":"zz__test_usergroup","prefs":{"channels":[],"groups":[]}}, ...}

Clear the description

curl -F token=... -F usergroup=SJ6JQHVBN --url 'https://slack.com/api/usergroups.update' -F description= -F handle="zz__test_usergroup" -F name="this is a name" -F channels=
{"usergroup":{"id":"SJ6JQHVBN","name":"this is a name","description":"","handle":"zz__test_usergroup","prefs":{"channels":[],"groups":[]}}, ...}


PR preparation

✅ Ran make pr-prop on HEAD (38367d1)

Should this be an issue instead
  • No is it a convenience method? (no new functionality, streamlines some use case)
  • No exposes a previously private type, const, method, etc.
  • No is it application specific (caching, retry logic, rate limiting, etc)
  • No is it performance related.
API changes

Since API changes have to be maintained they undergo a more detailed review and are more likely to require changes.

  • no tests, if you're adding to the API include at least a single test of the happy case.
    • I have added the test cases for updateUserGroup
  • If you can accomplish your goal without changing the API, then do so.
    • Please let me know if you prefer a way to deprecate the current methods and create new ones.
  • dependency changes. updates are okay. adding/removing need justification.
    • Nothing.
Examples of API changes that do not meet guidelines:

N/A

Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR and sorry for delay🙏

usergroups.go Outdated
}

// UpdateUserGroupsOptionChannels change the default channels of the User Group. (default: nil, so it's no-op)
func UpdateUserGroupsOptionChannels(channels *[]string) UpdateUserGroupsOption {
Copy link
Member

Choose a reason for hiding this comment

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

I think the type of channels should be []string.

usergroups.go Outdated
}

if userGroup.Description != "" {
values["description"] = []string{userGroup.Description}
if !reflect.ValueOf(params.Description).IsNil() {
Copy link
Member

Choose a reason for hiding this comment

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

[question]
Why do you use reflect.ValueOf and reflect.Value.IsNil?
I think you can use params.Description == nil in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my misunderstanding. Thanks!

@kanata2 kanata2 added feedback given When a review has been conducted and awaiting the response from the comitter(s) and removed needs review labels Jul 21, 2022
@kanata2 kanata2 added this to the v0.12.0 milestone Jul 21, 2022
@jmatsu
Copy link
Contributor Author

jmatsu commented Jul 25, 2022

@kanata2 Thanks for your review. Could you please have a look again?

@jmatsu jmatsu requested a review from kanata2 July 25, 2022 04:44
@kanata2 kanata2 removed the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Jul 25, 2022
Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM!

@kanata2 kanata2 merged commit 1edf0c7 into slack-go:master Jul 25, 2022
@kanata2
Copy link
Member

kanata2 commented Jul 25, 2022

Released! https://github.com/slack-go/slack/releases/tag/v0.11.2

@jmatsu jmatsu deleted the fix/usergroup_update_cannot_set_zero_value branch July 26, 2022 01:16
@kanata2 kanata2 removed this from the v0.12.0 milestone Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

updateUserGroup cannot clear the description and the default channels
2 participants