Commit 2e6e93fd authored by Tiger Watson's avatar Tiger Watson

Merge branch 'issue_345842' into 'master'

Deprecate test reports relationship with requirements

See merge request gitlab-org/gitlab!78120
parents 0ece4cc2 bd97abee
......@@ -9,10 +9,15 @@ module IssueWidgets
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: WorkItems::Type.base_types[:requirement] }) },
foreign_key: :issue_id, inverse_of: :requirement_issue, class_name: 'RequirementsManagement::TestReport'
has_many :test_reports, foreign_key: :issue_id, inverse_of: :requirement_issue, class_name: 'RequirementsManagement::TestReport'
has_one :requirement, class_name: 'RequirementsManagement::Requirement'
scope :for_requirement_iids, -> (requirements_iids) do
requirement.joins(:requirement)
.where(requirement: { iid: requirements_iids })
.where('requirement.project_id = issues.project_id') # Prevents filtering too many rows by iids. Greatly increases performance.
end
end
def requirement_sync_error!
......
......@@ -32,9 +32,8 @@ module RequirementsManagement
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_many :test_reports, through: :requirement_issue
has_many :recent_test_reports, -> { order('requirements_management_test_reports.created_at DESC') }, through: :requirement_issue, source: :test_reports
has_internal_id :iid, scope: :project
validate :only_requirement_type_issue
......@@ -62,10 +61,10 @@ module RequirementsManagement
scope :include_last_test_report_with_state, -> do
joins(
"INNER JOIN LATERAL (
SELECT DISTINCT ON (requirement_id) requirement_id, state
SELECT DISTINCT ON (issue_id) issue_id, state
FROM requirements_management_test_reports
WHERE requirement_id = requirements.id
ORDER BY requirement_id, created_at DESC LIMIT 1
WHERE issue_id = requirements.issue_id
ORDER BY issue_id, created_at DESC LIMIT 1
) AS test_reports ON true"
)
end
......@@ -75,7 +74,7 @@ module RequirementsManagement
end
scope :without_test_reports, -> do
left_joins(:test_reports).where(requirements_management_test_reports: { requirement_id: nil })
left_joins(:test_reports).where(requirements_management_test_reports: { issue_id: nil })
end
class << self
......
......@@ -5,18 +5,12 @@ module RequirementsManagement
include Sortable
include BulkInsertSafe
belongs_to :requirement, inverse_of: :test_reports
belongs_to :author, inverse_of: :test_reports, class_name: 'User'
belongs_to :build, class_name: 'Ci::Build'
belongs_to :requirement_issue, class_name: 'Issue', foreign_key: :issue_id
# RequirementsManagement::Requirement object is going to be deprecated
# and replaced with Issue of type requirement.
# For now we need to keep both columns in sync until Requirement object is removed.
# More information in: https://gitlab.com/groups/gitlab-org/-/epics/7148
validates :requirement, presence: true
validates :state, presence: true
validates :requirement_issue, presence: true
validate :only_requirement_type_issue
enum state: { passed: 1, failed: 2 }
......@@ -25,6 +19,9 @@ module RequirementsManagement
scope :with_build, -> { where.not(build_id: nil) }
scope :for_user_build, ->(user_id, build_id) { where(author_id: user_id, build_id: build_id) }
# We need this only to perform permission checks on policy
delegate :requirement, to: :requirement_issue, allow_nil: true
class << self
def persist_requirement_reports(build, ci_report)
timestamp = Time.current
......@@ -38,10 +35,9 @@ module RequirementsManagement
bulk_insert!(reports)
end
def build_report(author: nil, state:, requirement:, build: nil, timestamp: Time.current)
def build_report(author: nil, state:, requirement_issue:, build: nil, timestamp: Time.current)
new(
requirement_id: requirement.id,
issue_id: requirement.issue_id,
issue_id: requirement_issue.id,
build_id: build&.id,
author_id: build&.user_id || author&.id,
created_at: timestamp,
......@@ -53,8 +49,8 @@ module RequirementsManagement
def passed_reports_for_all_requirements(build, timestamp)
[].tap do |reports|
build.project.requirements.opened.select(:id, :issue_id).find_each do |requirement|
reports << build_report(state: :passed, requirement: requirement, build: build, timestamp: timestamp)
build.project.issues.requirement.opened.select(:id).find_each do |requirement_issue|
reports << build_report(state: :passed, requirement_issue: requirement_issue, build: build, timestamp: timestamp)
end
end
end
......@@ -64,14 +60,23 @@ module RequirementsManagement
iids = ci_report.requirements.keys
break [] if iids.empty?
build.project.requirements.opened.select(:id, :iid, :issue_id).where(iid: iids).each do |requirement|
find_requirement_issues_by(iids, build).each do |requirement_issue|
# ignore anything with any unexpected state
new_state = ci_report.requirements[requirement.iid.to_s]
new_state = ci_report.requirements[requirement_issue.requirement_iid.to_s]
next unless states.key?(new_state)
reports << build_report(state: new_state, requirement: requirement, build: build, timestamp: timestamp)
reports << build_report(state: new_state, requirement_issue: requirement_issue, build: build, timestamp: timestamp)
end
end
end
def find_requirement_issues_by(iids, build)
# Requirement objects are used as proxy to use same iids from before.
# It makes API endpoints and pipelines references still compatible with old and new requirements iids.
# For more information check: https://gitlab.com/gitlab-org/gitlab/-/issues/345842#note_810067092
requirement_issues = build.project.issues.opened.for_requirement_iids(iids)
requirement_issues.select('issues.id, requirement.iid as requirement_iid')
end
end
......
......@@ -13,7 +13,7 @@ module RequirementsManagement
def execute
return unless @build.project.feature_available?(:requirements)
return if @build.project.requirements.empty?
return if @build.project.issues.requirement.empty?
return if test_report_already_generated?
raise Gitlab::Access::AccessDeniedError unless can?(@build.user, :create_requirement_test_report, @build.project)
......
......@@ -28,7 +28,7 @@ module RequirementsManagement
def create_test_report_for(requirement)
return unless can?(current_user, :create_requirement_test_report, project)
TestReport.build_report(requirement: requirement, state: params[:last_test_report_state], author: current_user).save!
TestReport.build_report(requirement_issue: requirement.requirement_issue, state: params[:last_test_report_state], author: current_user).save!
end
def allowlisted_requirement_params
......
......@@ -126,7 +126,7 @@ module EE
requirements_created: count(RequirementsManagement::Requirement),
requirement_test_reports_manual: count(RequirementsManagement::TestReport.without_build),
requirement_test_reports_ci: count(RequirementsManagement::TestReport.with_build),
requirements_with_test_report: distinct_count(RequirementsManagement::TestReport, :requirement_id)
requirements_with_test_report: distinct_count(RequirementsManagement::TestReport, :issue_id)
}
end
......
......@@ -3,7 +3,7 @@
FactoryBot.define do
factory :test_report, class: 'RequirementsManagement::TestReport' do
author
requirement
requirement_issue
build factory: :ci_build
state { :passed }
end
......
......@@ -56,9 +56,9 @@ RSpec.describe RequirementsManagement::RequirementsFinder do
it 'returns matched requirements' do
create(:test_report, state: :passed)
create(:test_report, requirement: requirement1, state: :failed)
create(:test_report, requirement: requirement1, state: :passed)
create(:test_report, requirement: requirement3, state: :passed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :failed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :passed)
create(:test_report, requirement_issue: requirement3.requirement_issue, state: :passed)
is_expected.to match_array([requirement1, requirement3])
end
......
......@@ -43,10 +43,10 @@ RSpec.describe Resolvers::RequirementsManagement::RequirementsResolver do
end
it 'preloads correct latest test report' do
requirement_2_latest_report = create(:test_report, requirement: requirement2, created_at: 1.hour.ago)
create(:test_report, requirement: requirement1, created_at: 2.hours.ago)
create(:test_report, requirement: requirement2, created_at: 4.hours.ago)
requirement_3_latest_report = create(:test_report, requirement: requirement3, created_at: 3.hours.ago)
requirement_2_latest_report = create(:test_report, requirement_issue: requirement2.requirement_issue, created_at: 1.hour.ago)
create(:test_report, requirement_issue: requirement1.requirement_issue, created_at: 2.hours.ago)
create(:test_report, requirement_issue: requirement2.requirement_issue, created_at: 4.hours.ago)
requirement_3_latest_report = create(:test_report, requirement_issue: requirement3.requirement_issue, created_at: 3.hours.ago)
requirements = resolve_requirements(sort: 'created_desc').to_a
......@@ -57,9 +57,9 @@ RSpec.describe Resolvers::RequirementsManagement::RequirementsResolver do
context 'when filtering by last test report state' do
before do
create(:test_report, state: :failed)
create(:test_report, requirement: requirement1, state: :passed)
create(:test_report, requirement: requirement1, state: :failed)
create(:test_report, requirement: requirement3, state: :passed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :passed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :failed)
create(:test_report, requirement_issue: requirement3.requirement_issue, state: :passed)
end
it 'filters by failed requirements' do
......
......@@ -10,8 +10,8 @@ RSpec.describe Resolvers::RequirementsManagement::TestReportsResolver do
context 'with a project' do
let_it_be(:project) { create(:project, :private) }
let_it_be(:requirement) { create(:requirement, project: project, state: :opened, created_at: 5.hours.ago) }
let_it_be(:test_report1) { create(:test_report, requirement: requirement, created_at: 3.hours.ago) }
let_it_be(:test_report2) { create(:test_report, requirement: requirement, created_at: 4.hours.ago) }
let_it_be(:test_report1) { create(:test_report, requirement_issue: requirement.requirement_issue, created_at: 3.hours.ago) }
let_it_be(:test_report2) { create(:test_report, requirement_issue: requirement.requirement_issue, created_at: 4.hours.ago) }
before do
stub_licensed_features(requirements: true)
......
......@@ -202,9 +202,9 @@ RSpec.describe Gitlab::UsageData do
let_it_be(:requirement1) { create(:requirement) }
let_it_be(:requirement2) { create(:requirement) }
let_it_be(:requirement3) { create(:requirement) }
let_it_be(:test_report1) { create(:test_report, requirement: requirement1) }
let_it_be(:test_report2) { create(:test_report, requirement: requirement2, build: nil) }
let_it_be(:test_report3) { create(:test_report, requirement: requirement1) }
let_it_be(:test_report1) { create(:test_report, requirement_issue: requirement1.requirement_issue) }
let_it_be(:test_report2) { create(:test_report, requirement_issue: requirement2.requirement_issue, build: nil) }
let_it_be(:test_report3) { create(:test_report, requirement_issue: requirement1.requirement_issue) }
context 'when requirements are disabled' do
it 'returns empty hash' do
......
......@@ -19,26 +19,6 @@ RSpec.describe Issue do
it { is_expected.to have_one(:requirement) }
it { is_expected.to have_many(:test_reports) }
context 'for an issue with associated test report' do
let_it_be(:requirement_issue) do
ri = create(:requirement_issue)
create(:test_report, requirement_issue: ri, requirement: create(:requirement))
ri
end
context 'for an issue of type Requirement' do
specify { expect(requirement_issue.test_reports.count).to eq(1) }
end
context 'for an issue of a different type' do
before do
requirement_issue.update_attribute(:issue_type, :incident)
end
specify { expect(requirement_issue.test_reports.count).to eq(0) }
end
end
end
describe 'modules' do
......@@ -58,6 +38,25 @@ RSpec.describe Issue do
end
end
describe '.for_requirement_iids' do
let_it_be(:project) { create(:project) }
let_it_be(:requirement_1) { create(:requirement, project: project) }
let_it_be(:requirement_2) { create(:requirement, project: project) }
let_it_be(:requirement_3) { create(:requirement, project: project) }
let_it_be(:requirement_4) { create(:requirement, project: project) }
context 'when issue is of type requirement' do
it 'filters requirement issues by associated requirements iids' do
iids = [requirement_1.iid, requirement_3.iid, requirement_4.iid]
requirement_4.requirement_issue.update!(issue_type: WorkItems::Type.base_types[:issue])
requirement_issues = Issue.for_requirement_iids(iids)
expect(requirement_issues).to match_array([requirement_1.requirement_issue, requirement_3.requirement_issue])
end
end
end
describe '.on_status_page' do
let_it_be(:status_page_setting) { create(:status_page_setting, :enabled) }
let_it_be(:project) { status_page_setting.project }
......
......@@ -11,8 +11,8 @@ RSpec.describe RequirementsManagement::Requirement do
it { is_expected.to belong_to(:author).class_name('User') }
it { is_expected.to belong_to(:project) }
it { is_expected.to have_many(:test_reports) }
it { is_expected.to have_many(:recent_test_reports).order(created_at: :desc) }
it { is_expected.to have_many(:test_reports).through(:requirement_issue) }
it { is_expected.to have_many(:recent_test_reports).through(:requirement_issue).order('requirements_management_test_reports.created_at DESC') }
it_behaves_like 'a model with a requirement issue association'
end
......@@ -120,11 +120,11 @@ RSpec.describe RequirementsManagement::Requirement do
let_it_be(:requirement3) { create(:requirement) }
before do
create(:test_report, requirement: requirement1, state: :passed)
create(:test_report, requirement: requirement1, state: :failed)
create(:test_report, requirement: requirement2, state: :failed)
create(:test_report, requirement: requirement2, state: :passed)
create(:test_report, requirement: requirement3, state: :passed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :passed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :failed)
create(:test_report, requirement_issue: requirement2.requirement_issue, state: :failed)
create(:test_report, requirement_issue: requirement2.requirement_issue, state: :passed)
create(:test_report, requirement_issue: requirement3.requirement_issue, state: :passed)
end
subject { described_class.with_last_test_report_state(state) }
......@@ -148,7 +148,7 @@ RSpec.describe RequirementsManagement::Requirement do
let_it_be(:requirement2) { create(:requirement) }
before do
create(:test_report, requirement: requirement2, state: :passed)
create(:test_report, requirement_issue: requirement2.requirement_issue, state: :passed)
end
it 'returns requirements without test reports' do
......@@ -161,7 +161,7 @@ RSpec.describe RequirementsManagement::Requirement do
context 'when latest test report is passing' do
it 'returns passing' do
create(:test_report, requirement: requirement, state: :passed, build: nil)
create(:test_report, requirement_issue: requirement.requirement_issue, state: :passed, build: nil)
expect(requirement.last_test_report_state).to eq('passed')
end
......@@ -169,7 +169,7 @@ RSpec.describe RequirementsManagement::Requirement do
context 'when latest test report is failing' do
it 'returns failing' do
create(:test_report, requirement: requirement, state: :failed, build: nil)
create(:test_report, requirement_issue: requirement.requirement_issue, state: :failed, build: nil)
expect(requirement.last_test_report_state).to eq('failed')
end
......@@ -187,7 +187,7 @@ RSpec.describe RequirementsManagement::Requirement do
context 'when latest test report has a build' do
it 'returns false' do
create(:test_report, requirement: requirement, state: :passed)
create(:test_report, requirement_issue: requirement.requirement_issue, state: :passed)
expect(requirement.last_test_report_manually_created?).to eq(false)
end
......@@ -195,7 +195,7 @@ RSpec.describe RequirementsManagement::Requirement do
context 'when latest test report does not have a build' do
it 'returns true' do
create(:test_report, requirement: requirement, state: :passed, build: nil)
create(:test_report, requirement_issue: requirement.requirement_issue, state: :passed, build: nil)
expect(requirement.last_test_report_manually_created?).to eq(true)
end
......
......@@ -7,7 +7,6 @@ RSpec.describe RequirementsManagement::TestReport do
subject { build(:test_report) }
it { is_expected.to belong_to(:author).class_name('User') }
it { is_expected.to belong_to(:requirement) }
it { is_expected.to belong_to(:requirement_issue) }
it { is_expected.to belong_to(:build) }
end
......@@ -19,17 +18,10 @@ RSpec.describe RequirementsManagement::TestReport do
let(:requirement_issue) { build(:requirement_issue) }
it { is_expected.to validate_presence_of(:state) }
it { is_expected.to validate_presence_of(:requirement) }
it { is_expected.to validate_presence_of(:requirement_issue) }
context 'requirements associations' do
subject { build(:test_report, requirement: requirement_arg, requirement_issue: requirement_issue_arg) }
context 'when only requirement is set' do
let(:requirement_arg) { requirement }
let(:requirement_issue_arg) { nil }
specify { expect(subject).to be_valid }
end
subject { build(:test_report, requirement_issue: requirement_issue_arg) }
context 'when only requirement issue is set' do
let(:requirement_arg) { nil }
......@@ -91,7 +83,7 @@ RSpec.describe RequirementsManagement::TestReport do
end
it 'creates test report with expected status for each open requirement' do
requirement1 = create(:requirement, state: :opened, project: project, requirement_issue: create(:requirement_issue))
requirement1 = create(:requirement, state: :opened, project: project)
requirement2 = create(:requirement, state: :opened, project: project)
create(:requirement, state: :opened) # different project
create(:requirement, state: :archived, project: project) # archived
......@@ -101,11 +93,10 @@ RSpec.describe RequirementsManagement::TestReport do
reports = RequirementsManagement::TestReport.where(build: build)
expect(reports).to match_array([
have_attributes(requirement: requirement1,
requirement_issue: requirement1.requirement_issue,
have_attributes(requirement_issue: requirement1.requirement_issue,
author: build.user,
state: 'passed'),
have_attributes(requirement: requirement2,
have_attributes(requirement_issue: requirement2.requirement_issue,
author: build.user,
state: 'failed')
......@@ -138,18 +129,16 @@ RSpec.describe RequirementsManagement::TestReport do
let_it_be(:build_author) { create(:user) }
let_it_be(:build) { create(:ci_build, author: build_author) }
let_it_be(:requirement_issue) { create(:requirement_issue)}
let_it_be(:requirement) { create(:requirement, requirement_issue: requirement_issue, state: :opened) }
let(:now) { Time.current }
context 'when build is passed as argument' do
it 'builds test report with correct attributes' do
test_report = described_class.build_report(requirement: requirement, author: user, state: 'failed', build: build, timestamp: now)
test_report = described_class.build_report(requirement_issue: requirement_issue, author: user, state: 'failed', build: build, timestamp: now)
expect(test_report.author).to eq(build.author)
expect(test_report.build).to eq(build)
expect(test_report.requirement).to eq(requirement)
expect(test_report.requirement_issue).to eq(requirement.requirement_issue)
expect(test_report.requirement_issue).to eq(requirement_issue)
expect(test_report.state).to eq('failed')
expect(test_report.created_at).to eq(now)
end
......@@ -157,12 +146,11 @@ RSpec.describe RequirementsManagement::TestReport do
context 'when build is not passed as argument' do
it 'builds test report with correct attributes' do
test_report = described_class.build_report(requirement: requirement, author: user, state: 'passed', timestamp: now)
test_report = described_class.build_report(requirement_issue: requirement_issue, author: user, state: 'passed', timestamp: now)
expect(test_report.author).to eq(user)
expect(test_report.build).to eq(nil)
expect(test_report.requirement).to eq(requirement)
expect(test_report.requirement_issue).to eq(requirement.requirement_issue)
expect(test_report.requirement_issue).to eq(requirement_issue)
expect(test_report.state).to eq('passed')
expect(test_report.created_at).to eq(now)
end
......
......@@ -64,7 +64,7 @@ RSpec.describe 'getting a requirement list for a project' do
end
context 'query performance with test reports' do
let_it_be(:test_report) { create(:test_report, requirement: requirement, state: "passed") }
let_it_be(:test_report) { create(:test_report, requirement_issue: requirement.requirement_issue, state: "passed") }
let(:fields) do
<<~QUERY
......@@ -81,7 +81,7 @@ RSpec.describe 'getting a requirement list for a project' do
control = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) }
create_list(:requirement, 3, project: project) do |requirement|
create(:test_report, requirement: requirement, state: "passed")
create(:test_report, requirement_issue: requirement.requirement_issue, state: "passed")
end
expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(control)
......@@ -96,9 +96,9 @@ RSpec.describe 'getting a requirement list for a project' do
let_it_be(:requirement2) { create(:requirement, project: filter_project, author: other_user, title: 'something about kubernetes') }
before do
create(:test_report, requirement: requirement1, state: :failed)
create(:test_report, requirement: requirement1, state: :passed)
create(:test_report, requirement: requirement2, state: :failed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :failed)
create(:test_report, requirement_issue: requirement1.requirement_issue, state: :passed)
create(:test_report, requirement_issue: requirement2.requirement_issue, state: :failed)
post_graphql(query, current_user: current_user)
end
......
......@@ -8,8 +8,8 @@ RSpec.describe 'getting test reports of a requirement' do
let_it_be(:current_user) { create(:user) }
let_it_be(:project) { create(:project) }
let_it_be(:requirement) { create(:requirement, project: project) }
let_it_be(:test_report_1) { create(:test_report, requirement: requirement, created_at: 3.days.from_now) }
let_it_be(:test_report_2) { create(:test_report, requirement: requirement, created_at: 2.days.from_now) }
let_it_be(:test_report_1) { create(:test_report, requirement_issue: requirement.requirement_issue, created_at: 3.days.from_now) }
let_it_be(:test_report_2) { create(:test_report, requirement_issue: requirement.requirement_issue, created_at: 2.days.from_now) }
let(:test_reports_data) { graphql_data['project']['requirements']['edges'][0]['node']['testReports']['edges'] }
let(:fields) do
......@@ -61,7 +61,7 @@ RSpec.describe 'getting test reports of a requirement' do
context 'with pagination' do
let_it_be(:data_path) { [:project, :requirement, :testReports] }
let_it_be(:test_report_3) { create(:test_report, requirement: requirement, created_at: 4.days.ago) }
let_it_be(:test_report_3) { create(:test_report, requirement_issue: requirement.requirement_issue, created_at: 4.days.ago) }
def pagination_query(params)
graphql_query_for(:project, { full_path: project.full_path },
......
......@@ -39,7 +39,7 @@ RSpec.describe RequirementsManagement::ExportCsvService do
end
context 'includes' do
let_it_be(:report) { create(:test_report, requirement: requirement, state: :passed, build: nil, author: user) }
let_it_be(:report) { create(:test_report, requirement_issue: requirement.requirement_issue, state: :passed, build: nil, author: user) }
let(:time_format) { '%Y-%m-%d %H:%M:%S %Z' }
......
......@@ -22,9 +22,9 @@ RSpec.describe RequirementsManagement::ProcessTestReportsService do
end
context 'when there are requirements' do
let_it_be(:requirement1) { create(:requirement, state: :opened, project: project) }
let_it_be(:requirement2) { create(:requirement, state: :opened, project: project) }
let_it_be(:requirement3) { create(:requirement, state: :archived, project: project) }
let_it_be(:requirement) { create(:requirement_issue, state: :opened, project: project) }
let_it_be(:requirement2) { create(:requirement_issue, state: :opened, project: project) }
let_it_be(:requirement3) { create(:requirement_issue, state: :closed, project: project) }
context 'when user is not allowed to create requirements test reports' do
it 'raises an exception' do
......
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