From 4c8783636cdc279aea802760146d58e6259bed57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=99=88=20=20jacopo=20beschi=20=F0=9F=99=89?= Date: Fri, 1 Jun 2018 15:09:08 +0000 Subject: Resolve "Update `updated_at` on an issue/mr on every issue/mr changes" --- app/models/concerns/time_trackable.rb | 6 ------ app/models/timelog.rb | 4 ++-- app/services/issuable_base_service.rb | 5 ++++- changelogs/unreleased/46478-update-updated-at-on-mr.yml | 5 +++++ spec/factories/issues.rb | 1 + spec/fixtures/api/schemas/entities/issue.json | 2 +- spec/models/concerns/issuable_spec.rb | 5 +++-- spec/models/timelog_spec.rb | 3 +++ spec/requests/api/issues_spec.rb | 14 ++++++++++---- spec/services/issues/update_service_spec.rb | 8 +++++++- spec/services/merge_requests/update_service_spec.rb | 8 +++++++- 11 files changed, 43 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/46478-update-updated-at-on-mr.yml 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 -- cgit v1.2.1