Bug 12261 - Staff client next/previous links lead to unknown record
Summary: Staff client next/previous links lead to unknown record
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Juhani Seppälä
QA Contact: Testopia
URL:
Keywords:
Depends on: 10404
Blocks: 12481
  Show dependency treegraph
 
Reported: 2014-05-15 09:16 UTC by Fridolin Somers
Modified: 2015-06-04 23:33 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12261: Staff client next/previous links lead to unknown record (2.34 KB, patch)
2014-06-17 10:16 UTC, Juhani Seppälä
Details | Diff | Splinter Review
Bug 12261: Staff client next/previous links lead to unknown record (1.88 KB, patch)
2014-06-17 13:53 UTC, Juhani Seppälä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12261: Staff client next/previous links lead to unknown record (2.11 KB, patch)
2014-06-18 15:03 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 12261: Staff client next/previous links lead to unknown record (2.12 KB, patch)
2014-06-20 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Staff client next/previous links lead to unknown record (4.07 KB, patch)
2014-06-25 08:54 UTC, Juhani Seppälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2014-05-15 09:16:20 UTC
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).
Comment 1 Juhani Seppälä 2014-06-16 12:41:37 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?
Comment 2 Fridolin Somers 2014-06-16 15:59:47 UTC
(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.
Comment 3 Juhani Seppälä 2014-06-17 10:16:21 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2014-06-17 12:34:02 UTC
(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.
Comment 5 Owen Leonard 2014-06-17 12:49:06 UTC
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.
Comment 6 Juhani Seppälä 2014-06-17 13:42:46 UTC
(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?
Comment 7 Juhani Seppälä 2014-06-17 13:53:13 UTC Comment hidden (obsolete)
Comment 8 Juhani Seppälä 2014-06-17 13:59:28 UTC
Added small changes anyway - this one works for me on both single-page search results & larger search results.
Comment 9 Owen Leonard 2014-06-18 15:03:23 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-06-20 13:22:40 UTC
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>
Comment 11 Tomás Cohen Arazi 2014-06-21 22:04:02 UTC
Patch pushed to master.

Thanks Juhani, and congratulations for your first commit.
Comment 12 Galen Charlton 2014-06-24 16:00:32 UTC
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.
Comment 13 Juhani Seppälä 2014-06-25 06:05:34 UTC
I can reproduce what you've described - a bit embarassing for a first submit. I'm sorry for the headache this gives to anyone.
Comment 14 Jonathan Druart 2014-06-25 08:12:41 UTC
(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.
Comment 15 Juhani Seppälä 2014-06-25 08:54:59 UTC
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.
Comment 16 Jonathan Druart 2014-06-25 09:30:33 UTC
(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 :)
Comment 17 Juhani Seppälä 2014-06-25 09:49:57 UTC
(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.