362 add and edit attributes in sample types

app/controllers/sample_types_controller.rb

@@ -12,7 +12,9 @@ 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
 
   api_actions :index, :show, :create, :update, :destroy
 
@@ -201,4 +203,24 @@ 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, attribute_changes, @current_user)
+  end
 end

app/jobs/update_sample_metadata_job.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class UpdateSampleMetadataJob < ApplicationJob
+  queue_with_priority 1
+  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
+    end
+  end
+end

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?

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
 

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? %>
               <tr id="add-attribute-row">
                 <td colspan="6">
                   <%= button_link_to('Add new attribute', 'add', '#', id: 'add-attribute') %>

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 @@
   <td>
 
     <%= 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" } %>
   </td>
 
   <td class="text-center" title="<%= required_title_text %>">
@@ -132,7 +131,7 @@
                                       unit_id),
                    include_blank: true,
                    class: 'form-control',
-                   disabled: !allow_type_change, data: { attr: "unit" } %>
+                   data: { attr: "unit" } %>
   </td>
 
 

lib/seek/samples/sample_metadata_updater.rb

@@ -0,0 +1,32 @@
+# 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(samples, user, attribute_changes)
+        @samples = samples
+        @user = user
+        @attribute_change_maps = attribute_changes
+      end
+
+      def update_metadata
+        return if @attribute_change_maps.blank? || @samples.blank? || @user.nil?
+
+        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])
+            end
+            sample.update_column(:json_metadata, metadata.to_json)
+          end
+        end
+      end
+    end
+  end
+end

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)

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
@@ -687,22 +677,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 +737,40 @@ 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
+
   private
 
   def template_for_upload

test/unit/jobs/update_sample_metadata_job_test.rb

@@ -0,0 +1,40 @@
+# 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, @person.user, [])
+    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)
+      @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
+end

test/unit/sample_type_test.rb

@@ -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?