Bug 7290

Summary: New permission for receiving all (independent of library)
Product: Koha Reporter: claire.hernandez <claire.hernandez>
Component: AcquisitionsAssignee: Julian Maurice <julian.maurice>
Status: RESOLVED WISHLIST QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, frederic.durand, fridolin.somers, gitbot, jonathan.druart, katrin.fischer, koha, paola.rossi, stephane.delaye
Version: master   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7295    
Bug Blocks: 7163, 8667    
Attachments: More granular permissions for order receiving
More granular permissions for order receiving
More granular permissions for order receiving
Bug 7290: More granular permissions for order receiving
Bug 7290: More granular permissions for order receiving
Bug 7290: More granular permissions for order receiving
Bug 7290: More granular permissions for order receiving
Bug 7290: More granular permissions for order receiving
Bug 7290: Fix constraint name in updatedatabase.pl
Bug 7290: Use basket branch instead of order line branch
Bug 7290: More granular permissions for order receiving
Bug 7290: Fix constraint name in updatedatabase.pl
Bug 7290: Use basket branch instead of order line branch
More granular permissions for order receiving
Fix constraint name in updatedatabase.pl
Use basket branch instead of order line branch
Bug 7290: More granular permissions for order receiving
Bug 7290: Fix constraint name in updatedatabase.pl
Bug 7290: Use basket branch instead of order line branch
Bug 7290: QA fixes
Bug 7290: More granular permissions for order receiving
Bug 7290: QA fixes
Bug 7290: Fix CanUserReceiveBasket tests
Bug 7290: More granular permissions for order receiving
Bug 7290: QA fixes
Bug 7290: Fix CanUserReceiveBasket tests
Bug 7290: Add new permission to users that have 'order_receive'
Bug 7290: More granular permissions for order receiving
Bug 7290: QA fixes
Bug 7290: Fix CanUserReceiveBasket tests
Bug 7290: More granular permissions for order receiving
Bug 7290: QA fixes
Bug 7290: Fix CanUserReceiveBasket tests

Description claire.hernandez@biblibre.com 2011-12-02 09:24:21 UTC
adds order_receive_all permission to recieve a shipment whatever the branch associated (order line branch ou basket branch) (BibLibre MT6528)
Comment 1 Julian Maurice 2012-04-27 15:02:33 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-05-03 12:59:01 UTC Comment hidden (obsolete)
Comment 3 delaye 2012-05-24 11:47:53 UTC
i can't test in a sandbox

Applying: Bug 7290: More granular permissions for order receiving
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7290: More granular permissions for order receiving
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 7290 - new permission for recieving
Comment 4 Julian Maurice 2012-06-06 08:21:48 UTC
(In reply to comment #3)
> i can't test in a sandbox

No, this patch cannot be tested on sandboxes because it depends on another patch that is not in master (bug 7295).

But both patches (for bug 7295 and this bug) doesn't apply anymore on master. I will rebase and resubmit them.
Comment 5 Julian Maurice 2012-06-06 08:42:11 UTC Comment hidden (obsolete)
Comment 6 Julian Maurice 2012-08-08 12:53:58 UTC Comment hidden (obsolete)
Comment 7 Julian Maurice 2012-11-02 12:31:28 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-01-10 08:30:37 UTC
Applying: Bug 7290: More granular permissions for order receiving
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7290: More granular permissions for order receiving
Comment 9 Julian Maurice 2013-01-10 09:13:48 UTC Comment hidden (obsolete)
Comment 10 Frederic Durand 2013-03-18 11:46:26 UTC
I ve tried it on a sandbox, here s the error message :
Applying: Bug 7290: More granular permissions for order receiving
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7290: More granular permissions for order receiving
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 7290 - new permission for recieving
Comment 11 Fridolin Somers 2013-03-18 16:18:49 UTC
(In reply to comment #10)
> I ve tried it on a sandbox, here s the error message :
> Applying: Bug 7290: More granular permissions for order receiving
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 7290: More granular permissions for order receiving
> 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 7290 - new permission for recieving

This is because patch depends on Bug 7295.
Can not be tester in a sandbox.
Comment 12 I'm just a bot 2013-09-18 11:06:38 UTC
Applying: Bug 7290: More granular permissions for order receiving
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7290: More granular permissions for order receiving
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 13 Jonathan Druart 2013-09-18 12:40:31 UTC
(In reply to I'm just a bot from comment #12)
Depend on bug 7295
Comment 14 Jonathan Druart 2014-01-15 12:05:10 UTC
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Comment 15 Julian Maurice 2014-02-06 08:29:42 UTC Comment hidden (obsolete)
Comment 16 Koha Team University Lyon 3 2014-03-12 15:25:28 UTC
I want to test but the branch isn't saved into the order. I can save the order but if I modify it, there's no branch defined anymore.
Comment 17 Julian Maurice 2014-03-28 09:11:27 UTC Comment hidden (obsolete)
Comment 18 Julian Maurice 2014-03-28 09:11:34 UTC Comment hidden (obsolete)
Comment 19 Julian Maurice 2014-03-28 09:13:23 UTC
A problem in updatedatabase.pl made it failed to insert the new column aqorders.branch. That's why the branch was not saved.
Comment 20 Katrin Fischer 2014-03-28 09:20:15 UTC
Hi Julian, there has been a lot of discussion about an aqorders.branch column, see bug 11001. I am not sure, which was the conclusion right now, but it seems that it was dropped - comment 7. Why not use the branch from the staff patron who created the order?
Comment 21 Julian Maurice 2014-03-28 09:57:57 UTC
Hi Katrin, thanks for pointing the discussion.
I will remove code that add this column and will use basket branch instead.
Comment 22 Julian Maurice 2014-03-28 09:58:22 UTC Comment hidden (obsolete)
Comment 23 Paola Rossi 2014-05-21 08:51:58 UTC
I try to apply against master 3.15.00.052

Applying: Bug 7290: More granular permissions for order receiving
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/parcel.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
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/kohastructure.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
Auto-merging acqui/orderreceive.pl
Auto-merging acqui/neworderempty.pl
Auto-merging acqui/finishreceive.pl
Auto-merging C4/Acquisition.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 7290: More granular permissions for order receiving
Comment 24 Julian Maurice 2014-05-21 09:12:27 UTC Comment hidden (obsolete)
Comment 25 Julian Maurice 2014-05-21 09:12:38 UTC Comment hidden (obsolete)
Comment 26 Julian Maurice 2014-05-21 09:12:48 UTC Comment hidden (obsolete)
Comment 27 Paola Rossi 2014-05-21 13:59:49 UTC
I test against master 3.15.00.052

Using basket branch instead of order line branch, I've modified the test plan to the following one:

Add subpermission acquisition => order_receive_all

Users with order_receive_all can receive all orders.
Users with only order_receive can receive an order if:
  - basket branch is defined and is the same as the current working
    branch
  - basket branch is not defined

Use dataTables filters possibilities instead of custom JS solution in parcel.pl

Unit tests are available in t/Acquisition/CanUserReceiveBasket.t
--------------------------------------------
Everything is OK against the new test plan.
So I pass the patch to "Signed Off" status.
Comment 28 Paola Rossi 2014-05-21 14:02:07 UTC Comment hidden (obsolete)
Comment 29 Paola Rossi 2014-05-21 14:02:56 UTC Comment hidden (obsolete)
Comment 30 Paola Rossi 2014-05-21 14:03:28 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2014-07-06 21:14:29 UTC
I took a look at the code (at all 3 patches together) and noticed some small things:

1) Plase use the new isSuperlibrarian instead:
unless ($userflags->{superlibrarian}

2) Please update the database update comment:
+    print "Upgrade to $DBversion done (Add aqorders.branch and permission order_receive_all)\n";

3) Capitalization
I know it's an old line, but could you please fix?
+                    Duplicate Barcode

4) The new test file (thx!) doesn't output any comments. It would be nice to see some more documentation there.
Comment 32 Julian Maurice 2014-07-30 14:35:30 UTC Comment hidden (obsolete)
Comment 33 Julian Maurice 2014-07-30 14:35:43 UTC Comment hidden (obsolete)
Comment 34 Julian Maurice 2014-07-30 14:35:58 UTC Comment hidden (obsolete)
Comment 35 Julian Maurice 2014-07-30 14:36:11 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2014-08-18 12:07:17 UTC
Hi Julian,

I fixed the conflict in the database update and a tab char in parcels.tt, but now I got stuck with the tests failing:

 $ prove t/Acquisition/CanUserReceiveBasket.t
t/Acquisition/CanUserReceiveBasket.t .. C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
t/Acquisition/CanUserReceiveBasket.t .. 1/18 C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 46.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 47.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 48.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 54.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 64.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 65.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 66.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.

#   Failed test at t/Acquisition/CanUserReceiveBasket.t line 71.
C4::Context->userenv not defined! at /home/katrin/kohaclone/C4/Acquisition.pm line 870.
# Looks like you failed 8 tests of 18.
t/Acquisition/CanUserReceiveBasket.t .. Dubious, test returned 8 (wstat 2048, 0x800)
Failed 8/18 subtests 

Test Summary Report
-------------------
t/Acquisition/CanUserReceiveBasket.t (Wstat: 2048 Tests: 18 Failed: 8)
  Failed tests:  4-6, 9, 13-15, 17
  Non-zero exit status: 8

Could you also take a look at squashing these patches? Because of the chanes in the database structure (we started with adding a branch column to aqorders that was later removed from the patches) I think it could make sense here. Also if I saw correctly all patches are from you and signed only by Paola.
Comment 37 Julian Maurice 2014-08-18 13:20:53 UTC Comment hidden (obsolete)
Comment 38 Julian Maurice 2014-08-18 13:21:04 UTC Comment hidden (obsolete)
Comment 39 Julian Maurice 2014-08-18 13:21:13 UTC Comment hidden (obsolete)
Comment 40 Katrin Fischer 2014-08-18 21:50:27 UTC
Hi Julian, 

I have been thinking about this and have 2 things:

1) (normal) It takes away permissions from the current staff users. I think in order to stick to our best practice of not changing behaviour, we need to update staff users with order_receive permission to also have the order_receive_all permission.

2) Wondeirng, should I still be able to delete or move an order, that I am not allowed to receive? I guess it's intentional, but it feels a bit like taking away the nice one and leaving the dangerous actions :)

I tested with 2 baskets with 1 order line each. One basket belonging to my home library and the other to another libray. My staff user didn't have the order_receive_all permission.
Comment 41 Julian Maurice 2014-10-09 08:16:37 UTC Comment hidden (obsolete)
Comment 42 Julian Maurice 2014-10-09 08:16:52 UTC Comment hidden (obsolete)
Comment 43 Julian Maurice 2014-10-09 08:17:03 UTC Comment hidden (obsolete)
Comment 44 Julian Maurice 2014-10-09 08:17:10 UTC Comment hidden (obsolete)
Comment 45 Julian Maurice 2014-10-09 08:19:44 UTC
(In reply to Katrin Fischer from comment #40)
> 2) Wondeirng, should I still be able to delete or move an order, that I am
> not allowed to receive? I guess it's intentional, but it feels a bit like
> taking away the nice one and leaving the dangerous actions :)

Yes, you should still be able to delete or move an order you can't receive, but that's not a different behaviour than without the patch, right ?
Comment 46 Paola Rossi 2014-10-15 09:48:03 UTC
I've tried to apply the patches against master '3.17.00.027'

The 1-st patch :
 Bug 7290: More granular permissions for order receiving 
needed a trivial rebase about modifying updatedatabase.pl.

The 4-th patch tries to modify updatedatabase.pl again.

Would you squash all in the 1-st patch, and cancel the 4-th patch?
Is it possible and allowed?

Thanks in advance. 
I pass the patch to "Patch doesn't apply" status for now anyway.
Comment 47 Julian Maurice 2015-02-06 14:30:14 UTC Comment hidden (obsolete)
Comment 48 Julian Maurice 2015-02-06 14:30:23 UTC Comment hidden (obsolete)
Comment 49 Julian Maurice 2015-02-06 14:30:34 UTC Comment hidden (obsolete)
Comment 50 Paola Rossi 2015-02-16 14:53:30 UTC
Created attachment 35935 [details] [review]
Bug 7290: More granular permissions for order receiving

I've applied the 3 patches against master 3.19.00.011 head 13603.

The first patch needed a trivial rebase.

Upgrading the DB, for all the patrons with "order_receive" permission ON the "order_receive_all" permission had been set to ON.

Everything is OK as required by the test plan (see comment #37).

I pass the patch to "Signed Off" status.
Comment 51 Paola Rossi 2015-02-16 14:53:55 UTC
Created attachment 35936 [details] [review]
Bug 7290: QA fixes
Comment 52 Paola Rossi 2015-02-16 14:54:22 UTC
Created attachment 35937 [details] [review]
Bug 7290: Fix CanUserReceiveBasket tests
Comment 53 Katrin Fischer 2015-03-22 21:46:39 UTC
Sorry. patch no longer applies and conflicts are not trivial - please fix!
Comment 54 Katrin Fischer 2015-03-22 21:50:40 UTC
My branch was not on current master... sorry! 
Switching back to signed off, continuing work on this later.
Comment 55 Katrin Fischer 2015-03-22 22:42:08 UTC
Some first findings:

Can you please amend your tests to have some explanations?
1..18
ok 1
ok 2
...

... and while you are on it, fix the little tab that crept into 
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt?
+    <h3>Error receiving items:</h3>
Comment 56 Katrin Fischer 2015-03-22 22:43:34 UTC
... please also fix the databaes update to say XXX - it will make testing this easier!
Comment 57 Katrin Fischer 2015-03-23 07:01:20 UTC
Hm, some thoughts on permission management in acq for the parcel list:

- I have removed all _all permissions from my staff patron
- I have set AcqViewBaskets to 'created or managed by staff member'

I can't access a basket now, that belongs to another library and was also created by another staff user than me.

But: On the parcel page, I can still see all the information about the orders. I can't receive them (good!), but I can still delete and transfer the orders (not good!)

Thinking about big libraries and the need to hide information from others, I feel like there should be a combination of permissions here, that results in me not seeing the order lines of other libraries at all. 

I'd expect the combination above should do it, but it doesn't.

When I also remove the 'order_manage' permission, I get a permission error on clicking 'transfer' and 'delete' - but the links shouldn't be showing in the first place.

I think the current situation is not very good here :(

The feature itself seems to be working ok, only found a small bug so far:
Manipulating the URL to gain access to an order I cannot receive it displays a nice error message, but there is a small error in the heading on the page:

Receive items from : Books Books Books [HASH(0x530f938)] (order #138)

Waiting for the follow-ups and giving the permission problem some more thought.
Comment 58 Katrin Fischer 2015-03-23 07:03:03 UTC
Another small problem found:

Being superlibrarian I still can't access the receive page, but I think I should be able to.
Comment 59 Katrin Fischer 2015-03-23 07:04:09 UTC
(In reply to Katrin Fischer from comment #58)
> Another small problem found:
> 
> Being superlibrarian I still can't access the receive page, but I think I
> should be able to.

Scratch that - logging out and logging in again fixed it. I can access the receive page as superlibrairan now.
Comment 60 Katrin Fischer 2019-02-04 23:06:59 UTC
Hi Julian, I noticed this is one of 2 remaining bugs on bug 7295 - do you intend to work on it again?
Comment 61 David Cook 2022-08-22 03:34:53 UTC
No movement for years and no response to Katrin's question from several years ago, so moving to "WISHLIST".