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...
Created attachment 15977 [details] [review] patch to Dewey.pm I may need to try git send-email again; don't think I had mail configured on my dev server. I'm also not versed in Koha's unit testing, so haven't tried to add a test yet.
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
Created attachment 19195 [details] [review] patch for ClassSortRoutine_Dewey.t Here's the test to go with the first patch
Created attachment 19383 [details] [review] 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>
Created attachment 19384 [details] [review] 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>
Created attachment 19385 [details] [review] 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>
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).
Created attachment 19386 [details] [review] 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>
Created attachment 19387 [details] [review] 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>
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.
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>
Pushed to master. Thanks, Jason!
This patch has been pushed to 3.12.x, will be in 3.12.3. Thanks Jason!
Pushed to 3.10.x, will be in 3.10.10
Pushed to 3.8.x, will be in 3.8.17