Summary: | Staff client next/previous links lead to unknown record | ||
---|---|---|---|
Product: | Koha | Reporter: | Fridolin Somers <fridolin.somers> |
Component: | Templates | Assignee: | Juhani Seppälä <jseppal> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | gmcharlt, jonathan.druart, jseppal, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11817 | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | 10404 | ||
Bug Blocks: | 12481 | ||
Attachments: |
Bug 12261: Staff client next/previous links lead to unknown record
Bug 12261: Staff client next/previous links lead to unknown record [SIGNED-OFF] Bug 12261: Staff client next/previous links lead to unknown record Bug 12261: Staff client next/previous links lead to unknown record Staff client next/previous links lead to unknown record |
Description
Fridolin Somers
2014-05-15 09:16:20 UTC
Hello, I looked into this issue: if (me.curPos === me.searchCookie.results.length && me.searchCookie.total == me.curPos + me.searchCookie.offset) { nextbutton = '<span id="browse-next" class="browse-button">' + me.curPos + ' ' + me.searchCookie.results.length + ' »</span>'; } else { nextbutton = '<a href="#" id="browse-next" class="browse-button">' + me.curPos + ' ' + me.searchCookie.results.length + ' »</a>'; } To me it looks like an off-by-one issue with searchCookie.results.length. searchCookie.total seems to also be returning undefined leading that if-clause evaluating to false. I'm still pretty new to Koha development and am trying still to get a hold of things - may I propose a fix for this issue? (In reply to Juhani Seppälä from comment #1) > may I propose a fix for this issue? Hello and welcome to Koha. Your fixe will be most welcome. Created attachment 28882 [details] [review] Bug 12261: Staff client next/previous links lead to unknown record When using Staff client next/previous links after a search : If the current record is the last of the results, clicking on "next" will lead to the page of an unknown record with message : The record you requested does not exist (NaN). To test: 1) Do an intranet catalog search that has more than 1 results. 2) Click on the last search result and then click the "Next"-button from the top-left navigation. 3) Confirm that you get thrown to a page with the message: "The record you requested does not exist (NaN).". 4) Apply patch. 5) Repeat steps 1 - 2. 6) Confirm that the navigation button for "Next" is greyed out. (In reply to Juhani Seppälä from comment #3) > Created attachment 28882 [details] [review] [review] Thanks for the patch Juhani! I'm assigning the bug to you (since you submitted a patch) and setting the status of the bug to "Needs signoff" so that the bug shows up in the queue of bugs to be tested. Testing in master I get a different buggy behavior than is described in this bug report. 1) Did a search in the staff client which returned a few pages of results. 2) Clicked on the last page of results. 3) Clicked on the last result on the last page. 4) Clicked on the "Next" button. 5) Instead of getting an error, I'm redirected to a record which originally appeared on the first page of the search results returned in step 1. It looks like I'm redirected to the 12th record in particular. I get the same behavior before and after the patch. Tested in FF30 and Chrome 35 on Windows 7. (In reply to Owen Leonard from comment #5) > Testing in master I get a different buggy behavior than is described in this > bug report. > > 1) Did a search in the staff client which returned a few pages of results. > 2) Clicked on the last page of results. > 3) Clicked on the last result on the last page. > 4) Clicked on the "Next" button. > 5) Instead of getting an error, I'm redirected to a record which originally > appeared on the first page of the search results returned in step 1. It > looks like I'm redirected to the 12th record in particular. > > I get the same behavior before and after the patch. Tested in FF30 and > Chrome 35 on Windows 7. Ah, yes, sorry. Looks like I didn't properly test the patch with big search results (multi-page). I can take another look at this later - should the status be changed for this bug? Created attachment 28907 [details] [review] Bug 12261: Staff client next/previous links lead to unknown record When using Staff client next/previous links after a search : If the current record is the last of the results, clicking on "next" will lead to the page of an unknown record with message : The record you requested does not exist (NaN). To test: 1) Do an intranet catalog search that has more than 1 results. 2) Click on the last search result and then click the "Next"-button from the top-left navigation. 3) Confirm that you get thrown to a page with the message: "The record you requested does not exist (NaN).". 4) Apply patch. 5) Repeat steps 1 - 2. 6) Confirm that the navigation button for "Next" is greyed out. Added small changes anyway - this one works for me on both single-page search results & larger search results. Created attachment 28944 [details] [review] [SIGNED-OFF] Bug 12261: Staff client next/previous links lead to unknown record When using Staff client next/previous links after a search : If the current record is the last of the results, clicking on "next" will lead to the page of an unknown record with message : The record you requested does not exist (NaN). To test: 1) Do an intranet catalog search that has more than 1 results. 2) Click on the last search result and then click the "Next"-button from the top-left navigation. 3) Confirm that you get thrown to a page with the message: "The record you requested does not exist (NaN).". 4) Apply patch. 5) Repeat steps 1 - 2. 6) Confirm that the navigation button for "Next" is greyed out. Signed-off-by: Owen Leonard <oleonard@myacpl.org> This patch fixes the problem with last result in both single pages of search results and multiple pages of search results. Created attachment 29100 [details] [review] Bug 12261: Staff client next/previous links lead to unknown record When using Staff client next/previous links after a search : If the current record is the last of the results, clicking on "next" will lead to the page of an unknown record with message : The record you requested does not exist (NaN). To test: 1) Do an intranet catalog search that has more than 1 results. 2) Click on the last search result and then click the "Next"-button from the top-left navigation. 3) Confirm that you get thrown to a page with the message: "The record you requested does not exist (NaN).". 4) Apply patch. 5) Repeat steps 1 - 2. 6) Confirm that the navigation button for "Next" is greyed out. Signed-off-by: Owen Leonard <oleonard@myacpl.org> This patch fixes the problem with last result in both single pages of search results and multiple pages of search results. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Patch pushed to master. Thanks Juhani, and congratulations for your first commit. While evaluating this for inclusion in 3.16.x, I ran into an apparent regression. Consider a search that spans multiple pages in the results list. When I go to last hit in the first page, the next link is disabled. It should instead be active and clicking on it should permit traveling to the first hit on the second page of results. I can reproduce what you've described - a bit embarassing for a first submit. I'm sorry for the headache this gives to anyone. (In reply to Galen Charlton from comment #12) > While evaluating this for inclusion in 3.16.x, I ran into an apparent > regression. > > Consider a search that spans multiple pages in the results list. When I go > to last hit in the first page, the next link is disabled. It should instead > be active and clicking on it should permit traveling to the first hit on the > second page of results. Yep, sorry no to catch that before. I have just retested and recreated the issue. It seems that the me.searchCookie.total variable is always undefined, maybe a lead. Created attachment 29207 [details] [review] Staff client next/previous links lead to unknown record When using Staff client next/previous links after a search : If the current record is the last of the results, clicking on "next" will lead to the page of an unknown record with message : The record you requested does not exist (NaN). To test: 1) Do an intranet catalog search that has more than 1 results. 2) Click on the last search result and then click the "Next"-button from the top-left navigation. 3) Confirm that you get thrown to a page with the message: "The record you requested does not exist (NaN).". 4) Apply patch. 5) Do an intranet search with more than 1 but less results than fit on a single page. 6) Click on the last result on the page and confirm that the "Next" navigation button is greyd out. 6) Do an intranet search with multi-page search results. 7) Click on the last result of a page that's not the final page of the results & confirm that "Next" is not grey out. 8) Navigate to the last page of the results and click on the final result & confirm that "Next" is greyd out. (In reply to Juhani Seppälä from comment #15) > Created attachment 29207 [details] [review] [review] Please reopen a new bug report and link it to this one. > When using Staff client next/previous links after a search : > If the current record is the last of the results, clicking on "next" will > lead to the page of an unknown record with message : > > The record you requested does not exist (NaN). This commit message is wrong, it is not the behavior on master since the patch from this bug report is pushed :) (In reply to Jonathan Druart from comment #16) > (In reply to Juhani Seppälä from comment #15) > > Created attachment 29207 [details] [review] [review] [review] > > Please reopen a new bug report and link it to this one. > > > When using Staff client next/previous links after a search : > > If the current record is the last of the results, clicking on "next" will > > lead to the page of an unknown record with message : > > > > The record you requested does not exist (NaN). > > This commit message is wrong, it is not the behavior on master since the > patch from this bug report is pushed :) Will open a new one for it - thanks for your patience. |