From a07c4e6f87bc130ab76fe737cae891adb20662af Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 3 Oct 2024 13:32:25 +0100 Subject: [PATCH 1/2] Allow setter for external resources to remove redundant ones #1029 --- app/models/concerns/has_external_resources.rb | 16 +++++ test/models/material_test.rb | 66 +++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/app/models/concerns/has_external_resources.rb b/app/models/concerns/has_external_resources.rb index b5943a8a5..15dc602cf 100644 --- a/app/models/concerns/has_external_resources.rb +++ b/app/models/concerns/has_external_resources.rb @@ -33,4 +33,20 @@ def remove_duplicate_external_resources resources = external_resources.to_a.sort_by { |x| x.created_at || 1.year.from_now } (resources - resources.uniq { |r| [r.url, r.title] }).each(&:mark_for_destruction) end + + # Allow setting of external resources from an array of hashes/params. + # Unlike `external_resources_attributes=`, this will clear any redundant external resources, and will preserve any + # existing external resources with the same title & URL. + def external_resources=(ers) + existing = external_resources + objs = ers.map do |er| + if er.is_a?(ExternalResource) + er + else + existing.detect { |ex| ex.url == er[:url] && ex.title == er[:title] } || external_resources.build(er) + end + end + + super(objs) + end end diff --git a/test/models/material_test.rb b/test/models/material_test.rb index 33b9882a3..383667929 100644 --- a/test/models/material_test.rb +++ b/test/models/material_test.rb @@ -391,6 +391,72 @@ class MaterialTest < ActiveSupport::TestCase assert_not_includes material.external_resources, new_resource, 'Should preserve oldest external resource' end + test 'should set external resources from params' do + material = materials(:good_material) + assert_empty material.external_resources + + assert_difference('ExternalResource.count', 2) do + material.update!(external_resources: [{ title: 'Cool Website!', url: 'https://tess.elixir-uk.org/' }, + { title: 'Cooler Website!', url: 'https://tess.elixir-europe.org/' }]) + end + + er = material.reload.external_resources + assert_equal 2, er.count + assert_equal 'Cool Website!', er[0].title + assert_equal 'https://tess.elixir-uk.org/', er[0].url + assert_equal 'Cooler Website!', er[1].title + assert_equal 'https://tess.elixir-europe.org/', er[1].url + end + + test 'should remove redundant external resources and preserve IDs of retained ones' do + material = materials(:material_with_external_resource) + original_resources = material.external_resources.to_a + assert_equal 3, original_resources.length + + # [ + # { url: "https://tess.elixir-uk.org/", title: "TeSS" }, + # { url: "https://bio.tools/tool/SR-Tesseler", title: "SR-Tesseler" }, + # { url: "https://fairsharing.org/bsg-p123456", title: "Share Fairing" } + # ] + + assert_difference('ExternalResource.count', -1) do + material.update!(external_resources: [{ title: 'TeSS', url: 'https://tess.elixir-uk.org/' }, + { title: 'Changed title', url: 'https://bio.tools/tool/SR-Tesseler' }]) + end + + er = material.reload.external_resources + assert_equal 2, er.count + assert_equal 'TeSS', er[0].title + assert_equal 'https://tess.elixir-uk.org/', er[0].url + assert_equal original_resources[0].id, er[0].id, 'Should have preserved original ExternalResource' + assert_equal 'Changed title', er[1].title + assert_equal 'https://bio.tools/tool/SR-Tesseler', er[1].url + assert_not_equal original_resources[1].id, er[1].id, 'Should have replaced modified ExternalResource' + end + + test 'can set external resources using objects or params' do + material = materials(:material_with_external_resource) + original_resources = material.external_resources.to_a + assert_equal 3, original_resources.length + + assert_no_difference('ExternalResource.count') do + material.update!(external_resources: original_resources.first(2) + + [{ title: 'Zombocom', url: 'https://zombo.com' }]) + end + + er = material.reload.external_resources + assert_equal 3, er.count + assert_equal 'TeSS', er[0].title + assert_equal 'https://tess.elixir-uk.org/', er[0].url + assert_equal original_resources[0].id, er[0].id, 'Should have preserved first ExternalResource' + assert_equal 'SR-Tesseler', er[1].title + assert_equal 'https://bio.tools/tool/SR-Tesseler', er[1].url + assert_equal original_resources[1].id, er[1].id, 'Should have preserved second ExternalResource' + assert_equal 'Zombocom', er[2].title + assert_equal 'https://zombo.com', er[2].url + assert_not_equal original_resources[2].id, er[2].id, 'Should have replaced third ExternalResource' + end + test 'verified users scope' do bad_user = users(:unverified_user) bad_material = bad_user.materials.build(title: 'bla', url: 'http://example.com/spam', description: 'vvv', From eff9fcb75bc2382cf02796725cc143160d68aa1d Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Thu, 10 Oct 2024 10:46:11 +0100 Subject: [PATCH 2/2] Allow `external_resources` to be passed as params. Bump RDF parsing gem --- Gemfile.lock | 43 +++++++++++++------------ app/controllers/events_controller.rb | 3 +- app/controllers/materials_controller.rb | 1 + test/workers/source_test_worker_test.rb | 2 +- 4 files changed, 26 insertions(+), 23 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index b47932fef..75b8cf9e3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,6 +1,6 @@ GIT remote: https://github.com/ElixirTeSS/TeSS_RDF_Extractors - revision: f2e44f0eeeb299f0bc10f725496b39d6246938f0 + revision: c89fcad2a46ead316af14b270d26c6ba924646b6 branch: master specs: tess_rdf_extractors (1.1.0) @@ -115,8 +115,8 @@ GEM i18n (>= 1.6, < 2) minitest (>= 5.1) tzinfo (~> 2.0) - addressable (2.8.5) - public_suffix (>= 2.0.2, < 6.0) + addressable (2.8.7) + public_suffix (>= 2.0.2, < 7.0) aes_key_wrap (1.1.0) ahoy_matey (4.2.1) activesupport (>= 5.2) @@ -136,6 +136,7 @@ GEM erubi (>= 1.0.0) rack (>= 0.9.0) rouge (>= 1.0.0) + bigdecimal (3.1.8) bindata (2.5.0) bindex (0.8.1) binding_of_caller (1.0.0) @@ -162,7 +163,7 @@ GEM json_schema (~> 0.14, >= 0.14.3) openapi_parser (~> 1.0) rack (>= 1.5) - concurrent-ruby (1.3.3) + concurrent-ruby (1.3.4) connection_pool (2.4.1) countries (5.6.0) unaccent (~> 0.3) @@ -215,7 +216,7 @@ GEM globalid (1.2.1) activesupport (>= 6.1) gravtastic (3.2.6) - haml (6.2.3) + haml (6.3.0) temple (>= 0.8.2) thor tilt @@ -264,13 +265,14 @@ GEM bindata faraday (~> 2.0) faraday-follow_redirects - json-ld (3.3.1) + json-ld (3.3.2) htmlentities (~> 4.3) json-canonicalization (~> 1.0) link_header (~> 0.0, >= 0.0.8) multi_json (~> 1.15) rack (>= 2.2, < 4) rdf (~> 3.3) + rexml (~> 3.2) json-ld-preloaded (3.3.0) json-ld (~> 3.3) rdf (~> 3.3) @@ -333,7 +335,7 @@ GEM listen (3.8.0) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) - logger (1.6.0) + logger (1.6.1) loofah (2.22.0) crass (~> 1.0.2) nokogiri (>= 1.12.0) @@ -370,7 +372,7 @@ GEM multi_xml (0.6.0) multipart-post (2.4.0) nested_form (0.3.2) - net-http-persistent (4.0.2) + net-http-persistent (4.0.4) connection_pool (~> 2.2) net-imap (0.4.14) date @@ -383,7 +385,7 @@ GEM net-protocol netrc (0.11.0) nio4r (2.7.3) - nokogiri (1.16.6) + nokogiri (1.16.7) mini_portile2 (~> 2.8.2) racc (~> 1.4) numerizer (0.1.1) @@ -425,19 +427,19 @@ GEM pry-byebug (3.10.1) byebug (~> 11.0) pry (>= 0.13, < 0.15) - psych (5.1.1.1) + psych (5.1.2) stringio public_activity (2.0.2) actionpack (>= 5.0.0) activerecord (>= 5.0) i18n (>= 0.5.0) railties (>= 5.0.0) - public_suffix (5.0.4) + public_suffix (6.0.1) puma (6.4.3) nio4r (~> 2.0) pundit (2.3.1) activesupport (>= 3.0.0) - racc (1.8.0) + racc (1.8.1) rack (2.2.9) rack-cors (2.0.2) rack (>= 2.0.0) @@ -498,8 +500,9 @@ GEM rb-fsevent (0.11.2) rb-inotify (0.10.1) ffi (~> 1.0) - rdf (3.3.1) + rdf (3.3.2) bcp47_spec (~> 0.2) + bigdecimal (~> 3.1, >= 3.1.5) link_header (~> 0.0, >= 0.0.8) rdf-aggregate-repo (3.3.0) rdf (~> 3.3) @@ -557,7 +560,7 @@ GEM rdf-turtle (3.3.0) ebnf (~> 2.4) rdf (~> 3.3) - rdf-vocab (3.3.0) + rdf-vocab (3.3.1) rdf (~> 3.3) rdf-xsd (3.3.0) rdf (~> 3.3) @@ -579,8 +582,7 @@ GEM netrc (~> 0.8) reverse_markdown (2.1.1) nokogiri - rexml (3.3.6) - strscan + rexml (3.3.8) rouge (4.1.3) rsolr (2.5.0) builder (>= 2.1.2) @@ -697,8 +699,7 @@ GEM actionpack (>= 5.2) activesupport (>= 5.2) sprockets (>= 3.0.0) - stringio (3.1.0) - strscan (3.1.0) + stringio (3.1.1) swd (2.0.2) activesupport (>= 3) attr_required (>= 0.0.5) @@ -712,8 +713,8 @@ GEM climate_control (>= 0.0.3, < 1.0) terser (1.1.17) execjs (>= 0.3.0, < 3) - thor (1.3.1) - tilt (2.3.0) + thor (1.3.2) + tilt (2.4.0) timeout (0.4.1) turbo-rails (1.5.0) actionpack (>= 6.0.0) @@ -733,7 +734,7 @@ GEM unf_ext unf_ext (0.0.8.2) unicode-display_width (2.4.2) - unicode-types (1.9.0) + unicode-types (1.10.0) validate_email (0.1.6) activemodel (>= 3.0) mail (>= 2.2.5) diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index dff5cc072..b92fb53e5 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -235,7 +235,8 @@ def event_params { node_names: [] }, { target_audience: [] }, { eligibility: [] }, :visible, { host_institutions: [] }, :capacity, :contact, :recognition, :learning_objectives, :prerequisites, :tech_requirements, :cost_basis, :cost_value, :cost_currency, :language, - external_resources_attributes: %i[id url title _destroy], material_ids: [], + external_resources_attributes: %i[id url title _destroy], + external_resources: [:url, :title], material_ids: [], llm_interaction_attributes: %i[id scrape_or_process model prompt input output needs_processing _destroy], locked_fields: []) end diff --git a/app/controllers/materials_controller.rb b/app/controllers/materials_controller.rb index 397958716..d83e2567c 100644 --- a/app/controllers/materials_controller.rb +++ b/app/controllers/materials_controller.rb @@ -174,6 +174,7 @@ def material_params { operation_names: [] }, { operation_uris: [] }, { node_ids: [] }, { node_names: [] }, { fields: [] }, external_resources_attributes: [:id, :url, :title, :_destroy], + external_resources: [:url, :title], event_ids: [], locked_fields: []) end diff --git a/test/workers/source_test_worker_test.rb b/test/workers/source_test_worker_test.rb index e2520d7bb..bba337594 100644 --- a/test/workers/source_test_worker_test.rb +++ b/test/workers/source_test_worker_test.rb @@ -105,7 +105,7 @@ class SourceTestWorkerTest < ActiveSupport::TestCase assert sample assert results[:run_time] > 0 assert results[:finished_at] > 1.day.ago - ext_res = sample[:external_resources_attributes] + ext_res = sample[:external_resources] assert_equal 1, ext_res.length res = ext_res.first assert res.is_a?(Hash)