Merge branch '44949-do-not-update-updated_at-on-an-issue-when-reordering-it' into 'master'

Do not change updated_at on an issue when reordering on an issue board

See merge request gitlab-org/gitlab-ce!29677

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

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

changelogs/unreleased/44949-do-not-update-updated_at-on-an-issue-when-reordering-it.yml

+---
+title: Will not update issue timestamps when changing positions in a list
+merge_request: 29677
+author:
+type: changed

spec/support/shared_examples/services/boards/issues_move_service.rb

 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)
+
+      expect(issue.reload.assignees).to contain_exactly(assignee)
+    end
 
-      params.merge!(move_after_id: issue1.id, move_before_id: issue2.id)
+    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