Merge branch 'winh-issues-api-epic_id-parameter' into 'master'

Expose epic_id parameter in issues API

See merge request gitlab-org/gitlab!19953

app/services/issues/base_service.rb

@@ -36,3 +36,5 @@ module Issues
     end
   end
 end
+
+Issues::BaseService.prepend_if_ee('EE::Issues::BaseService')

doc/api/issues.md

@@ -627,7 +627,8 @@ POST /projects/:id/issues
 | `merge_request_to_resolve_discussions_of` | integer        | no       | The IID of a merge request in which to resolve all issues. This will fill the issue with a default description and mark all discussions as resolved. When passing a description or title, these values will take precedence over the default values.|
 | `discussion_to_resolve`                   | string         | no       | The ID of a discussion to resolve. This will fill in the issue with a default description and mark the discussion as resolved. Use in combination with `merge_request_to_resolve_discussions_of`. |
 | `weight` **(STARTER)**                    | integer        | no       | The weight of the issue. Valid values are greater than or equal to 0. |
-| `epic_iid` **(ULTIMATE)** | integer | no | IID of the epic to add the issue to. Valid values are greater than or equal to 0. |
+| `epic_id` **(ULTIMATE)** | integer | no | ID of the epic to add the issue to. Valid values are greater than or equal to 0. |
+| `epic_iid` **(ULTIMATE)** | integer | no | IID of the epic to add the issue to. Valid values are greater than or equal to 0. (deprecated, [will be removed in 13.0](https://gitlab.com/gitlab-org/gitlab/issues/35157)) |
 
 ```bash
 curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/4/issues?title=Issues%20with%20auth&labels=bug
@@ -729,7 +730,8 @@ PUT /projects/:id/issues/:issue_iid
 | `due_date`     | string  | no       | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11`                                           |
 | `weight` **(STARTER)** | integer | no | The weight of the issue. Valid values are greater than or equal to 0. 0                                                                    |
 | `discussion_locked` | boolean | no  | Flag indicating if the issue's discussion is locked. If the discussion is locked only project members can add or edit comments. |
-| `epic_iid` **(ULTIMATE)** | integer | no | IID of the epic to add the issue to. Valid values are greater than or equal to 0. |
+| `epic_id` **(ULTIMATE)** | integer | no | ID of the epic to add the issue to. Valid values are greater than or equal to 0. |
+| `epic_iid` **(ULTIMATE)** | integer | no | IID of the epic to add the issue to. Valid values are greater than or equal to 0. (deprecated, [will be removed in 13.0](https://gitlab.com/gitlab-org/gitlab/issues/35157)) |
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/4/issues/85?state_event=close

ee/app/services/ee/issues/base_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Issues
+    module BaseService
+      extend ::Gitlab::Utils::Override
+
+      override :filter_params
+      def filter_params(issue)
+        set_epic_param(issue)
+        super
+      end
+
+      private
+
+      def set_epic_param(issue)
+        epic = find_epic(issue)
+        return unless epic
+
+        unless can?(current_user, :admin_epic, epic)
+          raise ::Gitlab::Access::AccessDeniedError
+        end
+
+        params[:epic] = epic
+      end
+
+      def find_epic(issue)
+        id = params.delete(:epic_id)
+        return unless id.present?
+
+        group = issue.project.group
+        return unless group.present?
+
+        EpicsFinder.new(current_user, group_id: group.id,
+                        include_ancestor_groups: true).find(id)
+      end
+    end
+  end
+end

ee/app/services/ee/issues/create_service.rb

@@ -5,18 +5,6 @@ module EE
     module CreateService
       extend ::Gitlab::Utils::Override
 
-      override :filter_params
-      def filter_params(issue)
-        epic_iid = params.delete(:epic_iid)
-        group = issue.project.group
-        if epic_iid.present? && group && can?(current_user, :admin_epic, group)
-          finder = EpicsFinder.new(current_user, group_id: group.id)
-          params[:epic] = finder.find_by!(iid: epic_iid) # rubocop: disable CodeReuse/ActiveRecord
-        end
-
-        super
-      end
-
       override :before_create
       def before_create(issue)
         handle_issue_epic_link(issue)

ee/app/services/ee/issues/update_service.rb

@@ -17,18 +17,6 @@ module EE
         result
       end
 
-      override :filter_params
-      def filter_params(issue)
-        epic_iid = params.delete(:epic_iid)
-        group = issue.project.group
-        if epic_iid.present? && group && can?(current_user, :admin_epic, group)
-          finder = EpicsFinder.new(current_user, group_id: group.id)
-          params[:epic] = finder.find_by!(iid: epic_iid) # rubocop: disable CodeReuse/ActiveRecord
-        end
-
-        super
-      end
-
       private
 
       def handle_epic(issue)

ee/changelogs/unreleased/winh-issues-api-epic_id-parameter.yml

+---
+title: Expose epic_id parameter in issues API
+merge_request: 19953
+author:
+type: changed

ee/lib/ee/api/helpers/common_helpers.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Helpers
+      module CommonHelpers
+        extend ActiveSupport::Concern
+        extend ::Gitlab::Utils::Override
+
+        override :convert_parameters_from_legacy_format
+        def convert_parameters_from_legacy_format(params)
+          params.tap do |params|
+            # this can be removed with API v5, see
+            # https://gitlab.com/gitlab-org/gitlab/issues/35089
+            iid = params.delete(:epic_iid)
+
+            if iid.present?
+              epic = EpicsFinder.new(current_user, group_id: group.id, iids: [iid]).first
+              not_found!('Epic') unless epic
+
+              params[:epic_id] = epic.id
+            end
+          end
+
+          super
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/helpers/issues_helpers.rb

@@ -9,7 +9,9 @@ module EE
         prepended do
           params :optional_issue_params_ee do
             optional :weight, type: Integer, desc: 'The weight of the issue'
-            optional :epic_iid, type: Integer, desc: 'The IID of an epic to associate the issue with'
+            optional :epic_id, type: Integer, desc: 'The ID of an epic to associate the issue with'
+            optional :epic_iid, type: Integer, desc: 'The IID of an epic to associate the issue with (deprecated)'
+            mutually_exclusive :epic_id, :epic_iid
           end
 
           params :optional_issues_params_ee do
@@ -22,7 +24,7 @@ module EE
 
           override :update_params_at_least_one_of
           def update_params_at_least_one_of
-            [*super, :weight, :epic_iid]
+            [*super, :weight, :epic_id, :epic_iid]
           end
 
           override :sort_options

ee/spec/requests/api/issues_spec.rb

@@ -79,51 +79,6 @@ describe API::Issues, :mailer do
     end
   end
 
-  shared_examples 'sets epic_iid' do
-    context 'with epics feature' do
-      before do
-        stub_licensed_features(epics: true)
-      end
-
-      it 'sets epic on issue' do
-        subject
-
-        expect(epic_issue.epic).to eq(epic)
-      end
-    end
-
-    context 'without epics feature' do
-      before do
-        stub_licensed_features(epics: false)
-      end
-
-      it 'does not set epic on issue' do
-        subject
-
-        expect(epic_issue.epic).not_to eq(epic)
-      end
-    end
-  end
-
-  shared_examples 'ignores epic_iid' do
-    before do
-      stub_licensed_features(epics: true)
-    end
-
-    it 'does not contain epic_iid in response' do
-      subject
-
-      expect(response).to have_gitlab_http_status(:success)
-      expect(epic_issue_response_for(epic_issue)).not_to have_key('epic_iid')
-    end
-
-    it 'does not set epic on issue' do
-      subject
-
-      expect(epic_issue.epic).not_to eq(epic)
-    end
-  end
-
   describe "GET /issues" do
     context "when authenticated" do
       it 'matches V4 response schema' do
@@ -270,6 +225,81 @@ describe API::Issues, :mailer do
     end
   end
 
+  shared_examples 'with epic parameter' do
+    let(:params) { { title: 'issue with epic', epic_id: epic.id } }
+
+    context 'for a group project' do
+      let(:target_project) { group_project }
+
+      context 'with epics feature' do
+        before do
+          stub_licensed_features(epics: true)
+        end
+
+        context 'when user can admin epics' do
+          before do
+            group.add_owner(user)
+          end
+
+          it 'sets epic on issue' do
+            request
+
+            expect(response).to have_gitlab_http_status(:success)
+            expect(json_response['epic_iid']).to eq(epic.iid)
+          end
+        end
+
+        context 'when user can not edit epics' do
+          before do
+            group.add_guest(user)
+          end
+
+          it 'returns an error' do
+            request
+
+            expect(response).to have_gitlab_http_status(403)
+            expect(json_response['message']).to eq('403 Forbidden')
+          end
+        end
+      end
+
+      context 'without epics feature' do
+        before do
+          stub_licensed_features(epics: false)
+          group.add_owner(user)
+        end
+
+        it 'does not set epic on issue' do
+          request
+
+          expect(response).to have_gitlab_http_status(404)
+          expect(json_response['message']).to eq('404 Not found')
+        end
+      end
+
+      context 'when both epic_id and epic_iid is used' do
+        let(:params) { { title: 'issue with epic', epic_id: epic.id, epic_iid: epic.iid } }
+
+        it 'returns an error' do
+          request
+
+          expect(response).to have_gitlab_http_status(400)
+        end
+      end
+    end
+
+    context 'for a user project' do
+      let(:target_project) { project }
+
+      it 'does not set epic on issue' do
+        request
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(json_response).not_to have_key('epic_iid')
+      end
+    end
+  end
+
   describe "POST /projects/:id/issues" do
     it 'creates a new project issue' do
       post api("/projects/#{project.id}/issues", user),
@@ -285,26 +315,8 @@ describe API::Issues, :mailer do
       expect(json_response['assignees'].first['name']).to eq(user2.name)
     end
 
-    context 'with epic parameter' do
-      let(:epic_issue) { Issue.last }
-      let(:params) { { title: 'issue with epic', epic_iid: epic.iid } }
-
-      context 'for a group project' do
-        subject { post api("/projects/#{group_project.id}/issues", user), params: params }
-
-        before do
-          group.add_owner(user)
-        end
-
-        include_examples 'exposes epic'
-        include_examples 'sets epic_iid'
-      end
-
-      context 'for a user project' do
-        subject { post api("/projects/#{project.id}/issues", user), params: params }
-
-        include_examples 'ignores epic_iid'
-      end
+    it_behaves_like 'with epic parameter' do
+      let(:request) { post api("/projects/#{target_project.id}/issues", user), params: params }
     end
   end
 
@@ -357,25 +369,9 @@ describe API::Issues, :mailer do
   end
 
   describe 'PUT /projects/:id/issues/:issue_id to update epic' do
-    context 'for a group project' do
-      let!(:epic_issue) { create(:issue, project: group_project) }
-
-      subject { put api("/projects/#{group_project.id}/issues/#{epic_issue.iid}", user), params: { epic_iid: epic.iid } }
-
-      before do
-        group.add_owner(user)
-      end
-
-      include_examples 'exposes epic'
-      include_examples 'sets epic_iid'
-    end
-
-    context 'for a user project' do
-      let!(:epic_issue) { create(:issue, project: project) }
-
-      subject { put api("/projects/#{project.id}/issues/#{epic_issue.iid}", user), params: { epic_iid: epic.iid } }
-
-      include_examples 'ignores epic_iid'
+    it_behaves_like 'with epic parameter' do
+      let(:epic_issue) { create(:issue, project: target_project) }
+      let(:request) { put api("/projects/#{target_project.id}/issues/#{epic_issue.iid}", user), params: params }
     end
   end
 

ee/spec/services/ee/issues/create_service_spec.rb

@@ -6,19 +6,20 @@ describe Issues::CreateService do
   let(:group)   { create(:group) }
   let(:project) { create(:project, group: group) }
   let(:user)    { create(:user) }
-
-  let(:issue) { described_class.new(project, user, opts).execute }
+  let(:epic)    { create(:epic, group: group) }
+  let(:service) { described_class.new(project, user, params) }
 
   before do
     stub_licensed_features(epics: true)
-
-    project.add_maintainer(user)
   end
 
   context 'quick actions' do
+    before do
+      project.add_maintainer(user)
+    end
+
     context '/epic action' do
-      let(:epic) { create(:epic, group: group) }
-      let(:opts) do
+      let(:params) do
         {
           title: 'New issue',
           description: "/epic #{epic.to_reference(project)}"
@@ -26,6 +27,8 @@ describe Issues::CreateService do
       end
 
       it 'adds an issue to the passed epic' do
+        issue = service.execute
+
         expect(issue).to be_persisted
         expect(issue.epic).to eq(epic)
       end
@@ -36,5 +39,9 @@ describe Issues::CreateService do
     it_behaves_like 'new issuable with scoped labels' do
       let(:parent) { project }
     end
+
+    it_behaves_like 'issue with epic_id parameter' do
+      let(:execute) { service.execute }
+    end
   end
 end

ee/spec/services/ee/issues/update_service_spec.rb

@@ -129,7 +129,7 @@ describe Issues::UpdateService do
         end
       end
 
-      context 'when issue does belongs to an epic' do
+      context 'when issue belongs to an epic' do
         before do
           issue.update!(epic: epic)
         end
@@ -154,5 +154,10 @@ describe Issues::UpdateService do
       let(:issuable) { issue }
       let(:parent) { project }
     end
+
+    it_behaves_like 'issue with epic_id parameter' do
+      let(:execute) { described_class.new(project, user, params).execute(issue) }
+      let(:epic) { create(:epic, group: group) }
+    end
   end
 end

ee/spec/support/shared_examples/services/issue_epic_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'issue with epic_id parameter' do
+  before do
+    stub_licensed_features(epics: true)
+  end
+
+  context 'when epic_id does not exist' do
+    let(:params) { { title: 'issue1', epic_id: -1 } }
+
+    it 'raises an exception' do
+      expect { execute }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
+  context 'when user can not add issues to the epic' do
+    before do
+      project.add_maintainer(user)
+    end
+
+    let(:params) { { title: 'issue1', epic_id: epic.id } }
+
+    it 'raises an exception' do
+      expect { execute }.to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+
+  context 'when user can add issues to the epic' do
+    before do
+      group.add_owner(user)
+      project.add_maintainer(user)
+    end
+
+    let(:params) { { title: 'issue1', epic_id: epic.id } }
+
+    it 'creates epic issue link' do
+      issue = execute
+
+      expect(issue).to be_persisted
+      expect(issue.epic).to eq(epic)
+    end
+  end
+end

lib/api/helpers/common_helpers.rb

@@ -15,3 +15,5 @@ module API
     end
   end
 end
+
+API::Helpers::CommonHelpers.prepend_if_ee('EE::API::Helpers::CommonHelpers')