Add pagination support to 'all' tab #280

jazairi · 2025-11-18T20:25:18Z

Why these changes are being introduced:

We intentionally decided to implement pagination for the 'all' tab separately to avoid excessively large tickets.

Relevant ticket(s):

How this addresses that need:

This updates the Search Controller and Analyzer Model to handle pagination from multiple sources ('all' tab) in addition to single sources.

Side effects of this change:

I found it very difficult to follow where and how the 'per-page' value is set. I tried to improve the situation by moving this value to env. I'm not sure it fully makes sense it, but I doubt it will without a full refactor of the data flow architecture.

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod. (N.b. ENV is optional and will be added as needed.)
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

Additional context needed to review

This was originally going to close just USE-178 (basic pagination), but as I worked on it, it made sense to also include USE-179 (pagination edge cases). Please consider any edge cases that are not currently covered in the tests.

Code Reviewer

Code

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

coveralls · 2025-11-18T20:26:56Z

Pull Request Test Coverage Report for Build 19514377591

Details

60 of 60 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 98.023%

Totals
Change from base Build 19513748822:	0.01%
Covered Lines:	942
Relevant Lines:	961

💛 - Coveralls

JPrevost · 2025-11-19T15:11:59Z

README.md

 - `REQUEST_PERIOD` - time in minutes used along with `REQUESTS_PER_PERIOD`
 - `REDIRECT_REQUESTS_PER_PERIOD`- number of requests that can be made that the query string starts with our legacy redirect parameter to throttle per `REQUEST_PERIOD`
 - `REDIRECT_REQUEST_PERIOD`- time in minutes used along with `REDIRECT_REQUEST_PERIOD`
+- `RESULTS_PER_PAGE`: The number of results to display per page. Use an odd number to avoid peculiarities. Defaults to 20 if unset.


Shouldn't this be recommending an even number, not odd?

JPrevost · 2025-11-19T15:15:18Z

app/controllers/search_controller.rb

    @errors = data[:errors]
  end

  def load_all_results


I really like the smaller methods here. It's much easier to read with well named methods.

JPrevost · 2025-11-19T15:20:05Z

app/controllers/search_controller.rb

    current_page = @enhanced_query[:page] || 1
-    per_page = @enhanced_query[:per_page] || 20
+    per_page = if @active_tab == 'all'
+                 ENV.fetch('RESULTS_PER_PAGE',


Non-blocking: this feels like an unnecessary line break that makes it slightly harder to follow

I think that was rubocop's doing, oddly enough. I'll try changing it back.

JPrevost · 2025-11-19T15:26:05Z

app/models/analyzer.rb

  PRIMO_MAX_OFFSET = 960

-  def initialize(enhanced_query, response, source)
+  def initialize(enhanced_query, response, source, secondary_response = nil)


Would you mind adding a docstrings to the methods in this model clarifying what the input variables are? It's clear to me at this point what is going on because I've read the PR, but I fear looking at this in isolation in the future will result in confusion.

Great call!

JPrevost · 2025-11-19T15:31:38Z

app/models/analyzer.rb

    else
-      0
+      @pagination[:hits] = response&.dig(:hits) || 0
+      @pagination[:per_page] = ENV.fetch('RESULTS_PER_PAGE', '20').to_i


No change requested at this time, but we should consider a future refactor to set this once at Rails startup rather than the constant Env.fetch logic. I doubt it would change anything performance-wise, but it would be a lot cleaner code to stop repeating this.

You mean like setting it as a global constant? That would be a big improvement, especially if we ever want to change the default number of results.

I was thinking more Rails custom config or something similar:
https://guides.rubyonrails.org/configuring.html#custom-configuration

So maybe we'd end up with Rails.configuration.timdexui.per_page = ENV.fetch('RESULTS_PER_PAGE', '20').to_i which we could then access throughout the app.

I'm not sure we've done that in any of our apps, but doing that or setting a global constant in an initializer might be worth exploring in the future.

Oh, interesting. I haven't used custom config before, but it looks useful!

JPrevost · 2025-11-19T15:33:55Z

app/models/analyzer.rb

-    return 0 unless response.is_a?(Hash)
-
-    response.dig('info', 'total') || 0
+  def calculate_standard_pagination


I can't see a difference between calculate_standard_pagination and calculate_all_pagination other than one uses next_page which seems to be the same as the logic in the one not using it.

If they are indeed different, please add a docstring explaining the difference as I've read it a few times and am confused :)

Good catch! That was a relic from a previous iteration. Will consolidate.

JPrevost · 2025-11-19T15:41:09Z

test/models/analyzer_test.rb

+    refute pagination.key?(:prev)
+  end
+
+  test 'analyzer handles one API returning zero results for all tab' do


Probably not necessary, but I'd feel better about it... can we make this test analyzer handles second API returning zero results for all tab and then add another test for analyzer handles first API returning zero results for all tab

JPrevost · 2025-11-19T15:43:13Z

test/models/analyzer_test.rb

+    refute pagination.key?(:prev)
+  end
+
+  test 'analyzer handles missing primo response for all tab' do


Maybe this is the test I was asking for above. Shouldn't the pagination call look like:

pagination = Analyzer.new(eq, { hits: 0 }, :all, {hits: 100).pagination

to approximate a missing Primo result or is the order not consistent in the calling code?

This is another relic from an earlier iteration. The order of the args used to matter, but that made me nervous, so I refactored it so it's agnostic. Still, I think your comment above is right: we should test for the absence of the first and second result set, because either is a possible edge case.

jazairi · 2025-11-19T19:35:53Z

@JPrevost Thanks for the great feedback! Could you let me know if the latest commit addresses everything? Looking at this with fresh eyes, I made a couple of additional changes:

The response and secondary_response params have been renamed to hits and secondary_hits. The original param name no longer really makes sense, now that we are only passing hits and not the full response.
I've also changed the Search Controller to pass hits to the Analyzer Model as an integer, not an object. (I'm not sure the key is useful.)
I updated a couple of method names in the Analyzer to hopefully better reflect their functions.

JPrevost

Why these changes are being introduced: We intentionally decided to implement pagination for the 'all' tab separately to avoid excessively large tickets. Relevant ticket(s): - [USE-178](https://mitlibraries.atlassian.net/browse/USE-178) - [USE-179](https://mitlibraries.atlassian.net/browse/USE-179) How this addresses that need: This updates the Search Controller and Analyzer Model to handle pagination from multiple sources ('all' tab) in addition to single sources. Side effects of this change: I found it very difficult to follow where and how the 'per-page' value is set. I tried to improve the situation by moving this value to env. I'm not sure it fully makes sense it, but I doubt it will without a full refactor of the data flow architecture.

jazairi requested review from JPrevost and matt-bernhardt November 18, 2025 20:26

mitlib temporarily deployed to timdex-ui-pi-use-178-p5i4x0q2s November 18, 2025 20:27 Inactive

JPrevost self-assigned this Nov 19, 2025

JPrevost requested changes Nov 19, 2025

View reviewed changes

jazairi temporarily deployed to timdex-ui-pi-use-178-p5i4x0q2s November 19, 2025 19:31 Inactive

JPrevost approved these changes Nov 19, 2025

View reviewed changes

jazairi force-pushed the use-178 branch from b5d3164 to faedd17 Compare November 19, 2025 19:45

jazairi temporarily deployed to timdex-ui-pi-use-178-p5i4x0q2s November 19, 2025 19:46 Inactive

jazairi force-pushed the use-178 branch from faedd17 to c984134 Compare November 19, 2025 19:48

jazairi temporarily deployed to timdex-ui-pi-use-178-p5i4x0q2s November 19, 2025 19:49 Inactive

jazairi merged commit 8e489da into main Nov 19, 2025
5 checks passed

jazairi deleted the use-178 branch November 19, 2025 19:51

Add pagination support to 'all' tab #280

Add pagination support to 'all' tab #280

Uh oh!

Conversation

jazairi commented Nov 18, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why these changes are being introduced:

Relevant ticket(s):

How this addresses that need:

Side effects of this change:

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19514377591

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazairi commented Nov 19, 2025

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jazairi commented Nov 18, 2025 •

edited by atlassian bot

Loading

coveralls commented Nov 18, 2025 •

edited

Loading