Bug 22996

Summary: Move barcode separators to a preference
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: ToolsAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19585
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23580
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23254
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Adds preference `BarcodeSeparators` **NOTE**: If you currently depend on a comma, semicolon, pipe character or hyphen as a barcode separator within inventory tool, please ADD them to this new preference. The default behaviour is set back to a carriage return, linefeed or whitespace now.
Version(s) released in:
19.11.00
Attachments: Bug 22996: Add new preference BarcodeSeparators
Bug 22996: Move barcode separators to a preference
Bug 22996: Add new preference BarcodeSeparators
Bug 22996: Move barcode separators to a preference
Bug 22996: Add new preference BarcodeSeparators
Bug 22996: Move barcode separators to a preference
Bug 22996: Add new preference BarcodeSeparators
Bug 22996: Move barcode separators to a preference

Description Marcel de Rooy 2019-05-28 13:30:06 UTC

    
Comment 1 Marcel de Rooy 2019-05-28 14:57:36 UTC
Created attachment 90155 [details] [review]
Bug 22996: Add new preference BarcodeSeparators

Test plan:
Run new install or upgrade.
Check pref value.
Comment 2 Marcel de Rooy 2019-05-28 14:57:39 UTC
Created attachment 90156 [details] [review]
Bug 22996: Move barcode separators to a preference

This patch makes batchMod.pl and inventory.pl pick the barcode
separators from a new pref BarcodeSeparators (instead of hardcoding
these separators differently).
A few other code locations may be potential candidates for such a change
too.

Test plan:
Test inventory with a few variations of BarcodeSeparators.
Test Batch item modification similarly.
Comment 3 Jonathan Druart 2019-05-29 12:44:39 UTC
I think the separator should be selected from the interface, it depends on user's preference.
Comment 4 Marcel de Rooy 2019-05-29 13:42:22 UTC
(In reply to Jonathan Druart from comment #3)
> I think the separator should be selected from the interface, it depends on
> user's preference.

Not sure if that would be really relevant, since the barcodes belong to one system. But yes, it could be done, perhaps with the pref as default value.
But I am suggesting to open a new report for that enhancement. The focus of this development is consolidation of the approach in inventory and batchmod.

Possible candidates for future extension:
circ/circulation.pl:        push @$barcodes, split( /\s\n/, $list );
circ/circulation.pl:                    foreach my $barcode ( sort split(/\s*\|\s*/, $chosen->{barcode}) ) {
course_reserves/batch_add_items.pl:        my @barcodes = uniq( split( /\s\n/, $barcodes ) );
svc/letters/preview:    my ( $barcode, $borrowernumber ) = split '\|', $data_preview;
tools/stockrotation.pl:            my @data = split(/\n/, $barcode);
virtualshelves/shelves.pl:                my @barcodes = split /\n/, $barcodes; # Entries are effectively passed in as a <cr> separated list
Comment 5 Marcel de Rooy 2019-05-29 13:42:47 UTC
Created attachment 90185 [details] [review]
Bug 22996: Add new preference BarcodeSeparators

The pref value is used within a regex character class like [pref].
This means that we should precede hyphen with a backslash; the pipe char
or dot does not need escaping.

Test plan:
Run new install or upgrade.
Check pref value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2019-05-29 13:42:51 UTC
Created attachment 90186 [details] [review]
Bug 22996: Move barcode separators to a preference

This patch makes batchMod.pl and inventory.pl pick the barcode
separators from a new pref BarcodeSeparators (instead of hardcoding
these separators differently).
A few other code locations may be potential candidates for such a change
too.

Test plan:
Test inventory with a few variations of BarcodeSeparators.
Test Batch item modification similarly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Kyle M Hall 2019-09-06 11:27:48 UTC
Created attachment 92636 [details] [review]
Bug 22996: Add new preference BarcodeSeparators

The pref value is used within a regex character class like [pref].
This means that we should precede hyphen with a backslash; the pipe char
or dot does not need escaping.

Test plan:
Run new install or upgrade.
Check pref value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2019-09-06 11:28:04 UTC
Created attachment 92637 [details] [review]
Bug 22996: Move barcode separators to a preference

This patch makes batchMod.pl and inventory.pl pick the barcode
separators from a new pref BarcodeSeparators (instead of hardcoding
these separators differently).
A few other code locations may be potential candidates for such a change
too.

Test plan:
Test inventory with a few variations of BarcodeSeparators.
Test Batch item modification similarly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Tomás Cohen Arazi 2019-09-06 18:33:04 UTC
Marcel, depending on the flexibility we want, I would suggest we use a different approach (less permissive):

Barcode separator characters:
    -
        - Use the following characters as barcode separators:
        - pref: BarcodeSeparators
          choices:
              '\s': space
              '\t': tab
..

Willing to PQA on this one as a first step, but would like your opinion.
Comment 10 Marcel de Rooy 2019-09-09 07:03:06 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> Marcel, depending on the flexibility we want, I would suggest we use a
> different approach (less permissive):
> 
> Barcode separator characters:
>     -
>         - Use the following characters as barcode separators:
>         - pref: BarcodeSeparators
>           choices:
>               '\s': space
>               '\t': tab
> ..
> 
> Willing to PQA on this one as a first step, but would like your opinion.

The default is whitespace, LF or CR. Almost as restrictive as yours ;)
The inventory script already had: /[\n\r,;|-]/
So I think the current approach allows for all tastes.
Why cut in flexibility?
Comment 11 Tomás Cohen Arazi 2019-09-09 13:34:36 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Tomás Cohen Arazi from comment #9)
> > Marcel, depending on the flexibility we want, I would suggest we use a
> > different approach (less permissive):
> > 
> > Barcode separator characters:
> >     -
> >         - Use the following characters as barcode separators:
> >         - pref: BarcodeSeparators
> >           choices:
> >               '\s': space
> >               '\t': tab
> > ..
> > 
> > Willing to PQA on this one as a first step, but would like your opinion.
> 
> The default is whitespace, LF or CR. Almost as restrictive as yours ;)
> The inventory script already had: /[\n\r,;|-]/
> So I think the current approach allows for all tastes.
> Why cut in flexibility?

I hate error prone (typo) configurations, but hey, this is an enhancement to the currently hardcoded stuff.
Comment 12 Tomás Cohen Arazi 2019-09-09 13:40:49 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Tomás Cohen Arazi from comment #9)
> > Marcel, depending on the flexibility we want, I would suggest we use a
> > different approach (less permissive):
> > 
> > Barcode separator characters:
> >     -
> >         - Use the following characters as barcode separators:
> >         - pref: BarcodeSeparators
> >           choices:
> >               '\s': space
> >               '\t': tab
> > ..
> > 
> > Willing to PQA on this one as a first step, but would like your opinion.
> 
> The default is whitespace, LF or CR. Almost as restrictive as yours ;)
> The inventory script already had: /[\n\r,;|-]/
> So I think the current approach allows for all tastes.
> Why cut in flexibility?

Would you object to makes this a YAML list so no 'split' is required on the controllers?
Comment 13 Tomás Cohen Arazi 2019-09-09 16:55:32 UTC
Created attachment 92668 [details] [review]
Bug 22996: Add new preference BarcodeSeparators

The pref value is used within a regex character class like [pref].
This means that we should precede hyphen with a backslash; the pipe char
or dot does not need escaping.

Test plan:
Run new install or upgrade.
Check pref value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2019-09-09 16:55:37 UTC
Created attachment 92669 [details] [review]
Bug 22996: Move barcode separators to a preference

This patch makes batchMod.pl and inventory.pl pick the barcode
separators from a new pref BarcodeSeparators (instead of hardcoding
these separators differently).
A few other code locations may be potential candidates for such a change
too.

Test plan:
Test inventory with a few variations of BarcodeSeparators.
Test Batch item modification similarly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Martin Renvoize 2019-09-10 08:36:43 UTC
Nice work!

Pushed to master for 19.11.00
Comment 16 Marcel de Rooy 2019-09-10 10:51:37 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> Would you object to makes this a YAML list so no 'split' is required on the
> controllers?

Thx Tomas. Sure, no objection. Open up a new report and show what you mean. I will be happy to sign off.
Comment 17 Fridolin Somers 2019-09-13 12:49:39 UTC
Enhancement not pushed to 19.05.x