Description
Katrin Fischer
2024-07-17 11:54:27 UTC
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. (In reply to Brendan Lawlor from comment #33) > this needs mroe cleanup Sure :) QA: Looking here now 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! 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. Would love to see this one fixed. (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. 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> 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> 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> Created attachment 173657 [details] [review] Bug 37392: (follow-up) Tidy and improve the description of patron limits in staff interface (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) :-) 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> 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> 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> 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> 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> 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 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. (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 Pushed for 24.11! Well done everyone, thank you! 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. (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 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 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 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. Follow-up fixed broken tests locally. Pushed to main. Thanks Nick! 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> Thanks for fixing! Is it possible to backport this to 24.05? Doesn't apply clean to 24.05.x, no backport. 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) Rebased for 24.05, only test conflicts, please consider |