From 80c846412d6439e664ba69fda5d94ccdf99b4ed0 Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Thu, 7 May 2020 14:35:09 -0600 Subject: [PATCH] MM-24692: Add a since parameter to getGroups api (#14444) * add a since parameter to getGroups api * update for lint error * when using since, return deleted groups as well. * update flaky test, groups have same create time Co-authored-by: mattermod --- api4/group.go | 11 +++++++++++ api4/group_test.go | 33 +++++++++++++++++++++++++++++++++ model/client4.go | 3 +++ model/group.go | 1 + store/sqlstore/group_store.go | 9 ++++++++- store/storetest/group_store.go | 32 +++++++++++++++++++++++++++++++- 6 files changed, 87 insertions(+), 2 deletions(-) diff --git a/api4/group.go b/api4/group.go index 597da6962f..c29a05ff5e 100644 --- a/api4/group.go +++ b/api4/group.go @@ -8,6 +8,7 @@ import ( "fmt" "io/ioutil" "net/http" + "strconv" "strings" "github.com/mattermost/mattermost-server/v5/audit" @@ -730,6 +731,16 @@ func getGroups(c *Context, w http.ResponseWriter, r *http.Request) { opts.NotAssociatedToChannel = channelID } + sinceString := r.URL.Query().Get("since") + if len(sinceString) > 0 { + since, parseError := strconv.ParseInt(sinceString, 10, 64) + if parseError != nil { + c.SetInvalidParam("since") + return + } + opts.Since = since + } + groups, err := c.App.GetGroups(c.Params.Page, c.Params.PerPage, opts) if err != nil { c.Err = err diff --git a/api4/group_test.go b/api4/group_test.go index e7e4c65019..bf3f487ce5 100644 --- a/api4/group_test.go +++ b/api4/group_test.go @@ -851,6 +851,8 @@ func TestGetGroups(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() + // make sure "createdDate" for next group is after one created in InitBasic() + time.Sleep(2 * time.Millisecond) id := model.NewId() group, err := th.App.CreateGroup(&model.Group{ DisplayName: "dn-foo_" + id, @@ -860,6 +862,7 @@ func TestGetGroups(t *testing.T) { RemoteId: model.NewId(), }) assert.Nil(t, err) + start := group.UpdateAt - 1 opts := model.GroupSearchOpts{ PageOpts: &model.PageOpts{ @@ -911,6 +914,36 @@ func TestGetGroups(t *testing.T) { _, response = th.Client.GetGroups(opts) assert.Nil(t, response.Error) + + // test "since", should only return group created in this test, not th.Group + opts.Since = start + groups, response = th.Client.GetGroups(opts) + assert.Nil(t, response.Error) + assert.Len(t, groups, 1) + // test correct group returned + assert.Equal(t, groups[0].Id, group.Id) + + // delete group, should still return + th.App.DeleteGroup(group.Id) + groups, response = th.Client.GetGroups(opts) + assert.Nil(t, response.Error) + assert.Len(t, groups, 1) + assert.Equal(t, groups[0].Id, group.Id) + + // test with current since value, return none + opts.Since = model.GetMillis() + groups, response = th.Client.GetGroups(opts) + assert.Nil(t, response.Error) + assert.Empty(t, groups) + + // make sure delete group is not returned without Since + opts.Since = 0 + groups, response = th.Client.GetGroups(opts) + assert.Nil(t, response.Error) + //'Normal getGroups should not return delete groups + assert.Len(t, groups, 1) + // make sure it returned th.Group,not group + assert.Equal(t, groups[0].Id, th.Group.Id) } func TestGetGroupsByUserId(t *testing.T) { diff --git a/model/client4.go b/model/client4.go index 0a8907657d..61dc976ed9 100644 --- a/model/client4.go +++ b/model/client4.go @@ -3781,6 +3781,9 @@ func (c *Client4) GetGroups(opts GroupSearchOpts) ([]*Group, *Response) { opts.FilterAllowReference, opts.Q, ) + if opts.Since > 0 { + path = fmt.Sprintf("%s&since=%v", path, opts.Since) + } if opts.PageOpts != nil { path = fmt.Sprintf("%s&page=%v&per_page=%v", path, opts.PageOpts.Page, opts.PageOpts.PerPage) } diff --git a/model/group.go b/model/group.go index 9892a6fcff..aceddcf104 100644 --- a/model/group.go +++ b/model/group.go @@ -79,6 +79,7 @@ type GroupSearchOpts struct { IncludeMemberCount bool FilterAllowReference bool PageOpts *PageOpts + Since int64 } type PageOpts struct { diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 8b2f60e4dc..98cbee181a 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -1144,9 +1144,16 @@ func (s *SqlGroupStore) GetGroups(page, perPage int, opts model.GroupSearchOpts) groupsQuery = groupsQuery. From("UserGroups g"). - Where("g.DeleteAt = 0"). OrderBy("g.DisplayName") + if opts.Since > 0 { + groupsQuery = groupsQuery.Where(sq.Gt{ + "g.UpdateAt": opts.Since, + }) + } else { + groupsQuery = groupsQuery.Where("g.DeleteAt = 0") + } + if perPage != 0 { groupsQuery = groupsQuery. Limit(uint64(perPage)). diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 903abc24f5..6874cde2d4 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -3010,6 +3010,8 @@ func testGetGroups(t *testing.T, ss store.Store) { team1, err := ss.Team().Save(team1) require.Nil(t, err) + startCreateTime := team1.UpdateAt - 1 + // Create Channel1 channel1 := &model.Channel{ TeamId: model.NewId(), @@ -3148,10 +3150,12 @@ func testGetGroups(t *testing.T, ss store.Store) { require.Nil(t, err) user2.DeleteAt = 1 - ss.User().Update(user2, true) + u2Update, _ := ss.User().Update(user2, true) group2NameSubstring := "group-2" + endCreateTime := u2Update.New.UpdateAt + 1 + testCases := []struct { Name string Page int @@ -3309,6 +3313,32 @@ func testGetGroups(t *testing.T, ss store.Store) { return true }, }, + { + Name: "Use Since return all", + Opts: model.GroupSearchOpts{FilterAllowReference: true, Since: startCreateTime}, + Page: 0, + PerPage: 100, + Resultf: func(groups []*model.Group) bool { + if len(groups) == 0 { + return false + } + for _, g := range groups { + if g.DeleteAt != 0 { + return false + } + } + return true + }, + }, + { + Name: "Use Since return none", + Opts: model.GroupSearchOpts{FilterAllowReference: true, Since: endCreateTime}, + Page: 0, + PerPage: 100, + Resultf: func(groups []*model.Group) bool { + return len(groups) == 0 + }, + }, } for _, tc := range testCases {