Repository: slack-go/slack
Pull request #1082 (2022-06-28 17:11:03)

Fixes updateUserGroup cannot clear the description and the default channels
Fix: https://github.com/slack-go/slack/issues/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
- [x] No ~is it a convenience method? (no new functionality, streamlines some use case)~
- [x] No ~exposes a previously private type, const, method, etc.~
- [x] No ~is it application specific (caching, retry logic, rate limiting, etc)~
- [x] 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



Patch:
diff --git a/usergroups.go b/usergroups.go
index 9417f81..c5d7a17 100644
--- a/usergroups.go
+++ b/usergroups.go
@@ -183,32 +183,77 @@ func (api *Client) GetUserGroupsContext(ctx context.Context, options ...GetUserG
 	return response.UserGroups, nil
 }
 
+// UpdateUserGroupsOption options for the UpdateUserGroup method call.
+type UpdateUserGroupsOption func(*UpdateUserGroupsParams)
+
+// UpdateUserGroupsOptionName change the name of the User Group (default: empty, so it's no-op)
+func UpdateUserGroupsOptionName(name string) UpdateUserGroupsOption {
+	return func(params *UpdateUserGroupsParams) {
+		params.Name = name
+	}
+}
+
+// UpdateUserGroupsOptionHandle change the handle of the User Group (default: empty, so it's no-op)
+func UpdateUserGroupsOptionHandle(handle string) UpdateUserGroupsOption {
+	return func(params *UpdateUserGroupsParams) {
+		params.Handle = handle
+	}
+}
+
+// UpdateUserGroupsOptionDescription change the description of the User Group. (default: nil, so it's no-op)
+func UpdateUserGroupsOptionDescription(description *string) UpdateUserGroupsOption {
+	return func(params *UpdateUserGroupsParams) {
+		params.Description = description
+	}
+}
+
+// UpdateUserGroupsOptionChannels change the default channels of the User Group. (default: unspecified, so it's no-op)
+func UpdateUserGroupsOptionChannels(channels []string) UpdateUserGroupsOption {
+	return func(params *UpdateUserGroupsParams) {
+		params.Channels = &channels
+	}
+}
+
+// UpdateUserGroupsParams contains arguments for UpdateUserGroup method call
+type UpdateUserGroupsParams struct {
+	Name        string
+	Handle      string
+	Description *string
+	Channels    *[]string
+}
+
 // UpdateUserGroup will update an existing user group
-func (api *Client) UpdateUserGroup(userGroup UserGroup) (UserGroup, error) {
-	return api.UpdateUserGroupContext(context.Background(), userGroup)
+func (api *Client) UpdateUserGroup(userGroupID string, options ...UpdateUserGroupsOption) (UserGroup, error) {
+	return api.UpdateUserGroupContext(context.Background(), userGroupID, options...)
 }
 
 // UpdateUserGroupContext will update an existing user group with a custom context
-func (api *Client) UpdateUserGroupContext(ctx context.Context, userGroup UserGroup) (UserGroup, error) {
+func (api *Client) UpdateUserGroupContext(ctx context.Context, userGroupID string, options ...UpdateUserGroupsOption) (UserGroup, error) {
+	params := UpdateUserGroupsParams{}
+
+	for _, opt := range options {
+		opt(&params)
+	}
+
 	values := url.Values{
 		"token":     {api.token},
-		"usergroup": {userGroup.ID},
+		"usergroup": {userGroupID},
 	}
 
-	if userGroup.Name != "" {
-		values["name"] = []string{userGroup.Name}
+	if params.Name != "" {
+		values["name"] = []string{params.Name}
 	}
 
-	if userGroup.Handle != "" {
-		values["handle"] = []string{userGroup.Handle}
+	if params.Handle != "" {
+		values["handle"] = []string{params.Handle}
 	}
 
-	if userGroup.Description != "" {
-		values["description"] = []string{userGroup.Description}
+	if params.Description != nil {
+		values["description"] = []string{*params.Description}
 	}
 
-	if len(userGroup.Prefs.Channels) > 0 {
-		values["channels"] = []string{strings.Join(userGroup.Prefs.Channels, ",")}
+	if params.Channels != nil {
+		values["channels"] = []string{strings.Join(*params.Channels, ",")}
 	}
 
 	response, err := api.userGroupRequest(ctx, "usergroups.update", values)


Test patch:
diff --git a/usergroups_test.go b/usergroups_test.go
index 5b92dcd..9586cf1 100644
--- a/usergroups_test.go
+++ b/usergroups_test.go
@@ -45,16 +45,11 @@ func newUserGroupsHandler() *userGroupsHandler {
 	}
 }
 
-func (ugh *userGroupsHandler) accumulateFormValue(k string, r *http.Request) {
-	if v := r.FormValue(k); v != "" {
-		ugh.gotParams[k] = v
-	}
-}
-
 func (ugh *userGroupsHandler) handler(w http.ResponseWriter, r *http.Request) {
-	ugh.accumulateFormValue("name", r)
-	ugh.accumulateFormValue("description", r)
-	ugh.accumulateFormValue("handle", r)
+	r.ParseForm()
+	for k, v := range r.Form {
+		ugh.gotParams[k] = v[0]
+	}
 	w.Header().Set("Content-Type", "application/json")
 	w.Write([]byte(ugh.response))
 }
@@ -73,6 +68,7 @@ func TestCreateUserGroup(t *testing.T) {
 				Description: "Marketing gurus, PR experts and product advocates.",
 				Handle:      "marketing-team"},
 			map[string]string{
+				"token":       "testing-token",
 				"name":        "Marketing Team",
 				"description": "Marketing gurus, PR experts and product advocates.",
 				"handle":      "marketing-team",
@@ -184,3 +180,101 @@ func TestGetUserGroups(t *testing.T) {
 		t.Errorf("Got %#v, want %#v", userGroups[0], S0614TZR7)
 	}
 }
+
+func updateUserGroupsHandler() *userGroupsHandler {
+	return &userGroupsHandler{
+		gotParams: make(map[string]string),
+		response: `{
+    "ok": true,
+    "usergroup": {
+        "id": "S0615G0KT",
+        "team_id": "T060RNRCH",
+        "is_usergroup": true,
+        "name": "Marketing Team",
+        "description": "Marketing gurus, PR experts and product advocates.",
+        "handle": "marketing-team",
+        "is_external": false,
+        "date_create": 1446746793,
+        "date_update": 1446746793,
+        "date_delete": 0,
+        "auto_type": null,
+        "created_by": "U060RNRCZ",
+        "updated_by": "U060RNRCZ",
+        "deleted_by": null,
+        "prefs": {
+            "channels": [
+				"channel1",
+				"channel2"
+            ],
+            "groups": [
+
+            ]
+        },
+        "user_count": 0
+    }
+}`,
+	}
+}
+func TestUpdateUserGroup(t *testing.T) {
+	once.Do(startServer)
+	api := New("testing-token", OptionAPIURL("http://"+serverAddr+"/"))
+
+	emptyDescription := ""
+	presenceDescription := "Marketing gurus, PR experts and product advocates."
+
+	tests := []struct {
+		options    []UpdateUserGroupsOption
+		wantParams map[string]string
+	}{
+		{
+			[]UpdateUserGroupsOption{
+				UpdateUserGroupsOptionName("Marketing Team"),
+				UpdateUserGroupsOptionHandle("marketing-team"),
+			},
+			map[string]string{
+				"token":     "testing-token",
+				"usergroup": "S0615G0KT",
+				"name":      "Marketing Team",
+				"handle":    "marketing-team",
+			},
+		},
+		{
+			[]UpdateUserGroupsOption{
+				UpdateUserGroupsOptionDescription(&presenceDescription),
+				UpdateUserGroupsOptionChannels([]string{"channel1", "channel2"}),
+			},
+			map[string]string{
+				"token":       "testing-token",
+				"usergroup":   "S0615G0KT",
+				"description": "Marketing gurus, PR experts and product advocates.",
+				"channels":    "channel1,channel2",
+			},
+		},
+		{
+			[]UpdateUserGroupsOption{
+				UpdateUserGroupsOptionDescription(&emptyDescription),
+				UpdateUserGroupsOptionChannels([]string{}),
+			},
+			map[string]string{
+				"token":       "testing-token",
+				"usergroup":   "S0615G0KT",
+				"description": "",
+				"channels":    "",
+			},
+		},
+	}
+
+	var rh *userGroupsHandler
+	http.HandleFunc("/usergroups.update", func(w http.ResponseWriter, r *http.Request) { rh.handler(w, r) })
+
+	for i, test := range tests {
+		rh = updateUserGroupsHandler()
+		_, err := api.UpdateUserGroup("S0615G0KT", test.options...)
+		if err != nil {
+			t.Fatalf("%d: Unexpected error: %s", i, err)
+		}
+		if !reflect.DeepEqual(rh.gotParams, test.wantParams) {
+			t.Errorf("%d: Got params %#v, want %#v", i, rh.gotParams, test.wantParams)
+		}
+	}
+}