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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ See `Optional Environment Variables` for more information.
- `MIT_PRIMO_URL`: The base URL for MIT Libraries' Primo instance (used to generate record links).
- `PRIMO_API_KEY`: The Primo Search API key.
- `PRIMO_API_URL`: The Primo Search API base URL.
- `PRIMO_INST_ID`: Our Ex Libris Instituion ID
- `PRIMO_SCOPE`: The Primo Search API `scope` used in Primo UI links. Does not affect our API calls. Ask Enterprise Systems for value.
- `PRIMO_TAB`: The Primo Search API `tab` used in Primo UI links. Does not affect our API calls. Ask Enterprise Systems for value.
- `PRIMO_VID`: The Primo Search API `vid` (or 'view ID`) param. Used in both our API calls and Primo UI. Ask Enterprise Systems for value.
Expand Down
12 changes: 11 additions & 1 deletion app/javascript/controllers/content_loader_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ export default class extends Controller {
load() {
fetch(this.urlValue)
.then(response => response.text())
.then(html => this.element.innerHTML = html)
.then(html => {
this.element.innerHTML = html;
// Hide primo links if libkey link is present
if (this.element.querySelector('.libkey-link')) {
const resultGet = this.element.closest('.result-get');
if (resultGet) {
const primoLinks = resultGet.querySelectorAll('.primo-link');
primoLinks.forEach(link => link.style.display = 'none');
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking comment:
This is probably a question for @djanelle-mit , but I'm wondering if we can track these removals somehow, to know how often they occur. The one test search I did for exoplanet signature detection methods returned 15 different primo links, most of which ended up being suppressed, although some remained.

My guess at a question around this would be something like: "How many times do links with the primo-link class appear on a results page? How many times to links with that class get removed from a results page?" . I don't remember what trigger options exist for Matomo tag containers, so I'm not sure what this might look like in practice.

If we're going to leave a structure like this in place for long, however, I'd like to consider having it be something that we record analytics about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add it to the analytics doc! I'm still operating with an assumption that we can detect if an element with a particular selector was loaded (and not seen), let's make sure we try to track this.

}
}
})
}
}
1 change: 1 addition & 0 deletions app/models/libkey.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def self.lookup(type:, identifier:, libkey_client: nil)
Sentry.set_tags('mitlib.libkeyurl': url)
Sentry.set_tags('mitlib.libkeystatus': e.message)
Sentry.capture_message('Unexpected Libkey response status')
Rails.logger.error("Unexpected Libkey response status: #{e.message}")
nil
rescue HTTP::Error
Rails.logger.error('Libkey connection error')
Expand Down
56 changes: 51 additions & 5 deletions app/models/normalize_primo_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,55 @@ def links
links << { 'url' => record_link, 'kind' => 'full record' }
end

# Add openurl if available
links << { 'url' => openurl, 'kind' => 'openurl' } if openurl.present?
# If $$Topenurl_article
if @record['pnx']['links'] && @record['pnx']['links']['openurl'] && openurl.present?
links << { 'url' => openurl, 'kind' => 'Check Availability' }
end

# Add PDF if available
if @record['pnx']['links'] && @record['pnx']['links']['linktopdf']

parsed_string = parse_link_string(@record['pnx']['links']['linktopdf'].first)

if parsed_string['U'].present?
links << { 'url' => parsed_string['U'],
'kind' => 'View PDF' }
end
end

# Add HTML if available
if @record['pnx']['links'] && @record['pnx']['links']['linktohtml']

parsed_string = parse_link_string(@record['pnx']['links']['linktohtml'].first)

if parsed_string['U'].present?
links << { 'url' => parsed_string['U'],
'kind' => 'View HTML' }
end
end

# Return links if we found any
links.any? ? links : []
end

# Parses a link string into a hash of key-value pairs.
# The link string is a series of key-value pairs separated by $$, where each pair is prefixed by a single character.
# For example: "$$Uhttps://example.com$$TView PDF" would be parsed into { 'U' => 'https://example.com', 'T' => 'View PDF' }
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking question:
Is this construct documented anywhere? How stable is it? My assumption is that this isn't something new, but I'm unfamiliar with it as a format.

I like having a method that will decompose a single string like this into its component parts, and agree with not trying to pluck out values in this method - that happens in the context of whatever logic is fishing for something.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no documentation anywhere, we've never used this in Bento so it is new to our applications with this code.

The Primo API doesn't seem to ever change in terms of new features being added and the documentation is genuinely awful. Nearly everything we do with it is reverse engineered and this is sadly no different. This seems to work... but we have no idea if they ever meant it to work this way. If it stops working, we should still get our OpenURL links which do not rely on this logic. These are basically "extra" links that often (but not always) get replaced with LibKey links.

def parse_link_string(link_string)
return unless link_string.start_with?('$$')

parts = link_string.split('$$')
hash = {}
parts.each do |part|
next if part.empty?

key = part[0]
value = part[1..-1]
hash[key] = value
end
hash
end

def citation
# We don't want to include citations for Alma records at this time. If we include them in the future they need
# to be cleaned up as they currently look like `Engineering village 2$$QEngineering village 2`
Expand Down Expand Up @@ -263,22 +305,26 @@ def openurl

def construct_primo_openurl
return unless @record['delivery']['almaOpenurl']
# Primo OpenURL links to some formats are not helpful
return if format == 'Journal'
return if format == 'Video'

# Here we are converting the Alma link resolver URL provided by the Primo
# Search API to redirect to the Primo UI. This is done for UX purposes,
# as the regular Alma link resolver URLs redirect to a plaintext
# disambiguation page.
primo_openurl_base = [ENV.fetch('MIT_PRIMO_URL', nil),
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking comment:

This is probably not this ticket, and may be part of the refactoring that Isra is looking at, but I'm realizing that we're doing very similar things in this method and the record_link method, but each implementation is slightly different.

I don't know if it needs to be, but part of me wants to recommend a ticket to refactor how we build links into Primo, to try and adopt a common syntax / approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have mixed feelings about this and have only had two sips of coffee so far so this may make more sense later...

Isn't one creating an API link and another creating UI links? While they may be similar, I don't think they'll ever be the same and abstracting the things that are similar when things are distinct feels like it could get confusing rather than simplifying over time. I'm not against looking into this further, but I want us to consider it from both a "DRY" and a "is this really repeating even if it looks like it" perspective.

'/discovery/openurl?institution=',
ENV.fetch('EXL_INST_ID', nil),
ENV.fetch('PRIMO_INST_ID', nil),
'&vid=',
ENV.fetch('PRIMO_VID', nil),
'&'].join
primo_openurl = @record['delivery']['almaOpenurl'].gsub(ENV.fetch('ALMA_OPENURL', nil), primo_openurl_base)

# The ctx params appear to break Primo openurls, so we need to remove them.
params = Rack::Utils.parse_nested_query(primo_openurl)
filtered = params.delete_if { |key, _value| key.starts_with?('ctx') }
# params = Rack::Utils.parse_nested_query(primo_openurl)
Copy link
Member

Choose a reason for hiding this comment

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

Question:
I'm assuming that we're commenting out this filtering because it is no longer needed, and ctx parameters are okay to include. That said, if this is really the case it seems like we could just update line 328 to be URI::DEFAULT_PARSER.unescape(primo_openurl.to_param) and drop lines 324-327 entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh!... I meant to go back and do additional testing. I couldn't find an example where the old conditions made sense but hadn't done exhaustive testing. I'll either update this after doing additional testing or open a ticket to look into this further.

Copy link
Member Author

Choose a reason for hiding this comment

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

@matt-bernhardt I've opened a ticket to do more testing of the OpenURLs to see if we can find any that don't work with this simpler logic. Success criteria are either to revert to stripping ctx or cleaning up the logic if we confirm we no longer need to strip that.

# filtered = params.delete_if { |key, _value| key.starts_with?('ctx') }
filtered = primo_openurl
URI::DEFAULT_PARSER.unescape(filtered.to_param)
end

Expand Down
2 changes: 1 addition & 1 deletion app/views/libkey/lookup.html.erb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<% if Libkey.enabled? && @libkey.present? %>
<%= link_to( @libkey[:text], @libkey[:link], class: 'button', style: 'border: none;' ) %>
<%= link_to( @libkey[:text], @libkey[:link], class: 'button libkey-link' ) %>
<% end %>
9 changes: 7 additions & 2 deletions app/views/search/_result_primo.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
</h3>

<div class="result-metadata">

<p class="pub-info">
<span><%= result[:format] %></span>
<span><%= result[:year] %></span>
Expand Down Expand Up @@ -63,19 +62,25 @@
<% if result[:links].present? %>
<% result[:links].each do |link| %>
<% if link['kind'].downcase == 'full record' %>
<%# Full record link button uses the same link as the title link %>
<% if Feature.enabled?(:record_link) %>
<%= link_to 'View full record', link['url'], class: 'button' %>
<% end %>
<%# Primo supplies PDF and HTML links in addition to the OpenURL variant that may be useful. %>
<%# We hide links with the `primo-link` css class if LibKey returns results. %>
<% else %>
<%= link_to link['kind'].titleize, link['url'], class: 'button' %>
<%= link_to link['kind'], link['url'], class: 'button primo-link' %>
<% end %>
<% end %>
<% end %>

<%# LibKey links are our preferred links so we call the API when possible to get these preferred links %>
<% if Libkey.enabled? && result[:doi].present? %>
<%= render(partial: 'trigger_libkey', locals: { kind: 'doi', identifier: result[:doi] }) %>
<% elsif Libkey.enabled? && result[:pmid].present? %>
<%= render(partial: 'trigger_libkey', locals: { kind: 'pmid', identifier: result[:pmid] }) %>
<% end %>

<% if result[:availability].present? %>
<span class="availability"><%= availability(result[:availability], result[:location], result[:other_availability]) %></span>
<% end %>
Expand Down
20 changes: 20 additions & 0 deletions test/fixtures/primo/full_record.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,26 @@
"22110403"
]
},
"links": {
"openurl": [
"$$Topenurl_article"
],
"openurlfulltext": [
"$$Topenurlfull_article"
],
"backlink": [
"$$Uhttp://pascal-francis.inist.fr/vibad/index.php?action=getRecordDetail&idt=21178602$$DView record in Pascal Francis$$Hfree_for_read"
],
"thumbnail": [
"$$Tsyndetics_thumb_exl"
],
"linktopdf": [
"$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/pdf/20464433$$EPDF$$P50$$Gjstor$$H"
],
"linktohtml": [
"$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/20464433$$EHTML$$P50$$Gjstor$$H"
]
},
"facets": {
"frbrtype": [
"5"
Expand Down
27 changes: 22 additions & 5 deletions test/models/normalize_primo_record_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,29 @@ def cdi_record
assert_equal 'full record', normalized[:links].first['kind']

# Second link should be the Alma openurl
assert_equal 'openurl', normalized[:links].second['kind']
assert_equal 'Check Availability', normalized[:links].second['kind']
end

test 'handles missing links' do
normalized = NormalizePrimoRecord.new(minimal_record, 'test').normalize
assert_empty normalized[:links]
end

test 'parse_link_string creates expected data structure' do
# Strings that don't start with $$ should not be processed
link_string = 'https://example.com?param1=value1&param2=value2'
assert_nil NormalizePrimoRecord.new(full_record, 'test').send(:parse_link_string, link_string)

# Extract properly formatted links
link_string = '$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/pdf/20464433$$EPDF$$P50$$Gjstor$$H'
expected = { 'U' => 'https://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/pdf/20464433', 'E' => 'PDF', 'P' => '50', 'G' => 'jstor', 'H' => '' }
assert_equal expected, NormalizePrimoRecord.new(full_record, 'test').send(:parse_link_string, link_string)

link_string = '$$Uhttps://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/20464433$$EHTML$$P50$$Gjstor$$H'
expected = { 'U' => 'https://libproxy.mit.edu/login?&url=https://www.jstor.org/stable/20464433', 'E' => 'HTML', 'P' => '50', 'G' => 'jstor', 'H' => '' }
assert_equal expected, NormalizePrimoRecord.new(full_record, 'test').send(:parse_link_string, link_string)
end

test 'normalizes citation for cdi records' do
record = full_record.dup
record['pnx']['control']['recordid'] = ['cdi_crossref_primary_10_1234_test_article']
Expand Down Expand Up @@ -286,8 +301,10 @@ def cdi_record
link_kinds = normalized[:links].map { |link| link['kind'] }

assert_includes link_kinds, 'full record'
assert_includes link_kinds, 'openurl'
assert_equal 2, normalized[:links].length # Only full record and openurl
assert_includes link_kinds, 'Check Availability'
assert_includes link_kinds, 'View PDF'
assert_includes link_kinds, 'View HTML'
assert_equal 4, normalized[:links].length
end

# Additional coverage tests for existing methods
Expand Down Expand Up @@ -359,14 +376,14 @@ def cdi_record
test 'handles openurl server validation' do
# Test with matching server
normalized = NormalizePrimoRecord.new(full_record, 'test').normalize
openurl_link = normalized[:links].find { |link| link['kind'] == 'openurl' }
openurl_link = normalized[:links].find { |link| link['kind'] == 'Check Availability' }
assert_not_nil openurl_link

# Test with mismatched server. Should log warning but still return URL
record = full_record.deep_dup
record['delivery']['almaOpenurl'] = 'https://different.server.com/openurl?param=value'
normalized = NormalizePrimoRecord.new(record, 'test').normalize
openurl_link = normalized[:links].find { |link| link['kind'] == 'openurl' }
openurl_link = normalized[:links].find { |link| link['kind'] == 'Check Availability' }
assert_not_nil openurl_link
end

Expand Down