From 4d58c0feea93d313b59df9b3017a4bfa0a7943ee Mon Sep 17 00:00:00 2001 From: Baptiste Wojtkowski Date: Wed, 7 Aug 2024 14:18:20 +0200 Subject: [PATCH] Bug 14250: Patron with fines cannot be discharged anymore Content-Type: text/plain; charset=utf-8 TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Bug 14250: (QA follow-up) Deduplicate checks Signed-off-by: Kyle M Hall Bug 14250: (follow-up) Update messages Update the text for the reasons the patron cannot be discharged. Bug 14250: (follow-up) Refacto the algorithm for discharge messages TEST PLAN: 1 - In the intranet add fines to a client and ensure they have no checkout 2 - Set syspref useDischarge to 'Allow' 3 - On client profile click on "Discharge", see it is possible 4 - On the opac, go to the tab "Ask for discharge", see it is possible 5 - Apply patch 6 - Click on the button "Ask for discharge" -> it will not be possible and you will be propperly messaged 7 - Return to the tab "Ask for a discharge", there will be no button anymore 8 - On the intranet page, there will be no button anymore 9 - Check out a book 10 - Repeat 7 and 8 and see the message include issue with checkout 11 - Remove fine and repeat 7 & 8 Note: removing the useless has_checkout and has_debt Bug 14250: (follow-up) Refacto discharge messages display The script that determines if one should display messages to the user, informing him whether they are allowed or not to ask for a discharge is quite hard to understand. For sake of readability, transforming the unless to an if since the error behaviour was in the unless and the success in the else. Changing to success behaviour in the if and failure in the else. Bug 14250: Tidy --- Koha/Patron/Discharge.pm | 25 ++++++++-- .../prog/en/modules/members/discharge.tt | 18 ++++++- .../bootstrap/en/modules/opac-discharge.tt | 23 +++++++-- members/discharge.pl | 4 +- opac/opac-discharge.pl | 15 ++++-- t/db_dependent/Patron/Borrower_Discharge.t | 49 ++++++++++++++++--- 6 files changed, 109 insertions(+), 25 deletions(-) diff --git a/Koha/Patron/Discharge.pm b/Koha/Patron/Discharge.pm index 6f702061cc..7da95ab7c3 100644 --- a/Koha/Patron/Discharge.pm +++ b/Koha/Patron/Discharge.pm @@ -38,8 +38,22 @@ sub can_be_discharged { my $patron = Koha::Patrons->find( $params->{borrowernumber} ); return unless $patron; - my $has_pending_checkouts = $patron->checkouts->count; - return $has_pending_checkouts ? 0 : 1; + my $can_be_discharged = 1; + my $problems = {}; + + my $checkouts = $patron->checkouts->count; + if ($checkouts) { + $can_be_discharged = 0; + $problems->{checkouts} = $checkouts; + } + + my $debt = $patron->account->outstanding_debits->total_outstanding; + if ( $debt > 0 ) { + $can_be_discharged = 0; + $problems->{debt} = $debt; + } + + return ( $can_be_discharged, $problems ); } sub is_discharged { @@ -61,7 +75,8 @@ sub request { my ($params) = @_; my $borrowernumber = $params->{borrowernumber}; return unless $borrowernumber; - return unless can_be_discharged( { borrowernumber => $borrowernumber } ); + my ($can) = can_be_discharged( { borrowernumber => $borrowernumber } ); + return unless $can; my $rs = Koha::Database->new->schema->resultset('Discharge'); return $rs->create( @@ -75,7 +90,9 @@ sub request { sub discharge { my ($params) = @_; my $borrowernumber = $params->{borrowernumber}; - return unless $borrowernumber and can_be_discharged( { borrowernumber => $borrowernumber } ); + + my ($can) = can_be_discharged( { borrowernumber => $borrowernumber } ); + return unless $borrowernumber and $can; # Cancel reserves my $patron = Koha::Patrons->find($borrowernumber); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt index 9ebb53d753..88f85eadcb 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt @@ -2,6 +2,7 @@ [% USE Koha %] [% USE Asset %] [% USE KohaDates %] +[% USE Price %] [% USE AuthorisedValues %] [% USE Branches %] [% PROCESS 'i18n.inc' %] @@ -52,10 +53,23 @@ [% END %] [% UNLESS can_be_discharged %] -

Cannot edit discharge: the patron has checked out items.

+

A discharge cannot be generated for this patron because:

+ [% ELSE %] [% IF patron.holds.count %] -

Patron has holds. They will be cancelled if the discharge is generated.

+

The patron has holds. The holds will be cancelled if the discharge is generated.

+ [% END %] + + [% IF discharge_problems.debt %] +
  • The patron has unpaid charges of [% discharge_problems.debt | $Price %]
  • [% END %]
    diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt index 5114ca7a9c..1c1bba80a0 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-discharge.tt @@ -1,5 +1,6 @@ [% USE raw %] [% USE Koha %] +[% USE Price %] [% USE AdditionalContents %] [% SET OpacNav = AdditionalContents.get( location => "OpacNav", lang => lang, library => logged_in_user.branchcode || default_branch, blocktitle => 0 ) %] [% SET OpacNavBottom = AdditionalContents.get( location => "OpacNavBottom", lang => lang, library => logged_in_user.branchcode || default_branch, blocktitle => 0 ) %] @@ -41,8 +42,6 @@

    Get your discharge

    [% ELSIF pending %]

    Your discharge will be available on this page within a few days.

    - [% ELSIF has_issues %] -

    You cannot be discharged, you have checked out items. Please return items before asking for a discharge.

    [% ELSIF not messages %]

    What is a discharge?

    Warning: This request is only valid if you are in good standing with the library. Once the application is made, you can not borrow library materials.

    - [% IF has_checkouts %] -
    You cannot be discharged, you have checked out items. Please return items before asking for a discharge.
    - [% ELSE %] + [% IF can_be_discharged %] [% INCLUDE 'csrf-token.inc' %]
    + [% ELSE %] + [% IF failure %] +

    There was an error during the discharge process

    + [% END %] +
    You cannot be discharged because: +
      + [% IF discharge_problems.checkouts %] +
    • You have [% discharge_problems.checkouts | html %] item(s) checked out. Please return your checked out items before reapplying.
    • + [% END %] + + [% IF discharge_problems.debt %] +
    • You have unpaid charges of [% discharge_problems.debt | $Price %]. Please pay your charges before reapplying.
    • + [% END %] +
    +
    [% END %] [% END %]
    diff --git a/members/discharge.pl b/members/discharge.pl index 7798794a1c..b4dda3a13b 100755 --- a/members/discharge.pl +++ b/members/discharge.pl @@ -65,7 +65,8 @@ output_and_exit_if_error( { module => 'members', logged_in_user => $logged_in_user, current_patron => $patron } ); -my $can_be_discharged = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $borrowernumber } ); +my ( $can_be_discharged, $discharge_problems ) = + Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $borrowernumber } ); # Generating discharge if needed if ( $op eq 'cud-discharge' && $input->param('discharge') and $can_be_discharged ) { @@ -112,6 +113,7 @@ my @validated_discharges = Koha::Patron::Discharge::get_validated( $template->param( patron => $patron, can_be_discharged => $can_be_discharged, + discharge_problems => $discharge_problems, validated_discharges => \@validated_discharges, ); diff --git a/opac/opac-discharge.pl b/opac/opac-discharge.pl index 04325fa930..cc79467382 100755 --- a/opac/opac-discharge.pl +++ b/opac/opac-discharge.pl @@ -45,10 +45,15 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( } ); -my $can_be_discharged = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $loggedinuser } ); -if ( $can_be_discharged == 0 ) { - $template->param( has_checkouts => 1 ); -} +my ( $can_be_discharged, $discharge_problems ) = + Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $loggedinuser } ); +my $patron = Koha::Patrons->find($loggedinuser); +my $has_pending_checkouts = $patron->checkouts->count; + +$template->param( + can_be_discharged => $can_be_discharged, + discharge_problems => $discharge_problems, +); my $pending = Koha::Patron::Discharge::count( { @@ -73,7 +78,7 @@ if ( $op eq 'cud-request' ) { if ($success) { $template->param( success => 1 ); } else { - $template->param( has_issues => 1 ); + $template->param( failure => 1 ); } } elsif ( $op eq 'get' ) { unless ($available) { diff --git a/t/db_dependent/Patron/Borrower_Discharge.t b/t/db_dependent/Patron/Borrower_Discharge.t index 9274096d96..869ccc18c1 100755 --- a/t/db_dependent/Patron/Borrower_Discharge.t +++ b/t/db_dependent/Patron/Borrower_Discharge.t @@ -16,7 +16,7 @@ use Modern::Perl; -use Test::More tests => 23; +use Test::More tests => 32; use Test::MockModule; use Test::Warn; @@ -89,10 +89,10 @@ $builder->build_sample_item( ); AddIssue( $patron, $barcode ); -is( - Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ), 0, - 'A patron with issues cannot be discharged' -); +my ( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ); +is( $can, 0, 'A patron with issues cannot be discharged' ); +is( $problems->{checkouts}, 1, "Patron has checkouts" ); +is( $problems->{debt}, undef, "Patron has no debt" ); is( Koha::Patron::Discharge::request( { borrowernumber => $patron->borrowernumber } ), undef, @@ -105,13 +105,46 @@ is( is_deeply( [Koha::Patron::Discharge::get_pendings], [], 'There is no pending discharge request' ); is_deeply( [Koha::Patron::Discharge::get_validated], [], 'There is no validated discharge' ); +# Discharge not possible with issues and fines +$patron->account->add_debit( + { + amount => 0.1, + interface => C4::Context->interface, + type => 'OVERDUE' + } +); + +( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ); +is( + $can, 0, + 'A patron with fines and checkouts cannot be discharged' +); +is( $problems->{checkouts}, 1, "Patron has checkouts" ); +is( $problems->{debt}, 0.1, "Patron has debt" ); + AddReturn($barcode); -# Discharge possible without issue +( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ); +is( + $can, 0, + 'A patron with fines cannot be discharged' +); +is( $problems->{checkouts}, undef, "Patron has checkouts" ); +is( $problems->{debt}, 0.1, "Patron has debt" ); + +$patron->account->pay( + { + amount => 0.1, + } +); + +# Discharge possible without issue or fine +( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ); is( - Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ), 1, - 'A patron without issues can be discharged' + $can, 1, + 'A patron without issues or fines can be discharged' ); +is( scalar keys %$problems, 0, "No dishcharge problems found" ); is( Koha::Patron::Discharge::generate_as_pdf, undef, "Confirm failure when lacking borrower number" ); -- 2.39.5