From 5323bdc6cd5bfb32fe581beaae656d9e6206a166 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 20 Apr 2021 15:14:41 +0100 Subject: [PATCH] Bug 25711: (QA follow-up) Refactor to clarify logic At the point where 'ExpireReservesMaxPickUpDelayCharge' can/should kick in, we should already have an item waiting (else what's triggered a pickup delay). As such we can drop the fall through default of using the syspref when we don't yet have an item. We can also save us some calculations and database hits by only calculating the charge value when we've actually been asked to charge. So we also move the block inside the 'charge_cancel_fee' block. Signed-off-by: Martin Renvoize Signed-off-by: Petro Vashchuk Signed-off-by: Martin Renvoize --- Koha/Hold.pm | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 84c8bc32b71..1602c2f9ec7 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -785,8 +785,9 @@ sub cancel { C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber }); # and, if desired, charge a cancel fee - my $charge; - if (my $item = $self->item) { + if ( $params->{'charge_cancel_fee'} ) { + my $charge; + my $item = $self->item; my $branchcode = C4::Reserves::GetReservesControlBranch($item->unblessed, $self->borrower->unblessed); my $rule = Koha::CirculationRules->get_effective_rule( @@ -799,11 +800,7 @@ sub cancel { ); my $rule_value = $rule && $rule->rule_value // ''; $charge = $rule_value ne '' ? $rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); - } else { - $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); - } - if ( $charge && $params->{'charge_cancel_fee'} ) { my $account = Koha::Account->new( { patron_id => $self->borrowernumber } ); $account->add_debit( @@ -815,7 +812,7 @@ sub cancel { type => 'RESERVE_EXPIRED', item_id => $self->itemnumber } - ); + ) if $charge; } C4::Log::logaction( 'HOLDS', 'CANCEL', $self->reserve_id, $self, undef, $original ) -- 2.47.1