Bug 12481

Summary: Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into
Product: Koha Reporter: Juhani Seppälä <jseppal>
Component: TemplatesAssignee: Juhani Seppälä <jseppal>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: jonathan.druart+koha, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12510
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12261    
Bug Blocks: 13265    
Attachments: Bug 12481: Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into
Bug 12481: Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into
Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into
Bug 12481: Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into
Bug 12481 - Follow up: Link to results is not refreshed when item is on next or prev result page
Bug 12481: Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into
Bug 12481: Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into

Description Juhani Seppälä 2014-06-25 10:26:30 UTC
When doing a staff client catalog search with more than 1 page of results and clicking the last result on any search result page, the top-left navigation button for "next" is greyed out.

The expected behaviour would be to move into the next page of results. The "next" navigation should only be prevented when the very last search result is selected in the detail-view. 

As suggested in the linked bug 12261, this is a regression from my attempt to fix the issue with the navigation letting the user moving out of bound in relation to the result array. As suggested in bug 12261, the original cause was in part due to the total number of results not being carried in the search cookie. Secondly, the offset value that was being used in the navigation checks shouldn't be used in there (the checks for allowing movement within the results) because the cookie value is only updated upon a result click and not when the user is navigating using the detail-view.

I wrongly submitted a patch to bug 12261 since it was already pushed. The patch brings the total number of search results available (as I think it was orignally planned, since the non-existing value was being used) to the browser.js which handles the navigation and makes use of that.

I marked this as blocking 12261 - not sure if this is right...
Comment 1 Juhani Seppälä 2014-06-25 10:51:32 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-06-25 11:07:19 UTC
Does this patch needs signoff?
If yes, you missed to change the status :)
And if yes, I will change the status to failed QA, because the patch does not work if you get more than 1 page of results.
me.curPos is the position of the result on the current page, not the position on the total number of results.
Comment 3 Juhani Seppälä 2014-06-25 12:03:09 UTC
(In reply to Jonathan Druart from comment #2)
> Does this patch needs signoff?
> If yes, you missed to change the status :)
> And if yes, I will change the status to failed QA, because the patch does
> not work if you get more than 1 page of results.
> me.curPos is the position of the result on the current page, not the
> position on the total number of results.

You're right, I'm obviously a bit confused with regards to the fundamental purpose of some of the js I'm looking at. However, think that making the total results available to the navigation js is a starting point.
Comment 4 Juhani Seppälä 2014-06-26 07:55:49 UTC Comment hidden (obsolete)
Comment 5 Juhani Seppälä 2014-06-26 08:23:05 UTC
Newest patch does not apply - does not apply to master, attaching again.
Comment 6 Juhani Seppälä 2014-06-26 08:35:00 UTC Comment hidden (obsolete)
Comment 7 Juhani Seppälä 2014-06-26 08:37:27 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2014-06-28 21:04:06 UTC
I suppose that the first patch is obsolete? Seems to be the same code.

I applied the second patch, did a search wit a couple of pages as result, went to the first detail from one of the result pages and clicked "Next" until the end of this page. Then it seems that I am "trapped" on this result page, after the last entry the first entry of the same result page is displayed (should be the first entry of the next page).
Comment 9 Juhani Seppälä 2014-06-30 06:18:38 UTC
(In reply to Marc Véron from comment #8)
> I suppose that the first patch is obsolete? Seems to be the same code.
> 
> I applied the second patch, did a search wit a couple of pages as result,
> went to the first detail from one of the result pages and clicked "Next"
> until the end of this page. Then it seems that I am "trapped" on this result
> page, after the last entry the first entry of the same result page is
> displayed (should be the first entry of the next page).

This is not the only interesting behaviour I've come across with the navigation. There's another issue where the results of the page get concatenated infinitely on page reload, which originally led me to believe the results-array in the cookie was the whole results set. These should probably be reported separately? The behaviour you're describing should be unrelated to my patch and I've come across it before doing any changes. Can't be 100%, obviously.

Related js (which I have not touched):

    var browseRecords = function (movement) {
        var newSearchPos = me.curPos + movement;
        if (newSearchPos > me.searchCookie.results.length - 1) {
            window.location = '/cgi-bin/koha/catalogue/search.pl?' + decodeURIComponent(me.searchCookie.query) + '&limit=' + decodeURIComponent(me.searchCookie.limit) + '&sort=' + me.searchCookie.sort + '&gotoPage=detail.pl&gotoNumber=first&searchid=' + me.searchid + '&offset=' + newSearchPos;
        } else if (newSearchPos < 0) {
            window.location = '/cgi-bin/koha/catalogue/search.pl?' + decodeURIComponent(me.searchCookie.query) + '&limit=' + decodeURIComponent(me.searchCookie.limit) + '&sort=' + me.searchCookie.sort + '&gotoPage=detail.pl&gotoNumber=last&searchid=' + me.searchid + '&offset=' + (me.offset - me.searchCookie.pagelen);
        } else {
            window.location = window.location.href.replace('biblionumber=' + biblionumber, 'biblionumber=' + me.searchCookie.results[newSearchPos]);
        }
    }
Comment 10 Marc Véron 2014-06-30 14:52:49 UTC
Comment on attachment 29275 [details] [review]
Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into

Obsoleting patch #1 (suppose it is replaced by patch #2)
Comment 11 Marc Véron 2014-06-30 18:58:14 UTC Comment hidden (obsolete)
Comment 12 Juhani Seppälä 2014-07-01 06:34:11 UTC
(In reply to Marc Véron from comment #11)
> Created attachment 29386 [details] [review] [review]
> Bug 12481 - Follow up: Link to results is  not refreshed when item is on
> next or prev result page
> 
> Test plan for this follow up
> 
> Reproduce issue without patch:
> 
> Do a search that results in 3 or more pages.
> Print out list or do the same search in an other browser (to have a
> reference for the following)
> Go to a detail view and step through details to a detail that ist on result
> #3
> Click on Link "Return to details"
> Result: The first page will display (instead of page #2)
> 
> With patch, the correct page should display.

Hi, I'm unable to reproduce what you're describing with only my patch applied. What I'm doing:
1. Do a search with 3 or more pages of results.
2. Select a result from the first page and navigate into the third page using the detail-view.
3. Select "return to results" on a results on the third page.
Comment 13 Marc Véron 2014-07-01 11:19:42 UTC
Comment on attachment 29386 [details] [review]
Bug 12481 - Follow up: Link to results is  not refreshed when item is on next or prev result page

Hi Juhani,
Strange, I tested again with your patch only and can not longer reproduce the behaviour. I used a result set with about 10 pages. Yesterday, I had two tabs open, in one I had the result list (>10 pages) and in the other, I stepped through the single views andgot the odd behaviour.

I obsolete now my follow up patch and will sign-off your patch.
Comment 14 Marc Véron 2014-07-01 11:25:31 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2014-07-01 11:31:22 UTC
(In reply to Juhani Seppälä from comment #9)

(...)
> This is not the only interesting behaviour I've come across with the
> navigation. There's another issue where the results of the page get
> concatenated infinitely on page reload, which originally led me to believe
> the results-array in the cookie was the whole results set. These should
> probably be reported separately?

I propose that you report this separately.
Comment 16 Juhani Seppälä 2014-07-01 11:43:10 UTC
(In reply to Marc Véron from comment #15)
> (In reply to Juhani Seppälä from comment #9)
> 
> (...)
> > This is not the only interesting behaviour I've come across with the
> > navigation. There's another issue where the results of the page get
> > concatenated infinitely on page reload, which originally led me to believe
> > the results-array in the cookie was the whole results set. These should
> > probably be reported separately?
> 
> I propose that you report this separately.

Thanks for looking into this Marc and helping with my regression. I should've been more precise there: it's the result array in the search cookie that's getting concatenated needlessly on page reloads - not sure if this can actually cause any harm currently, but the concatenation on page reloads doesn't look intentional in any case. I can create a new bug for this.
Comment 17 Jonathan Druart 2014-07-01 14:13:36 UTC
Created attachment 29408 [details] [review]
Bug 12481: Staff client detail-view "next" link is greyed out when the last search result of any results page is clicked or navigated into

When doing a staff client catalog search with more than 1 page of results
and clicking the last result on any search result page, the top-left navigation
button for "next" is greyed out.

Tested on newest Firefox & Chromium. Attempts to restore originally planned checks for navigation
with the exception of not using results.length due to buggy behaviour where the results get concatenated
upon page reload & "return to results" navigation - a separate issue?

To test:
1) Do an intranet catalog search that has more than 1 page of results.
2) Click on the last result on a page that is not the last one & confirm that the navigation button "next" is greyed out.
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 greyed out.
6) Do an intranet search with more than 1 page of 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.

http://bugs.koha-community.org/show_bug.cgi?id=12481

This patch prevents the "Next" button on detail view to be grayed out at the end of e result page.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 18 Jonathan Druart 2014-07-01 14:16:24 UTC
(In reply to Juhani Seppälä from comment #16)
> I can create a new bug for this.

Did you create it?
Comment 19 Juhani Seppälä 2014-07-01 14:36:15 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Juhani Seppälä from comment #16)
> > I can create a new bug for this.
> 
> Did you create it?

Yes, was about to post it here for Marc as well: Bug 12510(In reply to Jonathan Druart from comment #18)
> (In reply to Juhani Seppälä from comment #16)
> > I can create a new bug for this.
> 
> Did you create it?

Yes, sorry, was about to post it here for Marc as well, see Bug 12510.
Comment 20 Tomás Cohen Arazi 2014-07-03 13:22:00 UTC
Pushed to master.

Thanks Juhani!