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

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
User Accounts
--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

3.1.4
Bugzilla 3.4
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

v3
5.87 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Right now bless_groups returns a hash, and it should be returning Bugzilla::Group objects instead.
(Assignee)

Comment 1

10 years ago
Created attachment 326897 [details] [diff] [review]
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...
Assignee: user-accounts → mkanat
Status: NEW → ASSIGNED
Attachment #326897 - Flags: review?(LpSolit)
(Assignee)

Updated

10 years ago
Blocks: 442013

Comment 2

9 years ago
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.
Attachment #326897 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 3

9 years ago
(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.
(Assignee)

Comment 4

9 years ago
Created attachment 332649 [details] [diff] [review]
v2
Attachment #326897 - Attachment is obsolete: true
Attachment #332649 - Flags: review?(LpSolit)

Comment 5

9 years ago
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
Attachment #332649 - Flags: review?(LpSolit) → review+

Updated

9 years ago
Flags: approval+

Comment 6

9 years ago
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.
(Assignee)

Comment 7

9 years ago
Created attachment 333740 [details] [diff] [review]
v3

You were right about sql_in. It required enough changes that I think it should be reviewed again.
Attachment #332649 - Attachment is obsolete: true
Attachment #333740 - Flags: review?(LpSolit)

Comment 8

9 years ago
Comment on attachment 333740 [details] [diff] [review]
v3

Looks good. r=LpSolit
Attachment #333740 - Flags: review?(LpSolit) → review+
(Assignee)

Comment 9

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 3.4

Updated

9 years ago
Blocks: 451449
(Assignee)

Updated

9 years ago
Blocks: 460379
(Assignee)

Comment 10

9 years ago
Added to the release notes for Bugzilla 3.4 in bug 494037.
You need to log in before you can comment on or make changes to this bug.