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.
As well valid for current master?
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
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>
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.
(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.
See last two comments
Also: Patch doesn't apply!
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>
Created attachment 73178 [details] [review] Bug 16117: Give methods better names
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
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>
Created attachment 81274 [details] [review] Bug 16117: Give methods better names
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 :)
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>
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.
Created attachment 155498 [details] [review] Bug 16117: (QA follow-up) Fix QA issues
(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
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?
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
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>
Created attachment 158532 [details] [review] Bug 16117: (QA follow-up) Fix QA issues
(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 [U+1F44D]
Created attachment 161990 [details] [review] Bug 16117: (QA follow-up): tidy up code & fix test name
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.