Bug 35950 - Move the handling of statistics patron logic out of CanBookBeIssued
Summary: Move the handling of statistics patron logic out of CanBookBeIssued
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Testopia
URL:
Keywords:
Depends on: 35840
Blocks: 16122
  Show dependency treegraph
 
Reported: 2024-01-30 15:40 UTC by Lucas Gass
Modified: 2024-03-11 16:56 UTC (History)
4 users (show)

See Also:
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


Attachments
Bug 35950: Move AddReturn call into circulation.pl (2.59 KB, patch)
2024-02-29 20:50 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35950: Move AddReturn call into circulation.pl (3.11 KB, patch)
2024-02-29 21:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35950: Adjust tests (1.99 KB, patch)
2024-02-29 21:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35950: Move AddReturn call into circulation.pl (3.20 KB, patch)
2024-03-01 18:42 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 35950: Adjust tests (2.05 KB, patch)
2024-03-01 18:42 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 35950: Move AddReturn call into circulation.pl (3.49 KB, patch)
2024-03-08 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35950: Adjust tests (1.87 KB, patch)
2024-03-08 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 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 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 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 2024-02-29 20:50:45 UTC
Created attachment 162616 [details] [review]
Bug 35950: Move AddReturn call into circulation.pl
Comment 4 Lucas Gass 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 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 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 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 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 ?