Bugzilla – Attachment 170664 Details for
Bug 14250
Don't allow to generate discharges for patrons with fines
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14250: (QA follow-up) Tidy code
Bug-14250-QA-follow-up-Tidy-code.patch (text/plain), 17.94 KB, created by
Kyle M Hall (khall)
on 2024-08-23 18:12:59 UTC
(
hide
)
Description:
Bug 14250: (QA follow-up) Tidy code
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2024-08-23 18:12:59 UTC
Size:
17.94 KB
patch
obsolete
>From 1918bc35dd533c42ef4bd8a712882c231df72253 Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 14250
:
170136
|
170228
|
170581
|
170607
|
170662
|
170663
|
170664
|
170705
|
170706
|
170707
|
170708
|
171025
|
171026
|
171271
|
171272
|
171273
|
171274
|
171275
|
171276