https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37383 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #1 from Lucas Gass <lucas@bywatersolutions.com> --- Have a look at my patch on Bug 37383, I hope that it solves both this problem and the one reported there. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #3 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 169434 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169434&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |37383, 37378 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37378 [Bug 37378] Patron searches can fail when library groups are set to 'Limit patron data access by group ' https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37383 [Bug 37383] No Edit Item button on biblio detail page for items where holding branch is not logged in branch -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169433|0 |1 is obsolete| | --- Comment #6 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 169515 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169515&action=edit Bug 37392: Unit test -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #8 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 169517 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169517&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #10 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #12 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #14 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Biblibre Sandboxes <sandboxes@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169516|0 |1 is obsolete| | --- Comment #20 from Biblibre Sandboxes <sandboxes@biblibre.com> --- Created attachment 170393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170393&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Michaela Sieber <michaela.sieber@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #23 from Lucas Gass <lucas@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@ptfs-europe.co | |m QA Contact|testopia@bugs.koha-communit |pedro.amorim@ptfs-europe.co |y.org |m --- Comment #24 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #28 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- @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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Brendan Lawlor <blawlor@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #170393|0 |1 is obsolete| | --- Comment #32 from Brendan Lawlor <blawlor@clamsnet.org> --- Created attachment 172699 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172699&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Brendan Lawlor <blawlor@clamsnet.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |blawlor@clamsnet.org --- Comment #34 from 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #35 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Brendan Lawlor from comment #33)

this needs mroe cleanup

Sure :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA: Looking here now -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #38 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 George Williams (NEKLS) <george@nekls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |george@nekls.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #40 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #38)

In general, the code is not very clear although this is kind of a new feature..

Just some thoughts about the POD of involved methods (apart from their ugly names..): my $can_see = $patron->can_see_things_from( $branchcode ); Return true if the I<Koha::Patron> can perform some action on the given thing => Can see is imho different from "performing actions on" ? Same for libraries_where_can_see_things POD "an aribitarary [...] action is allowed to be taken" => is that seeing? "the thing can see thing's infos" => really? how clear is that?

I do not plan to deal with all the names here - we can do that on a follow-up - can you suggest better language?

The else branch is not intuitive. Because the empty list return of libraries_where_can_see_things is confusing. } elsif ( my @branches = $self->libraries_where_can_see_things($params) ) { $can = ( any { $_ eq $branchcode } @branches ) ? 1 : 0; } else { # This should be the case of not finding any limits above, so we can $can = 1; POD libraries_where_can_see_things "An empty array means no restriction" ! But if there is no userenv, it also returns empty list. So you can see all. The name of the routine together with an empty list as return is kind of confusing!

I just adjust the call within that conditional - how would you prefer it to be written?

If you do have the specified permission, the @restricted_branchcodes is EMPTY too. So you can see.. If you dont have the permission, we are checking the library groups.

We are calling get_root_ancestor: if the group has no parent, the ancestor is the group itself. If it has the feature enabled, we are calling ->parent again (no result) and we CRASH on ->all_libraries. If we have another ancestor but no root has the feature enabled, we are again getting empty list. So can see all. If some root ancestor has the specified feature enabled say ft_hide_patron_info, then all libraries under the parent group are added to @restricted. Ultimately, if the branchcode is found in this list in can_see_things_from, you can 'see things''. (Side note: If the group with that feature has no parent, undef is returned and it looks like calling ->all_libraries will CRASH again?)

Each library in a group is a row in the groups table - so calling 'library_groups' will return the library memberships - they will always have a top level parent above them. Did you actually make this crash, or was a theoretical? I cannot make it crash and it should not because of the structure

Suppose patron library L1 is part of group A with ft enabled. And the branchcode L2 we are looking for is not in those groups/subgroups. But L2 is part of group B that has not enabled the feature. A patron from a library under A (or below) can see L1, but cannot see L2. (Why not: B did not enable ft)

Yeah, it is more limit members to seeing other members

A patron from a library under B and no other groups, can see L1 and can see L2. A patron from a library that has no group, can see L1 and L2. Note that the description "Hide patron's info for librarians outside of this group" is confusing/misleading. Librarians btw?

I tried to improve the wording - feel free to edit further

Actually you are hiding L2 from an A patron. And L1 can be seen by all above. So isnt it: Hide patron info outside "this group tree or something"? What about L3 if it is part of A with ft and part of B without ft btw? Etc.

The B membership won't matter, because the feature doesn't apply, so that tree will be ignored. The A membership will limit L3 to itself and A (depending on what level, hopefully I parsed correctly)

Maybe I am missing something here. Please clarify and prevent the code crashing on no parent.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #172698|0 |1 is obsolete| | Attachment #172699|0 |1 is obsolete| | Attachment #172700|0 |1 is obsolete| | --- Comment #41 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 173654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173654&action=edit Bug 37392: Unit test Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #43 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 173656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173656&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #45 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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) :-) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173654|0 |1 is obsolete| | --- Comment #46 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 173699 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173699&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173656|0 |1 is obsolete| | --- Comment #48 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 173701 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173701&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #50 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 173703 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173703&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #51 from Martin Renvoize (ashimema) <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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173657|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #53 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #54 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #56 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #58 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed, | |regression, | |rel_24_11_candidate | --- Comment #60 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Follow-up fixed broken tests locally. Pushed to main. Thanks Nick! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|RM_priority | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |RESOLVED Resolution|--- |FIXED --- Comment #63 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Doesn't apply clean to 24.05.x, no backport. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|FIXED |--- Status|RESOLVED |REOPENED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_24_05_candidate --- Comment #65 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Rebased for 24.05, only test conflicts, please consider -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 --- Comment #67 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- (In reply to Alex Buckley from comment #66)

(In reply to Nick Clemens (kidclamp) from comment #65)

...

Rebased for 24.05, only test conflicts, please consider

Hi Nick,

For considering 24.05 backport for next month (i.e. 24.05.08), could you please let us know which of these patches should be applied to the 24.05.x branch?

Thanks

You should only need the "[24.05.x] Edit item permission by library group is broken patch". It applies cleanly as of today. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz --- Comment #69 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- When testing: * Test editing the items with the test user, only A is allowed to be edited. I was able to edit both A and B without doing the following: * Remove the view_borrower_infos_from_any_libraries permission from test user. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Bug 37392 depends on bug 37378, which changed state. Bug 37378 Summary: Patron searches can fail when library groups are set to 'Limit patron data access by group' https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37378 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37392 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Needs documenting --- Comment #71 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11 -- You are receiving this mail because: You are watching all bug changes.