Skip to content
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.

Sign up for GitHub

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

Fix record navigation when results are loaded with js #4196

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Fix record navigation when results are loaded with js #4196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions module/VuFind/src/VuFind/AjaxHandler/GetSearchResults.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ protected function getSearchResults(ParamsHelper $requestParams): ?Results
$this->saveSearchToHistory($results);
}

$requestParams->getController()->resultScroller()->init($results);

// Always save search parameters, since these are namespaced by search
// class ID.
$this->searchMemory->rememberParams($results->getParams());
Expand Down
4 changes: 1 addition & 3 deletions module/VuFind/src/VuFind/Controller/AbstractSearch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,9 +423,7 @@ protected function getSearchResultsView($setupCallback = null)
}

// Set up results scroller:
if ($this->resultScrollerActive()) {
$this->resultScroller()->init($results);
}
$this->resultScroller()->init($results);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has the initialization been made unconditional? Isn't this going to cause initialization in contexts where it is supposed to be disabled? And shouldn't we also check for an active result scroller in the AJAX handler?

Copy link
Contributor Author

@ThoWagen ThoWagen Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no obvious way to access the 'resultScrollerActive' method in 'GetSearchResults' because it is configured for each search and record controller.

I figured that always initializing the result scroller does not cause any problems but avoids introducing new complexities. There might be possible applications in the future where the result scroller is used but Record->next_prev_navigation is false.

But if I'm missing something here I can also try to refactor this so that the decision if the scroller should be initialized is made on the results of the searches instead of a method in the search controller.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ThoWagen, I think I'm inclined to agree with you, but I'd love to get another opinion (maybe from @EreMaijala, if and when he has time) in case there's something we're overlooking. A couple of related thoughts:

1.) The only potential disadvantage I can see to unconditionally initializing the scroller is that in a system with a mix of different search types, if a user is performing a mix of different search types in multiple tabs/windows, there is a possibility that the result scroller data will get evicted more quickly, since we only store a limited amount in the session. I'm not sure how likely this scenario is (I suspect not very), but it at least seems like a possible problem.

2.) If we don't conditionally initialize the result scroller results, I think we might be able to eliminate the resultScrollerActive method from all of the search controllers, which would nicely reduce redundancy in the code (I've never liked having to override this in two places to make it work right). But maybe that's also a sign that this decision belongs somewhere else, like in the search params object -- if we moved it there, we could potentially make more decisions in the ResultScroller plugin instead of the controllers, and that might be the best of both worlds. But it would also require more refactoring, and some of the edge cases where the behavior is based on factors other than just the search backend might be ugly. I'm not strongly advocating for this much change, but just brainstorming!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding 1.) We could change that, so that the limit is applied separately for each backend.
Regarding 2.) I'm willing to do refactoring here, since that should certainly be an improvement. But let's see if Ere has an opinion on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding 1.) I think if we have a backend-specific limit, it defeats the purpose of a limit since the goal is to prevent excessive session growth, and a system with a lot of different backends could end up with huge sessions. Maybe this could be offset by having a configurable overall limit and configurable backend-specific limits, but that could get ugly and complicated, and would people actually know to configure it correctly?

Regarding 2.) I agree that we should wait and see what Ere thinks. I know he's having a busy month, though, so we shouldn't expect to hear from him right away. Maybe we should plan to talk about this on the next Community Call if it hasn't been addressed by then, just so we don't lose track of it! I'll give this an 11.0 milestone to ensure it ends up on the agenda.


foreach ($results->getErrors() as $error) {
try {
Expand Down
32 changes: 31 additions & 1 deletion module/VuFind/tests/integration-tests/src/VuFindTest/Mink/NextPrevNavTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class NextPrevNavTest extends \VuFindTest\Integration\MinkTestCase
*
* @return void
*/
public function testEmptySearchResultsCauseNoProblems()
public function testEmptySearchResultsCauseNoProblems(): void
{
$this->changeConfigs(
['config' => ['Record' => ['next_prev_navigation' => true, 'first_last_navigation' => true]]]
Expand All @@ -71,4 +71,34 @@ public function testEmptySearchResultsCauseNoProblems()
$this->findCssAndGetText($page, 'div.media-body > h1[property=name]')
);
}

/**
* If next_prev_navigation and first_last_navigation are set to true
* and a search results have been loaded via JS the navigation should
* be shown in the results.
*
* @return void
*/
public function testJSCauseNoProblems(): void
{
$this->changeConfigs(
['config' => ['Record' => ['next_prev_navigation' => true, 'first_last_navigation' => true]]]
);

// when a search returns no results
// make sure no errors occur when visiting a collection record after
$session = $this->getMinkSession();
$page = $session->getPage();

$session->visit($this->getVuFindUrl() . '/Search/Results?type=AllField');
$this->waitForPageLoad($page);

$this->clickCss($page, '.search-header .pagination-simple .page-next');
$this->waitForPageLoad($page);

$this->clickCss($page, '#result0 a.getFull');
$this->waitForPageLoad($page);

$this->findCss($page, 'nav .pager');
}
}
Loading