From cfd7789f847882c750722702eaa3692216f49d9c Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 10:31:54 +0200 Subject: [PATCH 01/49] Add sample metadata updater module --- lib/seek/samples/sample_metadata_updater.rb | 38 +++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 lib/seek/samples/sample_metadata_updater.rb diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb new file mode 100644 index 0000000000..adc114dba6 --- /dev/null +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Seek + module Samples + + class SampleMetadataUpdateException < StandardError; end + + # Class to handle the updating of sample metadata after updating Sample type + class SampleMetadataUpdater + def initialize(sample_type) + @sample_type = sample_type + end + + def update_metadata + samples = @sample_type.samples + # Should raise an exception if the user does not have permission to update all the samples in this sample type. + raise SampleMetadataUpdateException('Invalid permissions! You need editing permission to all samples in this sample type.') unless samples.all?(&:can_edit?) + + sample_attributes = @sample_type.sample_attributes.map(&:title) + + samples.each do |sample| + metadata = JSON.parse(sample.json_metadata) + missing_attributes = sample_attributes - metadata.keys + missing_attributes.map do |attr| + metadata[attr] = nil + end + + removed_attributes = metadata.keys - sample_attributes + removed_attributes.map do |attr| + metadata.delete(attr) + end + # For now permission are skipped and all samples will be updated + Sample.find(sample.id).update_column(json_metadata, metadata.to_json) + end + end + end + end +end \ No newline at end of file From 133718f73bce70370a4a05f308f58ff0633ed8dc Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 10:33:02 +0200 Subject: [PATCH 02/49] Add Sample Metadata upater job --- app/jobs/update_sample_metadata_job.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 app/jobs/update_sample_metadata_job.rb diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb new file mode 100644 index 0000000000..5feac0cd97 --- /dev/null +++ b/app/jobs/update_sample_metadata_job.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class UpdateSampleMetadataJob < ApplicationJob + queue_with_priority 1 + queue_as :QueueNames::SAMPLES + + def perform(sample_type) + Seek::Samples::SampleMetadataUpdater.new(sample_type).update_metadata + end +end From 9c5aec7269c291b63dacbdbf555bd35608c79292 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 10:33:36 +0200 Subject: [PATCH 03/49] Add after_action for updating the sample json metadata --- app/controllers/sample_types_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index b40d6a9f63..fffbe8fb32 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -13,6 +13,7 @@ class SampleTypesController < ApplicationController before_action :auth_to_create, only: %i[new create] before_action :project_membership_required, only: %i[create new select filter_for_select] + after_action :update_sample_json_metadata, only: :update api_actions :index, :show, :create, :update, :destroy @@ -201,4 +202,8 @@ def check_no_created_samples redirect_to @sample_type end end + + def update_sample_json_metadata + UpdateSampleMetadataJob.perform_later(@sample_type) + end end From 90ac8b4a84227df66614c12eaf0a560a1f4baf81 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 13:00:56 +0200 Subject: [PATCH 04/49] Skip if sample metadata keys correspond with sample attribute titles --- lib/seek/samples/sample_metadata_updater.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index adc114dba6..27068c4ee4 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -20,6 +20,9 @@ def update_metadata samples.each do |sample| metadata = JSON.parse(sample.json_metadata) + # Skip this sample if the sample attributes are the same as the JSON metadata keys + next if metadata.keys == sample_attributes + missing_attributes = sample_attributes - metadata.keys missing_attributes.map do |attr| metadata[attr] = nil @@ -30,7 +33,7 @@ def update_metadata metadata.delete(attr) end # For now permission are skipped and all samples will be updated - Sample.find(sample.id).update_column(json_metadata, metadata.to_json) + sample.update_column(:json_metadata, metadata.to_json) end end end From 037521b62d77a4d3e8fac1080ab190c5b33a8ed2 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 13:02:29 +0200 Subject: [PATCH 05/49] Remove add new attribute constraint --- app/models/sample_type.rb | 4 ---- app/views/isa_studies/_sample_types_form.html.erb | 2 +- app/views/sample_types/_form.html.erb | 2 +- lib/seek/samples/sample_type_editing_constraints.rb | 5 ----- test/unit/samples/sample_type_editing_constraints_test.rb | 8 -------- 5 files changed, 2 insertions(+), 19 deletions(-) diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 4483afb499..46d44fd500 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -244,10 +244,6 @@ def validate_against_editing_constraints if a.marked_for_destruction? && !c.allow_attribute_removal?(a) errors.add(:sample_attributes, "cannot be removed, there are existing samples using this attribute (#{a.title})") end - - if a.new_record? && !c.allow_new_attribute? - errors.add(:sample_attributes, "cannot be added, new attributes are not allowed (#{a.title})") - end end end diff --git a/app/views/isa_studies/_sample_types_form.html.erb b/app/views/isa_studies/_sample_types_form.html.erb index 910f300b7f..03a874e7fc 100644 --- a/app/views/isa_studies/_sample_types_form.html.erb +++ b/app/views/isa_studies/_sample_types_form.html.erb @@ -52,7 +52,7 @@ display_isa_tag: true } %> <% end %> - <% unless sample_type.uploaded_template? || !sample_type.editing_constraints.allow_new_attribute? %> + <% unless sample_type.uploaded_template? %> > <%= button_link_to('Add new attribute', 'add', '#', id: add_attribute_id ) %> diff --git a/app/views/sample_types/_form.html.erb b/app/views/sample_types/_form.html.erb index bec7e0838c..1be3c88ab6 100644 --- a/app/views/sample_types/_form.html.erb +++ b/app/views/sample_types/_form.html.erb @@ -49,7 +49,7 @@ display_isa_tag: false } %> <% end %> - <% unless @sample_type.uploaded_template? || !@sample_type.editing_constraints.allow_new_attribute? %> + <% unless @sample_type.uploaded_template? %> <%= button_link_to('Add new attribute', 'add', '#', id: 'add-attribute') %> diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index d5c35aa78d..3bbbd2bff0 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -49,11 +49,6 @@ def allow_attribute_removal?(attr) end end - # whether a new attribtue can be created - def allow_new_attribute? - !samples? - end - # whether the name of the attribute can be changed def allow_name_change?(attr) if attr.is_a?(SampleAttribute) diff --git a/test/unit/samples/sample_type_editing_constraints_test.rb b/test/unit/samples/sample_type_editing_constraints_test.rb index 4cfdc03a7a..bd552c9db3 100644 --- a/test/unit/samples/sample_type_editing_constraints_test.rb +++ b/test/unit/samples/sample_type_editing_constraints_test.rb @@ -159,14 +159,6 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase refute c.send(:all_blank?, 'full name') end - test 'allow_new_attribute' do - # currently only allowed if there are not samples - c = Seek::Samples::SampleTypeEditingConstraints.new(sample_type_with_samples) - refute c.allow_new_attribute? - c = Seek::Samples::SampleTypeEditingConstraints.new(FactoryBot.create(:simple_sample_type)) - assert c.allow_new_attribute? - end - test 'allow editing isa tag' do person = FactoryBot.create(:person) project = person.projects.first From caeab809192b1a80284aa018732b89d3424244fe Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 13:05:02 +0200 Subject: [PATCH 06/49] Don't start job if there are samples --- app/controllers/sample_types_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index fffbe8fb32..90506153dd 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -204,6 +204,6 @@ def check_no_created_samples end def update_sample_json_metadata - UpdateSampleMetadataJob.perform_later(@sample_type) + UpdateSampleMetadataJob.perform_later(@sample_type) unless @sample_type.samples.blank? end end From 8ce597beee68bab6cf59b874ff2c56d91af0dc07 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 13:05:12 +0200 Subject: [PATCH 07/49] typo --- app/jobs/update_sample_metadata_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index 5feac0cd97..140ca064cc 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -2,7 +2,7 @@ class UpdateSampleMetadataJob < ApplicationJob queue_with_priority 1 - queue_as :QueueNames::SAMPLES + queue_as QueueNames::SAMPLES def perform(sample_type) Seek::Samples::SampleMetadataUpdater.new(sample_type).update_metadata From d78ea795c69ae067eaf077c563aca197b56ab41e Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 13:06:01 +0200 Subject: [PATCH 08/49] Make unit editable --- app/views/sample_types/_sample_attribute_form.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/sample_types/_sample_attribute_form.html.erb b/app/views/sample_types/_sample_attribute_form.html.erb index 1adb8d8327..9013cf8692 100644 --- a/app/views/sample_types/_sample_attribute_form.html.erb +++ b/app/views/sample_types/_sample_attribute_form.html.erb @@ -132,7 +132,7 @@ unit_id), include_blank: true, class: 'form-control', - disabled: !allow_type_change, data: { attr: "unit" } %> + data: { attr: "unit" } %> From fd18c8b1c47bffb71a066bd6ec9e7583dc8bb482 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 13:11:09 +0200 Subject: [PATCH 09/49] Fix sample types controller test add attribute button is still visible, even when the sample type has samples. --- test/functional/sample_types_controller_test.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 9c60c8d95d..6769c15115 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -439,16 +439,6 @@ class SampleTypesControllerTest < ActionController::TestCase get :edit, params: { id: type.id } assert_response :success assert_select 'a#add-attribute', count: 1 - - sample = FactoryBot.create(:patient_sample, contributor: @person, - sample_type: FactoryBot.create(:patient_sample_type, project_ids: @project_ids, contributor: @person)) - type = sample.sample_type - refute_empty type.samples - assert type.can_edit? - - get :edit, params: { id: type.id } - assert_response :success - assert_select 'a#add-attribute', count: 0 end test 'cannot access when disabled' do From 36e355af1ddace6a6575841a44e3c0dd855e2695 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jul 2024 13:49:19 +0200 Subject: [PATCH 10/49] test for adding new attribute --- .../sample_types_controller_test.rb | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 6769c15115..49c1c80482 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -753,6 +753,28 @@ class SampleTypesControllerTest < ActionController::TestCase assert_select 'a', text: 'Manage Sample Type', count: 0 end + test 'add new attribute to an existing sample type populated with samples' do + sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) + samples = (1..10).map do |i| + FactoryBot.create(:sample, contributor: @person, project_ids: @project_ids, sample_type:) + end + refute_empty sample_type.samples + login_as(@person) + get :edit, params: { id: sample_type.id } + assert_response :success + assert_select 'a#add-attribute', count: 1 + assert_difference('SampleAttribute.count') do + put :update, params: { id: sample_type.id, sample_type: { + sample_attributes_attributes: { + '1': { title: 'new attribute', sample_attribute_type_id: @string_type.id } + } + } } + end + assert_redirected_to sample_type_path(sample_type) + sample_type.reload + assert_equal 'new attribute', sample_type.sample_attributes.last.title + end + private def template_for_upload From f8941523d49a992639ac2e54ac195372f9b3fbca Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 10 Oct 2024 11:16:27 +0200 Subject: [PATCH 11/49] linting --- test/functional/sample_types_controller_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 49c1c80482..cfce8e2d5b 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -755,8 +755,8 @@ class SampleTypesControllerTest < ActionController::TestCase test 'add new attribute to an existing sample type populated with samples' do sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) - samples = (1..10).map do |i| - FactoryBot.create(:sample, contributor: @person, project_ids: @project_ids, sample_type:) + (1..10).map do |_i| + FactoryBot.create(:sample, contributor: @person, project_ids: @project_ids, sample_type: sample_type) end refute_empty sample_type.samples login_as(@person) From fd3a532ffb4d01fc36d7a07719b8fc9c4294b1db Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 10 Oct 2024 13:00:06 +0200 Subject: [PATCH 12/49] Add UpdateSampleMetadataTest --- .../jobs/update_sample_metadata_job_test.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 test/unit/jobs/update_sample_metadata_job_test.rb diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb new file mode 100644 index 0000000000..822f13b893 --- /dev/null +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'test_helper' + +class UpdateSampleMetadataJobTest < ActiveSupport::TestCase + def setup + @person = FactoryBot.create(:person) + @project = @person.projects.first + @sample_type = FactoryBot.create(:simple_sample_type, project_ids: [@project.id], contributor: @person, policy: FactoryBot.create(:public_policy)) + (1..10).each do |_i| + FactoryBot.create(:sample, sample_type: @sample_type, contributor: @person) + end + end + + def teardown + # Do nothing + end + + test 'perform' do + User.with_current_user(@person.user) do + UpdateSampleMetadataJob.new.perform(@sample_type) + end + end +end From a631ae36834d4dbf90c1361e4ed4ce73569bfe3e Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 15 Oct 2024 12:57:44 +0200 Subject: [PATCH 13/49] Add test Checks if the sample metadata keys (attribute title) has changed after the job ran. --- test/unit/jobs/update_sample_metadata_job_test.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index 822f13b893..f1a7909f66 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -21,4 +21,19 @@ def teardown UpdateSampleMetadataJob.new.perform(@sample_type) end end + + test 'Check sample metadata after performing the job' do + User.with_current_user(@person.user) do + assert_equal @sample_type.sample_attributes.first.title, 'the_title' + @sample_type.sample_attributes.first.update!(title: 'new title') + assert_equal @sample_type.sample_attributes.first.title, 'new title' + refute_equal @sample_type.sample_attributes.first.title, 'the_title' + UpdateSampleMetadataJob.new.perform(@sample_type) + @sample_type.samples.each do |sample| + json_metadata = JSON.parse sample.json_metadata + assert json_metadata.keys.include?('new title') + refute json_metadata.keys.include?('the_title') + end + end + end end From 662fbdf5a2406239049da8d7a2044ec4935dab74 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 15 Oct 2024 15:43:03 +0200 Subject: [PATCH 14/49] Fix the Sample metadata updater Pass a Hash describing wich attributes have a different title. --- app/controllers/sample_types_controller.rb | 15 +++++++++++++- app/jobs/update_sample_metadata_job.rb | 4 ++-- lib/seek/samples/sample_metadata_updater.rb | 20 +++++-------------- .../sample_types_controller_test.rb | 1 + .../jobs/update_sample_metadata_job_test.rb | 5 +++-- 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index 90506153dd..4d81e40919 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -12,6 +12,7 @@ class SampleTypesController < ApplicationController before_action :find_assets, only: [:index] before_action :auth_to_create, only: %i[new create] before_action :project_membership_required, only: %i[create new select filter_for_select] + before_action :old_attributes, only: %i[update] after_action :update_sample_json_metadata, only: :update @@ -203,7 +204,19 @@ def check_no_created_samples end end + def old_attributes + return if @sample_type.sample_attributes.blank? + + @old_sample_type_attributes = @sample_type.sample_attributes + end + def update_sample_json_metadata - UpdateSampleMetadataJob.perform_later(@sample_type) unless @sample_type.samples.blank? + return if @sample_type.samples.blank? || @old_sample_type_attributes.blank? + + attribute_changes = @sample_type.sample_attributes.each do |attr| + old_attr = @old_sample_type_attributes.detect { |oa| oa.id == attr.id } + { id: attr.id, old_title: old_attr&.title, new_title: attr.title } unless old_attr&.title == attr.title + end + UpdateSampleMetadataJob.perform_later(@sample_type, attribute_changes) unless attribute_changes.blank? end end diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index 140ca064cc..492c98a4ae 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -4,7 +4,7 @@ class UpdateSampleMetadataJob < ApplicationJob queue_with_priority 1 queue_as QueueNames::SAMPLES - def perform(sample_type) - Seek::Samples::SampleMetadataUpdater.new(sample_type).update_metadata + def perform(sample_type, attribute_changes = []) + Seek::Samples::SampleMetadataUpdater.new(sample_type, attribute_changes).update_metadata end end diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index 27068c4ee4..68a2d9c22e 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -7,8 +7,9 @@ class SampleMetadataUpdateException < StandardError; end # Class to handle the updating of sample metadata after updating Sample type class SampleMetadataUpdater - def initialize(sample_type) + def initialize(sample_type, attribute_changes) @sample_type = sample_type + @attribute_change_maps = attribute_changes end def update_metadata @@ -16,23 +17,12 @@ def update_metadata # Should raise an exception if the user does not have permission to update all the samples in this sample type. raise SampleMetadataUpdateException('Invalid permissions! You need editing permission to all samples in this sample type.') unless samples.all?(&:can_edit?) - sample_attributes = @sample_type.sample_attributes.map(&:title) - samples.each do |sample| metadata = JSON.parse(sample.json_metadata) - # Skip this sample if the sample attributes are the same as the JSON metadata keys - next if metadata.keys == sample_attributes - - missing_attributes = sample_attributes - metadata.keys - missing_attributes.map do |attr| - metadata[attr] = nil - end - - removed_attributes = metadata.keys - sample_attributes - removed_attributes.map do |attr| - metadata.delete(attr) + # Update the metadata keys with the new attribute titles + @attribute_change_maps.each do |change| + metadata[change[:new_title]] = metadata.delete(change[:old_title]) end - # For now permission are skipped and all samples will be updated sample.update_column(:json_metadata, metadata.to_json) end end diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index cfce8e2d5b..f3ae8979a1 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -677,6 +677,7 @@ class SampleTypesControllerTest < ActionController::TestCase end end + # TODO: This test fails now because the sample type is not immutable anymore. test 'validates changes against editing constraints' do @sample_type.samples.create!(data: { the_title: 'yes' }, sample_type: @sample_type, project_ids: @project_ids) diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index f1a7909f66..dba91bc006 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -22,13 +22,14 @@ def teardown end end - test 'Check sample metadata after performing the job' do + test 'Check sample metadata after updating the attribute title' do User.with_current_user(@person.user) do assert_equal @sample_type.sample_attributes.first.title, 'the_title' @sample_type.sample_attributes.first.update!(title: 'new title') + attribute_change_maps = [{id: @sample_type.sample_attributes.first.id, old_title: 'the_title', new_title: 'new title' }] assert_equal @sample_type.sample_attributes.first.title, 'new title' refute_equal @sample_type.sample_attributes.first.title, 'the_title' - UpdateSampleMetadataJob.new.perform(@sample_type) + UpdateSampleMetadataJob.new.perform(@sample_type, attribute_change_maps) @sample_type.samples.each do |sample| json_metadata = JSON.parse sample.json_metadata assert json_metadata.keys.include?('new title') From f4a59ac64752d3c240195e47ed23e7e062999938 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Oct 2024 16:42:54 +0200 Subject: [PATCH 15/49] Check user --- app/controllers/sample_types_controller.rb | 16 ++++++++----- app/jobs/update_sample_metadata_job.rb | 4 ++-- lib/seek/samples/sample_metadata_updater.rb | 21 ++++++++-------- .../jobs/update_sample_metadata_job_test.rb | 24 +++++++++---------- 4 files changed, 34 insertions(+), 31 deletions(-) diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index 4d81e40919..73932dcfcf 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -207,16 +207,20 @@ def check_no_created_samples def old_attributes return if @sample_type.sample_attributes.blank? - @old_sample_type_attributes = @sample_type.sample_attributes + @old_sample_type_attributes = @sample_type.sample_attributes.map { |attr| { id: attr.id, title: attr.title } } end def update_sample_json_metadata return if @sample_type.samples.blank? || @old_sample_type_attributes.blank? - attribute_changes = @sample_type.sample_attributes.each do |attr| - old_attr = @old_sample_type_attributes.detect { |oa| oa.id == attr.id } - { id: attr.id, old_title: old_attr&.title, new_title: attr.title } unless old_attr&.title == attr.title - end - UpdateSampleMetadataJob.perform_later(@sample_type, attribute_changes) unless attribute_changes.blank? + attribute_changes = @sample_type.sample_attributes.map do |attr| + old_attr = @old_sample_type_attributes.detect { |oa| oa[:id] == attr.id } + next if old_attr.nil? + + { id: attr.id, old_title: old_attr[:title], new_title: attr.title } unless old_attr[:title] == attr.title + end.compact + return if attribute_changes.blank? + + UpdateSampleMetadataJob.perform_later(@sample_type, attribute_changes, @current_user) end end diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index 492c98a4ae..d4bc420f98 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -4,7 +4,7 @@ class UpdateSampleMetadataJob < ApplicationJob queue_with_priority 1 queue_as QueueNames::SAMPLES - def perform(sample_type, attribute_changes = []) - Seek::Samples::SampleMetadataUpdater.new(sample_type, attribute_changes).update_metadata + def perform(sample_type, attribute_changes = [], user) + Seek::Samples::SampleMetadataUpdater.new(sample_type, attribute_changes, user).update_metadata end end diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index 68a2d9c22e..2aa195f9fe 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -7,23 +7,24 @@ class SampleMetadataUpdateException < StandardError; end # Class to handle the updating of sample metadata after updating Sample type class SampleMetadataUpdater - def initialize(sample_type, attribute_changes) + def initialize(sample_type, attribute_changes, user) @sample_type = sample_type @attribute_change_maps = attribute_changes + @user = user end def update_metadata - samples = @sample_type.samples - # Should raise an exception if the user does not have permission to update all the samples in this sample type. - raise SampleMetadataUpdateException('Invalid permissions! You need editing permission to all samples in this sample type.') unless samples.all?(&:can_edit?) + return if @attribute_change_maps.blank? || @sample_type.samples.blank? || @sample_type.nil? || @user.nil? - samples.each do |sample| - metadata = JSON.parse(sample.json_metadata) - # Update the metadata keys with the new attribute titles - @attribute_change_maps.each do |change| - metadata[change[:new_title]] = metadata.delete(change[:old_title]) + User.with_current_user(@user) do + @sample_type.samples.each do |sample| + metadata = JSON.parse(sample.json_metadata) + # Update the metadata keys with the new attribute titles + @attribute_change_maps.each do |change| + metadata[change[:new_title]] = metadata.delete(change[:old_title]) + end + sample.update_column(:json_metadata, metadata.to_json) end - sample.update_column(:json_metadata, metadata.to_json) end end end diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index dba91bc006..e978fd2e22 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -18,23 +18,21 @@ def teardown test 'perform' do User.with_current_user(@person.user) do - UpdateSampleMetadataJob.new.perform(@sample_type) + UpdateSampleMetadataJob.new.perform(@sample_type, [], @person.user) end end test 'Check sample metadata after updating the attribute title' do - User.with_current_user(@person.user) do - assert_equal @sample_type.sample_attributes.first.title, 'the_title' - @sample_type.sample_attributes.first.update!(title: 'new title') - attribute_change_maps = [{id: @sample_type.sample_attributes.first.id, old_title: 'the_title', new_title: 'new title' }] - assert_equal @sample_type.sample_attributes.first.title, 'new title' - refute_equal @sample_type.sample_attributes.first.title, 'the_title' - UpdateSampleMetadataJob.new.perform(@sample_type, attribute_change_maps) - @sample_type.samples.each do |sample| - json_metadata = JSON.parse sample.json_metadata - assert json_metadata.keys.include?('new title') - refute json_metadata.keys.include?('the_title') - end + assert_equal @sample_type.sample_attributes.first.title, 'the_title' + @sample_type.sample_attributes.first.update!(title: 'new title') + attribute_change_maps = [{id: @sample_type.sample_attributes.first.id, old_title: 'the_title', new_title: 'new title' }] + assert_equal @sample_type.sample_attributes.first.title, 'new title' + refute_equal @sample_type.sample_attributes.first.title, 'the_title' + UpdateSampleMetadataJob.new.perform(@sample_type, attribute_change_maps, @person.user) + @sample_type.samples.each do |sample| + json_metadata = JSON.parse sample.json_metadata + assert json_metadata.keys.include?('new title') + refute json_metadata.keys.include?('the_title') end end end From 8b20eaf9b78800c3f2feef525a8eda6f7fc2c65c Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Oct 2024 16:57:34 +0200 Subject: [PATCH 16/49] Change `allow_name_change?` User should only be able to change the sample type attribute's title if the user has editing permission to all samples. --- lib/seek/samples/sample_type_editing_constraints.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index 3bbbd2bff0..c1a310208c 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -53,12 +53,11 @@ def allow_attribute_removal?(attr) def allow_name_change?(attr) if attr.is_a?(SampleAttribute) return true if attr.new_record? - return false if inherited?(attr) attr = attr.accessor_name end if attr - !samples? + samples.all?(&:can_edit?) else true end From 4a079c193fb025adf11b11882c2377b2430d22b3 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Oct 2024 17:02:46 +0200 Subject: [PATCH 17/49] Add explaination Add explaination to why changing the attribute's name is prohibited --- app/views/sample_types/_sample_attribute_form.html.erb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/sample_types/_sample_attribute_form.html.erb b/app/views/sample_types/_sample_attribute_form.html.erb index 9013cf8692..0696353b6d 100644 --- a/app/views/sample_types/_sample_attribute_form.html.erb +++ b/app/views/sample_types/_sample_attribute_form.html.erb @@ -31,6 +31,7 @@ <% allow_isa_tag_change = constraints.allow_isa_tag_change?(sample_attribute) if display_isa_tag %> <% required_title_text = allow_required ? nil : "You are not allowed to make this attribute required.\nSome samples have no value for this attribute." %> <% is_title_title_text = allow_required ? nil : "You are not allowed to change this attribute's title field.\nSome samples have no value for this attribute." %> +<% change_name_text = allow_name_change ? nil : "You are not allowed to change the name of this attribute.\nYou need at least editing permission to all samples in this sample type." %> @@ -42,7 +43,7 @@ - <%= text_field_tag "#{field_name_prefix}[title]", title, class: 'form-control', + <%= text_field_tag "#{field_name_prefix}[title]", title, class: 'form-control', title: change_name_text, placeholder: 'Attribute name', disabled: !allow_name_change, data: { attr: "title" } %> From 2547489f6cf6588b32e3523f58e8b587afa8f444 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Oct 2024 17:41:12 +0200 Subject: [PATCH 18/49] Fix test --- test/functional/sample_types_controller_test.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index f3ae8979a1..5605be9bc1 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -677,10 +677,21 @@ class SampleTypesControllerTest < ActionController::TestCase end end - # TODO: This test fails now because the sample type is not immutable anymore. test 'validates changes against editing constraints' do - @sample_type.samples.create!(data: { the_title: 'yes' }, sample_type: @sample_type, project_ids: @project_ids) + other_person = FactoryBot.create(:person) + other_person.add_to_project_and_institution(@project, @person.institutions.first) + @sample_type.samples.create!(data: { the_title: 'Just look at me!' }, sample_type: @sample_type, + project_ids: @project_ids, policy: FactoryBot.create(:private_policy)) + login_as other_person + + @sample_type.samples.create!(data: { the_title: 'Don\'t look at me!' }, sample_type: @sample_type, + project_ids: @project_ids, policy: FactoryBot.create(:private_policy)) + + login_as @person + get :edit, params: { id: @sample_type } + assert_response :success + assert_select 'input#sample_type_sample_attributes_attributes_0_title[disabled=?]', 'disabled' assert_no_difference('ActivityLog.count') do put :update, params: { id: @sample_type, sample_type: { sample_attributes_attributes: { From 928ffc493626c6627c3025b88f2658087c69d028 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 16 Oct 2024 18:19:33 +0200 Subject: [PATCH 19/49] Change existing title constraints test Add new scenario's where tests should fail or succeed --- .../sample_type_editing_constraints_test.rb | 48 +++++++++++++------ 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/test/unit/samples/sample_type_editing_constraints_test.rb b/test/unit/samples/sample_type_editing_constraints_test.rb index bd552c9db3..05bb63423e 100644 --- a/test/unit/samples/sample_type_editing_constraints_test.rb +++ b/test/unit/samples/sample_type_editing_constraints_test.rb @@ -21,20 +21,38 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase end test 'allow title change?' do - c = Seek::Samples::SampleTypeEditingConstraints.new(sample_type_with_samples) - refute c.allow_name_change?(:address) - attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'address' } - refute_nil attr - refute c.allow_name_change?(attr) - assert c.allow_name_change?(nil) + person = FactoryBot.create(:person) - # ok if there are no samples - c = Seek::Samples::SampleTypeEditingConstraints.new(FactoryBot.create(:simple_sample_type)) - assert c.allow_name_change?(:the_title) - attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'the_title' } - refute_nil attr - assert c.allow_name_change?(attr) - assert c.allow_name_change?(nil) + User.with_current_user(person.user) do + # Not OK if te user doesn't have editing permission over all samples + st1 = sample_type_with_samples + refute st1.samples.all?(&:can_edit?) + c = Seek::Samples::SampleTypeEditingConstraints.new(st1) + refute c.allow_name_change?(:address) + attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'address' } + refute_nil attr + refute c.allow_name_change?(attr) + assert c.allow_name_change?(nil) + + # OK if there are no samples + st2 = FactoryBot.create(:simple_sample_type) + c = Seek::Samples::SampleTypeEditingConstraints.new(st2) + assert c.allow_name_change?(:the_title) + attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'the_title' } + refute_nil attr + assert c.allow_name_change?(attr) + assert c.allow_name_change?(nil) + + # OK if user has editing permission over all samples + st3 = sample_type_with_samples(person) + assert st3.samples.all?(&:can_edit?) + c = Seek::Samples::SampleTypeEditingConstraints.new(st3) + assert c.allow_name_change?(:address) + attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'address' } + refute_nil attr + assert c.allow_name_change?(attr) + assert c.allow_name_change?(nil) + end end test 'allow type change?' do @@ -216,8 +234,8 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase # - the address attribute includes some blanks # - the postcode is always blank # - full name and age are required and always have values - def sample_type_with_samples - person = FactoryBot.create(:person) + def sample_type_with_samples(person = nil) + person ||= FactoryBot.create(:person) sample_type = User.with_current_user(person.user) do project = person.projects.first From d44fafcb3c00b33cc622bcb7a12635de9fd9dfc5 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 21 Oct 2024 15:25:43 +0200 Subject: [PATCH 20/49] Add test for changing name of attribute. --- .../sample_types_controller_test.rb | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 5605be9bc1..4cef1f1178 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -787,6 +787,79 @@ class SampleTypesControllerTest < ActionController::TestCase assert_equal 'new attribute', sample_type.sample_attributes.last.title end + test 'change attribute name' do + other_person = FactoryBot.create(:person) + st_policy = FactoryBot.create(:private_policy, + permissions: [FactoryBot.create(:manage_permission, contributor: other_person)]) + sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person, + policy_id: st_policy.id) + sample_type.sample_attributes << FactoryBot.create(:sample_attribute, title: 'new title', + sample_attribute_type: @string_type, is_title: false, sample_type: sample_type) + assert_equal sample_type.sample_attributes.last.title, 'new title' + login_as(@person) + + # Scenario 1: Sample type does not have any samples + # @person can change the attribute title + get :edit, params: { id: sample_type.id } + assert_response :success + assert_select 'tr.sample-attribute:has(input[data-attr="title"][value=?])', 'new title' do |tr| + assert_select tr, 'input[data-attr="title"]' do |input| + assert_select input, '[disabled]', count: 0 + assert_select input, '[title]', count: 0 + end + end + + # Scenario 2: Sample type has samples and @person has permission to edit all samples + # @person can change the attribute title + (1..10).map do |i| + editing_policy = FactoryBot.create(:private_policy, + permissions: [FactoryBot.create(:edit_permission, contributor: @person), + FactoryBot.create(:manage_permission, contributor: other_person)]) + FactoryBot.create(:sample, data: { 'new title': "new title sample #{i}" }, + contributor: other_person, project_ids: @project_ids, sample_type: sample_type, + policy_id: editing_policy.id) + end + sample_type.reload + get :edit, params: { id: sample_type.id } + assert_response :success + assert_equal sample_type.samples.count, 10 + assert(sample_type.samples.all? { |sample| sample.can_edit?(@person) }) + + assert_select 'tr.sample-attribute:has(input[data-attr="title"][value=?])', 'new title' do |tr| + assert_select tr, 'input[data-attr="title"]' do |input| + assert_select input, '[disabled]', count: 0 + assert_select input, '[title]', count: 0 + end + end + + + # Scenario 3: Sample type has samples but @person does not have permission to edit all samples + # @person cannot change the attribute title + (1..10).map do |i| + viewing_policy = FactoryBot.create(:private_policy, + permissions: [FactoryBot.create(:permission, contributor: @person), + FactoryBot.create(:manage_permission, contributor: other_person)]) + FactoryBot.create(:sample, data: { 'new title': "new title sample #{i}" }, + contributor: other_person, + project_ids: @project_ids, sample_type: sample_type, policy_id: viewing_policy.id) + end + + sample_type.reload + get :edit, params: { id: sample_type.id } + assert_response :success + assert_equal sample_type.samples.count, 20 + refute(sample_type.samples.all? { |sample| sample.can_edit?(@person) }) + + assert_select 'tr.sample-attribute:has(input[data-attr="title"][value=?])', 'new title' do |tr| + assert_select tr, 'input[data-attr="title"]' do |input| + assert_equal input.attr('disabled').text, 'disabled' + assert_equal input.attr('title').text, + "You are not allowed to change the name of this attribute.\nYou need at least editing permission to all samples in this sample type." + end + end + + end + private def template_for_upload From 3cb7b13cb536293373fc48fad94fb5ffb10103a0 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 21 Oct 2024 16:13:52 +0200 Subject: [PATCH 21/49] Allow adding optional attribute to existing sample type --- .../sample_type_editing_constraints.rb | 2 +- .../sample_types_controller_test.rb | 20 +++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index c1a310208c..9f599e503e 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -21,7 +21,7 @@ def samples? # if attr is nil, indicates a new attribute. required is not allowed if there are already samples def allow_required?(attr) if attr.is_a?(SampleAttribute) - return true if attr.new_record? + return true if attr.new_record? && @sample_type.new_record? return false if inherited?(attr) attr = attr.accessor_name diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 4cef1f1178..59fca0173c 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -775,16 +775,28 @@ class SampleTypesControllerTest < ActionController::TestCase get :edit, params: { id: sample_type.id } assert_response :success assert_select 'a#add-attribute', count: 1 - assert_difference('SampleAttribute.count') do - put :update, params: { id: sample_type.id, sample_type: { + + # Should be able to add an optional new attribute to a sample type with samples + assert_difference('SampleAttribute.count', 1) do + patch :update, params: { id: sample_type.id, sample_type: { sample_attributes_attributes: { - '1': { title: 'new attribute', sample_attribute_type_id: @string_type.id } + '1': { title: 'new optional attribute', sample_attribute_type_id: @string_type.id, required: '0' } } } } end assert_redirected_to sample_type_path(sample_type) sample_type.reload - assert_equal 'new attribute', sample_type.sample_attributes.last.title + assert_equal 'new optional attribute', sample_type.sample_attributes.last.title + + # Should not be able to add a mandatory new attribute to a sample type with samples + assert_no_difference('SampleAttribute.count') do + patch :update, params: { id: sample_type.id, sample_type: { + sample_attributes_attributes: { + '2': { title: 'new mandatory attribute', sample_attribute_type_id: @string_type.id, required: '1' } + } + } } + end + end test 'change attribute name' do From fc0a0b01a4c5060946119bc2c09e81d28fac2004 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 22 Oct 2024 12:02:28 +0200 Subject: [PATCH 22/49] Adjusted error message if attribute.new_record? --- app/models/sample_attribute.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/sample_attribute.rb b/app/models/sample_attribute.rb index 9740e0c3fe..307f5b5cb6 100644 --- a/app/models/sample_attribute.rb +++ b/app/models/sample_attribute.rb @@ -97,7 +97,8 @@ def force_required_when_is_title def validate_against_editing_constraints c = sample_type.editing_constraints - error_message = "cannot be changed (#{title_was})" # Use pre-change title in error message. + attr_title = self.new_record? ? title : title_was + error_message = "cannot be changed (#{attr_title})" # Use pre-change title in error message. errors.add(:title, error_message) if title_changed? && !c.allow_name_change?(self) From 675a39bb021ba2105d8100f82bbfcfb5c2fb6e70 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 22 Oct 2024 12:04:13 +0200 Subject: [PATCH 23/49] Restructure constraints More readable --- .../sample_type_editing_constraints.rb | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index 9f599e503e..bf298d2bd8 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -26,11 +26,11 @@ def allow_required?(attr) attr = attr.accessor_name end - if attr - !blanks?(attr) - else - !samples? - end + + return false unless attr + return !blanks?(attr) if samples? + + true end # an attribute could be removed if all are currently blank @@ -56,11 +56,10 @@ def allow_name_change?(attr) attr = attr.accessor_name end - if attr - samples.all?(&:can_edit?) - else - true - end + return false unless attr + return samples.all?(&:can_edit?) if samples? + + true end # whether the type for the attribute can be changed From 190cd762e70ce95cc06ce0bca7efe379b82edc1e Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 22 Oct 2024 12:05:43 +0200 Subject: [PATCH 24/49] Put job in with_current_user block Effect of adding `samples.all?(&:can_edit?)` clause ini editing constraints. --- .../jobs/update_sample_metadata_job_test.rb | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index e978fd2e22..d465b04e4f 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -23,16 +23,17 @@ def teardown end test 'Check sample metadata after updating the attribute title' do - assert_equal @sample_type.sample_attributes.first.title, 'the_title' - @sample_type.sample_attributes.first.update!(title: 'new title') - attribute_change_maps = [{id: @sample_type.sample_attributes.first.id, old_title: 'the_title', new_title: 'new title' }] - assert_equal @sample_type.sample_attributes.first.title, 'new title' - refute_equal @sample_type.sample_attributes.first.title, 'the_title' - UpdateSampleMetadataJob.new.perform(@sample_type, attribute_change_maps, @person.user) - @sample_type.samples.each do |sample| - json_metadata = JSON.parse sample.json_metadata - assert json_metadata.keys.include?('new title') - refute json_metadata.keys.include?('the_title') + User.with_current_user(@person.user) do + @sample_type.sample_attributes.first.update!(title: 'new title') + attribute_change_maps = [{id: @sample_type.sample_attributes.first.id, old_title: 'the_title', new_title: 'new title' }] + assert_equal @sample_type.sample_attributes.first.title, 'new title' + refute_equal @sample_type.sample_attributes.first.title, 'the_title' + UpdateSampleMetadataJob.new.perform(@sample_type, attribute_change_maps, @person.user) + @sample_type.samples.each do |sample| + json_metadata = JSON.parse sample.json_metadata + assert json_metadata.keys.include?('new title') + refute json_metadata.keys.include?('the_title') + end end end end From 60763a12f16ce9bdec0f59d045d6f98acef2f1cb Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 22 Oct 2024 12:06:11 +0200 Subject: [PATCH 25/49] Split test in optional and mandatory test --- test/unit/sample_type_test.rb | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/test/unit/sample_type_test.rb b/test/unit/sample_type_test.rb index d03b2b5eb6..d1a91cb5c8 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -901,10 +901,18 @@ def setup assert sample_type.valid? - # Adding attribute - sample_type.sample_attributes.build(title: 'test 123') + # Adding optional attribute + sample_type.sample_attributes.build(title: 'optional test 123', sample_attribute_type: FactoryBot.create(:string_sample_attribute_type), required: false, is_title: false, linked_sample_type: nil) + assert sample_type.valid? + assert sample_type.errors.none? + + sample_type.reload + assert sample_type.valid? + + # Adding mandatory attribute + sample_type.sample_attributes.build(title: 'mandatory test 123', sample_attribute_type: FactoryBot.create(:string_sample_attribute_type), required: true, is_title: false, linked_sample_type: nil) refute sample_type.valid? - assert sample_type.errors.added?(:sample_attributes, 'cannot be added, new attributes are not allowed (test 123)') + assert sample_type.errors.added?(:'sample_attributes.required', 'cannot be changed (mandatory test 123)') sample_type.reload assert sample_type.valid? From d107a7b3326acf8fd75b0b4a23be04c9affe44ac Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 22 Oct 2024 17:01:37 +0200 Subject: [PATCH 26/49] check if samples instead of returning false --- lib/seek/samples/sample_type_editing_constraints.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index bf298d2bd8..9ca6dbd8ca 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -27,7 +27,7 @@ def allow_required?(attr) attr = attr.accessor_name end - return false unless attr + return !samples? unless attr return !blanks?(attr) if samples? true @@ -56,7 +56,7 @@ def allow_name_change?(attr) attr = attr.accessor_name end - return false unless attr + return !samples? unless attr return samples.all?(&:can_edit?) if samples? true From 07da6a16ebc5e0c4c18620b6af53549cf8b2d823 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 22 Oct 2024 17:01:59 +0200 Subject: [PATCH 27/49] test should fail if samples --- test/unit/samples/sample_type_editing_constraints_test.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/unit/samples/sample_type_editing_constraints_test.rb b/test/unit/samples/sample_type_editing_constraints_test.rb index 05bb63423e..e16a9912d7 100644 --- a/test/unit/samples/sample_type_editing_constraints_test.rb +++ b/test/unit/samples/sample_type_editing_constraints_test.rb @@ -32,7 +32,8 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'address' } refute_nil attr refute c.allow_name_change?(attr) - assert c.allow_name_change?(nil) + # Not OK if attribute = nil and the sample type has samples + refute c.allow_name_change?(nil) # OK if there are no samples st2 = FactoryBot.create(:simple_sample_type) @@ -41,6 +42,7 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'the_title' } refute_nil attr assert c.allow_name_change?(attr) + # OK if attribute = nil and the sample type has no samples assert c.allow_name_change?(nil) # OK if user has editing permission over all samples @@ -51,7 +53,8 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'address' } refute_nil attr assert c.allow_name_change?(attr) - assert c.allow_name_change?(nil) + # Not OK if attribute = nil and the sample type has samples + refute c.allow_name_change?(nil) end end From 2dadb38d1d42b56facf05269d00e7f446cb23736 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 24 Oct 2024 10:10:54 +0200 Subject: [PATCH 28/49] Remove `allow_name_change?` completely --- app/models/sample_attribute.rb | 2 - .../_sample_attribute_form.html.erb | 6 +- .../sample_type_editing_constraints.rb | 13 --- .../sample_types_controller_test.rb | 101 ------------------ test/unit/sample_type_test.rb | 8 -- .../sample_type_editing_constraints_test.rb | 38 ------- 6 files changed, 2 insertions(+), 166 deletions(-) diff --git a/app/models/sample_attribute.rb b/app/models/sample_attribute.rb index 307f5b5cb6..f2c62cd237 100644 --- a/app/models/sample_attribute.rb +++ b/app/models/sample_attribute.rb @@ -100,8 +100,6 @@ def validate_against_editing_constraints attr_title = self.new_record? ? title : title_was error_message = "cannot be changed (#{attr_title})" # Use pre-change title in error message. - errors.add(:title, error_message) if title_changed? && !c.allow_name_change?(self) - unless c.allow_required?(self) errors.add(:is_title, error_message) if is_title_changed? errors.add(:required, error_message) if required_changed? diff --git a/app/views/sample_types/_sample_attribute_form.html.erb b/app/views/sample_types/_sample_attribute_form.html.erb index 0696353b6d..6ccedf1c0a 100644 --- a/app/views/sample_types/_sample_attribute_form.html.erb +++ b/app/views/sample_types/_sample_attribute_form.html.erb @@ -19,7 +19,6 @@ <% allow_required = constraints.allow_required?(sample_attribute) %> <% allow_attribute_removal = constraints.allow_attribute_removal?(sample_attribute) %> <% allow_type_change = constraints.allow_type_change?(sample_attribute) %> -<% allow_name_change = constraints.allow_name_change?(sample_attribute) %> <% seek_sample_multi = sample_attribute&.seek_sample_multi? %> <% link_to_self = sample_attribute && sample_attribute.deferred_link_to_self %> <% display_isa_tag ||= false %> @@ -31,7 +30,6 @@ <% allow_isa_tag_change = constraints.allow_isa_tag_change?(sample_attribute) if display_isa_tag %> <% required_title_text = allow_required ? nil : "You are not allowed to make this attribute required.\nSome samples have no value for this attribute." %> <% is_title_title_text = allow_required ? nil : "You are not allowed to change this attribute's title field.\nSome samples have no value for this attribute." %> -<% change_name_text = allow_name_change ? nil : "You are not allowed to change the name of this attribute.\nYou need at least editing permission to all samples in this sample type." %> @@ -43,8 +41,8 @@ - <%= text_field_tag "#{field_name_prefix}[title]", title, class: 'form-control', title: change_name_text, - placeholder: 'Attribute name', disabled: !allow_name_change, data: { attr: "title" } %> + <%= text_field_tag "#{field_name_prefix}[title]", title, class: 'form-control', + placeholder: 'Attribute name', data: { attr: "title" } %> diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index 9ca6dbd8ca..d4fab0aabc 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -49,19 +49,6 @@ def allow_attribute_removal?(attr) end end - # whether the name of the attribute can be changed - def allow_name_change?(attr) - if attr.is_a?(SampleAttribute) - return true if attr.new_record? - - attr = attr.accessor_name - end - return !samples? unless attr - return samples.all?(&:can_edit?) if samples? - - true - end - # whether the type for the attribute can be changed def allow_type_change?(attr) if attr.is_a?(SampleAttribute) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 59fca0173c..db48932a35 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -677,34 +677,6 @@ class SampleTypesControllerTest < ActionController::TestCase end end - test 'validates changes against editing constraints' do - other_person = FactoryBot.create(:person) - other_person.add_to_project_and_institution(@project, @person.institutions.first) - @sample_type.samples.create!(data: { the_title: 'Just look at me!' }, sample_type: @sample_type, - project_ids: @project_ids, policy: FactoryBot.create(:private_policy)) - - login_as other_person - - @sample_type.samples.create!(data: { the_title: 'Don\'t look at me!' }, sample_type: @sample_type, - project_ids: @project_ids, policy: FactoryBot.create(:private_policy)) - - login_as @person - get :edit, params: { id: @sample_type } - assert_response :success - assert_select 'input#sample_type_sample_attributes_attributes_0_title[disabled=?]', 'disabled' - assert_no_difference('ActivityLog.count') do - put :update, params: { id: @sample_type, sample_type: { - sample_attributes_attributes: { - '0' => { id: @sample_type.sample_attributes.first.id, pos: '1', title: 'banana', required: '1' } - } - } } - end - - assert_response :unprocessable_entity - assert_select 'div#error_explanation' do - assert_select 'ul > li', text: 'Sample attributes title cannot be changed (the_title)' - end - end test 'Should not be allowed to show the manage page of ISA-JSON compliant sample type' do with_config_value(:isa_json_compliance_enabled, true) do @@ -799,79 +771,6 @@ class SampleTypesControllerTest < ActionController::TestCase end - test 'change attribute name' do - other_person = FactoryBot.create(:person) - st_policy = FactoryBot.create(:private_policy, - permissions: [FactoryBot.create(:manage_permission, contributor: other_person)]) - sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person, - policy_id: st_policy.id) - sample_type.sample_attributes << FactoryBot.create(:sample_attribute, title: 'new title', - sample_attribute_type: @string_type, is_title: false, sample_type: sample_type) - assert_equal sample_type.sample_attributes.last.title, 'new title' - login_as(@person) - - # Scenario 1: Sample type does not have any samples - # @person can change the attribute title - get :edit, params: { id: sample_type.id } - assert_response :success - assert_select 'tr.sample-attribute:has(input[data-attr="title"][value=?])', 'new title' do |tr| - assert_select tr, 'input[data-attr="title"]' do |input| - assert_select input, '[disabled]', count: 0 - assert_select input, '[title]', count: 0 - end - end - - # Scenario 2: Sample type has samples and @person has permission to edit all samples - # @person can change the attribute title - (1..10).map do |i| - editing_policy = FactoryBot.create(:private_policy, - permissions: [FactoryBot.create(:edit_permission, contributor: @person), - FactoryBot.create(:manage_permission, contributor: other_person)]) - FactoryBot.create(:sample, data: { 'new title': "new title sample #{i}" }, - contributor: other_person, project_ids: @project_ids, sample_type: sample_type, - policy_id: editing_policy.id) - end - sample_type.reload - get :edit, params: { id: sample_type.id } - assert_response :success - assert_equal sample_type.samples.count, 10 - assert(sample_type.samples.all? { |sample| sample.can_edit?(@person) }) - - assert_select 'tr.sample-attribute:has(input[data-attr="title"][value=?])', 'new title' do |tr| - assert_select tr, 'input[data-attr="title"]' do |input| - assert_select input, '[disabled]', count: 0 - assert_select input, '[title]', count: 0 - end - end - - - # Scenario 3: Sample type has samples but @person does not have permission to edit all samples - # @person cannot change the attribute title - (1..10).map do |i| - viewing_policy = FactoryBot.create(:private_policy, - permissions: [FactoryBot.create(:permission, contributor: @person), - FactoryBot.create(:manage_permission, contributor: other_person)]) - FactoryBot.create(:sample, data: { 'new title': "new title sample #{i}" }, - contributor: other_person, - project_ids: @project_ids, sample_type: sample_type, policy_id: viewing_policy.id) - end - - sample_type.reload - get :edit, params: { id: sample_type.id } - assert_response :success - assert_equal sample_type.samples.count, 20 - refute(sample_type.samples.all? { |sample| sample.can_edit?(@person) }) - - assert_select 'tr.sample-attribute:has(input[data-attr="title"][value=?])', 'new title' do |tr| - assert_select tr, 'input[data-attr="title"]' do |input| - assert_equal input.attr('disabled').text, 'disabled' - assert_equal input.attr('title').text, - "You are not allowed to change the name of this attribute.\nYou need at least editing permission to all samples in this sample type." - end - end - - end - private def template_for_upload diff --git a/test/unit/sample_type_test.rb b/test/unit/sample_type_test.rb index d1a91cb5c8..4c05e152ba 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -925,14 +925,6 @@ def setup sample_type.reload assert sample_type.valid? - # Changing attribute title - sample_type.sample_attributes.last.title = 'banana' - refute sample_type.valid? - assert sample_type.errors.added?(:'sample_attributes.title', 'cannot be changed (patient)') - - sample_type.reload - assert sample_type.valid? - # Changing "required" attribute User.with_current_user(@person.user) do sample_type.samples.create!(data: { title: 'Lib-5', patient: nil }, sample_type: sample_type, diff --git a/test/unit/samples/sample_type_editing_constraints_test.rb b/test/unit/samples/sample_type_editing_constraints_test.rb index e16a9912d7..e21137af0d 100644 --- a/test/unit/samples/sample_type_editing_constraints_test.rb +++ b/test/unit/samples/sample_type_editing_constraints_test.rb @@ -20,44 +20,6 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase refute c.samples? end - test 'allow title change?' do - person = FactoryBot.create(:person) - - User.with_current_user(person.user) do - # Not OK if te user doesn't have editing permission over all samples - st1 = sample_type_with_samples - refute st1.samples.all?(&:can_edit?) - c = Seek::Samples::SampleTypeEditingConstraints.new(st1) - refute c.allow_name_change?(:address) - attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'address' } - refute_nil attr - refute c.allow_name_change?(attr) - # Not OK if attribute = nil and the sample type has samples - refute c.allow_name_change?(nil) - - # OK if there are no samples - st2 = FactoryBot.create(:simple_sample_type) - c = Seek::Samples::SampleTypeEditingConstraints.new(st2) - assert c.allow_name_change?(:the_title) - attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'the_title' } - refute_nil attr - assert c.allow_name_change?(attr) - # OK if attribute = nil and the sample type has no samples - assert c.allow_name_change?(nil) - - # OK if user has editing permission over all samples - st3 = sample_type_with_samples(person) - assert st3.samples.all?(&:can_edit?) - c = Seek::Samples::SampleTypeEditingConstraints.new(st3) - assert c.allow_name_change?(:address) - attr = c.sample_type.sample_attributes.detect { |t| t.accessor_name == 'address' } - refute_nil attr - assert c.allow_name_change?(attr) - # Not OK if attribute = nil and the sample type has samples - refute c.allow_name_change?(nil) - end - end - test 'allow type change?' do c = Seek::Samples::SampleTypeEditingConstraints.new(sample_type_with_samples) refute c.allow_type_change?(:address) From ed0d6bdb4ff7fb26280188ee13098ff45726b26f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 28 Oct 2024 08:53:57 +0100 Subject: [PATCH 29/49] Revert deletion of `allow_new_attrbute?` According to [comment in PR](https://github.com/seek4science/seek/pull/2032#discussion_r1813137258). --- app/views/isa_studies/_sample_types_form.html.erb | 2 +- lib/seek/samples/sample_type_editing_constraints.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/views/isa_studies/_sample_types_form.html.erb b/app/views/isa_studies/_sample_types_form.html.erb index 03a874e7fc..910f300b7f 100644 --- a/app/views/isa_studies/_sample_types_form.html.erb +++ b/app/views/isa_studies/_sample_types_form.html.erb @@ -52,7 +52,7 @@ display_isa_tag: true } %> <% end %> - <% unless sample_type.uploaded_template? %> + <% unless sample_type.uploaded_template? || !sample_type.editing_constraints.allow_new_attribute? %> > <%= button_link_to('Add new attribute', 'add', '#', id: add_attribute_id ) %> diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index d4fab0aabc..aab7fa449e 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -49,6 +49,12 @@ def allow_attribute_removal?(attr) end end + # This method was left in so it can be changed in the future + # Currently, it always returns true + # see https://github.com/seek4science/seek/pull/2032#discussion_r1813137258 + def allow_new_attribute? + true + end # whether the type for the attribute can be changed def allow_type_change?(attr) if attr.is_a?(SampleAttribute) From 520beb029afefb64bdb6b1ce2d9fc8e4b79962a2 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 28 Oct 2024 09:43:41 +0100 Subject: [PATCH 30/49] Update sample metadata in batches --- app/jobs/update_sample_metadata_job.rb | 6 ++++-- lib/seek/samples/sample_metadata_updater.rb | 10 +++++----- test/unit/jobs/update_sample_metadata_job_test.rb | 5 +++-- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index d4bc420f98..d34304ac5d 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -4,7 +4,9 @@ class UpdateSampleMetadataJob < ApplicationJob queue_with_priority 1 queue_as QueueNames::SAMPLES - def perform(sample_type, attribute_changes = [], user) - Seek::Samples::SampleMetadataUpdater.new(sample_type, attribute_changes, user).update_metadata + def perform(sample_type, user, attribute_changes = []) + sample_type.samples.in_batches(of: 1000) do |samples| + Seek::Samples::SampleMetadataUpdater.new(samples, user, attribute_changes).update_metadata + end end end diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index 2aa195f9fe..1bdc162d9e 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -7,17 +7,17 @@ class SampleMetadataUpdateException < StandardError; end # Class to handle the updating of sample metadata after updating Sample type class SampleMetadataUpdater - def initialize(sample_type, attribute_changes, user) - @sample_type = sample_type - @attribute_change_maps = attribute_changes + def initialize(samples, user, attribute_changes) + @samples = samples @user = user + @attribute_change_maps = attribute_changes end def update_metadata - return if @attribute_change_maps.blank? || @sample_type.samples.blank? || @sample_type.nil? || @user.nil? + return if @attribute_change_maps.blank? || @samples.blank? || @user.nil? User.with_current_user(@user) do - @sample_type.samples.each do |sample| + @samples.each do |sample| metadata = JSON.parse(sample.json_metadata) # Update the metadata keys with the new attribute titles @attribute_change_maps.each do |change| diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index d465b04e4f..c12b192c98 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -18,7 +18,7 @@ def teardown test 'perform' do User.with_current_user(@person.user) do - UpdateSampleMetadataJob.new.perform(@sample_type, [], @person.user) + UpdateSampleMetadataJob.new.perform(@sample_type, @person.user, []) end end @@ -28,7 +28,8 @@ def teardown attribute_change_maps = [{id: @sample_type.sample_attributes.first.id, old_title: 'the_title', new_title: 'new title' }] assert_equal @sample_type.sample_attributes.first.title, 'new title' refute_equal @sample_type.sample_attributes.first.title, 'the_title' - UpdateSampleMetadataJob.new.perform(@sample_type, attribute_change_maps, @person.user) + UpdateSampleMetadataJob.perform_now(@sample_type, @person.user, attribute_change_maps) + @sample_type.reload @sample_type.samples.each do |sample| json_metadata = JSON.parse sample.json_metadata assert json_metadata.keys.include?('new title') From 79ac49d8b8c71396f8074c08a357e4e40f83bf7f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 28 Oct 2024 10:29:00 +0100 Subject: [PATCH 31/49] Add lock to cache --- app/jobs/update_sample_metadata_job.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index d34304ac5d..2043a89a8e 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -5,8 +5,13 @@ class UpdateSampleMetadataJob < ApplicationJob queue_as QueueNames::SAMPLES def perform(sample_type, user, attribute_changes = []) - sample_type.samples.in_batches(of: 1000) do |samples| - Seek::Samples::SampleMetadataUpdater.new(samples, user, attribute_changes).update_metadata + begin + Rails.cache.write("sample_type_lock_#{sample_type.id}", true, expires_in: 1.hour) + sample_type.lock.samples.in_batches(of: 1000) do |samples| + Seek::Samples::SampleMetadataUpdater.new(samples, user, attribute_changes).update_metadata + end end + ensure + Rails.cache.delete("sample_type_lock_#{sample_type.id}") end end From 45daffdb303adb160cecf499a68b9c6a62abd7ea Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 28 Oct 2024 15:30:09 +0100 Subject: [PATCH 32/49] Update in batches --- lib/seek/samples/sample_metadata_updater.rb | 32 ++++++++++++++------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index 1bdc162d9e..df8d9a991b 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -7,25 +7,37 @@ class SampleMetadataUpdateException < StandardError; end # Class to handle the updating of sample metadata after updating Sample type class SampleMetadataUpdater - def initialize(samples, user, attribute_changes) - @samples = samples + def initialize(sample_type, user, attribute_changes) + @sample_type = sample_type @user = user @attribute_change_maps = attribute_changes end def update_metadata - return if @attribute_change_maps.blank? || @samples.blank? || @user.nil? + return if @attribute_change_maps.blank? || @sample_type.blank? || @user.nil? || @sample_type.samples.blank? - User.with_current_user(@user) do - @samples.each do |sample| - metadata = JSON.parse(sample.json_metadata) - # Update the metadata keys with the new attribute titles - @attribute_change_maps.each do |change| - metadata[change[:new_title]] = metadata.delete(change[:old_title]) + begin + User.with_current_user(@user) do + @sample_type.with_lock do + @sample_type.samples.in_batches(of: 1000).each do |batch| + batch.each do |sample| + metadata = JSON.parse(sample.json_metadata) + # Update the metadata keys with the new attribute titles + @attribute_change_maps.each do |change| + metadata[change[:new_title]] = metadata.delete(change[:old_title]) + end + sample.update_column(:json_metadata, metadata.to_json) + end + end end - sample.update_column(:json_metadata, metadata.to_json) end end + ensure + Rails.cache.delete("sample_type_lock_#{@sample_type.id}") + end + + def raise_sample_metadata_update_exception + raise SampleMetadataUpdateException.new('An unexpected error occurred while updating the sample metadata') end end end From acb2d44385efed1d6938c47f7da6e333573f7ed5 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 28 Oct 2024 15:31:41 +0100 Subject: [PATCH 33/49] Add caching for locking record --- app/controllers/sample_types_controller.rb | 11 +++ app/jobs/update_sample_metadata_job.rb | 8 +- app/models/sample_type.rb | 11 ++- app/views/sample_types/_form.html.erb | 6 ++ lib/seek/samples/sample_metadata_updater.rb | 1 + .../sample_types_controller_test.rb | 86 +++++++++++++++++++ .../jobs/update_sample_metadata_job_test.rb | 1 + test/unit/sample_type_test.rb | 14 +++ 8 files changed, 136 insertions(+), 2 deletions(-) diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index 73932dcfcf..3ad7258d4d 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -9,6 +9,7 @@ class SampleTypesController < ApplicationController before_action :find_and_authorize_requested_item, except: %i[create batch_upload index new template_details] before_action :find_sample_type, only: %i[batch_upload template_details] before_action :check_isa_json_compliance, only: %i[edit update manage manage_update] + before_action :check_if_locked, only: %i[edit manage update manage_update] before_action :find_assets, only: [:index] before_action :auth_to_create, only: %i[new create] before_action :project_membership_required, only: %i[create new select filter_for_select] @@ -223,4 +224,14 @@ def update_sample_json_metadata UpdateSampleMetadataJob.perform_later(@sample_type, attribute_changes, @current_user) end + + def check_if_locked + return unless @sample_type.locked? + return if @current_user&.is_admin? + + error_message = 'This sample type is locked and cannot be edited right now.' + flash[:error] = error_message + @sample_type.errors.add(:base, error_message) + redirect_to @sample_type + end end diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index 2043a89a8e..c13eb93d27 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -5,10 +5,16 @@ class UpdateSampleMetadataJob < ApplicationJob queue_as QueueNames::SAMPLES def perform(sample_type, user, attribute_changes = []) + Rails.cache.write("sample_type_lock_#{sample_type.id}", true, expires_in: 1.hour) + + Seek::Samples::SampleMetadataUpdater.new(sample_type, user, attribute_changes).update_metadata + end + + def perform_with_error(sample_type, user, attribute_changes = []) begin Rails.cache.write("sample_type_lock_#{sample_type.id}", true, expires_in: 1.hour) sample_type.lock.samples.in_batches(of: 1000) do |samples| - Seek::Samples::SampleMetadataUpdater.new(samples, user, attribute_changes).update_metadata + Seek::Samples::SampleMetadataUpdater.new(samples, user, attribute_changes).raise_sample_metadata_update_exception end end ensure diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 46d44fd500..a75293cf75 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -54,7 +54,8 @@ class SampleType < ApplicationRecord :validate_attribute_title_unique, :validate_attribute_accessor_names_unique, :validate_title_is_not_type_of_seek_sample_multi, - :validate_against_editing_constraints + :validate_against_editing_constraints, + :validate_sample_type_is_not_locked validates :projects, presence: true, projects: { self: true } accepts_nested_attributes_for :sample_attributes, allow_destroy: true @@ -110,6 +111,10 @@ def is_isa_json_compliant? (studies.any? || assays.any?) && has_only_isa_json_compliant_investigations && !isa_template.nil? end + def locked? + Rails.cache.fetch("sample_type_lock_#{id}").present? + end + def validate_value?(attribute_name, value) attribute = sample_attributes.detect { |attr| attr.title == attribute_name } raise UnknownAttributeException, "Unknown attribute #{attribute_name}" unless attribute @@ -247,6 +252,10 @@ def validate_against_editing_constraints end end + def validate_sample_type_is_not_locked + errors.add(:base, 'This sample type is locked and cannot be edited right now.') if locked? + end + def attribute_search_terms attribute_titles end diff --git a/app/views/sample_types/_form.html.erb b/app/views/sample_types/_form.html.erb index 1be3c88ab6..92220f151c 100644 --- a/app/views/sample_types/_form.html.erb +++ b/app/views/sample_types/_form.html.erb @@ -1,5 +1,11 @@ <% tab ||= 'manual' %> +<% if @sample_type.locked? %> +
+ Warning! This sample type is locked and should not be edited. +
+<% end %> + <%= sample_controlled_vocab_model_dialog('cv-modal') %> diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index df8d9a991b..f9a82c6ab0 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -15,6 +15,7 @@ def initialize(sample_type, user, attribute_changes) def update_metadata return if @attribute_change_maps.blank? || @sample_type.blank? || @user.nil? || @sample_type.samples.blank? + return unless @sample_type.locked? begin User.with_current_user(@user) do diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index db48932a35..14813cd073 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -5,6 +5,7 @@ class SampleTypesControllerTest < ActionController::TestCase include AuthenticatedTestHelper setup do + Rails.cache.clear FactoryBot.create(:person) # to prevent person being first person and therefore admin @person = FactoryBot.create(:project_administrator) @project = @person.projects.first @@ -17,6 +18,10 @@ class SampleTypesControllerTest < ActionController::TestCase @controlled_vocab_type = FactoryBot.create(:controlled_vocab_attribute_type) end + teardown do + Rails.cache.clear + end + test 'should get index' do get :index assert_response :success @@ -771,6 +776,87 @@ class SampleTypesControllerTest < ActionController::TestCase end + test 'check if sample type is locked' do + sys_admin = FactoryBot.create(:admin) + + # Add managing permission to the instance admin + @sample_type.policy.permissions << FactoryBot.create(:permission, contributor: sys_admin, access_type: Policy::MANAGING) + refute @sample_type.locked? + + login_as(@person) + + %i[edit manage].each do |action| + get action, params: { id: @sample_type.id } + assert_nil flash[:error] + assert_response :success + end + + # Locking the sample type + Rails.cache.write("sample_type_lock_#{@sample_type.id}", true) + assert @sample_type.locked? + + %i[edit manage].each do |action| + get action, params: { id: @sample_type.id } + assert_redirected_to sample_type_path(@sample_type) + assert_equal flash[:error], 'This sample type is locked and cannot be edited right now.' + end + + login_as(sys_admin) + %i[edit manage].each do |action| + get action, params: { id: @sample_type.id } + assert_nil flash[:error] + assert_response :success + end + end + + test 'update a locked sample type' do + sys_admin = FactoryBot.create(:admin) + other_person = FactoryBot.create(:person) + sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) + sample_type.policy.permissions << FactoryBot.create(:permission, contributor: sys_admin, access_type: Policy::MANAGING) + sample_type.policy.permissions << FactoryBot.create(:permission, contributor: other_person, access_type: Policy::MANAGING) + + (1..10).map do |_i| + FactoryBot.create(:sample, contributor: @person, project_ids: @project_ids, sample_type: sample_type) + end + + login_as(@person) + + refute @sample_type.locked? + + patch :update, params: { id: sample_type.id, sample_type: { + sample_attributes_attributes: { + '0': { id: sample_type.sample_attributes.detect(&:is_title), title: 'new title' } + } + } } + assert_redirected_to sample_type_path(sample_type) + assert sample_type.locked? + + login_as(other_person) + + patch :update, params: { id: sample_type.id, sample_type: { + sample_attributes_attributes: { + '0': { id: sample_type.sample_attributes.detect(&:is_title), title: 'new title' } + } + } } + sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') + + assert_redirected_to sample_type_path(sample_type) + assert(sample_type.locked?) + assert_equal flash[:error], 'This sample type is locked and cannot be edited right now.' + + login_as(sys_admin) + patch :update, params: { id: sample_type.id, sample_type: { + sample_attributes_attributes: { + '0': { id: sample_type.sample_attributes.detect(&:is_title), title: 'new title' } + } + } } + + assert(sample_type.locked?) + sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') + assert_nil flash[:error] + end + private def template_for_upload diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index c12b192c98..40323086ed 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -29,6 +29,7 @@ def teardown assert_equal @sample_type.sample_attributes.first.title, 'new title' refute_equal @sample_type.sample_attributes.first.title, 'the_title' UpdateSampleMetadataJob.perform_now(@sample_type, @person.user, attribute_change_maps) + refute Rails.cache.fetch("sample_type_lock_#{@sample_type.id}") @sample_type.reload @sample_type.samples.each do |sample| json_metadata = JSON.parse sample.json_metadata diff --git a/test/unit/sample_type_test.rb b/test/unit/sample_type_test.rb index 4c05e152ba..38ecff3092 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -6,6 +6,11 @@ def setup @person = FactoryBot.create(:person) @project = @person.projects.first @project_ids = [@project.id] + Rails.cache.clear + end + + def teardown + Rails.cache.clear end test 'validation' do @@ -1055,6 +1060,15 @@ def setup end end + test 'sample type is locked?' do + sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) + refute sample_type.locked? + Rails.cache.write("sample_type_lock_#{sample_type.id}", true) + assert sample_type.locked? + refute sample_type.valid? + assert sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') + end + private # sample type with 3 samples From 6f3c38219d4bda139b0c2b74f55236bd2d86e596 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 14:23:32 +0100 Subject: [PATCH 34/49] Simplify --- app/jobs/update_sample_metadata_job.rb | 25 +++++++++++++------------ app/models/sample_type.rb | 4 ++++ test/unit/sample_type_test.rb | 2 +- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index c13eb93d27..cd55e015e8 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -4,20 +4,21 @@ class UpdateSampleMetadataJob < ApplicationJob queue_with_priority 1 queue_as QueueNames::SAMPLES - def perform(sample_type, user, attribute_changes = []) - Rails.cache.write("sample_type_lock_#{sample_type.id}", true, expires_in: 1.hour) + attr_accessor :sample_type, :user, :attribute_changes + + before_enqueue do |job| + SampleType.find(job.arguments[0].id).set_lock + end - Seek::Samples::SampleMetadataUpdater.new(sample_type, user, attribute_changes).update_metadata + before_perform do |job| + SampleType.find(job.arguments[0].id).set_lock end - def perform_with_error(sample_type, user, attribute_changes = []) - begin - Rails.cache.write("sample_type_lock_#{sample_type.id}", true, expires_in: 1.hour) - sample_type.lock.samples.in_batches(of: 1000) do |samples| - Seek::Samples::SampleMetadataUpdater.new(samples, user, attribute_changes).raise_sample_metadata_update_exception - end - end - ensure - Rails.cache.delete("sample_type_lock_#{sample_type.id}") + def perform(sample_type, user, attribute_changes = []) + @sample_type = sample_type + @user = user + @attribute_changes = attribute_changes + + Seek::Samples::SampleMetadataUpdater.new(@sample_type, @user, @attribute_changes).update_metadata end end diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index a75293cf75..da6286b4f6 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -115,6 +115,10 @@ def locked? Rails.cache.fetch("sample_type_lock_#{id}").present? end + def set_lock + Rails.cache.write("sample_type_lock_#{id}", true, expires_in: 1.hour) + end + def validate_value?(attribute_name, value) attribute = sample_attributes.detect { |attr| attr.title == attribute_name } raise UnknownAttributeException, "Unknown attribute #{attribute_name}" unless attribute diff --git a/test/unit/sample_type_test.rb b/test/unit/sample_type_test.rb index 38ecff3092..19c8658022 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -1063,7 +1063,7 @@ def teardown test 'sample type is locked?' do sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) refute sample_type.locked? - Rails.cache.write("sample_type_lock_#{sample_type.id}", true) + sample_type.set_lock assert sample_type.locked? refute sample_type.valid? assert sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') From fea3836db5095e5187d9d508c0611962576fd75e Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 14:24:35 +0100 Subject: [PATCH 35/49] Tests --- .../sample_types_controller_test.rb | 2 +- .../jobs/update_sample_metadata_job_test.rb | 39 +++++++++++++++++-- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 14813cd073..ca1f5f4140 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -792,7 +792,7 @@ class SampleTypesControllerTest < ActionController::TestCase end # Locking the sample type - Rails.cache.write("sample_type_lock_#{@sample_type.id}", true) + @sample_type.set_lock assert @sample_type.locked? %i[edit manage].each do |action| diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index 40323086ed..0ccfe05e51 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -10,15 +10,33 @@ def setup (1..10).each do |_i| FactoryBot.create(:sample, sample_type: @sample_type, contributor: @person) end + Rails.cache.clear end def teardown - # Do nothing + Rails.cache.clear end test 'perform' do User.with_current_user(@person.user) do - UpdateSampleMetadataJob.new.perform(@sample_type, @person.user, []) + UpdateSampleMetadataJob.perform_now(@sample_type, @person.user, []) + end + end + + test 'enqueue' do + # Simulate the lock in the cache from the controller + job = UpdateSampleMetadataJob.new(@sample_type, @person.user, []) + User.with_current_user(@person.user) do + assert_enqueued_with(job: UpdateSampleMetadataJob, args: [@sample_type, @person.user, []]) do + job.enqueue + end + assert @sample_type.locked? + + perform_enqueued_jobs do + job.perform_now + end + + refute @sample_type.locked? end end @@ -29,7 +47,7 @@ def teardown assert_equal @sample_type.sample_attributes.first.title, 'new title' refute_equal @sample_type.sample_attributes.first.title, 'the_title' UpdateSampleMetadataJob.perform_now(@sample_type, @person.user, attribute_change_maps) - refute Rails.cache.fetch("sample_type_lock_#{@sample_type.id}") + refute @sample_type.locked? @sample_type.reload @sample_type.samples.each do |sample| json_metadata = JSON.parse sample.json_metadata @@ -38,4 +56,19 @@ def teardown end end end + + test 'perform with unexpected error' do + job = UpdateSampleMetadataJob.new(@sample_type, @person.user, 'bad_attribute_change_map') + assert_enqueued_with(job: UpdateSampleMetadataJob, args: [@sample_type, @person.user, 'bad_attribute_change_map']) do + job.enqueue + end + assert @sample_type.locked? + + perform_enqueued_jobs do + job.perform_now + rescue StandardError + # The sample type should be unlocked even if an error occurs + refute @sample_type.locked? + end + end end From 9216b4fc375913595056f94c5fc8c9483c1d4caf Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 14:29:44 +0100 Subject: [PATCH 36/49] Remove redundant begin block --- lib/seek/samples/sample_metadata_updater.rb | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index f9a82c6ab0..d4315c7cf2 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -17,22 +17,21 @@ def update_metadata return if @attribute_change_maps.blank? || @sample_type.blank? || @user.nil? || @sample_type.samples.blank? return unless @sample_type.locked? - begin - User.with_current_user(@user) do - @sample_type.with_lock do - @sample_type.samples.in_batches(of: 1000).each do |batch| - batch.each do |sample| - metadata = JSON.parse(sample.json_metadata) - # Update the metadata keys with the new attribute titles - @attribute_change_maps.each do |change| - metadata[change[:new_title]] = metadata.delete(change[:old_title]) - end - sample.update_column(:json_metadata, metadata.to_json) + User.with_current_user(@user) do + @sample_type.with_lock do + @sample_type.samples.in_batches(of: 1000).each do |batch| + batch.each do |sample| + metadata = JSON.parse(sample.json_metadata) + # Update the metadata keys with the new attribute titles + @attribute_change_maps.each do |change| + metadata[change[:new_title]] = metadata.delete(change[:old_title]) end + sample.update_column(:json_metadata, metadata.to_json) end end end end + ensure Rails.cache.delete("sample_type_lock_#{@sample_type.id}") end From cde9a256f8026bf6947c7c102755b74ba01b5702 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 14:47:39 +0100 Subject: [PATCH 37/49] Lock sample during update job --- lib/seek/samples/sample_metadata_updater.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index d4315c7cf2..f62157b7fe 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -21,17 +21,19 @@ def update_metadata @sample_type.with_lock do @sample_type.samples.in_batches(of: 1000).each do |batch| batch.each do |sample| - metadata = JSON.parse(sample.json_metadata) - # Update the metadata keys with the new attribute titles - @attribute_change_maps.each do |change| - metadata[change[:new_title]] = metadata.delete(change[:old_title]) + sample.with_lock do + metadata = JSON.parse(sample.json_metadata) + # Update the metadata keys with the new attribute titles + @attribute_change_maps.each do |change| + metadata[change[:new_title]] = metadata.delete(change[:old_title]) + end + sample.update_column(:json_metadata, metadata.to_json) end - sample.update_column(:json_metadata, metadata.to_json) end end end end - + ensure Rails.cache.delete("sample_type_lock_#{@sample_type.id}") end From 3d36b852241a7df42aa2de00f0c79db8e7483211 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 15:28:47 +0100 Subject: [PATCH 38/49] Move warning to show page --- app/views/sample_types/_buttons.html.erb | 2 +- app/views/sample_types/_form.html.erb | 6 ------ .../sample_types/select/_filtered_sample_types.html.erb | 2 +- app/views/sample_types/show.html.erb | 6 +++++- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/views/sample_types/_buttons.html.erb b/app/views/sample_types/_buttons.html.erb index 1628f0282e..bb8c3e9885 100644 --- a/app/views/sample_types/_buttons.html.erb +++ b/app/views/sample_types/_buttons.html.erb @@ -4,7 +4,7 @@ <%= button_link_to("View samples", 'view_samples', sample_type_samples_path(sample_type)) %> <% end %> -<% if Sample.can_create? %> +<% if Sample.can_create? && !sample_type.locked? %> <%= button_link_to("Create sample", 'new', new_sample_path(sample_type_id:sample_type.id)) %> <% end %> diff --git a/app/views/sample_types/_form.html.erb b/app/views/sample_types/_form.html.erb index 92220f151c..1be3c88ab6 100644 --- a/app/views/sample_types/_form.html.erb +++ b/app/views/sample_types/_form.html.erb @@ -1,11 +1,5 @@ <% tab ||= 'manual' %> -<% if @sample_type.locked? %> -
- Warning! This sample type is locked and should not be edited. -
-<% end %> - <%= sample_controlled_vocab_model_dialog('cv-modal') %> diff --git a/app/views/sample_types/select/_filtered_sample_types.html.erb b/app/views/sample_types/select/_filtered_sample_types.html.erb index 5b669903e8..f38b2dc5a3 100644 --- a/app/views/sample_types/select/_filtered_sample_types.html.erb +++ b/app/views/sample_types/select/_filtered_sample_types.html.erb @@ -7,7 +7,7 @@ <%= render :partial => "assets/resource_list_item", :object => sample_type %> - <% if Sample.can_create? %> + <% if Sample.can_create? && !sample_type.locked? %>
<%= link_to "New sample", new_sample_path(:sample_type_id => sample_type.id, project_ids: params[:project_ids]), class: "btn btn-primary" %>
diff --git a/app/views/sample_types/show.html.erb b/app/views/sample_types/show.html.erb index 4c0cf49526..1f97c3d5a7 100644 --- a/app/views/sample_types/show.html.erb +++ b/app/views/sample_types/show.html.erb @@ -1,5 +1,9 @@ <%= render partial: 'general/item_title', locals: { item: @sample_type, buttons_partial: 'buttons' } %> - +<% if @sample_type.locked? %> +
+ Warning! This sample type is locked and should not be edited. +
+<% end %> <%= render partial: 'general/show_page_tab_definitions' %>
From d46e749f28176c946df4fa35a63f79d7d957ced7 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 16:43:13 +0100 Subject: [PATCH 39/49] Move before_action up --- app/controllers/sample_types_controller.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index 3ad7258d4d..e5645ff3b4 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -6,10 +6,10 @@ class SampleTypesController < ApplicationController before_action :samples_enabled? before_action :check_no_created_samples, only: [:destroy] + before_action :check_if_locked, only: %i[edit manage manage_update update] before_action :find_and_authorize_requested_item, except: %i[create batch_upload index new template_details] before_action :find_sample_type, only: %i[batch_upload template_details] before_action :check_isa_json_compliance, only: %i[edit update manage manage_update] - before_action :check_if_locked, only: %i[edit manage update manage_update] before_action :find_assets, only: [:index] before_action :auth_to_create, only: %i[new create] before_action :project_membership_required, only: %i[create new select filter_for_select] @@ -222,12 +222,12 @@ def update_sample_json_metadata end.compact return if attribute_changes.blank? - UpdateSampleMetadataJob.perform_later(@sample_type, attribute_changes, @current_user) + UpdateSampleMetadataJob.perform_later(@sample_type, @current_user, attribute_changes) end def check_if_locked - return unless @sample_type.locked? - return if @current_user&.is_admin? + @sample_type ||= SampleType.find(params[:id]) + return unless @sample_type&.locked? error_message = 'This sample type is locked and cannot be edited right now.' flash[:error] = error_message From faa938a6b6ebfcf7d845b5f41ac33e049c90f3f6 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 16:44:32 +0100 Subject: [PATCH 40/49] Prevent adding samples to a locked sample type --- app/controllers/samples_controller.rb | 11 +++++++++ app/models/sample.rb | 13 +++++----- app/models/sample_type.rb | 11 +++++++++ app/views/sample_types/show.html.erb | 2 +- lib/seek/samples/sample_metadata_updater.rb | 12 ++++------ .../sample_types_controller_test.rb | 24 ------------------- test/functional/samples_controller_test.rb | 13 ++++++++++ test/unit/sample_test.rb | 10 ++++++++ test/unit/sample_type_test.rb | 13 ++++++++++ 9 files changed, 71 insertions(+), 38 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 7b90061e2b..1118894e18 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -8,6 +8,7 @@ class SamplesController < ApplicationController before_action :samples_enabled? before_action :find_index_assets, only: :index before_action :find_and_authorize_requested_item, except: [:index, :new, :create, :preview] + before_action :check_if_locked_sample_type, only: [:edit, :new] before_action :templates_enabled?, only: [:query, :query_form] before_action :auth_to_create, only: %i[new create batch_create] @@ -371,4 +372,14 @@ def templates_enabled? redirect_to select_sample_types_path end end + + def check_if_locked_sample_type + return unless params[:sample_type_id] + + sample_type = SampleType.find(params[:sample_type_id]) + return unless sample_type&.locked? + + flash[:error] = 'This sample type is locked. You cannot edit the sample.' + redirect_to sample_types_path(sample_type) + end end diff --git a/app/models/sample.rb b/app/models/sample.rb index 5406cdecf6..464d37c008 100644 --- a/app/models/sample.rb +++ b/app/models/sample.rb @@ -40,6 +40,7 @@ class Sample < ApplicationRecord validates_with SampleAttributeValidator validate :validate_added_linked_sample_permissions + validate :check_if_locked_sample_type, on: :create before_validation :set_title_to_title_attribute_value before_validation :update_sample_resource_links @@ -241,12 +242,12 @@ def validate_added_linked_sample_permissions return if $authorization_checks_disabled return if linked_samples.empty? previous_linked_samples = [] - unless new_record? - previous_linked_samples = Sample.find(id).referenced_samples - end + previous_linked_samples = Sample.find(id).referenced_samples unless new_record? additions = linked_samples - previous_linked_samples - if additions.detect { |sample| !sample.can_view? } - errors.add(:linked_samples, 'includes a new private sample') - end + errors.add(:linked_samples, 'includes a new private sample') if additions.detect { |sample| !sample.can_view? } + end + + def check_if_locked_sample_type + errors.add(:sample_type, 'is locked') if sample_type&.locked? end end diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index da6286b4f6..3dc81722c5 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -165,7 +165,18 @@ def self.can_create? can && (!Seek::Config.project_admin_sample_type_restriction || User.current_user.is_admin_or_project_administrator?) end + def can_edit?(user = User.current_user) + super && !locked? + end + + def can_manage?(user = User.current_user) + super && !locked? + end + def can_delete?(user = User.current_user) + # Should not be able to delete a sample type if it is locked + return false if locked? + # Users should be able to delete an ISA JSON compliant sample type that has linked sample attributes, # as long as it's ISA JSON compliant. if is_isa_json_compliant? diff --git a/app/views/sample_types/show.html.erb b/app/views/sample_types/show.html.erb index 1f97c3d5a7..53c5740fe2 100644 --- a/app/views/sample_types/show.html.erb +++ b/app/views/sample_types/show.html.erb @@ -1,7 +1,7 @@ <%= render partial: 'general/item_title', locals: { item: @sample_type, buttons_partial: 'buttons' } %> <% if @sample_type.locked? %>
- Warning! This sample type is locked and should not be edited. + Warning! This sample type is being edited by a background process and cannot be edited right now.
<% end %> <%= render partial: 'general/show_page_tab_definitions' %> diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index f62157b7fe..76971f3a01 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -21,14 +21,12 @@ def update_metadata @sample_type.with_lock do @sample_type.samples.in_batches(of: 1000).each do |batch| batch.each do |sample| - sample.with_lock do - metadata = JSON.parse(sample.json_metadata) - # Update the metadata keys with the new attribute titles - @attribute_change_maps.each do |change| - metadata[change[:new_title]] = metadata.delete(change[:old_title]) - end - sample.update_column(:json_metadata, metadata.to_json) + metadata = JSON.parse(sample.json_metadata) + # Update the metadata keys with the new attribute titles + @attribute_change_maps.each do |change| + metadata[change[:new_title]] = metadata.delete(change[:old_title]) end + sample.update_column(:json_metadata, metadata.to_json) end end end diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index ca1f5f4140..56482357e5 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -777,10 +777,6 @@ class SampleTypesControllerTest < ActionController::TestCase end test 'check if sample type is locked' do - sys_admin = FactoryBot.create(:admin) - - # Add managing permission to the instance admin - @sample_type.policy.permissions << FactoryBot.create(:permission, contributor: sys_admin, access_type: Policy::MANAGING) refute @sample_type.locked? login_as(@person) @@ -800,20 +796,11 @@ class SampleTypesControllerTest < ActionController::TestCase assert_redirected_to sample_type_path(@sample_type) assert_equal flash[:error], 'This sample type is locked and cannot be edited right now.' end - - login_as(sys_admin) - %i[edit manage].each do |action| - get action, params: { id: @sample_type.id } - assert_nil flash[:error] - assert_response :success - end end test 'update a locked sample type' do - sys_admin = FactoryBot.create(:admin) other_person = FactoryBot.create(:person) sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) - sample_type.policy.permissions << FactoryBot.create(:permission, contributor: sys_admin, access_type: Policy::MANAGING) sample_type.policy.permissions << FactoryBot.create(:permission, contributor: other_person, access_type: Policy::MANAGING) (1..10).map do |_i| @@ -844,17 +831,6 @@ class SampleTypesControllerTest < ActionController::TestCase assert_redirected_to sample_type_path(sample_type) assert(sample_type.locked?) assert_equal flash[:error], 'This sample type is locked and cannot be edited right now.' - - login_as(sys_admin) - patch :update, params: { id: sample_type.id, sample_type: { - sample_attributes_attributes: { - '0': { id: sample_type.sample_attributes.detect(&:is_title), title: 'new title' } - } - } } - - assert(sample_type.locked?) - sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') - assert_nil flash[:error] end private diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index 31e6a849a4..a110861645 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1584,6 +1584,19 @@ def rdf_test_object end + test 'should not add a sample to a locked sample type' 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]) + + get :new, params: { sample_type_id: sample_type.id } + assert_response :success + + assert_select 'input[type=submit][disabled=disabled]', count: 1 + end + def rdf_test_object FactoryBot.create(:max_sample, policy: FactoryBot.create(:public_policy)) end diff --git a/test/unit/sample_test.rb b/test/unit/sample_test.rb index f4cbfc822d..37799b1ed6 100644 --- a/test/unit/sample_test.rb +++ b/test/unit/sample_test.rb @@ -1455,4 +1455,14 @@ class SampleTest < ActiveSupport::TestCase end end + test 'add sample to a locked sample type' do + sample_type = FactoryBot.create(:simple_sample_type, project_ids: [FactoryBot.create(:project).id], contributor: FactoryBot.create(:person)) + sample_type.set_lock + assert sample_type.locked? + + assert_no_difference('Sample.count') do + sample = Sample.create(sample_type: sample_type, project_ids: [sample_type.projects.first.id]) + sample.errors.added?(:sample_type, 'is locked') + end + end end diff --git a/test/unit/sample_type_test.rb b/test/unit/sample_type_test.rb index 19c8658022..c506a74453 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -1069,6 +1069,19 @@ def teardown assert sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') end + test 'update locked sample type' do + sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) + (1..10).each do |i| + FactoryBot.create(:sample, sample_type: sample_type, project_ids: @project_ids, title: "Sample #{i}") + end + sample_type.with_lock do + # sample_type.samples << FactoryBot.create(:sample, sample_type: sample_type, project_ids: @project_ids, title: 'Sample 11') + # sample_type.title = 'Updated title' + sample_type.sample_attributes.first.title = 'Updated title' + sample_type.save! + end + end + private # sample type with 3 samples From 0d92bb4c48e4b110125d4da5b60465c17086b0f3 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 29 Oct 2024 17:38:21 +0100 Subject: [PATCH 41/49] Fix failing test --- test/functional/samples_controller_test.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index a110861645..a19271f42f 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1594,7 +1594,10 @@ def rdf_test_object get :new, params: { sample_type_id: sample_type.id } assert_response :success - assert_select 'input[type=submit][disabled=disabled]', count: 1 + sample_type.set_lock + get :new, params: { sample_type_id: sample_type.id } + assert_redirected_to sample_types_path(sample_type) + assert_equal flash[:error], 'This sample type is locked. You cannot edit the sample.' end def rdf_test_object From 3159cc3d92356ab72c756b89079153bff4ab6a98 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 30 Oct 2024 15:29:10 +0100 Subject: [PATCH 42/49] Remove database lock --- lib/seek/samples/sample_metadata_updater.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index 76971f3a01..0d992a58ad 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -18,16 +18,14 @@ def update_metadata return unless @sample_type.locked? User.with_current_user(@user) do - @sample_type.with_lock do - @sample_type.samples.in_batches(of: 1000).each do |batch| - batch.each do |sample| - metadata = JSON.parse(sample.json_metadata) - # Update the metadata keys with the new attribute titles - @attribute_change_maps.each do |change| - metadata[change[:new_title]] = metadata.delete(change[:old_title]) - end - sample.update_column(:json_metadata, metadata.to_json) + @sample_type.samples.in_batches(of: 1000).each do |batch| + batch.each do |sample| + metadata = JSON.parse(sample.json_metadata) + # Update the metadata keys with the new attribute titles + @attribute_change_maps.each do |change| + metadata[change[:new_title]] = metadata.delete(change[:old_title]) end + sample.update_column(:json_metadata, metadata.to_json) end end end From 6421eeda2f447c51253a163beda4a1b0f821e7cd Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 30 Oct 2024 16:50:52 +0100 Subject: [PATCH 43/49] Use Taks instead of DelayedJob --- app/jobs/update_sample_metadata_job.rb | 16 +++++----------- app/models/sample_type.rb | 7 ++----- lib/seek/samples/sample_metadata_updater.rb | 3 --- 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index cd55e015e8..c1e8015163 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -1,19 +1,9 @@ # frozen_string_literal: true -class UpdateSampleMetadataJob < ApplicationJob +class UpdateSampleMetadataJob < TaskJob queue_with_priority 1 queue_as QueueNames::SAMPLES - attr_accessor :sample_type, :user, :attribute_changes - - before_enqueue do |job| - SampleType.find(job.arguments[0].id).set_lock - end - - before_perform do |job| - SampleType.find(job.arguments[0].id).set_lock - end - def perform(sample_type, user, attribute_changes = []) @sample_type = sample_type @user = user @@ -21,4 +11,8 @@ def perform(sample_type, user, attribute_changes = []) Seek::Samples::SampleMetadataUpdater.new(@sample_type, @user, @attribute_changes).update_metadata end + + def task + arguments[0].sample_metadata_update_task + end end diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 3dc81722c5..4a170f3504 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -64,6 +64,7 @@ class SampleType < ApplicationRecord has_annotation_type :sample_type_tag, method_name: :tags + has_task :sample_metadata_update def investigations return [] if studies.empty? && assays.empty? @@ -112,11 +113,7 @@ def is_isa_json_compliant? end def locked? - Rails.cache.fetch("sample_type_lock_#{id}").present? - end - - def set_lock - Rails.cache.write("sample_type_lock_#{id}", true, expires_in: 1.hour) + sample_metadata_update_task&.in_progress? end def validate_value?(attribute_name, value) diff --git a/lib/seek/samples/sample_metadata_updater.rb b/lib/seek/samples/sample_metadata_updater.rb index 0d992a58ad..69a1fbdb15 100644 --- a/lib/seek/samples/sample_metadata_updater.rb +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -29,9 +29,6 @@ def update_metadata end end end - - ensure - Rails.cache.delete("sample_type_lock_#{@sample_type.id}") end def raise_sample_metadata_update_exception From 9bcbd7e9b77bdafdd39ff94a604074f4e9d0534b Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 30 Oct 2024 16:51:08 +0100 Subject: [PATCH 44/49] Fix tests --- app/controllers/sample_types_controller.rb | 1 + .../sample_types_controller_test.rb | 6 ++--- test/functional/samples_controller_test.rb | 5 ++-- test/unit/sample_test.rb | 7 ++++-- test/unit/sample_type_test.rb | 25 ++++--------------- 5 files changed, 16 insertions(+), 28 deletions(-) diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index e5645ff3b4..7cb6b3ec82 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -227,6 +227,7 @@ def update_sample_json_metadata def check_if_locked @sample_type ||= SampleType.find(params[:id]) + @sample_type.reload return unless @sample_type&.locked? error_message = 'This sample type is locked and cannot be edited right now.' diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 56482357e5..d45b6cae6e 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -41,7 +41,7 @@ class SampleTypesControllerTest < ActionController::TestCase assert_enqueued_with(job: SampleTypeUpdateJob) do assert_difference('ActivityLog.count') do assert_difference('SampleType.count') do - assert_difference('Task.count') do + assert_difference('Task.where(key: "template_generation").count') do post :create, params: { sample_type: { title: 'Hello!', project_ids: @project_ids, description: 'The description!!', @@ -787,8 +787,8 @@ class SampleTypesControllerTest < ActionController::TestCase assert_response :success end - # Locking the sample type - @sample_type.set_lock + # lock the sample type by adding a fake update task + UpdateSampleMetadataJob.perform_later(@sample_type, @person.user, []) assert @sample_type.locked? %i[edit manage].each do |action| diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index a19271f42f..94a06d3323 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1591,10 +1591,9 @@ def rdf_test_object sample_type = FactoryBot.create(:simple_sample_type, contributor: person, project_ids: [project.id]) - get :new, params: { sample_type_id: sample_type.id } - assert_response :success + # lock the sample type by adding a fake update task + UpdateSampleMetadataJob.perform_later(sample_type, person.user, []) - sample_type.set_lock get :new, params: { sample_type_id: sample_type.id } assert_redirected_to sample_types_path(sample_type) assert_equal flash[:error], 'This sample type is locked. You cannot edit the sample.' diff --git a/test/unit/sample_test.rb b/test/unit/sample_test.rb index 37799b1ed6..3daf68c5d5 100644 --- a/test/unit/sample_test.rb +++ b/test/unit/sample_test.rb @@ -1456,8 +1456,11 @@ class SampleTest < ActiveSupport::TestCase end test 'add sample to a locked sample type' do - sample_type = FactoryBot.create(:simple_sample_type, project_ids: [FactoryBot.create(:project).id], contributor: FactoryBot.create(:person)) - sample_type.set_lock + person = FactoryBot.create(:person) + sample_type = FactoryBot.create(:simple_sample_type, project_ids: [FactoryBot.create(:project).id], contributor: person) + + # lock the sample type by adding a fake update task + UpdateSampleMetadataJob.perform_later(sample_type, person.user, []) assert sample_type.locked? assert_no_difference('Sample.count') do diff --git a/test/unit/sample_type_test.rb b/test/unit/sample_type_test.rb index c506a74453..f5b7f93817 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -6,11 +6,6 @@ def setup @person = FactoryBot.create(:person) @project = @person.projects.first @project_ids = [@project.id] - Rails.cache.clear - end - - def teardown - Rails.cache.clear end test 'validation' do @@ -695,7 +690,7 @@ def teardown assert sample_type.new_record? refute sample_type.template_generation_task.pending? - assert_difference('Task.count', 1) do + assert_difference('Task.where(key: "template_generation").count', 1) do assert_enqueued_with(job: SampleTemplateGeneratorJob, args: [sample_type]) do disable_authorization_checks { sample_type.save! } assert sample_type.reload.template_generation_task.pending? @@ -1063,25 +1058,15 @@ def teardown test 'sample type is locked?' do sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) refute sample_type.locked? - sample_type.set_lock + + # lock the sample type by adding a fake update task + UpdateSampleMetadataJob.perform_later(sample_type, @person.user, []) + assert sample_type.locked? refute sample_type.valid? assert sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') end - test 'update locked sample type' do - sample_type = FactoryBot.create(:simple_sample_type, project_ids: @project_ids, contributor: @person) - (1..10).each do |i| - FactoryBot.create(:sample, sample_type: sample_type, project_ids: @project_ids, title: "Sample #{i}") - end - sample_type.with_lock do - # sample_type.samples << FactoryBot.create(:sample, sample_type: sample_type, project_ids: @project_ids, title: 'Sample 11') - # sample_type.title = 'Updated title' - sample_type.sample_attributes.first.title = 'Updated title' - sample_type.save! - end - end - private # sample type with 3 samples From b9b5235840251718509106d424504570d05c3601 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 30 Oct 2024 18:28:52 +0100 Subject: [PATCH 45/49] Fix tests --- test/functional/sample_types_controller_test.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index d45b6cae6e..4bc60b2544 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -816,7 +816,9 @@ class SampleTypesControllerTest < ActionController::TestCase '0': { id: sample_type.sample_attributes.detect(&:is_title), title: 'new title' } } } } + assert_nil flash[:error] assert_redirected_to sample_type_path(sample_type) + sample_type.reload assert sample_type.locked? login_as(other_person) @@ -826,6 +828,7 @@ class SampleTypesControllerTest < ActionController::TestCase '0': { id: sample_type.sample_attributes.detect(&:is_title), title: 'new title' } } } } + sample_type.reload sample_type.errors.added?(:base, 'This sample type is locked and cannot be edited right now.') assert_redirected_to sample_type_path(sample_type) From f68b44ee4246a47e9826b96c6e500a224b04d9a1 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 13 Nov 2024 15:35:18 +0100 Subject: [PATCH 46/49] Cleaning up --- app/controllers/samples_controller.rb | 2 +- app/models/sample.rb | 2 +- test/functional/sample_types_controller_test.rb | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index 1118894e18..3a67beab55 100644 --- a/app/controllers/samples_controller.rb +++ b/app/controllers/samples_controller.rb @@ -8,7 +8,7 @@ class SamplesController < ApplicationController before_action :samples_enabled? before_action :find_index_assets, only: :index before_action :find_and_authorize_requested_item, except: [:index, :new, :create, :preview] - before_action :check_if_locked_sample_type, only: [:edit, :new] + before_action :check_if_locked_sample_type, only: %i[edit new create update] before_action :templates_enabled?, only: [:query, :query_form] before_action :auth_to_create, only: %i[new create batch_create] diff --git a/app/models/sample.rb b/app/models/sample.rb index 464d37c008..e07e179794 100644 --- a/app/models/sample.rb +++ b/app/models/sample.rb @@ -40,7 +40,7 @@ class Sample < ApplicationRecord validates_with SampleAttributeValidator validate :validate_added_linked_sample_permissions - validate :check_if_locked_sample_type, on: :create + validate :check_if_locked_sample_type, on: %i[create update] before_validation :set_title_to_title_attribute_value before_validation :update_sample_resource_links diff --git a/test/functional/sample_types_controller_test.rb b/test/functional/sample_types_controller_test.rb index 4bc60b2544..2c7ca980c0 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -5,7 +5,6 @@ class SampleTypesControllerTest < ActionController::TestCase include AuthenticatedTestHelper setup do - Rails.cache.clear FactoryBot.create(:person) # to prevent person being first person and therefore admin @person = FactoryBot.create(:project_administrator) @project = @person.projects.first From 48e94de928d1374a44e9872cec6ff0589d75334f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 13 Nov 2024 16:00:02 +0100 Subject: [PATCH 47/49] use `state_allows_xxx?` instead of `can_xxx?` --- app/models/sample_type.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 4a170f3504..d4b009245d 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -162,18 +162,19 @@ def self.can_create? can && (!Seek::Config.project_admin_sample_type_restriction || User.current_user.is_admin_or_project_administrator?) end - def can_edit?(user = User.current_user) + def state_allows_edit?(*args) super && !locked? end - def can_manage?(user = User.current_user) + def state_allows_manage?(*args) super && !locked? end - def can_delete?(user = User.current_user) - # Should not be able to delete a sample type if it is locked - return false if locked? + def state_allows_delete?(*args) + super && !locked? + end + def can_delete?(user = User.current_user) # Users should be able to delete an ISA JSON compliant sample type that has linked sample attributes, # as long as it's ISA JSON compliant. if is_isa_json_compliant? From 5d5ae6bcb171aa092b2dffe53593daf067f7a5c0 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 13 Nov 2024 17:28:05 +0100 Subject: [PATCH 48/49] Enqueue `SampleTypeUpdateJob` after performing `UpdateSampleMetadataJob` --- app/jobs/update_sample_metadata_job.rb | 4 ++++ .../unit/jobs/update_sample_metadata_job_test.rb | 16 ++++++++-------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index c1e8015163..4b25fa147a 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -4,6 +4,10 @@ class UpdateSampleMetadataJob < TaskJob queue_with_priority 1 queue_as QueueNames::SAMPLES + after_perform do |job| + SampleTypeUpdateJob.perform_later(job.arguments.first, true) + end + def perform(sample_type, user, attribute_changes = []) @sample_type = sample_type @user = user diff --git a/test/unit/jobs/update_sample_metadata_job_test.rb b/test/unit/jobs/update_sample_metadata_job_test.rb index 0ccfe05e51..8f82b593a1 100644 --- a/test/unit/jobs/update_sample_metadata_job_test.rb +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -10,16 +10,12 @@ def setup (1..10).each do |_i| FactoryBot.create(:sample, sample_type: @sample_type, contributor: @person) end - Rails.cache.clear - end - - def teardown - Rails.cache.clear end test 'perform' do User.with_current_user(@person.user) do UpdateSampleMetadataJob.perform_now(@sample_type, @person.user, []) + assert_enqueued_with(job: SampleTypeUpdateJob, args: [@sample_type, true]) end end @@ -58,6 +54,9 @@ def teardown end test 'perform with unexpected error' do + # Clears the queue of all jobs + # Queue has already multiple SampleTypeUpdateJob jobs from the setup + clear_enqueued_jobs job = UpdateSampleMetadataJob.new(@sample_type, @person.user, 'bad_attribute_change_map') assert_enqueued_with(job: UpdateSampleMetadataJob, args: [@sample_type, @person.user, 'bad_attribute_change_map']) do job.enqueue @@ -66,9 +65,10 @@ def teardown perform_enqueued_jobs do job.perform_now - rescue StandardError - # The sample type should be unlocked even if an error occurs - refute @sample_type.locked? end + # The sample type should be unlocked even if an error occurs + refute @sample_type.locked? + # SampleTypeUpdateJob should not be enqueued when an error occurs + assert_no_enqueued_jobs only: SampleTypeUpdateJob end end From 7b6a600d60c3963b30996c85ddcf8fbe26ad3569 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 14 Nov 2024 10:34:40 +0100 Subject: [PATCH 49/49] Prevent saving each sample at update. --- app/jobs/update_sample_metadata_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb index 4b25fa147a..ce4e9358c5 100644 --- a/app/jobs/update_sample_metadata_job.rb +++ b/app/jobs/update_sample_metadata_job.rb @@ -5,7 +5,7 @@ class UpdateSampleMetadataJob < TaskJob queue_as QueueNames::SAMPLES after_perform do |job| - SampleTypeUpdateJob.perform_later(job.arguments.first, true) + SampleTypeUpdateJob.perform_later(job.arguments.first, false) end def perform(sample_type, user, attribute_changes = [])