Bug 9770

Summary: C4::ClassSortRoutine::Dewey can pad the wrong part of a call number internally
Product: Koha Reporter: Jason Etheridge <jason>
Component: CatalogingAssignee: Galen Charlton <gmcharlt>
Status: CLOSED FIXED QA Contact:
Severity: minor    
Priority: P5 - low CC: bgkriegel, chris, chrish, m.de.rooy, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: patch to Dewey.pm
patch for ClassSortRoutine_Dewey.t
Bug 9770: fix sorting of Dewey call numbers that contain prefixes
Bug 9770: test case for sorting of Dewey call numbers that contain prefixes
Bug 9770: fix sorting of Dewey call numbers that contain prefixes
Bug 9770: test case for sorting of Dewey call numbers that contain prefixes
Bug 9770: fix sorting of Dewey call numbers that contain prefixes
[PASSED QA] Bug 9770: test case for sorting of Dewey call numbers that contain prefixes
[PASSED QA] Bug 9770: fix sorting of Dewey call numbers that contain prefixes

Description Jason Etheridge 2013-03-08 15:09:19 UTC
The subroutine get_class_sort_key tokenizes a call number string (splitting on periods and whitespace) and counts the number of tokens that solely contain digits.  If there is only one such digit group, a comment in the code states that it will pad said digit group.  However, the bug is that the code assumes said digit group is the first token, when this may not be the case.  Patch to follow...
Comment 1 Jason Etheridge 2013-03-08 15:22:19 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2013-03-09 11:22:51 UTC
Hi Jason

 t/ClassSortRoutine_Dewey.t is what you want to take a look at.

The patch looks good, so if you just added a test to that file that will be excellent
Comment 3 Jason Etheridge 2013-06-21 19:15:48 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2013-07-04 00:51:42 UTC Comment hidden (obsolete)
Comment 5 Galen Charlton 2013-07-04 00:52:05 UTC Comment hidden (obsolete)
Comment 6 Galen Charlton 2013-07-04 00:52:18 UTC Comment hidden (obsolete)
Comment 7 Galen Charlton 2013-07-04 00:53:57 UTC
I've tested the patches and attached my signoff.  I've modified the commit messages to include test plans.

Setting this bug to Needs Signoff (Jason, the next step is for a non-ESI person to review the patch).
Comment 8 Chris Cormack 2013-07-04 02:14:50 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2013-07-04 02:15:03 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2013-07-14 12:12:58 UTC
Created attachment 19629 [details] [review]
[PASSED QA] Bug 9770: test case for sorting of Dewey call numbers that contain prefixes

This adds a test for C4::ClassSortRoutine::Dewey to check that the
call number "JR DVD 800.1" sorts before "JR DVD 900"

To test:

[1] Apply just this patch.
[1] Run prove -v t/ClassSortRoutine_Dewey.t
[2] Test #7 should fail.

Signed-off-by: Jason Etheridge <jason@esilibrary.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes test plan and QA script.
Comment 11 Katrin Fischer 2013-07-14 12:13:14 UTC
Created attachment 19630 [details] [review]
[PASSED QA] Bug 9770: fix sorting of Dewey call numbers that contain prefixes

C4::ClassSortRoutine::Dewey can pad the wrong part of a call number internally.

The subroutine get_class_sort_key tokenizes a call number string (splitting on
periods and whitespace) and counts the number of tokens that solely contain
digits.  If there is only one such digit group, a comment in the code states
that it will pad said digit group.  However, the bug is that the code assumes
said digit group is the first token, when this may not be the case.

In practice, this can cause poor sorting when used a call number is in the form
of PREFIX _space_ 3DIGITS.

To test:

[1] Create two item records whose class scheme is set to
    'ddc' (Dewey) and whose call numbers contain prefixes, e.g.,
    J DVD 700.1 ABC and J DVD 850 DEF.
[2] Use the inventory tool to produce a list of item items that include
    the two created in step 1.  Obsere that that items are sorted
    in the incorrect order, with "J DVD 850 DEF" coming before
    "J DVD 700.1 ABC".  Alternatively, run the following SQL
    to see the incorrect sort order:

    SELECT cn_sort, itemcallnumber
    FROM items
    WHERE itemcallnumber LIKE 'J DVD%'
    ORDER BY cn_sort;

[4] Apply this patch.
[5] Run misc/maintenance/touch_all_items.pl to force cn_sort to be
    recalculated.
[6] Repeat step 2 and verify that the call numbers are now sorted
    corrected.

Signed-off-by: Jason Etheridge <jason@esilibrary.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 12 Galen Charlton 2013-07-15 16:13:42 UTC
Pushed to master.  Thanks, Jason!
Comment 13 Tomás Cohen Arazi 2013-07-25 15:14:53 UTC
This patch has been pushed to 3.12.x, will be in 3.12.3.

Thanks Jason!
Comment 14 Bernardo Gonzalez Kriegel 2013-08-04 22:55:19 UTC
Pushed to 3.10.x, will be in 3.10.10
Comment 15 Chris Hall 2013-08-20 09:16:33 UTC
Pushed to 3.8.x, will be in 3.8.17