Commit 4317a2a3 authored by Sean McGivern's avatar Sean McGivern Committed by Rémy Coutable

Fix `updated_at` doesn't apply to `state_event` updates of issues via API

parent 425377f3
...@@ -13,6 +13,14 @@ module Noteable ...@@ -13,6 +13,14 @@ module Noteable
end end
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 def base_class_name
self.class.base_class.name self.class.base_class.name
end end
......
...@@ -90,7 +90,7 @@ class Issue < ApplicationRecord ...@@ -90,7 +90,7 @@ class Issue < ApplicationRecord
state :closed state :closed
before_transition any => :closed do |issue| before_transition any => :closed do |issue|
issue.closed_at = Time.zone.now issue.closed_at = issue.system_note_timestamp
end end
before_transition closed: :opened do |issue| before_transition closed: :opened do |issue|
......
...@@ -5,7 +5,9 @@ class NoteSummary ...@@ -5,7 +5,9 @@ class NoteSummary
attr_reader :metadata attr_reader :metadata
def initialize(noteable, project, author, body, action: nil, commit_count: nil) 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 @metadata = { action: action, commit_count: commit_count }.compact
set_commit_params if note[:noteable].is_a?(Commit) set_commit_params if note[:noteable].is_a?(Commit)
......
...@@ -12,7 +12,7 @@ module ResourceEvents ...@@ -12,7 +12,7 @@ module ResourceEvents
label_hash = { label_hash = {
resource_column(resource) => resource.id, resource_column(resource) => resource.id,
user_id: user.id, user_id: user.id,
created_at: Time.now created_at: resource.system_note_timestamp
} }
labels = added_labels.map do |label| labels = added_labels.map do |label|
......
...@@ -258,7 +258,7 @@ module SystemNoteService ...@@ -258,7 +258,7 @@ module SystemNoteService
body = "created #{issue.to_reference} to continue this discussion" body = "created #{issue.to_reference} to continue this discussion"
note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body) 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.system_note_metadata = SystemNoteMetadata.new(action: 'discussion')
note note
......
---
title: "Respect updated_at attribute in notes produced by API calls"
merge_request: 27124
author: Ben Gamari
type: fixed
...@@ -230,9 +230,13 @@ module API ...@@ -230,9 +230,13 @@ module API
issue = user_project.issues.find_by!(iid: params.delete(:issue_iid)) issue = user_project.issues.find_by!(iid: params.delete(:issue_iid))
authorize! :update_issue, issue authorize! :update_issue, issue
# Setting created_at time only allowed for admins and project/group owners # Setting updated_at only allowed for admins and owners as well
unless current_user.admin? || user_project.owner == current_user || current_user.owned_groups.include?(user_project.owner) if params[:updated_at].present?
params.delete(:updated_at) 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 end
update_params = declared_params(include_missing: false).merge(request: request, api: true) update_params = declared_params(include_missing: false).merge(request: request, api: true)
......
...@@ -21,16 +21,20 @@ describe NoteSummary do ...@@ -21,16 +21,20 @@ describe NoteSummary do
describe '#note' do describe '#note' do
it 'returns note hash' 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 end
context 'when noteable is a commit' do 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 it 'returns note hash specific to commit' do
expect(create_note_summary.note).to eq( expect(create_note_summary.note).to eq(
noteable: nil, project: project, author: user, note: 'note', 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
end end
......
...@@ -11,6 +11,14 @@ describe SystemNoteService do ...@@ -11,6 +11,14 @@ describe SystemNoteService do
let(:noteable) { create(:issue, project: project) } let(:noteable) { create(:issue, project: project) }
let(:issue) { noteable } 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 shared_examples_for 'a system note' do
let(:expected_noteable) { noteable } let(:expected_noteable) { noteable }
let(:commit_count) { nil } let(:commit_count) { nil }
...@@ -137,6 +145,8 @@ describe SystemNoteService do ...@@ -137,6 +145,8 @@ describe SystemNoteService do
end end
context 'when assignee added' do context 'when assignee added' do
it_behaves_like 'a note with overridable created_at'
it 'sets the note text' do it 'sets the note text' do
expect(subject.note).to eq "assigned to @#{assignee.username}" expect(subject.note).to eq "assigned to @#{assignee.username}"
end end
...@@ -145,6 +155,8 @@ describe SystemNoteService do ...@@ -145,6 +155,8 @@ describe SystemNoteService do
context 'when assignee removed' do context 'when assignee removed' do
let(:assignee) { nil } let(:assignee) { nil }
it_behaves_like 'a note with overridable created_at'
it 'sets the note text' do it 'sets the note text' do
expect(subject.note).to eq 'removed assignee' expect(subject.note).to eq 'removed assignee'
end end
...@@ -168,6 +180,8 @@ describe SystemNoteService do ...@@ -168,6 +180,8 @@ describe SystemNoteService do
described_class.change_issue_assignees(issue, project, author, old_assignees).note described_class.change_issue_assignees(issue, project, author, old_assignees).note
end 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 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}" expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}"
end end
...@@ -213,6 +227,8 @@ describe SystemNoteService do ...@@ -213,6 +227,8 @@ describe SystemNoteService do
expect(subject.note).to eq "changed milestone to #{reference}" expect(subject.note).to eq "changed milestone to #{reference}"
end end
it_behaves_like 'a note with overridable created_at'
end end
context 'when milestone removed' do context 'when milestone removed' do
...@@ -221,6 +237,8 @@ describe SystemNoteService do ...@@ -221,6 +237,8 @@ describe SystemNoteService do
it 'sets the note text' do it 'sets the note text' do
expect(subject.note).to eq 'removed milestone' expect(subject.note).to eq 'removed milestone'
end end
it_behaves_like 'a note with overridable created_at'
end end
end end
...@@ -237,6 +255,8 @@ describe SystemNoteService do ...@@ -237,6 +255,8 @@ describe SystemNoteService do
it 'sets the note text to use the milestone name' 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)}" expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}"
end end
it_behaves_like 'a note with overridable created_at'
end end
context 'when milestone removed' do context 'when milestone removed' do
...@@ -245,6 +265,8 @@ describe SystemNoteService do ...@@ -245,6 +265,8 @@ describe SystemNoteService do
it 'sets the note text' do it 'sets the note text' do
expect(subject.note).to eq 'removed milestone' expect(subject.note).to eq 'removed milestone'
end end
it_behaves_like 'a note with overridable created_at'
end end
end end
end end
...@@ -254,6 +276,8 @@ describe SystemNoteService do ...@@ -254,6 +276,8 @@ describe SystemNoteService do
let(:due_date) { Date.today } let(:due_date) { Date.today }
it_behaves_like 'a note with overridable created_at'
it_behaves_like 'a system note' do it_behaves_like 'a system note' do
let(:action) { 'due_date' } let(:action) { 'due_date' }
end end
...@@ -280,6 +304,8 @@ describe SystemNoteService do ...@@ -280,6 +304,8 @@ describe SystemNoteService do
let(:status) { 'reopened' } let(:status) { 'reopened' }
let(:source) { nil } let(:source) { nil }
it_behaves_like 'a note with overridable created_at'
it_behaves_like 'a system note' do it_behaves_like 'a system note' do
let(:action) { 'opened' } let(:action) { 'opened' }
end end
...@@ -289,6 +315,8 @@ describe SystemNoteService do ...@@ -289,6 +315,8 @@ describe SystemNoteService do
let(:status) { 'opened' } let(:status) { 'opened' }
let(:source) { double('commit', gfm_reference: 'commit 123456') } let(:source) { double('commit', gfm_reference: 'commit 123456') }
it_behaves_like 'a note with overridable created_at'
it 'sets the note text' do it 'sets the note text' do
expect(subject.note).to eq "#{status} via commit 123456" expect(subject.note).to eq "#{status} via commit 123456"
end end
...@@ -338,6 +366,8 @@ describe SystemNoteService do ...@@ -338,6 +366,8 @@ describe SystemNoteService do
let(:action) { 'title' } let(:action) { 'title' }
end end
it_behaves_like 'a note with overridable created_at'
it 'sets the note text' do it 'sets the note text' do
expect(subject.note) expect(subject.note)
.to eq "changed title from **{-Old title-}** to **{+Lorem ipsum+}**" .to eq "changed title from **{-Old title-}** to **{+Lorem ipsum+}**"
...@@ -353,6 +383,8 @@ describe SystemNoteService do ...@@ -353,6 +383,8 @@ describe SystemNoteService do
let(:action) { 'description' } let(:action) { 'description' }
end end
it_behaves_like 'a note with overridable created_at'
it 'sets the note text' do it 'sets the note text' do
expect(subject.note).to eq('changed the description') expect(subject.note).to eq('changed the description')
end end
...@@ -478,6 +510,8 @@ describe SystemNoteService do ...@@ -478,6 +510,8 @@ describe SystemNoteService do
let(:action) { 'cross_reference' } let(:action) { 'cross_reference' }
end end
it_behaves_like 'a note with overridable created_at'
describe 'note_body' do describe 'note_body' do
context 'cross-project' do context 'cross-project' do
let(:project2) { create(:project, :repository) } let(:project2) { create(:project, :repository) }
......
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