Bug 10656 - shelving locations not alpha on search
Summary: shelving locations not alpha on search
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Owen Leonard
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-29 14:22 UTC by Nicole C. Engard
Modified: 2014-12-07 20:02 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
one example (82.32 KB, image/png)
2013-07-29 14:22 UTC, Nicole C. Engard
Details
another example (45.68 KB, image/png)
2013-07-29 14:22 UTC, Nicole C. Engard
Details
Bug 10656 - shelving locations not alpha on search (1.62 KB, patch)
2013-08-01 13:22 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10656 - shelving locations not alpha on search (1.70 KB, patch)
2013-08-01 13:37 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 10656 - shelving locations not alpha on search (1.80 KB, patch)
2013-08-20 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10656: add regression test (4.72 KB, patch)
2013-08-20 17:45 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10656: improve sorting of shelving location and collections on OPAC advanced search form (1.78 KB, patch)
2013-08-20 17:45 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10656: (follow-up) handle OPAC sorting of authvals where lib_opac is NULL (2.07 KB, patch)
2013-08-20 17:45 UTC, Galen Charlton
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10656: add regression test (4.87 KB, patch)
2013-08-20 21:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10656: improve sorting of shelving location and collections on OPAC advanced search form (1.85 KB, patch)
2013-08-20 21:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10656: (follow-up) handle OPAC sorting of authvals where lib_opac is NULL (2.17 KB, patch)
2013-08-20 21:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10656: add regression test (4.91 KB, patch)
2013-09-06 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10656: improve sorting of shelving location and collections on OPAC advanced search form (1.89 KB, patch)
2013-09-06 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10656: (follow-up) handle OPAC sorting of authvals where lib_opac is NULL (2.21 KB, patch)
2013-09-06 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2013-07-29 14:22:26 UTC
On the advanced search page the shelving locations don't seem to be in alphabetical order by description as they should be to match the other tabs.
Comment 1 Nicole C. Engard 2013-07-29 14:22:45 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2013-07-29 14:22:59 UTC
Created attachment 19979 [details]
another example
Comment 3 Nicole C. Engard 2013-07-29 14:23:13 UTC
Tested in 3.10.6 and master.
Comment 4 Nicole C. Engard 2013-08-01 12:03:42 UTC
Comment on attachment 19978 [details]
one example

there was a space in the description of periodicals on this one.
Comment 5 Nicole C. Engard 2013-08-01 12:04:37 UTC
In the second example the lib is set to the same as the code - the lib_opac is set to the text.  The search seems to be sorting on lib not lib_opac.
Comment 6 Owen Leonard 2013-08-01 13:22:38 UTC Comment hidden (obsolete)
Comment 7 Nicole C. Engard 2013-08-01 13:37:51 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-08-20 13:22:59 UTC Comment hidden (obsolete)
Comment 9 Galen Charlton 2013-08-20 17:45:35 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2013-08-20 17:45:41 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2013-08-20 17:45:45 UTC Comment hidden (obsolete)
Comment 12 Galen Charlton 2013-08-20 17:48:08 UTC
Sometimes patches that appear to be trivially correct ... aren't.  On the other hand, if you really want to get my attention, combining a signoff with a note that one has not actually tested the patch in question is a very good way to do so. :)

I've added a regression test and a follow-up that handles the case when lib_opac is null.  Setting to signed-off.
Comment 13 Katrin Fischer 2013-08-20 21:13:08 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2013-08-20 21:13:20 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2013-08-20 21:13:30 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2013-09-06 15:24:04 UTC
Created attachment 20825 [details] [review]
Bug 10656: add regression test

This patch adds regression tests to verify the sort
order when fetching authorised values in either staff
or OPAC mode.

This patch also wraps the Koha.t tests in a transaction, and
better handles skipping tests if Test::Deep is not installed.

To test:

[1] Verify that prove -v t/db_dependent/Koha.t passes

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tests pass with all patches applied.

http://bugs.koha-community.org/show_bug.cgi?id=10565
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2013-09-06 15:24:16 UTC
Created attachment 20826 [details] [review]
Bug 10656: improve sorting of shelving location and collections on OPAC advanced search form

Collection codes and shelving locations are displayed in the OPAC and
staff client via GetAuthorisedValues which currently sorts results by
"lib, lib_opac." Consequently if lib (the description for the staff
client) doesn't match lib_opac (the description for the OPAC) sorting
will appear to be nonsensical in the OPAC. GetAuthorisedValues can be
passed an $opac parameter, so this should be used to switch how reuslts
are sorted. This patch implements such a switch.

To test, modify your collection code or shelving location authorized
values so that lib and lib_opac do not match. Set your
AdvancedSearchTypes system preference to display the modified authorized
values and view the advanced search page in the OPAC and staff client.
Sorting should be correct in each case according to the correct value
(lib in the staff client, lib_opac in the OPAC).

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Tested in staff and opac and it works perfectly!

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2013-09-06 15:24:33 UTC
Created attachment 20827 [details] [review]
Bug 10656: (follow-up) handle OPAC sorting of authvals where lib_opac is NULL

The OPAC description for an authorized value is not required to be
populated.  In particular, if it is NULL, the staff description is
displayed instead.

This match makes sure that the sort order (in OPAC mode) uses either
the staff description or the OPAC description as needed for each
value.

To test:

[1] Make sure that AdvancedSearchTypes includes "ccode"
[1] Arrange your CCODE values so the sort order for staff labels
    is different from the sort order for OPAC descriptions.  Also,
    ensure that one of the OPAC descriptions is NULL.  For example,

    authorised_value | lib     | lib_opac
    --------------------------------------
    ZZZ              | A_STAFF | Z_PUBLIC
    DDD              | D_STAFF | NULL
    AAA              | Z_STAFF | A_PUBLIC

[2] Prior to the patch, any CCODE values where the OPAC description
    is NULL will sort first in the OPAC advanced search page, even
    if the displayed label shouldn't come first.
[3] Apply the patch.
[4] Verify that the collection list on the OPAC advanced search page
    is now correct.
[5] Verify that the sort order on the staff advanced search page
    has not changed.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works nicely, tested in staff and OPAC.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Galen Charlton 2013-09-06 16:49:20 UTC
Pushed to master.  Thanks, Owen!
Comment 20 Tomás Cohen Arazi 2013-09-16 14:06:37 UTC
This patch has been pushed to 3.12.x, will be in 3.12.5.

Thanks Owen!