Bug 26351 - Add plugin hooks to transform item barcodes
Summary: Add plugin hooks to transform item barcodes
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 26352
  Show dependency treegraph
 
Reported: 2020-09-02 13:31 UTC by Kyle M Hall
Modified: 2020-09-11 12:38 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26351: Add plugin hooks to transform item barcodes (11.88 KB, patch)
2020-09-02 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26351: Add plugin hooks to transform item barcodes (12.06 KB, patch)
2020-09-02 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26351: (QA follow-up) Fix QA script issue (992 bytes, patch)
2020-09-04 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-09-02 13:31:16 UTC
Moved from bug 25712

Some of our partners have unusual barcode requirements that have required us to transform scanned barcodes using javascript. This is not the most reliable method. It would make more sense to have Koha transform the barcodes on the backend using a plugin. We should add hooks to transform and generate new item and patron barcodes.
Comment 1 Kyle M Hall 2020-09-02 13:33:18 UTC
Created attachment 109532 [details] [review]
Bug 26351: Add plugin hooks to transform item barcodes

Some of our partners have unusual barcode requirements that have
required us to transform scanned barcodes using javascript. This is not
the most reliable method. It would make more sense to have Koha
transform the barcodes on the backend using a plugin. We should add
hooks to transform and generate new item and patron barcodes.

Test Plan:
1) Apply this patch
2) Download and install the Barcode Transformer plugin
   https://github.com/bywatersolutions/koha-plugin-barcode-transformer/releases/
3) Go to the plugin configuration page, set the configuration to the example configuration from the same page
4) In the item barcode field on the checkin and checkout pages,
   and anywhere else you can scan an item barcode, type in some
   valid barcodes, but prefix them with X and postfix them with
   Y, e.g. X123456Y
5) Note the letters are removed by Koha!

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 2 Kyle M Hall 2020-09-02 15:29:26 UTC
Created attachment 109548 [details] [review]
Bug 26351: Add plugin hooks to transform item barcodes

Some of our partners have unusual barcode requirements that have
required us to transform scanned barcodes using javascript. This is not
the most reliable method. It would make more sense to have Koha
transform the barcodes on the backend using a plugin. We should add
hooks to transform and generate new item and patron barcodes.

Test Plan:
1) Apply this patch
2) Download and install the Barcode Transformer plugin
   https://github.com/bywatersolutions/koha-plugin-barcode-transformer/releases/
3) Go to the plugin configuration page, set the configuration to the example configuration from the same page
4) In the item barcode field on the checkin and checkout pages,
   and anywhere else you can scan an item barcode, type in some
   valid barcodes, but prefix them with X and postfix them with
   Y, e.g. X123456Y
5) Note the letters are removed by Koha!

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 3 Katrin Fischer 2020-09-03 20:59:45 UTC
Lacking the time to test deeply here tonight, but there is a small fail on the QA tools:

 FAIL	t/lib/Koha/Plugin/Test.pm
   FAIL	  critic
		# Variables::ProhibitUnusedVariables: Got 1 violation(s).
Comment 4 Kyle M Hall 2020-09-04 15:29:46 UTC
Created attachment 109671 [details] [review]
Bug 26351: (QA follow-up) Fix QA script issue

* Fixes issue with barcode generate stub so perlcritic is happy
* Removes extra semicolon from return call in configure method
Comment 5 Julian Maurice 2020-09-11 09:08:32 UTC
I would like to make a suggestion about the hook name: IMO hooks should be named after an event (what just happened in Koha) rather than what the plugin is supposed to do. From what I understood this hook will be called every time a barcode is scanned. A plugin can choose to do other things than transforming the barcode (log it somewhere, transmit it to an external webservice, ...), so it would be more generic to call it "barcode_scanned" or something like that.

Also, I took a quick look at the code and I have a concern: what will happen if two (or more) plugins implement this hook ? It looks like only the return value from the first plugin will be used. We can imagine one want to implement item barcode transform in one plugin and patron barcode transform in another.
Comment 6 Kyle M Hall 2020-09-11 12:16:27 UTC
(In reply to Julian Maurice from comment #5)
> I would like to make a suggestion about the hook name: IMO hooks should be
> named after an event (what just happened in Koha) rather than what the
> plugin is supposed to do. From what I understood this hook will be called
> every time a barcode is scanned. A plugin can choose to do other things than
> transforming the barcode (log it somewhere, transmit it to an external
> webservice, ...), so it would be more generic to call it "barcode_scanned"
> or something like that.

I'm happy to change that!
 
> Also, I took a quick look at the code and I have a concern: what will happen
> if two (or more) plugins implement this hook ? It looks like only the return
> value from the first plugin will be used. We can imagine one want to
> implement item barcode transform in one plugin and patron barcode transform
> in another.

In it's current state, the last plugin 'wins'. A couple solutions would be to:
A) Modify call() to feed the result of the last plugin call into the next one
B) Split the hooks into separate methods

I'm much more inclined to go with B. What do you think Julian?
Comment 7 Julian Maurice 2020-09-11 12:38:17 UTC
(In reply to Kyle M Hall from comment #6)
> In it's current state, the last plugin 'wins'. A couple solutions would be
> to:
> A) Modify call() to feed the result of the last plugin call into the next one
> B) Split the hooks into separate methods
> 
> I'm much more inclined to go with B. What do you think Julian?

I think A would not work in all cases. There can be more than one parameter and it's not necessarily the same thing as the return value.
B would work in the example I mentioned, but would not work in other cases, like two plugins implementing the same method, one for transforming, the other for doing something else.

I've seen a pattern somewhere else that could work : plugins modify directly the parameters

  my $params = { barcode => $barcode };
  Koha::Plugins->call($hook_name, $params);
  $barcode = $params->{barcode};

It's not very intuitive, but it should work as expected in all cases, and there's no need to modify Koha::Plugins->call.