Bug 15836 - Labels: Offer configuration option for splitting call numbers
Summary: Labels: Offer configuration option for splitting call numbers
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords: rel_18_11_candidate
: 12201 (view as bug list)
Depends on: 14391
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-17 09:53 UTC by Katrin Fischer
Modified: 2018-11-16 14:24 UTC (History)
12 users (show)

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


Attachments
Bug 15836: DB changes (6.64 KB, patch)
2018-07-04 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (4.77 KB, patch)
2018-07-04 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: UI changes (40.35 KB, patch)
2018-07-04 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add the ability to defined custom methods to split call number in labels (37.97 KB, patch)
2018-07-04 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Mark C4::ClassSource subs DEPRECATED (2.38 KB, patch)
2018-07-04 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Screenshot of the feature (55.71 KB, image/png)
2018-07-04 16:12 UTC, Jonathan Druart
Details
Bug 15836: Add tests (2.08 KB, patch)
2018-07-17 18:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add missing svc script (3.91 KB, patch)
2018-07-18 15:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Handle non-existent regexs (1.92 KB, patch)
2018-07-27 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: UI changes (41.26 KB, patch)
2018-09-14 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add the ability to defined custom methods to split call number in labels (37.97 KB, patch)
2018-09-14 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Mark C4::ClassSource subs DEPRECATED (2.38 KB, patch)
2018-09-14 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add tests (2.08 KB, patch)
2018-09-14 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add missing svc script (3.91 KB, patch)
2018-09-14 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Handle non-existent regexs (1.92 KB, patch)
2018-09-14 22:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix display input in any situations (1.17 KB, patch)
2018-09-16 23:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: DB changes (6.80 KB, patch)
2018-10-10 13:14 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (4.93 KB, patch)
2018-10-10 13:14 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: UI changes (41.43 KB, patch)
2018-10-10 13:14 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: Add the ability to defined custom methods to split call number in labels (38.13 KB, patch)
2018-10-10 13:14 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: Mark C4::ClassSource subs DEPRECATED (2.54 KB, patch)
2018-10-10 13:14 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: Add tests (2.25 KB, patch)
2018-10-10 13:14 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: Add missing svc script (4.07 KB, patch)
2018-10-10 13:15 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: Handle non-existent regexs (2.08 KB, patch)
2018-10-10 13:15 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: Fix display input in any situations (1.41 KB, patch)
2018-10-10 13:15 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 15836: DB changes (6.80 KB, patch)
2018-10-12 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (4.93 KB, patch)
2018-10-12 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: UI changes (41.41 KB, patch)
2018-10-12 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add the ability to defined custom methods to split call number in labels (37.91 KB, patch)
2018-10-12 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Mark C4::ClassSource subs DEPRECATED (2.54 KB, patch)
2018-10-12 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add tests (2.25 KB, patch)
2018-10-12 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add missing svc script (4.07 KB, patch)
2018-10-12 18:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Handle non-existent regexs (2.08 KB, patch)
2018-10-12 18:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix display input in any situations (1.41 KB, patch)
2018-10-12 18:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: DB changes (6.83 KB, patch)
2018-10-23 20:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (4.98 KB, patch)
2018-10-23 20:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: UI changes (41.44 KB, patch)
2018-10-23 20:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: Add the ability to defined custom methods to split call number in labels (37.87 KB, patch)
2018-10-23 20:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: Mark C4::ClassSource subs DEPRECATED (2.60 KB, patch)
2018-10-23 20:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: Add tests (2.29 KB, patch)
2018-10-23 20:16 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: Add missing svc script (4.11 KB, patch)
2018-10-23 20:17 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: Handle non-existent regexs (2.10 KB, patch)
2018-10-23 20:17 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: Fix display input in any situations (1.46 KB, patch)
2018-10-23 20:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15836: DB changes (6.88 KB, patch)
2018-10-30 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (5.02 KB, patch)
2018-10-30 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: UI changes (41.49 KB, patch)
2018-10-30 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add the ability to defined custom methods to split call number in labels (37.99 KB, patch)
2018-10-30 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Mark C4::ClassSource subs DEPRECATED (2.63 KB, patch)
2018-10-30 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add tests (2.33 KB, patch)
2018-10-30 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add missing svc script (4.15 KB, patch)
2018-10-30 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Handle non-existent regexs (2.17 KB, patch)
2018-10-30 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix display input in any situations (1.49 KB, patch)
2018-10-30 15:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix update DB entry (1.42 KB, patch)
2018-11-08 19:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix DB structure (1.03 KB, patch)
2018-11-08 19:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (4.10 KB, patch)
2018-11-08 19:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix DB structure (1.09 KB, patch)
2018-11-08 20:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (4.16 KB, patch)
2018-11-08 20:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15836: (follow-up) Fix filters (1.68 KB, patch)
2018-11-08 20:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15836: DB changes (6.88 KB, patch)
2018-11-13 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: UI changes (41.32 KB, patch)
2018-11-13 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add the ability to defined custom methods to split call number in labels (37.99 KB, patch)
2018-11-13 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Mark C4::ClassSource subs DEPRECATED (2.63 KB, patch)
2018-11-13 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add tests (2.33 KB, patch)
2018-11-13 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Add missing svc script (4.15 KB, patch)
2018-11-13 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Handle non-existent regexs (2.17 KB, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix display input in any situations (1.49 KB, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix DB structure (1.09 KB, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: DBIC Schema change (4.16 KB, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: (follow-up) Fix filters (1.68 KB, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix deletion of class sources (1.35 KB, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Remove debug statement (845 bytes, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: (follow-up) Fix display input in any situations (1.21 KB, patch)
2018-11-13 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Fix failing tests (2.22 KB, patch)
2018-11-13 17:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Remove deprecated subroutines (10.84 KB, patch)
2018-11-13 17:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Remove deprecated subroutines (12.33 KB, patch)
2018-11-13 17:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15836: Remove deprecated subroutines (12.43 KB, patch)
2018-11-13 18:35 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 Katrin Fischer 2016-02-17 09:53:46 UTC
Currently the call number splitting seems to be mostly implemented for DDC and LC classifications. Those are both not very common in Germany and possibly other countries. A lot of our libraries use their own custom classification schemes so the call number splitting is something that should be individually configurable.

One option would be using a regex expression, that can be set in the layout additional to the standard splitting offered by the checkbox.

(see bug 2500 for some discussion about the splitting algorithms implemented atm)
Comment 2 Jonathan Druart 2018-07-04 16:11:22 UTC
Created attachment 76691 [details] [review]
Bug 15836: DB changes
Comment 3 Jonathan Druart 2018-07-04 16:11:27 UTC
Created attachment 76692 [details] [review]
Bug 15836: DBIC Schema change
Comment 4 Jonathan Druart 2018-07-04 16:11:31 UTC
Created attachment 76693 [details] [review]
Bug 15836: UI changes

The script admin/classsources.pl has been adjusted to follow usual
patterns used in other Koha scripts.
$op can be add_source, add_source_validate, etc.
Deletion steps have been removed (there is a JS confirmation box) to
simplify both script and template.
Comment 5 Jonathan Druart 2018-07-04 16:11:36 UTC
Created attachment 76694 [details] [review]
Bug 15836: Add the ability to defined custom methods to split call number in labels

Currently the call number splitting seems to be mostly implemented for
DDC and LC classifications.
Those are both not very common in some countries.
A lot of libraries use their own custom classification schemes so the call number
plitting is something that should be individually configurable.

This enhancement adds the ability to define custom splitting rules based
on regular expressions.

How does it work so far?
From C4/Labels/Label.pm there are 3 differents splitting methods defined, depending on items.cn_source.
  if cn_source is "lcc' or 'nlm' we split using Library::CallNumber::LC
  if cn_source is 'ddc' we split using a in-house method
Finally there is a fallback method to split on space
And nothing else is done for other cn_source

The idea of this patch is to mimick what was done for the "filing rules" and add
the ability to define "splitting rules" that will be used by the "Classification sources".
A classification source will then have:
  * a filing rule used to sort items by callnumbers
  * a splitting rule used to print labels

To acchieve this goal this enhancement will do the following
modifications at DB level:
* New table class_split_rules
* New column class_sources.class_split_rule

Test plan:
* Execute the update database entry to create the new table and
column.
I. UI Changes
a) Create/modify/delete a filing rule
b) Create/modify/delete a splitting rule
c) Create/modify/delete a classification source
=> A filing rule or splitting rule cannot be removed if used by a
classification source

II. Splitting rule using regular expressions
a) Create a splitting rule using the "Splitting routine" "RegEx"
b) Define several regular expressions, they will be applied one after
the other in the same order you define them.
Something like:
  s/\s/\n/g         # Break on spaces
  s/(\s?=)/\n=/g    # Break on = (unless it's done already)
  s/^(J|K)\n/$1 /   # Remove the first break if callnumber starts with J or K
c) You can test the regular expressions using filling the textarea with
a list of callnumbers. Then click "Test" and confirm the callnumbers are
split how you expected.
d) Finally create a new classification source that will use this new
splitting rule.

III. Print the label!
a) Create a layout. It should have the "Split call numbers" checkbox
ticked, and display itemcallnumber
b) Use this layout to export labels, use items with different
classification source ('lcc', 'ddc', but also the new one you have
create)
=> The callnumbers should have been split according to the regex you
defined earlier!

Notes:
* The update database entry fill the class_sources.class_split_rule
with the value of class_sources.class_sort_rule
If default rules exist it will not work, we should add a note in the
release notes (would be enough?)
* C4::ClassSplitRoutine::* should be moved to Koha::ClassSplitRule,
but it sounded better to keep the same pattern as ClassSortRoutines
* Should not we use a LONGTEXT for class_split_rules.split_regex instead
of VARCHAR(255)?

* class_sources.sql should be filled for other languages before pushed
to master!

IMPORTANT NOTES: The regular expressions are stored as it, and eval is
used to evaluate it (perlcritic raises a warning about it (Expression
form of "eval"). It can lead to serious security issues (execution of
arbitrary code on the server), especially if the modifier 'e' is used.
We could then remedy the situation with one of these following points:
- Assume that this DB data is safe (We can add a new permission?)
- Assume that the data is not safe and deal with possible attack
Cons: how be sure we are exhaustive? Making sure it matches ^s///[^e/]*$
would be enough?
- Use Template Toolkit syntax instead (Really safer?)
  [% callnumber.replace('\s', '\n').replace ... %]
- Cut the regex parts: find, replace, modifiers
like we already do for Marc modification template. Cons: we are going to
have escape problems, the "find" and "replace" parts should not be
handle the same way (think "\n", "\\n", "\1", "\s", etc.)
I did not manage to implement this one easily.
Comment 6 Jonathan Druart 2018-07-04 16:11:41 UTC
Created attachment 76695 [details] [review]
Bug 15836: Mark C4::ClassSource subs DEPRECATED

These subs are no longer used from scripts and should be removed. It
should be done on a separate bug report given that additional work is
needed
Comment 7 Jonathan Druart 2018-07-04 16:12:58 UTC
Created attachment 76696 [details]
Screenshot of the feature
Comment 8 Jonathan Druart 2018-07-17 18:55:58 UTC
Created attachment 77061 [details] [review]
Bug 15836: Add tests
Comment 9 Jonathan Druart 2018-07-17 18:56:11 UTC
Just found this test file in my local repo. It sounds like it belongs to this one...
Comment 10 Jonathan Druart 2018-07-18 15:48:42 UTC
Created attachment 77079 [details] [review]
Bug 15836: Add missing svc script
Comment 11 Katrin Fischer 2018-07-27 09:21:37 UTC
Hi Jonathan,

we have tested the basic functionality and are happy about the results of the test button and the results in the generated PDF.

We also found some issues:

1) There is an error if you try to edit an existing non-Regex splitting rule:
Template process failed: undef error - malformed JSON string, neither array, object, number, string or atom, at character offset 0 (before "(end of string)") at /usr/share/perl5/JSON.pm line 171.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

2) If you try to print the PDF for labels with ddc/loc splitting rule, there is also an error:
malformed JSON string, neither array, object, number, string or atom, at character offset 0 (before "(end of string)") at /usr/share/perl5/JSON.pm line 171


We are not sure on how to solve the security issue. I think switching to TT would not be good in this case, as it would be harder do write and people would try to do more formatting that is probably not supported by the labels right now.

Maybe an approach splitting the regex into muliple fields similar to the MARC modification templates GUI could work. Instead of hardcoding /ig, maybe a pull down? Open to your and others' suggestions.
Comment 12 Katrin Fischer 2018-07-27 09:23:03 UTC
> * Should not we use a LONGTEXT for class_split_rules.split_regex instead
> of VARCHAR(255)?

varchar(255) seems enough.
Comment 13 Jonathan Druart 2018-07-27 13:13:40 UTC
Created attachment 77306 [details] [review]
Bug 15836: Handle non-existent regexs

For other types of split rules
Comment 14 Jonathan Druart 2018-07-27 13:15:54 UTC
(In reply to Katrin Fischer from comment #11)
> Hi Jonathan,

Hi Katrin, thanks for testing!

> we have tested the basic functionality and are happy about the results of
> the test button and the results in the generated PDF.

Good to hear :)

> We also found some issues:

Must be fixed with the last patch.

> We are not sure on how to solve the security issue. I think switching to TT
> would not be good in this case, as it would be harder do write and people
> would try to do more formatting that is probably not supported by the labels
> right now.
> 
> Maybe an approach splitting the regex into muliple fields similar to the
> MARC modification templates GUI could work. Instead of hardcoding /ig, maybe
> a pull down? Open to your and others' suggestions.

My guess it that MMT does not work with $1 either, I think we will also need /e for that.
Comment 15 Katrin Fischer 2018-07-27 13:35:03 UTC
*** Bug 12201 has been marked as a duplicate of this bug. ***
Comment 16 Katrin Fischer 2018-07-27 14:50:58 UTC
The last patch fixes the 2 errors.

Looking at the functionality, we are ready to sign-off, but worried about the security issue.

If I understood correctly the use of $1 is related to the eval happening and it won't work without. For the current use case, the line s/^(J|K)\n/$1 could be replaced by 2 lines for K and J, not using $1. So we might be able to cut that feature, if it would help.
Comment 17 Jonathan Druart 2018-09-14 22:25:35 UTC
Created attachment 78791 [details] [review]
Bug 15836: UI changes

The script admin/classsources.pl has been adjusted to follow usual
patterns used in other Koha scripts.
$op can be add_source, add_source_validate, etc.
Deletion steps have been removed (there is a JS confirmation box) to
simplify both script and template.
Comment 18 Jonathan Druart 2018-09-14 22:25:39 UTC
Created attachment 78793 [details] [review]
Bug 15836: Add the ability to defined custom methods to split call number in labels

Currently the call number splitting seems to be mostly implemented for
DDC and LC classifications.
Those are both not very common in some countries.
A lot of libraries use their own custom classification schemes so the call number
plitting is something that should be individually configurable.

This enhancement adds the ability to define custom splitting rules based
on regular expressions.

How does it work so far?
From C4/Labels/Label.pm there are 3 differents splitting methods defined, depending on items.cn_source.
  if cn_source is "lcc' or 'nlm' we split using Library::CallNumber::LC
  if cn_source is 'ddc' we split using a in-house method
Finally there is a fallback method to split on space
And nothing else is done for other cn_source

The idea of this patch is to mimick what was done for the "filing rules" and add
the ability to define "splitting rules" that will be used by the "Classification sources".
A classification source will then have:
  * a filing rule used to sort items by callnumbers
  * a splitting rule used to print labels

To acchieve this goal this enhancement will do the following
modifications at DB level:
* New table class_split_rules
* New column class_sources.class_split_rule

Test plan:
* Execute the update database entry to create the new table and
column.
I. UI Changes
a) Create/modify/delete a filing rule
b) Create/modify/delete a splitting rule
c) Create/modify/delete a classification source
=> A filing rule or splitting rule cannot be removed if used by a
classification source

II. Splitting rule using regular expressions
a) Create a splitting rule using the "Splitting routine" "RegEx"
b) Define several regular expressions, they will be applied one after
the other in the same order you define them.
Something like:
  s/\s/\n/g         # Break on spaces
  s/(\s?=)/\n=/g    # Break on = (unless it's done already)
  s/^(J|K)\n/$1 /   # Remove the first break if callnumber starts with J or K
c) You can test the regular expressions using filling the textarea with
a list of callnumbers. Then click "Test" and confirm the callnumbers are
split how you expected.
d) Finally create a new classification source that will use this new
splitting rule.

III. Print the label!
a) Create a layout. It should have the "Split call numbers" checkbox
ticked, and display itemcallnumber
b) Use this layout to export labels, use items with different
classification source ('lcc', 'ddc', but also the new one you have
create)
=> The callnumbers should have been split according to the regex you
defined earlier!

Notes:
* The update database entry fill the class_sources.class_split_rule
with the value of class_sources.class_sort_rule
If default rules exist it will not work, we should add a note in the
release notes (would be enough?)
* C4::ClassSplitRoutine::* should be moved to Koha::ClassSplitRule,
but it sounded better to keep the same pattern as ClassSortRoutines
* Should not we use a LONGTEXT for class_split_rules.split_regex instead
of VARCHAR(255)?

* class_sources.sql should be filled for other languages before pushed
to master!

IMPORTANT NOTES: The regular expressions are stored as it, and eval is
used to evaluate it (perlcritic raises a warning about it (Expression
form of "eval"). It can lead to serious security issues (execution of
arbitrary code on the server), especially if the modifier 'e' is used.
We could then remedy the situation with one of these following points:
- Assume that this DB data is safe (We can add a new permission?)
- Assume that the data is not safe and deal with possible attack
Cons: how be sure we are exhaustive? Making sure it matches ^s///[^e/]*$
would be enough?
- Use Template Toolkit syntax instead (Really safer?)
  [% callnumber.replace('\s', '\n').replace ... %]
- Cut the regex parts: find, replace, modifiers
like we already do for Marc modification template. Cons: we are going to
have escape problems, the "find" and "replace" parts should not be
handle the same way (think "\n", "\\n", "\1", "\s", etc.)
I did not manage to implement this one easily.
Comment 19 Jonathan Druart 2018-09-14 22:25:45 UTC
Created attachment 78795 [details] [review]
Bug 15836: Mark C4::ClassSource subs DEPRECATED

These subs are no longer used from scripts and should be removed. It
should be done on a separate bug report given that additional work is
needed
Comment 20 Jonathan Druart 2018-09-14 22:25:49 UTC
Created attachment 78796 [details] [review]
Bug 15836: Add tests
Comment 21 Jonathan Druart 2018-09-14 22:25:54 UTC
Created attachment 78798 [details] [review]
Bug 15836: Add missing svc script
Comment 22 Jonathan Druart 2018-09-14 22:25:58 UTC
Created attachment 78799 [details] [review]
Bug 15836: Handle non-existent regexs

For other types of split rules
Comment 23 Jonathan Druart 2018-09-16 23:25:57 UTC
Created attachment 79029 [details] [review]
Bug 15836: Fix display input in any situations

Fix when switching an existing rule to RegEx
Comment 24 PTFS Europe Sandboxes 2018-10-10 13:12:17 UTC
Patch tested with a sandbox, by Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 25 PTFS Europe Sandboxes 2018-10-10 13:14:33 UTC
Created attachment 80339 [details] [review]
Bug 15836: DB changes

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 26 PTFS Europe Sandboxes 2018-10-10 13:14:38 UTC
Created attachment 80340 [details] [review]
Bug 15836: DBIC Schema change

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 27 PTFS Europe Sandboxes 2018-10-10 13:14:43 UTC
Created attachment 80341 [details] [review]
Bug 15836: UI changes

The script admin/classsources.pl has been adjusted to follow usual
patterns used in other Koha scripts.
$op can be add_source, add_source_validate, etc.
Deletion steps have been removed (there is a JS confirmation box) to
simplify both script and template.

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 28 PTFS Europe Sandboxes 2018-10-10 13:14:49 UTC
Created attachment 80342 [details] [review]
Bug 15836: Add the ability to defined custom methods to split call number in labels

Currently the call number splitting seems to be mostly implemented for
DDC and LC classifications.
Those are both not very common in some countries.
A lot of libraries use their own custom classification schemes so the call number
plitting is something that should be individually configurable.

This enhancement adds the ability to define custom splitting rules based
on regular expressions.

How does it work so far?
From C4/Labels/Label.pm there are 3 differents splitting methods defined, depending on items.cn_source.
  if cn_source is "lcc' or 'nlm' we split using Library::CallNumber::LC
  if cn_source is 'ddc' we split using a in-house method
Finally there is a fallback method to split on space
And nothing else is done for other cn_source

The idea of this patch is to mimick what was done for the "filing rules" and add
the ability to define "splitting rules" that will be used by the "Classification sources".
A classification source will then have:
  * a filing rule used to sort items by callnumbers
  * a splitting rule used to print labels

To acchieve this goal this enhancement will do the following
modifications at DB level:
* New table class_split_rules
* New column class_sources.class_split_rule

Test plan:
* Execute the update database entry to create the new table and
column.
I. UI Changes
a) Create/modify/delete a filing rule
b) Create/modify/delete a splitting rule
c) Create/modify/delete a classification source
=> A filing rule or splitting rule cannot be removed if used by a
classification source

II. Splitting rule using regular expressions
a) Create a splitting rule using the "Splitting routine" "RegEx"
b) Define several regular expressions, they will be applied one after
the other in the same order you define them.
Something like:
  s/\s/\n/g         # Break on spaces
  s/(\s?=)/\n=/g    # Break on = (unless it's done already)
  s/^(J|K)\n/$1 /   # Remove the first break if callnumber starts with J or K
c) You can test the regular expressions using filling the textarea with
a list of callnumbers. Then click "Test" and confirm the callnumbers are
split how you expected.
d) Finally create a new classification source that will use this new
splitting rule.

III. Print the label!
a) Create a layout. It should have the "Split call numbers" checkbox
ticked, and display itemcallnumber
b) Use this layout to export labels, use items with different
classification source ('lcc', 'ddc', but also the new one you have
create)
=> The callnumbers should have been split according to the regex you
defined earlier!

Notes:
* The update database entry fill the class_sources.class_split_rule
with the value of class_sources.class_sort_rule
If default rules exist it will not work, we should add a note in the
release notes (would be enough?)
* C4::ClassSplitRoutine::* should be moved to Koha::ClassSplitRule,
but it sounded better to keep the same pattern as ClassSortRoutines
* Should not we use a LONGTEXT for class_split_rules.split_regex instead
of VARCHAR(255)?

* class_sources.sql should be filled for other languages before pushed
to master!

IMPORTANT NOTES: The regular expressions are stored as it, and eval is
used to evaluate it (perlcritic raises a warning about it (Expression
form of "eval"). It can lead to serious security issues (execution of
arbitrary code on the server), especially if the modifier 'e' is used.
We could then remedy the situation with one of these following points:
- Assume that this DB data is safe (We can add a new permission?)
- Assume that the data is not safe and deal with possible attack
Cons: how be sure we are exhaustive? Making sure it matches ^s///[^e/]*$
would be enough?
- Use Template Toolkit syntax instead (Really safer?)
  [% callnumber.replace('\s', '\n').replace ... %]
- Cut the regex parts: find, replace, modifiers
like we already do for Marc modification template. Cons: we are going to
have escape problems, the "find" and "replace" parts should not be
handle the same way (think "\n", "\\n", "\1", "\s", etc.)
I did not manage to implement this one easily.

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 29 PTFS Europe Sandboxes 2018-10-10 13:14:54 UTC
Created attachment 80343 [details] [review]
Bug 15836: Mark C4::ClassSource subs DEPRECATED

These subs are no longer used from scripts and should be removed. It
should be done on a separate bug report given that additional work is
needed

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 30 PTFS Europe Sandboxes 2018-10-10 13:14:59 UTC
Created attachment 80344 [details] [review]
Bug 15836: Add tests

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 31 PTFS Europe Sandboxes 2018-10-10 13:15:05 UTC
Created attachment 80345 [details] [review]
Bug 15836: Add missing svc script

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 32 PTFS Europe Sandboxes 2018-10-10 13:15:11 UTC
Created attachment 80346 [details] [review]
Bug 15836: Handle non-existent regexs

For other types of split rules

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 33 PTFS Europe Sandboxes 2018-10-10 13:15:17 UTC
Created attachment 80347 [details] [review]
Bug 15836: Fix display input in any situations

Fix when switching an existing rule to RegEx

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 34 Josef Moravec 2018-10-12 09:36:55 UTC
Hi Jonathan, 

I have few questions:

1. Why do you decide to make new modules under C4 namespace? 

2. The patches are not applicable on top of bug 14931, should it depend on it?
Comment 35 Jonathan Druart 2018-10-12 14:50:51 UTC
(In reply to Josef Moravec from comment #34)
> Hi Jonathan, 
> 
> I have few questions:
> 
> 1. Why do you decide to make new modules under C4 namespace? 

I have mimicked what was done in C4::ClassSortRules. It sounded better to keep them tied at the same place (ClassSource.pm, ClassSortRoutine.pm and ClassSplitRoutine.pm)

> 2. The patches are not applicable on top of bug 14931, should it depend on
> it?

It's bug 14391, and it does not apply anymore. I will rebase on top when ready.
Comment 36 Jonathan Druart 2018-10-12 18:31:13 UTC
Created attachment 80548 [details] [review]
Bug 15836: DB changes

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 37 Jonathan Druart 2018-10-12 18:31:20 UTC
Created attachment 80549 [details] [review]
Bug 15836: DBIC Schema change

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 38 Jonathan Druart 2018-10-12 18:31:28 UTC
Created attachment 80550 [details] [review]
Bug 15836: UI changes

The script admin/classsources.pl has been adjusted to follow usual
patterns used in other Koha scripts.
$op can be add_source, add_source_validate, etc.
Deletion steps have been removed (there is a JS confirmation box) to
simplify both script and template.

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 39 Jonathan Druart 2018-10-12 18:31:35 UTC
Created attachment 80551 [details] [review]
Bug 15836: Add the ability to defined custom methods to split call number in labels

Currently the call number splitting seems to be mostly implemented for
DDC and LC classifications.
Those are both not very common in some countries.
A lot of libraries use their own custom classification schemes so the call number
plitting is something that should be individually configurable.

This enhancement adds the ability to define custom splitting rules based
on regular expressions.

How does it work so far?
From C4/Labels/Label.pm there are 3 differents splitting methods defined, depending on items.cn_source.
  if cn_source is "lcc' or 'nlm' we split using Library::CallNumber::LC
  if cn_source is 'ddc' we split using a in-house method
Finally there is a fallback method to split on space
And nothing else is done for other cn_source

The idea of this patch is to mimick what was done for the "filing rules" and add
the ability to define "splitting rules" that will be used by the "Classification sources".
A classification source will then have:
  * a filing rule used to sort items by callnumbers
  * a splitting rule used to print labels

To acchieve this goal this enhancement will do the following
modifications at DB level:
* New table class_split_rules
* New column class_sources.class_split_rule

Test plan:
* Execute the update database entry to create the new table and
column.
I. UI Changes
a) Create/modify/delete a filing rule
b) Create/modify/delete a splitting rule
c) Create/modify/delete a classification source
=> A filing rule or splitting rule cannot be removed if used by a
classification source

II. Splitting rule using regular expressions
a) Create a splitting rule using the "Splitting routine" "RegEx"
b) Define several regular expressions, they will be applied one after
the other in the same order you define them.
Something like:
  s/\s/\n/g         # Break on spaces
  s/(\s?=)/\n=/g    # Break on = (unless it's done already)
  s/^(J|K)\n/$1 /   # Remove the first break if callnumber starts with J or K
c) You can test the regular expressions using filling the textarea with
a list of callnumbers. Then click "Test" and confirm the callnumbers are
split how you expected.
d) Finally create a new classification source that will use this new
splitting rule.

III. Print the label!
a) Create a layout. It should have the "Split call numbers" checkbox
ticked, and display itemcallnumber
b) Use this layout to export labels, use items with different
classification source ('lcc', 'ddc', but also the new one you have
create)
=> The callnumbers should have been split according to the regex you
defined earlier!

Notes:
* The update database entry fill the class_sources.class_split_rule
with the value of class_sources.class_sort_rule
If default rules exist it will not work, we should add a note in the
release notes (would be enough?)
* C4::ClassSplitRoutine::* should be moved to Koha::ClassSplitRule,
but it sounded better to keep the same pattern as ClassSortRoutines
* Should not we use a LONGTEXT for class_split_rules.split_regex instead
of VARCHAR(255)?

* class_sources.sql should be filled for other languages before pushed
to master!

IMPORTANT NOTES: The regular expressions are stored as it, and eval is
used to evaluate it (perlcritic raises a warning about it (Expression
form of "eval"). It can lead to serious security issues (execution of
arbitrary code on the server), especially if the modifier 'e' is used.
We could then remedy the situation with one of these following points:
- Assume that this DB data is safe (We can add a new permission?)
- Assume that the data is not safe and deal with possible attack
Cons: how be sure we are exhaustive? Making sure it matches ^s///[^e/]*$
would be enough?
- Use Template Toolkit syntax instead (Really safer?)
  [% callnumber.replace('\s', '\n').replace ... %]
- Cut the regex parts: find, replace, modifiers
like we already do for Marc modification template. Cons: we are going to
have escape problems, the "find" and "replace" parts should not be
handle the same way (think "\n", "\\n", "\1", "\s", etc.)
I did not manage to implement this one easily.

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 40 Jonathan Druart 2018-10-12 18:31:42 UTC
Created attachment 80552 [details] [review]
Bug 15836: Mark C4::ClassSource subs DEPRECATED

These subs are no longer used from scripts and should be removed. It
should be done on a separate bug report given that additional work is
needed

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 41 Jonathan Druart 2018-10-12 18:31:48 UTC
Created attachment 80553 [details] [review]
Bug 15836: Add tests

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 42 Jonathan Druart 2018-10-12 18:31:55 UTC
Created attachment 80554 [details] [review]
Bug 15836: Add missing svc script

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 43 Jonathan Druart 2018-10-12 18:32:02 UTC
Created attachment 80555 [details] [review]
Bug 15836: Handle non-existent regexs

For other types of split rules

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 44 Jonathan Druart 2018-10-12 18:32:10 UTC
Created attachment 80556 [details] [review]
Bug 15836: Fix display input in any situations

Fix when switching an existing rule to RegEx

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 45 Chris Cormack 2018-10-23 20:15:39 UTC
Created attachment 81033 [details] [review]
Bug 15836: DB changes

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 46 Chris Cormack 2018-10-23 20:15:50 UTC
Created attachment 81034 [details] [review]
Bug 15836: DBIC Schema change

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 47 Chris Cormack 2018-10-23 20:16:01 UTC
Created attachment 81035 [details] [review]
Bug 15836: UI changes

The script admin/classsources.pl has been adjusted to follow usual
patterns used in other Koha scripts.
$op can be add_source, add_source_validate, etc.
Deletion steps have been removed (there is a JS confirmation box) to
simplify both script and template.

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 48 Chris Cormack 2018-10-23 20:16:13 UTC
Created attachment 81036 [details] [review]
Bug 15836: Add the ability to defined custom methods to split call number in labels

Currently the call number splitting seems to be mostly implemented for
DDC and LC classifications.
Those are both not very common in some countries.
A lot of libraries use their own custom classification schemes so the call number
plitting is something that should be individually configurable.

This enhancement adds the ability to define custom splitting rules based
on regular expressions.

How does it work so far?
From C4/Labels/Label.pm there are 3 differents splitting methods defined, depending on items.cn_source.
  if cn_source is "lcc' or 'nlm' we split using Library::CallNumber::LC
  if cn_source is 'ddc' we split using a in-house method
Finally there is a fallback method to split on space
And nothing else is done for other cn_source

The idea of this patch is to mimick what was done for the "filing rules" and add
the ability to define "splitting rules" that will be used by the "Classification sources".
A classification source will then have:
  * a filing rule used to sort items by callnumbers
  * a splitting rule used to print labels

To acchieve this goal this enhancement will do the following
modifications at DB level:
* New table class_split_rules
* New column class_sources.class_split_rule

Test plan:
* Execute the update database entry to create the new table and
column.
I. UI Changes
a) Create/modify/delete a filing rule
b) Create/modify/delete a splitting rule
c) Create/modify/delete a classification source
=> A filing rule or splitting rule cannot be removed if used by a
classification source

II. Splitting rule using regular expressions
a) Create a splitting rule using the "Splitting routine" "RegEx"
b) Define several regular expressions, they will be applied one after
the other in the same order you define them.
Something like:
  s/\s/\n/g         # Break on spaces
  s/(\s?=)/\n=/g    # Break on = (unless it's done already)
  s/^(J|K)\n/$1 /   # Remove the first break if callnumber starts with J or K
c) You can test the regular expressions using filling the textarea with
a list of callnumbers. Then click "Test" and confirm the callnumbers are
split how you expected.
d) Finally create a new classification source that will use this new
splitting rule.

III. Print the label!
a) Create a layout. It should have the "Split call numbers" checkbox
ticked, and display itemcallnumber
b) Use this layout to export labels, use items with different
classification source ('lcc', 'ddc', but also the new one you have
create)
=> The callnumbers should have been split according to the regex you
defined earlier!

Notes:
* The update database entry fill the class_sources.class_split_rule
with the value of class_sources.class_sort_rule
If default rules exist it will not work, we should add a note in the
release notes (would be enough?)
* C4::ClassSplitRoutine::* should be moved to Koha::ClassSplitRule,
but it sounded better to keep the same pattern as ClassSortRoutines
* Should not we use a LONGTEXT for class_split_rules.split_regex instead
of VARCHAR(255)?

* class_sources.sql should be filled for other languages before pushed
to master!

IMPORTANT NOTES: The regular expressions are stored as it, and eval is
used to evaluate it (perlcritic raises a warning about it (Expression
form of "eval"). It can lead to serious security issues (execution of
arbitrary code on the server), especially if the modifier 'e' is used.
We could then remedy the situation with one of these following points:
- Assume that this DB data is safe (We can add a new permission?)
- Assume that the data is not safe and deal with possible attack
Cons: how be sure we are exhaustive? Making sure it matches ^s///[^e/]*$
would be enough?
- Use Template Toolkit syntax instead (Really safer?)
  [% callnumber.replace('\s', '\n').replace ... %]
- Cut the regex parts: find, replace, modifiers
like we already do for Marc modification template. Cons: we are going to
have escape problems, the "find" and "replace" parts should not be
handle the same way (think "\n", "\\n", "\1", "\s", etc.)
I did not manage to implement this one easily.

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 49 Chris Cormack 2018-10-23 20:16:23 UTC
Created attachment 81037 [details] [review]
Bug 15836: Mark C4::ClassSource subs DEPRECATED

These subs are no longer used from scripts and should be removed. It
should be done on a separate bug report given that additional work is
needed

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 50 Chris Cormack 2018-10-23 20:16:33 UTC
Created attachment 81038 [details] [review]
Bug 15836: Add tests

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 51 Chris Cormack 2018-10-23 20:17:24 UTC
Created attachment 81039 [details] [review]
Bug 15836: Add missing svc script

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 52 Chris Cormack 2018-10-23 20:17:52 UTC
Created attachment 81040 [details] [review]
Bug 15836: Handle non-existent regexs

For other types of split rules

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 53 Chris Cormack 2018-10-23 20:18:05 UTC
Created attachment 81041 [details] [review]
Bug 15836: Fix display input in any situations

Fix when switching an existing rule to RegEx

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 54 Jonathan Druart 2018-10-30 15:55:31 UTC
Created attachment 81627 [details] [review]
Bug 15836: DB changes

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 55 Jonathan Druart 2018-10-30 15:55:39 UTC
Created attachment 81628 [details] [review]
Bug 15836: DBIC Schema change

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 56 Jonathan Druart 2018-10-30 15:55:53 UTC
Created attachment 81629 [details] [review]
Bug 15836: UI changes

The script admin/classsources.pl has been adjusted to follow usual
patterns used in other Koha scripts.
$op can be add_source, add_source_validate, etc.
Deletion steps have been removed (there is a JS confirmation box) to
simplify both script and template.

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 57 Jonathan Druart 2018-10-30 15:56:05 UTC
Created attachment 81630 [details] [review]
Bug 15836: Add the ability to defined custom methods to split call number in labels

Currently the call number splitting seems to be mostly implemented for
DDC and LC classifications.
Those are both not very common in some countries.
A lot of libraries use their own custom classification schemes so the call number
plitting is something that should be individually configurable.

This enhancement adds the ability to define custom splitting rules based
on regular expressions.

How does it work so far?
From C4/Labels/Label.pm there are 3 differents splitting methods defined, depending on items.cn_source.
  if cn_source is "lcc' or 'nlm' we split using Library::CallNumber::LC
  if cn_source is 'ddc' we split using a in-house method
Finally there is a fallback method to split on space
And nothing else is done for other cn_source

The idea of this patch is to mimick what was done for the "filing rules" and add
the ability to define "splitting rules" that will be used by the "Classification sources".
A classification source will then have:
  * a filing rule used to sort items by callnumbers
  * a splitting rule used to print labels

To acchieve this goal this enhancement will do the following
modifications at DB level:
* New table class_split_rules
* New column class_sources.class_split_rule

Test plan:
* Execute the update database entry to create the new table and
column.
I. UI Changes
a) Create/modify/delete a filing rule
b) Create/modify/delete a splitting rule
c) Create/modify/delete a classification source
=> A filing rule or splitting rule cannot be removed if used by a
classification source

II. Splitting rule using regular expressions
a) Create a splitting rule using the "Splitting routine" "RegEx"
b) Define several regular expressions, they will be applied one after
the other in the same order you define them.
Something like:
  s/\s/\n/g         # Break on spaces
  s/(\s?=)/\n=/g    # Break on = (unless it's done already)
  s/^(J|K)\n/$1 /   # Remove the first break if callnumber starts with J or K
c) You can test the regular expressions using filling the textarea with
a list of callnumbers. Then click "Test" and confirm the callnumbers are
split how you expected.
d) Finally create a new classification source that will use this new
splitting rule.

III. Print the label!
a) Create a layout. It should have the "Split call numbers" checkbox
ticked, and display itemcallnumber
b) Use this layout to export labels, use items with different
classification source ('lcc', 'ddc', but also the new one you have
create)
=> The callnumbers should have been split according to the regex you
defined earlier!

Notes:
* The update database entry fill the class_sources.class_split_rule
with the value of class_sources.class_sort_rule
If default rules exist it will not work, we should add a note in the
release notes (would be enough?)
* C4::ClassSplitRoutine::* should be moved to Koha::ClassSplitRule,
but it sounded better to keep the same pattern as ClassSortRoutines
* Should not we use a LONGTEXT for class_split_rules.split_regex instead
of VARCHAR(255)?

* class_sources.sql should be filled for other languages before pushed
to master!

IMPORTANT NOTES: The regular expressions are stored as it, and eval is
used to evaluate it (perlcritic raises a warning about it (Expression
form of "eval"). It can lead to serious security issues (execution of
arbitrary code on the server), especially if the modifier 'e' is used.
We could then remedy the situation with one of these following points:
- Assume that this DB data is safe (We can add a new permission?)
- Assume that the data is not safe and deal with possible attack
Cons: how be sure we are exhaustive? Making sure it matches ^s///[^e/]*$
would be enough?
- Use Template Toolkit syntax instead (Really safer?)
  [% callnumber.replace('\s', '\n').replace ... %]
- Cut the regex parts: find, replace, modifiers
like we already do for Marc modification template. Cons: we are going to
have escape problems, the "find" and "replace" parts should not be
handle the same way (think "\n", "\\n", "\1", "\s", etc.)
I did not manage to implement this one easily.

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 58 Jonathan Druart 2018-10-30 15:56:15 UTC
Created attachment 81631 [details] [review]
Bug 15836: Mark C4::ClassSource subs DEPRECATED

These subs are no longer used from scripts and should be removed. It
should be done on a separate bug report given that additional work is
needed

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 59 Jonathan Druart 2018-10-30 15:56:23 UTC
Created attachment 81632 [details] [review]
Bug 15836: Add tests

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 60 Jonathan Druart 2018-10-30 15:56:32 UTC
Created attachment 81633 [details] [review]
Bug 15836: Add missing svc script

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 61 Jonathan Druart 2018-10-30 15:56:40 UTC
Created attachment 81634 [details] [review]
Bug 15836: Handle non-existent regexs

For other types of split rules

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 62 Jonathan Druart 2018-10-30 15:56:50 UTC
Created attachment 81635 [details] [review]
Bug 15836: Fix display input in any situations

Fix when switching an existing rule to RegEx

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 63 Jonathan Druart 2018-10-30 16:00:28 UTC
I have added the "Sponsored-by" line" to the patches.
Comment 64 Katrin Fischer 2018-10-30 17:34:22 UTC
Thank you, Joubu!
Comment 65 Jonathan Druart 2018-11-08 19:22:50 UTC
Created attachment 82110 [details] [review]
Bug 15836: Fix update DB entry
Comment 66 Jonathan Druart 2018-11-08 19:45:37 UTC
Created attachment 82112 [details] [review]
Bug 15836: Fix DB structure

That was a leftover from the first approach, this table must not be
modified!
Comment 67 Jonathan Druart 2018-11-08 19:45:45 UTC
Created attachment 82113 [details] [review]
Bug 15836: DBIC Schema change
Comment 68 Katrin Fischer 2018-11-08 20:54:22 UTC
Created attachment 82118 [details] [review]
Bug 15836: Fix DB structure

That was a leftover from the first approach, this table must not be
modified!

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 69 Katrin Fischer 2018-11-08 20:54:29 UTC
Created attachment 82119 [details] [review]
Bug 15836: DBIC Schema change

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 70 Katrin Fischer 2018-11-08 20:54:37 UTC
Created attachment 82120 [details] [review]
Bug 15836: (follow-up) Fix filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 71 Katrin Fischer 2018-11-08 20:55:08 UTC
Retested on a fresh database created running the web installer with all sample data.
Comment 72 Nick Clemens 2018-11-13 14:14:41 UTC
I tried my best here but am still having issues

Minor conflicts on filters

After this patch I cannot delete classification sources, I end up on a blank page

There is debug code in /home/vagrant/kohaclone/svc/split_callnumbers (Data printer line 14)

I cannot save regex rules - I can add and test, but they are lost when I hit save.

Possibly I messed up something when fixing filters, can you please double check?
Comment 73 Jonathan Druart 2018-11-13 15:05:23 UTC
Created attachment 82289 [details] [review]
Bug 15836: DB changes

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 74 Jonathan Druart 2018-11-13 15:05:30 UTC
Created attachment 82290 [details] [review]
Bug 15836: UI changes

The script admin/classsources.pl has been adjusted to follow usual
patterns used in other Koha scripts.
$op can be add_source, add_source_validate, etc.
Deletion steps have been removed (there is a JS confirmation box) to
simplify both script and template.

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 75 Jonathan Druart 2018-11-13 15:05:37 UTC
Created attachment 82291 [details] [review]
Bug 15836: Add the ability to defined custom methods to split call number in labels

Currently the call number splitting seems to be mostly implemented for
DDC and LC classifications.
Those are both not very common in some countries.
A lot of libraries use their own custom classification schemes so the call number
plitting is something that should be individually configurable.

This enhancement adds the ability to define custom splitting rules based
on regular expressions.

How does it work so far?
From C4/Labels/Label.pm there are 3 differents splitting methods defined, depending on items.cn_source.
  if cn_source is "lcc' or 'nlm' we split using Library::CallNumber::LC
  if cn_source is 'ddc' we split using a in-house method
Finally there is a fallback method to split on space
And nothing else is done for other cn_source

The idea of this patch is to mimick what was done for the "filing rules" and add
the ability to define "splitting rules" that will be used by the "Classification sources".
A classification source will then have:
  * a filing rule used to sort items by callnumbers
  * a splitting rule used to print labels

To acchieve this goal this enhancement will do the following
modifications at DB level:
* New table class_split_rules
* New column class_sources.class_split_rule

Test plan:
* Execute the update database entry to create the new table and
column.
I. UI Changes
a) Create/modify/delete a filing rule
b) Create/modify/delete a splitting rule
c) Create/modify/delete a classification source
=> A filing rule or splitting rule cannot be removed if used by a
classification source

II. Splitting rule using regular expressions
a) Create a splitting rule using the "Splitting routine" "RegEx"
b) Define several regular expressions, they will be applied one after
the other in the same order you define them.
Something like:
  s/\s/\n/g         # Break on spaces
  s/(\s?=)/\n=/g    # Break on = (unless it's done already)
  s/^(J|K)\n/$1 /   # Remove the first break if callnumber starts with J or K
c) You can test the regular expressions using filling the textarea with
a list of callnumbers. Then click "Test" and confirm the callnumbers are
split how you expected.
d) Finally create a new classification source that will use this new
splitting rule.

III. Print the label!
a) Create a layout. It should have the "Split call numbers" checkbox
ticked, and display itemcallnumber
b) Use this layout to export labels, use items with different
classification source ('lcc', 'ddc', but also the new one you have
create)
=> The callnumbers should have been split according to the regex you
defined earlier!

Notes:
* The update database entry fill the class_sources.class_split_rule
with the value of class_sources.class_sort_rule
If default rules exist it will not work, we should add a note in the
release notes (would be enough?)
* C4::ClassSplitRoutine::* should be moved to Koha::ClassSplitRule,
but it sounded better to keep the same pattern as ClassSortRoutines
* Should not we use a LONGTEXT for class_split_rules.split_regex instead
of VARCHAR(255)?

* class_sources.sql should be filled for other languages before pushed
to master!

IMPORTANT NOTES: The regular expressions are stored as it, and eval is
used to evaluate it (perlcritic raises a warning about it (Expression
form of "eval"). It can lead to serious security issues (execution of
arbitrary code on the server), especially if the modifier 'e' is used.
We could then remedy the situation with one of these following points:
- Assume that this DB data is safe (We can add a new permission?)
- Assume that the data is not safe and deal with possible attack
Cons: how be sure we are exhaustive? Making sure it matches ^s///[^e/]*$
would be enough?
- Use Template Toolkit syntax instead (Really safer?)
  [% callnumber.replace('\s', '\n').replace ... %]
- Cut the regex parts: find, replace, modifiers
like we already do for Marc modification template. Cons: we are going to
have escape problems, the "find" and "replace" parts should not be
handle the same way (think "\n", "\\n", "\1", "\s", etc.)
I did not manage to implement this one easily.

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 76 Jonathan Druart 2018-11-13 15:05:43 UTC
Created attachment 82292 [details] [review]
Bug 15836: Mark C4::ClassSource subs DEPRECATED

These subs are no longer used from scripts and should be removed. It
should be done on a separate bug report given that additional work is
needed

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 77 Jonathan Druart 2018-11-13 15:05:50 UTC
Created attachment 82293 [details] [review]
Bug 15836: Add tests

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 78 Jonathan Druart 2018-11-13 15:05:58 UTC
Created attachment 82294 [details] [review]
Bug 15836: Add missing svc script

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 79 Jonathan Druart 2018-11-13 15:06:04 UTC
Created attachment 82295 [details] [review]
Bug 15836: Handle non-existent regexs

For other types of split rules

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 80 Jonathan Druart 2018-11-13 15:06:10 UTC
Created attachment 82296 [details] [review]
Bug 15836: Fix display input in any situations

Fix when switching an existing rule to RegEx

Sponsored-by: Goethe-Institut

Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 81 Jonathan Druart 2018-11-13 15:06:17 UTC
Created attachment 82297 [details] [review]
Bug 15836: Fix DB structure

That was a leftover from the first approach, this table must not be
modified!

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 82 Jonathan Druart 2018-11-13 15:06:23 UTC
Created attachment 82298 [details] [review]
Bug 15836: DBIC Schema change

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 83 Jonathan Druart 2018-11-13 15:06:29 UTC
Created attachment 82299 [details] [review]
Bug 15836: (follow-up) Fix filters

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 84 Jonathan Druart 2018-11-13 15:06:35 UTC
Created attachment 82300 [details] [review]
Bug 15836: Fix deletion of class sources
Comment 85 Jonathan Druart 2018-11-13 15:06:41 UTC
Created attachment 82301 [details] [review]
Bug 15836: Remove debug statement
Comment 86 Jonathan Druart 2018-11-13 15:06:47 UTC
Created attachment 82302 [details] [review]
Bug 15836: (follow-up) Fix display input in any situations

Koha::ClassSplitRule->regexs returns an array
Comment 87 Nick Clemens 2018-11-13 16:38:13 UTC
Awesome work all!

Pushed to master for 18.11
Comment 88 Jonathan Druart 2018-11-13 17:39:59 UTC
Created attachment 82317 [details] [review]
Bug 15836: Fix failing tests
Comment 89 Jonathan Druart 2018-11-13 17:40:06 UTC
Created attachment 82318 [details] [review]
Bug 15836: Remove deprecated subroutines

It will take too much time to fix the failures here, given the
simplicity of the subroutines I would be in favor of deleting them and
continue the move on a separate bug report to use Koha::Object-objects
Comment 90 Jonathan Druart 2018-11-13 17:42:42 UTC
Created attachment 82319 [details] [review]
Bug 15836: Remove deprecated subroutines

It will take too much time to fix the failures here, given the
simplicity of the subroutines I would be in favor of deleting them and
continue the move on a separate bug report to use Koha::Object-objects
Comment 91 Katrin Fischer 2018-11-13 18:35:23 UTC
Created attachment 82321 [details] [review]
Bug 15836: Remove deprecated subroutines

It will take too much time to fix the failures here, given the
simplicity of the subroutines I would be in favor of deleting them and
continue the move on a separate bug report to use Koha::Object-objects

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I've checked that everything removed is no longer in use.
Comment 92 Nick Clemens 2018-11-13 18:57:28 UTC
(In reply to Katrin Fischer from comment #91)
> Created attachment 82321 [details] [review] [review]
> Bug 15836: Remove deprecated subroutines
> 
> It will take too much time to fix the failures here, given the
> simplicity of the subroutines I would be in favor of deleting them and
> continue the move on a separate bug report to use Koha::Object-objects
> 
> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
> I've checked that everything removed is no longer in use.

Last two patches pushed to master
Comment 93 Martin Renvoize 2018-11-16 14:24:05 UTC
Enhancement, will not be backported to 18.05.x series.