From c8d2ae70fa40fb0fe1bbdf3dc2063015d5e9ef7e Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Tue, 17 Oct 2017 14:42:06 +0200 Subject: [PATCH] Move duplicated code to service [SCI-1665] --- Gemfile | 2 +- app/controllers/repositories_controller.rb | 3 +- app/controllers/teams_controller.rb | 28 ++-------- app/models/repository.rb | 40 +------------- app/models/team.rb | 35 +----------- .../import_repository/import_records.rb | 8 ++- .../import_repository/parse_repository.rb | 26 +-------- app/services/spreadsheet_parser.rb | 55 +++++++++++++++++++ 8 files changed, 70 insertions(+), 127 deletions(-) create mode 100644 app/services/spreadsheet_parser.rb diff --git a/Gemfile b/Gemfile index 65740b8d7..9a8576cd9 100644 --- a/Gemfile +++ b/Gemfile @@ -73,6 +73,7 @@ gem 'ruby-graphviz', '~> 1.2' # Graphviz for rails gem 'tinymce-rails', '~> 4.5.7' # Rich text editor gem 'base62' # Used for smart annotations +gem 'newrelic_rpm' group :development, :test do gem 'byebug' @@ -85,7 +86,6 @@ group :development, :test do end group :production do - gem 'newrelic_rpm' gem 'puma' gem 'rails_12factor' end diff --git a/app/controllers/repositories_controller.rb b/app/controllers/repositories_controller.rb index 6b638589c..7454f1b81 100644 --- a/app/controllers/repositories_controller.rb +++ b/app/controllers/repositories_controller.rb @@ -205,8 +205,7 @@ class RepositoriesController < ApplicationController return repository_response(t('teams.parse_sheet.errors.empty_file')) end - @temp_file = parsed_file.generate_temp_file - if @temp_file + if (@temp_file = parsed_file.generate_temp_file) respond_to do |format| format.json do render json: { diff --git a/app/controllers/teams_controller.rb b/app/controllers/teams_controller.rb index 32bd3d1d8..c09e06389 100644 --- a/app/controllers/teams_controller.rb +++ b/app/controllers/teams_controller.rb @@ -17,30 +17,10 @@ class TeamsController < ApplicationController end begin - sheet = Team.open_spreadsheet(params[:file]) - # Get data (it will trigger any errors as well) - if sheet.is_a?(Roo::CSV) - @header = sheet.row(1) - @columns = sheet.row(2) - elsif sheet.is_a?(Roo::Excelx) - i = 1 - sheet.each_row_streaming do |row| - @header = row.map(&:cell_value) if i == 1 - @columns = row.map(&:cell_value) if i == 2 - i += 1 - break if i > 2 - end - else - i = 1 - sheet.rows.each do |row| - @header = row.values if i == 1 - @columns = row.values if i == 2 - i += 1 - break if i > 2 - end - end + sheet = SpreadsheetParser.open_spreadsheet(params[:file]) + @header, @columns = SpreadsheetParser.first_two_rows(sheet) - unless @header && @header.any? && @columns && @columns.any? + unless @header.any? && @columns.any? return parse_sheet_error(t('teams.parse_sheet.errors.empty_file')) end @@ -93,7 +73,7 @@ class TeamsController < ApplicationController if @temp_file.session_id == session.id # Check if mappings exists or else we don't have anything to parse if params[:mappings] - @sheet = Team.open_spreadsheet(@temp_file.file) + @sheet = SpreadsheetParser.open_spreadsheet(@temp_file.file) # Check for duplicated values h1 = params[:mappings].clone.delete_if { |k, v| v.empty? } diff --git a/app/models/repository.rb b/app/models/repository.rb index 93b5a30e4..349651882 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -58,17 +58,6 @@ class Repository < ActiveRecord::Base end end - def open_spreadsheet(file) - filename = file.original_filename - file_path = file.path - - if file.class == Paperclip::Attachment && file.is_stored_on_s3? - fa = file.fetch - file_path = fa.path - end - generate_file(filename, file_path) - end - def available_repository_fields fields = {} # First and foremost add record name @@ -131,13 +120,7 @@ class Repository < ActiveRecord::Base unless col_compact.map(&:id).uniq.length == col_compact.length return { status: :error, nr_of_added: nr_of_added, total_nr: total_nr } end - rows = if sheet.is_a?(Roo::CSV) - sheet - elsif sheet.is_a?(Roo::Excelx) - sheet.each_row_streaming - else - sheet.rows - end + rows = SpreadsheetParser.spreadsheet_enumerator(sheet) # Now we can iterate through record data and save stuff into db rows.each do |row| @@ -202,25 +185,4 @@ class Repository < ActiveRecord::Base end { status: :ok, nr_of_added: nr_of_added, total_nr: total_nr } end - - private - - def generate_file(filename, file_path) - case File.extname(filename) - when '.csv' - Roo::CSV.new(file_path, extension: :csv) - when '.tsv' - Roo::CSV.new(file_path, csv_options: { col_sep: "\t" }) - when '.txt' - # This assumption is based purely on biologist's habits - Roo::CSV.new(file_path, csv_options: { col_sep: "\t" }) - when '.xlsx' - # Roo Excel parcel was replaced with Creek, but it can be enabled back, - # just swap lines below. But only one can be enabled at the same time. - # Roo::Excelx.new(file_path) - Creek::Book.new(file_path).sheets[0] - else - raise TypeError - end - end end diff --git a/app/models/team.rb b/app/models/team.rb index 57284d2d1..ad22d7d42 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -26,33 +26,6 @@ class Team < ActiveRecord::Base has_many :protocol_keywords, inverse_of: :team, dependent: :destroy has_many :tiny_mce_assets, inverse_of: :team, dependent: :destroy has_many :repositories, dependent: :destroy - # Based on file's extension opens file (used for importing) - def self.open_spreadsheet(file) - filename = file.original_filename - file_path = file.path - - if file.class == Paperclip::Attachment and file.is_stored_on_s3? - fa = file.fetch - file_path = fa.path - end - - case File.extname(filename) - when '.csv' then - Roo::CSV.new(file_path, extension: :csv) - when '.tsv' then - Roo::CSV.new(file_path, csv_options: { col_sep: "\t" }) - when '.txt' then - # This assumption is based purely on biologist's habits - Roo::CSV.new(file_path, csv_options: { col_sep: "\t" }) - when '.xlsx' then - # Roo Excel parcel was replaced with Creek, but it can be enabled back, - # just swap lines below. But only one can be enabled at the same time. - # Roo::Excelx.new(file_path) - Creek::Book.new(file_path).sheets[0] - else - raise TypeError - end - end def search_users(query = nil) a_query = "%#{query}%" @@ -96,13 +69,7 @@ class Team < ActiveRecord::Base end end - rows = if sheet.is_a?(Roo::CSV) - sheet - elsif sheet.is_a?(Roo::Excelx) - sheet.each_row_streaming - else - sheet.rows - end + rows = SpreadsheetParser.spreadsheet_enumerator(sheet) # Now we can iterate through sample data and save stuff into db rows.each do |row| diff --git a/app/services/import_repository/import_records.rb b/app/services/import_repository/import_records.rb index e9deea8b4..ba80a4b0a 100644 --- a/app/services/import_repository/import_records.rb +++ b/app/services/import_repository/import_records.rb @@ -17,9 +17,11 @@ module ImportRepository private def run_import_actions - @repository.import_records(@repository.open_spreadsheet(@temp_file.file), - @mappings, - @user) + @repository.import_records( + SpreadsheetParser.open_spreadsheet(@temp_file.file), + @mappings, + @user + ) end def run_checks diff --git a/app/services/import_repository/parse_repository.rb b/app/services/import_repository/parse_repository.rb index 23fe97b67..222b71274 100644 --- a/app/services/import_repository/parse_repository.rb +++ b/app/services/import_repository/parse_repository.rb @@ -5,37 +5,15 @@ module ImportRepository @file = options.fetch(:file) @repository = options.fetch(:repository) @session = options.fetch(:session) - @sheet = @repository.open_spreadsheet(@file) + @sheet = SpreadsheetParser.open_spreadsheet(@file) end def data - # Get data (it will trigger any errors as well) - if @sheet.is_a?(Roo::CSV) - header = @sheet.row(1) - columns = @sheet.row(2) - elsif @sheet.is_a?(Roo::Excelx) - i = 1 - @sheet.each_row_streaming do |row| - header = row.map(&:cell_value) if i == 1 - columns = row.map(&:cell_value) if i == 2 - i += 1 - break if i > 2 - end - else - i = 1 - @sheet.rows.each do |row| - header = row.values if i == 1 - columns = row.values if i == 2 - i += 1 - break if i > 2 - end - end + header, columns = SpreadsheetParser.first_two_rows(@sheet) # Fill in fields for dropdown @repository.available_repository_fields.transform_values! do |name| truncate(name, length: Constants::NAME_TRUNCATION_LENGTH_DROPDOWN) end - header ||= [] - columns ||= [] Data.new(header, columns, @repository.available_repository_fields, diff --git a/app/services/spreadsheet_parser.rb b/app/services/spreadsheet_parser.rb new file mode 100644 index 000000000..3d5a64774 --- /dev/null +++ b/app/services/spreadsheet_parser.rb @@ -0,0 +1,55 @@ +class SpreadsheetParser + # Based on file's extension opens file (used for importing) + def self.open_spreadsheet(file) + filename = file.original_filename + file_path = file.path + + if file.class == Paperclip::Attachment && file.is_stored_on_s3? + fa = file.fetch + file_path = fa.path + end + + case File.extname(filename) + when '.csv' + Roo::CSV.new(file_path, extension: :csv) + when '.tsv' + Roo::CSV.new(file_path, csv_options: { col_sep: "\t" }) + when '.txt' + # This assumption is based purely on biologist's habits + Roo::CSV.new(file_path, csv_options: { col_sep: "\t" }) + when '.xlsx' + # Roo Excel parcel was replaced with Creek, but it can be enabled back, + # just swap lines below. But only one can be enabled at the same time. + # Roo::Excelx.new(file_path) + Creek::Book.new(file_path).sheets[0] + else + raise TypeError + end + end + + def self.spreadsheet_enumerator(sheet) + if sheet.is_a?(Roo::CSV) + sheet + elsif sheet.is_a?(Roo::Excelx) + sheet.each_row_streaming + else + sheet.rows + end + end + + def self.first_two_rows(sheet) + rows = spreadsheet_enumerator(sheet) + header = [] + columns = [] + i = 1 + rows.each do |row| + # Creek XLSX parser returns Hash of the row, Roo - Array + row = row.is_a?(Hash) ? row.values.map(&:to_s) : row.map(&:to_s) + header = row if i == 1 && row + columns = row if i == 2 && row + i += 1 + break if i > 2 + end + return header, columns + end +end