Bug 10258 - Remove erroneous call to haspermission in basket.pl
Summary: Remove erroneous call to haspermission in basket.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Galen Charlton
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-16 09:58 UTC by Marcel de Rooy
Modified: 2014-12-07 20:02 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10258: Remove erroneous call to haspermission in basket.pl (1.75 KB, patch)
2013-05-16 10:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10258: Remove erroneous call to haspermission in basket.pl (1.76 KB, patch)
2013-06-03 01:21 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[ALT] Bug 10354: Remove erroneous call to haspermission in basket.pl (1.20 KB, patch)
2013-06-07 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10258: Remove erroneous call to haspermission in basket.pl (1.82 KB, patch)
2013-06-11 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10258: fix permissions check for setting basket group for order basket (6.36 KB, patch)
2013-06-13 16:16 UTC, Galen Charlton
Details | Diff | Splinter Review
screenshot of dialog described in comment (11.22 KB, image/png)
2013-06-16 15:02 UTC, Katrin Fischer
Details
Bug 10258: fix permissions check for setting basket group for order basket (6.36 KB, patch)
2013-06-17 01:14 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10258: offer to create basket group only when staff user has correct permission (2.25 KB, patch)
2013-06-17 01:14 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10258: fix permissions check for setting basket group for order basket (6.46 KB, patch)
2013-07-15 11:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10258: offer to create basket group only when staff user has correct permission (2.34 KB, patch)
2013-07-15 11:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-05-16 09:58:33 UTC
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.
Comment 1 Marcel de Rooy 2013-05-16 10:11:01 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-06-03 01:21:01 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-06-07 12:35:21 UTC
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'} )) {
Comment 4 Jonathan Druart 2013-06-07 12:37:36 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2013-06-07 14:57:38 UTC
(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.
Comment 6 Jonathan Druart 2013-06-11 15:18:08 UTC
QA comment:

This patch comments some useless lines of code. No side effect possible, but an issue still exists.

Marked as Passed QA.
Comment 7 Jonathan Druart 2013-06-11 15:18:26 UTC Comment hidden (obsolete)
Comment 8 Galen Charlton 2013-06-13 16:16:34 UTC Comment hidden (obsolete)
Comment 9 Galen Charlton 2013-06-13 16:19:13 UTC
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.
Comment 10 Marcel de Rooy 2013-06-13 16:36:26 UTC
Back to Needs signoff
Comment 11 Katrin Fischer 2013-06-16 11:46:02 UTC
Looking at this now :)
Comment 12 Katrin Fischer 2013-06-16 15:02:52 UTC
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.
Comment 13 Galen Charlton 2013-06-17 01:14:30 UTC Comment hidden (obsolete)
Comment 14 Galen Charlton 2013-06-17 01:14:46 UTC Comment hidden (obsolete)
Comment 15 Galen Charlton 2013-06-17 01:15:22 UTC
(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.
Comment 16 Marcel de Rooy 2013-07-15 11:59:04 UTC
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>
Comment 17 Marcel de Rooy 2013-07-15 11:59:09 UTC
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>
Comment 18 Marcel de Rooy 2013-07-15 12:00:48 UTC
Looks good to me. Thanks for improvements, Galen.
No complaints from koha-qa.

Passed QA
Comment 19 Galen Charlton 2013-07-15 16:15:51 UTC
Pushed to master.
Comment 20 Tomás Cohen Arazi 2013-07-25 15:13:10 UTC
This patch has been pushed to 3.12.x, will be in 3.12.3.

Thanks Galen!
Comment 21 Bernardo Gonzalez Kriegel 2013-08-04 22:55:48 UTC
Pushed to 3.10.x, will be in 3.10.10