Merge branch 'system-note-and-events-on-issue-create' into 'master'

Fix missing system notes and system events on create issue [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!61746

app/services/issuable_base_service.rb

@@ -184,7 +184,7 @@ class IssuableBaseService < ::BaseProjectService
       params[:assignee_ids] = process_assignee_ids(params, extra_assignee_ids: issuable.assignee_ids.to_a)
     end
 
-    issuable.assign_attributes(params)
+    issuable.assign_attributes(allowed_create_params(params))
 
     before_create(issuable)
 
@@ -194,6 +194,7 @@ class IssuableBaseService < ::BaseProjectService
 
     if issuable_saved
       create_system_notes(issuable, is_update: false) unless skip_system_notes
+      handle_changes(issuable, { params: params })
 
       after_create(issuable)
       execute_hooks(issuable)
@@ -233,7 +234,7 @@ class IssuableBaseService < ::BaseProjectService
     assign_requested_assignees(issuable)
 
     if issuable.changed? || params.present?
-      issuable.assign_attributes(params)
+      issuable.assign_attributes(allowed_update_params(params))
 
       if has_title_or_description_changed?(issuable)
         issuable.assign_attributes(last_edited_at: Time.current, last_edited_by: current_user)
@@ -260,7 +261,7 @@ class IssuableBaseService < ::BaseProjectService
           issuable, old_labels: old_associations[:labels], old_milestone: old_associations[:milestone]
         )
 
-        handle_changes(issuable, old_associations: old_associations)
+        handle_changes(issuable, old_associations: old_associations, params: params)
 
         new_assignees = issuable.assignees.to_a
         affected_assignees = (old_associations[:assignees] + new_assignees) - (old_associations[:assignees] & new_assignees)
@@ -505,6 +506,14 @@ class IssuableBaseService < ::BaseProjectService
   def update_timestamp?(issuable)
     issuable.changes.keys != ["relative_position"]
   end
+
+  def allowed_create_params(params)
+    params
+  end
+
+  def allowed_update_params(params)
+    params
+  end
 end
 
 IssuableBaseService.prepend_mod_with('IssuableBaseService')

app/services/issues/create_service.rb

@@ -40,6 +40,20 @@ module Issues
       super
     end
 
+    def handle_changes(issue, options)
+      super
+      old_associations = options.fetch(:old_associations, {})
+      old_assignees = old_associations.fetch(:assignees, [])
+
+      handle_assignee_changes(issue, old_assignees)
+    end
+
+    def handle_assignee_changes(issue, old_assignees)
+      return if issue.assignees == old_assignees
+
+      create_assignee_note(issue, old_assignees)
+    end
+
     def resolve_discussions_with_issue(issue)
       return if discussions_to_resolve.empty?
 

app/services/issues/update_service.rb

@@ -43,6 +43,7 @@ module Issues
     end
 
     def handle_changes(issue, options)
+      super
       old_associations = options.fetch(:old_associations, {})
       old_labels = old_associations.fetch(:labels, [])
       old_mentioned_users = old_associations.fetch(:mentioned_users, [])

app/services/merge_requests/base_service.rb

@@ -27,6 +27,33 @@ module MergeRequests
       enqueue_jira_connect_messages_for(merge_request)
     end
 
+    def handle_changes(merge_request, options)
+      old_associations = options.fetch(:old_associations, {})
+      old_assignees = old_associations.fetch(:assignees, [])
+      old_reviewers = old_associations.fetch(:reviewers, [])
+
+      handle_assignees_change(merge_request, old_assignees) if merge_request.assignees != old_assignees
+      handle_reviewers_change(merge_request, old_reviewers) if merge_request.reviewers != old_reviewers
+    end
+
+    def handle_assignees_change(merge_request, old_assignees)
+      MergeRequests::HandleAssigneesChangeService
+        .new(project: project, current_user: current_user)
+        .async_execute(merge_request, old_assignees)
+    end
+
+    def handle_reviewers_change(merge_request, old_reviewers)
+      affected_reviewers = (old_reviewers + merge_request.reviewers) - (old_reviewers & merge_request.reviewers)
+      create_reviewer_note(merge_request, old_reviewers)
+      notification_service.async.changed_reviewer_of_merge_request(merge_request, current_user, old_reviewers)
+      todo_service.reassigned_reviewable(merge_request, current_user, old_reviewers)
+      invalidate_cache_counts(merge_request, users: affected_reviewers.compact)
+
+      new_reviewers = merge_request.reviewers - old_reviewers
+      merge_request_activity_counter.track_users_review_requested(users: new_reviewers)
+      merge_request_activity_counter.track_reviewers_changed_action(user: current_user)
+    end
+
     def cleanup_environments(merge_request)
       Ci::StopEnvironmentsService.new(merge_request.source_project, current_user)
                                  .execute_for_merge_request(merge_request)

app/services/merge_requests/update_service.rb

@@ -15,6 +15,7 @@ module MergeRequests
     end
 
     def handle_changes(merge_request, options)
+      super
       old_associations = options.fetch(:old_associations, {})
       old_labels = old_associations.fetch(:labels, [])
       old_mentioned_users = old_associations.fetch(:mentioned_users, [])
@@ -31,8 +32,6 @@ module MergeRequests
       end
 
       handle_target_branch_change(merge_request)
-      handle_assignees_change(merge_request, old_assignees) if merge_request.assignees != old_assignees
-      handle_reviewers_change(merge_request, old_reviewers) if merge_request.reviewers != old_reviewers
       handle_milestone_change(merge_request)
       handle_draft_status_change(merge_request, changed_fields)
 
@@ -220,24 +219,6 @@ module MergeRequests
       end
     end
 
-    def handle_assignees_change(merge_request, old_assignees)
-      MergeRequests::HandleAssigneesChangeService
-        .new(project: project, current_user: current_user)
-        .async_execute(merge_request, old_assignees)
-    end
-
-    def handle_reviewers_change(merge_request, old_reviewers)
-      affected_reviewers = (old_reviewers + merge_request.reviewers) - (old_reviewers & merge_request.reviewers)
-      create_reviewer_note(merge_request, old_reviewers)
-      notification_service.async.changed_reviewer_of_merge_request(merge_request, current_user, old_reviewers)
-      todo_service.reassigned_reviewable(merge_request, current_user, old_reviewers)
-      invalidate_cache_counts(merge_request, users: affected_reviewers.compact)
-
-      new_reviewers = merge_request.reviewers - old_reviewers
-      merge_request_activity_counter.track_users_review_requested(users: new_reviewers)
-      merge_request_activity_counter.track_reviewers_changed_action(user: current_user)
-    end
-
     def create_branch_change_note(issuable, branch_type, event_type, old_branch, new_branch)
       SystemNoteService.change_branch(
         issuable, issuable.project, current_user, branch_type, event_type,

ee/app/services/ee/issuable_base_service.rb

@@ -58,5 +58,15 @@ module EE
         end
       end
     end
+
+    override :allowed_create_params
+    def allowed_create_params(params)
+      super(params).except(:epic)
+    end
+
+    override :allowed_update_params
+    def allowed_update_params(params)
+      super(params).except(:epic)
+    end
   end
 end

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

@@ -7,21 +7,16 @@ module EE
 
       class EpicAssignmentError < ::ArgumentError; end
 
-      def handle_epic(issue)
+      def filter_epic(issue)
         return unless epic_param_present?
 
         epic = epic_param(issue)
-        result = epic ? assign_epic(issue, epic) : unassign_epic(issue)
-        issue.reload_epic
 
-        if result[:status] == :error
-          raise EpicAssignmentError, result[:message]
-        end
+        params[:confidential] = true if !issue.persisted? && epic&.confidential
+        params[:epic] = epic
       end
 
       def assign_epic(issue, epic)
-        issue.confidential = true if !issue.persisted? && epic.confidential
-
         had_epic = issue.epic.present?
 
         link_params = { target_issuable: issue, skip_epic_dates_update: true }
@@ -86,6 +81,32 @@ module EE
 
         params[:sprint_id] = '' unless iteration
       end
+
+      override :handle_changes
+      def handle_changes(issue, options)
+        handle_epic_change(issue, options)
+      end
+
+      def handle_epic_change(issue, options)
+        return unless epic_param_present?
+
+        old_epic = issue.epic
+        epic = options.dig(:params, :epic)
+
+        return if !epic && !old_epic
+
+        result = if old_epic && !epic
+                   unassign_epic(issue)
+                 else
+                   assign_epic(issue, epic)
+                 end
+
+        issue.reload_epic
+
+        if result[:status] == :error
+          raise EpicAssignmentError, result[:message]
+        end
+      end
     end
   end
 end

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

@@ -7,7 +7,7 @@ module EE
 
       override :filter_params
       def filter_params(issue)
-        handle_epic(issue)
+        filter_epic(issue)
 
         super
       end

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

@@ -10,7 +10,7 @@ module EE
       def filter_params(issue)
         params.delete(:sprint_id) unless can_admin_issuable?(issue)
 
-        handle_epic(issue)
+        filter_epic(issue)
         filter_iteration
 
         super

ee/spec/features/issues/user_bulk_edits_issues_spec.rb

@@ -167,6 +167,11 @@ RSpec.describe 'Issues > Bulk edit issues' do
     end
 
     context 'at group level' do
+      before do
+        # avoid raising QueryLimiting exception for bulk inserts
+        stub_const("::Gitlab::QueryLimiting::Transaction::THRESHOLD", 110)
+      end
+
       it_behaves_like 'bulk edit option in sidebar', :group
       it_behaves_like 'bulk edit epic', :group
       it_behaves_like 'bulk edit health status', :group

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

@@ -4,9 +4,9 @@ require 'spec_helper'
 
 RSpec.describe Issues::CreateService do
   let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be_with_reload(:project) { create(:project, group: group) }
 
-  let(:project) { create(:project, group: group) }
-  let(:user) { create(:user) }
   let(:params) { { title: 'Awesome issue', description: 'please fix', weight: 9 } }
   let(:service) { described_class.new(project: project, current_user: user, params: params) }
 
@@ -67,11 +67,17 @@ RSpec.describe Issues::CreateService do
           end
 
           context 'with epic and milestone in commands only' do
-            let(:milestone) { create(:milestone, group: group, start_date: Date.today, due_date: 7.days.from_now) }
+            let_it_be(:milestone) { create(:milestone, group: group, start_date: Date.today, due_date: 7.days.from_now) }
+            let_it_be(:assignee_user1) { create(:user) }
+
+            before do
+              project.add_guest(assignee_user1)
+            end
+
             let(:params) do
               {
                 title: 'Awesome issue',
-                description: %(/epic #{epic.to_reference}\n/milestone #{milestone.to_reference}")
+                description: %(/epic #{epic.to_reference}\n/milestone #{milestone.to_reference}\n/assign #{assignee_user1.to_reference})
               }
             end
 
@@ -83,6 +89,20 @@ RSpec.describe Issues::CreateService do
               expect(epic.reload.start_date).to eq(milestone.start_date)
               expect(epic.due_date).to eq(milestone.due_date)
             end
+
+            it 'generates system notes for adding an epic and milestone' do
+              expect { service.execute }.to change(Note, :count).by(3).and(change(ResourceMilestoneEvent, :count).by(1))
+            end
+
+            context 'when assigning epic raises an exception' do
+              let(:mock_service) { double('service', execute: { status: :error, message: 'failed to assign epic' }) }
+
+              it 'assigns the issue passed to the provided epic' do
+                expect(EpicIssues::CreateService).to receive(:new).and_return(mock_service)
+
+                expect { service.execute }.to raise_error(EE::Issues::BaseService::EpicAssignmentError, 'failed to assign epic')
+              end
+            end
           end
 
           context 'when adding a public issue to confidential epic' do

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

@@ -4,9 +4,10 @@ require 'spec_helper'
 
 RSpec.describe Issues::UpdateService do
   let_it_be(:group) { create(:group) }
+  let_it_be_with_reload(:project) { create(:project, group: group) }
+  let_it_be_with_reload(:issue) { create(:issue, project: project) }
+  let_it_be(:epic) { create(:epic, group: group) }
 
-  let(:project) { create(:project, group: group) }
-  let(:issue) { create(:issue, project: project) }
   let(:author) { issue.author }
   let(:user) { author }
 
@@ -20,12 +21,12 @@ RSpec.describe Issues::UpdateService do
     end
 
     context 'refresh epic dates' do
-      let_it_be(:epic) { create(:epic) }
-
-      let(:issue) { create(:issue, epic: epic, project: project) }
+      before do
+        issue.update!(epic: epic)
+      end
 
       context 'updating milestone' do
-        let(:milestone) { create(:milestone, project: project) }
+        let_it_be(:milestone) { create(:milestone, project: project) }
 
         it 'calls UpdateDatesService' do
           expect(Epics::UpdateDatesService).to receive(:new).with([epic]).and_call_original.twice
@@ -36,7 +37,7 @@ RSpec.describe Issues::UpdateService do
       end
 
       context 'updating iteration' do
-        let(:iteration) { create(:iteration, group: group) }
+        let_it_be(:iteration) { create(:iteration, group: group) }
 
         context 'when issue does not already have an iteration' do
           it 'calls NotificationService#changed_iteration_issue' do
@@ -49,7 +50,7 @@ RSpec.describe Issues::UpdateService do
         end
 
         context 'when issue already has an iteration' do
-          let(:old_iteration) { create(:iteration, group: group) }
+          let_it_be(:old_iteration) { create(:iteration, group: group) }
 
           before do
             update_issue(iteration: old_iteration)
@@ -96,7 +97,7 @@ RSpec.describe Issues::UpdateService do
       context 'updating weight' do
         before do
           project.add_maintainer(user)
-          issue.update(weight: 3)
+          issue.update!(weight: 3)
         end
 
         context 'when weight is integer' do
@@ -158,20 +159,20 @@ RSpec.describe Issues::UpdateService do
       end
 
       context 'group iterations' do
-        let(:iteration) { create(:iteration, group: group) }
+        let_it_be(:iteration) { create(:iteration, group: group) }
 
         it_behaves_like 'creates iteration resource event'
       end
 
       context 'project iterations' do
-        let(:iteration) { create(:iteration, :skip_project_validation, project: project) }
+        let_it_be(:iteration) { create(:iteration, :skip_project_validation, project: project) }
 
         it_behaves_like 'creates iteration resource event'
       end
     end
 
     context 'changing issue_type' do
-      let!(:sla_setting) { create(:project_incident_management_setting, :sla_enabled, project: project) }
+      let_it_be(:sla_setting) { create(:project_incident_management_setting, :sla_enabled, project: project) }
 
       before do
         stub_licensed_features(incident_sla: true, quality_management: true)
@@ -186,8 +187,8 @@ RSpec.describe Issues::UpdateService do
       end
 
       context 'from incident to issue' do
-        let(:issue) { create(:incident, project: project) }
-        let!(:sla) { create(:issuable_sla, issue: issue) }
+        let_it_be(:issue) { create(:incident, project: project) }
+        let_it_be(:sla) { create(:issuable_sla, issue: issue) }
 
         it 'does not remove the SLA or create a new one' do
           expect { update_issue(issue_type: 'issue') }.not_to change(IssuableSla, :count)
@@ -212,10 +213,12 @@ RSpec.describe Issues::UpdateService do
         end
 
         context 'without sufficient permissions' do
-          let(:user) { create(:user) }
+          let_it_be(:guest) { create(:user) }
+
+          let(:user) { guest }
 
           before do
-            project.add_guest(user)
+            project.add_guest(guest)
           end
 
           it 'excludes the issue type param' do
@@ -230,9 +233,9 @@ RSpec.describe Issues::UpdateService do
         stub_licensed_features(epics: true)
       end
 
-      let(:epic) { create(:epic, group: group) }
+      let(:params) { { epic: epic } }
 
-      subject { update_issue(epic: epic) }
+      subject { update_issue(params) }
 
       context 'when a user does not have permissions to assign an epic' do
         it 'raises an exception' do
@@ -282,7 +285,7 @@ RSpec.describe Issues::UpdateService do
         end
 
         context 'when issue belongs to another epic' do
-          let(:epic2) { create(:epic, group: group) }
+          let_it_be(:epic2) { create(:epic, group: group) }
 
           before do
             issue.update!(epic: epic2)
@@ -308,6 +311,52 @@ RSpec.describe Issues::UpdateService do
             subject
           end
         end
+
+        context 'when updating issue epic and milestone and assignee attributes' do
+          let_it_be(:milestone) { create(:milestone, project: project) }
+          let_it_be(:assignee_user1) { create(:user) }
+
+          let(:params) { { epic: epic, milestone: milestone, assignees: [assignee_user1] } }
+
+          before do
+            project.add_guest(assignee_user1)
+          end
+
+          it 'assigns the issue passed to the provided epic' do
+            expect do
+              subject
+              issue.reload
+            end.to change { issue.epic }.from(nil).to(epic)
+                   .and(change { issue.milestone }.from(nil).to(milestone))
+                   .and(change(ResourceMilestoneEvent, :count).by(1))
+                   .and(change(Note, :count).by(3))
+          end
+
+          context 'when milestone and epic attributes are changed from description' do
+            let(:params) { { description: %(/epic #{epic.to_reference}\n/milestone #{milestone.to_reference}\n/assign #{assignee_user1.to_reference}) } }
+
+            it 'assigns the issue passed to the provided epic' do
+              expect do
+                subject
+                issue.reload
+              end.to change { issue.epic }.from(nil).to(epic)
+                     .and(change { issue.assignees }.from([]).to([assignee_user1]))
+                     .and(change { issue.milestone }.from(nil).to(milestone))
+                     .and(change(ResourceMilestoneEvent, :count).by(1))
+                     .and(change(Note, :count).by(4))
+            end
+          end
+
+          context 'when assigning epic raises an exception' do
+            let(:mock_service) { double('service', execute: { status: :error, message: 'failed to assign epic' }) }
+
+            it 'assigns the issue passed to the provided epic' do
+              expect(EpicIssues::CreateService).to receive(:new).and_return(mock_service)
+
+              expect { subject }.to raise_error(EE::Issues::BaseService::EpicAssignmentError, 'failed to assign epic')
+            end
+          end
+        end
       end
     end
 
@@ -316,8 +365,6 @@ RSpec.describe Issues::UpdateService do
         stub_licensed_features(epics: true)
       end
 
-      let(:epic) { create(:epic, group: group) }
-
       subject { update_issue(epic: nil) }
 
       context 'when a user has permissions to assign an epic' do
@@ -338,7 +385,9 @@ RSpec.describe Issues::UpdateService do
         end
 
         context 'when issue belongs to an epic' do
-          let!(:epic_issue) { create(:epic_issue, issue: issue, epic: epic)}
+          before do
+            issue.update!(epic: epic)
+          end
 
           it 'unassigns the epic' do
             expect { subject }.to change { issue.reload.epic }.from(epic).to(nil)
@@ -346,8 +395,7 @@ RSpec.describe Issues::UpdateService do
 
           it 'calls EpicIssues::DestroyService' do
             link_sevice = double
-            expect(EpicIssues::DestroyService).to receive(:new).with(EpicIssue.last, user)
-              .and_return(link_sevice)
+            expect(EpicIssues::DestroyService).to receive(:new).with(EpicIssue.last, user).and_return(link_sevice)
             expect(link_sevice).to receive(:execute).and_return({ status: :success })
 
             subject
@@ -362,10 +410,8 @@ RSpec.describe Issues::UpdateService do
           context 'but EpicIssues::DestroyService returns failure', :aggregate_failures do
             it 'does not send usage data for removed epic action' do
               link_sevice = double
-              expect(EpicIssues::DestroyService).to receive(:new).with(EpicIssue.last, user)
-                                                      .and_return(link_sevice)
+              expect(EpicIssues::DestroyService).to receive(:new).with(EpicIssue.last, user).and_return(link_sevice)
               expect(link_sevice).to receive(:execute).and_return({ status: :failure })
-
               expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_removed_from_epic_action)
 
               subject
@@ -382,17 +428,16 @@ RSpec.describe Issues::UpdateService do
 
     it_behaves_like 'issue with epic_id parameter' do
       let(:execute) { described_class.new(project: project, current_user: user, params: params).execute(issue) }
-      let(:epic) { create(:epic, group: group) }
     end
 
     context 'when epic_id is nil' do
       before do
         stub_licensed_features(epics: true)
         group.add_maintainer(user)
+        issue.update!(epic: epic)
       end
 
-      let(:epic) { create(:epic, group: group) }
-      let!(:epic_issue) { create(:epic_issue, epic: epic, issue: issue) }
+      let(:epic_issue) { issue.epic_issue }
       let(:params) { { epic_id: nil } }
 
       subject { update_issue(params) }
@@ -403,8 +448,7 @@ RSpec.describe Issues::UpdateService do
 
       it 'calls EpicIssues::DestroyService' do
         link_sevice = double
-        expect(EpicIssues::DestroyService).to receive(:new).with(epic_issue, user)
-          .and_return(link_sevice)
+        expect(EpicIssues::DestroyService).to receive(:new).with(epic_issue, user).and_return(link_sevice)
         expect(link_sevice).to receive(:execute).and_return({ status: :success })
 
         subject