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

changelogs/unreleased/fix-no-milestone-option-for-projects-endpoint-23194.yml

+---
+title: 'API: fix query response for `/projects/:id/issues?milestone="No%20Milestone"`'
+merge_request: 8457
+author: Panagiotis Atmatzidis, David Eisner

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`  |
 

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

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)