diff options
-rw-r--r-- | app/models/concerns/noteable.rb | 8 | ||||
-rw-r--r-- | app/models/issue.rb | 2 | ||||
-rw-r--r-- | app/services/note_summary.rb | 4 | ||||
-rw-r--r-- | app/services/resource_events/change_labels_service.rb | 2 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/53279-fix-updated_at-api.yml | 5 | ||||
-rw-r--r-- | lib/api/issues.rb | 10 | ||||
-rw-r--r-- | spec/services/note_summary_spec.rb | 10 | ||||
-rw-r--r-- | spec/services/system_note_service_spec.rb | 34 |
9 files changed, 67 insertions, 10 deletions
diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 3c74034b527..423ce7e7db1 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -13,6 +13,14 @@ module Noteable end end + # The timestamp of the note (e.g. the :updated_at attribute if provided via + # API call) + def system_note_timestamp + @system_note_timestamp || Time.now # rubocop:disable Gitlab/ModuleWithInstanceVariables + end + + attr_writer :system_note_timestamp + def base_class_name self.class.base_class.name end diff --git a/app/models/issue.rb b/app/models/issue.rb index 97c6dcc4745..261935fd054 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -90,7 +90,7 @@ class Issue < ApplicationRecord state :closed before_transition any => :closed do |issue| - issue.closed_at = Time.zone.now + issue.closed_at = issue.system_note_timestamp end before_transition closed: :opened do |issue| diff --git a/app/services/note_summary.rb b/app/services/note_summary.rb index 81f6f92f75c..60a68568833 100644 --- a/app/services/note_summary.rb +++ b/app/services/note_summary.rb @@ -5,7 +5,9 @@ class NoteSummary attr_reader :metadata def initialize(noteable, project, author, body, action: nil, commit_count: nil) - @note = { noteable: noteable, project: project, author: author, note: body } + @note = { noteable: noteable, + created_at: noteable.system_note_timestamp, + project: project, author: author, note: body } @metadata = { action: action, commit_count: commit_count }.compact set_commit_params if note[:noteable].is_a?(Commit) diff --git a/app/services/resource_events/change_labels_service.rb b/app/services/resource_events/change_labels_service.rb index 039d6e2ebad..b45e567079b 100644 --- a/app/services/resource_events/change_labels_service.rb +++ b/app/services/resource_events/change_labels_service.rb @@ -12,7 +12,7 @@ module ResourceEvents label_hash = { resource_column(resource) => resource.id, user_id: user.id, - created_at: Time.now + created_at: resource.system_note_timestamp } labels = added_labels.map do |label| diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index ea8ac7e4656..acbbb0da929 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -258,7 +258,7 @@ module SystemNoteService body = "created #{issue.to_reference} to continue this discussion" note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body) - note = Note.create(note_attributes.merge(system: true)) + note = Note.create(note_attributes.merge(system: true, created_at: issue.system_note_timestamp)) note.system_note_metadata = SystemNoteMetadata.new(action: 'discussion') note diff --git a/changelogs/unreleased/53279-fix-updated_at-api.yml b/changelogs/unreleased/53279-fix-updated_at-api.yml new file mode 100644 index 00000000000..c64dada7eaa --- /dev/null +++ b/changelogs/unreleased/53279-fix-updated_at-api.yml @@ -0,0 +1,5 @@ +--- +title: "Respect updated_at attribute in notes produced by API calls" +merge_request: 27124 +author: Ben Gamari +type: fixed diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 999a9cb5a82..000c00ea9f9 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -230,9 +230,13 @@ module API issue = user_project.issues.find_by!(iid: params.delete(:issue_iid)) authorize! :update_issue, issue - # Setting created_at time only allowed for admins and project/group owners - unless current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner) - params.delete(:updated_at) + # Setting updated_at only allowed for admins and owners as well + if params[:updated_at].present? + if current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner) + issue.system_note_timestamp = params[:updated_at] + else + params.delete(:updated_at) + end end update_params = declared_params(include_missing: false).merge(request: request, api: true) diff --git a/spec/services/note_summary_spec.rb b/spec/services/note_summary_spec.rb index a6cc2251e48..f6ee15f750c 100644 --- a/spec/services/note_summary_spec.rb +++ b/spec/services/note_summary_spec.rb @@ -21,16 +21,20 @@ describe NoteSummary do describe '#note' do it 'returns note hash' do - expect(create_note_summary.note).to eq(noteable: noteable, project: project, author: user, note: 'note') + Timecop.freeze do + expect(create_note_summary.note).to eq(noteable: noteable, project: project, author: user, note: 'note', + created_at: Time.now) + end end context 'when noteable is a commit' do - let(:noteable) { build(:commit) } + let(:noteable) { build(:commit, system_note_timestamp: Time.at(43)) } it 'returns note hash specific to commit' do expect(create_note_summary.note).to eq( noteable: nil, project: project, author: user, note: 'note', - noteable_type: 'Commit', commit_id: noteable.id + noteable_type: 'Commit', commit_id: noteable.id, + created_at: Time.at(43) ) end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index b917de14b2e..8d446d1c9d5 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -11,6 +11,14 @@ describe SystemNoteService do let(:noteable) { create(:issue, project: project) } let(:issue) { noteable } + shared_examples_for 'a note with overridable created_at' do + let(:noteable) { create(:issue, project: project, system_note_timestamp: Time.at(42)) } + + it 'the note has the correct time' do + expect(subject.created_at).to eq Time.at(42) + end + end + shared_examples_for 'a system note' do let(:expected_noteable) { noteable } let(:commit_count) { nil } @@ -137,6 +145,8 @@ describe SystemNoteService do end context 'when assignee added' do + it_behaves_like 'a note with overridable created_at' + it 'sets the note text' do expect(subject.note).to eq "assigned to @#{assignee.username}" end @@ -145,6 +155,8 @@ describe SystemNoteService do context 'when assignee removed' do let(:assignee) { nil } + it_behaves_like 'a note with overridable created_at' + it 'sets the note text' do expect(subject.note).to eq 'removed assignee' end @@ -168,6 +180,8 @@ describe SystemNoteService do described_class.change_issue_assignees(issue, project, author, old_assignees).note end + it_behaves_like 'a note with overridable created_at' + it 'builds a correct phrase when an assignee is added to a non-assigned issue' do expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}" end @@ -213,6 +227,8 @@ describe SystemNoteService do expect(subject.note).to eq "changed milestone to #{reference}" end + + it_behaves_like 'a note with overridable created_at' end context 'when milestone removed' do @@ -221,6 +237,8 @@ describe SystemNoteService do it 'sets the note text' do expect(subject.note).to eq 'removed milestone' end + + it_behaves_like 'a note with overridable created_at' end end @@ -237,6 +255,8 @@ describe SystemNoteService do it 'sets the note text to use the milestone name' do expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}" end + + it_behaves_like 'a note with overridable created_at' end context 'when milestone removed' do @@ -245,6 +265,8 @@ describe SystemNoteService do it 'sets the note text' do expect(subject.note).to eq 'removed milestone' end + + it_behaves_like 'a note with overridable created_at' end end end @@ -254,6 +276,8 @@ describe SystemNoteService do let(:due_date) { Date.today } + it_behaves_like 'a note with overridable created_at' + it_behaves_like 'a system note' do let(:action) { 'due_date' } end @@ -280,6 +304,8 @@ describe SystemNoteService do let(:status) { 'reopened' } let(:source) { nil } + it_behaves_like 'a note with overridable created_at' + it_behaves_like 'a system note' do let(:action) { 'opened' } end @@ -289,6 +315,8 @@ describe SystemNoteService do let(:status) { 'opened' } let(:source) { double('commit', gfm_reference: 'commit 123456') } + it_behaves_like 'a note with overridable created_at' + it 'sets the note text' do expect(subject.note).to eq "#{status} via commit 123456" end @@ -338,6 +366,8 @@ describe SystemNoteService do let(:action) { 'title' } end + it_behaves_like 'a note with overridable created_at' + it 'sets the note text' do expect(subject.note) .to eq "changed title from **{-Old title-}** to **{+Lorem ipsum+}**" @@ -353,6 +383,8 @@ describe SystemNoteService do let(:action) { 'description' } end + it_behaves_like 'a note with overridable created_at' + it 'sets the note text' do expect(subject.note).to eq('changed the description') end @@ -478,6 +510,8 @@ describe SystemNoteService do let(:action) { 'cross_reference' } end + it_behaves_like 'a note with overridable created_at' + describe 'note_body' do context 'cross-project' do let(:project2) { create(:project, :repository) } |