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
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
Created attachment 186986 [details] [review] Bug 40866: Make message translatable
Note sure whether that is the best place for that message given that we have to introduce Koha::I18N?
I would like to upload alternate patchs that moves or removes the other variables that were introduced by bug 9762, if that is ok.
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 :)
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 :)
Created attachment 187063 [details] [review] Bug 40866: Move or remove other variables added with the message
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.
The message 'Restriction overridden temporarily' is not meant to appear in circulation.pl, but in the Log Viewer (see Bug 9762).
We don't need to logaction here? Why is that being deleted?
FQA as per last comment.
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>
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>
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.
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.
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.
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.
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.
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.
Created attachment 187676 [details] [review] Bug 40866: Remove OVERRIDE state from action-logs.inc This state is no longer ever set anywhere
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Nice work everyone! Pushed to main for 25.11
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>
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?
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.
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.
Follow-up pushed to main
Can we please get this backported to 25.05 :)
Nice work everyone! Pushed to 25.05.x
Depends on Bug 9762 not in 24.11.x