Commit f020e4c1 authored by Eugenia Grieff's avatar Eugenia Grieff

Validate presence of requirement_issue

- Check requirement validation with context before
creating the requirement issue
- Update specs
parent 8598100a
...@@ -19,10 +19,13 @@ module Mutations ...@@ -19,10 +19,13 @@ module Mutations
params: args params: args
).execute ).execute
{ if requirement.errors.empty?
requirement: requirement.valid? ? requirement : nil, { requirement: requirement, errors: [] }
errors: errors_on_object(requirement) else
} requirement.errors.delete(:requirement_issue)
{ requirement: nil, errors: errors_on_object(requirement) }
end
end end
end end
end end
......
...@@ -27,6 +27,9 @@ module RequirementsManagement ...@@ -27,6 +27,9 @@ module RequirementsManagement
# This will be removed in https://gitlab.com/gitlab-org/gitlab/-/issues/329432 # 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 belongs_to :requirement_issue, class_name: 'Issue', foreign_key: :issue_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
validates :project, presence: true
validates :requirement_issue, presence: true, on: [:create, :update]
validates :issue_id, uniqueness: true validates :issue_id, uniqueness: true
has_many :test_reports, inverse_of: :requirement has_many :test_reports, inverse_of: :requirement
...@@ -39,8 +42,6 @@ module RequirementsManagement ...@@ -39,8 +42,6 @@ module RequirementsManagement
after_validation :invalidate_if_sync_error, on: [:update, :create] after_validation :invalidate_if_sync_error, on: [:update, :create]
delegate :title, delegate :title,
:project,
:project_id,
:author, :author,
:author_id, :author_id,
:description, :description,
......
...@@ -9,25 +9,26 @@ module RequirementsManagement ...@@ -9,25 +9,26 @@ module RequirementsManagement
module SyncWithRequirementIssue module SyncWithRequirementIssue
def sync_issue_for(requirement) 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) # 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 # so we'll check if both are valid before saving, we also pass special context
if requirement.valid? && (requirement.requirement_issue || requirement.new_record?) # to avoid validating requirement's issue presence which is not created yet
attributes = attrs_to_sync(requirement) return unless requirement.valid?(:before_requirement_issue)
return unless attributes.any?
synced_issue = perform_sync(requirement, attributes) attributes = attrs_to_sync(requirement)
return synced_issue if synced_issue.valid? return if !requirement.new_record? && attributes.empty?
requirement.requirement_issue_sync_error!(invalid_issue: synced_issue) synced_issue = perform_sync(requirement, attributes)
return synced_issue if synced_issue.valid?
::Gitlab::AppLogger.info( requirement.requirement_issue_sync_error!(invalid_issue: synced_issue)
message: "Requirement-Issue Sync: Associated issue could not be saved",
project_id: project.id,
user_id: current_user.id,
params: params
)
nil ::Gitlab::AppLogger.info(
end 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 attrs_to_sync(requirement) def attrs_to_sync(requirement)
......
...@@ -233,7 +233,7 @@ module EE ...@@ -233,7 +233,7 @@ module EE
# so the sync happens even if the Requirements feature is no longer available via the license. # so the sync happens even if the Requirements feature is no longer available via the license.
requirement = issue.requirement || RequirementsManagement::Requirement.new requirement = issue.requirement || RequirementsManagement::Requirement.new
requirement.assign_attributes(sync_attrs) requirement.assign_attributes(sync_attrs.merge(requirement_issue: issue))
requirement if requirement.changed? requirement if requirement.changed?
end end
......
...@@ -21,8 +21,8 @@ RSpec.describe RequirementsManagement::Requirement do ...@@ -21,8 +21,8 @@ RSpec.describe RequirementsManagement::Requirement do
subject { build(:requirement) } subject { build(:requirement) }
delegated_attributes = %i[ delegated_attributes = %i[
project project_id author author_id title title_html author author_id title title_html description
description description_html cached_markdown_version description_html cached_markdown_version
] ]
delegated_attributes.each do |attr_name| delegated_attributes.each do |attr_name|
...@@ -32,7 +32,7 @@ RSpec.describe RequirementsManagement::Requirement do ...@@ -32,7 +32,7 @@ RSpec.describe RequirementsManagement::Requirement do
context 'with nil attributes' do context 'with nil attributes' do
let_it_be(:requirement) { create(:requirement, project: project, author: user, description: 'Test', state: 'archived') } let_it_be(:requirement) { create(:requirement, project: project, author: user, description: 'Test', state: 'archived') }
(delegated_attributes - %i[project project_id title]).each do |attr_name| (delegated_attributes - [:title]).each do |attr_name|
it "returns delegated #{attr_name} value" do it "returns delegated #{attr_name} value" do
requirement.update_attribute(attr_name, nil) requirement.update_attribute(attr_name, nil)
...@@ -47,6 +47,8 @@ RSpec.describe RequirementsManagement::Requirement do ...@@ -47,6 +47,8 @@ RSpec.describe RequirementsManagement::Requirement do
subject { build(:requirement) } subject { build(:requirement) }
it { is_expected.to validate_uniqueness_of(:issue_id) } 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 it 'is limited to a unique requirement_issue' do
requirement_issue = create(:requirement_issue) requirement_issue = create(:requirement_issue)
......
...@@ -54,9 +54,12 @@ RSpec.describe RequirementsManagement::CreateRequirementService do ...@@ -54,9 +54,12 @@ RSpec.describe RequirementsManagement::CreateRequirementService do
end end
context 'when creation of requirement fails' do 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 it 'does not create issue' do
allow_next_instance_of(RequirementsManagement::Requirement) do |instance| allow_next_instance_of(RequirementsManagement::Requirement) do |instance|
allow(instance).to receive(:valid?).and_return(false) allow(instance).to receive(:valid?).and_return(false)
allow(instance).to receive(:valid?).with(:before_requirement_issue).and_return(false)
end end
expect { subject }. to change { Issue.count }.by(0) expect { subject }. to change { Issue.count }.by(0)
...@@ -65,14 +68,22 @@ RSpec.describe RequirementsManagement::CreateRequirementService do ...@@ -65,14 +68,22 @@ RSpec.describe RequirementsManagement::CreateRequirementService do
end end
context 'when creation of issue fails' do context 'when creation of issue fails' do
it 'does not create requirement' do before do
allow_next_instance_of(Issue) do |instance| allow_next_instance_of(Issue) do |instance|
allow(instance).to receive(:valid?).and_return(false) allow(instance).to receive(:valid?).and_return(false)
end end
end
it 'does not create requirement' do
expect { subject }. to change { Issue.count }.by(0) expect { subject }. to change { Issue.count }.by(0)
.and change { RequirementsManagement::Requirement.count }.by(0) .and change { RequirementsManagement::Requirement.count }.by(0)
end end
it 'logs error' do
expect(::Gitlab::AppLogger).to receive(:info).with(a_hash_including(message: /Associated issue/))
subject
end
end end
end end
end end
......
...@@ -190,16 +190,11 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do ...@@ -190,16 +190,11 @@ RSpec.describe RequirementsManagement::UpdateRequirementService do
end end
end end
context 'if there is no requirement_issue' do it 'does not call the Issues::UpdateService when requirement is invalid' do
before do requirement.project = nil
requirement.update!(requirement_issue: nil) expect(Issues::UpdateService).not_to receive(:new)
end
it 'does not call the Issues::UpdateService' do
expect(Issues::UpdateService).not_to receive(:new)
subject subject
end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment