From 7a3eed86f94d46c93645d2a3aa3fe202bb4b5264 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 21 Sep 2023 14:38:39 +0200 Subject: [PATCH 1/4] Add function to define whether an assay has linked child assays --- app/models/assay.rb | 66 +++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/app/models/assay.rb b/app/models/assay.rb index 13b2b9279e..ab24b065a4 100644 --- a/app/models/assay.rb +++ b/app/models/assay.rb @@ -1,10 +1,9 @@ class Assay < ApplicationRecord - include Seek::Ontologies::AssayOntologyTypes - enum status: [:planned, :running, :completed, :cancelled, :failed] + enum status: %i[planned running completed cancelled failed] belongs_to :assignee, class_name: 'Person' - + # needs to before acts_as_isa - otherwise auto_index=>false is overridden by Seek::Search::CommonFields if Seek::Config.solr_enabled searchable(auto_index: false) do @@ -24,7 +23,7 @@ class Assay < ApplicationRecord acts_as_isa acts_as_snapshottable - belongs_to :sample_type + belongs_to :sample_type belongs_to :assay_class has_many :assay_organisms, dependent: :destroy, inverse_of: :assay @@ -49,9 +48,9 @@ class Assay < ApplicationRecord has_one :investigation, through: :study has_one :external_asset, as: :seek_entity, dependent: :destroy - validates :assay_type_uri, presence:true + validates :assay_type_uri, presence: true validates_with AssayTypeUriValidator - validates :technology_type_uri, absence:true, if: :is_modelling? + validates :technology_type_uri, absence: true, if: :is_modelling? validates_with TechnologyTypeUriValidator validates_presence_of :contributor validates_presence_of :assay_class @@ -66,6 +65,10 @@ class Assay < ApplicationRecord enforce_authorization_on_association :study, :view + def has_linked_child_assay? + sample_type.linked_sample_attributes.any? + end + def default_contributor User.current_user.try :person end @@ -91,7 +94,7 @@ def is_experimental? end def associated_samples_through_sample_type - (sample_type.nil? || sample_type.samples.nil?) ? [] : sample_type.samples + sample_type.nil? || sample_type.samples.nil? ? [] : sample_type.samples end # Create or update relationship of this assay to another, with a specific relationship type and version @@ -103,9 +106,7 @@ def associate(asset, options = {}) else assay_asset = assay_assets.detect { |aa| aa.asset == asset } - if assay_asset.nil? - assay_asset = assay_assets.build - end + assay_asset = assay_assets.build if assay_asset.nil? assay_asset.asset = asset assay_asset.version = asset.version if asset && asset.respond_to?(:version) @@ -124,12 +125,12 @@ def associate(asset, options = {}) end def self.simple_associated_asset_types - [:models, :sops, :publications, :documents] + %i[models sops publications documents] end # Associations where there is additional metadata on the association, i.e. `direction` def self.complex_associated_asset_types - [:data_files, :samples, :placeholders] + %i[data_files samples placeholders] end def assets @@ -166,7 +167,9 @@ def avatar_key def clone_with_associations new_object = dup new_object.policy = policy.deep_copy - new_object.assay_assets = assay_assets.select { |aa| self.class.complex_associated_asset_types.include?(aa.asset_type.underscore.pluralize.to_sym) }.map(&:dup) + new_object.assay_assets = assay_assets.select do |aa| + self.class.complex_associated_asset_types.include?(aa.asset_type.underscore.pluralize.to_sym) + end.map(&:dup) self.class.simple_associated_asset_types.each do |type| new_object.send("#{type}=", try(type)) end @@ -187,7 +190,8 @@ def human_disease_terms # organism may be either an ID or Organism instance # strain_id should be the id of the strain # culture_growth should be the culture growth instance - def associate_organism(organism, strain_id = nil, culture_growth_type = nil, tissue_and_cell_type_id = nil, tissue_and_cell_type_title = nil) + def associate_organism(organism, strain_id = nil, culture_growth_type = nil, tissue_and_cell_type_id = nil, + tissue_and_cell_type_title = nil) organism = Organism.find(organism) if organism.is_a?(Numeric) || organism.is_a?(String) strain = organism.strains.find_by_id(strain_id) tissue_and_cell_type = nil @@ -197,22 +201,22 @@ def associate_organism(organism, strain_id = nil, culture_growth_type = nil, tis tissue_and_cell_type = TissueAndCellType.where(title: tissue_and_cell_type_title).first_or_create! end - unless AssayOrganism.exists_for?(self, organism, strain, culture_growth_type, tissue_and_cell_type) - assay_organism = AssayOrganism.new(assay: self, organism: organism, culture_growth_type: culture_growth_type, - strain: strain, tissue_and_cell_type: tissue_and_cell_type) - assay_organisms << assay_organism - end + return if AssayOrganism.exists_for?(self, organism, strain, culture_growth_type, tissue_and_cell_type) + + assay_organism = AssayOrganism.new(assay: self, organism:, culture_growth_type:, + strain:, tissue_and_cell_type:) + assay_organisms << assay_organism end # Associates a human disease with the assay # human disease may be either an ID or HumanDisease instance def associate_human_disease(human_disease) human_disease = HumanDisease.find(human_disease) if human_disease.is_a?(Numeric) || human_disease.is_a?(String) - assay_human_disease = AssayHumanDisease.new(assay: self, human_disease: human_disease) + assay_human_disease = AssayHumanDisease.new(assay: self, human_disease:) - unless AssayHumanDisease.exists_for?(human_disease, self) - assay_human_diseases << assay_human_disease - end + return if AssayHumanDisease.exists_for?(human_disease, self) + + assay_human_diseases << assay_human_disease end # overides that from Seek::RDF::RdfGeneration, as Assay entity depends upon the AssayClass (modelling, or experimental) of the Assay @@ -224,15 +228,15 @@ def external_asset_search_terms external_asset ? external_asset.search_terms : [] end - def samples_attributes= attributes + def samples_attributes=(attributes) set_assay_assets_for('Sample', attributes) end - def data_files_attributes= attributes + def data_files_attributes=(attributes) set_assay_assets_for('DataFile', attributes) end - def placeholders_attributes= attributes + def placeholders_attributes=(attributes) set_assay_assets_for('Placeholder', attributes) end @@ -243,7 +247,7 @@ def self.filter_by_projects(projects) private def set_assay_assets_for(type, attributes) - type_assay_assets, other_assay_assets = self.assay_assets.partition { |aa| aa.asset_type == type } + type_assay_assets, other_assay_assets = assay_assets.partition { |aa| aa.asset_type == type } new_type_assay_assets = [] attributes.each do |attrs| @@ -252,15 +256,13 @@ def set_assay_assets_for(type, attributes) if existing new_type_assay_assets << existing.tap { |e| e.assign_attributes(attrs) } else - aa = self.assay_assets.build(attrs) - if aa.asset && aa.asset.can_view? - new_type_assay_assets << aa - end + aa = assay_assets.build(attrs) + new_type_assay_assets << aa if aa.asset && aa.asset.can_view? end end self.assay_assets = (other_assay_assets + new_type_assay_assets) - self.assay_assets + assay_assets end def related_publication_ids From c5b978433f62f8219c21d68a813dd6bc38c5d08d Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 21 Sep 2023 14:47:01 +0200 Subject: [PATCH 2/4] Add before delete action to fix the linkage between the assays --- app/controllers/assays_controller.rb | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 796642c82d..827e330491 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -4,8 +4,10 @@ class AssaysController < ApplicationController include Seek::AssetsCommon before_action :assays_enabled? - before_action :find_assets, :only=>[:index] - before_action :find_and_authorize_requested_item, :only=>[:edit, :update, :destroy, :manage, :manage_update, :show, :new_object_based_on_existing_one] + before_action :find_assets, only: [:index] + before_action :find_and_authorize_requested_item, + only: %i[edit update destroy manage manage_update show new_object_based_on_existing_one] + before_action :fix_assay_linkage, only: [:destroy] before_action :delete_linked_sample_types, only: [:destroy] #project_membership_required_appended is an alias to project_membership_required, but is necessary to include the actions @@ -118,6 +120,21 @@ def delete_linked_sample_types @assay.sample_type.destroy end + def fix_assay_linkage + return unless is_single_page_assay? + return unless @assay.has_linked_child_assay? + + previous_assay_linked_st_id = @assay.sample_type.sample_attributes.first.linked_sample_type_id + + next_assay = Assay.all.select do |a| + a.sample_type&.sample_attributes&.first&.linked_sample_type_id == @assay.sample_type_id + end&.first + next_assay_st_attr = next_assay.sample_type&.sample_attributes&.first + return unless next_assay || previous_assay_linked_st_id || next_assay_st_attr + + next_assay_st_attr.update(linked_sample_type_id: previous_assay_linked_st_id) + end + def update update_assay_organisms @assay, params update_assay_human_diseases @assay, params From 3e0c9aa5eb56e2caae5cb881e17104c6d2495436 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 21 Sep 2023 14:52:53 +0200 Subject: [PATCH 3/4] Some formatting and refactoring --- app/controllers/assays_controller.rb | 90 +++++++++++++--------------- 1 file changed, 42 insertions(+), 48 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 827e330491..e441bd0532 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -1,5 +1,4 @@ class AssaysController < ApplicationController - include Seek::IndexPager include Seek::AssetsCommon @@ -10,9 +9,9 @@ class AssaysController < ApplicationController before_action :fix_assay_linkage, only: [:destroy] before_action :delete_linked_sample_types, only: [:destroy] - #project_membership_required_appended is an alias to project_membership_required, but is necessary to include the actions - #defined in the application controller - before_action :project_membership_required_appended, :only=>[:new_object_based_on_existing_one] + # project_membership_required_appended is an alias to project_membership_required, but is necessary to include the actions + # defined in the application controller + before_action :project_membership_required_appended, only: [:new_object_based_on_existing_one] include Seek::Publishing::PublishingCommon @@ -32,69 +31,63 @@ def new_object_based_on_existing_one end @existing_assay.data_files.each do |d| - if !d.can_view? + unless d.can_view? notice_message << "Some or all #{t('data_file').pluralize} of the existing #{t('assays.assay')} cannot be viewed, you may specify your own!
" break end end @existing_assay.sops.each do |s| - if !s.can_view? - notice_message << "Some or all #{t('sop').pluralize} of the existing #{ t('assays.assay')} cannot be viewed, you may specify your own!
" + unless s.can_view? + notice_message << "Some or all #{t('sop').pluralize} of the existing #{t('assays.assay')} cannot be viewed, you may specify your own!
" break end end @existing_assay.models.each do |m| - if !m.can_view? + unless m.can_view? notice_message << "Some or all #{t('model').pluralize} of the existing #{t('assays.assay')} cannot be viewed, you may specify your own!
" break end end @existing_assay.documents.each do |d| - if !d.can_view? + unless d.can_view? notice_message << "Some or all #{t('document').pluralize} of the existing #{t('assays.assay')} cannot be viewed, you may specify your own!
" break end end - unless notice_message.blank? - flash.now[:notice] = notice_message.html_safe - end + flash.now[:notice] = notice_message.html_safe unless notice_message.blank? - render :action=>"new" + render action: 'new' else - flash[:error]="You do not have the necessary permissions to copy this #{t('assays.assay')}" + flash[:error] = "You do not have the necessary permissions to copy this #{t('assays.assay')}" redirect_to @existing_assay end end def new - @assay=setup_new_asset - @assay_class=params[:class] + @assay = setup_new_asset + @assay_class = params[:class] @permitted_params = assay_params if params[:assay] - #jump straight to experimental if modelling analysis is disabled - @assay_class ||= "experimental" unless Seek::Config.modelling_analysis_enabled + # jump straight to experimental if modelling analysis is disabled + @assay_class ||= 'experimental' unless Seek::Config.modelling_analysis_enabled - @assay.assay_class=AssayClass.for_type(@assay_class) unless @assay_class.nil? + @assay.assay_class = AssayClass.for_type(@assay_class) unless @assay_class.nil? - respond_to do |format| - format.html - end + respond_to(&:html) end def edit - respond_to do |format| - format.html - end + respond_to(&:html) end def create - params[:assay_class_id] ||= AssayClass.for_type("experimental").id + params[:assay_class_id] ||= AssayClass.for_type('experimental').id @assay = Assay.new(assay_params) update_assay_organisms @assay, params update_assay_human_diseases @assay, params - @assay.contributor=current_person + @assay.contributor = current_person update_sharing_policies @assay update_annotations(params[:tag_list], @assay) update_relationships(@assay, params) @@ -103,17 +96,16 @@ def create respond_to do |format| flash[:notice] = "#{t('assays.assay')} was successfully created." format.html { redirect_to(@assay) } - format.json {render json: @assay, include: [params[:include]]} + format.json { render json: @assay, include: [params[:include]] } end else respond_to do |format| - format.html { render :action => "new", status: :unprocessable_entity } + format.html { render action: 'new', status: :unprocessable_entity } format.json { render json: json_api_errors(@assay), status: :unprocessable_entity } end end end - def delete_linked_sample_types return unless is_single_page_assay? @@ -146,26 +138,26 @@ def update if @assay.update(assay_params) flash[:notice] = "#{t('assays.assay')} was successfully updated." format.html { redirect_to(@assay) } - format.json {render json: @assay, include: [params[:include]]} + format.json { render json: @assay, include: [params[:include]] } else - format.html { render :action => "edit", status: :unprocessable_entity } + format.html { render action: 'edit', status: :unprocessable_entity } format.json { render json: json_api_errors(@assay), status: :unprocessable_entity } end end end - def update_assay_organisms assay,params + def update_assay_organisms(assay, params) organisms = params[:assay_organism_ids] || params[:assay][:organism_ids] || [] assay.assay_organisms = [] # This means new AssayOrganisms are created every time the assay is updated! Array(organisms).each do |text| # TODO: Refactor this to use proper nested params: - o_id, strain,strain_id,culture_growth_type_text,t_id,t_title=text.split(",") - culture_growth=CultureGrowthType.find_by_title(culture_growth_type_text) - assay.associate_organism(o_id, strain_id, culture_growth,t_id,t_title) + o_id, strain, strain_id, culture_growth_type_text, t_id, t_title = text.split(',') + culture_growth = CultureGrowthType.find_by_title(culture_growth_type_text) + assay.associate_organism(o_id, strain_id, culture_growth, t_id, t_title) end end - def update_assay_human_diseases assay,params + def update_assay_human_diseases(assay, params) human_diseases = params[:assay_human_disease_ids] || params[:assay][:human_disease_ids] || [] assay.assay_human_diseases = [] Array(human_diseases).each do |human_disease_id| @@ -175,10 +167,9 @@ def update_assay_human_diseases assay,params def show respond_to do |format| - format.html { render(params[:only_content] ? { layout: false } : {})} - format.rdf { render :template=>'rdf/show'} - format.json {render json: @assay, include: [params[:include]]} - + format.html { render(params[:only_content] ? { layout: false } : {}) } + format.rdf { render template: 'rdf/show' } + format.json { render json: @assay, include: [params[:include]] } end end @@ -186,16 +177,19 @@ def show def assay_params params.require(:assay).permit(:title, :description, :study_id, :assay_class_id, :assay_type_uri, :technology_type_uri, - :license, *creator_related_params, :position, { document_ids: []}, + :license, *creator_related_params, :position, { document_ids: [] }, { sop_ids: [] }, { model_ids: [] }, - { samples_attributes: [:asset_id, :direction] }, - { data_files_attributes: [:asset_id, :direction, :relationship_type_id] }, - { placeholders_attributes: [:asset_id, :direction, :relationship_type_id] }, + { samples_attributes: %i[asset_id direction] }, + { data_files_attributes: %i[asset_id direction relationship_type_id] }, + { placeholders_attributes: %i[asset_id direction relationship_type_id] }, { publication_ids: [] }, { custom_metadata_attributes: determine_custom_metadata_keys }, - { discussion_links_attributes:[:id, :url, :label, :_destroy] } - ).tap do |assay_params| - assay_params[:document_ids].select! { |id| Document.find_by_id(id).try(:can_view?) } if assay_params.key?(:document_ids) + { discussion_links_attributes: %i[id url label _destroy] }).tap do |assay_params| + if assay_params.key?(:document_ids) + assay_params[:document_ids].select! do |id| + Document.find_by_id(id).try(:can_view?) + end + end assay_params[:sop_ids].select! { |id| Sop.find_by_id(id).try(:can_view?) } if assay_params.key?(:sop_ids) assay_params[:model_ids].select! { |id| Model.find_by_id(id).try(:can_view?) } if assay_params.key?(:model_ids) end From 2279c2ca896518c8da561b7a201ba0528feec2e7 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Sun, 15 Oct 2023 20:57:38 +0200 Subject: [PATCH 4/4] Write test for deleting middle assay --- app/controllers/assays_controller.rb | 12 +++--- app/models/assay.rb | 6 ++- test/functional/assays_controller_test.rb | 49 ++++++++++++++++++++++- 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index e441bd0532..858a12adae 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -116,13 +116,15 @@ def fix_assay_linkage return unless is_single_page_assay? return unless @assay.has_linked_child_assay? - previous_assay_linked_st_id = @assay.sample_type.sample_attributes.first.linked_sample_type_id + previous_assay_linked_st_id = @assay.previous_linked_assay_sample_type&.id + + next_assay = Assay.all.detect do |a| + a.sample_type&.sample_attributes&.first&.linked_sample_type_id == @assay.sample_type_id + end - next_assay = Assay.all.select do |a| - a.sample_type&.sample_attributes&.first&.linked_sample_type_id == @assay.sample_type_id - end&.first next_assay_st_attr = next_assay.sample_type&.sample_attributes&.first - return unless next_assay || previous_assay_linked_st_id || next_assay_st_attr + + return unless next_assay && previous_assay_linked_st_id && next_assay_st_attr next_assay_st_attr.update(linked_sample_type_id: previous_assay_linked_st_id) end diff --git a/app/models/assay.rb b/app/models/assay.rb index ab24b065a4..11805ed5d8 100644 --- a/app/models/assay.rb +++ b/app/models/assay.rb @@ -65,8 +65,12 @@ class Assay < ApplicationRecord enforce_authorization_on_association :study, :view + def previous_linked_assay_sample_type + sample_type.sample_attributes.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }&.linked_sample_type + end + def has_linked_child_assay? - sample_type.linked_sample_attributes.any? + sample_type&.linked_sample_attributes&.any? end def default_contributor diff --git a/test/functional/assays_controller_test.rb b/test/functional/assays_controller_test.rb index 86b8c10ef0..3aaee4f9f1 100644 --- a/test/functional/assays_controller_test.rb +++ b/test/functional/assays_controller_test.rb @@ -1923,7 +1923,12 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links test 'should delete empty assay with linked sample type' do person = FactoryBot.create(:person) - assay_sample_type = FactoryBot.create :linked_sample_type, contributor: person + project = person.projects.first + source_st = FactoryBot.create(:isa_source_sample_type, contributor: person, projects: [project]) + sample_collection_st = FactoryBot.create(:isa_sample_collection_sample_type, contributor: person, projects: [project], + linked_sample_type: source_st) + + assay_sample_type = FactoryBot.create :isa_assay_sample_type, linked_sample_type: sample_collection_st, contributor: person, isa_template: Template.find_by_title('ISA Assay 1') assay = FactoryBot.create(:assay, policy:FactoryBot.create(:private_policy, permissions:[FactoryBot.create(:permission,contributor: person, access_type:Policy::EDITING)]), sample_type: assay_sample_type, @@ -1937,4 +1942,46 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links end end end + + test 'should fix sample type linkage when middle assay is deleted' do + # person = User.current_user.person + person = FactoryBot.create(:person) + project = person.projects.first + investigation = FactoryBot.create(:investigation, projects: [project]) + + source_st = FactoryBot.create(:isa_source_sample_type, contributor: person, projects: [project]) + sample_collection_st = FactoryBot.create(:isa_sample_collection_sample_type, contributor: person, projects: [project], + linked_sample_type: source_st) + + assay_st1 = FactoryBot.create(:isa_assay_sample_type, contributor: person, projects: [project], + linked_sample_type: sample_collection_st, isa_template: Template.find_by_title('ISA Assay 1')) + + assay_st2 = FactoryBot.create(:isa_assay_sample_type, contributor: person, projects: [project], + linked_sample_type: assay_st1, isa_template: Template.find_by_title('ISA Assay 1')) + + assay_st3 = FactoryBot.create(:isa_assay_sample_type, contributor: person, projects: [project], + linked_sample_type: assay_st2, isa_template: Template.find_by_title('ISA Assay 1')) + + study = FactoryBot.create(:study, investigation:, contributor: person, + policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)]), + sops: [FactoryBot.create(:sop, policy: FactoryBot.create(:public_policy))], + sample_types: [source_st, sample_collection_st]) + + assay1 = FactoryBot.create(:assay, study:, contributor: person, sample_type: assay_st1, + policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)])) + assay2 = FactoryBot.create(:assay, study:, contributor: person, sample_type: assay_st2, + policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)])) + assay3 = FactoryBot.create(:assay, study:, contributor: person, sample_type: assay_st3, + policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)])) + + login_as(person) + + assert_difference "Assay.count", -1 do + delete :destroy, params: { id: assay2.id, return_to: '/single_pages/' } + end + + assay3.reload + + assert_equal(assay3.previous_linked_assay_sample_type&.id, assay1.sample_type&.id) + end end