diff --git a/app/models/milestone_note.rb b/app/models/milestone_note.rb
index 2ff9791feb0b81dd0912c6fbde90f26f3e207bdd..19171e682b7e53fef7ae073f708be8b944c716aa 100644
--- a/app/models/milestone_note.rb
+++ b/app/models/milestone_note.rb
@@ -17,6 +17,6 @@ def note_html
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
diff --git a/app/services/issuable/common_system_notes_service.rb b/app/services/issuable/common_system_notes_service.rb
index 67cf212691f5be1d1519e91127f30f9cf5c9535d..195616857dcfb1c527b07c47ff657b886646dda3 100644
--- a/app/services/issuable/common_system_notes_service.rb
+++ b/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 @@ def execute(issuable, old_labels: [], is_update: true)
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 @@ def create_time_spent_note
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
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 50431e50110383bec498285819491591d353ed93..9f26944cbc5a13c34aae72f187cc991689a32d15 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -241,7 +241,8 @@ def update(issuable)
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 @@ def associations_before_update(issuable)
{
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
diff --git a/app/services/resource_events/change_milestone_service.rb b/app/services/resource_events/change_milestone_service.rb
index ea196822f743a02f63383138b572773baf7cf554..f09c2e1d735cbb8838f8f10329cd22a5a059e7c6 100644
--- a/app/services/resource_events/change_milestone_service.rb
+++ b/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 @@ def build_resource_args
{
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
diff --git a/ee/app/services/ee/issuable/common_system_notes_service.rb b/ee/app/services/ee/issuable/common_system_notes_service.rb
index f3617c161c3fd50f1f41db3ea3b761e5461f6d05..81f695030adaa9bf5baafcae8401079fc1025c0e 100644
--- a/ee/app/services/ee/issuable/common_system_notes_service.rb
+++ b/ee/app/services/ee/issuable/common_system_notes_service.rb
@@ -7,7 +7,7 @@ module CommonSystemNotesService
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
diff --git a/spec/models/milestone_note_spec.rb b/spec/models/milestone_note_spec.rb
index 9e77ef91bb29aae64578f757d968c3d6f5366b92..aad65cf034693316e8ab2ef1a2b60cfc810df472 100644
--- a/spec/models/milestone_note_spec.rb
+++ b/spec/models/milestone_note_spec.rb
@@ -14,5 +14,15 @@
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
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 8c1800c495fe520ac67504383b2d9598d76d3b67..bcc7a27802d24a0be423c706dc468dad6a12fbb0 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -92,6 +92,7 @@ def update_merge_request(opts)
labels: [],
mentioned_users: [user2],
assignees: [user3],
+ milestone: nil,
total_time_spent: 0,
description: "FYI #{user2.to_reference}"
}
diff --git a/spec/services/resource_events/change_milestone_service_spec.rb b/spec/services/resource_events/change_milestone_service_spec.rb
index bc634fadadd39d72c782ba5ba896cc33a865d1e2..dec01d0db8d2ce57704e1449a6d9357bd95c5336 100644
--- a/spec/services/resource_events/change_milestone_service_spec.rb
+++ b/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
diff --git a/spec/support/shared_examples/services/resource_events/change_milestone_service_shared_examples.rb b/spec/support/shared_examples/services/resource_events/change_milestone_service_shared_examples.rb
index 77f64e5e8f8c8343f4a9d8934f9632b6f5b209ef..c5f84e205cfab31857425dc3457f6d1f8633f526 100644
--- a/spec/support/shared_examples/services/resource_events/change_milestone_service_shared_examples.rb
+++ b/spec/support/shared_examples/services/resource_events/change_milestone_service_shared_examples.rb
@@ -4,7 +4,7 @@
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 @@
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