Current code in basket.pl contains an invalid call of haspermission: if ($basket->{closedate} && haspermission({ acquisition => 'group_manage'} )) { The first parameter should be userid, the second should be flagsrequired. Looking at the code, it is useless to just insert the first parameter here via $member->{userid}, since this would result in an empty basket group list. To resolve this, more should be done in the template. Currently, the erroneous call results in getting back some hash probably filled with zeroes. But the hash makes the if test above prove true for a closed basket. So in reality, any user will see here all basket groups. Removing this erroneous call for haspermission will therefore not make a difference. The proposed patch adds a FIXME.
Created attachment 18166 [details] [review] Bug 10258: Remove erroneous call to haspermission in basket.pl First parameter (userid) was not passed making the test useless. Removed the test for now. Current behavior does not change. Just adding the first parameter here does not make sense. Using the permission here needs more than that. Test plan: Open a closed basket in Acquisition. Check if you see all available basketgroups in the combo at the right side.
Created attachment 18592 [details] [review] Bug 10258: Remove erroneous call to haspermission in basket.pl First parameter (userid) was not passed making the test useless. Removed the test for now. Current behavior does not change. Just adding the first parameter here does not make sense. Using the permission here needs more than that. Test plan: Open a closed basket in Acquisition. Check if you see all available basketgroups in the combo at the right side. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Marcel, Does the following code fix the problem? - my $member = GetMember(borrowernumber => $loggedinuser); - if ($basket->{closedate} && haspermission({ acquisition => 'group_manage'} )) { + my $member = GetMember(borrowernumber => $loggedinuser)->{userid}; + if ($basket->{closedate} && haspermission($uid, { acquisition => 'group_manage'} )) {
Created attachment 18736 [details] [review] [ALT] Bug 10354: Remove erroneous call to haspermission in basket.pl
(In reply to comment #3) > Marcel, Does the following code fix the problem? > > - my $member = GetMember(borrowernumber => $loggedinuser); > - if ($basket->{closedate} && haspermission({ acquisition => > 'group_manage'} )) { > + my $member = GetMember(borrowernumber => $loggedinuser)->{userid}; > + if ($basket->{closedate} && haspermission($uid, { acquisition => > 'group_manage'} )) { Hi Jonathan, What I meant with adding the userid is not the solution, is what you propose here. I think that if you do this, you need to change the template too. With your patch, a user without this permission sees the button Change group and in the combo Add new group. If he clicks the button, he gets a login form warning him about permissions. I would not opt for that. Perhaps it would be better to remove the combo and button in this case; more friendly. Can I obsolete your patch now and let you do this in a qa followup? Thanks.
QA comment: This patch comments some useless lines of code. No side effect possible, but an issue still exists. Marked as Passed QA.
Created attachment 18853 [details] [review] Bug 10258: Remove erroneous call to haspermission in basket.pl First parameter (userid) was not passed making the test useless. Removed the test for now. Current behavior does not change. Just adding the first parameter here does not make sense. Using the permission here needs more than that. Test plan: Open a closed basket in Acquisition. Check if you see all available basketgroups in the combo at the right side. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 18944 [details] [review] Bug 10258: fix permissions check for setting basket group for order basket Improve the code that displays and allows staff to set the basket group from the basket details page for a closed basket. Prior to this patch, a staff member who did not have the group_manage acquisition permission would still see a control to change the group that the basket belongs to; attempting to change the group would present with with a login page. This patch also does some tidying of how basket group details are passed to the template. To test: [1] Create an order basket and close it. Do not assign it to a basket group. [2] View the basket details while logged in as a staff user who has the order_manage acquisitions permission but not the group_manage. The displayed basket group should be "No group". [3] Switch to a staff user who also has the group_manage permission, then view the basket details again. The basket group field should now be a select input that allows you to change the basket group. [4] Change the basket group. Verify that the basket group you selected is now displayed as the current group for that order basket. The basket group delivery and billing place fields should also now be displayed. [5] Close the basket group set in the previous step, then view the basket details again. This time, the basket group name should be displayed with a suffix of " (closed)", and no input to change the group should be displayed. [6] Swith to a staff user who does not have the group_manage permission, view the basket details, and verify that the basket name is displayed with a suffix of " (closed)". Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Rather than push a patch that adds FIXMEs, I've attached a counter-patch that is a more complete fix for the problem. Setting to in discussion; if the patch submitter agrees with the general idea, let's take the counter-patch through the normal signoff/QA process.
Back to Needs signoff
Looking at this now :)
Created attachment 19039 [details] screenshot of dialog described in comment I think we are almost there, but there is one little thing I am still worried about: When I close the basket with the user that has not group_manage permission, I still get offered the yellow dialog on closing the basket that asks me if I want to create a basket group with the same name as the order and put my order in it. If you check the checkbox and save you go to an error page: Error: You do not have permission to access this page. Click to log out... I think ideally we should not have this happen, but remove that bit from the dialog.
Created attachment 19053 [details] [review] Bug 10258: fix permissions check for setting basket group for order basket Improve the code that displays and allows staff to set the basket group from the basket details page for a closed basket. Prior to this patch, a staff member who did not have the group_manage acquisition permission would still see a control to change the group that the basket belongs to; attempting to change the group would present with with a login page. This patch also does some tidying of how basket group details are passed to the template. To test: [1] Create an order basket and close it. Do not assign it to a basket group. [2] View the basket details while logged in as a staff user who has the order_manage acquisitions permission but not the group_manage. The displayed basket group should be "No group". [3] Switch to a staff user who also has the group_manage permission, then view the basket details again. The basket group field should now be a select input that allows you to change the basket group. [4] Change the basket group. Verify that the basket group you selected is now displayed as the current group for that order basket. The basket group delivery and billing place fields should also now be displayed. [5] Close the basket group set in the previous step, then view the basket details again. This time, the basket group name should be displayed with a suffix of " (closed)", and no input to change the group should be displayed. [6] Swith to a staff user who does not have the group_manage permission, view the basket details, and verify that the basket name is displayed with a suffix of " (closed)". Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 19054 [details] [review] Bug 10258: offer to create basket group only when staff user has correct permission If the staff user does not have the group_manage acquisition permission, do not offer to create a new basket group when closing an order basket. This avoids a situation where if a staff member without that permission tries to close a basket and chose the option to create a bakset group, they would be redirected to the login page. To test: [1] Log in as a staff user that does not have the acquisition/group_manage permission. [2] Create a new order basket, attach at least one order line to it, then close it. [3] Verify that the confirmation page does not offer to create a basket group with the same name as the order basket. [4] Log in as a staff user that has the acquisition/group_manage permission. [5] Create and close an order basket. [6] Verify that this time, the confirmation page *does* offer to create a basket group. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
(In reply to comment #12) > Created attachment 19039 [details] > screenshot of dialog described in comment > > I think we are almost there, but there is one little thing I am still > worried about: When I close the basket with the user that has not > group_manage permission, I still get offered the yellow dialog on closing > the basket that asks me if I want to create a basket group with the same > name as the order and put my order in it. If you check the checkbox and save > you go to an error page: Error: You do not have permission to access this > page. Click to log out... > I think ideally we should not have this happen, but remove that bit from the > dialog. I've added a follow-up patch to do exactly that.
Created attachment 19648 [details] [review] Bug 10258: fix permissions check for setting basket group for order basket Improve the code that displays and allows staff to set the basket group from the basket details page for a closed basket. Prior to this patch, a staff member who did not have the group_manage acquisition permission would still see a control to change the group that the basket belongs to; attempting to change the group would present with with a login page. This patch also does some tidying of how basket group details are passed to the template. To test: [1] Create an order basket and close it. Do not assign it to a basket group. [2] View the basket details while logged in as a staff user who has the order_manage acquisitions permission but not the group_manage. The displayed basket group should be "No group". [3] Switch to a staff user who also has the group_manage permission, then view the basket details again. The basket group field should now be a select input that allows you to change the basket group. [4] Change the basket group. Verify that the basket group you selected is now displayed as the current group for that order basket. The basket group delivery and billing place fields should also now be displayed. [5] Close the basket group set in the previous step, then view the basket details again. This time, the basket group name should be displayed with a suffix of " (closed)", and no input to change the group should be displayed. [6] Swith to a staff user who does not have the group_manage permission, view the basket details, and verify that the basket name is displayed with a suffix of " (closed)". Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 19649 [details] [review] Bug 10258: offer to create basket group only when staff user has correct permission If the staff user does not have the group_manage acquisition permission, do not offer to create a new basket group when closing an order basket. This avoids a situation where if a staff member without that permission tries to close a basket and chose the option to create a bakset group, they would be redirected to the login page. To test: [1] Log in as a staff user that does not have the acquisition/group_manage permission. [2] Create a new order basket, attach at least one order line to it, then close it. [3] Verify that the confirmation page does not offer to create a basket group with the same name as the order basket. [4] Log in as a staff user that has the acquisition/group_manage permission. [5] Create and close an order basket. [6] Verify that this time, the confirmation page *does* offer to create a basket group. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me. Thanks for improvements, Galen. No complaints from koha-qa. Passed QA
Pushed to master.
This patch has been pushed to 3.12.x, will be in 3.12.3. Thanks Galen!
Pushed to 3.10.x, will be in 3.10.10