Bug 7292

Summary: New permission for claims
Product: Koha Reporter: claire.hernandez <claire.hernandez>
Component: AcquisitionsAssignee: Julian Maurice <julian.maurice>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: chris, dcook, gitbot, jonathan.druart, julian.maurice, katrin.fischer, koha, martin.renvoize, paola.rossi
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7295    
Bug Blocks: 7163    
Attachments: More granular permissions for claiming orders
Bug 7292: More granular permissions for claiming orders
Bug 7292: More granular permissions for claiming orders
Bug 7292: More granular permissions for claiming orders
Bug 7292: More granular permissions for claiming orders
Bug 7292: New permission for claims (database changes only)
Bug 7292: More granular permissions for claiming orders
[PASSED QA] Bug 7292: New permission for claims (database changes only)
[PASSED QA] Bug 7292: More granular permissions for claiming orders
Bug 7292: German translation for new 'claim for all' permission
Bug 7292: Add new permission to users that have 'order_receive' permission
Bug 7292: New permission for claims (database changes only)
Bug 7292: More granular permissions for claiming orders
Bug 7292: Add new permission to users that have 'order_receive' permission
Bug 7292: German translation for new 'claim for all' permission
New permission for claims (database changes only)
More granular permissions for claiming orders
Add new permission to users that have 'order_receive' permission
German translation for new 'claim for all' permission
Bug 7292: New permission for claims (database changes only)
Bug 7292: More granular permissions for claiming orders
Bug 7292: Add new permission to users that have 'order_receive' permission
Bug 7292: German translation for new 'claim for all' permission
Bug 7292: Use IsSuperLibrarian() instead of $userflags->{superlibrarian}

Description claire.hernandez@biblibre.com 2011-12-02 09:31:17 UTC
"new permission for claims: the librarian can have a permission ""claim for all"". Without this permission, he can claim only on basket he is a ""user"" of
" (BibLibre MT6542)
Comment 1 Julian Maurice 2012-05-16 13:25:49 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-05-16 13:31:44 UTC
To test:
  - Apply patch for bug 7295 first and apply this patch
  - Run the updatedatabase.pl script
  - Go to acqui/lateorders.pl
Checkboxes in last column must display only if you are an authorised user of the order's basket (creator, user, or connected to same branch as the basket branch, see bug 7295 for more details)
  - Grant permission 'order_claim_for_all' to your user and go back to acqui/lateorders.pl
You must be able to claim for all displayed orders.

Without GranularPermissions sysprefs, 'acquisition' permission should allow you to claim for all orders
Comment 3 Katrin Fischer 2012-05-16 16:11:52 UTC
Hi Julian,
I think this is a similar situation as we have for baskets. For baskets you can currently set globally if
- you can see only your own baskets
- all baskets of your branch
- all baskets in the system

So I was thinking if we should have somthing similar for claiming. It might make sense to have an option to only be able to claim orders of your own library/branch.
Comment 4 Julian Maurice 2012-05-18 07:29:39 UTC
Hi Katrin,

Actually, this patch prevent users to claim on baskets they're not authorised to manage. So it take into account the AcqViewBaskets system preference:
 - If 'all', user can claim for all orders
 - If 'branch', user can claim for basket of his branch
 - If 'user', user can claim for baskets he own or he's a 'user' of

I think we don't need another system preference here.
Comment 5 Katrin Fischer 2012-05-18 07:38:01 UTC
Ok. :)
Comment 6 Katrin Fischer 2012-05-18 07:40:32 UTC
Hm, thinking about the possible cases:

If the user can not manage baskets at all or can only manage basket he/she created and has the permission to claim all order - he/she will always be able to claim all baskets from all branches, is that right?

And I like that this is a permission, not a system preference :)
Comment 7 Julian Maurice 2012-05-18 10:11:30 UTC
(In reply to comment #6)
> Hm, thinking about the possible cases:
> 
> If the user can not manage baskets at all or can only manage basket he/she
> created and has the permission to claim all order - he/she will always be
> able to claim all baskets from all branches, is that right?

That's right, 'order_claim_for_all' is like a 'superlibrarian' permission for claims.
Comment 8 Chris Cormack 2012-07-15 09:44:52 UTC
Applying: Bug 7292: More granular permissions for claiming orders
fatal: sha1 information is lacking or useless (installer/data/mysql/de-DE/mandatory/userpermissions.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 9 Julian Maurice 2012-07-18 14:04:55 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2012-08-02 07:12:33 UTC
Same error with this one:
Applying: Bug 7292: More granular permissions for claiming orders
fatal: sha1 information is lacking or useless (installer/data/mysql/de-DE/mandatory/userpermissions.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge
Comment 11 Julian Maurice 2012-08-02 07:40:38 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2013-01-10 09:18:33 UTC Comment hidden (obsolete)
Comment 13 I'm just a bot 2013-09-18 11:07:08 UTC
Applying: Bug 7292: More granular permissions for claiming orders
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7292: More granular permissions for claiming orders
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 14 Julian Maurice 2013-09-23 10:20:37 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2013-09-23 10:21:18 UTC
Patch recreated on master
Comment 16 Jonathan Druart 2014-01-15 12:06:46 UTC
fatal: sha1 information is lacking or useless (installer/data/mysql/de-DE/mandatory/userpermissions.sql).

Please provide 1 file for DB changes and 1 file (at least) for others changes.
Comment 17 Julian Maurice 2014-02-06 14:30:21 UTC Comment hidden (obsolete)
Comment 18 Julian Maurice 2014-02-06 14:30:24 UTC Comment hidden (obsolete)
Comment 19 Julian Maurice 2014-02-06 14:31:57 UTC
Patches split and rebased on master
Comment 20 Koha Team University Lyon 3 2014-03-12 13:25:32 UTC
I try first with a user who haven't the permission "order_claim_for_all Claim for all orders". On the table of Late orders,the user have checkbox in front of the order line from basket that he've created or that he was added in the list of users authorized for management of this basket. He can see the other line order but he can't check it for claims.

I change the permissions to add "order_claim_for_all Claim for all orders". The users can check all the order, even those he haven't created or he doesn't manage.

Ok for me.
Comment 21 Katrin Fischer 2014-03-14 14:33:19 UTC
It might be silly, but I am wondering a bit about the code for the new permission - order_claims_for_all, we have so far:
budget_manage_all
order_manage_all
claim_serials
So I think claim_order_all or order_claims_all would have been a bit more consistent. ... But I am going to continue with QA :)
Comment 22 Katrin Fischer 2014-03-14 15:35:23 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2014-03-14 15:35:27 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2014-03-14 15:35:31 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2014-03-14 15:44:11 UTC
Hi Julian,

I had passed it, but then it occured to me, that introducing this new permission will mean taking away permissions for existing users as we don't update those. Could you make sure that we set the permission for existing staff users in the database update? I think from what I see in the template that would be all users having order_receive permission?
Comment 26 Julian Maurice 2014-03-28 10:20:47 UTC Comment hidden (obsolete)
Comment 27 Julian Maurice 2014-03-28 10:23:11 UTC
You're right Katrin,
this patch modify updatedatabase.pl to add automatically the new permission.
Comment 28 Paola Rossi 2014-05-21 09:21:01 UTC
I try to apply against master 3.15.00.052
I think that the patches need to be rebased.

I could resolve the CONFLICT on the first patch:
Applying: Bug 7292: New permission for claims (database changes only)
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/uk-UA/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/ru-RU/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/userpermissions.sql
Auto-merging installer/data/mysql/it-IT/necessari/userpermissions.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/userpermissions.sql
Auto-merging installer/data/mysql/es-ES/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/de-DE/mandatory/userpermissions.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 7292: New permission for claims (database changes only)

Anyway, the second patch doesn't apply:
Applying: Bug 7292: More granular permissions for claiming orders
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt

I pass the patches to "Patch doesn't apply" status.
Comment 29 Julian Maurice 2014-05-23 12:10:11 UTC Comment hidden (obsolete)
Comment 30 Julian Maurice 2014-05-23 12:10:26 UTC Comment hidden (obsolete)
Comment 31 Julian Maurice 2014-05-23 12:10:44 UTC Comment hidden (obsolete)
Comment 32 Julian Maurice 2014-05-23 12:11:37 UTC Comment hidden (obsolete)
Comment 33 Paola Rossi 2014-05-27 12:15:06 UTC
I test against master 3.17.00.007

The patch adds the new permission to users that have 'order_receive' permission: OK.
updatedatabase.pl needs to be rebased.

The test is OK.

I pass the patch to "Signed Off" status.
Comment 34 Paola Rossi 2014-05-27 12:17:19 UTC Comment hidden (obsolete)
Comment 35 Paola Rossi 2014-05-27 12:17:56 UTC Comment hidden (obsolete)
Comment 36 Paola Rossi 2014-05-27 12:18:50 UTC Comment hidden (obsolete)
Comment 37 Paola Rossi 2014-05-27 12:19:18 UTC Comment hidden (obsolete)
Comment 38 Katrin Fischer 2014-07-06 21:17:44 UTC
Same here as for the other permission, could you please update to use the new superlibrarian check? 

+if ($userflags->{superlibrarian}
Comment 39 Julian Maurice 2014-07-30 14:46:48 UTC
Created attachment 30341 [details] [review]
Bug 7292: New permission for claims (database changes only)

Rebased on master
Comment 40 Julian Maurice 2014-07-30 14:47:02 UTC
Created attachment 30342 [details] [review]
Bug 7292: More granular permissions for claiming orders

Rebased on master
Comment 41 Julian Maurice 2014-07-30 14:47:15 UTC
Created attachment 30343 [details] [review]
Bug 7292: Add new permission to users that have 'order_receive' permission

Rebased on master
Comment 42 Julian Maurice 2014-07-30 14:47:31 UTC
Created attachment 30344 [details] [review]
Bug 7292: German translation for new 'claim for all' permission

Rebased on master
Comment 43 Julian Maurice 2014-07-30 14:47:41 UTC
Created attachment 30345 [details] [review]
Bug 7292: Use IsSuperLibrarian() instead of $userflags->{superlibrarian}
Comment 44 Katrin Fischer 2014-08-18 10:58:05 UTC
Just a note from code review:

-        $template->{VARS}->{'info_claim'} = 1;
+            $template->param(info_claim => 1);

The first line was correct (TT), the second is still working (HTP), but outdated.
Comment 45 Katrin Fischer 2014-08-18 11:32:59 UTC
The database update works very nicely, seeting the permissions. But when I tried retesting the feature itself, I have run into problems:

- AcqViewBaskets: created or managed by staff user
- Staff patron doesn't have claim_for_all permission
- aqorders.branch != staff patron home branch
- aqorders.authorisedby != staff patron borrowernumber
- Tried logging in and out after making the changes.

... and it still shows up in the late orders list. What am I missing?
Comment 46 Julian Maurice 2014-10-09 09:38:46 UTC
Patron should also not have the permission order_manage_all and should not be in the basket's users list.
Maybe we should not check for order_manage_all here. What do you think ?
Comment 47 Martin Renvoize 2020-02-24 16:29:34 UTC
This still seems a nice to have... but needs an extensive rebase to apply
Comment 48 David Cook 2022-08-22 03:35:51 UTC
No movement for years and no response to Katrin's question from several years ago (on bug 7290 or bug 7163), so moving to "WISHLIST".
Comment 49 Katrin Fischer 2022-08-22 06:17:57 UTC
I'd still like to have this - but resetting to new as we probably need to start fresh.