Description
Marcel de Rooy
2019-05-28 13:30:06 UTC
Created attachment 90155 [details] [review] Bug 22996: Add new preference BarcodeSeparators Test plan: Run new install or upgrade. Check pref value. 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. I think the separator should be selected from the interface, it depends on user's preference. (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 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> 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> 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> 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> 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. (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? (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. (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? 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> 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> Nice work! Pushed to master for 19.11.00 (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. Enhancement not pushed to 19.05.x |