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
MBS-10478: Show useful message if too large search result page requested #1930
Conversation
The reported ticket is as a bug (because of saying "no results" where results exist). Would you make it into an improvement to redirect, or keep as a bug? |
@brainzbot, retest this please |
Hmm. I like the intent to do something useful here, though in the back of my mind I'm worrying about two things
Since I can't imagine this situation is all that common outside of clicking on an old/saved link, I wonder if it wouldn't be better to just show an error with a link to repeat the search (which will at least be rate limited) if they want to. |
Hmm. I did it like this because we already have pagination elsewhere that goes to the last available page if too large a number is given. If you think it's a worry for the search server, I guess we could just display "This search only has X pages, {link|go to last}?" instead or something. It's less comfortable/helpful, but it might be worth it for safety? |
I'd prefer displaying a message, personally. I think this situation isn't that common, so it shouldn't be too much of an inconvenience. I worry about the current patch making it more trivial for a malicious actor to overload the search server. |
Changed to show a message (example on PR description) |
$show_largest_available_page ? ( | ||
largest_available_page => $largest_available_page | ||
) : () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to determine all of this from the pager given to the component already? e.g. there's pager.last_page
, does that match $largest_available_page
? And does pager.current_page
match $page
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pager.last_page
does give the same as I needed, yes. pager.current_page
doesn't show the passed parameter though, it is the same as pager.last_page
if I pass any page number above that number. I could work with that just fine and take the page from $c.req.uri, the only problem is that pager.last_page
is 1 even if there's no results at all (including in page 1) so if I ask for page 2 of a no-results search, I'd still think "the last page of results is 1". With the ceil()
calculation no results = 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also pager.total_entries
-- maybe that can let you distinguish a page 1 with no results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That absolutely does it, so this is a looot simpler now. Thanks!
This currently loads the page and just says "no results", which is quite misleading and also useless. We have all the info to show something useful, so we should do that and at least tell the user which page is the last they can go to.
* master: Update POT files using the production database Update translations from Transifex MBS-11758: Fix more hydration errors due to missing strings t/hydration_i18n.t: fix l() function regexp Remove some unnecessary line breaks MBS-11303: Access $c via React context APIs Remove action={$c.req.uri} Add entities.mjs & generate entities.json from it Pass entities.json through jq --indent 4 Update tests image for CircleCI after MBS-12527 Regenerate cpanfile.snapshot files after MBS-12527 Prevent future inconsistencies in alias locales Fix eslint rule import/extensions (#2598) Add missing extensions to PaginatedSearchResults MBS-10478: Show useful message if too large search result page requested (#1930) MBS-12527: Update to DateTime::Locale 1.35 Fix locale dumping script to use MBS Constants Use descriptive variables names in two places .flowconfig: Set enforce_class_annotations=true .flowconfig: Set enforce_this_annotations=true .flowconfig: Set enforce_local_inference_annotations=true Move displayCoverArtTypes to module scope Recommend Node.js version in INSTALL.md MBS-12525: Add fallback sucrase loader for Node.js < 16.12.0 .flowconfig: Set inference_mode=constrain_writes Upgrade Flow to 0.184.0
Implement MBS-10478
This currently loads the page and just says "no results", which is quite misleading and also useless. We have all the info to show something useful, so we should do that and at least tell the user which page is the last they can go to.