From da14c72e05872f0aaf8a587d1032f67294bb8026 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 23 Aug 2024 13:40:09 -0400 Subject: [PATCH] Bug 14250: (QA follow-up) Deduplicate checks Signed-off-by: Kyle M Hall --- Koha/Patron/Discharge.pm | 42 ++++++++++++------- .../prog/en/modules/members/discharge.tt | 17 ++++---- .../bootstrap/en/modules/opac-discharge.tt | 19 +++++---- members/discharge.pl | 13 ++---- opac/opac-discharge.pl | 12 ++++-- t/db_dependent/Patron/Borrower_Discharge.t | 24 ++++++++--- 6 files changed, 79 insertions(+), 48 deletions(-) diff --git a/Koha/Patron/Discharge.pm b/Koha/Patron/Discharge.pm index a72f7f2b7b3..7a3a7923ec1 100644 --- a/Koha/Patron/Discharge.pm +++ b/Koha/Patron/Discharge.pm @@ -39,13 +39,22 @@ sub can_be_discharged { my $patron = Koha::Patrons->find( $params->{borrowernumber} ); return unless $patron; - my $has_pending_checkouts = $patron->checkouts->count; - return 0 if $has_pending_checkouts; + my $can_be_discharged = 1; + my $problems = {}; - my $has_debt = ( $patron->account->outstanding_debits->total_outstanding > 0 ); - return 0 if $has_debt; + 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 1; + return ( $can_be_discharged, $problems ); } sub is_discharged { @@ -53,10 +62,10 @@ sub is_discharged { return unless $params->{borrowernumber}; my $borrowernumber = $params->{borrowernumber}; - my $restricted = Koha::Patrons->find( $borrowernumber )->is_debarred; - my @validated = get_validated({borrowernumber => $borrowernumber}); + my $restricted = Koha::Patrons->find($borrowernumber)->is_debarred; + my @validated = get_validated( { borrowernumber => $borrowernumber } ); - if ($restricted && @validated) { + if ( $restricted && @validated ) { return 1; } else { return 0; @@ -67,19 +76,24 @@ 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({ - borrower => $borrowernumber, - needed => dt_from_string, - }); + return $rs->create( + { + borrower => $borrowernumber, + needed => dt_from_string, + } + ); } 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 9353e2cd45d..33e765187a3 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt @@ -1,6 +1,7 @@ [% USE raw %] [% USE Asset %] [% USE KohaDates %] +[% USE Price %] [% USE AuthorisedValues %] [% USE Branches %] [% PROCESS 'i18n.inc' %] @@ -54,15 +55,13 @@ [% UNLESS can_be_discharged %]

Cannot edit discharge for following reasons:

[% ELSE %] [% IF patron.holds.count %] 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 b0330a57aaa..43cac60998d 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 ) %] @@ -63,14 +64,18 @@ [% UNLESS can_be_discharged %]
You cannot be discharged for following reasons:
    - [% IF has_debt %] -
  • You have unpaid fines. Please pay your fines before reapplying.
  • + [% IF discharge_problems.checkouts %] +
  • + You have [% discharge_problems.checkouts | html %] item(s) checked out. + Please return them before reapplying. +
  • [% END %] - [% IF has_checkouts %] -
  • You have checked out items. Please return them before reapplying.
  • - [% END %] - [% IF !has_checkouts and !has_debt %] -
  • Patron cannot be discharged for an unknown reason, please contact a librarian for more information
  • + + [% IF discharge_problems.debt %] +
  • + They have unpaid fines totaling [% discharge_problems.debt | $Price %]. + Please pay your fines before reapplying. +
  • [% END %]
diff --git a/members/discharge.pl b/members/discharge.pl index 2742bad084a..c16e4d8cd12 100755 --- a/members/discharge.pl +++ b/members/discharge.pl @@ -61,16 +61,10 @@ my $logged_in_user = Koha::Patrons->find( $loggedinuser ); my $patron = Koha::Patrons->find( $borrowernumber ); output_and_exit_if_error( $input, $cookie, $template, { module => 'members', logged_in_user => $logged_in_user, current_patron => $patron } ); -my $can_be_discharged = Koha::Patron::Discharge::can_be_discharged({ +my ( $can_be_discharged, $discharge_problems ) = Koha::Patron::Discharge::can_be_discharged({ borrowernumber => $borrowernumber }); - -unless ($can_be_discharged) { - my $has_checkout = $patron->checkouts->count; - my $has_fine = $patron->account->outstanding_debits->total_outstanding; -} - # Generating discharge if needed if ( $op eq 'cud-discharge' && $input->param('discharge') and $can_be_discharged ) { my $is_discharged = Koha::Patron::Discharge::is_discharged({ @@ -111,8 +105,9 @@ my @validated_discharges = Koha::Patron::Discharge::get_validated({ }); $template->param( - patron => $patron, - can_be_discharged => $can_be_discharged, + 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 5e6869be444..7eeab30e9b7 100755 --- a/opac/opac-discharge.pl +++ b/opac/opac-discharge.pl @@ -43,14 +43,18 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user({ type => "opac", }); -my $can_be_discharged = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $loggedinuser } ); +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; my $has_debt = ( $patron->account->outstanding_debits->total_outstanding > 0 ); -$template->param( can_be_discharged => $can_be_discharged ); -$template->param( has_checkouts => $has_pending_checkouts ); -$template->param( has_debt => $has_debt ); +$template->param( + can_be_discharged => $can_be_discharged, + discharge_problems => $discharge_problems, + has_checkouts => $has_pending_checkouts, + has_debt => $has_debt, +); my $pending = Koha::Patron::Discharge::count({ borrowernumber => $loggedinuser, diff --git a/t/db_dependent/Patron/Borrower_Discharge.t b/t/db_dependent/Patron/Borrower_Discharge.t index 1ebbdc633df..3444ac19dbf 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 => 25; +use Test::More tests => 32; use Test::MockModule; use Test::Warn; @@ -84,7 +84,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, 'No request done if patron has issues' ); is( Koha::Patron::Discharge::discharge({ borrowernumber => $patron->borrowernumber }), undef, 'No discharge done if patron has issues' ); @@ -100,14 +103,23 @@ $patron->account->add_debit( } ); +( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged({ borrowernumber => $patron->borrowernumber }); is( - Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ), 0, + $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 ); -is( Koha::Patron::Discharge::can_be_discharged({ borrowernumber => $patron->borrowernumber }), 0, 'A patron with fines cannot be discharged' ); +( $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( @@ -117,10 +129,12 @@ $patron->account->pay( ); # 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, + $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.2