Bug 10856 - Improve the previous and next items on the shelf browser
Summary: Improve the previous and next items on the shelf browser
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 5500 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-09-10 11:51 UTC by Jonathan Druart
Modified: 2014-12-07 20:02 UTC (History)
4 users (show)

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


Attachments
MT11899: Improve the previous and next items on the shelf browser (2.30 KB, patch)
2013-09-10 14:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10856: Improve the previous and next items on the shelf browser (2.25 KB, patch)
2013-09-10 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10856: Improve the previous and next items on the shelf browser (40.81 KB, patch)
2013-09-12 07:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10856: If callnumbers are equals, order should be on itemnumbers (2.03 KB, patch)
2013-09-13 07:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10856: Improve the previous and next items on the shelf browser (40.91 KB, patch)
2013-09-17 12:50 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10856: If callnumbers are equals, order should be on itemnumbers (2.12 KB, patch)
2013-09-17 12:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser (4.71 KB, patch)
2013-09-17 12:51 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser (4.74 KB, patch)
2013-09-18 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10856: Improve the previous and next items on the shelf browser (40.92 KB, patch)
2013-09-22 18:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10856: If callnumbers are equals, order should be on itemnumbers (2.14 KB, patch)
2013-09-22 18:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser (5.21 KB, patch)
2013-09-22 18:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10856: Fix cover display in shelf browser (4.71 KB, patch)
2013-09-27 09:52 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10856: Fix cover display in shelf browser (4.80 KB, patch)
2013-09-30 16:43 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 10856: Improve the previous and next items on the shelf browser (40.95 KB, patch)
2013-09-30 17:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10856: If callnumbers are equals, order should be on itemnumbers (2.14 KB, patch)
2013-09-30 17:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser (5.27 KB, patch)
2013-09-30 17:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10856: Fix cover display in shelf browser (4.90 KB, patch)
2013-09-30 17:42 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-09-10 11:51:56 UTC
The new and previous links should be a link to the new and previous items in the list.
Comment 1 Katrin Fischer 2013-09-10 11:54:53 UTC
Hi Jonathan, I am not sure I understand the intended change?
Isn't that how it works now? Showing you what's next and previous on the shelf?
Comment 2 Jonathan Druart 2013-09-10 14:45:30 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-09-10 14:46:08 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-09-10 14:48:45 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Jonathan, I am not sure I understand the intended change?
> Isn't that how it works now? Showing you what's next and previous on the
> shelf?

I agree with you, the description is not clear...
In fact the patch simply increments by 1 the gap.
Comment 5 Owen Leonard 2013-09-10 16:42:19 UTC
This still doesn't work the way I expect it should. Perhaps it's not working correctly, or perhaps we have different expectations.

I think that if I start from here:

[<] [1] [2] [3] [4] [5] [6] [>]

...and click [>], I should see:

[<] [7] [8] [9] [10] [11] [12] [>]

This is the behavior one finds on, for example, Amazon.com with their "Customers Who Bought This Item Also Bought" feature.

After applying this patch I go from this:

[<] [1] [2] [3] [4] [5] [6] [>]

...to this:

[<] [4] [5] [6] [7] [8] [9] [>]

...and that doesn't make sense to me.

I don't want to push my luck, but could a solution to Bug 5500 be rolled into this one?
Comment 6 Jonathan Druart 2013-09-12 07:44:06 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2013-09-12 07:45:01 UTC
(In reply to Owen Leonard from comment #5)
> This still doesn't work the way I expect it should. Perhaps it's not working
> correctly, or perhaps we have different expectations.
> 
> I think that if I start from here:
> 
> [<] [1] [2] [3] [4] [5] [6] [>]
> 
> ...and click [>], I should see:
> 
> [<] [7] [8] [9] [10] [11] [12] [>]

The last patch implements this behavior :)
Comment 8 Jonathan Druart 2013-09-12 07:45:33 UTC
(In reply to Owen Leonard from comment #5)
> I don't want to push my luck, but could a solution to Bug 5500 be rolled
> into this one?

And it fixes bug 5500
Comment 9 Jonathan Druart 2013-09-12 07:46:07 UTC
*** Bug 5500 has been marked as a duplicate of this bug. ***
Comment 10 Owen Leonard 2013-09-12 16:09:27 UTC
This is great stuff--I'm really excited about this patch! However, I'm still seeing some weird behavior from the paging buttons:

With JavaScript disabled, the "next" button appears to work the same as the "previous" button.

With JavaScript enabled, I think the calculations of the next and previous is not being calculated correctly. Here are some results from my test. In this example "787" represents the starting item number, the item on which the "browse shelf" link was clicked. Each row of numbers shows the item numbers in the set of results.

+---------------+------+-------+-------+------+------+------+------+
| Starting set: | 8164 | 10263 | 10508 |  787 |  793 |  798 |  802 |
| Next set:     |  798 |   793 |   787 | 1281 | 1283 | 1284 | 1286 |
| Next set:     |  798 |   793 |   797 | 1318 | 1322 | 1324 | 1326 |
+---------------+------+-------+-------+------+------+------+------+

For reference, here are the relevant item numbers from my database in order starting with 787:

+------------+----------+----------------+
| itemnumber | cn_sort  | itemcallnumber |
+------------+----------+----------------+
|        787 | J_ABBOTT | J Abbott       |
|        793 | J_ABBOTT | J Abbott       |
|        798 | J_ABBOTT | J Abbott       |
|        802 | J_ABBOTT | J Abbott       |
|        803 | J_ABBOTT | J Abbott       |
|       1278 | J_ABBOTT | J Abbott       |
|       1280 | J_ABBOTT | J Abbott       |
|       1281 | J_ABBOTT | J Abbott       |
|       1283 | J_ABBOTT | J Abbott       |
|       1284 | J_ABBOTT | J Abbott       |
|       1286 | J_ABBOTT | J Abbott       |
|       1288 | J_ABBOTT | J Abbott       |
|       1289 | J_ABBOTT | J Abbott       |
|       1291 | J_ABBOTT | J Abbott       |
|       1318 | J_ABBOTT | J Abbott       |
|       1322 | J_ABBOTT | J Abbott       |
|       1324 | J_ABBOTT | J Abbott       |
|       1326 | J_ABBOTT | J Abbott       |
+------------+----------+----------------+

Looks like the right-hand side of the results is being calculated correctly but not the left hand.
Comment 11 Jonathan Druart 2013-09-13 07:58:13 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-09-13 08:00:16 UTC
(In reply to Owen Leonard from comment #10)
> This is great stuff--I'm really excited about this patch! However, I'm still
> seeing some weird behavior from the paging buttons:
> 
> With JavaScript disabled, the "next" button appears to work the same as the
> "previous" button.

Oops, bad c/p. Fixed!

> Looks like the right-hand side of the results is being calculated correctly
> but not the left hand.

This already seems to exist on master. The query did not sort by itemnumbers DESC on the left side. Should be fixed.
Comment 13 Owen Leonard 2013-09-17 12:50:47 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2013-09-17 12:50:57 UTC Comment hidden (obsolete)
Comment 15 Owen Leonard 2013-09-17 12:51:35 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-09-18 14:53:25 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-09-22 18:38:05 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2013-09-22 18:38:16 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2013-09-22 18:38:27 UTC Comment hidden (obsolete)
Comment 20 Galen Charlton 2013-09-23 18:38:43 UTC
This is close, but I found a regression when local cover images are enabled.  Specifically, if you bring up the shelf browser, then travel on it in either direction, local covers aren't displayed in the shelf browser.

Unfortunately, just adding "KOHA.LocalCover.GetCoverFromBibnumber(true);" to the AJAX success handler isn't enough, as doing that also causes a duplicate of the main record's local cover image to be inserted each time you advance the shelf browser.

I have not yet tested with any other cover image provider.

I should emphasize that I really like the idea of this patch; it just needs to handle cover images better.
Comment 21 Jonathan Druart 2013-09-24 12:08:17 UTC
(In reply to Galen Charlton from comment #20)
> This is close, but I found a regression when local cover images are enabled.
> Specifically, if you bring up the shelf browser, then travel on it in either
> direction, local covers aren't displayed in the shelf browser.
> 
> Unfortunately, just adding "KOHA.LocalCover.GetCoverFromBibnumber(true);" to
> the AJAX success handler isn't enough, as doing that also causes a duplicate
> of the main record's local cover image to be inserted each time you advance
> the shelf browser.
> 
> I have not yet tested with any other cover image provider.
> 
> I should emphasize that I really like the idea of this patch; it just needs
> to handle cover images better.

Galen,
I cannot reproduce your issue.
The pref OPACLocalCoverImages is enabled, I go on a detail page, click on "Browse shelf".
I play with the left/right arrow and the local cover image still appears.
Comment 22 Owen Leonard 2013-09-25 14:38:45 UTC
> > Specifically, if you bring up the shelf browser, then travel on it in either
> > direction, local covers aren't displayed in the shelf browser.

I can reproduce this problem in Firefox and Chrome on Windows. If I start from a record that has a local cover, the local cover image appears when I first open the shelf browser. If I click forward and then back again, the local cover no longer appears where it did.
Comment 23 Julian Maurice 2013-09-27 09:52:04 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2013-09-27 11:34:19 UTC
Thanks Julian!
Comment 25 I'm just a bot 2013-09-29 06:23:19 UTC
Patch applied cleanly, go forth and signoff
Comment 26 Owen Leonard 2013-09-30 16:43:12 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2013-09-30 17:41:45 UTC
Created attachment 21670 [details] [review]
[PASSED QA] Bug 10856: Improve the previous and next items on the shelf browser

The next and previous links should completely refresh the shelf.

For example:
[<] [1] [2] [3] [4] [5] [6] [>]
Before this patch, the next and previous links were the same as the 1
and 6.
With this patch, after clicking on next, we will get:
[<] [7] [8] [9] [10] [11] [12] [13] [>]

This patch adds a new ajax script to get the shelf browser block.

Test plan:
- On a detail biblio page, click on a "Browse shelf" link.
- Play with the next and previous links.
- Deactivate Javascript (using NoScript for example) and check that you
  get the same behavior (but the page is reloaded).
- Launch the unit tests: prove t/db_dependent/ShelfBrowser.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
Comment 28 Katrin Fischer 2013-09-30 17:41:58 UTC
Created attachment 21671 [details] [review]
[PASSED QA] Bug 10856: If callnumbers are equals, order should be on itemnumbers

If a lot of items has the same callnumber, the order should be on the
itemnumbers. Otherwise the left side is always filled with the same
items.

+ Fix a bad c/p for the next link (when js is disabled).

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 29 Katrin Fischer 2013-09-30 17:42:08 UTC
Created attachment 21672 [details] [review]
[PASSED QA] Bug 10856 [Follow-up] Improve the previous and next items on the shelf browser

In Firefox at least, the shelf browser cannot be reopened after
hiding it with the "close shelf browser" link. This followup improves
the behavior of the "close shelf browser" link so that the shelf browser
can be redisplayed.

To test, open a bibliographic detail page in the OPAC and click a
"browse shelf" link. Click the "close shelf browser" link--the shelf
browser should be hidden. Click the original "browse shelf" link and the
shelf browser should reappear without reloading the page.

Test with Firefox and Chrome (at least).

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and QA script pass.

Testing notes:
- New unit tests in t/db_dependent/ShelfBrowser.t pass.
- System preference  OPACShelfBrowser still works as expected.
- Closing and opening the shelf browser works as expected.
- Next and Previous links show new and nicer behaviour.
- Logs are clean.

Tested with Firefox and Chromium under Ubuntu.

Notes: The currently displayed record could maybe be highlighted
a bit better.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 30 Katrin Fischer 2013-09-30 17:42:38 UTC
Created attachment 21673 [details] [review]
[PASSED QA] Bug 10856: Fix cover display in shelf browser

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
I was able to reproduce the problem with local covers and
the patch fixes it in my tests.
Comment 31 Galen Charlton 2013-10-04 16:02:47 UTC
Pushed to master.  Thanks, Jonathan!