From 0a12fef33aec10b26110d71b79201fa296e99cc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Fri, 26 Jul 2019 15:18:37 +0200 Subject: [PATCH] Fix pagination bug on LDAP groups users listing (#11711) * Fix pagination bug on LDAP groups users listing * Fixing consistency in parameters names --- store/sqlstore/group_store.go | 4 ++-- store/store.go | 2 +- store/storetest/group_store.go | 23 +++++++++++++++++------ store/storetest/mocks/GroupStore.go | 10 +++++----- 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index dffa34d2a4..981b597361 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -228,7 +228,7 @@ func (s *SqlGroupStore) GetMemberUsers(groupID string) ([]*model.User, *model.Ap return groupMembers, nil } -func (s *SqlGroupStore) GetMemberUsersPage(groupID string, offset int, limit int) ([]*model.User, *model.AppError) { +func (s *SqlGroupStore) GetMemberUsersPage(groupID string, page int, perPage int) ([]*model.User, *model.AppError) { var groupMembers []*model.User query := ` @@ -248,7 +248,7 @@ func (s *SqlGroupStore) GetMemberUsersPage(groupID string, offset int, limit int OFFSET :Offset` - if _, err := s.GetReplica().Select(&groupMembers, query, map[string]interface{}{"GroupId": groupID, "Limit": limit, "Offset": offset}); err != nil { + if _, err := s.GetReplica().Select(&groupMembers, query, map[string]interface{}{"GroupId": groupID, "Limit": perPage, "Offset": page * perPage}); err != nil { return nil, model.NewAppError("SqlGroupStore.GroupGetMemberUsersPage", "store.select_error", nil, err.Error(), http.StatusInternalServerError) } diff --git a/store/store.go b/store/store.go index b78c3d1b6c..1e68eb6b09 100644 --- a/store/store.go +++ b/store/store.go @@ -591,7 +591,7 @@ type GroupStore interface { Delete(groupID string) (*model.Group, *model.AppError) GetMemberUsers(groupID string) ([]*model.User, *model.AppError) - GetMemberUsersPage(groupID string, offset int, limit int) ([]*model.User, *model.AppError) + GetMemberUsersPage(groupID string, page int, perPage int) ([]*model.User, *model.AppError) GetMemberCount(groupID string) (int64, *model.AppError) UpsertMember(groupID string, userID string) (*model.GroupMember, *model.AppError) DeleteMember(groupID string, userID string) (*model.GroupMember, *model.AppError) diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index cbd4442088..76cd9fcbec 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -510,19 +510,30 @@ func testGroupGetMemberUsersPage(t *testing.T, ss store.Store) { _, err = ss.Group().UpsertMember(group.Id, user2.Id) require.Nil(t, err) + u3 := &model.User{ + Email: MakeEmail(), + Username: model.NewId(), + } + user3, err := ss.User().Save(u3) + require.Nil(t, err) + + _, err = ss.Group().UpsertMember(group.Id, user3.Id) + require.Nil(t, err) + // Check returns members groupMembers, err := ss.Group().GetMemberUsersPage(group.Id, 0, 100) require.Nil(t, err) - require.Equal(t, 2, len(groupMembers)) + require.Equal(t, 3, len(groupMembers)) // Check page 1 - groupMembers, err = ss.Group().GetMemberUsersPage(group.Id, 0, 1) + groupMembers, err = ss.Group().GetMemberUsersPage(group.Id, 0, 2) require.Nil(t, err) - require.Equal(t, 1, len(groupMembers)) - require.Equal(t, user2.Id, groupMembers[0].Id) + require.Equal(t, 2, len(groupMembers)) + require.Equal(t, user3.Id, groupMembers[0].Id) + require.Equal(t, user2.Id, groupMembers[1].Id) // Check page 2 - groupMembers, err = ss.Group().GetMemberUsersPage(group.Id, 1, 1) + groupMembers, err = ss.Group().GetMemberUsersPage(group.Id, 1, 2) require.Nil(t, err) require.Equal(t, 1, len(groupMembers)) require.Equal(t, user1.Id, groupMembers[0].Id) @@ -539,7 +550,7 @@ func testGroupGetMemberUsersPage(t *testing.T, ss store.Store) { // Should not return deleted members groupMembers, err = ss.Group().GetMemberUsersPage(group.Id, 0, 100) require.Nil(t, err) - require.Equal(t, 1, len(groupMembers)) + require.Equal(t, 2, len(groupMembers)) } func testUpsertMember(t *testing.T, ss store.Store) { diff --git a/store/storetest/mocks/GroupStore.go b/store/storetest/mocks/GroupStore.go index 3a370fd158..217bafd399 100644 --- a/store/storetest/mocks/GroupStore.go +++ b/store/storetest/mocks/GroupStore.go @@ -577,13 +577,13 @@ func (_m *GroupStore) GetMemberUsers(groupID string) ([]*model.User, *model.AppE return r0, r1 } -// GetMemberUsersPage provides a mock function with given fields: groupID, offset, limit -func (_m *GroupStore) GetMemberUsersPage(groupID string, offset int, limit int) ([]*model.User, *model.AppError) { - ret := _m.Called(groupID, offset, limit) +// GetMemberUsersPage provides a mock function with given fields: groupID, page, perPage +func (_m *GroupStore) GetMemberUsersPage(groupID string, page int, perPage int) ([]*model.User, *model.AppError) { + ret := _m.Called(groupID, page, perPage) var r0 []*model.User if rf, ok := ret.Get(0).(func(string, int, int) []*model.User); ok { - r0 = rf(groupID, offset, limit) + r0 = rf(groupID, page, perPage) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.User) @@ -592,7 +592,7 @@ func (_m *GroupStore) GetMemberUsersPage(groupID string, offset int, limit int) var r1 *model.AppError if rf, ok := ret.Get(1).(func(string, int, int) *model.AppError); ok { - r1 = rf(groupID, offset, limit) + r1 = rf(groupID, page, perPage) } else { if ret.Get(1) != nil { r1 = ret.Get(1).(*model.AppError)