From 0e35aed745501b6927737bf6f91813a972fb4011 Mon Sep 17 00:00:00 2001
From: charlie ablett <cablett@gitlab.com>
Date: Thu, 17 Jun 2021 22:16:04 +1200
Subject: [PATCH] Requirement migration: Sync title and description changes
Between Requirements and Requirement Issues
Wrap updates in pseudo-transaction
Add validation flags when mirrored object
not valid
Changelog: added
EE: true
---
.../issue_widgets/acts_like_requirement.rb | 18 +++
.../requirements_management/requirement.rb | 24 +++
ee/app/services/ee/issues/update_service.rb | 52 +++++++
.../update_requirement_service.rb | 34 ++++-
.../services/ee/issues/update_service_spec.rb | 144 ++++++++++++++++++
.../update_requirement_service_spec.rb | 128 +++++++++++++++-
6 files changed, 397 insertions(+), 3 deletions(-)
diff --git a/ee/app/models/concerns/issue_widgets/acts_like_requirement.rb b/ee/app/models/concerns/issue_widgets/acts_like_requirement.rb
index ef3adc2b2af9dd..78d1ed7567b82d 100644
--- a/ee/app/models/concerns/issue_widgets/acts_like_requirement.rb
+++ b/ee/app/models/concerns/issue_widgets/acts_like_requirement.rb
@@ -5,10 +5,28 @@ module ActsLikeRequirement
extend ActiveSupport::Concern
included do
+ attr_accessor :requirement_sync_error
+
+ after_validation :invalidate_if_sync_error, on: [:update]
+
# This will mean that non-Requirement issues essentially ignore this relationship and always return []
has_many :test_reports, -> { joins(:requirement_issue).where(issues: { issue_type: WorkItem::Type.base_types[:requirement] }) },
foreign_key: :issue_id, inverse_of: :requirement_issue, class_name: 'RequirementsManagement::TestReport'
has_one :requirement, class_name: 'RequirementsManagement::Requirement'
end
+
+ def requirement_sync_error!
+ self.requirement_sync_error = true
+ end
+
+ def invalidate_if_sync_error
+ return unless requirement_sync_error
+ return unless requirement
+
+ # Mirror errors from requirement so that users can adjust accordingly
+ errors = requirement.errors.full_messages.to_sentence
+ errors = errors.presence || "Associated requirement was invalid and changes could not be applied."
+ self.errors.add(:base, errors)
+ end
end
end
diff --git a/ee/app/models/requirements_management/requirement.rb b/ee/app/models/requirements_management/requirement.rb
index c15040091b851c..3fef609de323f0 100644
--- a/ee/app/models/requirements_management/requirement.rb
+++ b/ee/app/models/requirements_management/requirement.rb
@@ -40,6 +40,8 @@ class Requirement < ApplicationRecord
validate :only_requirement_type_issue
+ after_validation :invalidate_if_sync_error, on: [:update]
+
enum state: { opened: 1, archived: 2 }
scope :for_iid, -> (iid) { where(iid: iid) }
@@ -82,6 +84,10 @@ def search(query)
def simple_sorts
super.except('name_asc', 'name_desc')
end
+
+ def sync_params
+ [:title, :description]
+ end
end
# In the next iteration we will support also group-level requirements
@@ -105,5 +111,23 @@ def last_test_report_manually_created?
def only_requirement_type_issue
errors.add(:requirement_issue, "must be a `requirement`. You cannot associate a Requirement with an issue of type #{requirement_issue.issue_type}.") if requirement_issue && !requirement_issue.requirement? && will_save_change_to_issue_id?
end
+
+ def requirement_issue_sync_error!
+ self.requirement_issue_sync_error = true
+ end
+
+ private
+
+ attr_accessor :requirement_issue_sync_error
+
+ def invalidate_if_sync_error
+ return unless requirement_issue_sync_error
+ return unless requirement_issue
+
+ # Mirror errors from requirement issue so that users can adjust accordingly
+ errors = requirement_issue.errors.full_messages.to_sentence
+ errors = errors.presence || "Associated issue was invalid and changes could not be applied."
+ self.errors.add(:base, errors)
+ end
end
end
diff --git a/ee/app/services/ee/issues/update_service.rb b/ee/app/services/ee/issues/update_service.rb
index 586f6e3667b8e3..6350a0e0c3b7f6 100644
--- a/ee/app/services/ee/issues/update_service.rb
+++ b/ee/app/services/ee/issues/update_service.rb
@@ -8,6 +8,7 @@ module UpdateService
override :filter_params
def filter_params(issue)
+ params.delete(:skip_auth)
params.delete(:sprint_id) unless can_admin_issuable?(issue)
filter_epic(issue)
@@ -31,6 +32,11 @@ def execute(issue)
result
end
+ override :can_admin_issuable?
+ def can_admin_issuable?(issuable)
+ skip_auth || super
+ end
+
override :handle_changes
def handle_changes(issue, _options)
super
@@ -38,8 +44,43 @@ def handle_changes(issue, _options)
handle_iteration_change(issue)
end
+ override :before_update
+ def before_update(issue, **args)
+ super
+
+ # This is part of the migration from Requirement (the first class object)
+ # to Issue/Work Item (of type Requirement).
+ # We can't wrap the change in a Transaction (see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64929#note_647123684)
+ # so we'll check if both are valid before saving
+
+ # Don't bother syncing if it's possibly spam
+ return if issue.spam? || !issue.requirement
+
+ # Keep requirement objects in sync: gitlab-org/gitlab#323779
+ self.requirement_to_sync = prepare_requirement_for_sync(issue)
+ return unless requirement_to_sync
+
+ # This prevents the issue from being saveable
+ issue.requirement_sync_error! unless requirement_to_sync.valid?
+ end
+
+ override :after_update
+ def after_update(_issue)
+ super
+
+ return unless requirement_to_sync
+
+ unless requirement_to_sync.save
+ # We checked that it was valid earlier but it still did not save. Uh oh.
+ # This requires a manual re-sync and an investigation as to why this happened.
+ ::Gitlab::AppLogger.info(message: "Requirement-Issue Sync: Associated requirement could not be saved", project_id: project.id, user_id: current_user.id, params: params)
+ end
+ end
+
private
+ attr_accessor :skip_auth, :requirement_to_sync
+
def handle_iteration_change(issue)
return unless issue.previous_changes.include?('sprint_id')
@@ -66,6 +107,17 @@ def handle_promotion(issue)
Epics::IssuePromoteService.new(project: issue.project, current_user: current_user).execute(issue)
end
+
+ def prepare_requirement_for_sync(issue)
+ sync_params = RequirementsManagement::Requirement.sync_params
+ sync_attrs = issue.attributes.with_indifferent_access.slice(*sync_params)
+
+ # Update the requirement manually rather than through RequirementsManagement::Requirement::UpdateService,
+ # so the sync happens even if the Requirements feature is no longer available via the license.
+ requirement = issue.requirement
+ requirement.assign_attributes(sync_attrs)
+ requirement if requirement.changed?
+ end
end
end
end
diff --git a/ee/app/services/requirements_management/update_requirement_service.rb b/ee/app/services/requirements_management/update_requirement_service.rb
index 48d340927bacc5..6667e9b77e0c8d 100644
--- a/ee/app/services/requirements_management/update_requirement_service.rb
+++ b/ee/app/services/requirements_management/update_requirement_service.rb
@@ -6,8 +6,23 @@ def execute(requirement)
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_requirement, project)
attrs = whitelisted_requirement_params
- requirement.update(attrs)
+ requirement.assign_attributes(attrs)
+
+ # This is part of the migration from Requirement (the first class object)
+ # to Issue/Work Item (of type Requirement).
+ # We can't wrap the change in a Transaction (see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64929#note_647123684)
+ # so we'll check if both are valid before saving
+ if requirement.valid? && requirement.requirement_issue
+ updated_issue = sync_with_requirement_issue(requirement)
+
+ if updated_issue&.invalid?
+ requirement.requirement_issue_sync_error!
+ ::Gitlab::AppLogger.info(message: "Requirement-Issue Sync: Associated issue could not be saved", project_id: project.id, user_id: current_user.id, params: params)
+ end
+ end
+
+ requirement.save
create_test_report_for(requirement) if manually_create_test_report?
requirement
@@ -28,5 +43,22 @@ def create_test_report_for(requirement)
def whitelisted_requirement_params
params.slice(:title, :description, :state)
end
+
+ def sync_with_requirement_issue(requirement)
+ sync_params = RequirementsManagement::Requirement.sync_params
+ changed_attrs = requirement.changed.map(&:to_sym) & sync_params
+
+ return unless changed_attrs.any?
+
+ sync_attrs = requirement.attributes.with_indifferent_access.slice(*changed_attrs)
+
+ requirement_issue = requirement.requirement_issue
+
+ # Skip authorisation so we don't risk a permissions mismatch while still getting the advantages
+ # of the service, such as system notes.
+ params = sync_attrs.merge(skip_auth: true)
+ ::Issues::UpdateService.new(project: project, current_user: current_user, params: params)
+ .execute(requirement_issue)
+ end
end
end
diff --git a/ee/spec/services/ee/issues/update_service_spec.rb b/ee/spec/services/ee/issues/update_service_spec.rb
index 72f90a835d68c7..852841437e06e1 100644
--- a/ee/spec/services/ee/issues/update_service_spec.rb
+++ b/ee/spec/services/ee/issues/update_service_spec.rb
@@ -523,5 +523,149 @@ def update_issue(opts)
include_examples 'no trigger status page publish'
end
end
+
+ describe 'sync Requirement work item with Requirement object' do
+ let_it_be(:title) { 'title' }
+ let_it_be(:description) { 'description' }
+ let_it_be(:new_title) { 'new title' }
+ let_it_be(:new_description) { 'new description' }
+
+ let_it_be_with_reload(:issue) { create(:issue, issue_type: :requirement, project: project, title: title, description: description) }
+
+ let(:params) do
+ { state: :closed, title: new_title, description: new_description }
+ end
+
+ subject { update_issue(params) }
+
+ shared_examples 'keeps issue and its requirement in sync' do
+ it 'keeps title and description in sync' do
+ subject
+
+ requirement.reload
+ issue.requirement.reload
+
+ expect(requirement).to have_attributes(
+ title: issue.requirement.title,
+ description: issue.requirement.description)
+ end
+ end
+
+ shared_examples 'does not persist any changes' do
+ it 'does not update the issue' do
+ expect { subject }.not_to change { issue.reload.attributes }
+ end
+
+ it 'does not update the requirement' do
+ expect { subject }.not_to change { requirement.reload.attributes }
+ end
+ end
+
+ before do
+ issue.requirement_sync_error = false
+ issue.clear_spam_flags!
+ end
+
+ context 'if there is an associated requirement' do
+ let_it_be_with_reload(:requirement) { create(:requirement, title: title, description: description, requirement_issue: issue, project: project) }
+
+ it 'does not update the unrelated field' do
+ expect { subject }.not_to change { requirement.reload.state }
+ end
+
+ it 'updates the synced requirement with title and/or description' do
+ subject
+
+ expect(requirement.reload).to have_attributes(
+ title: new_title,
+ description: new_description)
+ end
+
+ context 'when the issue title is very long' do
+ # The Requirement title limit is 255 in the application as well as the DB.
+ # The Issue limit is 255 in the application not the DB, so we should be OK.
+ # See https://gitlab.com/gitlab-org/gitlab/blob/7a42426/app/models/concerns/issuable.rb#L30
+ let_it_be(:new_title) { 'a' * 300 }
+
+ it_behaves_like 'does not persist any changes'
+ end
+
+ it_behaves_like 'keeps issue and its requirement in sync'
+
+ context 'if update of issue fails' do
+ let(:params) do
+ { title: nil }
+ end
+
+ it_behaves_like 'keeps issue and its requirement in sync'
+ it_behaves_like 'does not persist any changes'
+ end
+
+ context 'if update of issue succeeds but update of requirement fails' do
+ let(:params) do
+ { title: 'some magically valid title for issue but not requirement' }
+ end
+
+ before do
+ allow(issue).to receive(:requirement).and_return(requirement)
+ end
+
+ context 'when requirement is not valid' do
+ before do
+ expect(requirement).to receive(:valid?).and_return(false).at_least(:once)
+ end
+
+ it_behaves_like 'keeps issue and its requirement in sync'
+ it_behaves_like 'does not persist any changes'
+
+ it 'adds an informative sync error to issue' do
+ subject
+
+ expect(issue.errors[:base]).to include(/Associated requirement/)
+ end
+ end
+
+ context 'if requirement is valid but still does not save' do
+ before do
+ allow(issue.requirement).to receive(:valid?).and_return(true).at_least(:once)
+ allow(requirement).to receive(:save).and_return(false)
+ end
+
+ it 'adds a helpful log' do
+ expect(::Gitlab::AppLogger).to receive(:info).with(a_hash_including(message: /Associated requirement/))
+
+ subject
+ end
+ end
+ end
+
+ # spam checking also uses a flag on Issue so we want to ensure they play nicely together
+ # If an issue is marked spam, we do not want to try syncing
+ # since the issue will not be saved this time (maybe next time if a successful recaptcha has been included)
+ context 'if the issue is also marked as spam' do
+ before do
+ issue.spam!
+ end
+
+ it_behaves_like 'keeps issue and its requirement in sync'
+ it_behaves_like 'does not persist any changes'
+
+ it 'only shows the spam error' do
+ subject
+
+ expect(issue.errors[:base]).to include(/spam/)
+ expect(issue.errors[:base]).not_to include(/sync/)
+ end
+ end
+ end
+
+ context 'if there is no associated requirement' do
+ it 'does not call the RequirementsManagement::UpdateRequirementService' do
+ expect(RequirementsManagement::UpdateRequirementService).not_to receive(:new)
+
+ update_issue(params)
+ end
+ end
+ end
end
end
diff --git a/ee/spec/services/requirements_management/update_requirement_service_spec.rb b/ee/spec/services/requirements_management/update_requirement_service_spec.rb
index d7bd439a21d23d..10ff487c4f312d 100644
--- a/ee/spec/services/requirements_management/update_requirement_service_spec.rb
+++ b/ee/spec/services/requirements_management/update_requirement_service_spec.rb
@@ -3,13 +3,20 @@
require 'spec_helper'
RSpec.describe RequirementsManagement::UpdateRequirementService do
+ let_it_be(:title) { 'title' }
+ let_it_be(:description) { 'description' }
+
+ let(:new_title) { 'new title' }
+ let(:new_description) { 'new description' }
+
let_it_be(:project) { create(:project)}
let_it_be(:user) { create(:user) }
- let_it_be(:requirement) { create(:requirement, project: project) }
+ let_it_be_with_reload(:requirement) { create(:requirement, project: project, title: title, description: description) }
let(:params) do
{
- title: 'foo',
+ title: new_title,
+ description: new_description,
state: 'archived',
created_at: 2.days.ago,
author_id: create(:user).id
@@ -40,6 +47,123 @@
)
end
+ context 'when updating title or description' do
+ shared_examples 'keeps requirement and its requirement_issue in sync' do
+ it 'keeps title and description in sync' do
+ subject
+
+ requirement.reload
+ requirement.requirement_issue.reload
+
+ expect(requirement).to have_attributes(
+ title: requirement.requirement_issue.title,
+ description: requirement.requirement_issue.description)
+ end
+ end
+
+ shared_examples 'does not persist any changes' do
+ it 'does not update the requirement' do
+ expect { subject }.not_to change { requirement.reload.attributes }
+ end
+
+ it 'does not update the requirement issue' do
+ expect { subject }.not_to change { requirement_issue.reload.attributes }
+ end
+ end
+
+ context 'if there is an associated requirement_issue' do
+ let_it_be_with_reload(:requirement_issue) { create(:requirement_issue, requirement: requirement, title: title, description: description) }
+
+ let(:params) do
+ { title: new_title, description: new_description }
+ end
+
+ it 'updates the synced requirement_issue with title and description' do
+ expect { subject }
+ .to change { requirement.requirement_issue.description }.from(description).to(new_description)
+ .and change { requirement.requirement_issue.title }.from(title).to(new_title)
+ end
+
+ context 'when updating only title' do
+ let(:params) do
+ { title: new_title }
+ end
+
+ it "updates requirement's title" do
+ expect { subject }.to change { requirement.requirement_issue.reload.title }.from(title).to(new_title)
+ end
+
+ it_behaves_like 'keeps requirement and its requirement_issue in sync'
+ end
+
+ context "updates requirement's description" do
+ let(:params) do
+ { description: new_description }
+ end
+
+ it 'updates description' do
+ expect { subject }.to change { requirement.requirement_issue.reload.description }.from(description).to(new_description)
+ end
+
+ it_behaves_like 'keeps requirement and its requirement_issue in sync'
+ end
+
+ context 'if update fails' do
+ let(:params) do
+ { title: nil }
+ end
+
+ it_behaves_like 'does not persist any changes'
+ it_behaves_like 'keeps requirement and its requirement_issue in sync'
+
+ context 'if update of requirement succeeds but update of issue fails' do
+ let(:params) do
+ { title: 'some magically valid title for requirement but not issue' }
+ end
+
+ before do
+ allow_next_instance_of(::Issues::UpdateService) do |service|
+ allow(service).to receive(:execute).and_return(requirement_issue)
+ end
+
+ allow(requirement_issue).to receive(:invalid?).and_return(true).at_least(:once)
+ end
+
+ it_behaves_like 'keeps requirement and its requirement_issue in sync'
+ it_behaves_like 'does not persist any changes'
+
+ it 'adds an informative sync error to issue' do
+ expect(::Gitlab::AppLogger).to receive(:info).with(a_hash_including(message: /Associated issue/))
+
+ subject
+
+ expect(requirement.errors[:base]).to include(/Associated issue/)
+ end
+ end
+ end
+
+ context 'when updating some unrelated field' do
+ let(:params) do
+ { state: :archived }
+ end
+
+ it 'does not update the associated requirement issue' do
+ expect { subject }.not_to change { requirement.requirement_issue.state }
+ end
+
+ it_behaves_like 'keeps requirement and its requirement_issue in sync'
+ end
+ end
+
+ context 'if there is no requirement_issue' do
+ it 'does not call the Issues::UpdateService' do
+ expect(Issues::UpdateService).not_to receive(:new)
+
+ subject
+ end
+ end
+ end
+
context 'when updating last test report state' do
context 'as passing' do
it 'creates passing test report with null build_id' do
--
GitLab