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)

2.19.2

Tracking

()

RESOLVED FIXED
Bugzilla 2.22

People

(Reporter: Wurblzap, Assigned: Wurblzap)

References

Details

Attachments

(2 files, 4 obsolete files)

(In reply to bug 119485, comment #36)

> >+    canSeeUser($otherUserID)
> 
>   Nit: That really should be a method of Bugzilla::User, I'd think.
Attached patch Patch (obsolete) — Splinter Review
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?
Status: NEW → ASSIGNED
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 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-
(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.  


(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.
(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. :-)
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.
(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.
(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.
(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 :)
(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. :-)
Attached patch Patch 1.2 (obsolete) — Splinter Review
(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 ;)
Attachment #179861 - Attachment is obsolete: true
Attachment #181030 - Flags: review?(mkanat)
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
(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 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?
(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.
Attachment #181030 - Flags: review?(mkanat)
Attached patch Patch 1.3 (obsolete) — Splinter Review
(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 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+
Attachment #183477 - Flags: review?(wicked) → review?(bugzilla)
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 on attachment 183477 [details] [diff] [review]
Patch 1.3

This looks good by inspection.	I still need to do some testing.
Whiteboard: patch needs testing
Comment on attachment 183477 [details] [diff] [review]
Patch 1.3

r=joel for HEAD only
Attachment #183477 - Flags: review?(bugreport) → review+
Comment on attachment 183477 [details] [diff] [review]
Patch 1.3

oops... bitrotted though.
Please resubmit with rot fixed
Attachment #183477 - Flags: review+ → review-
Attached patch Patch 1.3, unrotted (obsolete) — Splinter Review
Attachment #183477 - Attachment is obsolete: true
Attachment #190222 - Flags: review?(mkanat)
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+
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
That's ok for me.
Flags: approval? → approval+
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!
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+
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
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.

Attachment

General

Created:
Updated:
Size: