Grapify the issues API

lib/api/helpers.rb

@@ -217,22 +217,6 @@ module API
       end
     end
 
-    def issuable_order_by
-      if params["order_by"] == 'updated_at'
-        'updated_at'
-      else
-        'created_at'
-      end
-    end
-
-    def issuable_sort
-      if params["sort"] == 'asc'
-        :asc
-      else
-        :desc
-      end
-    end
-
     def filter_by_iid(items, iid)
       items.where(iid: iid)
     end

lib/api/issues.rb

 module API
-  # Issues API
   class Issues < Grape::API
+    include PaginationParams
+
     before { authenticate! }
 
     helpers do
@@ -20,77 +21,68 @@ module API
         issues.includes(:milestone).where('milestones.title' => milestone)
       end
 
-      def issue_params
-        new_params = declared(params, include_parent_namespace: false, include_missing: false).to_h
-        new_params = new_params.with_indifferent_access
-        new_params.delete(:id)
-        new_params.delete(:issue_id)
+      params :issues_params do
+        optional :labels, type: String, desc: 'Comma-separated list of label names'
+        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',
+                        desc: 'Return issues sorted in `asc` or `desc` order.'
+        use :pagination
+      end
 
-        new_params
+      params :issue_params do
+        optional :description, type: String, desc: 'The description of an issue'
+        optional :assignee_id, type: Integer, desc: 'The ID of a user to assign issue'
+        optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign issue'
+        optional :labels, type: String, desc: 'Comma-separated list of label names'
+        optional :due_date, type: String, desc: 'Date time string in the format YEAR-MONTH-DAY'
+        optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential'
+        optional :state_event, type: String, values: %w[open close],
+                               desc: 'State of the issue'
       end
     end
 
     resource :issues do
-      # Get currently authenticated user's issues
-      #
-      # Parameters:
-      #   state (optional) - Return "opened" or "closed" issues
-      #   labels (optional) - Comma-separated list of label names
-      #   order_by (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at`
-      #   sort (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
-      #
-      # Example Requests:
-      #   GET /issues
-      #   GET /issues?state=opened
-      #   GET /issues?state=closed
-      #   GET /issues?labels=foo
-      #   GET /issues?labels=foo,bar
-      #   GET /issues?labels=foo,bar&state=opened
+      desc "Get currently authenticated user's issues" do
+        success Entities::Issue
+      end
+      params do
+        optional :state, type: String, values: %w[opened closed all], default: 'all',
+                         desc: 'Return opened, closed, or all issues'
+        use :issues_params
+      end
       get do
         issues = current_user.issues.inc_notes_with_associations
-        issues = filter_issues_state(issues, params[:state]) unless params[:state].nil?
+        issues = filter_issues_state(issues, params[:state])
         issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil?
-        issues = issues.reorder(issuable_order_by => issuable_sort)
+        issues = issues.reorder(params[:order_by] => params[:sort])
 
         present paginate(issues), with: Entities::Issue, current_user: current_user
       end
     end
 
+    params do
+      requires :id, type: String, desc: 'The ID of a group'
+    end
     resource :groups do
-      # Get a list of group issues
-      #
-      # Parameters:
-      #   id (required) - The ID of a group
-      #   state (optional) - Return "opened" or "closed" issues
-      #   labels (optional) - Comma-separated list of label names
-      #   milestone (optional) - Milestone title
-      #   order_by (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at`
-      #   sort (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
-      #
-      # Example Requests:
-      #   GET /groups/:id/issues
-      #   GET /groups/:id/issues?state=opened
-      #   GET /groups/:id/issues?state=closed
-      #   GET /groups/:id/issues?labels=foo
-      #   GET /groups/:id/issues?labels=foo,bar
-      #   GET /groups/:id/issues?labels=foo,bar&state=opened
-      #   GET /groups/:id/issues?milestone=1.0.0
-      #   GET /groups/:id/issues?milestone=1.0.0&state=closed
+      desc 'Get a list of group issues' do
+        success Entities::Issue
+      end
+      params do
+        optional :state, type: String, values: %w[opened closed all], default: 'opened',
+                         desc: 'Return opened, closed, or all issues'
+        use :issues_params
+      end
       get ":id/issues" do
-        group = find_group!(params[:id])
+        group = find_group!(params.delete(:id))
 
-        params[:state] ||= 'opened'
         params[:group_id] = group.id
         params[:milestone_title] = params.delete(:milestone)
         params[:label_name] = params.delete(:labels)
 
-        if params[:order_by] || params[:sort]
-          # The Sortable concern takes 'created_desc', not 'created_at_desc' (for example)
-          params[:sort] = "#{issuable_order_by.sub('_at', '')}_#{issuable_sort}"
-        end
-
         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
@@ -98,32 +90,19 @@ module API
     params do
       requires :id, type: String, desc: 'The ID of a project'
     end
-
     resource :projects do
-      # Get a list of project issues
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   iid (optional) - Return the project issue having the given `iid`
-      #   state (optional) - Return "opened" or "closed" issues
-      #   labels (optional) - Comma-separated list of label names
-      #   milestone (optional) - Milestone title
-      #   order_by (optional) - Return requests ordered by `created_at` or `updated_at` fields. Default is `created_at`
-      #   sort (optional) - Return requests sorted in `asc` or `desc` order. Default is `desc`
-      #
-      # Example Requests:
-      #   GET /projects/:id/issues
-      #   GET /projects/:id/issues?state=opened
-      #   GET /projects/:id/issues?state=closed
-      #   GET /projects/:id/issues?labels=foo
-      #   GET /projects/:id/issues?labels=foo,bar
-      #   GET /projects/:id/issues?labels=foo,bar&state=opened
-      #   GET /projects/:id/issues?milestone=1.0.0
-      #   GET /projects/:id/issues?milestone=1.0.0&state=closed
-      #   GET /issues?iid=42
+      desc 'Get a list of project issues' do
+        success Entities::Issue
+      end
+      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'
+        use :issues_params
+      end
       get ":id/issues" do
         issues = IssuesFinder.new(current_user, project_id: user_project.id).execute.inc_notes_with_associations
-        issues = filter_issues_state(issues, params[:state]) unless params[:state].nil?
+        issues = filter_issues_state(issues, params[:state])
         issues = filter_issues_labels(issues, params[:labels]) unless params[:labels].nil?
         issues = filter_by_iid(issues, params[:iid]) unless params[:iid].nil?
 
@@ -131,59 +110,49 @@ module API
           issues = filter_issues_milestone(issues, params[:milestone])
         end
 
-        issues = issues.reorder(issuable_order_by => issuable_sort)
-
+        issues = issues.reorder(params[:order_by] => params[:sort])
         present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project
       end
 
-      # Get a single project issue
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   issue_id (required) - The ID of a project issue
-      # Example Request:
-      #   GET /projects/:id/issues/:issue_id
+      desc 'Get a single project issue' do
+        success Entities::Issue
+      end
+      params do
+        requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+      end
       get ":id/issues/:issue_id" do
-        @issue = find_project_issue(params[:issue_id])
-        present @issue, with: Entities::Issue, current_user: current_user, project: user_project
+        issue = find_project_issue(params[:issue_id])
+        present issue, with: Entities::Issue, current_user: current_user, project: user_project
       end
 
-      # Create a new project issue
-      #
-      # Parameters:
-      #   id (required)                                      - The ID of a project
-      #   title (required)                                   - The title of an issue
-      #   description (optional)                             - The description of an issue
-      #   assignee_id (optional)                             - The ID of a user to assign issue
-      #   milestone_id (optional)                            - The ID of a milestone to assign issue
-      #   labels (optional)                                  - The labels of an issue
-      #   created_at (optional)                              - Date time string, ISO 8601 formatted
-      #   due_date (optional)                                - Date time string in the format YEAR-MONTH-DAY
-      #   confidential (optional)                            - Boolean parameter if the issue should be confidential
-      #   merge_request_for_resolving_discussions (optional) - The IID of a merge request for which to resolve discussions
-      # Example Request:
-      #   POST /projects/:id/issues
+      desc 'Create a new project issue' do
+        success Entities::Issue
+      end
+      params do
+        requires :title, type: String, desc: 'The title of an issue'
+        optional :created_at, type: DateTime,
+                              desc: 'Date time when the issue was created. Available only for admins and project owners.'
+        optional :merge_request_for_resolving_discussions, type: Integer,
+                                                           desc: 'The IID of a merge request for which to resolve discussions'
+        use :issue_params
+      end
       post ':id/issues' do
-        required_attributes! [:title]
-
-        keys = [:title, :description, :assignee_id, :milestone_id, :due_date, :confidential, :labels, :merge_request_for_resolving_discussions]
-        keys << :created_at if current_user.admin? || user_project.owner == current_user
-        attrs = attributes_for_keys(keys)
+        # Setting created_at time only allowed for admins and project owners
+        unless current_user.admin? || user_project.owner == current_user
+          params.delete(:created_at)
+        end
 
-        attrs[:labels] = params[:labels] if params[:labels]
+        issue_params = declared_params(include_missing: false)
 
         if merge_request_iid = params[:merge_request_for_resolving_discussions]
-          attrs[:merge_request_for_resolving_discussions] = MergeRequestsFinder.new(current_user, project_id: user_project.id).
+          issue_params[:merge_request_for_resolving_discussions] = MergeRequestsFinder.new(current_user, project_id: user_project.id).
             execute.
             find_by(iid: merge_request_iid)
         end
 
-        # Convert and filter out invalid confidential flags
-        attrs['confidential'] = to_boolean(attrs['confidential'])
-        attrs.delete('confidential') if attrs['confidential'].nil?
-
-        issue = ::Issues::CreateService.new(user_project, current_user, attrs.merge(request: request, api: true)).execute
-
+        issue = ::Issues::CreateService.new(user_project,
+                                            current_user,
+                                            issue_params.merge(request: request, api: true)).execute
         if issue.spam?
           render_api_error!({ error: 'Spam detected' }, 400)
         end
@@ -199,31 +168,26 @@ module API
         success Entities::Issue
       end
       params do
-        requires :id, type: String, desc: 'The ID of a project'
-        requires :issue_id, type: Integer, desc: "The ID of a project issue"
-        optional :title, type: String, desc: 'The new title of the issue'
-        optional :description, type: String, desc: 'The description of an issue'
-        optional :assignee_id, type: Integer, desc: 'The ID of a user to assign issue'
-        optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign issue'
-        optional :labels, type: String, desc: 'The labels of an issue'
-        optional :state_event, type: String, values: ['close', 'reopen'], desc: 'The state event of an issue'
-        # TODO 9.0, use the Grape DateTime type here
-        optional :updated_at, type: String, desc: 'Date time string, ISO 8601 formatted'
-        optional :due_date, type: String, desc: 'Date time string in the format YEAR-MONTH-DAY'
-        # TODO 9.0, use the Grape boolean type here
-        optional :confidential, type: String, desc: 'Boolean parameter if the issue should be confidential'
+        requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+        optional :title, type: String, desc: 'The title of an issue'
+        optional :updated_at, type: DateTime,
+                              desc: 'Date time when the issue was updated. Available only for admins and project owners.'
+        use :issue_params
+        at_least_one_of :title, :description, :assignee_id, :milestone_id,
+                        :labels, :created_at, :due_date, :confidential, :state_event
       end
       put ':id/issues/:issue_id' do
-        issue = user_project.issues.find(params[:issue_id])
+        issue = user_project.issues.find(params.delete(:issue_id))
         authorize! :update_issue, issue
 
-        # Convert and filter out invalid confidential flags
-        params[:confidential] = to_boolean(params[:confidential])
-        params.delete(:confidential) if params[:confidential].nil?
-
-        params.delete(:updated_at) unless current_user.admin? || user_project.owner == current_user
+        # Setting created_at time only allowed for admins and project owners
+        unless current_user.admin? || user_project.owner == current_user
+          params.delete(:updated_at)
+        end
 
-        issue = ::Issues::UpdateService.new(user_project, current_user, issue_params).execute(issue)
+        issue = ::Issues::UpdateService.new(user_project,
+                                            current_user,
+                                            declared_params(include_missing: false)).execute(issue)
 
         if issue.valid?
           present issue, with: Entities::Issue, current_user: current_user, project: user_project
@@ -232,19 +196,19 @@ module API
         end
       end
 
-      # Move an existing issue
-      #
-      # Parameters:
-      #  id (required)            - The ID of a project
-      #  issue_id (required)      - The ID of a project issue
-      #  to_project_id (required) - The ID of the new project
-      # Example Request:
-      #   POST /projects/:id/issues/:issue_id/move
+      desc 'Move an existing issue' do
+        success Entities::Issue
+      end
+      params do
+        requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+        requires :to_project_id, type: Integer, desc: 'The ID of the new project'
+      end
       post ':id/issues/:issue_id/move' do
-        required_attributes! [:to_project_id]
+        issue = user_project.issues.find_by(id: params[:issue_id])
+        not_found!('Issue') unless issue
 
-        issue = user_project.issues.find(params[:issue_id])
-        new_project = Project.find(params[:to_project_id])
+        new_project = Project.find_by(id: params[:to_project_id])
+        not_found!('Project') unless new_project
 
         begin
           issue = ::Issues::MoveService.new(user_project, current_user).execute(issue, new_project)
@@ -254,16 +218,13 @@ module API
         end
       end
 
-      #
-      # Delete a project issue
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      #   issue_id (required) - The ID of a project issue
-      # Example Request:
-      #   DELETE /projects/:id/issues/:issue_id
+      desc 'Delete a project issue'
+      params do
+        requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+      end
       delete ":id/issues/:issue_id" do
         issue = user_project.issues.find_by(id: params[:issue_id])
+        not_found!('Issue') unless issue
 
         authorize!(:destroy_issue, issue)
         issue.destroy

spec/requests/api/issues_spec.rb

@@ -72,13 +72,6 @@ describe API::Issues, api: true  do
         expect(json_response.last).to have_key('web_url')
       end
 
-      it "adds pagination headers and keep query params" do
-        get api("/issues?state=closed&per_page=3", user)
-        expect(response.headers['Link']).to eq(
-          '<http://www.example.com/api/v3/issues?page=1&per_page=3&private_token=%s&state=closed>; rel="first", <http://www.example.com/api/v3/issues?page=1&per_page=3&private_token=%s&state=closed>; rel="last"' % [user.private_token, user.private_token]
-        )
-      end
-
       it 'returns an array of closed issues' do
         get api('/issues?state=closed', user)
         expect(response).to have_http_status(200)
@@ -649,9 +642,8 @@ describe API::Issues, api: true  do
       post api("/projects/#{project.id}/issues", user),
         title: 'new issue', confidential: 'foo'
 
-      expect(response).to have_http_status(201)
-      expect(json_response['title']).to eq('new issue')
-      expect(json_response['confidential']).to be_falsy
+      expect(response).to have_http_status(400)
+      expect(json_response['error']).to eq('confidential is invalid')
     end
 
     it "sends notifications for subscribers of newly added labels" do
@@ -862,8 +854,8 @@ describe API::Issues, api: true  do
         put api("/projects/#{project.id}/issues/#{confidential_issue.id}", user),
           confidential: 'foo'
 
-        expect(response).to have_http_status(200)
-        expect(json_response['confidential']).to be_truthy
+        expect(response).to have_http_status(400)
+        expect(json_response['error']).to eq('confidential is invalid')
       end
     end
   end
@@ -985,6 +977,14 @@ describe API::Issues, api: true  do
         expect(json_response['state']).to eq 'opened'
       end
     end
+
+    context 'when issue does not exist' do
+      it 'returns 404 when trying to move an issue' do
+        delete api("/projects/#{project.id}/issues/123", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
   end
 
   describe '/projects/:id/issues/:issue_id/move' do
@@ -1033,6 +1033,7 @@ describe API::Issues, api: true  do
                  to_project_id: target_project.id
 
         expect(response).to have_http_status(404)
+        expect(json_response['message']).to eq('404 Issue Not Found')
       end
     end
 
@@ -1042,6 +1043,7 @@ describe API::Issues, api: true  do
                  to_project_id: target_project.id
 
         expect(response).to have_http_status(404)
+        expect(json_response['message']).to eq('404 Project Not Found')
       end
     end