|
Description
Kyle M Hall (khall)
2019-06-19 18:17:53 UTC
Created attachment 90815 [details] [review] Bug 23165: Renewals for Self Checkout (SCO) module do not record branchcode in statistics Koha considers SCO to be part of the OPAC, but the code related to OpacRenewalBranch is in opac-renew.pl. Because of this, the branchcode does not get set when a renewal takes place via the self check module. It would make more sense to move this logic into C4::Circulation::AddRenewal to centralize it and ensure that the statistics branch is set no matter where in the OPAC AddRenewal may be called. Test Plan: 1) Using the SCO module, renew an item 2) View the statistics table row for the renewal, note that branch is empty 3) Apply this patch 4) Restart all the things! 5) Renew the item again 6) View the stats table row for the new renewal, the branch code should be set as per the system preference OpacRenewalBranch Could it be nice to allow for some indicator that the renewal has happened via SCO? The patrons have to come into the library to do it unlike when using the OPAC, so it might be interesting for stats. That sounds like a good idea but is definitely beyond the scope of this bug. That would be more of an enhancement where all SCO and SCI actions would get namespace separate from 'opac'. I'm going to file a separate enhancement bug for that, it's a great idea! (In reply to Katrin Fischer from comment #2) > Could it be nice to allow for some indicator that the renewal has happened > via SCO? The patrons have to come into the library to do it unlike when > using the OPAC, so it might be interesting for stats. Created attachment 90997 [details] [review] Bug 23165: Renewals for Self Checkout (SCO) module do not record branchcode in statistics Koha considers SCO to be part of the OPAC, but the code related to OpacRenewalBranch is in opac-renew.pl. Because of this, the branchcode does not get set when a renewal takes place via the self check module. It would make more sense to move this logic into C4::Circulation::AddRenewal to centralize it and ensure that the statistics branch is set no matter where in the OPAC AddRenewal may be called. Test Plan: 1) Using the SCO module, renew an item 2) View the statistics table row for the renewal, note that branch is empty 3) Apply this patch 4) Restart all the things! 5) Renew the item again 6) View the stats table row for the new renewal, the branch code should be set as per the system preference OpacRenewalBranch Created attachment 91016 [details] [review] Bug 23165: Unit tests Kyle, the patches look correct and actually solve the specific use case you are tackling. BUT... $ git grep AddRenewal shows there are some places in which the branchcode is still calculated outside and passed: Koha/REST/V1/Checkouts.pm circ/renew.pl misc/cronjobs/automatic_renewals.pl svc/renew also some tests, but I guess they are mocking userenv to match what they pass as parameter, and that's why they are still green. In some cases it might make sense to use userenv (maybe the staff controllers), but I'm not sure (haven't checked) if there is an override available on the UI to allow specifying the branchcode. (In reply to Tomás Cohen Arazi from comment #6) > Kyle, the patches look correct and actually solve the specific use case you > are tackling. BUT... > > $ git grep AddRenewal > > shows there are some places in which the branchcode is still calculated > outside and passed: > > Koha/REST/V1/Checkouts.pm > circ/renew.pl > misc/cronjobs/automatic_renewals.pl > svc/renew > > also some tests, but I guess they are mocking userenv to match what they > pass as parameter, and that's why they are still green. > > In some cases it might make sense to use userenv (maybe the staff > controllers), but I'm not sure (haven't checked) if there is an override > available on the UI to allow specifying the branchcode. This code is only responsible for calculating the branchcode if the renewal is done from the opac. I don't believe any of those calls you listed are opac based, right? Are you prososing we should consolidate all 'branchcode' logic within the sub and remove the branchcode parameter altogether? I'm wondering if there's any race in the different logics for calculating the branchcode. I haven't checked it, but will do at the office! (In reply to Tomás Cohen Arazi from comment #8) > I'm wondering if there's any race in the different logics for calculating > the branchcode. I haven't checked it, but will do at the office! I can't imagine how this code could have race conditions, but I'm interested in what the results of your research are! Created attachment 91227 [details] [review] Bug 23165: (QA follow-up) Don't set if it's already set for non-opac calls Comment on attachment 91227 [details] [review] Bug 23165: (QA follow-up) Don't set if it's already set for non-opac calls Bug 17781 directly contravenes this bug. The original patch behaviors is correct. Thank goodness for unit tests! > Bug 17781 directly contravenes this bug. The original patch behaviors is
> correct. Thank goodness for unit tests!
s/bug/patch
I hope I can do a signoff on this as a demo at the hackfest in Lund tomorrow, super awesome if noone beats me to it. :-) (In reply to Magnus Enger from comment #13) > I hope I can do a signoff on this as a demo at the hackfest in Lund > tomorrow, super awesome if noone beats me to it. :-) Reset the status. I will QA after your SO. Created attachment 94364 [details] [review] Bug 23165: Unit tests Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Created attachment 94365 [details] [review] Bug 23165: Renewals for Self Checkout (SCO) module do not record branchcode in statistics Koha considers SCO to be part of the OPAC, but the code related to OpacRenewalBranch is in opac-renew.pl. Because of this, the branchcode does not get set when a renewal takes place via the self check module. It would make more sense to move this logic into C4::Circulation::AddRenewal to centralize it and ensure that the statistics branch is set no matter where in the OPAC AddRenewal may be called. Test Plan: 1) Using the SCO module, renew an item 2) View the statistics table row for the renewal, note that branch is empty 3) Apply this patch 4) Restart all the things! 5) Renew the item again 6) View the stats table row for the new renewal, the branch code should be set as per the system preference OpacRenewalBranch Signed-off-by: Magnus Enger <magnus@libriotech.no> CONFLICT (content): Merge conflict in C4/Circulation.pm More general code cleanup on other bug, moved the unit tests from here to there *** This bug has been marked as a duplicate of bug 24759 *** |