From 7fc0e57490f30b47d51a2dadce771b6adbbaace8 Mon Sep 17 00:00:00 2001 From: Jumpei Matsuda Date: Wed, 29 Jun 2022 00:31:53 +0900 Subject: [PATCH 1/3] fix: Create UpdateUserGroup option and allow empty string/array as the values --- usergroups.go | 70 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 12 deletions(-) diff --git a/usergroups.go b/usergroups.go index 9417f8177..2990421e2 100644 --- a/usergroups.go +++ b/usergroups.go @@ -3,6 +3,7 @@ package slack import ( "context" "net/url" + "reflect" "strings" ) @@ -183,32 +184,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: nil, 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(¶ms) + } + 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 !reflect.ValueOf(params.Description).IsNil() { + values["description"] = []string{*params.Description} } - if len(userGroup.Prefs.Channels) > 0 { - values["channels"] = []string{strings.Join(userGroup.Prefs.Channels, ",")} + if !reflect.ValueOf(params.Channels).IsNil() { + values["channels"] = []string{strings.Join(*params.Channels, ",")} } response, err := api.userGroupRequest(ctx, "usergroups.update", values) From 38367d1ba7a2afb4321629f6b52fe1653927f900 Mon Sep 17 00:00:00 2001 From: Jumpei Matsuda Date: Wed, 29 Jun 2022 01:14:30 +0900 Subject: [PATCH 2/3] test: make sure empty values are sent --- usergroups_test.go | 115 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 106 insertions(+), 9 deletions(-) diff --git a/usergroups_test.go b/usergroups_test.go index 5b92dcdf1..d98ca257e 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,104 @@ 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." + + emptyChannels := []string{} + presenceChannels := []string{"channel1", "channel2"} + + 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(&presenceChannels), + }, + map[string]string{ + "token": "testing-token", + "usergroup": "S0615G0KT", + "description": "Marketing gurus, PR experts and product advocates.", + "channels": "channel1,channel2", + }, + }, + { + []UpdateUserGroupsOption{ + UpdateUserGroupsOptionDescription(&emptyDescription), + UpdateUserGroupsOptionChannels(&emptyChannels), + }, + 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) + } + } +} From c3c208411babf75cc5daad33c0168ee7c6c2bfda Mon Sep 17 00:00:00 2001 From: Jumpei Matsuda Date: Mon, 25 Jul 2022 13:37:54 +0900 Subject: [PATCH 3/3] fix: Don't use meaningless slice pointers and reflect package --- usergroups.go | 11 +++++------ usergroups_test.go | 7 ++----- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/usergroups.go b/usergroups.go index 2990421e2..c5d7a176b 100644 --- a/usergroups.go +++ b/usergroups.go @@ -3,7 +3,6 @@ package slack import ( "context" "net/url" - "reflect" "strings" ) @@ -208,10 +207,10 @@ func UpdateUserGroupsOptionDescription(description *string) UpdateUserGroupsOpti } } -// UpdateUserGroupsOptionChannels change the default channels of the User Group. (default: nil, so it's no-op) -func UpdateUserGroupsOptionChannels(channels *[]string) UpdateUserGroupsOption { +// 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 + params.Channels = &channels } } @@ -249,11 +248,11 @@ func (api *Client) UpdateUserGroupContext(ctx context.Context, userGroupID strin values["handle"] = []string{params.Handle} } - if !reflect.ValueOf(params.Description).IsNil() { + if params.Description != nil { values["description"] = []string{*params.Description} } - if !reflect.ValueOf(params.Channels).IsNil() { + if params.Channels != nil { values["channels"] = []string{strings.Join(*params.Channels, ",")} } diff --git a/usergroups_test.go b/usergroups_test.go index d98ca257e..9586cf167 100644 --- a/usergroups_test.go +++ b/usergroups_test.go @@ -222,9 +222,6 @@ func TestUpdateUserGroup(t *testing.T) { emptyDescription := "" presenceDescription := "Marketing gurus, PR experts and product advocates." - emptyChannels := []string{} - presenceChannels := []string{"channel1", "channel2"} - tests := []struct { options []UpdateUserGroupsOption wantParams map[string]string @@ -244,7 +241,7 @@ func TestUpdateUserGroup(t *testing.T) { { []UpdateUserGroupsOption{ UpdateUserGroupsOptionDescription(&presenceDescription), - UpdateUserGroupsOptionChannels(&presenceChannels), + UpdateUserGroupsOptionChannels([]string{"channel1", "channel2"}), }, map[string]string{ "token": "testing-token", @@ -256,7 +253,7 @@ func TestUpdateUserGroup(t *testing.T) { { []UpdateUserGroupsOption{ UpdateUserGroupsOptionDescription(&emptyDescription), - UpdateUserGroupsOptionChannels(&emptyChannels), + UpdateUserGroupsOptionChannels([]string{}), }, map[string]string{ "token": "testing-token",