Bug 23165 - Renewals for Self Checkout (SCO) module do not record branchcode in statistics
Summary: Renewals for Self Checkout (SCO) module do not record branchcode in statistics
Status: RESOLVED DUPLICATE of bug 24759
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-19 18:17 UTC by Kyle M Hall
Modified: 2020-02-28 16:30 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23165: Renewals for Self Checkout (SCO) module do not record branchcode in statistics (3.89 KB, patch)
2019-06-19 18:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23165: Renewals for Self Checkout (SCO) module do not record branchcode in statistics (3.96 KB, patch)
2019-06-25 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23165: Unit tests (6.37 KB, patch)
2019-06-26 11:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23165: (QA follow-up) Don't set if it's already set for non-opac calls (835 bytes, patch)
2019-07-03 13:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23165: Unit tests (6.43 KB, patch)
2019-10-17 09:00 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 23165: Renewals for Self Checkout (SCO) module do not record branchcode in statistics (4.00 KB, patch)
2019-10-17 09:01 UTC, Magnus Enger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2019-06-19 18:17:53 UTC
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.
Comment 1 Kyle M Hall 2019-06-19 18:21:28 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
Comment 2 Katrin Fischer 2019-06-20 13:39:24 UTC
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.
Comment 3 Kyle M Hall 2019-06-25 13:21:51 UTC
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.
Comment 4 Kyle M Hall 2019-06-25 13:33:46 UTC
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
Comment 5 Kyle M Hall 2019-06-26 11:44:44 UTC
Created attachment 91016 [details] [review]
Bug 23165: Unit tests
Comment 6 Tomás Cohen Arazi 2019-07-02 18:09:22 UTC
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.
Comment 7 Kyle M Hall 2019-07-02 19:38:11 UTC
(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?
Comment 8 Tomás Cohen Arazi 2019-07-02 21:48:14 UTC
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!
Comment 9 Kyle M Hall 2019-07-03 11:20:01 UTC
(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!
Comment 10 Kyle M Hall 2019-07-03 13:21:40 UTC
Created attachment 91227 [details] [review]
Bug 23165: (QA follow-up) Don't set  if it's already set for non-opac calls
Comment 11 Kyle M Hall 2019-07-03 13:45:34 UTC
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!
Comment 12 Kyle M Hall 2019-07-03 13:46:06 UTC
> Bug 17781 directly contravenes this bug. The original patch behaviors is
> correct. Thank goodness for unit tests!

s/bug/patch
Comment 13 Magnus Enger 2019-10-16 20:03:22 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2019-10-17 00:05:39 UTC Comment hidden (obsolete)
Comment 15 Magnus Enger 2019-10-17 09:00:46 UTC
Created attachment 94364 [details] [review]
Bug 23165: Unit tests

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised.
Comment 16 Magnus Enger 2019-10-17 09:01:22 UTC
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>
Comment 17 Jonathan Druart 2019-11-07 12:46:31 UTC
CONFLICT (content): Merge conflict in C4/Circulation.pm
Comment 18 Nick Clemens 2020-02-28 16:30:23 UTC
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 ***