Merge branch '345109-timeline-event-system-note-on-edit' into 'master'

Add a system note when timeline event was edited

See merge request gitlab-org/gitlab!84276

ee/app/services/ee/system_note_service.rb

@@ -124,6 +124,10 @@ module EE
       incidents_service(timeline_event.incident).add_timeline_event(timeline_event)
     end
 
+    def edit_timeline_event(timeline_event, author, was_changed:)
+      incidents_service(timeline_event.incident).edit_timeline_event(timeline_event, author, was_changed: was_changed)
+    end
+
     def delete_timeline_event(noteable, author)
       incidents_service(noteable).delete_timeline_event(author)
     end

ee/app/services/incident_management/timeline_events/update_service.rb

@@ -20,6 +20,8 @@ module IncidentManagement
         return error_no_permissions unless allowed?
 
         if timeline_event.update(update_params)
+          add_system_note(timeline_event)
+
           success(timeline_event)
         else
           error_in_save(timeline_event)
@@ -33,6 +35,27 @@ module IncidentManagement
       def update_params
         { updated_by_user: user, note: note.presence, occurred_at: occurred_at.presence }.compact
       end
+
+      def add_system_note(timeline_event)
+        return unless Feature.enabled?(:incident_timeline, incident.project, default_enabled: :yaml)
+
+        changes = was_changed(timeline_event)
+        return if changes == :none
+
+        SystemNoteService.edit_timeline_event(timeline_event, user, was_changed: changes)
+      end
+
+      def was_changed(timeline_event)
+        changes = timeline_event.previous_changes
+        occurred_at_changed = changes.key?('occurred_at')
+        note_changed = changes.key?('note')
+
+        return :occurred_at_and_note if occurred_at_changed && note_changed
+        return :occurred_at if occurred_at_changed
+        return :note if note_changed
+
+        :none
+      end
     end
   end
 end

ee/app/services/system_notes/incidents_service.rb

@@ -2,6 +2,12 @@
 
 module SystemNotes
   class IncidentsService < ::SystemNotes::BaseService
+    CHANGED_TEXT = {
+      occurred_at: 'the event time/date on ',
+      note: 'the text on ',
+      occurred_at_and_note: 'the event time/date and text on '
+    }.freeze
+
     def initialize(noteable:)
       @noteable = noteable
       @project = noteable.project
@@ -16,6 +22,15 @@ module SystemNotes
       create_note(NoteSummary.new(noteable, project, author, body, action: 'timeline_event'))
     end
 
+    def edit_timeline_event(timeline_event, author, was_changed:)
+      anchor = "timeline_event_#{timeline_event.id}"
+      path = url_helpers.project_issues_incident_path(project, noteable, anchor: anchor)
+      changed_text = CHANGED_TEXT.fetch(was_changed, '')
+      body = "edited #{changed_text}[incident timeline event](#{path})"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'timeline_event'))
+    end
+
     def delete_timeline_event(author)
       body = 'deleted an incident timeline event'
 

ee/spec/services/incident_management/timeline_events/update_service_spec.rb

@@ -13,6 +13,7 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do
 
   before do
     stub_licensed_features(incident_timeline_events: true)
+    stub_feature_flags(incident_timeline: project)
   end
 
   describe '#execute' do
@@ -30,6 +31,17 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do
       end
     end
 
+    shared_examples 'passing the correct was_changed value' do |was_changed|
+      it 'passes the correct was_changed value into SysteNoteService.edit_timeline_event' do
+        expect(SystemNoteService)
+          .to receive(:edit_timeline_event)
+          .with(timeline_event, user, was_changed: was_changed)
+          .and_call_original
+
+        execute
+      end
+    end
+
     subject(:execute) { described_class.new(timeline_event, user, params).execute }
 
     context 'when user has permissions' do
@@ -44,10 +56,27 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do
           .and change { timeline_event.occurred_at }.to(params[:occurred_at])
       end
 
+      it 'creates a system note' do
+        expect { execute }.to change { incident.notes.reload.count }.by(1)
+      end
+
+      it_behaves_like 'passing the correct was_changed value', :occurred_at_and_note
+
+      context 'when incident_timeline feature flag is disabled' do
+        before do
+          stub_feature_flags(incident_timeline: false)
+        end
+
+        it 'does not add a system note' do
+          expect { execute }.not_to change { incident.notes }
+        end
+      end
+
       context 'when note is nil' do
         let(:params) { { occurred_at: occurred_at } }
 
         it_behaves_like 'successful response'
+        it_behaves_like 'passing the correct was_changed value', :occurred_at
 
         it 'does not update the note' do
           expect { execute }.not_to change { timeline_event.reload.note }
@@ -62,6 +91,7 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do
         let(:params) { { note: '', occurred_at: occurred_at } }
 
         it_behaves_like 'successful response'
+        it_behaves_like 'passing the correct was_changed value', :occurred_at
 
         it 'does not update the note' do
           expect { execute }.not_to change { timeline_event.reload.note }
@@ -76,6 +106,7 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do
         let(:params) { { note: 'Updated note' } }
 
         it_behaves_like 'successful response'
+        it_behaves_like 'passing the correct was_changed value', :note
 
         it 'updates the note' do
           expect { execute }.to change { timeline_event.note }.to(params[:note])
@@ -85,6 +116,26 @@ RSpec.describe IncidentManagement::TimelineEvents::UpdateService do
           expect { execute }.not_to change { timeline_event.reload.occurred_at }
         end
       end
+
+      context 'when both occurred_at and note is nil' do
+        let(:params) { {} }
+
+        it_behaves_like 'successful response'
+
+        it 'does not update the note' do
+          expect { execute }.not_to change { timeline_event.note }
+        end
+
+        it 'does not update occurred_at' do
+          expect { execute }.not_to change { timeline_event.reload.occurred_at }
+        end
+
+        it 'does not call SysteNoteService.edit_timeline_event' do
+          expect(SystemNoteService).not_to receive(:edit_timeline_event)
+
+          execute
+        end
+      end
     end
 
     context 'when user does not have permissions' do

ee/spec/services/system_note_service_spec.rb

@@ -186,6 +186,18 @@ RSpec.describe SystemNoteService do
     end
   end
 
+  describe '.edit_timeline_event' do
+    let(:timeline_event) { instance_double('IncidentManagement::TimelineEvent', incident: noteable, project: project) }
+
+    it 'calls IncidentsService' do
+      expect_next_instance_of(::SystemNotes::IncidentsService) do |service|
+        expect(service).to receive(:edit_timeline_event).with(timeline_event, author, was_changed: :occurred_at)
+      end
+
+      described_class.edit_timeline_event(timeline_event, author, was_changed: :occurred_at)
+    end
+  end
+
   describe '.delete_timeline_event' do
     it 'calls IncidentsService' do
       expect_next_instance_of(::SystemNotes::IncidentsService) do |service|

ee/spec/services/system_notes/incidents_service_spec.rb

@@ -25,6 +25,50 @@ RSpec.describe SystemNotes::IncidentsService do
     end
   end
 
+  describe '#edit_timeline_event' do
+    let(:was_changed) { :unknown }
+    let(:path) { project_issues_incident_path(project, incident, anchor: "timeline_event_#{timeline_event.id}") }
+
+    subject { described_class.new(noteable: incident).edit_timeline_event(timeline_event, author, was_changed: was_changed) }
+
+    it_behaves_like 'a system note' do
+      let(:noteable) { incident }
+      let(:action) { 'timeline_event' }
+    end
+
+    context "when only timeline event's occurred_at was changed" do
+      let(:was_changed) { :occurred_at }
+
+      it 'posts the correct text to the system note' do
+        expect(subject.note).to match("edited the event time/date on [incident timeline event](#{path})")
+      end
+    end
+
+    context "when only timeline event's note was changed" do
+      let(:was_changed) { :note }
+
+      it 'posts the correct text to the system note' do
+        expect(subject.note).to match("edited the text on [incident timeline event](#{path})")
+      end
+    end
+
+    context "when both timeline events occurred_at and note was changed" do
+      let(:was_changed) { :occurred_at_and_note }
+
+      it 'posts the correct text to the system note' do
+        expect(subject.note).to match("edited the event time/date and text on [incident timeline event](#{path})")
+      end
+    end
+
+    context "when was changed reason is unknown" do
+      let(:was_changed) { :unknown }
+
+      it 'posts the correct text to the system note' do
+        expect(subject.note).to match("edited [incident timeline event](#{path})")
+      end
+    end
+  end
+
   describe '#delete_timeline_event' do
     subject { described_class.new(noteable: incident).delete_timeline_event(author) }