When IndependantBranches syspref is enabled, a 'regular' user can only delete items belonging to his/her library. But a superlibrarian should have the permission to delete items from all libraries. He can't for the time being.
Created attachment 5460 [details] [review] Proposed patch How to test? - On a multi-libraries Koha, activate IndependantBranches - Log in with a superlibrarian user - Find a biblio with one item from another library than the user home library - Click on Edit > Edit Items - On the list of items, all lines have Delete link - If you try to delete an item from another library than the user home library, deletion will fail.
Looking at the patch (on the mailing list) userenv->{flags} != 1 isn't the best way to check for superlibrarian status. I think that userenv->{flags} %2 != 1 is the preferred way.
I confirm that, if user has more than the superlibrarian permission checked, userenv->{flags} will be greater than 1
Created attachment 5771 [details] [review] Amende proposed patch Corrected patch. Thanks Mikeal and Julian.
Created attachment 5839 [details] [review] Signed-off patch I confirmed the buggy behavior following the steps provided. After applying the patch I was able to delete items from another branch successfully.
Created attachment 5857 [details] [review] Bug 6885 Allow superlib to delete items from any library When IndependantBranches syspref is enabled, a 'regular' user can only delete items belonging to his/her library. But a superlibrarian should have the permission to delete items from all libraries. He can't for the time being. This is fix by this patch. How to test? - On a multi-libraries Koha, activate IndependantBranches - Log in with a superlibrarian user - Find a biblio with one item from another library than the user home library - Click on Edit > Edit Items - On the list of items, all lines have Delete link - If you try to delete an item from another library than the user home library, deletion will fail. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
QA comment * good description * small fix, written like everywhere in the code to check against superlibrarian permission => passed QA
This will work to add the permissions check, yes, and it is consistent with how superlibrarian is tested in other parts of the code. This method (modulo 2) is not universal, however, and another bug report should be filed to apply a uniform checking methodology across the entire codebase. I recommend bitwise AND. Passed QA.
(In reply to comment #8) > This will work to add the permissions check, yes, and it is consistent > with how superlibrarian is tested in other parts of the code. This > method (modulo 2) is not universal, however, and another bug report > should be filed to apply a uniform checking methodology across the > entire codebase. I recommend bitwise AND. Yes, you're correct, and there are a lot of locations in Koha where permissions flags are not properly tested. A 'grep -R flags' show them. The superlibrarian permission should be tested like this: if $flags & 1 cataloguing: $flags & 4 and so on...
No changes to templates, pushed
I just tested this on Koha 3.6 and was unable to delete and item even though I was logged in as a superlibrarian ... can anyone confirm that this is or isn't fixed?
(In reply to comment #11) > I just tested this on Koha 3.6 and was unable to delete and item even though I > was logged in as a superlibrarian Unable to delete because the link wasn't there, or because of an error? It's working for me.
I clicked DELETE (the link was there) and then it said I couldn't delete because it wasn't at my branch.
The patch is included in HEAD/3.6. If you are on a git install, you can check: git log --grep="Bug 6885"
This bug is back in HEAD/3.6.x. This is due to a follow up patch which commit id is: cdd798ec3216c6019adccc40935e93736ce4dc3b The condition has to be inverted.
Created attachment 7039 [details] [review] Follow up patch
Created attachment 7046 [details] [review] Bug 6885 Follow up patch, invert tested condition Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Not sure the brackets are nessecary, but I think I remember learning bitwise operators have a lower priority than other relational operators so wanted to make sure we notted the result of the bitwise and, not the flag
Created attachment 7047 [details] [review] Bug 6885 Follow up patch, invert tested condition Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Not sure the brackets are nessecary, but I think I remember learning bitwise operators have a lower priority than other relational operators so wanted to make sure we notted the result of the bitwise and, not the flag
> Not sure the brackets are nessecary, but I think I remember learning > bitwise operators have a lower priority than other relational > operators so wanted to make sure we notted the result of the bitwise > and, not the flag That's a wise decision. Thanks. And it adds readability.
QA comment: tiny patch, nothing to say (except the condition is a little bit hard to read now, but could if be different ? I'm not sure)