From d5aba6860f67007c888deabbcf1705881573b0c1 Mon Sep 17 00:00:00 2001 From: Hammat Wele Date: Tue, 30 Sep 2025 16:37:45 +0000 Subject: [PATCH] Bug 40866: [25.05.x] Remove original implementation of Bug 9762 and implement alternative squashed patchset for 25.05 Signed-off-by: Martin Renvoize Signed-off-by: Andrew Fuerste Henry Signed-off-by: Paul Derscheid --- C4/Circulation.pm | 76 +-------------- C4/Reserves.pm | 24 ++++- Koha/REST/V1/Holds.pm | 7 ++ circ/circulation.pl | 33 ------- circ/renew.pl | 49 +--------- circ/returns.pl | 27 ------ .../prog/en/includes/action-logs.inc | 10 ++ .../prog/en/modules/circ/circulation.tt | 5 - .../prog/en/modules/tools/viewlog.tt | 22 +++++ reserve/request.pl | 28 ------ svc/renew | 52 +---------- t/db_dependent/Circulation.t | 21 ++++- t/db_dependent/Reserves.t | 92 ++++++++++++++++++- t/db_dependent/api/v1/holds.t | 72 ++++++++++++++- 14 files changed, 248 insertions(+), 270 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 454b1109bc3..1dbc4cf0046 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -825,15 +825,11 @@ sub CanBookBeIssued { my $circ_library = Koha::Libraries->find( _GetCircControlBranch( $item_object, $patron ) ); my $now = dt_from_string(); - my @message_log; $duedate ||= CalcDateDue( $now, $effective_itemtype, $circ_library->branchcode, $patron ); if ( DateTime->compare( $duedate, $now ) == -1 ) { # duedate cannot be before now $needsconfirmation{INVALID_DATE} = $duedate; - if ($issueconfirmed) { - push( @message_log, "sticky due date is invalid or due date in the past" ); - } } my $fees = Koha::Charges::Fees->new( @@ -879,9 +875,6 @@ sub CanBookBeIssued { } if ( $patron->is_debarred ) { $issuingimpossible{DEBARRED} = 1; - if ($issueconfirmed) { - push( @message_log, "borrower is restricted" ); - } } if ( $patron->is_expired ) { @@ -954,16 +947,10 @@ sub CanBookBeIssued { } else { if ( $patron_borrowing_status->{noissuescharge}->{overlimit} && $allowfineoverride ) { $needsconfirmation{DEBT} = $patron_borrowing_status->{noissuescharge}->{charge}; - if ($issueconfirmed) { - push( @message_log, "borrower had amend" ); - } } elsif ( $patron_borrowing_status->{noissuescharge}->{overlimit} && !$allowfineoverride ) { $issuingimpossible{DEBT} = $patron_borrowing_status->{noissuescharge}->{charge}; } elsif ( $patron_borrowing_status->{noissuescharge}->{charge} > 0 && $allfinesneedoverride ) { $needsconfirmation{DEBT} = $patron_borrowing_status->{noissuescharge}->{charge}; - if ($issueconfirmed) { - push( @message_log, "borrower had amend" ); - } } } @@ -1044,9 +1031,6 @@ sub CanBookBeIssued { $needsconfirmation{issued_surname} = $patron->surname; $needsconfirmation{issued_cardnumber} = $patron->cardnumber; $needsconfirmation{issued_borrowernumber} = $patron->borrowernumber; - if ($issueconfirmed) { - push( @message_log, "item is checked out for someone else" ); - } } } } @@ -1074,9 +1058,6 @@ sub CanBookBeIssued { $needsconfirmation{current_loan_count} = $toomany->{count}; $needsconfirmation{max_loans_allowed} = $toomany->{max_allowed}; $needsconfirmation{circulation_rule_TOO_MANY} = $toomany->{circulation_rule}; - if ($issueconfirmed) { - push( @message_log, "too many checkout" ); - } } else { $issuingimpossible{TOO_MANY} = $toomany->{reason}; $issuingimpossible{current_loan_count} = $toomany->{count}; @@ -1109,9 +1090,6 @@ sub CanBookBeIssued { } else { $needsconfirmation{NOT_FOR_LOAN_FORCING} = 1; $needsconfirmation{item_notforloan} = $item_object->notforloan; - if ($issueconfirmed) { - push( @message_log, "item not for loan" ); - } } } else { @@ -1157,9 +1135,6 @@ sub CanBookBeIssued { my $code = $av->count ? $av->next->lib : ''; $needsconfirmation{ITEM_LOST} = $code if ( C4::Context->preference("IssueLostItem") eq 'confirm' ); $alerts{ITEM_LOST} = $code if ( C4::Context->preference("IssueLostItem") eq 'alert' ); - if ($issueconfirmed) { - push( @message_log, "item lost" ); - } } if ( C4::Context->preference("IndependentBranches") ) { my $userenv = C4::Context->userenv; @@ -1260,13 +1235,6 @@ sub CanBookBeIssued { $needsconfirmation{'resbranchcode'} = $res->{branchcode}; $needsconfirmation{'reswaitingdate'} = $res->{'waitingdate'}; $needsconfirmation{'reserve_id'} = $res->{reserve_id}; - - if ($issueconfirmed) { - push( - @message_log, - "item is on reserve and waiting, but has been reserved by some other patron" - ); - } } elsif ( $restype eq "Reserved" ) { # The item is on reserve for someone else. @@ -1278,10 +1246,6 @@ sub CanBookBeIssued { $needsconfirmation{'resbranchcode'} = $res->{branchcode}; $needsconfirmation{'resreservedate'} = $res->{reservedate}; $needsconfirmation{'reserve_id'} = $res->{reserve_id}; - - if ($issueconfirmed) { - push( @message_log, "item is on reserve for someone else" ); - } } elsif ( $restype eq "Transferred" ) { # The item is determined hold being transferred for someone else. @@ -1293,10 +1257,6 @@ sub CanBookBeIssued { $needsconfirmation{'resbranchcode'} = $res->{branchcode}; $needsconfirmation{'resreservedate'} = $res->{reservedate}; $needsconfirmation{'reserve_id'} = $res->{reserve_id}; - - if ($issueconfirmed) { - push( @message_log, "item is determined hold being transferred for someone else" ); - } } elsif ( $restype eq "Processing" ) { # The item is determined hold being processed for someone else. @@ -1308,10 +1268,6 @@ sub CanBookBeIssued { $needsconfirmation{'resbranchcode'} = $res->{branchcode}; $needsconfirmation{'resreservedate'} = $res->{reservedate}; $needsconfirmation{'reserve_id'} = $res->{reserve_id}; - - if ($issueconfirmed) { - push( @message_log, "item is determined hold being processed for someone else" ); - } } } } @@ -1355,9 +1311,6 @@ sub CanBookBeIssued { if ( $restriction_age && $patron->dateofbirth && $restriction_age > $patron->get_age() ) { if ( C4::Context->preference('AgeRestrictionOverride') ) { $needsconfirmation{AGE_RESTRICTION} = "$agerestriction"; - if ($issueconfirmed) { - push( @message_log, "age restriction" ); - } } else { $issuingimpossible{AGE_RESTRICTION} = "$agerestriction"; } @@ -1416,34 +1369,7 @@ sub CanBookBeIssued { } } - my $borrower = $patron; - my $user = C4::Context->userenv->{number}; - my $branchcode = C4::Context->userenv->{branch}; - - # action, cardnumber, barcode, date, heure, user, branche - if ($issueconfirmed) { - - my $infos = ( - { - message => \@message_log, - borrowernumber => $borrower->borrowernumber, - barcode => $barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "CIRCULATION", "ISSUE", - $borrower->{'borrowernumber'}, - $json_infos, - ) if C4::Context->preference("IssueLog"); - } - - return ( \%issuingimpossible, \%needsconfirmation, \%alerts, \%messages, \@message_log ); + return ( \%issuingimpossible, \%needsconfirmation, \%alerts, \%messages ); } =head2 CanBookBeReturned diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 04b43af2507..4ace8d99075 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -22,6 +22,8 @@ package C4::Reserves; use Modern::Perl; +use JSON qw( to_json ); + use C4::Accounts; use C4::Biblio qw( GetMarcFromKohaField ); use C4::Circulation qw( CheckIfIssuedToPatron GetAgeRestriction GetBranchItemRule ); @@ -192,6 +194,8 @@ sub AddReserve { my $itemtype = $params->{itemtype}; my $non_priority = $params->{non_priority}; my $item_group_id = $params->{item_group_id}; + my $confirmations = $params->{confirmations}; + my $forced = $params->{forced}; $resdate ||= dt_from_string; @@ -261,8 +265,24 @@ sub AddReserve { # record patron activity $hold->patron->update_lastseen('hold'); - logaction( 'HOLDS', 'CREATE', $hold->id, $hold ) - if C4::Context->preference('HoldsLog'); + # Log the hold creation + if ( C4::Context->preference('HoldsLog') ) { + my $info = $hold->id; + if ( defined($confirmations) || defined($forced) ) { + $info = to_json( + { + hold => $hold->id, + branchcode => $hold->branchcode, + biblionumber => $hold->biblionumber, + itemnumber => $hold->itemnumber, + confirmations => $confirmations, + forced => $forced + }, + { pretty => 1, canonical => 1 } + ); + } + logaction( 'HOLDS', 'CREATE', $hold->id, $info ); + } my $reserve_id = $hold->id(); diff --git a/Koha/REST/V1/Holds.pm b/Koha/REST/V1/Holds.pm index 53abf9c2cdf..f4f74d655a4 100644 --- a/Koha/REST/V1/Holds.pm +++ b/Koha/REST/V1/Holds.pm @@ -191,6 +191,12 @@ sub add { my $priority = C4::Reserves::CalculatePriority($biblio_id); + # Build confirmations array for logging overrides + my $confirmations = []; + if ($override_all) { + push @{$confirmations}, 'HOLD_POLICY_OVERRIDE'; + } + my $hold_id = C4::Reserves::AddReserve( { branchcode => $pickup_library_id, @@ -206,6 +212,7 @@ sub add { itemtype => $item_type, non_priority => $non_priority, item_group_id => $item_group_id, + confirmations => $confirmations, } ); diff --git a/circ/circulation.pl b/circ/circulation.pl index d85defd0ad1..bc268f499b0 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -39,7 +39,6 @@ use C4::Members; use C4::Biblio qw( TransformMarcToKoha ); use C4::Search qw( new_record_from_zebra ); use C4::Reserves qw( ModReserveAffect ); -use C4::Log qw( logaction ); use Koha::Holds; use C4::Context; use CGI::Session; @@ -326,9 +325,6 @@ if ($patron) { # STEP 3 : ISSUING # # -my $message; -my @message; - if ( @$barcodes && $op eq 'cud-checkout' ) { my $checkout_infos; for my $barcode (@$barcodes) { @@ -564,34 +560,6 @@ if ( @$barcodes && $op eq 'cud-checkout' ) { } ); $template_params->{issue} = $issue; - - my $borrower = $patron; - my $borrowernumber = $patron->borrowernumber; - my $user = C4::Context->userenv->{number}; - my $branchcode = C4::Context->userenv->{branch}; - $message = "Restriction overridden temporarily"; - @message = ("Restriction overridden temporarily"); - - if ($issueconfirmed) { - my $infos = ( - { - message => \@message, - borrowernumber => $borrowernumber, - barcode => $barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "CIRCULATION", "ISSUE", - $borrower->{'borrowernumber'}, - $json_infos, - ) if C4::Context->preference("IssueLog"); - } $session->clear('auto_renew'); $inprocess = 1; } @@ -810,7 +778,6 @@ $template->param( stickyduedate => $stickyduedate, duedatespec => $duedatespec, restoreduedatespec => $restoreduedatespec, - message => $message, totaldue => sprintf( '%.2f', $balance ), # FIXME not used in template? inprocess => $inprocess, $view => 1, diff --git a/circ/renew.pl b/circ/renew.pl index 069312f85b1..fcc9860a9a9 100755 --- a/circ/renew.pl +++ b/circ/renew.pl @@ -24,7 +24,6 @@ use C4::Context; use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_html_with_http_headers ); use C4::Circulation qw( barcodedecode CanBookBeRenewed GetLatestAutoRenewDate AddRenewal ); -use C4::Log qw( logaction ); use Koha::DateUtils qw( dt_from_string ); use Koha::Database; use Koha::BiblioFrameworks; @@ -66,42 +65,19 @@ if ( $op eq 'cud-renew' && $barcode ) { $patron = $checkout->patron; my $borrowernumber = $patron->borrowernumber; - my $user = C4::Context->userenv->{number}; - my $branchcode = C4::Context->userenv->{branch}; - my @message; if ( ( $patron->is_debarred || q{} ) lt dt_from_string()->ymd() ) { my $confirmations; my $can_renew; my $info; ( $can_renew, $error, $info ) = CanBookBeRenewed( $patron, $checkout, $override_limit ); - push @{$confirmations}, $override_limit if $override_limit; + push @{$confirmations}, 'RENEWAL_LIMIT' if $override_limit; if ( $error && ( $error eq 'on_reserve' ) ) { if ($override_holds) { $can_renew = 1; $error = undef; - - @message = ("Override Renew hold for another"); - - my $infos = ( - { - message => \@message, - borrowernumber => $borrowernumber, - barcode => $barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "CIRCULATION", "RENEWAL", - $patron->borrowernumber, - $json_infos, - ) if C4::Context->preference("RenewalLog"); + push @{$confirmations}, 'ON_RESERVE'; } else { $can_renew = 0; } @@ -133,27 +109,6 @@ if ( $op eq 'cud-renew' && $barcode ) { } ); $template->param( date_due => $date_due ); - - @message = ("Override limit Renew"); - - my $infos = ( - { - message => \@message, - borrowernumber => $borrowernumber, - barcode => $barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "CIRCULATION", "RENEWAL", - $patron->borrowernumber, - $json_infos, - ) if C4::Context->preference("RenewalLog"); } } else { $error = "patron_restricted"; diff --git a/circ/returns.pl b/circ/returns.pl index 1721092cf5b..f4871fa9d2e 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -42,7 +42,6 @@ use C4::Members; use C4::Output qw( output_html_with_http_headers ); use C4::Reserves qw( ModReserve ModReserveAffect CheckReserves ); use C4::RotatingCollections; -use C4::Log qw( logaction ); use Koha::AuthorisedValues; use Koha::BiblioFrameworks; use Koha::Calendar; @@ -159,32 +158,6 @@ if ( $query->param('reserve_id') && $op eq 'cud-affect_reserve' ) { $transfer->transit; } } -} else { - my $message = "return an element that is reserved"; - my @message = ("return an element that is reserved"); - my $user = C4::Context->userenv->{number}; - my $branchcode = C4::Context->userenv->{branch}; - my $barcode = $query->param('barcode'); - - my $infos = ( - { - message => \@message, - - #'card number' => $cardnumber, - barcode => $barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "CIRCULATION", "RETURN", - $session, - $json_infos, - ) if C4::Context->preference("ReturnLog"); } if ( $query->param('recall_id') && $op eq 'cud-affect_recall' ) { diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc index 6b0deca6366..262ecd79718 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/action-logs.inc @@ -179,6 +179,8 @@ [% BLOCK translate_log_override %] [% SWITCH override %] + [% CASE 'HOLD_POLICY_OVERRIDE' %] + Hold policy override [% CASE 'INVALID_DATE' %] Invalid date confirmed [% CASE 'DEBT_GUARANTEES' %] @@ -189,6 +191,8 @@ Patron had debt [% CASE 'USERBLOCKEDOVERDUE' %] Patron had overdues + [% CASE 'USERBLOCKEDNOENDDATE' %] + Patron had an indefinite restriction [% CASE 'ADDITIONAL_MATERIALS' %] Additional materials were confirmed [% CASE 'RENEW_ISSUE' %] @@ -247,6 +251,12 @@ Override Renew hold for another [% CASE 'TOO_MUCH' %] Override auto-renew too much owing restriction + [% CASE 'CURRENTISSUE' %] + Item is currently checked out to this patron + [% CASE 'UNSEEN' %] + Unseen renewal + [% CASE 'SEEN' %] + Seen renewal [% CASE %] [% override | html %] [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt index 29aa6532b97..d774abc194c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ -788,11 +788,6 @@

Item checked out

[% END %] - [% IF ( message ) %] - [% INCLUDE 'patron-toolbar.inc' %] -

No patron matched [% message | html %]

- [% END %] - [% IF patron %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt index 44e6e4b20e2..03efca0532b 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt @@ -349,6 +349,28 @@
[% PROCESS translate_log_override override=confirmation %]
[% END %] + [% ELSIF ( loopro.module == 'HOLDS' && loopro.json_found ) %] +
+ + [% FOREACH force IN loopro.json.forced %] +
[% PROCESS translate_log_override override=force %]
+ [% END %] + [% FOREACH confirmation IN loopro.json.confirmations %] +
[% PROCESS translate_log_override override=confirmation %]
+ [% END %] +
[% ELSIF loopro.module == "SYSTEMPREFERENCE" || loopro.module == "REPORTS" || loopro.module == "NEWS" %]
[% loopro.info | trim | html %]
diff --git a/reserve/request.pl b/reserve/request.pl index dd2023c6b51..a553070e4b3 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -40,7 +40,6 @@ use C4::Serials qw( CountSubscriptionFromBiblionumber ); use C4::Circulation qw( _GetCircControlBranch GetBranchItemRule ); use Koha::DateUtils qw( dt_from_string ); use C4::Search qw( enabled_staff_search_views ); -use C4::Log qw( logaction ); use Koha::Biblios; use Koha::Checkouts; @@ -570,33 +569,6 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) $num_alreadyheld++; } - my $borrower = $patron; - my $borrowernumber = $patron->borrowernumber; - my $user = C4::Context->userenv->{number}; - my $branchcode = C4::Context->userenv->{branch}; - my $message = "Override limitation of hold"; - my @message = ("Override limitation of hold"); - my $barcode = $item->{barcode}; - - my $infos = ( - { - message => \@message, - borrowernumber => $borrowernumber, - barcode => $barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "HOLD", "ISSUE", - $borrower->{'borrowernumber'}, - $json_infos, - ) if C4::Context->preference("IssueLog"); - push( @available_itemtypes, $item->{itype} ); } else { diff --git a/svc/renew b/svc/renew index a2877d5f7fa..c3b8547a06a 100755 --- a/svc/renew +++ b/svc/renew @@ -26,7 +26,6 @@ use Try::Tiny; use C4::Circulation qw( AddRenewal CanBookBeRenewed ); use C4::Context; use C4::Auth qw(check_cookie_auth); -use C4::Log qw( logaction ); use Koha::DateUtils qw(output_pref dt_from_string); @@ -54,10 +53,6 @@ my $date_due; if ( $input->param('date_due') ) { $date_due = dt_from_string( scalar $input->param('date_due') ); } -my $borrower = Koha::Patrons->find($borrowernumber); -my $user = C4::Context->userenv->{'number'}; -my $message; -my @message; my $data; $data->{itemnumber} = $itemnumber; @@ -75,56 +70,13 @@ my $confirmations; if ( $data->{error} && $data->{error} eq 'on_reserve' && C4::Context->preference('AllowRenewalOnHoldOverride') ) { $data->{renew_okay} = 1; $data->{error} = undef; - - push @{$confirmations}, 'ON_RESERVE' if $override_limit; - $message = "Override Renew hold for another"; - @message = ("Override Renew hold for another"); - - my $infos = ( - { - message => \@message, - borrowernumber => $borrowernumber, - barcode => $item->barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "CIRCULATION", "RENEWAL", - $borrower->{'borrowernumber'}, - $json_infos, - ) if C4::Context->preference("IssueLog"); + push @{$confirmations}, 'ON_RESERVE'; } if ( $data->{error} && $data->{error} eq 'too_many' && C4::Context->preference('AllowRenewalLimitOverride') ) { $data->{renew_okay} = 1; $data->{error} = undef; - - $message = "Override limit Renew"; - @message = ("Override limit Renew"); - - my $infos = ( - { - message => \@message, - borrowernumber => $borrowernumber, - barcode => $item->barcode, - manager_id => $user, - branchcode => $branchcode, - } - ); - - my $json_infos = JSON->new->utf8->pretty->encode($infos); - $json_infos =~ s/"/'/g; - - logaction( - "CIRCULATION", "RENEWAL", - $borrower->{'borrowernumber'}, - $json_infos, - ) if C4::Context->preference("IssueLog"); + push @{$confirmations}, 'RENEWAL_LIMIT' if $override_limit; } if ( $data->{renew_okay} || ( $seen && $data->{error} eq 'too_unseen' ) ) { diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index aa5821daf0b..ab52b04cb52 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -19,7 +19,7 @@ use Modern::Perl; use utf8; use Test::NoWarnings; -use Test::More tests => 80; +use Test::More tests => 81; use Test::Exception; use Test::MockModule; use Test::Deep qw( cmp_deeply ); @@ -7907,6 +7907,25 @@ subtest 'Bug 9762: AddRenewal override JSON logging' => sub { is_deeply( $log_data->{forced}, ['TOO_MUCH'], 'Forced overrides logged correctly' ); }; +subtest 'Bug 40866: CanBookBeIssued returns correct number of values' => sub { + plan tests => 2; + + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $patron = + $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library->branchcode } } ); + my $item = $builder->build_sample_item( { library => $library->branchcode } ); + + t::lib::Mocks::mock_userenv( { patron => $patron, branchcode => $library->branchcode } ); + + # CanBookBeIssued should return exactly 4 values after Bug 40866 cleanup + # (issuingimpossible, needsconfirmation, alerts, messages) + # The 5th return value (@message_log) from the original Bug 9762 implementation was removed + my @result = C4::Circulation::CanBookBeIssued( $patron, $item->barcode ); + + is( scalar @result, 4, 'CanBookBeIssued returns exactly 4 values' ); + ok( ref( $result[0] ) eq 'HASH', 'First return value is issuingimpossible hashref' ); +}; + $schema->storage->txn_rollback; C4::Context->clear_syspref_cache(); $branches = Koha::Libraries->search(); diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 79d6796f82e..345a485a2fd 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 70; +use Test::More tests => 71; use Test::NoWarnings; use Test::MockModule; use Test::Warn; @@ -25,6 +25,7 @@ use Test::Warn; use t::lib::Mocks; use t::lib::TestBuilder; +use JSON qw( from_json ); use MARC::Record; use DateTime::Duration; @@ -2004,3 +2005,92 @@ subtest 'CheckReserves() item type tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'Bug 40866: AddReserve override JSON logging' => sub { + plan tests => 8; + + $schema->storage->txn_begin; + + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { branchcode => $library->branchcode } + } + ); + my $biblio = $builder->build_sample_biblio(); + my $item = $builder->build_sample_item( + { + library => $library->branchcode, + biblionumber => $biblio->biblionumber + } + ); + + t::lib::Mocks::mock_userenv( { patron => $patron, branchcode => $library->branchcode } ); + t::lib::Mocks::mock_preference( 'HoldsLog', 1 ); + + # Clear any existing logs + Koha::ActionLogs->search( { module => 'HOLDS', action => 'CREATE' } )->delete; + + # Test 1: Normal hold without overrides - should log hold ID only + my $hold_id = C4::Reserves::AddReserve( + { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $biblio->biblionumber, + priority => 1, + } + ); + ok( $hold_id, 'Hold created successfully' ); + + my $logs = Koha::ActionLogs->search( + { + module => 'HOLDS', + action => 'CREATE', + object => $hold_id + }, + { order_by => { -desc => 'timestamp' } } + ); + is( $logs->count, 1, 'One log entry created for normal hold' ); + my $log = $logs->next; + is( $log->info, $hold_id, 'Normal hold logs hold ID only' ); + + # Cancel the hold for next test + my $hold = Koha::Holds->find($hold_id); + $hold->cancel; + + # Test 2: Hold with override confirmation - should log JSON with confirmations + my $hold_id2 = C4::Reserves::AddReserve( + { + branchcode => $library->branchcode, + borrowernumber => $patron->borrowernumber, + biblionumber => $biblio->biblionumber, + priority => 1, + confirmations => ['HOLD_POLICY_OVERRIDE'], + forced => [] + } + ); + ok( $hold_id2, 'Hold with override created successfully' ); + + $logs = Koha::ActionLogs->search( + { + module => 'HOLDS', + action => 'CREATE', + object => $hold_id2 + }, + { order_by => { -desc => 'timestamp' } } + ); + is( $logs->count, 1, 'One log entry created for override hold' ); + $log = $logs->next; + + my $log_data = eval { from_json( $log->info ) }; + ok( !$@, 'Log info is valid JSON' ); + ok( exists $log_data->{confirmations}, 'JSON contains confirmations array' ); + is_deeply( + $log_data->{confirmations}, + ['HOLD_POLICY_OVERRIDE'], + 'Confirmations logged correctly' + ); + + $schema->storage->txn_rollback; +}; diff --git a/t/db_dependent/api/v1/holds.t b/t/db_dependent/api/v1/holds.t index e98d8b125e0..f0a55194ef7 100755 --- a/t/db_dependent/api/v1/holds.t +++ b/t/db_dependent/api/v1/holds.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 18; +use Test::More tests => 19; use Test::NoWarnings; use Test::MockModule; use Test::Mojo; @@ -25,6 +25,7 @@ use t::lib::TestBuilder; use t::lib::Mocks; use DateTime; +use JSON qw( from_json ); use Mojo::JSON qw(encode_json); use C4::Context; @@ -32,6 +33,7 @@ use Koha::Patrons; use C4::Reserves qw( AddReserve CanItemBeReserved CanBookBeReserved ); use C4::Items; +use Koha::ActionLogs; use Koha::Database; use Koha::DateUtils qw( dt_from_string output_pref ); use Koha::Biblios; @@ -1885,3 +1887,71 @@ subtest 'PUT /holds/{hold_id}/lowest_priority tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'Bug 40866: API hold creation with override logs confirmations' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + + my $librarian = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 1 } + } + ); + my $password = 'thePassword123'; + $librarian->set_password( { password => $password, skip_validation => 1 } ); + my $userid = $librarian->userid; + + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 0 } + } + ); + + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $biblio = $builder->build_sample_biblio; + + t::lib::Mocks::mock_preference( 'AllowHoldPolicyOverride', 1 ); + t::lib::Mocks::mock_preference( 'HoldsLog', 1 ); + + # Clear any existing logs + Koha::ActionLogs->search( { module => 'HOLDS', action => 'CREATE' } )->delete; + + my $post_data = { + patron_id => $patron->id, + biblio_id => $biblio->id, + pickup_library_id => $library->branchcode, + }; + + # Create hold with override + my $hold_id = + $t->post_ok( "//$userid:$password@/api/v1/holds" => { 'x-koha-override' => 'any' } => json => $post_data ) + ->status_is(201)->tx->res->json->{hold_id}; + + ok( $hold_id, 'Hold created via API with override' ); + + # Verify the hold was logged with override information + my $logs = Koha::ActionLogs->search( + { + module => 'HOLDS', + action => 'CREATE', + object => $hold_id + }, + { order_by => { -desc => 'timestamp' } } + ); + + is( $logs->count, 1, 'One log entry created for API hold with override' ); + + my $log = $logs->next; + my $log_data = eval { from_json( $log->info ) }; + ok( !$@, 'Log info is valid JSON' ); + is_deeply( + $log_data->{confirmations}, + ['HOLD_POLICY_OVERRIDE'], + 'HOLD_POLICY_OVERRIDE logged in confirmations array' + ); + + $schema->storage->txn_rollback; +}; -- 2.39.5