253588 - Change Bugzilla->user to be always usable even if user is logged out

Status: RESOLVED FIXED

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

Description

[Joel Peshkin]

Various functions that should move from globals.pl to Bugzilla::User are needed
even when a logged-out user is calling the code.  In order to make that
functionality available, we must have both a way to create a user object that is
logged out and we must expunge all the places in other code that rely on
Bugzilla->uder being undefined to establish that the user is not logged in.

Comment 1

[Christian Reis]

This is an excellent cleanup. Thanks for filing it.

Comment 2

[Erik Stambaugh]

Attachment: Creates a 'no-user' Bugzilla->user object

Well, here it is.  It looks like it works, too.

Comment 3

[Joel Peshkin]

Comment on attachment 154786 [details] [diff] [review]
Creates a 'no-user' Bugzilla->user object

I've already been over this code, but I had my hands on it too much to be the
sole reviews, so consider it half-reviewed :-)

Comment 4

[Erik Stambaugh]

Attachment: update

11:51 <@kiko> +my $login = Bugzilla->user->id ? Bugzilla->user->login : "";
11:51 <@kiko> shouldn't we just make user->login return "" if the user is a
dummy?

Comment 5

[Christian Reis]

Comment on attachment 154788 [details] [diff] [review]
update


Overall, r=kiko, though there are comments you may want to take into account
before requesting approval. I don't  need to review this again if all you fix
are nits, it's really trivial and the most important part isn't what the patch
contain, but what it misses. Good work.

>Index: Bugzilla.pm
> sub logout {
>     my ($class, $option) = @_;
>-    if (! $_user) {
>+    if (! $_user->{'id'}) {

Two questions: can if be just $_user->id, and what happens if the user calls
logout() before login()? 

>Index: buglist.cgi
>-    my $userid = DBNameToIdAndCheck(Bugzilla->user->login);
>+    my $userid = Bugzilla->user->id;

Was this just being silly?

>Index: describecomponents.cgi
>-        Bugzilla->login(LOGIN_REQUIRED) unless Bugzilla->user;
>+        Bugzilla->login(LOGIN_REQUIRED);

Also sillyness?

>Index: query.cgi
> my $user = Bugzilla->user;
>-my $userid = $user ? $user->id : 0;
>+my $userid = $user->id;

Looks like this $user variable here is not being used anywhere else..

>Index: votes.cgi
>-    my $canedit = 1 if (Bugzilla->user &&
>+    my $canedit = 1 if ($userid &&
>                         $name eq Bugzilla->user->login);

Can this fit on one line?

>Index: Bugzilla/Bug.pm
$self->{'qa_contact'}{'id'});
>-    my $isreporter = Bugzilla->user && 
>+    my $isreporter = Bugzilla->user->id && 
>                      Bugzilla->user->id == $self->{'reporter'}{'id'};

Hmmm. Can the reporter id ever be zero, or is this just a sanity check? Maybe
mark this with some XXX comment.

>Index: Bugzilla/User.pm
>+Creates a new C{Bugzilla::User> object for the given user id.  If no matching
>+user is found, or if no user id was given, a blank object is created with no
>+user attributes.

Hmmm. If no matching user is found? Shouldn't that be an explicit error, trying
to instantiate a user with an invalid userid? My only intention here would be
to allow a user to be created even if no login is performed.

Can you also outline here also what the user lifecycle is -- where it's being
created and what happens when a login is triggered?

Comment 6

[Christian Reis]

(In reply to comment #5)
> Looks like this $user variable here is not being used anywhere else..
> 
> >Index: votes.cgi
> >-    my $canedit = 1 if (Bugzilla->user &&
> >+    my $canedit = 1 if ($userid &&
> >                         $name eq Bugzilla->user->login);
> 
> Can this fit on one line?

More importantly, can this just become ($name eq Bugzilla->user->login), or is
this dangerous because $name isn't being validated?

Comment 7

[Erik Stambaugh]

(In reply to comment #6)

> > Can this fit on one line?
> 
> More importantly, can this just become ($name eq Bugzilla->user->login), or is
> this dangerous because $name isn't being validated?
 
I think we're safer leaving it as-is.

Comment 8

[Erik Stambaugh]

Attachment: update update

Takes the above into account

Comment 9

[Joel Peshkin]

Comment on attachment 154796 [details] [diff] [review]
update update

r=joel
[noting kiko's prior review  as well]

Comment 10

[Christian Reis]

Comment on attachment 154796 [details] [diff] [review]
update update

>Index: Bugzilla/User.pm
>+Creates a new C{Bugzilla::User> object for the given user id.  If no user
>+id was given, a blank object is created with no user attributes.
>+
>+If an id was given but there was no matching user found, undef is returned.

My only question is: is this not an error condition? Should we silently fail or
do callsites worry about this for us?

Comment 11

[Joel Peshkin]

The callsites should and do handle the worrying.

In fact, I am about to try to figure out how to keep Search.pm from throwing
errors if somebody goofs and taking Gerv's series or whining with it.  In the
case of whining, it'll just take out the one report and the rest will get picked
up minutes later.  In the case of the series stuff, it would stop all colection.

Comment 12

[Joel Peshkin]

checked in

Comment 13

[Bradley Baetz (:bbaetz)]

The reason for this originally was to allow code which used |if ($user)| to
still work. I think theres been one pass over most of that stuff already,
though, and its now more trounle than it was worth. Thanks for doing this.