User management in basket : add a list of librarians ""using"" a given basket. Any librarian can add himself to the ""user list"". Add filter to users to basket list display (BibLibre MT6531)
Created attachment 9309 [details] [review] More granular permissions for baskets Add branch info to baskets Add a list of borrowers that are allowed to manage a basket (one list for each basket). Add a new subpermission: acquisition => order_manage_all If user is superlibrarian, or if he has permission acquisition = 1(GranularPermissions = OFF), or subpermission acquisition => order_manage_all (GranularPermissions = ON), he's authorised to manage all baskets. Depending on syspref AcqViewBaskets: 'all': user can manage all baskets 'branch': user can manage baskets of his branch (the basket branch is taken into account, not the branch of the basket's creator). If basket branch is not defined, all user can manage this basket. 'user': user can manage baskets he created, and baskets he's in their user list There are unit tests in t/Acquisition/CanUserManageBasket.t, which require Test::MockModule You can edit basket's branch and users list in basket modification page (acqui/basket.pl).
Test in a sandbox : - add premission order_manage to a patron - create basket and add a user list with this patron - syspref AcqViewBaskets: 'all' => Patron can see and add command in all basket - add premission order_manage to a patron - create basket and add a user list with this patron - syspref AcqViewBaskets: 'user' => Patron can see and add command only in the basket where he's in the user list - add premission order_manage to a patron of library A - create basket and add a branch : library A - syspref AcqViewBaskets: 'user' => Patron can see and add command only in the basket where the branxh A is added - add premission order_manage to a patron of library A - create baskets - syspref AcqViewBaskets: 'branch' => Patron can see and add command all baskets of the library A - add premission order_manage_all to a patron of library A - create baskets - syspref AcqViewBaskets: 'branch' => Patron can see the list of baskets but can't click view and go on the details (http://pro.test1.biblibre.com/cgi-bin/koha/acqui/basket.pl?basketno=2) : access denied : Error: You do not have permission to access this page. Click to log out In all this test, the button "close this basket" have disappeared from the details page of basket :http://pro.test1.biblibre.com/cgi-bin/koha/acqui/basket.pl?basketno=2
You still needs the order_manage permission to access pages. order_manage_all is just an additional permission to grant more rights. So you have to check both.
Created attachment 9943 [details] [review] More granular permissions for baskets Patch rebased on master
Created attachment 10227 [details] [review] Attachment to Bug 7295 - list of librarian managing a basket Bug 7295: More granular permissions for baskets Add branch info to baskets Add a list of borrowers that are allowed to manage a basket (one list for each basket). Add a new subpermission: acquisition => order_manage_all rebase If user is superlibrarian, or if he has permission acquisition = 1 (GranularPermissions = OFF), or subpermission acquisition => order_manage_all (GranularPermissions = ON). He's authorised to manage all baskets. Depending on syspref AcqViewBaskets: 'all': user can manage all baskets 'branch': user can manage baskets of his branch (the basket branch is taken into account, not the branch of the basket's creator). If basket branch is not defined, all user can manage this basket. 'user': user can manage baskets he created, and baskets he's in their user list There are unit tests in t/Acquisition/CanUserManageBasket.t, which require Test::MockModule You can edit basket's branch and users list in basket modification page (acqui/basket.pl)
Hi Julian, I started testing this today, here are some first notes: 1) The list of libraries is not ordered by description but code. 2) I think it should not be possible to change the branch, when independantbranches is turned on? 3) Why is the default of the branch "no branch"? Shouldn't it be the branch of the basket creator or the branch the staff member is currently logged in at? 4) We keep adding lots of functionality and options to acquisitons. This is good, but also adds a lot of complexity. I think most libraries don't need this granular level of control, so perhaps we should think about moving some of the new features into system preferences. For this feature I think it would be great if unique ids could be added to the <li> elements on the basket summary page. This way both options can be hidden, if a library does not want to use them. 5) The descriptions of the permissions are not very clear and don't show up next to each other in my installation. I am not sure what can be done about this. There is also bug 3849 about this.
Created attachment 10885 [details] [review] Bug 7295 - list of librarian managing a basket Rebased on master
Created attachment 10886 [details] [review] Follow-up: Several fixes - List of libraries in basket.pl is now sorted by branch name, not code - When IndependantBranches is ON, user has only the possibility to set basket branch to its own branch, or to no branch at all. - When basket do not belong to any branch, selected branch by default is connection branch (was 'no branch') - added id attributes to both added li elements - change description of 'order_manage_all' permission to make it clearer. - remove Test::MockModule dependency
Hi Katrin. I believe that all your recommendations have been taken into account. At least I hope :)
Created attachment 11282 [details] [review] Bug 7295: More granular permissions for baskets Rebased on master
Created attachment 11283 [details] [review] Bug 7295: Follow-up: Several fixes Rebased on master
I am trying to test this patch but have a few questions. With superlibrarian permissions and AcqViewBasket set to "Show baskets in system, regardless of owner," I get a message in red telling me that I am not authorized to access BasketA. I tried it logged in from the branch that matches the basket, as well as logged in from a different branch, but I get the same message either way. I expected to be able to see and manage all baskets with these permissions... Am I missing something? Also, some of the acq permissions seem a little bit confusing to me: order_manage - Manage orders & basket order_receive - Manage orders & basket These two permissions have the exact same description. Do I need them both to be able to manage baskets? order_manage_all - Manage all orders and baskets, regardless of restrictions on them Would this one override the AcqViewBasket sys pref? For example, if I have the order_manage_all permission, but AcqViewBasket is set to "Show baskets from staff member's library," should I be able to see and manage ALL baskets, or only see and manage the baskets from my library? What if AcqViewBasket is set to "Show baskets in system, regardless of owner" but the order_manage_all permission is not checked? What should happen then?
If you are superlibrarian, or have the permissions 'order_manage' and 'order_manage_all' (or 'acquisition' if GranularPermissions syspref is not set), you do not need any other permission, and the value of AcqViewBaskets is not important. So you shouldn't see this message, but I'm not able to reproduce. Are you logged in with the mysql user? Did you run updatedatabase.pl? Can you check the log files while testing to see if something goes wrong? To clarify things a little bit: order_manage allows you to manage (create/modify/delete) baskets and orders order_receive allows you to receive these orders. Yes, order_manage_all override AcqViewBasket syspref. If ON, you will be able to manage all baskets. If OFF and AcqViewBasket syspref is set to "Show baskets in system...", you will be able to manage all baskets too.
Created attachment 13210 [details] [review] Bug 7295: More granular permissions for baskets Rebased on master
Created attachment 13211 [details] [review] Bug 7295: Follow-up: Several fixes Rebased on master
Created attachment 14515 [details] [review] Bug 7295: More granular permissions for baskets Rebased on master
Created attachment 14516 [details] [review] Bug 7295: Follow-up: Several fixes Rebased on master
Created attachment 16398 [details] [review] Bug 7295: More granular permissions for baskets Rebased patch
Created attachment 16399 [details] [review] Bug 7295: Follow-up: Several fixes Rebased patch
Patch tested with a sandbox, by Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 16474 [details] [review] Bug 8037 - Add holds and funds to items already received in parcel.pl Adds a column to indicate holds on recieved items, as well as adding a new column for fund and showing the subtotals per fund above the total subtotal. Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
comment 21 is not for this patch (8037)
Created attachment 16497 [details] [review] [SIGNED-OFF] Bug 7295: More granular permissions for baskets Add branch info to baskets Add a list of borrowers that are allowed to manage a basket (one list for each basket). Add a new subpermission: acquisition => order_manage_all If user is superlibrarian, or if he has permission acquisition = 1 (GranularPermissions = OFF), or subpermission acquisition => order_manage_all (GranularPermissions = ON). He's authorised to manage all baskets. Depending on syspref AcqViewBaskets: 'all': user can manage all baskets 'branch': user can manage baskets of his branch (the basket branch is taken into account, not the branch of the basket's creator). If basket branch is not defined, all user can manage this basket. 'user': user can manage baskets he created, and baskets he's in their user list There are unit tests in t/Acquisition/CanUserManageBasket.t, which require Test::MockModule You can edit basket's branch and users list in basket modification page (acqui/basket.pl) Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 16498 [details] [review] [SIGNED-OFF] Bug 7295: Follow-up: Several fixes - List of libraries in basket.pl is now sorted by branch name, not code - When IndependantBranches is ON, user has only the possibility to set basket branch to its own branch, or to no branch at all. - When basket do not belong to any branch, selected branch by default is connection branch (was 'no branch') - added id attributes to both added li elements - change description of 'order_manage_all' permission to make it clearer. - remove Test::MockModule dependency Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Julian, I am looking at this now and have the tests complaining - can you please provide a follow-up? * koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt FAIL forbidden patterns FAIL The patch introduces a forbidden pattern: simple-quote string (129) Thx!
Very small thing, also for a follow up: When adding basket managers there is a little note about permissions (+1 for that :) ), but the granular permissions preference it mentions has been removed from the codebase. We are now always using granular permissions by default. Only staff with superlibrarian or acquisitions permissions (or order_manage permission if granular permissions are enabled) are returned in the search results
Hi Julian, I have found 2 things that I think need to be fixed: 1) System preference AcqViewBaskets is set to "from staff member's library only". On master this means, that the user can only see baskets, that have been created by staff from his home library. With your patches applied the system preference is (I think) looking at the newly created branch column in the basket table. For older baskets they are all set to "no branch" - so all baskets in the system suddenly show up. 2) System preference AcqViewBaskets is set to "from staff member's library only". I have an order created at library A - on the managers list is a staff member from library B, but he can't see the order until I give permissions to manage _all_ orders. Should the manager's list override pref/permission? I am not sure about this, but maybe if it's not possible to overwrite we should not allow adding the user to the manager's list.
First : thanks Katrin for your "things". Your second question is a good one. For Koha Team Lyon 3, the managers should see the basket, so the manager's list override pref/permission. For us, we could change one's branch. According Pierre Angot, for a budget, we can already mix owner and library or user (Koha 3.10). We would like to test it but something went wrong. Secondly, problem on the sandbox : Something went wrong ! Applying: Bug 7295: More granular permissions for baskets Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 7295: More granular permissions for baskets When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Bug 7295 - list of librarian managing a basket
Created attachment 17358 [details] [review] Bug 7295 [follow-up] More granular permissions for baskets - updatedatabase.pl now fills the new aqbasket.branch with branchcode of basket owner (aqbasket.authorisedby). So the behavior is unchanged if you have set your syspref AcqViewBaskets to 'branch' - If AcqViewBaskets='branch', prevent librarian to add users that are not from the same branch than the basket.
Created attachment 17359 [details] [review] [SIGNED-OFF] Bug 7295: More granular permissions for baskets Rebased on master
Created attachment 17360 [details] [review] [SIGNED-OFF] Bug 7295: Follow-up: Several fixes Rebased on master
Created attachment 17361 [details] [review] Bug 7295 [follow-up] More granular permissions for baskets - updatedatabase.pl now fills the new aqbasket.branch with branchcode of basket owner (aqbasket.authorisedby). So the behavior is unchanged if you have set your syspref AcqViewBaskets to 'branch' - If AcqViewBaskets='branch', prevent librarian to add users that are not from the same branch than the basket.
This last patch should fix problems raised by Katrin.
Created attachment 17706 [details] [review] Bug 7295: More granular permissions for baskets Add branch info to baskets Add a list of borrowers that are allowed to manage a basket (one list for each basket). Add a new subpermission: acquisition => order_manage_all If user is superlibrarian, or if he has permission acquisition = 1 (GranularPermissions = OFF), or subpermission acquisition => order_manage_all (GranularPermissions = ON). He's authorised to manage all baskets. Depending on syspref AcqViewBaskets: 'all': user can manage all baskets 'branch': user can manage baskets of his branch (the basket branch is taken into account, not the branch of the basket's creator). If basket branch is not defined, all user can manage this basket. 'user': user can manage baskets he created, and baskets he's in their user list There are unit tests in t/Acquisition/CanUserManageBasket.t, which require Test::MockModule You can edit basket's branch and users list in basket modification page (acqui/basket.pl) Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 17707 [details] [review] Bug 7295: Follow-up: Several fixes - List of libraries in basket.pl is now sorted by branch name, not code - When IndependantBranches is ON, user has only the possibility to set basket branch to its own branch, or to no branch at all. - When basket do not belong to any branch, selected branch by default is connection branch (was 'no branch') - added id attributes to both added li elements - change description of 'order_manage_all' permission to make it clearer. - remove Test::MockModule dependency Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 17708 [details] [review] Bug 7295 [follow-up] More granular permissions for baskets - updatedatabase.pl now fills the new aqbasket.branch with branchcode of basket owner (aqbasket.authorisedby). So the behavior is unchanged if you have set your syspref AcqViewBaskets to 'branch' - If AcqViewBaskets='branch', prevent librarian to add users that are not from the same branch than the basket. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18338 [details] [review] [SIGNED-OFF] Bug 7295: More granular permissions for baskets Add branch info to baskets Add a list of borrowers that are allowed to manage a basket (one list for each basket). Add a new subpermission: acquisition => order_manage_all If user is superlibrarian, or if he has permission acquisition = 1 (GranularPermissions = OFF), or subpermission acquisition => order_manage_all (GranularPermissions = ON). He's authorised to manage all baskets. Depending on syspref AcqViewBaskets: 'all': user can manage all baskets 'branch': user can manage baskets of his branch (the basket branch is taken into account, not the branch of the basket's creator). If basket branch is not defined, all user can manage this basket. 'user': user can manage baskets he created, and baskets he's in their user list There are unit tests in t/Acquisition/CanUserManageBasket.t, which require Test::MockModule You can edit basket's branch and users list in basket modification page (acqui/basket.pl) Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18339 [details] [review] [SIGNED-OFF] Bug 7295: Follow-up: Several fixes - List of libraries in basket.pl is now sorted by branch name, not code - When IndependantBranches is ON, user has only the possibility to set basket branch to its own branch, or to no branch at all. - When basket do not belong to any branch, selected branch by default is connection branch (was 'no branch') - added id attributes to both added li elements - change description of 'order_manage_all' permission to make it clearer. - remove Test::MockModule dependency Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 18340 [details] [review] [SIGNED-OFF] Bug 7295 [follow-up] More granular permissions for baskets - updatedatabase.pl now fills the new aqbasket.branch with branchcode of basket owner (aqbasket.authorisedby). So the behavior is unchanged if you have set your syspref AcqViewBaskets to 'branch' - If AcqViewBaskets='branch', prevent librarian to add users that are not from the same branch than the basket. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19751 [details] [review] Bug 7295: More granular permissions for baskets Add branch info to baskets Add a list of borrowers that are allowed to manage a basket (one list for each basket). Add a new subpermission: acquisition => order_manage_all If user is superlibrarian, or if he has permission acquisition = 1 (GranularPermissions = OFF), or subpermission acquisition => order_manage_all (GranularPermissions = ON). He's authorised to manage all baskets. Depending on syspref AcqViewBaskets: 'all': user can manage all baskets 'branch': user can manage baskets of his branch (the basket branch is taken into account, not the branch of the basket's creator). If basket branch is not defined, all user can manage this basket. 'user': user can manage baskets he created, and baskets he's in their user list There are unit tests in t/Acquisition/CanUserManageBasket.t, which require Test::MockModule You can edit basket's branch and users list in basket modification page (acqui/basket.pl) Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19752 [details] [review] Bug 7295: Follow-up: Several fixes - List of libraries in basket.pl is now sorted by branch name, not code - When IndependantBranches is ON, user has only the possibility to set basket branch to its own branch, or to no branch at all. - When basket do not belong to any branch, selected branch by default is connection branch (was 'no branch') - added id attributes to both added li elements - change description of 'order_manage_all' permission to make it clearer. - remove Test::MockModule dependency Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19753 [details] [review] Bug 7295 [follow-up] More granular permissions for baskets - updatedatabase.pl now fills the new aqbasket.branch with branchcode of basket owner (aqbasket.authorisedby). So the behavior is unchanged if you have set your syspref AcqViewBaskets to 'branch' - If AcqViewBaskets='branch', prevent librarian to add users that are not from the same branch than the basket. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Rebased first patch for trivial updatedatabase conflict. This was already mentioned before (from koha-qa): FAIL koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt FAIL forbidden patterns forbidden pattern: simple-quote string (line 138) Failed QA
Created attachment 19754 [details] [review] Bug 7295 [follow-up] More granular permissions for baskets Replace simple-quoted string by double-quoted string
Created attachment 20769 [details] [review] Bug 7295: More granular permissions for baskets Add branch info to baskets Add a list of borrowers that are allowed to manage a basket (one list for each basket). Add a new subpermission: acquisition => order_manage_all If user is superlibrarian, or if he has permission acquisition = 1 (GranularPermissions = OFF), or subpermission acquisition => order_manage_all (GranularPermissions = ON). He's authorised to manage all baskets. Depending on syspref AcqViewBaskets: 'all': user can manage all baskets 'branch': user can manage baskets of his branch (the basket branch is taken into account, not the branch of the basket's creator). If basket branch is not defined, all user can manage this basket. 'user': user can manage baskets he created, and baskets he's in their user list There are unit tests in t/Acquisition/CanUserManageBasket.t, which require Test::MockModule You can edit basket's branch and users list in basket modification page (acqui/basket.pl) Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 20770 [details] [review] Bug 7295: Follow-up: Several fixes - List of libraries in basket.pl is now sorted by branch name, not code - When IndependantBranches is ON, user has only the possibility to set basket branch to its own branch, or to no branch at all. - When basket do not belong to any branch, selected branch by default is connection branch (was 'no branch') - added id attributes to both added li elements - change description of 'order_manage_all' permission to make it clearer. - remove Test::MockModule dependency Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 20771 [details] [review] Bug 7295 [follow-up] More granular permissions for baskets Replace simple-quoted string by double-quoted string Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Pushed to master. Thanks, Julian!