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

ee/app/models/concerns/issue_widgets/acts_like_requirement.rb

@@ -5,10 +5,28 @@ module IssueWidgets
     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

ee/app/models/requirements_management/requirement.rb

@@ -40,6 +40,8 @@ module RequirementsManagement
 
     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 @@ module RequirementsManagement
       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 @@ module RequirementsManagement
     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

ee/app/services/ee/issues/update_service.rb

@@ -8,6 +8,7 @@ module EE
 
       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 @@ module EE
         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 @@ module EE
         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 @@ module EE
 
         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

ee/app/services/requirements_management/update_requirement_service.rb

@@ -6,8 +6,23 @@ module RequirementsManagement
       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 @@ module RequirementsManagement
     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

ee/spec/services/ee/issues/update_service_spec.rb

@@ -523,5 +523,149 @@ RSpec.describe Issues::UpdateService do
         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

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 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
         )
       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