From dc1e7f478f150a33d53a07114d65aead54569f48 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 7 Apr 2017 15:42:51 -0300 Subject: [PATCH] Bug 18403: Patron discharges This patch deals with patron's discharges. Test plan: Same as previously you will need to request dischages at the OPAC. On the staff interface the logged in user should not be allowed to see discharge from patrons outside his library group. The number of discharges waiting displayed on the mainpage should be correct as well. Signed-off-by: Signed-off-by: Jon McGowan Bug 18403: (follow-up) Patron discharges Fix QA issue: forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 150) Signed-off-by: Jonathan Druart --- Koha/Patron/Discharge.pm | 30 ++++++++---- .../prog/en/modules/members/discharges.tt | 2 +- members/discharge.pl | 4 +- members/discharges.pl | 5 +- t/db_dependent/Patron/Borrower_Discharge.t | 55 ++++++++++++++++++---- 5 files changed, 71 insertions(+), 25 deletions(-) diff --git a/Koha/Patron/Discharge.pm b/Koha/Patron/Discharge.pm index a9a6b8d46d..44bcc5dcca 100644 --- a/Koha/Patron/Discharge.pm +++ b/Koha/Patron/Discharge.pm @@ -27,8 +27,7 @@ sub count { $values->{validated} = { '!=', undef }; } - my $rs = Koha::Database->new->schema->resultset('Discharge'); - return $rs->search( $values )->count; + return search_limited( $values )->count; } sub can_be_discharged { @@ -50,9 +49,9 @@ sub is_discharged { my $borrowernumber = $params->{borrowernumber}; my $restricted = Koha::Patrons->find( $borrowernumber )->is_debarred; - my $validated = get_validated({borrowernumber => $borrowernumber}); + my @validated = get_validated({borrowernumber => $borrowernumber}); - if ($restricted && $validated) { + if ($restricted && @validated) { return 1; } else { return 0; @@ -163,9 +162,7 @@ sub get_pendings { ( defined $branchcode ? ( 'borrower.branchcode' => $branchcode ) : () ), }; - my $rs = Koha::Database->new->schema->resultset('Discharge'); - my @rs = $rs->search( $cond, { join => 'borrower' } ); - return \@rs; + return search_limited( $cond ); } sub get_validated { @@ -179,10 +176,23 @@ sub get_validated { ( defined $branchcode ? ( 'borrower.branchcode' => $branchcode ) : () ), }; - my $rs = Koha::Database->new->schema->resultset('Discharge'); - my @rs = $rs->search( $cond, { join => 'borrower' } ); - return \@rs; + return search_limited( $cond ); } +# TODO This module should be based on Koha::Object[s] +sub search_limited { + my ( $params, $attributes ) = @_; + my $userenv = C4::Context->userenv; + my @restricted_branchcodes; + if ( $userenv ) { + my $logged_in_user = Koha::Patrons->find( $userenv->{number} ); + @restricted_branchcodes = $logged_in_user->libraries_where_can_see_patrons; + } + $params->{'borrower.branchcode'} = { -in => \@restricted_branchcodes } if @restricted_branchcodes; + $attributes->{join} = 'borrower'; + + my $rs = Koha::Database->new->schema->resultset('Discharge'); + return $rs->search( $params, { join => 'borrower' } ); +} 1; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharges.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharges.tt index 0998acd6cb..91e9091496 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharges.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/discharges.tt @@ -23,7 +23,7 @@ - [% FOR d IN pending_discharges %] + [% FOREACH d IN pending_discharges %] [% d.borrower.surname %], [% d.borrower.firstname %] Allow diff --git a/members/discharge.pl b/members/discharge.pl index ed1c7acb54..ca7c44f45e 100755 --- a/members/discharge.pl +++ b/members/discharge.pl @@ -102,7 +102,7 @@ if ( $input->param('discharge') and $can_be_discharged ) { } # Already generated discharges -my $validated_discharges = Koha::Patron::Discharge::get_validated({ +my @validated_discharges = Koha::Patron::Discharge::get_validated({ borrowernumber => $borrowernumber, }); @@ -132,7 +132,7 @@ $template->param( branchcode => $patron->branchcode, has_reserves => $has_reserves, can_be_discharged => $can_be_discharged, - validated_discharges => $validated_discharges, + validated_discharges => \@validated_discharges, ); $template->param( dischargeview => 1, ); diff --git a/members/discharges.pl b/members/discharges.pl index 165a1cb90d..665d2732be 100755 --- a/members/discharges.pl +++ b/members/discharges.pl @@ -52,10 +52,9 @@ if( $op eq 'allow' ) { }) if $borrowernumber; } -my $pending_discharges = Koha::Patron::Discharge::get_pendings({ +my @pending_discharges = Koha::Patron::Discharge::get_pendings({ branchcode => $branchcode }); - -$template->param( pending_discharges => $pending_discharges ); +$template->param( pending_discharges => \@pending_discharges ); output_html_with_http_headers $input, $cookie, $template->output; diff --git a/t/db_dependent/Patron/Borrower_Discharge.t b/t/db_dependent/Patron/Borrower_Discharge.t index ddc6835886..cf08a88308 100644 --- a/t/db_dependent/Patron/Borrower_Discharge.t +++ b/t/db_dependent/Patron/Borrower_Discharge.t @@ -15,7 +15,7 @@ # with Koha; if not, see . use Modern::Perl; -use Test::More tests => 18; +use Test::More tests => 19; use Test::Warn; use MARC::Record; @@ -43,13 +43,16 @@ my $another_library = $builder->build({ source => 'Branch' }); my $itemtype = $builder->build({ source => 'Itemtype' })->{itemtype}; C4::Context->_new_userenv('xxx'); -C4::Context->set_userenv(0, 0, 0, 'firstname', 'surname', $library->{branchcode}, $library->{branchcode}, '', '', '', ''); my $patron = $builder->build({ source => 'Borrower', value => { branchcode => $library->{branchcode}, + flags => 1, # superlibrarian } }); +my $p = Koha::Patrons->find( $patron->{borrowernumber} ); +set_logged_in_user( $p ); + my $patron2 = $builder->build({ source => 'Borrower', value => { @@ -60,8 +63,10 @@ my $patron3 = $builder->build({ source => 'Borrower', value => { branchcode => $another_library->{branchcode}, + flags => undef, } }); +my $p3 = Koha::Patrons->find( $patron3->{borrowernumber} ); # Discharge not possible with issues my ( $biblionumber ) = AddBiblio( MARC::Record->new, ''); @@ -80,8 +85,8 @@ is( Koha::Patron::Discharge::can_be_discharged({ borrowernumber => $patron->{bor 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_deeply( [ Koha::Patron::Discharge::get_pendings ], [], 'There is no pending discharge request' ); +is_deeply( [ Koha::Patron::Discharge::get_validated ], [], 'There is no validated discharge' ); AddReturn( $barcode ); @@ -96,18 +101,18 @@ Koha::Patron::Discharge::discharge( { borrowernumber => $patron2->{borrowernumbe 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( 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' ); # 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)'); +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)'); # Check if PDF::FromHTML is installed. my $check = eval { require PDF::FromHTML; }; @@ -126,5 +131,37 @@ else { # FIXME Should be a Koha::Object object 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 + set_logged_in_user( $p ); + 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 + set_logged_in_user( $p3 ); + 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; +sub set_logged_in_user { + my ($patron) = @_; + C4::Context->set_userenv( + $patron->borrowernumber, $patron->userid, + $patron->cardnumber, 'firstname', + 'surname', $patron->library->branchcode, + 'Midway Public Library', $patron->flags, + '', '' + ); +} -- 2.11.0