https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #40 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #38)
In general, the code is not very clear although this is kind of a new feature..
Just some thoughts about the POD of involved methods (apart from their ugly names..): my $can_see = $patron->can_see_things_from( $branchcode ); Return true if the I<Koha::Patron> can perform some action on the given thing => Can see is imho different from "performing actions on" ? Same for libraries_where_can_see_things POD "an aribitarary [...] action is allowed to be taken" => is that seeing? "the thing can see thing's infos" => really? how clear is that?
I do not plan to deal with all the names here - we can do that on a follow-up - can you suggest better language?
The else branch is not intuitive. Because the empty list return of libraries_where_can_see_things is confusing. } 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; POD libraries_where_can_see_things "An empty array means no restriction" ! But if there is no userenv, it also returns empty list. So you can see all. The name of the routine together with an empty list as return is kind of confusing!
I just adjust the call within that conditional - how would you prefer it to be written?
If you do have the specified permission, the @restricted_branchcodes is EMPTY too. So you can see.. If you dont have the permission, we are checking the library groups.
We are calling get_root_ancestor: if the group has no parent, the ancestor is the group itself. If it has the feature enabled, we are calling ->parent again (no result) and we CRASH on ->all_libraries. If we have another ancestor but no root has the feature enabled, we are again getting empty list. So can see all. If some root ancestor has the specified feature enabled say ft_hide_patron_info, then all libraries under the parent group are added to @restricted. Ultimately, if the branchcode is found in this list in can_see_things_from, you can 'see things''. (Side note: If the group with that feature has no parent, undef is returned and it looks like calling ->all_libraries will CRASH again?)
Each library in a group is a row in the groups table - so calling 'library_groups' will return the library memberships - they will always have a top level parent above them. Did you actually make this crash, or was a theoretical? I cannot make it crash and it should not because of the structure
Suppose patron library L1 is part of group A with ft enabled. And the branchcode L2 we are looking for is not in those groups/subgroups. But L2 is part of group B that has not enabled the feature. A patron from a library under A (or below) can see L1, but cannot see L2. (Why not: B did not enable ft)
Yeah, it is more limit members to seeing other members
A patron from a library under B and no other groups, can see L1 and can see L2. A patron from a library that has no group, can see L1 and L2. Note that the description "Hide patron's info for librarians outside of this group" is confusing/misleading. Librarians btw?
I tried to improve the wording - feel free to edit further
Actually you are hiding L2 from an A patron. And L1 can be seen by all above. So isnt it: Hide patron info outside "this group tree or something"? What about L3 if it is part of A with ft and part of B without ft btw? Etc.
The B membership won't matter, because the feature doesn't apply, so that tree will be ignored. The A membership will limit L3 to itself and A (depending on what level, hopefully I parsed correctly)
Maybe I am missing something here. Please clarify and prevent the code crashing on no parent.
-- You are receiving this mail because: You are watching all bug changes.