Closed
Bug 279693
Opened 20 years ago
Closed 20 years ago
Move UserCanBlessGroup() into a Bugzilla::User function
Categories
(Bugzilla :: Bugzilla-General, enhancement, P3)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: mkanat, Assigned: mkanat)
References
Details
Attachments
(1 file, 3 obsolete files)
7.86 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
I'm going to take UserCanBlessGroup and make it into a real OO function in
Bugzilla::User. It shouldn't be too hard.
Assignee | ||
Comment 1•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #172325 -
Flags: review?
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #172325 -
Flags: review? → review?(wurblzap)
Assignee | ||
Updated•20 years ago
|
Attachment #172325 -
Flags: review?(wurblzap) → review?(LpSolit)
![]() |
||
Comment 2•20 years ago
|
||
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+. :)
Attachment #172325 -
Flags: review?(LpSolit) → review-
![]() |
||
Comment 3•20 years ago
|
||
One more thing. Your patch is bitrotten due to globals.pl.
Assignee | ||
Comment 4•20 years ago
|
||
Oh, hey, nice catch on both of those, actually. Those were both bugs. :-)
It should work properly, now. :-)
Assignee | ||
Updated•20 years ago
|
Attachment #172325 -
Attachment is obsolete: true
Attachment #175366 -
Flags: review?(LpSolit)
Assignee | ||
Comment 5•20 years ago
|
||
Fixed a few things that LpSolit pointed out in IRC.
Attachment #175366 -
Attachment is obsolete: true
Attachment #175382 -
Flags: review?(LpSolit)
Assignee | ||
Comment 6•20 years ago
|
||
Attachment #175382 -
Attachment is obsolete: true
Attachment #175384 -
Flags: review?(LpSolit)
Assignee | ||
Updated•20 years ago
|
Attachment #175366 -
Flags: review?(LpSolit)
Assignee | ||
Updated•20 years ago
|
Attachment #175382 -
Flags: review?(LpSolit)
![]() |
||
Comment 7•20 years ago
|
||
Comment on attachment 175384 [details] [diff] [review]
v3.1
OK, now I agree. r=LpSolit
Attachment #175384 -
Flags: review?(LpSolit) → review+
![]() |
||
Updated•20 years ago
|
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Assignee | ||
Updated•20 years ago
|
Priority: -- → P3
Updated•20 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 8•20 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•