Merge branch 'add-previous-milestone-id-to-milestone-change-events-pd' into 'master'

Add previous milestone to resource milestone remove events

See merge request gitlab-org/gitlab!31451

app/models/milestone_note.rb

@@ -17,6 +17,6 @@ class MilestoneNote < SyntheticNote
 
   def note_text(html: false)
     format = milestone&.group_milestone? ? :name : :iid
-    milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}"
+    event.remove? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}"
   end
 end

app/services/issuable/common_system_notes_service.rb

@@ -4,7 +4,7 @@ module Issuable
   class CommonSystemNotesService < ::BaseService
     attr_reader :issuable
 
-    def execute(issuable, old_labels: [], is_update: true)
+    def execute(issuable, old_labels: [], old_milestone: nil, is_update: true)
       @issuable = issuable
 
       # We disable touch so that created system notes do not update
@@ -22,17 +22,13 @@ module Issuable
         end
 
         create_due_date_note if issuable.previous_changes.include?('due_date')
-        create_milestone_note if has_milestone_changes?
+        create_milestone_note(old_milestone) if issuable.previous_changes.include?('milestone_id')
         create_labels_note(old_labels) if old_labels && issuable.labels != old_labels
       end
     end
 
     private
 
-    def has_milestone_changes?
-      issuable.previous_changes.include?('milestone_id')
-    end
-
     def handle_time_tracking_note
       if issuable.previous_changes.include?('time_estimate')
         create_time_estimate_note
@@ -98,15 +94,19 @@ module Issuable
       SystemNoteService.change_time_spent(issuable, issuable.project, issuable.time_spent_user)
     end
 
-    def create_milestone_note
+    def create_milestone_note(old_milestone)
       if milestone_changes_tracking_enabled?
-        # Creates a synthetic note
-        ResourceEvents::ChangeMilestoneService.new(issuable, current_user).execute
+        create_milestone_change_event(old_milestone)
       else
         SystemNoteService.change_milestone(issuable, issuable.project, current_user, issuable.milestone)
       end
     end
 
+    def create_milestone_change_event(old_milestone)
+      ResourceEvents::ChangeMilestoneService.new(issuable, current_user, old_milestone: old_milestone)
+        .execute
+    end
+
     def milestone_changes_tracking_enabled?
       ::Feature.enabled?(:track_resource_milestone_change_events, issuable.project)
     end

app/services/issuable_base_service.rb

@@ -241,7 +241,8 @@ class IssuableBaseService < BaseService
       end
 
       if issuable_saved
-        Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels: old_associations[:labels])
+        Issuable::CommonSystemNotesService.new(project, current_user).execute(
+          issuable, old_labels: old_associations[:labels], old_milestone: old_associations[:milestone])
 
         handle_changes(issuable, old_associations: old_associations)
 
@@ -364,7 +365,8 @@ class IssuableBaseService < BaseService
       {
         labels: issuable.labels.to_a,
         mentioned_users: issuable.mentioned_users(current_user).to_a,
-        assignees: issuable.assignees.to_a
+        assignees: issuable.assignees.to_a,
+        milestone: issuable.try(:milestone)
       }
     associations[:total_time_spent] = issuable.total_time_spent if issuable.respond_to?(:total_time_spent)
     associations[:description] = issuable.description

app/services/resource_events/change_milestone_service.rb

@@ -2,13 +2,14 @@
 
 module ResourceEvents
   class ChangeMilestoneService
-    attr_reader :resource, :user, :event_created_at, :milestone
+    attr_reader :resource, :user, :event_created_at, :milestone, :old_milestone
 
-    def initialize(resource, user, created_at: Time.now)
+    def initialize(resource, user, created_at: Time.now, old_milestone:)
       @resource = resource
       @user = user
       @event_created_at = created_at
       @milestone = resource&.milestone
+      @old_milestone = old_milestone
     end
 
     def execute
@@ -26,7 +27,7 @@ module ResourceEvents
       {
         user_id: user.id,
         created_at: event_created_at,
-        milestone_id: milestone&.id,
+        milestone_id: action == :add ? milestone&.id : old_milestone&.id,
         state: ResourceMilestoneEvent.states[resource.state],
         action: ResourceMilestoneEvent.actions[action],
         key => resource.id

ee/app/services/ee/issuable/common_system_notes_service.rb

@@ -7,7 +7,7 @@ module EE
       attr_reader :issuable
 
       override :execute
-      def execute(_issuable, old_labels: [], is_update: true)
+      def execute(_issuable, old_labels: [], old_milestone: nil, is_update: true)
         super
 
         ActiveRecord::Base.no_touching do

spec/models/milestone_note_spec.rb

@@ -14,5 +14,15 @@ describe MilestoneNote do
     it_behaves_like 'a system note', exclude_project: true do
       let(:action) { 'milestone' }
     end
+
+    context 'with a remove milestone event' do
+      let(:milestone) { create(:milestone) }
+      let(:event) { create(:resource_milestone_event, action: :remove, issue: noteable, milestone: milestone) }
+
+      it 'creates the expected note' do
+        expect(subject.note_html).to include('removed milestone')
+        expect(subject.note_html).not_to include('changed milestone to')
+      end
+    end
   end
 end

spec/services/merge_requests/update_service_spec.rb

@@ -92,6 +92,7 @@ describe MergeRequests::UpdateService, :mailer do
               labels: [],
               mentioned_users: [user2],
               assignees: [user3],
+              milestone: nil,
               total_time_spent: 0,
               description: "FYI #{user2.to_reference}"
             }

spec/services/resource_events/change_milestone_service_spec.rb

@@ -3,11 +3,9 @@
 require 'spec_helper'
 
 describe ResourceEvents::ChangeMilestoneService do
-  it_behaves_like 'a milestone events creator' do
-    let(:resource) { create(:issue) }
-  end
-
-  it_behaves_like 'a milestone events creator' do
-    let(:resource) { create(:merge_request) }
+  [:issue, :merge_request].each do |issuable|
+    it_behaves_like 'a milestone events creator' do
+      let(:resource) { create(issuable) }
+    end
   end
 end

spec/support/shared_examples/services/resource_events/change_milestone_service_shared_examples.rb

@@ -4,7 +4,7 @@ shared_examples 'a milestone events creator' do
   let_it_be(:user) { create(:user) }
 
   let(:created_at_time) { Time.utc(2019, 12, 30) }
-  let(:service) { described_class.new(resource, user, created_at: created_at_time) }
+  let(:service) { described_class.new(resource, user, created_at: created_at_time, old_milestone: nil) }
 
   context 'when milestone is present' do
     let_it_be(:milestone) { create(:milestone) }
@@ -25,10 +25,13 @@ shared_examples 'a milestone events creator' do
       resource.milestone = nil
     end
 
+    let(:old_milestone) { create(:milestone, project: resource.project) }
+    let(:service) { described_class.new(resource, user, created_at: created_at_time, old_milestone: old_milestone) }
+
     it 'creates the expected event records' do
       expect { service.execute }.to change { ResourceMilestoneEvent.count }.by(1)
 
-      expect_event_record(ResourceMilestoneEvent.last, action: 'remove', milestone: nil, state: 'opened')
+      expect_event_record(ResourceMilestoneEvent.last, action: 'remove', milestone: old_milestone, state: 'opened')
     end
   end