Repository navigation
Conversation
1 new issue
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
|
|
||
| fullTextOptionActions.forEach((action) => { | ||
| if (action !== preferredAction) { | ||
| action.remove() |
There was a problem hiding this comment.
🟡 Changes recommended
OpenAlex and Alma actions are not fully included in the new override logic, allowing overlapping fallback links to remain visible.
3 open findings
What changed in this PR
Refactors fulfillment-link selection into a testable policy and adds explicit metadata for asynchronous link reconciliation.
Changes:
- Introduces and tests
FulfillmentPolicy. - Adds fulfillment source/type metadata to rendered links.
- Updates content loading to suppress and deduplicate fallback actions.
| File | Description |
|---|---|
app/models/fulfillment_policy.rb |
Centralizes Primo and Browzine link policy. |
app/javascript/controllers/content_loader_controller.js |
Reconciles asynchronously loaded actions. |
app/views/search/_result_primo.html.erb |
Uses the new policy. |
app/views/alma/sru.html.erb |
Identifies Alma actions. |
app/views/openalex/work.html.erb |
Identifies OpenAlex actions. |
app/views/thirdiron/libkey.html.erb |
Identifies LibKey override actions. |
app/views/thirdiron/browzine.html.erb |
Identifies Browzine actions. |
test/models/fulfillment_policy_test.rb |
Tests policy behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coverage Report for CI Build 38652313555Coverage increased (+0.03%) to 98.324%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Why these changes are being introduced: The logic to resolve which fulfillment links to display lives mostly in the view layer. As this feature grows in complexity, the logic becomes harder to understand, and bugs creep in. Most recently, we noticed that two full-text options links can appear for a given record. A more idiomatic approach would be to shift the logic to the model layer, thus making it more visible, testable, and hopefully more understandable. We also need to refactor the Content Loader controller to make certain actions explicit and guard against duplication. Relevant ticket(s): - [USE-708](https://mitlibraries.atlassian.net/browse/USE-708) How this addresses that need: - Alma full-text options link actions are now made explicit in the Content Loader controller. - The Content Loader controller now checks for a Primo override action, which suppresses Primo links when they are not needed (e.g., a link is provided by Browzine/LibKey/etc). - Logic in the view layer is abstracted to a new Fulfillment Policy class. Side effects of this change: Complexity is not quite reduced as much as it is redistributed to the new Fulfillment Policy class. However, the goal of this change is less to reduce complexity and more to improve understandability.
… in override cleanup' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
Can you add some clear documentation in the FulfillmentPolicy class to help me understand the intent? I think it might benefit from a rename to clarify it is just a PrimoFulfillmentPolicy class while doing that? If you disagree that is fine and just the added docs should be enough to allow me to more easily review this change.
Once the added docs are in place, I'll more confidently be able to proceed with the review.
| @@ -0,0 +1,72 @@ | |||
| class FulfillmentPolicy | |||
There was a problem hiding this comment.
Class and non-private method level documentation would be super helpful.
We are making this abstraction because this is complicated logic. The lack of docs here explaining what this class does or why in plain language is just moving the confusion for me.
| @result = result || {} | ||
| end | ||
|
|
||
| def primo_actions(record_link_enabled:) |
There was a problem hiding this comment.
There are only methods for primo and browzine? Is this not (yet?) a FulfillmentPolicy class so much as a Primo/Browzine full text link class? I'm having a hard time understanding if all fulfillment links are affected by this policy logic and if so how (and if not, why not)
There was a problem hiding this comment.
Good questions. Yes, this policy is intentionally scoped to Primo, specifically the result[:links] object and whether Browzine emits a full-text options link. I suspect renaming the policy and adding better docs will help clarify this.
| <% end %> | ||
| </div> | ||
|
|
||
| <%# Primo supplies various fulfillment links, which may overlap with LibKey/Browzine/etc. %> |
There was a problem hiding this comment.
I don't think I fully understood that we didn't have any fulfillment links for non-Primo records at this time. I think we'll add some in the future. I wonder if it would be worthwhile to name the new policy a bit more specifically to ensure it is clear it is only for Primo Records and not Timdex records?
|
|
||
| private | ||
|
|
||
| attr_reader :result |
There was a problem hiding this comment.
Non-blocking. It's unclear to me why this is preferred over just using @result directly. I understand what it enables, but I feel like our code normally leans into the style of calling @result over attr_reader :result.
There was a problem hiding this comment.
I had the same question, actually. GPT convinced me that it's preferable to use a private reader rather than directly touching the instance variable everywhere. However, I think the fact that we were both thrown by this makes me wonder if the approach that's more idiomatic within our team is the better one, even if it isn't the optimal one.
| const resultGet = resultContent.querySelector('.result-get') | ||
| if (almaFulltextOptions && resultGet) { | ||
| resultGet.appendChild(almaFulltextOptions) | ||
| const firstLibkeyActions = resultGet.querySelector('.libkey-actions') |
There was a problem hiding this comment.
firstLibkeyActions is named to clarify that Libkey Actions need to be displayed first and not that these are somehow the first LibKey Actions and more are coming later?
There was a problem hiding this comment.
Ah, yeah, that is ambiguous. It was intended to mean the first matching libkey-actions container in the DOM query. I think libkeyActionsContainer might be clearer.
| <% if ThirdIron.enabled? && @browzine.present? && @browzine[:browzine_link].present? %> | ||
| <% 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' } %> | ||
| <%= 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', action_type: 'full_text_options', action_source: 'browzine', overrides_primo: true } %> |
There was a problem hiding this comment.
How confident are we that this is the preferred full text options link?
There was a problem hiding this comment.
Great question. It's probably not.
Back in June, the original full-text options implementation was to add a link to the full record if a record has Browzine links. That's how we ended up with the duplicate 'Full-text options' links, and this preserves the original behavior requested by UXWS.
That said, I'd imagine we'd prefer the SRU link, if available, since it goes directly to the full-text options section of the Primo record.
| <% if @libkey[:pdf_link].present? || @libkey[:html_link].present? %> | ||
| <% if @libkey[:pdf_link].present? %> | ||
| <%= link_to( @libkey[:pdf_link][:text], @libkey[:pdf_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[:pdf_link][:text] } ) %> | ||
| <%= link_to( @libkey[:pdf_link][:text], @libkey[:pdf_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[:pdf_link][:text], action_type: 'full_text', action_source: 'libkey', overrides_primo: true } ) %> |
There was a problem hiding this comment.
What is the affect of pdf_links and html_links both having the same action_type and action_source and both overriding Primo? Are we losing PDF or HTML or is this somehow retaining both?
There was a problem hiding this comment.
The override refers specifically to 'full-text options' links. Basically, if we have a better fulfillment link to show (PDF and/or HTML), we show it instead of 'full-text options' or whatever might be included in result[:links]. If both PDF and HTML links are available, both would show. We're not suppressing them.
I'm wondering now, though, if I'm prematurely optimizing. I suspect we might get pushback for suppressing any links. Personally, I think it's good UX to streamline the CTA options, but I realize it's not my call to make.
- Remove Primo link suppression - Clarify docs - Prefer full-text options coming from Alma or Primo during dedupe - Use instance variable instead of attr_reader for clarity
|
@JPrevost Would you mind reviewing the latest commit before we merge? I know your comments were non-blocking, but they were thoughtful improvements, and I want to make sure we get this right. (Or, as right as we can, given what we currently know.) Once this lands in staging, I'm planning to ask Darcy for QA just to be sure it's working the way UXWS expects. |
Why these changes are being introduced:
The logic to resolve which fulfillment links to
display lives mostly in the view layer. As this
feature grows in complexity, the logic becomes
harder to understand, and bugs creep in. Most
recently, we noticed that two full-text options
links can appear for a given record.
A more idiomatic approach would be to shift the
logic to the model layer, thus making it more
visible, testable, and hopefully more
understandable. We also need to refactor the
Content Loader controller to make certain actions
explicit and guard against duplication.
Relevant ticket(s):
How this addresses that need:
Primo links when they are not needed (e.g., a
link is provided by Browzine/LibKey/etc).
Side effects of this change:
Complexity is not quite reduced as much as it is
redistributed to the new Fulfillment Policy class. However, the goal of this change is less to reduce complexity and more to improve understandability.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
All expected fulfillment links, including 'full-text options', should appear for a given record with no duplicates. Since this is a refactor, I'd also like the reviewer's opinion on whether the new approach is an improvement (or not).
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing