Refactor ZipGenotypingFiles (#527)

.rubocop.yml

@@ -22,3 +22,7 @@ Style/FrozenStringLiteralComment:
 
 Metrics/LineLength:
   Max: 100
+
+Metrics/BlockLength:
+  Exclude:
+    - 'spec/**/*.rb'

app/controllers/phenotypes_controller.rb

@@ -123,8 +123,11 @@ class PhenotypesController < ApplicationController
   end
 
   def get_genotypes
-    Sidekiq::Client.enqueue(Zipgenotypingfiles, params[:id],
-                            params[:variation], current_user.email)
+    ZipGenotypingFiles.perform_async(
+      params[:id],
+      params[:variation],
+      current_user.email
+    )
     @phenotype = Phenotype.find(params[:id])
     @variation = params[:variation]
   end

app/workers/zip_genotyping_files.rb

@@ -0,0 +1,81 @@
+# frozen_string_literal: true
+
+class ZipGenotypingFiles
+  include Sidekiq::Worker
+  sidekiq_options queue: :zipgenotyping, retry: 5, unique: true
+
+  def perform(phenotype_id, variation, target_address)
+    @phenotype = Phenotype.find(phenotype_id)
+    @variation = variation
+    @target_address = target_address
+    @time = Time.current.to_s.tr(':', '_')
+
+    if genotypes.empty?
+      send_no_results
+    else
+      zip_genotypes
+      send_results
+    end
+  end
+
+  def zip_genotypes
+    return if File.exist?(zip_file_path)
+
+    Zip::File.open(zip_file_path, Zip::File::CREATE) do |zipfile|
+      genotypes.each do |genotype|
+        zipfile.add(
+          zipped_file_name(genotype),
+          Rails.root.join('public', 'data', genotype.fs_filename)
+        )
+      end
+    end
+    File.chmod(0o777, zip_file_path)
+  end
+
+  def send_results
+    UserMailer.genotyping_results(
+      target_address,
+      zip_file_path.to_s,
+      phenotype.characteristic,
+      variation
+    ).deliver_later
+  end
+
+  def zipped_file_name(genotype)
+    "user#{genotype.user_id}_file#{genotype.id}_yearofbirth" \
+      "#{genotype.user.yearofbirth}_sex#{genotype.user.sex}.#{genotype.filetype}.txt"
+  end
+
+  def send_no_results
+    UserMailer.no_genotyping_results(
+      target_address,
+      phenotype.characteristic,
+      variation
+    ).deliver_later
+  end
+
+  private
+
+  attr_reader :phenotype, :variation, :target_address, :time
+
+  def genotypes
+    @genotypes ||= user_phenotypes.includes(:user).flat_map do |user_phenotype|
+      user_phenotype.user.genotypes
+    end
+  end
+
+  def user_phenotypes
+    UserPhenotype
+      .where(phenotype_id: phenotype.id)
+      .search(variation)
+  end
+
+  def zip_file_path
+    @zip_file_path ||= Rails.root.join(
+      'public',
+      'data',
+      'zip',
+      "#{phenotype.id}.#{time.to_s.tr(' ', '_')}.zip"
+    )
+  end
+end

app/workers/zipgenotypingfiles.rb

@@ -1,37 +0,0 @@
-# frozen_string_literal: true
-require 'zip'
-
-class Zipgenotypingfiles
-  include Sidekiq::Worker
-  sidekiq_options queue: :zipgenotyping, retry: 5, unique: true
-
-  def perform(phenotype_id, variation, target_address)
-    @user_phenotypes = UserPhenotype
-      .where(phenotype_id: phenotype_id)
-      .search(variation)
-    @genotyping_files = []
-    @user_phenotypes.each do |up|
-      @user = User.find_by_id(up.user_id)
-      if @user.genotypes.length != 0
-        @user.genotypes.each do |g|
-          @genotyping_files << g
-        end
-      end
-    end
-
-    if @genotyping_files != []
-      @time = Time.now.to_s.gsub(':', '_')
-      if File.exist?(::Rails.root.to_s + '/public/data/zip/' + phenotype_id.to_s + '.' + @time.to_s.gsub(' ', '_') + '.zip') == false
-        Zip::File.open(::Rails.root.to_s + '/public/data/zip/' + phenotype_id.to_s + '.' + @time.to_s.gsub(' ', '_') + '.zip', Zip::File::CREATE) do |zipfile|
-          @genotyping_files.each do |gen_file|
-            zipfile.add('user' + gen_file.user_id.to_s + '_file' + gen_file.id.to_s + '_yearofbirth' + gen_file.user.yearofbirth + '_sex' + gen_file.user.sex + '.' + gen_file.filetype + '.txt', ::Rails.root.to_s + '/public/data/' + gen_file.fs_filename)
-          end
-        end
-      end
-      system('chmod 777 ' + ::Rails.root.to_s + '/public/data/zip/' + phenotype_id.to_s + '.' + @time.to_s.gsub(' ', '_') + '.zip')
-      UserMailer.genotyping_results(target_address, '/data/zip/' + phenotype_id.to_s + '.' + @time.to_s.gsub(' ', '_') + '.zip', Phenotype.find_by_id(phenotype_id).characteristic, variation).deliver_later
-    else
-      UserMailer.no_genotyping_results(target_address, Phenotype.find_by_id(phenotype_id).characteristic, variation).deliver_later
-    end
-  end
-end

spec/factories/achievements.rb

@@ -2,6 +2,6 @@
 
 FactoryBot.define do
   factory :achievement do
-    award 'Foooooooo'
+    award { 'Foooooooo' }
   end
 end

spec/factories/fitbit_activities.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :fitbit_activity do
-    steps 100
-    floors 1
+    steps { 100 }
+    floors { 1 }
   end
 end

spec/factories/fitbit_bodies.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :fitbit_body do
-    weight '100'
-    bmi '200'
+    weight { '100' }
+    bmi { '200' }
   end
 end

spec/factories/fitbit_sleeps.rb

@@ -2,9 +2,9 @@
 
 FactoryBot.define do
   factory :fitbit_sleep do
-    minutes_asleep 480
-    minutes_awake 10
-    number_awakenings 1
-    minutes_to_sleep 10
+    minutes_asleep { 480 }
+    minutes_awake { 10 }
+    number_awakenings { 1 }
+    minutes_to_sleep { 10 }
   end
 end

spec/factories/genome_gov_papers.rb

@@ -2,6 +2,6 @@
 
 FactoryBot.define do
   factory :genome_gov_paper do
-    title 'A Genome.gov Paper'
+    title { 'A Genome.gov Paper' }
   end
 end

spec/factories/genotypes.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :genotype do
-    genotype_file_name 'foo.txt'
+    genotype_file_name { 'foo.txt' }
     user
   end
 end

spec/factories/mendeley_papers.rb

@@ -2,11 +2,11 @@
 
 FactoryBot.define do
   factory :mendeley_paper do
-    title 'Musterstudie'
+    title { 'Musterstudie' }
     uuid { UUIDTools::UUID.random_create.to_s }
-    first_author 'Max Mustermann'
-    mendeley_url 'http://example.com'
-    doi '10.1000/182'
-    pub_year 2013
+    first_author { 'Max Mustermann' }
+    mendeley_url { 'http://example.com' }
+    doi { '10.1000/182' }
+    pub_year { 2013 }
   end
 end

spec/factories/messages.rb

@@ -3,8 +3,8 @@
 FactoryBot.define do
   factory :message do
     user
-    sent false
-    subject 'HELLO WORLD'
-    body 'THIS IS AN AWESOME MESSAGE'
+    sent { false }
+    subject { 'HELLO WORLD' }
+    body { 'THIS IS AN AWESOME MESSAGE' }
   end
 end

spec/factories/phenotypes.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :phenotype do
-    characteristic 'Penis length'
+    characteristic { 'Penis length' }
 
     factory :phenotype_with_users do
       after :create do |t|

spec/factories/picture_phenotypes.rb

@@ -2,6 +2,6 @@
 
 FactoryBot.define do
   factory :picture_phenotype do
-    characteristic 'Eye color'
+    characteristic { 'Eye color' }
   end
 end

spec/factories/plos_papers.rb

@@ -2,6 +2,6 @@
 
 FactoryBot.define do
   factory :plos_paper do
-    title 'A PLOS Paper'
+    title { 'A PLOS Paper' }
   end
 end

spec/factories/snp_comments.rb

@@ -2,9 +2,9 @@
 
 FactoryBot.define do
   factory :snp_comment do
-    comment_text 'This is a great SNP!'
-    subject 'Great!'
-    user_id 1
-    snp_id 1
+    comment_text { 'This is a great SNP!' }
+    subject { 'Great!' }
+    user_id { 1 }
+    snp_id { 1 }
   end
 end

spec/factories/snpedia_papers.rb

@@ -2,6 +2,6 @@
 
 FactoryBot.define do
   factory :snpedia_paper do
-    url 'http://www.snpedia.com/index.php/Rs1234(A;C)'
+    url { 'http://www.snpedia.com/index.php/Rs1234(A;C)' }
   end
 end

spec/factories/snps.rb

@@ -5,8 +5,8 @@ FactoryBot.define do
     sequence(:name) { |i| "rs#{i}" }
     sequence(:position) { |i| i }
     sequence(:chromosome) { |i| i }
-    genotype_frequency('AA' => 1)
-    allele_frequency('A' => 0, 'T' => 0, 'G' => 0, 'C' => 0)
-    ranking 0
+    genotype_frequency { { 'AA' => 1 } }
+    allele_frequency { { 'A' => 0, 'T' => 0, 'G' => 0, 'C' => 0 } }
+    ranking { 0 }
   end
 end

spec/factories/user_phenotypes.rb

@@ -4,6 +4,6 @@ FactoryBot.define do
   factory :user_phenotype do
     association :user
     association :phenotype
-    variation 'pink'
+    variation { 'pink' }
   end
 end

spec/factories/user_picture_phenotypes.rb

@@ -2,6 +2,6 @@
 
 FactoryBot.define do
   factory :user_picture_phenotype do
-    variation 'pink'
+    variation { 'pink' }
   end
 end

spec/factories/user_snps.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :user_snp do
-    local_genotype 'AG'
+    local_genotype { 'AG' }
     genotype
     user
     snp

spec/factories/users.rb

@@ -2,11 +2,11 @@
 
 FactoryBot.define do
   factory :user do
-    name 'Dogbert'
+    name { 'Dogbert' }
     sequence(:email) { |i| "fubert#{i}@example.org" }
-    password 'strengjeheim'
-    password_confirmation 'strengjeheim'
-    sex 'yes please'
-    yearofbirth '1970'
+    password { 'strengjeheim' }
+    password_confirmation { 'strengjeheim' }
+    sex { 'yes please' }
+    yearofbirth { '1970' }
   end
 end

spec/integration/zipgenotypingfiles_spec.rb

@@ -1,31 +0,0 @@
-# frozen_string_literal: true
-RSpec.describe Zipgenotypingfiles do
-  let!(:user) { create(:user) }
-  let!(:phenotype) { create(:phenotype) }
-  let!(:user_phenotype) { create(:user_phenotype, phenotype: phenotype, user: user) }
-  let!(:genotype) do
-    create(
-      :genotype,
-      user: user,
-      genotype: File.open(Rails.root.join('spec', 'fixtures', 'files', 'empty'))
-    )
-  end
-  let(:file_path) { "data/zip/#{phenotype.id}.#{Time.now.strftime('%d.%m.%Y_%H_%M')}.zip" }
-  let(:fs_path) { Rails.root.join('public', file_path) }
-
-  around { |example| Timecop.freeze(Time.new(2015, 4, 6, 10, 21), &example) }
-
-  after { File.delete(fs_path) }
-
-  it "doesn't fail" do
-    subject.perform(phenotype.id, user_phenotype.variation, 'user@example.com')
-
-    mail = ActionMailer::Base.deliveries.last
-    
-    expect(mail.subject).to include('The data you requested is ready to be downloaded')
-    mail.parts.each do |part|
-      expect(part.body).to include("http://opensnp.org/#{file_path}")
-    end
-    expect(File.exist?(fs_path)).to be(true)
-  end
-end

spec/workers/zip_genotyping_files_spec.rb

@@ -0,0 +1,114 @@
+# frozen_string_literal: true
+
+RSpec.describe ZipGenotypingFiles do
+  subject(:worker) { described_class.new }
+
+  let!(:tentacle) { create(:phenotype, characteristic: 'tentacle') }
+
+  context 'when there are genotypes found' do
+    # User with matching phenotype and variant
+    let!(:user_1) { create(:user) }
+    let!(:genotype_1) do
+      create(:genotype, user: user_1, genotype: genotype_file_1)
+    end
+    let(:genotype_file_1) do
+      StringIO.new('user 1 genotype')
+    end
+    let!(:user_phenotype_1) do
+      create(
+        :user_phenotype,
+        user: user_1,
+        phenotype: tentacle,
+        variation: 'purple'
+      )
+    end
+
+    # Second user with matching phenotype and variant
+    let!(:user_2) { create(:user) }
+    let!(:genotype_2) do
+      create(:genotype, user: user_2, genotype: genotype_file_2)
+    end
+    let(:genotype_file_2) do
+      StringIO.new('user 2 genotype')
+    end
+    let!(:user_phenotype_2) do
+      create(
+        :user_phenotype,
+        user: user_2,
+        phenotype: tentacle,
+        variation: 'also purple'
+      )
+    end
+
+    # User with other variant
+    let!(:user_3) { create(:user) }
+    let!(:genotype_3) do
+      create(:genotype, user: user_3, genotype: genotype_file_3)
+    end
+    let(:genotype_file_3) do
+      StringIO.new('user 3 genotype')
+    end
+    let!(:user_phenotype_3) do
+      create(
+        :user_phenotype,
+        user: user_3,
+        phenotype: tentacle,
+        variation: 'green'
+      )
+    end
+
+    let!(:slime) { create(:phenotype, characteristic: 'slime') }
+
+    # User with other phenotype
+    let!(:user_4) { create(:user) }
+    let!(:genotype_4) do
+      create(:genotype, user: user_4, genotype: genotype_file_4)
+    end
+    let(:genotype_file_4) do
+      StringIO.new('user 4 genotype')
+    end
+    let!(:user_phenotype_4) do
+      create(
+        :user_phenotype,
+        user: user_4,
+        phenotype: slime,
+        variation: 'purple'
+      )
+    end
+
+    it 'zips genotyping files' do
+      worker.perform(
+        tentacle.id,
+        user_phenotype_1.variation,
+        'user@example.com'
+      )
+
+      mail = ActionMailer::Base.deliveries.last
+      expect(mail.subject)
+        .to eq('openSNP.org: The data you requested is ready to be downloaded')
+
+      file = mail.text_part.decoded[%r{data/zip/.*?\.zip}]
+
+      Zip::File.open(Rails.root.join('public', file)) do |zip_file|
+        expect(zip_file.entries.count).to eq(2)
+        expect(zip_file.glob("user#{user_1.id}_*.txt").first.get_input_stream.read)
+          .to eq(File.read(genotype_1.genotype.path))
+        expect(zip_file.glob("user#{user_2.id}_*.txt").first.get_input_stream.read)
+          .to eq(File.read(genotype_2.genotype.path))
+      end
+    end
+  end
+
+  context 'when there are no genotypes found' do
+    it 'tells the user' do
+      worker.perform(
+        tentacle.id,
+        'blue',
+        'user@example.com'
+      )
+
+      expect(ActionMailer::Base.deliveries.last.subject)
+        .to eq('openSNP.org: No genotyping files match your search')
+    end
+  end
+end