From f0f3246a2b06a4e0edd7d6e309a21aa559118921 Mon Sep 17 00:00:00 2001 From: Eugenia Grieff Date: Fri, 28 Jun 2019 09:47:54 +0000 Subject: [PATCH] Do not change updated_at on an issue when reordering on an issue board --- app/models/concerns/relative_positioning.rb | 4 +- app/services/issuable_base_service.rb | 13 +++- ...ated_at-on-an-issue-when-reordering-it.yml | 5 ++ .../services/boards/issues_move_service.rb | 62 ++++++++++++++++--- 4 files changed, 71 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/44949-do-not-update-updated_at-on-an-issue-when-reordering-it.yml diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index 46d2c345758..22b6b1d720c 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/app/models/concerns/relative_positioning.rb @@ -25,7 +25,7 @@ module RelativePositioning relative_position = position_between(max_relative_position, MAX_POSITION) object.relative_position = relative_position max_relative_position = relative_position - object.save + object.save(touch: false) end end end @@ -159,7 +159,7 @@ module RelativePositioning def save_positionable_neighbours return unless @positionable_neighbours - status = @positionable_neighbours.all?(&:save) + status = @positionable_neighbours.all? { |issue| issue.save(touch: false) } @positionable_neighbours = nil status diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 26132f1824a..02de080e0ba 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -205,7 +205,7 @@ class IssuableBaseService < BaseService end 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) issuable.assign_attributes(last_edited_at: Time.now, last_edited_by: current_user) @@ -213,11 +213,16 @@ class IssuableBaseService < BaseService 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 # the changed fields upon calling #save. 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 ActiveRecord::Base.no_touching do 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) issuable.milestone_id = nil unless issuable.milestone_available? end + + def update_timestamp?(issuable) + issuable.changes.keys != ["relative_position"] + end end diff --git a/changelogs/unreleased/44949-do-not-update-updated_at-on-an-issue-when-reordering-it.yml b/changelogs/unreleased/44949-do-not-update-updated_at-on-an-issue-when-reordering-it.yml new file mode 100644 index 00000000000..efc6af7845c --- /dev/null +++ b/changelogs/unreleased/44949-do-not-update-updated_at-on-an-issue-when-reordering-it.yml @@ -0,0 +1,5 @@ +--- +title: Will not update issue timestamps when changing positions in a list +merge_request: 29677 +author: +type: changed diff --git a/spec/support/shared_examples/services/boards/issues_move_service.rb b/spec/support/shared_examples/services/boards/issues_move_service.rb index 9dbd1d8e867..5359831f8f8 100644 --- a/spec/support/shared_examples/services/boards/issues_move_service.rb +++ b/spec/support/shared_examples/services/boards/issues_move_service.rb @@ -1,8 +1,17 @@ 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 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 } } + it_behaves_like 'updating timestamps' + it 'delegates the label changes to Issues::UpdateService' do service = double(: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(: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 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(: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 described_class.new(parent, user, params).execute(issue) issue.reload @@ -59,6 +72,8 @@ shared_examples 'issues move service' do |group| 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 } } + it_behaves_like 'updating timestamps' + it 'delegates the re-open proceedings to Issues::ReopenService' do expect_any_instance_of(Issues::ReopenService).to receive(:execute).with(issue).once @@ -75,10 +90,13 @@ shared_examples 'issues move service' do |group| end context 'when moving to same list' do - let(:issue) { create(:labeled_issue, project: project, labels: [bug, development]) } - let(:issue1) { create(:labeled_issue, project: project, labels: [bug, development]) } - let(:issue2) { 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(:assignee) { create(:user) } + let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } } + let(:issue1) { create(:labeled_issue, project: project, labels: [bug, development]) } + 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 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) end - it 'sorts issues' do - [issue, issue1, issue2].each do |issue| - issue.move_to_end && issue.save! - end + it 'keeps issues assignees' do + described_class.new(parent, user, params).execute(issue) - 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) expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position) 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 context 'when on a group board' do it 'sends the board_group_id parameter' do @@ -114,5 +150,13 @@ shared_examples 'issues move service' do |group| 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