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,10 @@
+# frozen_string_literal: true
+
+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
+  end
+end

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)
 

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

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

@@ -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." %>
 
 <tr class="sample-attribute <%= 'success' if sample_attribute.nil? -%><%='hidden' if hide_seek_sample_multi -%>" data-index="<%= index-%>">
   <th scope="row" class="attribute-position">
@@ -42,7 +43,7 @@
   </th>
   <td>
 
-    <%= 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" } %>
   </td>
 
@@ -132,7 +133,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(sample_type, attribute_changes, user)
+        @sample_type = sample_type
+        @attribute_change_maps = attribute_changes
+        @user = user
+      end
+
+      def update_metadata
+        return if @attribute_change_maps.blank? || @sample_type.samples.blank? || @sample_type.nil? || @user.nil?
+
+        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
+        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,24 +49,17 @@ 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)
           return true if attr.new_record?
-          return false if inherited?(attr)
 
           attr = attr.accessor_name
         end
-        if attr
-          !samples?
-        else
-          true
-        end
+        return !samples? unless attr
+        return samples.all?(&:can_edit?) if samples?
+
+        true
       end
 
       # whether the type for the attribute can be changed

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
@@ -688,8 +678,20 @@ class SampleTypesControllerTest < ActionController::TestCase
   end
 
   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: {
@@ -763,6 +765,113 @@ 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 '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

test/unit/jobs/update_sample_metadata_job_test.rb

@@ -0,0 +1,39 @@
+# 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.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