Merge branch 'issue_213078' into 'master'

Use EpicsFinder when fetching epics

See merge request gitlab-org/gitlab!30404

ee/app/finders/epics_finder.rb

@@ -50,7 +50,8 @@ class EpicsFinder < IssuableFinder
   end
 
   def execute
-    raise ArgumentError, 'group_id argument is missing' unless group
+    raise ArgumentError, 'group_id argument is missing' unless params[:group_id]
+    return Epic.none unless Ability.allowed?(current_user, :read_epic, group)
 
     items = init_collection
     items = by_created_at(items)
@@ -72,19 +73,6 @@ class EpicsFinder < IssuableFinder
     sort(items)
   end
 
-  def group
-    return unless params[:group_id]
-    return @group if defined?(@group)
-
-    group = Group.find(params[:group_id])
-
-    unless Ability.allowed?(current_user, :read_epic, group)
-      raise ActiveRecord::RecordNotFound.new("Could not find a Group with ID #{params[:group_id]}")
-    end
-
-    @group = group
-  end
-
   # rubocop: disable CodeReuse/ActiveRecord
   def init_collection
     groups = if params[:iids].present?
@@ -104,6 +92,13 @@ class EpicsFinder < IssuableFinder
 
   private
 
+  def group
+    return unless params[:group_id]
+    return @group if defined?(@group)
+
+    @group = Group.find(params[:group_id])
+  end
+
   def starts_with_iid(items)
     return items unless params[:iid_starts_with].present?
 

ee/app/services/groups/autocomplete_service.rb

@@ -23,15 +23,11 @@ module Groups
     # rubocop: enable CodeReuse/ActiveRecord
 
     def epics
-      # TODO: change to EpicsFinder once frontend supports epics from external groups.
+      # TODO: use include_descendant_groups: true optional parameter once frontend supports epics from external groups.
       # See https://gitlab.com/gitlab-org/gitlab/issues/6837
-      DeclarativePolicy.user_scope do
-        if Ability.allowed?(current_user, :read_epic, group)
-          group.epics.select(:iid, :title)
-        else
-          []
-        end
-      end
+      EpicsFinder.new(current_user, group_id: group.id)
+        .execute
+        .select(:iid, :title)
     end
 
     # rubocop: disable CodeReuse/ActiveRecord

ee/lib/ee/api/entities/issue.rb

@@ -7,12 +7,18 @@ module EE
         extend ActiveSupport::Concern
 
         prepended do
-          with_options if: -> (issue, options) { ::Ability.allowed?(options[:current_user], :read_epic, issue.project&.group) } do
+          with_options if: -> (issue, _) { issue.project.group&.feature_available?(:epics) } do
             expose :epic_iid do |issue|
-              issue.epic&.iid
+              authorized_epic_for(issue)&.iid
             end
 
-            expose :epic, using: EpicBaseEntity
+            expose :epic, using: EpicBaseEntity do |issue|
+              authorized_epic_for(issue)
+            end
+
+            def authorized_epic_for(issue)
+              issue.epic if ::Ability.allowed?(options[:current_user], :read_epic, issue.epic)
+            end
           end
         end
       end

ee/spec/finders/epics_finder_spec.rb

@@ -43,8 +43,8 @@ describe EpicsFinder do
       end
 
       context 'when user can not read epics of a group' do
-        it 'raises an error when group_id param is missing' do
-          expect { epics }.to raise_error { ArgumentError }
+        it 'returns empty collection' do
+          expect(epics).to be_empty
         end
       end
 

ee/spec/requests/api/issues_spec.rb

@@ -44,19 +44,31 @@ describe API::Issues, :mailer do
         subject
 
         expect(response).to have_gitlab_http_status(:success)
-        expect(epic_issue_response_for(epic_issue)['epic_iid']).to eq(epic.iid)
+        expect(epic_issue_response_for(issue_with_epic)['epic_iid']).to eq(epic.iid)
       end
 
       it 'contains epic in response' do
         subject
 
         expect(response).to have_gitlab_http_status(:success)
-        expect(epic_issue_response_for(epic_issue)['epic']).to eq({ "id" => epic.id,
+        expect(epic_issue_response_for(issue_with_epic)['epic']).to eq({ "id" => epic.id,
                                                                     "iid" => epic.iid,
                                                                     "group_id" => epic.group_id,
                                                                     "title" => epic.title,
                                                                     "url" => group_epic_path(epic.group, epic) })
       end
+
+      context 'and epic issue is not present' do
+        it 'exposes epic as nil' do
+          issue_with_epic.epic_issue.destroy
+
+          subject
+
+          response = epic_issue_response_for(issue_with_epic)
+          expect(response['epic']).to eq(nil)
+          expect(response['epic_id']).to eq(nil)
+        end
+      end
     end
 
     context 'without epics feature' do
@@ -68,14 +80,14 @@ describe API::Issues, :mailer do
         subject
 
         expect(response).to have_gitlab_http_status(:success)
-        expect(epic_issue_response_for(epic_issue)).not_to have_key('epic_iid')
+        expect(epic_issue_response_for(issue_with_epic)).not_to have_key('epic_iid')
       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')
+        expect(epic_issue_response_for(issue_with_epic)).not_to have_key('epic')
       end
     end
   end
@@ -150,7 +162,7 @@ describe API::Issues, :mailer do
     end
 
     include_examples 'exposes epic' do
-      let!(:epic_issue) { create(:issue, project: group_project, epic: epic) }
+      let!(:issue_with_epic) { create(:issue, project: group_project, epic: epic) }
     end
   end
 
@@ -176,7 +188,7 @@ describe API::Issues, :mailer do
     end
 
     context 'on personal project' do
-      let!(:epic_issue) { create(:issue, project: project, epic: epic) }
+      let!(:issue_with_epic) { create(:issue, project: project, epic: epic) }
 
       before do
         stub_licensed_features(epics: true)
@@ -186,12 +198,12 @@ describe API::Issues, :mailer do
         subject
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(epic_issue_response_for(epic_issue)).not_to have_key('epic_iid')
+        expect(epic_issue_response_for(issue_with_epic)).not_to have_key('epic_iid')
       end
     end
 
     context 'on group project' do
-      let!(:epic_issue) { create(:issue, project: group_project, epic: epic) }
+      let!(:issue_with_epic) { create(:issue, project: group_project, epic: epic) }
 
       subject { get api("/projects/#{group_project.id}/issues", user) }
 
@@ -201,9 +213,9 @@ describe API::Issues, :mailer do
 
   describe 'GET /project/:id/issues/:issue_id' do
     context 'on personal project' do
-      let!(:epic_issue) { create(:issue, project: project, epic: epic) }
+      let!(:issue_with_epic) { create(:issue, project: project, epic: epic) }
 
-      subject { get api("/projects/#{project.id}/issues/#{epic_issue.iid}", user) }
+      subject { get api("/projects/#{project.id}/issues/#{issue_with_epic.iid}", user) }
 
       before do
         stub_licensed_features(epics: true)
@@ -213,14 +225,14 @@ describe API::Issues, :mailer do
         subject
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(epic_issue_response_for(epic_issue)).not_to have_key('epic_iid')
+        expect(epic_issue_response_for(issue_with_epic)).not_to have_key('epic_iid')
       end
     end
 
     context 'on group project' do
-      let!(:epic_issue) { create(:issue, project: group_project, epic: epic) }
+      let!(:issue_with_epic) { create(:issue, project: group_project, epic: epic) }
 
-      subject { get api("/projects/#{group_project.id}/issues/#{epic_issue.iid}", user) }
+      subject { get api("/projects/#{group_project.id}/issues/#{issue_with_epic.iid}", user) }
 
       include_examples 'exposes epic'
     end
@@ -399,8 +411,8 @@ describe API::Issues, :mailer do
 
   describe 'PUT /projects/:id/issues/:issue_id to update epic' do
     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 }
+      let(:issue_with_epic) { create(:issue, project: target_project) }
+      let(:request) { put api("/projects/#{target_project.id}/issues/#{issue_with_epic.iid}", user), params: params }
     end
   end
 

ee/spec/services/groups/autocomplete_service_spec.rb

@@ -83,15 +83,19 @@ describe Groups::AutocompleteService do
   end
 
   describe '#epics' do
+    before do
+      stub_licensed_features(epics: true)
+    end
+
     it 'returns nothing if not allowed' do
-      allow(Ability).to receive(:allowed?).with(user, :read_epic, group).and_return(false)
+      guest = create(:user)
 
-      expect(subject.epics).to eq([])
+      epics = described_class.new(group, guest).epics
+
+      expect(epics).to be_empty
     end
 
     it 'returns epics from group' do
-      allow(Ability).to receive(:allowed?).with(user, :read_epic, group).and_return(true)
-
       expect(subject.epics.map(&:iid)).to contain_exactly(epic.iid)
     end
   end