Merge branch '12687-follow-up-from-allow-bulk-update-for-group-issues-milestones' into 'master'

Add parent argument to Bulk Update Service

Closes #12687

See merge request gitlab-org/gitlab!20259

app/controllers/concerns/issuable_actions.rb

@@ -121,7 +121,7 @@ module IssuableActions
   end
 
   def bulk_update
-    result = Issuable::BulkUpdateService.new(current_user, bulk_update_params).execute(resource_name)
+    result = Issuable::BulkUpdateService.new(parent, current_user, bulk_update_params).execute(resource_name)
     quantity = result[:count]
 
     render json: { notice: "#{quantity} #{resource_name.pluralize(quantity)} updated" }

app/services/issuable/bulk_update_service.rb

@@ -4,19 +4,18 @@ module Issuable
   class BulkUpdateService
     include Gitlab::Allowable
 
-    attr_accessor :current_user, :params
+    attr_accessor :parent, :current_user, :params
 
-    def initialize(user = nil, params = {})
-      @current_user, @params = user, params.dup
+    def initialize(parent, user = nil, params = {})
+      @parent, @current_user, @params = parent, user, params.dup
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def execute(type)
       model_class = type.classify.constantize
       update_class = type.classify.pluralize.constantize::UpdateService
 
       ids = params.delete(:issuable_ids).split(",")
-      items = model_class.where(id: ids)
+      items = find_issuables(parent, model_class, ids)
 
       permitted_attrs(type).each do |key|
         params.delete(key) unless params[key].present?
@@ -37,7 +36,6 @@ module Issuable
         success:  !items.count.zero?
       }
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     private
 
@@ -50,5 +48,15 @@ module Issuable
         attrs.push(:assignee_id)
       end
     end
+
+    def find_issuables(parent, model_class, ids)
+      if parent.is_a?(Project)
+        model_class.id_in(ids).of_projects(parent)
+      elsif parent.is_a?(Group)
+        model_class.id_in(ids).of_projects(parent.all_projects)
+      end
+    end
   end
 end
+
+Issuable::BulkUpdateService.prepend_if_ee('EE::Issuable::BulkUpdateService')

ee/app/services/ee/issuable/bulk_update_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Issuable
+    module BulkUpdateService
+      extend ::Gitlab::Utils::Override
+
+      private
+
+      override :find_issuables
+      def find_issuables(parent, model_class, ids)
+        return model_class.for_ids(ids).in_selected_groups(parent.self_and_descendants) if model_class == ::Epic
+
+        super
+      end
+    end
+  end
+end

ee/spec/services/ee/issuable/bulk_update_service_spec.rb

@@ -7,12 +7,17 @@ describe Issuable::BulkUpdateService do
   let(:group) { create(:group) }
 
   context 'with epics' do
-    subject { described_class.new(user, params).execute('epic') }
+    subject { described_class.new(group, user, params).execute('epic') }
 
     let(:epic1) { create(:epic, group: group, labels: [label1]) }
     let(:epic2) { create(:epic, group: group, labels: [label1]) }
     let(:label1) { create(:group_label, group: group) }
 
+    before do
+      group.add_reporter(user)
+      stub_licensed_features(epics: true)
+    end
+
     describe 'updating labels' do
       let(:label2) { create(:group_label, group: group, title: 'Bug') }
       let(:label3) { create(:group_label, group: group, title: 'suggestion') }
@@ -26,9 +31,19 @@ describe Issuable::BulkUpdateService do
         }
       end
 
+      context 'when epics are enabled' do
+        it 'updates epic labels' do
+          expect(subject[:success]).to be_truthy
+          expect(subject[:count]).to eq(issuables.count)
+
+          issuables.each do |issuable|
+            expect(issuable.reload.labels).to eq([label2, label3])
+          end
+        end
+      end
+
       context 'when epics are disabled' do
         before do
-          group.add_reporter(user)
           stub_licensed_features(epics: false)
         end
 
@@ -39,21 +54,18 @@ describe Issuable::BulkUpdateService do
         end
       end
 
-      context 'when epics are enabled' do
-        before do
-          group.add_reporter(user)
-          stub_licensed_features(epics: true)
-        end
-
-        it 'updates epic labels' do
-          result = subject
+      context 'when issuable_ids contain external epics' do
+        let(:epic3) { create(:epic, group: create(:group, parent: group), labels: [label1]) }
+        let(:outer_epic) { create(:epic, labels: [label1]) }
+        let(:params) { { issuable_ids: [epic1.id, epic3.id, outer_epic.id], add_label_ids: [label3.id] } }
 
-          expect(result[:success]).to be_truthy
-          expect(result[:count]).to eq(issuables.count)
+        it 'updates epics that belong to the parent group or descendants' do
+          expect(subject[:success]).to be_truthy
+          expect(subject[:count]).to eq(2)
 
-          issuables.each do |issuable|
-            expect(issuable.reload.labels).to eq([label2, label3])
-          end
+          expect(epic1.reload.labels).to eq([label1, label3])
+          expect(epic3.reload.labels).to eq([label1, label3])
+          expect(outer_epic.reload.labels).to eq([label1])
         end
       end
     end

spec/services/issuable/bulk_update_service_spec.rb

@@ -11,7 +11,7 @@ describe Issuable::BulkUpdateService do
       .reverse_merge(issuable_ids: Array(issuables).map(&:id).join(','))
 
     type = Array(issuables).first.model_name.param_key
-    Issuable::BulkUpdateService.new(user, bulk_update_params).execute(type)
+    Issuable::BulkUpdateService.new(parent, user, bulk_update_params).execute(type)
   end
 
   shared_examples 'updates milestones' do
@@ -184,6 +184,8 @@ describe Issuable::BulkUpdateService do
   end
 
   context 'with issuables at a project level' do
+    let(:parent) { project }
+
     describe 'close issues' do
       let(:issues) { create_list(:issue, 2, project: project) }
 
@@ -200,33 +202,6 @@ describe Issuable::BulkUpdateService do
         expect(project.issues.opened).to be_empty
         expect(project.issues.closed).not_to be_empty
       end
-
-      context 'when issue for a different project is created' do
-        let(:private_project) { create(:project, :private) }
-        let(:issue) { create(:issue, project: private_project, author: user) }
-
-        context 'when user has access to the project' do
-          it 'closes all issues passed' do
-            private_project.add_maintainer(user)
-
-            bulk_update(issues + [issue], state_event: 'close')
-
-            expect(project.issues.opened).to be_empty
-            expect(project.issues.closed).not_to be_empty
-            expect(private_project.issues.closed).not_to be_empty
-          end
-        end
-
-        context 'when user does not have access to project' do
-          it 'only closes all issues that the user has access to' do
-            bulk_update(issues + [issue], state_event: 'close')
-
-            expect(project.issues.opened).to be_empty
-            expect(project.issues.closed).not_to be_empty
-            expect(private_project.issues.closed).to be_empty
-          end
-        end
-      end
     end
 
     describe 'reopen issues' do
@@ -362,10 +337,29 @@ describe Issuable::BulkUpdateService do
         end
       end
     end
+
+    describe 'updating issues from external project' do
+      it 'updates only issues that belong to the parent project' do
+        issue1 = create(:issue, project: project)
+        issue2 = create(:issue, project: create(:project))
+        result = bulk_update([issue1, issue2], assignee_ids: [user.id])
+
+        expect(result[:success]).to be_truthy
+        expect(result[:count]).to eq(1)
+
+        expect(issue1.reload.assignees).to eq([user])
+        expect(issue2.reload.assignees).to be_empty
+      end
+    end
   end
 
   context 'with issuables at a group level' do
     let(:group) { create(:group) }
+    let(:parent) { group }
+
+    before do
+      group.add_reporter(user)
+    end
 
     describe 'updating milestones' do
       let(:milestone) { create(:milestone, group: group) }
@@ -398,11 +392,24 @@ describe Issuable::BulkUpdateService do
       let(:regression)     { create(:group_label, group: group) }
       let(:merge_requests) { create(:group_label, group: group) }
 
-      before do
-        group.add_reporter(user)
+      it_behaves_like 'updating labels'
     end
 
-      it_behaves_like 'updating labels'
+    describe 'with issues from external group' do
+      it 'updates issues that belong to the parent group or descendants' do
+        issue1 = create(:issue, project: create(:project, group: group))
+        issue2 = create(:issue, project: create(:project, group: create(:group)))
+        issue3 = create(:issue, project: create(:project, group: create(:group, parent: group)))
+        milestone = create(:milestone, group: group)
+        result = bulk_update([issue1, issue2, issue3], milestone_id: milestone.id)
+
+        expect(result[:success]).to be_truthy
+        expect(result[:count]).to eq(2)
+
+        expect(issue1.reload.milestone).to eq(milestone)
+        expect(issue2.reload.milestone).to be_nil
+        expect(issue3.reload.milestone).to eq(milestone)
+      end
     end
   end
 end