adds order_receive_all permission to recieve a shipment whatever the branch associated (order line branch ou basket branch) (BibLibre MT6528)
Created attachment 9314 [details] [review] More granular permissions for order receiving Add branch to aqorders. 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: - order branch is defined and is the same as the current working branch - order branch is not defined, basket branch is defined and basket branch is the same as the current working branch - order branch and basket branch are not defined Replace tablesorter by datatables in parcel.pl This patch needs patch for bug 7295 (for basket branch) Unit tests are available in t/Acquisition/CanUserReceiveBasket.t
Created attachment 9399 [details] [review] More granular permissions for order receiving Unit tests were missing from first patch
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
(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.
Created attachment 9944 [details] [review] More granular permissions for order receiving Patch rebased on master
Created attachment 11444 [details] [review] Bug 7290: More granular permissions for order receiving Add branch to aqorders. 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: - order branch is defined and is the same as the current working branch - order branch is not defined, basket branch is defined and basket branch is the same as the current working branch - order branch and basket branch are not defined Use dataTables filters possibilities instead of custom JS solution in parcel.pl This patch needs patch for bug 7295 (for basket branch) Unit tests are available in t/Acquisition/CanUserReceiveBasket.t -- patch rebased on master
Created attachment 13214 [details] [review] Bug 7290: More granular permissions for order receiving Rebased on master
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
Created attachment 14517 [details] [review] Bug 7290: More granular permissions for order receiving Rebased on master
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
(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.
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".
(In reply to I'm just a bot from comment #12) Depend on bug 7295
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Created attachment 25068 [details] [review] Bug 7290: More granular permissions for order receiving Add branch to aqorders. 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: - order branch is defined and is the same as the current working branch - order branch is not defined, basket branch is defined and basket branch is the same as the current working branch - order branch and basket branch are not defined Use dataTables filters possibilities instead of custom JS solution in parcel.pl Unit tests are available in t/Acquisition/CanUserReceiveBasket.t
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.
Created attachment 26669 [details] [review] Bug 7290: More granular permissions for order receiving Rebased on master
Created attachment 26671 [details] [review] Bug 7290: Fix constraint name in updatedatabase.pl Set the same constraint name as in kohastructure.sql
A problem in updatedatabase.pl made it failed to insert the new column aqorders.branch. That's why the branch was not saved.
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?
Hi Katrin, thanks for pointing the discussion. I will remove code that add this column and will use basket branch instead.
Created attachment 26676 [details] [review] Bug 7290: Use basket branch instead of order line branch and remove aqorders.branch from updatedatabase.pl and kohastructure.sql
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
Created attachment 28386 [details] [review] Bug 7290: More granular permissions for order receiving Rebased on master
Created attachment 28387 [details] [review] Bug 7290: Fix constraint name in updatedatabase.pl Rebased on master
Created attachment 28388 [details] [review] Bug 7290: Use basket branch instead of order line branch Rebased on master
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.
Created attachment 28405 [details] [review] More granular permissions for order receiving
Created attachment 28406 [details] [review] Fix constraint name in updatedatabase.pl
Created attachment 28407 [details] [review] Use basket branch instead of order line branch
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.
Created attachment 30337 [details] [review] Bug 7290: More granular permissions for order receiving Rebased on master
Created attachment 30338 [details] [review] Bug 7290: Fix constraint name in updatedatabase.pl Rebased on master
Created attachment 30339 [details] [review] Bug 7290: Use basket branch instead of order line branch Rebased on master
Created attachment 30340 [details] [review] Bug 7290: QA fixes 1) Use the new isSuperlibrarian instead of $userflags->{superlibrarian} 2) Update the database update comment 3) Fix capitalization of "Duplicate Barcode" 4) Add comments in t/Acquisition/CanUserReceiveBasket.t
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.
Created attachment 30926 [details] [review] Bug 7290: More granular permissions for order receiving 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, or - 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 Signed-off-by: Paola Rossi <paola.rossi@cineca.it> (squash of the 3 first patches, rebased on master)
Created attachment 30927 [details] [review] Bug 7290: QA fixes Rebased on master
Created attachment 30928 [details] [review] Bug 7290: Fix CanUserReceiveBasket tests CanUserReceiveBasket now verify the 'superlibrarian' status of user using $userenv->{flags} (IsSuperLibrarian)
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.
Created attachment 32074 [details] [review] Bug 7290: More granular permissions for order receiving Rebased on master
Created attachment 32075 [details] [review] Bug 7290: QA fixes Rebased on master
Created attachment 32076 [details] [review] Bug 7290: Fix CanUserReceiveBasket tests Rebased on master
Created attachment 32077 [details] [review] Bug 7290: Add new permission to users that have 'order_receive'
(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 ?
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.
Created attachment 35700 [details] [review] Bug 7290: More granular permissions for order receiving Rebased on master and squashed with 4th patch
Created attachment 35701 [details] [review] Bug 7290: QA fixes 1) Use the new isSuperlibrarian instead of $userflags->{superlibrarian} 2) Fix capitalization of "Duplicate Barcode" 3) Add comments in t/Acquisition/CanUserReceiveBasket.t
Created attachment 35702 [details] [review] Bug 7290: Fix CanUserReceiveBasket tests Rebased on master
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.
Created attachment 35936 [details] [review] Bug 7290: QA fixes
Created attachment 35937 [details] [review] Bug 7290: Fix CanUserReceiveBasket tests
Sorry. patch no longer applies and conflicts are not trivial - please fix!
My branch was not on current master... sorry! Switching back to signed off, continuing work on this later.
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>
... please also fix the databaes update to say XXX - it will make testing this easier!
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.
Another small problem found: Being superlibrarian I still can't access the receive page, but I think I should be able to.
(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.
Hi Julian, I noticed this is one of 2 remaining bugs on bug 7295 - do you intend to work on it again?
No movement for years and no response to Katrin's question from several years ago, so moving to "WISHLIST".