Bug 37392

Summary: Edit item permission by library group is broken
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: CatalogingAssignee: Nick Clemens (kidclamp) <nick>
Status: Pushed to stable --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: major    
Priority: P5 - low CC: blawlor, clemens.tubach, george, jonathan.druart, kyle, lauren_denny, lucas, m.de.rooy, martin.renvoize, michaela.sieber, mnero, nick, pedro.amorim, tomascohen
Version: MainKeywords: rel_24_05_candidate
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37383
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:
Bug Depends on: 37378, 37383, 29523    
Bug Blocks:    
Attachments: Bug 37392: Unit test
Bug 37392: can_see_things_from is always checking patron visibility
Bug 37392: Unit test
Bug 37392: can_see_things_from is always checking patron visibility
Bug 37392: Adjust routines
Bug 37392: Unit test
Bug 37392: can_see_things_from is always checking patron visibility
Bug 37392: Adjust routines
Bug 37392: Unit test
Bug 37392: can_see_things_from is always checking patron visibility
Bug 37392: Adjust routines
Bug 37392: Unit test
Bug 37392: can_see_things_from is always checking patron visibility
Bug 37392: Adjust routines
Bug 37392: (follow-up)
Bug 37392: Unit test
Bug 37392: can_see_things_from is always checking patron visibility
Bug 37392: Adjust routines
Bug 37392: (follow-up) Tidy and improve descriptions
Bug 37392: (QA follow-up) Attempt to clarify POD
Bug 37392: (follow-up) Limit a borrower not in a group and fix tests
Bug 37392: Fix API helpers testsa
Bug 37392: [24.05.x] Edit item permission by library group is broken

Description Katrin Fischer 2024-07-17 11:54:27 UTC
The library groups allow to set up library groups within the staff users can edit each others items. This was introduced in 23.05, but is now broken in 23.11.

It's not possible for a staff user to edit another libraries items now in some circumstances.

Test plan:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A 
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.   
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.


-----
After some digging to understand the change of behavior noticed between 23.05 and 23.11:

I believe commit 2c502d32 to be the culprit.
item.can_be_edit uses Patron::can_edit_item_from which calls
can_see_things_from.

The commit removed the library group specific code and introduced a call to libraries_where_can_see_patrons instead, probably assuming the thing was always a patron record. But here it is an item record.

Thinking we should not use "thing" in method names without at least explaining what thing can be...
Comment 1 Lucas Gass (lukeg) 2024-07-22 23:48:36 UTC
Have a look at my patch on Bug 37383, I hope that it solves both this problem and the one reported there.
Comment 2 Nick Clemens (kidclamp) 2024-07-23 15:42:06 UTC
Created attachment 169433 [details] [review]
Bug 37392: Unit test
Comment 3 Nick Clemens (kidclamp) 2024-07-23 15:42:09 UTC
Created attachment 169434 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.
Comment 4 Nick Clemens (kidclamp) 2024-07-23 15:43:34 UTC
(In reply to Lucas Gass from comment #1)
> Have a look at my patch on Bug 37383, I hope that it solves both this
> problem and the one reported there.

This does solve the problem and we should move that forward.

We should also address here the incorrect call in "can_see_things_from"

I have tried ot test and fix this, but I don't understand the groups well enough to write a test that fails befroe and passes after, though I am fairley certain the actual code change is right.
Comment 5 Kyle M Hall (khall) 2024-07-23 17:03:37 UTC
libraries_where_can_see_things has the line

my $library_groups = $self->library->library_groups({ $group_feature => 1 });

but library_groups() does not take a parameter, it always returns all related library groups, not filtered by feature. All attempts to fix this issue result in different unit test failures.

It seems like replacing all uses of libraries_where_can_see_things with the more specific counterpart is the best solution. Then we can remove that bad code altogether and have more DRY code.
Comment 6 Nick Clemens (kidclamp) 2024-07-24 19:21:58 UTC
Created attachment 169515 [details] [review]
Bug 37392: Unit test
Comment 7 Nick Clemens (kidclamp) 2024-07-24 19:22:00 UTC
Created attachment 169516 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.
Comment 8 Nick Clemens (kidclamp) 2024-07-24 19:22:03 UTC
Created attachment 169517 [details] [review]
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 mroe cleanup, but am submitting for any discussion
Comment 9 Katrin Fischer 2024-07-29 11:17:49 UTC
Hi Nick, how can we help to move this forward?
Comment 10 Nick Clemens (kidclamp) 2024-07-30 12:57:46 UTC
(In reply to Katrin Fischer from comment #9)
> Hi Nick, how can we help to move this forward?

Test and confirm that it works as described, and that you agree with the functionality. If so, I can write the additional tests needed
Comment 11 Katrin Fischer 2024-07-31 11:52:24 UTC
Nick, can you explain a bit how this relates to bug 37383?
Comment 12 Nick Clemens (kidclamp) 2024-07-31 12:50:27 UTC
(In reply to Katrin Fischer from comment #11)
> Nick, can you explain a bit how this relates to bug 37383?

That moves from using the "can_edit_items_from" routine to "libraries_where_can_edit_items"

It makes it closer to correct, but here I found the underlying routines still had inaccuracies.

If you have only groups that limit editing it would all work, which is why the test plan is correct, but as Kyle notes here - the parameter to limit by group type isn't actually effective - that patches here aim to fix that
Comment 13 Katrin Fischer 2024-07-31 13:37:14 UTC
So we would want multiple overlapping groups for testing with a mix of edit items and patron permissions?
Comment 14 Nick Clemens (kidclamp) 2024-07-31 13:48:04 UTC
(In reply to Katrin Fischer from comment #13)
> So we would want multiple overlapping groups for testing with a mix of edit
> items and patron permissions?

Yes, sorry, I thought I had posted more here, but it seems I did not :-)

I think ideally you would setup

Top level group
Consortia - set to limit both patrons and item edition

This would have a subgroup:
East libraries - With Centerville and Fairfield
East Hill libraries - With Midway and IPT
East Lake Libraries - With Fairview and Centervile

Each library should be limited to their siblings and descendants, so:
Centerville and Fairfield can see all the libraries in the group
Midway and IPT can only see each other
Fairview can see themselves and Centerville
Comment 15 Nick Clemens (kidclamp) 2024-07-31 13:48:51 UTC
(In reply to Nick Clemens (kidclamp) from comment #14)
> (In reply to Katrin Fischer from comment #13)
> > So we would want multiple overlapping groups for testing with a mix of edit
> > items and patron permissions?
> 
> Yes, sorry, I thought I had posted more here, but it seems I did not :-)
> 
> I think ideally you would setup
> 
> Top level group
> Consortia - set to limit both patrons and item edition
> 
> This would have a subgroup:
> East libraries - With Centerville and Fairfield
> East Hill libraries - With Midway and IPT
> East Lake Libraries - With Fairview and Centervile
> 
> Each library should be limited to their siblings and descendants, so:
> Centerville and Fairfield can see all the libraries in the group
> Midway and IPT can only see each other
> Fairview can see themselves and Centerville

Then test with only one limit enabled
Then make a second group with the other limit enabled and choose different libraries
Comment 16 Katrin Fischer 2024-08-15 09:47:49 UTC
Thanks Nick, I think that cleared up things. This is still on my list, but I am not sure when I will make time to help with testing. I'd love if someone else could help here. Maybe you are aware of someone else using this feature?
Comment 17 Katrin Fischer 2024-08-15 09:48:01 UTC
Should we switch to NSO now?
Comment 18 Lucas Gass (lukeg) 2024-08-15 13:46:24 UTC
(In reply to Katrin Fischer from comment #17)
> Should we switch to NSO now?

I think, yes.
Comment 19 Biblibre Sandboxes 2024-08-15 14:41:51 UTC
Created attachment 170392 [details] [review]
Bug 37392: Unit test

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 20 Biblibre Sandboxes 2024-08-15 14:41:54 UTC
Created attachment 170393 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 21 Biblibre Sandboxes 2024-08-15 14:41:56 UTC
Created attachment 170394 [details] [review]
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 mroe cleanup, but am submitting for any discussion

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 22 Marcel de Rooy 2024-08-16 09:12:53 UTC
 WARN   Koha/Patron.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 294, now: 296)

 WARN   t/db_dependent/Koha/Patrons.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 675, now: 691)
Comment 23 Lucas Gass (lukeg) 2024-08-16 13:42:59 UTC
(In reply to Marcel de Rooy from comment #22)
>  WARN   Koha/Patron.pm
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 294, now: 296)
> 
>  WARN   t/db_dependent/Koha/Patrons.t
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 675, now: 691)

Nick's patch does not touch these lines. Should we be tidying unrelated code? In my opinion, no.
Comment 24 Pedro Amorim 2024-08-16 16:15:28 UTC
Taking this bug for QA, assigning it to me to prevent others from also looking and possibly waste mine or their time.
I've taken a look at the code and I'm having a hard time following what's happening here.
I've left Nick a message to discuss this with him and hopefully bring me up to speed so that I can properly QA this or ultimately pass it to someone else if I'm unable to.
Comment 25 Marcel de Rooy 2024-09-20 09:16:34 UTC
+subtest
+    'libraries_where_can_see_patrons + libraries_where_can_see_things + can_see_patron_infos + search_limited+ can_see_patrons_from + can_edit_items_from'
+    => sub {

Hmm
Comment 26 Marcel de Rooy 2024-09-20 09:18:16 UTC
(In reply to Lucas Gass from comment #23)
> (In reply to Marcel de Rooy from comment #22)
> >  WARN   Koha/Patron.pm
> >    WARN   tidiness
> >                 The file is less tidy than before (bad/messy lines before:
> > 294, now: 296)
> > 
> >  WARN   t/db_dependent/Koha/Patrons.t
> >    WARN   tidiness
> >                 The file is less tidy than before (bad/messy lines before:
> > 675, now: 691)
> 
> Nick's patch does not touch these lines. Should we be tidying unrelated
> code? In my opinion, no.

The changed lines somehow cause an different outcome in untidy lines. So yes, it needs attention. Sorry.
Comment 27 Marcel de Rooy 2024-09-20 09:21:17 UTC
(In reply to Pedro Amorim from comment #24)
> Taking this bug for QA, assigning it to me to prevent others from also
> looking and possibly waste mine or their time.
> I've taken a look at the code and I'm having a hard time following what's
> happening here.
> I've left Nick a message to discuss this with him and hopefully bring me up
> to speed so that I can properly QA this or ultimately pass it to someone
> else if I'm unable to.

Sure, changing the status for now.
Comment 28 Pedro Amorim 2024-09-20 09:30:37 UTC
@Marcel I have not yet had the chance to pick this up again, and I failed to understand it well enough to do proper QA the first time. You are certainly more capable than me. If you're available here, I'm happy to swap places with you as QA Contact.
Comment 29 Katrin Fischer 2024-09-25 15:36:43 UTC
(In reply to Marcel de Rooy from comment #25)
> +subtest
> +    'libraries_where_can_see_patrons + libraries_where_can_see_things +
> can_see_patron_infos + search_limited+ can_see_patrons_from +
> can_edit_items_from'
> +    => sub {
> 
> Hmm

Can you explain or was this just a personal note for later?

Is this just failed for tidiness or is there another issue?
Comment 30 Marcel de Rooy 2024-09-26 05:38:42 UTC
(In reply to Katrin Fischer from comment #29)
> (In reply to Marcel de Rooy from comment #25)
> > +subtest
> > +    'libraries_where_can_see_patrons + libraries_where_can_see_things +
> > can_see_patron_infos + search_limited+ can_see_patrons_from +
> > can_edit_items_from'
> > +    => sub {
> > 
> > Hmm
> 
> Can you explain or was this just a personal note for later?
This is about subtest naming :) Thats just crazy.

> Is this just failed for tidiness or is there another issue?
Comment27/28 explains that. I will change to SO again in view of comment28.
Comment 31 Brendan Lawlor 2024-10-11 14:58:52 UTC
Created attachment 172698 [details] [review]
Bug 37392: Unit test

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 32 Brendan Lawlor 2024-10-11 14:58:54 UTC
Created attachment 172699 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 33 Brendan Lawlor 2024-10-11 14:58:56 UTC
Created attachment 172700 [details] [review]
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 mroe cleanup, but am submitting for any discussion

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 34 Brendan Lawlor 2024-10-11 15:05:45 UTC
I tested this for our consortium's use case where libraries with branches need to be able to edit their fellow branches items.

Thanks to everyone working on this bug. It is a really important permission for consortia. Hopefully it can be backported to 24.05.
Comment 35 Marcel de Rooy 2024-10-18 06:32:31 UTC
(In reply to Brendan Lawlor from comment #33)
> this needs mroe cleanup

Sure :)
Comment 36 Marcel de Rooy 2024-10-18 06:32:52 UTC
QA: Looking here now
Comment 37 Marcel de Rooy 2024-10-18 09:04:31 UTC
 WARN   Koha/Patron.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 281, now: 283)

 WARN   t/db_dependent/Koha/Patrons.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 671, now: 687)

Proving /usr/share/koha/t/db_dependent/Koha/Patrons.t OK!
Comment 38 Marcel de Rooy 2024-10-18 09:09:17 UTC
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?

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!
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?)

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)
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?
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.

Maybe I am missing something here. Please clarify and prevent the code crashing on no parent.
Comment 39 Katrin Fischer 2024-10-18 11:53:48 UTC
Would love to see this one fixed.
Comment 40 Nick Clemens (kidclamp) 2024-10-29 14:22:06 UTC
(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.
Comment 41 Nick Clemens (kidclamp) 2024-10-29 14:24:03 UTC
Created attachment 173654 [details] [review]
Bug 37392: Unit test

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 42 Nick Clemens (kidclamp) 2024-10-29 14:24:06 UTC
Created attachment 173655 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 43 Nick Clemens (kidclamp) 2024-10-29 14:24:09 UTC
Created attachment 173656 [details] [review]
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 <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 44 Nick Clemens (kidclamp) 2024-10-29 14:24:11 UTC
Created attachment 173657 [details] [review]
Bug 37392: (follow-up)

Tidy and improve the description of patron limits in staff interface
Comment 45 Nick Clemens (kidclamp) 2024-10-29 14:26:04 UTC
(In reply to Marcel de Rooy from comment #38)
> "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!
 
Additionally here, you should always have a userenv - this only applies to staff side, so you had better be logged in (or there is a bigger problem) :-)
Comment 46 Martin Renvoize (ashimema) 2024-10-30 07:23:39 UTC
Created attachment 173699 [details] [review]
Bug 37392: Unit test

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize (ashimema) 2024-10-30 07:23:43 UTC
Created attachment 173700 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize (ashimema) 2024-10-30 07:23:46 UTC
Created attachment 173701 [details] [review]
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 <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize (ashimema) 2024-10-30 07:23:50 UTC
Created attachment 173702 [details] [review]
Bug 37392: (follow-up) Tidy and improve descriptions

Tidy and improve the description of patron limits in staff interface

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize (ashimema) 2024-10-30 07:23:53 UTC
Created attachment 173703 [details] [review]
Bug 37392: (QA follow-up) Attempt to clarify POD

This aptch attemptes to tidy up and clarify the POD for various 'things'
methods in the Koha::Patron class.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize (ashimema) 2024-10-30 07:27:14 UTC
This is a rather confusing area of code.. and I'm really not at all sure about the reliance on 'only_my_library' without passing in 'this' user.  It's misleading and confusing.

But.. that predates the work here so I think we can, and should, handle that in a follow up bug.

This fixes an important regression, Passing QA
Comment 52 Katrin Fischer 2024-10-30 08:00:36 UTC
This doesn't apply, I think maybe something is wrong with the sequence - Martin, could you double check? I think first here probably needs to be last.
Comment 53 Nick Clemens (kidclamp) 2024-10-30 13:22:55 UTC
(In reply to Katrin Fischer from comment #52)
> This doesn't apply, I think maybe something is wrong with the sequence -
> Martin, could you double check? I think first here probably needs to be last.

Please try again :-) Obsoleted a duplicated patch
Comment 54 Katrin Fischer 2024-10-30 17:47:10 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 55 Jonathan Druart 2024-10-31 08:19:38 UTC
Jenkins is failing heavily. Koha_Main/3083/

The first failure is coming from here, I have not investigated others but might be related to this as well

t/db_dependent/ArticleRequests.t .. 4/36 
    #   Failed test 'Koha::ArticleRequests->search_limited should not return all article requests for restricted patron'
    #   at t/db_dependent/ArticleRequests.t line 184.
    #          got: '1'
    #     expected: '0'
    # Looks like you failed 1 test of 2.
Comment 56 Nick Clemens (kidclamp) 2024-11-01 11:50:43 UTC
(In reply to Jonathan Druart from comment #55)
> Jenkins is failing heavily. Koha_Main/3083/
> 
> The first failure is coming from here, I have not investigated others but
> might be related to this as well
> 
> t/db_dependent/ArticleRequests.t .. 4/36 
>     #   Failed test 'Koha::ArticleRequests->search_limited should not return
> all article requests for restricted patron'
>     #   at t/db_dependent/ArticleRequests.t line 184.
>     #          got: '1'
>     #     expected: '0'
>     # Looks like you failed 1 test of 2.

Working on these
Comment 57 Tomás Cohen Arazi (tcohen) 2024-11-01 11:57:16 UTC
Bisecting failed tests: t/db_dependent/Patron/Borrower_Discharge.t 

c352eab605602849af9760968a74470d3acce3a8 is the first bad commit
commit c352eab605602849af9760968a74470d3acce3a8
Author: Nick Clemens <nick@bywatersolutions.com>
Date:   Wed Jul 24 19:18:15 2024 +0000

    Bug 37392: Adjust routines
Comment 58 Tomás Cohen Arazi (tcohen) 2024-11-01 12:14:54 UTC
Bisecting failed tests: t/db_dependent/Koha/Object.t 

c352eab605602849af9760968a74470d3acce3a8 is the first bad commit
commit c352eab605602849af9760968a74470d3acce3a8
Author: Nick Clemens <nick@bywatersolutions.com>
Date:   Wed Jul 24 19:18:15 2024 +0000

    Bug 37392: Adjust routines
Comment 59 Nick Clemens (kidclamp) 2024-11-01 12:26:14 UTC
Created attachment 173871 [details] [review]
Bug 37392: (follow-up) Limit a borrower not in a group and fix tests

The previous patches took into account all the groups for a patron, but missed the case where a patron
didn't have permission to see outside their library, and their library is not in a group.

Code updated and a test added.

Other tests adjusted to ensure the feature to limit patrons was set in those groups.
Comment 60 Tomás Cohen Arazi (tcohen) 2024-11-01 12:35:48 UTC
Follow-up fixed broken tests locally. Pushed to main.
Thanks Nick!
Comment 61 Tomás Cohen Arazi (tcohen) 2024-11-01 19:35:08 UTC
Created attachment 173889 [details] [review]
Bug 37392: Fix API helpers testsa

This patch acknowledges the fact this patchset changed the called method
`libraries_where_can_see_patrons` for `libraries_where_can_see_things`.
And as such the mock was not working.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 62 Brendan Lawlor 2024-12-03 14:46:13 UTC
Thanks for fixing! Is it possible to backport this to 24.05?
Comment 63 Lucas Gass (lukeg) 2024-12-05 21:31:02 UTC
Doesn't apply clean to 24.05.x, no backport.
Comment 64 Nick Clemens (kidclamp) 2024-12-12 16:48:13 UTC
Created attachment 175413 [details] [review]
Bug 37392: [24.05.x] Edit item permission by library group is broken

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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 <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 37392: (follow-up) Tidy and improve descriptions

Tidy and improve the description of patron limits in staff interface

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 37392: (QA follow-up) Attempt to clarify POD

This aptch attemptes to tidy up and clarify the POD for various 'things'
methods in the Koha::Patron class.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 37392: (follow-up) Limit a borrower not in a group and fix tests

The previous patches took into account all the groups for a patron, but missed the case where a patron
didn't have permission to see outside their library, and their library is not in a group.

Code updated and a test added.

Other tests adjusted to ensure the feature to limit patrons was set in those groups.

Bug 37392: Fix API helpers testsa

This patch acknowledges the fact this patchset changed the called method
`libraries_where_can_see_patrons` for `libraries_where_can_see_things`.
And as such the mock was not working.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 37392: (QA tool fixes)
Comment 65 Nick Clemens (kidclamp) 2024-12-12 16:49:39 UTC
Rebased for 24.05, only test conflicts, please consider