Bug 30409 - barcodedecode() should always trim barcode
Summary: barcodedecode() should always trim barcode
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Peter Vashchuk
QA Contact: Joonas Kylmälä
URL:
Keywords:
: 22204 (view as bug list)
Depends on:
Blocks: 29114
  Show dependency treegraph
 
Reported: 2022-03-30 13:03 UTC by Fridolin Somers
Modified: 2022-06-14 11:26 UTC (History)
3 users (show)

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


Attachments
Bug 30409: barcodedecode() should always trim barcode (2.94 KB, patch)
2022-03-31 11:00 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (5.17 KB, patch)
2022-03-31 14:37 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (3.03 KB, patch)
2022-04-02 21:03 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (4.06 KB, patch)
2022-04-02 21:32 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (4.07 KB, patch)
2022-04-02 21:40 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (4.48 KB, patch)
2022-05-23 10:20 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (8.75 KB, patch)
2022-05-30 12:53 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (8.79 KB, patch)
2022-05-30 23:16 UTC, David Nind
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (8.48 KB, patch)
2022-06-06 08:23 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (8.48 KB, patch)
2022-06-06 15:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 30409: barcodedecode() should always trim barcode (8.63 KB, patch)
2022-06-12 08:26 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30409: (QA follow-up) Fix grammar in syspref help text (1.41 KB, patch)
2022-06-12 08:26 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 30409: (QA follow-up) Avoid uninitialized variable warnings (1.54 KB, patch)
2022-06-14 11:26 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 Fridolin Somers 2022-03-30 13:03:34 UTC
Items barcode has a UNIQ constraint in database, but when there are trailling spaces its is not considered has a duplicate.
Since we trim spaces the barcode in input of checkout/checkin whe should also trim in barcodedecode() so in any barcode creation/edition.
Comment 1 Peter Vashchuk 2022-03-31 11:00:15 UTC Comment hidden (obsolete)
Comment 2 Peter Vashchuk 2022-03-31 14:37:45 UTC Comment hidden (obsolete)
Comment 3 Peter Vashchuk 2022-04-02 21:03:04 UTC Comment hidden (obsolete)
Comment 4 Peter Vashchuk 2022-04-02 21:32:52 UTC Comment hidden (obsolete)
Comment 5 Peter Vashchuk 2022-04-02 21:40:22 UTC
Created attachment 132885 [details] [review]
Bug 30409: barcodedecode() should always trim barcode

Barcode is trimmed of leading/trailing whitespaces in many instances
before the barcodedecode sub was called. This patch instead makes that
barcodedecode sub is going to trim it itself and removes unnecessary,
and repetitive code that was used before barcodedecode was called.
Comment 6 Owen Leonard 2022-04-04 11:40:20 UTC
Could you please add a test plan?
Comment 7 Fridolin Somers 2022-04-28 02:53:35 UTC
*** Bug 22204 has been marked as a duplicate of this bug. ***
Comment 8 Andrew Nugged 2022-05-21 12:11:31 UTC
Petro, please prioritize.
Comment 9 Peter Vashchuk 2022-05-23 10:20:15 UTC Comment hidden (obsolete)
Comment 10 Joonas Kylmälä 2022-05-28 09:00:54 UTC
1) Add a test case to t/Circulation_barcodedecode.t
2) In addition the cases you handled already, please remove the duplicate whitespace trimming also from

- course_reserves/add_items.pl
- circ/branchtransfers.pl (grep for # FIXME: barcodeInputFilter and add barcodedecode call)
- circ/returns.pl (there's two occurences)
- circ/renew.pl (there's two occurences)
Comment 11 Peter Vashchuk 2022-05-30 12:53:33 UTC
Created attachment 135448 [details] [review]
Bug 30409: barcodedecode() should always trim barcode

Barcode is trimmed of leading/trailing whitespaces in many instances
before the barcodedecode sub was called. This patch instead makes that
barcodedecode sub is going to trim it itself and removes unnecessary,
and repetitive code that was used before barcodedecode was called.

Steps to test:
1. Edit item with any barcode, add a bunch of whitespaces at the start
and at the bottom of it. Save the item. Ensure that this action ruins
the barcode and ensure that the spaces are still there by editing the
same item again.
2. Apply the patch.
3. Edit the same item again in the same fashion. Ensure that now all
whitespaces are getting trimmed and it doesn't affect the barcode in
any negative way.
Comment 12 David Nind 2022-05-30 23:16:08 UTC
Created attachment 135453 [details] [review]
Bug 30409: barcodedecode() should always trim barcode

Barcode is trimmed of leading/trailing whitespaces in many instances
before the barcodedecode sub was called. This patch instead makes that
barcodedecode sub is going to trim it itself and removes unnecessary,
and repetitive code that was used before barcodedecode was called.

Steps to test:
1. Edit item with any barcode, add a bunch of whitespaces at the start
and at the bottom of it. Save the item. Ensure that this action ruins
the barcode and ensure that the spaces are still there by editing the
same item again.
2. Apply the patch.
3. Edit the same item again in the same fashion. Ensure that now all
whitespaces are getting trimmed and it doesn't affect the barcode in
any negative way.

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2022-05-30 23:49:58 UTC
After signing off, I checked again and have changed to Failed QA - as trimming spaces on the check out, check in, renew, course reserves forms doesn't seem to work.

It certainly works when editing the item and saving it - the spaces before and after are removed.

However, after applying the patch, but not editing the item where spaces before and after the barcode were added in step 1, I tried:
  . Circulation > Check out: doesn't work - barcode not found
  . Circulation > Check in: doesn't work - barcode not found
  . Circulation > Renew: doesn't work - barcode not found
  . Course reserves > Add reserves for [course name]: doesn't work - barcode not found
  . Search the catalog: works (finds record with item barcode, as it did before the patch was applied)

I hope I have understood what the patch is supposed to do correctly!

Notes: The OPAC continue to work as expected (search with spaces before and after the barcode, and advanced search).
Comment 14 David Nind 2022-05-30 23:52:29 UTC
If this is just supposed to fix the item edit form, then please change the status, and I'll sign off again.
Comment 15 Peter Vashchuk 2022-06-03 13:30:28 UTC Comment hidden (obsolete)
Comment 16 David Nind 2022-06-05 22:06:13 UTC
Hi Peter. I was too slow retesting, as the patch no longer applies 8-(...

git bz apply 30409

Bug 30409 - barcodedecode() should always trim barcode

135453 - Bug 30409: barcodedecode() should always trim barcode

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 30409: barcodedecode() should always trim barcode
.git/rebase-apply/patch:18: trailing whitespace.
For proper functioning of this filter, calling the function on the 
.git/rebase-apply/patch:22: trailing whitespace.
The optional $filter argument is to allow for testing or explicit 
.git/rebase-apply/patch:23: trailing whitespace.
behavior that ignores the System Pref.  Valid values are the same as the 
warning: 3 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	course_reserves/add_items.pl
.git/rebase-apply/patch:18: trailing whitespace.
For proper functioning of this filter, calling the function on the 
.git/rebase-apply/patch:22: trailing whitespace.
The optional $filter argument is to allow for testing or explicit 
.git/rebase-apply/patch:23: trailing whitespace.
behavior that ignores the System Pref.  Valid values are the same as the 
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging course_reserves/add_items.pl
CONFLICT (content): Merge conflict in course_reserves/add_items.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 30409: barcodedecode() should always trim barcode
Comment 17 Peter Vashchuk 2022-06-06 08:23:11 UTC
Created attachment 135705 [details] [review]
Bug 30409: barcodedecode() should always trim barcode

Barcode is trimmed of leading/trailing whitespaces in many instances
before the barcodedecode sub was called. This patch instead makes that
barcodedecode sub is going to trim it itself and removes unnecessary,
and repetitive code that was used before barcodedecode was called.

Steps to test:
1. Edit item with any barcode, add a bunch of whitespaces at the start
and at the bottom of it. Save the item. Ensure that this action ruins
the barcode and ensure that the spaces are still there by editing the
same item again.
2. Apply the patch.
3. Edit the same item again in the same fashion. Ensure that now all
whitespaces are getting trimmed and it doesn't affect the barcode in
any negative way.

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 Peter Vashchuk 2022-06-06 08:23:35 UTC
Rebased on current master
Comment 19 David Nind 2022-06-06 15:05:46 UTC
Created attachment 135731 [details] [review]
Bug 30409: barcodedecode() should always trim barcode

Barcode is trimmed of leading/trailing whitespaces in many instances
before the barcodedecode sub was called. This patch instead makes that
barcodedecode sub is going to trim it itself and removes unnecessary,
and repetitive code that was used before barcodedecode was called.

Steps to test:
1. Edit item with any barcode, add a bunch of whitespaces at the start
and at the bottom of it. Save the item. Ensure that this action ruins
the barcode and ensure that the spaces are still there by editing the
same item again.
2. Apply the patch.
3. Edit the same item again in the same fashion. Ensure that now all
whitespaces are getting trimmed and it doesn't affect the barcode in
any negative way.

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2022-06-06 15:27:29 UTC
(In reply to Peter Vashchuk from comment #18)
> Rebased on current master

Thanks Peter. 

Have retested and changed to signed off. Things now seem to work as expected.

I think what I was getting confused with, was that if a barcode in the actual item had spaces before and after, then this was not found on input forms. This was the case both before and after the patch was applied:
1. Edit a barcode for an item so that it has spaces before and after.
2. Check input forms such as Check out - item not found.
3. After patch applied, item still not found, for example Check out input form.
4. Edit item, save - spaces before and after removed and now found on input forms even if extra spaces added on input forms.

Other testing notes:

1. Card barcodes: worked before and after patch for Check out, Search patrons, and Patron lists.

2. Item barcodes: worked before and after patch applied:
   . From header block: Check out, Check in, Renew
   . Add reserves
   . Search (keyword)
   . From Circulation screen: Check out, Check in, Renew, Transfers, Overdue report
   . Batch checkout
   . Tools: Batch item deletion, Batch item modification

3. Item barcodes: didn't work before the patch, but now works after patch applied:
   . Tools: Barcode image generator, Rotating collections, Inventory

4. Item barcodes: didn't work before the patch, and don't work after patch - possible future enhancements:
  . Lists
  . Quick spine label creator
  . Item search - barcode is
Comment 21 Joonas Kylmälä 2022-06-12 08:26:23 UTC
Created attachment 135961 [details] [review]
Bug 30409: barcodedecode() should always trim barcode

Barcode is trimmed of leading/trailing whitespaces in many instances
before the barcodedecode sub was called. This patch instead makes that
barcodedecode sub is going to trim it itself and removes unnecessary,
and repetitive code that was used before barcodedecode was called.

Steps to test:
1. Edit item with any barcode, add a bunch of whitespaces at the start
and at the bottom of it. Save the item. Ensure that this action ruins
the barcode and ensure that the spaces are still there by editing the
same item again.
2. Apply the patch.
3. Edit the same item again in the same fashion. Ensure that now all
whitespaces are getting trimmed and it doesn't affect the barcode in
any negative way.

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

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 22 Joonas Kylmälä 2022-06-12 08:26:27 UTC
Created attachment 135962 [details] [review]
Bug 30409: (QA follow-up) Fix grammar in syspref help text

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 23 Joonas Kylmälä 2022-06-12 08:27:21 UTC
Passing QA. I noticed a small grammar error in the syspref text so added a follow-up for that.
Comment 24 Tomás Cohen Arazi 2022-06-14 10:56:26 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 25 Tomás Cohen Arazi 2022-06-14 11:26:39 UTC
Created attachment 136067 [details] [review]
Bug 30409: (QA follow-up) Avoid uninitialized variable warnings

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>