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)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: bugreport, Assigned: bugreport)
References
Details
Attachments
(1 file, 2 obsolete files)
18.98 KB,
patch
|
bugreport
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.20
Comment 1•20 years ago
|
||
This is an excellent cleanup. Thanks for filing it.
Comment 2•20 years ago
|
||
Well, here it is. It looks like it works, too.
Updated•20 years ago
|
Attachment #154786 -
Flags: review?(kiko)
Assignee | ||
Comment 3•20 years ago
|
||
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•20 years ago
|
||
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
Updated•20 years ago
|
Attachment #154788 -
Flags: review?(kiko)
Updated•20 years ago
|
Attachment #154786 -
Flags: review?(kiko)
Comment 5•20 years ago
|
||
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+
Comment 6•20 years ago
|
||
(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•20 years ago
|
||
(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•20 years ago
|
||
Takes the above into account
Attachment #154788 -
Attachment is obsolete: true
Assignee | ||
Comment 9•20 years ago
|
||
Comment on attachment 154796 [details] [diff] [review] update update r=joel [noting kiko's prior review as well]
Attachment #154796 -
Flags: review+
Assignee | ||
Updated•20 years ago
|
Flags: approval?
Comment 10•20 years ago
|
||
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?
Assignee | ||
Comment 11•20 years ago
|
||
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.
Updated•20 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 12•20 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 13•20 years ago
|
||
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.
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•