Description
Owen Leonard
2016-04-08 18:22:48 UTC
Created attachment 50089 [details] [review] Bug 16231 - Correct permission handling in subscription edit menu This patch corrects the way subscription edit menu items are displayed according to the user's permissions settings. The edit menu has been updated to use the menu item terms "Edit," "Duplicate," and "Delete" instead of "Edit subscription," "Edit as new (duplicate)", and "Delete subscription. The change makes the terms less redundant and more consistent with other similar menus in Koha. Font Awesome icons have been added to each menu item. This patch also removes a non-standard "fa-lg" class from some Font Awesome icons in the toolbar. To test, apply the patch and log in to the staff client as a user who has permission to create, edit, and delete subscriptions. - Locate an subscription and view the details for it. - The "Edit" menu should show three options: "Edit," "Duplicate," and "Delete." - Confirm that each menu item works correctly. - Repeat the process when logged in as a user with varying combinations of create, edit, and delete permissions. Created attachment 50195 [details] [review] Bug 16231 - Correct permission handling in subscription edit menu This patch corrects the way subscription edit menu items are displayed according to the user's permissions settings. The edit menu has been updated to use the menu item terms "Edit," "Duplicate," and "Delete" instead of "Edit subscription," "Edit as new (duplicate)", and "Delete subscription. The change makes the terms less redundant and more consistent with other similar menus in Koha. Font Awesome icons have been added to each menu item. This patch also removes a non-standard "fa-lg" class from some Font Awesome icons in the toolbar. To test, apply the patch and log in to the staff client as a user who has permission to create, edit, and delete subscriptions. - Locate an subscription and view the details for it. - The "Edit" menu should show three options: "Edit," "Duplicate," and "Delete." - Confirm that each menu item works correctly. - Repeat the process when logged in as a user with varying combinations of create, edit, and delete permissions. Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Comment on attachment 50195 [details] [review] Bug 16231 - Correct permission handling in subscription edit menu Review of attachment 50195 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/includes/serials-toolbar.inc @@ -50,4 @@ > <div class="btn-group"> > <button class="btn btn-small dropdown-toggle" data-toggle="dropdown"><i class="fa fa-pencil"></i> Edit <span class="caret"></span></button> > <ul class="dropdown-menu"> > - [% IF ( cannotedit ) %] <li class="disabled"> [% ELSE %] If you follow 'cannotedit' through the code, you can see that the permission check is quite a complicated one here: sub _can_do_on_subscription It also checks superserials and IndependentBranches. (In reply to Katrin Fischer from comment #3) > If you follow 'cannotedit' through the code That whole menu is wrapped in "[% UNLESS ( cannotedit ) %]," so I concluded that the checks on "cannotedit" inside the menu were redundant. Does that sound correct? I'm returning this to "signed off" because I think the QA issue is actually okay. I hope someone can take another look and let me know. Thx Owen, sorry I didn't come back to this yet. Hi Owen, started to test the permissions - especially looking at: - edit_subscription - delete_subscription - create_subscription When the user has only delete_subscription out of those three it appears that the option is not visible. Could you take a look please? Created attachment 50639 [details] [review] Bug 16231 [Follow-up] Correct permission handling in subscription edit menu This follow-up allows for the case where the user has only "delete_subscription" permission. The delete option might be available even if the template's "cannotedit" variable is true. To test, apply the patch and revise a staff user's permission to have only the "delete_subscription" permission under Serials. Log in as that users and locate a subscription and view the details for that subscription. You should see a toolbar on that page containing only a "Delete" button. Repeat the steps above, adding serials permission to the user one by one, confirming that the toolbar continues to show the correct options. Created attachment 50711 [details] [review] Bug 16231 [Follow-up] Correct permission handling in subscription edit menu This follow-up allows for the case where the user has only "delete_subscription" permission. The delete option might be available even if the template's "cannotedit" variable is true. To test, apply the patch and revise a staff user's permission to have only the "delete_subscription" permission under Serials. Log in as that users and locate a subscription and view the details for that subscription. You should see a toolbar on that page containing only a "Delete" button. Repeat the steps above, adding serials permission to the user one by one, confirming that the toolbar continues to show the correct options. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> (In reply to Owen Leonard from comment #8) > This follow-up allows for the case where the user has only > "delete_subscription" permission. The delete option might be > available even if the template's "cannotedit" variable is true. I don't understand why, could you please detail? I have not tested the patch, but looking at the diff it seems that it's a behavior change. Maybe my first test here was flawed. This is something I see retesting this: - Set independentbranches to prevent - Make sure staff patron doesn't have superserials permission = I should not be allowed to change serials from other branches - Go to another branches subscription (by URL manipulation) - With the patches I can: renew (opens wrong window?), receive (ew!) and DELETE - Without the patch on master, I can't delete (better) I'd say this is a change of the behaviour that we don't want - allowing deletions. But overall... permission handling in serials could still be improved a lot. Can we find a good fix for delete here and then see about the other problems later? I think if you don't have edit_subscription, but delete an 'cannotedit = 0 (can edit)' - then maybe show a single 'delete' button? Created attachment 79904 [details] [review] Bug 16231: Correct permission handling in subscription edit This patch corrects the way subscription edit menu items are displayed according to the user's permissions settings. To test, apply the patch and log in to the staff client as a user who has permission to create, edit, and delete subscriptions. - Locate an subscription and view the details for it. - Confirm that each "Edit" menu item works correctly. - Repeat the process when logged in as a user with varying combinations of create, edit, and delete permissions. - Test as a user limited by IndependentBranches. As far as I can tell you cannot delete a subscription without "edit" permission, despite the separate permissions. I think it has to do with out the "cannotedit" variable is defined. Created attachment 82336 [details] [review] Bug 16231: Correct permission handling in subscription edit This patch corrects the way subscription edit menu items are displayed according to the user's permissions settings. To test, apply the patch and log in to the staff client as a user who has permission to create, edit, and delete subscriptions. - Locate an subscription and view the details for it. - Confirm that each "Edit" menu item works correctly. - Repeat the process when logged in as a user with varying combinations of create, edit, and delete permissions. - Test as a user limited by IndependentBranches. Created attachment 83833 [details] [review] Bug 16231: Correct permission handling in subscription edit This patch corrects the way subscription edit menu items are displayed according to the user's permissions settings. To test, apply the patch and log in to the staff client as a user who has permission to create, edit, and delete subscriptions. - Locate an subscription and view the details for it. - Confirm that each "Edit" menu item works correctly. - Repeat the process when logged in as a user with varying combinations of create, edit, and delete permissions. - Test as a user limited by IndependentBranches. Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Created attachment 84680 [details] [review] Bug 16231: Correct permission handling in subscription edit This patch corrects the way subscription edit menu items are displayed according to the user's permissions settings. To test, apply the patch and log in to the staff client as a user who has permission to create, edit, and delete subscriptions. - Locate an subscription and view the details for it. - Confirm that each "Edit" menu item works correctly. - Repeat the process when logged in as a user with varying combinations of create, edit, and delete permissions. - Test as a user limited by IndependentBranches. Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.03 backported to 18.05.x for 18.05.09 (In reply to Owen Leonard from comment #14) > As far as I can tell you cannot delete a subscription without "edit" > permission, despite the separate permissions. I think it has to do with out > the "cannotedit" variable is defined. Not sure this is correct, in subscription-detail.pl: 52 # Permission needed if it is a deletion (del) : delete_subscription 53 # Permission needed otherwise : * 54 my $permission = ($op eq "del") ? "delete_subscription" : "*"; Not a big deal I'd say, but maybe a minor bug (?) |