Bug 16117

Summary: Log invalid patron cardnumbers and item barcodes
Product: Koha Reporter: Barton Chittenden <barton>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: RESOLVED WONTFIX QA Contact: Victor Grousset/tuxayo <victor>
Severity: enhancement    
Priority: P5 - low CC: amy, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kebliss, kyle.m.hall, kyle, m.de.rooy, nicole, veron, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 16117 - Feature request: Log borrower cardnumbers and item barcodes which are not valid
Bug 16117 - Feature request: Log borrower cardnumbers and item barcodes which are not valid
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
Bug 16117: Give methods better names
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
Bug 16117: Give methods better names
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
Bug 16117: (QA follow-up) Fix QA issues
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
Bug 16117: (QA follow-up) Fix QA issues
Bug 16117: (QA follow-up): tidy up code & fix test name

Description Barton Chittenden 2016-03-21 20:59:55 UTC
On any circ screen where a barcode could be scanned (circ.pl, checkin.pl, checkout and return boxes), we should have the option of logging failed barcode scans, for the purpose of trouble-shooting.
Comment 1 Marc Véron 2016-03-30 12:36:59 UTC
As well valid for current master?
Comment 2 Kyle M Hall 2017-07-28 17:00:47 UTC
Created attachment 65317 [details] [review]
Bug 16117 - Feature request: Log borrower cardnumbers and item barcodes which are not valid

We should be able to log invalid patrons and items for trouble-shooting purposes. It
is especially helpful when determining if there are issues with barcode scanners.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Attempt to check out to an invalid patron cardnumber
4) Attempt to checkout a non-existent barcode
5) Attempt to check in a non-existant barcode
6) Note nothing new is added to the statistics table
7) Enable the new system preferences LogInvalidItems and LogInvalidPatrons
8) Repeat steps 4-6
9) Note each action creates a new line in the statistics table
Comment 3 Marc Véron 2017-07-28 19:46:23 UTC
Created attachment 65319 [details] [review]
Bug 16117 - Feature request: Log borrower cardnumbers and item barcodes which are not valid

We should be able to log invalid patrons and items for trouble-shooting purposes. It
is especially helpful when determining if there are issues with barcode scanners.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Attempt to check out to an invalid patron cardnumber
4) Attempt to checkout a non-existent barcode
5) Attempt to check in a non-existant barcode
6) Note nothing new is added to the statistics table
7) Enable the new system preferences LogInvalidItems and LogInvalidPatrons
8) Repeat steps 4-6
9) Note each action creates a new line in the statistics table

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 4 Jonathan Druart 2017-08-09 17:53:14 UTC
Just a thought:
  Koha::Statistics->invalid_item( { item => $barcode } );
sounds like it will return something, not it will actually write.

I would suggest to rename the method with something more meaningful.
Waiting for another QA opinion.
Comment 5 Josef Moravec 2017-12-03 10:39:42 UTC
(In reply to Jonathan Druart from comment #4)
> Just a thought:
>   Koha::Statistics->invalid_item( { item => $barcode } );
> sounds like it will return something, not it will actually write.
> 
> I would suggest to rename the method with something more meaningful.
> Waiting for another QA opinion.

I agree, something like "log_invalid_item" or similar would be better.
Comment 6 Marcel de Rooy 2018-03-09 07:22:28 UTC
See last two comments
Comment 7 Katrin Fischer 2018-03-12 13:29:24 UTC
Also: Patch doesn't apply!
Comment 8 Kyle M Hall 2018-03-23 13:39:10 UTC
Created attachment 73177 [details] [review]
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid

We should be able to log invalid patrons and items for trouble-shooting purposes. It
is especially helpful when determining if there are issues with barcode scanners.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Attempt to check out to an invalid patron cardnumber
4) Attempt to checkout a non-existent barcode
5) Attempt to check in a non-existant barcode
6) Note nothing new is added to the statistics table
7) Enable the new system preferences LogInvalidItems and LogInvalidPatrons
8) Repeat steps 4-6
9) Note each action creates a new line in the statistics table

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 9 Kyle M Hall 2018-03-23 13:39:18 UTC
Created attachment 73178 [details] [review]
Bug 16117: Give methods better names
Comment 10 Katrin Fischer 2018-04-22 10:36:07 UTC
Patch doesn't apply, please rebase!

73178 - Bug 16117: Give methods better names

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
M	circ/circulation.pl
M	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
M	t/db_dependent/Stats.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Stats.t
CONFLICT (content): Merge conflict in t/db_dependent/Stats.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging circ/circulation.pl
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-16117-Log-borrower-cardnumbers-and-item-barcod-t7ci5h.patch
Comment 11 Kyle M Hall 2018-10-26 10:34:08 UTC
Created attachment 81273 [details] [review]
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid

We should be able to log invalid patrons and items for trouble-shooting purposes. It
is especially helpful when determining if there are issues with barcode scanners.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Attempt to check out to an invalid patron cardnumber
4) Attempt to checkout a non-existent barcode
5) Attempt to check in a non-existant barcode
6) Note nothing new is added to the statistics table
7) Enable the new system preferences LogInvalidItems and LogInvalidPatrons
8) Repeat steps 4-6
9) Note each action creates a new line in the statistics table

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Kyle M Hall 2018-10-26 10:34:19 UTC
Created attachment 81274 [details] [review]
Bug 16117: Give methods better names
Comment 13 Katrin Fischer 2018-10-28 00:46:22 UTC
1) QA script fails on POD:
 FAIL	Koha/Statistics.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD is missing for 'object_class'

2) The prefs appear under "Circulation", I would have expected them under "Logs". Can you please move or explain?

3) This covers Checkout and Checkin, but what about scanned renewals?

4) remove "Feature request" from atomicupdate print :)
Comment 14 Kyle M Hall 2023-07-14 15:24:39 UTC
Created attachment 153493 [details] [review]
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid

We should be able to log invalid patrons and items for trouble-shooting purposes. It
is especially helpful when determining if there are issues with barcode scanners.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Attempt to check out to an invalid patron cardnumber
4) Attempt to checkout a non-existent barcode
5) Attempt to check in a non-existant barcode
6) Note nothing new is added to the statistics table
7) Enable the new system preferences LogInvalidItems and LogInvalidPatrons
8) Repeat steps 4-6
9) Note each action creates a new line in the statistics table

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 15 Katrin Fischer 2023-09-09 18:15:10 UTC
Having a quick look here, some small changes need to be made:

1) System preferences in the yaml file are using yes/no instead of 0/1.

2) Not all points from comment#13 have been replied to or resolved, especially 2 and 3.
Comment 16 Kyle M Hall 2023-09-11 14:57:48 UTC
Created attachment 155498 [details] [review]
Bug 16117: (QA follow-up) Fix QA issues
Comment 17 Kyle M Hall 2023-09-11 14:58:45 UTC
(In reply to Katrin Fischer from comment #15)
> Having a quick look here, some small changes need to be made:
> 
> 1) System preferences in the yaml file are using yes/no instead of 0/1.

Fixed!

> 2) Not all points from comment#13 have been replied to or resolved,
> especially 2 and 3.

Prefs moved, and scanned renewals will be captured as they will go though CanBookBeIssued
Comment 18 Victor Grousset/tuxayo 2023-10-22 19:42:47 UTC
I get invalid_item logged when try a check out and check in but for renewals I get nothing either from /cgi-bin/koha/circ/renew.pl or from the header_search bar switched to "renew".

Did I miss something?
Comment 19 Katrin Fischer 2023-11-05 00:49:37 UTC
Sorry, but this needs some work to apply again:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
M	circ/circulation.pl
M	installer/data/mysql/mandatory/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
M	opac/sco/sco-main.pl
A	t/db_dependent/Stats.t
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): t/db_dependent/Stats.t deleted in HEAD and modified in Bug 16117: Log borrower cardnumbers and item barcodes which are not valid. Version Bug 16117: Log borrower cardnumbers and item barcodes which are not valid of t/db_dependent/Stats.t left in tree.
Auto-merging opac/sco/sco-main.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging circ/circulation.pl
Auto-merging C4/Circulation.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 16117: Log borrower cardnumbers and item barcodes which are not valid
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-16117-Log-borrower-cardnumbers-and-item-barcod-0_3t9irp.patch
Comment 20 Kyle M Hall 2023-11-06 14:20:31 UTC
Created attachment 158531 [details] [review]
Bug 16117: Log borrower cardnumbers and item barcodes which are not valid

We should be able to log invalid patrons and items for trouble-shooting purposes. It
is especially helpful when determining if there are issues with barcode scanners.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Attempt to check out to an invalid patron cardnumber
4) Attempt to checkout a non-existent barcode
5) Attempt to check in a non-existant barcode
6) Note nothing new is added to the statistics table
7) Enable the new system preferences LogInvalidItems and LogInvalidPatrons
8) Repeat steps 4-6
9) Note each action creates a new line in the statistics table

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 21 Kyle M Hall 2023-11-06 14:20:38 UTC
Created attachment 158532 [details] [review]
Bug 16117: (QA follow-up) Fix QA issues
Comment 22 Victor Grousset/tuxayo 2024-02-09 18:01:36 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> I get invalid_item logged when try a check out and check in but for renewals
> I get nothing either from /cgi-bin/koha/circ/renew.pl or from the
> header_search bar switched to "renew".
> 
> Did I miss something?

This is still not working.


----

> Log scanned invalid patron identifiers as statistics

The logging feature of Koha isn't referred as statistics. Is it called like that because these logs are not viewable in the log viewer? And only in the statistics table. Any opinions to confirm that this feature does not need the results viewable via the UI? I would have guessed yes but better get opinions from people knowing better the user needs for this.

---

note: there is no way to differentiate an invalid cardnumber from just searching a patron by name in the checkout header_search bar. So enabling logging here will lead to all of those searches by name being logged as false positives. I guess that's just a tradeoff the feature inherent of having field combining name and cardnumber search. not much to do here.


---

note: record details => holds => search patron: it's not logged but it looks ok to leave that for a follow up ticket if there is actual demand. (previous point will also apply here)


---

note: self check-in and self check-out works 👍
Comment 23 Victor Grousset/tuxayo 2024-02-09 18:01:51 UTC
Created attachment 161990 [details] [review]
Bug 16117: (QA follow-up): tidy up code & fix test name
Comment 24 Kyle M Hall 2024-02-12 11:52:08 UTC
I don't think this feature is worth pursuing any longer at this time. IIRC it was basically to show that a library's barcode scanning was occasionally misreading barcodes and not Koha failing to find the correct barcode. At this point we could implement something along these lines as a plugin if it were ever needed again.