From 1918bc35dd533c42ef4bd8a712882c231df72253 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 23 Aug 2024 18:11:40 +0000 Subject: [PATCH] Bug 14250: (QA follow-up) Tidy code --- opac/opac-discharge.pl | 74 +++++---- t/db_dependent/Patron/Borrower_Discharge.t | 170 ++++++++++++--------- 2 files changed, 143 insertions(+), 101 deletions(-) diff --git a/opac/opac-discharge.pl b/opac/opac-discharge.pl index 7eeab30e9b7..54fc526dfcc 100755 --- a/opac/opac-discharge.pl +++ b/opac/opac-discharge.pl @@ -21,7 +21,7 @@ use Modern::Perl; use Carp qw( carp ); use C4::Auth qw( get_template_and_user ); -use CGI qw( -utf8 ); +use CGI qw( -utf8 ); use C4::Context; use C4::Output qw( output_html_with_http_headers ); use Koha::Patrons; @@ -37,11 +37,13 @@ unless ( C4::Context->preference('useDischarge') ) { my $op = $input->param("op") // ''; # Getting the template and auth -my ( $template, $loggedinuser, $cookie ) = get_template_and_user({ - template_name => "opac-discharge.tt", - query => $input, - type => "opac", -}); +my ( $template, $loggedinuser, $cookie ) = get_template_and_user( + { + template_name => "opac-discharge.tt", + query => $input, + type => "opac", + } +); my ( $can_be_discharged, $discharge_problems ) = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $loggedinuser } ); @@ -50,38 +52,42 @@ 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, + can_be_discharged => $can_be_discharged, discharge_problems => $discharge_problems, - has_checkouts => $has_pending_checkouts, - has_debt => $has_debt, + has_checkouts => $has_pending_checkouts, + has_debt => $has_debt, ); -my $pending = Koha::Patron::Discharge::count({ - borrowernumber => $loggedinuser, - pending => 1, -}); -my $available = Koha::Patron::Discharge::is_discharged({borrowernumber => $loggedinuser}); +my $pending = Koha::Patron::Discharge::count( + { + borrowernumber => $loggedinuser, + pending => 1, + } +); +my $available = Koha::Patron::Discharge::is_discharged( { borrowernumber => $loggedinuser } ); if ( $op eq 'cud-request' ) { - if ($pending || $available) { + if ( $pending || $available ) { + # Request already done print $input->redirect("/cgi-bin/koha/opac-discharge.pl"); exit; } - my $success = Koha::Patron::Discharge::request({ - borrowernumber => $loggedinuser, - }); + my $success = Koha::Patron::Discharge::request( + { + borrowernumber => $loggedinuser, + } + ); if ($success) { $template->param( success => 1 ); - } - else { - $template->param( failure => 1 ); + } else { + $template->param( failure => 1 ); $template->param( has_issues => $has_pending_checkouts ); - $template->param( has_debt => $has_debt ); + $template->param( has_debt => $has_debt ); } -} -elsif ( $op eq 'get' ) { +} elsif ( $op eq 'get' ) { unless ($available) { + # No valid discharge to get print $input->redirect("/cgi-bin/koha/opac-discharge.pl"); exit; @@ -89,11 +95,13 @@ elsif ( $op eq 'get' ) { eval { # Getting member data - my $patron = Koha::Patrons->find( $loggedinuser ); - my $pdf_path = Koha::Patron::Discharge::generate_as_pdf({ - borrowernumber => $loggedinuser, - branchcode => $patron->branchcode, - }); + my $patron = Koha::Patrons->find($loggedinuser); + my $pdf_path = Koha::Patron::Discharge::generate_as_pdf( + { + borrowernumber => $loggedinuser, + branchcode => $patron->branchcode, + } + ); binmode(STDOUT); print $input->header( @@ -106,15 +114,15 @@ elsif ( $op eq 'get' ) { close $fh; print @lines; }; - if ( $@ ) { + if ($@) { carp $@; - $template->param( messages => [ {type => 'error', code => 'unable_to_generate_pdf'} ] ); + $template->param( messages => [ { type => 'error', code => 'unable_to_generate_pdf' } ] ); } else { + # no error, pdf is sent, so stop sending data to browser exit; } -} -else { +} else { $template->param( available => $available, pending => $pending, diff --git a/t/db_dependent/Patron/Borrower_Discharge.t b/t/db_dependent/Patron/Borrower_Discharge.t index 3444ac19dbf..bfb8cd3aead 100755 --- a/t/db_dependent/Patron/Borrower_Discharge.t +++ b/t/db_dependent/Patron/Borrower_Discharge.t @@ -25,7 +25,7 @@ use IPC::Cmd qw(can_run); use MARC::Record; use C4::Circulation qw( AddIssue AddReturn ); -use C4::Biblio qw( AddBiblio ); +use C4::Biblio qw( AddBiblio ); use C4::Context; use Koha::Patrons; @@ -35,7 +35,7 @@ use Koha::Database; use t::lib::TestBuilder; use t::lib::Mocks; -my $schema = Koha::Database->new->schema; +my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new; @@ -43,36 +43,42 @@ my $builder = t::lib::TestBuilder->new; my $dbh = C4::Context->dbh; $dbh->do(q|DELETE FROM discharges|); -my $library = $builder->build({ source => 'Branch' }); -my $another_library = $builder->build({ source => 'Branch' }); -my $itemtype = $builder->build({ source => 'Itemtype' })->{itemtype}; +my $library = $builder->build( { source => 'Branch' } ); +my $another_library = $builder->build( { source => 'Branch' } ); +my $itemtype = $builder->build( { source => 'Itemtype' } )->{itemtype}; C4::Context->_new_userenv('xxx'); -my $patron = $builder->build_object({ - class => 'Koha::Patrons', - value => { - branchcode => $library->{branchcode}, - flags => 1, # superlibrarian +my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + branchcode => $library->{branchcode}, + flags => 1, # superlibrarian + } } -}); -t::lib::Mocks::mock_userenv({ patron => $patron }); +); +t::lib::Mocks::mock_userenv( { patron => $patron } ); -my $patron2 = $builder->build_object({ - class => 'Koha::Patrons', - value => { - branchcode => $library->{branchcode}, +my $patron2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + branchcode => $library->{branchcode}, + } } -}); -my $patron3 = $builder->build_object({ - class => 'Koha::Patrons', - value => { - branchcode => $another_library->{branchcode}, - flags => undef, +); +my $patron3 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + branchcode => $another_library->{branchcode}, + flags => undef, + } } -}); +); # Discharge not possible with issues -my ( $biblionumber ) = AddBiblio( MARC::Record->new, ''); +my ($biblionumber) = AddBiblio( MARC::Record->new, '' ); my $barcode = 'BARCODE42'; $builder->build_sample_item( { @@ -84,15 +90,21 @@ $builder->build_sample_item( ); AddIssue( $patron, $barcode ); -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" ); +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' ); -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' ); +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' +); +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( @@ -103,24 +115,23 @@ $patron->account->add_debit( } ); -( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged({ borrowernumber => $patron->borrowernumber }); +( $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" ); +is( $problems->{checkouts}, 1, "Patron has checkouts" ); +is( $problems->{debt}, 0.1, "Patron has debt" ); -AddReturn( $barcode ); +AddReturn($barcode); -( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged({ borrowernumber => $patron->borrowernumber }); +( $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" ); - +is( $problems->{debt}, 0.1, "Patron has debt" ); $patron->account->pay( { @@ -129,33 +140,48 @@ $patron->account->pay( ); # Discharge possible without issue or fine -( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged({ borrowernumber => $patron->borrowernumber }); +( $can, $problems ) = Koha::Patron::Discharge::can_be_discharged( { borrowernumber => $patron->borrowernumber } ); is( $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"); +is( Koha::Patron::Discharge::generate_as_pdf, undef, "Confirm failure when lacking borrower number" ); # Verify that the user is not discharged anymore if the restriction has been lifted Koha::Patron::Discharge::discharge( { borrowernumber => $patron->borrowernumber } ); Koha::Patron::Discharge::discharge( { borrowernumber => $patron2->borrowernumber } ); Koha::Patron::Discharge::discharge( { borrowernumber => $patron3->borrowernumber } ); -is( Koha::Patron::Discharge::is_discharged( { borrowernumber => $patron->borrowernumber } ), 1, 'The patron has been discharged' ); -is( Koha::Patrons->find( $patron->borrowernumber )->is_debarred, '9999-12-31', 'The patron has been debarred after discharge' ); -is( scalar( Koha::Patron::Discharge::get_validated ), 3, 'There are 3 validated discharges' ); -is( scalar( Koha::Patron::Discharge::get_validated( { borrowernumber => $patron->borrowernumber } ) ), 1, 'There is 1 validated discharge for a given patron' ); -is( scalar( Koha::Patron::Discharge::get_validated( { branchcode => $library->{branchcode} } ) ), 2, 'There is 2 validated discharges for a given branchcode' ); # This is not used in the code yet +is( + Koha::Patron::Discharge::is_discharged( { borrowernumber => $patron->borrowernumber } ), 1, + 'The patron has been discharged' +); +is( + Koha::Patrons->find( $patron->borrowernumber )->is_debarred, '9999-12-31', + 'The patron has been debarred after discharge' +); +is( scalar(Koha::Patron::Discharge::get_validated), 3, 'There are 3 validated discharges' ); +is( + scalar( Koha::Patron::Discharge::get_validated( { borrowernumber => $patron->borrowernumber } ) ), 1, + 'There is 1 validated discharge for a given patron' +); +is( + scalar( Koha::Patron::Discharge::get_validated( { branchcode => $library->{branchcode} } ) ), 2, + 'There is 2 validated discharges for a given branchcode' +); # This is not used in the code yet Koha::Patron::Debarments::DelUniqueDebarment( { 'borrowernumber' => $patron->borrowernumber, 'type' => 'DISCHARGE' } ); ok( !Koha::Patrons->find( $patron->borrowernumber )->is_debarred, 'The debarment has been lifted' ); -ok( !Koha::Patron::Discharge::is_discharged( { borrowernumber => $patron->borrowernumber } ), 'The patron is not discharged after the restriction has been lifted' ); +ok( + !Koha::Patron::Discharge::is_discharged( { borrowernumber => $patron->borrowernumber } ), + 'The patron is not discharged after the restriction has been lifted' +); # Verify that the discharge works multiple times -Koha::Patron::Discharge::request({ borrowernumber => $patron->borrowernumber }); -is(scalar( Koha::Patron::Discharge::get_pendings ), 1, 'There is a pending discharge request (second time)'); +Koha::Patron::Discharge::request( { borrowernumber => $patron->borrowernumber } ); +is( scalar(Koha::Patron::Discharge::get_pendings), 1, 'There is a pending discharge request (second time)' ); Koha::Patron::Discharge::discharge( { borrowernumber => $patron->borrowernumber } ); -is_deeply( [ Koha::Patron::Discharge::get_pendings ], [], 'There is no pending discharge request (second time)'); +is_deeply( [Koha::Patron::Discharge::get_pendings], [], 'There is no pending discharge request (second time)' ); SKIP: { skip "Skipping because weasyprint is not installed", @@ -172,46 +198,54 @@ SKIP: { $mocked_ipc->mock( 'run', sub { return 0, 'Some error' } ); my $result; - warning_is - { $result = Koha::Patron::Discharge::generate_as_pdf( { borrowernumber => $patron->borrowernumber } ); } - 'Some error', + warning_is { $result = Koha::Patron::Discharge::generate_as_pdf( { borrowernumber => $patron->borrowernumber } ); } + 'Some error', 'Failed call to run() prints the generated error'; is( $result, undef, 'undef returned if failed run' ); $mocked_ipc->mock( 'can_run', undef ); - warning_is - { $result = Koha::Patron::Discharge::generate_as_pdf( { borrowernumber => $patron->borrowernumber } ); } - 'weasyprint not found!', + warning_is { $result = Koha::Patron::Discharge::generate_as_pdf( { borrowernumber => $patron->borrowernumber } ); } + 'weasyprint not found!', 'Expected failure because of missing weasyprint'; is( $result, undef, 'undef returned if missing weasyprint' ); } # FIXME Should be a Koha::Object object -is( ref(Koha::Patron::Discharge::request({ borrowernumber => $patron->borrowernumber })), 'Koha::Schema::Result::Discharge', 'Discharge request sent' ); +is( + ref( Koha::Patron::Discharge::request( { borrowernumber => $patron->borrowernumber } ) ), + 'Koha::Schema::Result::Discharge', 'Discharge request sent' +); subtest 'search_limited' => sub { plan tests => 4; $dbh->do(q|DELETE FROM discharges|); my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1' } )->store; my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2' } )->store; + # $patron and $patron2 are from the same library, $patron3 from another one # Logged in user is $patron, superlibrarian - t::lib::Mocks::mock_userenv({ patron => $patron }); - Koha::Library::Group->new({ parent_id => $group_1->id, branchcode => $patron->branchcode })->store(); - Koha::Library::Group->new({ parent_id => $group_2->id, branchcode => $patron3->branchcode })->store(); - Koha::Patron::Discharge::request({ borrowernumber => $patron->borrowernumber }); - Koha::Patron::Discharge::request({ borrowernumber => $patron2->borrowernumber }); - Koha::Patron::Discharge::request({ borrowernumber => $patron3->borrowernumber }); - is( scalar( Koha::Patron::Discharge::get_pendings), 3, 'With permission, all discharges are visible' ); - is( Koha::Patron::Discharge::count({pending => 1}), 3, 'With permission, all discharges are visible' ); + t::lib::Mocks::mock_userenv( { patron => $patron } ); + Koha::Library::Group->new( { parent_id => $group_1->id, branchcode => $patron->branchcode } )->store(); + Koha::Library::Group->new( { parent_id => $group_2->id, branchcode => $patron3->branchcode } )->store(); + Koha::Patron::Discharge::request( { borrowernumber => $patron->borrowernumber } ); + Koha::Patron::Discharge::request( { borrowernumber => $patron2->borrowernumber } ); + Koha::Patron::Discharge::request( { borrowernumber => $patron3->borrowernumber } ); + is( scalar(Koha::Patron::Discharge::get_pendings), 3, 'With permission, all discharges are visible' ); + is( Koha::Patron::Discharge::count( { pending => 1 } ), 3, 'With permission, all discharges are visible' ); # With patron 3 logged in, only discharges from their group are visible - t::lib::Mocks::mock_userenv({ patron => $patron3 }); - is( scalar( Koha::Patron::Discharge::get_pendings), 1, 'Without permission, only discharge from our group are visible' ); - is( Koha::Patron::Discharge::count({pending => 1}), 1, 'Without permission, only discharge from our group are visible' ); + t::lib::Mocks::mock_userenv( { patron => $patron3 } ); + is( + scalar(Koha::Patron::Discharge::get_pendings), 1, + 'Without permission, only discharge from our group are visible' + ); + is( + Koha::Patron::Discharge::count( { pending => 1 } ), 1, + 'Without permission, only discharge from our group are visible' + ); }; $schema->storage->txn_rollback; -- 2.39.2