250440 - when voting for bug, jump down to bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Krishna Sethuraman]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a2) Gecko/20040708 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a2) Gecko/20040708 Firefox/0.8.0+

Originally filed under http://bugs.kde.org/show_bug.cgi?id=84771 .

When I vote for a bug/wish, I'm taken to the top of my votes page.  It would be
much nicer if I was brought directly to the bug I'm voting for, rather than
having to scroll down to find it.  For example, when viewing bug 53752, and
clicking on 'vote', I am taken to
http://bugs.kde.org/votes.cgi?action=show_user&bug_id=53752 , and I have to find
the bug with 0 votes listed next to it.  It would be better if I was taken to

http://bugs.kde.org/votes.cgi?action=show_user&bug_id=53752#kmail

or possibly (and I think it would be very easy to implement this):

http://bugs.kde.org/votes.cgi?action=show_user&bug_id=53752#53752

with the votes.cgi script modified appropriately to generate the proper #
anchors, one for each bug/wish displayed on the page.

Reproducible: Always
Steps to Reproduce:
1. Have a lot of votes for bugs, so that it fills a couple pages
2. Click on 'Vote for this bug' from a bug page
3. Notice you have to scroll down to find the bug you clicked on so you can vote
for it



Expected Results:  
Jumped down the page to the bug/wish in question.

Comment 1

[Frédéric Buclin]

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

Comment 2

[Frédéric Buclin]

The reporter of the dupe suggested also to highlight the corresponding checkbox
(or the whole line) to make things really easier.

Comment 3

[GavinS]

Attachment: jump to the bug being voted on v1

Here is a first stab...

Comment 4

[Marc Schumann [:Wurblzap]]

Comment on attachment 169728 [details] [diff] [review]
jump to the bug being voted on v1

-    SendSQL("LOCK TABLES bugs write, votes write, products read, cc read,
-	      fielddefs read, user_group_map read, bug_group_map read");
+    SendSQL("LOCK TABLES bugs WRITE, votes WRITE, products READ, cc READ,
+	      fielddefs READ, user_group_map READ, bug_group_map READ");

Is this intentionally part of this bug's patch?

+  #		bugs: list of bugs the user has voted for.is voting for for
+  #		      the product

There's something wrong here... The line should probably end just before the
dot, right?

+[% style_urls = [ "skins/standard/voting.css" ] %]

Please put that inline into [% PROCESS header %].

+	     <td>[% IF bug.id == bug_id %]Enter New Vote here ->[% END %]</td>

How about s/->/&rarr;/ or something?

+  * The Initial Developer of the Original Code is Netscape Communications
+  * Corporation. Portions created by Netscape are
+  * Copyright (C) 1998 Netscape Communications Corporation. All
+  * Rights Reserved.

AFAIK that part should be omitted when creating new files.

+/* Highlight the row for the bug being voted on */

The highlight is too harsh for my personal taste, but that's just me of course.

Nice, user-friendly patch. And adding an INTERFACE section to the template,
too! :-)

Comment 5

[GavinS]

Attachment: jump to the bug being voted on v2

Comment 6

[GavinS]

(In reply to comment #4)
> (From update of attachment 169728 [details] [diff] [review] [edit])
> -    SendSQL("LOCK TABLES bugs write, votes write, products read, cc read,
> -	      fielddefs read, user_group_map read, bug_group_map read");
> +    SendSQL("LOCK TABLES bugs WRITE, votes WRITE, products READ, cc READ,
> +	      fielddefs READ, user_group_map READ, bug_group_map READ");
> 
> Is this intentionally part of this bug's patch?

Yes, but purely in the cause of standards consistency

> +/* Highlight the row for the bug being voted on */
> 
> The highlight is too harsh for my personal taste, but that's just me of course.

I've toned the colour down a bit, but a reviewer or approver or checker-inner
can always tweak the css

Comment 7

[Frédéric Buclin]

Comment on attachment 170283 [details] [diff] [review]
jump to the bug being voted on v2

The actual voting system is broken, see bug 277013. The LOCK TABLE is the
reason.

I would prefer if you don't include this part of the code into your patch.

Comment 8

[GavinS]

Attachment: jump to the bug being voted on v3

Patch without the conflicting LOACK tables change

Comment 9

[Marc Schumann [:Wurblzap]]

Comment on attachment 170289 [details] [diff] [review]
jump to the bug being voted on v3

All right.

+    border-width: medium;

(Although "thin" would have been sufficient for me :)

Comment 10

[Frédéric Buclin]

Comment on attachment 170289 [details] [diff] [review]
jump to the bug being voted on v3

Per discussion with Marc (wurblzap) on IRC, we agree that the LOCK TABLE bug
should be fix first. IMO, we should also consider not to allow users to edit
their votes if usevotes = 0. These are the two bugs I added as blockers of this
one.

So you should take into account if usevotes = 0/1. That will be very easy
because my patch will already check this in the "canedit" variable so that you
only need to check if canedit = 0/1. ;)

Two nits about your patch:
1) I agree with Marc, you should use border: thin instead of medium.

2) Maybe bz_bug_being_voted_on should be replaced by something like
current_bug, IMO.

Else your patch looks good! :)


I also noticed that some empty cells were missing in the table, but that's
another bug. Could you open a new bug if it does not exist yet? You can assign
it to me if you don't want to play with it. ;)

Comment 11

[GavinS]

OK - I'll update this tonight

Comment 12

[Marc Schumann [:Wurblzap]]

(In reply to comment #11)

If you update tonight, your patch may rot when the blockers go in.

(In reply to comment #10)
> 2) Maybe bz_bug_being_voted_on should be replaced by something like
> current_bug, IMO.

For my tastes, current_bug is not specific enough. You'll get my r+ with
bz_bug_being_voted_on :)

Comment 13

[GavinS]

Attachment: jump to the bug being voted on v4

Thinned out the css border, checked the canedit field, and added the canedit to
the new interface section (in anticipation of the blocking patches)

I'll risk bit-rot, and I'll update if necessary, but this will give people a
chance to review...

Comment 14

[Marc Schumann [:Wurblzap]]

Comment on attachment 170593 [details] [diff] [review]
jump to the bug being voted on v4

Denied review only because
template/en/default/bug/votes/list-for-user.html.tmpl has changed in bug
277013, so the patch doesn't apply any more.

Otherwise it's good, and I like the highlighting now :)

[An idea has come into my mind. If it's a textbox instead of a checkbox, maybe,
for bonus points, you can Javascript.focus the box of the bug in question to
save the poor user another click?]

>Index: template/en/default/bug/votes/list-for-user.html.tmpl
> [% PROCESS global/header.html.tmpl
>            title = "Show Votes"
>+           style_urls = [ "skins/standard/voting.css" ] 
>  %]

We'll get into a little trouble with the new code of bug 277013 here, which
reads

[% IF !header_done %]
  [% h2 = voting_user.login FILTER html %]
  [% PROCESS global/header.html.tmpl title = "Show Votes" %]
[% ELSE %]
  <hr>
[% END %]

That means we can include voting.css in the standard case, and we should. We'll
fail to do so if CGI.pl has called bug/process/results.html.tmpl before, which
I'd say we can live with.

Comment 15

[Marc Schumann [:Wurblzap]]

Frédéric pointed out that this is still blocked by bug 276605. I took a look,
the patches would conflict again.

Comment 16

[GavinS]

Attachment: jump to the bug being voted on v5

Updated to tip, and added javascript to both select and focus the bug to be
voted on input field - which I think works OK for both textbox and checkbox

Comment 17

[Frédéric Buclin]

Comment on attachment 172805 [details] [diff] [review]
jump to the bug being voted on v5

>Index: template/en/default/bug/votes/list-for-user.html.tmpl
>===================================================================
>-  <form method="post" action="votes.cgi">
>+  <form name="voting_form" method="post" action="votes.cgi">

Nit: "name" should be replaced by "id" if possible.


>-                  <input type="checkbox" name="[% bug.id %]" value="1"
>+                  <input type="checkbox" name="bug_[% bug.id %]" value="1"

>-                  <input name="[% bug.id %]" value="[% bug.count %]"
>+                  <input name="bug_[% bug.id %]" value="[% bug.count %]"

You cannot rename these fields! votes.cgi uses these names as bug IDs:

my @buglist = grep {/^[1-9][0-9]*$/} $cgi->param();

Doing so, all votes are being deleted because the bug list is now empty. Why do
you need to rename them?


Personally, I prefer the previous versions of your patch.

Comment 18

[GavinS]

Attachment: jump down to bug to be voted on v6

Yuck - sorry about that. The changes were to try and add Marcs suggestion to
use javascript to focus/select the textbox. I had to rename the field because
the javascript didn't like a numericly named object. I should know not to play
around with javascript - so I've dumped that.

Comment 19

[Frédéric Buclin]

Comment on attachment 172866 [details] [diff] [review]
jump down to bug to be voted on v6

>+[%# INTERFACE:
>+  # voting_user: hash containing a 'login' field

Why do we need a hash here?? Who knows! ;) But that's correct!


Works fine! r=LpSolit

Comment 20

[Marc Schumann [:Wurblzap]]

Just for the record, and to make the note not-only-mental: we still fail to load
voting.css if CGI.pl has called bug/process/results.html.tmpl before as per
comment 14, but I still think that this is something which we can live with.

Comment 21

[Frédéric Buclin]

Marc, this is not a problem because you don't want to see the "Enter new vote
here ->" comment and the gray background anymore as soon as you have voted for
that bug. I tested it and it works really as I expected. Maybe will we see a
"CSS file not found" in the apache log file, but this is really a minor point.

Comment 22

[GavinS]

Yes, if and when there is anything else useful in the voting.css file, we will have to ensure it is included 
in the case Marc mentions

Comment 23

[Dave Miller [:justdave]]

(In reply to comment #18)
> Yuck - sorry about that. The changes were to try and add Marcs suggestion to
> use javascript to focus/select the textbox. I had to rename the field because
> the javascript didn't like a numericly named object. I should know not to play
> around with javascript - so I've dumped that.

You could add an 'id' with the javascript-safe name and leave the 'name' alone.
 Or, you could change the code in the cgi to deal with the bug_ prefix and strip
it off.  I'll leave this for a day or so for comment, but since Marc and Fred
like it, I'll approve it as is if you're going to go ahead and leave it like this.

Comment 24

[Marc Schumann [:Wurblzap]]

(In reply to comment #23)

I'd say let's get this in for now. Bonus points may be earned in a separate bug :)

Comment 25

[Frédéric Buclin]

(In reply to comment #24)
> I'd say let's get this in for now. Bonus points may be earned in a separate bug :)

I agree with Marc. Adding JavaScript here is a minor enhancement compared to
this patch. Requesting approval again. :)

Comment 26

[Shane H. W. Travis]

Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.25; previous revision: 1.24
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
  edit.html.tmpl
new revision: 1.53; previous revision: 1.52
done
Checking in template/en/default/bug/votes/list-for-user.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/votes/list-for-
user.html.tmpl,v  <--  list-for-user.html.tmpl
new revision: 1.18; previous revision: 1.17
done
Checking in skins/standard/voting.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/voting.css,v  <--  voting.css
initial revision: 1.1
done

Comment 27

[GavinS]

Bug 281632 is for the javascript additions