Bug 9770 - C4::ClassSortRoutine::Dewey can pad the wrong part of a call number internally
Summary: C4::ClassSortRoutine::Dewey can pad the wrong part of a call number internally
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Galen Charlton
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-08 15:09 UTC by Jason Etheridge
Modified: 2014-12-07 20:02 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
patch to Dewey.pm (1.96 KB, patch)
2013-03-08 15:22 UTC, Jason Etheridge
Details | Diff | Splinter Review
patch for ClassSortRoutine_Dewey.t (1.29 KB, patch)
2013-06-21 19:15 UTC, Jason Etheridge
Details | Diff | Splinter Review
Bug 9770: fix sorting of Dewey call numbers that contain prefixes (2.78 KB, patch)
2013-07-04 00:51 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9770: test case for sorting of Dewey call numbers that contain prefixes (1.45 KB, patch)
2013-07-04 00:52 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9770: fix sorting of Dewey call numbers that contain prefixes (2.78 KB, patch)
2013-07-04 00:52 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9770: test case for sorting of Dewey call numbers that contain prefixes (1.50 KB, patch)
2013-07-04 02:14 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9770: fix sorting of Dewey call numbers that contain prefixes (2.83 KB, patch)
2013-07-04 02:15 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 9770: test case for sorting of Dewey call numbers that contain prefixes (1.60 KB, patch)
2013-07-14 12:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 9770: fix sorting of Dewey call numbers that contain prefixes (2.90 KB, patch)
2013-07-14 12:13 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 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