Merge branch 'issue_337190-sync_creation_of_requirements' into 'master'

Sync requirements creation with work items

See merge request gitlab-org/gitlab!71104

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

@@ -7,7 +7,7 @@ module IssueWidgets
     included do
       attr_accessor :requirement_sync_error
 
-      after_validation :invalidate_if_sync_error, on: [:update]
+      after_validation :invalidate_if_sync_error, on: [:update, :create]
 
       # 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] }) },
@@ -20,6 +20,7 @@ module IssueWidgets
     end
 
     def invalidate_if_sync_error
+      return unless requirement? # No need to invalidate if issue_type != requirement
       return unless requirement_sync_error
       return unless requirement
 

ee/app/models/requirements_management/requirement.rb

@@ -40,7 +40,7 @@ module RequirementsManagement
 
     validate :only_requirement_type_issue
 
-    after_validation :invalidate_if_sync_error, on: [:update]
+    after_validation :invalidate_if_sync_error, on: [:update, :create]
 
     enum state: { opened: 1, archived: 2 }
 
@@ -86,7 +86,7 @@ module RequirementsManagement
       end
 
       def sync_params
-        [:title, :description, :state]
+        [:title, :description, :state, :project_id, :author_id]
       end
     end
 
@@ -122,10 +122,10 @@ module RequirementsManagement
 
     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 = requirement_issue.errors.full_messages.to_sentence if requirement_issue
+
       errors = errors.presence || "Associated issue was invalid and changes could not be applied."
       self.errors.add(:base, errors)
     end

ee/app/services/concerns/requirements_management/sync_with_requirement_issue.rb

+# frozen_string_literal: true
+
+# This is part of the migration from Requirement (the first class object)
+# to Issue/Work Item (of type Requirement).
+# This file can be removed on cleanup phase, for more information check:
+# https://gitlab.com/gitlab-org/gitlab/-/issues/323779#hourglass-stage-v-dropping-of-requirements-table-cleanup-329432
+
+module RequirementsManagement
+  module SyncWithRequirementIssue
+    def sync_issue_for(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 || requirement.new_record?)
+        synced_issue = save_requirement_issue(requirement)
+
+        return synced_issue if synced_issue.valid?
+
+        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)
+
+        nil
+      end
+    end
+
+    def save_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)
+
+      perform_sync(requirement, sync_attrs)
+    end
+
+    # Overriden on subclasses
+    # To sync on create or on update
+    def perform_sync(requirement, attributes)
+      raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+    end
+  end
+end

ee/app/services/ee/issues/base_service.rb

@@ -185,6 +185,58 @@ module EE
 
         params[:iteration] = iteration if iteration
       end
+
+      attr_accessor :requirement_to_sync
+
+      def assign_requirement_to_be_synced_for(issue)
+        # 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?
+        return unless issue.requirement?
+
+        # Keep requirement objects in sync: gitlab-org/gitlab#323779
+        self.requirement_to_sync = assign_requirement_attributes(issue)
+
+        return unless requirement_to_sync
+
+        # This prevents the issue from being saveable
+        issue.requirement ||= requirement_to_sync
+        issue.requirement_sync_error! unless requirement_to_sync.valid?
+      end
+
+      def save_requirement
+        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.
+          log_params = params.slice(*RequirementsManagement::Requirement.sync_params)
+
+          ::Gitlab::AppLogger.info(
+            message: "Requirement-Issue Sync: Associated requirement could not be saved",
+            project_id: project.id,
+            user_id: current_user.id,
+            params: log_params
+          )
+        end
+      end
+
+      def assign_requirement_attributes(issue)
+        sync_params = RequirementsManagement::Requirement.sync_params
+        sync_attrs = issue.attributes.with_indifferent_access.slice(*sync_params)
+
+        # Update or create 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 || RequirementsManagement::Requirement.new
+
+        requirement.assign_attributes(sync_attrs)
+
+        requirement if requirement.changed?
+      end
     end
   end
 end

ee/app/services/ee/issues/create_service.rb

@@ -27,6 +27,22 @@ module EE
           end
         end
       end
+
+      override :before_create
+      def before_create(issue)
+        super
+
+        assign_requirement_to_be_synced_for(issue)
+      end
+
+      override :after_create
+      def after_create(issue)
+        super
+
+        requirement_to_sync.issue_id = issue.id if requirement_to_sync
+
+        save_requirement
+      end
     end
   end
 end

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

@@ -42,39 +42,18 @@ module EE
       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?
+        assign_requirement_to_be_synced_for(issue)
       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
+        save_requirement
       end
 
       private
 
-      attr_accessor :requirement_to_sync
-
       def handle_iteration_change(issue)
         return unless issue.previous_changes.include?('sprint_id')
 
@@ -101,17 +80,6 @@ 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/create_requirement_service.rb

@@ -3,6 +3,7 @@
 module RequirementsManagement
   class CreateRequirementService < ::BaseProjectService
     include Gitlab::Allowable
+    include SyncWithRequirementIssue
 
     # NOTE: Even though this class does not (yet) do spam checking, this constructor takes a
     # spam_params named argument in order to be consistent with the other issuable service
@@ -18,11 +19,29 @@ module RequirementsManagement
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :create_requirement, project)
 
       attrs = whitelisted_requirement_params.merge(author: current_user)
-      project.requirements.create(attrs)
+      requirement = project.requirements.new(attrs)
+
+      requirement.requirement_issue ||= sync_issue_for(requirement)
+      requirement.save
+
+      requirement
     end
 
     private
 
+    def perform_sync(requirement, attributes)
+      attributes[:issue_type] = 'requirement'
+      attributes[:author] = requirement.author
+      attributes[:project] = requirement.project
+
+      issue =
+        ::Issues::BuildService.new(project: project, current_user: current_user, params: attributes).execute
+
+      issue.save
+
+      issue
+    end
+
     def whitelisted_requirement_params
       params.slice(:title, :description)
     end

ee/app/services/requirements_management/update_requirement_service.rb

@@ -2,6 +2,8 @@
 
 module RequirementsManagement
   class UpdateRequirementService < BaseService
+    include SyncWithRequirementIssue
+
     def execute(requirement)
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_requirement, project)
 
@@ -9,18 +11,7 @@ module RequirementsManagement
 
       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
+      sync_issue_for(requirement)
 
       requirement.save
       create_test_report_for(requirement) if manually_create_test_report?
@@ -44,27 +35,10 @@ module RequirementsManagement
       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)
-
+    def perform_sync(requirement, attributes)
       requirement_issue = requirement.requirement_issue
 
-      state_change = sync_attrs.delete(:state)
-      update_requirement_issue_title_and_description(requirement_issue, sync_attrs)
-      update_requirement_issue_state(requirement_issue, state_change)
-    end
-
-    def update_requirement_issue_title_and_description(requirement_issue, params)
-      return requirement_issue unless params.any?
-
-      title_and_description = params.with_indifferent_access.slice(:title, :description)
-
-      ::Issues::UpdateService.new(project: project, current_user: current_user, params: title_and_description)
+      ::Issues::UpdateService.new(project: project, current_user: current_user, params: attributes)
         .execute(requirement_issue)
     end
 

ee/spec/services/ee/issues/create_service_spec.rb

@@ -214,6 +214,57 @@ RSpec.describe Issues::CreateService do
           end
         end
       end
+
+      context 'when issue is of requirement_type' do
+        let(:params) { { title: 'Requirement Issue', description: 'Should sync', issue_type: 'requirement' } }
+
+        before_all do
+          project.add_reporter(user)
+        end
+
+        before do
+          stub_licensed_features(requirements: true)
+        end
+
+        it 'creates one requirement and one requirement issue' do
+          expect { service.execute }. to change { Issue.count }.by(1)
+            .and change { RequirementsManagement::Requirement.count }.by(1)
+        end
+
+        it 'creates a requirement object with same parameters' do
+          issue = service.execute
+
+          requirement = issue.reload.requirement
+          expect(requirement.title).to eq(issue.title)
+          expect(requirement.description).to eq(issue.description)
+          expect(requirement.state).to eq(issue.state)
+          expect(requirement.project).to eq(issue.project)
+          expect(requirement.author).to eq(issue.author)
+          expect(issue.requirement?).to eq(true)
+        end
+
+        context 'when creation of requirement fails' do
+          it 'does not create issue' do
+            allow_next_instance_of(RequirementsManagement::Requirement) do |instance|
+              allow(instance).to receive(:valid?).and_return(false)
+            end
+
+            expect { service.execute }. to change { Issue.count }.by(0)
+              .and change { RequirementsManagement::Requirement.count }.by(0)
+          end
+        end
+
+        context 'when creation of issue fails' do
+          it 'does not create requirement' do
+            allow_next_instance_of(Issue) do |instance|
+              allow(instance).to receive(:valid?).and_return(false)
+            end
+
+            expect { service.execute }. to change { Issue.count }.by(0)
+              .and change { RequirementsManagement::Requirement.count }.by(0)
+          end
+        end
+      end
     end
 
     it_behaves_like 'new issuable with scoped labels' do

ee/spec/services/requirements_management/create_requirement_service_spec.rb

@@ -34,6 +34,47 @@ RSpec.describe RequirementsManagement::CreateRequirementService do
         expect(requirement.created_at).not_to eq(params[:created_at])
         expect(requirement.author_id).not_to eq(params[:author_id])
       end
+
+      context 'when syncing with requirement issues' do
+        it 'creates an issue and a requirement' do
+          expect { subject }.to change { Issue.count }.by(1)
+            .and change { RequirementsManagement::Requirement.count }.by(1)
+        end
+
+        it 'creates an associated issue of type requirement with same attributes' do
+          requirement = subject
+          issue = requirement.reload.requirement_issue
+
+          expect(issue).to be_persisted
+          expect(issue.title).to eq(requirement.title)
+          expect(issue.description).to eq(requirement.description)
+          expect(issue.author).to eq(requirement.author)
+          expect(issue.project).to eq(requirement.project)
+          expect(issue.requirement?).to eq(true)
+        end
+
+        context 'when creation of requirement fails' do
+          it 'does not create issue' do
+            allow_next_instance_of(RequirementsManagement::Requirement) do |instance|
+              allow(instance).to receive(:valid?).and_return(false)
+            end
+
+            expect { subject }. to change { Issue.count }.by(0)
+              .and change { RequirementsManagement::Requirement.count }.by(0)
+          end
+        end
+
+        context 'when creation of issue fails' do
+          it 'does not create requirement' do
+            allow_next_instance_of(Issue) do |instance|
+              allow(instance).to receive(:valid?).and_return(false)
+            end
+
+            expect { subject }. to change { Issue.count }.by(0)
+              .and change { RequirementsManagement::Requirement.count }.by(0)
+          end
+        end
+      end
     end
 
     context 'when user is not allowed to create requirements' do

ee/spec/services/requirements_management/update_requirement_service_spec.rb

@@ -164,7 +164,7 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
                 end
 
                 allow(requirement).to receive(:requirement_issue).and_return(requirement_issue)
-                allow(requirement_issue).to receive(:invalid?).and_return(true).at_least(:once)
+                allow(requirement_issue).to receive(:valid?).and_return(false).at_least(:once)
               end
 
               it_behaves_like 'keeps requirement and its requirement_issue in sync'