[Bug 23165] New: Renewals for Self Checkout (SCO) module do not record branchcode in statistics
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Bug ID: 23165 Summary: Renewals for Self Checkout (SCO) module do not record branchcode in statistics Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Self checkout Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90815 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90815&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Chris Slone <cslone@camdencountylibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |cslone@camdencountylibrary. | |org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- 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.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23203 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90815|0 |1 is obsolete| | --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 90997 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=90997&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90997|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 91016 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91016&action=edit Bug 23165: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90997|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Version|unspecified |master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 91227 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91227&action=edit Bug 23165: (QA follow-up) Don't set if it's already set for non-opac calls -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91227|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Comment on attachment 91227 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91227 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> ---
Bug 17781 directly contravenes this bug. The original patch behaviors is correct. Thank goodness for unit tests!
s/bug/patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@libriotech.no --- Comment #13 from Magnus Enger <magnus@libriotech.no> --- 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. :-) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #91016|0 |1 is obsolete| | --- Comment #15 from Magnus Enger <magnus@libriotech.no> --- Created attachment 94364 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=94364&action=edit Bug 23165: Unit tests Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #90997|0 |1 is obsolete| | --- Comment #16 from Magnus Enger <magnus@libriotech.no> --- Created attachment 94365 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=94365&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Status|Signed Off |Patch doesn't apply --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- CONFLICT (content): Merge conflict in C4/Circulation.pm -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23165 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |RESOLVED CC| |nick@bywatersolutions.com Resolution|--- |DUPLICATE --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- 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 *** -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org