From f994a3620311fb2d546e20b81a123c297d4b76a6 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 23 Jan 2024 16:36:07 +0100 Subject: [PATCH 01/49] Replace `is_single_page_assay?` by `is_isa_json_compliant?` --- app/controllers/assays_controller.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 007e3f7da9..a61deef54b 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -107,14 +107,14 @@ def create end def delete_linked_sample_types - return unless is_single_page_assay? + return unless @assay.is_isa_json_compliant? return if @assay.sample_type.nil? @assay.sample_type.destroy end def fix_assay_linkage - return unless is_single_page_assay? + return unless @assay.is_isa_json_compliant? return unless @assay.has_linked_child_assay? previous_assay_linked_st_id = @assay.previous_linked_sample_type&.id @@ -194,10 +194,4 @@ def assay_params assay_params[:model_ids].select! { |id| Model.find_by_id(id).try(:can_view?) } if assay_params.key?(:model_ids) end end - - def is_single_page_assay? - return false unless params.key?(:return_to) - - params[:return_to].start_with? '/single_pages/' - end end From a1faa5d8426b9fb67ce9ffb305555b14ba8462a4 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jan 2024 14:33:12 +0100 Subject: [PATCH 02/49] Add fix linkage to isa assays after a new one is created. --- app/controllers/isa_assays_controller.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/controllers/isa_assays_controller.rb b/app/controllers/isa_assays_controller.rb index cc0c8128ed..29df51f069 100644 --- a/app/controllers/isa_assays_controller.rb +++ b/app/controllers/isa_assays_controller.rb @@ -4,6 +4,7 @@ class IsaAssaysController < ApplicationController before_action :set_up_instance_variable before_action :find_requested_item, only: %i[edit update] + after_action :fix_assay_linkage_for_new_assays, only: :create def new if params[:is_assay_stream] @@ -64,6 +65,20 @@ def update end end + def fix_assay_linkage_for_new_assays + return unless @isa_assay.assay.is_isa_json_compliant? + + previous_assay_st = @isa_assay.assay.previous_linked_sample_type + previous_assay = previous_assay_st.assays.first + next_assay = previous_assay.next_linked_child_assay + next_assay.update(position: @isa_assay.assay.position + 1) + return unless next_assay + + current_assay_st = @isa_assay.assay.sample_type + previous_assay_st.update(linked_sample_attribute_ids: [@isa_assay.assay.sample_type.sample_attributes.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }.id]) + current_assay_st.update(linked_sample_attribute_ids: [next_assay.sample_type.sample_attributes.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }.id]) + end + private def isa_assay_params From f8c22007e0579f938de51b9137014912868d4813 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Wed, 24 Jan 2024 14:56:41 +0100 Subject: [PATCH 03/49] Order assays by position in assay_stream --- lib/treeview_builder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/treeview_builder.rb b/lib/treeview_builder.rb index 6f89ddf2d3..b810759ebb 100644 --- a/lib/treeview_builder.rb +++ b/lib/treeview_builder.rb @@ -17,7 +17,7 @@ def build_tree_data if investigation.is_isa_json_compliant? investigation.studies.map do |study| assay_stream_items = study.assay_streams.map do |assay_stream| - assay_items = assay_stream.child_assays.map do |child_assay| + assay_items = assay_stream.child_assays.order(:position).map do |child_assay| build_assay_item(child_assay) end build_assay_stream_item(assay_stream, assay_items) From 3e0237bdae936be963de3d142f1f0b0637c82b9f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 25 Jan 2024 15:27:26 +0100 Subject: [PATCH 04/49] Create function to find the input attribute --- app/models/sample_attribute.rb | 4 ++++ test/unit/sample_attribute_test.rb | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/app/models/sample_attribute.rb b/app/models/sample_attribute.rb index bf117fe9cc..97707ea259 100644 --- a/app/models/sample_attribute.rb +++ b/app/models/sample_attribute.rb @@ -25,6 +25,10 @@ class SampleAttribute < ApplicationRecord # whether this attribute is tied to a controlled vocab which is based on an ontology delegate :ontology_based?, to: :sample_controlled_vocab, allow_nil: true + def input_attribute? + isa_tag.nil? && title.downcase.include?('input') && seek_sample_multi? + end + def title=(title) super store_accessor_name diff --git a/test/unit/sample_attribute_test.rb b/test/unit/sample_attribute_test.rb index 8951913607..a9a045473f 100644 --- a/test/unit/sample_attribute_test.rb +++ b/test/unit/sample_attribute_test.rb @@ -513,6 +513,18 @@ class SampleAttributeTest < ActiveSupport::TestCase refute attribute.validate_value?('moonbeam') end + test 'is input attribute?' do + correct_input_attribute = FactoryBot.create(:sample_multi_sample_attribute, title: 'Input from previous sample type', isa_tag: nil, is_title: true, sample_type: FactoryBot.create(:simple_sample_type)) + assert correct_input_attribute.input_attribute? + + incorrect_input_attribute = FactoryBot.create(:sample_multi_sample_attribute, title: 'Input from previous sample type', isa_tag: FactoryBot.create(:default_isa_tag), is_title: true, sample_type: FactoryBot.create(:simple_sample_type)) + refute incorrect_input_attribute.input_attribute? + second_incorrect_input_attribute = FactoryBot.create(:sample_multi_sample_attribute, title: 'Ingoing material', isa_tag: nil, is_title: true, sample_type: FactoryBot.create(:simple_sample_type)) + refute second_incorrect_input_attribute.input_attribute? + third_incorrect_input_attribute = FactoryBot.create(:sample_sample_attribute, title: 'Input from previous sample type', isa_tag: nil, is_title: true, sample_type: FactoryBot.create(:simple_sample_type)) + refute third_incorrect_input_attribute.input_attribute? + end + private def valid_value?(attribute, value) From 8b5db61c6b161f5ae61edaab7b0cb50113d1cc5c Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 25 Jan 2024 15:28:47 +0100 Subject: [PATCH 05/49] Add functions to find the next and previous linked sample type --- app/models/sample_type.rb | 8 ++++++++ test/unit/sample_type_test.rb | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 01523e171d..729db7b338 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -60,6 +60,14 @@ class SampleType < ApplicationRecord has_annotation_type :sample_type_tag, method_name: :tags + def previous_linked_sample_type + sample_attributes.detect(&:input_attribute?)&.linked_sample_type + end + + def next_linked_sample_types + linked_sample_attributes.select(&:input_attribute?).map(&:sample_type).compact + end + def is_isa_json_compliant? studies.any? || assays.any? end diff --git a/test/unit/sample_type_test.rb b/test/unit/sample_type_test.rb index 69e73420dc..89ce2b81d6 100644 --- a/test/unit/sample_type_test.rb +++ b/test/unit/sample_type_test.rb @@ -1118,6 +1118,26 @@ def setup end + test 'previous linked sample type' do + first_sample_type = FactoryBot.create(:isa_source_sample_type) + second_sample_type = FactoryBot.create(:isa_sample_collection_sample_type, linked_sample_type: first_sample_type) + third_sample_type = FactoryBot.create(:isa_assay_material_sample_type, linked_sample_type: second_sample_type) + + assert_equal second_sample_type.previous_linked_sample_type, first_sample_type + refute_equal third_sample_type.previous_linked_sample_type, first_sample_type + refute_equal first_sample_type.previous_linked_sample_type, second_sample_type + end + + test 'next linked sample types' do + first_sample_type = FactoryBot.create(:isa_source_sample_type) + second_sample_type = FactoryBot.create(:isa_sample_collection_sample_type, linked_sample_type: first_sample_type) + third_sample_type = FactoryBot.create(:isa_assay_material_sample_type, linked_sample_type: second_sample_type) + + assert_equal first_sample_type.next_linked_sample_types, [second_sample_type] + assert_equal second_sample_type.next_linked_sample_types, [third_sample_type] + assert third_sample_type.next_linked_sample_types.blank? + end + private # sample type with 3 samples From 6b064775ed82d843c1ad23a67e0c11c4bd8d0128 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 26 Jan 2024 10:48:20 +0100 Subject: [PATCH 06/49] Simplify dynamic table helper --- app/helpers/dynamic_table_helper.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/dynamic_table_helper.rb b/app/helpers/dynamic_table_helper.rb index 6bdacab953..5a547108bd 100644 --- a/app/helpers/dynamic_table_helper.rb +++ b/app/helpers/dynamic_table_helper.rb @@ -22,7 +22,7 @@ def dt_aggregated(study, assay = nil) # Links all sample_types in a sequence of sample_types def link_sequence(sample_type) sequence = [sample_type] - while link = sample_type.sample_attributes.detect(&:seek_sample_multi?)&.linked_sample_type + while link = sample_type.previous_linked_sample_type sequence << link sample_type = link end @@ -96,8 +96,8 @@ def dt_cols(sample_type) is_seek_multi_sample = a.sample_attribute_type.base_type == Seek::Samples::BaseType::SEEK_SAMPLE_MULTI is_cv_list = a.sample_attribute_type.base_type == Seek::Samples::BaseType::CV_LIST cv_allows_free_text = a.allow_cv_free_text - attribute.merge!({ multi_link: true, linked_sample_type: a.linked_sample_type.id }) if is_seek_multi_sample - attribute.merge!({ multi_link: false, linked_sample_type: a.linked_sample_type.id }) if is_seek_sample + attribute.merge!({ multi_link: true, linked_sample_type: a.linked_sample_type_id }) if is_seek_multi_sample + attribute.merge!({ multi_link: false, linked_sample_type: a.linked_sample_type_id }) if is_seek_sample attribute.merge!({is_cv_list: , cv_allows_free_text:}) if is_cv_list attribute end From ce06b7c27443018841e4f8da12a6779353b74d0b Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 26 Jan 2024 11:24:27 +0100 Subject: [PATCH 07/49] Fix sample type linkage when inserting new assays --- app/controllers/assays_controller.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index a61deef54b..2252496db8 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -13,6 +13,9 @@ class AssaysController < ApplicationController # defined in the application controller before_action :project_membership_required_appended, only: [:new_object_based_on_existing_one] + # Only for ISA JSON compliant assays => Fix sample type linkage + before_action :fix_assay_linkage_for_new_assays, only: :create + include Seek::Publishing::PublishingCommon include Seek::IsaGraphExtensions @@ -178,6 +181,21 @@ def show private + def fix_assay_linkage_for_new_assays + return unless @isa_assay.assay.is_isa_json_compliant? + + previous_assay_st = @isa_assay.assay.sample_type.previous_linked_sample_type + previous_assay = previous_assay_st.assays.first + next_assay = previous_assay.next_linked_child_assay + + # In case no next assay (an assay was appended to the end of the stream), assay linkage does not have to be fixed. + return unless next_assay + + @isa_assay.assay.sample_type.linked_sample_attribute_ids = [next_assay.sample_type.sample_attributes.detect(&:input_attribute?).id] + previous_assay_st.update(linked_sample_attribute_ids: [@isa_assay.assay.sample_type.sample_attributes.detect(&:input_attribute?).id]) + end + + 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: [] }, From 69a4ef698e3d07f56857e8675d89ff711dd9be18 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 26 Jan 2024 11:25:27 +0100 Subject: [PATCH 08/49] Rearrange assay positions when inserting or deleting assays --- app/controllers/assays_controller.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 2252496db8..7a88327cbe 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -15,6 +15,7 @@ class AssaysController < ApplicationController # Only for ISA JSON compliant assays => Fix sample type linkage before_action :fix_assay_linkage_for_new_assays, only: :create + after_action :rearrange_assay_positions, only: [:create, :destroy] include Seek::Publishing::PublishingCommon @@ -195,6 +196,18 @@ def fix_assay_linkage_for_new_assays previous_assay_st.update(linked_sample_attribute_ids: [@isa_assay.assay.sample_type.sample_attributes.detect(&:input_attribute?).id]) end + def rearrange_assay_positions + current_assay_stream = @assay.assay_stream + next_assay = current_assay_stream.next_linked_child_assay + assay_position = 0 + + # While there is a next assay, increment position by + while next_assay + next_assay.update(position: assay_position) + next_assay = next_assay.next_linked_child_assay + assay_position += 1 + end + end def assay_params params.require(:assay).permit(:title, :description, :study_id, :assay_class_id, :assay_type_uri, :technology_type_uri, From 61c374f2c0ed60e535d25069e33ad4624b233c19 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 26 Jan 2024 11:30:01 +0100 Subject: [PATCH 09/49] Move code to private section --- app/controllers/assays_controller.rb | 61 +++++++++++++++------------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 7a88327cbe..482852abca 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -6,16 +6,19 @@ class AssaysController < ApplicationController 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 # defined in the application controller before_action :project_membership_required_appended, only: [:new_object_based_on_existing_one] - # Only for ISA JSON compliant assays => Fix sample type linkage + # Only for ISA JSON compliant assays + # => Delete sample type of deleted assay + before_action :fix_assay_linkage_when_deleting_assays, only: [:destroy] + # => Fix sample type linkage + before_action :delete_linked_sample_types, only: [:destroy] before_action :fix_assay_linkage_for_new_assays, only: :create - after_action :rearrange_assay_positions, only: [:create, :destroy] + # => Rearrange positions + after_action :rearrange_assay_positions, only: %i[create destroy] include Seek::Publishing::PublishingCommon @@ -110,30 +113,6 @@ def create end end - def delete_linked_sample_types - return unless @assay.is_isa_json_compliant? - return if @assay.sample_type.nil? - - @assay.sample_type.destroy - end - - def fix_assay_linkage - return unless @assay.is_isa_json_compliant? - return unless @assay.has_linked_child_assay? - - previous_assay_linked_st_id = @assay.previous_linked_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_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 @@ -182,7 +161,31 @@ def show private - def fix_assay_linkage_for_new_assays + def delete_linked_sample_types + return unless @assay.is_isa_json_compliant? + return if @assay.sample_type.nil? + + @assay.sample_type.destroy + end + + def fix_assay_linkage_when_deleting_assays + return unless @assay.is_isa_json_compliant? + return unless @assay.has_linked_child_assay? + + previous_assay_linked_st_id = @assay.previous_linked_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_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 fix_assay_linkage_for_new_assays return unless @isa_assay.assay.is_isa_json_compliant? previous_assay_st = @isa_assay.assay.sample_type.previous_linked_sample_type From bdd97c499effa46a72d874e7af38dc66e20a4d26 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 26 Jan 2024 11:54:17 +0100 Subject: [PATCH 10/49] simplify assay linkage when deleting --- app/controllers/assays_controller.rb | 4 +--- app/models/assay.rb | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 482852abca..0157bcfb12 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -174,9 +174,7 @@ def fix_assay_linkage_when_deleting_assays previous_assay_linked_st_id = @assay.previous_linked_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.next_linked_child_assay next_assay_st_attr = next_assay.sample_type&.sample_attributes&.first diff --git a/app/models/assay.rb b/app/models/assay.rb index ab9e5b95e6..16efa73edb 100644 --- a/app/models/assay.rb +++ b/app/models/assay.rb @@ -78,7 +78,7 @@ def previous_linked_sample_type if is_assay_stream? study.sample_types.second else - sample_type.sample_attributes.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }&.linked_sample_type + sample_type.previous_linked_sample_type end end From af66b5396975fe7c2b62986d2da8863debefc93f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 26 Jan 2024 13:16:23 +0100 Subject: [PATCH 11/49] Split functionality over assay and isa assay --- app/controllers/assays_controller.rb | 34 ++++------------------- app/controllers/isa_assays_controller.rb | 35 ++++++++++++++++-------- lib/seek/assets_common.rb | 16 ++++++++++- 3 files changed, 43 insertions(+), 42 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 0157bcfb12..5c0d89f294 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -13,12 +13,11 @@ class AssaysController < ApplicationController # Only for ISA JSON compliant assays # => Delete sample type of deleted assay - before_action :fix_assay_linkage_when_deleting_assays, only: [:destroy] + before_action :delete_linked_sample_types, only: :destroy # => Fix sample type linkage - before_action :delete_linked_sample_types, only: [:destroy] - before_action :fix_assay_linkage_for_new_assays, only: :create + before_action :fix_assay_linkage_when_deleting_assays, only: :destroy # => Rearrange positions - after_action :rearrange_assay_positions, only: %i[create destroy] + after_action :rearrange_assay_positions_at_destroy, only: :destroy include Seek::Publishing::PublishingCommon @@ -183,31 +182,8 @@ def fix_assay_linkage_when_deleting_assays next_assay_st_attr.update(linked_sample_type_id: previous_assay_linked_st_id) end -def fix_assay_linkage_for_new_assays - return unless @isa_assay.assay.is_isa_json_compliant? - - previous_assay_st = @isa_assay.assay.sample_type.previous_linked_sample_type - previous_assay = previous_assay_st.assays.first - next_assay = previous_assay.next_linked_child_assay - - # In case no next assay (an assay was appended to the end of the stream), assay linkage does not have to be fixed. - return unless next_assay - - @isa_assay.assay.sample_type.linked_sample_attribute_ids = [next_assay.sample_type.sample_attributes.detect(&:input_attribute?).id] - previous_assay_st.update(linked_sample_attribute_ids: [@isa_assay.assay.sample_type.sample_attributes.detect(&:input_attribute?).id]) - end - - def rearrange_assay_positions - current_assay_stream = @assay.assay_stream - next_assay = current_assay_stream.next_linked_child_assay - assay_position = 0 - - # While there is a next assay, increment position by - while next_assay - next_assay.update(position: assay_position) - next_assay = next_assay.next_linked_child_assay - assay_position += 1 - end + def rearrange_assay_positions_at_destroy + rearrange_assay_positions(@assay.assay_stream) end def assay_params diff --git a/app/controllers/isa_assays_controller.rb b/app/controllers/isa_assays_controller.rb index 29df51f069..b1af5583a7 100644 --- a/app/controllers/isa_assays_controller.rb +++ b/app/controllers/isa_assays_controller.rb @@ -4,7 +4,9 @@ class IsaAssaysController < ApplicationController before_action :set_up_instance_variable before_action :find_requested_item, only: %i[edit update] - after_action :fix_assay_linkage_for_new_assays, only: :create + before_action :initialize_isa_assay, only: :create + before_action :fix_assay_linkage_for_new_assays, only: :create + after_action :rearrange_assay_positions_create_isa_assay, only: :create def new if params[:is_assay_stream] @@ -15,10 +17,6 @@ def new end def create - @isa_assay = IsaAssay.new(isa_assay_params) - update_sharing_policies @isa_assay.assay - @isa_assay.assay.contributor = current_person - @isa_assay.sample_type.contributor = User.current_user.person if isa_assay_params[:sample_type] if @isa_assay.save redirect_to single_page_path(id: @isa_assay.assay.projects.first, item_type: 'assay', item_id: @isa_assay.assay, notice: 'The ISA assay was created successfully!') @@ -65,21 +63,34 @@ def update end end + private + def fix_assay_linkage_for_new_assays return unless @isa_assay.assay.is_isa_json_compliant? - previous_assay_st = @isa_assay.assay.previous_linked_sample_type + previous_assay_st = @isa_assay.assay.sample_type.previous_linked_sample_type previous_assay = previous_assay_st.assays.first - next_assay = previous_assay.next_linked_child_assay - next_assay.update(position: @isa_assay.assay.position + 1) + # In case an assay is inserted right at the beginning of an assay stream, + # the next assay is the current first one in the assay stream. + next_assay = previous_assay.nil? ? @isa_assay.assay.assay_stream.next_linked_child_assay : previous_assay.next_linked_child_assay + + # In case no next assay (an assay was appended to the end of the stream), assay linkage does not have to be fixed. return unless next_assay - current_assay_st = @isa_assay.assay.sample_type - previous_assay_st.update(linked_sample_attribute_ids: [@isa_assay.assay.sample_type.sample_attributes.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }.id]) - current_assay_st.update(linked_sample_attribute_ids: [next_assay.sample_type.sample_attributes.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }.id]) + @isa_assay.assay.sample_type.linked_sample_attribute_ids = [next_assay.sample_type.sample_attributes.detect(&:input_attribute?).id] + previous_assay_st.update(linked_sample_attribute_ids: [@isa_assay.assay.sample_type.sample_attributes.detect(&:input_attribute?).id]) end - private + def rearrange_assay_positions_create_isa_assay + rearrange_assay_positions(@isa_assay.assay.assay_stream) + end + + def initialize_isa_assay + @isa_assay = IsaAssay.new(isa_assay_params) + update_sharing_policies @isa_assay.assay + @isa_assay.assay.contributor = current_person + @isa_assay.sample_type.contributor = User.current_user.person if isa_assay_params[:sample_type] + end def isa_assay_params # TODO: get the params from a shared module diff --git a/lib/seek/assets_common.rb b/lib/seek/assets_common.rb index e72fbf3859..b528b19ac4 100644 --- a/lib/seek/assets_common.rb +++ b/lib/seek/assets_common.rb @@ -29,7 +29,7 @@ def find_display_asset(asset = instance_variable_get("@#{controller_name.singula def update_relationships(asset, params) Relationship.set_attributions(asset, params[:attributions]) end - + def request_contact resource = class_for_controller_name.find(params[:id]) details = params[:details] @@ -74,5 +74,19 @@ def params_for_controller method = "#{name}_params" send(method) end + + def rearrange_assay_positions(assay_stream) + disable_authorization_checks do + next_assay = assay_stream.next_linked_child_assay + assay_position = 0 + + # While there is a next assay, increment position by + while next_assay + next_assay.update(position: assay_position) + next_assay = next_assay.next_linked_child_assay + assay_position += 1 + end + end + end end end From e1c790357f38af2eeea3271b59b2728c767d929f Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 29 Jan 2024 07:57:45 +0100 Subject: [PATCH 12/49] Delete ST first, fix linkage later --- app/controllers/assays_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/assays_controller.rb b/app/controllers/assays_controller.rb index 5c0d89f294..140b5c0dd6 100644 --- a/app/controllers/assays_controller.rb +++ b/app/controllers/assays_controller.rb @@ -12,10 +12,10 @@ class AssaysController < ApplicationController before_action :project_membership_required_appended, only: [:new_object_based_on_existing_one] # Only for ISA JSON compliant assays - # => Delete sample type of deleted assay - before_action :delete_linked_sample_types, only: :destroy # => Fix sample type linkage before_action :fix_assay_linkage_when_deleting_assays, only: :destroy + # => Delete sample type of deleted assay + before_action :delete_linked_sample_types, only: :destroy # => Rearrange positions after_action :rearrange_assay_positions_at_destroy, only: :destroy @@ -175,7 +175,7 @@ def fix_assay_linkage_when_deleting_assays next_assay = @assay.next_linked_child_assay - next_assay_st_attr = next_assay.sample_type&.sample_attributes&.first + next_assay_st_attr = next_assay.sample_type&.sample_attributes&.detect(&:input_attribute?) return unless next_assay && previous_assay_linked_st_id && next_assay_st_attr From f4b6783055a2a72fd9643ce03f189540edfaedad Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 29 Jan 2024 07:58:33 +0100 Subject: [PATCH 13/49] Simplify deletion of linked sampletypes in studies --- app/controllers/studies_controller.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/app/controllers/studies_controller.rb b/app/controllers/studies_controller.rb index 2849b2f1a8..642494ae57 100644 --- a/app/controllers/studies_controller.rb +++ b/app/controllers/studies_controller.rb @@ -89,7 +89,7 @@ def update end def delete_linked_sample_types - return unless is_single_page_study? + return unless @study.is_isa_json_compliant? return if @study.sample_types.empty? # The study sample types must be destroyed in reversed order @@ -361,9 +361,3 @@ def study_params { extended_metadata_attributes: determine_extended_metadata_keys }) end end - -def is_single_page_study? - return false unless params.key?(:return_to) - - params[:return_to].start_with? '/single_pages/' -end From 532c9f3a7de07c6a8900050b33a15b8d75490796 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 29 Jan 2024 08:00:06 +0100 Subject: [PATCH 14/49] Fix existing assays controller tests --- app/models/assay.rb | 6 +++- app/models/sample_type.rb | 16 ++++++--- lib/seek/assets_common.rb | 2 ++ test/functional/assays_controller_test.rb | 41 +++++++++++++++++------ 4 files changed, 49 insertions(+), 16 deletions(-) diff --git a/app/models/assay.rb b/app/models/assay.rb index 16efa73edb..76153b0729 100644 --- a/app/models/assay.rb +++ b/app/models/assay.rb @@ -75,7 +75,7 @@ def is_assay_stream? def previous_linked_sample_type return unless is_isa_json_compliant? - if is_assay_stream? + if is_assay_stream? || first_assay_in_stream? study.sample_types.second else sample_type.previous_linked_sample_type @@ -102,6 +102,10 @@ def next_linked_child_assay end end + def first_assay_in_stream? + self == assay_stream.child_assays.first + end + def default_contributor User.current_user.try :person end diff --git a/app/models/sample_type.rb b/app/models/sample_type.rb index 729db7b338..36648afa04 100644 --- a/app/models/sample_type.rb +++ b/app/models/sample_type.rb @@ -128,11 +128,17 @@ def can_edit?(user = User.current_user) end def can_delete?(user = User.current_user) - can_edit?(user) && samples.empty? && - linked_sample_attributes.detect do |attr| - attr.sample_type && - attr.sample_type != self - end.nil? + # 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. + if is_isa_json_compliant? + can_edit?(user) && samples.empty? + else + can_edit?(user) && samples.empty? && + linked_sample_attributes.detect do |attr| + attr.sample_type && + attr.sample_type != self + end.nil? + end end def can_view?(user = User.current_user, referring_sample = nil, view_in_single_page = false) diff --git a/lib/seek/assets_common.rb b/lib/seek/assets_common.rb index b528b19ac4..226ad2407f 100644 --- a/lib/seek/assets_common.rb +++ b/lib/seek/assets_common.rb @@ -76,6 +76,8 @@ def params_for_controller end def rearrange_assay_positions(assay_stream) + return unless assay_stream + disable_authorization_checks do next_assay = assay_stream.next_linked_child_assay assay_position = 0 diff --git a/test/functional/assays_controller_test.rb b/test/functional/assays_controller_test.rb index 1996d36918..dae23793ae 100644 --- a/test/functional/assays_controller_test.rb +++ b/test/functional/assays_controller_test.rb @@ -1924,21 +1924,35 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links test 'should delete empty assay with linked sample type' do person = FactoryBot.create(:person) project = person.projects.first + investigation = FactoryBot.create(:investigation, projects: [project], is_isa_json_compliant: true, contributor: person) 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_material_sample_type, linked_sample_type: sample_collection_st, contributor: person, isa_template: FactoryBot.build(:isa_assay_material_template) + 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]) + assay_stream = FactoryBot.create(:assay_stream, study: , contributor: person) + assay_sample_type = FactoryBot.create :isa_assay_material_sample_type, linked_sample_type: sample_collection_st, + contributor: person, isa_template: FactoryBot.build(:isa_assay_material_template) assay = FactoryBot.create(:assay, - policy:FactoryBot.create(:private_policy, permissions:[FactoryBot.create(:permission,contributor: person, access_type:Policy::EDITING)]), + study: , + policy: FactoryBot.create(:private_policy, permissions:[FactoryBot.create(:permission,contributor: person, access_type:Policy::EDITING)]), sample_type: assay_sample_type, - contributor: person) + contributor: person, + assay_stream: ) + login_as(person) + assert assay.is_isa_json_compliant? + assert assay.sample_type.is_isa_json_compliant? + assert assay.sample_type.can_delete? + assert_difference('SampleType.count', -1) do assert_difference('Assay.count', -1) do - delete :destroy, params: { id: assay.id, return_to: '/single_pages/' } + delete :destroy, params: { id: assay.id } end end end @@ -1969,22 +1983,29 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links sops: [FactoryBot.create(:sop, policy: FactoryBot.create(:public_policy))], sample_types: [source_st, sample_collection_st]) - assay1 = FactoryBot.create(:assay, study: study, contributor: person, sample_type: assay_st1, - policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)])) + assay_stream = FactoryBot.create(:assay_stream, study: , contributor: person) + 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)]), + position: 0, assay_stream: ) assay2 = FactoryBot.create(:assay, study: study, contributor: person, sample_type: assay_st2, - policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)])) + policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)]), + position: 1, assay_stream: ) assay3 = FactoryBot.create(:assay, study: study, contributor: person, sample_type: assay_st3, - policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)])) + policy: FactoryBot.create(:private_policy, permissions: [FactoryBot.create(:permission, contributor: person, access_type: Policy::MANAGING)]), + position: 2, assay_stream: ) login_as(person) - assert_difference "Assay.count", -1 do - delete :destroy, params: { id: assay2.id, return_to: '/single_pages/' } + assert_difference("SampleType.count", -1) do + assert_difference("Assay.count", -1) do + delete :destroy, params: { id: assay2.id } + end end assay3.reload assert_equal(assay3.previous_linked_sample_type&.id, assay1.sample_type&.id) + assert_equal assay3.position, 1 end test 'do not get index if feature disabled' do From 88c205389d2e4b2311384a0e37886eaaeddae795 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Mon, 29 Jan 2024 08:09:38 +0100 Subject: [PATCH 15/49] Fix existing studies_controller tests --- test/functional/studies_controller_test.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/functional/studies_controller_test.rb b/test/functional/studies_controller_test.rb index a29d1c8891..c022e8aa04 100644 --- a/test/functional/studies_controller_test.rb +++ b/test/functional/studies_controller_test.rb @@ -1976,15 +1976,19 @@ def test_should_show_investigation_tab test 'should delete empty study with linked sample type' do person = FactoryBot.create(:person) - study_source_sample_type = FactoryBot.create :linked_sample_type, contributor: person - study_sample_sample_type = FactoryBot.create :linked_sample_type, contributor: person + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true) + study_source_sample_type = FactoryBot.create :isa_source_sample_type, contributor: person + study_sample_sample_type = FactoryBot.create :isa_sample_collection_sample_type, linked_sample_type: study_source_sample_type, contributor: person study = FactoryBot.create(:study, + investigation: , policy:FactoryBot.create(:private_policy, permissions:[FactoryBot.create(:permission,contributor: person, access_type:Policy::EDITING)]), sample_types: [study_source_sample_type, study_sample_sample_type], contributor: person) login_as(person) + assert study.is_isa_json_compliant? + assert_difference('SampleType.count', -2) do assert_difference('Study.count', -1) do delete :destroy, params: { id: study.id, return_to: '/single_pages/' } From 50dfe2ac2ec62445ac07a6580874e5c1d5626e43 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 13:58:12 +0100 Subject: [PATCH 16/49] Correct sample type linkage --- app/controllers/isa_assays_controller.rb | 16 +++++++++++----- app/models/assay.rb | 4 ++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/controllers/isa_assays_controller.rb b/app/controllers/isa_assays_controller.rb index b1af5583a7..fd7b9d278b 100644 --- a/app/controllers/isa_assays_controller.rb +++ b/app/controllers/isa_assays_controller.rb @@ -5,7 +5,7 @@ class IsaAssaysController < ApplicationController before_action :set_up_instance_variable before_action :find_requested_item, only: %i[edit update] before_action :initialize_isa_assay, only: :create - before_action :fix_assay_linkage_for_new_assays, only: :create + after_action :fix_assay_linkage_for_new_assays, only: :create after_action :rearrange_assay_positions_create_isa_assay, only: :create def new @@ -67,9 +67,11 @@ def update def fix_assay_linkage_for_new_assays return unless @isa_assay.assay.is_isa_json_compliant? + return if @isa_assay.assay.is_assay_stream? # Should not fix anything when creating an assay stream + + previous_sample_type = SampleType.find(params[:isa_assay][:input_sample_type_id]) + previous_assay = previous_sample_type.assays.first - previous_assay_st = @isa_assay.assay.sample_type.previous_linked_sample_type - previous_assay = previous_assay_st.assays.first # In case an assay is inserted right at the beginning of an assay stream, # the next assay is the current first one in the assay stream. next_assay = previous_assay.nil? ? @isa_assay.assay.assay_stream.next_linked_child_assay : previous_assay.next_linked_child_assay @@ -77,8 +79,12 @@ def fix_assay_linkage_for_new_assays # In case no next assay (an assay was appended to the end of the stream), assay linkage does not have to be fixed. return unless next_assay - @isa_assay.assay.sample_type.linked_sample_attribute_ids = [next_assay.sample_type.sample_attributes.detect(&:input_attribute?).id] - previous_assay_st.update(linked_sample_attribute_ids: [@isa_assay.assay.sample_type.sample_attributes.detect(&:input_attribute?).id]) + next_assay_input_attribute_id = next_assay.sample_type.sample_attributes.detect(&:input_attribute?).id + return unless next_assay_input_attribute_id + + # Add link of next assay sample type to currently created assay sample type + updated_lsai = @isa_assay.assay.sample_type.linked_sample_attribute_ids.push(next_assay_input_attribute_id) + @isa_assay.assay.sample_type.update(linked_sample_attribute_ids: updated_lsai) end def rearrange_assay_positions_create_isa_assay diff --git a/app/models/assay.rb b/app/models/assay.rb index 76153b0729..ce591f9534 100644 --- a/app/models/assay.rb +++ b/app/models/assay.rb @@ -96,9 +96,9 @@ def next_linked_child_assay return unless has_linked_child_assay? if is_assay_stream? - previous_linked_sample_type&.linked_sample_attributes&.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }&.sample_type&.assays&.first + child_assays.first else - sample_type.linked_sample_attributes.detect { |sa| sa.isa_tag.nil? && sa.title.include?('Input') }&.sample_type&.assays&.first + sample_type.next_linked_sample_types.map(&:assays).flatten.detect { |a| a.assay_stream_id == assay_stream_id } end end From 4a6c0eb53ef67414ddec3fd63cbcc4cd99145e1d Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 13:58:46 +0100 Subject: [PATCH 17/49] Fix existing tests --- test/factories/studies.rb | 2 +- test/unit/assay_test.rb | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/test/factories/studies.rb b/test/factories/studies.rb index 470ed950e3..afb01507f0 100644 --- a/test/factories/studies.rb +++ b/test/factories/studies.rb @@ -41,7 +41,7 @@ title { 'ISA JSON compliant study' } description { 'A study which is linked to an ISA JSON compliant investigation and has two sample types linked to it.' } after(:build) do |study| - study.investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true) + study.investigation ||= FactoryBot.create(:investigation, is_isa_json_compliant: true) source_st = FactoryBot.create(:isa_source_sample_type) sample_collection_st = FactoryBot.create(:isa_sample_collection_sample_type, linked_sample_type: source_st) study.sample_types = [source_st, sample_collection_st] diff --git a/test/unit/assay_test.rb b/test/unit/assay_test.rb index f72a1e7d50..653d618db4 100644 --- a/test/unit/assay_test.rb +++ b/test/unit/assay_test.rb @@ -752,7 +752,8 @@ def new_valid_assay end test 'isa json compliance' do - isa_json_compliant_study = FactoryBot.create(:isa_json_compliant_study) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true) + isa_json_compliant_study = FactoryBot.create(:isa_json_compliant_study, investigation: ) assert isa_json_compliant_study.is_isa_json_compliant? default_assay = FactoryBot.create(:assay, study: isa_json_compliant_study) @@ -775,10 +776,13 @@ def new_valid_assay end test 'previous linked sample type' do - isa_study = FactoryBot.create(:isa_json_compliant_study) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true) + isa_study = FactoryBot.create(:isa_json_compliant_study, investigation: ) def_study = FactoryBot.create(:study) assay_stream = FactoryBot.create(:assay_stream, study: isa_study) + assert assay_stream.is_assay_stream? + assert assay_stream.is_isa_json_compliant? assert_equal assay_stream.previous_linked_sample_type, isa_study.sample_types.second def_assay = FactoryBot.create(:assay, study:def_study) @@ -800,7 +804,8 @@ def new_valid_assay end test 'has_linked_child_assay?' do - isa_study = FactoryBot.create(:isa_json_compliant_study) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true) + isa_study = FactoryBot.create(:isa_json_compliant_study, investigation: ) def_study = FactoryBot.create(:study) def_assay = FactoryBot.create(:assay, study:def_study) @@ -820,12 +825,13 @@ def new_valid_assay end test 'next_linked_child_assay' do - isa_study = FactoryBot.create(:isa_json_compliant_study) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true) + isa_study = FactoryBot.create(:isa_json_compliant_study, investigation: ) def_study = FactoryBot.create(:study) def_assay = FactoryBot.create(:assay, study:def_study) assay_stream = FactoryBot.create(:assay_stream, study: isa_study) - first_isa_assay = FactoryBot.create(:isa_json_compliant_assay, study: isa_study) + first_isa_assay = FactoryBot.create(:isa_json_compliant_assay, study: isa_study, assay_stream: ) data_file_sample_type = FactoryBot.create(:isa_assay_data_file_sample_type, linked_sample_type: first_isa_assay.sample_type) second_isa_assay = FactoryBot.create(:assay, From 8366b9efac46ca2d6c56f526f0f55061943ca6fb Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 14:00:46 +0100 Subject: [PATCH 18/49] Write test for inserting new assay between assay stream and first assay --- test/functional/isa_assays_controller_test.rb | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index 1212dd21d1..a138244df5 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -281,4 +281,86 @@ def setup assert_select 'div.panel-heading', text: /Discussion Channels/i, count: 1 assert_select 'div.panel-heading', text: /Define Sample type for Assay/i, count: 1 end + + test 'insert assay between assay stream and experimental assay' do + # TODO: Test button text + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true, contributor: person) + study = FactoryBot.create(:isa_json_compliant_study, investigation: , contributor: person ) + + ## Create an assay stream + assay_stream = FactoryBot.create(:assay_stream, contributor: person, study: ) + assert assay_stream.is_assay_stream? + assert_equal assay_stream.previous_linked_sample_type, study.sample_types.second + assert_nil assay_stream.next_linked_child_assay + + ## Create an assay at the end of the stream + end_assay_sample_type = FactoryBot.create(:isa_assay_material_sample_type, + linked_sample_type: study.sample_types.second, + projects: [project], + contributor: person) + end_assay = FactoryBot.create(:assay, contributor: person, study: , sample_type: end_assay_sample_type, assay_stream: ) + + refute end_assay.is_assay_stream? + assert_equal end_assay.previous_linked_sample_type, assay_stream.previous_linked_sample_type, study.sample_types.second + assert_nil end_assay.next_linked_child_assay + + # Test assay linkage + ## Post intermediate assay + policy_attributes = { access_type: Policy::ACCESSIBLE, + permissions_attributes: project_permissions([projects.first], Policy::ACCESSIBLE) } + + intermediate_assay_attributes1 = { title: 'First intermediate assay', + study_id: study.id, + assay_class_id: AssayClass.for_type(Seek:: ISA:: AssayClass::EXP).id, + creator_ids: [person.id], + policy_attributes: , + assay_stream_id: assay_stream.id} + + intermediate_assay_sample_type_attributes1 = { title: "Intermediate Assay Sample type 1", + project_ids: [project.id], + sample_attributes_attributes: { + '0': { + pos: '1', title: 'a string', required: '1', is_title: '1', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, _destroy: '0', + isa_tag_id: FactoryBot.create(:other_material_isa_tag).id + }, + '1': { + pos: '2', title: 'protocol', required: '1', is_title: '0', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, + isa_tag_id: FactoryBot.create(:protocol_isa_tag).id, _destroy: '0' + }, + '2': { + pos: '3', title: 'Input sample', required: '1', + sample_attribute_type_id: FactoryBot.create(:sample_multi_sample_attribute_type).id, + linked_sample_type_id: study.sample_types.second.id, _destroy: '0' + }, + '3': { + pos: '4', title: 'Some material characteristic', required: '1', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, + _destroy: '0', + isa_tag_id: FactoryBot.create(:other_material_characteristic_isa_tag).id + } + } + } + + intermediate_isa_assay_attributes1 = { assay: intermediate_assay_attributes1, + input_sample_type_id: study.sample_types.second.id, + sample_type: intermediate_assay_sample_type_attributes1 } + + assert_difference "Assay.count", 1 do + assert_difference "SampleType.count", 1 do + post :create, params: { isa_assay: intermediate_isa_assay_attributes1 } + end + end + + isa_assay = assigns(:isa_assay) + assert_redirected_to single_page_path(id: project, item_type: 'assay', item_id: isa_assay.assay.id, notice: 'The ISA assay was created successfully!') + + assert_equal isa_assay.assay.sample_type.previous_linked_sample_type, study.sample_types.second + assert_equal isa_assay.assay.next_linked_child_assay, end_assay + end + end From 78c3a7e30ea0048e3183c8b90a6609882d132559 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 14:01:55 +0100 Subject: [PATCH 19/49] Write test for inserting an assay between two experimental assays --- test/functional/isa_assays_controller_test.rb | 96 ++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index a138244df5..435e974c88 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -248,7 +248,7 @@ def setup assert_select 'div#add_documents_form', text: /Documents/i, count: 0 assert_select 'div.panel-heading', text: /Discussion Channels/i, count: 0 assert_select 'div.panel-heading', text: /Define Sample type for Assay/i, count: 0 -end + end test 'show sops, publications, documents, and discussion channels if experimental assay' do person = FactoryBot.create(:person) @@ -363,4 +363,98 @@ def setup assert_equal isa_assay.assay.next_linked_child_assay, end_assay end + test 'insert assay between two experimental assays' do + # TODO: Test button text + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true, contributor: person) + study = FactoryBot.create(:isa_json_compliant_study, investigation: , contributor: person ) + + ## Create an assay stream + assay_stream = FactoryBot.create(:assay_stream, contributor: person, study: ) + assert assay_stream.is_assay_stream? + assert_equal assay_stream.previous_linked_sample_type, study.sample_types.second + assert_nil assay_stream.next_linked_child_assay + + ## Create an assay at the begin of the stream + begin_assay_sample_type = FactoryBot.create(:isa_assay_material_sample_type, + linked_sample_type: study.sample_types.second, + projects: [project], + contributor: person) + begin_assay = FactoryBot.create(:assay, title: 'Begin Assay', contributor: person, study: , sample_type: begin_assay_sample_type, assay_stream: ) + + ## Create an assay at the end of the stream + end_assay_sample_type = FactoryBot.create(:isa_assay_data_file_sample_type, + linked_sample_type: begin_assay_sample_type, + projects: [project], + contributor: person) + end_assay = FactoryBot.create(:assay, title: 'End Assay', contributor: person, study: , sample_type: end_assay_sample_type, assay_stream: ) + + refute end_assay.is_assay_stream? + assert_equal begin_assay.previous_linked_sample_type, assay_stream.previous_linked_sample_type, study.sample_types.second + assert_nil end_assay.next_linked_child_assay + + # Test assay linkage + ## Post intermediate assay + policy_attributes = { access_type: Policy::ACCESSIBLE, + permissions_attributes: project_permissions([projects.first], Policy::ACCESSIBLE) } + + intermediate_assay_attributes2 = { title: 'Second intermediate assay', + study_id: study.id, + assay_class_id: AssayClass.for_type(Seek:: ISA:: AssayClass::EXP).id, + creator_ids: [person.id], + policy_attributes: , + assay_stream_id: assay_stream.id} + + intermediate_assay_sample_type_attributes2 = { title: "Intermediate Assay Sample type 2", + project_ids: [project.id], + sample_attributes_attributes: { + '0': { + pos: '1', title: 'a string', required: '1', is_title: '1', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, _destroy: '0', + isa_tag_id: FactoryBot.create(:other_material_isa_tag).id + }, + '1': { + pos: '2', title: 'protocol', required: '1', is_title: '0', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, + isa_tag_id: FactoryBot.create(:protocol_isa_tag).id, _destroy: '0' + }, + '2': { + pos: '3', title: 'Input sample', required: '1', + sample_attribute_type_id: FactoryBot.create(:sample_multi_sample_attribute_type).id, + linked_sample_type_id: study.sample_types.second.id, _destroy: '0' + }, + '3': { + pos: '4', title: 'Some material characteristic', required: '1', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, + _destroy: '0', + isa_tag_id: FactoryBot.create(:other_material_characteristic_isa_tag).id + } + } + } + + intermediate_isa_assay_attributes2 = { assay: intermediate_assay_attributes2, + input_sample_type_id: begin_assay_sample_type.id, + sample_type: intermediate_assay_sample_type_attributes2 } + + + assert_difference "Assay.count", 1 do + assert_difference "SampleType.count", 1 do + post :create, params: { isa_assay: intermediate_isa_assay_attributes2 } + end + end + + isa_assay = assigns(:isa_assay) + assert_redirected_to single_page_path(id: project, item_type: 'assay', item_id: isa_assay.assay.id, notice: 'The ISA assay was created successfully!') + + puts "Assay added: #{isa_assay.assay.inspect}" + puts "Assay ST added: #{isa_assay.assay.sample_type.inspect}" + puts "Assay from DB added: #{Assay.last.inspect}" + + assert_equal begin_assay.previous_linked_sample_type, study.sample_types.second + assert_equal isa_assay.assay.sample_type.previous_linked_sample_type, begin_assay.sample_type + assert_equal isa_assay.assay.next_linked_child_assay, end_assay + end + end From fc4c991756b5ba6562596589468e44173598eb90 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 14:08:10 +0100 Subject: [PATCH 20/49] Actually better as a before_action since the sample_type is created before the assay --- app/controllers/isa_assays_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/isa_assays_controller.rb b/app/controllers/isa_assays_controller.rb index fd7b9d278b..178ec8bd55 100644 --- a/app/controllers/isa_assays_controller.rb +++ b/app/controllers/isa_assays_controller.rb @@ -5,7 +5,7 @@ class IsaAssaysController < ApplicationController before_action :set_up_instance_variable before_action :find_requested_item, only: %i[edit update] before_action :initialize_isa_assay, only: :create - after_action :fix_assay_linkage_for_new_assays, only: :create + before_action :fix_assay_linkage_for_new_assays, only: :create after_action :rearrange_assay_positions_create_isa_assay, only: :create def new From d2d1a90ec4fa9e2b0445855ee5518bd7b6ccea10 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 14:34:36 +0100 Subject: [PATCH 21/49] Change the button text when inserting an assay between existing assays --- app/views/assays/_buttons.html.erb | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/app/views/assays/_buttons.html.erb b/app/views/assays/_buttons.html.erb index 72c59241fe..38e9e5bf18 100644 --- a/app/views/assays/_buttons.html.erb +++ b/app/views/assays/_buttons.html.erb @@ -6,6 +6,22 @@ else t("assays.#{item.assay_class.long_key.delete(' ').underscore}") end + + isa_assay_verb ||= + if item&.is_assay_stream? + if item.next_linked_child_assay + "Insert a new" + else + "Design" + end + else + if item.next_linked_child_assay + "Insert a new" + else + "Design the next" + end + end + %> <%= render :partial => "subscriptions/subscribe", :locals => {:object => item} %> @@ -33,9 +49,9 @@ <% valid_assay = item&.is_isa_json_compliant? %> <% if valid_study && valid_assay %> <% if item&.is_assay_stream? %> - <%= button_link_to("Design #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.id)) %> + <%= button_link_to("#{isa_assay_verb} #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.id)) %> <% else %> - <%= button_link_to("Design the next #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.assay_stream_id)) %> + <%= button_link_to("#{isa_assay_verb} #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.assay_stream_id)) %> <% end %> <% end %> <% else %> From 3f75302a60650fc914d55ca3b2e9e072ee3e2891 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 14:36:25 +0100 Subject: [PATCH 22/49] Modify existing tests for inserting assays --- test/functional/assays_controller_test.rb | 27 ++++++++++++++++--- test/functional/isa_assays_controller_test.rb | 2 -- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/test/functional/assays_controller_test.rb b/test/functional/assays_controller_test.rb index dae23793ae..08bd48b4dd 100644 --- a/test/functional/assays_controller_test.rb +++ b/test/functional/assays_controller_test.rb @@ -2033,26 +2033,45 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links with_config_value(:isa_json_compliance_enabled, true) do current_user = FactoryBot.create(:user) login_as(current_user) - study = FactoryBot.create(:isa_json_compliant_study) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true, contributor: current_user.person) + study = FactoryBot.create(:isa_json_compliant_study, investigation: ) assay_stream = FactoryBot.create(:assay_stream, study: , contributor: current_user.person) - assay1 = FactoryBot.create(:isa_json_compliant_assay, contributor: current_user.person, study: , assay_stream:) - assay2 = FactoryBot.create(:isa_json_compliant_assay, contributor: current_user.person, study: , assay_stream:) + get :show, params: { id: assay_stream } + assert_response :success + + # If stream has no assays, it should say 'Design Assay' + assert_select 'a', text: /Design #{I18n.t('assay')}/i, count: 1 + + assay_sample_type1 = FactoryBot.create(:isa_assay_material_sample_type, linked_sample_type: study.sample_types.second) + assay1 = FactoryBot.create(:assay, contributor: current_user.person, study: , assay_stream:, sample_type: assay_sample_type1) assert_equal assay_stream.study, assay1.study get :show, params: { id: assay_stream } assert_response :success - assert_select 'a', text: /Design #{I18n.t('assay')}/i, count: 1 + # If stream has child assays, it should say 'Insert a new Assay' + assert_select 'a', text: /Insert a new #{I18n.t('assay')}/i, count: 1 get :show, params: { id: assay1 } assert_response :success + # If current assay doesn't have a next assay in the same stream, it should say 'Design the next Assay' assert_select 'a', text: /Design the next #{I18n.t('assay')}/i, count: 1 + assay_sample_type2 = FactoryBot.create(:isa_assay_material_sample_type, linked_sample_type: assay_sample_type1) + assay2 = FactoryBot.create(:assay, contributor: current_user.person, study: , assay_stream:, sample_type: assay_sample_type2) + + get :show, params: { id: assay1 } + assert_response :success + + # If current assay has a next assay in the same stream, it should say 'Insert a new Assay' + assert_select 'a', text: /Insert a new #{I18n.t('assay')}/i, count: 1 + get :show, params: { id: assay2 } assert_response :success + # If current assay is at the end of the stream, it should say 'Design the next Assay' again assert_select 'a', text: /Design the next #{I18n.t('assay')}/i, count: 1 end end diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index 435e974c88..2bafd4cb92 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -283,7 +283,6 @@ def setup end test 'insert assay between assay stream and experimental assay' do - # TODO: Test button text person = FactoryBot.create(:person) project = person.projects.first login_as(person) @@ -364,7 +363,6 @@ def setup end test 'insert assay between two experimental assays' do - # TODO: Test button text person = FactoryBot.create(:person) project = person.projects.first login_as(person) From 183c8ed5fc2a013426f333bd78c89c2eeff09b57 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 15:30:51 +0100 Subject: [PATCH 23/49] Disable button when inserting an assay before an assay with samples in its sample type --- app/views/assays/_buttons.html.erb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/app/views/assays/_buttons.html.erb b/app/views/assays/_buttons.html.erb index 38e9e5bf18..747e72addc 100644 --- a/app/views/assays/_buttons.html.erb +++ b/app/views/assays/_buttons.html.erb @@ -22,6 +22,7 @@ end end + hide_new_assays_button = item.next_linked_child_assay&.sample_type&.samples&.any? %> <%= render :partial => "subscriptions/subscribe", :locals => {:object => item} %> @@ -48,10 +49,14 @@ <% valid_study = item&.study&.is_isa_json_compliant? %> <% valid_assay = item&.is_isa_json_compliant? %> <% if valid_study && valid_assay %> - <% if item&.is_assay_stream? %> - <%= button_link_to("#{isa_assay_verb} #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.id)) %> + <% if hide_new_assays_button %> + <%= button_link_to("#{isa_assay_verb} #{t('assay')}", 'new', nil, disabled_reason: 'The next linked assay has samples. Cannot insert new assay here.') %> <% else %> - <%= button_link_to("#{isa_assay_verb} #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.assay_stream_id)) %> + <% if item&.is_assay_stream? %> + <%= button_link_to("#{isa_assay_verb} #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.id)) %> + <% else %> + <%= button_link_to("#{isa_assay_verb} #{t('assay')}", 'new', new_isa_assay_path(source_assay_id: item.id, study_id: item.study.id, single_page: params[:single_page], assay_stream_id: item.assay_stream_id)) %> + <% end %> <% end %> <% end %> <% else %> From a09434bee04dff1fcb8feb4ef3d86e60e041733a Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 16:02:23 +0100 Subject: [PATCH 24/49] Add test for disabled button --- test/functional/assays_controller_test.rb | 54 +++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/test/functional/assays_controller_test.rb b/test/functional/assays_controller_test.rb index 08bd48b4dd..6bde112931 100644 --- a/test/functional/assays_controller_test.rb +++ b/test/functional/assays_controller_test.rb @@ -2032,6 +2032,7 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links test 'display adjusted buttons if isa json compliant' do with_config_value(:isa_json_compliance_enabled, true) do current_user = FactoryBot.create(:user) + project = current_user.person.projects.first login_as(current_user) investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true, contributor: current_user.person) study = FactoryBot.create(:isa_json_compliant_study, investigation: ) @@ -2073,6 +2074,59 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links # If current assay is at the end of the stream, it should say 'Design the next Assay' again assert_select 'a', text: /Design the next #{I18n.t('assay')}/i, count: 1 + + source_sample = + FactoryBot.create :sample, + title: 'source 1', + sample_type: study.sample_types.first, + project_ids: [project.id], + data: { + 'Source Name': 'Source Name', + 'Source Characteristic 1': 'Source Characteristic 1', + 'Source Characteristic 2': + study.sample_types.first + .sample_attributes + .find_by_title('Source Characteristic 2') + .sample_controlled_vocab + .sample_controlled_vocab_terms + .first + .label + }, + contributor: current_user.person + + sample_sample = + FactoryBot.create :sample, + title: 'sample 1', + sample_type: study.sample_types.second, + project_ids: [project.id], + data: { + Input: [source_sample.id], + 'sample collection': 'sample collection', + 'sample collection parameter value 1': 'sample collection parameter value 1', + 'Sample Name': 'sample name', + 'sample characteristic 1': 'sample characteristic 1' + }, + contributor: current_user.person + + FactoryBot.create :sample, + title: 'assay 1 - sample 1', + sample_type: assay_sample_type1, + project_ids: [project.id], + data: { + Input: [sample_sample.id], + 'Protocol Assay 1': 'Protocol Assay 1', + 'Assay 1 parameter value 1': 'Assay 1 parameter value 1', + 'Extract Name': 'Extract Name', + 'other material characteristic 1': 'other material characteristic 1' + }, + contributor: current_user.person + + get :show, params: { id: assay_stream } + assert_response :success + + # If the next assay's sample type has samples, the 'new assay' button should be disabled' + assert_select 'a', text: /Insert a new #{I18n.t('assay')}/i, class: 'disabled', count: 1 + end end end From 518b8ce6b48246840268e5d743128ef7f2624cdc Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 17:34:09 +0100 Subject: [PATCH 25/49] Test prevent inserting new assay if next assay's sample type has samples --- test/functional/isa_assays_controller_test.rb | 138 +++++++++++++++++- 1 file changed, 134 insertions(+), 4 deletions(-) diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index 2bafd4cb92..b554085818 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -360,6 +360,8 @@ def setup assert_equal isa_assay.assay.sample_type.previous_linked_sample_type, study.sample_types.second assert_equal isa_assay.assay.next_linked_child_assay, end_assay + + # TODO: Test the assay positions after reorganising end test 'insert assay between two experimental assays' do @@ -446,13 +448,141 @@ def setup isa_assay = assigns(:isa_assay) assert_redirected_to single_page_path(id: project, item_type: 'assay', item_id: isa_assay.assay.id, notice: 'The ISA assay was created successfully!') - puts "Assay added: #{isa_assay.assay.inspect}" - puts "Assay ST added: #{isa_assay.assay.sample_type.inspect}" - puts "Assay from DB added: #{Assay.last.inspect}" - assert_equal begin_assay.previous_linked_sample_type, study.sample_types.second assert_equal isa_assay.assay.sample_type.previous_linked_sample_type, begin_assay.sample_type assert_equal isa_assay.assay.next_linked_child_assay, end_assay + + # TODO: Test the assay positions after reorganising end + test 'should not insert assay if next assay has samples' do + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true, contributor: person) + study = FactoryBot.create(:isa_json_compliant_study, investigation: , contributor: person ) + + ## Create an assay stream + assay_stream = FactoryBot.create(:assay_stream, contributor: person, study: ) + assert assay_stream.is_assay_stream? + assert_equal assay_stream.previous_linked_sample_type, study.sample_types.second + assert_nil assay_stream.next_linked_child_assay + + ## Create an assay at the begin of the stream + begin_assay_sample_type = FactoryBot.create(:isa_assay_material_sample_type, + linked_sample_type: study.sample_types.second, + projects: [project], + contributor: person) + begin_assay = FactoryBot.create(:assay, title: 'Begin Assay', contributor: person, study: , sample_type: begin_assay_sample_type, assay_stream: ) + + ## Create an assay at the end of the stream + end_assay_sample_type = FactoryBot.create(:isa_assay_data_file_sample_type, + linked_sample_type: begin_assay_sample_type, + projects: [project], + contributor: person) + end_assay = FactoryBot.create(:assay, title: 'End Assay', contributor: person, study: , sample_type: end_assay_sample_type, assay_stream: ) + + refute end_assay.is_assay_stream? + assert_equal begin_assay.previous_linked_sample_type, assay_stream.previous_linked_sample_type, study.sample_types.second + assert_nil end_assay.next_linked_child_assay + + source_sample = + FactoryBot.create :sample, + title: 'source 1', + sample_type: study.sample_types.first, + project_ids: [project.id], + data: { + 'Source Name': 'Source Name', + 'Source Characteristic 1': 'Source Characteristic 1', + 'Source Characteristic 2': + study.sample_types.first + .sample_attributes + .find_by_title('Source Characteristic 2') + .sample_controlled_vocab + .sample_controlled_vocab_terms + .first + .label + }, + contributor: person + + sample_sample = + FactoryBot.create :sample, + title: 'sample 1', + sample_type: study.sample_types.second, + project_ids: [project.id], + data: { + Input: [source_sample.id], + 'sample collection': 'sample collection', + 'sample collection parameter value 1': 'sample collection parameter value 1', + 'Sample Name': 'sample name', + 'sample characteristic 1': 'sample characteristic 1' + }, + contributor: person + + FactoryBot.create :sample, + title: 'Begin Material 1', + sample_type: begin_assay_sample_type, + project_ids: [project.id], + data: { + Input: [sample_sample.id], + 'Protocol Assay 1': 'Protocol Assay 1', + 'Assay 1 parameter value 1': 'Assay 1 parameter value 1', + 'Extract Name': 'Extract Name', + 'other material characteristic 1': 'other material characteristic 1' + }, + contributor: person + + + assert assay_stream.next_linked_child_assay.sample_type.samples.any? + + # Test assay linkage + ## Post intermediate assay + policy_attributes = { access_type: Policy::ACCESSIBLE, + permissions_attributes: project_permissions([projects.first], Policy::ACCESSIBLE) } + + intermediate_assay_attributes3 = { title: 'Third intermediate assay', + study_id: study.id, + assay_class_id: AssayClass.for_type(Seek:: ISA:: AssayClass::EXP).id, + creator_ids: [person.id], + policy_attributes: , + assay_stream_id: assay_stream.id} + + intermediate_assay_sample_type_attributes3 = { title: "Intermediate Assay Sample type 3", + project_ids: [project.id], + sample_attributes_attributes: { + '0': { + pos: '1', title: 'a string', required: '1', is_title: '1', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, _destroy: '0', + isa_tag_id: FactoryBot.create(:other_material_isa_tag).id + }, + '1': { + pos: '2', title: 'protocol', required: '1', is_title: '0', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, + isa_tag_id: FactoryBot.create(:protocol_isa_tag).id, _destroy: '0' + }, + '2': { + pos: '3', title: 'Input sample', required: '1', + sample_attribute_type_id: FactoryBot.create(:sample_multi_sample_attribute_type).id, + linked_sample_type_id: study.sample_types.second.id, _destroy: '0' + }, + '3': { + pos: '4', title: 'Some material characteristic', required: '1', + sample_attribute_type_id: FactoryBot.create(:string_sample_attribute_type).id, + _destroy: '0', + isa_tag_id: FactoryBot.create(:other_material_characteristic_isa_tag).id + } + } + } + + intermediate_isa_assay_attributes3 = { assay: intermediate_assay_attributes3, + input_sample_type_id: assay_stream.id, + sample_type: intermediate_assay_sample_type_attributes3 } + + assert_no_difference "Assay.count" do + assert_no_difference "SampleType.count" do + post :create, params: { isa_assay: intermediate_isa_assay_attributes3 } + end + end + assert_response :not_found + end end From d8aec7d8b3ba820c2cfa34c3f63a54e40f1c91a0 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 20:30:18 +0100 Subject: [PATCH 26/49] do not rearrange position if not isa json compliant or if assay stream --- app/controllers/isa_assays_controller.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/controllers/isa_assays_controller.rb b/app/controllers/isa_assays_controller.rb index 178ec8bd55..417ff0da4f 100644 --- a/app/controllers/isa_assays_controller.rb +++ b/app/controllers/isa_assays_controller.rb @@ -88,6 +88,9 @@ def fix_assay_linkage_for_new_assays end def rearrange_assay_positions_create_isa_assay + return if @isa_assay.assay.is_assay_stream? + return unless @isa_assay.assay.is_isa_json_compliant? + rearrange_assay_positions(@isa_assay.assay.assay_stream) end From 735896e745b3bb3ecba24a5d1e0d1fb075e1a283 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Tue, 30 Jan 2024 20:31:38 +0100 Subject: [PATCH 27/49] Fix tests --- app/forms/isa_assay.rb | 6 ++++++ app/models/assay.rb | 12 ++++++++++-- lib/seek/assets_common.rb | 17 ++++++++--------- test/functional/isa_assays_controller_test.rb | 8 ++++---- test/unit/assay_test.rb | 3 +++ 5 files changed, 31 insertions(+), 15 deletions(-) diff --git a/app/forms/isa_assay.rb b/app/forms/isa_assay.rb index a586c75851..7c6679d4b3 100644 --- a/app/forms/isa_assay.rb +++ b/app/forms/isa_assay.rb @@ -56,6 +56,12 @@ def populate(id) def validate_objects @assay.errors.each { |e| errors.add(:base, "[Assay]: #{e.full_message}") } unless @assay.valid? + if @assay.new_record? && @assay.next_linked_child_assay&.sample_type&.samples&.any? + next_assay_id = @assay.next_linked_child_assay.id + next_assay_title = @assay.next_linked_child_assay.title + errors.add(:base, "[Assay]: Not allowed to create an assay before assay '#{next_assay_id} - #{next_assay_title}'. It has samples linked to it.") + end + return if @assay.is_assay_stream? errors.add(:base, '[Assay]: The assay is missing a sample type.') if @sample_type.nil? diff --git a/app/models/assay.rb b/app/models/assay.rb index ce591f9534..1a6044e35d 100644 --- a/app/models/assay.rb +++ b/app/models/assay.rb @@ -96,14 +96,22 @@ def next_linked_child_assay return unless has_linked_child_assay? if is_assay_stream? - child_assays.first + first_assay_in_stream else sample_type.next_linked_sample_types.map(&:assays).flatten.detect { |a| a.assay_stream_id == assay_stream_id } end end + def first_assay_in_stream + if is_assay_stream? + child_assays.detect { |a| a.sample_type.previous_linked_sample_type == a.study.sample_types.second } + else + assay_stream.child_assays.detect { |a| a.sample_type.previous_linked_sample_type == a.study.sample_types.second } + end + end + def first_assay_in_stream? - self == assay_stream.child_assays.first + self == first_assay_in_stream end def default_contributor diff --git a/lib/seek/assets_common.rb b/lib/seek/assets_common.rb index 226ad2407f..5c36e2912b 100644 --- a/lib/seek/assets_common.rb +++ b/lib/seek/assets_common.rb @@ -78,16 +78,15 @@ def params_for_controller def rearrange_assay_positions(assay_stream) return unless assay_stream - disable_authorization_checks do - next_assay = assay_stream.next_linked_child_assay - assay_position = 0 + # updating the position should happen whether or not the user has the right permissions + next_assay = assay_stream.next_linked_child_assay + assay_position = 0 - # While there is a next assay, increment position by - while next_assay - next_assay.update(position: assay_position) - next_assay = next_assay.next_linked_child_assay - assay_position += 1 - end + # While there is a next assay, increment position by + while next_assay + Assay.find(next_assay.id).update_column(:position, assay_position) unless assay_position == next_assay.position + next_assay = next_assay.next_linked_child_assay + assay_position += 1 end end end diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index b554085818..7ad99d5b33 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -300,7 +300,7 @@ def setup linked_sample_type: study.sample_types.second, projects: [project], contributor: person) - end_assay = FactoryBot.create(:assay, contributor: person, study: , sample_type: end_assay_sample_type, assay_stream: ) + end_assay = FactoryBot.create(:assay, position: 0, contributor: person, study: , sample_type: end_assay_sample_type, assay_stream: ) refute end_assay.is_assay_stream? assert_equal end_assay.previous_linked_sample_type, assay_stream.previous_linked_sample_type, study.sample_types.second @@ -316,7 +316,7 @@ def setup assay_class_id: AssayClass.for_type(Seek:: ISA:: AssayClass::EXP).id, creator_ids: [person.id], policy_attributes: , - assay_stream_id: assay_stream.id} + assay_stream_id: assay_stream.id, position: 0} intermediate_assay_sample_type_attributes1 = { title: "Intermediate Assay Sample type 1", project_ids: [project.id], @@ -382,14 +382,14 @@ def setup linked_sample_type: study.sample_types.second, projects: [project], contributor: person) - begin_assay = FactoryBot.create(:assay, title: 'Begin Assay', contributor: person, study: , sample_type: begin_assay_sample_type, assay_stream: ) + begin_assay = FactoryBot.create(:assay, title: 'Begin Assay', position: 0, contributor: person, study: , sample_type: begin_assay_sample_type, assay_stream: ) ## Create an assay at the end of the stream end_assay_sample_type = FactoryBot.create(:isa_assay_data_file_sample_type, linked_sample_type: begin_assay_sample_type, projects: [project], contributor: person) - end_assay = FactoryBot.create(:assay, title: 'End Assay', contributor: person, study: , sample_type: end_assay_sample_type, assay_stream: ) + end_assay = FactoryBot.create(:assay, title: 'End Assay', position: 1, contributor: person, study: , sample_type: end_assay_sample_type, assay_stream: ) refute end_assay.is_assay_stream? assert_equal begin_assay.previous_linked_sample_type, assay_stream.previous_linked_sample_type, study.sample_types.second diff --git a/test/unit/assay_test.rb b/test/unit/assay_test.rb index 653d618db4..f16c9a2509 100644 --- a/test/unit/assay_test.rb +++ b/test/unit/assay_test.rb @@ -801,6 +801,7 @@ def new_valid_assay sample_type: data_file_sample_type) assert_equal second_isa_assay.previous_linked_sample_type, first_isa_assay.sample_type + assert_equal first_isa_assay.previous_linked_sample_type, isa_study.sample_types.second end test 'has_linked_child_assay?' do @@ -839,6 +840,8 @@ def new_valid_assay assay_stream: , sample_type: data_file_sample_type) + assert_equal assay_stream.first_assay_in_stream, first_isa_assay + assert first_isa_assay.first_assay_in_stream? assert_equal assay_stream.next_linked_child_assay, first_isa_assay assert_nil def_assay.next_linked_child_assay assert_equal first_isa_assay.next_linked_child_assay, second_isa_assay From 1ef73ff5c1a2355ca5bcbd39d601facbec3b66ca Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 1 Feb 2024 15:56:48 +0100 Subject: [PATCH 28/49] test assay position after deletion --- test/functional/assays_controller_test.rb | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/functional/assays_controller_test.rb b/test/functional/assays_controller_test.rb index 6bde112931..a46455b36c 100644 --- a/test/functional/assays_controller_test.rb +++ b/test/functional/assays_controller_test.rb @@ -2129,4 +2129,34 @@ def check_fixtures_for_authorization_of_sops_and_datafiles_links end end + + test 'assay position after deletion' do + with_config_value(:isa_json_compliance_enabled, true) do + person = FactoryBot.create(:person) + project = person.projects.first + login_as(person) + investigation = FactoryBot.create(:investigation, is_isa_json_compliant: true, contributor: person) + study = FactoryBot.create(:isa_json_compliant_study, investigation: ) + assay_stream = FactoryBot.create(:assay_stream, study: , contributor: person, position: 0) + + begin_assay_sample_type = FactoryBot.create(:isa_assay_material_sample_type, linked_sample_type: study.sample_types.second, projects: [project], contributor: person) + begin_assay = FactoryBot.create(:assay, contributor: person, study: , assay_stream:, sample_type: begin_assay_sample_type, position: 0) + + middle_assay_sample_type = FactoryBot.create(:isa_assay_material_sample_type, linked_sample_type: begin_assay_sample_type, projects: [project], contributor: person) + middle_assay = FactoryBot.create(:assay, contributor: person, study: , assay_stream:, sample_type: middle_assay_sample_type, position: 1) + + end_assay_sample_type = FactoryBot.create(:isa_assay_data_file_sample_type, linked_sample_type: middle_assay_sample_type, projects: [project], contributor: person) + end_assay = FactoryBot.create(:assay, contributor: person, study: , assay_stream:, sample_type: end_assay_sample_type, position: 2) + + assert_difference('Assay.count', -1) do + assert_difference('SampleType.count', -1) do + delete :destroy, params: {id: middle_assay} + end + end + + end_assay.reload + refute_equal end_assay.position, 2 + assert_equal end_assay.position, 1 + end + end end From e255e68021dfd3793865d3cbf14549262d0bc4ac Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 1 Feb 2024 17:01:37 +0100 Subject: [PATCH 29/49] Order assay streams by position --- app/views/isa_assays/_form.html.erb | 18 +++++++++++++----- lib/treeview_builder.rb | 2 +- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/views/isa_assays/_form.html.erb b/app/views/isa_assays/_form.html.erb index cc707ccf20..a6e55c458d 100644 --- a/app/views/isa_assays/_form.html.erb +++ b/app/views/isa_assays/_form.html.erb @@ -6,7 +6,7 @@ if @isa_assay.assay.new_record? if params[:is_assay_stream] - assay_position = 0 + assay_position = study.assay_streams.map(&:position).max + 1 assay_class_id = AssayClass.assay_stream.id is_assay_stream = true else @@ -48,7 +48,7 @@
<%= assay_fields.label :description -%>
- <%= assay_fields.text_area :description, :rows => 5, :class=>"form-control rich-text-edit" -%> + <%= assay_fields.text_area :description, rows: 5, class: "form-control rich-text-edit" -%>
<% if show_extended_metadata %> @@ -61,9 +61,17 @@ <%= assay_study_selection('isa_assay[assay][study_id]',@isa_assay.assay.study) %> - + <% if is_assay_stream %> +
+ <%= assay_fields.label "Assay position" -%>
+ <%= assay_fields.number_field :position, rows: 5, class: "form-control", value: assay_position %> +
+ <% else %> + + <% end %> + <%= assay_fields.hidden_field :assay_stream_id, value: assay_stream_id -%> <%= assay_fields.hidden_field :assay_class_id, value: assay_class_id -%> diff --git a/lib/treeview_builder.rb b/lib/treeview_builder.rb index b810759ebb..abb3211bce 100644 --- a/lib/treeview_builder.rb +++ b/lib/treeview_builder.rb @@ -16,7 +16,7 @@ def build_tree_data @project.investigations.map do |investigation| if investigation.is_isa_json_compliant? investigation.studies.map do |study| - assay_stream_items = study.assay_streams.map do |assay_stream| + assay_stream_items = study.assay_streams.sort_by{ |stream| stream.position }.map do |assay_stream| assay_items = assay_stream.child_assays.order(:position).map do |child_assay| build_assay_item(child_assay) end From d9c8af523e3604208adf806067af3a430738e81c Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Thu, 1 Feb 2024 17:06:17 +0100 Subject: [PATCH 30/49] Test positions after creating intermediate assays --- lib/seek/assets_common.rb | 3 +++ test/functional/isa_assays_controller_test.rb | 22 ++++++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/lib/seek/assets_common.rb b/lib/seek/assets_common.rb index 5c36e2912b..0a53cec0ce 100644 --- a/lib/seek/assets_common.rb +++ b/lib/seek/assets_common.rb @@ -78,6 +78,9 @@ def params_for_controller def rearrange_assay_positions(assay_stream) return unless assay_stream + # Reload the assay stream to also include newly created assays + assay_stream.reload + # updating the position should happen whether or not the user has the right permissions next_assay = assay_stream.next_linked_child_assay assay_position = 0 diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index 7ad99d5b33..bc7edf4d79 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -313,7 +313,7 @@ def setup intermediate_assay_attributes1 = { title: 'First intermediate assay', study_id: study.id, - assay_class_id: AssayClass.for_type(Seek:: ISA:: AssayClass::EXP).id, + assay_class_id: AssayClass.experimental.id, creator_ids: [person.id], policy_attributes: , assay_stream_id: assay_stream.id, position: 0} @@ -361,7 +361,13 @@ def setup assert_equal isa_assay.assay.sample_type.previous_linked_sample_type, study.sample_types.second assert_equal isa_assay.assay.next_linked_child_assay, end_assay - # TODO: Test the assay positions after reorganising + # Test the assay positions after reorganising + end_assay.reload + refute_equal end_assay.position, 0 + assert_equal end_assay.position, 1 + + isa_assay.assay.reload + assert_equal isa_assay.assay.position, 0 end test 'insert assay between two experimental assays' do @@ -402,7 +408,7 @@ def setup intermediate_assay_attributes2 = { title: 'Second intermediate assay', study_id: study.id, - assay_class_id: AssayClass.for_type(Seek:: ISA:: AssayClass::EXP).id, + assay_class_id: AssayClass.experimental.id, creator_ids: [person.id], policy_attributes: , assay_stream_id: assay_stream.id} @@ -452,7 +458,13 @@ def setup assert_equal isa_assay.assay.sample_type.previous_linked_sample_type, begin_assay.sample_type assert_equal isa_assay.assay.next_linked_child_assay, end_assay - # TODO: Test the assay positions after reorganising + # Test the assay positions after reorganising + end_assay.reload + refute_equal end_assay.position, 1 + assert_equal end_assay.position, 2 + + isa_assay.assay.reload + assert_equal isa_assay.assay.position, 1 end test 'should not insert assay if next assay has samples' do @@ -542,7 +554,7 @@ def setup intermediate_assay_attributes3 = { title: 'Third intermediate assay', study_id: study.id, - assay_class_id: AssayClass.for_type(Seek:: ISA:: AssayClass::EXP).id, + assay_class_id: AssayClass.experimental.id, creator_ids: [person.id], policy_attributes: , assay_stream_id: assay_stream.id} From 102aeb90597d6bda45459a30faa44bb5d8f877ed Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 2 Feb 2024 10:53:11 +0100 Subject: [PATCH 31/49] Fix / simplify unit tests --- test/factories/samples.rb | 78 +++++++++++++++++-- .../unit/helpers/dynamic_table_helper_test.rb | 76 ++++++++---------- 2 files changed, 107 insertions(+), 47 deletions(-) diff --git a/test/factories/samples.rb b/test/factories/samples.rb index 3f69eafbe0..56eed1d565 100644 --- a/test/factories/samples.rb +++ b/test/factories/samples.rb @@ -9,7 +9,7 @@ sample.set_attribute_value(:the_title, sample.title) if sample.data.key?(:the_title) end end - + factory(:patient_sample, parent: :sample) do association :sample_type, factory: :patient_sample_type, strategy: :create after(:build) do |sample| @@ -18,21 +18,21 @@ sample.set_attribute_value(:weight, 88.7) end end - + factory(:sample_from_file, parent: :sample) do sequence(:title) { |n| "Sample #{n}" } association :sample_type, factory: :strain_sample_type, strategy: :create - + after(:build) do |sample| sample.set_attribute_value(:name, sample.title) if sample.data.key?(:name) sample.set_attribute_value(:seekstrain, '1234') end - + after(:build) do |sample| sample.originating_data_file = FactoryBot.create(:strain_sample_data_file, projects:sample.projects, contributor:sample.contributor) if sample.originating_data_file.nil? end end - + factory(:min_sample, parent: :sample) do association :sample_type, factory: :min_sample_type, strategy: :create association :contributor, factory: :person, strategy: :create @@ -55,4 +55,72 @@ sample.set_attribute_value(:patients, [FactoryBot.create(:patient_sample).id.to_s, FactoryBot.create(:patient_sample).id.to_s]) end end + + factory(:isa_source, parent: :sample) do + sequence(:title) { |n| "Source sample #{n}" } + association :sample_type, factory: :isa_source_sample_type, strategy: :create + after(:build) do |sample| + sample.set_attribute_value('Source Name', sample.title) + sample.set_attribute_value('Source Characteristic 1', 'Source Characteristic 1') + sample.set_attribute_value('Source Characteristic 2', sample.sample_type.sample_attributes.find_by_title('Source Characteristic 2').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + sample.set_attribute_value('Source Characteristic 3', sample.sample_type.sample_attributes.find_by_title('Source Characteristic 3').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + end + end + + factory(:isa_sample, parent: :sample) do + transient do + linked_samples { nil } + end + sequence(:title) { |n| "Source #{n}" } + association :sample_type, factory: :isa_sample_collection_sample_type, strategy: :create + after(:build) do |sample, eval| + sample.data = { + Input: eval.linked_samples.map(&:id), + } + sample.set_attribute_value('Sample Name', sample.title) + sample.set_attribute_value('sample collection', 'sample collection') + sample.set_attribute_value('sample collection parameter value 1', 'sample collection parameter value 1') + sample.set_attribute_value('sample collection parameter value 2', sample.sample_type.sample_attributes.find_by_title('sample collection parameter value 2').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + sample.set_attribute_value('sample characteristic 1', 'sample characteristic 1') + sample.set_attribute_value('sample characteristic 2', sample.sample_type.sample_attributes.find_by_title('sample characteristic 2').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + end + end + + factory(:isa_material_assay_sample, parent: :sample) do + transient do + linked_samples { nil } + end + sequence(:title) { |n| "Material output #{n}" } + association :sample_type, factory: :isa_assay_material_sample_type, strategy: :create + after(:build) do |sample, eval| + sample.data = { + Input: eval.linked_samples.map(&:id), + } + sample.set_attribute_value('Extract Name', sample.title) + sample.set_attribute_value('Protocol Assay 1', 'Protocol Assay 1') + sample.set_attribute_value('Assay 1 parameter value 1', 'Assay 1 parameter value 1') + sample.set_attribute_value('Assay 1 parameter value 2', sample.sample_type.sample_attributes.find_by_title('Assay 1 parameter value 2').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + sample.set_attribute_value('other material characteristic 1', 'other material characteristic 1') + sample.set_attribute_value('other material characteristic 2', sample.sample_type.sample_attributes.find_by_title('other material characteristic 2').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + end + end + + factory(:isa_datafile_assay_sample, parent: :sample) do + transient do + linked_samples { nil } + end + sequence(:title) { |n| "Data file #{n}" } + association :sample_type, factory: :isa_assay_data_file_sample_type, strategy: :create + after(:build) do |sample, eval| + sample.data = { + Input: eval.linked_samples.map(&:id), + } + sample.set_attribute_value('File Name', sample.title) + sample.set_attribute_value('Protocol Assay 2', 'Protocol Assay 2') + sample.set_attribute_value('Assay 2 parameter value 1', 'Assay 2 parameter value 1') + sample.set_attribute_value('Assay 2 parameter value 2', sample.sample_type.sample_attributes.find_by_title('Assay 2 parameter value 2').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + sample.set_attribute_value('sample characteristic 1', 'sample characteristic 1') + sample.set_attribute_value('sample characteristic 2', sample.sample_type.sample_attributes.find_by_title('sample characteristic 2').sample_controlled_vocab.sample_controlled_vocab_terms.first.label) + end + end end diff --git a/test/unit/helpers/dynamic_table_helper_test.rb b/test/unit/helpers/dynamic_table_helper_test.rb index d1828e1966..af03f2b7a8 100644 --- a/test/unit/helpers/dynamic_table_helper_test.rb +++ b/test/unit/helpers/dynamic_table_helper_test.rb @@ -7,48 +7,44 @@ class DynamicTableHelperTest < ActionView::TestCase project = person.projects.first User.with_current_user(person.user) do - inv = FactoryBot.create(:investigation, projects: [project], contributor: person) + inv = FactoryBot.create(:investigation, projects: [project], contributor: person, is_isa_json_compliant: true) - sample_a1 = FactoryBot.create(:patient_sample, contributor: person) - type_a = sample_a1.sample_type - sample_a2 = FactoryBot.create(:patient_sample, sample_type: type_a, contributor: person) - sample_a3 = FactoryBot.create(:patient_sample, sample_type: type_a, contributor: person) + # Sample types + source_sample_type = FactoryBot.create(:isa_source_sample_type, projects: [project], contributor: person) + sample_collection_sample_type = FactoryBot.create(:isa_sample_collection_sample_type, projects: [project], contributor: person, linked_sample_type: source_sample_type) + material_assay_sample_type = FactoryBot.create(:isa_assay_material_sample_type, projects: [project], contributor: person, linked_sample_type: sample_collection_sample_type) - type_b = FactoryBot.create(:multi_linked_sample_type, project_ids: [project.id]) - type_b.sample_attributes.last.linked_sample_type = type_a - type_b.save! + # Samples + source1 = FactoryBot.create(:isa_source, sample_type: source_sample_type, contributor: person) + source2 = FactoryBot.create(:isa_source, sample_type: source_sample_type, contributor: person) + source3 = FactoryBot.create(:isa_source, sample_type: source_sample_type, contributor: person) - sample_b1 = type_b.samples.create!(data: { title: 'sample_b1', patient: [sample_a1.id] }, - sample_type: type_b, project_ids: [project.id]) + sample1 = FactoryBot.create(:isa_sample, sample_type: sample_collection_sample_type, contributor: person, linked_samples: [ source1 ]) + sample2 = FactoryBot.create(:isa_sample, sample_type: sample_collection_sample_type, contributor: person, linked_samples: [ source2 ]) - sample_b2 = type_b.samples.create!(data: { title: 'sample_b2', patient: [sample_a2.id] }, - sample_type: type_b, project_ids: [project.id]) + intermediate_material1 = FactoryBot.create(:isa_material_assay_sample, sample_type: material_assay_sample_type, contributor: person, linked_samples: [ sample1 ]) - type_c = FactoryBot.create(:multi_linked_sample_type, project_ids: [project.id]) - type_c.sample_attributes.last.linked_sample_type = type_b - type_c.save! - - sample_c1 = type_c.samples.create!(data: { title: 'sample_c1', patient: [sample_b1.id] }, - sample_type: type_c, project_ids: [project.id]) - - study = FactoryBot.create(:study, investigation: inv, contributor: person, sample_types: [type_a, type_b]) - assay = FactoryBot.create(:assay, study: study, contributor: person, sample_type: type_c, position: 1) + # ISA + study = FactoryBot.create(:study, investigation: inv, contributor: person, sample_types: [source_sample_type, sample_collection_sample_type]) + assay_stream = FactoryBot.create(:assay_stream, contributor: person, study: ) + assay = FactoryBot.create(:assay, study: , contributor: person, sample_type: material_assay_sample_type, position: 0) # Query with the Study: - # |-------------------------------------------------| - # | type_a | type_b | - # |------------------------|------------------------| - # | (status)(id)sample_a1 | (status)(id)sample_b1 | - # | (status)(id)sample_a2 | (status)(id)sample_b2 | - # | (status)(id)sample_a3 | x | - # |-------------------------------------------------| + # |---------------------------------------------------------| + # | source_sample_type | sample_collection_sample_type | + # |------------------------|------------------------ | + # | (status)(id)source1 | (status)(id)sample1 | + # | (status)(id)source2 | (status)(id)sample2 | + # | (status)(id)source3 | x | + # |---------------------------------------------------------| dt = dt_aggregated(study) + # Each sample types' attributes count + the sample.id columns_count = study.sample_types[0].sample_attributes.length + 2 columns_count += study.sample_types[1].sample_attributes.length + 2 - assert_equal type_a.samples.length, dt[:rows].length + assert_equal source_sample_type.samples.length, dt[:rows].length assert_equal columns_count, dt[:columns].length dt[:rows].each { |r| assert_equal columns_count, r.length } @@ -57,17 +53,17 @@ class DynamicTableHelperTest < ActionView::TestCase assert_equal true, (dt[:rows][2].any? { |x| x == '' }) # Query with the Assay: - # |-----------------------| - # | type_c | - # |-----------------------| - # | (status)(id)sample_c1 | - # |-----------------------| + # |------------------------------------| + # | material_assay_sample_type | + # |------------------------------------| + # | (status)(id)intermediate_material1 | + # |------------------------------------| dt = dt_aggregated(study, assay) # Each sample types' attributes count + the sample.id columns_count = assay.sample_type.sample_attributes.length + 2 - assert_equal type_c.samples.length, dt[:rows].length + assert_equal material_assay_sample_type.samples.length, dt[:rows].length assert_equal columns_count, dt[:columns].length dt[:rows].each { |r| assert_equal columns_count, r.length } @@ -76,13 +72,9 @@ class DynamicTableHelperTest < ActionView::TestCase end test 'Should return the sequence of sample_type links' do - type1 = FactoryBot.create(:simple_sample_type) - type2 = FactoryBot.create(:multi_linked_sample_type) - type3 = FactoryBot.create(:multi_linked_sample_type) - type2.sample_attributes.detect(&:seek_sample_multi?).linked_sample_type = type1 - type3.sample_attributes.detect(&:seek_sample_multi?).linked_sample_type = type2 - type2.save! - type3.save! + type1 = FactoryBot.create(:isa_source_sample_type) + type2 = FactoryBot.create(:isa_sample_collection_sample_type, linked_sample_type: type1) + type3 = FactoryBot.create(:isa_assay_material_sample_type, linked_sample_type: type2) sequence = link_sequence(type3) assert_equal sequence, [type3, type2, type1] From 8a6c20ccff7925b095f1ce64cb7e53115442c06d Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 2 Feb 2024 11:02:04 +0100 Subject: [PATCH 32/49] Fix the existing functional tests --- app/views/isa_assays/_form.html.erb | 2 +- test/functional/isa_assays_controller_test.rb | 7 +++++-- test/functional/studies_controller_test.rb | 7 ++++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/views/isa_assays/_form.html.erb b/app/views/isa_assays/_form.html.erb index a6e55c458d..dc314c94d0 100644 --- a/app/views/isa_assays/_form.html.erb +++ b/app/views/isa_assays/_form.html.erb @@ -6,7 +6,7 @@ if @isa_assay.assay.new_record? if params[:is_assay_stream] - assay_position = study.assay_streams.map(&:position).max + 1 + assay_position = study.assay_streams.any? ? study.assay_streams.map(&:position).max + 1 : 0 assay_class_id = AssayClass.assay_stream.id is_assay_stream = true else diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index bc7edf4d79..a2632c6881 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -228,8 +228,11 @@ def setup test 'hide sops, publications, documents, and discussion channels if assay stream' do person = FactoryBot.create(:person) - study = FactoryBot.create(:isa_json_compliant_study, contributor: person) - assay_stream = FactoryBot.create(:assay_stream, study: , contributor: person) + investigation = FactoryBot.create(:investigation, contributor: person, is_isa_json_compliant: true) + study = FactoryBot.create(:isa_json_compliant_study, contributor: person, investigation: ) + assay_stream = FactoryBot.create(:assay_stream, study: , contributor: person, position: 0) + + login_as(person) get :new, params: {study_id: study.id, is_assay_stream: true} assert_response :success diff --git a/test/functional/studies_controller_test.rb b/test/functional/studies_controller_test.rb index c022e8aa04..6ea7c70d67 100644 --- a/test/functional/studies_controller_test.rb +++ b/test/functional/studies_controller_test.rb @@ -2019,9 +2019,10 @@ def test_should_show_investigation_tab test 'display adjusted buttons if isa json compliant' do with_config_value(:isa_json_compliance_enabled, true) do - current_user = FactoryBot.create(:user) - login_as(current_user) - study = FactoryBot.create(:isa_json_compliant_study, contributor: current_user.person) + person = FactoryBot.create(:person) + login_as(person) + investigation = FactoryBot.create(:investigation, contributor: person, is_isa_json_compliant: true) + study = FactoryBot.create(:isa_json_compliant_study, contributor: person, investigation: ) get :show, params: { id: study } assert_response :success From 33a4e0c92c8346d5951a4e5557363f87eca2bd86 Mon Sep 17 00:00:00 2001 From: Kevin De Pelseneer Date: Fri, 2 Feb 2024 13:22:37 +0100 Subject: [PATCH 33/49] Test isa asssay positions --- app/views/isa_assays/_form.html.erb | 8 +++-- test/functional/isa_assays_controller_test.rb | 30 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/views/isa_assays/_form.html.erb b/app/views/isa_assays/_form.html.erb index dc314c94d0..20fe51faaf 100644 --- a/app/views/isa_assays/_form.html.erb +++ b/app/views/isa_assays/_form.html.erb @@ -1,5 +1,9 @@ <% assay = params[:isa_assay][:assay] if params.dig(:isa_assay, :assay) %> <% study = Study.find(params[:study_id] || assay[:study_id]) %> +<% def first_assay_in_stream? + params[:source_assay_id].nil? || (params[:source_assay_id] == params[:assay_stream_id]) + end +%> <% source_assay = Assay.find(params[:source_assay_id]) if params[:source_assay_id] assay_stream_id = params[:assay_stream_id] if params[:assay_stream_id] @@ -10,7 +14,7 @@ assay_class_id = AssayClass.assay_stream.id is_assay_stream = true else - assay_position = params[:source_assay_id].nil? ? 1 : source_assay.position + 1 + assay_position = first_assay_in_stream? ? 0 : source_assay.position + 1 assay_class_id = AssayClass.experimental.id is_assay_stream = false end @@ -68,7 +72,7 @@ <% else %> <% end %> diff --git a/test/functional/isa_assays_controller_test.rb b/test/functional/isa_assays_controller_test.rb index a2632c6881..ba746b6d70 100644 --- a/test/functional/isa_assays_controller_test.rb +++ b/test/functional/isa_assays_controller_test.rb @@ -600,4 +600,34 @@ def setup end assert_response :not_found end + + test 'position when creating assays' do + person = FactoryBot.create(:person) + investigation = FactoryBot.create(:investigation, contributor: person, is_isa_json_compliant: true) + study = FactoryBot.create(:isa_json_compliant_study, contributor: person, investigation: ) + + login_as(person) + + get :new, params: { study_id: study.id, is_assay_stream: true } + assert_response :success + # New assay stream should have position 0 and is of type 'number' + assert_select 'input[type=number][value=0]#isa_assay_assay_position', count: 1 + + assay_stream1 = FactoryBot.create(:assay_stream, study: , contributor: person, position: 0) + get :new, params: { study_id: study.id, is_assay_stream: true } + assert_response :success + # New assay stream should have position 1 and is of type 'number' + assert_select 'input[type=number][value=1]#isa_assay_assay_position', count: 1 + + assay_stream2 = FactoryBot.create(:assay_stream, study: , contributor: person, position: 5) + get :new, params: { study_id: study.id, is_assay_stream: true } + assert_response :success + # New assay stream should have position 6 and is of type 'number' + assert_select 'input[type=number][value=6]#isa_assay_assay_position', count: 1 + + get :new, params: {study_id: study.id, assay_stream_id: assay_stream1.id, source_assay_id: assay_stream1.id} + # New assay should have position 0 and is of type 'hidden' + assert_select 'input[type=hidden][value=0]#isa_assay_assay_position', count: 1 + + end end From 433a80c22809fba5f73c8566e8b3c0b572dada2e Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 6 Feb 2024 03:36:01 +0000 Subject: [PATCH 34/49] Bump nokogiri from 1.14.5 to 1.16.2 Bumps [nokogiri](https://github.com/sparklemotion/nokogiri) from 1.14.5 to 1.16.2. - [Release notes](https://github.com/sparklemotion/nokogiri/releases) - [Changelog](https://github.com/sparklemotion/nokogiri/blob/main/CHANGELOG.md) - [Commits](https://github.com/sparklemotion/nokogiri/compare/v1.14.5...v1.16.2) --- updated-dependencies: - dependency-name: nokogiri dependency-type: direct:production ... Signed-off-by: dependabot[bot] --- Gemfile | 2 +- Gemfile.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Gemfile b/Gemfile index 1803823005..b6c2f20f43 100644 --- a/Gemfile +++ b/Gemfile @@ -52,7 +52,7 @@ gem 'will_paginate', '~> 3.1' gem 'yaml_db' gem 'rails_autolink' gem 'rfc-822' -gem 'nokogiri', '~> 1.14.3' +gem 'nokogiri', '~> 1.16.2' #necessary for newer hashie dependency, original api_smith is no longer active gem 'api_smith', git: 'https://github.com/youroute/api_smith.git', ref: '1fb428cebc17b9afab25ac9f809bde87b0ec315b' gem 'rdf-virtuoso', '>= 0.2.0' diff --git a/Gemfile.lock b/Gemfile.lock index 1a2831e0db..71ab432279 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -460,7 +460,7 @@ GEM nokogiri (~> 1) rake mini_mime (1.1.5) - mini_portile2 (2.8.4) + mini_portile2 (2.8.5) minitest (5.20.0) minitest-reporters (1.5.0) ansi @@ -492,8 +492,8 @@ GEM net-protocol netrc (0.11.0) nio4r (2.7.0) - nokogiri (1.14.5) - mini_portile2 (~> 2.8.0) + nokogiri (1.16.2) + mini_portile2 (~> 2.8.2) racc (~> 1.4) nori (1.1.5) oauth2 (2.0.9) @@ -566,7 +566,7 @@ GEM puma (5.6.8) nio4r (~> 2.0) pyu-ruby-sasl (0.0.3.3) - racc (1.7.1) + racc (1.7.3) rack (2.2.8) rack-attack (6.6.0) rack (>= 1.0, < 3) @@ -1029,7 +1029,7 @@ DEPENDENCIES my_responds_to_parent! mysql2 net-ftp - nokogiri (~> 1.14.3) + nokogiri (~> 1.16.2) omniauth (~> 2.1.0) omniauth-github omniauth-rails_csrf_protection From 50e216c458acc9b7b00d470d3aad45603de7ad50 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Tue, 6 Feb 2024 10:42:58 +0000 Subject: [PATCH 35/49] update controller to take and handle a comma separated list of root uris #1690 --- .../sample_controlled_vocabs_controller.rb | 9 +- ...ample_controlled_vocabs_controller_test.rb | 65 +++++ .../ols/fetch_obo_haustorium.yml | 224 ++++++++++++++++++ 3 files changed, 295 insertions(+), 3 deletions(-) create mode 100644 test/vcr_cassettes/ols/fetch_obo_haustorium.yml diff --git a/app/controllers/sample_controlled_vocabs_controller.rb b/app/controllers/sample_controlled_vocabs_controller.rb index 05b65f3d77..c4549e7009 100644 --- a/app/controllers/sample_controlled_vocabs_controller.rb +++ b/app/controllers/sample_controlled_vocabs_controller.rb @@ -92,10 +92,13 @@ def fetch_ols_terms_html root_uri = params[:root_uri] raise 'No root URI provided' if root_uri.blank? - + @terms = [] client = Ebi::OlsClient.new - @terms = client.all_descendants(source_ontology, root_uri) - @terms.reject! { |t| t[:iri] == root_uri } unless params[:include_root_term] == '1' + root_uri.split(',').collect(&:strip).each do |uri| + terms = client.all_descendants(source_ontology, uri) + terms.reject! { |t| t[:iri] == uri } unless params[:include_root_term] == '1' + @terms = @terms | terms + end error_msg = "There are no descendant terms to populate the list." unless @terms.present? rescue StandardError => e error_msg = e.message diff --git a/test/functional/sample_controlled_vocabs_controller_test.rb b/test/functional/sample_controlled_vocabs_controller_test.rb index c8659b5320..4b16e523f8 100644 --- a/test/functional/sample_controlled_vocabs_controller_test.rb +++ b/test/functional/sample_controlled_vocabs_controller_test.rb @@ -306,6 +306,71 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_label[value=?]','trichome papilla' end + test 'fetch ols terms as HTML with multiple root uris and root term included' do + person = FactoryBot.create(:person) + login_as(person) + VCR.use_cassette('ols/fetch_obo_plant_cell_papilla') do + VCR.use_cassette('ols/fetch_obo_haustorium') do + get :fetch_ols_terms_html, params: { source_ontology_id: 'go', + root_uri: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', + include_root_term: '1' } + end + end + + assert_response :success + assert_select 'tr.sample-cv-term', count: 6 + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_0_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090395' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_0_parent_iri:not([value])' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_0_label[value=?]','plant cell papilla' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_1_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090397' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_1_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090395' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_1_label[value=?]','stigma papilla' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_2_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090396' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_2_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090395' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_2_label[value=?]','leaf papilla' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090705' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090395' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_label[value=?]','trichome papilla' + + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_4_iri[value=?]','http://purl.obolibrary.org/obo/GO_0085035' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_4_parent_iri:not([value])' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_4_label[value=?]','haustorium' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_5_iri[value=?]','http://purl.obolibrary.org/obo/GO_0085041' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_5_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0085035' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_5_label[value=?]','arbuscule' + + end + + test 'fetch ols terms as HTML with multiple root uris and no root term included' do + person = FactoryBot.create(:person) + login_as(person) + VCR.use_cassette('ols/fetch_obo_plant_cell_papilla') do + VCR.use_cassette('ols/fetch_obo_haustorium') do + get :fetch_ols_terms_html, params: { source_ontology_id: 'go', + root_uri: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', + include_root_term: '0' } + end + end + + assert_response :success + assert_select 'tr.sample-cv-term', count: 4 + + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_0_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090397' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_0_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090395' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_0_label[value=?]','stigma papilla' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_1_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090396' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_1_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090395' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_1_label[value=?]','leaf papilla' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_2_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090705' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_2_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0090395' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_2_label[value=?]','trichome papilla' + + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_iri[value=?]','http://purl.obolibrary.org/obo/GO_0085041' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_parent_iri[value=?]','http://purl.obolibrary.org/obo/GO_0085035' + assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_label[value=?]','arbuscule' + + end + test 'fetch ols terms as HTML without root term included' do person = FactoryBot.create(:person) login_as(person) diff --git a/test/vcr_cassettes/ols/fetch_obo_haustorium.yml b/test/vcr_cassettes/ols/fetch_obo_haustorium.yml new file mode 100644 index 0000000000..4d3b0063d4 --- /dev/null +++ b/test/vcr_cassettes/ols/fetch_obo_haustorium.yml @@ -0,0 +1,224 @@ +--- +http_interactions: +- request: + method: get + uri: https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035 + body: + encoding: US-ASCII + string: '' + headers: + Accept: + - application/json + User-Agent: + - rest-client/2.1.0 (linux x86_64) ruby/3.1.4p223 + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Host: + - www.ebi.ac.uk + response: + status: + code: 200 + message: '' + headers: + Vary: + - Access-Control-Request-Headers + - Access-Control-Request-Method + - Origin + Content-Type: + - application/json + Strict-Transport-Security: + - max-age=0 + Date: + - Tue, 06 Feb 2024 10:29:41 GMT + Transfer-Encoding: + - chunked + Content-Disposition: + - inline;filename=f.txt + body: + encoding: UTF-8 + string: |- + { + "iri" : "http://purl.obolibrary.org/obo/GO_0085035", + "lang" : "en", + "description" : [ "A projection from a cell or tissue that penetrates the host's cell wall and invaginates the host cell membrane.", "See also: extrahaustorial matrix ; GO:0085036 and extrahaustorial membrane ; GO:0085037." ], + "synonyms" : [ ], + "annotation" : { + "created_by" : [ "jl" ], + "creation_date" : [ "2010-07-27T03:42:24Z" ], + "has_obo_namespace" : [ "cellular_component" ], + "id" : [ "GO:0085035" ] + }, + "label" : "haustorium", + "ontology_name" : "go", + "ontology_prefix" : "GO", + "ontology_iri" : "http://purl.obolibrary.org/obo/go/extensions/go-plus.owl", + "is_obsolete" : false, + "term_replaced_by" : null, + "is_defining_ontology" : true, + "has_children" : true, + "is_root" : false, + "short_form" : "GO_0085035", + "obo_id" : "GO:0085035", + "in_subset" : null, + "obo_definition_citation" : [ { + "definition" : "A projection from a cell or tissue that penetrates the host's cell wall and invaginates the host cell membrane.", + "oboXrefs" : [ { + "database" : "GOC", + "id" : "pamgo_curators", + "description" : null, + "url" : null + } ] + } ], + "obo_xref" : null, + "obo_synonym" : null, + "is_preferred_root" : false, + "_links" : { + "self" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035?lang=en" + }, + "parents" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/parents" + }, + "ancestors" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/ancestors" + }, + "hierarchicalParents" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/hierarchicalParents" + }, + "hierarchicalAncestors" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/hierarchicalAncestors" + }, + "jstree" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/jstree" + }, + "children" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/children" + }, + "descendants" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/descendants" + }, + "hierarchicalChildren" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/hierarchicalChildren" + }, + "hierarchicalDescendants" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/hierarchicalDescendants" + }, + "graph" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/graph" + } + } + } + http_version: + recorded_at: Tue, 06 Feb 2024 10:29:41 GMT +- request: + method: get + uri: https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/children + body: + encoding: US-ASCII + string: '' + headers: + Accept: + - application/json + User-Agent: + - rest-client/2.1.0 (linux x86_64) ruby/3.1.4p223 + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Host: + - www.ebi.ac.uk + response: + status: + code: 200 + message: '' + headers: + Vary: + - Access-Control-Request-Headers + - Access-Control-Request-Method + - Origin + Content-Type: + - application/json + Strict-Transport-Security: + - max-age=0 + Date: + - Tue, 06 Feb 2024 10:29:41 GMT + Transfer-Encoding: + - chunked + body: + encoding: UTF-8 + string: |- + { + "_embedded" : { + "terms" : [ { + "iri" : "http://purl.obolibrary.org/obo/GO_0085041", + "lang" : "en", + "description" : [ "Highly branched symbiont haustoria within host root cortex cells, responsible for nutrient exchange.", "See also: periarbuscular membrane ; GO:0085042." ], + "synonyms" : [ ], + "annotation" : { + "created_by" : [ "jl" ], + "creation_date" : [ "2010-07-27T04:29:03Z" ], + "has_obo_namespace" : [ "cellular_component" ], + "id" : [ "GO:0085041" ] + }, + "label" : "arbuscule", + "ontology_name" : "go", + "ontology_prefix" : "GO", + "ontology_iri" : "http://purl.obolibrary.org/obo/go/extensions/go-plus.owl", + "is_obsolete" : false, + "term_replaced_by" : null, + "is_defining_ontology" : true, + "has_children" : false, + "is_root" : false, + "short_form" : "GO_0085041", + "obo_id" : "GO:0085041", + "in_subset" : null, + "obo_definition_citation" : [ { + "definition" : "Highly branched symbiont haustoria within host root cortex cells, responsible for nutrient exchange.", + "oboXrefs" : [ { + "database" : "GOC", + "id" : "pamgo_curators", + "description" : null, + "url" : null + } ] + } ], + "obo_xref" : null, + "obo_synonym" : null, + "is_preferred_root" : false, + "_links" : { + "self" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085041?lang=en" + }, + "parents" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085041/parents" + }, + "ancestors" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085041/ancestors" + }, + "hierarchicalParents" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085041/hierarchicalParents" + }, + "hierarchicalAncestors" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085041/hierarchicalAncestors" + }, + "jstree" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085041/jstree" + }, + "graph" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085041/graph" + } + } + } ] + }, + "_links" : { + "self" : { + "href" : "https://www.ebi.ac.uk/ols4/api/ontologies/go/terms/http%253A%252F%252Fpurl.obolibrary.org%252Fobo%252FGO_0085035/children?page=0&size=20" + } + }, + "page" : { + "size" : 20, + "totalElements" : 1, + "totalPages" : 1, + "number" : 0 + } + } + http_version: + recorded_at: Tue, 06 Feb 2024 10:29:41 GMT +recorded_with: VCR 2.9.3 From 9323541f8fcde2ad518384f1a8c2859c6e0a4eb5 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Tue, 6 Feb 2024 11:45:00 +0000 Subject: [PATCH 36/49] update the ols root term validation to support comma separated #1690 --- app/models/sample_controlled_vocab.rb | 18 +++++++++++++-- ...ample_controlled_vocabs_controller_test.rb | 23 +++++++++++++++++++ test/unit/sample_controlled_vocab_test.rb | 22 ++++++++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/app/models/sample_controlled_vocab.rb b/app/models/sample_controlled_vocab.rb index 41f954a002..0e060bf86a 100644 --- a/app/models/sample_controlled_vocab.rb +++ b/app/models/sample_controlled_vocab.rb @@ -1,5 +1,5 @@ class SampleControlledVocab < ApplicationRecord - # attr_accessible :title, :description, :sample_controlled_vocab_terms_attributes + include Seek::UrlValidation has_many :sample_controlled_vocab_terms, inverse_of: :sample_controlled_vocab, after_add: :update_sample_type_templates, @@ -16,8 +16,8 @@ class SampleControlledVocab < ApplicationRecord auto_strip_attributes :ols_root_term_uri validates :title, presence: true, uniqueness: true - validates :ols_root_term_uri, url: { allow_blank: true } validates :key, uniqueness: { allow_blank: true } + validate :validate_ols_root_term_uris accepts_nested_attributes_for :sample_controlled_vocab_terms, allow_destroy: true accepts_nested_attributes_for :repository_standard, reject_if: :check_repository_standard @@ -60,12 +60,26 @@ def ontology_based? source_ontology.present? && ols_root_term_uri.present? end + def validate_ols_root_term_uris + return if self.ols_root_term_uri.blank? + uris = self.ols_root_term_uri.split(',').collect(&:strip) + uris.each do |uri| + unless valid_url?(uri) + errors.add(:ols_root_term_uri, "invalid URI - #{uri}") + return false + end + end + self.ols_root_term_uri = uris.join(', ') + end + private def update_sample_type_templates(_term) sample_types.each(&:queue_template_generation) unless new_record? end + + class SystemVocabs # property -> database key MAPPING = { diff --git a/test/functional/sample_controlled_vocabs_controller_test.rb b/test/functional/sample_controlled_vocabs_controller_test.rb index 4b16e523f8..3fbefc8842 100644 --- a/test/functional/sample_controlled_vocabs_controller_test.rb +++ b/test/functional/sample_controlled_vocabs_controller_test.rb @@ -306,6 +306,29 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase assert_select 'input[type=hidden]#sample_controlled_vocab_sample_controlled_vocab_terms_attributes_3_label[value=?]','trichome papilla' end + test 'create with root uris' do + login_as(FactoryBot.create(:project_administrator)) + assert_difference('SampleControlledVocab.count') do + assert_difference('SampleControlledVocabTerm.count', 2) do + post :create, params: { sample_controlled_vocab: { title: 'plant_cell_papilla and haustorium', description: 'multiple root uris', + ols_root_term_uri: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', + sample_controlled_vocab_terms_attributes: { + '0' => { label: 'plant cell papilla', iri:'http://purl.obolibrary.org/obo/GO_0090395', parent_iri:'', _destroy: '0' }, + '1' => { label: 'haustorium', iri:'http://purl.obolibrary.org/obo/GO_0085035', parent_iri:'', _destroy: '0' } + } + } } + end + end + assert cv = assigns(:sample_controlled_vocab) + assert_redirected_to sample_controlled_vocab_path(cv) + assert_equal 'plant_cell_papilla and haustorium', cv.title + assert_equal 'multiple root uris', cv.description + assert_equal 2, cv.sample_controlled_vocab_terms.count + assert_equal ['plant cell papilla','haustorium'], cv.labels + assert_equal ['http://purl.obolibrary.org/obo/GO_0090395','http://purl.obolibrary.org/obo/GO_0085035'], cv.sample_controlled_vocab_terms.collect(&:iri) + assert_equal 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', cv.ols_root_term_uri + end + test 'fetch ols terms as HTML with multiple root uris and root term included' do person = FactoryBot.create(:person) login_as(person) diff --git a/test/unit/sample_controlled_vocab_test.rb b/test/unit/sample_controlled_vocab_test.rb index a2b07721ba..d5f6798b90 100644 --- a/test/unit/sample_controlled_vocab_test.rb +++ b/test/unit/sample_controlled_vocab_test.rb @@ -35,6 +35,28 @@ class SampleControlledVocabTest < ActiveSupport::TestCase end end + test 'validate ols ols_root_term_uris' do + vocab = SampleControlledVocab.new(title: 'multiple uris') + assert vocab.valid? + + vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395' + assert vocab.valid? + vocab.ols_root_term_uri = 'wibble' + refute vocab.valid? + + vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035' + assert vocab.valid? + vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, http://purl.obolibrary.org/obo/GO_0090396' + assert vocab.valid? + assert_equal 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, http://purl.obolibrary.org/obo/GO_0090396', vocab.ols_root_term_uri + vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, wibble' + refute vocab.valid? + + vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, ' + assert vocab.valid? + assert_equal 'http://purl.obolibrary.org/obo/GO_0090395', vocab.ols_root_term_uri + end + test 'validate unique key' do User.with_current_user(FactoryBot.create(:project_administrator).user) do SampleControlledVocab.create(title: 'no key') From 07ab74b5d05ece2da6264fa6ad51cc867c8ea72e Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Tue, 6 Feb 2024 13:34:56 +0000 Subject: [PATCH 37/49] pluralized ols_root_to_uri database field on sample controlled vocabs #1690 --- app/assets/javascripts/controlled_vocabs.js.erb | 4 ++-- .../sample_controlled_vocabs_controller.rb | 2 +- app/models/sample_controlled_vocab.rb | 12 ++++++------ .../sample_controlled_vocab_serializer.rb | 2 +- .../sample_controlled_vocabs/_form.html.erb | 2 +- app/views/sample_controlled_vocabs/show.html.erb | 2 +- .../data-annotations-controlled-vocab.json | 2 +- .../format-annotations-controlled-vocab.json | 2 +- .../operation-annotations-controlled-vocab.json | 2 +- .../topics-annotations-controlled-vocab.json | 2 +- ...2054_change_controlled_vocab_root_term_uri.rb | 5 +++++ db/schema.rb | 4 ++-- db/seeds/011_topics_controlled_vocab.seeds.rb | 2 +- .../012_operations_controlled_vocab.seeds.rb | 2 +- db/seeds/013_formats_controlled_vocab.seeds.rb | 2 +- db/seeds/014_data_controlled_vocab.seeds.rb | 2 +- lib/isa_exporter.rb | 2 +- lib/seek/isa_templates/template_extractor.rb | 2 +- lib/tasks/seek_dev.rake | 2 +- .../api/examples/sampleControlledVocabPatch.json | 2 +- .../sampleControlledVocabPatchResponse.json | 2 +- .../api/examples/sampleControlledVocabPost.json | 2 +- .../sampleControlledVocabPostResponse.json | 2 +- test/factories/sample_attribute_types.rb | 14 +++++++------- .../patch_max_sample_controlled_vocab.json.erb | 2 +- .../post_max_sample_controlled_vocab.json.erb | 2 +- .../get_max_sample_controlled_vocab.json.erb | 2 +- .../get_min_sample_controlled_vocab.json.erb | 2 +- .../sample_controlled_vocabs_controller_test.rb | 4 ++-- .../api/sample_controlled_vocab_api_test.rb | 2 +- test/unit/sample_controlled_vocab_test.rb | 16 ++++++++-------- 31 files changed, 56 insertions(+), 51 deletions(-) create mode 100644 db/migrate/20240206132054_change_controlled_vocab_root_term_uri.rb diff --git a/app/assets/javascripts/controlled_vocabs.js.erb b/app/assets/javascripts/controlled_vocabs.js.erb index db2a303dd3..214351975f 100644 --- a/app/assets/javascripts/controlled_vocabs.js.erb +++ b/app/assets/javascripts/controlled_vocabs.js.erb @@ -137,7 +137,7 @@ CVTerms = { dataType: "html", data: { source_ontology_id: $j('select#sample_controlled_vocab_source_ontology').val(), - root_uri: $j('#sample_controlled_vocab_ols_root_term_uri').val(), + root_uri: $j('#sample_controlled_vocab_ols_root_term_uris').val(), include_root_term: $j('#include_root_term:checked').val() }, success: function (resp) { @@ -179,7 +179,7 @@ CVTerms = { $j('select#sample_controlled_vocab_source_ontology').on('change', function () { var selected = this.selectedOptions[0]; if (selected.value == "") { - $j('#sample_controlled_vocab_ols_root_term_uri').val(''); + $j('#sample_controlled_vocab_ols_root_term_uris').val(''); $j('#ontology-root-uri').hide(); } else { $j('#ontology-root-uri').show(); diff --git a/app/controllers/sample_controlled_vocabs_controller.rb b/app/controllers/sample_controlled_vocabs_controller.rb index c4549e7009..d8842cca3e 100644 --- a/app/controllers/sample_controlled_vocabs_controller.rb +++ b/app/controllers/sample_controlled_vocabs_controller.rb @@ -132,7 +132,7 @@ def typeahead private def cv_params - params.require(:sample_controlled_vocab).permit(:title, :description, :group, :source_ontology, :ols_root_term_uri, + params.require(:sample_controlled_vocab).permit(:title, :description, :group, :source_ontology, :ols_root_term_uris, :required, :short_name, { sample_controlled_vocab_terms_attributes: %i[id _destroy label iri parent_iri] }) diff --git a/app/models/sample_controlled_vocab.rb b/app/models/sample_controlled_vocab.rb index 0e060bf86a..73bd6b82a4 100644 --- a/app/models/sample_controlled_vocab.rb +++ b/app/models/sample_controlled_vocab.rb @@ -13,7 +13,7 @@ class SampleControlledVocab < ApplicationRecord has_many :samples, through: :sample_types belongs_to :repository_standard, inverse_of: :sample_controlled_vocabs - auto_strip_attributes :ols_root_term_uri + auto_strip_attributes :ols_root_term_uris validates :title, presence: true, uniqueness: true validates :key, uniqueness: { allow_blank: true } @@ -57,19 +57,19 @@ def self.can_create? # whether the controlled vocab is linked to an ontology def ontology_based? - source_ontology.present? && ols_root_term_uri.present? + source_ontology.present? && ols_root_term_uris.present? end def validate_ols_root_term_uris - return if self.ols_root_term_uri.blank? - uris = self.ols_root_term_uri.split(',').collect(&:strip) + return if self.ols_root_term_uris.blank? + uris = self.ols_root_term_uris.split(',').collect(&:strip) uris.each do |uri| unless valid_url?(uri) - errors.add(:ols_root_term_uri, "invalid URI - #{uri}") + errors.add(:ols_root_term_uris, "invalid URI - #{uri}") return false end end - self.ols_root_term_uri = uris.join(', ') + self.ols_root_term_uris = uris.join(', ') end private diff --git a/app/serializers/sample_controlled_vocab_serializer.rb b/app/serializers/sample_controlled_vocab_serializer.rb index 7700a2a34f..dcccc2c08f 100644 --- a/app/serializers/sample_controlled_vocab_serializer.rb +++ b/app/serializers/sample_controlled_vocab_serializer.rb @@ -1,5 +1,5 @@ class SampleControlledVocabSerializer < BaseSerializer - attributes :title, :description, :source_ontology, :ols_root_term_uri, :short_name + attributes :title, :description, :source_ontology, :ols_root_term_uris, :short_name attributes :sample_controlled_vocab_terms_attributes has_many :sample_controlled_vocab_terms diff --git a/app/views/sample_controlled_vocabs/_form.html.erb b/app/views/sample_controlled_vocabs/_form.html.erb index a79d14aed1..54ad43a833 100644 --- a/app/views/sample_controlled_vocabs/_form.html.erb +++ b/app/views/sample_controlled_vocabs/_form.html.erb @@ -42,7 +42,7 @@
- <%= f.text_field :ols_root_term_uri, :class => 'form-control', :placeholder => 'e.g. http://www.ebi.ac.uk/efo/EFO_0000635' %> + <%= f.text_field :ols_root_term_uris, :class => 'form-control', :placeholder => 'e.g. http://www.ebi.ac.uk/efo/EFO_0000635' %>
diff --git a/app/views/sample_controlled_vocabs/show.html.erb b/app/views/sample_controlled_vocabs/show.html.erb index bbffcffc69..9b53dea7d9 100644 --- a/app/views/sample_controlled_vocabs/show.html.erb +++ b/app/views/sample_controlled_vocabs/show.html.erb @@ -11,7 +11,7 @@

Root URI: - <%= ols_root_term_link(@sample_controlled_vocab.source_ontology,@sample_controlled_vocab.ols_root_term_uri) %> + <%= ols_root_term_link(@sample_controlled_vocab.source_ontology,@sample_controlled_vocab.ols_root_term_uris) %>

<% end %> diff --git a/config/default_data/data-annotations-controlled-vocab.json b/config/default_data/data-annotations-controlled-vocab.json index 826415f073..f93a6c4896 100644 --- a/config/default_data/data-annotations-controlled-vocab.json +++ b/config/default_data/data-annotations-controlled-vocab.json @@ -1,7 +1,7 @@ { "title": "Data types", "description": "Data types, used for annotating. Describes information that is that can be processed by dedicated computational tools that can use the data as input or produce it as output. Initially seeded from the EDAM ontology", - "ols_root_term_uri": "http://edamontology.org/data_0006", + "ols_root_term_uris": "http://edamontology.org/data_0006", "source_ontology": "edam", "terms": [ { diff --git a/config/default_data/format-annotations-controlled-vocab.json b/config/default_data/format-annotations-controlled-vocab.json index 88fa862632..cffad4787d 100644 --- a/config/default_data/format-annotations-controlled-vocab.json +++ b/config/default_data/format-annotations-controlled-vocab.json @@ -1,7 +1,7 @@ { "title": "Data Formats", "description": "Data formats, used for annotating. Describes a defined way or layout of representing and structuring data in a computer file, blob, string, message, or elsewhere. Initially seeded from the EDAM ontology.", - "ols_root_term_uri": "http://edamontology.org/format_1915", + "ols_root_term_uris": "http://edamontology.org/format_1915", "source_ontology": "edam", "terms": [ { diff --git a/config/default_data/operation-annotations-controlled-vocab.json b/config/default_data/operation-annotations-controlled-vocab.json index fadf4e2bee..748695f726 100644 --- a/config/default_data/operation-annotations-controlled-vocab.json +++ b/config/default_data/operation-annotations-controlled-vocab.json @@ -1,7 +1,7 @@ { "title": "Operations", "description": "Operations, used for annotating. Describes a function that can take place over some inputs to produce an output. Initially seeded from the EDAM ontology", - "ols_root_term_uri": "http://edamontology.org/operation_0004", + "ols_root_term_uris": "http://edamontology.org/operation_0004", "source_ontology": "edam", "terms": [ { diff --git a/config/default_data/topics-annotations-controlled-vocab.json b/config/default_data/topics-annotations-controlled-vocab.json index b7dcdc0e1f..d09e156870 100644 --- a/config/default_data/topics-annotations-controlled-vocab.json +++ b/config/default_data/topics-annotations-controlled-vocab.json @@ -1,7 +1,7 @@ { "title": "Topics", "description": "Topics, used for annotating. Describes the domain, field of interest, of study, application, work, data, or technology. Initially seeded from the EDAM ontology.", - "ols_root_term_uri": "http://edamontology.org/topic_0003", + "ols_root_term_uris": "http://edamontology.org/topic_0003", "source_ontology": "edam", "terms": [ { diff --git a/db/migrate/20240206132054_change_controlled_vocab_root_term_uri.rb b/db/migrate/20240206132054_change_controlled_vocab_root_term_uri.rb new file mode 100644 index 0000000000..5bc256b6dd --- /dev/null +++ b/db/migrate/20240206132054_change_controlled_vocab_root_term_uri.rb @@ -0,0 +1,5 @@ +class ChangeControlledVocabRootTermUri < ActiveRecord::Migration[6.1] + def change + rename_column :sample_controlled_vocabs, :ols_root_term_uri, :ols_root_term_uris + end +end diff --git a/db/schema.rb b/db/schema.rb index 2f51e08778..c9d24b5489 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2024_01_12_141513) do +ActiveRecord::Schema.define(version: 2024_02_06_132054) do create_table "activity_logs", id: :integer, force: :cascade do |t| t.string "action" @@ -1747,7 +1747,7 @@ t.datetime "updated_at", null: false t.string "first_letter", limit: 1 t.string "source_ontology" - t.string "ols_root_term_uri" + t.string "ols_root_term_uris" t.string "short_name" t.string "key" t.integer "template_id" diff --git a/db/seeds/011_topics_controlled_vocab.seeds.rb b/db/seeds/011_topics_controlled_vocab.seeds.rb index e1c63a3293..aaa81baa0c 100644 --- a/db/seeds/011_topics_controlled_vocab.seeds.rb +++ b/db/seeds/011_topics_controlled_vocab.seeds.rb @@ -8,7 +8,7 @@ key: SampleControlledVocab::SystemVocabs.database_key_for_property(:topics), description: data[:description], source_ontology: data[:source_ontology], - ols_root_term_uri: data[:ols_root_term_uri]) + ols_root_term_uris: data[:ols_root_term_uris]) data[:terms].each do |term| vocab.sample_controlled_vocab_terms << SampleControlledVocabTerm.new(label: term[:label], iri: term[:iri], parent_iri: term[:parent_iri]) end diff --git a/db/seeds/012_operations_controlled_vocab.seeds.rb b/db/seeds/012_operations_controlled_vocab.seeds.rb index f16809ba2b..0ed2dbd13c 100644 --- a/db/seeds/012_operations_controlled_vocab.seeds.rb +++ b/db/seeds/012_operations_controlled_vocab.seeds.rb @@ -7,7 +7,7 @@ key: SampleControlledVocab::SystemVocabs.database_key_for_property(:operations), description: data[:description], source_ontology: data[:source_ontology], - ols_root_term_uri: data[:ols_root_term_uri]) + ols_root_term_uris: data[:ols_root_term_uris]) data[:terms].each do |term| vocab.sample_controlled_vocab_terms << SampleControlledVocabTerm.new(label: term[:label], iri: term[:iri], parent_iri: term[:parent_iri]) end diff --git a/db/seeds/013_formats_controlled_vocab.seeds.rb b/db/seeds/013_formats_controlled_vocab.seeds.rb index 2e65bd28ce..23f994e473 100644 --- a/db/seeds/013_formats_controlled_vocab.seeds.rb +++ b/db/seeds/013_formats_controlled_vocab.seeds.rb @@ -7,7 +7,7 @@ key: SampleControlledVocab::SystemVocabs.database_key_for_property(:data_formats), description: data[:description], source_ontology: data[:source_ontology], - ols_root_term_uri: data[:ols_root_term_uri]) + ols_root_term_uris: data[:ols_root_term_uris]) data[:terms].each do |term| vocab.sample_controlled_vocab_terms << SampleControlledVocabTerm.new(label: term[:label], iri: term[:iri], parent_iri: term[:parent_iri]) end diff --git a/db/seeds/014_data_controlled_vocab.seeds.rb b/db/seeds/014_data_controlled_vocab.seeds.rb index e01f199a70..3140159095 100644 --- a/db/seeds/014_data_controlled_vocab.seeds.rb +++ b/db/seeds/014_data_controlled_vocab.seeds.rb @@ -7,7 +7,7 @@ key: SampleControlledVocab::SystemVocabs.database_key_for_property(:data_types), description: data[:description], source_ontology: data[:source_ontology], - ols_root_term_uri: data[:ols_root_term_uri]) + ols_root_term_uris: data[:ols_root_term_uris]) data[:terms].each do |term| vocab.sample_controlled_vocab_terms << SampleControlledVocabTerm.new(label: term[:label], iri: term[:iri], parent_iri: term[:parent_iri]) end diff --git a/lib/isa_exporter.rb b/lib/isa_exporter.rb index 5d5a3ff288..853cb4f489 100644 --- a/lib/isa_exporter.rb +++ b/lib/isa_exporter.rb @@ -725,7 +725,7 @@ def get_ontology_details(sample_attribute, label, vocab_term) if vocab_term sample_attribute.sample_controlled_vocab.sample_controlled_vocab_terms.find_by_label(label)&.iri else - sample_attribute.sample_controlled_vocab.ols_root_term_uri + sample_attribute.sample_controlled_vocab.ols_root_term_uris end end term_accession = iri || '' diff --git a/lib/seek/isa_templates/template_extractor.rb b/lib/seek/isa_templates/template_extractor.rb index b048f8a56c..d0eb8d6f2c 100644 --- a/lib/seek/isa_templates/template_extractor.rb +++ b/lib/seek/isa_templates/template_extractor.rb @@ -46,7 +46,7 @@ def self.extract_templates { title: attribute['name'], source_ontology: is_ontology ? attribute['ontology']['name'] : nil, - ols_root_term_uri: is_ontology ? attribute['ontology']['rootTermURI'] : nil + ols_root_term_uris: is_ontology ? attribute['ontology']['rootTermURI'] : nil } ) end diff --git a/lib/tasks/seek_dev.rake b/lib/tasks/seek_dev.rake index d54973eb6c..f0405daca1 100644 --- a/lib/tasks/seek_dev.rake +++ b/lib/tasks/seek_dev.rake @@ -53,7 +53,7 @@ namespace :seek_dev do task(:dump_controlled_vocab, [:id] => :environment) do |_t, args| vocab = SampleControlledVocab.find(args.id) - json = { title: vocab.title, description: vocab.description, ols_root_term_uri: vocab.ols_root_term_uri, + json = { title: vocab.title, description: vocab.description, ols_root_term_uris: vocab.ols_root_term_uris, source_ontology: vocab.source_ontology, terms: [] } vocab.sample_controlled_vocab_terms.each do |term| json[:terms] << { label: term.label, iri: term.iri, parent_iri: term.parent_iri } diff --git a/public/api/examples/sampleControlledVocabPatch.json b/public/api/examples/sampleControlledVocabPatch.json index 70834236d3..da157689e0 100644 --- a/public/api/examples/sampleControlledVocabPatch.json +++ b/public/api/examples/sampleControlledVocabPatch.json @@ -6,7 +6,7 @@ "title": "new title", "description": "new description", "source_ontology": "new source ontology", - "ols_root_term_uri": "http://new-uri.org", + "ols_root_term_uris": "http://new-uri.org", "short_name": "new short name", "sample_controlled_vocab_terms_attributes": [ { diff --git a/public/api/examples/sampleControlledVocabPatchResponse.json b/public/api/examples/sampleControlledVocabPatchResponse.json index eb9543717b..b02456a5a8 100644 --- a/public/api/examples/sampleControlledVocabPatchResponse.json +++ b/public/api/examples/sampleControlledVocabPatchResponse.json @@ -6,7 +6,7 @@ "title": "new title", "description": "new description", "source_ontology": "new source ontology", - "ols_root_term_uri": "http://new-uri.org", + "ols_root_term_uris": "http://new-uri.org", "short_name": "new short name", "sample_controlled_vocab_terms_attributes": [ { diff --git a/public/api/examples/sampleControlledVocabPost.json b/public/api/examples/sampleControlledVocabPost.json index bb72ca6d9c..0a8ea33735 100644 --- a/public/api/examples/sampleControlledVocabPost.json +++ b/public/api/examples/sampleControlledVocabPost.json @@ -5,7 +5,7 @@ "title": "New Vocab Max", "description": "some description", "source_ontology": "EFO", - "ols_root_term_uri": null, + "ols_root_term_uris": null, "short_name": "short_name", "sample_controlled_vocab_terms_attributes": [ { diff --git a/public/api/examples/sampleControlledVocabPostResponse.json b/public/api/examples/sampleControlledVocabPostResponse.json index 947b4be3b0..e7fccd07f4 100644 --- a/public/api/examples/sampleControlledVocabPostResponse.json +++ b/public/api/examples/sampleControlledVocabPostResponse.json @@ -6,7 +6,7 @@ "title": "New Vocab Max", "description": "some description", "source_ontology": "EFO", - "ols_root_term_uri": null, + "ols_root_term_uris": null, "short_name": "short_name", "sample_controlled_vocab_terms_attributes": [ { diff --git a/test/factories/sample_attribute_types.rb b/test/factories/sample_attribute_types.rb index 5f4ce68c24..93706d1c13 100644 --- a/test/factories/sample_attribute_types.rb +++ b/test/factories/sample_attribute_types.rb @@ -129,7 +129,7 @@ factory(:ontology_sample_controlled_vocab, parent: :sample_controlled_vocab) do source_ontology { 'http://ontology.org' } - ols_root_term_uri { 'http://ontology.org/#parent' } + ols_root_term_uris { 'http://ontology.org/#parent' } after(:build) do |vocab| vocab.sample_controlled_vocab_terms << FactoryBot.build(:sample_controlled_vocab_term, label: 'Parent',iri:'http://ontology.org/#parent',parent_iri:'') vocab.sample_controlled_vocab_terms << FactoryBot.build(:sample_controlled_vocab_term, label: 'Mother',iri:'http://ontology.org/#mother',parent_iri:'http://ontology.org/#parent') @@ -139,7 +139,7 @@ factory(:topics_controlled_vocab, parent: :sample_controlled_vocab) do title { 'Topics' } - ols_root_term_uri { 'http://edamontology.org/topic_0003' } + ols_root_term_uris { 'http://edamontology.org/topic_0003' } key { SampleControlledVocab::SystemVocabs.database_key_for_property(:topics) } source_ontology { 'edam' } after(:build) do |vocab| @@ -152,7 +152,7 @@ factory(:operations_controlled_vocab, parent: :sample_controlled_vocab) do title { 'Operations' } - ols_root_term_uri { 'http://edamontology.org/operation_0004' } + ols_root_term_uris { 'http://edamontology.org/operation_0004' } key { SampleControlledVocab::SystemVocabs.database_key_for_property(:operations) } source_ontology { 'edam' } after(:build) do |vocab| @@ -165,7 +165,7 @@ factory(:data_types_controlled_vocab, parent: :sample_controlled_vocab) do title { 'Data' } - ols_root_term_uri { 'http://edamontology.org/data_0006' } + ols_root_term_uris { 'http://edamontology.org/data_0006' } key { SampleControlledVocab::SystemVocabs.database_key_for_property(:data_types) } source_ontology { 'edam' } after(:build) do |vocab| @@ -176,7 +176,7 @@ factory(:data_formats_controlled_vocab, parent: :sample_controlled_vocab) do title { 'Formats' } - ols_root_term_uri { 'http://edamontology.org/format_1915' } + ols_root_term_uris { 'http://edamontology.org/format_1915' } key { SampleControlledVocab::SystemVocabs.database_key_for_property(:data_formats) } source_ontology { 'edam' } after(:build) do |vocab| @@ -188,7 +188,7 @@ factory(:efo_ontology, class: SampleControlledVocab) do sequence(:title) { |n| "EFO ontology #{n}" } source_ontology { 'EFO' } - ols_root_term_uri { 'http://www.ebi.ac.uk/efo/EFO_0000635' } + ols_root_term_uris { 'http://www.ebi.ac.uk/efo/EFO_0000635' } after(:build) do |vocab| vocab.sample_controlled_vocab_terms << FactoryBot.build(:sample_controlled_vocab_term, label: 'anatomical entity') vocab.sample_controlled_vocab_terms << FactoryBot.build(:sample_controlled_vocab_term, label: 'retroperitoneal space') @@ -199,7 +199,7 @@ factory(:obi_ontology, class: SampleControlledVocab) do sequence(:title) { |n| "OBI ontology #{n}" } source_ontology { 'OBI' } - ols_root_term_uri { 'http://purl.obolibrary.org/obo/OBI_0000094' } + ols_root_term_uris { 'http://purl.obolibrary.org/obo/OBI_0000094' } after(:build) do |vocab| vocab.sample_controlled_vocab_terms << FactoryBot.build(:sample_controlled_vocab_term, label: 'dissection') vocab.sample_controlled_vocab_terms << FactoryBot.build(:sample_controlled_vocab_term, label: 'enzymatic cleavage') diff --git a/test/fixtures/json/requests/patch_max_sample_controlled_vocab.json.erb b/test/fixtures/json/requests/patch_max_sample_controlled_vocab.json.erb index 5e84d07778..af1715d021 100644 --- a/test/fixtures/json/requests/patch_max_sample_controlled_vocab.json.erb +++ b/test/fixtures/json/requests/patch_max_sample_controlled_vocab.json.erb @@ -6,7 +6,7 @@ "title": "new title", "description": "new description", "source_ontology": "new source ontology", - "ols_root_term_uri": "http://new-uri.org", + "ols_root_term_uris": "http://new-uri.org", "short_name": "new short name", "sample_controlled_vocab_terms_attributes": [ { diff --git a/test/fixtures/json/requests/post_max_sample_controlled_vocab.json.erb b/test/fixtures/json/requests/post_max_sample_controlled_vocab.json.erb index 97167af91b..9afd4e918c 100644 --- a/test/fixtures/json/requests/post_max_sample_controlled_vocab.json.erb +++ b/test/fixtures/json/requests/post_max_sample_controlled_vocab.json.erb @@ -5,7 +5,7 @@ "title": "New Vocab Max", "description": "some description", "source_ontology": "EFO", - "ols_root_term_uri": null, + "ols_root_term_uris": null, "short_name": "short_name", "sample_controlled_vocab_terms_attributes": [ { diff --git a/test/fixtures/json/responses/get_max_sample_controlled_vocab.json.erb b/test/fixtures/json/responses/get_max_sample_controlled_vocab.json.erb index 66f1f2414c..536f12227c 100644 --- a/test/fixtures/json/responses/get_max_sample_controlled_vocab.json.erb +++ b/test/fixtures/json/responses/get_max_sample_controlled_vocab.json.erb @@ -6,7 +6,7 @@ "title": "Organism", "description": "Description for organism", "source_ontology": "EFO", - "ols_root_term_uri": "", + "ols_root_term_uris": "", "short_name": "organism", "sample_controlled_vocab_terms_attributes": [ { diff --git a/test/fixtures/json/responses/get_min_sample_controlled_vocab.json.erb b/test/fixtures/json/responses/get_min_sample_controlled_vocab.json.erb index f824be5a53..8286fb87ab 100644 --- a/test/fixtures/json/responses/get_min_sample_controlled_vocab.json.erb +++ b/test/fixtures/json/responses/get_min_sample_controlled_vocab.json.erb @@ -6,7 +6,7 @@ "title": "Organism", "description": null, "source_ontology": null, - "ols_root_term_uri": null, + "ols_root_term_uris": null, "short_name": null, "sample_controlled_vocab_terms_attributes": null, "repository_standard": null diff --git a/test/functional/sample_controlled_vocabs_controller_test.rb b/test/functional/sample_controlled_vocabs_controller_test.rb index 3fbefc8842..7edebe38f5 100644 --- a/test/functional/sample_controlled_vocabs_controller_test.rb +++ b/test/functional/sample_controlled_vocabs_controller_test.rb @@ -311,7 +311,7 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase assert_difference('SampleControlledVocab.count') do assert_difference('SampleControlledVocabTerm.count', 2) do post :create, params: { sample_controlled_vocab: { title: 'plant_cell_papilla and haustorium', description: 'multiple root uris', - ols_root_term_uri: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', + ols_root_term_uris: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', sample_controlled_vocab_terms_attributes: { '0' => { label: 'plant cell papilla', iri:'http://purl.obolibrary.org/obo/GO_0090395', parent_iri:'', _destroy: '0' }, '1' => { label: 'haustorium', iri:'http://purl.obolibrary.org/obo/GO_0085035', parent_iri:'', _destroy: '0' } @@ -326,7 +326,7 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase assert_equal 2, cv.sample_controlled_vocab_terms.count assert_equal ['plant cell papilla','haustorium'], cv.labels assert_equal ['http://purl.obolibrary.org/obo/GO_0090395','http://purl.obolibrary.org/obo/GO_0085035'], cv.sample_controlled_vocab_terms.collect(&:iri) - assert_equal 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', cv.ols_root_term_uri + assert_equal 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', cv.ols_root_term_uris end test 'fetch ols terms as HTML with multiple root uris and root term included' do diff --git a/test/integration/api/sample_controlled_vocab_api_test.rb b/test/integration/api/sample_controlled_vocab_api_test.rb index 809f9439ae..e8def6b2d2 100644 --- a/test/integration/api/sample_controlled_vocab_api_test.rb +++ b/test/integration/api/sample_controlled_vocab_api_test.rb @@ -9,7 +9,7 @@ def setup login_as(FactoryBot.create(:project_administrator)) @sample_controlled_vocab = SampleControlledVocab.new({ title:"a title", description:"some description", - source_ontology: "EFO", ols_root_term_uri: "http://a_uri", + source_ontology: "EFO", ols_root_term_uris: "http://a_uri", short_name: "short_name" }) @sample_controlled_vocab_term = SampleControlledVocabTerm.new({ label: "organism", iri: "http://some_iri", diff --git a/test/unit/sample_controlled_vocab_test.rb b/test/unit/sample_controlled_vocab_test.rb index d5f6798b90..16ab2d1f62 100644 --- a/test/unit/sample_controlled_vocab_test.rb +++ b/test/unit/sample_controlled_vocab_test.rb @@ -39,22 +39,22 @@ class SampleControlledVocabTest < ActiveSupport::TestCase vocab = SampleControlledVocab.new(title: 'multiple uris') assert vocab.valid? - vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395' + vocab.ols_root_term_uris = 'http://purl.obolibrary.org/obo/GO_0090395' assert vocab.valid? - vocab.ols_root_term_uri = 'wibble' + vocab.ols_root_term_uris = 'wibble' refute vocab.valid? - vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035' + vocab.ols_root_term_uris = 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035' assert vocab.valid? - vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, http://purl.obolibrary.org/obo/GO_0090396' + vocab.ols_root_term_uris = 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, http://purl.obolibrary.org/obo/GO_0090396' assert vocab.valid? - assert_equal 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, http://purl.obolibrary.org/obo/GO_0090396', vocab.ols_root_term_uri - vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, wibble' + assert_equal 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, http://purl.obolibrary.org/obo/GO_0090396', vocab.ols_root_term_uris + vocab.ols_root_term_uris = 'http://purl.obolibrary.org/obo/GO_0090395, wibble' refute vocab.valid? - vocab.ols_root_term_uri = 'http://purl.obolibrary.org/obo/GO_0090395, ' + vocab.ols_root_term_uris = 'http://purl.obolibrary.org/obo/GO_0090395, ' assert vocab.valid? - assert_equal 'http://purl.obolibrary.org/obo/GO_0090395', vocab.ols_root_term_uri + assert_equal 'http://purl.obolibrary.org/obo/GO_0090395', vocab.ols_root_term_uris end test 'validate unique key' do From 186cbdece8053d5577ce71a801b4362b4ce8ed79 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Tue, 6 Feb 2024 14:16:00 +0000 Subject: [PATCH 38/49] pluralized root_uris parameter used to fetch terms #1690 --- .../javascripts/controlled_vocabs.js.erb | 2 +- .../sample_controlled_vocabs_controller.rb | 6 ++--- app/models/sample_controlled_vocab.rb | 2 +- .../sample_controlled_vocabs/_form.html.erb | 8 +++--- ...ample_controlled_vocabs_controller_test.rb | 26 +++++++++++++++---- test/unit/sample_controlled_vocab_test.rb | 4 +++ 6 files changed, 35 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/controlled_vocabs.js.erb b/app/assets/javascripts/controlled_vocabs.js.erb index 214351975f..c2fbd1af68 100644 --- a/app/assets/javascripts/controlled_vocabs.js.erb +++ b/app/assets/javascripts/controlled_vocabs.js.erb @@ -137,7 +137,7 @@ CVTerms = { dataType: "html", data: { source_ontology_id: $j('select#sample_controlled_vocab_source_ontology').val(), - root_uri: $j('#sample_controlled_vocab_ols_root_term_uris').val(), + root_uris: $j('#sample_controlled_vocab_ols_root_term_uris').val(), include_root_term: $j('#include_root_term:checked').val() }, success: function (resp) { diff --git a/app/controllers/sample_controlled_vocabs_controller.rb b/app/controllers/sample_controlled_vocabs_controller.rb index d8842cca3e..a06b2afce9 100644 --- a/app/controllers/sample_controlled_vocabs_controller.rb +++ b/app/controllers/sample_controlled_vocabs_controller.rb @@ -89,12 +89,12 @@ def fetch_ols_terms_html error_msg = nil begin source_ontology = params[:source_ontology_id] - root_uri = params[:root_uri] + root_uris = params[:root_uris] - raise 'No root URI provided' if root_uri.blank? + raise 'No root URI provided' if root_uris.blank? @terms = [] client = Ebi::OlsClient.new - root_uri.split(',').collect(&:strip).each do |uri| + root_uris.split(',').collect(&:strip).reject(&:blank?).each do |uri| terms = client.all_descendants(source_ontology, uri) terms.reject! { |t| t[:iri] == uri } unless params[:include_root_term] == '1' @terms = @terms | terms diff --git a/app/models/sample_controlled_vocab.rb b/app/models/sample_controlled_vocab.rb index 73bd6b82a4..cb7c8dc7ce 100644 --- a/app/models/sample_controlled_vocab.rb +++ b/app/models/sample_controlled_vocab.rb @@ -62,7 +62,7 @@ def ontology_based? def validate_ols_root_term_uris return if self.ols_root_term_uris.blank? - uris = self.ols_root_term_uris.split(',').collect(&:strip) + uris = self.ols_root_term_uris.split(',').collect(&:strip).reject(&:blank?) uris.each do |uri| unless valid_url?(uri) errors.add(:ols_root_term_uris, "invalid URI - #{uri}") diff --git a/app/views/sample_controlled_vocabs/_form.html.erb b/app/views/sample_controlled_vocabs/_form.html.erb index 54ad43a833..74556c98ef 100644 --- a/app/views/sample_controlled_vocabs/_form.html.erb +++ b/app/views/sample_controlled_vocabs/_form.html.erb @@ -39,17 +39,19 @@ You have selected the ontology, click the link to browse on the Ontology Lookup Service in another tab and find the suitable root term URI. You should then copy that URI into the field below. + If you wish to include terms from more than one root, then add the URI's separated by a comma (,).
+
- + <%= f.text_field :ols_root_term_uris, :class => 'form-control', :placeholder => 'e.g. http://www.ebi.ac.uk/efo/EFO_0000635' %>
diff --git a/test/functional/sample_controlled_vocabs_controller_test.rb b/test/functional/sample_controlled_vocabs_controller_test.rb index 7edebe38f5..85a9f08650 100644 --- a/test/functional/sample_controlled_vocabs_controller_test.rb +++ b/test/functional/sample_controlled_vocabs_controller_test.rb @@ -273,7 +273,7 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase login_as(person) VCR.use_cassette('ols/fetch_obo_bad_term') do get :fetch_ols_terms_html, params: { source_ontology_id: 'go', - root_uri: 'http://purl.obolibrary.org/obo/banana', + root_uris: 'http://purl.obolibrary.org/obo/banana', include_root_term: '1' } assert_response :unprocessable_entity @@ -286,7 +286,7 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase login_as(person) VCR.use_cassette('ols/fetch_obo_plant_cell_papilla') do get :fetch_ols_terms_html, params: { source_ontology_id: 'go', - root_uri: 'http://purl.obolibrary.org/obo/GO_0090395', + root_uris: 'http://purl.obolibrary.org/obo/GO_0090395', include_root_term: '1' } end @@ -335,7 +335,7 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase VCR.use_cassette('ols/fetch_obo_plant_cell_papilla') do VCR.use_cassette('ols/fetch_obo_haustorium') do get :fetch_ols_terms_html, params: { source_ontology_id: 'go', - root_uri: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', + root_uris: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', include_root_term: '1' } end end @@ -370,7 +370,7 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase VCR.use_cassette('ols/fetch_obo_plant_cell_papilla') do VCR.use_cassette('ols/fetch_obo_haustorium') do get :fetch_ols_terms_html, params: { source_ontology_id: 'go', - root_uri: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', + root_uris: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', include_root_term: '0' } end end @@ -394,12 +394,28 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase end + test 'fetch ols terms as HTML with multiple root uris forgiving of trailing comma' do + person = FactoryBot.create(:person) + login_as(person) + VCR.use_cassette('ols/fetch_obo_plant_cell_papilla') do + VCR.use_cassette('ols/fetch_obo_haustorium') do + get :fetch_ols_terms_html, params: { source_ontology_id: 'go', + root_uris: 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, ', + include_root_term: '0' } + end + end + + assert_response :success + assert_select 'tr.sample-cv-term', count: 4 + end + + test 'fetch ols terms as HTML without root term included' do person = FactoryBot.create(:person) login_as(person) VCR.use_cassette('ols/fetch_obo_plant_cell_papilla') do get :fetch_ols_terms_html, params: { source_ontology_id: 'go', - root_uri: 'http://purl.obolibrary.org/obo/GO_0090395' } + root_uris: 'http://purl.obolibrary.org/obo/GO_0090395' } end assert_response :success assert_select 'tr.sample-cv-term', count: 3 diff --git a/test/unit/sample_controlled_vocab_test.rb b/test/unit/sample_controlled_vocab_test.rb index 16ab2d1f62..6918c77450 100644 --- a/test/unit/sample_controlled_vocab_test.rb +++ b/test/unit/sample_controlled_vocab_test.rb @@ -55,6 +55,10 @@ class SampleControlledVocabTest < ActiveSupport::TestCase vocab.ols_root_term_uris = 'http://purl.obolibrary.org/obo/GO_0090395, ' assert vocab.valid? assert_equal 'http://purl.obolibrary.org/obo/GO_0090395', vocab.ols_root_term_uris + + vocab.ols_root_term_uris = 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035, ' + assert vocab.valid? + assert_equal 'http://purl.obolibrary.org/obo/GO_0090395, http://purl.obolibrary.org/obo/GO_0085035', vocab.ols_root_term_uris end test 'validate unique key' do From 65c6e47de996568e2215d954066adcffdeac69e3 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Tue, 6 Feb 2024 14:19:53 +0000 Subject: [PATCH 39/49] handle multiple root uris when showing the CV #1690 --- app/helpers/samples_helper.rb | 8 +++++--- app/views/sample_controlled_vocabs/show.html.erb | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/helpers/samples_helper.rb b/app/helpers/samples_helper.rb index a0c7b0bd50..bd4bfd36dd 100644 --- a/app/helpers/samples_helper.rb +++ b/app/helpers/samples_helper.rb @@ -283,9 +283,11 @@ def ols_ontology_link(ols_id) link_to(link,link,target: :_blank) end - def ols_root_term_link(ols_id, term_uri) - ols_link = "#{Ebi::OlsClient::ROOT_URL}/ontologies/#{ols_id}/terms?iri=#{term_uri}" - link_to(term_uri, ols_link, target: :_blank) + def ols_root_term_link(ols_id, term_uris) + term_uris.split(',').collect(&:strip).collect do |uri| + ols_link = "#{Ebi::OlsClient::ROOT_URL}/ontologies/#{ols_id}/terms?iri=#{uri}" + link_to(uri, ols_link, target: :_blank) + end.join(', ').html_safe end def get_extra_info(sample) diff --git a/app/views/sample_controlled_vocabs/show.html.erb b/app/views/sample_controlled_vocabs/show.html.erb index 9b53dea7d9..9f8cca04ca 100644 --- a/app/views/sample_controlled_vocabs/show.html.erb +++ b/app/views/sample_controlled_vocabs/show.html.erb @@ -10,7 +10,7 @@ <%= ols_ontology_link(@sample_controlled_vocab.source_ontology) %>

- Root URI: + Root URIs: <%= ols_root_term_link(@sample_controlled_vocab.source_ontology,@sample_controlled_vocab.ols_root_term_uris) %>

<% end %> From fab6eb2dca4441e7b723c74ba75578dc65ed606d Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Tue, 6 Feb 2024 14:45:13 +0000 Subject: [PATCH 40/49] fix javascript for deleting new terms #1690 relied on a class being set of .success, which if set allowed it to be immediately removed rather than being marked for removal. Fixes a bug introduced with #1691 --- .../_term_form_row_disabled.html.erb | 10 +++++++++- .../fetch_ols_terms_html.html.erb | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/views/sample_controlled_vocabs/_term_form_row_disabled.html.erb b/app/views/sample_controlled_vocabs/_term_form_row_disabled.html.erb index ef7bf9fd7d..2be8514245 100644 --- a/app/views/sample_controlled_vocabs/_term_form_row_disabled.html.erb +++ b/app/views/sample_controlled_vocabs/_term_form_row_disabled.html.erb @@ -1,4 +1,12 @@ - +<% + new_term ||= false + row_class = 'sample-cv-term' + # will be marked in green, and also the javascript will immediately remove rather than mark for removal + if new_term + row_class+=' success' + end +%> + <%= hidden_field_tag "sample_controlled_vocab[sample_controlled_vocab_terms_attributes][#{index}][label]", term.label %>
<%= term.label %>
diff --git a/app/views/sample_controlled_vocabs/fetch_ols_terms_html.html.erb b/app/views/sample_controlled_vocabs/fetch_ols_terms_html.html.erb index d070cfc3db..ab892a4fa3 100644 --- a/app/views/sample_controlled_vocabs/fetch_ols_terms_html.html.erb +++ b/app/views/sample_controlled_vocabs/fetch_ols_terms_html.html.erb @@ -1,5 +1,5 @@ <% @terms.each_with_index do |term, index| %> - <%= render partial: 'sample_controlled_vocabs/term_form_row_disabled', locals: { index: index, term: OpenStruct.new(term)} %> + <%= render partial: 'sample_controlled_vocabs/term_form_row_disabled', locals: { index: index, term: OpenStruct.new(term), new_term: true} %> <% end %> \ No newline at end of file From 5a3c22af861eff76ae1af6048d0c861d884bcd91 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Wed, 7 Feb 2024 13:55:35 +0000 Subject: [PATCH 41/49] Use instance name in large file warning (for git file). Fixes #1733 --- app/views/git/_add_file_form.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/git/_add_file_form.html.erb b/app/views/git/_add_file_form.html.erb index e56ae42f1b..cc689a942c 100644 --- a/app/views/git/_add_file_form.html.erb +++ b/app/views/git/_add_file_form.html.erb @@ -39,7 +39,7 @@ From 2d64a261509202bf45b453c41613a3bf0dc42935 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Wed, 7 Feb 2024 17:09:39 +0000 Subject: [PATCH 42/49] Fix managing git files with `.json` extension responding with JSON in browser. Fixes #1732 --- app/controllers/git_controller.rb | 14 ++++++- test/integration/git_format_test.rb | 57 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 test/integration/git_format_test.rb diff --git a/app/controllers/git_controller.rb b/app/controllers/git_controller.rb index 9a283e78af..05e86cca36 100644 --- a/app/controllers/git_controller.rb +++ b/app/controllers/git_controller.rb @@ -233,8 +233,18 @@ def coerce_format # However this results in an UnknownFormat error when trying to load the HTML view, as Rails still seems to be # looking for an e.g. application/yaml view. # You can fix this by adding { defaults: { format: :html } }, but then it is not possible to request JSON, - # even with an explicit `Accept: application/json` header! -Finn - request.format = :html unless json_api_request? + # even with an explicit `Accept: application/json` header! + # + # GitLab uses a monkeypatch to avoid this: + # https://gitlab.com/gitlab-org/gitlab/-/blob/7a0c278e/config/initializers/action_dispatch_http_mime_negotiation.rb + # but not sure what the wider consequences of that are. + # -Finn + accept_format = request.accepts.first + if accept_format.nil? || accept_format.ref == '*/*' + request.format = :html + else + request.format = accept_format.symbol + end end def file_content diff --git a/test/integration/git_format_test.rb b/test/integration/git_format_test.rb new file mode 100644 index 0000000000..60cdadb15a --- /dev/null +++ b/test/integration/git_format_test.rb @@ -0,0 +1,57 @@ +require 'test_helper' + +class GitFormatTest < ActionDispatch::IntegrationTest + test 'gets appropriate response format from git controller' do + workflow = FactoryBot.create(:local_git_workflow, policy: FactoryBot.create(:public_policy)) + git_version = workflow.git_version + disable_authorization_checks do + git_version.add_file('dmp.json', open_fixture_file('dmp.json')) + git_version.add_file('file', open_fixture_file('file')) + git_version.add_file('html_file.html', open_fixture_file('html_file.html')) + git_version.save! + end + + ['dmp.json', 'file', 'html_file.html', 'diagram.png'].each do |path| + # Default to HTML response + get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => '*/*' } + assert_response :success + assert_equal 'text/html; charset=utf-8', response.headers['Content-Type'] + assert response.body.start_with?('') + + # Even without any Accept + get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => '' } + assert_response :success + assert_equal 'text/html; charset=utf-8', response.headers['Content-Type'] + assert response.body.start_with?('') + + # Default headers + get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => 'text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5'} + assert_response :success + assert_equal 'text/html; charset=utf-8', response.headers['Content-Type'] + assert response.body.start_with?('') + + # Explicit HTML + get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => 'text/html' } + assert_response :success + assert_equal 'text/html; charset=utf-8', response.headers['Content-Type'] + assert response.body.start_with?('') + + # Permit JSON response + get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => 'application/json' } + assert_response :success + assert_equal 'application/vnd.api+json; charset=utf-8', response.headers['Content-Type'] + assert_equal path, JSON.parse(response.body)['path'] + + # Provide JSON response if Accept prefers + get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => 'application/json;q=0.9,text/html;q=0.8,*/*;q=0.5' } + assert_response :success + assert_equal 'application/vnd.api+json; charset=utf-8', response.headers['Content-Type'] + assert_equal path, JSON.parse(response.body)['path'] + + # Otherwise unrecognized format + assert_raises(ActionController::UnknownFormat) do + get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => 'application/zip' } + end + end + end +end \ No newline at end of file From 85c26d7ec87faeca0963b89d82001e93535422b6 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 8 Feb 2024 09:59:56 +0000 Subject: [PATCH 43/49] Monkeypatch MimeNegotiation to ignore format from extension --- app/controllers/application_controller.rb | 5 +++ app/controllers/git_controller.rb | 33 +++++-------------- .../action_dispatch_http_mime_negotiation.rb | 23 +++++++++++++ test/integration/git_format_test.rb | 6 ---- 4 files changed, 36 insertions(+), 31 deletions(-) create mode 100644 config/initializers/action_dispatch_http_mime_negotiation.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b9c458fc5c..ee63880624 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -677,4 +677,9 @@ def safe_class_lookup(class_name, raise: true) end helper_method :safe_class_lookup + + # See: config/initializers/action_dispatch_http_mime_negotiation.rb + def self.ignore_format_from_extension + false + end end diff --git a/app/controllers/git_controller.rb b/app/controllers/git_controller.rb index 05e86cca36..bbcce46bbd 100644 --- a/app/controllers/git_controller.rb +++ b/app/controllers/git_controller.rb @@ -8,7 +8,6 @@ class GitController < ApplicationController before_action :fetch_git_version before_action :get_tree, only: [:tree] before_action :get_blob, only: [:blob, :download, :raw] - before_action :coerce_format user_content_actions :raw @@ -17,6 +16,11 @@ class GitController < ApplicationController rescue_from Git::InvalidPathException, with: :render_invalid_path_error rescue_from URI::InvalidURIError, with: :render_invalid_url_error + # See: config/initializers/action_dispatch_http_mime_negotiation.rb + def self.ignore_format_from_extension + true + end + def browse respond_to do |format| format.html @@ -25,12 +29,12 @@ def browse def tree respond_to do |format| - format.json { render json: @tree, adapter: :attributes, root: '' } if request.xhr? format.html { render partial: 'tree' } else format.html end + format.json { render json: @tree, adapter: :attributes, root: '' } end end @@ -40,12 +44,12 @@ def download def blob respond_to do |format| - format.json { render json: @blob, adapter: :attributes, root: '' } if request.xhr? format.html { render partial: 'blob' } else format.html end + format.json { render json: @blob, adapter: :attributes, root: '' } end end @@ -118,7 +122,6 @@ def update def operation_response(notice = nil, status: 200) respond_to do |format| - format.json { render json: { }, status: status, adapter: :attributes, root: '' } format.html do if request.xhr? render partial: 'files', locals: { resource: @parent_resource, git_version: @git_version }, status: status @@ -127,6 +130,7 @@ def operation_response(notice = nil, status: 200) redirect_to polymorphic_path(@parent_resource, tab: 'files') end end + format.json { render json: { }, status: status, adapter: :attributes, root: '' } end end @@ -226,27 +230,6 @@ def add_remote_file @git_version.save! end - def coerce_format - # I have to do this because Rails doesn't seem to be behaving as expected. - # In routes.rb, the git routes are scoped with "format: false", so Rails should disregard the extension - # (e.g. /git/1/blob/my_file.yml) when determining the response format. - # However this results in an UnknownFormat error when trying to load the HTML view, as Rails still seems to be - # looking for an e.g. application/yaml view. - # You can fix this by adding { defaults: { format: :html } }, but then it is not possible to request JSON, - # even with an explicit `Accept: application/json` header! - # - # GitLab uses a monkeypatch to avoid this: - # https://gitlab.com/gitlab-org/gitlab/-/blob/7a0c278e/config/initializers/action_dispatch_http_mime_negotiation.rb - # but not sure what the wider consequences of that are. - # -Finn - accept_format = request.accepts.first - if accept_format.nil? || accept_format.ref == '*/*' - request.format = :html - else - request.format = accept_format.symbol - end - end - def file_content file_params.key?(:content) ? StringIO.new(Base64.decode64(file_params[:content])) : file_params[:data] end diff --git a/config/initializers/action_dispatch_http_mime_negotiation.rb b/config/initializers/action_dispatch_http_mime_negotiation.rb new file mode 100644 index 0000000000..30d12c9252 --- /dev/null +++ b/config/initializers/action_dispatch_http_mime_negotiation.rb @@ -0,0 +1,23 @@ +# Monkeypatch because Rails doesn't seem to be behaving as expected. +# In routes.rb, the git routes are scoped with "format: false", so Rails should disregard the extension +# (e.g. /git/1/blob/my_file.yml) when determining the response format. +# However this results in an UnknownFormat error when trying to load the HTML view, as Rails still seems to be +# looking for an e.g. application/yaml view. +# You can fix this by adding { defaults: { format: :html } }, but then it is not possible to request JSON, +# even with an explicit `Accept: application/json` header! +# +# Inspired by GitLab's change: +# https://gitlab.com/gitlab-org/gitlab/-/blob/7a0c278e/config/initializers/action_dispatch_http_mime_negotiation.rb +# -Finn + +module ActionDispatch + module Http + module MimeNegotiation + alias original_format_from_path_extension format_from_path_extension + + def format_from_path_extension + controller_class&.ignore_format_from_extension ? nil : original_format_from_path_extension + end + end + end +end diff --git a/test/integration/git_format_test.rb b/test/integration/git_format_test.rb index 60cdadb15a..9d892f8061 100644 --- a/test/integration/git_format_test.rb +++ b/test/integration/git_format_test.rb @@ -42,12 +42,6 @@ class GitFormatTest < ActionDispatch::IntegrationTest assert_equal 'application/vnd.api+json; charset=utf-8', response.headers['Content-Type'] assert_equal path, JSON.parse(response.body)['path'] - # Provide JSON response if Accept prefers - get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => 'application/json;q=0.9,text/html;q=0.8,*/*;q=0.5' } - assert_response :success - assert_equal 'application/vnd.api+json; charset=utf-8', response.headers['Content-Type'] - assert_equal path, JSON.parse(response.body)['path'] - # Otherwise unrecognized format assert_raises(ActionController::UnknownFormat) do get "/workflows/#{workflow.id}/git/1/blob/#{path}", headers: { 'Accept' => 'application/zip' } From 46bdb1653f64aaca05ab65dbf45643f3c8307114 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 8 Feb 2024 10:05:49 +0000 Subject: [PATCH 44/49] Patch `ignore_format_from_extension` into `ActionController::Base` In case we ever use any controllers from engines etc. that don't inherit from `ApplicationController` --- app/controllers/application_controller.rb | 5 ----- .../initializers/action_dispatch_http_mime_negotiation.rb | 8 ++++++++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ee63880624..b9c458fc5c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -677,9 +677,4 @@ def safe_class_lookup(class_name, raise: true) end helper_method :safe_class_lookup - - # See: config/initializers/action_dispatch_http_mime_negotiation.rb - def self.ignore_format_from_extension - false - end end diff --git a/config/initializers/action_dispatch_http_mime_negotiation.rb b/config/initializers/action_dispatch_http_mime_negotiation.rb index 30d12c9252..d37c457204 100644 --- a/config/initializers/action_dispatch_http_mime_negotiation.rb +++ b/config/initializers/action_dispatch_http_mime_negotiation.rb @@ -21,3 +21,11 @@ def format_from_path_extension end end end + +module ActionController + class Base + def self.ignore_format_from_extension + false + end + end +end From ad1d8f8dffb5ec2d0629faae095111114b01a214 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Wed, 7 Feb 2024 11:16:41 +0000 Subject: [PATCH 45/49] update docker compose to use mysql 8.0 #1743 tested updating an existing database, and also starting with a fresh install --- docker-compose-relative-root.yml | 4 ++-- docker-compose-virtuoso.yml | 4 ++-- docker-compose-with-email.yml | 4 ++-- docker-compose.yml | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docker-compose-relative-root.yml b/docker-compose-relative-root.yml index 79ecc5d6fb..c9f214cbbf 100644 --- a/docker-compose-relative-root.yml +++ b/docker-compose-relative-root.yml @@ -1,9 +1,9 @@ version: '3' services: db: # Database implementation, in this case MySQL - image: mysql:5.7 + image: mysql:8.0 container_name: seek-mysql - command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci + command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci --log-error-verbosity=1 restart: always stop_grace_period: 1m30s env_file: diff --git a/docker-compose-virtuoso.yml b/docker-compose-virtuoso.yml index 339571384f..626485b5a5 100644 --- a/docker-compose-virtuoso.yml +++ b/docker-compose-virtuoso.yml @@ -1,9 +1,9 @@ version: '3' services: db: # Database implementation, in this case MySQL - image: mysql:5.7 + image: mysql:8.0 container_name: seek-mysql - command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci + command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci --log-error-verbosity=1 restart: always env_file: - docker/db.env diff --git a/docker-compose-with-email.yml b/docker-compose-with-email.yml index 2da2c1831f..19beb35095 100644 --- a/docker-compose-with-email.yml +++ b/docker-compose-with-email.yml @@ -1,9 +1,9 @@ version: '3' services: db: # Database implementation, in this case MySQL - image: mysql:5.7 + image: mysql:8.0 container_name: seek-mysql - command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci + command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci --log-error-verbosity=1 restart: always stop_grace_period: 1m30s env_file: diff --git a/docker-compose.yml b/docker-compose.yml index caee6980d5..e784ad4286 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,9 +1,9 @@ version: '3' services: db: # Database implementation, in this case MySQL - image: mysql:5.7 + image: mysql:8.0 container_name: seek-mysql - command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci + command: --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci --log-error-verbosity=1 restart: always stop_grace_period: 1m30s env_file: From a21c3df626f34c24a6cb6ba8b4ef991cd293d91b Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Mon, 5 Feb 2024 15:29:37 +0000 Subject: [PATCH 46/49] updates to the file view icon (magnifier) to also link to explore for excel, csv and tsv files #1711 --- app/views/assets/_view_content.html.erb | 10 +++++++++- test/functional/data_files_controller_test.rb | 19 ++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/app/views/assets/_view_content.html.erb b/app/views/assets/_view_content.html.erb index ca4b1574e7..a6c807019e 100644 --- a/app/views/assets/_view_content.html.erb +++ b/app/views/assets/_view_content.html.erb @@ -2,9 +2,10 @@ <% options = {} %> <% url = nil %> <% asset = content_blob.asset %> +<% asset_version = content_blob.asset_version %> <% if Seek::Util.inline_viewable_content_types.include?(asset.class) %> - <% if content_blob.is_content_viewable? %> + <% if content_blob.is_content_viewable? && !content_blob.is_supported_spreadsheet_format? %> <%# Use fancy lightbox thing for displaying images in page %> <% if content_blob.is_image? %> <% zoom_width = content_blob.width.to_i.clamp(Seek::ActsAsFleximageExtension::STANDARD_SIZE, @@ -17,6 +18,13 @@ <% url = polymorphic_path([asset, content_blob], action: 'view_content', code: params[:code]) %> <% options = { title: 'View contents of this file' } %> <% end %> + <% elsif !asset.is_a?(Model) && content_blob.is_supported_spreadsheet_format? %> + <% if content_blob.is_extractable_spreadsheet? %> + <% url = polymorphic_path([asset], action: 'explore', code: params[:code], version: asset_version) %> + <% options = { title: 'Explore the contents of this file' } %> + <% else %> + <% options = { disabled_reason: "This spreadsheet is too big to be explored (larger than #{Seek::Config.max_extractable_spreadsheet_size} MB), or the file does not exist." } %> + <% end %> <% elsif !asset.is_a?(Model) && asset.is_downloadable_asset? %> <% if content_blob.file_exists? %> <% options[:disabled_reason] = "This #{text_for_resource(asset).downcase} is unable to be viewed in-browser. " + diff --git a/test/functional/data_files_controller_test.rb b/test/functional/data_files_controller_test.rb index ceec265aec..14d319c299 100644 --- a/test/functional/data_files_controller_test.rb +++ b/test/functional/data_files_controller_test.rb @@ -608,7 +608,7 @@ def test_title end end - test 'show explore button' do + test 'show explore button and magnify icon' do df = FactoryBot.create(:small_test_spreadsheet_datafile) login_as(df.contributor.user) get :show, params: { id: df } @@ -617,9 +617,12 @@ def test_title assert_select 'a[href=?]', explore_data_file_path(df, version: df.version), count: 1 assert_select 'a.disabled', text: 'Explore', count: 0 end + assert_select 'div.fileinfo span.filename' do + assert_select 'a[href=?][title=?]', explore_data_file_path(df, version: df.version), 'Explore the contents of this file', count: 1 + end end - test 'show explore button for csv file' do + test 'show explore button and magnify icon for csv file' do df = FactoryBot.create(:csv_spreadsheet_datafile) login_as(df.contributor.user) get :show, params: { id: df } @@ -628,6 +631,9 @@ def test_title assert_select 'a[href=?]', explore_data_file_path(df, version: df.version), count: 1 assert_select 'a.disabled', text: 'Explore', count: 0 end + assert_select 'div.fileinfo span.filename' do + assert_select 'a[href=?][title=?]', explore_data_file_path(df, version: df.version), 'Explore the contents of this file', count: 1 + end end @@ -642,9 +648,12 @@ def test_title assert_select 'a[href=?]', explore_data_file_path(df, version: df.version), count: 0 assert_select 'a', text: 'Explore', count: 0 end + assert_select 'div.fileinfo span.filename' do + assert_select 'a[href=?][title=?]', explore_data_file_path(df, version: df.version), 'Explore the contents of this file', count: 0 + end end - test 'show disabled explore button if spreadsheet too big' do + test 'show disabled explore button and magnify icon if spreadsheet too big' do df = FactoryBot.create(:small_test_spreadsheet_datafile) login_as(df.contributor.user) with_config_value(:max_extractable_spreadsheet_size, 0) do @@ -655,6 +664,10 @@ def test_title assert_select 'a[href=?]', explore_data_file_path(df, version: df.version), count: 0 assert_select 'a.disabled', text: 'Explore', count: 1 end + assert_select 'div.fileinfo span.filename' do + assert_select 'a[href=?][title=?]', explore_data_file_path(df, version: df.version), 'Explore the contents of this file', count: 0 + assert_select 'span > a.disabled > img', count: 1 + end end test 'should download datafile from standard route' do From ae505c9410d77793ffa34f9f9a0643c8a0a9b276 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Thu, 8 Feb 2024 15:40:20 +0000 Subject: [PATCH 47/49] allow sample_controlled_vocab update if can_edit? #1693 --- .../sample_controlled_vocabs_controller.rb | 2 +- ...ample_controlled_vocabs_controller_test.rb | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/app/controllers/sample_controlled_vocabs_controller.rb b/app/controllers/sample_controlled_vocabs_controller.rb index a06b2afce9..c1dc43e426 100644 --- a/app/controllers/sample_controlled_vocabs_controller.rb +++ b/app/controllers/sample_controlled_vocabs_controller.rb @@ -6,7 +6,7 @@ class SampleControlledVocabsController < ApplicationController before_action :samples_enabled?, except: :typeahead before_action :login_required, except: %i[show index] - before_action :is_user_admin_auth, only: %i[destroy update] + before_action :is_user_admin_auth, only: %i[destroy] before_action :find_and_authorize_requested_item, except: %i[index new create] before_action :find_assets, only: :index before_action :auth_to_create, only: %i[new create] diff --git a/test/functional/sample_controlled_vocabs_controller_test.rb b/test/functional/sample_controlled_vocabs_controller_test.rb index 85a9f08650..1777de0b92 100644 --- a/test/functional/sample_controlled_vocabs_controller_test.rb +++ b/test/functional/sample_controlled_vocabs_controller_test.rb @@ -173,6 +173,42 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase assert_response :redirect end + test 'can_edit permission required to edit' do + login_as(FactoryBot.create(:person)) + cv = FactoryBot.create(:apples_sample_controlled_vocab) + assert cv.can_edit? + + get :edit, params: { id: cv.id } + assert_response :success + + cv2 = FactoryBot.create(:topics_controlled_vocab) + refute cv2.can_edit? + + get :edit, params: { id: cv2.id } + assert_response :redirect + end + + test 'can_edit permission required to update' do + login_as(FactoryBot.create(:person)) + cv_bad = FactoryBot.create(:topics_controlled_vocab) + refute cv_bad.can_edit? + + cv_good = FactoryBot.create(:apples_sample_controlled_vocab) + assert cv_good.can_edit? + + put :update, params: { id: cv_good, sample_controlled_vocab: { title: 'updated title' } } + assert_redirected_to sample_controlled_vocab_path(cv_good) + refute flash[:error] + assert_equal 'updated title',assigns(:sample_controlled_vocab).title + assert_equal 'updated title',cv_good.reload.title + + put :update, params: { id: cv_bad, sample_controlled_vocab: { title: 'updated title' } } + assert_redirected_to sample_controlled_vocab_path(cv_bad) + assert flash[:error] + refute_equal 'updated title',cv_bad.reload.title + + end + test 'index' do cv = FactoryBot.create(:apples_sample_controlled_vocab) get :index From eeb94b24d681aba51149f1cff2785a049e5d7be1 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Thu, 8 Feb 2024 16:01:48 +0000 Subject: [PATCH 48/49] also tie sample_controlled_vocab destroy to can_delete? #1693 --- .../sample_controlled_vocabs_controller.rb | 1 - ...ample_controlled_vocabs_controller_test.rb | 30 +++++++++++++++---- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/app/controllers/sample_controlled_vocabs_controller.rb b/app/controllers/sample_controlled_vocabs_controller.rb index c1dc43e426..2cc2664038 100644 --- a/app/controllers/sample_controlled_vocabs_controller.rb +++ b/app/controllers/sample_controlled_vocabs_controller.rb @@ -6,7 +6,6 @@ class SampleControlledVocabsController < ApplicationController before_action :samples_enabled?, except: :typeahead before_action :login_required, except: %i[show index] - before_action :is_user_admin_auth, only: %i[destroy] before_action :find_and_authorize_requested_item, except: %i[index new create] before_action :find_assets, only: :index before_action :auth_to_create, only: %i[new create] diff --git a/test/functional/sample_controlled_vocabs_controller_test.rb b/test/functional/sample_controlled_vocabs_controller_test.rb index 1777de0b92..f8a450e584 100644 --- a/test/functional/sample_controlled_vocabs_controller_test.rb +++ b/test/functional/sample_controlled_vocabs_controller_test.rb @@ -181,6 +181,7 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase get :edit, params: { id: cv.id } assert_response :success + # a system vocab cannot be edited or deleted cv2 = FactoryBot.create(:topics_controlled_vocab) refute cv2.can_edit? @@ -190,6 +191,8 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase test 'can_edit permission required to update' do login_as(FactoryBot.create(:person)) + + # a system vocab cannot be edited or deleted cv_bad = FactoryBot.create(:topics_controlled_vocab) refute cv_bad.can_edit? @@ -235,15 +238,32 @@ class SampleControlledVocabsControllerTest < ActionController::TestCase assert_response :redirect end - test 'need to be project member to destroy' do - login_as(FactoryBot.create(:user)) - cv = FactoryBot.create(:apples_sample_controlled_vocab) + test 'can_delete permission required to destroy' do + login_as(FactoryBot.create(:person)) + + # a system vocab cannot be edited or deleted + cv_bad = FactoryBot.create(:topics_controlled_vocab) + refute cv_bad.can_delete? + + cv_good = FactoryBot.create(:apples_sample_controlled_vocab) + assert cv_good.can_delete? + + assert_difference('SampleControlledVocab.count', -1) do + assert_difference('SampleControlledVocabTerm.count', -4) do + delete :destroy, params: { id: cv_good } + end + end + assert_redirected_to sample_controlled_vocabs_path + refute flash[:error] + assert_no_difference('SampleControlledVocab.count') do assert_no_difference('SampleControlledVocabTerm.count') do - delete :destroy, params: { id: cv } + delete :destroy, params: { id: cv_bad } end end - assert_response :redirect + assert_redirected_to sample_controlled_vocab_path(cv_bad) + assert flash[:error] + end test 'cannot access when disabled' do From f3647e4d56b9d2b4ab1c41ad5116c39f279a0209 Mon Sep 17 00:00:00 2001 From: Stuart Owen Date: Fri, 9 Feb 2024 17:00:30 +0000 Subject: [PATCH 49/49] fix integration api tests #1693 --- test/integration/api/sample_controlled_vocab_api_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/integration/api/sample_controlled_vocab_api_test.rb b/test/integration/api/sample_controlled_vocab_api_test.rb index e8def6b2d2..56827da107 100644 --- a/test/integration/api/sample_controlled_vocab_api_test.rb +++ b/test/integration/api/sample_controlled_vocab_api_test.rb @@ -17,4 +17,12 @@ def setup @sample_controlled_vocab.sample_controlled_vocab_terms << @sample_controlled_vocab_term @sample_controlled_vocab.save! end + + def private_resource + # setting the key to a known key will make it a system vocab which isn't editable or deletable + @sample_controlled_vocab.update_column(:key, SampleControlledVocab::SystemVocabs.database_key_for_property(:topics)) + refute @sample_controlled_vocab.can_edit? + refute @sample_controlled_vocab.can_delete? + @sample_controlled_vocab + end end