Commit 57a2ac18 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'more-group-api-sorting-options' into 'master'

Fix group pagination and add sort by id to groups and subgroups

Closes #47409

See merge request gitlab-org/gitlab-ce!19665
parents 74cecce5 24865a4e
title: Fixed pagination of groups API
merge_request: 19665
author: Marko, Peter
type: added
---
title: Added id sorting option to GET groups and subgroups API
merge_request: 19665
author: Marko, Peter
type: added
...@@ -12,7 +12,7 @@ Parameters: ...@@ -12,7 +12,7 @@ Parameters:
| `skip_groups` | array of integers | no | Skip the group IDs passed | | `skip_groups` | array of integers | no | Skip the group IDs passed |
| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) | | `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
| `search` | string | no | Return the list of authorized groups matching the search criteria | | `search` | string | no | Return the list of authorized groups matching the search criteria |
| `order_by` | string | no | Order groups by `name` or `path`. Default is `name` | | `order_by` | string | no | Order groups by `name`, `path` or `id`. Default is `name` |
| `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` | | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
| `statistics` | boolean | no | Include group statistics (admins only) | | `statistics` | boolean | no | Include group statistics (admins only) |
| `with_custom_attributes` | boolean | no | Include [custom attributes](custom_attributes.md) in response (admins only) | | `with_custom_attributes` | boolean | no | Include [custom attributes](custom_attributes.md) in response (admins only) |
...@@ -96,7 +96,7 @@ Parameters: ...@@ -96,7 +96,7 @@ Parameters:
| `skip_groups` | array of integers | no | Skip the group IDs passed | | `skip_groups` | array of integers | no | Skip the group IDs passed |
| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) | | `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
| `search` | string | no | Return the list of authorized groups matching the search criteria | | `search` | string | no | Return the list of authorized groups matching the search criteria |
| `order_by` | string | no | Order groups by `name` or `path`. Default is `name` | | `order_by` | string | no | Order groups by `name`, `path` or `id`. Default is `name` |
| `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` | | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
| `statistics` | boolean | no | Include group statistics (admins only) | | `statistics` | boolean | no | Include group statistics (admins only) |
| `with_custom_attributes` | boolean | no | Include [custom attributes](custom_attributes.md) in response (admins only) | | `with_custom_attributes` | boolean | no | Include [custom attributes](custom_attributes.md) in response (admins only) |
......
...@@ -32,7 +32,7 @@ module API ...@@ -32,7 +32,7 @@ module API
optional :all_available, type: Boolean, desc: 'Show all group that you have access to' optional :all_available, type: Boolean, desc: 'Show all group that you have access to'
optional :search, type: String, desc: 'Search for a specific group' optional :search, type: String, desc: 'Search for a specific group'
optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user' optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user'
optional :order_by, type: String, values: %w[name path], default: 'name', desc: 'Order by name or path' optional :order_by, type: String, values: %w[name path id], default: 'name', desc: 'Order by name, path or id'
optional :sort, type: String, values: %w[asc desc], default: 'asc', desc: 'Sort by asc (ascending) or desc (descending)' optional :sort, type: String, values: %w[asc desc], default: 'asc', desc: 'Sort by asc (ascending) or desc (descending)'
use :pagination use :pagination
end end
...@@ -46,7 +46,9 @@ module API ...@@ -46,7 +46,9 @@ module API
groups = GroupsFinder.new(current_user, find_params).execute groups = GroupsFinder.new(current_user, find_params).execute
groups = groups.search(params[:search]) if params[:search].present? groups = groups.search(params[:search]) if params[:search].present?
groups = groups.where.not(id: params[:skip_groups]) if params[:skip_groups].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 groups
end end
......
...@@ -138,10 +138,15 @@ describe API::Groups do ...@@ -138,10 +138,15 @@ describe API::Groups do
context "when using sorting" do context "when using sorting" do
let(:group3) { create(:group, name: "a#{group1.name}", path: "z#{group1.path}") } let(:group3) { create(:group, name: "a#{group1.name}", path: "z#{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) { json_response.map { |group| group['name'] } }
let(:response_groups_ids) { json_response.map { |group| group['id'] } }
before do before do
group3.add_owner(user1) group3.add_owner(user1)
group4.add_owner(user1)
group5.add_owner(user1)
end end
it "sorts by name ascending by default" do it "sorts by name ascending by default" do
...@@ -150,7 +155,7 @@ describe API::Groups do ...@@ -150,7 +155,7 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq([group3.name, group1.name]) expect(response_groups).to eq(Group.visible_to_user(user1).order(:name).pluck(:name))
end end
it "sorts in descending order when passed" do it "sorts in descending order when passed" do
...@@ -159,16 +164,52 @@ describe API::Groups do ...@@ -159,16 +164,52 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq([group1.name, group3.name]) expect(response_groups).to eq(Group.visible_to_user(user1).order(name: :desc).pluck(:name))
end end
it "sorts by the order_by param" do it "sorts by path in order_by param" do
get api("/groups", user1), order_by: "path" get api("/groups", user1), order_by: "path"
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(response).to include_pagination_headers expect(response).to include_pagination_headers
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(response_groups).to eq([group1.name, group3.name]) expect(response_groups).to eq(Group.visible_to_user(user1).order(:path).pluck(:name))
end
it "sorts by id in the order_by param" do
get api("/groups", user1), order_by: "id"
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).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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment