Description
hebah
2022-06-08 21:02:53 UTC
It would be incredibly useful to have the interface added to the statistics table. Our use case is this - We need to calculate self-checkout percentage, by library location. At the system-wide level (all branches), we can do this using the action_logs table. However, if we need granularity at the library branch level, we currently cannot provide accurate data for reporting/comparisons. For our use case, we would use the interface to compare Intranet (staff) vs SIP (self-checkouts), at the branch level. There are other use cases where the interface (context) at the branch level might be useful too. This would be great! Once it's added to statistics, it would be helpful to have it added to the pseudonymization options too. Created attachment 145982 [details] [review] Bug 30928: Add interface column: DB update, schema changes, and UpdateStats Created attachment 146042 [details] [review] Bug 30928: Update Accounts.pm and Recalls.pm Created attachment 146171 [details] [review] Bug 30928: Add interface column: DB update, schema changes, and UpdateStats To test: Apply patch, updatedatabase, and restart_all Do each of the following actions and check the statistics table making sure the interface is properly being recorded: RENEWALS: -Renewal from staff client -Renew from OPAC -Renew from API. You could add something like this to IntranetUSerJS: $.ajax({ url: '/api/v1/checkouts/338/renewal', method: 'POST', contentType: 'application/json', success: function(result) { console.log('renewed') }, }); -Renewal from automatic_renewals -Renewal from SIP ( can use sip_cli_emulator.pl ) CHECKOUTS: -Checkout from staff client -Checkout from SCO -Checkout from API -Checkout from SIP ( can use sip_cli_emulator.pl ) CHECKIN: -Checkin from staff client -Check in from batch -Check in from API -Check in from SCI -Check in from SIP ( can use sip_cli_emulator.pl ) RECALLS: -OPAC recall -Staff client recall ACCOUNTING: -Make a payment -Make a payemnt from SIP ( can use sip_cli_emulator.pl ) prove -v /kohadevbox/koha/t/db_dependent/Circulation.t Tests should all pass Created attachment 146172 [details] [review] Bug 30928: Add tests prove -v t/db_dependent/Koha/Statistics.t prove -v t/db_dependent/Stats.t Testing Notes (KTD): 1 error: ACCOUNTING - When a payment was made from the staff interface, the Statistics table incorrectly logged that the transaction was made through the OPAC 2 unable to test: - I'm not sure how to test CHECKIN and CHECKOUT via REST API. Any chance you can include JS snippets for that in your test plan, like you did for a renewal? - Everything else, including prove commands, worked as expected :)
> 1 error:
> ACCOUNTING
> - When a payment was made from the staff interface, the Statistics table
> incorrectly logged that the transaction was made through the OPAC
Hi, Emily. I am not recreating this. Can you provide some specific steps you use to log the transaction as OPAC via the staff client?
Suddenly I wasn't able to reproduce it either...it turned out to be a very specific interaction between the intranet, OPAC, and SCO logins. It also looks like a separate bug that has nothing to do with your patch. (It didn't occur to me yesterday to compare with the system logs - but having checked them, the logs show the same error, both with and without the patch. Sorry for missing that yesterday!) Moved it to bug 32965. Created attachment 146653 [details] Bug 30642: (follow-up) Remove use of deleted file Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 146654 [details] [review] Bug 30928: Add interface column: DB update, schema changes, and UpdateStats To test: Apply patch, updatedatabase, and restart_all Do each of the following actions and check the statistics table making sure the interface is properly being recorded: RENEWALS: -Renewal from staff client -Renew from OPAC -Renew from API. You could add something like this to IntranetUSerJS: $.ajax({ url: '/api/v1/checkouts/338/renewal', method: 'POST', contentType: 'application/json', success: function(result) { console.log('renewed') }, }); -Renewal from automatic_renewals -Renewal from SIP ( can use sip_cli_emulator.pl ) CHECKOUTS: -Checkout from staff client -Checkout from SCO -Checkout from API -Checkout from SIP ( can use sip_cli_emulator.pl ) CHECKIN: -Checkin from staff client -Check in from batch -Check in from API -Check in from SCI -Check in from SIP ( can use sip_cli_emulator.pl ) RECALLS: -OPAC recall -Staff client recall ACCOUNTING: -Make a payment -Make a payemnt from SIP ( can use sip_cli_emulator.pl ) prove -v /kohadevbox/koha/t/db_dependent/Circulation.t Tests should all pass Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov> Testing Notes: Couldn't find any routes for checkin/checkout in the API documentation. All other use cases behave as expected, and tests pass. Patch no longer applies - merge conflict in C4/Circulation.pm Please rebase Created attachment 147838 [details] [review] Bug 30928: Add tests prove -v t/db_dependent/Koha/Statistics.t prove -v t/db_dependent/Stats.t Created attachment 147839 [details] [review] Bug 30928: Add interface column: DB update, schema changes, and UpdateStats To test: Apply patch, updatedatabase, and restart_all Do each of the following actions and check the statistics table making sure the interface is properly being recorded: RENEWALS: -Renewal from staff client -Renew from OPAC -Renew from API. You could add something like this to IntranetUSerJS: $.ajax({ url: '/api/v1/checkouts/338/renewal', method: 'POST', contentType: 'application/json', success: function(result) { console.log('renewed') }, }); -Renewal from automatic_renewals -Renewal from SIP ( can use sip_cli_emulator.pl ) CHECKOUTS: -Checkout from staff client -Checkout from SCO -Checkout from API -Checkout from SIP ( can use sip_cli_emulator.pl ) CHECKIN: -Checkin from staff client -Check in from batch -Check in from API -Check in from SCI -Check in from SIP ( can use sip_cli_emulator.pl ) RECALLS: -OPAC recall -Staff client recall ACCOUNTING: -Make a payment -Make a payemnt from SIP ( can use sip_cli_emulator.pl ) prove -v /kohadevbox/koha/t/db_dependent/Circulation.t Tests should all pass Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov> Rebased for current master Please move schema update to a separate patch. Once fixed I'll be happy to pass QA. Created attachment 150386 [details] [review] Bug 30928: Add interface to statistics Created attachment 150387 [details] [review] Bug 30928: DB and Schema updates (In reply to Aleisha Amohia from comment #17) > Please move schema update to a separate patch. Once fixed I'll be happy to > pass QA. Thanks Aleisha! I have separated them. Created attachment 150433 [details] [review] Bug 30928: Add tests prove -v t/db_dependent/Koha/Statistics.t prove -v t/db_dependent/Stats.t Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 150434 [details] [review] Bug 30928: Add interface to statistics Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 150435 [details] [review] Bug 30928: (QA follow-up) DB updates Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 150436 [details] [review] Bug 30928: (QA follow-up) Schema updates Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Gentle message from the RM: the QA script tells the atomicupdate doesn't have the right permissions. I'm pushing this already, but keep in mind for future submissions. Pushed to master for 23.05. Nice work everyone, thanks! I am rebasing now bug 33608 on top of this one. We have a failure after this patch set on Koha/Pseudonymization.t Will add a follow-up here Hmm. It may come from another pushed patch set.. (In reply to Marcel de Rooy from comment #28) > Hmm. It may come from another pushed patch set.. It must be the authority relinking stuff that was pushed too. That makes sample_biblio crash in Pseudonymization.t. So not caused here definitely. (In reply to Marcel de Rooy from comment #29) > (In reply to Marcel de Rooy from comment #28) > > Hmm. It may come from another pushed patch set.. > > It must be the authority relinking stuff that was pushed too. That makes > sample_biblio crash in Pseudonymization.t. > So not caused here definitely. Opened bug 33718 for these issues. Disabling AutoLinkBiblios helps to get it passing again. Enhancement - not backporting to 22.11.x It looks like only backend changes. Nothing to add/change in the manual. Closing, but feel free to include more details if this needs to be documented in the manual. Will add a follow-up for this development on 33608. It would be good to default to C4::Context->interface instead of NULL, or always needing to pass it. (In reply to Marcel de Rooy from comment #33) > Will add a follow-up for this development on 33608. It would be good to > default to C4::Context->interface instead of NULL, or always needing to pass > it. But much harder to spot missed/wrong values this way :) (In reply to Katrin Fischer from comment #34) > (In reply to Marcel de Rooy from comment #33) > > Will add a follow-up for this development on 33608. It would be good to > > default to C4::Context->interface instead of NULL, or always needing to pass > > it. > > But much harder to spot missed/wrong values this way :) Please explain what you mean. NULL would be a missing value, I suppose, that we fix there. If C4::Context->interface would provide a wrong value, we should address that elsewhere. But that should not be possible, since it already contains a list of valid options. |