117760 - Templatise showvotes.cgi and incorporate doeditvotes.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Gervase Markham [:gerv]]

This should be all one cgi - votes.cgi. Templates, taint mode, etc. etc. There's
also several UI improvements.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's the first cut. It's missing the change to show_bug.cgi to call it rather
than the old CGI name because that change will have to be made after the
show_bug.cgi templatisation is checked in (bug 110012).

Gerv

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Tweaked it a bit. Still looking for review :-)

Gerv

Comment 3

[Bradley Baetz (:bbaetz)]

I'll look at this tomorrow, but while you're doing this, and breaking (the
probably small number of) existing bookmarks, can you fix bug 54159 by using an
email address rather than the user id while you are at it? Else I'll do it after
this is in - I just don't want to break bookmarks twice.

Comment 4

[Gervase Markham [:gerv]]

We seem to be on the same track here - I just sent email about this. I'm happy
to make that fix if everyone thinks it's OK. 

Gerv

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.3

This has the fix to key by email address. One thing it's missing is changes to
the enter_bug and footer voting links, but I'll fix those up when the smoke
clears surrounding the enter_bug templatisation.

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 66989 [details] [diff] [review]
Patch v.3

I haven't actually applied this one yet; I'm just reading code.

>Index: votes.cgi
>===================================================================
>RCS file: votes.cgi
>diff -N votes.cgi
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ votes.cgi	29 Jan 2002 23:41:38 -0000

>+
>+# Use the template toolkit (http://www.template-toolkit.org/)
>+use Template;
>+
>+# Create the global template object that processes templates
>+my $template = Template->new(
....

Use the global template stuff.


>+    SendSQL("SELECT profiles.login_name, votes.who, votes.count 
>+             FROM votes, profiles 
>+             WHERE votes.bug_id = " . SqlQuote($bug_id) . " 
>+               AND profiles.userid = votes.who");

Don't use SqlQuote here. You know its a number, because you've checked it
above, and if you hadn't
checked it then this would have passed the taint checks because SqlQuote
detaints.

>+    
>+    SendSQL("LOCK TABLES bugs READ, products READ, votes WRITE");

Ug, this locking stuff is a pain. But thats the way it was, so you can leave it
for the moment.

>+            # Next if user can't see this bug. So, the totals will be correct
>+            # and they can see there are votes 'missing', but not on what bug
>+            # they are. This seems a reasonable compromise; the alternative is
>+            # to lie in the totals. XXX
>+            # next if !CanSeeBug($id, $who, $::usergroupset);  

Did you mean to leave that commented out? You could fudge the totals by using
SelectVisible.

>+        }
>+        elsif ($::FORM{'delete_all_votes'} == 0) {
>+            print "Location: votes.cgi\n\n";

print "Status: 301 Permanent Redirect"; before the location line.

>+            exit();
>+        }
>+    }
>+
>+    # Call ValidateBugID on each bug ID to make sure it is a positive
>+    # integer representing an existing bug that the user is authorized 
>+    # to access, and make sure the number of votes submitted is also
>+    # a non-negative integer (a series of digits not preceded by a
>+    # minus sign).
>+    foreach my $id (@buglist) {
>+      ValidateBugID($id);
>+      ($::FORM{$id} =~ /^\d+$/)
>+        || DisplayError("Only use non-negative numbers for your bug votes.")
>+        && exit;
>+    }

Use detaint_natural rather than the regexp. Did that pass taint checks?

The tempaltes look fine at a glance, but I'm not going to really comment until
I see what they look like.

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.4

> print "Status: 301 Permanent Redirect"; before the location line.

Are you sure? This just sends the user back to the beginning if they cancel
deleting all their votes. Is it really a "permanent redirect"?

All review comments addressed.

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

Comment on attachment 71171 [details] [diff] [review]
Patch v.4

OK, I didn't realise that apache added 302 automatically if there is a location
header. You can ignore 
that then.

>+        $onevoteonly = 1 if ($canedit &&
>+                             min($::prodmaxvotes{$product},
>+                                 $maxvotesperbug{$product}) < 2);

What does canedit have to do with it? And don't you want the min to be == 1,
not < 2? Practically they're
the same, but....

>+        elsif ($::FORM{'delete_all_votes'} == 0) {
>+            print "Status: 301 Permanent Redirect\n";

You're right - lose the 301.

>+            print "Location: votes.cgi\n\n";
>+            exit();
>+        }

>Index: template/default/voting/delete_all_votes.tmpl
>===================================================================
>RCS file: template/default/voting/delete_all_votes.tmpl
>diff -N template/default/voting/delete_all_votes.tmpl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ template/default/voting/delete_all_votes.tmpl	24 Feb 2002 08:45:19 -0000

Maybe we need a global confirm template?

I'd prefer something like that, honestly.

Testing comments:

If you enter an invalid username, theres just raw html displayed as text.

You need to lock cc AS selectVisible_cc READ if you lock the bugs table, and
want to call CanSeeBug
inside the LOCK calls. This means that I can't actually test this patch...

Comment 9

[Gervase Markham [:gerv]]

> Maybe we need a global confirm template?
> 
> I'd prefer something like that, honestly.

I'd say it would end up so parameterised as to be hardly worth having. And then
it gives admins problems if they want one sort of confirm to be different to
another.

Testing comments:

> If you enter an invalid username, theres just raw html displayed as text.

That's a lack of templatisation in DBNameToIDAndCheck(). It'll get fixed
eventually, but it's Not This Bug(TM).

> You need to lock cc AS selectVisible_cc READ if you lock the bugs table, and
> want to call CanSeeBug inside the LOCK calls. This means that I can't 
> actually test this patch...

I don't understand locking much - could you explain more carefully, and tell me
why it means you can't test it? Thanks :-)

Gerv

Comment 10

[Bradley Baetz (:bbaetz)]

Templatisation is a separate issue; displaying html tags in the window either
needs to be fixed by a bug which blocks this, or somehow fixed by manually doing
teh content-type header.

Mysql requires that if one table in a query is locked, all tables must be. So
when you call selectVisible, and the bugs table is locked, but the cc table
isn't, then that gives a server error. You would have seen that if you'd tested
looking at a bug.

Comment 11

[Matthew Tuck [:CodeMachine]]

> I'd say it would end up so parameterised as to be hardly worth having. And
> then it gives admins problems if they want one sort of confirm to be different
> to another.

There is already a global confirm template in the administration rewrite, that
of the value X being on bugs.  It is quite parameterised, to be sure, but then
the whole administration rewrite is, so it's no hassle.

However, it wouldn't handle this case I don't think.  I would be interested in
some sort of confirm template fragment though.

Comment 12

[Gervase Markham [:gerv]]

bbaetz, you said:

"Mysql requires that if one table in a query is locked, all tables must be. So
when you call selectVisible, and the bugs Mysql requires that if one table in a
query is locked, all tables must be. So
when you call selectVisible, and the bugs table is locked, but the cc table
isn't, then that gives a server error. You would have seen that if you'd tested
looking at a bug."

This patch doesn't call selectVisible, and does nothing with the CC table.

Gerv

Comment 13

[Dave Miller [:justdave]]

But you are calling "CanSeeBug()", which calls selectVisible internally, so you
need to lock the cc table anyway.

Comment 14

[Bradley Baetz (:bbaetz)]

It calls cansebug, which calls selectvisible.

Just try using your patch to see a particular bug, and you'll see teh problem.

Comment 15

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Ah, I get it now :-)

Gerv

Comment 16

[Bradley Baetz (:bbaetz)]

Comment on attachment 72832 [details] [diff] [review]
Patch v.5

This is OK, as far as it goes (you still need the changes to existing files)

However, teh error message if you mistype the email address is still raw html.
If there is a 2.16 bug to fix this, then please mark that bug as blocking this
one. Otherwise work arround it in this bug in some way.

Comment 17

[u44081]

Attachment: Patch v.6

(X)HTML fixes (& in URLs, checked="checked"). Obsoletes patch v.5.

Comment 18

[Bradley Baetz (:bbaetz)]

I have the same comments wrt & -> & that I did for duplicates.cgi - is this
safe for all browers?

Comment 19

[Dave Miller [:justdave]]

It better be safe, because it's a violation of the HTML *3.2* spec to not do it
that way, and even Navigator 4 knew how to read HTML 3.2 :-)

Comment 20

[u44081]

True, and I'm not aware of any browsers that wouldn't cope with &'s in URLs.

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 74601 [details] [diff] [review]
Patch v.6

Having chatted to Ville, he's going to do the XHTML stuff as a later patch if
it's still thought necessary.

Gerv

Comment 22

[Gervase Markham [:gerv]]

Attachment: Patch v.6

OK, this should be the real deal. Ready for review.

Re: plain text error.
The error is in DBNameToIDAndCheck(). The problem is that currently, sometimes
this function is called before a Content-Type is printed, and sometimes after.
So, I can't easily change the function to fix it. And I can't print the
Content-Type before, because some other error functions assume it hasn't been
printed.

Given that the only way of getting this error is editing the URL, I think it's
minor enough that we can let it go.

Gerv

Comment 23

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

Comment on attachment 76297 [details] [diff] [review]
Patch v.6

Index: votes.cgi

>+    my $who = DBNameToIdAndCheck($name);

Use DBname_to_id and throw your own error if the result is false.


>+        if (!MoreSQLData()) {
>+            SendSQL("INSERT INTO votes (who, bug_id, count) 
>+                     VALUES ($who, $bug_id, 0)");
>+        }

Calling MoreSQLData without a subsequent FetchOneColumn or FetchSQLData 
is dangerous.  Use "if (!FetchOneColumn()) {" instead.


>+            my ($prod, $max) = (FetchSQLData());

Nit: parentheses redundant; FetchSQLData returns an array, and array context
is determined by the paretheses around "$prod, $max" anyway.


>+        next if ($::prodmaxvotes{$product} <= 0);

Nit: parentheses unnecessary when conditions used in postfix notation.


>+            next if (!defined($status));

Nit: unnecessary parentheses.


>+            my ($id, $prod, $max) = (FetchSQLData());

Nit: unnecessary parentheses



Index: CGI.pl

>+            $html .= " | <A HREF=\"votes.cgi?action=show_\">My votes</A>\n";

Missing "votes" after "show_".



Index: template/default/voting/show_bug_votes.tmpl



Index: template/default/voting/show_user_votes.tmpl

+		   <input name="[% bug.id %]" value="[% bug.count %]" 
+			  size="2" maxlength="2" />

size="2" is a big improvement, but maxlength="2" makes it impossible for
installations to allow users to vote more than 99 votes for a single
bug, which is currently possible.  The maxlength attribute should either
be calculated or left out.



Index: template/default/voting/delete_all_votes.tmpl

This UI in this file is overcomplicated, since "No" and "Review your votes" 
have the same effect.  There should just be two options here:

Yes, delete all my votes
No, go back and review my votes



Index: template/default/show/show_bug.html.tmpl

Most of these patches are from the show_bug audit, but the voting-related stuff
looks fine.



Index: template/default/sidebar/xul.tmpl

Comment 24

[Bradley Baetz (:bbaetz)]

What myk said - do the error handling yourself, + fix his other nits.

Comment 25

[Gervase Markham [:gerv]]

Attachment: Patch v.7

All review comments addressed.

Gerv

Comment 26

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

Comment on attachment 77217 [details] [diff] [review]
Patch v.7

>Index: votes.cgi

>+    my $name = $::FORM{'user'} || $::COOKIE{'Bugzilla_login'};
>+    my $who = DBname_to_id($name);
>+    
>+    # After DBNameToIdAndCheck is templatised and prints a Content-Type, 
>+    # the above should revert to a call to that function, and this 
>+    # special error handling should go away.
>+    if (!$who) {
>+        DisplayError("$name is not a valid username.\n");
>+        exit;

$name needs to be html_quoted in this error message.


>Index: CGI.pl

interdiff shows the following change between patches v6 and v7 that needs
to be undone (and the URL corrected):

-	     $html .= " | <A HREF=\"votes.cgi?action=show_\">My votes</A>\n";
+	     $html .= " | <A HREF=\"showvotes.cgi\">My votes</A>\n";

Comment 27

[Gervase Markham [:gerv]]

Attachment: Patch v.8

Nits fixed. Looking for review.

Gerv

Comment 28

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

Comment on attachment 77656 [details] [diff] [review]
Patch v.8

This version is missing the change from showvotes.cgi -> votes.cgi
in CGI.pl.  Also, template file names should use hyphens, i.e.:

delete_all_votes -> delete-all-votes

-myk

Comment 29

[Gervase Markham [:gerv]]

> This version is missing the change from showvotes.cgi -> votes.cgi
> in CGI.pl.  

Hmm. That change has been showing up in all of my patches except for the correct
one :-) I'll add it back in.

> Also, template file names should use hyphens, i.e.:
> delete_all_votes -> delete-all-votes

Sure. Are these the only remaining issues to get r=myk?

Gerv

Comment 30

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

>Sure. Are these the only remaining issues to get r=myk?

Yes, unless another showvotes shows up. :-)

Comment 31

[Gervase Markham [:gerv]]

Attachment: Patch v.9

With requested changes; has r=myk.

Gerv

Comment 32

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

Comment on attachment 77746 [details] [diff] [review]
Patch v.9

right, r=myk

Comment 33

[Bradley Baetz (:bbaetz)]

Comment on attachment 77746 [details] [diff] [review]
Patch v.9

r=bbaetz

Remember template version strings and cvs removal of showvotes.cgi and
doeditvotes.cgi.

Comment 34

[Gervase Markham [:gerv]]

Fixed.

Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
initial revision: 1.1
done
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.142; previous revision: 1.141
done
Checking in votehelp.html;
/cvsroot/mozilla/webtools/bugzilla/votehelp.html,v  <--  votehelp.html
new revision: 1.9; previous revision: 1.8
done
Checking in template/default/show/show_bug.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/show_bug.html.tmpl,v 
<--  show_bug.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/default/sidebar/xul.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/sidebar/xul.tmpl,v  <-- 
xul.tmpl
new revision: 1.5; previous revision: 1.4
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/voting/delete-all-votes.html.tmpl,v
done
Checking in template/default/voting/delete-all-votes.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/voting/delete-all-votes.html.tmpl,v
 <--  delete-all-votes.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/voting/show-bug-votes.html.tmpl,v
done
Checking in template/default/voting/show-bug-votes.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/voting/show-bug-votes.html.tmpl,v
 <--  show-bug-votes.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/voting/show-user-votes.html.tmpl,v
done
Checking in template/default/voting/show-user-votes.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/voting/show-user-votes.html.tmpl,v
 <--  show-user-votes.html.tmpl
initial revision: 1.1
done

Removing showvotes.cgi;
/cvsroot/mozilla/webtools/bugzilla/showvotes.cgi,v  <--  showvotes.cgi
new revision: delete; previous revision: 1.11
done
Removing doeditvotes.cgi;
/cvsroot/mozilla/webtools/bugzilla/doeditvotes.cgi,v  <--  doeditvotes.cgi
new revision: delete; previous revision: 1.16
done

Gerv