Commit ed49d690 authored by Mycroft's avatar Mycroft Committed by Heinrich Lee Yu

Fix edited timestamp of comments

Fixes the timestamp so that it is not updated when transforming or
resolving them
parent d4638e5d
...@@ -30,7 +30,6 @@ class Note < ApplicationRecord ...@@ -30,7 +30,6 @@ class Note < ApplicationRecord
# Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes. # 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 # 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 alias_attribute :last_edited_by, :updated_by
# Attribute containing rendered and redacted Markdown as generated by # Attribute containing rendered and redacted Markdown as generated by
...@@ -349,7 +348,13 @@ class Note < ApplicationRecord ...@@ -349,7 +348,13 @@ class Note < ApplicationRecord
!system? !system?
end 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. # This makes sure it is only marked as edited when the note body is updated.
def edited? def edited?
return false if updated_by.blank? return false if updated_by.blank?
......
...@@ -7,12 +7,7 @@ module Notes ...@@ -7,12 +7,7 @@ module Notes
old_mentioned_users = note.mentioned_users(current_user).to_a old_mentioned_users = note.mentioned_users(current_user).to_a
note.assign_attributes(params.merge(updated_by: current_user)) note.assign_attributes(params)
note.with_transaction_returning_status do
update_confidentiality(note)
note.save
end
track_note_edit_usage_for_issues(note) if note.for_issue? track_note_edit_usage_for_issues(note) if note.for_issue?
track_note_edit_usage_for_merge_requests(note) if note.for_merge_request? track_note_edit_usage_for_merge_requests(note) if note.for_merge_request?
...@@ -28,6 +23,15 @@ module Notes ...@@ -28,6 +23,15 @@ module Notes
note.note = content note.note = content
end 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? unless only_commands || note.for_personal_snippet?
note.create_new_cross_references!(current_user) note.create_new_cross_references!(current_user)
......
---
title: Fix edited timestamp updated when transforming / resolving comments
merge_request: 55671
author: Mycroft Kang @TaehyeokKang
type: fixed
# 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
3bd7e839c4f93716a7e893bf9184306a1fcfd401e5b54f4393e5138e2776f5e0
\ No newline at end of file
...@@ -14613,7 +14613,8 @@ CREATE TABLE notes ( ...@@ -14613,7 +14613,8 @@ CREATE TABLE notes (
change_position text, change_position text,
resolved_by_push boolean, resolved_by_push boolean,
review_id bigint, review_id bigint,
confidential boolean confidential boolean,
last_edited_at timestamp with time zone
); );
CREATE SEQUENCE notes_id_seq CREATE SEQUENCE notes_id_seq
...@@ -84,6 +84,7 @@ Note: ...@@ -84,6 +84,7 @@ Note:
- discussion_id - discussion_id
- original_discussion_id - original_discussion_id
- confidential - confidential
- last_edited_at
LabelLink: LabelLink:
- id - id
- target_type - target_type
......
...@@ -336,6 +336,25 @@ RSpec.describe Note do ...@@ -336,6 +336,25 @@ RSpec.describe Note do
end end
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 describe "edited?" do
let(:note) { build(:note, updated_by_id: nil, created_at: Time.current, updated_at: Time.current + 5.hours) } let(:note) { build(:note, updated_by_id: nil, created_at: Time.current, updated_at: Time.current + 5.hours) }
......
...@@ -64,6 +64,40 @@ RSpec.describe Notes::UpdateService do ...@@ -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.to change { counter.unique_events(event_names: event, start_date: 1.day.ago, end_date: 1.day.from_now) }.by(1)
end 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 context 'when the notable is a merge request' do
let(:merge_request) { create(:merge_request, source_project: project) } 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}") } let(:note) { create(:note, project: project, noteable: merge_request, author: user, note: "Old note #{user2.to_reference}") }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment