Merge branch '323781-delegate-attributes-in-requirement-model' into 'master'

Delegate Requirement model attributes to associated issue

See merge request gitlab-org/gitlab!74296

ee/app/graphql/mutations/requirements_management/create_requirement.rb

@@ -19,10 +19,13 @@ module Mutations
           params: args
         ).execute
 
-        {
-          requirement: requirement.valid? ? requirement : nil,
-          errors: errors_on_object(requirement)
-        }
+        if requirement.errors.empty?
+          { requirement: requirement, errors: [] }
+        else
+          requirement.errors.delete(:requirement_issue)
+
+          { requirement: nil, errors: errors_on_object(requirement) }
+        end
       end
     end
   end

ee/app/models/requirements_management/requirement.rb

@@ -12,6 +12,7 @@ module RequirementsManagement
     # but to avoid downtime and deployment issues `requirements` is still used
     # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/30052#note_329556542
     self.table_name = 'requirements'
+    STATE_MAP = { opened: 'opened', closed: 'archived' }.with_indifferent_access.freeze
 
     cache_markdown_field :title, pipeline: :single_line
     cache_markdown_field :description, issuable_reference_expansion_enabled: true
@@ -26,22 +27,30 @@ module RequirementsManagement
     # This will be removed in https://gitlab.com/gitlab-org/gitlab/-/issues/329432
     belongs_to :requirement_issue, class_name: 'Issue', foreign_key: :issue_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
-    validates :issue_id, uniqueness: true, allow_nil: true
+    validates :project, presence: true
+    validates :requirement_issue, presence: true, on: [:create, :update]
+
+    validates :issue_id, uniqueness: true
 
     has_many :test_reports, inverse_of: :requirement
     has_many :recent_test_reports, -> { order(created_at: :desc) }, class_name: 'TestReport', inverse_of: :requirement
 
     has_internal_id :iid, scope: :project
 
-    validates :author, :project, :title, presence: true
-
-    validates :title, length: { maximum: Issuable::TITLE_LENGTH_MAX }
-    validates :title_html, length: { maximum: Issuable::TITLE_HTML_LENGTH_MAX }, allow_blank: true
-
     validate :only_requirement_type_issue
 
     after_validation :invalidate_if_sync_error, on: [:update, :create]
 
+    delegate :title,
+             :author,
+             :author_id,
+             :description,
+             :description_html,
+             :title_html,
+             :cached_markdown_version,
+             to: :requirement_issue,
+             allow_nil: true
+
     enum state: { opened: 1, archived: 2 }
 
     scope :for_iid, -> (iid) { where(iid: iid) }
@@ -112,19 +121,25 @@ module RequirementsManagement
       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
+    def requirement_issue_sync_error!(invalid_issue:)
+      self.invalid_requirement_issue = invalid_issue
+    end
+
+    def state
+      return unless requirement_issue&.requirement?
+
+      STATE_MAP[requirement_issue.state]
     end
 
     private
 
-    attr_accessor :requirement_issue_sync_error
+    attr_accessor :invalid_requirement_issue # Used to retrieve error messages
 
     def invalidate_if_sync_error
-      return unless requirement_issue_sync_error
+      return unless invalid_requirement_issue
 
       # Mirror errors from requirement issue so that users can adjust accordingly
-      errors = requirement_issue.errors.full_messages.to_sentence if requirement_issue
+      errors = invalid_requirement_issue.errors.full_messages.to_sentence if invalid_requirement_issue
 
       errors = errors.presence || "Associated issue was invalid and changes could not be applied."
       self.errors.add(:base, errors)

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

@@ -9,29 +9,32 @@ 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)
+      # so we'll check if both are valid before saving, we also pass special context
+      # to avoid validating requirement's issue presence which is not created yet
+      return unless requirement.valid?(:before_requirement_issue)
 
-        return synced_issue if synced_issue.valid?
+      attributes = attrs_to_sync(requirement)
+      return if !requirement.new_record? && attributes.empty?
 
-        requirement.requirement_issue_sync_error!
+      synced_issue = perform_sync(requirement, attributes)
+      return synced_issue if synced_issue.valid?
 
-        ::Gitlab::AppLogger.info(message: "Requirement-Issue Sync: Associated issue could not be saved", project_id: project.id, user_id: current_user.id, params: params)
+      requirement.requirement_issue_sync_error!(invalid_issue: synced_issue)
 
-        nil
-      end
+      ::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
 
-    def save_requirement_issue(requirement)
+    def attrs_to_sync(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)
+      requirement.attributes.with_indifferent_access.slice(*changed_attrs)
     end
 
     # Overriden on subclasses

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

@@ -233,7 +233,7 @@ module EE
         # 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.assign_attributes(sync_attrs.merge(requirement_issue: issue))
 
         requirement if requirement.changed?
       end

ee/app/services/requirements_management/create_requirement_service.rb

@@ -19,7 +19,13 @@ module RequirementsManagement
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, :create_requirement, project)
 
       attrs = whitelisted_requirement_params.merge(author: current_user)
-      requirement = project.requirements.new(attrs)
+      create_requirement(project, attrs)
+    end
+
+    private
+
+    def create_requirement(project, attributes)
+      requirement = project.requirements.new(attributes)
 
       requirement.requirement_issue ||= sync_issue_for(requirement)
       requirement.save
@@ -27,8 +33,6 @@ module RequirementsManagement
       requirement
     end
 
-    private
-
     def perform_sync(requirement, attributes)
       attributes[:issue_type] = 'requirement'
       attributes[:author] = requirement.author

ee/spec/factories/requirements_management/requirements.rb

@@ -6,5 +6,9 @@ FactoryBot.define do
     author
     title { generate(:title) }
     title_html { "<h2>#{title}</h2>" }
+    requirement_issue do
+      issue_state = state.to_s == 'archived' ? 'closed' : 'opened'
+      association(:issue, issue_type: :requirement, project: project, author: author, title: title, description: description, state: issue_state)
+    end
   end
 end

ee/spec/models/requirements_management/requirement_spec.rb

@@ -17,23 +17,38 @@ RSpec.describe RequirementsManagement::Requirement do
     it_behaves_like 'a model with a requirement issue association'
   end
 
-  describe 'validations' do
+  describe 'delegate' do
     subject { build(:requirement) }
 
-    it { is_expected.to validate_presence_of(:project) }
-    it { is_expected.to validate_presence_of(:author) }
-    it { is_expected.to validate_presence_of(:title) }
+    delegated_attributes = %i[
+      author author_id title title_html description
+      description_html cached_markdown_version
+    ]
 
-    it { is_expected.to validate_length_of(:title).is_at_most(::Issuable::TITLE_LENGTH_MAX) }
-    it { is_expected.to validate_length_of(:title_html).is_at_most(::Issuable::TITLE_HTML_LENGTH_MAX) }
+    delegated_attributes.each do |attr_name|
+      it { is_expected.to delegate_method(attr_name).to(:requirement_issue).allow_nil }
+    end
 
-    context 'with requirement issue' do
-      let(:ri) { create(:requirement_issue) }
+    context 'with nil attributes' do
+      let_it_be(:requirement) { create(:requirement, project: project, author: user, description: 'Test', state: 'archived') }
 
-      subject { build(:requirement, requirement_issue: ri) }
+      (delegated_attributes - [:title]).each do |attr_name|
+        it "returns delegated #{attr_name} value" do
+          requirement.update_attribute(attr_name, nil)
 
-      it { is_expected.to validate_uniqueness_of(:issue_id).allow_nil }
+          expect(requirement.send(attr_name)).not_to be_nil
+          expect(requirement.send(attr_name)).to eq(requirement.requirement_issue.send(attr_name))
+        end
+      end
     end
+  end
+
+  describe 'validations' do
+    subject { build(:requirement) }
+
+    it { is_expected.to validate_uniqueness_of(:issue_id) }
+    it { is_expected.to validate_presence_of(:project) }
+    it { is_expected.to validate_presence_of(:requirement_issue) }
 
     it 'is limited to a unique requirement_issue' do
       requirement_issue = create(:requirement_issue)
@@ -203,4 +218,32 @@ RSpec.describe RequirementsManagement::Requirement do
       end
     end
   end
+
+  describe '#state' do
+    let_it_be_with_reload(:requirement) { create(:requirement, state: :archived) }
+
+    context 'when linked requirement issue is not present' do
+      before do
+        requirement.requirement_issue = nil
+      end
+
+      it 'returns nil' do
+        expect(requirement.state).to be_nil
+      end
+    end
+
+    context 'when linked requirement issue is present' do
+      it 'returns requirement issue stored state' do
+        requirement.requirement_issue.state = 'opened'
+
+        expect(requirement.state).to eq('opened')
+      end
+
+      it 'returns mapped value for state' do
+        requirement.requirement_issue.state = 'closed'
+
+        expect(requirement.state).to eq('archived')
+      end
+    end
+  end
 end

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

@@ -573,8 +573,8 @@ RSpec.describe Issues::UpdateService do
       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 }
+        it 'updates the mapped field' do
+          expect { subject }.to change { requirement.reload.state }.from("opened").to("archived")
         end
 
         it 'updates the synced requirement with title and/or description' do

ee/spec/services/requirements_management/create_requirement_service_spec.rb

@@ -54,9 +54,12 @@ RSpec.describe RequirementsManagement::CreateRequirementService do
         end
 
         context 'when creation of requirement fails' do
+          let_it_be(:requirement2) { create(:requirement, project: project, state: :opened, author: user) }
+
           it 'does not create issue' do
             allow_next_instance_of(RequirementsManagement::Requirement) do |instance|
               allow(instance).to receive(:valid?).and_return(false)
+              allow(instance).to receive(:valid?).with(:before_requirement_issue).and_return(false)
             end
 
             expect { subject }. to change { Issue.count }.by(0)
@@ -65,14 +68,22 @@ RSpec.describe RequirementsManagement::CreateRequirementService do
         end
 
         context 'when creation of issue fails' do
-          it 'does not create requirement' do
+          before do
             allow_next_instance_of(Issue) do |instance|
               allow(instance).to receive(:valid?).and_return(false)
             end
+          end
 
+          it 'does not create requirement' do
             expect { subject }. to change { Issue.count }.by(0)
               .and change { RequirementsManagement::Requirement.count }.by(0)
           end
+
+          it 'logs error' do
+            expect(::Gitlab::AppLogger).to receive(:info).with(a_hash_including(message: /Associated issue/))
+
+            subject
+          end
         end
       end
     end

ee/spec/services/requirements_management/update_requirement_service_spec.rb

@@ -190,12 +190,11 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
           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)
+        it 'does not call the Issues::UpdateService when requirement is invalid' do
+          requirement.project = nil
+          expect(Issues::UpdateService).not_to receive(:new)
 
-            subject
-          end
+          subject
         end
       end
 

ee/spec/support/shared_examples/services/sync_issue_and_requirement_state_shared_examples.rb

@@ -32,7 +32,7 @@ shared_examples 'sync requirement with issue state' do
         subject
 
         expect(issue.reload.state).to eq(issue_expected_state)
-        expect(requirement.reload.state).to eq(requirement_initial_state)
+        expect(requirement.reload.state).to be_nil
       end
     end