Commit 6d151372 authored by Mehmet Emin INAC's avatar Mehmet Emin INAC

Send only the Vulnerabilities::Export#id to background jobs

We can already access the exportables through export object and sending
the exportable ID to background jobs does not add any value but instead
gives the user of the interface the chance to make mistake like sending
inconsistent pair of export & exportable ID.
parent c14515d2
...@@ -4,19 +4,19 @@ module VulnerabilityExports ...@@ -4,19 +4,19 @@ module VulnerabilityExports
class CreateService class CreateService
include Gitlab::Allowable include Gitlab::Allowable
attr_reader :project, :author, :format attr_reader :exportable, :author, :format
def initialize(project, author, format:) def initialize(exportable, author, format:)
@project = project @exportable = exportable
@author = author @author = author
@format = format @format = format
end end
def execute def execute
raise Gitlab::Access::AccessDeniedError unless can?(author, :create_vulnerability_export, project) raise Gitlab::Access::AccessDeniedError unless can?(author, :create_vulnerability_export, exportable)
vulnerability_export = Vulnerabilities::Export.create(project: project, format: format, author: author) vulnerability_export = Vulnerabilities::Export.create(exportable: exportable, format: format, author: author)
::VulnerabilityExports::ExportWorker.perform_async(project.id, vulnerability_export.id) ::VulnerabilityExports::ExportWorker.perform_async(vulnerability_export.id)
vulnerability_export vulnerability_export
end end
end end
......
...@@ -8,14 +8,9 @@ module VulnerabilityExports ...@@ -8,14 +8,9 @@ module VulnerabilityExports
idempotent! idempotent!
def perform(project_id, vulnerability_export_id) def perform(_exportable_id = nil, vulnerability_export_id) # rubocop:disable Style/OptionalArguments
project = Project.find_by_id(project_id) vulnerability_export = Vulnerabilities::Export.find_by_id(vulnerability_export_id)
return unless project vulnerability_export&.destroy!
vulnerability_export = project.vulnerability_exports.find_by_id(vulnerability_export_id)
return unless vulnerability_export
vulnerability_export.destroy!
end end
end end
end end
...@@ -34,7 +34,7 @@ describe API::VulnerabilityExports do ...@@ -34,7 +34,7 @@ describe API::VulnerabilityExports do
end end
it 'schedules job for export' do it 'schedules job for export' do
expect(::VulnerabilityExports::ExportWorker).to receive(:perform_async).with(project.id, anything) expect(::VulnerabilityExports::ExportWorker).to receive(:perform_async).with(anything)
create_vulnerability_export create_vulnerability_export
end end
......
...@@ -14,7 +14,7 @@ describe VulnerabilityExports::CreateService do ...@@ -14,7 +14,7 @@ describe VulnerabilityExports::CreateService do
let(:project) { create(:project, :public, group: group) } let(:project) { create(:project, :public, group: group) }
let(:format) { 'csv' } let(:format) { 'csv' }
subject { described_class.new(project, user, format: format).execute } subject(:create_export) { described_class.new(project, user, format: format).execute }
describe '#execute' do describe '#execute' do
context 'when security dashboard feature is disabled' do context 'when security dashboard feature is disabled' do
...@@ -23,27 +23,33 @@ describe VulnerabilityExports::CreateService do ...@@ -23,27 +23,33 @@ describe VulnerabilityExports::CreateService do
end end
it 'raises an "access denied" error' do it 'raises an "access denied" error' do
expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError) expect { create_export }.to raise_error(Gitlab::Access::AccessDeniedError)
end end
end end
context 'when security dashboard feature is enabled' do context 'when security dashboard feature is enabled' do
let(:recent_vulnerability_export) { Vulnerabilities::Export.last }
before do
allow(::VulnerabilityExports::ExportWorker).to receive(:perform_async)
end
it 'does not raise an "access denied" error' do it 'does not raise an "access denied" error' do
expect { subject }.not_to raise_error expect { create_export }.not_to raise_error
end end
it 'creates new Vulnerabilities::Export' do it 'creates new Vulnerabilities::Export' do
expect { subject }.to change { Vulnerabilities::Export.count }.from(0).to(1) expect { create_export }.to change { Vulnerabilities::Export.count }.from(0).to(1)
end end
it 'schedules ::VulnerabilityExports::ExportWorker background job' do it 'schedules ::VulnerabilityExports::ExportWorker background job' do
expect(::VulnerabilityExports::ExportWorker).to receive(:perform_async).with(project.id, anything) create_export
subject expect(::VulnerabilityExports::ExportWorker).to have_received(:perform_async).with(recent_vulnerability_export.id)
end end
it 'returns new Vulnerabilities::Export with project and format assigned' do it 'returns new Vulnerabilities::Export with project and format assigned' do
expect(subject).to have_attributes(project_id: project.id, format: format) expect(create_export).to have_attributes(project_id: project.id, format: format)
end end
end end
end end
......
...@@ -4,40 +4,40 @@ require 'spec_helper' ...@@ -4,40 +4,40 @@ require 'spec_helper'
RSpec.describe VulnerabilityExports::ExportDeletionWorker, type: :worker do RSpec.describe VulnerabilityExports::ExportDeletionWorker, type: :worker do
describe '#perform' do describe '#perform' do
let_it_be(:project) { create(:project) }
let_it_be(:vulnerability_export) { create(:vulnerability_export, :finished, :csv, :with_csv_file, project: project) }
let(:worker) { described_class.new } let(:worker) { described_class.new }
subject { worker.perform(project.id, vulnerability_export.id) } subject(:delete_vulnerability_export) { worker.perform(vulnerability_export_id) }
context 'when vulnerability export does not exist' do context 'when vulnerability export does not exist' do
subject { worker.perform(project.id, 9999) } let(:vulnerability_export_id) { nil }
it 'does not raise exception' do it 'does not raise exception' do
expect { subject }.not_to raise_error expect { delete_vulnerability_export }.not_to raise_error
end end
it 'does not delete any vulnerability export from database' do it 'does not delete any vulnerability export from database' do
expect { subject }.not_to change { Vulnerabilities::Export.count } expect { delete_vulnerability_export }.not_to change { Vulnerabilities::Export.count }
end end
end end
context 'when vulnerability export exists' do context 'when vulnerability export exists' do
let_it_be(:vulnerability_export) { create(:vulnerability_export, :finished, :csv, :with_csv_file) }
let(:vulnerability_export_id) { vulnerability_export.id }
context 'when destroy can be performed successfully' do context 'when destroy can be performed successfully' do
it 'destroys vulnerability export' do it 'destroys vulnerability export' do
subject expect { delete_vulnerability_export }.to change { Vulnerabilities::Export.find_by_id(vulnerability_export.id) }.to(nil)
expect(Vulnerabilities::Export.find_by_id(vulnerability_export.id)).to be_nil
end end
end end
context 'when destroy fails' do context 'when destroy fails' do
before do before do
allow_any_instance_of(Vulnerabilities::Export).to receive(:destroy!).and_raise(ActiveRecord::RecordNotFound) allow_any_instance_of(Vulnerabilities::Export).to receive(:destroy!).and_raise(ActiveRecord::Rollback)
end end
it 'raises exception' do it 'raises exception' do
expect { subject }.to raise_error(ActiveRecord::RecordNotFound) expect { delete_vulnerability_export }.to raise_error(ActiveRecord::Rollback)
end 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