Update Primo author link encoding #1400
Conversation
Why are these changes being introduced: * Author linking does not encode the author names which sometimes made the links to Primo not work as expected Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/PW-132 How does this address that need: * Encodes the author component of the URL Document any side effects to this change: I didn't look at anything else as we so rarely touch this code I don't remember how a lot of it works and I didn't want to pull strings that caused much more work than we intended at this time.
app/models/normalize_primo_common.rb
Outdated
| ENV.fetch('PRIMO_VID', nil)].join | ||
| end | ||
|
|
||
| def clean_author(author) |
There was a problem hiding this comment.
I exctrated this out thinking the solution may have been more complex than just encoding. I left it as it's own method as author_link has quite a bit going on. I'm not sure if this makes things better or worse. I'm happy to collapse it or leave it as is.
There was a problem hiding this comment.
I'm fine with making this its own method, but what do you think about changing the method name? clean_author might be confusing alongside sanitize_authors. Maybe encode_author, or something to that effect?
There was a problem hiding this comment.
Great idea. I didn't even notice the confusing naming until you mentioned it. Thanks!
| 'https://mit.primo.exlibrisgroup.com/discovery/search?query=creator,exact,Rudolph, J.&tab=all&search_scope=all&vid=FAKE_PRIMO_VID'], | ||
| 'https://mit.primo.exlibrisgroup.com/discovery/search?query=creator,exact,Rudolph%2C%20J.&tab=all&search_scope=all&vid=FAKE_PRIMO_VID'], | ||
| result.authors.first | ||
| end | ||
|
|
||
| test 'multiple authors in a single element are appropriately parsed' do | ||
| result = monkeys['results'].second | ||
| assert_not_equal ['Beran, Michael J ; Smith, J. David'], result.authors.first.first | ||
| assert_equal ['Beran, Michael J', | ||
| 'https://mit.primo.exlibrisgroup.com/discovery/search?query=creator,exact,Beran, Michael J&tab=all&search_scope=all&vid=FAKE_PRIMO_VID'], | ||
| 'https://mit.primo.exlibrisgroup.com/discovery/search?query=creator,exact,Beran%2C%20Michael%20J&tab=all&search_scope=all&vid=FAKE_PRIMO_VID'], | ||
| result.authors.first | ||
| assert_equal ['Smith, J. David', | ||
| 'https://mit.primo.exlibrisgroup.com/discovery/search?query=creator,exact,Smith, J. David&tab=all&search_scope=all&vid=FAKE_PRIMO_VID'], | ||
| 'https://mit.primo.exlibrisgroup.com/discovery/search?query=creator,exact,Smith%2C%20J.%20David&tab=all&search_scope=all&vid=FAKE_PRIMO_VID'], | ||
| result.authors.second |
There was a problem hiding this comment.
I just noticed that many things change automatically with my local styles check on save. Sorry for the noise. These are the only intentional changes to this file
There was a problem hiding this comment.
Thanks for pulling this and applying the fix! I added a comment in the thread you started about where to put the URI encoding. I wouldn't ordinarily suggest such a small change, but in this case it might make this model a bit easier to grok the next time we come back to this repo (which hopefully be never).
app/models/normalize_primo_common.rb
Outdated
| ENV.fetch('PRIMO_VID', nil)].join | ||
| end | ||
|
|
||
| def clean_author(author) |
There was a problem hiding this comment.
I'm fine with making this its own method, but what do you think about changing the method name? clean_author might be confusing alongside sanitize_authors. Maybe encode_author, or something to that effect?
Code review identified two method names that were likely to be misleading in the future. This updates the newly introduced method to be less similar to the existing method.
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Document any side effects to this change:
I didn't look at anything else as we so rarely touch this code I don't remember how a lot of it works and I didn't want to pull strings that caused much more work than we intended at this time.
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO