[Bug 41932] New: Allow optional zero-padding in items.cn_sort
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Bug ID: 41932 Summary: Allow optional zero-padding in items.cn_sort Initiative type: --- Sponsorship --- status: Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Cataloging Assignee: koha-bugs@lists.koha-community.org Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org CC: m.de.rooy@rijksmuseum.nl We have callnumbers like 1 A 1, 10 A 10, 100 A 100 and 1000 A 100. Alphabetically sorting those does not work (100 A 100 < 2 A 2) This proposal includes adding a syspref to define callnumber formats that also specify the desired length when zero-padding cn_sort. This would be active when using C4::ClassSortRoutine::Generic. In the above case we would like to sort them with 0001 A 001, 0010 A 010, 0100 A 100 and 1000 A 100. So the first digit block has length 4, the second block 3. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41936 CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- I'm interested in this. I'm having a similar problem with the generic classification source. I have one library where it would be nice to detect digits and zero pad them to N places to have a more intelligent sort rather than lexicographic sorting. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Are you likely to work on this, Marcel? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #2)
Are you likely to work on this, Marcel?
Yeah see Assignee/status. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Removing 'cruft' from the callnumber should be done after zero padding: | RCE TEST 1.33.45 Mid (1) | RCE_TEST_001033045_MID_1 | We want 1.33.45 not to be interpreted as one digit group (13345) but as three digit groups since the dot serves as delimiter. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 193984 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193984&action=edit Bug 41932: Adjust generic class sort to allow zero-padding Test plan: Run t/ClassSortRoutine_Generic.t Add a few callnumber formats in ClassSortGenericFormats like: ^A:1,3;1,2;;^B:1,2 In words: callnumber starts with A, 1 or more digits (pad to 3), then 1 or more digits (pad to 2). Or callnumber starts with B, one or more digits (pad to 2). Enable generic call number sorting rules in Classification configuration. Now save a few items with callnumbers: 1 A 1, 10 A 2, B2, B11. Check if cn_sort now matches: 001_A_01, 100_A_02, B02, B11. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #2)
Are you likely to work on this, Marcel?
Here is an impression of where I am at now. This comes from 24.11. I need to polish a bit still: add a preference and description, add a db revision, test on main etc. But it is almost ready. Chose for a preference approach above plugin approach here. Many libraries may want to selectively zero pad with multiple schemes. This is possible by an intelligent use of the pref introduced here working with a filter regex and a list of minimum length versus minimum sort length values. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- (In reply to Marcel de Rooy from comment #6)
(In reply to David Cook from comment #2)
Are you likely to work on this, Marcel?
Here is an impression of where I am at now. This comes from 24.11. I need to polish a bit still: add a preference and description, add a db revision, test on main etc. But it is almost ready.
Chose for a preference approach above plugin approach here. Many libraries may want to selectively zero pad with multiple schemes. This is possible by an intelligent use of the pref introduced here working with a filter regex and a list of minimum length versus minimum sort length values.
Thanks for taking this on. I wanted to do it for one library but couldn't get sponsorship for it. I had thought about doing a totally separate name for it rather than generic though just to have a cleaner separation. Are you thinking of adding the syspref inside C4/ClassSortRoutine/Generic.pm? Or would it be in the caller of get_class_sort_key? Probably cleaner to do the latter. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |dcook@prosentient.com.au |y.org | --- Comment #8 from David Cook <dcook@prosentient.com.au> --- I suppose whether we used a different name than Generic or used a syspref, there will be a little bit of work to do in applying it to existing items. I mean a touch_all_items.pl will do the trick but I suppose we don't want to alter existing behaviour without an option (like a syspref or a different name). Anyway, look forward to seeing more. Happy to QA this one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #7)
Are you thinking of adding the syspref inside C4/ClassSortRoutine/Generic.pm? Or would it be in the caller of get_class_sort_key? Probably cleaner to do the latter.
Since the syspref is so tight to the routine that does the work, I think that the current handling within Generic is cleaner. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I have also been wondering if instead of Generic it would be easier to tie this to a new option to avoid any risk of behavior change. Also an idea: To make this more flexible, could it make sense to be able to use a regex based approach to define the "sections" you want to have padded? Almost all of our libraries use their "own" classification scheme. A lot have some form of sequential counter. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Katrin Fischer from comment #10)
I have also been wondering if instead of Generic it would be easier to tie this to a new option to avoid any risk of behavior change.
There is no behavior change whatsoever when the preference is empty. There is no pressing reason to duplicate code here (just one new subroutine).
Also an idea: To make this more flexible, could it make sense to be able to use a regex based approach to define the "sections" you want to have padded? Almost all of our libraries use their "own" classification scheme. A lot have some form of sequential counter.
Did you look at the code? This is very flexible already. The pref contains a list of callnumber 'formats'. Each format has an optional regex for filtering (as you mention) and a list of number pairs that control zero-padding digit groups (with a minimum length for each digit group and a padding length). See also the POD in the module and the examples in the test script. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193984|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 194991 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194991&action=edit Bug 41932: Adjust generic class sort to allow zero-padding Test plan: Run t/ClassSortRoutine_Generic.t Add a few callnumber formats in ClassSortGenericFormats like: ^A:1,3;1,2;;^B:1,2 In words: callnumber starts with A, 1 or more digits (pad to 3), then 1 or more digits (pad to 2). Or callnumber starts with B, one or more digits (pad to 2). Enable generic call number sorting rules in Classification configuration. Now save a few items with callnumbers: 1 A 1, 10 A 2, B2, B11. Check if cn_sort now matches: 001_A_01, 100_A_02, B02, B11. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 194992 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194992&action=edit Bug 41932: Database revision for adding new preference Test plan: Run install or upgrade. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 194993 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194993&action=edit Bug 41932: Pref description Test plan: Find the pref under Cataloging/Record structure. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Note: I ran touch_items with the following pref value on a production database: 8,8;; 1,4;1,4;1,4;1,4;; 1,4;1,4;1,4;; 1,4;1,4;; 1,4 We have lots of different types of callnumbers and some weird edge cases etc. But this works fine. The first 8;8 is a kind of no-op that does match dates in a call number and makes that the other formats are skipped in that case. E.g. VEIL-20201010/11 The rest is going from four digit groups back to one digit group in a callnumber. The first four digit groups in any callnumber are zero-padded to 4 positions. 1 A 12 becomes 0001_A_0012. V II 149 1938-1939 (61) becomes V_II_0149_19381939_0061 When adding regexes to the formats, the thing becomes even more flexible. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (In reply to Marcel de Rooy from comment #15)
Note: I ran touch_items with the following pref value on a production database:
8,8;; 1,4;1,4;1,4;1,4;; 1,4;1,4;1,4;; 1,4;1,4;; 1,4
I think it'll take some time to wrap my head around what this means... Is the whitespace significant? It seems like these formats would be better provided on a per source basis rather than for all Generics? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #16)
(In reply to Marcel de Rooy from comment #15)
Note: I ran touch_items with the following pref value on a production database:
8,8;; 1,4;1,4;1,4;1,4;; 1,4;1,4;1,4;; 1,4;1,4;; 1,4
I think it'll take some time to wrap my head around what this means...
Is the whitespace significant?
It seems like these formats would be better provided on a per source basis rather than for all Generics?
Sure. I deocumented it on the pref description, in the module, test plan and the test script shows examples. The whitespace is added for readability. And it is ignored in the code. I can obviously move this to a separate module. But all my items are on Generic and this is just a little pref-controlled tweak of Generic imo. See also earlier remarks. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #18 from David Cook <dcook@prosentient.com.au> --- (In reply to Marcel de Rooy from comment #17)
(In reply to David Cook from comment #16)
(In reply to Marcel de Rooy from comment #15)
Note: I ran touch_items with the following pref value on a production database:
8,8;; 1,4;1,4;1,4;1,4;; 1,4;1,4;1,4;; 1,4;1,4;; 1,4
I think it'll take some time to wrap my head around what this means...
Is the whitespace significant?
It seems like these formats would be better provided on a per source basis rather than for all Generics?
Sure. I deocumented it on the pref description, in the module, test plan and the test script shows examples.
Yeah no I read through that documentation. The syntax is just not very user-friendly. (Not that I have any better recommendations.)
The whitespace is added for readability. And it is ignored in the code.
Cool, I was hoping that was the case.
I can obviously move this to a separate module. But all my items are on Generic and this is just a little pref-controlled tweak of Generic imo. See also earlier remarks.
It just seems a bit risky? Like it would be very easy to get the wrong rule applying to the wrong items? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #18)
I can obviously move this to a separate module. But all my items are on Generic and this is just a little pref-controlled tweak of Generic imo. See also earlier remarks.
It just seems a bit risky? Like it would be very easy to get the wrong rule applying to the wrong items?
What I mean is... even as a separate module I don't think it would be enough? What I've been finding in some libraries is that they make up a variety of call number schemes and expect them all to sort automagically. Of course, they'd have to choose the correct scheme, so maybe having them all as Generic is best to reduce mental load on cataloguers. I don't know. It's a tricky one. In a way, something is better than what we have now. I'm going to need to think more on this one. I should try some of my real world call number examples... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #19)
It just seems a bit risky? Like it would be very easy to get the wrong rule applying to the wrong items?
What I mean is... even as a separate module I don't think it would be enough?
What I've been finding in some libraries is that they make up a variety of call number schemes and expect them all to sort automagically. Of course, they'd have to choose the correct scheme, so maybe having them all as Generic is best to reduce mental load on cataloguers.
I don't know. It's a tricky one. In a way, something is better than what we have now. I'm going to need to think more on this one.
I should try some of my real world call number examples...
I guess you should try it out. I replaced 400.000 callnumbers and we have a lot of different call number schemes.. At first I was trying to match specific schemes with the (optional) filter regex, but the more general rule I created later (see above) works perfectly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- (In reply to Marcel de Rooy from comment #20)
I guess you should try it out. I replaced 400.000 callnumbers and we have a lot of different call number schemes.. At first I was trying to match specific schemes with the (optional) filter regex, but the more general rule I created later (see above) works perfectly.
That's cool. Yeah, I'm going to give it a try. Soon I hope as I am pretty keen for this one. It would be a big help to a few of my libraries. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #22 from David Cook <dcook@prosentient.com.au> --- Trying this out now and it's pretty cool. I'm writing additional unit tests to do my testing, and I realise that it would be great to have a UI where people could test their classification configuration/sources. I'm going to raise a report for that: bug 42123 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=42123 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194991|0 |1 is obsolete| | --- Comment #23 from David Cook <dcook@prosentient.com.au> --- Created attachment 195609 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195609&action=edit Bug 41932: Adjust generic class sort to allow zero-padding Test plan: Run t/ClassSortRoutine_Generic.t Add a few callnumber formats in ClassSortGenericFormats like: ^A:1,3;1,2;;^B:1,2 In words: callnumber starts with A, 1 or more digits (pad to 3), then 1 or more digits (pad to 2). Or callnumber starts with B, one or more digits (pad to 2). Enable generic call number sorting rules in Classification configuration. Now save a few items with callnumbers: 1 A 1, 10 A 2, B2, B11. Check if cn_sort now matches: 001_A_01, 100_A_02, B02, B11. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194992|0 |1 is obsolete| | --- Comment #24 from David Cook <dcook@prosentient.com.au> --- Created attachment 195610 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195610&action=edit Bug 41932: Database revision for adding new preference Test plan: Run install or upgrade. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194993|0 |1 is obsolete| | --- Comment #25 from David Cook <dcook@prosentient.com.au> --- Created attachment 195611 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195611&action=edit Bug 41932: Pref description Test plan: Find the pref under Cataloging/Record structure. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #26 from David Cook <dcook@prosentient.com.au> --- Created attachment 195612 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195612&action=edit Bug 41932: (follow-up) Improve syspref description and add more unit tests This change improves the formatting of the syspref description, and tries to clarify how the zero-padding formats work along with some examples. The unit tests include the examples from the syspref description as well as some examples of real world generic call numbers. Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|dcook@prosentient.com.au |testopia@bugs.koha-communit | |y.org --- Comment #27 from David Cook <dcook@prosentient.com.au> --- This is really cool. Thanks, Marcel, for putting this together. I took the liberty of adding some more unit tests and tweaking the syspref description to hopefully make it easier to understand. Once I understood the zero-padding formats, it became really clear how it all works. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #28 from David Cook <dcook@prosentient.com.au> --- If this were pushed tomorrow, people wouldn't see any difference with their generic call numbers, because there are no formats out of the box, so it's a very low risk change. It would be such a game changer for a number of my libraries. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #27)
This is really cool. Thanks, Marcel, for putting this together. I took the liberty of adding some more unit tests and tweaking the syspref description to hopefully make it easier to understand.
Once I understood the zero-padding formats, it became really clear how it all works.
Ah, great. Thx for testing and adding the follow-up. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #30 from David Cook <dcook@prosentient.com.au> --- Would love to get this into Koha. Maybe after the next big release it'll be easier... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #30)
Would love to get this into Koha. Maybe after the next big release it'll be easier...
Everything gets stuck in the SO queue nowadays. I dont know why this should wait for the next release. No behavior change when you dont change the pref. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #32 from David Cook <dcook@prosentient.com.au> --- (In reply to Marcel de Rooy from comment #31)
(In reply to David Cook from comment #30)
Would love to get this into Koha. Maybe after the next big release it'll be easier...
Everything gets stuck in the SO queue nowadays. I dont know why this should wait for the next release. No behavior change when you dont change the pref.
Agreed. It's such a low risk change. That said, I suppose the test plans aren't super user-friendly. David Nind mentioned recently if something sits for a while it might be because he (and others) aren't able to understand the test plan (if there is one). Because if we could get a SO then we could handle the QA part. I'm done for the week now (5:19pm on Friday in Australia), but hopefully it can still make it into 26.05! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195609|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195610|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195611|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #195612|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #33 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 198700 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198700&action=edit Bug 41932: Adjust generic class sort to allow zero-padding Test plan: Run t/ClassSortRoutine_Generic.t Add a few callnumber formats in ClassSortGenericFormats like: ^A:1,3;1,2;;^B:1,2 In words: callnumber starts with A, 1 or more digits (pad to 3), then 1 or more digits (pad to 2). Or callnumber starts with B, one or more digits (pad to 2). Enable generic call number sorting rules in Classification configuration. Now save a few items with callnumbers: 1 A 1, 10 A 2, B2, B11. Check if cn_sort now matches: 001_A_01, 100_A_02, B02, B11. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #34 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 198701 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198701&action=edit Bug 41932: Database revision for adding new preference Test plan: Run install or upgrade. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> [EDIT] Removed options, explanation and type -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #35 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 198702 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198702&action=edit Bug 41932: Pref description Test plan: Find the pref under Cataloging/Record structure. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 198703 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198703&action=edit Bug 41932: (follow-up) Improve syspref description and add more unit tests This change improves the formatting of the syspref description, and tries to clarify how the zero-padding formats work along with some examples. The unit tests include the examples from the syspref description as well as some examples of real world generic call numbers. Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Rebased for change on systempreference fields options, explanation and type. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #38 from David Cook <dcook@prosentient.com.au> --- (In reply to Marcel de Rooy from comment #37)
Rebased for change on systempreference fields options, explanation and type.
Cheers! I wonder if we should try to recruit someone else to sign off so that I can mark it Passed QA? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #39 from David Nind <david@davidnind.com> --- The patches still apply, but the tests didn't pass for me on current main. Questions: 1. I'm not sure how to do step 9 of the consolidated test plan I wrote below. Is there anything else that needs configuring in Koha? 2. Description of this issue - I've started to try and explain this change in words I can understand, but since I'm not familiar with this area, it needs some more work and there are some gaps! 3. For the system preference description, suggested changes (happy to add a follow-up patch with suggestion): - first sentence: callnumbers to call numbers - i.e. to: that is - Examples - 2 first level bullet points with the examples: - ^A\s:2,4;1,3 ... Examples (with zero-padding number used for the sort): .. XXXX (zero-padded number) .. XXXX (zero-padded number) - 1 4 ... Examples: .. XXXX (zero-padded number) .. XXXX (zero-padded number) 4. For the input field for the system preference, should there be more space to add formats? Description =========== The new ClassSortGenericFormats system preference allows libraries to define custom call number sorting rules for the generic classification filing rule. Currently: What is the problem with an example With this change: What this allows libraries to do Examples: [provide a before and after example, maybe we should have some examples for "common" types of things libraries try to do] Testing notes (using KTD) ========================= 1. Reproduce the issue: didn't attempt to 2. Apply the patches 3. Update the database: updatedatabase 4. Restart everything: restart_all 5. Tests should pass: prove t/ClassSortRoutine_Generic.t 6. Add a few call number formats in the new ClassSortGenericFormats system preference: ^A:1,3;1,2;;^B:1,2 In words: call number starts with A, 1 or more digits (pad to 3), then 1 or more digits (pad to 2). Or call number starts with B, one or more digits (pad to 2). 7. Enable generic call number sorting rules in the Classification configuration (if no generic rule already): - Administration > Cataloging > Classification configuration - Classification filing rules - existing rule in place for KTD: . general, Generic call number filing rules, Generic 8. Edit some items and add call numbers (o - Full call number): 1 A 1, 10 A 2, B2, B11. ==>9. Check if cn_sort now matches: 001_A_01, 100_A_02, B02, B11 Not sure how to do this or if there is something else required to make this work. SQL query I tried: select * from items where itemcallnumber IS NOT NULL; - Not sure what I should see, this is what is in the cn_sort column: . 1 A 1: 1_000000000000000_A_1 . 10 A 2: 10_000000000000000_A_2 . B2: B_2_000000000000000 . B11: B_11_000000000000000 - If I add the call numbers to a single record with four items (Perl template toolkit (255)), and sort on the call number column in the holdings table: . 1 A 1 . 10 A 2 . B11 . B2 10. Review the description for the new ClassSortGenericFormats and check that it makes sense - feedback welcome to improve this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #40 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 198700 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198700 Bug 41932: Adjust generic class sort to allow zero-padding Review of attachment 198700: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=41932&attachment=198700) ----------------------------------------------------------------- Upon review... the code is not very easy to read at all and I think it might not validate/sanitize the inputs enough, although I haven't done a practical check yet. We want to be careful when it comes to regex inputs. I wish we hadn't added regex to other parts of Koha because it's a nightmare to secure. While the code is concise, it's very difficult to read. It's Perlish for sure but it makes code review and maintenance much harder. Might be worth breaking the zeropad function into a few different functions and unit testing them... ::: C4/ClassSortRoutine/Generic.pm @@ +90,5 @@
+ $format =~ /:/ + ? split /:/, $format, 2 + : ( q{}, $format ); + if ($filter) { + my $test = eval { $key =~ /$filter/ ? 1 : 0 };
Upon review, it seems that $filter isn't sufficiently validated. @@ +98,5 @@
+ + my @pairs = map { my $p = $_; $p =~ s/\s*//g; $p } map { split /,/, $_, 2 } split /;/, $pairs; + if ( @pairs % 2 > 0 || grep { /\D|^0?$/ } @pairs ) { + + # We expect: even number, only digits, not zero or empty
That said, it looks like we validate some things here but maybe that's the pairs? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #41 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Moving to FQA. Will have a look. Thx for your feedback, David and David. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #42 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I have added a helper sub to validate the format: + my ( $filter, @pairs ) = _parse_format($format); + next unless @pairs; + next if $filter && $key !~ /$filter/; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #43 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- And have added a new slightly modified syntax to POD of _zero_pad_callnumber: + The following syntax is used for this preference: + pref: format|pref;;format + format: filter,pairs + pairs: digits,padlen|digits,padlen,pairs + filter: ""|"^string"|"string$"|"string" + + So the pref contains a number of formats. Each format has a + double quoted filter, followed by a list of pairs (two numbers). + Each pair contains a minimum number of digits to be found and + a pad length. + + For security reasons the filter is not a full regex. But the filter + supports 'starts with', 'ends with' and 'contains'. The string may + contain a-z, 0-9, whitespace and a few special characters. See the + constant ALLOWED_FILTER_CHARS. + + When a format leads to a match, the remaining formats are skipped. + This means that the order of formats in the preference is important. + + Whitespace can be used for readability between formats, filters or + pairs (as in following example). + + Suppose that pref (with one format) is set to: "^T ", 2,5, 4,6 + This reads as: The call number should start with T and a space and + should contain at least two digit groups (a group of 2 or more, and + a group of 4 or more). The first group is padded to length 5, the + second group to 6. + + More examples can be found in t/ClassSortRoutine_Generic.t. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #44 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Will submit the patches next week. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #45 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Description =========== The new ClassSortGenericFormats system preference allows libraries to define custom call number sorting rules for the generic classification filing rule. Currently: Call numbers are sorted alphabetically. This makes 10 A 11 come before 10 A 9, or 19 before 9, etc. With this change: You can add zero-padding to call number parts so that the call number sort field cn_sort contains 0010_A_0011 and 0010_A_0009, or 0019 and 0009 as mentioned above. Now they will sort correctly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198700|0 |1 is obsolete| | Attachment #198701|0 |1 is obsolete| | Attachment #198702|0 |1 is obsolete| | Attachment #198703|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #46 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 202611 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202611&action=edit Bug 41932: Adjust generic class sort to allow zero-padding Test plan: See next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #47 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 202612 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202612&action=edit Bug 41932: (follow-up) Simplify filter for security This commit downgrades the filter from a full regex to an expression that filters on 'starts with', 'contains' or 'ends with' (only using the ^ and $). Replacing the regex was requested for security reasons. Since filter is required now, the optional colon(:) between filter and pairs list is replaced by a required comma. Test plan: See also next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #48 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 202613 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202613&action=edit Bug 41932: (follow-up) Add quote in regex, and add info hash Test plan: [1] Run t/ClassSortRoutine_Generic.t [2] Interface tests [a] Make sure that 'z-Other/Generic Classification Scheme' is marked in use on Administration / Classification configuration. [b] Go to System preferences. Set ClassSortGenericFormats to: "^A",1,3,1,2;; "^B",1,4 [c] Edit some items under biblio X and change call numbers to: A 1 12-1, A 2, B2, B22c, B123 45, C4 [d] Verify that cn_sort reflects that change with the sql query: select itemcallnumber, cn_sort from items where biblionumber=X A_001_121, A_2, B0002, B0022c, B0123_45, C4 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #49 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 202614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202614&action=edit Bug 41932: Database revision for adding new preference Test plan: Run install or upgrade. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> [EDIT] Removed options, explanation and type -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #50 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 202615 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202615&action=edit Bug 41932: Pref description Test plan: Find the pref under Cataloging/Record structure. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #51 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 202616 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202616&action=edit Bug 41932: (follow-up) Improve syspref description and add more unit tests This change improves the formatting of the syspref description, and tries to clarify how the zero-padding formats work along with some examples. The unit tests include the examples from the syspref description as well as some examples of real world generic call numbers. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #52 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (ADJUSTED) Testing notes (using KTD) ========================= 1. Reproduce the issue: Look for two call numbers that use the generic classification filing rule. Or change call number in two items to A 11 and A 9. Find cn_sort in the items table for those two items. Use a sql query like select itemcallnumber, cn_sort from items where biblionumber=X You should see A_11 and A_9. (Sorting A_11 before A_9 while we like 9<11.) 2. Apply the patches 3. Update the database: updatedatabase 4. Restart everything: restart_all 5. Tests should pass: prove t/ClassSortRoutine_Generic.t 6. Add a few call number formats in the new ClassSortGenericFormats system preference: "^A",1,3,1,2;; "^B",1,4 7. Enable generic call number sorting rules in the Classification configuration (if no generic rule already): - Administration > Cataloging > Classification configuration - Classification filing rules - existing rule in place for KTD: . general, Generic call number filing rules, Generic 8. Edit some items and add call numbers (o - Full call number): A 1 12-1, A 2, B2, B22c, B123 45, C4 9. Check the following sql query: select itemcallnumber, cn_sort from items where biblionumber=X. Does cn_sort match: A_001_121, A_2, B0002, B0022c, B0123_45, C4 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #53 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Nind from comment #39)
The patches still apply, but the tests didn't pass for me on current main. They should pass now on current main.
Questions:
1. I'm not sure how to do step 9 of the consolidated test plan I wrote below. Is there anything else that needs configuring in Koha? There may be a few issues here. I updated the test plan examples since copy and paste corrupted it probably. You need to really change a call number to update cn_sort. If you just save an item, the call number is not marked "dirty" and cn_sort will not be affected. You should have the correct rule in effect. When you default to Dewey and you add an item, they will look like 1_000000000000000_A_1 as you mentioned. Should we explicitly say: set $2 to Other and change call number? Or change the pref DefaultClassificationSource ?
I updated the description and testing notes. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #54 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #40)
Upon review... the code is not very easy to read at all and I think it might not validate/sanitize the inputs enough, although I haven't done a practical check yet. We want to be careful when it comes to regex inputs. I wish we hadn't added regex to other parts of Koha because it's a nightmare to secure.
While the code is concise, it's very difficult to read. It's Perlish for sure but it makes code review and maintenance much harder. Might be worth breaking the zeropad function into a few different functions and unit testing them...
Upon review, it seems that $filter isn't sufficiently validated.
That said, it looks like we validate some things here but maybe that's the pairs? Yes, it was too compact. I moved a part to _parse_format and rearranged it. Since format is no regex anymore, it can be validated easier (both filter and
I removed the regex feature. Altough I think that matching only should not be a nightmare.. pairs list). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #55 from David Cook <dcook@prosentient.com.au> --- (In reply to Marcel de Rooy from comment #54)
(In reply to David Cook from comment #40)
Upon review... the code is not very easy to read at all and I think it might not validate/sanitize the inputs enough, although I haven't done a practical check yet. We want to be careful when it comes to regex inputs. I wish we hadn't added regex to other parts of Koha because it's a nightmare to secure.
I removed the regex feature. Altough I think that matching only should not be a nightmare..
I'm not sure if you saw my message on Mattermost, but I've tried Regular Expression Denial of Service (ReDoS) with Perl using payloads and strategies outlined by folk like OWASP, and I can't get Perl to break. So while there may be a theoretical problem with match inputs... I think in practice it's OK. So if you did want to include the regex I think it would be OK. Sorry for the run around on this one! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #56 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Cook from comment #55)
(In reply to Marcel de Rooy from comment #54)
(In reply to David Cook from comment #40)
Upon review... the code is not very easy to read at all and I think it might not validate/sanitize the inputs enough, although I haven't done a practical check yet. We want to be careful when it comes to regex inputs. I wish we hadn't added regex to other parts of Koha because it's a nightmare to secure.
I removed the regex feature. Altough I think that matching only should not be a nightmare..
I'm not sure if you saw my message on Mattermost, but I've tried Regular Expression Denial of Service (ReDoS) with Perl using payloads and strategies outlined by folk like OWASP, and I can't get Perl to break.
So while there may be a theoretical problem with match inputs... I think in practice it's OK. So if you did want to include the regex I think it would be OK.
Sorry for the run around on this one!
Lets gp with the current set of patches now. Time to get this moving.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #57 from David Nind <david@davidnind.com> --- Thanks for the improvements to the system preference description and the updated test plan. The tests fail for me[1]. Everything else seems to be working as per the test plan, although there doesn't seem to be leading 0s in the cn_sort column as per step 9 of the test plan they do sort correctly in the staff interface. Testing notes (using KTD): 1. Before the patches, I used The definitive guide to Catalyst (biblionumber = 146) with four items. Added A 11, A9, A 25 as call numbers and left one blank. +----------------+----------------------+ | itemcallnumber | cn_sort | +----------------+----------------------+ | A 11 | A_11_000000000000000 | | A 9 | A_9_000000000000000 | | A 25 | A_25_000000000000000 | | NULL | _ | +----------------+----------------------+ When sorting the holdings table on the call number column - sort order ascending is: A 11, A 25, A 9 (instead of A 9, A 11, A 25) 2. After the patch, I used Intermediate Perl (biblionumber = 126) and added call numbers. +----------------+-------------------------+ | itemcallnumber | cn_sort | +----------------+-------------------------+ | A 2 | A_2_000000000000000 | | B2 | B_2_000000000000000 | | A 1 12-1 | A_1_000000000000000_121 | | B22c | B_22C | | B123 45 | B_123_450000000000000 | | C4 | C_4_000000000000000 | +----------------+-------------------------+ When sorting the holdings table on the call number column - sort order ascending is: A 1 12-1 A 2 B123 45 B2 B22c C4 [1] Test failure ================ Tests fail: prove t/ClassSortRoutine_Generic.t t/ClassSortRoutine_Generic.t .. 1/3 # Failed test 'no warnings' # at /usr/share/perl/5.40/Test/Builder.pm line 193. # There were 1 warning(s) # Previous test 0 '' # Attempt to call undefined import method with arguments ("get_class_sort_key") via package "C4::ClassSortRoutine::Generic" (Perhaps you forgot to load the package?) at t/ClassSortRoutine_Generic.t line 8. # at t/ClassSortRoutine_Generic.t line 8. # main::BEGIN() called at t/ClassSortRoutine_Generic.t line 8 # eval {...} called at t/ClassSortRoutine_Generic.t line 8 # # Looks like you failed 1 test of 3. t/ClassSortRoutine_Generic.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/3 subtests Test Summary Report ------------------- t/ClassSortRoutine_Generic.t (Wstat: 256 (exited 1) Tests: 3 Failed: 1) Failed test: 3 Non-zero exit status: 1 Files=1, Tests=3, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.38 cusr 0.07 csys = 0.48 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #58 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Nind from comment #57)
The tests fail for me[1].
Tests fail:
prove t/ClassSortRoutine_Generic.t t/ClassSortRoutine_Generic.t .. 1/3 # Failed test 'no warnings' # at /usr/share/perl/5.40/Test/Builder.pm line 193. # There were 1 warning(s) # Previous test 0 '' # Attempt to call undefined import method with arguments ("get_class_sort_key") via package "C4::ClassSortRoutine::Generic" (Perhaps you forgot to load the package?) at t/ClassSortRoutine_Generic.t line 8. # at t/ClassSortRoutine_Generic.t line 8. # main::BEGIN() called at t/ClassSortRoutine_Generic.t line 8 # eval {...} called at t/ClassSortRoutine_Generic.t line 8 # # Looks like you failed 1 test of 3. t/ClassSortRoutine_Generic.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/3 subtests
Looks like this might be a perl version issue (perhaps you run Debian 13 with perl 5.40, while I still run a D12 container with perl 5.36) I removed the unneeded import now. Could you run the test again? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #59 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- This proves that it is a perl issue: [1] Create module Test.pm package Test; use Modern::Perl; sub test1 { print 1; } 1; [2] Create script test.pl use Modern::Perl; use Test qw(test2); Test::test1(); You can run test.pl under Debian 12, but it fails under Debian 13. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #60 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 203267 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203267&action=edit Bug 41932: (QA follow-up) Remove import Apparently, this worked under D12 but not under D13, related to the perl version (5.36 resp. 5.40). Test plan: Run t/ClassSortRoutine_Generic.t (under D13, perl 5.40) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #61 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Nind from comment #57)
2. After the patch, I used Intermediate Perl (biblionumber = 126) and added call numbers.
+----------------+-------------------------+ | itemcallnumber | cn_sort | +----------------+-------------------------+ | A 2 | A_2_000000000000000 | | B2 | B_2_000000000000000 | | A 1 12-1 | A_1_000000000000000_121 | | B22c | B_22C | | B123 45 | B_123_450000000000000 | | C4 | C_4_000000000000000 | +----------------+-------------------------+
Yeah, this doesnt look good. It tells me that Dewey is used. Could you please check the following statements from comment53: You need to really change a call number to update cn_sort. If you just save an item, the call number is not marked "dirty" and cn_sort will not be affected. You should have the correct rule in effect. When you default to Dewey and you add an item, they will look like 1_000000000000000_A_1 as you mentioned. Should we explicitly say: set $2 to Other and change call number? Or change the pref DefaultClassificationSource ? Did you change the $2 value to Other for those items? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #62 from David Nind <david@davidnind.com> --- (In reply to Marcel de Rooy from comment #61)
(In reply to David Nind from comment #57)
2. After the patch, I used Intermediate Perl (biblionumber = 126) and added call numbers.
+----------------+-------------------------+ | itemcallnumber | cn_sort | +----------------+-------------------------+ | A 2 | A_2_000000000000000 | | B2 | B_2_000000000000000 | | A 1 12-1 | A_1_000000000000000_121 | | B22c | B_22C | | B123 45 | B_123_450000000000000 | | C4 | C_4_000000000000000 | +----------------+-------------------------+
Yeah, this doesnt look good. It tells me that Dewey is used. Could you please check the following statements from comment53:
You need to really change a call number to update cn_sort. If you just save an item, the call number is not marked "dirty" and cn_sort will not be affected. You should have the correct rule in effect. When you default to Dewey and you add an item, they will look like 1_000000000000000_A_1 as you mentioned. Should we explicitly say: set $2 to Other and change call number? Or change the pref DefaultClassificationSource ?
Did you change the $2 value to Other for those items?
The tests now pass (KTD uses Debian 13 (Trixie) by default). I didn't originally understand what was meant above, but I think I've now worked it out: 1. In the authorized value option for 952$2 (BKS framework) there is cn_source 2. There is no cn_source authorized values category under Administration > Basic parameters > Authorized values. 3. When adding or editing items there are two values in the dropdown list: - Dewey Decimal Classification - Library of Congress Classification 4. When I add a new item it defaults to Dewey Decimal Classification for 952$2 5. If I change DefaultClassificationSource to "Other/Generic Classification Scheme", there are now three items listed in the 952$2 dropdown list 6. Before changing the classification scheme in 952$2, the database query results were: select itemcallnumber, cn_sort from items where biblionumber=126; +----------------+-------------------------+ | itemcallnumber | cn_sort | +----------------+-------------------------+ | A 1 12-1 | A_1_000000000000000_121 | | B2 | B_2_000000000000000 | | A 2 | A_2_000000000000000 | | B22c | B_22C | | B123 45 | B_123_450000000000000 | | C4 | C_4_000000000000000 | +----------------+-------------------------+ 7. After changing each item so that 952$2 is now "Other/...." I get the results shown in the test plan: select itemcallnumber, cn_sort from items where biblionumber=126; +----------------+-----------+ | itemcallnumber | cn_sort | +----------------+-----------+ | A 1 12-1 | A_001_121 | | B2 | B0002 | | A 2 | A_2 | | B22c | B0022C | | B123 45 | B0123_45 | | C4 | C4 | +----------------+-----------+ 8. Which I think is what is required. 9. For the record that I added call numbers to before the patch, I changed the 952$2 to "Other/...": - Before and after the patch, the SQL results were: select itemcallnumber, cn_sort from items where biblionumber=146; +----------------+----------------------+ | itemcallnumber | cn_sort | +----------------+----------------------+ | A 11 | A_11_000000000000000 | | A 9 | A_9_000000000000000 | | A 25 | A_25_000000000000000 | | NULL | _ | +----------------+----------------------+ - After the patch, and changing the 952$2 for the items so that it is "Other/...", the SQL results are now: select itemcallnumber, cn_sort from items where biblionumber=146; +----------------+---------+ | itemcallnumber | cn_sort | +----------------+---------+ | A 11 | A_11 | | A 9 | A_9 | | A 25 | A_25 | | NULL | | +----------------+---------+ So the missing step (for me) to make this work is to set DefaultClassificationSource system preference to "Other/...." and use that option for 952$2 (or maybe modify Classification configuration > Classification sources > z (Other/Generic Classification Scheme) so that "In use" = "Yes"?). Then it shows up in the dropdown list for 952$2. However, even if I update the items for the record I added call numbers to before the patch, it no longer sorts as expected (sorts as A 11, A 25, A 9; instead of A 9, A 11, A 25). Is this because it would need something in the new ClassSortGenericFormats system preference to make it sort correctly? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #63 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looks good, David. I will refine the test plan just a bit to overcome your last remarks by adding an order by clause and add the Other source for $2. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #64 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (ADJUSTED) Testing notes (using KTD) ========================= 1. Reproduce the issue: Look for two call numbers that use the generic classification filing rule. Or change call number in two items to A 11 and A 9. Find cn_sort in the items table for those two items. Use a sql query like select itemcallnumber, cn_sort from items where biblionumber=X order by cn_sort You should see A_11 and A_9. (Sorting A_11 before A_9 while we like 9<11.) 2. Apply the patches 3. Update the database: updatedatabase 4. Restart everything: restart_all 5. Tests should pass: prove t/ClassSortRoutine_Generic.t 6. Add a few call number formats in the new ClassSortGenericFormats system preference: "^A",1,3,1,2;; "^B",1,4 7. Enable generic call number sorting rules in the Classification configuration (if no generic rule already): - Administration > Cataloging > Classification configuration - Classification filing rules - existing rule in place for KTD: . general, Generic call number filing rules, Generic 8. Edit some items and add call numbers (o - Full call number) as below. Make sure that item subfield $2 points to Other/Generic.. A 1 12-1, A 2, B2, B22c, B123 45, C4 9. Check the following sql query: select itemcallnumber, cn_sort from items where biblionumber=X order by cn_sort Does cn_sort match: A_001_121, A_2, B0002, B0022c, B0123_45, C4 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #65 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- select itemcallnumber, cn_sort from items where biblionumber=4 order by cn_sort; +----------------+-----------+ | itemcallnumber | cn_sort | +----------------+-----------+ | A 1 12-1 | A_001_121 | | A 2 | A_2 | | B2 | B0002 | | B22c | B0022C | | B123 45 | B0123_45 | | C4 | C4 | +----------------+-----------+ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202611|0 |1 is obsolete| | Attachment #202612|0 |1 is obsolete| | Attachment #202613|0 |1 is obsolete| | Attachment #202614|0 |1 is obsolete| | Attachment #202615|0 |1 is obsolete| | Attachment #202616|0 |1 is obsolete| | Attachment #203267|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #66 from David Nind <david@davidnind.com> --- Created attachment 203409 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203409&action=edit Bug 41932: Adjust generic class sort to allow zero-padding Test plan: See next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #67 from David Nind <david@davidnind.com> --- Created attachment 203410 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203410&action=edit Bug 41932: (follow-up) Simplify filter for security This commit downgrades the filter from a full regex to an expression that filters on 'starts with', 'contains' or 'ends with' (only using the ^ and $). Replacing the regex was requested for security reasons. Since filter is required now, the optional colon(:) between filter and pairs list is replaced by a required comma. Test plan: See also next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #68 from David Nind <david@davidnind.com> --- Created attachment 203411 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203411&action=edit Bug 41932: (follow-up) Add quote in regex, and add info hash Test plan: [1] Run t/ClassSortRoutine_Generic.t [2] Interface tests [a] Make sure that 'z-Other/Generic Classification Scheme' is marked in use on Administration / Classification configuration. [b] Go to System preferences. Set ClassSortGenericFormats to: "^A",1,3,1,2;; "^B",1,4 [c] Edit some items under biblio X and change call numbers to: A 1 12-1, A 2, B2, B22c, B123 45, C4 [d] Verify that cn_sort reflects that change with the sql query: select itemcallnumber, cn_sort from items where biblionumber=X A_001_121, A_2, B0002, B0022c, B0123_45, C4 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #69 from David Nind <david@davidnind.com> --- Created attachment 203412 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203412&action=edit Bug 41932: Database revision for adding new preference Test plan: Run install or upgrade. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> [EDIT] Removed options, explanation and type Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #70 from David Nind <david@davidnind.com> --- Created attachment 203413 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203413&action=edit Bug 41932: Pref description Test plan: Find the pref under Cataloging/Record structure. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #71 from David Nind <david@davidnind.com> --- Created attachment 203414 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203414&action=edit Bug 41932: (follow-up) Improve syspref description and add more unit tests This change improves the formatting of the syspref description, and tries to clarify how the zero-padding formats work along with some examples. The unit tests include the examples from the syspref description as well as some examples of real world generic call numbers. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 --- Comment #72 from David Nind <david@davidnind.com> --- Created attachment 203415 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203415&action=edit Bug 41932: (QA follow-up) Remove import Apparently, this worked under D12 but not under D13, related to the perl version (5.36 resp. 5.40). Test plan: Run t/ClassSortRoutine_Generic.t (under D13, perl 5.40) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41932 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed --- Comment #73 from David Nind <david@davidnind.com> --- Thnks Marcel! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org