Bug 35950

Summary: Move the handling of statistics patron logic out of CanBookBeIssued
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: Architecture, internals, and plumbingAssignee: Lucas Gass (lukeg) <lucas>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrew, fridolin.somers, m.de.rooy, phil
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04
Circulation function:
Bug Depends on: 35840    
Bug Blocks: 16122, 37505    
Attachments: Bug 35950: Move AddReturn call into circulation.pl
Bug 35950: Move AddReturn call into circulation.pl
Bug 35950: Adjust tests
Bug 35950: Move AddReturn call into circulation.pl
Bug 35950: Adjust tests
Bug 35950: Move AddReturn call into circulation.pl
Bug 35950: Adjust tests

Description Lucas Gass (lukeg) 2024-01-30 15:40:47 UTC
Currently CanBookBeIssued handles the logic of what to do with statistical patron checkouts and a call to AddReturn. ( see Bug 27992 )

Maybe that logic would be better handled elsewhere?  But where?  Koha::Patron->statistical_checkout? 

Also see Bug 35840
Comment 1 Martin Renvoize (ashimema) 2024-02-12 17:10:59 UTC
I think the AddReturn should be moved into circ/circulation.pl after the call to CanBookBeIssued when CanBookBeIssued returns a 'stat' response.. I also think that perhaps the actual recording of the state should be dropped from CanBookBeIssued and only take place within AddReturn.
Comment 2 Lucas Gass (lukeg) 2024-02-29 15:06:52 UTC
(In reply to Martin Renvoize from comment #1)
> I think the AddReturn should be moved into circ/circulation.pl after the
> call to CanBookBeIssued when CanBookBeIssued returns a 'stat' response.. I
> also think that perhaps the actual recording of the state should be dropped
> from CanBookBeIssued and only take place within AddReturn.

Thank you, I agree.
Comment 3 Lucas Gass (lukeg) 2024-02-29 20:50:45 UTC
Created attachment 162616 [details] [review]
Bug 35950: Move AddReturn call into circulation.pl
Comment 4 Lucas Gass (lukeg) 2024-02-29 21:06:07 UTC
Created attachment 162617 [details] [review]
Bug 35950: Move AddReturn call into circulation.pl

There should be no change in beahavior. Following the test plan from Bug 35840.

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.
Comment 5 Lucas Gass (lukeg) 2024-02-29 21:06:09 UTC
Created attachment 162618 [details] [review]
Bug 35950: Adjust tests
Comment 6 Andrew Fuerste-Henry 2024-03-01 18:42:17 UTC
Created attachment 162686 [details] [review]
Bug 35950: Move AddReturn call into circulation.pl

There should be no change in beahavior. Following the test plan from Bug 35840.

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 7 Andrew Fuerste-Henry 2024-03-01 18:42:19 UTC
Created attachment 162687 [details] [review]
Bug 35950: Adjust tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 8 Martin Renvoize (ashimema) 2024-03-08 14:57:18 UTC
Created attachment 162966 [details] [review]
Bug 35950: Move AddReturn call into circulation.pl

There should be no change in beahavior. Following the test plan from Bug 35840.

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2024-03-08 14:57:21 UTC
Created attachment 162967 [details] [review]
Bug 35950: Adjust tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2024-03-08 14:57:41 UTC
Thanks Lucas, all looks good here, Passing QA
Comment 11 Katrin Fischer 2024-03-08 17:43:57 UTC
Looking at this I was wondering, if moving from .pm to .pl is really the right move here concerning the API. Digging a little deeper, I think we are using AddIssue without CanBookBeIssued in the API, so this would not cause a change.

I'll push, but leaving this note here in case I missed something in my line of thinking! :)
Comment 12 Katrin Fischer 2024-03-08 17:59:35 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 13 Fridolin Somers 2024-03-11 14:20:42 UTC
Pushed to 23.11.x for 23.11.04
Comment 14 Fridolin Somers 2024-03-11 14:24:24 UTC
There are a lot of call on CanBookBeIssued() : ILS-DI, SIP, SCO ...
Any need to also deal with AddReturn() ?
Especially SCO may be used for local circulation no ?