diff --git a/app/controllers/sample_types_controller.rb b/app/controllers/sample_types_controller.rb index b40d6a9f63..7cb6b3ec82 100644 --- a/app/controllers/sample_types_controller.rb +++ b/app/controllers/sample_types_controller.rb @@ -6,13 +6,16 @@ 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 :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 api_actions :index, :show, :create, :update, :destroy @@ -201,4 +204,35 @@ def check_no_created_samples redirect_to @sample_type end end + + def old_attributes + return if @sample_type.sample_attributes.blank? + + @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.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, @current_user, attribute_changes) + end + + 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.' + flash[:error] = error_message + @sample_type.errors.add(:base, error_message) + redirect_to @sample_type + end end diff --git a/app/controllers/samples_controller.rb b/app/controllers/samples_controller.rb index c2738c0bd5..4e0c928e32 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: %i[edit new create update] 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/jobs/update_sample_metadata_job.rb b/app/jobs/update_sample_metadata_job.rb new file mode 100644 index 0000000000..ce4e9358c5 --- /dev/null +++ b/app/jobs/update_sample_metadata_job.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class UpdateSampleMetadataJob < TaskJob + queue_with_priority 1 + queue_as QueueNames::SAMPLES + + after_perform do |job| + SampleTypeUpdateJob.perform_later(job.arguments.first, false) + end + + 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 + + def task + arguments[0].sample_metadata_update_task + end +end diff --git a/app/models/sample.rb b/app/models/sample.rb index 5406cdecf6..e07e179794 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: %i[create update] 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_attribute.rb b/app/models/sample_attribute.rb index 9740e0c3fe..f2c62cd237 100644 --- a/app/models/sample_attribute.rb +++ b/app/models/sample_attribute.rb @@ -97,9 +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. - - errors.add(:title, error_message) if title_changed? && !c.allow_name_change?(self) + attr_title = self.new_record? ? title : title_was + error_message = "cannot be changed (#{attr_title})" # Use pre-change title in error message. unless c.allow_required?(self) errors.add(:is_title, error_message) if is_title_changed? diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 4483afb499..d4b009245d 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 @@ -63,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? @@ -110,6 +112,10 @@ def is_isa_json_compliant? (studies.any? || assays.any?) && has_only_isa_json_compliant_investigations && !isa_template.nil? end + def locked? + sample_metadata_update_task&.in_progress? + end + def validate_value?(attribute_name, value) attribute = sample_attributes.detect { |attr| attr.title == attribute_name } raise UnknownAttributeException, "Unknown attribute #{attribute_name}" unless attribute @@ -156,6 +162,18 @@ def self.can_create? can && (!Seek::Config.project_admin_sample_type_restriction || User.current_user.is_admin_or_project_administrator?) end + def state_allows_edit?(*args) + super && !locked? + end + + def state_allows_manage?(*args) + super && !locked? + end + + 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. @@ -244,13 +262,13 @@ 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 + 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/_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 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/app/views/sample_types/_sample_attribute_form.html.erb b/app/views/sample_types/_sample_attribute_form.html.erb index 1adb8d8327..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 %> @@ -43,7 +42,7 @@ <%= text_field_tag "#{field_name_prefix}[title]", title, class: 'form-control', - placeholder: 'Attribute name', disabled: !allow_name_change, data: { attr: "title" } %> + placeholder: 'Attribute name', data: { attr: "title" } %> @@ -132,7 +131,7 @@ unit_id), include_blank: true, class: 'form-control', - disabled: !allow_type_change, data: { attr: "unit" } %> + data: { attr: "unit" } %> 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..53c5740fe2 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 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 new file mode 100644 index 0000000000..69a1fbdb15 --- /dev/null +++ b/lib/seek/samples/sample_metadata_updater.rb @@ -0,0 +1,39 @@ +# 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, user, attribute_changes) + @sample_type = sample_type + @user = user + @attribute_change_maps = attribute_changes + end + + def update_metadata + return if @attribute_change_maps.blank? || @sample_type.blank? || @user.nil? || @sample_type.samples.blank? + return unless @sample_type.locked? + + User.with_current_user(@user) 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 + + def raise_sample_metadata_update_exception + raise SampleMetadataUpdateException.new('An unexpected error occurred while updating the sample metadata') + end + end + end +end \ No newline at end of file diff --git a/lib/seek/samples/sample_type_editing_constraints.rb b/lib/seek/samples/sample_type_editing_constraints.rb index d5c35aa78d..aab7fa449e 100644 --- a/lib/seek/samples/sample_type_editing_constraints.rb +++ b/lib/seek/samples/sample_type_editing_constraints.rb @@ -21,16 +21,16 @@ 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 end - if attr - !blanks?(attr) - else - !samples? - end + + return !samples? unless attr + return !blanks?(attr) if samples? + + true end # an attribute could be removed if all are currently blank @@ -49,26 +49,12 @@ def allow_attribute_removal?(attr) end end - # whether a new attribtue can be created + # 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? - !samples? + true 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? - return false if inherited?(attr) - - attr = attr.accessor_name - end - if attr - !samples? - else - true - end - 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 9c60c8d95d..2c7ca980c0 100644 --- a/test/functional/sample_types_controller_test.rb +++ b/test/functional/sample_types_controller_test.rb @@ -17,6 +17,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 @@ -36,7 +40,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!!', @@ -439,16 +443,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 @@ -687,22 +681,6 @@ class SampleTypesControllerTest < ActionController::TestCase end end - test 'validates changes against editing constraints' do - @sample_type.samples.create!(data: { the_title: 'yes' }, sample_type: @sample_type, project_ids: @project_ids) - - 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 @@ -763,6 +741,100 @@ 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) + (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) + get :edit, params: { id: sample_type.id } + assert_response :success + assert_select 'a#add-attribute', count: 1 + + # 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 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 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 'check if sample type is locked' do + 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 + + # 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| + 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 + end + + test 'update a locked sample type' do + 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: 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_nil flash[:error] + assert_redirected_to sample_type_path(sample_type) + sample_type.reload + 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.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) + assert(sample_type.locked?) + assert_equal flash[:error], 'This sample type is locked and cannot be edited right now.' + end + private def template_for_upload diff --git a/test/functional/samples_controller_test.rb b/test/functional/samples_controller_test.rb index 23d5293b90..f171b57a58 100644 --- a/test/functional/samples_controller_test.rb +++ b/test/functional/samples_controller_test.rb @@ -1593,6 +1593,21 @@ 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]) + + # lock the sample type by adding a fake update task + UpdateSampleMetadataJob.perform_later(sample_type, person.user, []) + + 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 FactoryBot.create(:max_sample, policy: FactoryBot.create(:public_policy)) end 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..8f82b593a1 --- /dev/null +++ b/test/unit/jobs/update_sample_metadata_job_test.rb @@ -0,0 +1,74 @@ +# 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 + + 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 + + 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 + + test 'Check sample metadata after updating the attribute title' do + 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.perform_now(@sample_type, @person.user, attribute_change_maps) + refute @sample_type.locked? + @sample_type.reload + @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 + + 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 + end + assert @sample_type.locked? + + perform_enqueued_jobs do + job.perform_now + 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 diff --git a/test/unit/sample_test.rb b/test/unit/sample_test.rb index f4cbfc822d..3daf68c5d5 100644 --- a/test/unit/sample_test.rb +++ b/test/unit/sample_test.rb @@ -1455,4 +1455,17 @@ class SampleTest < ActiveSupport::TestCase end end + test 'add sample to a locked sample type' do + 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 + 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 d03b2b5eb6..f5b7f93817 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -690,7 +690,7 @@ def setup 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? @@ -901,26 +901,26 @@ def setup assert sample_type.valid? - # Adding attribute - sample_type.sample_attributes.build(title: 'test 123') - refute sample_type.valid? - assert sample_type.errors.added?(:sample_attributes, 'cannot be added, new attributes are not allowed (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? - # Removing attribute (via nested attributes) - sample_type.sample_attributes_attributes = { id: sample_type.sample_attributes.last.id, _destroy: '1' } + # 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 removed, there are existing samples using this attribute (patient)') + assert sample_type.errors.added?(:'sample_attributes.required', 'cannot be changed (mandatory test 123)') sample_type.reload assert sample_type.valid? - # Changing attribute title - sample_type.sample_attributes.last.title = 'banana' + # Removing attribute (via nested attributes) + sample_type.sample_attributes_attributes = { id: sample_type.sample_attributes.last.id, _destroy: '1' } refute sample_type.valid? - assert sample_type.errors.added?(:'sample_attributes.title', 'cannot be changed (patient)') + assert sample_type.errors.added?(:sample_attributes, 'cannot be removed, there are existing samples using this attribute (patient)') sample_type.reload assert sample_type.valid? @@ -1055,6 +1055,18 @@ 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? + + # 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 + private # sample type with 3 samples diff --git a/test/unit/samples/sample_type_editing_constraints_test.rb b/test/unit/samples/sample_type_editing_constraints_test.rb index 4cfdc03a7a..e21137af0d 100644 --- a/test/unit/samples/sample_type_editing_constraints_test.rb +++ b/test/unit/samples/sample_type_editing_constraints_test.rb @@ -20,23 +20,6 @@ class SampleTypeEditingConstraintsTest < ActiveSupport::TestCase refute c.samples? 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) - - # 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) - end - test 'allow type change?' do c = Seek::Samples::SampleTypeEditingConstraints.new(sample_type_with_samples) refute c.allow_type_change?(:address) @@ -159,14 +142,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 @@ -224,8 +199,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