Adds availablility information to records #292
Conversation
Pull Request Test Coverage Report for Build 19902084837Details
💛 - Coveralls |
Why are these changes being introduced: * This information is useful to users to understand if an item is available for checkout or needs to be requested. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-246 How does this address that need: * Creates a helper method to format availability information * Updates the result partial to display availability information Document any side effects to this change: * Location information is now expected to be an array with three elements instead of a string to allow for more flexibility in formatting.
JPrevost
force-pushed
the
use-246-holdings
branch
from
78bee5d to
d9c26ca
Compare
app/helpers/results_helper.rb
Outdated
| # Expects an array with three elements: [library name, location name, call number] | ||
| def location(loc) | ||
| "<strong>#{loc[0]}</strong> #{loc[1]} (#{loc[2]})" | ||
| end |
There was a problem hiding this comment.
From my end I think I have everything I need: an element around the library title to style as needed, and an icon element with a unique class to change colors if desired.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
I've got two comments, one is a concern about rendering the availability statement inside the links.each loop that feels like it might not be what we want, and the other is more of a vague disquiet about the format of data being returned by the NormalizePrimo model.
I'm willing to approve this as-is if there's something about the links and availability information that prevents there from being any availability statement for a record with multiple links - I just don't know if this condition ever arises, because I'm not sure the code here handles that the way we're wanting.
My other concern is more of a general statement - if hearing it doesn't trip any concerns for you, it won't be a blocker for my approval.
| <% if result[:links].present? %> | ||
| <% result[:links].each do |link| %> | ||
| <%= link_to link['kind'].titleize, link['url'], class: 'button' %> | ||
| <% if result[:availability].present? %> |
There was a problem hiding this comment.
This may be a requested change, but I'm not 100% sure.
How likely is it that records have multiple :links entries? It feels a bit awkward, here, to be calling the availability() helper for each link (inside the .each loop, rather than after it) when it doesn't seem like the content will be changing for each one. I'm not sure what constraints exist in Primo, though, so maybe this condition will never arise? The one search I've done looking for results with multiple links had availability information for records with a single location - which isn't a great sample size, but I'm not sure how long I want to fish for the exact condition in our records.
There was a problem hiding this comment.
Ha, great catch. I'm not positive if this is an actual problem but it definitely feels wrong.
We only ever want this showing up once, so popping it in a loop makes no sense even if it happens to work.
I intend to change this.
app/models/normalize_primo_record.rb
Outdated
|
|
||
| loc = @record['delivery']['bestlocation'] | ||
| ["#{loc['mainLocation']} #{loc['subLocation']}", loc['callNumber']] | ||
| [loc['mainLocation'], loc['subLocation'], loc['callNumber']] |
There was a problem hiding this comment.
I'm slightly worried that this approach is a bit more fragile, by returning a list of entries that aren't all the same, than it would be if it was returned in key-value format. Doing it this way requires the helper method to deal with loc[0] instead of loc[:mainLocation], which feels a bit more robust.
The concern isn't enough for me to ask for a change, and the prior state of this element was also list, so it isn't a regression - but I'm concerned enough to mention it here in case we're all uneasy. From a quick review of the normalizer model, it's pretty thorough in being object-like for differentiated values, rather than list-like, but there may be cases I'm missing (or this may have been hashed out already and I'm late to the party).
I'd be more concerned if there was any conditional logic in this method where these three values might not all be populated for some reason - so the indexing numbers are at least stable.
There was a problem hiding this comment.
Noted. I think this is technically fine in terms of functionality based on what data seems to come back from the API, but I agree being more explicit in the normalization would be easier to reason with and thus preferred.
i.e. I think this is stable but unclear but we can likely make it stable and clear.
I'll take a closer look and will likely make a change here.
There was a problem hiding this comment.
Ah, location is a common field in which both TIMDEX and Primo need to return the same data structure for us to eventually collapse to a single view. I'll move some of the Helper logic into the Primo normalizer model so we are returning a string instead of an array to better match the current TIMDEX normalizer.
We should move this logic to `holdings` as TIMDEX and Primo are currently normalizing different types of information to a single field. TIMDEX has both `locations` and `holdings` information available whereas Primo only has `locations`. The information in TIMDEX `holdings` aligns well with what Primo `locations` is doing for us here. Since we are not using TIMDEX locations or holdings in search results that use the normalizer, I have opted to resolve that under a separate ticket rather than fixing as part of this work. The data structure I am using here for Primo locations should align well with data available in TIMDEX holdings when we move both to USE normalized holdings.
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Document any side effects to this change:
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing