150829 - 'My Votes' link missing from footer

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

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?

Comment 1

[Bradley Baetz (:bbaetz)]

regression; -> 2.16

Comment 2

[Gervase Markham [:gerv]]

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

Comment 3

[Bradley Baetz (:bbaetz)]

Ug. What about making GetVersionTable() set this?

Comment 4

[Matthew Tuck [:CodeMachine]]

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.

Comment 5

[Bradley Baetz (:bbaetz)]

Bleh.

Yeah, you're right - we do need to call GetVersionCache all the time.

How did the old cod edeal with this/

Comment 6

[Dave Miller [:justdave]]

*** Bug 151420 has been marked as a duplicate of this bug. ***

Comment 7

[Gervase Markham [:gerv]]

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

[Matthew Tuck [:CodeMachine]]

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.

Comment 9

[Bradley Baetz (:bbaetz)]

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

[Gervase Markham [:gerv]]

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

Comment 11

[Bradley Baetz (:bbaetz)]

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

[Gervase Markham [:gerv]]

OK. Go for it :-)

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

Attachment: v1

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

[Gervase Markham [:gerv]]

> 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

Comment 15

[Bradley Baetz (:bbaetz)]

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

[Vaskin Kissoyan]

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

[Gervase Markham [:gerv]]

bbaetz: I'll concede the other points, but what are "ordering issues"?

Gerv

Comment 18

[Bradley Baetz (:bbaetz)]

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

[Gervase Markham [:gerv]]

Comment on attachment 88121 [details] [diff] [review]
v1

r=gerv. Whatever - if it works, let's get it into 2.16 :-)

Gerv

Comment 20

[Bradley Baetz (:bbaetz)]

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.

Comment 21

[Jouni Heikniemi]

-> patch author bbaetz

Comment 22

[Dave Miller [:justdave]]

bbaetz: this is your patch, and you marked it needs-work on yourself.  Status?

Comment 23

[Bradley Baetz (:bbaetz)]

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?

Comment 24

[Bradley Baetz (:bbaetz)]

Attachment: v2

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

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 90462 [details] [diff] [review]
v2

Looks good, works.  r=myk. Needs a second review.

Comment 26

[Jouni Heikniemi]

Comment on attachment 90462 [details] [diff] [review]
v2

r=jouni

Comment 27

[Bradley Baetz (:bbaetz)]

Checked in, trunk + branch