Bug 7295 - list of librarian managing a basket
Summary: list of librarian managing a basket
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 7163 7290 7292 18756
  Show dependency treegraph
 
Reported: 2011-12-02 09:35 UTC by claire.hernandez@biblibre.com
Modified: 2017-07-06 17:21 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
More granular permissions for baskets (42.48 KB, patch)
2012-04-26 15:38 UTC, Julian Maurice
Details | Diff | Splinter Review
More granular permissions for baskets (42.48 KB, patch)
2012-06-06 08:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Attachment to Bug 7295 - list of librarian managing a basket (42.46 KB, patch)
2012-06-11 15:22 UTC, Michael Davis
Details | Diff | Splinter Review
Bug 7295 - list of librarian managing a basket (42.16 KB, patch)
2012-07-17 09:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up: Several fixes (17.52 KB, patch)
2012-07-17 09:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: More granular permissions for baskets (42.40 KB, patch)
2012-08-02 07:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: Follow-up: Several fixes (17.52 KB, patch)
2012-08-02 07:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: More granular permissions for baskets (41.73 KB, patch)
2012-11-02 12:28 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: Follow-up: Several fixes (17.51 KB, patch)
2012-11-02 12:28 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: More granular permissions for baskets (41.71 KB, patch)
2013-01-10 09:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: Follow-up: Several fixes (17.50 KB, patch)
2013-01-10 09:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: More granular permissions for baskets (41.76 KB, patch)
2013-03-19 14:26 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 7295: Follow-up: Several fixes (17.51 KB, patch)
2013-03-19 14:27 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.15 KB, patch)
2013-03-20 10:11 UTC, Paul Poulain
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7295: More granular permissions for baskets (41.77 KB, patch)
2013-03-20 11:20 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7295: Follow-up: Several fixes (17.56 KB, patch)
2013-03-20 11:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295 [follow-up] More granular permissions for baskets (7.12 KB, patch)
2013-04-11 10:17 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7295: More granular permissions for baskets (41.73 KB, patch)
2013-04-11 10:18 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7295: Follow-up: Several fixes (17.56 KB, patch)
2013-04-11 10:19 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295 [follow-up] More granular permissions for baskets (7.12 KB, patch)
2013-04-11 10:19 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: More granular permissions for baskets (41.78 KB, patch)
2013-04-26 13:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7295: Follow-up: Several fixes (17.61 KB, patch)
2013-04-26 13:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7295 [follow-up] More granular permissions for baskets (7.17 KB, patch)
2013-04-26 13:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7295: More granular permissions for baskets (41.80 KB, patch)
2013-05-23 09:47 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7295: Follow-up: Several fixes (17.61 KB, patch)
2013-05-23 09:47 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7295 [follow-up] More granular permissions for baskets (7.17 KB, patch)
2013-05-23 09:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: More granular permissions for baskets (42.07 KB, patch)
2013-07-18 08:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7295: Follow-up: Several fixes (17.61 KB, patch)
2013-07-18 08:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7295 [follow-up] More granular permissions for baskets (7.18 KB, patch)
2013-07-18 08:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7295 [follow-up] More granular permissions for baskets (1.11 KB, patch)
2013-07-18 09:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7295: More granular permissions for baskets (41.81 KB, patch)
2013-09-04 09:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7295: Follow-up: Several fixes (17.61 KB, patch)
2013-09-04 09:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7295 [follow-up] More granular permissions for baskets (1.16 KB, patch)
2013-09-04 09:18 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description claire.hernandez@biblibre.com 2011-12-02 09:35:34 UTC
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)
Comment 1 Julian Maurice 2012-04-26 15:38:38 UTC Comment hidden (obsolete)
Comment 2 Koha Team University Lyon 3 2012-05-25 10:38:51 UTC
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
Comment 3 Julian Maurice 2012-06-04 09:04:57 UTC
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.
Comment 4 Julian Maurice 2012-06-06 08:41:31 UTC Comment hidden (obsolete)
Comment 5 Michael Davis 2012-06-11 15:22:44 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2012-06-17 22:27:36 UTC
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.
Comment 7 Julian Maurice 2012-07-17 09:11:58 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2012-07-17 09:12:42 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2012-07-17 09:16:06 UTC
Hi Katrin.

I believe that all your recommendations have been taken into account. At least I hope :)
Comment 10 Julian Maurice 2012-08-02 07:37:17 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2012-08-02 07:37:42 UTC Comment hidden (obsolete)
Comment 12 Melia Meggs 2012-08-17 22:14:09 UTC
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?
Comment 13 Julian Maurice 2012-08-20 08:35:14 UTC
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.
Comment 14 Julian Maurice 2012-11-02 12:28:33 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2012-11-02 12:28:50 UTC Comment hidden (obsolete)
Comment 16 Julian Maurice 2013-01-10 09:11:56 UTC Comment hidden (obsolete)
Comment 17 Julian Maurice 2013-01-10 09:12:32 UTC Comment hidden (obsolete)
Comment 18 Fridolin Somers 2013-03-19 14:26:21 UTC Comment hidden (obsolete)
Comment 19 Fridolin Somers 2013-03-19 14:27:20 UTC Comment hidden (obsolete)
Comment 20 Paul Poulain 2013-03-20 10:10:59 UTC
Patch tested with a sandbox, by Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Comment 21 Paul Poulain 2013-03-20 10:11:18 UTC Comment hidden (obsolete)
Comment 22 Koha Team University Lyon 3 2013-03-20 10:24:21 UTC
comment 21 is not for this patch (8037)
Comment 23 Julian Maurice 2013-03-20 11:20:32 UTC Comment hidden (obsolete)
Comment 24 Julian Maurice 2013-03-20 11:20:44 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2013-03-20 14:44:44 UTC
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!
Comment 26 Katrin Fischer 2013-03-20 15:22:41 UTC
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
Comment 27 Katrin Fischer 2013-03-20 15:43:22 UTC
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.
Comment 28 Koha Team AMU 2013-03-20 16:51:51 UTC
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
Comment 29 Julian Maurice 2013-04-11 10:17:39 UTC Comment hidden (obsolete)
Comment 30 Julian Maurice 2013-04-11 10:18:42 UTC Comment hidden (obsolete)
Comment 31 Julian Maurice 2013-04-11 10:19:01 UTC Comment hidden (obsolete)
Comment 32 Julian Maurice 2013-04-11 10:19:09 UTC Comment hidden (obsolete)
Comment 33 Julian Maurice 2013-04-11 10:20:20 UTC
This last patch should fix problems raised by Katrin.
Comment 34 Kyle M Hall 2013-04-26 13:38:25 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2013-04-26 13:38:35 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2013-04-26 13:38:42 UTC Comment hidden (obsolete)
Comment 37 Julian Maurice 2013-05-23 09:47:38 UTC Comment hidden (obsolete)
Comment 38 Julian Maurice 2013-05-23 09:47:59 UTC Comment hidden (obsolete)
Comment 39 Julian Maurice 2013-05-23 09:48:10 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2013-07-18 08:53:46 UTC Comment hidden (obsolete)
Comment 41 Marcel de Rooy 2013-07-18 08:53:51 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2013-07-18 08:53:55 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2013-07-18 08:55:50 UTC
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
Comment 44 Julian Maurice 2013-07-18 09:05:21 UTC Comment hidden (obsolete)
Comment 45 Chris Cormack 2013-09-04 09:18:16 UTC
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>
Comment 46 Chris Cormack 2013-09-04 09:18:30 UTC
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>
Comment 47 Chris Cormack 2013-09-04 09:18:47 UTC
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>
Comment 48 Galen Charlton 2013-10-31 16:43:37 UTC
Pushed to master.  Thanks, Julian!