https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392
--- Comment #40 from Nick Clemens (kidclamp) <[email protected]> --- (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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
