Bug 31252 - Advanced search in staff interface should call barcodedecode if the search index is a barcode
Summary: Advanced search in staff interface should call barcodedecode if the search in...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 26351
Blocks:
  Show dependency treegraph
 
Reported: 2022-07-28 12:32 UTC by Kyle M Hall
Modified: 2023-12-28 20:42 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31252: Advanced search in staff interface should call barcodedecode if the search index is a barcode (1.93 KB, patch)
2022-07-28 12:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 31252: Advanced search in staff interface should call barcodedecode if the search index is a barcode (1.97 KB, patch)
2022-09-13 19:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 31252: Advanced search in staff interface should call barcodedecode if the search index is a barcode (2.03 KB, patch)
2022-10-07 13:16 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 Kyle M Hall 2022-07-28 12:32:47 UTC
We should run any inputted barcode through barcodedecode before passing it to any subroutines. This was missed during the initial development of bug 26351.
Comment 1 Kyle M Hall 2022-07-28 12:41:19 UTC
Created attachment 138202 [details] [review]
Bug 31252: Advanced search in staff interface should call barcodedecode if the search index is a barcode

We should run any inputted barcode through barcodedecode before passing it to any subroutines.
This was missed during the initial development of bug 26351.

Test Plan:
1) Set itemBarcodeInputFilter to "Remove spaces from"
2) Browse to the staff side advanced search
3) Run a barcode search for "MY TEST"
4) Note the message returned is:
   No results match your search for 'bc,wrdl: "MY TEST"'.
5) Apply this patch
6) Restart all the things!
7) Repeat your search
8) Note the message returned is:
   No results match your search for 'bc,wrdl: "MYTEST"'.
9) Note the lack of the space between MY and TEST!
Comment 2 Katrin Fischer 2022-07-30 15:25:08 UTC
I like this, but I was a little worried it could create problems with the other barcode filters.

So what happens if the barcode is not scanned but for example copied from a report or from the items table (already being converted?)

Are we sure the existing filters will not "double-convert" those?

So I have tested (looking at the test for the barcode filters):

libsuite8
Unconverted: b000126 searches for:
Converted: IMS-b-126 searches for: CPL-IMS-b--126
--> unwanted conversion
This is odd, because the test covers this and they should actually be the same?
The additional space should not upset Koha, so we can probably ignore this.

Also tested CueCat, T, EAN etc. and it looks good.
Comment 3 Katrin Fischer 2022-07-30 15:31:06 UTC
Looking at this again... the libsuite8 conversion is an odd one:

IMS-b-126 searches for: CPL-IMS-b--126 => Where is the branchcode suddenly coming from?

This should not happen, IMS-B-125 should stay identical on conversion?

t/Circulation_barcodedecode.
'libsuite8' => ['b000126', 'b12', 'B0126', 'IMS-B-126', 'ims-b-126','CD0000024','00123','11998'],
'libsuite8' => ['IMS-b-126', 'IMS-b-12', 'IMS-B-126', 'IMS-B-126', 'ims-b-126','IMS-CD-24','IMS-b-123','IMS-b-11998'],

Can you have a look please?
Comment 4 Katrin Fischer 2022-08-02 11:30:36 UTC
I did some more research and it looks like we already use the filter like that in a lot of places. If there is an issue with libsuite8 we should probably move it to a separate bug.
Comment 5 Katrin Fischer 2022-08-02 12:17:56 UTC
I just ran into a case where the library has barcodes with trailing spaces from a batch of faulty barcode labels. Could it be an option to add the ItemBarcodeInputFilter into the item edit form too? (just wondering if you have an opinion about this, totally a separate bug)
Comment 6 David Nind 2022-09-13 19:20:10 UTC
Created attachment 140604 [details] [review]
Bug 31252: Advanced search in staff interface should call barcodedecode if the search index is a barcode

We should run any inputted barcode through barcodedecode before passing it to any subroutines.
This was missed during the initial development of bug 26351.

Test Plan:
1) Set itemBarcodeInputFilter to "Remove spaces from"
2) Browse to the staff side advanced search
3) Run a barcode search for "MY TEST"
4) Note the message returned is:
   No results match your search for 'bc,wrdl: "MY TEST"'.
5) Apply this patch
6) Restart all the things!
7) Repeat your search
8) Note the message returned is:
   No results match your search for 'bc,wrdl: "MYTEST"'.
9) Note the lack of the space between MY and TEST!

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2022-09-13 19:21:42 UTC
Patch still applies and have signed off, as everything works as per the test plan.

Have changed status to 'In Discussion' as I think Katrin's comments need addressing.
Comment 8 Kyle M Hall 2022-09-14 10:31:29 UTC
Katrin, I think both the issue with the libsuite8 filter, and adding an option to add the ItemBarcodeInputFilter into the item edit form deserve their own bug reports!

(In reply to Katrin Fischer from comment #3)
> Looking at this again... the libsuite8 conversion is an odd one:
> 
> IMS-b-126 searches for: CPL-IMS-b--126 => Where is the branchcode suddenly
> coming from?
> 
> This should not happen, IMS-B-125 should stay identical on conversion?
> 
> t/Circulation_barcodedecode.
> 'libsuite8' => ['b000126', 'b12', 'B0126', 'IMS-B-126',
> 'ims-b-126','CD0000024','00123','11998'],
> 'libsuite8' => ['IMS-b-126', 'IMS-b-12', 'IMS-B-126', 'IMS-B-126',
> 'ims-b-126','IMS-CD-24','IMS-b-123','IMS-b-11998'],
> 
> Can you have a look please?

That's out of scope
Comment 9 Katrin Fischer 2022-09-14 11:11:22 UTC
Sorry, maybe too many comments from me here :) You'll have to read them all:


(In reply to Katrin Fischer from comment #5)
> I just ran into a case where the library has barcodes with trailing spaces
> from a batch of faulty barcode labels. Could it be an option to add the
> ItemBarcodeInputFilter into the item edit form too? (just wondering if you
> have an opinion about this, totally a separate bug)

Yep, totally a separate bug ;)

(In reply to Katrin Fischer from comment #4)
> I did some more research and it looks like we already use the filter like
> that in a lot of places. If there is an issue with libsuite8 we should
> probably move it to a separate bug.

Again agreed ;)

I am ok to move on with this patch set.
Comment 10 David Nind 2022-09-14 12:19:13 UTC
Thanks Kyle and Katrin!
Comment 11 Katrin Fischer 2022-10-07 13:16:52 UTC
Created attachment 141498 [details] [review]
Bug 31252: Advanced search in staff interface should call barcodedecode if the search index is a barcode

We should run any inputted barcode through barcodedecode before passing it to any subroutines.
This was missed during the initial development of bug 26351.

Test Plan:
1) Set itemBarcodeInputFilter to "Remove spaces from"
2) Browse to the staff side advanced search
3) Run a barcode search for "MY TEST"
4) Note the message returned is:
   No results match your search for 'bc,wrdl: "MY TEST"'.
5) Apply this patch
6) Restart all the things!
7) Repeat your search
8) Note the message returned is:
   No results match your search for 'bc,wrdl: "MYTEST"'.
9) Note the lack of the space between MY and TEST!

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 12 Tomás Cohen Arazi 2022-10-17 20:48:29 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 13 Lucas Gass 2022-12-05 20:32:49 UTC
Missing 22.05.x dependencies, no backport