442016 - Bugzilla::User::bless_groups should be returning Bugzilla::Group objects

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Max Kanat-Alexander]

Right now bless_groups returns a hash, and it should be returning Bugzilla::Group objects instead.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

The only functionality difference is that this is now using visible_groups_as_string instead of visible_groups_direct. If a group is visible, it's visible. We don't need to limit it to direct grants. I don't even know why we're checking that, really, since visibility and can_bless should be entirely separate--if I check bless but not visibility, I still expect the user to be able to bless...

Comment 2

[Frédéric Buclin]

Comment on attachment 326897 [details] [diff] [review]
v1

>Index: Bugzilla/User.pm

>+        'SELECT DISTINCT groups.id
>+           FROM groups, user_group_map, group_group_map AS ggm

Nit: shouldn't this syntax be changed to use INNER JOIN?


>+                         AND ggm.grant_type = ' . GROUP_BLESS . '

IMO, it's better to have GROUP_BLESS outside the query and use a placeholder instead.


>+        $query .= " AND groups.id in (" . $self->visible_groups_as_string . ")";

s/in/IN/. Also, $self->visible_groups_as_string may be empty, in which case you get IN (), which fails. That's the reason of my r-.


Otherwise, it looks good.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> >+        'SELECT DISTINCT groups.id
> >+           FROM groups, user_group_map, group_group_map AS ggm
> 
> Nit: shouldn't this syntax be changed to use INNER JOIN?

  No, not necessary.

> >+                         AND ggm.grant_type = ' . GROUP_BLESS . '
> 
> IMO, it's better to have GROUP_BLESS outside the query and use a placeholder
> instead.

  No, for the query cache/planner, it's better to paste it in directly.

  New patch coming up.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Comment 5

[Frédéric Buclin]

Comment on attachment 332649 [details] [diff] [review]
v2

>Index: Bugzilla/User.pm

>+                         AND ggm.member_id IN(' . $self->groups_as_string 

Nit: the reason you don't use $dbh->sql_in() is because you don't expect more than 1000 groups, right?


>+        $query .= " AND groups.id in (" . $self->visible_groups_as_string . ")";

Nit: "in" should be uppercase.


>+that you need to be able to see a a group in order to bless it.

Nit: "a a group"


I didn't test your patch, but it looks good, and I agree with the change from _direct to _inherited. r=LpSolit

Comment 6

[Frédéric Buclin]

Comment on attachment 332649 [details] [diff] [review]
v2

>Index: Bugzilla/User.pm

>+    return grep($_->id eq $group_id, @{ $self->bless_groups }) ? 1 : 0;

One more thing to fix: replace 'eq' by '==' as you are comparing integers.

Comment 7

[Max Kanat-Alexander]

Attachment: v3

You were right about sql_in. It required enough changes that I think it should be reviewed again.

Comment 8

[Frédéric Buclin]

Comment on attachment 333740 [details] [diff] [review]
v3

Looks good. r=LpSolit

Comment 9

[Max Kanat-Alexander]

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.148; previous revision: 1.147
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.121; previous revision: 1.120
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.170; previous revision: 1.169
done

Comment 10

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.