Merge branch '244986-remove-milestone-quick-actions' into 'master'

Remove milestone quick actions for incident issues

Closes #244986

See merge request gitlab-org/gitlab!41882

lib/gitlab/quick_actions/issue_and_merge_request_actions.rb

@@ -91,6 +91,7 @@ module Gitlab
         params '%"milestone"'
         types Issue, MergeRequest
         condition do
+          quick_action_target.supports_milestone? &&
           current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project) &&
             find_milestones(project, state: 'active').any?
         end
@@ -113,6 +114,7 @@ module Gitlab
         condition do
           quick_action_target.persisted? &&
             quick_action_target.milestone_id? &&
+            quick_action_target.supports_milestone? &&
             current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
         end
         command :remove_milestone do

spec/services/notes/quick_actions_service_spec.rb

@@ -30,10 +30,9 @@ RSpec.describe Notes::QuickActionsService do
         end
 
         it 'closes noteable, sets labels, assigns, and sets milestone to noteable, and leave no note' do
-          content, update_params = service.execute(note)
-          service.apply_updates(update_params, note)
+          content = execute(note)
 
-          expect(content).to eq ''
+          expect(content).to be_empty
           expect(note.noteable).to be_closed
           expect(note.noteable.labels).to match_array(labels)
           expect(note.noteable.assignees).to eq([assignee])
@@ -54,19 +53,13 @@ RSpec.describe Notes::QuickActionsService do
           end
 
           it 'does not create issue relation' do
-            expect do
-              _, update_params = service.execute(note)
-              service.apply_updates(update_params, note)
-            end.not_to change { IssueLink.count }
+            expect { execute(note) }.not_to change { IssueLink.count }
           end
         end
 
         context 'user is allowed to relate issues' do
           it 'creates issue relation' do
-            expect do
-              _, update_params = service.execute(note)
-              service.apply_updates(update_params, note)
-            end.to change { IssueLink.count }.by(1)
+            expect { execute(note) }.to change { IssueLink.count }.by(1)
           end
         end
       end
@@ -79,10 +72,9 @@ RSpec.describe Notes::QuickActionsService do
         let(:note_text) { '/reopen' }
 
         it 'opens the noteable, and leave no note' do
-          content, update_params = service.execute(note)
-          service.apply_updates(update_params, note)
+          content = execute(note)
 
-          expect(content).to eq ''
+          expect(content).to be_empty
           expect(note.noteable).to be_open
         end
       end
@@ -92,10 +84,9 @@ RSpec.describe Notes::QuickActionsService do
           let(:note_text) { '/spend 1h' }
 
           it 'adds time to noteable, adds timelog with nil note_id and has no content' do
-            content, update_params = service.execute(note)
-            service.apply_updates(update_params, note)
+            content = execute(note)
 
-            expect(content).to eq ''
+            expect(content).to be_empty
             expect(note.noteable.time_spent).to eq(3600)
             expect(Timelog.last.note_id).to be_nil
           end
@@ -122,8 +113,7 @@ RSpec.describe Notes::QuickActionsService do
         end
 
         it 'closes noteable, sets labels, assigns, and sets milestone to noteable' do
-          content, update_params = service.execute(note)
-          service.apply_updates(update_params, note)
+          content = execute(note)
 
           expect(content).to eq "HELLO\nWORLD"
           expect(note.noteable).to be_closed
@@ -141,14 +131,87 @@ RSpec.describe Notes::QuickActionsService do
         let(:note_text) { "HELLO\n/reopen\nWORLD" }
 
         it 'opens the noteable' do
-          content, update_params = service.execute(note)
-          service.apply_updates(update_params, note)
+          content = execute(note)
 
           expect(content).to eq "HELLO\nWORLD"
           expect(note.noteable).to be_open
         end
       end
     end
+
+    describe '/milestone' do
+      let(:issue) { create(:issue, project: project) }
+      let(:note_text) { %(/milestone %"#{milestone.name}") }
+      let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) }
+
+      context 'on an incident' do
+        before do
+          issue.update!(issue_type: :incident)
+        end
+
+        it 'leaves the note empty' do
+          expect(execute(note)).to be_empty
+        end
+
+        it 'does not assign the milestone' do
+          expect { execute(note) }.not_to change { issue.reload.milestone }
+        end
+      end
+
+      context 'on a merge request' do
+        let(:note_mr) { create(:note_on_merge_request, project: project, note: note_text) }
+
+        it 'leaves the note empty' do
+          expect(execute(note_mr)).to be_empty
+        end
+
+        it 'assigns the milestone' do
+          expect { execute(note) }.to change { issue.reload.milestone }.from(nil).to(milestone)
+        end
+      end
+    end
+
+    describe '/remove_milestone' do
+      let(:issue) { create(:issue, project: project, milestone: milestone) }
+      let(:note_text) { '/remove_milestone' }
+      let(:note) { create(:note_on_issue, noteable: issue, project: project, note: note_text) }
+
+      context 'on an issue' do
+        it 'leaves the note empty' do
+          expect(execute(note)).to be_empty
+        end
+
+        it 'removes the milestone' do
+          expect { execute(note) }.to change { issue.reload.milestone }.from(milestone).to(nil)
+        end
+      end
+
+      context 'on an incident' do
+        before do
+          issue.update!(issue_type: :incident)
+        end
+
+        it 'leaves the note empty' do
+          expect(execute(note)).to be_empty
+        end
+
+        it 'does not remove the milestone' do
+          expect { execute(note) }.not_to change { issue.reload.milestone }
+        end
+      end
+
+      context 'on a merge request' do
+        let(:note_mr) { create(:note_on_merge_request, project: project, note: note_text) }
+
+        it 'leaves the note empty' do
+          expect(execute(note_mr)).to be_empty
+        end
+
+        it 'removes the milestone' do
+          expect { execute(note) }.to change { issue.reload.milestone }.from(milestone).to(nil)
+        end
+      end
+    end
   end
 
   describe '.noteable_update_service' do
@@ -215,7 +278,7 @@ RSpec.describe Notes::QuickActionsService do
     end
 
     it_behaves_like 'note on noteable that supports quick actions' do
-      let_it_be(:merge_request, reload: true) { create(:merge_request, source_project: project) }
+      let(:merge_request) { create(:merge_request, source_project: project) }
       let(:note) { build(:note_on_merge_request, project: project, noteable: merge_request) }
     end
   end
@@ -239,11 +302,17 @@ RSpec.describe Notes::QuickActionsService do
       end
 
       it 'adds only one assignee from the list' do
-        _, update_params = service.execute(note)
-        service.apply_updates(update_params, note)
+        execute(note)
 
         expect(note.noteable.assignees.count).to eq(1)
       end
     end
   end
+
+  def execute(note)
+    content, update_params = service.execute(note)
+    service.apply_updates(update_params, note)
+
+    content
+  end
 end