267494 - If param(usevotes) not true, 'Voter' column should not appear in email preferences

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Shane H. W. Travis]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: 

If your site doesn't use votes at all (as ours doesn't), then the fact that 
there is a 'Voter' column in userprefs.cgi?tab=email confuses a lot of people 
because they aren't used to dealing with it. Voting-related stuff doesn't 
appear anywhere else (that I'm aware) if usevotes is disabled, so no reason it 
should appear here either.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Shane H. W. Travis]

Attachment: Patch against 2.16.7

As I know this still to be an issue as of 2.16.7, here's a patch to fix it.

Comment 2

[Shane H. W. Travis]

Attachment: Patch against 2.18rc3 and 2.19.1

Still an issue as of 2.18rc3 as well, so on the assumption that it's going to
get CONFIRMED, here's a patch to fix it on that branch.

Comment 3

[Frédéric Buclin]

I don't think this is an issue for 2.16 and 2.18. IMHO, this should only go into
the trunk.

Comment 4

[Shane H. W. Travis]

Comment on attachment 164426 [details] [diff] [review]
Patch against 2.18rc3 and 2.19.1

(In reply to comment #3)
> I don't think this is an issue for 2.16 and 2.18.

If by "not an issue" you mean "doesn't happen", you're wrong; the behaviour as
described definitely exists in 2.16 and 2.18.

If by "not an issue" you mean "so trivial that nobody cares"... <shrug> All I
can say is that WE cared, and if we cared, then maybe someone else will too.

I don't expect anyone to holler 'stop the presses!' and retrofit this into
2.16/2.18, but it was trivial to make the patches (code fix was done locally
for 2.16.7 already) so I included it too. Maybe it does someone some good.

2.18rc3 patch works against 2.19.1 as well, so renamed.

Comment 5

[Frédéric Buclin]

(In reply to comment #3)
> I don't think this is an issue for 2.16 and 2.18.

What I mean here is that this is not a bug, neither something which prevents
Bugzilla from working fine nor a UI problem. This is "only" a (valid) new
feature you would like to see!

Comment 6

[Shane H. W. Travis]

(In reply to comment #5)
> What I mean here is that this is not a bug ... nor a UI problem. 

You say, "This is new functionality. It doesn't exist now, and everything works 
fine without it, so adding it is an enhancement."

I say, "This is *missing* functionality. The fact that it doesn't exist now is 
a defect in the original implementation of the usevotes parameter... although 
admittedly a minimal one (hence the 'minor' severity) since everything works 
fine without it."

You say to-may-to, I say to-mah-to... </singing> :)

Anyway, let's call the whole thing off; the patches work, they fix the 
described issue, and I don't really care where they get checked in (although 
they're small and localized enough that they shouldn't break anything 
regardless of where they get added). Bug/enhancement semantics aside, that's 
all that really matters, right?

Comment 7

[Vlad Dascalu]

Comment on attachment 164425 [details] [diff] [review]
Patch against 2.16.7

This is a bug in the UI. It's like adding an option for "strawberries" when we
have no strawberries to talk about. :-)

Comment 8

[Vlad Dascalu]

Comment on attachment 164426 [details] [diff] [review]
Patch against 2.18rc3 and 2.19.1

The nit regarding having long lines over 80 chars still applies, but it's only
a nit.

This is a low risk patch. Might be worthy for the stable branches.

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 164425 [details] [diff] [review]
Patch against 2.16.7

>--- email.html.tmpl	2002-07-17 12:28:37.000000000 -0600
>+++ email.html.tmpl.usevotes	2004-11-03 11:57:59.000000000 -0600
>@@ -99,7 +101,8 @@
> <table width="100%" border="1">
>   <tr>
>     <td colspan="[% useqacontact ? '5' : '4' %]" align="center" width="50%">
>-      <b>When my relationship to this bug is:</b>
>+    <td colspan="[% (useqacontact AND usevotes) ? '5' : ((useqacontact OR usevotes) ? '4' : '3') %]" align="center" width="50%">
>+     <b>When my relationship to this bug is:</b>
>     </td>
>     <td rowspan="2" width="50%">
>       <b>I want to receive mail when:</b>

You're adding a line with the conditional td colspan, but you're not removing
the original one, so it's in here twice now.

Comment 10

[Dave Miller [:justdave]]

It touches templates, which means we're screwing with the localizers by
committing this change.  On the 2.16 branch, since it's neither a security
issue, nor a cause of dataloss, I'm a bit loath to approve it there.  Since we
don't have a final release of 2.18, yet, I can deal with it there though.

Comment 11

[Vlad Dascalu]

(In reply to comment #10)
> It touches templates, which means we're screwing with the localizers by
> committing this change.

From IRC:

<vladd_> However it doesn't screw localizers.
<vladd_> It just keeps the bug on the localized BZs.
<vladd_> (aka they are already screwed)
<vladd_> There are no side-effects besides fixing the English installs.

Comment 12

[Shane H. W. Travis]

Attachment: Patch against 2.16.7, second try

D'oh! Nice catch, Dave. Here's the patch with the extra line removed.

Comment 13

[Vlad Dascalu]

Comment on attachment 164439 [details] [diff] [review]
Patch against 2.16.7, second try

+    <td colspan="[% (useqacontact AND usevotes) ? '5' : ((useqacontact OR
usevotes) ? '4' : '3') %]" align="center" width="50%">
+     <b>When my relationship to this bug is:</b>

Bad identation (one space only instead of two).

In the light of justdave's comment we could increase the template versioning to
1.1. Anyway, this was denied for 2.16 checkin, so unless Dave changes his mind,
it's not worthy.

Comment 14

[Frédéric Buclin]

Hmm....

What bothers me here is about this line:

+    <td colspan="[% (useqacontact AND usevotes) ? '5' : ((useqacontact OR
usevotes) ? '4' : '3') %]" align="center" width="50%">

We say width="50%" but we can have only 3 columns each explicitly having
width="10%". How does the display looks like on different browsers? Will we have
20% of blank line between the checkboxes and the description or becomes each
column wider?

Comment 15

[Shane H. W. Travis]

Attachment: Patch against 2.16.7, third try

Personally, I'm with Vlad - it doesn't HURT anyone, it just fixes a bug in the
existing English 2.16 installations... and the patch is easy enough to
understand that people could apply it to custom templates too if they wanted
to.

Whether or not Dave changes his mind, though, here's the third (and hopefully
final) patch against 2.16.7. Fixes Vlad's spacing nit, and that's all. Might
end up being useful to someone at some point.

Comment 16

[Shane H. W. Travis]

> How does the display looks like on different browsers?

In-house, we use IE, Netscape, and Mozilla. On each of those, the columns just 
get wider. Can't speak to other browsers, but I trust they'd be similar.

Comment 17

[Vlad Dascalu]

Comment on attachment 164441 [details] [diff] [review]
Patch against 2.16.7, third try

-    <td colspan="[% useqacontact ? '5' : '4' %]" align="center" width="50%">
-      <b>When my relationship to this bug is:</b>
+     <td colspan="[% (useqacontact AND usevotes) ? '5' : ((useqacontact OR
usevotes) ? '4' : '3') %]" align="center" width="50%">
+     <b>When my relationship to this bug is:</b>
     </td>

I wanted it the other way around :-)

<td> and </td> should have the same number of spaces to their left side,
compared to "column 0". Any tag should open and close on the same level.

The first inner tag should be at "2 spaces" away from its parent tag. The first
one was correct because <td> and </td> were on the same identation level, but
<b> was only one space away :-) Now, <b> is zero spaces away, and <td> is no
longer aligned with </td> :-)

Comment 18

[Shane H. W. Travis]

Attachment: Patch against 2.16.7, fourth try

Great Ghu save me from spacing nitpickers. :)

Here you go, Vlad... Catch!

Comment 19

[Dave Miller [:justdave]]

OK, I'm convinced.  Go for it.

Comment 20

[Frédéric Buclin]

+    <td colspan="[% (useqacontact AND usevotes) ? '5' : ((useqacontact OR
usevotes) ? '4' : '3') %]" align="center" width="50%">
       <b>When my relationship to this [% terms.bug %] is:</b>
     </td>
     <td rowspan="2" width="50%">

I tested Firefox 1.0 RC1, Opera 7.54 and Konqueror 3.2.0 on Mandrake Linux 10.0
Official using this patch on the trunk.

What these browsers do is to increase in some unknown way the width of each
column in order to fit the whole width of the page. That means they respect the
fact that each "Reporter", "Owner", "QAcontact", "CClist" and "Voter" column
must have the *same* width, and the description column width is set accordingly,
that is, is *more* than 50% except when all 5+1 columns are displayed. In
conclusion, none of these browsers respects the width="50%" given here.

As <td width...> is deprecated by HTML 4.01, I tried to use
<colgroup span=[3-5] width="1*"></colgroup>
<colgroup width="50%"></colgroup>

Both groups of columns have half the width of the table, as expected, but the
width of the first [3-5] columns is not equal using Konqueror and Opera. I do
not know what IE does, but <colgroup> and <col> do not seem to be widely
implemented.

So my suggestion would be to drop both width="50%" attributes, for consistency.
This could make the description column quite large if only 3 columns are
displayed (70% of the table)...

Comment 21

[Vlad Dascalu]

<justdave> hmm, you do realize that the way the preferences update code works,
if that column isn't displayed, it sets the user's preferences for that category
to off...

Comment 22

[Vlad Dascalu]

<justdave> which means if votes are later enabled, nobody will get mail on bugs
they voted for initially, if they've visited their preferences and saved them.

Comment 23

[Shane H. W. Travis]

Comment on attachment 164446 [details] [diff] [review]
Patch against 2.16.7, fourth try

Dave, please confirm; this should be ready to go for both 2.16 and 2.18 now.

Comment 24

[Vlad Dascalu]

Right now this happens for the QA column as well, so at least we will be
consistent with that. We should open a new bug regarding "disappearing prefs for
turned off params" if we want the "save" operation not to effect them. Until
then, we have this effect in the current code-base, and introducing it for the
voters prefs will not change the current behaviour :-).

As for the width thing, it happens in the current code base with the QA column
going on/off as well. So probably we should open a new bug about that as well.
Anyway, width is depreciated, and a bug about HTML-cleanup/CSS-conversion should
be opened/should fix that as well.

Comment 25

[Vlad Dascalu]

Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.14; previous revision: 1.13
done

Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.13.2.1; previous revision: 1.13
done

Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.1.2.4; previous revision: 1.1.2.3
done

Comment 26

[Vlad Dascalu]

Backing out patch since it brokes the tinderbox builds...


not ok 69 - (en/default) template/en/default/account/prefs/email.html.tmpl has
unfiltered directives:
#   103:(useqacontact AND usevotes) ? '5' : ((useqacontact OR usevotes) ? '4' : '3')
# --ERROR
#     Failed test (t/008filter.t at line 161)


Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.15; previous revision: 1.14
done

Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.13.2.2; previous revision: 1.13.2.1
done

Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.1.2.5; previous revision: 1.1.2.4
done

Comment 27

[Marc Schumann [:Wurblzap]]

The conditional directives stuff in 008filters.t that came with the bug 174942
patch is missing on the 2.16 branch.

Comment 28

[Shane H. W. Travis]

Sorry, I know nothing about tinderbox and what it's trying to prevent, so I 
don't know what's broken here. The code works, and does what it's supposed to, 
but obviously I've triggered some alarm. Vlad, if you know WHAT alarm, or how 
to fix it, please just do so and re-submit; if it waits for me to learn enough 
to do it myself, it could be a while. :(

If anyone wants to explain what I'm missing (so that I know what to fix), I'll 
get to it ASAP so this can be re-closed.

Comment 29

[GavinS]

(In reply to comment #28)
> If anyone wants to explain what I'm missing (so that I know what to fix), I'll 
> get to it ASAP so this can be re-closed.

In case no-one else has mentioned this on IRC, there is a runtests.sh script in
the Bugzilla distribution. tinderbox repeatedly runs these tests (to check for
compilation warnings, typos, missing template filtering etc...) and the results
are here:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Bugzilla

Comment 30

[Shane H. W. Travis]

Attachment: Mods to filterexceptions.pl to get the 2.16 patch to pass the tests

Thanks for the info Gavin. Unfortunately, I can't do IRC from work, and seldom
do it from home, so without your info I'd still be scratching my head. (Never
run these tests before; ya learn something new every day.)

This patch fixes the filterexceptions.pl for 2.16 so that the original patches
will pass the t/008filter test. It already seemed to pass for 2.18 just fine,
so no patch against that version, but if needed the same change could be made
in the same place for 218/tip.

Comment 31

[Vlad Dascalu]

Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.16; previous revision: 1.15
done

Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.13.2.3; previous revision: 1.13.2.2
done

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.1.2.6; previous revision: 1.1.2.5
done