diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-01-13 08:18:39 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-01-13 08:18:39 +0000 |
commit | 7ddeefe02519fdc59917f0101eb0babad25ed98d (patch) | |
tree | ad0b99ba5445a1b18fcd2a6d841e49514e001e7e | |
parent | f80ab37cc1450fb9dc61149dbc10c245f3681e45 (diff) | |
parent | 7ef1c6408e95c780e947e5c56cfae10b3dbb03d4 (diff) | |
download | gitlab-ce-7ddeefe02519fdc59917f0101eb0babad25ed98d.tar.gz |
Merge branch '23194-fix-no-milestone-option-for-projects-endpoint' into 'master'
Fixes the incoherent filtering by milestone in `/projects/:id/issues` and `/groups/:id/issues` endpoints
Closes #23194
See merge request !8457
-rw-r--r-- | changelogs/unreleased/fix-no-milestone-option-for-projects-endpoint-23194.yml | 4 | ||||
-rw-r--r-- | doc/api/issues.md | 3 | ||||
-rw-r--r-- | lib/api/issues.rb | 49 | ||||
-rw-r--r-- | spec/requests/api/issues_spec.rb | 80 |
4 files changed, 112 insertions, 24 deletions
diff --git a/changelogs/unreleased/fix-no-milestone-option-for-projects-endpoint-23194.yml b/changelogs/unreleased/fix-no-milestone-option-for-projects-endpoint-23194.yml new file mode 100644 index 00000000000..98066537723 --- /dev/null +++ b/changelogs/unreleased/fix-no-milestone-option-for-projects-endpoint-23194.yml @@ -0,0 +1,4 @@ +--- +title: 'API: fix query response for `/projects/:id/issues?milestone="No%20Milestone"`' +merge_request: 8457 +author: Panagiotis Atmatzidis, David Eisner diff --git a/doc/api/issues.md b/doc/api/issues.md index 119125bcd3d..dd84afd7c73 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -23,12 +23,15 @@ GET /issues?state=closed GET /issues?labels=foo GET /issues?labels=foo,bar GET /issues?labels=foo,bar&state=opened +GET /issues?milestone=1.0.0 +GET /issues?milestone=1.0.0&state=opened ``` | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `state` | string | no | Return all issues or just those that are `opened` or `closed`| | `labels` | string | no | Comma-separated list of label names, issues with any of the labels will be returned | +| `milestone` | string| no | The milestone title | | `order_by`| string | no | Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at` | | `sort` | string | no | Return requests sorted in `asc` or `desc` order. Default is `desc` | diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 54b97402426..161269cbd41 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -5,13 +5,31 @@ module API before { authenticate! } helpers do - # TODO: Remove in 9.0 and switch to IssueFinder-based label filtering - def filter_issues_labels(issues, labels) - issues.includes(:labels).where('labels.title' => labels.split(',')) + def find_issues(args = {}) + args = params.merge(args) + + args.delete(:id) + args[:milestone_title] = args.delete(:milestone) + + match_all_labels = args.delete(:match_all_labels) + labels = args.delete(:labels) + args[:label_name] = labels if match_all_labels + + args[:search] = "#{Issue.reference_prefix}#{args.delete(:iid)}" if args.key?(:iid) + + issues = IssuesFinder.new(current_user, args).execute.inc_notes_with_associations + + # TODO: Remove in 9.0 pass `label_name: args.delete(:labels)` to IssuesFinder + if !match_all_labels && labels.present? + issues = issues.includes(:labels).where('labels.title' => labels.split(',')) + end + + issues.reorder(args[:order_by] => args[:sort]) end params :issues_params do optional :labels, type: String, desc: 'Comma-separated list of label names' + optional :milestone, type: String, desc: 'Milestone title' optional :order_by, type: String, values: %w[created_at updated_at], default: 'created_at', desc: 'Return issues ordered by `created_at` or `updated_at` fields.' optional :sort, type: String, values: %w[asc desc], default: 'desc', @@ -40,9 +58,7 @@ module API use :issues_params end get do - issues = IssuesFinder.new(current_user, scope: 'all', author_id: current_user.id, state: params[:state]).execute.inc_notes_with_associations - issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? - issues = issues.reorder(params[:order_by] => params[:sort]) + issues = find_issues(scope: 'authored') present paginate(issues), with: Entities::Issue, current_user: current_user end @@ -61,15 +77,10 @@ module API use :issues_params end get ":id/issues" do - group = find_group!(params.delete(:id)) + group = find_group!(params[:id]) - params[:group_id] = group.id - params[:milestone_title] = params.delete(:milestone) - params[:label_name] = params.delete(:labels) + issues = find_issues(group_id: group.id, state: params[:state] || 'opened', match_all_labels: true) - issues = IssuesFinder.new(current_user, params).execute - - issues = issues.reorder(params[:order_by] => params[:sort]) present paginate(issues), with: Entities::Issue, current_user: current_user end end @@ -84,17 +95,13 @@ module API params do optional :state, type: String, values: %w[opened closed all], default: 'all', desc: 'Return opened, closed, or all issues' - optional :iid, type: Integer, desc: 'The IID of the issue' + optional :iid, type: Integer, desc: 'Return the issue having the given `iid`' use :issues_params end get ":id/issues" do - issues = IssuesFinder.new(current_user, - project_id: user_project.id, - state: params[:state], - milestone_title: params[:milestone]).execute.inc_notes_with_associations - issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil? - issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil? - issues = issues.reorder(params[:order_by] => params[:sort]) + project = find_project(params[:id]) + + issues = find_issues(project_id: project.id) present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index a786dc9edb3..432ba8f0f26 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -107,6 +107,7 @@ describe API::Issues, api: true do it 'returns an array of labeled issues when at least one label matches' do get api("/issues?labels=#{label.title},foo,bar", user) + expect(response).to have_http_status(200) expect(json_response).to be_an Array expect(json_response.length).to eq(1) @@ -136,6 +137,51 @@ describe API::Issues, api: true do expect(json_response.length).to eq(0) end + it 'returns an empty array if no issue matches milestone' do + get api("/issues?milestone=#{empty_milestone.title}", user) + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(0) + end + + it 'returns an empty array if milestone does not exist' do + get api("/issues?milestone=foo", user) + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(0) + end + + it 'returns an array of issues in given milestone' do + get api("/issues?milestone=#{milestone.title}", user) + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(2) + expect(json_response.first['id']).to eq(issue.id) + expect(json_response.second['id']).to eq(closed_issue.id) + end + + it 'returns an array of issues matching state in milestone' do + get api("/issues?milestone=#{milestone.title}"\ + '&state=closed', user) + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(1) + expect(json_response.first['id']).to eq(closed_issue.id) + end + + it 'returns an array of issues with no milestone' do + get api("/issues?milestone=#{Milestone::None.title}", author) + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(1) + expect(json_response.first['id']).to eq(confidential_issue.id) + end + it 'sorts by created_at descending by default' do get api('/issues', user) response_dates = json_response.map { |issue| issue['created_at'] } @@ -318,6 +364,15 @@ describe API::Issues, api: true do expect(json_response.first['id']).to eq(group_closed_issue.id) end + it 'returns an array of issues with no milestone' do + get api("#{base_url}?milestone=#{Milestone::None.title}", user) + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(1) + expect(json_response.first['id']).to eq(group_confidential_issue.id) + end + it 'sorts by created_at descending by default' do get api(base_url, user) response_dates = json_response.map { |issue| issue['created_at'] } @@ -357,7 +412,6 @@ describe API::Issues, api: true do describe "GET /projects/:id/issues" do let(:base_url) { "/projects/#{project.id}" } - let(:title) { milestone.title } it "returns 404 on private projects for other users" do private_project = create(:empty_project, :private) @@ -433,8 +487,9 @@ describe API::Issues, api: true do expect(json_response.first['labels']).to eq([label.title]) end - it 'returns an array of labeled project issues when at least one label matches' do + it 'returns an array of labeled project issues where all labels match' do get api("#{base_url}/issues?labels=#{label.title},foo,bar", user) + expect(response).to have_http_status(200) expect(json_response).to be_an Array expect(json_response.length).to eq(1) @@ -463,7 +518,8 @@ describe API::Issues, api: true do end it 'returns an array of issues in given milestone' do - get api("#{base_url}/issues?milestone=#{title}", user) + get api("#{base_url}/issues?milestone=#{milestone.title}", user) + expect(response).to have_http_status(200) expect(json_response).to be_an Array expect(json_response.length).to eq(2) @@ -480,6 +536,15 @@ describe API::Issues, api: true do expect(json_response.first['id']).to eq(closed_issue.id) end + it 'returns an array of issues with no milestone' do + get api("#{base_url}/issues?milestone=#{Milestone::None.title}", user) + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(1) + expect(json_response.first['id']).to eq(confidential_issue.id) + end + it 'sorts by created_at descending by default' do get api("#{base_url}/issues", user) response_dates = json_response.map { |issue| issue['created_at'] } @@ -547,12 +612,21 @@ describe API::Issues, api: true do it 'returns a project issue by iid' do get api("/projects/#{project.id}/issues?iid=#{issue.iid}", user) + expect(response.status).to eq 200 + expect(json_response.length).to eq 1 expect(json_response.first['title']).to eq issue.title expect(json_response.first['id']).to eq issue.id expect(json_response.first['iid']).to eq issue.iid end + it 'returns an empty array for an unknown project issue iid' do + get api("/projects/#{project.id}/issues?iid=#{issue.iid + 10}", user) + + expect(response.status).to eq 200 + expect(json_response.length).to eq 0 + end + it "returns 404 if issue id not found" do get api("/projects/#{project.id}/issues/54321", user) expect(response).to have_http_status(404) |