Bug 40866

Summary: Corrections to override logging
Product: Koha Reporter: Lucas Gass (lukeg) <lucas>
Component: Staff interfaceAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Needs documenting --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: caroline.cyr-la-rose, fridolin.somers, gmcharlt, hammat.wele, jonathan.druart, laura, martin.renvoize, michael.hafen, paul.derscheid, philippe.blouin, wainuiwitikapark
Version: MainKeywords: rel_25_05_candidate
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00,25.05.05
Circulation function:
Bug Depends on: 9762    
Bug Blocks:    
Attachments: Bug 40866: Fix incorrect display of override message on normal checkouts
Bug 40866: Make message translatable
Bug 40866: Move or remove other variables added with the message
Bug 40866: Fix incorrect display of override message on normal checkouts
Bug 40866: Fix incorrect display of override message on normal checkouts
Bug 40866: Remove unused $message variable from circulation
Bug 40866: Remove original Bug 9762 implementation remnants (Part 1)
Bug 40866: Remove original Bug 9762 implementation remnants (Part 2 - Renewals)
Bug 40866: Remove original Bug 9762 implementation remnants (Part 3 - Returns)
Bug 40866: Implement Alternative pattern for holds override logging (Part 4)
Bug 40866: Add unit tests for hold override logging
Bug 40866: Add missing override states to action-logs.inc
Bug 40866: Remove OVERRIDE state from action-logs.inc
Bug 40866: Fix incorrect display of override message on normal checkouts
Bug 40866: Remove unused $message variable from circulation
Bug 40866: Remove original Bug 9762 implementation remnants (Part 1)
Bug 40866: Remove original Bug 9762 implementation remnants (Part 2 - Renewals)
Bug 40866: Remove original Bug 9762 implementation remnants (Part 3 - Returns)
Bug 40866: Implement Alternative pattern for holds override logging (Part 4)
Bug 40866: Add unit tests for hold override logging
Bug 40866: Add missing override states to action-logs.inc
Bug 40866: Remove OVERRIDE state from action-logs.inc
Bug 40866: Fix incorrect display of override message on normal checkouts
Bug 40866: Remove unused $message variable from circulation
Bug 40866: Remove original Bug 9762 implementation remnants (Part 1)
Bug 40866: Remove original Bug 9762 implementation remnants (Part 2 - Renewals)
Bug 40866: Remove original Bug 9762 implementation remnants (Part 3 - Returns)
Bug 40866: Implement Alternative pattern for holds override logging (Part 4)
Bug 40866: Add unit tests for hold override logging
Bug 40866: Add missing override states to action-logs.inc
Bug 40866: Remove OVERRIDE state from action-logs.inc
Bug 40866: [25.05.x] Remove original implementation of Bug 9762 and implement alternative squashed patchset for 25.05
Bug 40866: (follow-up) Remove obsolete CanBookBeIssued.t test file

Description Lucas Gass (lukeg) 2025-09-23 19:35:11 UTC
To test:

1. Check a thing out to a patron, any patron and any thing. 
2. See the message "No patron matched Restriction overridden temporarily"

This seems to be a product of 9762.

The string also seems to be untranslatable: 
Bug https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9762#c117
Comment 1 Paul Derscheid 2025-09-26 16:08:30 UTC
Created attachment 186985 [details] [review]
Bug 40866: Fix incorrect display of override message on normal checkouts

- The message "No patron matched Restriction overridden temporarily" was appearing on every checkout, even when no restriction override occurred.
- This was caused by the message being set unconditionally after AddIssue in circ/circulation.pl.
- The fix moves the message assignment inside the if ($issueconfirmed) block, ensuring it only displays when an override is actually confirmed.

To test:
1. Check out an item to a patron normally.
2. "Restriction overridden temporarily" message appears.
3. Apply patches
4. Repeat 1.
5. Verify that no "Restriction overridden temporarily" message appears.
6. Check out an item that requires an override (e.g., not for loan item with AllowNotForLoanOverride enabled).
7. Confirm the override in the UI.
8. Verify that the "Restriction overridden temporarily" message now appears.
9. Sign-off
Comment 2 Paul Derscheid 2025-09-26 16:08:32 UTC
Created attachment 186986 [details] [review]
Bug 40866: Make message translatable
Comment 3 Paul Derscheid 2025-09-26 16:09:24 UTC
Note sure whether that is the best place for that message given that we have to introduce Koha::I18N?
Comment 4 Michael Hafen 2025-09-29 17:16:39 UTC
I would like to upload alternate patchs that moves or removes the other variables that were introduced by bug 9762, if that is ok.
Comment 5 Paul Derscheid 2025-09-29 17:24:13 UTC
Nah, it's too late for that @Michael Hafen.
Fixing issues in the original bugs post push to maintenance branches always causes confusion.
If the patches in this bug are not sufficient, add more or open a new bug, please :)
Comment 6 Michael Hafen 2025-09-29 17:26:48 UTC
I will add a patch to this bug then.  I would like these variables to be kept together.
Thank you.

(In reply to Paul Derscheid from comment #5)
> Nah, it's too late for that @Michael Hafen.
> Fixing issues in the original bugs post push to maintenance branches always
> causes confusion.
> If the patches in this bug are not sufficient, add more or open a new bug,
> please :)
Comment 7 Michael Hafen 2025-09-29 17:31:42 UTC
Created attachment 187063 [details] [review]
Bug 40866: Move or remove other variables added with the  message
Comment 8 Hammat wele 2025-09-30 16:45:40 UTC
Created attachment 187116 [details] [review]
Bug 40866: Fix incorrect display of override message on normal checkouts

- The message 'No patron matched Restriction overridden temporarily' was appearing on every checkout, even when no restriction override occurred.
- This was caused by the message being set unconditionally after AddIssue in circ/circulation.pl.
- The message should not appear in circulation.pl, but in the Log Viewer.

To test:

1. Check out an item to a patron normally.
2. 'Restriction overridden temporarily' message appears.
3. Apply patches
4. Repeat 1.
  ---> Verify that no 'Restriction overridden temporarily' message appears.
5. Check out an item that requires an override (e.g., not for loan item with AllowNotForLoanOverride enabled).
  ---> Verify that no 'Restriction overridden temporarily' message appears.
6. Go to Tool -> Log viewer.
7. Enter the borrowernumber in Object field and submit.
  ---> Verify that the 'Restriction overridden temporarily' message is logged correctly.
Comment 9 Hammat wele 2025-09-30 16:45:57 UTC
The message 'Restriction overridden temporarily' is not meant to appear in circulation.pl, but in the Log Viewer (see Bug 9762).
Comment 10 Lucas Gass (lukeg) 2025-10-06 20:27:37 UTC
We don't need to logaction here? Why is that being deleted?
Comment 11 Jonathan Druart 2025-10-07 07:06:05 UTC
FQA as per last comment.
Comment 12 Martin Renvoize (ashimema) 2025-10-09 08:17:44 UTC
Created attachment 187627 [details] [review]
Bug 40866: Fix incorrect display of override message on normal checkouts

- The message 'No patron matched Restriction overridden temporarily' was appearing on every checkout, even when no restriction override occurred.
- This was caused by the message being set unconditionally after AddIssue in circ/circulation.pl.
- The message should not appear in circulation.pl, but in the Log Viewer.

To test:

1. Check out an item to a patron normally.
2. 'Restriction overridden temporarily' message appears.
3. Apply patches
4. Repeat 1.
  ---> Verify that no 'Restriction overridden temporarily' message appears.
5. Check out an item that requires an override (e.g., not for loan item with AllowNotForLoanOverride enabled).
  ---> Verify that no 'Restriction overridden temporarily' message appears.
6. Go to Tool -> Log viewer.
7. Enter the borrowernumber in Object field and submit.
  ---> Verify that the 'Restriction overridden temporarily' message is logged correctly.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 13 Martin Renvoize (ashimema) 2025-10-09 08:17:47 UTC
Created attachment 187628 [details] [review]
Bug 40866: Remove unused $message variable from circulation

The $message variable was declared in circ/circulation.pl but never
actually set, making the "No patron matched" template block unreachable.

This is a remnant from old code that was incompletely cleaned when that
functionality was changed/removed.

This patch removes the unused variable, template parameter, and the
unreachable template block.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 14 Martin Renvoize (ashimema) 2025-10-09 08:17:49 UTC
Created attachment 187629 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 1)

This patch removes the duplicate logging code from the original
Bug 9762 implementation that was not properly removed when the
Alternative implementation was merged.

The original implementation added separate logaction() calls in
CanBookBeIssued and circulation.pl that created additional log
entries with a different JSON format. This caused double logging
when the Alternative implementation (which logs via AddIssue) was
merged on top.

Changes in this patch:

C4/Circulation.pm:
- Removed @message_log array and all pushes to it
- Removed the separate logaction() call at the end of CanBookBeIssued
- Removed 5th return value (@message_log) from CanBookBeIssued
- Removed all if ($issueconfirmed) blocks that were populating @message_log

circ/circulation.pl:
- Removed the problematic line that added OVERRIDDEN to every confirmed
  checkout: $needsconfirmation->{'OVERRIDDEN'} = $issueconfirmed

This addresses the double logging issue where circulation overrides
were being logged twice with different formats.
Comment 15 Martin Renvoize (ashimema) 2025-10-09 08:17:52 UTC
Created attachment 187630 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 2 - Renewals)

This patch removes the duplicate logging code from renewal scripts
(circ/renew.pl and svc/renew) that was part of the original Bug 9762
implementation.

The original implementation created separate logaction() calls with
custom JSON messages whenever renewal overrides occurred. This caused
double logging alongside the Alternative implementation which properly
logs overrides via the confirmations parameter in AddRenewal.

Changes:

circ/renew.pl:
- Removed unused variables: $user, $branchcode, @message
- Removed separate logaction() calls for override logging
- Changed to use confirmations array with proper codes:
  * 'RENEWAL_LIMIT' for override_limit
  * 'ON_RESERVE' for override_holds
- Removed C4::Log qw(logaction) import

svc/renew:
- Removed unused variables: $borrower, $user, $message, @message
- Removed separate logaction() calls for override logging
- Changed to use confirmations array with proper codes:
  * 'ON_RESERVE' for on_reserve override
  * 'RENEWAL_LIMIT' for too_many override
- Removed C4::Log qw(logaction) import

The Alternative implementation in AddRenewal will now handle all
override logging consistently through the confirmations parameter.
Comment 16 Martin Renvoize (ashimema) 2025-10-09 08:17:54 UTC
Created attachment 187631 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 3 - Returns)

This patch removes the duplicate logging code from circ/returns.pl that
was part of the original Bug 9762 implementation.

The original implementation added a separate logaction() call when
ignoring holds on return. This code created a custom JSON log entry
with the message "return an element that is reserved".

However, this logging appears to have been orphaned - there is no
corresponding Alternative implementation for return override logging
in the current codebase. The code was creating log entries but they
were not properly integrated with any override confirmation workflow.

Changes:

circ/returns.pl:
- Removed entire else block that created custom override log entries
- Removed unused variables: $message, @message, $user, $branchcode, $barcode
- Removed C4::Log qw(logaction) import as it's no longer needed

This eliminates the orphaned logging code and ensures returns.pl only
uses standard Koha logging mechanisms.
Comment 17 Martin Renvoize (ashimema) 2025-10-09 08:17:57 UTC
Created attachment 187632 [details] [review]
Bug 40866: Implement Alternative pattern for holds override logging (Part 4)

This patch implements proper override logging for holds following the
Alternative implementation pattern used for checkouts and renewals.

Instead of just removing the orphaned logging code from the original
Bug 9762 implementation, this patch properly implements override logging
by updating AddReserve to accept and log override information, similar
to how AddIssue handles confirmations and forced overrides.

Changes:

C4/Reserves.pm:
- Added JSON qw(to_json) import
- Updated AddReserve to accept confirmations and forced parameters
- Enhanced the existing logaction call to include override information
  in JSON format when overrides are present
- Log format matches the Alternative implementation pattern:
  * hold ID
  * branchcode
  * biblionumber
  * itemnumber
  * confirmations array
  * forced array

Koha/REST/V1/Holds.pm:
- Added confirmations array building in the add() method
- Pass 'HOLD_POLICY_OVERRIDE' confirmation when override_all is true
- Pass confirmations parameter to AddReserve call

reserve/request.pl:
- Removed orphaned original implementation logging code
- Removed unused variables and imports (message, @message, user, etc.)
- Removed C4::Log import

koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc:
- Added 'HOLD_POLICY_OVERRIDE' case to translate_log_override block
- Displays as "Hold policy override" in log viewer

koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt:
- Added JSON display handling for HOLDS module (similar to CIRCULATION)
- Displays hold ID, biblionumber, and optional itemnumber with links
- Shows forced and confirmation overrides using translate_log_override

This ensures hold overrides are logged consistently with the Alternative
implementation pattern, providing proper audit trails for policy overrides
that are properly displayed in the log viewer.
Comment 18 Martin Renvoize (ashimema) 2025-10-09 08:18:00 UTC
Created attachment 187633 [details] [review]
Bug 40866: Add unit tests for hold override logging

This patch adds unit tests to verify the hold override logging
functionality implemented in the previous patches. The tests cover the
logging of hold overrides in `C4/Reserves.pm` and the `Koha/REST/V1/Holds.pm`
API endpoint.

To test:
1. Run the following tests and confirm they pass:
   - prove -lrv t/db_dependent/Circulation.t
   - prove -lrv t/db_dependent/Reserves.t
   - prove -lrv t/db_dependent/api/v1/holds.t
2. All tests should pass.

Changes:

t/db_dependent/Circulation.t:
- Added tests for hold override scenarios in circulation.

t/db_dependent/Reserves.t:
- Added tests to verify that hold overrides are correctly logged when
  a hold is placed.

t/db_dependent/api/v1/holds.t:
- Added tests to ensure that the holds API correctly logs overrides
  when a hold is created with the `x-koha-override` header.
Comment 19 Martin Renvoize (ashimema) 2025-10-09 15:09:05 UTC
Created attachment 187675 [details] [review]
Bug 40866: Add missing override states to action-logs.inc

This follow-up adds missing override state translations that were
discovered during testing. The patchset logs various override states
but some were not covered in the log viewer display template.

Missing states added:
- USERBLOCKEDNOENDDATE: Patron had an indefinite restriction
- CURRENTISSUE: Item is currently checked out to this patron
- UNSEEN: Unseen renewal
- SEEN: Seen renewal

These states are now properly displayed in the action logs viewer
when reviewing circulation override logging.
Comment 20 Martin Renvoize (ashimema) 2025-10-09 15:19:24 UTC
Created attachment 187676 [details] [review]
Bug 40866: Remove OVERRIDE state from action-logs.inc

This state is no longer ever set anywhere
Comment 21 Andrew Fuerste-Henry 2025-10-09 15:21:11 UTC
Created attachment 187677 [details] [review]
Bug 40866: Fix incorrect display of override message on normal checkouts

- The message 'No patron matched Restriction overridden temporarily' was appearing on every checkout, even when no restriction override occurred.
- This was caused by the message being set unconditionally after AddIssue in circ/circulation.pl.
- The message should not appear in circulation.pl, but in the Log Viewer.

To test:

1. Check out an item to a patron normally.
2. 'Restriction overridden temporarily' message appears.
3. Apply patches
4. Repeat 1.
  ---> Verify that no 'Restriction overridden temporarily' message appears.
5. Check out an item that requires an override (e.g., not for loan item with AllowNotForLoanOverride enabled).
  ---> Verify that no 'Restriction overridden temporarily' message appears.
6. Go to Tool -> Log viewer.
7. Enter the borrowernumber in Object field and submit.
  ---> Verify that the 'Restriction overridden temporarily' message is logged correctly.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 22 Andrew Fuerste-Henry 2025-10-09 15:21:14 UTC
Created attachment 187678 [details] [review]
Bug 40866: Remove unused $message variable from circulation

The $message variable was declared in circ/circulation.pl but never
actually set, making the "No patron matched" template block unreachable.

This is a remnant from old code that was incompletely cleaned when that
functionality was changed/removed.

This patch removes the unused variable, template parameter, and the
unreachable template block.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 23 Andrew Fuerste-Henry 2025-10-09 15:21:17 UTC
Created attachment 187679 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 1)

This patch removes the duplicate logging code from the original
Bug 9762 implementation that was not properly removed when the
Alternative implementation was merged.

The original implementation added separate logaction() calls in
CanBookBeIssued and circulation.pl that created additional log
entries with a different JSON format. This caused double logging
when the Alternative implementation (which logs via AddIssue) was
merged on top.

Changes in this patch:

C4/Circulation.pm:
- Removed @message_log array and all pushes to it
- Removed the separate logaction() call at the end of CanBookBeIssued
- Removed 5th return value (@message_log) from CanBookBeIssued
- Removed all if ($issueconfirmed) blocks that were populating @message_log

circ/circulation.pl:
- Removed the problematic line that added OVERRIDDEN to every confirmed
  checkout: $needsconfirmation->{'OVERRIDDEN'} = $issueconfirmed

This addresses the double logging issue where circulation overrides
were being logged twice with different formats.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 24 Andrew Fuerste-Henry 2025-10-09 15:21:20 UTC
Created attachment 187680 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 2 - Renewals)

This patch removes the duplicate logging code from renewal scripts
(circ/renew.pl and svc/renew) that was part of the original Bug 9762
implementation.

The original implementation created separate logaction() calls with
custom JSON messages whenever renewal overrides occurred. This caused
double logging alongside the Alternative implementation which properly
logs overrides via the confirmations parameter in AddRenewal.

Changes:

circ/renew.pl:
- Removed unused variables: $user, $branchcode, @message
- Removed separate logaction() calls for override logging
- Changed to use confirmations array with proper codes:
  * 'RENEWAL_LIMIT' for override_limit
  * 'ON_RESERVE' for override_holds
- Removed C4::Log qw(logaction) import

svc/renew:
- Removed unused variables: $borrower, $user, $message, @message
- Removed separate logaction() calls for override logging
- Changed to use confirmations array with proper codes:
  * 'ON_RESERVE' for on_reserve override
  * 'RENEWAL_LIMIT' for too_many override
- Removed C4::Log qw(logaction) import

The Alternative implementation in AddRenewal will now handle all
override logging consistently through the confirmations parameter.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 25 Andrew Fuerste-Henry 2025-10-09 15:21:23 UTC
Created attachment 187681 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 3 - Returns)

This patch removes the duplicate logging code from circ/returns.pl that
was part of the original Bug 9762 implementation.

The original implementation added a separate logaction() call when
ignoring holds on return. This code created a custom JSON log entry
with the message "return an element that is reserved".

However, this logging appears to have been orphaned - there is no
corresponding Alternative implementation for return override logging
in the current codebase. The code was creating log entries but they
were not properly integrated with any override confirmation workflow.

Changes:

circ/returns.pl:
- Removed entire else block that created custom override log entries
- Removed unused variables: $message, @message, $user, $branchcode, $barcode
- Removed C4::Log qw(logaction) import as it's no longer needed

This eliminates the orphaned logging code and ensures returns.pl only
uses standard Koha logging mechanisms.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 26 Andrew Fuerste-Henry 2025-10-09 15:21:27 UTC
Created attachment 187682 [details] [review]
Bug 40866: Implement Alternative pattern for holds override logging (Part 4)

This patch implements proper override logging for holds following the
Alternative implementation pattern used for checkouts and renewals.

Instead of just removing the orphaned logging code from the original
Bug 9762 implementation, this patch properly implements override logging
by updating AddReserve to accept and log override information, similar
to how AddIssue handles confirmations and forced overrides.

Changes:

C4/Reserves.pm:
- Added JSON qw(to_json) import
- Updated AddReserve to accept confirmations and forced parameters
- Enhanced the existing logaction call to include override information
  in JSON format when overrides are present
- Log format matches the Alternative implementation pattern:
  * hold ID
  * branchcode
  * biblionumber
  * itemnumber
  * confirmations array
  * forced array

Koha/REST/V1/Holds.pm:
- Added confirmations array building in the add() method
- Pass 'HOLD_POLICY_OVERRIDE' confirmation when override_all is true
- Pass confirmations parameter to AddReserve call

reserve/request.pl:
- Removed orphaned original implementation logging code
- Removed unused variables and imports (message, @message, user, etc.)
- Removed C4::Log import

koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc:
- Added 'HOLD_POLICY_OVERRIDE' case to translate_log_override block
- Displays as "Hold policy override" in log viewer

koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt:
- Added JSON display handling for HOLDS module (similar to CIRCULATION)
- Displays hold ID, biblionumber, and optional itemnumber with links
- Shows forced and confirmation overrides using translate_log_override

This ensures hold overrides are logged consistently with the Alternative
implementation pattern, providing proper audit trails for policy overrides
that are properly displayed in the log viewer.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 27 Andrew Fuerste-Henry 2025-10-09 15:21:30 UTC
Created attachment 187683 [details] [review]
Bug 40866: Add unit tests for hold override logging

This patch adds unit tests to verify the hold override logging
functionality implemented in the previous patches. The tests cover the
logging of hold overrides in `C4/Reserves.pm` and the `Koha/REST/V1/Holds.pm`
API endpoint.

To test:
1. Run the following tests and confirm they pass:
   - prove -lrv t/db_dependent/Circulation.t
   - prove -lrv t/db_dependent/Reserves.t
   - prove -lrv t/db_dependent/api/v1/holds.t
2. All tests should pass.

Changes:

t/db_dependent/Circulation.t:
- Added tests for hold override scenarios in circulation.

t/db_dependent/Reserves.t:
- Added tests to verify that hold overrides are correctly logged when
  a hold is placed.

t/db_dependent/api/v1/holds.t:
- Added tests to ensure that the holds API correctly logs overrides
  when a hold is created with the `x-koha-override` header.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 28 Andrew Fuerste-Henry 2025-10-09 15:21:33 UTC
Created attachment 187684 [details] [review]
Bug 40866: Add missing override states to action-logs.inc

This follow-up adds missing override state translations that were
discovered during testing. The patchset logs various override states
but some were not covered in the log viewer display template.

Missing states added:
- USERBLOCKEDNOENDDATE: Patron had an indefinite restriction
- CURRENTISSUE: Item is currently checked out to this patron
- UNSEEN: Unseen renewal
- SEEN: Seen renewal

These states are now properly displayed in the action logs viewer
when reviewing circulation override logging.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 29 Andrew Fuerste-Henry 2025-10-09 15:21:36 UTC
Created attachment 187685 [details] [review]
Bug 40866: Remove OVERRIDE state from action-logs.inc

This state is no longer ever set anywhere

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 30 Paul Derscheid 2025-10-09 15:36:38 UTC
Created attachment 187686 [details] [review]
Bug 40866: Fix incorrect display of override message on normal checkouts

- The message 'No patron matched Restriction overridden temporarily' was appearing on every checkout, even when no restriction override occurred.
- This was caused by the message being set unconditionally after AddIssue in circ/circulation.pl.
- The message should not appear in circulation.pl, but in the Log Viewer.

To test:

1. Check out an item to a patron normally.
2. 'Restriction overridden temporarily' message appears.
3. Apply patches
4. Repeat 1.
  ---> Verify that no 'Restriction overridden temporarily' message appears.
5. Check out an item that requires an override (e.g., not for loan item with AllowNotForLoanOverride enabled).
  ---> Verify that no 'Restriction overridden temporarily' message appears.
6. Go to Tool -> Log viewer.
7. Enter the borrowernumber in Object field and submit.
  ---> Verify that the 'Restriction overridden temporarily' message is logged correctly.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 31 Paul Derscheid 2025-10-09 15:36:43 UTC
Created attachment 187687 [details] [review]
Bug 40866: Remove unused $message variable from circulation

The $message variable was declared in circ/circulation.pl but never
actually set, making the "No patron matched" template block unreachable.

This is a remnant from old code that was incompletely cleaned when that
functionality was changed/removed.

This patch removes the unused variable, template parameter, and the
unreachable template block.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 32 Paul Derscheid 2025-10-09 15:36:47 UTC
Created attachment 187688 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 1)

This patch removes the duplicate logging code from the original
Bug 9762 implementation that was not properly removed when the
Alternative implementation was merged.

The original implementation added separate logaction() calls in
CanBookBeIssued and circulation.pl that created additional log
entries with a different JSON format. This caused double logging
when the Alternative implementation (which logs via AddIssue) was
merged on top.

Changes in this patch:

C4/Circulation.pm:
- Removed @message_log array and all pushes to it
- Removed the separate logaction() call at the end of CanBookBeIssued
- Removed 5th return value (@message_log) from CanBookBeIssued
- Removed all if ($issueconfirmed) blocks that were populating @message_log

circ/circulation.pl:
- Removed the problematic line that added OVERRIDDEN to every confirmed
  checkout: $needsconfirmation->{'OVERRIDDEN'} = $issueconfirmed

This addresses the double logging issue where circulation overrides
were being logged twice with different formats.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 33 Paul Derscheid 2025-10-09 15:36:52 UTC
Created attachment 187689 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 2 - Renewals)

This patch removes the duplicate logging code from renewal scripts
(circ/renew.pl and svc/renew) that was part of the original Bug 9762
implementation.

The original implementation created separate logaction() calls with
custom JSON messages whenever renewal overrides occurred. This caused
double logging alongside the Alternative implementation which properly
logs overrides via the confirmations parameter in AddRenewal.

Changes:

circ/renew.pl:
- Removed unused variables: $user, $branchcode, @message
- Removed separate logaction() calls for override logging
- Changed to use confirmations array with proper codes:
  * 'RENEWAL_LIMIT' for override_limit
  * 'ON_RESERVE' for override_holds
- Removed C4::Log qw(logaction) import

svc/renew:
- Removed unused variables: $borrower, $user, $message, @message
- Removed separate logaction() calls for override logging
- Changed to use confirmations array with proper codes:
  * 'ON_RESERVE' for on_reserve override
  * 'RENEWAL_LIMIT' for too_many override
- Removed C4::Log qw(logaction) import

The Alternative implementation in AddRenewal will now handle all
override logging consistently through the confirmations parameter.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 34 Paul Derscheid 2025-10-09 15:36:57 UTC
Created attachment 187690 [details] [review]
Bug 40866: Remove original Bug 9762 implementation remnants (Part 3 - Returns)

This patch removes the duplicate logging code from circ/returns.pl that
was part of the original Bug 9762 implementation.

The original implementation added a separate logaction() call when
ignoring holds on return. This code created a custom JSON log entry
with the message "return an element that is reserved".

However, this logging appears to have been orphaned - there is no
corresponding Alternative implementation for return override logging
in the current codebase. The code was creating log entries but they
were not properly integrated with any override confirmation workflow.

Changes:

circ/returns.pl:
- Removed entire else block that created custom override log entries
- Removed unused variables: $message, @message, $user, $branchcode, $barcode
- Removed C4::Log qw(logaction) import as it's no longer needed

This eliminates the orphaned logging code and ensures returns.pl only
uses standard Koha logging mechanisms.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 35 Paul Derscheid 2025-10-09 15:37:03 UTC
Created attachment 187691 [details] [review]
Bug 40866: Implement Alternative pattern for holds override logging (Part 4)

This patch implements proper override logging for holds following the
Alternative implementation pattern used for checkouts and renewals.

Instead of just removing the orphaned logging code from the original
Bug 9762 implementation, this patch properly implements override logging
by updating AddReserve to accept and log override information, similar
to how AddIssue handles confirmations and forced overrides.

Changes:

C4/Reserves.pm:
- Added JSON qw(to_json) import
- Updated AddReserve to accept confirmations and forced parameters
- Enhanced the existing logaction call to include override information
  in JSON format when overrides are present
- Log format matches the Alternative implementation pattern:
  * hold ID
  * branchcode
  * biblionumber
  * itemnumber
  * confirmations array
  * forced array

Koha/REST/V1/Holds.pm:
- Added confirmations array building in the add() method
- Pass 'HOLD_POLICY_OVERRIDE' confirmation when override_all is true
- Pass confirmations parameter to AddReserve call

reserve/request.pl:
- Removed orphaned original implementation logging code
- Removed unused variables and imports (message, @message, user, etc.)
- Removed C4::Log import

koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc:
- Added 'HOLD_POLICY_OVERRIDE' case to translate_log_override block
- Displays as "Hold policy override" in log viewer

koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt:
- Added JSON display handling for HOLDS module (similar to CIRCULATION)
- Displays hold ID, biblionumber, and optional itemnumber with links
- Shows forced and confirmation overrides using translate_log_override

This ensures hold overrides are logged consistently with the Alternative
implementation pattern, providing proper audit trails for policy overrides
that are properly displayed in the log viewer.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 36 Paul Derscheid 2025-10-09 15:37:10 UTC
Created attachment 187692 [details] [review]
Bug 40866: Add unit tests for hold override logging

This patch adds unit tests to verify the hold override logging
functionality implemented in the previous patches. The tests cover the
logging of hold overrides in `C4/Reserves.pm` and the `Koha/REST/V1/Holds.pm`
API endpoint.

To test:
1. Run the following tests and confirm they pass:
   - prove -lrv t/db_dependent/Circulation.t
   - prove -lrv t/db_dependent/Reserves.t
   - prove -lrv t/db_dependent/api/v1/holds.t
2. All tests should pass.

Changes:

t/db_dependent/Circulation.t:
- Added tests for hold override scenarios in circulation.

t/db_dependent/Reserves.t:
- Added tests to verify that hold overrides are correctly logged when
  a hold is placed.

t/db_dependent/api/v1/holds.t:
- Added tests to ensure that the holds API correctly logs overrides
  when a hold is created with the `x-koha-override` header.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 37 Paul Derscheid 2025-10-09 15:38:07 UTC
Created attachment 187693 [details] [review]
Bug 40866: Add missing override states to action-logs.inc

This follow-up adds missing override state translations that were
discovered during testing. The patchset logs various override states
but some were not covered in the log viewer display template.

Missing states added:
- USERBLOCKEDNOENDDATE: Patron had an indefinite restriction
- CURRENTISSUE: Item is currently checked out to this patron
- UNSEEN: Unseen renewal
- SEEN: Seen renewal

These states are now properly displayed in the action logs viewer
when reviewing circulation override logging.

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 38 Paul Derscheid 2025-10-09 15:38:17 UTC
Created attachment 187694 [details] [review]
Bug 40866: Remove OVERRIDE state from action-logs.inc

This state is no longer ever set anywhere

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 39 Lucas Gass (lukeg) 2025-10-09 21:02:55 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 40 Lucas Gass (lukeg) 2025-10-09 21:21:26 UTC
Created attachment 187707 [details] [review]
Bug 40866: [25.05.x] Remove original implementation of Bug 9762 and implement alternative squashed patchset for 25.05

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 41 Lucas Gass (lukeg) 2025-10-09 22:37:56 UTC
We now have some failing test here in:

t/db_dependent/Circulation/CanBookBeIssued.t

I think this is part of the original implementation and they should be removed?
Comment 42 Martin Renvoize (ashimema) 2025-10-10 10:11:00 UTC
Dang.. could have sworn I'd run that test.  You are completely correct, those tests just need removing.. I'll have a follow-up inbound shortly.
Comment 43 Martin Renvoize (ashimema) 2025-10-10 10:16:45 UTC
Created attachment 187725 [details] [review]
Bug 40866: (follow-up) Remove obsolete CanBookBeIssued.t test file

This test file was specifically testing the message_log return value
from CanBookBeIssued, which was removed as part of the bug 40866
patchset. The circulation message functionality has been reimplemented
elsewhere with proper test coverage, making this test file redundant.
Comment 44 Lucas Gass (lukeg) 2025-10-10 13:18:28 UTC
Follow-up pushed to main
Comment 45 Wainui Witika-Park 2025-10-16 20:48:25 UTC
Can we please get this backported to 25.05 :)
Comment 46 Paul Derscheid 2025-10-29 09:09:57 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 47 Fridolin Somers 2025-10-31 14:02:08 UTC
Depends on Bug 9762 not in 24.11.x