Merge pull request #2032 from ELIXIR-Belgium/362-add-and-edit-attribu…

…tes-in-sample-types

362 add and edit attributes in sample types

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

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

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

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

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

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

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 %>
 

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>
 
 

app/views/sample_types/select/_filtered_sample_types.html.erb

@@ -7,7 +7,7 @@
             <%= render :partial => "assets/resource_list_item", :object => sample_type %>
           </div>
         </div>
-        <% if Sample.can_create? %>
+        <% if Sample.can_create? && !sample_type.locked? %>
           <div class="col-sm-2">
             <%= link_to "New sample", new_sample_path(:sample_type_id => sample_type.id, project_ids: params[:project_ids]), class: "btn btn-primary" %>
           </div>

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? %>
+  <div id="locked-sample-type-warning" class="alert alert-danger">
+    <strong>Warning!</strong> This sample type is being edited by a background process and cannot be edited right now.
+  </div>
+<% end %>
 <%= render partial: 'general/show_page_tab_definitions' %>
 
 <div class="tab-content">

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

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)