-
Notifications
You must be signed in to change notification settings - Fork 0
Create integration with Libkey #303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| class LibkeyController < ApplicationController | ||
| layout false | ||
|
|
||
| def lookup | ||
| return unless Libkey.enabled? && expected_params? | ||
|
|
||
| @libkey = Libkey.lookup(type: params[:type], identifier: params[:identifier]) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def expected_params? | ||
| params[:type].present? && params[:identifier].present? | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| # TODO: Need some documentation block to explain what we use Libkey for... | ||
| class Libkey | ||
| class LookupFailure < StandardError; end | ||
|
|
||
| BASEURL = 'https://public-api.thirdiron.com/public/v1/libraries'.freeze | ||
|
|
||
| # enabled? confirms that all required environment variables are set. | ||
| # | ||
| # @return Boolean | ||
| def self.enabled? | ||
| libkey_id.present? && libkey_key.present? | ||
| end | ||
|
|
||
| def self.lookup(type:, identifier:, libkey_client: nil) | ||
| return unless enabled? | ||
| return unless %w[doi pmid].include?(type) | ||
|
|
||
| url = libkey_url(type, identifier) | ||
|
|
||
| libkey_http = setup(url, libkey_client) | ||
|
|
||
| begin | ||
| raw_response = libkey_http.timeout(6).get(url) | ||
| raise LookupFailure, raw_response.status unless raw_response.status == 200 | ||
|
|
||
| json_response = JSON.parse(raw_response.to_s) | ||
| extract_metadata(json_response) | ||
| rescue LookupFailure => e | ||
| Sentry.set_tags('mitlib.libkeyurl': url) | ||
| Sentry.set_tags('mitlib.libkeystatus': e.message) | ||
| Sentry.capture_message('Unexpected Libkey response status') | ||
| nil | ||
| rescue HTTP::Error | ||
| Rails.logger.error('Libkey connection error') | ||
| { 'error' => 'A connection error has occurred' } | ||
| rescue JSON::ParserError | ||
| Rails.logger.error('Libkey parsing error') | ||
| { 'error' => 'A parsing error has occurred' } | ||
| end | ||
| end | ||
|
|
||
| def self.extract_metadata(external_data) | ||
| return unless external_data['data']['bestIntegratorLink'] | ||
|
|
||
| { | ||
| link: external_data['data']['bestIntegratorLink']['bestLink'], | ||
| text: external_data['data']['bestIntegratorLink']['recommendedLinkText'], | ||
| type: external_data['data']['bestIntegratorLink']['linkType'] # Not sure whether this belongs here. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what this comment is referring to now and I suspect it will be even more confusing later...more context or removing the comment is likely to lead to less confusion later.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The response from Libkey includes three fields, but currently we're only using two in the payload back to the UI (link and text). I suppose the simplest posture would be to just drop This thread is related in my mind to the others below it, which all deal with "what does UX want to appear from Libkey" |
||
| } | ||
| end | ||
|
|
||
| def self.libkey_id | ||
| ENV.fetch('LIBKEY_ID', nil) | ||
| end | ||
|
|
||
| def self.libkey_key | ||
| ENV.fetch('LIBKEY_KEY', nil) | ||
| end | ||
|
|
||
| def self.libkey_url(type, identifier) | ||
| "#{BASEURL}/#{libkey_id}/articles/#{type}/#{identifier}?access_token=#{libkey_key}" | ||
| end | ||
|
|
||
| def self.setup(url, libkey_client) | ||
| libkey_client || HTTP.persistent(url) | ||
| .headers(accept: 'application/json') | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| <% if Libkey.enabled? && @libkey.present? %> | ||
| <%= link_to( @libkey[:text], @libkey[:link], class: 'button button-primary' ) %> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume we'll style this differently in upcoming PRs (either the initial integration or Dave's follow up). We'll also have multiple links available right? HTML, PDF like Primo UI has?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ticket for this says that styling will happen afterwards, so I'm choosing to ignore that issue for now. The thing I'm not sure about, but which will be part of the handoff with Dave in the following PR, is whether the Libkey integration should result in only one Libkey link or not. There will already be others that come back from Primo / TIMDEX, but my understanding of the Libkey integration was that we wanted to keep things simple and just show one libkey link (the "best integration link" in their response). If that needs to be adjusted, my preference is to have that conversation as part of the UI integration in the next PR. |
||
| <% end %> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| require 'test_helper' | ||
|
|
||
| class LibkeyControllerTest < ActionDispatch::IntegrationTest | ||
| test 'lookup route exists with no content' do | ||
| # No cassette because this never results in traffic to Libkey | ||
| get '/lookup' | ||
|
|
||
| assert_response :success | ||
| assert_equal response.body, '' | ||
| end | ||
|
|
||
| test 'lookup route returns nothing without required parameters' do | ||
| # No cassettes because these never result in traffic to Libkey | ||
| # "type" value only | ||
| get '/lookup?type=doi' | ||
|
|
||
| assert_equal response.body, '' | ||
|
|
||
| # "identifier" value only | ||
| get '/lookup?identifier=10.1038/s41567-023-02305-y' | ||
|
|
||
| assert_equal response.body, '' | ||
| end | ||
|
|
||
| test 'lookup route returns HTML for valid parameters' do | ||
| VCR.use_cassette('libkey doi') do | ||
| get '/lookup?type=doi&identifier=10.1038/s41567-023-02305-y' | ||
|
|
||
| assert_response :success | ||
| assert_select 'a.button-primary', { count: 1 } | ||
| end | ||
|
|
||
| VCR.use_cassette('libkey pmid') do | ||
| get '/lookup?type=pmid&identifier=22110403' | ||
|
|
||
| assert_response :success | ||
| assert_select 'a.button-primary', { count: 1 } | ||
| end | ||
| end | ||
|
|
||
| test 'lookup for non-existent identifier returns blank' do | ||
| # Libkey responds here, so we have a cassette - but the response is empty | ||
| VCR.use_cassette('libkey nonexistent') do | ||
| get '/lookup?type=doi&identifier=foobar' | ||
|
|
||
| assert_response :success | ||
| assert_equal response.body, '' | ||
| end | ||
| end | ||
|
|
||
| test 'no response when either env var is not set' do | ||
| # No cassette because this never results in traffic to Libkey | ||
| ClimateControl.modify(LIBKEY_ID: nil) do | ||
| get '/lookup?type=doi&identifier=10.1038/s41567-023-02305-y' | ||
|
|
||
| assert_response :success | ||
| assert_equal response.body, '' | ||
| end | ||
|
|
||
| ClimateControl.modify(LIBKEY_KEY: nil) do | ||
| get '/lookup?type=doi&identifier=10.1038/s41567-023-02305-y' | ||
|
|
||
| assert_response :success | ||
| assert_equal response.body, '' | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| require 'test_helper' | ||
|
|
||
| class LibkeyMockResponse | ||
| attr_reader :status | ||
|
|
||
| def initialize(status, body) | ||
| @status = status | ||
| @body = body | ||
| end | ||
|
|
||
| def to_s | ||
| @body | ||
| end | ||
| end | ||
|
|
||
| class LibkeyConnectionError | ||
| def timeout(_) | ||
| self | ||
| end | ||
|
|
||
| def get(_url) | ||
| raise HTTP::ConnectionError, 'forced connection failure' | ||
| end | ||
| end | ||
|
|
||
| class LibkeyParsingError | ||
| def timeout(_) | ||
| self | ||
| end | ||
|
|
||
| def get(_url) | ||
| LibkeyMockResponse.new(200, 'This is not valid json') | ||
| end | ||
| end | ||
|
|
||
| class LibkeyTest < ActiveSupport::TestCase | ||
| test 'enabled? method returns true if both env are set' do | ||
| ClimateControl.modify(LIBKEY_ID: 'foo', LIBKEY_KEY: 'bar') do | ||
| assert Libkey.enabled? | ||
| end | ||
| end | ||
|
|
||
| test 'enabled? method returns false if either env not set' do | ||
| ClimateControl.modify(LIBKEY_ID: nil) do | ||
| refute Libkey.enabled? | ||
| end | ||
|
|
||
| ClimateControl.modify(LIBKEY_KEY: nil) do | ||
| refute Libkey.enabled? | ||
| end | ||
| end | ||
|
|
||
| test 'lookup does nothing with a type other than "doi" or "pmid"' do | ||
| refute Libkey.lookup(type: 'foo', identifier: 'foobar') | ||
| end | ||
|
|
||
| test 'lookup does work with any identifier value' do | ||
| VCR.use_cassette('libkey nonexistent') do | ||
| result = Libkey.lookup(type: 'doi', identifier: 'foobar') | ||
|
|
||
| refute result | ||
| end | ||
| end | ||
|
|
||
| test 'lookup gets a valid response for DOIs' do | ||
| VCR.use_cassette('libkey doi') do | ||
| result = Libkey.lookup(type: 'doi', identifier: '10.1038/s41567-023-02305-y') | ||
|
|
||
| assert_instance_of Hash, result | ||
| assert_equal result.keys, %i[link text type] | ||
| end | ||
| end | ||
|
|
||
| test 'lookup gets a valid response for PMIDs' do | ||
| VCR.use_cassette('libkey pmid') do | ||
| result = Libkey.lookup(type: 'pmid', identifier: '22110403') | ||
|
|
||
| assert_instance_of Hash, result | ||
| assert_equal result.keys, %i[link text type] | ||
| end | ||
| end | ||
|
|
||
| test 'libkey model catches connection errors' do | ||
| libkey_client = LibkeyConnectionError.new | ||
|
|
||
| result = Libkey.lookup(type: 'doi', identifier: '10.1038/s41567-023-02305-y', libkey_client:) | ||
|
|
||
| assert_instance_of Hash, result | ||
| assert_equal 'A connection error has occurred', result['error'] | ||
| end | ||
|
|
||
| test 'libkey model catches parsing errors' do | ||
| libkey_client = LibkeyParsingError.new | ||
|
|
||
| result = Libkey.lookup(type: 'doi', identifier: '10.1038/s41567-023-02305-y', libkey_client:) | ||
|
|
||
| assert_instance_of Hash, result | ||
| assert_equal 'A parsing error has occurred', result['error'] | ||
| end | ||
| end |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we only getting
bestLinkback or are we also seeing multiple links and only usingbestLinkin this implementation?It's unclear to me what the intended USE UI behavior is (i.e. just Best Link or also HTML, Browse links, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API response includes many more fields, but my thought for now was to start with the best integration link only. The next PR will have a more involved review that includes UX weighing in about their presentation and UX requirements.