diff options
| -rw-r--r-- | changelogs/unreleased/fix-groups-api-ordering.yml | 4 | ||||
| -rw-r--r-- | lib/api/groups.rb | 4 | ||||
| -rw-r--r-- | spec/requests/api/groups_spec.rb | 32 |
3 files changed, 38 insertions, 2 deletions
diff --git a/changelogs/unreleased/fix-groups-api-ordering.yml b/changelogs/unreleased/fix-groups-api-ordering.yml new file mode 100644 index 00000000000..3a6a7f84356 --- /dev/null +++ b/changelogs/unreleased/fix-groups-api-ordering.yml @@ -0,0 +1,4 @@ +title: Fixed pagination of groups API +merge_request: 19665 +author: Marko, Peter +type: added diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 9d0b55c35c7..c7f41aba854 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -46,7 +46,9 @@ module API groups = GroupsFinder.new(current_user, find_params).execute groups = groups.search(params[:search]) if params[:search].present? groups = groups.where.not(id: params[:skip_groups]) if params[:skip_groups].present? - groups = groups.reorder(params[:order_by] => params[:sort]) + order_options = { params[:order_by] => params[:sort] } + order_options["id"] ||= "asc" + groups = groups.reorder(order_options) groups end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index f5218ebfac2..da23fdd7dca 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -138,12 +138,15 @@ describe API::Groups do context "when using sorting" do let(:group3) { create(:group, name: "a#{group1.name}", path: "z#{group1.path}") } - let(:group4) { create(:group, name: "z#{group1.name}", path: "y#{group1.path}") } + let(:group4) { create(:group, name: "same-name", path: "y#{group1.path}") } + let(:group5) { create(:group, name: "same-name") } let(:response_groups) { json_response.map { |group| group['name'] } } + let(:response_groups_ids) { json_response.map { |group| group['id'] } } before do group3.add_owner(user1) group4.add_owner(user1) + group5.add_owner(user1) end it "sorts by name ascending by default" do @@ -181,6 +184,33 @@ describe API::Groups do expect(json_response).to be_an Array expect(response_groups).to eq(Group.visible_to_user(user1).order(:id).pluck(:name)) end + + it "sorts also by descending id with pagination fix" do + get api("/groups", user1), order_by: "id", sort: "desc" + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(response_groups).to eq(Group.visible_to_user(user1).order(id: :desc).pluck(:name)) + end + + it "sorts identical keys by id for good pagination" do + get api("/groups", user1), search: "same-name", order_by: "name" + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort) + end + + it "sorts descending identical keys by id for good pagination" do + get api("/groups", user1), search: "same-name", order_by: "name", sort: "desc" + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(response_groups_ids).to eq(Group.select { |group| group['name'] == 'same-name' }.map { |group| group['id'] }.sort) + end end context 'when using owned in the request' do |