From 95a4aa6e21eed840826a74e41daef3dec7b88a3a Mon Sep 17 00:00:00 2001 From: cablett Date: Wed, 15 Feb 2023 17:10:11 +1300 Subject: [PATCH] Import basic work item data via CSV - No user-facing changes --- app/services/import_csv/base_service.rb | 3 +- .../issuable/import_csv/base_service.rb | 2 +- app/services/work_items/import_csv_service.rb | 50 ++++++++++ .../import_csv_service_spec.rb | 5 +- spec/fixtures/work_items_missing_header.csv | 3 + spec/fixtures/work_items_valid.csv | 3 + spec/services/import_csv/base_service_spec.rb | 39 +++++++- .../issues/import_csv_service_spec.rb | 3 +- .../work_items/import_csv_service_spec.rb | 99 +++++++++++++++++++ .../import_csv_service_shared_examples.rb | 38 +++++++ ...able_import_csv_service_shared_examples.rb | 37 +------ 11 files changed, 237 insertions(+), 45 deletions(-) create mode 100644 app/services/work_items/import_csv_service.rb create mode 100644 spec/fixtures/work_items_missing_header.csv create mode 100644 spec/fixtures/work_items_valid.csv create mode 100644 spec/services/work_items/import_csv_service_spec.rb create mode 100644 spec/support/services/import_csv_service_shared_examples.rb diff --git a/app/services/import_csv/base_service.rb b/app/services/import_csv/base_service.rb index feb76425fb43bb..628c322db1327a 100644 --- a/app/services/import_csv/base_service.rb +++ b/app/services/import_csv/base_service.rb @@ -46,8 +46,9 @@ def process_csv results[:error_lines].push(line_no) end end - rescue ArgumentError, CSV::MalformedCSVError + rescue ArgumentError, CSV::MalformedCSVError => e results[:parse_error] = true + results[:error_lines].push(e.line_number) if e.respond_to?(:line_number) end def with_csv_lines diff --git a/app/services/issuable/import_csv/base_service.rb b/app/services/issuable/import_csv/base_service.rb index 83cf5a6745364e..9ef9fb76e3c550 100644 --- a/app/services/issuable/import_csv/base_service.rb +++ b/app/services/issuable/import_csv/base_service.rb @@ -21,7 +21,7 @@ def validate_headers_presence!(headers) headers.downcase! if headers return if headers && headers.include?('title') && headers.include?('description') - raise CSV::MalformedCSVError + raise CSV::MalformedCSVError.new('Invalid CSV format - missing required headers.', 1) end end end diff --git a/app/services/work_items/import_csv_service.rb b/app/services/work_items/import_csv_service.rb new file mode 100644 index 00000000000000..e93905b8ca53bb --- /dev/null +++ b/app/services/work_items/import_csv_service.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +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.') + + def execute + raise NotAvailableError if ::Feature.disabled?(:import_export_work_items_csv, project) + + super + end + + def email_results_to_user + # todo as part of https://gitlab.com/gitlab-org/gitlab/-/issues/379153 + end + + private + + def create_object(attributes) + super[:work_item] + end + + def create_object_class + ::WorkItems::CreateService + end + + override :attributes_for + def attributes_for(row) + { + title: row[:title], + work_item_type: WorkItems::Type.default_issue_type + } + end + + override :validate_headers_presence! + def validate_headers_presence!(headers) + headers.downcase! if headers + return if headers && required_headers.all? { |rh| headers.include?(rh) } + + required_headers_message = "Required headers are missing. Required headers are #{required_headers.join(', ')}" + raise CSV::MalformedCSVError.new(required_headers_message, 1) + end + + def required_headers + %w[title].freeze + end + end +end diff --git a/ee/spec/services/requirements_management/import_csv_service_spec.rb b/ee/spec/services/requirements_management/import_csv_service_spec.rb index c666b212c5626b..1c96d13f37b475 100644 --- a/ee/spec/services/requirements_management/import_csv_service_spec.rb +++ b/ee/spec/services/requirements_management/import_csv_service_spec.rb @@ -5,6 +5,7 @@ RSpec.describe RequirementsManagement::ImportCsvService, feature_category: :requirements_management do let_it_be_with_refind(:project) { create(:project) } let_it_be(:user) { create(:user) } + let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } let(:service) do uploader = FileUploader.new(project) @@ -32,8 +33,6 @@ end context 'when user cannot create requirements' do - let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } - before do project.add_guest(user) end @@ -42,8 +41,6 @@ end context 'when requirements feature is not available' do - let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } - before do stub_licensed_features(requirements: false) end diff --git a/spec/fixtures/work_items_missing_header.csv b/spec/fixtures/work_items_missing_header.csv new file mode 100644 index 00000000000000..c1f21947469765 --- /dev/null +++ b/spec/fixtures/work_items_missing_header.csv @@ -0,0 +1,3 @@ +type,created_at +issue,2021-01-01 +task,2023-02-02 diff --git a/spec/fixtures/work_items_valid.csv b/spec/fixtures/work_items_valid.csv new file mode 100644 index 00000000000000..c8d646fd17bdc1 --- /dev/null +++ b/spec/fixtures/work_items_valid.csv @@ -0,0 +1,3 @@ +title,type +馬のスモモモモモモモモ,issue +Do this task,task diff --git a/spec/services/import_csv/base_service_spec.rb b/spec/services/import_csv/base_service_spec.rb index 0c0ed40ff4d21d..7a4b53832a6430 100644 --- a/spec/services/import_csv/base_service_spec.rb +++ b/spec/services/import_csv/base_service_spec.rb @@ -24,6 +24,39 @@ it_behaves_like 'abstract method', :validate_headers_presence!, "any" it_behaves_like 'abstract method', :create_object_class + context 'when given a class' do + let(:importer_klass) do + Class.new(described_class) do + def attributes_for(row) + { title: row[:title] } + end + + def validate_headers_presence!(headers) + raise CSV::MalformedCSVError.new("Missing required headers", 1) unless headers.present? + end + + def create_object_class + Class.new + end + + def email_results_to_user + # no-op + end + end + end + + let(:service) do + uploader = FileUploader.new(project) + uploader.store!(file) + + importer_klass.new(user, project, uploader) + end + + 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 @@ -42,19 +75,19 @@ end end - context 'with ; as separator' do + context 'when separator is ;' do let(:separator) { ';' } it_behaves_like 'header with valid separators' end - context 'with \t as separator' do + context 'when separator is \t' do let(:separator) { "\t" } it_behaves_like 'header with valid separators' end - context 'with , as separator' do + context 'when separator is ,' do let(:separator) { ',' } it_behaves_like 'header with valid separators' diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb index 90e360f9cf15d2..6a147782209fea 100644 --- a/spec/services/issues/import_csv_service_spec.rb +++ b/spec/services/issues/import_csv_service_spec.rb @@ -6,6 +6,7 @@ let(:project) { create(:project) } let(:user) { create(:user) } let(:assignee) { create(:user, username: 'csv_assignee') } + let(:file) { fixture_file_upload('spec/fixtures/csv_complex.csv') } let(:service) do uploader = FileUploader.new(project) uploader.store!(file) @@ -19,8 +20,6 @@ end describe '#execute' do - let(:file) { fixture_file_upload('spec/fixtures/csv_complex.csv') } - subject { service.execute } it 'sets all issueable attributes and executes quick actions' do diff --git a/spec/services/work_items/import_csv_service_spec.rb b/spec/services/work_items/import_csv_service_spec.rb new file mode 100644 index 00000000000000..658062ecdbec7d --- /dev/null +++ b/spec/services/work_items/import_csv_service_spec.rb @@ -0,0 +1,99 @@ +# 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('spec/fixtures/work_items_valid.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(:work_items) { ::WorkItems::WorkItemsFinder.new(user, project: project).execute } + + subject { service.execute } + + describe '#execute' do + context 'when user has permission' do + before do + project.add_guest(user) + end + + 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 + + describe 'imported work item details' do + it 'sets work item attributes' do + result = subject + + expect(work_items.reload).to contain_exactly( + have_attributes( + title: '馬のスモモモモモモモモ', + work_item_type_id: issue_type.id + ), + have_attributes( + title: 'Do this task', + work_item_type_id: issue_type.id + ) + ) + + expect(result[:success]).to eq(2) + expect(result[:error_lines]).to eq([]) + expect(result[:parse_error]).to eq(false) + end + + it 'defaults all work items to issue type' do + subject + + expect(work_items.reload).to all(have_attributes(work_item_type: issue_type)) + end + end + end + + context 'when file is missing necessary headers' do + let(:file) { fixture_file_upload('spec/fixtures/work_items_missing_header.csv') } + + it 'creates no records' do + result = subject + + expect(result[:success]).to eq(0) + expect(result[:error_lines]).to eq([1]) + expect(result[:parse_error]).to eq(true) + end + + it 'creates no work items' do + expect { subject }.not_to change { work_items.count } + end + end + + context 'when import_export_work_items_csv feature flag is off' do + before do + stub_feature_flags(import_export_work_items_csv: false) + end + + it 'returns 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) + end + end + end +end diff --git a/spec/support/services/import_csv_service_shared_examples.rb b/spec/support/services/import_csv_service_shared_examples.rb new file mode 100644 index 00000000000000..1555497ae48c42 --- /dev/null +++ b/spec/support/services/import_csv_service_shared_examples.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.shared_examples_for 'importer with email notification' do + it 'notifies user of import result' do + expect(Notify).to receive_message_chain(email_method, :deliver_later) + + subject + end +end + +RSpec.shared_examples 'correctly handles invalid files' do + shared_examples_for 'invalid file' do + it 'returns invalid file error' do + expect(subject[:success]).to eq(0) + expect(subject[:parse_error]).to eq(true) + end + end + + context 'when given file with unsupported extension' do + let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } + + it_behaves_like 'invalid file' + end + + context 'when given empty file' do + let(:file) { fixture_file_upload('spec/fixtures/csv_empty.csv') } + + it_behaves_like 'invalid file' + end + + context 'when given file without headers' do + let(:file) { fixture_file_upload('spec/fixtures/csv_no_headers.csv') } + + it_behaves_like 'invalid file' + end +end diff --git a/spec/support/services/issuable_import_csv_service_shared_examples.rb b/spec/support/services/issuable_import_csv_service_shared_examples.rb index 0dea6cfb729159..71740ba8ab2f2a 100644 --- a/spec/support/services/issuable_import_csv_service_shared_examples.rb +++ b/spec/support/services/issuable_import_csv_service_shared_examples.rb @@ -18,45 +18,14 @@ end end - shared_examples_for 'importer with email notification' do - it 'notifies user of import result' do - expect(Notify).to receive_message_chain(email_method, :deliver_later) - - subject - end - end - - shared_examples_for 'invalid file' do - it 'returns invalid file error' do - expect(subject[:success]).to eq(0) - expect(subject[:parse_error]).to eq(true) - end - - it_behaves_like 'importer with email notification' - it_behaves_like 'an issuable importer' - end - describe '#execute' do before do project.add_developer(user) end - context 'invalid file extension' do - let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } - - it_behaves_like 'invalid file' - end - - context 'empty file' do - let(:file) { fixture_file_upload('spec/fixtures/csv_empty.csv') } - - it_behaves_like 'invalid file' - end - - context 'file without headers' do - let(:file) { fixture_file_upload('spec/fixtures/csv_no_headers.csv') } - - it_behaves_like 'invalid file' + it_behaves_like 'correctly handles invalid files' do + it_behaves_like 'importer with email notification' + it_behaves_like 'an issuable importer' end context 'with a file generated by Gitlab CSV export' do -- GitLab