Merge branch 'fix-edited-timestamp' into 'master'

Fix edited timestamp updated when transforming / resolving comments

See merge request gitlab-org/gitlab!55671

app/models/note.rb

@@ -30,7 +30,6 @@ class Note < ApplicationRecord
 
   # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes.
   # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102
-  alias_attribute :last_edited_at, :updated_at
   alias_attribute :last_edited_by, :updated_by
 
   # Attribute containing rendered and redacted Markdown as generated by
@@ -349,7 +348,13 @@ class Note < ApplicationRecord
     !system?
   end
 
-  # Since we're using `updated_at` as `last_edited_at`, it could be touched by transforming / resolving a note.
+  # We used `last_edited_at` as an alias of `updated_at` before.
+  # This makes it compatible with the previous way without data migration.
+  def last_edited_at
+    super || updated_at
+  end
+
+  # Since we used `updated_at` as `last_edited_at`, it could be touched by transforming / resolving a note.
   # This makes sure it is only marked as edited when the note body is updated.
   def edited?
     return false if updated_by.blank?

app/services/notes/update_service.rb

@@ -7,12 +7,7 @@ module Notes
 
       old_mentioned_users = note.mentioned_users(current_user).to_a
 
-      note.assign_attributes(params.merge(updated_by: current_user))
-
-      note.with_transaction_returning_status do
-        update_confidentiality(note)
-        note.save
-      end
+      note.assign_attributes(params)
 
       track_note_edit_usage_for_issues(note) if note.for_issue?
       track_note_edit_usage_for_merge_requests(note) if note.for_merge_request?
@@ -28,6 +23,15 @@ module Notes
         note.note = content
       end
 
+      if note.note_changed?
+        note.assign_attributes(last_edited_at: Time.current, updated_by: current_user)
+      end
+
+      note.with_transaction_returning_status do
+        update_confidentiality(note)
+        note.save
+      end
+
       unless only_commands || note.for_personal_snippet?
         note.create_new_cross_references!(current_user)
 

changelogs/unreleased/fix-edited-timestamp.yml

+---
+title: Fix edited timestamp updated when transforming / resolving comments
+merge_request: 55671
+author: Mycroft Kang @TaehyeokKang
+type: fixed

db/migrate/20210303053341_add_last_edited_at_and_last_edited_by_id_to_notes.rb

+# frozen_string_literal: true
+
+class AddLastEditedAtAndLastEditedByIdToNotes < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :notes, :last_edited_at, :datetime_with_timezone
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :notes, :last_edited_at
+    end
+  end
+end

db/schema_migrations/20210303053341

+3bd7e839c4f93716a7e893bf9184306a1fcfd401e5b54f4393e5138e2776f5e0
\ No newline at end of file

db/structure.sql

@@ -14686,7 +14686,8 @@ CREATE TABLE notes (
     change_position text,
     resolved_by_push boolean,
     review_id bigint,
-    confidential boolean
+    confidential boolean,
+    last_edited_at timestamp with time zone
 );
 
 CREATE SEQUENCE notes_id_seq

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -84,6 +84,7 @@ Note:
 - discussion_id
 - original_discussion_id
 - confidential
+- last_edited_at
 LabelLink:
 - id
 - target_type

spec/models/note_spec.rb

@@ -336,6 +336,25 @@ RSpec.describe Note do
     end
   end
 
+  describe "last_edited_at" do
+    let(:timestamp) { Time.current }
+    let(:note) { build(:note, last_edited_at: nil, created_at: timestamp, updated_at: timestamp + 5.hours) }
+
+    context "with last_edited_at" do
+      it "returns last_edited_at" do
+        note.last_edited_at = timestamp
+
+        expect(note.last_edited_at).to eq(timestamp)
+      end
+    end
+
+    context "without last_edited_at" do
+      it "returns updated_at" do
+        expect(note.last_edited_at).to eq(timestamp + 5.hours)
+      end
+    end
+  end
+
   describe "edited?" do
     let(:note) { build(:note, updated_by_id: nil, created_at: Time.current, updated_at: Time.current + 5.hours) }
 

spec/services/notes/update_service_spec.rb

@@ -64,6 +64,40 @@ RSpec.describe Notes::UpdateService do
       end.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1)
     end
 
+    context 'when note text was changed' do
+      let!(:note) { create(:note, project: project, noteable: issue, author: user2, note: "Old note #{user3.to_reference}") }
+      let(:edit_note_text) { update_note({ note: 'new text' }) }
+
+      it 'update last_edited_at' do
+        travel_to(1.day.from_now) do
+          expect { edit_note_text }.to change { note.reload.last_edited_at }
+        end
+      end
+
+      it 'update updated_by' do
+        travel_to(1.day.from_now) do
+          expect { edit_note_text }.to change { note.reload.updated_by }
+        end
+      end
+    end
+
+    context 'when note text was not changed' do
+      let!(:note) { create(:note, project: project, noteable: issue, author: user2, note: "Old note #{user3.to_reference}") }
+      let(:does_not_edit_note_text) { update_note({}) }
+
+      it 'does not update last_edited_at' do
+        travel_to(1.day.from_now) do
+          expect { does_not_edit_note_text }.not_to change { note.reload.last_edited_at }
+        end
+      end
+
+      it 'does not update updated_by' do
+        travel_to(1.day.from_now) do
+          expect { does_not_edit_note_text }.not_to change { note.reload.updated_by }
+        end
+      end
+    end
+
     context 'when the notable is a merge request' do
       let(:merge_request) { create(:merge_request, source_project: project) }
       let(:note) { create(:note, project: project, noteable: merge_request, author: user, note: "Old note #{user2.to_reference}") }