Closed
Bug 150829
Opened 23 years ago
Closed 23 years ago
'My Votes' link missing from footer
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: bbaetz, Assigned: bbaetz)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
5.51 KB,
patch
|
myk
:
review+
jouni
:
review+
|
Details | Diff | Splinter Review |
The 'My Votes' link isn't in the footer. This is because the condition is:
[% IF user.login AND use_votes %]
use_votes is set in globals.pl to $::anyvotesallowed.
$::anyvotesallowed is read in from teh versioncache, which hasn't been loaded at
this point. Oops...
bug_form.pl uses the user_votes var name, too; doesn't this conflict?
Assignee | ||
Comment 1•23 years ago
|
||
regression; -> 2.16
Comment 2•23 years ago
|
||
Eventually, all these horrible globals can go away and become part of the One
Great Global, $vars. In the mean time, would it be an acceptable fix to make it so:
$::vars{'use_votes'} = $::anyvotesallowed;
appeared at the end of data/versioncache?
As for the clash: you are correct. I think we can just remove the instance from
bug_form.pl; currently, the My Votes link would only appear on the bug view if
the product the bug is in allowed voting. This seems counter-intuitive. It can
use the global flag like everything else.
Gerv
Gerv
Assignee | ||
Comment 3•23 years ago
|
||
Ug. What about making GetVersionTable() set this?
Comment 4•23 years ago
|
||
Given this should be on all logged in footers, and it requires information in
the version cache, it seems a possibility that calling GetVersionCache should be
done during quietly_check_login or confirm_login.
Assignee | ||
Comment 5•23 years ago
|
||
Bleh.
Yeah, you're right - we do need to call GetVersionCache all the time.
How did the old cod edeal with this/
Comment 6•23 years ago
|
||
*** Bug 151420 has been marked as a duplicate of this bug. ***
Comment 7•23 years ago
|
||
Putting in an always-called call to GetVersionTable() would also be a fix; it
has built-in protection about being called twice. This could go at the end of
globals.pl.
Gerv
Comment 8•23 years ago
|
||
The only problem is we want to move all GetVersionTables to after
quietly_check_logins. Perhaps the eventual solution there is to do a
quietly_check_login automatically as well.
Assignee | ||
Comment 9•23 years ago
|
||
Why do we want to move GetVersionTable to after we get the logins?
I did it that way incase it returned an error, but the current code never errors
out anyway. And if it did, the installation would have more problems that
working out if the footer was correct.
Comment 10•23 years ago
|
||
The whole situation of what code to always run need a serious tidy up, it's
true. But we need to work out the minimum risk fix for 2.16. I'd say that was to
call GetVersionTable() as roughly the first thing we do in globals.pl. If we end
up calling it twice, fine (although we could remove all the other calls
trivially, as well.)
Gerv
Assignee | ||
Comment 11•23 years ago
|
||
OTOH, The 'my votes' link only comes up if you are logged in.
The only question I have about putting it into globals.pl is 'Will this break
programs running from contrib/?' I suspect that the answer is, for some of them,
yes, but OTOH they may alrady be broken.
Lets:
a) add it to quietly_check_login, for 2.16
b) Work out what parts of the versioncache we actually still need/want/etc
c) Remove the uneeded bits
d) _Then_ work out where to put it
Our trick of removing the file when certain changes are made, and letting it get
regenerated later, won't work under mod_perl by default. Nor will our fussing
with the global namespace.
Comment 12•23 years ago
|
||
OK. Go for it :-)
Gerv
Assignee | ||
Comment 13•23 years ago
|
||
This adds it to the user hash on the basis that:
a) We don't conflict with the var for bug_form.pl, this way
b) We may want to make it depend on if the user has any votes - the page isn't
very useful otherwise, plus it would solve the problem of when to call
GetVersionTable
c) Its a simple, quick, easy, fix, which we need for 2.16
Comment 14•23 years ago
|
||
> This adds it to the user hash on the basis that:
>
> a) We don't conflict with the var for bug_form.pl, this way
But it should "conflict" - the two are supposed to be the same. If they aren't,
that's a bug.
> b) We may want to make it depend on if the user has any votes - the page isn't
> very useful otherwise, plus it would solve the problem of when to call
> GetVersionTable
If the user has no votes, the page should say "You have no votes", and perhaps
link to the page explaining voting.
> c) Its a simple, quick, easy, fix, which we need for 2.16
If you are going to put it in the user hash, call it "has_votes", not
"use_votes". But I believe it makes no sense there. Put it in the root of $vars,
and remove the other version in bug_form.pl.
Gerv
Assignee | ||
Comment 15•23 years ago
|
||
a) No, they're not the same. Remember, votes are per product. The footer should
show if there are any votes, anywhere, but we should only let people vote on a
bug if that particular product allows votes.
b) Well, thats what the page says
c) Yeah, but we can't easily do that, because of ordering issues.
Comment 16•23 years ago
|
||
IMHO, the patch works fine, tested ok here. When there are no votes it simply
says " You are currently not voting on any bugs. "
Comment 17•23 years ago
|
||
bbaetz: I'll concede the other points, but what are "ordering issues"?
Gerv
Assignee | ||
Comment 18•23 years ago
|
||
The ordering issue is that we set this when we create the template object, but
thats done before we call GetVersionTable, which is needed. If we call
GetVersionTable earlier (and always, in globals.pl), then its called before the
code whch checks for stuff like shutdownhtml. If we move that arround (and,
IIRC, that means changing files, which may create other oridering issues), then
that causes other problems. Its not impossible to solve, but its not a simple
change either.
Since we don't use a wrapper func to process a template (yet) we can't do it there.
Comment 19•23 years ago
|
||
Comment on attachment 88121 [details] [diff] [review]
v1
r=gerv. Whatever - if it works, let's get it into 2.16 :-)
Gerv
Attachment #88121 -
Flags: review+
Assignee | ||
Comment 20•23 years ago
|
||
Comment on attachment 88121 [details] [diff] [review]
v1
This patch needs-work, because quey.cgi needs to check this var too, even if
we're not logged in.
Sigh.
Attachment #88121 -
Flags: review-
Comment 22•23 years ago
|
||
bbaetz: this is your patch, and you marked it needs-work on yourself. Status?
Assignee | ||
Comment 23•23 years ago
|
||
So, we need a solution to this.
use_votes has to be made available even when noone has logged in at all, because
of bug 155031. I think I should change the bug_form vote param name, since its
conceptually different.
However, I'm not sure where to put this check, any more. Maybe query.cgi should
handle it differently?
Or maybe use_votes should really be a param. Thats not happening for 2.16, though.
Thoughts?
Blocks: 155031
Assignee | ||
Comment 24•23 years ago
|
||
Making it a param isn't that hard...
Disadvantages:
- Not everywhere is checked (eg votes.cgi doesn't check). Votes.cgi doesn't
check currently, though. A lot of the use* stuff isn't checked that well. We
should tidy it up at some stage.
- Its not automatic. It is, however, more predictable.
Advantages:
- Its a lot simpler
- It works
Attachment #88121 -
Attachment is obsolete: true
Comment 25•23 years ago
|
||
Comment on attachment 90462 [details] [diff] [review]
v2
Looks good, works. r=myk. Needs a second review.
Attachment #90462 -
Flags: review+
Comment 26•23 years ago
|
||
Comment on attachment 90462 [details] [diff] [review]
v2
r=jouni
Attachment #90462 -
Flags: review+
Assignee | ||
Comment 27•23 years ago
|
||
Checked in, trunk + branch
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•12 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
•