From 036ee390ed5d570a8133e7860aee56f20e4126d1 Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Fri, 14 Jul 2017 16:19:03 +0200 Subject: [PATCH 1/2] Fix error messages in repository import and uploading files starting with hyphen[SCI-1484] --- app/assets/javascripts/repositories/index.js | 5 ++ app/controllers/repositories_controller.rb | 70 ++++++++++---------- config/initializers/paperclip.rb | 6 +- 3 files changed, 44 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/repositories/index.js b/app/assets/javascripts/repositories/index.js index d6150415e..3ff5474cf 100644 --- a/app/assets/javascripts/repositories/index.js +++ b/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("" + + data.responseJSON.message + ''); }); } diff --git a/app/controllers/repositories_controller.rb b/app/controllers/repositories_controller.rb index ca41b2ed4..656f2385b 100644 --- a/app/controllers/repositories_controller.rb +++ b/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 @@ -320,13 +320,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 diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb index 2cdeb718e..4f91e6ce7 100644 --- a/config/initializers/paperclip.rb +++ b/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, From ece27b265242f08d974a7f6a176f8e4abc72f635 Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Tue, 18 Jul 2017 14:54:35 +0200 Subject: [PATCH 2/2] Fix importing files with empty header cells and error messages improvements [SCI-1484] --- Gemfile | 2 +- Gemfile.lock | 10 +++--- app/controllers/repositories_controller.rb | 5 +-- app/controllers/teams_controller.rb | 3 +- app/models/repository.rb | 24 +++++++++----- .../import_repository/parse_repository.rb | 7 ++-- .../_parse_records_modal.html.erb | 32 ++++++++++--------- .../samples/_parse_samples_modal.html.erb | 32 ++++++++++--------- config/locales/en.yml | 4 ++- 9 files changed, 65 insertions(+), 54 deletions(-) diff --git a/Gemfile b/Gemfile index cae686400..75fadeeee 100644 --- a/Gemfile +++ b/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' diff --git a/Gemfile.lock b/Gemfile.lock index 40a51e8f3..eb70da5f8 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/controllers/repositories_controller.rb b/app/controllers/repositories_controller.rb index 656f2385b..bb56e7938 100644 --- a/app/controllers/repositories_controller.rb +++ b/app/controllers/repositories_controller.rb @@ -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 diff --git a/app/controllers/teams_controller.rb b/app/controllers/teams_controller.rb index 244128f11..622cff038 100644 --- a/app/controllers/teams_controller.rb +++ b/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 diff --git a/app/models/repository.rb b/app/models/repository.rb index e47e79501..0e6c193c3 100644 --- a/app/models/repository.rb +++ b/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 diff --git a/app/services/import_repository/parse_repository.rb b/app/services/import_repository/parse_repository.rb index e40c91280..192f4b61e 100644 --- a/app/services/import_repository/parse_repository.rb +++ b/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 diff --git a/app/views/repositories/_parse_records_modal.html.erb b/app/views/repositories/_parse_records_modal.html.erb index 05925e6c5..c582b8bbe 100644 --- a/app/views/repositories/_parse_records_modal.html.erb +++ b/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) %>
- <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %> -
- <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %> -
+ <% if th.nil? %> + <%= t('repositories.import_records.no_header_name') %> <% else %> - <%= th %> + <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %> +
+ <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %> +
+ <% else %> + <%= th %> + <% end %> <% end %> <% end %> - <% @import_data.rows.each do |row| %> - + + +

<%= t('teams.parse_sheet.example_value') %>

+ + <% @import_data.columns.each do |td| %> -

<%= t('teams.parse_sheet.example_value') %>

+ <%= td %> - <% row.each do |td| %> - - <%= td[1] %> - - <% end %> - - <% end %> + <% end %> + diff --git a/app/views/samples/_parse_samples_modal.html.erb b/app/views/samples/_parse_samples_modal.html.erb index 31a946b88..613c3d444 100644 --- a/app/views/samples/_parse_samples_modal.html.erb +++ b/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) %>
- <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %> -
- <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %> -
+ <% if th.nil? %> + <%= t('samples.modal_import.no_header_name') %> <% else %> - <%= th %> + <% if th.length > Constants::NAME_TRUNCATION_LENGTH_DROPDOWN %> +
+ <%= truncate(th, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) %> +
+ <% else %> + <%= th %> + <% end %> <% end %> <% end %> - <% @rows.each do |row| %> - + + +

<%= t('teams.parse_sheet.example_value') %>

+ + <% @columns.each do |td| %> -

<%= t('teams.parse_sheet.example_value') %>

+ <%= td %> - <% row.each do |td| %> - - <%= td[1] %> - - <% end %> - - <% end %> + <% end %> + diff --git a/config/locales/en.yml b/config/locales/en.yml index fa31517ed..af6c491fe 100644 --- a/config/locales/en.yml +++ b/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"