Remove caching of CSV file49231-import-issues-csv

Load whole file in memory to simplify code

9 files changed, 23 insertions, 31 deletions

diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index 3771c97fdc8..c114e16edf8 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -7,12 +7,12 @@ module UploadsActions
   UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
 
   def create
-    link_to_file = UploadService.new(model, params[:file], uploader_class).execute
+    uploader = UploadService.new(model, params[:file], uploader_class).execute
 
     respond_to do |format|
-      if link_to_file
+      if uploader
         format.json do
-          render json: { link: link_to_file.to_h }
+          render json: { link: uploader.to_h }
         end
       else
         format.json do
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 8e256b766df..d70417e710e 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -222,7 +222,7 @@ class ProjectPolicy < BasePolicy
   rule { owner | admin | guest | group_member }.prevent :request_access
   rule { ~request_access_enabled }.prevent :request_access
 
-  rule { developer & can?(:create_issue) }.enable :import_issues
+  rule { can?(:developer_access) & can?(:create_issue) }.enable :import_issues
 
   rule { can?(:developer_access) }.policy do
     enable :admin_merge_request
diff --git a/app/services/issues/import_csv_service.rb b/app/services/issues/import_csv_service.rb
index 7fa2ecc3afd..ef08fafa7cc 100644
--- a/app/services/issues/import_csv_service.rb
+++ b/app/services/issues/import_csv_service.rb
@@ -2,29 +2,26 @@
 
 module Issues
   class ImportCsvService
-    def initialize(user, project, upload)
+    def initialize(user, project, csv_io)
       @user = user
       @project = project
-      @uploader = upload.build_uploader
+      @csv_io = csv_io
       @results = { success: 0, error_lines: [], parse_error: false }
     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|
+      csv_data = @csv_io.open(&:read).force_encoding(Encoding::UTF_8)
+
+      CSV.new(csv_data, col_sep: detect_col_sep(csv_data.lines.first), headers: true).each.with_index(2) do |row, line_no|
         issue = Issues::CreateService.new(@project, @user, title: row[0], description: row[1]).execute
 
         if issue.persisted?
@@ -41,9 +38,7 @@ module Issues
       Notify.import_issues_csv_email(@user.id, @project.id, @results).deliver_now
     end
 
-    def detect_col_sep
-      header = File.open(@uploader.file.path, &:readline)
-
+    def detect_col_sep(header)
       if header.include?(",")
         ","
       elsif header.include?(";")
@@ -54,12 +49,5 @@ module Issues
         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/views/projects/issues/import_csv/_modal.html.haml b/app/views/projects/issues/import_csv/_modal.html.haml
index bef34c93e63..18768307341 100644
--- a/app/views/projects/issues/import_csv/_modal.html.haml
+++ b/app/views/projects/issues/import_csv/_modal.html.haml
@@ -15,7 +15,7 @@
             .form-group
               = label_tag :file, _('Upload CSV file'), class: 'label-bold'
               %div
-                = file_field_tag :file, accept: 'text/csv', required: true
+                = file_field_tag :file, accept: '.csv,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.')
               = _('The maximum file size allowed is %{size}.') % { size: number_to_human_size(Gitlab::CurrentSettings.max_attachment_size.megabytes) }
diff --git a/app/views/shared/empty_states/_issues.html.haml b/app/views/shared/empty_states/_issues.html.haml
index 76f6a294cb3..0434860dec4 100644
--- a/app/views/shared/empty_states/_issues.html.haml
+++ b/app/views/shared/empty_states/_issues.html.haml
@@ -1,6 +1,6 @@
 - button_path = local_assigns.fetch(:button_path, false)
 - project_select_button = local_assigns.fetch(:project_select_button, false)
-- show_import_button = local_assigns.fetch(:show_import_button, false)
+- show_import_button = local_assigns.fetch(:show_import_button, false) && Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, @project)
 - has_button = button_path || project_select_button
 
 .row.empty-state
diff --git a/app/workers/import_issues_csv_worker.rb b/app/workers/import_issues_csv_worker.rb
index 5d8c02a0605..d44fdfec8ae 100644
--- a/app/workers/import_issues_csv_worker.rb
+++ b/app/workers/import_issues_csv_worker.rb
@@ -3,12 +3,16 @@
 class ImportIssuesCsvWorker
   include ApplicationWorker
 
+  sidekiq_retries_exhausted do |job|
+    Upload.find(job['args'][2]).destroy
+  end
+
   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 = Issues::ImportCsvService.new(@user, @project, @upload.build_uploader)
     importer.execute
 
     @upload.destroy
diff --git a/lib/gitlab/email/attachment_uploader.rb b/lib/gitlab/email/attachment_uploader.rb
index bffea697f9a..3323ce60158 100644
--- a/lib/gitlab/email/attachment_uploader.rb
+++ b/lib/gitlab/email/attachment_uploader.rb
@@ -23,8 +23,8 @@ module Gitlab
               content_type: attachment.content_type
             }
 
-            link = UploadService.new(project, file).execute.to_h
-            attachments << link if link
+            uploader = UploadService.new(project, file).execute
+            attachments << uploader.to_h if uploader
           ensure
             tmp.close!
           end
diff --git a/spec/mailers/emails/issues_spec.rb b/spec/mailers/emails/issues_spec.rb
index 68c87803b78..09253cf8003 100644
--- a/spec/mailers/emails/issues_spec.rb
+++ b/spec/mailers/emails/issues_spec.rb
@@ -13,19 +13,19 @@ describe Emails::Issues do
     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 }
+      @results = { success: 165, error_lines: [], parse_error: false }
 
       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 }
+      @results = { success: 0, error_lines: [], parse_error: true }
 
       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 }
+      @results = { success: 0, error_lines: [23, 34, 58], parse_error: false }
 
       expect(subject).to have_body_text "23, 34, 58"
     end
diff --git a/spec/services/issues/import_csv_service_spec.rb b/spec/services/issues/import_csv_service_spec.rb
index 848dc2c2cd2..516a1137319 100644
--- a/spec/services/issues/import_csv_service_spec.rb
+++ b/spec/services/issues/import_csv_service_spec.rb
@@ -10,7 +10,7 @@ describe Issues::ImportCsvService do
     uploader = FileUploader.new(project)
     uploader.store!(file)
 
-    described_class.new(user, project, uploader.upload).execute
+    described_class.new(user, project, uploader).execute
   end
 
   describe '#execute' do