From a11558cd5d1cd876588dba7b05e09257f32be098 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 20 May 2024 09:15:40 -0400 Subject: [PATCH 1/2] Download links target _blank and append UI marker Why are these changes being introduced: * We want to display a message in our restricted CDN auth flow that instructs a user to close the window/tab when the download is complete to return to their search if they came from TIMDEX UI. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-317 * https://mitlibraries.atlassian.net/browse/GDT-304 How does this address that need: * Adds `target=_blank` to download link tags * appends `?timdex-ui=true` to download link URLs Document any side effects to this change: * A bunch of tests reformatted automatically on save. The only relevant changes to the tests were on lines 246-247 --- app/helpers/record_helper.rb | 3 +- app/views/record/_access_buttons.html.erb | 2 +- test/helpers/record_helper_test.rb | 238 +++++++++++----------- 3 files changed, 122 insertions(+), 121 deletions(-) diff --git a/app/helpers/record_helper.rb b/app/helpers/record_helper.rb index 9618ffdb..ff254d27 100644 --- a/app/helpers/record_helper.rb +++ b/app/helpers/record_helper.rb @@ -86,7 +86,8 @@ def gis_access_link(metadata) if access_type(metadata) == 'unknown: check with owning institution' links.select { |link| link['kind'] == 'Website' }.first['url'] else - links.select { |link| link['kind'] == 'Download' && link['text'] == 'Data' }.first['url'] + url = links.select { |link| link['kind'] == 'Download' && link['text'] == 'Data' }.first['url'] + "#{url}?timdexui=true" end end diff --git a/app/views/record/_access_buttons.html.erb b/app/views/record/_access_buttons.html.erb index 8a021dff..06037439 100644 --- a/app/views/record/_access_buttons.html.erb +++ b/app/views/record/_access_buttons.html.erb @@ -4,7 +4,7 @@ <% if access_type(@record) == 'no authentication required' %> Download geodata files <% elsif access_type(@record) == 'MIT authentication required' %> - + Download geodata files MIT authentication <% else %> diff --git a/test/helpers/record_helper_test.rb b/test/helpers/record_helper_test.rb index 6ce92a28..e1ca3668 100644 --- a/test/helpers/record_helper_test.rb +++ b/test/helpers/record_helper_test.rb @@ -103,148 +103,148 @@ class RecordHelperTest < ActionView::TestCase end test 'access_type returns nil if corresponding right is blank' do - rights = { - 'rights' => [ - { - 'description' => 'foo', - 'kind' => 'bar' - } - ] - } + rights = { + 'rights' => [ + { + 'description' => 'foo', + 'kind' => 'bar' + } + ] + } assert_nil access_type(rights) end test 'access_type returns the description of the corresponding right' do rights = { - 'rights' => [ - { - 'description' => 'foo', - 'kind' => 'bar' - }, - { - 'description' => 'no authentication required', - 'kind' => 'Access to files' - } - ] - } + 'rights' => [ + { + 'description' => 'foo', + 'kind' => 'bar' + }, + { + 'description' => 'no authentication required', + 'kind' => 'Access to files' + } + ] + } assert_equal 'no authentication required', access_type(rights) end test 'gis_access_link returns nil if access type is blank' do links_no_rights = { - 'links' => [ - { - 'kind' => 'Download', - 'text' => 'Data', - 'url' => 'https://example.org/dz_f7regions_2016.zip' - }, - { - 'kind' => 'Website', - 'text' => 'Website', - 'url' => 'https://example.org/gismit:dz_f7regions_2016' - } - ] - } + 'links' => [ + { + 'kind' => 'Download', + 'text' => 'Data', + 'url' => 'https://example.org/dz_f7regions_2016.zip' + }, + { + 'kind' => 'Website', + 'text' => 'Website', + 'url' => 'https://example.org/gismit:dz_f7regions_2016' + } + ] + } assert_nil access_type(links_no_rights) assert_nil gis_access_link(links_no_rights) end test 'gis_access_link returns nil if links are blank' do rights_no_links = { - 'rights' => [ - { - 'description' => 'foo', - 'kind' => 'bar' - }, - { - 'description' => 'no authentication required', - 'kind' => 'Access to files' - } - ] - } + 'rights' => [ + { + 'description' => 'foo', + 'kind' => 'bar' + }, + { + 'description' => 'no authentication required', + 'kind' => 'Access to files' + } + ] + } assert_nil gis_access_link(rights_no_links) end test 'gis_access_link is website URL for non-MIT records' do access_elsewhere = { - 'rights' => [ - { - 'description' => 'foo', - 'kind' => 'bar' - }, - { - 'description' => 'unknown: check with owning institution', - 'kind' => 'Access to files' - } - ], - 'links' => [ - { - 'kind' => 'Download', - 'text' => 'Data', - 'url' => 'https://example.org/dz_f7regions_2016.zip' - }, - { - 'kind' => 'Website', - 'text' => 'Website', - 'url' => 'https://example.org/gismit:dz_f7regions_2016' - } - ], - 'provider' => 'Spelman' - } + 'rights' => [ + { + 'description' => 'foo', + 'kind' => 'bar' + }, + { + 'description' => 'unknown: check with owning institution', + 'kind' => 'Access to files' + } + ], + 'links' => [ + { + 'kind' => 'Download', + 'text' => 'Data', + 'url' => 'https://example.org/dz_f7regions_2016.zip' + }, + { + 'kind' => 'Website', + 'text' => 'Website', + 'url' => 'https://example.org/gismit:dz_f7regions_2016' + } + ], + 'provider' => 'Spelman' + } assert_equal 'https://example.org/gismit:dz_f7regions_2016', gis_access_link(access_elsewhere) end test 'gis_access_link is data download URL for MIT records' do access_free = { - 'rights' => [ - { - 'description' => 'foo', - 'kind' => 'bar' - }, - { - 'description' => 'no authentication required', - 'kind' => 'Access to files' - } - ], - 'links' => [ - { - 'kind' => 'Download', - 'text' => 'Data', - 'url' => 'https://example.org/dz_f7regions_2016.zip' - }, - { - 'kind' => 'Website', - 'text' => 'Website', - 'url' => 'https://example.org/gismit:dz_f7regions_2016' - } - ] - } + 'rights' => [ + { + 'description' => 'foo', + 'kind' => 'bar' + }, + { + 'description' => 'no authentication required', + 'kind' => 'Access to files' + } + ], + 'links' => [ + { + 'kind' => 'Download', + 'text' => 'Data', + 'url' => 'https://example.org/dz_f7regions_2016.zip' + }, + { + 'kind' => 'Website', + 'text' => 'Website', + 'url' => 'https://example.org/gismit:dz_f7regions_2016' + } + ] + } access_auth = { - 'rights' => [ - { - 'description' => 'foo', - 'kind' => 'bar' - }, - { - 'description' => 'MIT authentication required', - 'kind' => 'Access to files' - } - ], - 'links' => [ - { - 'kind' => 'Download', - 'text' => 'Data', - 'url' => 'https://example.org/dz_f7regions_2016.zip' - }, - { - 'kind' => 'Website', - 'text' => 'Website', - 'url' => 'https://example.org/gismit:dz_f7regions_2016' - } - ] - } - assert_equal 'https://example.org/dz_f7regions_2016.zip', gis_access_link(access_free) - assert_equal 'https://example.org/dz_f7regions_2016.zip', gis_access_link(access_auth) + 'rights' => [ + { + 'description' => 'foo', + 'kind' => 'bar' + }, + { + 'description' => 'MIT authentication required', + 'kind' => 'Access to files' + } + ], + 'links' => [ + { + 'kind' => 'Download', + 'text' => 'Data', + 'url' => 'https://example.org/dz_f7regions_2016.zip' + }, + { + 'kind' => 'Website', + 'text' => 'Website', + 'url' => 'https://example.org/gismit:dz_f7regions_2016' + } + ] + } + assert_equal 'https://example.org/dz_f7regions_2016.zip?timdexui=true', gis_access_link(access_free) + assert_equal 'https://example.org/dz_f7regions_2016.zip?timdexui=true', gis_access_link(access_auth) end test 'source_metadata_available? returns true if source metadata link exists' do @@ -272,7 +272,7 @@ class RecordHelperTest < ActionView::TestCase test 'parse_nested_field returns nil for fields that are not nested' do string_field = 'string' - array_of_strings_field = ['string', 'other_string'] + array_of_strings_field = %w[string other_string] assert_nil parse_nested_field(string_field) assert_nil parse_nested_field(array_of_strings_field) end @@ -304,8 +304,8 @@ class RecordHelperTest < ActionView::TestCase test 'deduplicate_subjects returns only unique subjects' do # within the same subject - duplicative_subject = [{ 'kind' => 'foo', 'value' => ['bar', 'bar', 'baz']}] - assert_equal [['bar', 'baz']], deduplicate_subjects(duplicative_subject) + duplicative_subject = [{ 'kind' => 'foo', 'value' => %w[bar bar baz] }] + assert_equal [%w[bar baz]], deduplicate_subjects(duplicative_subject) # across multiple subjects multiple_duplicative_subjects = [{ 'kind' => 'foo', 'value' => ['bar'] }, @@ -315,7 +315,7 @@ class RecordHelperTest < ActionView::TestCase test 'deduplicate_subjects ignores case' do # within the same subject - duplicative_subject = [{ 'kind' => 'foo', 'value' => ['Bar', 'BAR', 'bar']}] + duplicative_subject = [{ 'kind' => 'foo', 'value' => %w[Bar BAR bar] }] assert_equal [['Bar']], deduplicate_subjects(duplicative_subject) # across multiple subjects From 54a2e465e28911b945a437a491a458f81f7f40c0 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 24 May 2024 09:14:18 -0400 Subject: [PATCH 2/2] Handle URLs that may already have query params A few complex lines were moved to their own methods to make reading the method a bit easier --- Gemfile.lock | 1 + app/helpers/record_helper.rb | 20 +++++++++++++++++--- test/helpers/record_helper_test.rb | 12 ++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5b79a882..bc49f0e0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -361,6 +361,7 @@ GEM PLATFORMS arm64-darwin-22 + arm64-darwin-23 x86_64-darwin-19 x86_64-darwin-20 x86_64-darwin-21 diff --git a/app/helpers/record_helper.rb b/app/helpers/record_helper.rb index ff254d27..142ef5d6 100644 --- a/app/helpers/record_helper.rb +++ b/app/helpers/record_helper.rb @@ -84,13 +84,27 @@ def gis_access_link(metadata) # At this point, we don't show download links for non-MIT records. For MIT records, the download link is stored # consistently as a download link. We are confirming that the link text is 'Data' for added confirmation. if access_type(metadata) == 'unknown: check with owning institution' - links.select { |link| link['kind'] == 'Website' }.first['url'] + website_url(links) else - url = links.select { |link| link['kind'] == 'Download' && link['text'] == 'Data' }.first['url'] - "#{url}?timdexui=true" + url = download_url(links) + append_timdexui(url) end end + def website_url(links) + links.select { |link| link['kind'] == 'Website' }.first['url'] + end + + def download_url(links) + links.select { |link| link['kind'] == 'Download' && link['text'] == 'Data' }.first['url'] + end + + def append_timdexui(url) + uri = Addressable::URI.parse(url) + uri.query_values = (uri.query_values || {}).merge(timdexui: true) + uri.to_s + end + def access_type(metadata) access_right = metadata['rights']&.select { |right| right['kind'] == 'Access to files' } return if access_right.blank? diff --git a/test/helpers/record_helper_test.rb b/test/helpers/record_helper_test.rb index e1ca3668..cbec11b6 100644 --- a/test/helpers/record_helper_test.rb +++ b/test/helpers/record_helper_test.rb @@ -247,6 +247,18 @@ class RecordHelperTest < ActionView::TestCase assert_equal 'https://example.org/dz_f7regions_2016.zip?timdexui=true', gis_access_link(access_auth) end + test 'append_timdexui_with_no_existing_query_values' do + url = 'https://example.org/dz_f7regions_2016.zip' + assert_equal('https://example.org/dz_f7regions_2016.zip?timdexui=true', + append_timdexui(url)) + end + + test 'append_timdexui_with_existing_query_values' do + url = 'https://example.org/dz_f7regions_2016.zip?hallo=goodbye' + assert_equal('https://example.org/dz_f7regions_2016.zip?hallo=goodbye&timdexui=true', + append_timdexui(url)) + end + test 'source_metadata_available? returns true if source metadata link exists' do links = [{ 'kind' => 'Download', 'text' => 'Source Metadata', 'url' => 'https://example.org/metadata.zip' }] assert source_metadata_available?(links)