diff options
author | Heinrich Lee Yu <heinrich@gitlab.com> | 2018-12-06 03:51:30 +0300 |
---|---|---|
committer | Heinrich Lee Yu <hleeyu@gmail.com> | 2019-01-07 06:16:58 +0300 |
commit | 3c026971149c95f076b8c50a52ddbfed139d5b20 (patch) | |
tree | 6bf0e1e8e28968887156dd524dc37e498229a9e6 | |
parent | 876ab436fabf2f44e2a6912262f980256b7c9736 (diff) |
Import CSV Backend
Process CSV uploads async using a worker then email results
21 files changed, 381 insertions, 12 deletions
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index ae48a02f623..f88eb9e0322 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -155,11 +155,11 @@ class Projects::IssuesController < Projects::ApplicationController def can_create_branch can_create = current_user && can?(current_user, :push_code, @project) && - issue.can_be_worked_on? + @issue.can_be_worked_on? respond_to do |format| format.json do - render json: { can_create_branch: can_create, suggested_branch_name: issue.suggested_branch_name } + render json: { can_create_branch: can_create, suggested_branch_name: @issue.suggested_branch_name } end end end @@ -176,10 +176,19 @@ class Projects::IssuesController < Projects::ApplicationController end def import_csv - redirect_to( - project_issues_path(project), - notice: _("Your issues are being imported. Once finished, you'll get a confirmation email.") - ) + return render_404 unless Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, project) + + service = UploadService.new(project, params[:file]) + + if service.execute + ImportIssuesCsvWorker.perform_async(current_user.id, project.id, service.uploader.upload.id) + + flash[:notice] = _("Your issues are being imported. Once finished, you'll get a confirmation email.") + else + flash[:alert] = _("File upload error.") + end + + redirect_to project_issues_path(project) end protected diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 370e6d2f90b..654ae211310 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -77,6 +77,17 @@ module Emails mail_answer_thread(issue, issue_thread_options(updated_by_user.id, recipient.id, reason)) end + def import_issues_csv_email(user_id, project_id, results) + @user = User.find(user_id) + @project = Project.find(project_id) + @results = results + + mail(to: @user.notification_email, subject: subject('Imported issues')) do |format| + format.html { render layout: 'mailer' } + format.text { render layout: 'mailer' } + end + end + private def setup_issue_mail(issue_id, recipient_id) diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 2ac4610967d..80e0a17c312 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -76,6 +76,10 @@ class NotifyPreview < ActionMailer::Preview Notify.changed_milestone_issue_email(user.id, issue.id, milestone, user.id) end + def import_issues_csv_email + Notify.import_issues_csv_email(user, project, { success: 3, errors: [5, 6, 7], valid_file: true }) + end + def closed_merge_request_email Notify.closed_merge_request_email(user.id, issue.id, user.id).message end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 3146f26bed5..0026a88b972 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -223,6 +223,7 @@ class ProjectPolicy < BasePolicy rule { ~request_access_enabled }.prevent :request_access rule { can?(:developer_access) }.policy do + enable :import_issues enable :admin_merge_request enable :admin_milestone enable :update_merge_request diff --git a/app/services/issues/import_csv_service.rb b/app/services/issues/import_csv_service.rb new file mode 100644 index 00000000000..411000a7066 --- /dev/null +++ b/app/services/issues/import_csv_service.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Issues + class ImportCsvService + def initialize(user, project, upload) + @user = user + @project = project + @uploader = upload.build_uploader + @results = { success: 0, errors: [], valid_file: true } + end + + def execute + # Cache remote file locally for processing + @uploader.cache_stored_file! unless @uploader.file_storage? + + process_csv + email_results_to_user + + cleanup_cache unless @uploader.file_storage? + + @results + end + + private + + def process_csv + CSV.foreach(@uploader.file.path, col_sep: detect_col_sep, headers: true).with_index(2) do |row, line_no| + issue = Issues::CreateService.new(@project, @user, title: row[0], description: row[1]).execute + + if issue.persisted? + @results[:success] += 1 + else + @results[:errors].push(line_no) + end + end + rescue ArgumentError, CSV::MalformedCSVError + @results[:valid_file] = false + end + + def email_results_to_user + Notify.import_issues_csv_email(@user.id, @project.id, @results).deliver_now + end + + def detect_col_sep + header = File.open(@uploader.file.path, &:readline) + + if header.include?(",") + "," + elsif header.include?(";") + ";" + elsif header.include?("\t") + "\t" + else + raise CSV::MalformedCSVError + end + end + + def cleanup_cache + cached_file_path = @uploader.file.cache_path + + File.delete(cached_file_path) + Dir.delete(File.dirname(cached_file_path)) + end + end +end diff --git a/app/services/upload_service.rb b/app/services/upload_service.rb index 39909ee4f82..47903eb48c3 100644 --- a/app/services/upload_service.rb +++ b/app/services/upload_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class UploadService + attr_accessor :uploader + def initialize(model, file, uploader_class = FileUploader, **uploader_context) @model, @file, @uploader_class, @uploader_context = model, file, uploader_class, uploader_context end @@ -8,10 +10,10 @@ class UploadService def execute return nil unless @file && @file.size <= max_attachment_size - uploader = @uploader_class.new(@model, nil, @uploader_context) - uploader.store!(@file) + @uploader = @uploader_class.new(@model, nil, @uploader_context) + @uploader.store!(@file) - uploader.to_h + @uploader.to_h end private diff --git a/app/views/notify/import_issues_csv_email.html.haml b/app/views/notify/import_issues_csv_email.html.haml new file mode 100644 index 00000000000..a6a797a5fb7 --- /dev/null +++ b/app/views/notify/import_issues_csv_email.html.haml @@ -0,0 +1,18 @@ +- text_style = 'font-size:16px; text-align:center; line-height:30px;' + +%p{ style: text_style } + Your CSV import for project + %a{ href: project_url(@project), style: "color:#3777b0; text-decoration:none;" } + = @project.full_name + has been completed. + +%p{ style: text_style } + #{pluralize(@results[:success], 'issue')} imported successfully. + +- if @results[:errors].present? + %p{ style: text_style } + Errors found on line #{'number'.pluralize(@results[:errors].size)}: #{@results[:errors].join(', ')}. + +- unless @results[:valid_file] + %p{ style: text_style } + Error parsing CSV file. diff --git a/app/views/notify/import_issues_csv_email.text.erb b/app/views/notify/import_issues_csv_email.text.erb new file mode 100644 index 00000000000..54a1762c4ec --- /dev/null +++ b/app/views/notify/import_issues_csv_email.text.erb @@ -0,0 +1,11 @@ +Your CSV import for project <%= @project.full_name %> (<%= project_url(@project) %>) has been completed. + +<%= pluralize(@results[:success], 'issue') %> imported successfully. + +<% if @results[:errors].present? %> +Errors found on line <%= 'number'.pluralize(@results[:errors].size) %>: <%= @results[:errors].join(', ') %>. +<% end %> + +<% unless @results[:valid_file] %> +Error parsing CSV file. +<% end %> diff --git a/app/views/projects/issues/_nav_btns.html.haml b/app/views/projects/issues/_nav_btns.html.haml index 4a4b8a9fcad..fd6559e37ba 100644 --- a/app/views/projects/issues/_nav_btns.html.haml +++ b/app/views/projects/issues/_nav_btns.html.haml @@ -1,5 +1,5 @@ - show_feed_buttons = local_assigns.fetch(:show_feed_buttons, true) -- show_import_button = local_assigns.fetch(:show_import_button, true) && Feature.enabled?(:issues_import_csv) +- show_import_button = local_assigns.fetch(:show_import_button, true) && Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, @project) - show_export_button = local_assigns.fetch(:show_export_button, true) .nav-controls.issues-nav-controls diff --git a/app/views/projects/issues/import_csv/_modal.html.haml b/app/views/projects/issues/import_csv/_modal.html.haml index e8576a2f17d..03a6c79ff74 100644 --- a/app/views/projects/issues/import_csv/_modal.html.haml +++ b/app/views/projects/issues/import_csv/_modal.html.haml @@ -13,9 +13,9 @@ %p = _("Your issues will be imported in the background. Once finished, you'll get a confirmation email.") .form-group - = label_tag :file, _('Upload CSV File'), class: 'label-bold' + = label_tag :file, _('Upload CSV file'), class: 'label-bold' %div - = file_field_tag :file, accept: '.csv', required: true + = file_field_tag :file, accept: 'text/csv', required: true %p.text-secondary = _('It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected.') %p.text-secondary diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index d3cf21db335..223ddc80c88 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -140,3 +140,4 @@ - detect_repository_languages - repository_cleanup - delete_stored_files +- import_issues_csv diff --git a/app/workers/import_issues_csv_worker.rb b/app/workers/import_issues_csv_worker.rb new file mode 100644 index 00000000000..5d8c02a0605 --- /dev/null +++ b/app/workers/import_issues_csv_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class ImportIssuesCsvWorker + include ApplicationWorker + + def perform(current_user_id, project_id, upload_id) + @user = User.find(current_user_id) + @project = Project.find(project_id) + @upload = Upload.find(upload_id) + + importer = Issues::ImportCsvService.new(@user, @project, @upload) + importer.execute + + @upload.destroy + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 3ee32678f34..3e8c218052d 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -85,3 +85,4 @@ - [repository_cleanup, 1] - [delete_stored_files, 1] - [remote_mirror_notification, 2] + - [import_issues_csv, 2] diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1bb94c5a61a..4a77e09bed9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2683,6 +2683,9 @@ msgstr "" msgid "Edit identity for %{user_name}" msgstr "" +msgid "Edit issues" +msgstr "" + msgid "Email" msgstr "" @@ -3067,6 +3070,9 @@ msgstr "" msgid "File templates" msgstr "" +msgid "File upload error." +msgstr "" + msgid "Files" msgstr "" @@ -3573,6 +3579,9 @@ msgstr "" msgid "Import" msgstr "" +msgid "Import CSV" +msgstr "" + msgid "Import Projects from Gitea" msgstr "" @@ -3591,6 +3600,9 @@ msgstr "" msgid "Import in progress" msgstr "" +msgid "Import issues" +msgstr "" + msgid "Import multiple repositories by uploading a manifest file." msgstr "" @@ -3729,6 +3741,9 @@ msgstr "" msgid "Issues, merge requests, pushes and comments." msgstr "" +msgid "It must have a header row and at least two columns: the first column is the issue title and the second column is the issue description. The separator is automatically detected." +msgstr "" + msgid "It's you" msgstr "" @@ -6440,6 +6455,12 @@ msgstr "" msgid "Subscribe at project level" msgstr "" +msgid "Subscribe to RSS feed" +msgstr "" + +msgid "Subscribe to calendar" +msgstr "" + msgid "Subscribed" msgstr "" @@ -6602,6 +6623,9 @@ msgstr "" msgid "The maximum file size allowed is %{max_attachment_size}mb" msgstr "" +msgid "The maximum file size allowed is %{size}." +msgstr "" + msgid "The maximum file size allowed is 200KB." msgstr "" @@ -7284,6 +7308,9 @@ msgstr "" msgid "Upload <code>GoogleCodeProjectHosting.json</code> here:" msgstr "" +msgid "Upload CSV file" +msgstr "" + msgid "Upload New File" msgstr "" @@ -7830,6 +7857,12 @@ msgstr "" msgid "Your groups" msgstr "" +msgid "Your issues are being imported. Once finished, you'll get a confirmation email." +msgstr "" + +msgid "Your issues will be imported in the background. Once finished, you'll get a confirmation email." +msgstr "" + msgid "Your name" msgstr "" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index a239ac16c0d..df21dc7bc85 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1026,6 +1026,72 @@ describe Projects::IssuesController do end end + describe 'POST #import_csv' do + let(:project) { create(:project, :public) } + let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } + + context 'feature disabled' do + it 'returns 404' do + sign_in(user) + project.add_maintainer(user) + + stub_feature_flags(issues_import_csv: false) + + import_csv + + expect(response).to have_gitlab_http_status :not_found + end + end + + context 'unauthorized' do + it 'returns 404 for guests' do + sign_out(:user) + + import_csv + + expect(response).to have_gitlab_http_status :not_found + end + + it 'returns 404 for project members with reporter role' do + sign_in(user) + project.add_reporter(user) + + import_csv + + expect(response).to have_gitlab_http_status :not_found + end + end + + context 'authorized' do + before do + sign_in(user) + project.add_developer(user) + end + + it "returns 302 for project members with developer role" do + import_csv + + expect(flash[:notice]).to include('Your issues are being imported') + expect(response).to redirect_to(project_issues_path(project)) + end + + it "shows error when upload fails" do + allow_any_instance_of(UploadService).to receive(:execute).and_return(nil) + + import_csv + + expect(flash[:alert]).to include('File upload error.') + expect(response).to redirect_to(project_issues_path(project)) + end + end + + def import_csv + post :import_csv, namespace_id: project.namespace.to_param, + project_id: project.to_param, + file: file + end + end + describe 'GET #discussions' do let!(:discussion) { create(:discussion_note_on_issue, noteable: issue, project: issue.project) } context 'when authenticated' do diff --git a/spec/fixtures/csv_comma.csv b/spec/fixtures/csv_comma.csv new file mode 100644 index 00000000000..e477a27d243 --- /dev/null +++ b/spec/fixtures/csv_comma.csv @@ -0,0 +1,4 @@ +title,description +Issue in 中文,Test description +"Hello","World" +"Title with quote""",Description diff --git a/spec/fixtures/csv_semicolon.csv b/spec/fixtures/csv_semicolon.csv new file mode 100644 index 00000000000..679797489e2 --- /dev/null +++ b/spec/fixtures/csv_semicolon.csv @@ -0,0 +1,5 @@ +title;description
+Issue in 中文;Test description
+Title with, comma;"Description"
+
+"Hello";"World"
diff --git a/spec/fixtures/csv_tab.csv b/spec/fixtures/csv_tab.csv new file mode 100644 index 00000000000..f801794ea9c --- /dev/null +++ b/spec/fixtures/csv_tab.csv @@ -0,0 +1,4 @@ +title description +Issue in 中文 Test description + "Error Row" +"Hello" "World" diff --git a/spec/mailers/emails/issues_spec.rb b/spec/mailers/emails/issues_spec.rb new file mode 100644 index 00000000000..68c87803b78 --- /dev/null +++ b/spec/mailers/emails/issues_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'email_spec' + +describe Emails::Issues do + include EmailSpec::Matchers + + describe "#import_issues_csv_email" do + let(:user) { create(:user) } + let(:project) { create(:project) } + + subject { Notify.import_issues_csv_email(user.id, project.id, @results) } + + it "shows number of successful issues imported" do + @results = { success: 165, errors: [], valid_file: true } + + expect(subject).to have_body_text "165 issues imported" + end + + it "shows error when file is invalid" do + @results = { success: 0, errors: [], valid_file: false } + + expect(subject).to have_body_text "Error parsing CSV" + end + + it "shows line numbers with errors" do + @results = { success: 0, errors: [23, 34, 58], valid_file: false } + + expect(subject).to have_body_text "23, 34, 58" + end + end +end diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb new file mode 100644 index 00000000000..f5e58bc9a9e --- /dev/null +++ b/spec/services/issues/import_csv_service_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Issues::ImportCsvService do + let(:project) { create(:project) } + let(:user) { create(:user) } + + subject do + uploader = FileUploader.new(project) + uploader.store!(file) + + described_class.new(user, project, uploader.upload).execute + end + + describe '#execute' do + context 'invalid file' do + let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } + + it 'returns invalid file error' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(0) + expect(subject[:valid_file]).to eq(false) + end + end + + context 'comma delimited file' do + let(:file) { fixture_file_upload('spec/fixtures/csv_comma.csv') } + + it 'imports CSV without errors' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(3) + expect(subject[:errors]).to eq([]) + expect(subject[:valid_file]).to eq(true) + end + end + + context 'tab delimited file with error row' do + let(:file) { fixture_file_upload('spec/fixtures/csv_tab.csv') } + + it 'imports CSV with some error rows' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(2) + expect(subject[:errors]).to eq([3]) + expect(subject[:valid_file]).to eq(true) + end + end + + context 'semicolon delimited file with CRLF' do + let(:file) { fixture_file_upload('spec/fixtures/csv_semicolon.csv') } + + it 'imports CSV with a blank row' do + expect_any_instance_of(Notify).to receive(:import_issues_csv_email) + + expect(subject[:success]).to eq(3) + expect(subject[:errors]).to eq([4]) + expect(subject[:valid_file]).to eq(true) + end + end + end +end diff --git a/spec/workers/import_issues_csv_worker_spec.rb b/spec/workers/import_issues_csv_worker_spec.rb new file mode 100644 index 00000000000..89370c4890d --- /dev/null +++ b/spec/workers/import_issues_csv_worker_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ImportIssuesCsvWorker do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:upload) { create(:upload) } + + let(:worker) { described_class.new } + + describe '#perform' do + it 'calls #execute on Issues::ImportCsvService and destroys upload' do + expect_any_instance_of(Issues::ImportCsvService).to receive(:execute).and_return({ success: 5, errors: [], valid_file: true }) + + worker.perform(user.id, project.id, upload.id) + + expect { upload.reload }.to raise_error ActiveRecord::RecordNotFound + end + end +end |