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...
Have a look at my patch on Bug 37383, I hope that it solves both this problem and the one reported there.
Created attachment 169433 [details] [review] Bug 37392: Unit test
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.
(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.
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.
Created attachment 169515 [details] [review] Bug 37392: Unit test
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.
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
Hi Nick, how can we help to move this forward?
(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
Nick, can you explain a bit how this relates to bug 37383?
(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
So we would want multiple overlapping groups for testing with a mix of edit items and patron permissions?
(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
(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
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?
Should we switch to NSO now?
(In reply to Katrin Fischer from comment #17) > Should we switch to NSO now? I think, yes.
Created attachment 170392 [details] [review] Bug 37392: Unit test Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
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>
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>
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)
(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.
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.
+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
(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.
(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.
@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.
(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?
(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.
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>
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>
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>
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.