Bug 9762 - Log circulation overrides
Summary: Log circulation overrides
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Hammat wele
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
: 21422 (view as bug list)
Depends on:
Blocks: 22794
  Show dependency treegraph
 
Reported: 2013-03-07 13:01 UTC by Nicole C. Engard
Modified: 2025-04-25 18:59 UTC (History)
15 users (show)

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


Attachments
Bug 9762: Log circulation overrides (21.93 KB, patch)
2023-04-12 14:48 UTC, solene.ngamga
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (29.49 KB, patch)
2023-09-26 13:21 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (28.78 KB, patch)
2024-03-25 17:22 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: fix log for test 10 13 14 15 16 17 (5.07 KB, patch)
2024-03-27 18:07 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (35.50 KB, patch)
2024-03-28 15:20 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (35.38 KB, patch)
2024-03-28 15:46 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param issueconfirmed (3.85 KB, patch)
2024-04-02 20:30 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (7.99 KB, patch)
2024-04-03 17:39 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.04 KB, patch)
2024-04-04 14:53 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: change test (1.88 KB, patch)
2024-04-05 14:13 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (33.08 KB, patch)
2024-04-08 14:32 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (7.99 KB, patch)
2024-04-08 14:32 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.04 KB, patch)
2024-04-08 14:32 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.07 KB, patch)
2024-04-08 14:43 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (33.33 KB, patch)
2024-08-13 16:53 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (8.04 KB, patch)
2024-08-13 16:54 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.07 KB, patch)
2024-08-13 16:54 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (31.62 KB, patch)
2025-01-17 15:17 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (11.92 KB, patch)
2025-01-28 20:24 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.07 KB, patch)
2025-01-28 20:24 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (28.50 KB, patch)
2025-01-28 20:24 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (14.93 KB, patch)
2025-02-13 15:54 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.07 KB, patch)
2025-02-13 15:54 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (29.46 KB, patch)
2025-02-13 15:54 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (29.46 KB, patch)
2025-02-13 16:06 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (47.30 KB, patch)
2025-04-22 16:50 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (8.63 KB, patch)
2025-04-22 16:50 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.08 KB, patch)
2025-04-22 16:50 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (follow-up) Re-instate bug 17729 (1.47 KB, patch)
2025-04-22 16:50 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (follow-up) Re-instate bug 37868 (3.84 KB, patch)
2025-04-22 16:50 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762:(QA follow-up) Fix executable bit (552 bytes, patch)
2025-04-22 16:50 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (41.00 KB, patch)
2025-04-23 08:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (8.05 KB, patch)
2025-04-23 08:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.08 KB, patch)
2025-04-23 08:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (follow-up) Re-instate bug 17729 (1.47 KB, patch)
2025-04-23 08:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (follow-up) Re-instate bug 37868 (3.84 KB, patch)
2025-04-23 08:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762:(QA follow-up) Fix executable bit (552 bytes, patch)
2025-04-23 08:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (QA follow-up) Add missing issueconfirmed check (1.34 KB, patch)
2025-04-23 08:29 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Log circulation overrides (26.36 KB, patch)
2025-04-23 10:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Change how to pass the param (8.05 KB, patch)
2025-04-23 10:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: Test CanBookBeIssued.pm (13.08 KB, patch)
2025-04-23 10:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (QA follow-up) Fix executable bit (553 bytes, patch)
2025-04-23 10:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (QA follow-up) Add missing issueconfirmed check (1.34 KB, patch)
2025-04-23 10:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (QA follow-up) Fix failing test (820 bytes, patch)
2025-04-23 10:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (QA follow-up) Remove superflous string construction (9.45 KB, patch)
2025-04-23 10:15 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (Alternative) Log circulation overrides (10.22 KB, patch)
2025-04-23 13:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (Alternative) Log circulation overrides (14.12 KB, patch)
2025-04-23 14:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (Alternative) Log circulation overrides (15.89 KB, patch)
2025-04-23 15:10 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (Alternative) Log circulation overrides (16.82 KB, patch)
2025-04-23 15:19 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (Alternative) Log circulation overrides (16.82 KB, patch)
2025-04-25 16:21 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 9762: (Alternative) Handle renewals (9.84 KB, patch)
2025-04-25 16:21 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2013-03-07 13:01:25 UTC
It would be great if there were a way to track whenever someone overrides a warning message in circulation.  Whether it's ignoring a hold for someone or checking out even though the patron owes too much - any of the warnings with buttons listed here: http://manual.koha-community.org/3.12/en/circulation.html#checkoutwarn or here: http://manual.koha-community.org/3.12/en/checkingin.html#checkinmsg  would be tracked.
Comment 1 Katrin Fischer 2019-06-08 05:55:15 UTC
*** Bug 21422 has been marked as a duplicate of this bug. ***
Comment 2 Caroline Cyr La Rose 2021-01-07 17:04:27 UTC
I'm trying to make a list of all possibilities where someone can override a restriction. I don't know if it's useful for a developer...

Circulation -- Checking out

- Check out an item that is age restricted (AgeRestrictionOverride syspref)
- Check out an item that has a not for loan status (AllowNotForLoanOverride syspref)
- Check out an item that has a lost status (IssueLostItem syspref)
- Check out an item to a patron who has reached the checkout limit (AllowTooManyOverride syspref)
- Check out an item to a patron who is not allowed to borrow this item type
- Check out an item to a patron who has unpaid fines (AllFinesNeedOverride and/or AllowFineOverride + noissuescharge sysprefs)
- Check out an item on hold for someone else
- Check out an item on hold ready for pickup by someone else
- Check out an item already checked out to someone else (AutoReturnCheckedOutItems syspref)
- Check out to a patron who has restrictions

Circulation -- Checking in

- Ignore a hold upon check in
- Ignore a hold & transfer upon check in

Circulation -- Renewing

- Renew an item on hold for someone else
- Renew an item that has reached the maximum amount of renewals

Holds

- Place a hold for a patron who is not allowed to put this item type on hold
- Place a hold for a patron who has reached the maximum amount of holds
- Place an item-level hold when rules say this is not allowed
Comment 3 Martha Fuerst 2021-01-29 18:18:06 UTC
We are also very interested in this logging.

Other things to consider in addition to Caroline's list, depending on how sysprefs are set:

- Check out an item to a patron who has guarantee's who have unpaid fines (NoIssuesChargeGuarantees)
- Check out an item to a patron who has overdues (OverduesBlockCirc)
- Renew an item for a patron who has overdues (OverduesBlockRenewing)
- Override circulation fine rules via "specify dute dute"

Thanks for the attention on this enhancement!

-Marti Fuerst, Huntsville-Madison County Public Library
Comment 4 Michael Adamyk 2021-02-01 15:00:31 UTC
As a consortia library, this would also be helpful for us. If we have librarians overriding restrictions (such as circulation rules), it can cause problems if the rules are for another library. Knowing which login is doing this would help track down what's going on.
Comment 5 Michael Adamyk 2021-02-01 15:01:46 UTC
As a consortia library, this would also be helpful for us. If we have librarians overriding restrictions (such as circulation rules), it can cause problems if the rules are for another library. Knowing which login is doing this would help track down what's going on.
Comment 6 solene.ngamga 2023-04-12 14:48:40 UTC Comment hidden (obsolete)
Comment 7 Lucas Gass (lukeg) 2023-04-14 14:48:55 UTC
Solene,

Most of this is working and this will be a great addition! A few things though:

-Do these strings need to translatable? 
-This is going to need unit tests.
Comment 8 Katrin Fischer 2023-04-15 21:12:55 UTC
(In reply to Lucas Gass from comment #7)
> Solene,
> 
> Most of this is working and this will be a great addition! A few things
> though:
> 
> -Do these strings need to translatable? 
> -This is going to need unit tests.

The messages in action_logs are usually not translated, but they are also usually not sentences/text, but data changes in JSON and other machine friendly syntax. Could we use codes instead of plain text and then translate them on the action log viewer? This would make them translatable and we could also adjust the texts easier.

I expected to find override in the descriptions or it being a new type of action. Maybe we could be a little bit clearer that these were all override actions.
Comment 9 Emily-Rose Francoeur 2023-09-26 13:21:17 UTC
Created attachment 156222 [details] [review]
Bug 9762: Log circulation overrides

This patch allows you to save commands that have been overridden by someone.

The patch has been updated but is not yet completed. Tests 10, 13, 14, and 17 are still not functional. Tests 15 and 16 need to be fixed as they do not display the barcode. All other tests should be functional.

To test:

--After each step, a message will be displayed in the action_logs table and in Tools > Log viewer --

Circulation -- Checking out

1) Check out an item that is age-restricted (AgeRestrictionOverride syspref)
2) Check out an item that has a “not for loan” status (AllowNotForLoanOverride syspref)
3) Check out an item that has a “lost” status (IssueLostItem syspref)
4) Check out an item to a patron who has reached the checkout limit (AllowTooManyOverride syspref)
5) Check out an item to a patron who is not allowed to borrow this item type
6) Check out an item to a patron who has unpaid fines (AllFinesNeedOverride and/or AllowFineOverride + noissuescharge sysprefs)
7) Check out an item on hold for someone else
8) Check out an item on hold ready for pickup by someone else
9) Check out an item already checked out to someone else (AutoReturnCheckedOutItems syspref)
10) Check out to a patron who has restrictions

Circulation -- Checking in

11) Ignore a hold upon check in
12) Ignore a hold and transfer upon check in

Circulation – Renewing

13) Renew an item on hold for someone else (AllowRenewalOnHoldOverride syspref)
14) Renew an item that has reached the maximum number of renewals (AllowRenewalLimitOverride syspref)

Holds

15) Place a hold for a patron who is not allowed to place this item type on hold (AllowHoldPolicyOverride syspref)
16) Place a hold for a patron who has reached the maximum number of holds
17) Place an item-level hold when rules dictate that this is not allowed
Comment 10 Matthias Le Gac 2024-03-25 17:22:21 UTC Comment hidden (obsolete)
Comment 11 Matthias Le Gac 2024-03-27 18:07:48 UTC Comment hidden (obsolete)
Comment 12 Lucas Gass (lukeg) 2024-03-27 19:18:29 UTC
Lots of issues here. 

1. renew.pl now fails with this error:

Could not compile /kohadevbox/koha/circ/renew.pl: Global symbol "$borrower" requires explicit package name (did you forget to declare "my $borrower"?) at /kohadevbox/koha/circ/renew.pl line 67.

2. Looks like some debugging code is leftover 

3. request.pl has a whole block of code commented out starting with: 

# elsif ( C4::Context->preference('AllowHoldPolicyOverride') ){

4. Why remove the check for "unless( $skip_record_index )" in AddRenewal? I think there are times when we want to skip indexing. 

5.  FAIL	C4/Circulation.pm
   FAIL	  pod coverage
		POD is missing for CanBookBeReturned

6.  FAIL	circ/renew.pl
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 28, now: 44)
   FAIL	  valid
		Global symbol "$borrower" requires explicit package name (did you forget to declare "my $borrower"?)
		Global symbol "$issue" requires explicit package name (did you forget to declare "my $issue"?)
		Global symbol "$borrower" requires explicit package name (did you forget to declare "my $borrower"?)
		Global symbol "$borrower" requires explicit package name (did you forget to declare "my $borrower"?)
		Global symbol "$borrower" requires explicit package name (did you forget to declare "my $borrower"?)
		Global symbol "$borrower" requires explicit package name (did you forget to declare "my $borrower"?)
		circ/renew.pl had compilation errors.


7. Changes to CanBookBeIssued and AddRenewal probably need test coverage.
Comment 13 Matthias Le Gac 2024-03-28 15:20:50 UTC Comment hidden (obsolete)
Comment 14 Matthias Le Gac 2024-03-28 15:23:39 UTC
For the "skip_record_index" I don't now why Emily deleted this code but I thing we need this code so I put it back. Tell me if it's good??
Comment 15 Matthias Le Gac 2024-03-28 15:25:54 UTC
And I will make a new patch for test Circulation.pm
Comment 16 Matthias Le Gac 2024-03-28 15:46:35 UTC Comment hidden (obsolete)
Comment 17 Matthias Le Gac 2024-04-02 20:30:14 UTC Comment hidden (obsolete)
Comment 18 Matthias Le Gac 2024-04-03 17:39:38 UTC Comment hidden (obsolete)
Comment 19 Matthias Le Gac 2024-04-04 14:53:58 UTC Comment hidden (obsolete)
Comment 20 Matthias Le Gac 2024-04-04 14:58:33 UTC
I created a separate test file from circulation.t because I think it's simpler as it tests the message sending for the method CanBookBeIssued in Circulation.pm method and also on master and without having applied the patch the Circulation.t test didn't work so I couldn't test my tests.
Comment 21 Matthias Le Gac 2024-04-05 14:13:59 UTC Comment hidden (obsolete)
Comment 22 Matthias Le Gac 2024-04-05 14:14:55 UTC Comment hidden (obsolete)
Comment 23 Matthias Le Gac 2024-04-08 14:32:48 UTC Comment hidden (obsolete)
Comment 24 Matthias Le Gac 2024-04-08 14:32:51 UTC Comment hidden (obsolete)
Comment 25 Matthias Le Gac 2024-04-08 14:32:54 UTC Comment hidden (obsolete)
Comment 26 Matthias Le Gac 2024-04-08 14:33:43 UTC
Rebase to master
Comment 27 Matthias Le Gac 2024-04-08 14:43:53 UTC Comment hidden (obsolete)
Comment 28 Anneli Österman 2024-04-26 10:59:26 UTC
I tested in Bywater's sandbox points 1, 2, 3, 10 and 16 and was about to comment that these doesn't work because there were no logs for those borrowers whose restriction was overridden. Then I checked the action_logs table with SQL report and noticed that there actually were logs for overrides. But there were no object recorded which is why I did not find anything at first with the log viewer.

Should the borrowernumber of the borrower whose resctriction is overridden be recorded as well to the object column in action_logs? It would help to find those overrides more easily. I intuitively looked for them from borrowers logs.
Comment 29 Matthias Le Gac 2024-04-26 20:11:54 UTC
I think the info is already there, either in the "Object column", there's a link to the person who checkout. If that's what you mean. The logs are in Tools > Log viewer.
Comment 30 Anneli Österman 2024-04-29 09:39:29 UTC
Patron's borrowernumber was in info column but not in object column which is usually used when action concerns certain borrower.

For example, if someone asks, how is it possible that this borrower has issues even when borrower is restricted. I would start to investigate the problem by searching with patron's borrowernumber from object column and would get nothing. I would have to know that in restriction overrides patron's borrowernumber is in info column and I would have to search using that one.

What I'm trying to say is that if there were a problem, I would start from patron's action log view. Now you kind of have start from staff memeber's action log view but the problem is, that you do not know which staff member to look for. Or you have to make large search by info column when you get lots of unnecessary hits with itemnumbers or other numbers that happens to be same as the borrowernumber.
Comment 31 Hammat wele 2024-08-13 16:53:59 UTC Comment hidden (obsolete)
Comment 32 Hammat wele 2024-08-13 16:54:03 UTC Comment hidden (obsolete)
Comment 33 Hammat wele 2024-08-13 16:54:08 UTC Comment hidden (obsolete)
Comment 34 Hammat wele 2024-08-13 16:55:06 UTC
Rebase to main
Comment 35 Martin Renvoize (ashimema) 2024-11-29 15:59:08 UTC
Sorry Hammat, needs another rebase.
Comment 36 William Lavoie 2025-01-17 15:17:44 UTC Comment hidden (obsolete)
Comment 37 William Lavoie 2025-01-17 15:18:29 UTC
Rebased
Comment 38 Lucas Gass (lukeg) 2025-01-17 15:46:15 UTC
Sorry William, I just pulled main this AM and tried applying this, it does not cleanly apply.
Comment 39 William Lavoie 2025-01-28 20:24:45 UTC Comment hidden (obsolete)
Comment 40 William Lavoie 2025-01-28 20:24:48 UTC Comment hidden (obsolete)
Comment 41 William Lavoie 2025-01-28 20:24:52 UTC Comment hidden (obsolete)
Comment 42 William Lavoie 2025-02-13 15:54:12 UTC Comment hidden (obsolete)
Comment 43 William Lavoie 2025-02-13 15:54:15 UTC Comment hidden (obsolete)
Comment 44 William Lavoie 2025-02-13 15:54:18 UTC Comment hidden (obsolete)
Comment 45 William Lavoie 2025-02-13 16:06:40 UTC Comment hidden (obsolete)
Comment 46 William Lavoie 2025-02-13 16:13:14 UTC
This needed a rebase.
Comment 47 Lucas Gass (lukeg) 2025-04-01 14:40:25 UTC
(In reply to William Lavoie from comment #46)
> This needed a rebase.

Sorry William, there are quite a few problems now:


1. There are some code issues:

"my" variable $dbh masks earlier declaration in same scope at /kohadevbox/koha/C4/Circulation.pm line 3669.
"my" variable $patron masks earlier declaration in same scope at /kohadevbox/koha/C4/Circulation.pm line 3676.


2. I would check your release_tools, many things are not tidy 

3. Unit tests are failing
Comment 48 Martin Renvoize (ashimema) 2025-04-22 16:50:37 UTC Comment hidden (obsolete)
Comment 49 Martin Renvoize (ashimema) 2025-04-22 16:50:41 UTC Comment hidden (obsolete)
Comment 50 Martin Renvoize (ashimema) 2025-04-22 16:50:45 UTC Comment hidden (obsolete)
Comment 51 Martin Renvoize (ashimema) 2025-04-22 16:50:49 UTC Comment hidden (obsolete)
Comment 52 Martin Renvoize (ashimema) 2025-04-22 16:50:52 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize (ashimema) 2025-04-22 16:50:57 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize (ashimema) 2025-04-22 17:00:42 UTC
Rebased, but still failed QA for now with the following tests failing:

Running tests (1)
        * Proving /kohadevbox/koha/t/db_dependent/Circulation/CanBookBeIssued.t KO!

#   Failed test 'Borrower had amend message displayed'
#   at /kohadevbox/koha/t/db_dependent/Circulation/CanBookBeIssued.t line 231.
#          got: undef
#     expected: 'borrower had amend'
 # Looks like you failed 1 test of 9.
 /kohadevbox/koha/t/db_dependent/Circulation/CanBookBeIssued.t ..
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/9 subtests

Test Summary Report
-------------------
 /kohadevbox/koha/t/db_dependent/Circulation/CanBookBeIssued.t (Wstat: 256 (exited 1) Tests: 9 Failed: 1)
  Failed test:   4
   Non-zero exit status: 1
Files=1, Tests=9,  2 wallclock secs ( 0.01 usr  0.00 sys +  1.40 cusr  0.16 csys =  1.57 CPU)
 Result: FAIL


Running more tests (1)
        * Proving t/db_dependent/Circulation.t KO!
    # Looks like you planned 27 tests but ran 5.

#   Failed test 'CanBookBeIssued & AllowReturnToBranch'
#   at t/db_dependent/Circulation.t line 2546.
Can't call method "itemnumber" on an undefined value at /kohadevbox/koha/C4/Circulation.pm line 1478.
 # Looks like your test exited with 11 just after 27.
 t/db_dependent/Circulation.t ..
Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 50/76 subtests

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 2816 (exited 11) Tests: 27 Failed: 1)
  Failed test:  26
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 76 tests but ran 27.
Files=1, Tests=27,  5 wallclock secs ( 0.01 usr  0.01 sys +  3.57 cusr  0.44 csys =  4.03 CPU)
 Result: FAIL

I've not got a moment right now to work through those test failures I'm afraid.
Comment 55 Martin Renvoize (ashimema) 2025-04-23 08:28:39 UTC Comment hidden (obsolete)
Comment 56 Martin Renvoize (ashimema) 2025-04-23 08:28:43 UTC Comment hidden (obsolete)
Comment 57 Martin Renvoize (ashimema) 2025-04-23 08:28:47 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize (ashimema) 2025-04-23 08:28:51 UTC Comment hidden (obsolete)
Comment 59 Martin Renvoize (ashimema) 2025-04-23 08:28:55 UTC Comment hidden (obsolete)
Comment 60 Martin Renvoize (ashimema) 2025-04-23 08:28:59 UTC Comment hidden (obsolete)
Comment 61 Martin Renvoize (ashimema) 2025-04-23 08:29:03 UTC Comment hidden (obsolete)
Comment 62 Martin Renvoize (ashimema) 2025-04-23 08:51:12 UTC
Comment on attachment 181340 [details] [review]
Bug 9762: Log circulation overrides

Review of attachment 181340 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +832,4 @@
>      my $circ_library = Koha::Libraries->find( _GetCircControlBranch( $item_object, $patron ) );
>  
>      my $now = dt_from_string();
> +    my $message;

I don't see `$message` passed anywhere.. we seem to construct what could be a huge concatenated string below, but then never output it anywhere.

We also have this on line 1822:

  `my ( $allowed, $message ) = CanBookBeReturned( $item_unblessed, C4::Context->userenv->{branch} );`

This will overwrite whatever was in $message above.. and it's intended to just be a branchcode passed back in that case.
Comment 63 Martin Renvoize (ashimema) 2025-04-23 10:15:25 UTC Comment hidden (obsolete)
Comment 64 Martin Renvoize (ashimema) 2025-04-23 10:15:29 UTC Comment hidden (obsolete)
Comment 65 Martin Renvoize (ashimema) 2025-04-23 10:15:35 UTC Comment hidden (obsolete)
Comment 66 Martin Renvoize (ashimema) 2025-04-23 10:15:39 UTC Comment hidden (obsolete)
Comment 67 Martin Renvoize (ashimema) 2025-04-23 10:15:43 UTC Comment hidden (obsolete)
Comment 68 Martin Renvoize (ashimema) 2025-04-23 10:15:47 UTC Comment hidden (obsolete)
Comment 69 Martin Renvoize (ashimema) 2025-04-23 10:15:51 UTC Comment hidden (obsolete)
Comment 70 Martin Renvoize (ashimema) 2025-04-23 10:17:43 UTC
Squashed rebase follow-ups and made some more rebase corrections.

One test is still failing, and the string type record is still an issue.. I still need to test this all in detail too before I think it's QA runnnable.
Comment 71 Martin Renvoize (ashimema) 2025-04-23 10:26:13 UTC
This somehow feels like it's recording twice or is just at the wrong level to me.. 

I think I'd remove the changes to C4::Circulation::CanBookBeIssued and perform the record in the controllers.. or perhaps in AddIssue, AddRenewal.. those are the places we actually store the Issue/Renewal so it seems somewhat odd to record the override before the actual Issue/Renewal takes place to me.
Comment 72 Martin Renvoize (ashimema) 2025-04-23 13:26:42 UTC
Created attachment 181371 [details] [review]
Bug 9762: (Alternative) Log circulation overrides

An alternative approach to the original submission for bug 9762. Instead
of recording a distinct action log line for a set of override messages
in addition to the standard issue record, this patch updates the
existing issue log to include any confirmation or impossible responses
from the preceeding CanBookBeIssued call.

We continue to record these as a JSON structure as the prior patch did,
but instead of human messages we use the predefined codes as returned
from the existing CanBookBeIssued response.

To test:

After each step, a single action log message should be displayed under
`Tools > Log viewer`. The message itself will now consist of a JSON
string with an array of 'confirmations' and 'forced' strings
representing the overriden feedback from CanBookBeIssued.

Circulation -- Checking out

1) Check out an item that is age-restricted (AgeRestrictionOverride syspref)
2) Check out an item that has a “not for loan” status (AllowNotForLoanOverride
   syspref)
3) Check out an item that has a “lost” status (IssueLostItem syspref)
4) Check out an item to a patron who has reached the checkout limit
   (AllowTooManyOverride syspref)
5) Check out an item to a patron who is not allowed to borrow this item type
6) Check out an item to a patron who has unpaid fines (AllFinesNeedOverride
   and/or AllowFineOverride + noissuescharge sysprefs)
7) Check out an item on hold for someone else
8) Check out an item on hold ready for pickup by someone else
9) Check out an item already checked out to someone else
   (AutoReturnCheckedOutItems syspref)
10) Check out to a patron who has restrictions

Circulation -- Checking in

11) Ignore a hold upon check in
12) Ignore a hold and transfer upon check in

Circulation – Renewing

13) Renew an item on hold for someone else
    (AllowRenewalOnHoldOverride syspref)
14) Renew an item that has reached the maximum number of renewals
    (AllowRenewalLimitOverride syspref)

Holds

15) Place a hold for a patron who is not allowed to place this item
    type on hold (AllowHoldPolicyOverride syspref)
16) Place a hold for a patron who has reached the maximum number of
    holds
17) Place an item-level hold when rules dictate that this is not
    allowed
Comment 73 Martin Renvoize (ashimema) 2025-04-23 13:27:54 UTC
So.. I've added an alternative approach for comment.  It's not fully implemented yet as it only covers calls to AddIssue and we should also consider Renewal and Return override as the original patchset does.
Comment 74 Martin Renvoize (ashimema) 2025-04-23 14:44:10 UTC Comment hidden (obsolete)
Comment 75 Martin Renvoize (ashimema) 2025-04-23 15:10:31 UTC Comment hidden (obsolete)
Comment 76 Martin Renvoize (ashimema) 2025-04-23 15:19:32 UTC Comment hidden (obsolete)
Comment 77 Martin Renvoize (ashimema) 2025-04-23 15:21:58 UTC
Forgot to mention.. this version is translatable ;)
Comment 78 Martin Renvoize (ashimema) 2025-04-25 16:21:53 UTC Comment hidden (obsolete)
Comment 79 Martin Renvoize (ashimema) 2025-04-25 16:21:57 UTC Comment hidden (obsolete)