From c58afdb651cd019064caa4ac0de79d8d577b915b Mon Sep 17 00:00:00 2001
From: Kevin De Pelseneer <kevin.depelseneer@psb.ugent.be>
Date: Tue, 19 Sep 2023 10:52:30 +0200
Subject: [PATCH] Refactor the upload_samples function

---
 app/controllers/single_pages_controller.rb | 224 ++++++++++++---------
 1 file changed, 129 insertions(+), 95 deletions(-)

diff --git a/app/controllers/single_pages_controller.rb b/app/controllers/single_pages_controller.rb
index 510593e00c..a16e525ce7 100644
--- a/app/controllers/single_pages_controller.rb
+++ b/app/controllers/single_pages_controller.rb
@@ -1,5 +1,6 @@
 require 'isatab_converter'
 
+# Controller for the Single Page view
 class SinglePagesController < ApplicationController
   include Seek::AssetsCommon
   include Seek::Sharing::SharingCommon
@@ -15,9 +16,7 @@ class SinglePagesController < ApplicationController
   def show
     @project = Project.find(params[:id])
     @folders = project_folders
-    respond_to do |format|
-      format.html
-    end
+    respond_to(&:html)
   end
 
   def index; end
@@ -62,21 +61,20 @@ def export_isa
   end
 
   def download_samples_excel
-
     sample_ids, sample_type_id, study_id, assay_id = Rails.cache.read(params[:uuid]).values_at(:sample_ids, :sample_type_id,
-                                                                                     :study_id, :assay_id)
+                                                                                               :study_id, :assay_id)
 
     @study = Study.find(study_id)
     @assay = Assay.find(assay_id) unless assay_id.nil?
     @project = @study.projects.first
-    @samples = Sample.where(id: sample_ids)&.authorized_for(:view).sort_by(&:id)
+    @samples = Sample.where(id: sample_ids)&.authorized_for(:view)&.sort_by(&:id)
 
-    raise "Nothing to export to Excel." if @samples.nil? || @samples == [] || sample_type_id.nil?
+    raise 'Nothing to export to Excel.' if @samples.nil? || @samples == [] || sample_type_id.nil?
 
     @sample_type = SampleType.find(sample_type_id)
 
     sample_attributes = @sample_type.sample_attributes.map do |sa|
-      obj = if (sa.sample_controlled_vocab_id.nil?)
+      obj = if sa.sample_controlled_vocab_id.nil?
               { sa_cv_title: sa.title, sa_cv_id: nil }
             else
               { sa_cv_title: sa.title, sa_cv_id: sa.sample_controlled_vocab_id }
@@ -84,17 +82,20 @@ def download_samples_excel
       obj.merge({ required: sa.required })
     end
 
-    @sa_cv_terms = [{ "name" => "id", "has_cv" => false, "data" => nil, "allows_custom_input" => nil, "required" => nil },
-                    { "name" => "uuid", "has_cv" => false, "data" => nil, "allows_custom_input" => nil, "required" => nil }]
+    @sa_cv_terms = [{ 'name' => 'id', 'has_cv' => false, 'data' => nil, 'allows_custom_input' => nil, 'required' => nil },
+                    { 'name' => 'uuid', 'has_cv' => false, 'data' => nil, 'allows_custom_input' => nil,
+                      'required' => nil }]
 
     sample_attributes.map do |sa|
-        if sa[:sa_cv_id].nil?
-          @sa_cv_terms.push({ "name" => sa[:sa_cv_title], "has_cv" => false, "data" => nil, "allows_custom_input" => nil, "required" => sa[:required] })
-        else
-          allows_custom_input = SampleControlledVocab.find(sa[:sa_cv_id])&.custom_input
-          sa_terms = SampleControlledVocabTerm.where(sample_controlled_vocab_id: sa[:sa_cv_id]).map(&:label)
-          @sa_cv_terms.push({ "name" => sa[:sa_cv_title], "has_cv" => true, "data" => sa_terms, "allows_custom_input" => allows_custom_input, "required" => sa[:required] })
-        end
+      if sa[:sa_cv_id].nil?
+        @sa_cv_terms.push({ 'name' => sa[:sa_cv_title], 'has_cv' => false, 'data' => nil,
+                            'allows_custom_input' => nil, 'required' => sa[:required] })
+      else
+        allows_custom_input = SampleControlledVocab.find(sa[:sa_cv_id])&.custom_input
+        sa_terms = SampleControlledVocabTerm.where(sample_controlled_vocab_id: sa[:sa_cv_id]).map(&:label)
+        @sa_cv_terms.push({ 'name' => sa[:sa_cv_title], 'has_cv' => true, 'data' => sa_terms,
+                            'allows_custom_input' => allows_custom_input, 'required' => sa[:required] })
+      end
     end
     @template = Template.find(@sample_type.template_id)
 
@@ -103,13 +104,15 @@ def download_samples_excel
     flash[:error] = e.message
     respond_to do |format|
       format.html { redirect_to single_page_path(@project.id) }
-      format.json { render json: { parameters: { sample_ids: sample_ids, sample_type_id: sample_type_id, study_id: study_id } } }
+      format.json do
+        render json: { parameters: { sample_ids:, sample_type_id:, study_id: } }
+      end
     end
   end
 
   def export_to_excel
     cache_uuid = UUID.new.generate
-    id_label = "#{Seek::Config::instance_name} id"
+    id_label = "#{Seek::Config.instance_name} id"
     sample_ids = JSON.parse(params[:sample_data]).map { |sample| sample[id_label] unless sample[id_label] == '#HIDDEN' }
     sample_type_id = JSON.parse(params[:sample_type_id])
     study_id = JSON.parse(params[:study_id])
@@ -141,8 +144,8 @@ def upload_samples
 
     sample_type_id_ui = params[:sample_type_id].to_i
 
-    unless is_valid_workbook?(wb)
-      raise "Invalid workbook! Cannot process this spreadsheet. Consider first exporting the table as a spreadsheet for the proper format."
+    unless valid_workbook?(wb)
+      raise 'Invalid workbook! Cannot process this spreadsheet. Consider first exporting the table as a spreadsheet for the proper format.'
     end
 
     # Extract Samples metadata from spreadsheet
@@ -150,8 +153,6 @@ def upload_samples
     @study = Study.find(study_id)
     sample_type_id = metadata_sheet.cell(5, 2).value.to_i
     @sample_type = SampleType.find(sample_type_id)
-    template_id = metadata_sheet.cell(8, 2).value.to_i
-    template = Template.find(template_id)
     is_assay = @sample_type.assays.any?
     @assay = @sample_type.assays.first
 
@@ -170,13 +171,7 @@ def upload_samples
       sa_attr.title if sa_attr.sample_attribute_type.base_type == 'SeekSampleMulti'
     end
 
-    sample_fields = samples_sheet.row(1).actual_cells.map { |field| field&.value&.sub(' *', '') }.compact
-    samples_data = (2..samples_sheet.actual_rows.size).map do |i|
-      sample_cells = samples_sheet.row(i).cells
-      sample_cells.map { |cell| cell&.value }.drop(1) unless sample_cells.all? { |cell| (cell&.value == '' || cell&.value.nil?) }
-    end
-
-    samples_data.compact!
+    sample_fields, samples_data = get_spreadsheet_data(samples_sheet)
 
     # Compare Excel header row to Sample Type Sample Attributes
     # Should raise an error if they don't match
@@ -187,11 +182,72 @@ def upload_samples
     end
 
     # Construct Samples objects from Excel data
-    excel_samples = samples_data.map do |excel_sample|
+    excel_samples = generate_excel_samples(samples_data, sample_fields)
+
+    existing_excel_samples = excel_samples.map { |sample| sample unless sample['id'].nil? }.compact
+    new_excel_samples = excel_samples.map { |sample| sample if sample['id'].nil? }.compact
+
+    # Retrieve all samples of the Sample Type, also the unauthorized ones
+    @db_samples = sample_type_samples(@sample_type)
+    # Retrieve the Sample Types samples wich are authorized for editing
+    @authorized_db_samples = sample_type_samples(@sample_type, :edit)
+
+    # Determine whether samples have been modified or not,
+    # and checking whether the user is permitted to edit them
+    @unauthorized_samples, @update_samples = separate_unauthorized_samples(existing_excel_samples, @db_samples,
+                                                                           @authorized_db_samples)
+
+    # Determine if the new samples are no duplicates of existing ones,
+    # based on the attribute values
+    @possible_duplicates, @new_samples = separate_possible_duplicates(new_excel_samples, @db_samples)
+
+    upload_data = { study: @study,
+                    assay: @assay,
+                    sampleType: @sample_type,
+                    excel_samples:,
+                    existingExcelSamples: existing_excel_samples,
+                    newExcelSamples: new_excel_samples,
+                    updateSamples: @update_samples,
+                    newSamples: @new_samples,
+                    possibleDuplicates: @possible_duplicates,
+                    dbSamples: @db_samples,
+                    authorized_db_samples: @authorized_db_samples,
+                    unauthorized_samples: @unauthorized_samples }
+
+    respond_to do |format|
+      format.json { render json: { uploadData: upload_data } }
+      format.html { render 'single_pages/sample_upload_content', { layout: false } }
+    end
+  rescue StandardError => e
+    flash[:error] = e.message
+    respond_to do |format|
+      format.html { redirect_to single_page_path(@project), status: :bad_request }
+      format.json { render json: { error: e }, status: :bad_request }
+    end
+  end
+
+  private
+
+  def get_spreadsheet_data(samples_sheet)
+    sample_fields = samples_sheet.row(1).actual_cells.map { |field| field&.value&.sub(' *', '') }.compact
+    samples_data = (2..samples_sheet.actual_rows.size).map do |i|
+      sample_cells = samples_sheet.row(i).cells
+      next if sample_cells.all? { |cell| (cell&.value == '' || cell&.value.nil?) }
+
+      sample_cells.map do |cell|
+        cell&.value
+      end.drop(1)
+    end.compact
+
+    [sample_fields, samples_data]
+  end
+
+  def generate_excel_samples(samples_data, sample_fields)
+    samples_data.map do |excel_sample|
       obj = {}
       (0..sample_fields.size - 1).map do |i|
         if @multiple_input_fields.include?(sample_fields[i])
-          parsed_excel_input_samples = JSON.parse(excel_sample[i].gsub(/\"=>/x, "\":")).map do |subsample|
+          parsed_excel_input_samples = JSON.parse(excel_sample[i].gsub(/"=>/x, '":')).map do |subsample|
             # Uploader should at least have viewing permissions for the inputs he's using
             unless Sample.find(subsample['id'])&.authorized_for_view?
               raise "Unauthorized Sample was detected in spreadsheet: #{subsample.inspect}"
@@ -212,33 +268,34 @@ def upload_samples
       end
       obj
     end
+  end
 
-    existing_excel_samples = excel_samples.map { |sample| sample unless sample['id'].nil? }.compact
-    new_excel_samples = excel_samples.map { |sample| sample if sample['id'].nil? }.compact
-
-    @db_samples = @sample_type.samples&.map do |sample|
-      attributes = JSON.parse(sample[:json_metadata])
-      { 'id' => sample.id,
-        'uuid' => sample.uuid }.merge(attributes)
-    end
-
-    @authorized_db_samples = @sample_type.samples&.authorized_for(:edit)&.map do |sample|
-      attributes = JSON.parse(sample[:json_metadata])
-      { 'id' => sample.id,
-        'uuid' => sample.uuid }.merge(attributes)
+  def sample_type_samples(sample_type, authorization_method = nil)
+    if authorization_method
+      sample_type.samples&.authorized_for(authorization_method)&.map do |sample|
+        attributes = JSON.parse(sample[:json_metadata])
+        { 'id' => sample.id,
+          'uuid' => sample.uuid }.merge(attributes)
+      end
+    else
+      sample_type.samples&.map do |sample|
+        attributes = JSON.parse(sample[:json_metadata])
+        { 'id' => sample.id,
+          'uuid' => sample.uuid }.merge(attributes)
+      end
     end
+  end
 
-    # Determine whether samples have been modified or not,
-    # and checking whether the user is permitted to edit them
-    @unauthorized_samples = []
-    @update_samples = []
+  def separate_unauthorized_samples(existing_excel_samples, db_samples, authorized_db_samples)
+    update_samples = []
+    unauthorized_samples = []
     existing_excel_samples.map do |ees|
-      db_sample = @db_samples.select { |s| s['id'] == ees['id'] }.first
+      db_sample = db_samples.select { |s| s['id'] == ees['id'] }.first
 
       # An exception is raised if the ID of an existing Sample cannot be found in the DB
       raise "Sample with id '#{ees['id']}' does not exist in the database. Sample upload was aborted!" if db_sample.nil?
 
-      is_authorized_for_update = @authorized_db_samples.select { |s| s['id'] == ees['id'] }.any?
+      is_authorized_for_update = authorized_db_samples.select { |s| s['id'] == ees['id'] }.any?
 
       is_changed = false
 
@@ -251,21 +308,22 @@ def upload_samples
 
       if is_changed
         if is_authorized_for_update
-          @update_samples.append(ees)
+          update_samples.append(ees)
         else
-          @unauthorized_samples.append(ees)
+          unauthorized_samples.append(ees)
         end
       end
     end
+    [unauthorized_samples, update_samples]
+  end
 
-    # Determine if the new samples are no duplicates of existing ones,
-    # based on the attribute values
-    @possible_duplicates = []
-    @new_samples = []
+  def separate_possible_duplicates(new_excel_samples, db_samples)
+    possible_duplicates = []
+    new_samples = []
     new_excel_samples.map do |nes|
       is_duplicate = true
 
-      @db_samples.map do |dbs|
+      db_samples.map do |dbs|
         dbs.map do |k, v|
           unless %w[id uuid].include?(k)
             is_duplicate = (nes[k] == v)
@@ -274,48 +332,24 @@ def upload_samples
         end
 
         if is_duplicate
-          @possible_duplicates.append(nes.merge({ 'duplicate' => dbs }))
+          possible_duplicates.append(nes.merge({ 'duplicate' => dbs }))
           break
         end
       end
 
-      if @db_samples.none?
-        @new_samples.append(nes)
+      if db_samples.none?
+        new_samples.append(nes)
       else
-        @new_samples.append(nes) unless is_duplicate
+        new_samples.append(nes) unless is_duplicate
       end
     end
+    [possible_duplicates, new_samples]
+  end
 
-    upload_data = { study: @study,
-                    assay: @assay,
-                    sampleType: @sample_type,
-                    excel_samples: excel_samples,
-                    existingExcelSamples: existing_excel_samples,
-                    newExcelSamples: new_excel_samples,
-                    updateSamples: @update_samples,
-                    newSamples: @new_samples,
-                    possibleDuplicates: @possible_duplicates,
-                    dbSamples: @db_samples,
-                    authorized_db_samples: @authorized_db_samples,
-                    unauthorized_samples: @unauthorized_samples }
-
-    respond_to do |format|
-      format.json { render json: { uploadData: upload_data } }
-      format.html { render 'single_pages/sample_upload_content', { layout: false } }
-    end
-  rescue StandardError => e
-    flash[:error] = e.message
-    respond_to do |format|
-      format.html { redirect_to single_page_path(@project), status: :bad_request }
-      format.json { render json: { error: e }, status: :bad_request }
-    end
-
-end
-
-  private
-
-  def is_valid_workbook?(wb)
-    !((wb.sheet_names.map { |sheet| %w[Metadata Samples cv_ontology].include? sheet }.include? false) && (wb.sheets.size != 3))
+  def valid_workbook?(workbook)
+    !((workbook.sheet_names.map do |sheet|
+         %w[Metadata Samples cv_ontology].include? sheet
+       end.include? false) && (workbook.sheets.size != 3))
   end
 
   def set_up_instance_variable
@@ -328,8 +362,8 @@ def find_authorized_investigation
   end
 
   def check_user_logged_in
-    unless current_user
-      render json: { status: :unprocessable_entity, error: 'You must be logged in to access batch sharing permission.' }
-    end
+    return if current_user
+
+    render json: { status: :unprocessable_entity, error: 'You must be logged in to access batch sharing permission.' }
   end
 end