Bug 22996 - Move barcode separators to a preference
Summary: Move barcode separators to a preference
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-28 13:30 UTC by Marcel de Rooy
Modified: 2020-11-30 21:44 UTC (History)
4 users (show)

See Also:
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 (3.05 KB, patch)
2019-05-28 14:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22996: Move barcode separators to a preference (3.38 KB, patch)
2019-05-28 14:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22996: Add new preference BarcodeSeparators (3.30 KB, patch)
2019-05-29 13:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22996: Move barcode separators to a preference (3.44 KB, patch)
2019-05-29 13:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22996: Add new preference BarcodeSeparators (3.36 KB, patch)
2019-09-06 11:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22996: Move barcode separators to a preference (3.57 KB, patch)
2019-09-06 11:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22996: Add new preference BarcodeSeparators (3.40 KB, patch)
2019-09-09 16:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22996: Move barcode separators to a preference (3.61 KB, patch)
2019-09-09 16:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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