Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions app/controllers/thirdiron_controller.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require 'uri'

class ThirdironController < ApplicationController
layout false

Expand All @@ -14,11 +16,28 @@ def browzine
return unless ThirdIron.enabled? && params[:issn].present?

@browzine = Browzine.lookup(issn: params[:issn])
@full_record_url = safe_full_record_url(params[:full_record_url])
end
Comment thread
jazairi marked this conversation as resolved.

private

def expected_params?
params[:type].present? && params[:identifier].present?
end

def safe_full_record_url(url)
return nil unless url.is_a?(String)

url = url.strip
return nil if url.blank?

parsed = URI.parse(url)
return nil unless parsed.is_a?(URI::HTTP)
return nil if parsed.host.blank?
Comment thread
Copilot marked this conversation as resolved.
return nil if parsed.userinfo.present?

parsed.to_s
Comment thread
Copilot marked this conversation as resolved.
rescue URI::InvalidURIError, ArgumentError
nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function with high complexity (count = 5): safe_full_record_url [qlty:function-complexity]

end
Comment thread
jazairi marked this conversation as resolved.
end
10 changes: 10 additions & 0 deletions app/helpers/results_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ def article?(format)
format.match?(/\barticle\b/i)
end

# Extracts the full record URL from a result's links
#
# @param result [Hash] A normalized result hash with :links
# @return [String, nil] The URL of the 'full record' link, or nil if not found
def full_record_url(result)
return unless result.is_a?(Hash)

result[:links]&.find { |link| link['kind'] == 'full record' }&.dig('url')
end

# Determines if a result has any fulfillment links to render
#
# @param result [Hash] A normalized Primo result hash
Expand Down
6 changes: 3 additions & 3 deletions app/views/search/_result_primo.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
<div class="title-lockup">
<%# NOTE: the order of the two items in this lockup are being swapped visually with CSS. The markup is ordered to make the screen reading experience more logical %>
<h3 class="record-title">
<% if result[:links]&.find { |link| link['kind'] == 'full record' } %>
<%= link_to(result[:title], result[:links].find { |link| link['kind'] == 'full record' }['url'], data: { content_piece: 'Result Title' }) %>
<% if full_record_url(result) %>
<%= link_to(result[:title], full_record_url(result), data: { content_piece: 'Result Title' }) %>
<% else %>
<%= result[:title] %>
<% end %>
Expand Down Expand Up @@ -122,7 +122,7 @@
<%# Trigger BrowZine lookup (render inside result-get so injected HTML
is part of the flex `.result-get` area and receives expected styles) %>
<% if ThirdIron.enabled? && result[:format].downcase == 'journal' && result[:issn].present? %>
<%= render(partial: 'trigger_browzine', locals: { issn: result[:issn] }) %>
<%= render(partial: 'trigger_browzine', locals: { issn: result[:issn], full_record_url: full_record_url(result) }) %>
<% end %>
</div>
<% end %>
Expand Down
4 changes: 3 additions & 1 deletion app/views/search/_trigger_browzine.html.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<% return unless ThirdIron.enabled? %>

<% data_url = "/browzine?issn=#{issn}" %>
<% query_params = { issn: issn } %>
<% query_params[:full_record_url] = full_record_url if full_record_url.present? %>
<% data_url = "/browzine?#{query_params.to_query}" %>

<span class="libkey-container"
data-controller="content-loader"
Expand Down
17 changes: 12 additions & 5 deletions app/views/thirdiron/browzine.html.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
<% if ThirdIron.enabled? && @browzine.present? %>
<% if @browzine[:browzine_link].present? %>
<div class="libkey-actions">
<%= link_to @browzine[:browzine_link][:text], @browzine[:browzine_link][:link], class: 'button libkey-link', data: { matomo_seen: "Results, Browzine Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Browzine Link Engaged, Link: {{getElementText}}", content_piece: @browzine[:browzine_link][:text] } %>
</div>
<%# Dedicated Browzine endpoint response for journal records.
Rendered when: A Primo journal result with ISSN triggers a Browzine API lookup via /browzine?issn=X&full_record_url=Y
Use case: Primary fulfillment source for journal articles (via trigger_browzine partial from _result_primo.html.erb)
Returns: Browzine link + optional "Full-text options" button (links to Primo record)
%>
<% if ThirdIron.enabled? && @browzine.present? && @browzine[:browzine_link].present? %>
Comment thread
JPrevost marked this conversation as resolved.
<% if @full_record_url.present? %>
<%= link_to 'Full-text options', @full_record_url, class: 'button libkey-link', data: { matomo_seen: "Results, Full-text Options Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Full-text Options Link Engaged, Link: {{getElementText}}", content_piece: 'Full-text options' } %>
<% end %>

<div class="libkey-actions">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the intent of this libkey-actions div and whether this full text options link belongs inside or outside of it. I know it isn't libkey related, but it's possible that div was named as such as a means to group the links coming out of this partial when the only links were libkey. It may be worth checking in with Dave to confirm if he has input into whether this new link is in that div or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the link outside of this div was a styling workaround, similar to removing the button class. When the Full-text options link is placed inside the div, both it and the Browse journal link render as if they are the first link in the list.

I'll tag Dave on this to see if he has insight on this and the button class issue.

<%= link_to @browzine[:browzine_link][:text], @browzine[:browzine_link][:link], class: 'libkey-link', data: { matomo_seen: "Results, Browzine Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Browzine Link Engaged, Link: {{getElementText}}", content_piece: @browzine[:browzine_link][:text] } %>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why button is being removed? Don't we add it to all the fulfillment links and the css styles them as buttons or not conditionally?

</div>
<% end %>
8 changes: 7 additions & 1 deletion app/views/thirdiron/libkey.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,16 @@
<%= link_to( @libkey[:best_integrator_link][:text], @libkey[:best_integrator_link][:link], class: 'button libkey-link', data: { matomo_seen: "Results, LibKey Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, LibKey Link Engaged, Link: {{getElementText}}", content_piece: @libkey[:best_integrator_link][:text] } ) %>
<% end %>

<%# Browzine link returned as part of LibKey fulfillment response.
Rendered when: LibKey API includes a browzine_link in its response
Use case: Secondary fulfillment option alongside PDF, HTML, or other direct links from LibKey
Note: Different from dedicated browzine.html.erb which is called directly for journals.
LibKey Browzine link appears when looking up DOI/PMID fulfillment (articles, etc.)
%>
<%# Display browzine link if available. This should always display if we have it regardless of other links. %>
<% if @libkey[:browzine_link].present? %>
<div class="libkey-actions">
<%= link_to @libkey[:browzine_link][:text], @libkey[:browzine_link][:link], class: 'button libkey-link', data: { matomo_seen: "Results, Browzine Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Browzine Link Engaged, Link: {{getElementText}}", content_piece: @libkey[:browzine_link][:text] } %>
<%= link_to @libkey[:browzine_link][:text], @libkey[:browzine_link][:link], class: 'libkey-link', data: { matomo_seen: "Results, Browzine Link Seen, Tab: {{getActiveTabName}}", matomo_click: "Results, Browzine Link Engaged, Link: {{getElementText}}", content_piece: @libkey[:browzine_link][:text] } %>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why button is being removed? Don't we add it to all the fulfillment links and the css styles them as buttons or not conditionally?

It's unclear to me when this template comes into play versus the same link in the browzine template. I'm concerned whatever condition this is here for should include the new full text options as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but the addition of the Full text options link affects the conditional styling in such a way that Browse journal renders as a primary button rather than the intended underlined styling. The easiest path I could figure to fix that was to remove the button class, but that doesn't necessarily mean it's the best path.

</div>
<% end %>

Expand Down
32 changes: 30 additions & 2 deletions test/controllers/thirdiron_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ class ThirdironControllerTest < ActionDispatch::IntegrationTest
get '/libkey?type=doi&identifier=10.1038/s41567-023-02305-y'

assert_response :success
assert_select 'a.button', { count: 3 }
assert_select 'a.libkey-link', { count: 3 }
end

VCR.use_cassette('libkey pmid') do
get '/libkey?type=pmid&identifier=22110403'

assert_response :success
assert_select 'a.button', { count: 3 }
assert_select 'a.libkey-link', { count: 3 }
end
end

Expand Down Expand Up @@ -111,7 +111,35 @@ class ThirdironControllerTest < ActionDispatch::IntegrationTest
get '/browzine?issn=1546170X'

assert_response :success

# Only browzine link, no button since no full_record_url provided
assert_select 'a.button', { count: 0 }
assert_select 'a.libkey-link', { count: 1 }
end
end

test 'browzine route with full_record_url returns both links' do
VCR.use_cassette('browzine issn') do
get '/browzine?issn=1546170X&full_record_url=https://example.com/full-record'

assert_response :success

# Button (Full-text options) and browzine link both have libkey-link class
assert_select 'a.button', { count: 1 }
assert_select 'a.button[href="https://example.com/full-record"]'
assert_select 'a.libkey-link', { count: 2 }
end
Comment thread
jazairi marked this conversation as resolved.
end
Comment thread
Copilot marked this conversation as resolved.

test 'browzine route ignores unsafe full_record_url values' do
VCR.use_cassette('browzine issn') do
get '/browzine?issn=1546170X&full_record_url=javascript:alert(1)'

assert_response :success

# Unsafe URL should not produce a "Full-text options" button
assert_select 'a.button', { count: 0 }
assert_select 'a.libkey-link', { count: 1 }
end
end

Expand Down
34 changes: 34 additions & 0 deletions test/helpers/results_helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,38 @@ class ResultsHelperTest < ActionView::TestCase
})
end
end

test 'full_record_url returns the URL when result has a full record link' do
result = {
links: [
{ 'kind' => 'PDF', 'url' => 'https://pdf.example.com' },
{ 'kind' => 'full record', 'url' => 'https://primo.example.com/record' }
]
}
assert_equal 'https://primo.example.com/record', full_record_url(result)
end

test 'full_record_url returns nil when result has no links' do
result = {}
assert_nil full_record_url(result)
end

test 'full_record_url returns nil when result has links but no full record link' do
result = {
links: [
{ 'kind' => 'PDF', 'url' => 'https://pdf.example.com' },
{ 'kind' => 'HTML', 'url' => 'https://html.example.com' }
]
}
assert_nil full_record_url(result)
end

test 'full_record_url returns nil when result is nil' do
assert_nil full_record_url(nil)
end

test 'full_record_url returns nil when result[:links] is nil' do
result = { links: nil }
assert_nil full_record_url(result)
end
end