diff --git a/app/services/import_csv/base_service.rb b/app/services/import_csv/base_service.rb index 628c322db1327aa931a4a246457f6a346f8c95a8..1d27a5811c7c620987dfb2b64f1657dc208df3fb 100644 --- a/app/services/import_csv/base_service.rb +++ b/app/services/import_csv/base_service.rb @@ -2,6 +2,8 @@ module ImportCsv class BaseService + include Gitlab::Utils::StrongMemoize + def initialize(user, project, csv_io) @user = user @project = project @@ -9,6 +11,8 @@ def initialize(user, project, csv_io) @results = { success: 0, error_lines: [], parse_error: false } end + PreprocessError = Class.new(StandardError) + def execute process_csv email_results_to_user @@ -36,7 +40,22 @@ def create_object_class raise NotImplementedError end + def validate_structure! + header_line = csv_data.lines.first + + validate_headers_presence!(header_line) + detect_col_sep + end + + def preprocess! + # any logic can be added in subclasses if needed + # hence just a no-op rather than NotImplementedError + end + def process_csv + validate_structure! + preprocess! + with_csv_lines.each do |row, line_no| attributes = attributes_for(row) @@ -49,21 +68,27 @@ def process_csv rescue ArgumentError, CSV::MalformedCSVError => e results[:parse_error] = true results[:error_lines].push(e.line_number) if e.respond_to?(:line_number) + rescue PreprocessError + results[:parse_error] = false end def with_csv_lines - csv_data = @csv_io.open(&:read).force_encoding(Encoding::UTF_8) - validate_headers_presence!(csv_data.lines.first) - CSV.new( csv_data, - col_sep: detect_col_sep(csv_data.lines.first), + col_sep: detect_col_sep, headers: true, header_converters: :symbol ).each.with_index(2) end - def detect_col_sep(header) + def csv_data + @csv_io.open(&:read).force_encoding(Encoding::UTF_8) + end + strong_memoize_attr :csv_data + + def detect_col_sep + header = csv_data.lines.first + if header.include?(",") "," elsif header.include?(";") @@ -74,6 +99,7 @@ def detect_col_sep(header) raise CSV::MalformedCSVError.new('Invalid CSV format', 1) end end + strong_memoize_attr :detect_col_sep def create_object(attributes) # NOTE: CSV imports are performed by workers, so we do not have a request context in order diff --git a/app/services/work_items/import_csv_service.rb b/app/services/work_items/import_csv_service.rb index 4c00d14e9f47955d9482ea5bfb348c202bffd93c..e83561832f3b1b9cf3cb1843dffda6da52862cc4 100644 --- a/app/services/work_items/import_csv_service.rb +++ b/app/services/work_items/import_csv_service.rb @@ -4,14 +4,29 @@ module WorkItems class ImportCsvService < ImportCsv::BaseService extend ::Gitlab::Utils::Override - NotAvailableError = StandardError.new('This feature is currently behind a feature flag and it is not available.') + FeatureNotAvailableError = StandardError.new( + 'This feature is currently behind a feature flag and it is not available.' + ) + NotAuthorizedError = StandardError.new('You do not have permission to import work items in this project.') + + override :initialize + def initialize(*args) + super + + @type_errors = { + blank: [], + missing: {}, + disallowed: {} + } + end def self.required_headers - %w[title].freeze + %w[title type].freeze end def execute - raise NotAvailableError if ::Feature.disabled?(:import_export_work_items_csv, project) + raise FeatureNotAvailableError if ::Feature.disabled?(:import_export_work_items_csv, project) + raise NotAuthorizedError unless Ability.allowed?(user, :create_work_item, project) super end @@ -22,6 +37,8 @@ def email_results_to_user private + attr_accessor :type_errors + def create_object(attributes) super[:work_item] end @@ -34,7 +51,7 @@ def create_object_class def attributes_for(row) { title: row[:title], - work_item_type: WorkItems::Type.default_issue_type + work_item_type: match_work_item_type(row[:type]) } end @@ -48,5 +65,52 @@ def validate_headers_presence!(headers) required_headers_message = "Required headers are missing. Required headers are #{required_headers.join(', ')}" raise CSV::MalformedCSVError.new(required_headers_message, 1) end + + def match_work_item_type(work_item_type) + match = available_work_item_types[work_item_type&.downcase] + match[:type] if match + end + + def available_work_item_types + { + issue: { + allowed: Ability.allowed?(user, :create_issue, project), + type: WorkItems::Type.default_by_type(:issue) + } + }.with_indifferent_access + end + strong_memoize_attr :available_work_item_types + + def preprocess! + with_csv_lines.each do |row, line_no| + work_item_type = row[:type]&.strip&.downcase + + if work_item_type.blank? + type_errors[:blank] << line_no + elsif missing?(work_item_type) + # does this work item exist in the range of work items we support? + (type_errors[:missing][work_item_type] ||= []) << line_no + elsif !allowed?(work_item_type) + (type_errors[:disallowed][work_item_type] ||= []) << line_no + end + end + + return if type_errors[:blank].empty? && + type_errors[:missing].blank? && + type_errors[:disallowed].blank? + + results[:type_errors] = type_errors + raise PreprocessError + end + + def missing?(work_item_type_name) + !available_work_item_types.key?(work_item_type_name) + end + + def allowed?(work_item_type_name) + !!available_work_item_types[work_item_type_name][:allowed] + end end end + +WorkItems::ImportCsvService.prepend_mod diff --git a/ee/app/services/ee/work_items/import_csv_service.rb b/ee/app/services/ee/work_items/import_csv_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..37b85888134643a1bb9e5f443311638fe3c016fa --- /dev/null +++ b/ee/app/services/ee/work_items/import_csv_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module EE + module WorkItems + module ImportCsvService + extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override + + override :available_work_item_types + def available_work_item_types + super.merge({ + requirement: { + allowed: can_create_requirements?, + type: ::WorkItems::Type.default_by_type(:requirement) + } + }).with_indifferent_access + end + + def can_create_requirements? + project.licensed_feature_available?(:requirements) && + Ability.allowed?(user, :create_requirement, project) + end + end + end +end diff --git a/ee/spec/fixtures/work_items_invalid_types.csv b/ee/spec/fixtures/work_items_invalid_types.csv new file mode 100644 index 0000000000000000000000000000000000000000..797420447ac58ff06d78acc67272430302ce03db --- /dev/null +++ b/ee/spec/fixtures/work_items_invalid_types.csv @@ -0,0 +1,4 @@ +title,type +Invalid issue,ISSUE!!! +Invalid requirement,requirement🔨 +Nonexistent type,nonsense?? diff --git a/ee/spec/fixtures/work_items_valid_types.csv b/ee/spec/fixtures/work_items_valid_types.csv new file mode 100644 index 0000000000000000000000000000000000000000..9deb622080f0d627ed7107e96b6f6cb7dc0fdb45 --- /dev/null +++ b/ee/spec/fixtures/work_items_valid_types.csv @@ -0,0 +1,3 @@ +title,type +Valid issue,issue +Valid requirement,requirement diff --git a/ee/spec/services/ee/work_items/import_csv_service_spec.rb b/ee/spec/services/ee/work_items/import_csv_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..33efe2de559b56dbb982aa012a9cf9ceeadd715b --- /dev/null +++ b/ee/spec/services/ee/work_items/import_csv_service_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::ImportCsvService, feature_category: :team_planning do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:author) { create(:user, username: 'csv_author') } + let(:file) { fixture_file_upload('ee/spec/fixtures/work_items_valid_types.csv') } + let(:service) do + uploader = FileUploader.new(project) + uploader.store!(file) + + described_class.new(user, project, uploader) + end + + let_it_be(:issue_type) { ::WorkItems::Type.default_issue_type } + let_it_be(:requirement_type) { ::WorkItems::Type.default_by_type(:requirement) } + + let(:work_items) { ::WorkItems::WorkItemsFinder.new(user, project: project).execute } + + subject { service.execute } + + describe '#execute', :aggregate_failures do + before do + project.add_maintainer(user) + stub_licensed_features(requirements: true) + end + + context 'when file is valid' do + context 'when all types are available' do + it 'creates the expected number of work items' do + expect { subject }.to change { work_items.count }.by 2 + end + + it 'sets work item attributes' do + result = subject + + expect(work_items.reload).to contain_exactly( + have_attributes( + title: 'Valid issue', + work_item_type_id: issue_type.id + ), + have_attributes( + title: 'Valid requirement', + work_item_type_id: requirement_type.id + ) + ) + + expect(result[:success]).to eq(2) + expect(result[:error_lines]).to be_empty + expect(result[:type_errors]).to be_nil + expect(result[:parse_error]).to eq(false) + end + end + + context 'when some types are unavailable' do + let(:file) { fixture_file_upload('ee/spec/fixtures/work_items_invalid_types.csv') } + + it 'throws an error and does not import' do + result = subject + + expect(result[:parse_error]).to eq(false) + expect(result[:type_errors]).to match({ + blank: [], + disallowed: {}, + missing: { + "issue!!!" => [2], + "requirement🔨" => [3], + "nonsense??" => [4] + } + }) + end + end + end + + context 'when user cannot create type' do + context 'when types include Requirement' do + shared_examples 'does not create requirement' do + specify do + result = subject + + expect(work_items.reload).not_to include( + have_attributes( + title: 'Valid requirement' + ) + ) + + expect(result[:parse_error]).to eq(false) + expect(result[:type_errors]).to match({ + blank: [], + disallowed: { "requirement" => [3] }, + missing: {} + }) + end + end + + context 'when Requirement is not licensed' do + before do + stub_licensed_features(requirements: false) + end + + it_behaves_like 'does not create requirement' + end + + context 'when user cannot create a Requirement' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :create_requirement, anything).and_return(false) + end + + it_behaves_like 'does not create requirement' + end + end + end + end +end diff --git a/spec/fixtures/work_items_invalid_types.csv b/spec/fixtures/work_items_invalid_types.csv new file mode 100644 index 0000000000000000000000000000000000000000..b82e90354519ff6eb19425ce7e4850c33a6331f7 --- /dev/null +++ b/spec/fixtures/work_items_invalid_types.csv @@ -0,0 +1,4 @@ +title,type +Invalid issue,isssue +Invalid Issue,issue!! +Missing type, diff --git a/spec/fixtures/work_items_missing_header.csv b/spec/fixtures/work_items_missing_header.csv index c1f21947469765c7af5a4c470d41b9ff569d6cc2..1a2e7ed42ab815e95acb36ff722b534441077883 100644 --- a/spec/fixtures/work_items_missing_header.csv +++ b/spec/fixtures/work_items_missing_header.csv @@ -1,3 +1,3 @@ type,created_at issue,2021-01-01 -task,2023-02-02 +other_issue,2023-02-02 diff --git a/spec/fixtures/work_items_valid.csv b/spec/fixtures/work_items_valid.csv index c8d646fd17bdc1ba0c6fa9e1521c490377765bfc..12f2f8bc81641cc1f2357efa8eac8adc1c4b3030 100644 --- a/spec/fixtures/work_items_valid.csv +++ b/spec/fixtures/work_items_valid.csv @@ -1,3 +1,3 @@ title,type -馬のスモモモモモモモモ,issue -Do this task,task +馬のスモモモモモモモモ,Issue +Issue with alternate case,ISSUE diff --git a/spec/fixtures/work_items_valid_types.csv b/spec/fixtures/work_items_valid_types.csv new file mode 100644 index 0000000000000000000000000000000000000000..f4cca9e65f1b22cdb61f6ef253814603a2fb3212 --- /dev/null +++ b/spec/fixtures/work_items_valid_types.csv @@ -0,0 +1,3 @@ +title,type +Valid issue,issue +Valid issue with alternate case,ISSUE diff --git a/spec/services/import_csv/base_service_spec.rb b/spec/services/import_csv/base_service_spec.rb index 7a4b53832a643059d394172eaf37d19d321008e9..93fff0d546a17211e1b9098da52f2f75eca76dde 100644 --- a/spec/services/import_csv/base_service_spec.rb +++ b/spec/services/import_csv/base_service_spec.rb @@ -55,42 +55,34 @@ def email_results_to_user subject { service.execute } it_behaves_like 'correctly handles invalid files' - end - - describe '#detect_col_sep' do - context 'when header contains invalid separators' do - it 'raises error' do - header = 'Name&email' - - expect { subject.send(:detect_col_sep, header) }.to raise_error(CSV::MalformedCSVError) - end - end - context 'when header is valid' do - shared_examples 'header with valid separators' do - let(:header) { "Name#{separator}email" } + describe '#detect_col_sep' do + using RSpec::Parameterized::TableSyntax - it 'returns separator value' do - expect(subject.send(:detect_col_sep, header)).to eq(separator) - end - end - - context 'when separator is ;' do - let(:separator) { ';' } + let(:file) { double } - it_behaves_like 'header with valid separators' + before do + allow(service).to receive_message_chain('csv_data.lines.first').and_return(header) end - context 'when separator is \t' do - let(:separator) { "\t" } - - it_behaves_like 'header with valid separators' + where(:sep_character, :valid) do + '&' | false + '?' | false + ';' | true + ',' | true + "\t" | true end - context 'when separator is ,' do - let(:separator) { ',' } + with_them do + let(:header) { "Name#{sep_character}email" } - it_behaves_like 'header with valid separators' + it 'responds appropriately' do + if valid + expect(service.send(:detect_col_sep)).to eq sep_character + else + expect { service.send(:detect_col_sep) }.to raise_error(CSV::MalformedCSVError) + end + end end end end diff --git a/spec/services/work_items/import_csv_service_spec.rb b/spec/services/work_items/import_csv_service_spec.rb index 83530c87c9966d0babf2684779279b66935ea2d7..504001f475558394140a41079592bc22f4c6fea5 100644 --- a/spec/services/work_items/import_csv_service_spec.rb +++ b/spec/services/work_items/import_csv_service_spec.rb @@ -6,7 +6,7 @@ let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:author) { create(:user, username: 'csv_author') } - let(:file) { fixture_file_upload('spec/fixtures/work_items_valid.csv') } + let(:file) { fixture_file_upload('spec/fixtures/work_items_valid_types.csv') } let(:service) do uploader = FileUploader.new(project) uploader.store!(file) @@ -21,43 +21,62 @@ subject { service.execute } - describe '#execute' do + describe '#execute', :aggregate_failures do context 'when user has permission' do before do - project.add_guest(user) + project.add_reporter(user) end it_behaves_like 'importer with email notification' - context 'when file is valid' do - it 'creates the expected number of work items' do - expect { subject }.to change { work_items.count }.by 2 - end + context 'when file format is valid' do + context 'when work item types are available' do + it 'creates the expected number of work items' do + expect { subject }.to change { work_items.count }.by 2 + end - describe 'imported work item details' do it 'sets work item attributes' do result = subject expect(work_items.reload).to contain_exactly( have_attributes( - title: '馬のスモモモモモモモモ', + title: 'Valid issue', work_item_type_id: issue_type.id ), have_attributes( - title: 'Do this task', + title: 'Valid issue with alternate case', work_item_type_id: issue_type.id ) ) expect(result[:success]).to eq(2) expect(result[:error_lines]).to eq([]) + expect(result[:type_errors]).to be_nil expect(result[:parse_error]).to eq(false) end + end - it 'defaults all work items to issue type' do - subject + context 'when csv contains work item types that are missing or not available' do + let(:file) { fixture_file_upload('spec/fixtures/work_items_invalid_types.csv') } - expect(work_items.reload).to all(have_attributes(work_item_type: issue_type)) + it 'creates no work items' do + expect { subject }.not_to change { work_items.count } + end + + it 'returns the correct result' do + result = subject + + expect(result[:success]).to eq(0) + expect(result[:error_lines]).to be_empty # there are problematic lines detailed below + expect(result[:parse_error]).to eq(false) + expect(result[:type_errors]).to match({ + blank: [4], + disallowed: {}, # tested in the EE version + missing: { + "isssue" => [2], + "issue!!" => [3] + } + }) end end end @@ -70,6 +89,7 @@ expect(result[:success]).to eq(0) expect(result[:error_lines]).to eq([1]) + expect(result[:type_errors]).to be_nil expect(result[:parse_error]).to eq(true) end @@ -83,30 +103,16 @@ stub_feature_flags(import_export_work_items_csv: false) end - it 'returns an error' do + it 'raises an error' do expect { subject }.to raise_error(/This feature is currently behind a feature flag and it is not available./) end end end context 'when user does not have permission' do - it 'errors on those lines', :aggregate_failures do - result = subject - - expect(result[:success]).to eq(0) - expect(result[:error_lines]).to match_array([2, 3]) - expect(result[:parse_error]).to eq(false) + it 'raises an error' do + expect { subject }.to raise_error(/You do not have permission to import work items in this project/) end end end - - context 'when user does not have permission' do - it 'errors on those lines', :aggregate_failures do - result = subject - - expect(result[:success]).to eq(0) - expect(result[:error_lines]).to eq([2, 3]) - expect(result[:parse_error]).to eq(false) - end - end end