Closed Bug 279693 Opened 20 years ago Closed 20 years ago

Move UserCanBlessGroup() into a Bugzilla::User function

Categories

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

2.19.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(1 file, 3 obsolete files)

I'm going to take UserCanBlessGroup and make it into a real OO function in Bugzilla::User. It shouldn't be too hard.
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.
Attachment #172325 - Flags: review?
Status: NEW → ASSIGNED
Attachment #172325 - Flags: review? → review?(wurblzap)
Attachment #172325 - Flags: review?(wurblzap) → review?(LpSolit)
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-
One more thing. Your patch is bitrotten due to globals.pl.
Attached patch v2 (obsolete) — Splinter Review
Oh, hey, nice catch on both of those, actually. Those were both bugs. :-) It should work properly, now. :-)
Attachment #172325 - Attachment is obsolete: true
Attachment #175366 - Flags: review?(LpSolit)
Attached patch v3 (obsolete) — Splinter Review
Fixed a few things that LpSolit pointed out in IRC.
Attachment #175366 - Attachment is obsolete: true
Attachment #175382 - Flags: review?(LpSolit)
Attached patch v3.1Splinter Review
Attachment #175382 - Attachment is obsolete: true
Attachment #175384 - Flags: review?(LpSolit)
Attachment #175366 - Flags: review?(LpSolit)
Attachment #175382 - Flags: review?(LpSolit)
Comment on attachment 175384 [details] [diff] [review] v3.1 OK, now I agree. r=LpSolit
Attachment #175384 - Flags: review?(LpSolit) → review+
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Priority: -- → P3
Flags: approval? → approval+
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.

Attachment

General

Created:
Updated:
Size: