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.

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

Do not jump to single search result when jumpto is disabled #3131

Conversation

ThoWagen
Copy link
Contributor

If one has jump_to_single_search_result set to true and only gets one result in a column of combined search one jumps to this record. This should not happen and is fixed in this PR.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @ThoWagen, this is a good catch! However, do we really need to add a blockJumpto parameter? I thought the existing code to set jumpto to false was designed to have the same effect. Could it be we just need to check for a false jumpto in processJumpToOnlyResult and the other changes are unnecessary?

(I'm happy to merge this as-is if there's a reason I'm overlooking for its necessity, but just wondering if we can simplify).

@demiankatz demiankatz added this to the 9.1 milestone Sep 29, 2023
@ThoWagen
Copy link
Contributor Author

I was not sure if there are cases where jumpto is set to false in the query parameter but if jump_to_single_search_result=true there should still be a redirect. This would not be very intuitive but it is the current logic and I only wanted to fix this for combined searches.

I can definitely change this if there are no unwanted side effects.

@demiankatz
Copy link
Member

@ThoWagen, the jumpto parameter as currently implemented should either be a positive integer (the index of the record to jump to), or false (indicating that jumping is explicitly disabled). So I think it's fair to say that jumpto=false serves exactly the same function as your proposed blockJumpTo parameter. The only thing to be careful about in processJumpToOnlyResult is the fact that we only want to block jumping if the parameter is EXPLICITLY set to false, but NOT if it is unset. This could probably be most easily addressed by defaulting to true in processJumpToOnlyResult, e.g.

$jumpto = $this->params()->fromQuery('jumpto', true);
if (
    $jumpto
    && ...

What do you think?

@ThoWagen
Copy link
Contributor Author

That makes sense. I changed it.

@demiankatz demiankatz changed the title Block jumpto in combined searches Do not jump to single search result when jumpto is disabled Sep 29, 2023
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Looks good to me, and passes both the automated test suite and my own manual checks. I've just added a comment to the code to provide some additional context. Thanks again for catching and fixing this!

@demiankatz demiankatz merged commit 972c9d7 into vufind-org:dev Sep 29, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants