Do not change updated_at on an issue when reordering on an issue board
This commit is contained in:
parent
dbd86f0059
commit
f0f3246a2b
|
@ -25,7 +25,7 @@ module RelativePositioning
|
||||||
relative_position = position_between(max_relative_position, MAX_POSITION)
|
relative_position = position_between(max_relative_position, MAX_POSITION)
|
||||||
object.relative_position = relative_position
|
object.relative_position = relative_position
|
||||||
max_relative_position = relative_position
|
max_relative_position = relative_position
|
||||||
object.save
|
object.save(touch: false)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -159,7 +159,7 @@ module RelativePositioning
|
||||||
def save_positionable_neighbours
|
def save_positionable_neighbours
|
||||||
return unless @positionable_neighbours
|
return unless @positionable_neighbours
|
||||||
|
|
||||||
status = @positionable_neighbours.all?(&:save)
|
status = @positionable_neighbours.all? { |issue| issue.save(touch: false) }
|
||||||
@positionable_neighbours = nil
|
@positionable_neighbours = nil
|
||||||
|
|
||||||
status
|
status
|
||||||
|
|
|
@ -205,7 +205,7 @@ class IssuableBaseService < BaseService
|
||||||
end
|
end
|
||||||
|
|
||||||
if issuable.changed? || params.present?
|
if issuable.changed? || params.present?
|
||||||
issuable.assign_attributes(params.merge(updated_by: current_user))
|
issuable.assign_attributes(params)
|
||||||
|
|
||||||
if has_title_or_description_changed?(issuable)
|
if has_title_or_description_changed?(issuable)
|
||||||
issuable.assign_attributes(last_edited_at: Time.now, last_edited_by: current_user)
|
issuable.assign_attributes(last_edited_at: Time.now, last_edited_by: current_user)
|
||||||
|
@ -213,11 +213,16 @@ class IssuableBaseService < BaseService
|
||||||
|
|
||||||
before_update(issuable)
|
before_update(issuable)
|
||||||
|
|
||||||
|
# Do not touch when saving the issuable if only changes position within a list. We should call
|
||||||
|
# this method at this point to capture all possible changes.
|
||||||
|
should_touch = update_timestamp?(issuable)
|
||||||
|
|
||||||
|
issuable.updated_by = current_user if should_touch
|
||||||
# We have to perform this check before saving the issuable as Rails resets
|
# We have to perform this check before saving the issuable as Rails resets
|
||||||
# the changed fields upon calling #save.
|
# the changed fields upon calling #save.
|
||||||
update_project_counters = issuable.project && update_project_counter_caches?(issuable)
|
update_project_counters = issuable.project && update_project_counter_caches?(issuable)
|
||||||
|
|
||||||
if issuable.with_transaction_returning_status { issuable.save }
|
if issuable.with_transaction_returning_status { issuable.save(touch: should_touch) }
|
||||||
# We do not touch as it will affect a update on updated_at field
|
# We do not touch as it will affect a update on updated_at field
|
||||||
ActiveRecord::Base.no_touching do
|
ActiveRecord::Base.no_touching do
|
||||||
Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels: old_associations[:labels])
|
Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels: old_associations[:labels])
|
||||||
|
@ -402,4 +407,8 @@ class IssuableBaseService < BaseService
|
||||||
def ensure_milestone_available(issuable)
|
def ensure_milestone_available(issuable)
|
||||||
issuable.milestone_id = nil unless issuable.milestone_available?
|
issuable.milestone_id = nil unless issuable.milestone_available?
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def update_timestamp?(issuable)
|
||||||
|
issuable.changes.keys != ["relative_position"]
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Will not update issue timestamps when changing positions in a list
|
||||||
|
merge_request: 29677
|
||||||
|
author:
|
||||||
|
type: changed
|
|
@ -1,8 +1,17 @@
|
||||||
shared_examples 'issues move service' do |group|
|
shared_examples 'issues move service' do |group|
|
||||||
|
shared_examples 'updating timestamps' do
|
||||||
|
it 'updates updated_at' do
|
||||||
|
expect {described_class.new(parent, user, params).execute(issue)}
|
||||||
|
.to change {issue.reload.updated_at}
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context 'when moving an issue between lists' do
|
context 'when moving an issue between lists' do
|
||||||
let(:issue) { create(:labeled_issue, project: project, labels: [bug, development]) }
|
let(:issue) { create(:labeled_issue, project: project, labels: [bug, development]) }
|
||||||
let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } }
|
let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list2.id } }
|
||||||
|
|
||||||
|
it_behaves_like 'updating timestamps'
|
||||||
|
|
||||||
it 'delegates the label changes to Issues::UpdateService' do
|
it 'delegates the label changes to Issues::UpdateService' do
|
||||||
service = double(:service)
|
service = double(:service)
|
||||||
expect(Issues::UpdateService).to receive(:new).and_return(service)
|
expect(Issues::UpdateService).to receive(:new).and_return(service)
|
||||||
|
@ -24,6 +33,8 @@ shared_examples 'issues move service' do |group|
|
||||||
let(:issue) { create(:labeled_issue, project: project, labels: [bug, development, testing, regression]) }
|
let(:issue) { create(:labeled_issue, project: project, labels: [bug, development, testing, regression]) }
|
||||||
let(:params) { { board_id: board1.id, from_list_id: list2.id, to_list_id: closed.id } }
|
let(:params) { { board_id: board1.id, from_list_id: list2.id, to_list_id: closed.id } }
|
||||||
|
|
||||||
|
it_behaves_like 'updating timestamps'
|
||||||
|
|
||||||
it 'delegates the close proceedings to Issues::CloseService' do
|
it 'delegates the close proceedings to Issues::CloseService' do
|
||||||
expect_any_instance_of(Issues::CloseService).to receive(:execute).with(issue).once
|
expect_any_instance_of(Issues::CloseService).to receive(:execute).with(issue).once
|
||||||
|
|
||||||
|
@ -46,6 +57,8 @@ shared_examples 'issues move service' do |group|
|
||||||
let(:issue) { create(:labeled_issue, project: project, labels: [bug, development, testing, regression], milestone: milestone) }
|
let(:issue) { create(:labeled_issue, project: project, labels: [bug, development, testing, regression], milestone: milestone) }
|
||||||
let(:params) { { board_id: board1.id, from_list_id: list2.id, to_list_id: backlog.id } }
|
let(:params) { { board_id: board1.id, from_list_id: list2.id, to_list_id: backlog.id } }
|
||||||
|
|
||||||
|
it_behaves_like 'updating timestamps'
|
||||||
|
|
||||||
it 'keeps labels and milestone' do
|
it 'keeps labels and milestone' do
|
||||||
described_class.new(parent, user, params).execute(issue)
|
described_class.new(parent, user, params).execute(issue)
|
||||||
issue.reload
|
issue.reload
|
||||||
|
@ -59,6 +72,8 @@ shared_examples 'issues move service' do |group|
|
||||||
let(:issue) { create(:labeled_issue, :closed, project: project, labels: [bug]) }
|
let(:issue) { create(:labeled_issue, :closed, project: project, labels: [bug]) }
|
||||||
let(:params) { { board_id: board1.id, from_list_id: closed.id, to_list_id: list2.id } }
|
let(:params) { { board_id: board1.id, from_list_id: closed.id, to_list_id: list2.id } }
|
||||||
|
|
||||||
|
it_behaves_like 'updating timestamps'
|
||||||
|
|
||||||
it 'delegates the re-open proceedings to Issues::ReopenService' do
|
it 'delegates the re-open proceedings to Issues::ReopenService' do
|
||||||
expect_any_instance_of(Issues::ReopenService).to receive(:execute).with(issue).once
|
expect_any_instance_of(Issues::ReopenService).to receive(:execute).with(issue).once
|
||||||
|
|
||||||
|
@ -75,10 +90,13 @@ shared_examples 'issues move service' do |group|
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when moving to same list' do
|
context 'when moving to same list' do
|
||||||
let(:issue) { create(:labeled_issue, project: project, labels: [bug, development]) }
|
let(:assignee) { create(:user) }
|
||||||
let(:issue1) { create(:labeled_issue, project: project, labels: [bug, development]) }
|
let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
|
||||||
let(:issue2) { create(:labeled_issue, project: project, labels: [bug, development]) }
|
let(:issue1) { create(:labeled_issue, project: project, labels: [bug, development]) }
|
||||||
let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
|
let(:issue2) { create(:labeled_issue, project: project, labels: [bug, development]) }
|
||||||
|
let(:issue) do
|
||||||
|
create(:labeled_issue, project: project, labels: [bug, development], assignees: [assignee])
|
||||||
|
end
|
||||||
|
|
||||||
it 'returns false' do
|
it 'returns false' do
|
||||||
expect(described_class.new(parent, user, params).execute(issue)).to eq false
|
expect(described_class.new(parent, user, params).execute(issue)).to eq false
|
||||||
|
@ -90,18 +108,36 @@ shared_examples 'issues move service' do |group|
|
||||||
expect(issue.reload.labels).to contain_exactly(bug, development)
|
expect(issue.reload.labels).to contain_exactly(bug, development)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'sorts issues' do
|
it 'keeps issues assignees' do
|
||||||
[issue, issue1, issue2].each do |issue|
|
described_class.new(parent, user, params).execute(issue)
|
||||||
issue.move_to_end && issue.save!
|
|
||||||
end
|
|
||||||
|
|
||||||
params.merge!(move_after_id: issue1.id, move_before_id: issue2.id)
|
expect(issue.reload.assignees).to contain_exactly(assignee)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'sorts issues' do
|
||||||
|
reorder_issues(params, issues: [issue, issue1, issue2])
|
||||||
|
|
||||||
described_class.new(parent, user, params).execute(issue)
|
described_class.new(parent, user, params).execute(issue)
|
||||||
|
|
||||||
expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
|
expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'does not update updated_at' do
|
||||||
|
reorder_issues(params, issues: [issue, issue1, issue2])
|
||||||
|
|
||||||
|
updated_at = issue.updated_at
|
||||||
|
updated_at1 = issue1.updated_at
|
||||||
|
updated_at2 = issue2.updated_at
|
||||||
|
|
||||||
|
Timecop.travel(1.minute.from_now) do
|
||||||
|
described_class.new(parent, user, params).execute(issue)
|
||||||
|
end
|
||||||
|
|
||||||
|
expect(issue.reload.updated_at.change(usec: 0)).to eq updated_at.change(usec: 0)
|
||||||
|
expect(issue1.reload.updated_at.change(usec: 0)).to eq updated_at1.change(usec: 0)
|
||||||
|
expect(issue2.reload.updated_at.change(usec: 0)).to eq updated_at2.change(usec: 0)
|
||||||
|
end
|
||||||
|
|
||||||
if group
|
if group
|
||||||
context 'when on a group board' do
|
context 'when on a group board' do
|
||||||
it 'sends the board_group_id parameter' do
|
it 'sends the board_group_id parameter' do
|
||||||
|
@ -114,5 +150,13 @@ shared_examples 'issues move service' do |group|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def reorder_issues(params, issues: [])
|
||||||
|
issues.each do |issue|
|
||||||
|
issue.move_to_end && issue.save!
|
||||||
|
end
|
||||||
|
|
||||||
|
params.merge!(move_after_id: issues[1].id, move_before_id: issues[2].id)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue