From c352eab605602849af9760968a74470d3acce3a8 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 24 Jul 2024 19:18:15 +0000 Subject: [PATCH] Bug 37392: Adjust routines The current code only handled a single layer of groups - top level setting the features, and libraries directly underneath. The code, however, was not correctly checking the features, and was limiting to single like when no restrictions found. This patch gets the root ancestor for a group, checks the desired feature against than group, then fetches all children of the current group and makes them allowed - i.e. when a library is in a group, all siblings and descendants in that group or subgroups can be accessed I adjust some typos in the tests too, this needs more cleanup in the future, but am submitting for any discussion Signed-off-by: Michaela Sieber Signed-off-by: Brendan Lawlor Signed-off-by: Martin Renvoize Signed-off-by: Katrin Fischer --- Koha/Patron.pm | 14 ++++++++++---- t/db_dependent/Koha/Patrons.t | 31 ++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 7de0d8c800..6440939573 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1926,6 +1926,9 @@ sub can_see_things_from { $can = 1; } elsif ( my @branches = $self->libraries_where_can_see_things($params) ) { $can = ( any { $_ eq $branchcode } @branches ) ? 1 : 0; + } else { + # This should be the case of not finding any limits above, so we can + $can = 1; } return $can; } @@ -1994,18 +1997,21 @@ sub libraries_where_can_see_things { ) ) { - my $library_groups = $self->library->library_groups({ $group_feature => 1 }); + my $library_groups = $self->library->library_groups(); if ( $library_groups->count ) { while ( my $library_group = $library_groups->next ) { + my $root = Koha::Library::Groups->get_root_ancestor({ id => $library_group->id }); + next unless $root->$group_feature; my $parent = $library_group->parent; - if ( $parent->has_child( $self->branchcode ) ) { - push @restricted_branchcodes, $parent->children->get_column('branchcode'); + my @children = $parent->all_libraries; + foreach my $child (@children){ + push @restricted_branchcodes, $child->branchcode; + } } } - @restricted_branchcodes = ( $self->branchcode ) unless @restricted_branchcodes; } } diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 38525589d4..2e5115a67c 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -83,6 +83,8 @@ is( Koha::Patrons->search->count, $nb_of_patrons + 2, 'The 2 patrons should have my $retrieved_patron_1 = Koha::Patrons->find( $new_patron_1->borrowernumber ); is( $retrieved_patron_1->cardnumber, $new_patron_1->cardnumber, 'Find a patron by borrowernumber should return the correct patron' ); + + subtest 'library' => sub { plan tests => 2; is( $retrieved_patron_1->library->branchcode, $library->{branchcode}, 'Koha::Patron->library should return the correct library' ); @@ -1365,22 +1367,33 @@ subtest # Pfiou, we can start now! subtest 'libraries_where_can_see_things' => sub { - plan tests => 2; - t::lib::Mocks::mock_userenv( { patron => $patron_11_2 } ); + plan tests => 4; + t::lib::Mocks::mock_userenv( { patron => $patron_11_1 } ); my $params = { - permission => 'editcatalogue', - subpermission => 'edit_any_item', - group_feature => 'ft_limit_item_editing', + permission => 'borrowers', + subpermission => 'view_borrower_infos_from_any_libraries', + group_feature => 'ft_hide_patron_info', }; - my @branchcodes = $patron_11_2->libraries_where_can_see_things($params); + my @branchcodes = $patron_11_1->libraries_where_can_see_things($params); is_deeply( - \@branchcodes, [ sort ( $library_11->branchcode, $library_12->branchcode ) ], + \@branchcodes, [ ], q|patron_11_1 has view_borrower_infos_from_any_libraries => No restriction| ); + @branchcodes = $patron_11_1->libraries_where_can_see_things($params); + is_deeply( + \@branchcodes, [ ], + q|confirming second/cached request is the same patron_11_1 has view_borrower_infos_from_any_libraries => No restriction| + ); + @branchcodes = $patron_11_2->libraries_where_can_see_things($params); is_deeply( \@branchcodes, [ sort ( $library_11->branchcode, $library_12->branchcode ) ], - q|patron_11_1 has view_borrower_infos_from_any_libraries => No restriction| + q|patron_11_2 can only view from group| + ); + @branchcodes = $patron_11_2->libraries_where_can_see_things($params); + is_deeply( + \@branchcodes, [ sort ( $library_11->branchcode, $library_12->branchcode ) ], + q|confirming second/cached request is the same patron_11_2 can only view from group| ); }; @@ -1400,7 +1413,7 @@ subtest $patron_11_2->can_edit_items_from( $library_21->branchcode ), "We can edit an item from outside our group as the group does not limit item editing" ); - $group_2->ft_limit_item_editing(1)->store(); + $group_1->ft_limit_item_editing(1)->store(); $patron_11_2 = Koha::Patrons->find( $patron_11_2->borrowernumber ); #FIXME We refetch the patron because library lists are cached in an extra hash key -- 2.39.5