From 14bcd1f0a152fa926f7eb4096616f6db22b818dc Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Thu, 28 Nov 2019 08:11:02 -0500 Subject: [PATCH] MM-18357: Adds pagination to team search. (#12910) * MM-18357: Adds pagination to team search. * MM-18357: Adds new client method for paginated requests. * MM-18357: Adds feedback about non-supported pagination-permissions combo. * MM-18357: Removes unnecessary conversion. * MM-18357: Removes paginate parameter and uses nil on page and perpage instead. --- api4/team.go | 21 ++++++++- api4/team_test.go | 73 ++++++++++++++++++++++++++++++ app/plugin_api.go | 3 +- app/team.go | 9 +++- cmd/mattermost/commands/team.go | 2 +- i18n/en.json | 8 ++++ model/client4.go | 17 +++++++ model/team_search.go | 8 +++- store/sqlstore/team_store.go | 20 ++++++++ store/store.go | 1 + store/storetest/mocks/TeamStore.go | 32 +++++++++++++ 11 files changed, 187 insertions(+), 7 deletions(-) diff --git a/api4/team.go b/api4/team.go index aa2daaa98e..9b26abb189 100644 --- a/api4/team.go +++ b/api4/team.go @@ -789,13 +789,22 @@ func searchTeams(c *Context, w http.ResponseWriter, r *http.Request) { } var teams []*model.Team + var totalCount int64 var err *model.AppError if c.App.SessionHasPermissionTo(c.App.Session, model.PERMISSION_LIST_PRIVATE_TEAMS) && c.App.SessionHasPermissionTo(c.App.Session, model.PERMISSION_LIST_PUBLIC_TEAMS) { - teams, err = c.App.SearchAllTeams(props.Term) + teams, totalCount, err = c.App.SearchAllTeams(props) } else if c.App.SessionHasPermissionTo(c.App.Session, model.PERMISSION_LIST_PRIVATE_TEAMS) { + if props.Page != nil || props.PerPage != nil { + c.Err = model.NewAppError("searchTeams", "api.team.search_teams.pagination_not_implemented.private_team_search", nil, "", http.StatusNotImplemented) + return + } teams, err = c.App.SearchPrivateTeams(props.Term) } else if c.App.SessionHasPermissionTo(c.App.Session, model.PERMISSION_LIST_PUBLIC_TEAMS) { + if props.Page != nil || props.PerPage != nil { + c.Err = model.NewAppError("searchTeams", "api.team.search_teams.pagination_not_implemented.public_team_search", nil, "", http.StatusNotImplemented) + return + } teams, err = c.App.SearchPublicTeams(props.Term) } else { teams = []*model.Team{} @@ -808,7 +817,15 @@ func searchTeams(c *Context, w http.ResponseWriter, r *http.Request) { c.App.SanitizeTeams(c.App.Session, teams) - w.Write([]byte(model.TeamListToJson(teams))) + var payload []byte + if props.Page != nil && props.PerPage != nil { + twc := &model.TeamsWithCount{Teams: teams, TotalCount: totalCount} + payload = model.TeamsWithCountToJson(twc) + } else { + payload = []byte(model.TeamListToJson(teams)) + } + + w.Write(payload) } func teamExists(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/api4/team_test.go b/api4/team_test.go index 53ff834b74..bb923004d3 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -922,6 +922,79 @@ func TestSearchAllTeams(t *testing.T) { CheckUnauthorizedStatus(t, resp) } +func TestSearchAllTeamsPaged(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + commonRandom := model.NewId() + teams := [3]*model.Team{} + + for i := 0; i < 3; i++ { + uid := model.NewId() + newTeam, err := th.App.CreateTeam(&model.Team{ + DisplayName: fmt.Sprintf("%s %d %s", commonRandom, i, uid), + Name: fmt.Sprintf("%s-%d-%s", commonRandom, i, uid), + Type: model.TEAM_OPEN, + Email: th.GenerateTestEmail(), + }) + require.Nil(t, err) + teams[i] = newTeam + } + + testCases := []struct { + Name string + Search *model.TeamSearch + ExpectedTeams []string + ExpectedTotalCount int64 + }{ + { + Name: "Get all teams on one page", + Search: &model.TeamSearch{Term: commonRandom, Page: model.NewInt(0), PerPage: model.NewInt(100)}, + ExpectedTeams: []string{teams[0].Id, teams[1].Id, teams[2].Id}, + ExpectedTotalCount: 3, + }, + { + Name: "Get 2 teams on the first page", + Search: &model.TeamSearch{Term: commonRandom, Page: model.NewInt(0), PerPage: model.NewInt(2)}, + ExpectedTeams: []string{teams[0].Id, teams[1].Id}, + ExpectedTotalCount: 3, + }, + { + Name: "Get 1 team on the second page", + Search: &model.TeamSearch{Term: commonRandom, Page: model.NewInt(1), PerPage: model.NewInt(2)}, + ExpectedTeams: []string{teams[2].Id}, + ExpectedTotalCount: 3, + }, + { + Name: "SearchTeamsPaged paginates results by default", + Search: &model.TeamSearch{Term: commonRandom}, + ExpectedTeams: []string{teams[0].Id, teams[1].Id, teams[2].Id}, + ExpectedTotalCount: 3, + }, + { + Name: "No results", + Search: &model.TeamSearch{Term: model.NewId()}, + ExpectedTeams: []string{}, + ExpectedTotalCount: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + teams, count, resp := th.SystemAdminClient.SearchTeamsPaged(tc.Search) + require.Nil(t, resp.Error) + require.Equal(t, tc.ExpectedTotalCount, count) + require.Equal(t, len(tc.ExpectedTeams), len(teams)) + for i, team := range teams { + require.Equal(t, tc.ExpectedTeams[i], team.Id) + } + }) + } + + _, _, resp := th.Client.SearchTeamsPaged(&model.TeamSearch{Term: commonRandom, PerPage: model.NewInt(100)}) + require.Equal(t, "api.team.search_teams.pagination_not_implemented.public_team_search", resp.Error.Id) + require.Equal(t, http.StatusNotImplemented, resp.StatusCode) +} + func TestSearchAllTeamsSanitization(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() diff --git a/app/plugin_api.go b/app/plugin_api.go index 8aeb2c8f8d..69d1385bfa 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -149,7 +149,8 @@ func (api *PluginAPI) GetTeam(teamId string) (*model.Team, *model.AppError) { } func (api *PluginAPI) SearchTeams(term string) ([]*model.Team, *model.AppError) { - return api.app.SearchAllTeams(term) + teams, _, err := api.app.SearchAllTeams(&model.TeamSearch{Term: term}) + return teams, err } func (api *PluginAPI) GetTeamByName(name string) (*model.Team, *model.AppError) { diff --git a/app/team.go b/app/team.go index e1bee92456..37219395c4 100644 --- a/app/team.go +++ b/app/team.go @@ -710,8 +710,13 @@ func (a *App) GetAllPublicTeamsPageWithCount(offset int, limit int) (*model.Team return &model.TeamsWithCount{Teams: teams, TotalCount: totalCount}, nil } -func (a *App) SearchAllTeams(term string) ([]*model.Team, *model.AppError) { - return a.Srv.Store.Team().SearchAll(term) +// SearchAllTeams returns a team list and the total count of the results +func (a *App) SearchAllTeams(searchOpts *model.TeamSearch) ([]*model.Team, int64, *model.AppError) { + if searchOpts.IsPaginated() { + return a.Srv.Store.Team().SearchAllPaged(searchOpts.Term, *searchOpts.Page, *searchOpts.PerPage) + } + results, err := a.Srv.Store.Team().SearchAll(searchOpts.Term) + return results, int64(len(results)), err } func (a *App) SearchPublicTeams(term string) ([]*model.Team, *model.AppError) { diff --git a/cmd/mattermost/commands/team.go b/cmd/mattermost/commands/team.go index e5a8f72e7c..685c813701 100644 --- a/cmd/mattermost/commands/team.go +++ b/cmd/mattermost/commands/team.go @@ -310,7 +310,7 @@ func searchTeamCmdF(command *cobra.Command, args []string) error { var teams []*model.Team for _, searchTerm := range args { - foundTeams, err := a.SearchAllTeams(searchTerm) + foundTeams, _, err := a.SearchAllTeams(&model.TeamSearch{Term: searchTerm}) if err != nil { return err } diff --git a/i18n/en.json b/i18n/en.json index b3c1580c0d..f00248d0b4 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -2034,6 +2034,14 @@ "id": "api.team.remove_user_from_team.removed", "translation": "%v removed from the team." }, + { + "id": "api.team.search_teams.pagination_not_implemented.private_team_search", + "translation": "Pagination not implemented for private-only team search." + }, + { + "id": "api.team.search_teams.pagination_not_implemented.public_team_search", + "translation": "Pagination not implemented for public-only team search." + }, { "id": "api.team.set_team_icon.array.app_error", "translation": "Empty array under 'image' in request" diff --git a/model/client4.go b/model/client4.go index 817621e787..b519a19792 100644 --- a/model/client4.go +++ b/model/client4.go @@ -1661,6 +1661,23 @@ func (c *Client4) SearchTeams(search *TeamSearch) ([]*Team, *Response) { return TeamListFromJson(r.Body), BuildResponse(r) } +// SearchTeamsPaged returns a page of teams and the total count matching the provided search term. +func (c *Client4) SearchTeamsPaged(search *TeamSearch) ([]*Team, int64, *Response) { + if search.Page == nil { + search.Page = NewInt(0) + } + if search.PerPage == nil { + search.PerPage = NewInt(100) + } + r, err := c.DoApiPost(c.GetTeamsRoute()+"/search", search.ToJson()) + if err != nil { + return nil, 0, BuildErrorResponse(r, err) + } + defer closeBody(r) + twc := TeamsWithCountFromJson(r.Body) + return twc.Teams, twc.TotalCount, BuildResponse(r) +} + // TeamExists returns true or false if the team exist or not. func (c *Client4) TeamExists(name, etag string) (bool, *Response) { r, err := c.DoApiGet(c.GetTeamByNameRoute(name)+"/exists", etag) diff --git a/model/team_search.go b/model/team_search.go index e0676022d6..042d353910 100644 --- a/model/team_search.go +++ b/model/team_search.go @@ -9,7 +9,13 @@ import ( ) type TeamSearch struct { - Term string `json:"term"` + Term string `json:"term"` + Page *int `json:"page,omitempty"` + PerPage *int `json:"per_page,omitempty"` +} + +func (t *TeamSearch) IsPaginated() bool { + return t.Page != nil && t.PerPage != nil } // ToJson convert a TeamSearch to json string diff --git a/store/sqlstore/team_store.go b/store/sqlstore/team_store.go index aedceb03a5..8563fc5ace 100644 --- a/store/sqlstore/team_store.go +++ b/store/sqlstore/team_store.go @@ -305,6 +305,26 @@ func (s SqlTeamStore) SearchAll(term string) ([]*model.Team, *model.AppError) { return teams, nil } +// SearchAllPaged returns a teams list and the total count of teams that matched the search. +func (s SqlTeamStore) SearchAllPaged(term string, page int, perPage int) ([]*model.Team, int64, *model.AppError) { + var teams []*model.Team + var totalCount int64 + offset := page * perPage + + term = sanitizeSearchTerm(term, "\\") + + if _, err := s.GetReplica().Select(&teams, "SELECT * FROM Teams WHERE Name LIKE :Term OR DisplayName LIKE :Term ORDER BY DisplayName, Name LIMIT :Limit OFFSET :Offset", map[string]interface{}{"Term": term + "%", "Limit": perPage, "Offset": offset}); err != nil { + return nil, 0, model.NewAppError("SqlTeamStore.SearchAllPage", "store.sql_team.search_all_team.app_error", nil, "term="+term+", "+err.Error(), http.StatusInternalServerError) + } + + totalCount, err := s.GetReplica().SelectInt("SELECT COUNT(*) FROM Teams WHERE Name LIKE :Term OR DisplayName LIKE :Term", map[string]interface{}{"Term": term + "%"}) + if err != nil { + return nil, 0, model.NewAppError("SqlTeamStore.SearchAllPage", "store.sql_team.search_all_team.app_error", nil, "term="+term+", "+err.Error(), http.StatusInternalServerError) + } + + return teams, totalCount, nil +} + func (s SqlTeamStore) SearchOpen(term string) ([]*model.Team, *model.AppError) { var teams []*model.Team diff --git a/store/store.go b/store/store.go index 847edc0597..e71e625359 100644 --- a/store/store.go +++ b/store/store.go @@ -68,6 +68,7 @@ type TeamStore interface { Get(id string) (*model.Team, *model.AppError) GetByName(name string) (*model.Team, *model.AppError) SearchAll(term string) ([]*model.Team, *model.AppError) + SearchAllPaged(term string, page int, perPage int) ([]*model.Team, int64, *model.AppError) SearchOpen(term string) ([]*model.Team, *model.AppError) SearchPrivate(term string) ([]*model.Team, *model.AppError) GetAll() ([]*model.Team, *model.AppError) diff --git a/store/storetest/mocks/TeamStore.go b/store/storetest/mocks/TeamStore.go index 191d8be7fa..1da0823888 100644 --- a/store/storetest/mocks/TeamStore.go +++ b/store/storetest/mocks/TeamStore.go @@ -908,6 +908,38 @@ func (_m *TeamStore) SearchAll(term string) ([]*model.Team, *model.AppError) { return r0, r1 } +// SearchAllPaged provides a mock function with given fields: term, page, perPage +func (_m *TeamStore) SearchAllPaged(term string, page int, perPage int) ([]*model.Team, int64, *model.AppError) { + ret := _m.Called(term, page, perPage) + + var r0 []*model.Team + if rf, ok := ret.Get(0).(func(string, int, int) []*model.Team); ok { + r0 = rf(term, page, perPage) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*model.Team) + } + } + + var r1 int64 + if rf, ok := ret.Get(1).(func(string, int, int) int64); ok { + r1 = rf(term, page, perPage) + } else { + r1 = ret.Get(1).(int64) + } + + var r2 *model.AppError + if rf, ok := ret.Get(2).(func(string, int, int) *model.AppError); ok { + r2 = rf(term, page, perPage) + } else { + if ret.Get(2) != nil { + r2 = ret.Get(2).(*model.AppError) + } + } + + return r0, r1, r2 +} + // SearchOpen provides a mock function with given fields: term func (_m *TeamStore) SearchOpen(term string) ([]*model.Team, *model.AppError) { ret := _m.Called(term)