diff --git a/ee/app/models/ee/epic.rb b/ee/app/models/ee/epic.rb index 89500ab130ae041b3345a55741f4baba37526432..a3df1af4917cd37fac58216bc80bdf3321031ee1 100644 --- a/ee/app/models/ee/epic.rb +++ b/ee/app/models/ee/epic.rb @@ -733,6 +733,16 @@ def exportable_restricted_associations super + [:notes] end + # When syncing an epic and its associated work item the column updated_at may become out of sync due to + # further actions executed outside the nested transaction (e.g. when creating system notes). + # To account for this mismatch we call this method on the updated epic to set the same updated_at value on the + # work item while bypassing validations and callbacks + def sync_work_item_updated_at + return unless work_item && group.epic_sync_to_work_item_enabled? + + work_item.update_column(:updated_at, updated_at) + end + private def validate_parent_epic diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb index 993d419b7ebca17d9bfd68fea2c587adc8e1737c..9304ffc01924d520b84913ca0711bb8c3b775b13 100644 --- a/ee/app/models/ee/note.rb +++ b/ee/app/models/ee/note.rb @@ -89,6 +89,20 @@ def skip_notification? for_vulnerability? || super end + override :touch_noteable + def touch_noteable + return super unless for_epic? + + assoc = association(:noteable) + noteable_object = assoc.loaded? ? noteable : assoc.scope.select(:id, :updated_at, :issue_id, :group_id, :iid).take + + noteable_object&.touch + # Ensure epic and work items are kept in sync after creating notes on the epic + noteable_object&.sync_work_item_updated_at + + noteable_object + end + def usage_ping_track_updated_epic_note(user) return unless for_epic? diff --git a/ee/app/services/epics/create_service.rb b/ee/app/services/epics/create_service.rb index 77f9e508ba3278a96d137f79bf78116155688485..1232f5c2301b52b52ef15e95e968ec4fef484e99 100644 --- a/ee/app/services/epics/create_service.rb +++ b/ee/app/services/epics/create_service.rb @@ -36,7 +36,8 @@ def transaction_create(epic) work_item.relative_position = epic.id work_item.title_html = epic.title_html work_item.description_html = epic.description_html - work_item.save! + work_item.updated_at = epic.updated_at + work_item.save!(touch: false) end end diff --git a/ee/app/services/epics/reopen_service.rb b/ee/app/services/epics/reopen_service.rb index 63901ea3abe17a1ee3ef083c6aaf525c53ba96d3..1819edf986ed4ad0a9c26298601fcb0f5f67faef 100644 --- a/ee/app/services/epics/reopen_service.rb +++ b/ee/app/services/epics/reopen_service.rb @@ -18,6 +18,7 @@ def reopen_epic(epic) next true unless work_item work_item.reopen! + epic.sync_work_item_updated_at end rescue StateMachines::InvalidTransition diff --git a/ee/spec/features/epics/user_uses_quick_actions_spec.rb b/ee/spec/features/epics/user_uses_quick_actions_spec.rb index c16268b484510c3717a709a1c938116598ee57df..96201681a39d87c0519529e260a06601bc9dc2a3 100644 --- a/ee/spec/features/epics/user_uses_quick_actions_spec.rb +++ b/ee/spec/features/epics/user_uses_quick_actions_spec.rb @@ -23,7 +23,7 @@ it 'applies quick action' do # TODO: remove threshold after epic-work item sync # issue: https://gitlab.com/gitlab-org/gitlab/-/issues/438295 - allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(110) + allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(112) epic_2 = create(:epic, group: group) visit group_epic_path(group, epic_2) wait_for_requests diff --git a/ee/spec/models/epic_spec.rb b/ee/spec/models/epic_spec.rb index 80564014135e1b33f78f0611354b7be22f058b46..c97691f9f5d7b409b8323befdd2aa8e1b3e3bee5 100644 --- a/ee/spec/models/epic_spec.rb +++ b/ee/spec/models/epic_spec.rb @@ -1558,4 +1558,26 @@ def as_item(item) expect(epic4.linked_items_count).to eq(0) end end + + describe '#sync_work_item_updated_at' do + let_it_be(:epic) { create(:epic, group: group) } + + it 'updates the updated_at column on the work item' do + expect(epic.work_item).to receive(:update_column).with(:updated_at, epic.updated_at).once + + epic.sync_work_item_updated_at + end + + context 'when sync_epic_to_work_item feature flag is disabled' do + before do + stub_feature_flags(sync_epic_to_work_item: false) + end + + it 'does not update the updated_at column on the work item' do + expect(epic.work_item).not_to receive(:update_column) + + epic.sync_work_item_updated_at + end + end + end end diff --git a/ee/spec/models/note_spec.rb b/ee/spec/models/note_spec.rb index 8355dfb7baa6c7914ad29633ac36662e17e3b9f6..2b6d05affd2555182d5af88b7d3a9daea999eff6 100644 --- a/ee/spec/models/note_spec.rb +++ b/ee/spec/models/note_spec.rb @@ -31,6 +31,55 @@ end end + describe 'callbacks' do + describe '#touch_noteable' do + it 'calls #touch on the noteable' do + noteable = create(:issue) + note = build(:note, project: noteable.project, noteable: noteable) + + expect(note).to receive(:touch_noteable).and_call_original + expect(note.noteable).to receive(:touch) + + note.save! + end + + context 'when noteable is an epic' do + let_it_be(:noteable) { create(:epic) } + let(:note) { build(:note, project: nil, noteable: noteable) } + let(:noteable_association) { note.association(:noteable) } + + before do + allow(noteable_association).to receive(:loaded?).and_return(object_loaded) + allow(note).to receive(:touch_noteable).and_call_original + end + + context 'when noteable is loaded' do + let(:object_loaded) { true } + + it 'calls #touch and #sync_work_item_updated_at on the noteable' do + expect(note.noteable).to receive(:touch) + expect(note.noteable).to receive(:sync_work_item_updated_at) + + note.save! + end + end + + context 'when noteable is not loaded' do + let(:object_loaded) { false } + + it 'calls #touch and #sync_work_item_updated_at on the noteable' do + expect_any_instance_of(::Epic) do |epic| + expect(epic).to receive(:touch) + expect(epic).to receive(:sync_work_item_updated_at) + end + + note.save! + end + end + end + end + end + describe '#ensure_namespace_id' do context 'for an epic note' do let_it_be(:epic) { create(:epic) } diff --git a/ee/spec/requests/api/epics_spec.rb b/ee/spec/requests/api/epics_spec.rb index 7904bff500ded34a09c84cf7bca3bef4be5da0af..e60c5c634123d8939d9bcb52aa831834add65d9f 100644 --- a/ee/spec/requests/api/epics_spec.rb +++ b/ee/spec/requests/api/epics_spec.rb @@ -655,6 +655,9 @@ context 'when epics feature is enabled' do before do + # TODO: remove threshold after epic-work item sync + # issue: https://gitlab.com/gitlab-org/gitlab/-/issues/438295 + allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(103) stub_licensed_features(epics: true, subepics: true) group.add_developer(user) end @@ -673,7 +676,6 @@ context 'when the request is correct' do before do - allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(101) post api(url, user), params: params end @@ -823,7 +825,7 @@ context 'with rate limiter', :freeze_time, :clean_gitlab_redis_rate_limiting do before do - allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(101) + allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(103) stub_application_setting(issues_create_limit: 1) end @@ -875,7 +877,7 @@ it 'creates a new epic with labels param as array' do # TODO: remove threshold after epic-work item sync # issue: https://gitlab.com/gitlab-org/gitlab/-/issues/438295 - allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(136) + allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(138) params[:labels] = ['label1', 'label2', 'foo, bar', '&,?'] post api(url, user), params: params @@ -926,7 +928,7 @@ stub_licensed_features(epics: true, subepics: true) # TODO: reduce threshold after epic-work item sync # issue: https://gitlab.com/gitlab-org/gitlab/-/issues/438295 - allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(148) + allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(153) end it_behaves_like 'PUT request permissions for admin mode' do @@ -969,7 +971,7 @@ before do # TODO: reduce threshold after epic-work item sync # issue: https://gitlab.com/gitlab-org/gitlab/-/issues/438295 - allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(175) + allow(Gitlab::QueryLimiting::Transaction).to receive(:threshold).and_return(180) group.add_developer(user) end diff --git a/ee/spec/services/epic_issues/create_service_spec.rb b/ee/spec/services/epic_issues/create_service_spec.rb index b3fdb3aac63524be44cb70100f1eb99b63367fdc..5daf5d4bdda48672c693238afdb06a0c4e6c6e91 100644 --- a/ee/spec/services/epic_issues/create_service_spec.rb +++ b/ee/spec/services/epic_issues/create_service_spec.rb @@ -226,6 +226,13 @@ def assign_issue(references) expect(created_link.relative_position).to eq(epic.work_item.child_links[0].relative_position) end + it 'keeps epic timestamps in sync' do + expect { subject }.to change { EpicIssue.count }.by(1) + .and(change { WorkItems::ParentLink.count }.by(1)) + + expect(epic.updated_at).to eq(epic.work_item.updated_at) + end + context 'when work item already has a parent' do before do create(:epic_issue, epic: epic, issue: issue) diff --git a/ee/spec/services/epic_issues/destroy_service_spec.rb b/ee/spec/services/epic_issues/destroy_service_spec.rb index 08a90e512a2fad1b392289249bd40d7c65e63e20..f344b0af761ea8b92f9b899de72e21bd6d29c741 100644 --- a/ee/spec/services/epic_issues/destroy_service_spec.rb +++ b/ee/spec/services/epic_issues/destroy_service_spec.rb @@ -110,9 +110,11 @@ allow(Epics::UpdateDatesService).to receive(:new).and_call_original end - it 'removes the epic and work item link' do + it 'removes the epic and work item link and keep epic in sync' do expect { subject }.to change { EpicIssue.count }.by(-1) .and(change { WorkItems::ParentLink.count }.by(-1)) + + expect(epic.reload.updated_at).to eq(epic.work_item.updated_at) end context 'when feature flag is disabled' do diff --git a/ee/spec/services/epics/epic_links/create_service_spec.rb b/ee/spec/services/epics/epic_links/create_service_spec.rb index bc818ae2dac163400cc4cd48055e87c76423f4fc..29a0075ca39d19b3374321c00b37fb9178ffeb12 100644 --- a/ee/spec/services/epics/epic_links/create_service_spec.rb +++ b/ee/spec/services/epics/epic_links/create_service_spec.rb @@ -573,6 +573,14 @@ def add_epic(references) expect(child_epic.reload.relative_position).to eq(child_work_item.parent_link.reload.relative_position) end + it 'keeps epics timestamp in sync' do + expect { create_link }.to change { parent_epic.children.count }.by(1) + .and(change { WorkItems::ParentLink.count }.by(1)) + + expect(parent_epic.reload.updated_at).to eq(parent_epic.work_item.updated_at) + expect(child_epic.reload.updated_at).to eq(child_epic.work_item.updated_at) + end + it 'does not create resource event for the work item' do expect(WorkItems::ResourceLinkEvent).not_to receive(:create) @@ -604,11 +612,13 @@ def add_epic(references) expect(child_epic2.reload.relative_position).to eq(child_work_item2.reload.parent_link.relative_position) end - it 'does not create resource event for the work item' do - expect(WorkItems::ResourceLinkEvent).not_to receive(:create) - + it 'keeps epics timestamp in sync' do expect { create_link }.to change { parent_epic.children.count }.by(2) .and(change { WorkItems::ParentLink.count }.by(2)) + + expect(parent_epic.reload.updated_at).to eq(parent_epic.work_item.updated_at) + expect(child_epic.reload.updated_at).to eq(child_epic.work_item.updated_at) + expect(child_epic2.reload.updated_at).to eq(child_epic2.work_item.updated_at) end it 'creates system notes only for the epics' do diff --git a/ee/spec/services/epics/epic_links/destroy_service_spec.rb b/ee/spec/services/epics/epic_links/destroy_service_spec.rb index 9872016461bf3f2cad8950b8affc60e67a690874..4dd66c34b125984ec3bd5da9fe199207be7fe149 100644 --- a/ee/spec/services/epics/epic_links/destroy_service_spec.rb +++ b/ee/spec/services/epics/epic_links/destroy_service_spec.rb @@ -133,6 +133,8 @@ def remove_epic_relation(child_epic) expect(parent_epic.reload.children).not_to include(child_epic) expect(parent.reload.work_item_children).not_to include(child) + expect(parent_epic.updated_at).to eq(parent_epic.work_item.updated_at) + expect(child_epic.updated_at).to eq(child_epic.work_item.updated_at) end it 'does not create resource event for the work item' do diff --git a/ee/spec/services/epics/related_epic_links/create_service_spec.rb b/ee/spec/services/epics/related_epic_links/create_service_spec.rb index 228de60b47125bdd28cacd3d6ea035de0fea6593..436f12f6e83dce9f70bd7bc9815cc47deea531bb 100644 --- a/ee/spec/services/epics/related_epic_links/create_service_spec.rb +++ b/ee/spec/services/epics/related_epic_links/create_service_spec.rb @@ -152,6 +152,9 @@ expect(WorkItems::RelatedWorkItemLink.find_by!(target: epic_b.work_item)) .to have_attributes(source: epic_a.work_item, link_type: IssuableLink::TYPE_RELATES_TO) + + expect(epic_a.reload.updated_at).to eq(epic_a.work_item.updated_at) + expect(epic_b.reload.updated_at).to eq(epic_b.work_item.updated_at) end context 'when link type is blocking' do diff --git a/ee/spec/services/epics/related_epic_links/destroy_service_spec.rb b/ee/spec/services/epics/related_epic_links/destroy_service_spec.rb index 639baf6cc76d6fbb07448ec664a078ee0a0438df..fde1eaaacf04ed81afde27913cf38a8491687c18 100644 --- a/ee/spec/services/epics/related_epic_links/destroy_service_spec.rb +++ b/ee/spec/services/epics/related_epic_links/destroy_service_spec.rb @@ -55,6 +55,8 @@ expect(source.reload.work_item.notes).to be_empty expect(target.reload.work_item.notes).to be_empty + expect(source.updated_at).to eq(source.work_item.updated_at) + expect(target.updated_at).to eq(target.work_item.updated_at) end end diff --git a/ee/spec/services/epics/tree_reorder_service_spec.rb b/ee/spec/services/epics/tree_reorder_service_spec.rb index 6d0970c71a4e8dfc9894fee86cd96e4f978eb159..656c3f53042d0a8fd50a69e92078443c1ecf6412 100644 --- a/ee/spec/services/epics/tree_reorder_service_spec.rb +++ b/ee/spec/services/epics/tree_reorder_service_spec.rb @@ -270,6 +270,13 @@ expect(subject[:status]).to eq(:success) end + it 'keeps epics timestamps in sync' do + expect(subject[:status]).to eq(:success) + + expect(old_parent.updated_at).to eq(old_parent.work_item.updated_at) + expect(new_parent.updated_at).to eq(new_parent.work_item.updated_at) + end + context 'when feature flag is turned off' do before do stub_feature_flags(sync_epic_work_item_order: false) diff --git a/ee/spec/support/shared_examples/services/epic/sync_work_item_shared_examples.rb b/ee/spec/support/shared_examples/services/epic/sync_work_item_shared_examples.rb index 2cf0d1a5d0892383ee497659024b347dff774a4a..109f2fdd702175b01cea6947046efd698b97d427 100644 --- a/ee/spec/support/shared_examples/services/epic/sync_work_item_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/epic/sync_work_item_shared_examples.rb @@ -26,6 +26,7 @@ expect(work_item.state).to eq(epic.state) expect(work_item.author).to eq(epic.author) expect(work_item.created_at).to eq(epic.created_at) + expect(work_item.updated_at).to eq(epic.updated_at) expect(work_item.state).to eq(epic.state) expect(work_item.external_key).to eq(epic.external_key) expect(work_item.lock_version).to eq(epic.lock_version) @@ -46,6 +47,7 @@ if epic.parent expect(work_item.work_item_parent).to eq(epic.parent.work_item) + expect(work_item.work_item_parent.updated_at).to eq(epic.parent.updated_at) expect(work_item.parent_link.relative_position).to eq(epic.relative_position) else expect(work_item.work_item_parent).to be_nil @@ -65,7 +67,11 @@ related_epic_issue_ids = epic.unauthorized_related_epics.map(&:issue_id) related_work_item_ids = work_item.related_issues(authorize: false).map(&:id) + related_epic_updated_at = epic.unauthorized_related_epics.map(&:updated_at) + related_work_item_updated_at = work_item.related_issues(authorize: false).map(&:updated_at) + expect(related_work_item_ids).to match(related_epic_issue_ids) + expect(related_epic_updated_at).to match(related_work_item_updated_at) end # Data we do not want to sync yet