From a350caaf7f11b6532ead34e8aa7a693556f263c6 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Jul 2025 13:02:05 +0200 Subject: [PATCH 01/28] Rename to preview_upload_samples --- .../javascripts/single_page/index.js.erb | 2 +- app/controllers/single_pages_controller.rb | 2 +- config/routes.rb | 1 + .../single_pages_controller_test.rb | 44 +++++++++---------- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/single_page/index.js.erb b/app/assets/javascripts/single_page/index.js.erb index 0225c1e949..77d855c464 100644 --- a/app/assets/javascripts/single_page/index.js.erb +++ b/app/assets/javascripts/single_page/index.js.erb @@ -305,7 +305,7 @@ async function batchUpdateSample(sampleTypes) { async function handleUploadSubmit(formData){ $j.ajax({ type: 'POST', - url: "<%= upload_samples_single_pages_path %>", + url: "<%= preview_upload_samples_single_pages_path %>", data: formData, dataType: 'html', processData: false, diff --git a/app/controllers/single_pages_controller.rb b/app/controllers/single_pages_controller.rb index ef75dd94fb..121807657f 100644 --- a/app/controllers/single_pages_controller.rb +++ b/app/controllers/single_pages_controller.rb @@ -118,7 +118,7 @@ def export_to_excel end end - def upload_samples + def preview_upload_samples uploaded_file = params[:file] project_id = params[:project_id] @project = Project.find(project_id) diff --git a/config/routes.rb b/config/routes.rb index 62cef507fc..a04979f0c1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -805,6 +805,7 @@ post :batch_sharing_permission_changed post :export_to_excel, action: :export_to_excel get :download_samples_excel, action: :download_samples_excel + post :preview_upload_samples, action: :preview_upload_samples post :upload_samples, action: :upload_samples end end diff --git a/test/functional/single_pages_controller_test.rb b/test/functional/single_pages_controller_test.rb index e7fdb075e2..5dcbb0c475 100644 --- a/test/functional/single_pages_controller_test.rb +++ b/test/functional/single_pages_controller_test.rb @@ -103,8 +103,8 @@ def setup :project, :source_sample_type ) - post :upload_samples, params: { file:, project_id: project.id, - sample_type_id: source_sample_type.id } + post :preview_upload_samples, params: { file:, project_id: project.id, + sample_type_id: source_sample_type.id } assert_response :bad_request assert_equal flash[:error], "Please upload a valid spreadsheet file with extension '.xlsx'" @@ -120,8 +120,8 @@ def setup file_path = 'upload_single_page/01_combo_update_sources_spreadsheet.xlsx' file = fixture_file_upload(file_path, 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet') - post :upload_samples, as: :json, params: { file:, project_id: project.id, - sample_type_id: sample_collection_sample_type.id } + post :preview_upload_samples, as: :json, params: { file:, project_id: project.id, + sample_type_id: sample_collection_sample_type.id } assert_response :bad_request end @@ -136,8 +136,8 @@ def setup file_path = 'upload_single_page/02_invalid_workbook.xlsx' file = fixture_file_upload(file_path, 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet') - post :upload_samples, as: :json, params: { file:, project_id: project.id, - sample_type_id: source_sample_type.id } + post :preview_upload_samples, as: :json, params: { file:, project_id: project.id, + sample_type_id: source_sample_type.id } assert_response :bad_request end @@ -152,8 +152,8 @@ def setup file_path = 'upload_single_page/01_combo_update_sources_spreadsheet.xlsx' file = fixture_file_upload(file_path, 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet') - post :upload_samples, as: :json, params: { file:, project_id: project.id, - sample_type_id: source_sample_type.id } + post :preview_upload_samples, as: :json, params: { file:, project_id: project.id, + sample_type_id: source_sample_type.id } response_data = JSON.parse(response.body)['uploadData'] db_samples = response_data['dbSamples'] @@ -167,8 +167,8 @@ def setup assert_equal new_samples.size, 2 assert_equal possible_duplicates.size, 1 - post :upload_samples, as: :html, params: { file:, project_id: project.id, - sample_type_id: source_sample_type.id } + post :preview_upload_samples, as: :html, params: { file:, project_id: project.id, + sample_type_id: source_sample_type.id } assert_response :success @@ -207,8 +207,8 @@ def setup file_path = 'upload_single_page/03_combo_update_samples_spreadsheet.xlsx' file = fixture_file_upload(file_path, 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet') - post :upload_samples, as: :json, params: { file:, project_id: project.id, - sample_type_id: sample_collection_sample_type.id } + post :preview_upload_samples, as: :json, params: { file:, project_id: project.id, + sample_type_id: sample_collection_sample_type.id } response_data = JSON.parse(response.body)['uploadData'] updated_samples = response_data['updateSamples'] @@ -220,8 +220,8 @@ def setup assert_equal new_samples.size, 2 assert_equal possible_duplicates.size, 1 - post :upload_samples, as: :html, params: { file:, project_id: project.id, - sample_type_id: sample_collection_sample_type.id } + post :preview_upload_samples, as: :html, params: { file:, project_id: project.id, + sample_type_id: sample_collection_sample_type.id } assert_response :success @@ -260,8 +260,8 @@ def setup file_path = 'upload_single_page/04_combo_update_assay_samples_spreadsheet.xlsx' file = fixture_file_upload(file_path, 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet') - post :upload_samples, as: :json, params: { file:, project_id: project.id, - sample_type_id: assay_sample_type.id } + post :preview_upload_samples, as: :json, params: { file:, project_id: project.id, + sample_type_id: assay_sample_type.id } response_data = JSON.parse(response.body)['uploadData'] updated_samples = response_data['updateSamples'] @@ -273,8 +273,8 @@ def setup assert_equal new_samples.size, 1 assert_equal possible_duplicates.size, 1 - post :upload_samples, as: :html, params: { file:, project_id: project.id, - sample_type_id: assay_sample_type.id } + post :preview_upload_samples, as: :html, params: { file:, project_id: project.id, + sample_type_id: assay_sample_type.id } assert_response :success @@ -315,8 +315,8 @@ def setup file_path = 'upload_single_page/01_combo_update_sources_spreadsheet.xlsx' file = fixture_file_upload(file_path, 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet') - post :upload_samples, as: :json, params: { file:, project_id: project.id, - sample_type_id: source_sample_type.id } + post :preview_upload_samples, as: :json, params: { file:, project_id: project.id, + sample_type_id: source_sample_type.id } response_data = JSON.parse(response.body)['uploadData'] updated_samples = response_data['updateSamples'] @@ -331,8 +331,8 @@ def setup possible_duplicates = response_data['possibleDuplicates'] assert(possible_duplicates.size, 1) - post :upload_samples, as: :html, params: { file:, project_id: project.id, - sample_type_id: source_sample_type.id } + post :preview_upload_samples, as: :html, params: { file:, project_id: project.id, + sample_type_id: source_sample_type.id } assert_response :success From 16959c8db716889e1b372a6c5ca01b12586fe43b Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Jul 2025 17:14:42 +0200 Subject: [PATCH 02/28] Create TaskJobs for updating and creating --- app/jobs/samples_batch_create_job.rb | 14 ++++++++++++++ app/jobs/samples_batch_update_job.rb | 11 +++++++++++ 2 files changed, 25 insertions(+) create mode 100644 app/jobs/samples_batch_create_job.rb create mode 100644 app/jobs/samples_batch_update_job.rb diff --git a/app/jobs/samples_batch_create_job.rb b/app/jobs/samples_batch_create_job.rb new file mode 100644 index 0000000000..dc10e10039 --- /dev/null +++ b/app/jobs/samples_batch_create_job.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class SamplesBatchCreateJob < TaskJob + queue_with_priority 1 + queue_as QueueNames::SAMPLES + + def perform + + end + + def task + arguments[0].samples_batch_create_task + end +end diff --git a/app/jobs/samples_batch_update_job.rb b/app/jobs/samples_batch_update_job.rb new file mode 100644 index 0000000000..221e50490c --- /dev/null +++ b/app/jobs/samples_batch_update_job.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class SamplesBatchUpdateJob < TaskJob + def perform + + end + + def task + arguments[0].samples_batch_update_task + end +end From 7499ffb6ca46b34bc561f572dd77c3ef61116c6e Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Jul 2025 17:16:50 +0200 Subject: [PATCH 03/28] Move `update_sample_with_params` to sharable location --- app/controllers/samples_controller.rb | 10 ++-------- lib/seek/samples/samples_common.rb | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 8 deletions(-) create mode 100644 lib/seek/samples/samples_common.rb diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 254c879f95..33709b30e0 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -1,7 +1,7 @@ class SamplesController < ApplicationController respond_to :html include Seek::PreviewHandling - include Seek::AssetsCommon + include Seek::Samples::SamplesCommon include Seek::IndexPager include Seek::JSONMetadata @@ -304,13 +304,7 @@ def sample_params(sample_type = nil, parameters = params) discussion_links_attributes:[:id, :url, :label, :_destroy]) end - def update_sample_with_params(parameters = params, sample = @sample) - sample.assign_attributes(sample_params(sample.sample_type, parameters)) - update_sharing_policies(sample, parameters) - update_annotations(parameters[:tag_list], sample) - update_relationships(sample, parameters) - sample - end + def find_index_assets if params[:sample_type_id] diff --git a/lib/seek/samples/samples_common.rb b/lib/seek/samples/samples_common.rb new file mode 100644 index 0000000000..81e9f496b1 --- /dev/null +++ b/lib/seek/samples/samples_common.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +module Seek + module Samples + include Seek::AssetsCommon + + module SamplesCommon + def update_sample_with_params(parameters = params, sample = @sample) + sample.assign_attributes(sample_params(sample.sample_type, parameters)) + update_sharing_policies(sample, parameters) + update_annotations(parameters[:tag_list], sample) + update_relationships(sample, parameters) + sample + end + end + end +end From ce2db8553bc5629a939e345ec335632f8f5499f9 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Jul 2025 17:19:56 +0200 Subject: [PATCH 04/28] Add controller method --- app/controllers/single_pages_controller.rb | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/app/controllers/single_pages_controller.rb b/app/controllers/single_pages_controller.rb index 121807657f..b7dae061e3 100644 --- a/app/controllers/single_pages_controller.rb +++ b/app/controllers/single_pages_controller.rb @@ -236,6 +236,30 @@ def preview_upload_samples end end + def upload_samples_by_spreadsheet + errors = [] + results = [] + param_converter = Seek::Api::ParameterConverter.new("samples") + new_samples_params = param_converter.convert(params[:newSamples]) + updated_samples_params = param_converter.convert(params[:updatedSamples]) + # SamplesBatchCreateJob.perform_later() + # SamplesBatchUpdateJob.perform_later() + status = errors.empty? ? :ok : :unprocessable_entity + flash[:notice] = 'A background job has been launched. Samples are being added as we speak!' + rescue StandardError => e + flash[:error] = e.message + ensure + respond_to do |format| + format.html do + redirect_to single_page_path(@project), status: status + end + format.json do + render json: { status: status, errors: errors, results: results }, status: status + end + end + end + + private def get_spreadsheet_data(samples_sheet) From ea893a0358ccb2fcf3218ea1c43a20cdb46da10f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Jul 2025 17:20:23 +0200 Subject: [PATCH 05/28] change route action --- config/routes.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index a04979f0c1..f796be96ca 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -806,7 +806,7 @@ post :export_to_excel, action: :export_to_excel get :download_samples_excel, action: :download_samples_excel post :preview_upload_samples, action: :preview_upload_samples - post :upload_samples, action: :upload_samples + post :upload_samples, action: :upload_samples_by_spreadsheet end end From 03672da2e1d699baad5b9e322282afa2aa386951 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 24 Jul 2025 11:37:42 +0200 Subject: [PATCH 06/28] Move batch create functionality to shared library --- app/controllers/samples_controller.rb | 21 +++------------------ lib/seek/samples/samples_common.rb | 24 ++++++++++++++++++++++-- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 33709b30e0..d22dd90069 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -2,6 +2,8 @@ class SamplesController < ApplicationController respond_to :html include Seek::PreviewHandling include Seek::Samples::SamplesCommon + include Seek::AssetsCommon + include Seek::Publishing::PublishingCommon include Seek::IndexPager include Seek::JSONMetadata @@ -15,7 +17,6 @@ class SamplesController < ApplicationController before_action :auth_to_create, only: %i[new create batch_create] include Seek::ISAGraphExtensions - include Seek::Publishing::PublishingCommon api_actions :index, :show, :create, :update, :destroy, :batch_create @@ -148,23 +149,7 @@ def filter end def batch_create - errors = [] - results = [] - param_converter = Seek::Api::ParameterConverter.new("samples") - Sample.transaction do - params[:data].each do |par| - converted_params = param_converter.convert(par) - sample_type = SampleType.find_by_id(converted_params.dig(:sample, :sample_type_id)) - sample = Sample.new(sample_type: sample_type) - sample = update_sample_with_params(converted_params, sample) - if sample.save - results.push({ ex_id: par[:ex_id], id: sample.id }) - else - errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) - end - end - raise ActiveRecord::Rollback if errors.any? - end + results, errors = batch_create_samples(params).values_at(:results, :errors) status = errors.empty? ? :ok : :unprocessable_entity render json: { status: status, errors: errors, results: results }, status: :ok end diff --git a/lib/seek/samples/samples_common.rb b/lib/seek/samples/samples_common.rb index 81e9f496b1..b422903165 100644 --- a/lib/seek/samples/samples_common.rb +++ b/lib/seek/samples/samples_common.rb @@ -1,16 +1,36 @@ # frozen_string_literal: true module Seek module Samples - include Seek::AssetsCommon module SamplesCommon - def update_sample_with_params(parameters = params, sample = @sample) + def update_sample_with_params(parameters = params, sample) sample.assign_attributes(sample_params(sample.sample_type, parameters)) update_sharing_policies(sample, parameters) update_annotations(parameters[:tag_list], sample) update_relationships(sample, parameters) sample end + + def batch_create_samples(params) + errors = [] + results = [] + param_converter = Seek::Api::ParameterConverter.new("samples") + Sample.transaction do + params[:data].each do |par| + converted_params = param_converter.convert(par) + sample_type = SampleType.find_by_id(converted_params.dig(:sample, :sample_type_id)) + sample = Sample.new(sample_type: sample_type) + sample = update_sample_with_params(converted_params, sample) + if sample.save + results.push({ ex_id: par[:ex_id], id: sample.id }) + else + errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) + end + end + raise ActiveRecord::Rollback if errors.any? + end + { results: results, errors: errors } + end end end end From aacdda880989af16a18b5e9f43a138cd5a1fcfaa Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 24 Jul 2025 11:47:47 +0200 Subject: [PATCH 07/28] Move batch update functionality to the shared library --- app/controllers/samples_controller.rb | 20 ++------------------ lib/seek/samples/samples_common.rb | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index d22dd90069..3ca999110e 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -155,25 +155,9 @@ def batch_create end def batch_update - errors = [] - param_converter = Seek::Api::ParameterConverter.new("samples") - Sample.transaction do - params[:data].each do |par| - begin - converted_params = param_converter.convert(par) - sample = Sample.find(par[:id]) - raise 'shouldnt get this far without manage rights' unless sample.can_manage? - sample = update_sample_with_params(converted_params, sample) - saved = sample.save - errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) unless saved - rescue - errors.push({ ex_id: par[:ex_id], error: "Can not be updated." }) - end - end - raise ActiveRecord::Rollback if errors.any? - end + results, errors = batch_update_samples(params).values_at(:results, :errors) status = errors.empty? ? :ok : :unprocessable_entity - render json: { status: status, errors: errors }, status: :ok + render json: { status: status, errors: errors, results: results }, status: :ok end def batch_delete diff --git a/lib/seek/samples/samples_common.rb b/lib/seek/samples/samples_common.rb index b422903165..68e6a6d3e5 100644 --- a/lib/seek/samples/samples_common.rb +++ b/lib/seek/samples/samples_common.rb @@ -31,6 +31,31 @@ def batch_create_samples(params) end { results: results, errors: errors } end + + def batch_update_samples(params) + errors = [] + results = [] + param_converter = Seek::Api::ParameterConverter.new("samples") + Sample.transaction do + params[:data].each do |par| + begin + converted_params = param_converter.convert(par) + sample = Sample.find(par[:id]) + raise 'shouldn\'t get this far without editing rights' unless sample.can_edit? + sample = update_sample_with_params(converted_params, sample) + if sample.save + results.push({ ex_id: par[:ex_id], id: sample.id }) + else + errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) + end + rescue StandardError => e + errors.push({ ex_id: par[:ex_id], error: "Can not be updated.\n#{e.message}" }) unless saved + end + end + raise ActiveRecord::Rollback if errors.any? + end + { results: results, errors: errors } + end end end end From 19c7f8038fea422f1df77336932763a61bab9fb4 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 25 Jul 2025 10:58:38 +0200 Subject: [PATCH 08/28] Add functionality to controller --- app/controllers/single_pages_controller.rb | 50 ++++++++++++------- app/jobs/samples_batch_create_job.rb | 16 +++++- app/jobs/samples_batch_update_job.rb | 19 ++++++- app/models/mailer.rb | 14 ++++++ app/models/sample_type.rb | 5 +- .../sample_upload_content.html.erb | 18 ++++--- lib/seek/samples/samples_common.rb | 2 +- 7 files changed, 94 insertions(+), 30 deletions(-) diff --git a/app/controllers/single_pages_controller.rb b/app/controllers/single_pages_controller.rb index b7dae061e3..6800183a84 100644 --- a/app/controllers/single_pages_controller.rb +++ b/app/controllers/single_pages_controller.rb @@ -10,7 +10,6 @@ class SinglePagesController < ApplicationController before_action :check_user_logged_in, only: %i[batch_sharing_permission_preview batch_change_permission_for_selected_items] respond_to :html, :js - def show @project = Project.find(params[:id]) @folders = project_folders @@ -237,31 +236,44 @@ def preview_upload_samples end def upload_samples_by_spreadsheet - errors = [] - results = [] - param_converter = Seek::Api::ParameterConverter.new("samples") - new_samples_params = param_converter.convert(params[:newSamples]) - updated_samples_params = param_converter.convert(params[:updatedSamples]) - # SamplesBatchCreateJob.perform_later() - # SamplesBatchUpdateJob.perform_later() - status = errors.empty? ? :ok : :unprocessable_entity - flash[:notice] = 'A background job has been launched. Samples are being added as we speak!' + # new_samples_params = params.require(:newSamples).permit + # updated_samples_params = params.require(:updatedSamples).permit(:updatedSamples) + # sample_type = SampleType.find(params[:sampleTypeId]) + new_samples_params, updated_samples_params, sample_type_id = spreadsheet_upload_params.values_at(:new_samples, :updated_samples, :sample_type_id) + + sample_type = SampleType.find(sample_type_id) + project = sample_type.projects.first + if sample_type.locked? + raise 'Batch upload not permitted. Sample Type is locked! Wait until the lock is removed and try again.' + end + + SamplesBatchCreateJob.perform_later(sample_type, new_samples_params, false, @current_user) + SamplesBatchUpdateJob.perform_later(sample_type, updated_samples_params, false, @current_user) + + result = 'A background job has been launched. Samples are being added as we speak!' + status = :ok + flash[:notice] = result rescue StandardError => e flash[:error] = e.message + result = "An error occurred!\n#{e.message}\n#{e.backtrace.join("\n")}" + status = :bad_request ensure - respond_to do |format| - format.html do - redirect_to single_page_path(@project), status: status - end - format.json do - render json: { status: status, errors: errors, results: results }, status: status - end - end + render json: { result: result, status: status } end - private + def spreadsheet_upload_params + new_sample_params = params.require(:newSamples).permit! + updated_sample_params = params.require(:updatedSamples).permit! + + { + new_samples: new_sample_params, + updated_samples: updated_sample_params, + sample_type_id: params.require(:sampleTypeId), + } + end + 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| diff --git a/app/jobs/samples_batch_create_job.rb b/app/jobs/samples_batch_create_job.rb index dc10e10039..a113ee3b5b 100644 --- a/app/jobs/samples_batch_create_job.rb +++ b/app/jobs/samples_batch_create_job.rb @@ -3,9 +3,23 @@ class SamplesBatchCreateJob < TaskJob queue_with_priority 1 queue_as QueueNames::SAMPLES + include Seek::Samples::SamplesCommon - def perform + def perform(sample_type, new_sample_params, send_email, user) + results, errors = batch_create_samples(new_sample_params).values_at(:results, :errors) + if send_email && Seek::Config::email_enabled && user + project = sample_type.projects.first + if sample_type.assays.empty? + item_type = 'study' + item_id = sample_type.studies.first + else + item_type = 'assay' + item_id = sample_type.assays.first + end + + Mailer.notify_user_after_spreadsheet_extraction(user, project, item_type, item_id, results, errors).deliver_now + end end def task diff --git a/app/jobs/samples_batch_update_job.rb b/app/jobs/samples_batch_update_job.rb index 221e50490c..c05e3cc0d8 100644 --- a/app/jobs/samples_batch_update_job.rb +++ b/app/jobs/samples_batch_update_job.rb @@ -1,8 +1,25 @@ # frozen_string_literal: true class SamplesBatchUpdateJob < TaskJob - def perform + queue_with_priority 1 + queue_as QueueNames::SAMPLES + include Seek::Samples::SamplesCommon + def perform(sample_type, updated_samples_params, send_email, user) + results, errors = batch_update_samples(updated_samples_params).values_at(:results, :errors) + + if send_email && Seek::Config::email_enabled && user + project = sample_type.projects.first + if sample_type.assays.empty? + item_type = 'study' + item_id = sample_type.studies.first + else + item_type = 'assay' + item_id = sample_type.assays.first + end + + Mailer.notify_user_after_spreadsheet_extraction(user, project, item_type, item_id, results, errors).deliver_now + end end def task diff --git a/app/models/mailer.rb b/app/models/mailer.rb index 3f92484ccb..d22bd7d63b 100644 --- a/app/models/mailer.rb +++ b/app/models/mailer.rb @@ -360,6 +360,20 @@ def notify_admins_project_creation_rejected(responder, requester, project_name, subject: subject) end + def notify_user_after_spreadsheet_extraction(user, project, item_type, item_id, results, errors) + @user = user + @project = project + @item_type = item_type + @item_id = item_id + @results = results + @errors = errors + subject = '' + mail(from: Seek::Config.noreply_sender, + to: notifiee.email_with_name, + subject: subject, + template_name: :notify_user_after_spreadsheet_extraction,) + end + private def admin_emails diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 30c5f4a3cb..73370bc3b2 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -65,6 +65,9 @@ class SampleType < ApplicationRecord has_annotation_type :sample_type_tag, method_name: :tags has_task :sample_metadata_update + has_task :samples_batch_create + has_task :samples_batch_update + def investigations return [] if studies.empty? && assays.empty? @@ -113,7 +116,7 @@ def is_isa_json_compliant? end def locked? - sample_metadata_update_task&.in_progress? + [sample_metadata_update_task, samples_batch_create_task, samples_batch_update_task].map(&:in_progress?).any? end def validate_value?(attribute_name, value) diff --git a/app/views/single_pages/sample_upload_content.html.erb b/app/views/single_pages/sample_upload_content.html.erb index b8b1f51dc2..cd6aeab8a9 100644 --- a/app/views/single_pages/sample_upload_content.html.erb +++ b/app/views/single_pages/sample_upload_content.html.erb @@ -84,12 +84,16 @@ $j('#close-upload-xlsx-modal-btn').prop('disabled', true); try{ - const postCall = await uploadAjaxCall("<%= batch_create_samples_path %>", "POST", { data: JSON.stringify(newSamples) }); - const putCall = await uploadAjaxCall("<%= batch_update_samples_path %>", "PUT", { data: JSON.stringify(updatedSamples) }); - let errors = {newSamples: postCall.errors, updatedSamples: putCall.errors}; - if (errors.newSamples.length > 0 || errors.updatedSamples.length > 0) { - throw new Error(JSON.stringify(errors)); - } + const body = { data: JSON.stringify({ + sampleTypeId: <%= @sample_type.id %>, + newSamples: newSamples, + updatedSamples: updatedSamples, + }) + }; + const uploadSamplesCall = await uploadAjaxCall("<%= upload_samples_single_pages_path %>", "POST", body); + if (uploadSamplesCall.status !== 'ok') { + throw new Error(JSON.stringify(uploadSamplesCall.result)); + } $j('#sample-upload-spinner').hide(); closeModalForm(); location.reload(); @@ -222,7 +226,7 @@ }, error: function(err) { console.log("err", err); - alert(json.stringify(err)); + alert(JSON.stringify(err)); } }); }; diff --git a/lib/seek/samples/samples_common.rb b/lib/seek/samples/samples_common.rb index 68e6a6d3e5..5ffa772092 100644 --- a/lib/seek/samples/samples_common.rb +++ b/lib/seek/samples/samples_common.rb @@ -49,7 +49,7 @@ def batch_update_samples(params) errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) end rescue StandardError => e - errors.push({ ex_id: par[:ex_id], error: "Can not be updated.\n#{e.message}" }) unless saved + errors.push({ ex_id: par[:ex_id], error: "Can not be updated.\n#{e.message}" }) end end raise ActiveRecord::Rollback if errors.any? From 97ca2a47e621b336a000fd643547fe3f86b4324a Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 18 Aug 2025 16:02:24 +0200 Subject: [PATCH 09/28] Move back to samples controller --- app/controllers/samples_controller.rb | 12 +++++- app/controllers/single_pages_controller.rb | 37 ------------------- .../sample_upload_content.html.erb | 3 +- lib/seek/samples/samples_common.rb | 36 ++++++++---------- 4 files changed, 27 insertions(+), 61 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 3ca999110e..21c926af4e 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -149,13 +149,13 @@ def filter end def batch_create - results, errors = batch_create_samples(params).values_at(:results, :errors) + results, errors = batch_create_samples(params, @current_user).values_at(:results, :errors) status = errors.empty? ? :ok : :unprocessable_entity render json: { status: status, errors: errors, results: results }, status: :ok end def batch_update - results, errors = batch_update_samples(params).values_at(:results, :errors) + results, errors = batch_update_samples(params, @current_user).values_at(:results, :errors) status = errors.empty? ? :ok : :unprocessable_entity render json: { status: status, errors: errors, results: results }, status: :ok end @@ -252,6 +252,14 @@ def query_form private + def update_sample_with_params(parameters = params, sample) + sample.assign_attributes(sample_params(sample.sample_type, parameters)) + update_sharing_policies(sample, parameters) + update_annotations(parameters[:tag_list], sample) + update_relationships(sample, parameters) + sample + end + def sample_params(sample_type = nil, parameters = params) sample_type_param_keys = [] diff --git a/app/controllers/single_pages_controller.rb b/app/controllers/single_pages_controller.rb index 6800183a84..75f0e22519 100644 --- a/app/controllers/single_pages_controller.rb +++ b/app/controllers/single_pages_controller.rb @@ -235,45 +235,8 @@ def preview_upload_samples end end - def upload_samples_by_spreadsheet - # new_samples_params = params.require(:newSamples).permit - # updated_samples_params = params.require(:updatedSamples).permit(:updatedSamples) - # sample_type = SampleType.find(params[:sampleTypeId]) - new_samples_params, updated_samples_params, sample_type_id = spreadsheet_upload_params.values_at(:new_samples, :updated_samples, :sample_type_id) - - sample_type = SampleType.find(sample_type_id) - project = sample_type.projects.first - if sample_type.locked? - raise 'Batch upload not permitted. Sample Type is locked! Wait until the lock is removed and try again.' - end - - SamplesBatchCreateJob.perform_later(sample_type, new_samples_params, false, @current_user) - SamplesBatchUpdateJob.perform_later(sample_type, updated_samples_params, false, @current_user) - - result = 'A background job has been launched. Samples are being added as we speak!' - status = :ok - flash[:notice] = result - rescue StandardError => e - flash[:error] = e.message - result = "An error occurred!\n#{e.message}\n#{e.backtrace.join("\n")}" - status = :bad_request - ensure - render json: { result: result, status: status } - end - private - def spreadsheet_upload_params - new_sample_params = params.require(:newSamples).permit! - updated_sample_params = params.require(:updatedSamples).permit! - - { - new_samples: new_sample_params, - updated_samples: updated_sample_params, - sample_type_id: params.require(:sampleTypeId), - } - end - 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| diff --git a/app/views/single_pages/sample_upload_content.html.erb b/app/views/single_pages/sample_upload_content.html.erb index cd6aeab8a9..7497e51cbd 100644 --- a/app/views/single_pages/sample_upload_content.html.erb +++ b/app/views/single_pages/sample_upload_content.html.erb @@ -90,13 +90,12 @@ updatedSamples: updatedSamples, }) }; - const uploadSamplesCall = await uploadAjaxCall("<%= upload_samples_single_pages_path %>", "POST", body); + const uploadSamplesCall = await uploadAjaxCall("<%= spreadsheet_upload_samples_path %>", "POST", body); if (uploadSamplesCall.status !== 'ok') { throw new Error(JSON.stringify(uploadSamplesCall.result)); } $j('#sample-upload-spinner').hide(); closeModalForm(); - location.reload(); } catch (error){ alert(`Error: ${error}`); $j('#sample-upload-spinner').hide(); diff --git a/lib/seek/samples/samples_common.rb b/lib/seek/samples/samples_common.rb index 5ffa772092..384a6d4252 100644 --- a/lib/seek/samples/samples_common.rb +++ b/lib/seek/samples/samples_common.rb @@ -1,20 +1,13 @@ # frozen_string_literal: true module Seek - module Samples + module Samples - module SamplesCommon - def update_sample_with_params(parameters = params, sample) - sample.assign_attributes(sample_params(sample.sample_type, parameters)) - update_sharing_policies(sample, parameters) - update_annotations(parameters[:tag_list], sample) - update_relationships(sample, parameters) - sample - end - - def batch_create_samples(params) - errors = [] - results = [] - param_converter = Seek::Api::ParameterConverter.new("samples") + module SamplesCommon + def batch_create_samples(params, user) + errors = [] + results = [] + param_converter = Seek::Api::ParameterConverter.new("samples") + User.with_current_user(user) do Sample.transaction do params[:data].each do |par| converted_params = param_converter.convert(par) @@ -29,13 +22,15 @@ def batch_create_samples(params) end raise ActiveRecord::Rollback if errors.any? end - { results: results, errors: errors } end + { results: results, errors: errors } + end - def batch_update_samples(params) - errors = [] - results = [] - param_converter = Seek::Api::ParameterConverter.new("samples") + def batch_update_samples(params, user) + errors = [] + results = [] + param_converter = Seek::Api::ParameterConverter.new("samples") + User.with_current_user(user) do Sample.transaction do params[:data].each do |par| begin @@ -54,8 +49,9 @@ def batch_update_samples(params) end raise ActiveRecord::Rollback if errors.any? end - { results: results, errors: errors } end + { results: results, errors: errors } end end + end end From 79aefac4a751db958b795c7c43f4cc2d4f107c0a Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:12:18 +0200 Subject: [PATCH 10/28] Fix params --- app/controllers/samples_controller.rb | 42 ++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 21c926af4e..d62a904e3d 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -168,7 +168,7 @@ def batch_delete sample = Sample.find(par[:id]) errors.push({ ex_id: par[:ex_id], error: "Can not be deleted." }) if !(sample.can_delete? && sample.destroy) rescue - errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) + errors.push({ ex_id: par[:ex_id], error: sample&.errors&.messages }) end end raise ActiveRecord::Rollback if errors.any? @@ -252,12 +252,33 @@ def query_form private - def update_sample_with_params(parameters = params, sample) - sample.assign_attributes(sample_params(sample.sample_type, parameters)) - update_sharing_policies(sample, parameters) - update_annotations(parameters[:tag_list], sample) - update_relationships(sample, parameters) - sample + def spreadsheet_upload_params(parameters = params) + sample_type_id = parameters.permit(:sampleTypeId).dig(:sampleTypeId) + sample_type = SampleType.find(sample_type_id) + + param_converter = Seek::Api::ParameterConverter.new("samples") + + converted_new_samples_params = [] + if parameters.key?(:newSamples) + raw_new_samples_params = parameters.fetch(:newSamples, {}).dig(:data) + unless raw_new_samples_params.blank? + raw_new_samples_params.each do |par| + converted_new_samples_params << sample_params(sample_type, param_converter.convert(par)) + end + end + end + + converted_updated_samples_params = [] + if parameters.key?(:updatedSamples) + raw_updated_samples_params = parameters.fetch(:updatedSamples, {}).dig(:data) + unless raw_updated_samples_params.blank? + raw_updated_samples_params.each do |par| + converted_updated_samples_params << sample_params(sample_type, param_converter.convert(par)) + end + end + end + + { sample_type_id: sample_type_id, new_sample_params: converted_new_samples_params, updated_sample_params: converted_updated_samples_params } end def sample_params(sample_type = nil, parameters = params) @@ -282,6 +303,13 @@ def sample_params(sample_type = nil, parameters = params) end + def update_sample_with_params(parameters = params, sample) + sample.assign_attributes(sample_params(sample.sample_type, parameters)) + update_sharing_policies(sample, parameters) + update_annotations(parameters[:tag_list], sample) + update_relationships(sample, parameters) + sample + end def find_index_assets if params[:sample_type_id] From 43626d2ecf7d7748767eadc3f838dff6b7c0337b Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:13:37 +0200 Subject: [PATCH 11/28] Combine jobs --- app/jobs/samples_batch_create_job.rb | 28 ---------------------------- app/jobs/samples_batch_update_job.rb | 28 ---------------------------- app/jobs/samples_batch_upload_job.rb | 16 ++++++++++++++++ app/models/sample_type.rb | 7 ++++++- 4 files changed, 22 insertions(+), 57 deletions(-) delete mode 100644 app/jobs/samples_batch_create_job.rb delete mode 100644 app/jobs/samples_batch_update_job.rb create mode 100644 app/jobs/samples_batch_upload_job.rb diff --git a/app/jobs/samples_batch_create_job.rb b/app/jobs/samples_batch_create_job.rb deleted file mode 100644 index a113ee3b5b..0000000000 --- a/app/jobs/samples_batch_create_job.rb +++ /dev/null @@ -1,28 +0,0 @@ -# frozen_string_literal: true - -class SamplesBatchCreateJob < TaskJob - queue_with_priority 1 - queue_as QueueNames::SAMPLES - include Seek::Samples::SamplesCommon - - def perform(sample_type, new_sample_params, send_email, user) - results, errors = batch_create_samples(new_sample_params).values_at(:results, :errors) - - if send_email && Seek::Config::email_enabled && user - project = sample_type.projects.first - if sample_type.assays.empty? - item_type = 'study' - item_id = sample_type.studies.first - else - item_type = 'assay' - item_id = sample_type.assays.first - end - - Mailer.notify_user_after_spreadsheet_extraction(user, project, item_type, item_id, results, errors).deliver_now - end - end - - def task - arguments[0].samples_batch_create_task - end -end diff --git a/app/jobs/samples_batch_update_job.rb b/app/jobs/samples_batch_update_job.rb deleted file mode 100644 index c05e3cc0d8..0000000000 --- a/app/jobs/samples_batch_update_job.rb +++ /dev/null @@ -1,28 +0,0 @@ -# frozen_string_literal: true - -class SamplesBatchUpdateJob < TaskJob - queue_with_priority 1 - queue_as QueueNames::SAMPLES - include Seek::Samples::SamplesCommon - - def perform(sample_type, updated_samples_params, send_email, user) - results, errors = batch_update_samples(updated_samples_params).values_at(:results, :errors) - - if send_email && Seek::Config::email_enabled && user - project = sample_type.projects.first - if sample_type.assays.empty? - item_type = 'study' - item_id = sample_type.studies.first - else - item_type = 'assay' - item_id = sample_type.assays.first - end - - Mailer.notify_user_after_spreadsheet_extraction(user, project, item_type, item_id, results, errors).deliver_now - end - end - - def task - arguments[0].samples_batch_update_task - end -end diff --git a/app/jobs/samples_batch_upload_job.rb b/app/jobs/samples_batch_upload_job.rb new file mode 100644 index 0000000000..175c475313 --- /dev/null +++ b/app/jobs/samples_batch_upload_job.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class SamplesBatchUploadJob < TaskJob + queue_with_priority 1 + queue_as QueueNames::SAMPLES + + def perform(sample_type_id, new_sample_params, updated_sample_params, user, send_email=false) + processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params:, updated_sample_params:, user:, send_email:) + processor.process! + end + + def task + sample_type = SampleType.find(arguments[0]) + sample_type.sample_batch_upload_task + end +end diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 73370bc3b2..df40dfe05b 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -67,6 +67,7 @@ class SampleType < ApplicationRecord has_task :sample_metadata_update has_task :samples_batch_create has_task :samples_batch_update + has_task :sample_batch_upload def investigations return [] if studies.empty? && assays.empty? @@ -116,7 +117,11 @@ def is_isa_json_compliant? end def locked? - [sample_metadata_update_task, samples_batch_create_task, samples_batch_update_task].map(&:in_progress?).any? + sample_metadata_update_task.in_progress? + end + + def batch_upload_in_progess? + sample_batch_upload_task.in_progress? end def validate_value?(attribute_name, value) From 7a4d5389316b5544a8b99f20d4fb637f577538bc Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:14:45 +0200 Subject: [PATCH 12/28] Add controller method --- app/controllers/samples_controller.rb | 39 +++++++++++++++++++++++++++ config/routes.rb | 1 + 2 files changed, 40 insertions(+) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index d62a904e3d..c10079deae 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -148,6 +148,45 @@ def filter end end + def upload_samples_by_spreadsheet + new_samples_params, updated_samples_params, sample_type_id = spreadsheet_upload_params(params).values_at(:new_sample_params, :updated_sample_params, :sample_type_id) + + sample_type = SampleType.find(sample_type_id) + + raise "Sample Type with ID '#{sample_type_id}' not found." if sample_type.nil? + + if sample_type.locked? + raise 'Batch upload not allowed. Sample Type is currently locked! Wait until the lock is removed and try again.' + end + + if sample_type.batch_upload_in_progess? + raise 'Batch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later.' + end + + unless sample_type.can_view? + raise 'Batch upload not allowed. You need at least viewing permission to the sample type!' + end + + total_transactions = new_samples_params.count + updated_samples_params.count + if total_transactions <100 + SamplesBatchUploadJob.perform_now(sample_type_id, new_samples_params, updated_samples_params, @current_user) + result = 'Samples successfully created.' + else + SamplesBatchUploadJob.perform_later(sample_type_id, new_samples_params, updated_samples_params, @current_user) + result = 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' + end + + status = :ok + flash[:notice] = result + rescue StandardError => e + flash[:error] = e.message + result = "An error occurred!\n#{e.message}\n#{e.backtrace.join("\n")}" + status = :bad_request + ensure + render json: { result: result, status: status }, status: status + end + + def batch_create results, errors = batch_create_samples(params, @current_user).values_at(:results, :errors) status = errors.empty? ? :ok : :unprocessable_entity diff --git a/config/routes.rb b/config/routes.rb index f796be96ca..0e551a422e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -713,6 +713,7 @@ delete :batch_delete get :query_form post :query + post :spreadsheet_upload, action: :upload_samples_by_spreadsheet end resources :people, :programmes, :projects, :assays, :studies, :investigations, :data_files, :sops, :publications, :samples, :sample_types, :strains, :organisms, :collections, From d82dfca43ad053c00bd733bd4ab56fafce5428e8 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:15:50 +0200 Subject: [PATCH 13/28] Create SampleBatchProcessor service --- .../samples/sample_batch_processor.rb | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 app/services/samples/sample_batch_processor.rb diff --git a/app/services/samples/sample_batch_processor.rb b/app/services/samples/sample_batch_processor.rb new file mode 100644 index 0000000000..2450c5b6bf --- /dev/null +++ b/app/services/samples/sample_batch_processor.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true +module Samples + class SampleBatchProcessor + + def initialize(sample_type_id:, new_sample_params:, updated_sample_params:, user:, send_email:) + @sample_type = SampleType.find(sample_type_id) + @projects = @sample_type.projects + @new_sample_params = new_sample_params + @updated_sample_params = updated_sample_params + @user = user + @send_email = send_email + @results = [] + @errors = [] + + validate! + end + + def process! + create_samples + update_samples + send_email if @send_email && Seek::Config::email_enabled && !@user.nil? + end + + private + + def validate! + raise "Missing sample_type_id or sample type not found" if @sample_type.nil? + raise "No projects associated with this Sample Type" if @projects.empty? + raise "Missing new_sample_params" if @new_sample_params.nil? + raise "Missing updated_sample_params" if @updated_sample_params.nil? + raise "Missing user" if @user.nil? + end + + def create_samples + User.with_current_user(@user) do + Sample.transaction do + @new_sample_params.each do |par| + sample = Sample.new(sample_type: @sample_type) + sample.assign_attributes(par) + if sample.save + result_message = "Sample '#{sample.title}' successfully created." + @results << result_message + Rails.logger.info result_message + else + error_message = "Sample could be created. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." + @errors << error_message + Rails.logger.error error_message + raise ActiveRecord::Rollback + end + end + end + end + end + + def update_samples + User.with_current_user(@user) do + Sample.transaction do + @updated_sample_params.each do |par| + sample_id = par[:id] + sample = Sample.find(sample_id) + raise "Not permitted to update sample" if sample.can_edit?(@user) + sample.assign_attributes(par) + unless sample.save + raise ActiveRecord::Rollback + end + end + end + end + end + + def send_email + if @sample_type.assays.empty? && @sample_type.studies.any? + item_type = 'study' + item_id = @sample_type.studies.first + elsif @sample_type.assays.any? &&@sample_type.studies.empty? + item_type = 'assay' + item_id = @sample_type.assays.first + else + item_type = 'sample_type' + item_id = @sample_type.id + end + + Mailer.notify_user_after_spreadsheet_extraction(@user, @projects, item_type, item_id, results, errors).deliver_now + end + end +end From ee61abd718ef44653face7deaf42806be387b843 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:16:17 +0200 Subject: [PATCH 14/28] Write new samples test through spreadsheet upload --- test/functional/samples_controller_test.rb | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index 5d5cfca6d4..4689fc4034 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1837,12 +1837,67 @@ def rdf_test_object assert_equal flash[:error], 'This sample type is locked. You cannot edit the sample.' end + test 'upload new samples by spreadsheet' do + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + + sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) + # refute sample_type.locked? + new_samples_params = new_samples_spreadsheet_upload_params(5, sample_type.id, project.id) + params = { sampleTypeId: sample_type.id, + newSamples: { data: new_samples_params }, + updatedSamples: { data: [] }} + + assert_difference('Sample.count', 5) do + post :upload_samples_by_spreadsheet, params: params + end + response_body = JSON.parse(response.body) + assert_response :success + assert_equal response_body['result'], 'Samples successfully created.' + + end + def rdf_test_object FactoryBot.create(:max_sample, policy: FactoryBot.create(:public_policy)) end private + def new_samples_spreadsheet_upload_params(n, sample_type_id, project_id) + params = [] + (0..n-1).each do |i| + params << { sampleTypeId: sample_type_id, + ex_id: "new-#{i}-#{sample_type_id}", + data: { + type: "samples", + attributes: { + attribute_map: { + the_title: "Mouse #{i}" + }, + policy: { + access: "edit", + permissions: [ + { + resource: { + type: "Project", + id: project_id + }, + access: "no_access" + } + ] + } + }, + relationships: { + projects: { data: [{ type: "projects", id: project_id }] }, + sample_type: { data: { type: "sample_types", id: sample_type_id } } + } + } + } + end + params + end + def populated_patient_sample person = FactoryBot.create(:person) sample = Sample.new title: 'My Sample', policy: FactoryBot.create(:public_policy), From 9cabed1456cb6a2675927a79ded61cd333453957 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:16:54 +0200 Subject: [PATCH 15/28] Add mailer for spreadsheet extraction --- app/models/mailer.rb | 8 ++++++-- .../notify_user_after_spreadsheet_extraction.erb | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 app/views/mailer/notify_user_after_spreadsheet_extraction.erb diff --git a/app/models/mailer.rb b/app/models/mailer.rb index d22bd7d63b..4f7109692e 100644 --- a/app/models/mailer.rb +++ b/app/models/mailer.rb @@ -367,9 +367,13 @@ def notify_user_after_spreadsheet_extraction(user, project, item_type, item_id, @item_id = item_id @results = results @errors = errors - subject = '' + subject = if errors.empty? + "Spreadsheet upload completed successfully" + else + "spreadsheet upload Failed" + end mail(from: Seek::Config.noreply_sender, - to: notifiee.email_with_name, + to: user.email_with_name, subject: subject, template_name: :notify_user_after_spreadsheet_extraction,) end diff --git a/app/views/mailer/notify_user_after_spreadsheet_extraction.erb b/app/views/mailer/notify_user_after_spreadsheet_extraction.erb new file mode 100644 index 0000000000..d8d154fbb7 --- /dev/null +++ b/app/views/mailer/notify_user_after_spreadsheet_extraction.erb @@ -0,0 +1,14 @@ +Hi <%= @notifiee.person.name %>, + +<% if @errors.empty? %> + Your spreadsheet extraction has been successfully completed. + Here are the results: + <% @results.each do |res| %> +
  • <%= res %>
    • + <% end %> +<% else %> + Your spreadsheet extraction encountered one or more errors. Please correct them and try again. + <% @errors.each do |err| %> +
  • <%= err %>
    • + <% end %> +<% end %> From 6a2358941a68775a60a965ba7bdb299ef1e6fd9e Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:17:11 +0200 Subject: [PATCH 16/28] formatting --- .../sample_upload_content.html.erb | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/app/views/single_pages/sample_upload_content.html.erb b/app/views/single_pages/sample_upload_content.html.erb index 7497e51cbd..e6da0baafe 100644 --- a/app/views/single_pages/sample_upload_content.html.erb +++ b/app/views/single_pages/sample_upload_content.html.erb @@ -158,7 +158,7 @@ attrMap["sampleUploadAction"] = action; - if (action === "update"){ + if (action === "update") { return { id: objId, ex_id: `update-${index}-<%= @sample_type.id %>`, @@ -170,38 +170,38 @@ } }; } else { - return { - ex_id: `new-${index}-<%= @sample_type.id %>`, - data:{ - type: "samples", - tags: null, - attributes: { - policy: { - access: getAccess(projectDefaultPolicy), - permissions: [ - { - resource: { type: "Project", id: <%= @project.id %> }, - access: getPermission(projectDefaultPolicy, <%= @project.id %>) - } - ] - }, - attribute_map: attrMap - }, - relationships: { - projects: { - data: [ - {type: "projects", id: <%= @project.id %>} - ] - }, - sample_type: { - data: { - type: "sample_types", - id: <%= @sample_type.id %> - } + return { + ex_id: `new-${index}-<%= @sample_type.id %>`, + data: { + type: "samples", + tags: null, + attributes: { + policy: { + access: getAccess(projectDefaultPolicy), + permissions: [ + { + resource: {type: "Project", id: <%= @project.id %>}, + access: getPermission(projectDefaultPolicy, <%= @project.id %>) } + ] + }, + attribute_map: attrMap + }, + relationships: { + projects: { + data: [ + {type: "projects", id: <%= @project.id %>} + ] + }, + sample_type: { + data: { + type: "sample_types", + id: <%= @sample_type.id %> } } - }; + } + } + }; } } From 53421812622e646d69f80029340996940195dbc6 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 09:29:57 +0200 Subject: [PATCH 17/28] set sample default back --- app/controllers/samples_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index c10079deae..290e03ffb0 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -342,7 +342,7 @@ def sample_params(sample_type = nil, parameters = params) end - def update_sample_with_params(parameters = params, sample) + def update_sample_with_params(parameters = params, sample = @sample) sample.assign_attributes(sample_params(sample.sample_type, parameters)) update_sharing_policies(sample, parameters) update_annotations(parameters[:tag_list], sample) From e7605657d1591ed8ebd26509d8833f818c47637b Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 12:57:54 +0200 Subject: [PATCH 18/28] Fix typo + raise error in controller An error in the controller should be raised if the SampleBatchProcessor has errors. --- app/controllers/samples_controller.rb | 15 +++++++++------ app/jobs/samples_batch_upload_job.rb | 2 +- app/models/sample_type.rb | 2 +- app/services/samples/sample_batch_processor.rb | 17 +++++++++++++---- test/functional/samples_controller_test.rb | 16 ---------------- 5 files changed, 24 insertions(+), 28 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 290e03ffb0..b1189db59a 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -149,7 +149,7 @@ def filter end def upload_samples_by_spreadsheet - new_samples_params, updated_samples_params, sample_type_id = spreadsheet_upload_params(params).values_at(:new_sample_params, :updated_sample_params, :sample_type_id) + new_sample_params, updated_sample_params, sample_type_id = spreadsheet_upload_params(params).values_at(:new_sample_params, :updated_sample_params, :sample_type_id) sample_type = SampleType.find(sample_type_id) @@ -159,7 +159,7 @@ def upload_samples_by_spreadsheet raise 'Batch upload not allowed. Sample Type is currently locked! Wait until the lock is removed and try again.' end - if sample_type.batch_upload_in_progess? + if sample_type.batch_upload_in_progress? raise 'Batch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later.' end @@ -167,12 +167,15 @@ def upload_samples_by_spreadsheet raise 'Batch upload not allowed. You need at least viewing permission to the sample type!' end - total_transactions = new_samples_params.count + updated_samples_params.count + total_transactions = new_sample_params.count + updated_sample_params.count if total_transactions <100 - SamplesBatchUploadJob.perform_now(sample_type_id, new_samples_params, updated_samples_params, @current_user) + processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params:, updated_sample_params:, user: @current_user) + processor.process! + raise "The following errors occurred: #{processor.errors.join("\n")}" unless processor.errors.empty? + result = 'Samples successfully created.' else - SamplesBatchUploadJob.perform_later(sample_type_id, new_samples_params, updated_samples_params, @current_user) + SamplesBatchUploadJob.perform_later(sample_type_id, new_sample_params, updated_sample_params, @current_user, true) result = 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' end @@ -180,7 +183,7 @@ def upload_samples_by_spreadsheet flash[:notice] = result rescue StandardError => e flash[:error] = e.message - result = "An error occurred!\n#{e.message}\n#{e.backtrace.join("\n")}" + result = "One or more errors occurred:\n#{e.message}\n#{e.backtrace.join("\n")}" status = :bad_request ensure render json: { result: result, status: status }, status: status diff --git a/app/jobs/samples_batch_upload_job.rb b/app/jobs/samples_batch_upload_job.rb index 175c475313..2752ad59d5 100644 --- a/app/jobs/samples_batch_upload_job.rb +++ b/app/jobs/samples_batch_upload_job.rb @@ -4,7 +4,7 @@ class SamplesBatchUploadJob < TaskJob queue_with_priority 1 queue_as QueueNames::SAMPLES - def perform(sample_type_id, new_sample_params, updated_sample_params, user, send_email=false) + def perform(sample_type_id, new_sample_params, updated_sample_params, user, send_email) processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params:, updated_sample_params:, user:, send_email:) processor.process! end diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index df40dfe05b..b56562dd12 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -120,7 +120,7 @@ def locked? sample_metadata_update_task.in_progress? end - def batch_upload_in_progess? + def batch_upload_in_progress? sample_batch_upload_task.in_progress? end diff --git a/app/services/samples/sample_batch_processor.rb b/app/services/samples/sample_batch_processor.rb index 2450c5b6bf..d3e0a6ab78 100644 --- a/app/services/samples/sample_batch_processor.rb +++ b/app/services/samples/sample_batch_processor.rb @@ -2,7 +2,8 @@ module Samples class SampleBatchProcessor - def initialize(sample_type_id:, new_sample_params:, updated_sample_params:, user:, send_email:) + attr_reader :errors + def initialize(sample_type_id:, new_sample_params:, updated_sample_params:, user:, send_email: false) @sample_type = SampleType.find(sample_type_id) @projects = @sample_type.projects @new_sample_params = new_sample_params @@ -35,7 +36,7 @@ def create_samples User.with_current_user(@user) do Sample.transaction do @new_sample_params.each do |par| - sample = Sample.new(sample_type: @sample_type) + sample = Sample.new(sample_type: @sample_type, policy: @sample_type.policy, projects: @projects) sample.assign_attributes(par) if sample.save result_message = "Sample '#{sample.title}' successfully created." @@ -58,9 +59,17 @@ def update_samples @updated_sample_params.each do |par| sample_id = par[:id] sample = Sample.find(sample_id) - raise "Not permitted to update sample" if sample.can_edit?(@user) + raise "Sample with id '#{sample_id}' not found." if sample.nil? + raise "Not permitted to update sample." if sample.can_edit?(@user) sample.assign_attributes(par) - unless sample.save + if sample.save + result_message = "Sample '[ID: #{sample.id}] #{sample.title}' successfully updated." + @results << result_message + Rails.logger.info result_message + else + error_message = "Sample could be created. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." + @errors << error_message + Rails.logger.error error_message raise ActiveRecord::Rollback end end diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index 4689fc4034..dfbd595c7b 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1874,23 +1874,7 @@ def new_samples_spreadsheet_upload_params(n, sample_type_id, project_id) attributes: { attribute_map: { the_title: "Mouse #{i}" - }, - policy: { - access: "edit", - permissions: [ - { - resource: { - type: "Project", - id: project_id - }, - access: "no_access" - } - ] } - }, - relationships: { - projects: { data: [{ type: "projects", id: project_id }] }, - sample_type: { data: { type: "sample_types", id: sample_type_id } } } } } From c3ebfa69befc861a5f2401b1011e175915568989 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 13:14:22 +0200 Subject: [PATCH 19/28] Add background job test --- test/functional/samples_controller_test.rb | 26 ++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index dfbd595c7b..c2764fbafb 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1843,8 +1843,7 @@ def rdf_test_object login_as(person) sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) - # refute sample_type.locked? - new_samples_params = new_samples_spreadsheet_upload_params(5, sample_type.id, project.id) + new_samples_params = new_samples_spreadsheet_upload_params(5, sample_type.id) params = { sampleTypeId: sample_type.id, newSamples: { data: new_samples_params }, updatedSamples: { data: [] }} @@ -1855,7 +1854,25 @@ def rdf_test_object response_body = JSON.parse(response.body) assert_response :success assert_equal response_body['result'], 'Samples successfully created.' + end + test 'upload new samples by spreadsheet as background job' do + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + + sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) + new_samples_params = new_samples_spreadsheet_upload_params(101, sample_type.id) + params = { sampleTypeId: sample_type.id, + newSamples: { data: new_samples_params }, + updatedSamples: { data: [] }} + assert_enqueued_jobs(1, only: SamplesBatchUploadJob) do + post :upload_samples_by_spreadsheet, params: params + end + response_body = JSON.parse(response.body) + assert_response :success + assert_equal response_body['result'], 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' + assert sample_type.batch_upload_in_progress? end def rdf_test_object @@ -1864,10 +1881,11 @@ def rdf_test_object private - def new_samples_spreadsheet_upload_params(n, sample_type_id, project_id) + def new_samples_spreadsheet_upload_params(n, sample_type_id) params = [] (0..n-1).each do |i| - params << { sampleTypeId: sample_type_id, + params << { + sampleTypeId: sample_type_id, ex_id: "new-#{i}-#{sample_type_id}", data: { type: "samples", From 052cf4eb99904b4ad1cd058e081cb1db168f26dc Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 22 Aug 2025 14:23:31 +0200 Subject: [PATCH 20/28] Add batch update tests --- app/controllers/samples_controller.rb | 5 +- .../samples/sample_batch_processor.rb | 2 +- test/functional/samples_controller_test.rb | 81 ++++++++++++++++++- 3 files changed, 84 insertions(+), 4 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index b1189db59a..f27e3b5c13 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -173,7 +173,7 @@ def upload_samples_by_spreadsheet processor.process! raise "The following errors occurred: #{processor.errors.join("\n")}" unless processor.errors.empty? - result = 'Samples successfully created.' + result = 'Samples uploaded successfully!' else SamplesBatchUploadJob.perform_later(sample_type_id, new_sample_params, updated_sample_params, @current_user, true) result = 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' @@ -315,7 +315,8 @@ def spreadsheet_upload_params(parameters = params) raw_updated_samples_params = parameters.fetch(:updatedSamples, {}).dig(:data) unless raw_updated_samples_params.blank? raw_updated_samples_params.each do |par| - converted_updated_samples_params << sample_params(sample_type, param_converter.convert(par)) + sample_id = par[:id] + converted_updated_samples_params << sample_params(sample_type, param_converter.convert(par)).merge(id: sample_id) end end end diff --git a/app/services/samples/sample_batch_processor.rb b/app/services/samples/sample_batch_processor.rb index d3e0a6ab78..e24f0fc922 100644 --- a/app/services/samples/sample_batch_processor.rb +++ b/app/services/samples/sample_batch_processor.rb @@ -60,7 +60,7 @@ def update_samples sample_id = par[:id] sample = Sample.find(sample_id) raise "Sample with id '#{sample_id}' not found." if sample.nil? - raise "Not permitted to update sample." if sample.can_edit?(@user) + raise "Not permitted to update sample." unless sample.can_edit?(@user) sample.assign_attributes(par) if sample.save result_message = "Sample '[ID: #{sample.id}] #{sample.title}' successfully updated." diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index c2764fbafb..97c22a31d9 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1853,7 +1853,7 @@ def rdf_test_object end response_body = JSON.parse(response.body) assert_response :success - assert_equal response_body['result'], 'Samples successfully created.' + assert_equal response_body['result'], 'Samples uploaded successfully!' end test 'upload new samples by spreadsheet as background job' do @@ -1875,6 +1875,66 @@ def rdf_test_object assert sample_type.batch_upload_in_progress? end + test ' upload updated samples by spreadsheet' do + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + + sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) + samples = [] + (1..5).each do |i| + samples << Sample.create(sample_type: sample_type, + data: { + "the_title": "Zebra fish #{i}", + }, + project_ids: [person.projects.first.id], + policy: sample_type.policy, + contributor: person) + end + sample_type.reload + assert samples.none? { |s| s.title.downcase.include? 'updated' } + update_samples_params = update_samples_spreadsheet_upload_params(samples) + params = { sampleTypeId: sample_type.id, + newSamples: { data: [] }, + updatedSamples: { data: update_samples_params }} + post :upload_samples_by_spreadsheet, params: params + response_body = JSON.parse(response.body) + assert_response :success + assert_equal response_body['result'], 'Samples uploaded successfully!' + samples.map(&:reload) + assert samples.all? { |s| s.title.downcase.include? 'updated' } + end + + test ' upload updated samples by spreadsheet as a background job' do + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + + sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) + samples = [] + (1..101).each do |i| + samples << Sample.create(sample_type: sample_type, + data: { + "the_title": "Zebra fish #{i}", + }, + project_ids: [person.projects.first.id], + policy: sample_type.policy, + contributor: person) + end + sample_type.reload + assert samples.none? { |s| s.title.downcase.include? 'updated' } + update_samples_params = update_samples_spreadsheet_upload_params(samples) + params = { sampleTypeId: sample_type.id, + newSamples: { data: [] }, + updatedSamples: { data: update_samples_params }} + assert_enqueued_jobs(1, only: SamplesBatchUploadJob) do + post :upload_samples_by_spreadsheet, params: params + end + response_body = JSON.parse(response.body) + assert_response :success + assert_equal response_body['result'], 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' + end + def rdf_test_object FactoryBot.create(:max_sample, policy: FactoryBot.create(:public_policy)) end @@ -1900,6 +1960,25 @@ def new_samples_spreadsheet_upload_params(n, sample_type_id) params end + def update_samples_spreadsheet_upload_params(samples) + params = [] + samples.each do |sample| + params << { + "ex_id": "update-#{sample.id}-#{sample.sample_type.id}", + "id": "#{ sample.id }", + "data": { + "type": "samples", + "attributes": { + "attribute_map": { + "the_title": "#{sample.title} - Updated" + } + } + } + } + end + params + end + def populated_patient_sample person = FactoryBot.create(:person) sample = Sample.new title: 'My Sample', policy: FactoryBot.create(:public_policy), From cd768f709b6b1939ac6d79b64ada3ad470fa2a6c Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 25 Aug 2025 11:29:45 +0200 Subject: [PATCH 21/28] Remove stack trace --- app/controllers/samples_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index f27e3b5c13..340c4715ac 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -183,7 +183,7 @@ def upload_samples_by_spreadsheet flash[:notice] = result rescue StandardError => e flash[:error] = e.message - result = "One or more errors occurred:\n#{e.message}\n#{e.backtrace.join("\n")}" + result = "One or more errors occurred:\n#{e.message}" status = :bad_request ensure render json: { result: result, status: status }, status: status From b8638cff8b87203c3273b707e3b77e83cc21442f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 25 Aug 2025 11:40:42 +0200 Subject: [PATCH 22/28] Add test Checks whether an exception is raised when samples are uploaded to a sample type with a running background job. --- test/functional/samples_controller_test.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index 97c22a31d9..baceb4a3fb 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1933,6 +1933,23 @@ def rdf_test_object response_body = JSON.parse(response.body) assert_response :success assert_equal response_body['result'], 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' + + # Should not be able to samples through a spreadsheet when a background job is running + new_samples_params = new_samples_spreadsheet_upload_params(5, sample_type.id) + update_samples_params = update_samples_spreadsheet_upload_params(samples.first(5)) + + params = { + sampleTypeId: sample_type.id, + newSamples: { data: new_samples_params }, + updatedSamples: { data: update_samples_params } + } + + assert_no_difference('Sample.count') do + post :upload_samples_by_spreadsheet, params: params + end + response_body = JSON.parse(response.body) + assert_response :bad_request + assert_equal response_body['result'], "One or more errors occurred:\nBatch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later." end def rdf_test_object From a7a630cc4d3ee41e153d90f70b03921de8a9e977 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 25 Aug 2025 15:29:50 +0200 Subject: [PATCH 23/28] Use structs from index.js.erb --- .../sample_upload_content.html.erb | 63 ++++--------------- 1 file changed, 13 insertions(+), 50 deletions(-) diff --git a/app/views/single_pages/sample_upload_content.html.erb b/app/views/single_pages/sample_upload_content.html.erb index e6da0baafe..efb787edd3 100644 --- a/app/views/single_pages/sample_upload_content.html.erb +++ b/app/views/single_pages/sample_upload_content.html.erb @@ -39,7 +39,7 @@ <% end %> - <% if (@can_upload and !any_samples_to_upload?) %> + <% if @can_upload && !any_samples_to_upload? %>
    - <%= render :partial=>"isa_assays/assay_samples", locals: { assay: assay} -%> + <% if assay_sample_type_table_is_readonly %> +
    +

    This table is currently locked because of a background job in progress.

    +

    Please try again later!

    +
    + <% end %> +
    > + <%= render :partial=>"isa_assays/assay_samples", locals: { assay: assay} -%> +
    <%= render :partial=>"isa_assays/assay_table", locals: { assay: assay} -%> diff --git a/app/views/isa_studies/_study_design.html.erb b/app/views/isa_studies/_study_design.html.erb index 2d954f4b92..8979b820ab 100644 --- a/app/views/isa_studies/_study_design.html.erb +++ b/app/views/isa_studies/_study_design.html.erb @@ -5,6 +5,8 @@ study_protocol_action = displaying_single_page? ? "highlightTreeviewItem('study_protocol')" : "loadDynamicTableFromDefaultView('study_protocol')" study_samples_table_action = displaying_single_page? ? "highlightTreeviewItem('study_samples_table')" : "loadDynamicTableFromDefaultView('study_samples_table')" study_experiment_overview_action = displaying_single_page? ? "highlightTreeviewItem('study_experiment_overview')" : "loadDynamicTableFromDefaultView('study_experiment_overview')" + study_source_table_is_readonly = [:batch_upload_in_progress?, :locked?].any? { |method| study&.sample_types&.first&.send(method) } + study_sample_table_is_readonly = [:batch_upload_in_progress?, :locked?].any? { |method| study&.sample_types&.second&.send(method) } %> <% if valid_study %> @@ -26,13 +28,29 @@
    - <%= render :partial=>"isa_studies/source_material", locals: { study: study} -%> + <% if study_source_table_is_readonly %> +
    +

    This table is currently locked because of a background job in progress.

    +

    Please try again later!

    +
    + <% end %> +
    > + <%= render :partial=>"isa_studies/source_material", locals: { study: study} -%> +
    <%= render :partial=>"isa_studies/sop", locals: { sops: study&.sops} -%>
    - <%= render :partial=>"isa_studies/study_samples", locals: { study: study} -%> + <% if study_sample_table_is_readonly %> +
    +

    This table is currently locked because of a background job in progress.

    +

    Please try again later!

    +
    + <% end %> +
    > + <%= render :partial=>"isa_studies/study_samples", locals: { study: study} -%> +
    <%= render :partial=>"isa_studies/study_table", locals: { study: study} -%> @@ -50,7 +68,7 @@ From ef5f53c4eb10a2fa7407b3870a34c97e8013d1b6 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 27 Aug 2025 08:36:17 +0200 Subject: [PATCH 25/28] Harmonize with batch_create and batch_upload --- app/controllers/samples_controller.rb | 150 +++++++++++------- app/jobs/samples_batch_create_job.rb | 11 ++ ...oad_job.rb => samples_batch_update_job.rb} | 8 +- .../samples/sample_batch_processor.rb | 52 ++++-- config/routes.rb | 1 - lib/seek/samples/samples_common.rb | 57 ------- test/functional/samples_controller_test.rb | 129 ++++++++------- 7 files changed, 204 insertions(+), 204 deletions(-) create mode 100644 app/jobs/samples_batch_create_job.rb rename app/jobs/{samples_batch_upload_job.rb => samples_batch_update_job.rb} (55%) delete mode 100644 lib/seek/samples/samples_common.rb diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 340c4715ac..bd108df813 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -1,7 +1,6 @@ class SamplesController < ApplicationController respond_to :html include Seek::PreviewHandling - include Seek::Samples::SamplesCommon include Seek::AssetsCommon include Seek::Publishing::PublishingCommon include Seek::IndexPager @@ -148,58 +147,100 @@ def filter end end - def upload_samples_by_spreadsheet - new_sample_params, updated_sample_params, sample_type_id = spreadsheet_upload_params(params).values_at(:new_sample_params, :updated_sample_params, :sample_type_id) - + def batch_create + errors = [] + results = [] + sample_type_id = params.permit(:sample_type_id).dig(:sample_type_id) + parameters = batch_upload_sample_params(params, sample_type_id, false) sample_type = SampleType.find(sample_type_id) - raise "Sample Type with ID '#{sample_type_id}' not found." if sample_type.nil? + if sample_type.nil? + err_message = "Sample Type with ID '#{sample_type_id}' not found." + errors.push err_message + raise err_message + end if sample_type.locked? - raise 'Batch upload not allowed. Sample Type is currently locked! Wait until the lock is removed and try again.' + err_message = 'Batch upload not allowed. Sample Type is currently locked! Wait until the lock is removed and try again.' + errors.push err_message + raise err_message end - if sample_type.batch_upload_in_progress? - raise 'Batch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later.' + if parameters.count < 100 + batch_create_processor = Samples::SampleBatchProcessor.new(sample_type_id: sample_type_id, + new_sample_params: parameters, + updated_sample_params: [], + user: @current_user) + batch_create_processor.create! + results.concat(batch_create_processor.results) + errors.concat(batch_create_processor.errors) + raise "The following errors occurred: #{errors.join("\n")}" unless errors.empty? + else + SamplesBatchCreateJob.perform_later(sample_type_id, parameters, @current_user, true) + results = ['A background job has been launched.'] end + status = :ok + rescue StandardError => e + flash[:error] = e.message + status = :unprocessable_entity + ensure + render json: { + errors: errors, + results: results, + status: status + }, + status: status + end - unless sample_type.can_view? - raise 'Batch upload not allowed. You need at least viewing permission to the sample type!' + def batch_update + errors = [] + results = [] + sample_type_id = params.permit(:sample_type_id).dig(:sample_type_id) + parameters = batch_upload_sample_params(params, sample_type_id, true) + sample_type = SampleType.find(sample_type_id) + + if sample_type.nil? + err_message = "Sample Type with ID '#{sample_type_id}' not found." + errors.push err_message + raise err_message end - total_transactions = new_sample_params.count + updated_sample_params.count - if total_transactions <100 - processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params:, updated_sample_params:, user: @current_user) - processor.process! - raise "The following errors occurred: #{processor.errors.join("\n")}" unless processor.errors.empty? + if sample_type.locked? + err_message = 'Batch upload not allowed. Sample Type is currently locked! Wait until the lock is removed and try again.' + errors.push err_message + raise err_message + end - result = 'Samples uploaded successfully!' - else - SamplesBatchUploadJob.perform_later(sample_type_id, new_sample_params, updated_sample_params, @current_user, true) - result = 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' + if sample_type.batch_upload_in_progress? + err_message = 'Batch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later.' + errors.push err_message + raise err_message end + if parameters.count < 100 + batch_update_processor = Samples::SampleBatchProcessor.new(sample_type_id: sample_type_id, + new_sample_params: [], + updated_sample_params: parameters, + user: @current_user) + batch_update_processor.update! + errors.concat(batch_update_processor.errors) + results.concat(batch_update_processor.results) + raise "The following errors occurred: #{errors.join("\n")}" unless errors.empty? + else + SamplesBatchUpdateJob.perform_later(sample_type_id, [], parameters, @current_user, true) + results = ['A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.'] + end status = :ok - flash[:notice] = result rescue StandardError => e flash[:error] = e.message - result = "One or more errors occurred:\n#{e.message}" - status = :bad_request + status = :unprocessable_entity ensure - render json: { result: result, status: status }, status: status - end - - - def batch_create - results, errors = batch_create_samples(params, @current_user).values_at(:results, :errors) - status = errors.empty? ? :ok : :unprocessable_entity - render json: { status: status, errors: errors, results: results }, status: :ok - end - - def batch_update - results, errors = batch_update_samples(params, @current_user).values_at(:results, :errors) - status = errors.empty? ? :ok : :unprocessable_entity - render json: { status: status, errors: errors, results: results }, status: :ok + render json: { + errors: errors, + results: results, + status: status + }, + status: status end def batch_delete @@ -294,34 +335,21 @@ def query_form private - def spreadsheet_upload_params(parameters = params) - sample_type_id = parameters.permit(:sampleTypeId).dig(:sampleTypeId) - sample_type = SampleType.find(sample_type_id) - + def batch_upload_sample_params(parameters = params, sample_type_id, upload) param_converter = Seek::Api::ParameterConverter.new("samples") - - converted_new_samples_params = [] - if parameters.key?(:newSamples) - raw_new_samples_params = parameters.fetch(:newSamples, {}).dig(:data) - unless raw_new_samples_params.blank? - raw_new_samples_params.each do |par| - converted_new_samples_params << sample_params(sample_type, param_converter.convert(par)) - end + batch_params = [] + parameters[:data].each do |par| + converted_params = param_converter.convert(par) + ex_id = par[:ex_id] + sample_type = SampleType.find_by_id(sample_type_id) + conv_par = sample_params(sample_type, converted_params).merge(ex_id: ex_id) + if upload + sample_id = par[:id] + conv_par.merge!(id: sample_id) end + batch_params << conv_par end - - converted_updated_samples_params = [] - if parameters.key?(:updatedSamples) - raw_updated_samples_params = parameters.fetch(:updatedSamples, {}).dig(:data) - unless raw_updated_samples_params.blank? - raw_updated_samples_params.each do |par| - sample_id = par[:id] - converted_updated_samples_params << sample_params(sample_type, param_converter.convert(par)).merge(id: sample_id) - end - end - end - - { sample_type_id: sample_type_id, new_sample_params: converted_new_samples_params, updated_sample_params: converted_updated_samples_params } + batch_params end def sample_params(sample_type = nil, parameters = params) diff --git a/app/jobs/samples_batch_create_job.rb b/app/jobs/samples_batch_create_job.rb new file mode 100644 index 0000000000..ce1f344495 --- /dev/null +++ b/app/jobs/samples_batch_create_job.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class SamplesBatchCreateJob < ApplicationJob + queue_with_priority 1 + queue_as QueueNames::SAMPLES + + def perform(sample_type_id, parameters, user, send_email) + processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params: parameters, updated_sample_params: [], user:, send_email:) + processor.create! + end +end diff --git a/app/jobs/samples_batch_upload_job.rb b/app/jobs/samples_batch_update_job.rb similarity index 55% rename from app/jobs/samples_batch_upload_job.rb rename to app/jobs/samples_batch_update_job.rb index 2752ad59d5..95edf3ae05 100644 --- a/app/jobs/samples_batch_upload_job.rb +++ b/app/jobs/samples_batch_update_job.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true -class SamplesBatchUploadJob < TaskJob +class SamplesBatchUpdateJob < TaskJob queue_with_priority 1 queue_as QueueNames::SAMPLES - def perform(sample_type_id, new_sample_params, updated_sample_params, user, send_email) - processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params:, updated_sample_params:, user:, send_email:) - processor.process! + def perform(sample_type_id, parameters, user, send_email) + processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params: [], updated_sample_params: parameters, user:, send_email:) + processor.update! end def task diff --git a/app/services/samples/sample_batch_processor.rb b/app/services/samples/sample_batch_processor.rb index e24f0fc922..129357cbc4 100644 --- a/app/services/samples/sample_batch_processor.rb +++ b/app/services/samples/sample_batch_processor.rb @@ -3,7 +3,8 @@ module Samples class SampleBatchProcessor attr_reader :errors - def initialize(sample_type_id:, new_sample_params:, updated_sample_params:, user:, send_email: false) + attr_reader :results + def initialize(sample_type_id:, new_sample_params: [], updated_sample_params: [], user:, send_email: false) @sample_type = SampleType.find(sample_type_id) @projects = @sample_type.projects @new_sample_params = new_sample_params @@ -22,6 +23,16 @@ def process! send_email if @send_email && Seek::Config::email_enabled && !@user.nil? end + def create! + create_samples + send_email if @send_email && Seek::Config::email_enabled && !@user.nil? + end + + def update! + update_samples + send_email if @send_email && Seek::Config::email_enabled && !@user.nil? + end + private def validate! @@ -36,16 +47,17 @@ def create_samples User.with_current_user(@user) do Sample.transaction do @new_sample_params.each do |par| + ex_id = par.delete(:ex_id) sample = Sample.new(sample_type: @sample_type, policy: @sample_type.policy, projects: @projects) sample.assign_attributes(par) if sample.save - result_message = "Sample '#{sample.title}' successfully created." - @results << result_message - Rails.logger.info result_message + result = { ex_id: ex_id, message: "Sample '#{sample.title}' successfully created." } + @results << result + Rails.logger.info result else - error_message = "Sample could be created. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." - @errors << error_message - Rails.logger.error error_message + error = { ex_id: ex_id, message: "Sample '#{sample.title}' could not be created. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." } + @errors << error + Rails.logger.error error raise ActiveRecord::Rollback end end @@ -57,19 +69,29 @@ def update_samples User.with_current_user(@user) do Sample.transaction do @updated_sample_params.each do |par| + ex_id = par.delete(:ex_id) sample_id = par[:id] sample = Sample.find(sample_id) - raise "Sample with id '#{sample_id}' not found." if sample.nil? - raise "Not permitted to update sample." unless sample.can_edit?(@user) + + if sample.nil? + @errors << "Sample with id '#{sample_id}' not found." + next + end + + unless sample.can_edit?(@user) + @errors << "Not permitted to update this sample." + next + end + sample.assign_attributes(par) if sample.save - result_message = "Sample '[ID: #{sample.id}] #{sample.title}' successfully updated." - @results << result_message - Rails.logger.info result_message + result = { ex_id: ex_id, message: "Sample '[ID: #{sample.id}] #{sample.title}' successfully updated." } + @results << result + Rails.logger.info result else - error_message = "Sample could be created. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." - @errors << error_message - Rails.logger.error error_message + error = { ex_id: ex_id, message: "Sample '[ID: #{sample.id}] #{sample.title}' could not be updated. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." } + @errors << error + Rails.logger.error error raise ActiveRecord::Rollback end end diff --git a/config/routes.rb b/config/routes.rb index 0e551a422e..f796be96ca 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -713,7 +713,6 @@ delete :batch_delete get :query_form post :query - post :spreadsheet_upload, action: :upload_samples_by_spreadsheet end resources :people, :programmes, :projects, :assays, :studies, :investigations, :data_files, :sops, :publications, :samples, :sample_types, :strains, :organisms, :collections, diff --git a/lib/seek/samples/samples_common.rb b/lib/seek/samples/samples_common.rb deleted file mode 100644 index 384a6d4252..0000000000 --- a/lib/seek/samples/samples_common.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true -module Seek - module Samples - - module SamplesCommon - def batch_create_samples(params, user) - errors = [] - results = [] - param_converter = Seek::Api::ParameterConverter.new("samples") - User.with_current_user(user) do - Sample.transaction do - params[:data].each do |par| - converted_params = param_converter.convert(par) - sample_type = SampleType.find_by_id(converted_params.dig(:sample, :sample_type_id)) - sample = Sample.new(sample_type: sample_type) - sample = update_sample_with_params(converted_params, sample) - if sample.save - results.push({ ex_id: par[:ex_id], id: sample.id }) - else - errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) - end - end - raise ActiveRecord::Rollback if errors.any? - end - end - { results: results, errors: errors } - end - - def batch_update_samples(params, user) - errors = [] - results = [] - param_converter = Seek::Api::ParameterConverter.new("samples") - User.with_current_user(user) do - Sample.transaction do - params[:data].each do |par| - begin - converted_params = param_converter.convert(par) - sample = Sample.find(par[:id]) - raise 'shouldn\'t get this far without editing rights' unless sample.can_edit? - sample = update_sample_with_params(converted_params, sample) - if sample.save - results.push({ ex_id: par[:ex_id], id: sample.id }) - else - errors.push({ ex_id: par[:ex_id], error: sample.errors.messages }) - end - rescue StandardError => e - errors.push({ ex_id: par[:ex_id], error: "Can not be updated.\n#{e.message}" }) - end - end - raise ActiveRecord::Rollback if errors.any? - end - end - { results: results, errors: errors } - end - end - end -end diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index baceb4a3fb..e4f3287d57 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1071,31 +1071,31 @@ def rdf_test_object test 'batch_create' do person = FactoryBot.create(:person) login_as(person) - type = FactoryBot.create(:patient_sample_type) + type = FactoryBot.create(:patient_sample_type, contributor: person) assay = FactoryBot.create(:assay, contributor: person) assert_difference('Sample.count', 2) do assert_difference('AssayAsset.count', 1) do - post :batch_create, params: { data: [ - { ex_id: "1", data: { type: "samples", - attributes: { attribute_map: { "full name": 'Fred Smith', "age": '22', - "weight": '22.1', "postcode": 'M13 9PL' } }, - relationships: { assays: { data: [{ id: assay.id, type: 'assays' }] }, - projects: { data: [{ id: person.projects.first.id, - type: "projects" }] }, - sample_type: { data: { id: type.id, type: "sample_types" } } } } }, - { ex_id: "2", data: { type: "samples", - attributes: { attribute_map: { "full name": 'David Tailor', "age": '33', - "weight": '33.1', "postcode": 'M12 8PL' } }, - relationships: { projects: { data: [{ id: person.projects.first.id, type: "projects" }] }, - sample_type: { data: { id: type.id, - type: "sample_types" } } } } }] } + post :batch_create, params: { data: [ + { ex_id: "1", data: { type: "samples", + attributes: { attribute_map: { "full name": 'Fred Smith', "age": '22', + "weight": '22.1', "postcode": 'M13 9PL' } }, + relationships: { assays: { data: [{ id: assay.id, type: 'assays' }] }, + projects: { data: [{ id: person.projects.first.id, + type: "projects" }] }, + sample_type: { data: { id: type.id, type: "sample_types" } } } } }, + { ex_id: "2", data: { type: "samples", + attributes: { attribute_map: { "full name": 'David Tailor', "age": '33', + "weight": '33.1', "postcode": 'M12 8PL' } }, + relationships: { projects: { data: [{ id: person.projects.first.id, type: "projects" }] }, + sample_type: { data: { id: type.id, + type: "sample_types" } } } } }], + sample_type_id: type.id } end end # For the Single Page to work properly, these must be included in the response assert response.body.include?('results') assert response.body.include?('errors') - assert response.body.include?('status') samples = Sample.last(2) sample1 = samples.first @@ -1116,9 +1116,8 @@ def rdf_test_object test 'terminate batch_create if error' do person = FactoryBot.create(:person) - creator = FactoryBot.create(:person) + type = FactoryBot.create(:patient_sample_type, contributor: person, projects: [person.projects.first]) login_as(person) - type = FactoryBot.create(:patient_sample_type) assert_difference('Sample.count', 0) do post :batch_create, params: {data: [ {ex_id: "1",data:{type: "samples", attributes:{attribute_map:{"full name": 'Fred Smith', "age": '22', "weight": '22.1' ,"postcode": 'M13 9PL'}}, @@ -1126,7 +1125,8 @@ def rdf_test_object sample_type: { data:{id: type.id, type: "sample_types"}}}}}, {ex_id: "2", data:{type: "samples",attributes:{attribute_map:{"wrong attribute": 'David Tailor', "age": '33', "weight": '33.1' ,"postcode": 'M12 8PL'}}, tags: nil,relationships:{projects: {data:[{id: person.projects.first.id, type: "projects"}]}, - sample_type: {data:{id: type.id, type: "sample_types"}}}}}]} + sample_type: {data:{id: type.id, type: "sample_types"}}}}}], + sample_type_id: type.id } end json_response = JSON.parse(response.body) @@ -1136,12 +1136,13 @@ def rdf_test_object test 'batch_update' do - login_as(FactoryBot.create(:person)) creator = FactoryBot.create(:person) - sample1 = populated_patient_sample - sample2 = populated_patient_sample - type_id1 = sample1.sample_type.id - type_id2 = sample2.sample_type.id + login_as(creator) + type = FactoryBot.create(:patient_sample_type, contributor: creator) + sample1 = FactoryBot.create(:patient_sample, sample_type: type, contributor: creator) + sample2 = FactoryBot.create(:patient_sample, sample_type: type, contributor: creator) + assert sample1.can_edit?(creator) + assert sample2.can_edit?(creator) assert_empty sample1.creators assert_no_difference('Sample.count') do @@ -1153,26 +1154,25 @@ def rdf_test_object {id: sample2.id, data: {type: "samples", attributes: { attribute_map: { "full name": 'David Tailor', "age": '33', "weight": '33.1' }, - creator_ids: [creator.id]}}}]} + creator_ids: [creator.id]}}}], + sample_type_id: type.id} assert_equal [creator], sample1.creators end # For the Single Page to work properly, these must be included in the response assert response.body.include?('errors') - assert response.body.include?('status') - samples = Sample.limit(2) - first_updated_sample = samples[0] - assert_equal type_id1, first_updated_sample.sample_type.id + first_updated_sample = Sample.find(sample1.id) + assert_equal type.id, first_updated_sample.sample_type_id assert_equal 'Alfred Marcus', first_updated_sample.title assert_equal 'Alfred Marcus', first_updated_sample.get_attribute_value('full name') assert_equal 22, first_updated_sample.get_attribute_value(:age) assert_nil first_updated_sample.get_attribute_value(:postcode) assert_equal 22.1, first_updated_sample.get_attribute_value(:weight) - last_updated_sample = samples[1] - assert_equal type_id2, last_updated_sample.sample_type.id + last_updated_sample = Sample.find(sample2.id) + assert_equal type.id, last_updated_sample.sample_type.id assert_equal 'David Tailor', last_updated_sample.title assert_equal 'David Tailor', last_updated_sample.get_attribute_value('full name') assert_equal 33, last_updated_sample.get_attribute_value(:age) @@ -1844,16 +1844,18 @@ def rdf_test_object sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) new_samples_params = new_samples_spreadsheet_upload_params(5, sample_type.id) - params = { sampleTypeId: sample_type.id, - newSamples: { data: new_samples_params }, - updatedSamples: { data: [] }} + params = { sample_type_id: sample_type.id, + data: new_samples_params } assert_difference('Sample.count', 5) do - post :upload_samples_by_spreadsheet, params: params + post :batch_create, params: params end + response_body = JSON.parse(response.body) assert_response :success - assert_equal response_body['result'], 'Samples uploaded successfully!' + sample_type.reload.samples.all? do |sample| + response_body['results'].include? "Sample '#{sample.title}' successfully created." + end end test 'upload new samples by spreadsheet as background job' do @@ -1862,17 +1864,17 @@ def rdf_test_object login_as(person) sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) - new_samples_params = new_samples_spreadsheet_upload_params(101, sample_type.id) - params = { sampleTypeId: sample_type.id, - newSamples: { data: new_samples_params }, - updatedSamples: { data: [] }} - assert_enqueued_jobs(1, only: SamplesBatchUploadJob) do - post :upload_samples_by_spreadsheet, params: params + new_samples_params = new_samples_spreadsheet_upload_params(100, sample_type.id) + params = { sample_type_id: sample_type.id, + data: new_samples_params } + + assert_enqueued_jobs(1, only: SamplesBatchCreateJob) do + post :batch_create, params: params end + response_body = JSON.parse(response.body) assert_response :success - assert_equal response_body['result'], 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' - assert sample_type.batch_upload_in_progress? + assert response_body['results'].include?('A background job has been launched.') end test ' upload updated samples by spreadsheet' do @@ -1894,15 +1896,14 @@ def rdf_test_object sample_type.reload assert samples.none? { |s| s.title.downcase.include? 'updated' } update_samples_params = update_samples_spreadsheet_upload_params(samples) - params = { sampleTypeId: sample_type.id, - newSamples: { data: [] }, - updatedSamples: { data: update_samples_params }} - post :upload_samples_by_spreadsheet, params: params + params = { sample_type_id: sample_type.id, + data: update_samples_params } + post :batch_update, params: params response_body = JSON.parse(response.body) assert_response :success - assert_equal response_body['result'], 'Samples uploaded successfully!' - samples.map(&:reload) - assert samples.all? { |s| s.title.downcase.include? 'updated' } + assert sample_type.reload.samples.all? do |sample| + response_body['results'].include?("Sample '[ID: #{sample.id}] #{sample.title}' successfully updated.") + end end test ' upload updated samples by spreadsheet as a background job' do @@ -1912,7 +1913,7 @@ def rdf_test_object sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) samples = [] - (1..101).each do |i| + (1..100).each do |i| samples << Sample.create(sample_type: sample_type, data: { "the_title": "Zebra fish #{i}", @@ -1924,32 +1925,29 @@ def rdf_test_object sample_type.reload assert samples.none? { |s| s.title.downcase.include? 'updated' } update_samples_params = update_samples_spreadsheet_upload_params(samples) - params = { sampleTypeId: sample_type.id, - newSamples: { data: [] }, - updatedSamples: { data: update_samples_params }} - assert_enqueued_jobs(1, only: SamplesBatchUploadJob) do - post :upload_samples_by_spreadsheet, params: params + params = { sample_type_id: sample_type.id, + data: update_samples_params } + assert_enqueued_jobs(1, only: SamplesBatchUpdateJob) do + post :batch_update, params: params end response_body = JSON.parse(response.body) assert_response :success - assert_equal response_body['result'], 'A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.' + assert_equal response_body['results'], ['A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.'] # Should not be able to samples through a spreadsheet when a background job is running - new_samples_params = new_samples_spreadsheet_upload_params(5, sample_type.id) update_samples_params = update_samples_spreadsheet_upload_params(samples.first(5)) params = { - sampleTypeId: sample_type.id, - newSamples: { data: new_samples_params }, - updatedSamples: { data: update_samples_params } + sample_type_id: sample_type.id, + data: update_samples_params } assert_no_difference('Sample.count') do - post :upload_samples_by_spreadsheet, params: params + post :batch_update, params: params end response_body = JSON.parse(response.body) - assert_response :bad_request - assert_equal response_body['result'], "One or more errors occurred:\nBatch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later." + assert_response :unprocessable_entity + assert response_body['errors'].include?("Batch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later.") end def rdf_test_object @@ -1962,7 +1960,6 @@ def new_samples_spreadsheet_upload_params(n, sample_type_id) params = [] (0..n-1).each do |i| params << { - sampleTypeId: sample_type_id, ex_id: "new-#{i}-#{sample_type_id}", data: { type: "samples", From 3c3bdd552191c98465b54017d89ed458f2c316de Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 27 Aug 2025 11:10:12 +0200 Subject: [PATCH 26/28] add batch_delete to service object --- app/controllers/samples_controller.rb | 80 ++++++++++++++----- app/jobs/samples_batch_create_job.rb | 2 +- app/jobs/samples_batch_update_job.rb | 2 +- .../samples/sample_batch_processor.rb | 55 ++++++++++--- test/functional/samples_controller_test.rb | 18 ++--- 5 files changed, 113 insertions(+), 44 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index bd108df813..f154c278d3 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -151,7 +151,7 @@ def batch_create errors = [] results = [] sample_type_id = params.permit(:sample_type_id).dig(:sample_type_id) - parameters = batch_upload_sample_params(params, sample_type_id, false) + parameters = batch_upload_sample_params(params, sample_type_id) sample_type = SampleType.find(sample_type_id) if sample_type.nil? @@ -168,9 +168,8 @@ def batch_create if parameters.count < 100 batch_create_processor = Samples::SampleBatchProcessor.new(sample_type_id: sample_type_id, - new_sample_params: parameters, - updated_sample_params: [], - user: @current_user) + batch_process_params: parameters, + user: @current_user) batch_create_processor.create! results.concat(batch_create_processor.results) errors.concat(batch_create_processor.errors) @@ -196,7 +195,7 @@ def batch_update errors = [] results = [] sample_type_id = params.permit(:sample_type_id).dig(:sample_type_id) - parameters = batch_upload_sample_params(params, sample_type_id, true) + parameters = batch_upload_sample_params(params, sample_type_id) sample_type = SampleType.find(sample_type_id) if sample_type.nil? @@ -219,8 +218,7 @@ def batch_update if parameters.count < 100 batch_update_processor = Samples::SampleBatchProcessor.new(sample_type_id: sample_type_id, - new_sample_params: [], - updated_sample_params: parameters, + batch_process_params: parameters, user: @current_user) batch_update_processor.update! errors.concat(batch_update_processor.errors) @@ -245,19 +243,48 @@ def batch_update def batch_delete errors = [] - Sample.transaction do - params[:data].each do |par| - begin - sample = Sample.find(par[:id]) - errors.push({ ex_id: par[:ex_id], error: "Can not be deleted." }) if !(sample.can_delete? && sample.destroy) - rescue - errors.push({ ex_id: par[:ex_id], error: sample&.errors&.messages }) - end - end - raise ActiveRecord::Rollback if errors.any? + results = [] + sample_type_id = params.permit(:sample_type_id).dig(:sample_type_id) + parameters = batch_delete_sample_params + sample_type = SampleType.find(sample_type_id) + + if sample_type.nil? + err_message = "Sample Type with ID '#{sample_type_id}' not found." + errors.push err_message + raise err_message + end + + if sample_type.locked? + err_message = 'Batch upload not allowed. Sample Type is currently locked! Wait until the lock is removed and try again.' + errors.push err_message + raise err_message + end + + if sample_type.batch_upload_in_progress? + err_message = 'Batch upload not allowed. There is already a background job in progress for this Sample Type. Please wait and try again later.' + errors.push err_message + raise err_message end - status = errors.empty? ? :ok : :unprocessable_entity - render json: { status: status, errors: errors }, status: :ok + + batch_update_processor = Samples::SampleBatchProcessor.new(sample_type_id: sample_type_id, + batch_process_params: parameters, + user: @current_user) + batch_update_processor.delete! + errors.concat(batch_update_processor.errors) + results.concat(batch_update_processor.results) + raise "The following errors occurred: #{errors.join("\n")}" unless errors.empty? + + status = :ok + rescue StandardError => e + flash[:error] = e.message + status = :unprocessable_entity + ensure + render json: { + errors: errors, + results: results, + status: status + }, + status: status end def typeahead @@ -335,15 +362,23 @@ def query_form private - def batch_upload_sample_params(parameters = params, sample_type_id, upload) + def batch_delete_sample_params(parameters = params) + batch_params = [] + parameters[:data].each do |par| + batch_params << par.permit(:id, :ex_id) + end + batch_params + end + + def batch_upload_sample_params(parameters = params, sample_type_id) param_converter = Seek::Api::ParameterConverter.new("samples") batch_params = [] parameters[:data].each do |par| converted_params = param_converter.convert(par) ex_id = par[:ex_id] sample_type = SampleType.find_by_id(sample_type_id) - conv_par = sample_params(sample_type, converted_params).merge(ex_id: ex_id) - if upload + conv_par = sample_params(sample_type, converted_params).merge(ex_id: ex_id) unless ex_id.blank? + if parameters[:action] == 'batch_update' sample_id = par[:id] conv_par.merge!(id: sample_id) end @@ -422,6 +457,7 @@ def filter_linked_samples(samples, link, options, template_attribute) end end end + def templates_enabled? unless Seek::Config.isa_json_compliance_enabled flash[:error] = 'Not available' diff --git a/app/jobs/samples_batch_create_job.rb b/app/jobs/samples_batch_create_job.rb index ce1f344495..21ddbbc2c3 100644 --- a/app/jobs/samples_batch_create_job.rb +++ b/app/jobs/samples_batch_create_job.rb @@ -5,7 +5,7 @@ class SamplesBatchCreateJob < ApplicationJob queue_as QueueNames::SAMPLES def perform(sample_type_id, parameters, user, send_email) - processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params: parameters, updated_sample_params: [], user:, send_email:) + processor = Samples::SampleBatchProcessor.new(sample_type_id:, batch_process_params: parameters, user:, send_email:) processor.create! end end diff --git a/app/jobs/samples_batch_update_job.rb b/app/jobs/samples_batch_update_job.rb index 95edf3ae05..3b6f445a93 100644 --- a/app/jobs/samples_batch_update_job.rb +++ b/app/jobs/samples_batch_update_job.rb @@ -5,7 +5,7 @@ class SamplesBatchUpdateJob < TaskJob queue_as QueueNames::SAMPLES def perform(sample_type_id, parameters, user, send_email) - processor = Samples::SampleBatchProcessor.new(sample_type_id:, new_sample_params: [], updated_sample_params: parameters, user:, send_email:) + processor = Samples::SampleBatchProcessor.new(sample_type_id:, batch_process_params: parameters, user:, send_email:) processor.update! end diff --git a/app/services/samples/sample_batch_processor.rb b/app/services/samples/sample_batch_processor.rb index 129357cbc4..7fe8bb40e1 100644 --- a/app/services/samples/sample_batch_processor.rb +++ b/app/services/samples/sample_batch_processor.rb @@ -4,11 +4,10 @@ class SampleBatchProcessor attr_reader :errors attr_reader :results - def initialize(sample_type_id:, new_sample_params: [], updated_sample_params: [], user:, send_email: false) + def initialize(sample_type_id:, batch_process_params: [], user:, send_email: false) @sample_type = SampleType.find(sample_type_id) @projects = @sample_type.projects - @new_sample_params = new_sample_params - @updated_sample_params = updated_sample_params + @batch_process_params = batch_process_params @user = user @send_email = send_email @results = [] @@ -33,20 +32,23 @@ def update! send_email if @send_email && Seek::Config::email_enabled && !@user.nil? end + def delete! + delete_samples + end + private def validate! raise "Missing sample_type_id or sample type not found" if @sample_type.nil? raise "No projects associated with this Sample Type" if @projects.empty? - raise "Missing new_sample_params" if @new_sample_params.nil? - raise "Missing updated_sample_params" if @updated_sample_params.nil? + raise "Missing new_sample_params" if @batch_process_params.nil? raise "Missing user" if @user.nil? end def create_samples User.with_current_user(@user) do Sample.transaction do - @new_sample_params.each do |par| + @batch_process_params.each do |par| ex_id = par.delete(:ex_id) sample = Sample.new(sample_type: @sample_type, policy: @sample_type.policy, projects: @projects) sample.assign_attributes(par) @@ -57,7 +59,7 @@ def create_samples else error = { ex_id: ex_id, message: "Sample '#{sample.title}' could not be created. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." } @errors << error - Rails.logger.error error + Rails.logger.info error raise ActiveRecord::Rollback end end @@ -68,18 +70,18 @@ def create_samples def update_samples User.with_current_user(@user) do Sample.transaction do - @updated_sample_params.each do |par| + @batch_process_params.each do |par| ex_id = par.delete(:ex_id) sample_id = par[:id] sample = Sample.find(sample_id) if sample.nil? - @errors << "Sample with id '#{sample_id}' not found." + @errors << { ex_id: ex_id, message: "Sample with id '#{sample_id}' not found." } next end unless sample.can_edit?(@user) - @errors << "Not permitted to update this sample." + @errors << { ex_id: ex_id, message: "Not permitted to update this sample." } next end @@ -91,7 +93,38 @@ def update_samples else error = { ex_id: ex_id, message: "Sample '[ID: #{sample.id}] #{sample.title}' could not be updated. Please correct these errors:\n#{sample.errors.full_messages.to_sentence}." } @errors << error - Rails.logger.error error + Rails.logger.info error + raise ActiveRecord::Rollback + end + end + end + end + end + + def delete_samples + User.with_current_user(@user) do + Sample.transaction do + @batch_process_params.each do |par| + ex_id = par.delete(:ex_id) + sample_id = par[:id] + sample = Sample.find(sample_id) + + if sample.nil? + @errors << { ex_id: ex_id, message: "Sample with id '#{sample_id}' not found." } + next + end + + unless sample.can_delete? + @errors << { ex_id: ex_id, message: "Not permitted to delete this sample." } + next + end + + if sample.destroy + @results << { ex_id: ex_id, message: "Sample '[ID: #{sample.id}] #{sample.title}' successfully deleted." } + else + error = { ex_id: ex_id, error: sample.errors.full_messages.to_sentence } + @errors << error + Rails.logger.info error raise ActiveRecord::Rollback end end diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index e4f3287d57..963454849e 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1134,7 +1134,6 @@ def rdf_test_object assert_equal "2", json_response["errors"][0]["ex_id"].to_s end - test 'batch_update' do creator = FactoryBot.create(:person) login_as(creator) @@ -1147,12 +1146,14 @@ def rdf_test_object assert_no_difference('Sample.count') do put :batch_update, params: {data: [ - {id: sample1.id, + {id: sample1.id, + ex_id: '1', data: {type: "samples", attributes: { attribute_map: { "full name": 'Alfred Marcus', "age": '22', "weight": '22.1' }, creator_ids: [creator.id]}}}, - {id: sample2.id, - data: {type: "samples", + {id: sample2.id, + ex_id: '2', + data: {type: "samples", attributes: { attribute_map: { "full name": 'David Tailor', "age": '33', "weight": '33.1' }, creator_ids: [creator.id]}}}], sample_type_id: type.id} @@ -1182,15 +1183,14 @@ def rdf_test_object test 'batch_delete' do person = FactoryBot.create(:person) - sample1 = FactoryBot.create(:patient_sample, contributor: person) - sample2 = FactoryBot.create(:patient_sample, contributor: person) - type1 = sample1.sample_type - type2 = sample1.sample_type + type = FactoryBot.create(:patient_sample_type, contributor: person, projects: [person.projects.first]) + sample1 = FactoryBot.create(:patient_sample, contributor: person, sample_type: type, projects: [person.projects.first]) + sample2 = FactoryBot.create(:patient_sample, contributor: person, sample_type: type, projects: [person.projects.first]) login_as(person.user) assert sample1.can_delete? assert sample2.can_delete? assert_difference('Sample.count', -2) do - delete :batch_delete, params: { data: [ {id: sample1.id}, {id: sample2.id}] } + delete :batch_delete, params: { data: [ { id: sample1.id, ex_id: '1' }, { id: sample2.id, ex_id: '2' } ], sample_type_id: type.id } end # For the Single Page to work properly, these must be included in the response From 11685e05b3a77d7c72d6be20ff9f638dd2577ca8 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 27 Aug 2025 15:19:26 +0200 Subject: [PATCH 27/28] Add sample type id to ajax calls --- .../single_page/dynamic_table.js.erb | 3 +- .../javascripts/single_page/index.js.erb | 16 +++--- app/controllers/samples_controller.rb | 2 +- app/models/sample_type.rb | 2 - .../sample_upload_content.html.erb | 56 +++++++++++-------- config/routes.rb | 1 - test/integration/api/sample_api_test.rb | 11 ++-- 7 files changed, 50 insertions(+), 41 deletions(-) diff --git a/app/assets/javascripts/single_page/dynamic_table.js.erb b/app/assets/javascripts/single_page/dynamic_table.js.erb index 6ee1597be0..74674863b8 100644 --- a/app/assets/javascripts/single_page/dynamic_table.js.erb +++ b/app/assets/javascripts/single_page/dynamic_table.js.erb @@ -48,7 +48,7 @@ function sanitizeData(data) { const objectInputTemp = '' + ''; const typeaheadSamplesUrl = "<%= typeahead_samples_path(linked_sample_type_id: '_LINKED_') %>"; @@ -416,6 +416,7 @@ const handleSelect = (e) => { this.options.callback(); } if (disableLoading) disableLoading(); + location.reload(); }, headers: function () { return this.table diff --git a/app/assets/javascripts/single_page/index.js.erb b/app/assets/javascripts/single_page/index.js.erb index 77d855c464..ad5a95c8ad 100644 --- a/app/assets/javascripts/single_page/index.js.erb +++ b/app/assets/javascripts/single_page/index.js.erb @@ -168,6 +168,7 @@ async function fetchTerms(elem, cvId) { async function batchCreateSample(sampleTypes, projectDefaultPolicy) { try { let data = []; + let sample_type_id = sampleTypes[0].sampleTypeId; sampleTypes.forEach((s) => { s.samples.forEach((sa, k) => { data.push( @@ -179,7 +180,7 @@ async function batchCreateSample(sampleTypes, projectDefaultPolicy) { if (data.length == 0) { return; } - return ajaxCall("<%= batch_create_samples_path %>", "POST", { data: JSON.stringify({ data }) }); + return ajaxCall("<%= batch_create_samples_path %>", "POST", { data: JSON.stringify({ sample_type_id, data }) }); } catch (e) { console.log(e); } @@ -188,16 +189,17 @@ async function batchCreateSample(sampleTypes, projectDefaultPolicy) { async function batchDeleteSample(sampleTypes) { try { let data = []; + let sample_type_id = sampleTypes[0].sampleTypeId; sampleTypes.forEach((s) => { s.samples.forEach((sa, k) => { - data.push(batchSampleDeleteStruct(sa.exId, sa.id)); + data.push(batchSampleDeleteStruct(sa.exId, sa.id, s)); }); }); if (data.length == 0) { return; } - return ajaxCall("<%= batch_delete_samples_path %>", "DELETE", { data: JSON.stringify({ data }) }); + return ajaxCall("<%= batch_delete_samples_path %>", "DELETE", { data: JSON.stringify({ sample_type_id, data }) }); } catch (e) { console.log(e); } @@ -215,7 +217,7 @@ async function exportToExcel(tableName, studyId, assayId, sampleTypeId) { // Checks whether the dynamic table has errors // The excel export will be aborted as long as the dynamic table has errors - hasErrorcells = $j(`table[id=${tableName}]`).find('select.select2__error').size() > 0 + const hasErrorcells = $j(`table[id=${tableName}]`).find('select.select2__error').size() > 0; if (hasErrorcells) { alert('It appears this sample table has some errors. Please correct the errors in the current sample table and try downloading again.'); return; @@ -273,8 +275,7 @@ async function exportToExcel(tableName, studyId, assayId, sampleTypeId) { } }) .done( function(response){ - downloadUrl = `<%= download_samples_excel_single_pages_path() %>?uuid=${response.uuid}`; - window.location.href = downloadUrl; + window.location.href = `<%= download_samples_excel_single_pages_path() %>?uuid=${response.uuid}`; }) .fail( function(response){ alert(`Failed to export through excel!\nStatus: ${response.status}\nError: ${JSON.stringify(response.error().statusText)}`); @@ -287,6 +288,7 @@ async function exportToExcel(tableName, studyId, assayId, sampleTypeId) { async function batchUpdateSample(sampleTypes) { try { let data = []; + let sample_type_id = sampleTypes[0].sampleTypeId; sampleTypes.forEach((s) => { s.samples.forEach((sa, k) => { data.push(batchSampleUpdateStruct(sa.exId, sa.data, sa.id)); @@ -296,7 +298,7 @@ async function batchUpdateSample(sampleTypes) { if (data.length == 0) { return; } - return ajaxCall("<%= batch_update_samples_path %>", "PUT", { data: JSON.stringify({ data }) }); + return ajaxCall("<%= batch_update_samples_path %>", "PUT", { data: JSON.stringify({ sample_type_id, data }) }); } catch (e) { console.log(e); } diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index f154c278d3..8f564c9780 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -2,7 +2,6 @@ class SamplesController < ApplicationController respond_to :html include Seek::PreviewHandling include Seek::AssetsCommon - include Seek::Publishing::PublishingCommon include Seek::IndexPager include Seek::JSONMetadata @@ -16,6 +15,7 @@ class SamplesController < ApplicationController before_action :auth_to_create, only: %i[new create batch_create] include Seek::ISAGraphExtensions + include Seek::Publishing::PublishingCommon api_actions :index, :show, :create, :update, :destroy, :batch_create diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index b56562dd12..2629bc2169 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -65,8 +65,6 @@ class SampleType < ApplicationRecord has_annotation_type :sample_type_tag, method_name: :tags has_task :sample_metadata_update - has_task :samples_batch_create - has_task :samples_batch_update has_task :sample_batch_upload def investigations diff --git a/app/views/single_pages/sample_upload_content.html.erb b/app/views/single_pages/sample_upload_content.html.erb index efb787edd3..57fe9cf479 100644 --- a/app/views/single_pages/sample_upload_content.html.erb +++ b/app/views/single_pages/sample_upload_content.html.erb @@ -71,37 +71,45 @@ } function submitUpload(){ - const newSamples = { data: getNewSamples() }; - const updatedSamples= { data: getUpdateSamples() }; + const sampleTypeId = '<%= @sample_type.id %>' + const newSamples = { sample_type_id: sampleTypeId, data: getNewSamples() }; + const updatedSamples= { sample_type_id: sampleTypeId, data: getUpdateSamples() }; - debugger; if (newSamples.data === "abort") return; makeSampleUploadAjaxCalls(newSamples, updatedSamples); } async function makeSampleUploadAjaxCalls(newSamples, updatedSamples){ - $j('#sample-upload-spinner').show(); - $j('#upload-xlsx-content-btn').prop('disabled', true); - $j('#close-upload-xlsx-modal-btn').prop('disabled', true); - - try{ - const body = { data: JSON.stringify({ - sampleTypeId: <%= @sample_type.id %>, - newSamples: newSamples, - updatedSamples: updatedSamples, - }) - }; - const uploadSamplesCall = await uploadAjaxCall("<%= spreadsheet_upload_samples_path %>", "POST", body); - if (uploadSamplesCall.status !== 'ok') { - throw new Error(JSON.stringify(uploadSamplesCall.result)); + const spinner = $j('#sample-upload-spinner'); + const uploadButton = $j('#upload-xlsx-content-btn'); + const closeButton = $j('#close-upload-xlsx-modal-btn'); + + spinner.show(); + uploadButton.prop('disabled', true); + closeButton.prop('disabled', true); + + try { + const [postCall, putCall] = await Promise.all([ + uploadAjaxCall("<%= batch_create_samples_path %>", "POST", {data: JSON.stringify(newSamples)}), + uploadAjaxCall("<%= batch_update_samples_path %>", "PUT", {data: JSON.stringify(updatedSamples)}) + ]); + + if (postCall.status !== 'ok') { + throw new Error(`Sample creation errors:\n${JSON.stringify(postCall.errors)}`); } - $j('#sample-upload-spinner').hide(); + + if (putCall.status !== 'ok') { + throw new Error(`Sample updating errors:\n${JSON.stringify(putCall.errors)}`); + } + closeModalForm(); - } catch (error){ - alert(`Error: ${error}`); - $j('#sample-upload-spinner').hide(); - $j('#upload-xlsx-content-btn').prop('disabled', false); - $j('#close-upload-xlsx-modal-btn').prop('disabled', false); + } catch (error) { + alert(`Error during sample upload:\n${error}`); + } finally { + spinner.hide(); + uploadButton.prop('disabled', false); + closeButton.prop('disabled', false); + location.reload(); } } @@ -162,7 +170,7 @@ const assayId = '<%= @assay&.id %>'; if (action === "update") { - return batchSampleUpdateStruct(`update-${index}-${sampleTypeId}`, attrMap, objId); + return batchSampleUpdateStruct(`update-${index}-${sampleTypeId}`, attrMap, objId, sampleTypeId); } else { return batchSampleCreateStruct(`new-${index}-${sampleTypeId}`,attrMap, sampleTypeId, pid, defaultPermissions, assayId); } diff --git a/config/routes.rb b/config/routes.rb index f796be96ca..6ee1562e6c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -806,7 +806,6 @@ post :export_to_excel, action: :export_to_excel get :download_samples_excel, action: :download_samples_excel post :preview_upload_samples, action: :preview_upload_samples - post :upload_samples, action: :upload_samples_by_spreadsheet end end diff --git a/test/integration/api/sample_api_test.rb b/test/integration/api/sample_api_test.rb index 6d8686e8ee..d511c61814 100644 --- a/test/integration/api/sample_api_test.rb +++ b/test/integration/api/sample_api_test.rb @@ -236,15 +236,16 @@ def setup project = FactoryBot.create(:project) institution = FactoryBot.create(:institution) person.add_to_project_and_institution(project, institution) - investigation = FactoryBot.create(:investigation, contributor: person) - study = FactoryBot.create(:study, contributor: person) - type = FactoryBot.create(:patient_sample_type, contributor: person) - assay = FactoryBot.create(:assay, contributor: person, sample_type: type) + investigation = FactoryBot.create(:investigation, contributor: person, projects: [project]) + study = FactoryBot.create(:study, contributor: person, investigation: investigation) + type = FactoryBot.create(:patient_sample_type, contributor: person, projects: [project]) + assay = FactoryBot.create(:assay, contributor: person, sample_type: type, study: study) other_person = FactoryBot.create(:person) user_login(other_person) other_person.add_to_project_and_institution(project, institution) params = { + "sample_type_id": "#{type.id}", "data": [ { "ex_id": "1", @@ -283,7 +284,7 @@ def setup assert_difference('AssayAsset.count', 0) do assert_difference('SampleType.count', 0) do post "/samples/batch_create", as: :json, params: params, headers: { 'Authorization' => write_access_auth } - assert_response :success + assert_response :unprocessable_entity end end end From cb9a385cbf5f34cadc0e6e9c9b28af4810f751f6 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 1 Sep 2025 15:54:58 +0200 Subject: [PATCH 28/28] PR review comments --- app/controllers/samples_controller.rb | 2 +- app/models/mailer.rb | 4 ++-- app/services/samples/sample_batch_processor.rb | 4 ++-- .../notify_user_after_spreadsheet_extraction.erb | 12 +++++++----- test/functional/samples_controller_test.rb | 4 ++-- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 8f564c9780..04ad94799d 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -225,7 +225,7 @@ def batch_update results.concat(batch_update_processor.results) raise "The following errors occurred: #{errors.join("\n")}" unless errors.empty? else - SamplesBatchUpdateJob.perform_later(sample_type_id, [], parameters, @current_user, true) + SamplesBatchUpdateJob.perform_later(sample_type_id, parameters, @current_user, true) results = ['A background job has been launched. This Sample Type will now lock itself as long as the background job is in progress.'] end status = :ok diff --git a/app/models/mailer.rb b/app/models/mailer.rb index 4f7109692e..694f04363b 100644 --- a/app/models/mailer.rb +++ b/app/models/mailer.rb @@ -370,12 +370,12 @@ def notify_user_after_spreadsheet_extraction(user, project, item_type, item_id, subject = if errors.empty? "Spreadsheet upload completed successfully" else - "spreadsheet upload Failed" + "Spreadsheet upload failed" end mail(from: Seek::Config.noreply_sender, to: user.email_with_name, subject: subject, - template_name: :notify_user_after_spreadsheet_extraction,) + template_name: :notify_user_after_spreadsheet_extraction) end private diff --git a/app/services/samples/sample_batch_processor.rb b/app/services/samples/sample_batch_processor.rb index 7fe8bb40e1..0d2d8bfe03 100644 --- a/app/services/samples/sample_batch_processor.rb +++ b/app/services/samples/sample_batch_processor.rb @@ -136,7 +136,7 @@ def send_email if @sample_type.assays.empty? && @sample_type.studies.any? item_type = 'study' item_id = @sample_type.studies.first - elsif @sample_type.assays.any? &&@sample_type.studies.empty? + elsif @sample_type.assays.any? && @sample_type.studies.empty? item_type = 'assay' item_id = @sample_type.assays.first else @@ -144,7 +144,7 @@ def send_email item_id = @sample_type.id end - Mailer.notify_user_after_spreadsheet_extraction(@user, @projects, item_type, item_id, results, errors).deliver_now + Mailer.notify_user_after_spreadsheet_extraction(@user, @projects, item_type, item_id, @results, @errors).deliver_now end end end diff --git a/app/views/mailer/notify_user_after_spreadsheet_extraction.erb b/app/views/mailer/notify_user_after_spreadsheet_extraction.erb index d8d154fbb7..66146ec396 100644 --- a/app/views/mailer/notify_user_after_spreadsheet_extraction.erb +++ b/app/views/mailer/notify_user_after_spreadsheet_extraction.erb @@ -1,14 +1,16 @@ -Hi <%= @notifiee.person.name %>, +Hi <%= @user.person.name %>, <% if @errors.empty? %> - Your spreadsheet extraction has been successfully completed. + Your spreadsheet extraction for <%= @item_type.humanize %> with ID '<%= @item_id %>' has been successfully completed. Here are the results: <% @results.each do |res| %> -
  • <%= res %>
    • + - <%= res %> + <% end %> <% else %> - Your spreadsheet extraction encountered one or more errors. Please correct them and try again. + Your spreadsheet extraction for <%= @item_type.humanize %> with ID '<%= @item_id %>' encountered one or more errors. Please correct them and try again. <% @errors.each do |err| %> -
  • <%= err %>
    • + - <%= err %> + <% end %> <% end %> diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index 963454849e..c93b58cb23 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1877,7 +1877,7 @@ def rdf_test_object assert response_body['results'].include?('A background job has been launched.') end - test ' upload updated samples by spreadsheet' do + test 'upload updated samples by spreadsheet' do person = FactoryBot.create(:person) project = person.projects.first login_as(person) @@ -1906,7 +1906,7 @@ def rdf_test_object end end - test ' upload updated samples by spreadsheet as a background job' do + test 'upload updated samples by spreadsheet as a background job' do person = FactoryBot.create(:person) project = person.projects.first login_as(person)