Closed
Bug 284264
Opened 19 years ago
Closed 19 years ago
Move canSeeUser from editusers.cgi to Bugzilla::User.pm
Categories
(Bugzilla :: Administration, task, P3)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.22
People
(Reporter: Wurblzap, Assigned: Wurblzap)
References
Details
Attachments
(2 files, 4 obsolete files)
8.80 KB,
patch
|
Wurblzap
:
review+
|
Details | Diff | Splinter Review |
620 bytes,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
(In reply to bug 119485, comment #36) > >+ canSeeUser($otherUserID) > > Nit: That really should be a method of Bugzilla::User, I'd think.
Assignee | ||
Comment 1•19 years ago
|
||
Moving visibleGroupsAsString along with it, because it's needed by canSeeUser (now can_see_user), and it makes sense in User.pm, too.
Attachment #179861 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Comment 2•19 years ago
|
||
I'm going to review this now, but it's really the sort of change that I think we should hold until 2.22.
Severity: normal → enhancement
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.22
Comment 3•19 years ago
|
||
Comment on attachment 179861 [details] [diff] [review] Patch >+sub can_see_user { >+ my ($self, $otherUserID) = @_; >+ my $query; >+ >+ if (Param('usevisibilitygroups')) { >+ my $visibleGroups = $self->visible_groups_as_string(); >+ $query = qq{SELECT COUNT(DISTINCT userid) >+ FROM profiles, user_group_map >+ WHERE userid = ? >+ AND user_id = userid >+ AND isbless = 0 >+ AND group_id IN ($visibleGroups) It seems like we should instead be calling $self->groups and then parsing it against visibleGroups, right? $self->groups gives us a more comprehensive list. It also avoids duplicating the SQL. >+ } >+ return Bugzilla->dbh->selectrow_array($query, undef, $otherUserID); Just to be consistent with some of our other functions, I'd like it to return 1 for true and 0 for false. (I'm trying to make that the general standard for boolean functions.) >+sub visible_groups_as_string { >+ my $self = shift; >+ return join(', ', -1, @{$self->visible_groups_direct()}); >+} If this is going to be a general-use function in User.pm (and not a private subroutine), it shouldn't include -1 in the list. Also, why do we only call _direct? Is there not a visible_groups that also gives us inherited groups? (Why don't we want inherited groups.) >+=item C<can_see_user(user_id)> >+ >+Determines whether the user can see a user. (Checks for existence, too.) We should be more specific, and say that it returns 1 if the user exists and is visible to this user, 0 otherwise. r-, but just until answering the questions. If all the questions have some really good answers, then you can r? again. (But do fix the nits.)
Attachment #179861 -
Flags: review? → review-
Comment 4•19 years ago
|
||
(In reply to comment #3) > >+ > >+ if (Param('usevisibilitygroups')) { > >+ my $visibleGroups = $self->visible_groups_as_string(); > >+ $query = qq{SELECT COUNT(DISTINCT userid) > >+ FROM profiles, user_group_map > >+ WHERE userid = ? > >+ AND user_id = userid > >+ AND isbless = 0 > >+ AND group_id IN ($visibleGroups) > > It seems like we should instead be calling $self->groups and then parsing it > against visibleGroups, right? $self->groups gives us a more comprehensive list. > It also avoids duplicating the SQL. > Just because I am in a group with someone, I cannot necessarily see him. For example, all of us may be in editbugs together. So, I must be in a group that is permitted to see a group that he is in.
Comment 5•19 years ago
|
||
(In reply to comment #4) > Just because I am in a group with someone, I cannot necessarily see him. For > example, all of us may be in editbugs together. So, I must be in a group that is > permitted to see a group that he is in. Right. But we should still be getting the list of groups that *I* am in by calling $self->groups instead of doing some custom SQL. The SQL in $self->groups is considerably more complicated than this SQL, and I'm sure it's for some good reason.
Comment 6•19 years ago
|
||
(In reply to comment #5) > Right. But we should still be getting the list of groups that *I* am in Or maybe the groups that *he* is in, actually. So instead of taking a $otherUserId, we should just take another User object and call ->groups on it. It's already convenient in editusers anyway. :-)
Comment 7•19 years ago
|
||
OK, that argument makes sense. We'll have to scrub how this is used in editusers. Editusers could have several resons for knowing who is who... 1) Limiting someone with Bless privileges to a subset of users 2) Displaying who is visible as a result of direct or inherited permissions. Let's make sure we 2xr this carefully.
Assignee | ||
Comment 8•19 years ago
|
||
(In reply to comment #7) > We'll have to scrub how this is used in editusers. > > Editusers could have several resons for knowing who is who... > 1) Limiting someone with Bless privileges to a subset of users > 2) Displaying who is visible as a result of direct or inherited permissions. > > Let's make sure we 2xr this carefully. Or rather, let's modify the current behaviour in another bug :) Imho, we shouldn't give editusers people more visibility than other people. If a site decides editusers people _should_ have, say, complete visibility, they can be given complete visibility easily, using standard groups rules.
Comment 9•19 years ago
|
||
(In reply to comment #8) > Or rather, let's modify the current behaviour in another bug :) Actually, based on the current name of the functions, I'm fairly sure that the current behavior is erroneous, at least for can_see_user. It really *should* be using $self->groups and not its own custom SQL to get a user's group list.
Assignee | ||
Comment 10•19 years ago
|
||
(In reply to comment #9) > (In reply to comment #8) > > Or rather, let's modify the current behaviour in another bug :) > > Actually, based on the current name of the functions, I'm fairly sure that the > current behavior is erroneous, at least for can_see_user. It really *should* be > using $self->groups and not its own custom SQL to get a user's group list. Max, I'm all with you on correcting _erroneous_ behaviour. If you look at comment 8, all I'm saying is that we shouldn't create an exception for editusers, thus changing current _working_ behaviour :)
Comment 11•19 years ago
|
||
(In reply to comment #10) > Max, I'm all with you on correcting _erroneous_ behaviour. If you look at > comment 8, all I'm saying is that we shouldn't create an exception for > editusers, thus changing current _working_ behaviour :) Yeah, I agree that we shouldn't change anything that isn't erroneous for 2.20. For 2.22, we can do whatever we'd like, provided that we test it well. :-)
Assignee | ||
Comment 12•19 years ago
|
||
(In reply to comment #3) > It seems like we should instead be calling $self->groups and then parsing it > against visibleGroups, right? $self->groups gives us a more comprehensive > list. > It also avoids duplicating the SQL. Maybe. Imho, there is something to be said for letting work be done by the DB rather than by perl. Let's discuss this in another bug; this is about moving canSeeUser to User.pm. > >+ return Bugzilla->dbh->selectrow_array($query, undef, $otherUserID); > > Just to be consistent with some of our other functions, I'd like it to > return > 1 for true and 0 for false. (I'm trying to make that the general standard for > boolean functions.) Well, that's what it does already. > >+sub visible_groups_as_string { > >+ my $self = shift; > >+ return join(', ', -1, @{$self->visible_groups_direct()}); > >+} > > If this is going to be a general-use function in User.pm (and not a private > subroutine), it shouldn't include -1 in the list. That's another bug, with more implications. In fact, it's been fixed in the meantime as bug 285153. > Also, why do we only call _direct? Is there not a visible_groups that also > gives us inherited groups? (Why don't we want inherited groups.) Calling visible_groups_inherited now. > >+=item C<can_see_user(user_id)> > >+ > >+Determines whether the user can see a user. (Checks for existence, too.) > > We should be more specific, and say that it returns 1 if the user exists and > is visible to this user, 0 otherwise. Ok. Fixing a regression of bug 285153 along the way (groupsUserMayBless was given the wrong user object in userDataToVars). You may ask me now to fix this in a separate bug ;)
Assignee | ||
Updated•19 years ago
|
Attachment #179861 -
Attachment is obsolete: true
Attachment #181030 -
Flags: review?(mkanat)
Assignee | ||
Updated•19 years ago
|
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Assignee | ||
Comment 13•19 years ago
|
||
(In reply to comment #12) > Fixing a regression of bug 285153 along the way (groupsUserMayBless was given > the wrong user object in userDataToVars). You may ask me now to fix this in a > separate bug ;) It turns out this is not a regression but really a part of this bug. The effect I noticed was introduced by the visible_groups_as_list movement part of the patch.
Comment 14•19 years ago
|
||
Comment on attachment 181030 [details] [diff] [review] Patch 1.2 >+sub can_see_user { >+ my ($self, $otherUser) = @_; >+ my $query; >+ >+ # If we got a user ID instead of a User object, generate the object from it. >+ ref($otherUser) || ($otherUser = new Bugzilla::User($otherUser)) || return 0; Why creating a new object as the only thing you are interested in is it's ID?
Comment 15•19 years ago
|
||
(In reply to comment #14) > Why creating a new object as the only thing you are interested in is it's ID? Yeah, I was also going to ask that -- the function should just be passed a User object, it should never have to create its own User object.
Assignee | ||
Updated•19 years ago
|
Attachment #181030 -
Flags: review?(mkanat)
Assignee | ||
Comment 16•19 years ago
|
||
(In reply to comment #14) > Why creating a new object as the only thing you are interested in is it's ID? I agree. We have the user object handy, though, and we should imo work on objects preferably, so the sub now requires a user object, rendering the code even a little simpler.
Attachment #181030 -
Attachment is obsolete: true
Attachment #183477 -
Flags: review?(mkanat)
Comment 17•19 years ago
|
||
Comment on attachment 183477 [details] [diff] [review] Patch 1.3 OK, this looks correct to me. :-) But it needs some testing, still.
Attachment #183477 -
Flags: review?(wicked)
Attachment #183477 -
Flags: review?(mkanat)
Attachment #183477 -
Flags: review+
Updated•19 years ago
|
Attachment #183477 -
Flags: review?(wicked) → review?(bugzilla)
Comment 18•19 years ago
|
||
Comment on attachment 183477 [details] [diff] [review] Patch 1.3 glob has not reviewed it yet and joel was involved in the discussion. Requesting joel for review.
Attachment #183477 -
Flags: review?(bugzilla) → review?(bugreport)
Comment 19•19 years ago
|
||
Comment on attachment 183477 [details] [diff] [review] Patch 1.3 This looks good by inspection. I still need to do some testing.
Updated•19 years ago
|
Whiteboard: patch needs testing
Comment 20•19 years ago
|
||
Comment on attachment 183477 [details] [diff] [review] Patch 1.3 r=joel for HEAD only
Attachment #183477 -
Flags: review?(bugreport) → review+
Comment 21•19 years ago
|
||
Comment on attachment 183477 [details] [diff] [review] Patch 1.3 oops... bitrotted though. Please resubmit with rot fixed
Attachment #183477 -
Flags: review+ → review-
Assignee | ||
Comment 22•19 years ago
|
||
Attachment #183477 -
Attachment is obsolete: true
Attachment #190222 -
Flags: review?(mkanat)
Comment 23•19 years ago
|
||
Comment on attachment 190222 [details] [diff] [review] Patch 1.3, unrotted r=mkanat based on observing that it does seem to just be a direct un-bitrot, and joel tested and r+'ed the previous patch.
Attachment #190222 -
Flags: review?(mkanat) → review+
Comment 24•19 years ago
|
||
I think this goes to HEAD, and not to the 2.20 branch, unless there's something I'm forgetting about it.
Flags: approval?
Whiteboard: patch needs testing
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Assignee | ||
Comment 25•19 years ago
|
||
That's ok for me.
Updated•19 years ago
|
Flags: approval? → approval+
Comment 26•19 years ago
|
||
Comment on attachment 190222 [details] [diff] [review] Patch 1.3, unrotted >--- head/summarize_time.cgi 2005-06-02 10:29:19.601852800 +0200 >+++ patched/summarize_time.cgi 2005-05-11 00:49:50.562227200 +0200 >@@ -322,7 +322,7 @@ > WHERE longdescs.bug_id IN ($buglist) > $date_bits } . > $dbh->sql_group_by('longdescs.bug_id', >- 'bugs.short_desc, bugs.bug_status, >+ 'bugs.short_desc, bugs.bug_status, > longdescs.bug_when') . qq{ > ORDER BY longdescs.bug_when}; > $sth = $dbh->prepare($q); Before commiting this patch, I would like to understand why summarize_time.cgi is part of this patch. I see no difference!
Assignee | ||
Comment 27•19 years ago
|
||
Bug 286686 changed the line ending in the line in question. It seems to me we saw an effect of cvs/diff Win/*nix magic.
Attachment #190222 -
Attachment is obsolete: true
Attachment #190378 -
Flags: review+
Comment 28•19 years ago
|
||
Checking in editusers.cgi; /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v <-- editusers.cgi new revision: 1.93; previous revision: 1.92 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.66; previous revision: 1.65 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 29•16 years ago
|
||
r=mkanat on IRC. Checked in on all branches >= 2.22.
Attachment #333066 -
Flags: review+
You need to log in
before you can comment on or make changes to this bug.
Description
•