Closed Bug 253588 Opened 20 years ago Closed 20 years ago

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

Categories

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

2.19
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: bugreport, Assigned: bugreport)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.20
This is an excellent cleanup. Thanks for filing it.
Well, here it is.  It looks like it works, too.
Attachment #154786 - Flags: review?(kiko)
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 :-)
Attached patch update (obsolete) — Splinter Review
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?
Attachment #154786 - Attachment is obsolete: true
Attachment #154788 - Flags: review?(kiko)
Attachment #154786 - Flags: review?(kiko)
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?
Attachment #154788 - Flags: review?(kiko) → review+
(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?

(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.
Attached patch update updateSplinter Review
Takes the above into account
Attachment #154788 - Attachment is obsolete: true
Comment on attachment 154796 [details] [diff] [review]
update update

r=joel
[noting kiko's prior review  as well]
Attachment #154796 - Flags: review+
Flags: approval?
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?
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.

Flags: approval? → approval+
checked in
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: