279693 - Move UserCanBlessGroup() into a Bugzilla::User function

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P3)

Description

[Max Kanat-Alexander]

I'm going to take UserCanBlessGroup and make it into a real OO function in
Bugzilla::User. It shouldn't be too hard.

Comment 1

[Max Kanat-Alexander]

Attachment: Refactor UserCanBlessGroup into Bugzilla::User

OK! I did a lot of testing on this to make sure that it works properly. It has
many advantages:

(1) It's much more OO.
(2) It now works just like Bugzilla->user->groups does, which is cool.
(3) It only uses one SQL statement, instead of two.

Comment 2

[Frédéric Buclin]

Comment on attachment 172325 [details] [diff] [review]
Refactor UserCanBlessGroup into Bugzilla::User

>+sub bless_groups {
>+    my $self = shift;
>+
>+    return $self->{bless_groups} if defined $self->{bless_groups};
>+    return {} unless $self->id;
>+
>+    my $dbh = Bugzilla->dbh;
>+    # Get groups for which the user has direct bless privs, or
>+    # for which they have inherited bless privs.
>+    my $bless_groups = $dbh->selectcol_arrayref(
>+        q{SELECT DISTINCT groups.name, groups.id
>+            FROM groups, user_group_map, group_group_map
>+           WHERE user_group_map.user_id = ?
>+             AND user_group_map.isbless = 1
>+             AND (groups.id=user_group_map.group_id
>+                  OR (groups.id = grantor_id
>+                      AND group_group_map.grant_type = } . GROUP_BLESS .
>+                   q{ AND user_group_map.group_id = member_id))},
>+         { Columns=>[1,2] },
>+         $self->{id});

If the user has no direct bless privs but only inherited bless privs, your SQL
request returns no result due to "user_group_map.isbless = 1". The reason you
missed that point in editusers.cgi is because the script checks:
UserInGroup("editusers") || Bugzilla->user->can_bless($name) so that if the
first test is successful, the second one is not checked.

From what I see, a more correct SQL request would be:

>+        q{SELECT DISTINCT groups.name, groups.id
>+            FROM groups, user_group_map, group_group_map
>+           WHERE user_group_map.user_id = ?
>+             AND ((user_group_map.isbless = 1
>+                   AND groups.id=user_group_map.group_id)
>+                  OR (groups.id = grantor_id
>+                      AND group_group_map.grant_type = } . GROUP_BLESS .
>+                   q{ AND user_group_map.group_id = member_id))},

The only thing I changed here is parens. Please check that the JOIN with
group_group_map is still valid when the first test in parens is true, as we
don't need to look at group_group_map in this case. I think we get a cartesian
product, but that should not matter.


>+    if (!scalar(@_)) {
>+        # If we're called eithout an argument, just return 
>+        # whether or not we can bless at all.
>+        return scalar($self->bless_groups);
>     }

What does scalar($self->bless_groups) return? If $self->bless_groups is empty
or not, or the number of groups the user can bless? If it is the number of
groups, then your description is wrong as you mention that it returns either 0
or 1.


If you correct parens above and give me a short answer about the last question,
I'm ok for a r+. :)

Comment 3

[Frédéric Buclin]

One more thing. Your patch is bitrotten due to globals.pl.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Oh, hey, nice catch on both of those, actually. Those were both bugs. :-)

It should work properly, now. :-)

Comment 5

[Max Kanat-Alexander]

Attachment: v3

Fixed a few things that LpSolit pointed out in IRC.

Comment 6

[Max Kanat-Alexander]

Attachment: v3.1

Comment 7

[Frédéric Buclin]

Comment on attachment 175384 [details] [diff] [review]
v3.1

OK, now I agree. r=LpSolit

Comment 8

[Max Kanat-Alexander]

There was teeny-tiny bitrot: My name had already been added to User.pm before
this patch. :-) So I just left out that part of the patch when applying it. :-)

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.78; previous revision: 1.77
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.307; previous revision: 1.306
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.39; previous revision: 1.38
done