From 3c026971149c95f076b8c50a52ddbfed139d5b20 Mon Sep 17 00:00:00 2001
From: Heinrich Lee Yu <heinrich@gitlab.com>
Date: Thu, 6 Dec 2018 08:51:30 +0800
Subject: Import CSV Backend

Process CSV uploads async using a worker then email results
---
 app/controllers/projects/issues_controller.rb      | 21 +++++--
 app/mailers/emails/issues.rb                       | 11 ++++
 app/mailers/previews/notify_preview.rb             |  4 ++
 app/policies/project_policy.rb                     |  1 +
 app/services/issues/import_csv_service.rb          | 65 +++++++++++++++++++++
 app/services/upload_service.rb                     |  8 ++-
 app/views/notify/import_issues_csv_email.html.haml | 18 ++++++
 app/views/notify/import_issues_csv_email.text.erb  | 11 ++++
 app/views/projects/issues/_nav_btns.html.haml      |  2 +-
 .../projects/issues/import_csv/_modal.html.haml    |  4 +-
 app/workers/all_queues.yml                         |  1 +
 app/workers/import_issues_csv_worker.rb            | 16 ++++++
 config/sidekiq_queues.yml                          |  1 +
 locale/gitlab.pot                                  | 33 +++++++++++
 .../controllers/projects/issues_controller_spec.rb | 66 ++++++++++++++++++++++
 spec/fixtures/csv_comma.csv                        |  4 ++
 spec/fixtures/csv_semicolon.csv                    |  5 ++
 spec/fixtures/csv_tab.csv                          |  4 ++
 spec/mailers/emails/issues_spec.rb                 | 33 +++++++++++
 spec/services/issues/import_csv_service_spec.rb    | 64 +++++++++++++++++++++
 spec/workers/import_issues_csv_worker_spec.rb      | 21 +++++++
 21 files changed, 381 insertions(+), 12 deletions(-)
 create mode 100644 app/services/issues/import_csv_service.rb
 create mode 100644 app/views/notify/import_issues_csv_email.html.haml
 create mode 100644 app/views/notify/import_issues_csv_email.text.erb
 create mode 100644 app/workers/import_issues_csv_worker.rb
 create mode 100644 spec/fixtures/csv_comma.csv
 create mode 100644 spec/fixtures/csv_semicolon.csv
 create mode 100644 spec/fixtures/csv_tab.csv
 create mode 100644 spec/mailers/emails/issues_spec.rb
 create mode 100644 spec/services/issues/import_csv_service_spec.rb
 create mode 100644 spec/workers/import_issues_csv_worker_spec.rb

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
-- 
cgit v1.2.1