Merge branch '46478-update-updated-at-on-mr' into 'master'

Resolve "Update `updated_at` on an issue/mr on every issue/mr changes"

Closes #46478

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

11 files changed, 43 insertions, 18 deletions

diff --git a/app/models/concerns/time_trackable.rb b/app/models/concerns/time_trackable.rb
index 1caf47072bc..0fc321c52bc 100644
--- a/app/models/concerns/time_trackable.rb
+++ b/app/models/concerns/time_trackable.rb
@@ -30,8 +30,6 @@ module TimeTrackable
 
     return if @time_spent == 0
 
-    touch if touchable?
-
     if @time_spent == :reset
       reset_spent_time
     else
@@ -59,10 +57,6 @@ module TimeTrackable
 
   private
 
-  def touchable?
-    valid? && persisted?
-  end
-
   def reset_spent_time
     timelogs.new(time_spent: total_time_spent * -1, user: @time_spent_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end
diff --git a/app/models/timelog.rb b/app/models/timelog.rb
index e166cf69703..f4c5c581a11 100644
--- a/app/models/timelog.rb
+++ b/app/models/timelog.rb
@@ -2,8 +2,8 @@ class Timelog < ActiveRecord::Base
   validates :time_spent, :user, presence: true
   validate :issuable_id_is_present
 
-  belongs_to :issue
-  belongs_to :merge_request
+  belongs_to :issue, touch: true
+  belongs_to :merge_request, touch: true
   belongs_to :user
 
   def issuable
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 1f67e3ecf9d..683f64e82ad 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -183,7 +183,10 @@ class IssuableBaseService < BaseService
     old_associations = associations_before_update(issuable)
 
     label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
-    params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids)
+    if labels_changing?(issuable.label_ids, label_ids)
+      params[:label_ids] = label_ids
+      issuable.touch
+    end
 
     if issuable.changed? || params.present?
       issuable.assign_attributes(params.merge(updated_by: current_user))
diff --git a/changelogs/unreleased/46478-update-updated-at-on-mr.yml b/changelogs/unreleased/46478-update-updated-at-on-mr.yml
new file mode 100644
index 00000000000..c58b4fc8f84
--- /dev/null
+++ b/changelogs/unreleased/46478-update-updated-at-on-mr.yml
@@ -0,0 +1,5 @@
+---
+title: Updates updated_at on label changes
+merge_request: 19065
+author: Jacopo Beschi @jacopo-beschi
+type: fixed
diff --git a/spec/factories/issues.rb b/spec/factories/issues.rb
index 998080a3dd5..3a35bdd25de 100644
--- a/spec/factories/issues.rb
+++ b/spec/factories/issues.rb
@@ -3,6 +3,7 @@ FactoryBot.define do
     title { generate(:title) }
     project
     author { project.creator }
+    updated_by { author }
 
     trait :confidential do
       confidential true
diff --git a/spec/fixtures/api/schemas/entities/issue.json b/spec/fixtures/api/schemas/entities/issue.json
index 38467b4ca20..00abe73ec8a 100644
--- a/spec/fixtures/api/schemas/entities/issue.json
+++ b/spec/fixtures/api/schemas/entities/issue.json
@@ -27,7 +27,7 @@
     "due_date": { "type": "date" },
     "confidential": { "type": "boolean" },
     "discussion_locked": { "type": ["boolean", "null"] },
-    "updated_by_id": { "type": ["string", "null"] },
+    "updated_by_id": { "type": ["integer", "null"] },
     "time_estimate": { "type": "integer" },
     "total_time_spent": { "type": "integer" },
     "human_time_estimate": { "type": ["integer", "null"] },
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index bd6bf5b0712..1cfd526834c 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -12,6 +12,7 @@ describe Issuable do
     it { is_expected.to belong_to(:author) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
     it { is_expected.to have_many(:todos).dependent(:destroy) }
+    it { is_expected.to have_many(:labels) }
 
     context 'Notes' do
       let!(:note) { create(:note, noteable: issue, project: issue.project) }
@@ -274,8 +275,8 @@ describe Issuable do
 
     it 'skips coercion for not Integer values' do
       expect { issue.time_estimate = nil }.to change { issue.time_estimate }.to(nil)
-      expect { issue.time_estimate = 'invalid time' }.not_to raise_error(StandardError)
-      expect { issue.time_estimate = 22.33 }.not_to raise_error(StandardError)
+      expect { issue.time_estimate = 'invalid time' }.not_to raise_error
+      expect { issue.time_estimate = 22.33 }.not_to raise_error
     end
   end
 
diff --git a/spec/models/timelog_spec.rb b/spec/models/timelog_spec.rb
index 6e30798356c..a0c93c531ea 100644
--- a/spec/models/timelog_spec.rb
+++ b/spec/models/timelog_spec.rb
@@ -5,6 +5,9 @@ RSpec.describe Timelog do
   let(:issue) { create(:issue) }
   let(:merge_request) { create(:merge_request) }
 
+  it { is_expected.to belong_to(:issue).touch(true) }
+  it { is_expected.to belong_to(:merge_request).touch(true) }
+
   it { is_expected.to be_valid }
 
   it { is_expected.to validate_presence_of(:time_spent) }
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 3106083293f..4181f4ebbbe 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -1351,19 +1351,25 @@ describe API::Issues do
       expect(json_response['labels']).to eq([label.title])
     end
 
-    it 'removes all labels' do
-      put api("/projects/#{project.id}/issues/#{issue.iid}", user), labels: ''
+    it 'removes all labels and touches the record' do
+      Timecop.travel(1.minute.from_now) do
+        put api("/projects/#{project.id}/issues/#{issue.iid}", user), labels: ''
+      end
 
       expect(response).to have_gitlab_http_status(200)
       expect(json_response['labels']).to eq([])
+      expect(json_response['updated_at']).to be > Time.now
     end
 
-    it 'updates labels' do
-      put api("/projects/#{project.id}/issues/#{issue.iid}", user),
+    it 'updates labels and touches the record' do
+      Timecop.travel(1.minute.from_now) do
+        put api("/projects/#{project.id}/issues/#{issue.iid}", user),
           labels: 'foo,bar'
+      end
       expect(response).to have_gitlab_http_status(200)
       expect(json_response['labels']).to include 'foo'
       expect(json_response['labels']).to include 'bar'
+      expect(json_response['updated_at']).to be > Time.now
     end
 
     it 'allows special label names' do
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 23b1134b5a3..158541d36e3 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -337,12 +337,18 @@ describe Issues::UpdateService, :mailer do
 
       context 'when the labels change' do
         before do
-          update_issue(label_ids: [label.id])
+          Timecop.freeze(1.minute.from_now) do
+            update_issue(label_ids: [label.id])
+          end
         end
 
         it 'marks todos as done' do
           expect(todo.reload.done?).to eq true
         end
+
+        it 'updates updated_at' do
+          expect(issue.reload.updated_at).to be > Time.now
+        end
       end
     end
 
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 5279ea6164e..bd2e91f1f7a 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -326,12 +326,18 @@ describe MergeRequests::UpdateService, :mailer do
 
       context 'when the labels change' do
         before do
-          update_merge_request({ label_ids: [label.id] })
+          Timecop.freeze(1.minute.from_now) do
+            update_merge_request({ label_ids: [label.id] })
+          end
         end
 
         it 'marks pending todos as done' do
           expect(pending_todo.reload).to be_done
         end
+
+        it 'updates updated_at' do
+          expect(merge_request.reload.updated_at).to be > Time.now
+        end
       end
 
       context 'when the assignee changes' do