Fix missing system notes and system events on create issue

This fixes missing events when milestone or assignees are set
together with setting the epic on issue creation. This was
happening due to epic_issues relationship being saved before issue
changes were handled, which also triggered an issue.save which
would then lose issue changes. This moves the save of epic_issue
relationship to happen at issue.save time.

Changelog: fixed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/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