Merge pull request #744 from okriuchykhin/ok_SCI_1484

Fix error messages in repository import and uploading files starting with hyphen [SCI-1484]

Gemfile

@@ -46,7 +46,7 @@ gem 'ajax-datatables-rails', '~> 0.3.1'
 gem 'commit_param_routing' # Enables different submit actions in the same form to route to different actions in controller
 gem 'kaminari'
 gem "i18n-js", ">= 3.0.0.rc11" # Localization in javascript files
-gem 'roo', '~> 2.1.0' # Spreadsheet parser
+gem 'roo', '~> 2.7.1' # Spreadsheet parser
 gem 'wicked_pdf'
 gem 'silencer' # Silence certain Rails logs
 gem 'wkhtmltopdf-heroku'

Gemfile.lock

@@ -190,9 +190,8 @@ GEM
       jquery-rails
       rails (>= 3.2.0)
     newrelic_rpm (4.0.0.332)
-    nokogiri (1.6.8)
+    nokogiri (1.6.8.1)
       mini_portile2 (~> 2.1.0)
-      pkg-config (~> 1.1.7)
     nokogumbo (1.4.10)
       nokogiri
     oj (2.17.4)
@@ -206,7 +205,6 @@ GEM
     parser (2.4.0.0)
       ast (~> 2.2)
     pg (0.18.4)
-    pkg-config (1.1.7)
     polyglot (0.3.5)
     powerpack (0.1.1)
     puma (2.15.3)
@@ -257,7 +255,7 @@ GEM
       algorithms (~> 0.6.1)
       stream (~> 0.5.0)
     rkelly-remix (0.0.7)
-    roo (2.1.1)
+    roo (2.7.1)
       nokogiri (~> 1)
       rubyzip (~> 1.1, < 2.0.0)
     rubocop (0.48.1)
@@ -268,7 +266,7 @@ GEM
       unicode-display_width (~> 1.0, >= 1.0.1)
     ruby-graphviz (1.2.2)
     ruby-progressbar (1.8.1)
-    rubyzip (1.1.7)
+    rubyzip (1.2.1)
     sanitize (4.4.0)
       crass (~> 1.0.2)
       nokogiri (>= 1.4.4)
@@ -390,7 +388,7 @@ DEPENDENCIES
   recaptcha
   remotipart (~> 1.2)
   rgl
-  roo (~> 2.1.0)
+  roo (~> 2.7.1)
   rubocop
   ruby-graphviz (~> 1.2)
   rubyzip

app/assets/javascripts/repositories/index.js

@@ -26,6 +26,11 @@
           });
         repositoryRecordsImporter();
       });
+    }).on('ajax:error', function(ev, data) {
+      $(this).find('.form-group').addClass('has-error');
+      $(this).find('.form-group').find('.help-block').remove();
+      $(this).find('.form-group').append("<span class='help-block'>" +
+                                         data.responseJSON.message + '</span>');
     });
   }
 

app/controllers/repositories_controller.rb

@@ -184,27 +184,27 @@ class RepositoriesController < ApplicationController
 
   def parse_sheet
     repository = current_team.repositories.find_by_id(params[:id])
-    parsed_file = ImportRepository::ParseRepository.new(
-      file: params[:file],
-      repository: repository,
-      session: session
-    )
 
-    respond_to do |format|
-      unless params[:file]
-        repository_response(t('teams.parse_sheet.errors.no_file_selected'))
-        return
-      end
-      begin
-        if parsed_file.too_large?
-          repository_response(t('general.file.size_exceeded',
-                                file_size: Constants::FILE_MAX_SIZE_MB))
-        elsif parsed_file.empty?
-          flash[:notice] = t('teams.parse_sheet.errors.empty_file')
-          redirect_to back and return
-        else
-          @import_data = parsed_file.data
-          if parsed_file.generated_temp_file?
+    unless params[:file]
+      repository_response(t('teams.parse_sheet.errors.no_file_selected'))
+      return
+    end
+    begin
+      parsed_file = ImportRepository::ParseRepository.new(
+        file: params[:file],
+        repository: repository,
+        session: session
+      )
+      if parsed_file.too_large?
+        repository_response(t('general.file.size_exceeded',
+                              file_size: Constants::FILE_MAX_SIZE_MB))
+      elsif parsed_file.empty?
+        flash[:notice] = t('teams.parse_sheet.errors.empty_file')
+        redirect_to back and return
+      else
+        @import_data = parsed_file.data
+        if parsed_file.generated_temp_file?
+          respond_to do |format|
             format.json do
               render json: {
                 html: render_to_string(
@@ -212,16 +212,16 @@ class RepositoriesController < ApplicationController
                 )
               }
             end
-          else
-            repository_response(t('teams.parse_sheet.errors.temp_file_failure'))
           end
+        else
+          repository_response(t('teams.parse_sheet.errors.temp_file_failure'))
         end
-      rescue ArgumentError, CSV::MalformedCSVError
-        repository_response(t('teams.parse_sheet.errors.invalid_file',
-                              encoding: ''.encoding))
-      rescue TypeError
-        repository_response(t('teams.parse_sheet.errors.invalid_extension'))
       end
+    rescue ArgumentError, CSV::MalformedCSVError
+      repository_response(t('teams.parse_sheet.errors.invalid_file',
+                            encoding: ''.encoding))
+    rescue TypeError
+      repository_response(t('teams.parse_sheet.errors.invalid_extension'))
     end
   end
 
@@ -238,8 +238,9 @@ class RepositoriesController < ApplicationController
                                 number_of_rows: status[:nr_of_added])
             render json: {}, status: :ok
           else
-            flash[:alert] = t('repositories.import_records.error_flash',
-                              message: status[:errors])
+            flash[:alert] =
+              t('repositories.import_records.partial_success_flash',
+                nr: status[:nr_of_added], total_nr: status[:total_nr])
             render json: {}, status: :unprocessable_entity
           end
         else
@@ -320,13 +321,15 @@ class RepositoriesController < ApplicationController
   end
 
   def repository_response(message)
-    format.html do
-      flash[:alert] = message
-      redirect_to :back
-    end
-    format.json do
-      render json: { message: message },
-        status: :unprocessable_entity
+    respond_to do |format|
+      format.html do
+        flash[:alert] = message
+        redirect_to :back
+      end
+      format.json do
+        render json: { message: message },
+          status: :unprocessable_entity
+      end
     end
   end
 

app/controllers/teams_controller.rb

@@ -36,8 +36,7 @@ class TeamsController < ApplicationController
 
             # Get data (it will trigger any errors as well)
             @header = sheet.row(1)
-            @rows = [];
-            @rows << Hash[[@header, sheet.row(2)].transpose]
+            @columns = sheet.row(2)
 
             # Fill in fields for dropdown
             @available_fields = @team.get_available_sample_fields

app/models/repository.rb

@@ -109,9 +109,10 @@ class Repository < ActiveRecord::Base
 
   # Imports records
   def import_records(sheet, mappings, user)
-    errors = []
+    errors = false
     custom_fields = []
     name_index = -1
+    total_nr = 0
     nr_of_added = 0
 
     mappings.each.with_index do |(_k, value), index|
@@ -127,13 +128,17 @@ class Repository < ActiveRecord::Base
 
     # Now we can iterate through record data and save stuff into db
     (2..sheet.last_row).each do |i|
-      error = []
+      total_nr += 1
+      cell_error = false
       record_row = RepositoryRow.new(name: sheet.row(i)[name_index],
                                  repository: self,
                                  created_by: user,
                                  last_modified_by: user)
 
-      next unless record_row.valid?
+      unless record_row.valid?
+        errors = true
+        next
+      end
       sheet.row(i).each.with_index do |value, index|
         if custom_fields[index] && value
           rep_column = RepositoryTextValue.new(
@@ -145,10 +150,11 @@ class Repository < ActiveRecord::Base
               repository_column: custom_fields[index]
             }
           )
-          error << rep_column.errors.messages unless rep_column.save
+          cell_error = true unless rep_column.save
         end
       end
-      if error.any?
+      if cell_error
+        errors = true
         record_row.destroy
       else
         nr_of_added += 1
@@ -156,10 +162,12 @@ class Repository < ActiveRecord::Base
       end
     end
 
-    if errors.count > 0
-      return { status: :error, errors: errors, nr_of_added: nr_of_added }
+    if errors
+      return { status: :error,
+               nr_of_added: nr_of_added,
+               total_nr: total_nr }
     end
-    { status: :ok, nr_of_added: nr_of_added }
+    { status: :ok, nr_of_added: nr_of_added, total_nr: total_nr }
   end
 
   private

app/services/import_repository/parse_repository.rb

@@ -11,15 +11,14 @@ module ImportRepository
     def data
       # Get data (it will trigger any errors as well)
       header = @sheet.row(1)
-      rows = []
-      rows << Hash[[header, @sheet.row(2)].transpose]
+      columns = @sheet.row(2)
       # Fill in fields for dropdown
       @repository.available_repository_fields.transform_values! do |name|
         truncate(name, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN)
       end
       @temp_file = TempFile.create(session_id: @session.id, file: @file)
       Data.new(header,
-               rows,
+               columns,
                @repository.available_repository_fields,
                @repository,
                @temp_file)
@@ -47,7 +46,7 @@ module ImportRepository
     end
 
     Data = Struct.new(
-      :header, :rows, :available_fields, :repository, :temp_file
+      :header, :columns, :available_fields, :repository, :temp_file
     )
   end
 end

app/views/repositories/_parse_records_modal.html.erb

@@ -30,29 +30,31 @@
                                include_blank: t('teams.parse_sheet.do_not_include_column'),
                                hide_label: true) %>
                   <br />
-                  <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %>
-                    <div class="modal-tooltip">
-                      <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %>
-                    </div>
+                  <% if th.nil? %>
+                    <i><%= t('repositories.import_records.no_header_name') %></i>
                   <% else %>
-                    <%= th %>
+                    <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %>
+                      <div class="modal-tooltip">
+                        <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %>
+                      </div>
+                    <% else %>
+                      <%= th %>
+                    <% end %>
                   <% end %>
                 </th>
               <% end %>
             </thead>
             <tbody>
-              <%  @import_data.rows.each do |row| %>
-                <tr>
+              <tr>
+                <td>
+                  <p><%= t('teams.parse_sheet.example_value') %></p>
+                </td>
+                <% @import_data.columns.each do |td| %>
                   <td>
-                    <p><%= t('teams.parse_sheet.example_value') %></p>
+                    <%= td %>
                   </td>
-                  <% row.each do |td| %>
-                    <td>
-                      <%= td[1] %>
-                    </td>
-                  <% end %>
-                </tr>
-              <% end %>
+                <% end %>
+              </tr>
             </tbody>
           </table>
         </div>

app/views/samples/_parse_samples_modal.html.erb

@@ -25,29 +25,31 @@
                              include_blank: t('teams.parse_sheet.do_not_include_column'),
                              hide_label: true) %>
                 <br />
-                <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %>
-                  <div class="modal-tooltip">
-                    <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %>
-                  </div>
+                <% if th.nil? %>
+                  <i><%= t('samples.modal_import.no_header_name') %></i>
                 <% else %>
-                  <%= th %>
+                  <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %>
+                    <div class="modal-tooltip">
+                      <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %>
+                    </div>
+                  <% else %>
+                    <%= th %>
+                  <% end %>
                 <% end %>
               </th>
             <% end %>
           </thead>
           <tbody>
-            <% @rows.each do |row| %>
-              <tr>
+            <tr>
+              <td>
+                <p><%= t('teams.parse_sheet.example_value') %></p>
+              </td>
+              <% @columns.each do |td| %>
                 <td>
-                  <p><%= t('teams.parse_sheet.example_value') %></p>
+                  <%= td %>
                 </td>
-                <% row.each do |td| %>
-                  <td>
-                    <%= td[1] %>
-                  </td>
-                <% end %>
-              </tr>
-            <% end %>
+              <% end %>
+            </tr>
           </tbody>
         </table>
       </div>

config/initializers/paperclip.rb

@@ -80,7 +80,7 @@ module Paperclip
     # Determine file content type from its name
     def content_types_from_name
       @content_types_from_name ||=
-        Paperclip.run('mimetype', '-b :file_name', file_name: @name).chomp
+        Paperclip.run('mimetype', '-b -- :file_name', file_name: @name).chomp
     end
 
     # Determine file media type from its name
@@ -91,7 +91,7 @@ module Paperclip
     # Determine file content type from mimetype command
     def type_from_mimetype_command
       @type_from_mimetype_command ||=
-        Paperclip.run('mimetype', '-b :file', file: @file.path).chomp
+        Paperclip.run('mimetype', '-b -- :file', file: @file.path).chomp
     end
 
     # Determine file media type from mimetype command
@@ -104,7 +104,7 @@ module Paperclip
     def type_from_file_command
       unless defined? @type_from_file_command
         @type_from_file_command =
-          Paperclip.run('file', '-b --mime :file', file: @file.path)
+          Paperclip.run('file', '-b --mime -- :file', file: @file.path)
                    .split(/[:;]\s+/).first
 
         if allowed_spoof_exception?(@type_from_file_command,

config/locales/en.yml

@@ -911,8 +911,9 @@ en:
     add_new_record: "Add new item"
     import_records:
       import: 'Import'
+      no_header_name: 'No column name'
       success_flash: "%{number_of_rows} new item(s) successfully imported."
-      error_flash: "Something went wrong: %{message}"
+      partial_success_flash: "%{nr} of %{total_nr} successfully imported. Other rows contained errors."
       error_message:
         temp_file_not_found: "This file could not be found. Your session might expire."
         session_expired: "Your session expired. Please try again."
@@ -1009,6 +1010,7 @@ en:
     modal_import:
       title: "Import samples"
       notice: "You may upload .csv file (comma separated) or tab separated file (.txt or .tdv) or Excel file (.xls, .xlsx). First row should include header names, followed by rows with sample data."
+      no_header_name: 'No column name'
       upload: "Upload file"
     modal_delete:
       title: "Delete samples"