Commit bd97abee authored by Felipe Artur's avatar Felipe Artur

Deprecate test reports relationship with requirements

Now that all test reports have issue_id column populated
we can deprecate its relatioships with requirements and use
work items(issues of requirement type).
Requirements objects will only be used as proxy to keep its
iids compatible.

Changelog: changed
EE: true
parent 75980735
......@@ -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,15 +60,24 @@ 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
def only_requirement_type_issue
......
......@@ -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
......
......@@ -123,7 +123,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)
......
......@@ -200,9 +200,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