diff --git a/.rubocop.yml b/.rubocop.yml index b7b21128..fb99c562 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -28,3 +28,7 @@ Style/FrozenStringLiteralComment: Metrics/LineLength: Max: 100 + +Metrics/BlockLength: + Exclude: + - 'spec/**/*.rb' diff --git a/.travis.yml b/.travis.yml index 2ea2137b..0a6de680 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,7 @@ addons: apt: packages: - libhiredis-dev + chrome: stable postgresql: '9.3' services: - redis-server diff --git a/Gemfile b/Gemfile index 2350b10b..f98a7b05 100644 --- a/Gemfile +++ b/Gemfile @@ -71,7 +71,8 @@ group :test do gem 'webmock' gem 'vcr' gem 'capybara' - gem 'poltergeist' + gem 'selenium-webdriver', require: false + gem 'chromedriver-helper' gem 'database_cleaner' gem 'timecop' end diff --git a/Gemfile.lock b/Gemfile.lock index 87a192b6..3ae8ecb1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -62,6 +62,8 @@ GEM tzinfo (~> 1.1) addressable (2.5.1) public_suffix (~> 2.0, >= 2.0.2) + archive-zip (0.7.0) + io-like (~> 0.3.0) arel (6.0.4) ast (2.3.0) authlogic (3.6.0) @@ -86,9 +88,13 @@ GEM rack (>= 1.0.0) rack-test (>= 0.5.4) xpath (~> 2.0) + childprocess (0.8.0) + ffi (~> 1.0, >= 1.0.11) + chromedriver-helper (1.1.0) + archive-zip (~> 0.7.0) + nokogiri (~> 1.6) chronic (0.10.2) climate_control (0.2.0) - cliver (0.3.2) cocaine (0.5.8) climate_control (>= 0.0.3, < 1.0) coderay (1.1.1) @@ -146,6 +152,7 @@ GEM http-cookie (1.0.3) domain_name (~> 0.5) i18n (0.8.1) + io-like (0.3.0) jquery-rails (4.3.1) rails-dom-testing (>= 1, < 3) railties (>= 4.2.0) @@ -212,10 +219,6 @@ GEM plos (0.0.7) nokogiri rest-client - poltergeist (1.15.0) - capybara (~> 2.1) - cliver (~> 0.3.1) - websocket-driver (>= 0.2.0) power_assert (1.0.2) powerpack (0.1.1) pry (0.10.4) @@ -317,6 +320,9 @@ GEM tilt (>= 1.1, < 3) scrypt (3.0.5) ffi-compiler (>= 1.0, < 2.0) + selenium-webdriver (3.8.0) + childprocess (~> 0.5) + rubyzip (~> 1.0) sentry-raven (2.7.1) faraday (>= 0.7.6, < 1.0) shellany (0.0.1) @@ -376,9 +382,6 @@ GEM addressable (>= 2.3.6) crack (>= 0.3.2) hashdiff - websocket-driver (0.6.5) - websocket-extensions (>= 0.1.0) - websocket-extensions (0.1.2) whenever (0.9.7) chronic (>= 0.6.3) will_paginate (3.1.5) @@ -397,6 +400,7 @@ DEPENDENCIES bcrypt-ruby capistrano (~> 2.0) capybara + chromedriver-helper composite_primary_keys (~> 8.0) database_cleaner dotenv-rails @@ -420,7 +424,6 @@ DEPENDENCIES pg pg_search plos - poltergeist pry-rails rails (~> 4.2) rails3-generators @@ -432,6 +435,7 @@ DEPENDENCIES rvm-capistrano (= 1.4.4) sanitize sass-rails + selenium-webdriver sentry-raven shoulda-context sidekiq diff --git a/app/controllers/genotypes_controller.rb b/app/controllers/genotypes_controller.rb index 462b9d1f..47958512 100644 --- a/app/controllers/genotypes_controller.rb +++ b/app/controllers/genotypes_controller.rb @@ -6,10 +6,10 @@ class GenotypesController < ApplicationController def index @title = "Listing all genotypings" - @genotypes = - Genotype - .includes(:user) - .order("#{sort_column} #{sort_direction}") + @genotypes = Genotype + .successfully_parsed + .includes(:user) + .order("#{sort_column} #{sort_direction}") @genotypes_paginate = @genotypes.paginate(page: params[:page], per_page: 20) end @@ -19,8 +19,9 @@ def new end def create - @genotype = Genotype.create(genotype_params) + @genotype = Genotype.new(genotype_params) @genotype.user = current_user + @genotype.parse_status = 'queued' if @genotype.valid? && @genotype.save Preparsing.perform_async(@genotype.id) # award for genotyping-upload @@ -36,7 +37,11 @@ def create if current_user.has_sequence == false current_user.toggle!(:has_sequence) end - redirect_to(current_user, notice: 'Genotype was successfully uploaded! Parsing and annotating might take a couple of hours days.') + + redirect_to( + edit_user_path(current_user, anchor: 'genotypes'), + notice: t('.uploaded_successfully') + ) else render action: 'new' end @@ -61,6 +66,14 @@ def destroy redirect_to current_user end + def download + genotype = Genotype.find(params[:id]) + send_data( + File.open(genotype.genotype.path), + filename: genotype.genotype_file_name + ) + end + private def sort_column @@ -74,5 +87,4 @@ def sort_direction def genotype_params params.require(:genotype).permit(:genotype, :filetype) end - end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index d161db0d..81b5979f 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -17,4 +17,27 @@ def page_navigation_links(pages) def table_row_sequence_number(paginated, current_page_index) paginated.per_page * (paginated.current_page - 1) + current_page_index + 1 end + + def download_button(url) + link_to(url, class: 'btn btn-default btn-sm', title: t('download')) do + glyphicon('download-alt') + end + end + + def delete_button(url, opts = {}) + opts = { + method: 'delete', + class: 'btn btn-danger btn-sm', + title: t('delete'), + data: { + confirm: t('.confirm_delete', default: t('confirm_delete')) + } + }.deep_merge(opts) + + link_to(url, opts) { glyphicon('trash') } + end + + def glyphicon(name) + tag('span', class: "glyphicon glyphicon-#{name}") + end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb new file mode 100644 index 00000000..8f0513c2 --- /dev/null +++ b/app/helpers/users_helper.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module UsersHelper + def genotype_parse_status_label(parse_status) + label_class = case parse_status + when 'queued' then 'label-default' + when 'parsing' then 'label-warning' + when 'done' then 'label-success' + when 'error' then 'label-danger' + end + content_tag('span', parse_status, class: "label #{label_class}") + end +end diff --git a/app/models/genotype.rb b/app/models/genotype.rb index f752f073..27802519 100644 --- a/app/models/genotype.rb +++ b/app/models/genotype.rb @@ -5,6 +5,8 @@ class Genotype < ActiveRecord::Base belongs_to :user has_many :user_snps, dependent: :delete_all validates_presence_of :user + validates :parse_status, inclusion: { in: %w[queued parsing done error] }, + allow_nil: true has_attached_file :genotype, url: '/data/:fs_filename', path: "#{Rails.root}/public/data/:fs_filename" @@ -14,6 +16,10 @@ class Genotype < ActiveRecord::Base size: { in: 0..400.megabytes } do_not_validate_attachment_file_type :genotype + def self.successfully_parsed + where("parse_status IS NULL OR parse_status = 'done'") + end + def is_image? false end diff --git a/app/views/genotypes/index.html.erb b/app/views/genotypes/index.html.erb index ae916dc2..52b9d64a 100644 --- a/app/views/genotypes/index.html.erb +++ b/app/views/genotypes/index.html.erb @@ -31,7 +31,7 @@ <%= g.id %> <%= g.created_at%> <%= g.filetype %> - <%= link_to "Download", '../data/' + g.fs_filename, class: "btn btn-default" %> + <%= download_button(download_genotype_path(g)) %> <% end %> diff --git a/app/views/users/_edit.html.erb b/app/views/users/_edit.html.erb index cd8035e9..eb246313 100644 --- a/app/views/users/_edit.html.erb +++ b/app/views/users/_edit.html.erb @@ -4,12 +4,41 @@
@@ -39,8 +68,48 @@
+
+

Your Genotypes

+ <%= link_to(t('.genotypes.upload_new'), new_genotype_path, class: 'btn btn-default pull-right') %> + + + + + + + + + + + + + <% @user.genotypes.order('created_at DESC').each do |genotype| %> + + + + + + + + + <% end %> + +
<%= Genotype.human_attribute_name(:filetype) %><%= Genotype.human_attribute_name(:filename) %><%= Genotype.human_attribute_name(:created_at) %><%= Genotype.human_attribute_name(:parse_status) %><%= t('.genotypes.imported_snps') %>
<%= genotype.filetype %><%= genotype.genotype_file_name %><%= genotype.created_at %><%= genotype_parse_status_label(genotype.parse_status) %><%= genotype.user_snps.count %> + <%= download_button(download_genotype_path(genotype)) %> + <%= delete_button( + genotype, + data: { + confirm: t('.genotypes.confirm_delete', file_name: genotype.genotype_file_name) + } + ) %> +
+
+ <%= t('.genotypes.parse_error_info', error_label: genotype_parse_status_label('error')).html_safe %> +
+
+
-

Your Phenotypes

+

Your Phenotypes

<%= f.label :sex, "Chromosomal Sex"%>

@@ -74,7 +143,7 @@ <%= link_to "Add phenotype", {controller: "phenotypes", action: "new"}, class:"btn btn-default center-block settings__add-phenotype-button" %>
-
+
@@ -118,14 +187,7 @@
-

Deleting

-

Genotypes

- <% if not @user.genotypes.empty? %> - <% @user.genotypes.each do |ug| %> -

<%= link_to "Genotype #{ug.genotype_file_name}", ug, method: "delete", confirm: "Are you sure you want to delete genotype #{ug.genotype_file_name.to_s}?", class: "btn btn-default settings__deleting-button" %>

- <% end %> - <% end %> -

Account

+

Delete account

<%= link_to "Delete this account", @user, method: "delete", class: "btn btn-default settings__deleting-button", data: { confirm: 'Are you sure you want to delete your account?' } %>

diff --git a/app/workers/parsing.rb b/app/workers/parsing.rb index 11617d3f..bbf5c877 100644 --- a/app/workers/parsing.rb +++ b/app/workers/parsing.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Parsing include Sidekiq::Worker - sidekiq_options queue: :user_snps, retry: 5, unique: true + sidekiq_options queue: :user_snps, retry: false, unique: true attr_reader :genotype, :temp_table_name, :normalized_csv, :stats, :start_time @@ -21,11 +21,13 @@ def perform(genotype_id) send_logged(:insert_into_snps) send_logged(:insert_into_user_snps) end + genotype.update!(parse_status: 'done') send_logged(:notify_user) stats[:duration] = "#{(Time.current - start_time).round(3)}s" logger.info("Finished parsing: #{stats.to_a.map { |s| s.join('=') }.join(', ')}") rescue => e + genotype.update!(parse_status: 'error') unless genotype.parse_status == 'done' logger.error("Failed with #{e.class}: #{e.message}") raise end @@ -58,6 +60,7 @@ def normalize_csv row[3].is_a?(String) && (1..2).include?(row[3].length) end @normalized_csv = csv.map { |row| row.join(',') }.join("\n") + raise(ParseError, 'No data found in file') if @normalized_csv.empty? stats[:rows_after_parsing] = csv.length end @@ -234,4 +237,6 @@ def send_logged(method) logger.info("calling of method `#{method}` took #{took} s") ret end + + ParseError = Class.new(RuntimeError) end diff --git a/app/workers/preparsing.rb b/app/workers/preparsing.rb index 0f4e2d16..2eee513a 100644 --- a/app/workers/preparsing.rb +++ b/app/workers/preparsing.rb @@ -4,11 +4,11 @@ class Preparsing include Sidekiq::Worker - # only retry 10 times - after that, the genotyping probably has already been deleted - sidekiq_options queue: :preparse, retry: 10, unique: true + sidekiq_options queue: :preparse, retry: false, unique: true def perform(genotype_id) genotype = Genotype.find(genotype_id) + genotype.update!(parse_status: 'parsing') logger.info "Starting preparse" biggest = '' @@ -28,8 +28,8 @@ def perform(genotype_id) logger.info "copied file" end - rescue - logger.info "nothing to unzip, seems to be a text-file in the first place" + rescue Zip::Error + logger.info 'nothing to unzip, seems to be a text-file in the first place' end # now that they are unzipped, check if they're actually proper files @@ -112,19 +112,14 @@ def perform(genotype_id) # not proper file! if not file_is_ok + genotype.update!(parse_status: 'error') if file_is_duplicate UserMailer.duplicate_file(genotype.user_id).deliver_later - system("rm #{Rails.root}/public/data/#{genotype.fs_filename}") - Genotype.find_by_id(genotype.id).delete elsif file_has_mails UserMailer.file_has_mails(genotype.user_id).deliver - system("rm #{Rails.root}/public/data/#{genotype.fs_filename}") - Genotype.find_by_id(genotype.id).delete else UserMailer.parsing_error(genotype.user_id).deliver_later logger.info "file is not ok, sending email" - system("rm #{Rails.root}/public/data/#{genotype.fs_filename}") - Genotype.find_by_id(genotype.id).delete end else logger.info "Updating genotype with md5sum #{md5}" @@ -134,5 +129,8 @@ def perform(genotype_id) Parsing.perform_async(genotype.id) end + rescue StandardError + genotype.update!(parse_status: 'error') + raise end end diff --git a/config/locales/en.genotypes.yml b/config/locales/en.genotypes.yml new file mode 100644 index 00000000..41e748de --- /dev/null +++ b/config/locales/en.genotypes.yml @@ -0,0 +1,4 @@ +en: + genotypes: + create: + uploaded_successfully: 'Genotype was successfully uploaded! Parsing and annotating might take a couple of minutes.' diff --git a/config/locales/en.users.yml b/config/locales/en.users.yml new file mode 100644 index 00000000..1bc56797 --- /dev/null +++ b/config/locales/en.users.yml @@ -0,0 +1,12 @@ +en: + users: + edit: + genotypes: + confirm_delete: 'Are you sure you want to delete genotype %{file_name}?' + imported_snps: 'Imported SNPs' + upload_new: 'Upload new genotype' + parse_error_info: > + If the parse status shows %{error_label}, check if your uploaded file + looks alright and try again. Note: While zipped files (.zip) work, + gzipped ones (.gz) do not for now. If it still does not work, + contact us. diff --git a/config/locales/en.yml b/config/locales/en.yml index 250291ba..df82fae9 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -4,9 +4,15 @@ en: hello: "Hello world" activerecord: + attributes: + genotype: + created_at: 'Uploaded at' errors: models: user_phenotype: attributes: phenotype: taken: 'has already been entered.' + confirm_delete: 'Are you sure?' + delete: 'delete' + download: 'download' diff --git a/config/routes.rb b/config/routes.rb index ed8bfb48..c56ff998 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -18,7 +18,9 @@ end end resources :user_picture_phenotypes - resources :genotypes + resources :genotypes do + get 'download', on: :member + end resources :user_phenotypes resources :snps resources :users diff --git a/db/migrate/20171231121548_add_parse_status_to_genotypes.rb b/db/migrate/20171231121548_add_parse_status_to_genotypes.rb new file mode 100644 index 00000000..c7503042 --- /dev/null +++ b/db/migrate/20171231121548_add_parse_status_to_genotypes.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddParseStatusToGenotypes < ActiveRecord::Migration + def change + add_column :genotypes, :parse_status, :string + end +end diff --git a/db/structure.sql b/db/structure.sql index 23081466..57c6d620 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -501,7 +501,8 @@ CREATE TABLE genotypes ( genotype_file_name character varying(255), genotype_content_type character varying(255), genotype_file_size integer, - genotype_updated_at timestamp without time zone + genotype_updated_at timestamp without time zone, + parse_status character varying ); @@ -2169,3 +2170,5 @@ INSERT INTO schema_migrations (version) VALUES ('20161226175703'); INSERT INTO schema_migrations (version) VALUES ('20171113104813'); +INSERT INTO schema_migrations (version) VALUES ('20171231121548'); + diff --git a/spec/features/delete_genotype_spec.rb b/spec/features/delete_genotype_spec.rb index 0a153fd1..833a2152 100644 --- a/spec/features/delete_genotype_spec.rb +++ b/spec/features/delete_genotype_spec.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true -RSpec.feature 'Delete a genotype', sidekiq: :inline do + +RSpec.feature 'Delete a genotype', :js, sidekiq: :inline do let(:user) { create(:user, name: 'Gregor Mendel') } let!(:genotype) { create(:genotype, user: user, genotype_file_name: 'test.txt') } let(:award) { Achievement.find_by(award: 'Published genotyping') } @@ -16,8 +17,12 @@ click_on('Gregor Mendel') click_on('Settings') - click_on('Deleting') - click_on('Genotype test.txt') + click_on('Your genotypes') + within('#genotypes') do + page.accept_confirm( + "Are you sure you want to delete genotype #{genotype.genotype_file_name}" + ) { find('[title="delete"]').click } + end expect(page).to have_content('Your Genotyping will be deleted. ' \ 'This may take a few minutes.') diff --git a/spec/features/upload_genotype_spec.rb b/spec/features/upload_genotype_spec.rb index a76f4c3e..1d16082d 100644 --- a/spec/features/upload_genotype_spec.rb +++ b/spec/features/upload_genotype_spec.rb @@ -7,12 +7,49 @@ sign_in(user) end + let(:genotype) { Genotype.last } + scenario 'uploads first genotype' do visit '/genotypes/new' + attach_file('genotype[genotype]', File.absolute_path('test/data/23andMe_test.csv')) choose '23andme-format' - click_on 'Upload' - expect(page).to have_content('Genotype was successfully uploaded!') - expect(page).to have_content("You've unlocked an achievement:") + Sidekiq::Testing.disable! do + click_on 'Upload' + expect(page).to have_content('Genotype was successfully uploaded!') + expect(page).to have_content("You've unlocked an achievement:") + end + + expect(page).to have_content('Your genotypes') + + within('#genotypes') do + expect(find_all('table tbody tr td').map(&:text)).to eq( + [ + '23andme', + genotype.genotype_file_name, + genotype.created_at.to_s, + 'queued', + '0', + '' + ] + ) + end + + Preparsing.perform_async(genotype.id) + + visit current_url + + within('#genotypes') do + expect(find_all('table tbody tr td').map(&:text)).to eq( + [ + '23andme', + genotype.genotype_file_name, + genotype.created_at.to_s, + 'done', + '5', + '' + ] + ) + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3f086cbf..33dd7344 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -10,9 +10,8 @@ require 'factory_girl_rails' require 'pry-rails' unless ENV['CI'] require 'authlogic/test_case' -require 'capybara/poltergeist' -Capybara.javascript_driver = :poltergeist +Capybara.javascript_driver = :chrome_headless # Requires supporting ruby files with custom matchers and macros, etc, # in spec/support/ and its subdirectories. diff --git a/spec/support/chrome_webdriver.rb b/spec/support/chrome_webdriver.rb new file mode 100644 index 00000000..64978e74 --- /dev/null +++ b/spec/support/chrome_webdriver.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'selenium/webdriver' + +Capybara.register_driver :chrome do |app| + Capybara::Selenium::Driver.new(app, browser: :chrome) +end + +Capybara.register_driver :chrome_headless do |app| + capabilities = Selenium::WebDriver::Remote::Capabilities.chrome( + chromeOptions: { args: %w[headless disable-gpu] } + ) + + Capybara::Selenium::Driver.new( + app, + browser: :chrome, + desired_capabilities: capabilities + ) +end diff --git a/spec/workers/parsing_spec.rb b/spec/workers/parsing_spec.rb index 43e50a7b..673be66f 100644 --- a/spec/workers/parsing_spec.rb +++ b/spec/workers/parsing_spec.rb @@ -1,18 +1,53 @@ # frozen_string_literal: true + +# TODO: Add test cases for different filetypes. describe Parsing do - describe '#notify_user' do - let(:mail) { double('mail') } - let(:genotype) { double('genotype', id: 1) } - let(:stats) { { foos: 7 } } - - it 'sends an email to the user' do - subject.instance_variable_set(:@stats, stats) - subject.instance_variable_set(:@genotype, genotype) - expect(UserMailer).to receive(:finished_parsing).with(genotype.id, stats) - .and_return(mail) - expect(mail).to receive(:deliver_later) - - subject.notify_user - end + let(:genotype) do + create( + :genotype, + genotype: File.open(File.absolute_path('test/data/23andMe_test.csv')), + filetype: '23andme', + parse_status: 'parsing' + ) + end + + let(:emails) do + ActionMailer::Base.deliveries + end + + it 'parses a 23andme file' do + described_class.new.perform(genotype.id) + + genotype.reload + + expect(genotype.user_snps.count).to eq(5) + expect( + genotype + .user_snps + .order(:snp_name) + .pluck(:genotype_id, :snp_name, :local_genotype) + ).to eq( + [ + [genotype.id, 'rs11240777', 'AG'], + [genotype.id, 'rs12124819', 'AG'], + [genotype.id, 'rs3094315', 'AA'], + [genotype.id, 'rs3131972', 'GG'], + [genotype.id, 'rs4477212', 'AA'] + ] + ) + expect(genotype.parse_status).to eq('done') + + expect(emails.count).to eq(1) + expect(emails.first.subject).to eq('Finished parsing your genotyping') + end + + it 'sets the parse status to "error" if parsing failed' do + genotype.update!(genotype: StringIO.new('💥')) + + expect do + described_class.new.perform(genotype.id) + end.to raise_error(Parsing::ParseError, 'No data found in file') + + expect(genotype.reload.parse_status).to eq('error') end end diff --git a/spec/workers/preparsing_spec.rb b/spec/workers/preparsing_spec.rb new file mode 100644 index 00000000..4d91cddc --- /dev/null +++ b/spec/workers/preparsing_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +RSpec.describe Preparsing do + context 'when the genotype is valid' do + let(:genotype) do + create( + :genotype, + genotype: File.open(File.absolute_path('test/data/23andMe_test.csv')), + filetype: '23andme', + parse_status: 'queued' + ) + end + + it "updates the genotype's parse status" do + described_class.new.perform(genotype.id) + + expect(genotype.reload.parse_status).to eq('parsing') + end + end + + context 'when the genotype file is faulty' do + let(:genotype) do + create(:genotype, genotype: StringIO.new('XXX')) + end + + it "updates the genotype's parse status" do + described_class.new.perform(genotype.id) + + expect(genotype.reload.parse_status).to eq('error') + end + end + + context 'when the worker raises an error' do + let(:genotype) do + create(:genotype) + end + + before do + expect(File).to receive(:open).and_raise('Meh.') + end + + it "updates the genotype's parse status" do + expect do + described_class.new.perform(genotype.id) + end.to raise_error('Meh.') + + expect(genotype.reload.parse_status).to eq('error') + end + end +end diff --git a/test/functional/genotypes_controller_test.rb b/test/functional/genotypes_controller_test.rb index 01d1fd59..f92d52d6 100644 --- a/test/functional/genotypes_controller_test.rb +++ b/test/functional/genotypes_controller_test.rb @@ -62,7 +62,7 @@ class GenotypesControllerTest < ActionController::TestCase { genotype: genotype_file, filetype: "23andme"} end end - assert_redirected_to user_path(@user) + assert_redirected_to edit_user_path(@user, anchor: 'genotypes') assert_equal @publishing_award.id, UserAchievement.last.achievement_id FileUtils.rm("#{Rails.root}/test/fixtures/testdatensatz1_23andme.txt") end