From e77abe07ca7ff717397d321fcacb8dd21729f1ce Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 28 Aug 2020 10:01:04 -0400 Subject: [PATCH] Bug 26282: Allow staff to decide if a hold cancellation notice will be sent when cancelling a hold Bug 25534 adds the option to send hold cancellation notices when a reason was given. It would be nice if you could give a reason but still decide not to send to the patron. Say the patron called and will not be able to pick up numerous holds for some time. You cancel them all with "patron requested cancellation" but don't want to send multiple notices. I could imagine a checkbox next to the reason to activate/deactivate sending the notice. Test Plan: 1) Apply this patch 2) Visit each area in Koha where a hold can be canceled with a reason 3) Note the new 'Notify patron' checkbox 4) Test canceling a hold with and without this checkbox checked 5) Verify leaving it unchecked does not trigger a notice to be sent to the patron Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi Signed-off-by: Josef Moravec Rebased-by: Victor Grousset/tuxayo --- C4/Reserves.pm | 5 +- Koha/Hold.pm | 3 +- circ/pendingreserves.pl | 24 +++-- .../mysql/en/mandatory/sample_notices.yml | 14 +++ .../prog/en/includes/holds_table.inc | 8 ++ .../prog/en/includes/patron-detail-tabs.inc | 3 + .../prog/en/modules/circ/circulation.tt | 2 +- .../prog/en/modules/circ/pendingreserves.tt | 9 ++ .../prog/en/modules/reserve/request.tt | 26 ++++-- misc/cronjobs/holds/cancel_expired_holds.pl | 16 +++- reserve/modrequest.pl | 3 + reserve/request.pl | 13 ++- t/db_dependent/Koha/Hold.t | 91 ++++++++++++++++++- 13 files changed, 190 insertions(+), 27 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 729d5d0eae7..cdf4bb6f0ba 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -933,6 +933,7 @@ Cancels all reserves with an expiration date from before today. sub CancelExpiredReserves { my $cancellation_reason = shift; + my $notify = shift; my $today = dt_from_string(); my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); my $expireWaiting = C4::Context->preference('ExpireReservesMaxPickUpDelay'); @@ -957,6 +958,7 @@ sub CancelExpiredReserves { my $cancel_params = {}; $cancel_params->{cancellation_reason} = $cancellation_reason if defined($cancellation_reason); + $cancel_params->{notify_patron} = $notify if defined($notify); if ( defined($hold->found) && $hold->found eq 'W' ) { $cancel_params->{charge_cancel_fee} = 1; } @@ -1032,6 +1034,7 @@ sub ModReserve { my $biblionumber = $params->{'biblionumber'}; my $cancellation_reason = $params->{'cancellation_reason'}; my $date = $params->{expirationdate}; + my $notify_patron = $params->{'notify_patron'}; return if defined $rank && $rank eq "n"; @@ -1053,7 +1056,7 @@ sub ModReserve { my $original = C4::Context->preference('HoldsLog') ? $hold->unblessed : undef; if ( $rank eq "del" ) { - $hold->cancel({ cancellation_reason => $cancellation_reason }); + $hold->cancel({ cancellation_reason => $cancellation_reason, notify_patron => $notify_patron }); } elsif ($hold->found && $hold->priority eq '0' && $date) { diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 1faec873ec2..7a7e0457887 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -695,6 +695,7 @@ my $cancel_hold = $hold->cancel( [ charge_cancel_fee => 1||0, ] [ cancellation_reason => $cancellation_reason, ] [ skip_holds_queue => 1||0 ] + [ notify_patron => 1||0, ] } ); @@ -738,7 +739,7 @@ sub cancel { }, undef, $self->id ); - if ( $params->{cancellation_reason} ) { + if ( $params->{notify_patron} ) { my $letter = C4::Letters::GetPreparedLetter( module => 'reserves', letter_code => 'HOLD_CANCELLATION', diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl index 366d0009af7..c3fdc404274 100755 --- a/circ/pendingreserves.pl +++ b/circ/pendingreserves.pl @@ -55,7 +55,8 @@ if ( $op eq 'cud-cancel_reserve' and $reserve_id ) { my $hold = Koha::Holds->find( $reserve_id ); if ( $hold ) { my $cancellation_reason = $input->param('cancellation-reason'); - $hold->cancel({ cancellation_reason => $cancellation_reason }); + my $cancellation_notify_patron = $input->param('cancellation-notify-patron'); + $hold->cancel({ cancellation_reason => $cancellation_reason, notify_patron => $cancellation_notify_patron }); push @messages, { type => 'message', code => 'hold_cancelled' }; } } elsif ( $op =~ m|^mark_as_lost| ) { @@ -299,14 +300,21 @@ foreach my $bibnum ( @biblionumbers ){ push @holds_info, $hold_info; } +my $cancel_notify_templates = Koha::Notice::Templates->search({ module => 'reserves', code => 'HOLD_CANCELLATION' }); +my $cancel_notice_branches; +while ( my $notice = $cancel_notify_templates->next ) { + $cancel_notice_branches->{$notice->branchcode} = 1; +} + $template->param( - todaysdate => $today, - from => $startdate, - to => $enddate, - holds_info => \@holds_info, - HoldsToPullStartDate => C4::Context->preference('HoldsToPullStartDate') || PULL_INTERVAL, - HoldsToPullEndDate => C4::Context->preference('ConfirmFutureHolds') || 0, - messages => \@messages, + cancel_notice_branches => $cancel_notice_branches, + todaysdate => $today, + from => $startdate, + to => $enddate, + holds_info => \@holds_info, + HoldsToPullStartDate => C4::Context->preference('HoldsToPullStartDate') || PULL_INTERVAL, + messages => \@messages, + HoldsToPullEndDate => C4::Context->preference('ConfirmFutureHolds') || 0, ); output_html_with_http_headers $input, $cookie, $template->output; diff --git a/installer/data/mysql/en/mandatory/sample_notices.yml b/installer/data/mysql/en/mandatory/sample_notices.yml index ec0e2386dab..c10bad0104e 100644 --- a/installer/data/mysql/en/mandatory/sample_notices.yml +++ b/installer/data/mysql/en/mandatory/sample_notices.yml @@ -2535,3 +2535,17 @@ tables: - "Your hold on [% hold.biblio.title %] ([% hold.biblio.id %]) has been confirmed." - "You will be notified by the library when your item is available for pickup." - "Thank you!" + + - module: reserves + code: HOLD_CANCELLATION + branchcode: "" + name: "Notification of hold cancellation" + is_html: 0 + title: "Your hold was cancelled" + message_transport_type: email + lang: default + content: + - "[%- USE AuthorisedValues -%]" + - "Dear [% borrower.firstname %] [% borrower.surname %]," + - "" + - "Your hold for [% biblio.title %] was cancelled [% IF hold.cancellation_reason %] for the following reason: [% AuthorisedValues.GetByCode( 'HOLD_CANCELLATION', hold.cancellation_reason ) %][% END %]" diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc index 26edf3bf79c..b15a94504ff 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc @@ -93,6 +93,14 @@ [% hold.priority | html %] [% END %] + + [% IF cancel_notice_branches.size %] + [% SET bc = hold.patron.branchcode %] + [% SET all_bc = "" %] + [% UNLESS cancel_notice_branches.$bc || cancel_notice_branches.$all_bc %] + Cancellation will not receive a notice. + [% END %] + [% END %] [%- IF ( CAN_user_reserveforothers_modify_holds_priority ) -%] [%- UNLESS hold.found -%] diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/patron-detail-tabs.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/patron-detail-tabs.inc index bf6de17daf8..a2392601131 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/patron-detail-tabs.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/patron-detail-tabs.inc @@ -175,6 +175,9 @@ [% END %] + + + [% 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 c31e66bd869..4951fda1e16 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ -916,7 +916,7 @@ - [% INCLUDE 'patron-detail-tabs.inc' patronpage = "circ" %] + [% INCLUDE 'patron-detail-tabs.inc' patronpage = "circ" %] [% ELSIF borrowernumber # IF patron %]
Patron not found. Return to search
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt index 701aab6dca5..aff4c8be87c 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt @@ -220,6 +220,15 @@ [% END %] + + [% IF cancel_notice_branches.size %] + [% SET bc = hold.patron.branchcode %] + [% SET all_bc = "" %] + [% IF cancel_notice_branches.$bc || cancel_notice_branches.$all_bc %] + + + [% END %] + [% END %] [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt index 39ed6cd9909..88e9496f7ad 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt @@ -1256,7 +1256,7 @@ @@ -1304,7 +1307,7 @@

[% Branches.GetName( b ) | html %]

- [% INCLUDE holds_table.inc holds=holds_by_branch %] + [% INCLUDE holds_table.inc holds=holds_by_branch cancel_notice_branches=cancel_notice_branches %]
[% END # /FOREACh b %] [% END # /IF ( branchcodes.empty ) %] @@ -1450,7 +1453,7 @@
[% SET hold_cancellation = AuthorisedValues.GetAuthValueDropbox('HOLD_CANCELLATION') %] - [% IF hold_cancellation.count %] + [% IF hold_cancellation.count && cancel_notice_branches.size %] + + + [% END %]
@@ -1531,12 +1537,14 @@ $.fn.select2.defaults.set("dropdownAutoWidth", true ); $(document).ready(function() { - $('#cancellation-reason-fieldset').hide(); + $('#cancellation-reason-fieldset, .no-notice-warning').hide(); $('.rank-request').on('change', function() { if ( $(".rank-request option:selected[value='del']").length ) { $('#cancellation-reason-fieldset').show(); + $('.no-notice-warning').show(); } else { $('#cancellation-reason-fieldset').hide(); + $('.no-notice-warning').hide(); } }); @@ -1852,11 +1860,11 @@ e.preventDefault; cancel_link = $(this); $("#cancel_modal_form #inputs").empty(); - let reserve_id = cancel_link.data('id'); - let biblionumber = cancel_link.data('biblionumber'); - $("#cancel_modal_form #inputs").append(''); - $("#cancel_modal_form #inputs").append(''); - $("#cancel_modal_form #inputs").append(''); + let reserve_id = cancel_link.data('id'); + let biblionumber = cancel_link.data('biblionumber'); + $("#cancel_modal_form #inputs").append(''); + $("#cancel_modal_form #inputs").append(''); + $("#cancel_modal_form #inputs").append(''); $('#cancelModal').modal('show'); return false; }); diff --git a/misc/cronjobs/holds/cancel_expired_holds.pl b/misc/cronjobs/holds/cancel_expired_holds.pl index ed977678b70..81921dab910 100755 --- a/misc/cronjobs/holds/cancel_expired_holds.pl +++ b/misc/cronjobs/holds/cancel_expired_holds.pl @@ -25,12 +25,12 @@ cancel_expired_holds.pl - cron script to cancel holds as they expire =head1 SYNOPSIS ./cancel_expired_holds.pl - ./cancel_expired_holds.pl --reason="EXPIRED" + ./cancel_expired_holds.pl --reason="EXPIRED" --notify or, in crontab: 0 1 * * * cancel_expired_holds.pl - 0 1 * * * cancel_expired_holds.pl --reason="EXPIRED" + 0 1 * * * cancel_expired_holds.pl --reason="EXPIRED" --notify =head1 DESCRIPTION @@ -56,25 +56,31 @@ Print a brief help message and exits. =item B<--reason> -Optionally adds a reason for cancellation (which will trigger a notice to be sent to the patron) +Optionally adds a reason for cancellation + +=item B<--notify> + +Optionally trigger a notice to be sent to the patron =back =cut my $help = 0; +my $notify = 0; my $reason; my $command_line_options = join(" ",@ARGV); GetOptions( 'help|?' => \$help, - 'reason=s' => \$reason + 'notify' => \$notify, + 'reason=s' => \$reason, ) or pod2usage(1); pod2usage(1) if $help; cronlogaction({ info => $command_line_options }); -C4::Reserves::CancelExpiredReserves($reason); +C4::Reserves::CancelExpiredReserves($reason, $notify); cronlogaction({ action => 'End', info => "COMPLETED" }); diff --git a/reserve/modrequest.pl b/reserve/modrequest.pl index fbf960a5541..648fae89a46 100755 --- a/reserve/modrequest.pl +++ b/reserve/modrequest.pl @@ -56,6 +56,8 @@ if( $op eq 'cud-cancelall' || $op eq 'cud-modifyall' ) { undef $itemnumber[$i] if !$itemnumber[$i]; my $suspend_until = $query->param( "suspend_until_" . $reserve_id[$i] ); my $cancellation_reason = $query->param("cancellation-reason"); + my $cancellation_notify_patron = $query->param("cancellation-notify-patron"); + my $params = { rank => $rank[$i], reserve_id => $reserve_id[$i], @@ -64,6 +66,7 @@ if( $op eq 'cud-cancelall' || $op eq 'cud-modifyall' ) { itemnumber => $itemnumber[$i], defined $suspend_until ? ( suspend_until => $suspend_until ) : (), cancellation_reason => $cancellation_reason, + notify_patron => $cancellation_notify_patron, }; if (C4::Context->preference('AllowHoldDateInFuture')) { $params->{reservedate} = $reservedates[$i] || undef; diff --git a/reserve/request.pl b/reserve/request.pl index 5d0887764d0..d9c9c6ec324 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -113,8 +113,9 @@ if ( $op eq 'cud-move' ) { elsif ( $op eq 'cud-cancel' ) { my $reserve_id = $input->param('reserve_id'); my $cancellation_reason = $input->param("cancellation-reason"); + my $cancellation_notify_patron = $input->param("cancellation-notify-patron"); my $hold = Koha::Holds->find($reserve_id); - $hold->cancel( { cancellation_reason => $cancellation_reason } ) if $hold; + $hold->cancel( { cancellation_reason => $cancellation_reason, notify_patron => $cancellation_notify_patron } ) if $hold; } elsif ( $op eq 'cud-setLowestPriority' ) { my $reserve_id = $input->param('reserve_id'); @@ -725,6 +726,16 @@ $template->param(borrowernumber => $borrowernumber_hold); $template->param( failed_holds => \@failed_holds ); +# Show hold cancellation reason only if notice exists +my $cancel_notify_templates = Koha::Notice::Templates->search({ module => 'reserves', code => 'HOLD_CANCELLATION' }); +my $cancel_notice_branches; +while ( my $notice = $cancel_notify_templates->next ) { + $cancel_notice_branches->{$notice->branchcode} = 1; +} +$template->param( + cancel_notice_branches => $cancel_notice_branches, +); + # printout the page output_html_with_http_headers $input, $cookie, $template->output; diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index 99dae133a24..44988e9ff85 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 14; +use Test::More tests => 15; use Test::Exception; use Test::MockModule; @@ -1089,3 +1089,92 @@ subtest 'change_type() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'cancel() tests' => sub { + + plan tests => 11; + + $schema->storage->txn_begin; + + my $get_prepared_letter_called; + + # Mock GetPreparedLetter so it raises a warning we can look for + # and returns undef, so no call to EnqueueLetter happens + my $mocked_letter = Test::MockModule->new("C4::Letters"); + $mocked_letter->mock( 'GetPreparedLetter', sub { + $get_prepared_letter_called = 1; + return; + }); + + my $hold = $builder->build_object( + { + class => 'Koha::Holds', + value => { + cancellationdate => undef, + priority => 1, + cancellation_reason => undef, + } + } + ); + + # leave this things out of the test + t::lib::Mocks::mock_preference( 'ExpireReservesMaxPickUpDelayCharge', 0 ); + t::lib::Mocks::mock_preference( 'HoldsLog', 0 ); + + $hold = $builder->build_object( + { + class => 'Koha::Holds', + value => { + cancellationdate => undef, + priority => 1, + cancellation_reason => undef, + } + } + ); + + $get_prepared_letter_called = 0; + $hold->cancel({ cancellation_reason => 'Some reason' }); + ok( !$get_prepared_letter_called, 'GetPreparedLetter not called' ); + + isnt( $hold->cancellationdate, undef, 'cancellationdate gets set to the passed value' ); + is( $hold->priority, 0, 'priority gets set to 0' ); + is( $hold->cancellation_reason, 'Some reason', 'cancellation_reason is set to the passed value' ); + + $hold = $builder->build_object( + { + class => 'Koha::Holds', + value => { + cancellationdate => undef, + priority => 1, + cancellation_reason => undef, + } + } + ); + + $get_prepared_letter_called = 0; + $hold->cancel({ cancellation_reason => 'Some reason', notify_patron => 1 }); + ok( $get_prepared_letter_called, 'GetPreparedLetter called if notify_patron and cancellation_reason passed' ); + + isnt( $hold->cancellationdate, undef, 'cancellationdate gets set to the passed value' ); + is( $hold->priority, 0, 'priority gets set to 0' ); + is( $hold->cancellation_reason, 'Some reason', 'cancellation_reason is set to the passed value' ); + + $hold = $builder->build_object( + { + class => 'Koha::Holds', + value => { + cancellationdate => undef, + priority => 1, + cancellation_reason => undef, + } + } + ); + + $get_prepared_letter_called = 0; + $hold->cancel({ notify_patron => 1 }); + isnt( $hold->cancellationdate, undef, 'cancellationdate gets set to the passed value' ); + is( $hold->priority, 0, 'priority gets set to 0' ); + is( $hold->cancellation_reason, undef, 'cancellation_reason not passed' ); + + $schema->storage->txn_rollback; +}; -- 2.39.3 (Apple Git-146)