199012 - Default (and b.m.o.) bug email should have "change prefs" line

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Gervase Markham [:gerv]]

The default new-or-changed bug email should have at the bottom:

Configure email: %urlbase%userprefs.cgi?tab=email

[e-mail] says:

"Hi,

I created a Mozilla Bugzilla account some months ago and didn't mess with any of
the preferences.  I've been continuously annoyed by notifications of completely
trivial events on bugs I've commented on or voted for.  Stuff like people being
added or removed from CC, other bugs being marked as dupes, etc.  It's gotten to
the point where I didn't want to contribute because I didn't want all this crap
e-mail.  I *finally* realized today that this stuff is controlled by my global
prefs, not via any settings specific to each bug.

Anyway, I expect other new users may also experience this frustration/confusion.
 Would it make sense for the default bugzilla account prefs to specify e-mail
notification for a much more select subset of events (ones an average user is
likely to care about)?  Maybe bugzilla notification e-mails should include a
line "To change when you recieve these E-mails, visit the E-mail tab of your
account preferences"?"

Gerv

Comment 1

[Jacob Steenhagen]

I remember last time we tried to put something at the bottom of the e-mail we
upset quite a few people and ended up removing it from the message body on bmo
(the reasons stuff).

That said, we could make it similar to the tagline for [e-mail]:

----
To view or change your email settings, visit:
<%urlbase%userprefs.cgi?tab=email>

(end suggested text)
Notice the "----\n" which will cause Mozilla to gray it out, hopefully allowing
the Mozilla developers to disrgard it :)

Comment 2

[Jacob Steenhagen]

Make that be (apparently Mozilla only grays the remainder if it's two dashes):

--
To view or change your email settings, visit:
<%urlbase%userprefs.cgi?tab=email>

Comment 3

[Gervase Markham [:gerv]]

Two dashes and a space, actually :-) "-- \n".

The reason people didn't like the previous text was because it was variable.
This text is fixed, and so is mentally filterable. Putting it under a sigsep
will also help - that's a good plan.

Gerv

Comment 4

[Jacob Steenhagen]

Ah, a space... no wonder I couldn't get the darned thing to go gray :)

Comment 5

[Jacob Steenhagen]

Attachment: Patch v1

I'm flexable as far as placing the "-- \n" before or after the %reasonsbody%; I
chose before because the reason seems better grouped with the pref link than
left with the changes.

Comment 6

[Gervase Markham [:gerv]]

That's OK, so far as it goes, but I think we need consensus that this is a good
idea (because of our negative experiences with reasonsbody).

Also, I chose:
Configure email: %urlbase%userprefs.cgi?tab=email

for the tagline after a lot of thought. The shorter it is, the easier it is to
ignore; and I think the above is as short as it can be while still having
sufficient meaning. If it's a full sentence, you find yourself rereading it
again and again. That was part of the problem with reasonsbody.

Gerv

Comment 7

[Jacob Steenhagen]

I chose the sentence because it's commonly used on mailing lists. Also, it is a
static description of the line below it, not a container of variable data.

I agree whole-hartedly that burring variable information in a sentence makes it
difficult to understand. That's one of the reasons I don't like the review
request messages and why I tried to get a more structured approach in bug 179582.

Comment 8

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

Like Gerv, I have mixed feelings about this.  I think it makes sense, but I
remember how people felt about reasonsbody.  In any case, Gerv is right about
compressing it onto one line.

I wonder if it would be possible to give users a link that actually turned off
mail of the type they had just received.  Then we could say (some compressed
version of):

Stop sending me this kind of mail: http...

This would not only take the user to the email preferences page (where they
could modify other settings as well), it would immediately turn off the
preference that was sending them the annoying email.  Since users have no need
to change their preferences until they get an email they don't want, this should
almost always be the right thing to do, and in the rare case when it isn't what
the user wanted, they can easily change it back.

If we did this, we'd have to make the preferences page report changes better. 
Right now it just says "The changes to your email settings have been saved.",
whereas it would have to say what specific preferences have been changed and how
so the user knows what they did.  We'd also have to figure out what to do about
emails sent for multiple reasons.

Comment 9

[Gervase Markham [:gerv]]

I'm not sure Myk's proposal would work; he's already mentioned some of the reasons:

- If the link takes you to a config page, but changes the config as it loads it,
  you can't be certain what it's changed
- This problem is compounded by the "multiple reasons mail" problem
- The feedback from a change doesn't say what changed; and doing so in an 
  understandable manner might be a challenge.

I think if it's below a sigsep and on a single line, that makes it safe enough
to try it out. If we get negative feedback, we can take it off b.m.o. again.

Gerv

Comment 10

[Gervase Markham [:gerv]]

So, can we go with:

-- 
Configure email: %urlbase%userprefs.cgi?tab=email

and see what the reaction is?

Gerv

Comment 11

[Bradley Baetz (:bbaetz)]

There are standard List-* headers we can use - pine tells you when those are
present, at least, and we can chuck a url in there. Not sure what other mailers
do that by default.

Comment 12

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

Yeah, my proposal is problematic; better to give a generic URL for now.  One
minor point: instead of "Configure email" say "Configure bugmail" to make the
message Bugzilla-specific (ideally this is "Configure Bugzilla email
notifications, but that's too long, and "bugmail" is the commonly used
abbreviation of that phrase).

Comment 13

[Gervase Markham [:gerv]]

This bugmail should have the new "changed" line on it.

Gerv

Comment 14

[Gervase Markham [:gerv]]

Another line of space, perhaps?

Gerv

Comment 15

[Gervase Markham [:gerv]]

Hmm. We have a slight problem. I've just made a patch for the trunk, but it
involves (of course) a line with a trailing space. I, for one, use an editor
which strips trailing spaces. I can, of course, set it not to do so for this
edit, but this means that everyone who edits defparams.pl in future will need to
also.

I have a cunning plan :-)

Gerv

Comment 16

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Here's a new patch, implementing the current suggestions. I've tested it
locally, and it works well - and avoids the trailing-space issue.

Gerv

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 119130 [details] [diff] [review]
Patch v.2

'Eww!' to the space thing

Did someone turn this on on bmo? Its really really annoying - given all teh
spaces arround the blank %reasonbody%, there are 5-6 lines of blank space
before this sig, so it ends up on a separate scrrren in my mailer. Stuff which
fitted on one now needs an extra keypress to scroll through.

Comment 18

[Gervase Markham [:gerv]]

> 'Eww!' to the space thing

How would you solve the problem? :-)

> Did someone turn this on on bmo? 

Yes, me, following on from Myk's comment 12.

> Its really really annoying - given all teh
> spaces arround the blank %reasonbody%, 

%reasonsbody% is not blank, it's not present at all. Having checked my recent
bugmail, I see a blank space of either two or three lines, depending on whether
the comment was submitted with a trailing \n. There are two lines of space in
the template, so that people don't get the sig mixed up with the content.

Perhaps the solution is to remove trailing whitespace from submitted comments?

Gerv

Comment 19

[Bradley Baetz (:bbaetz)]

There are sometimes up to 5, if there are no comments.

The blank lines between the header an the comments, and the comments and the
reasonboyd, and the reasonbody and the footer, are not trimmed, plus theres an
empty line where the %xxx% stuff becomes ''. We trim the entire result, but that
doesn't help, since there is now stuff at the end.

Please remove this. It is incredibly annoying. We already tell people to change
their email prefs every single time they make a change. This is really
useless... the orignal comment seems to be complaining that ccing yourself to a
a bug means that you get ccd on changes. I really don't think that tahts an
issue, or non-intuitive.

we should probably default cc-mail changes to off, but thats a separate issue.

Comment 20

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

Comment on attachment 119130 [details] [diff] [review]
Patch v.2

>Index: defparams.pl

>+--%space%

This is not the only place where we have spaces at the ends of lines.  I
actually intentionally place spaces at the ends of lines fairly regularly, f.e.
at the ends of lines in multi-line text strings to make rearranging the strings
easier.  Dave seemed to concur on IRC that being able to leave spaces at the
ends of lines is better:

<myk> am i the only one who likes being able to leave spaces at the end of
lines?
<justdave> myk: it's dangerous because some editors remove them automatically
<justdave> otherwise I'd say yes, it's better that way
<myk> justdave: generally those editors let you turn off the feature
<myk> justdave: and "dangerous" is relative.  the worst i've seen is a patch
with unnecessary whitespace-only changes, but that can be fixed; people can
turn off that feature or tell CVS to ignore those changes

My suggestion would be to disable that feature in your editor, at least when
editing Bugzilla scripts, or tell CVS to ignore the white-space only changes
when making patches and checking in changes.  Code seems like a bad place to
work around editor issues in general, and this issue doesn't seem serious
enough to be an exception.

Comment 21

[Gervase Markham [:gerv]]

> My suggestion would be to disable that feature in your editor, at least when
> editing Bugzilla scripts, 

This is a pain - partly because turning it off and on for Bugzilla only would be
annoying (and I'd forget), and partly because it's a useful feature. If you've
reformatted something and accidentally left trailing whitespace, you can't see
it - but the editor cleans it up for you.

> or tell CVS to ignore the white-space only changes
> when making patches and checking in changes.

That's also dangerous - a diff -wu is not a proper representation of the changes
you made. And sometimes you want to make whitespace changes.

But, there's a related issue - if we just have "-- ", someone editing the mail
format may accidentally remove the space, and not understand why mail programs
are no longer greying out his footer. Having the space explicit solves that problem.

Gerv

Comment 22

[Gervase Markham [:gerv]]

> The blank lines between the header an the comments, and the comments and the
> reasonboyd, and the reasonbody and the footer, are not trimmed,

This seems to me like an encouragement to unblock the mail changes so we can
templatise mail :-)

> Please remove this. It is incredibly annoying. We already tell people to 
> change their email prefs every single time they make a change. This is really
> useless...

I've had several mails from people saying things like "How do I turn all this
mail off? All I did was report a bug and now I'm drowning in the stuff." This
change is designed to help those people. 

Let's give it two weeks. If it hasn't faded into the background and people are
still finding it irritating, we could think again.

Gerv

Comment 23

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

Comment on attachment 119130 [details] [diff] [review]
Patch v.2

>This is a pain - partly because turning it off and on for Bugzilla only would be
>annoying (and I'd forget), and partly because it's a useful feature. If you've
>reformatted something and accidentally left trailing whitespace, you can't see
>it - but the editor cleans it up for you.

That's fine for new code you write, but there's trailing whitespace in Bugzilla
that was left there intentionally, and this feature will mess it up when you
edit those files.

>But, there's a related issue - if we just have "-- ", someone editing the mail
>format may accidentally remove the space, and not understand why mail programs
>are no longer greying out his footer. Having the space explicit solves that problem.

Oh, ok, that's a good reason.

>Index: defparams.pl

>+--%space%
> 
>-%reasonsbody%'
>+%reasonsbody%
>+
>+Configure bugmail: %urlbase%userprefs.cgi?tab=email'

The standard maximum acceptable signature length is four lines.  This patch
wastes has two lines of white space between the configuration URL and the
reasons, making it that much more likely for the sig to exceed the standard
length on installations using reasons.	Also, the configuration URL line is the
same for every bug mail, while the reasons change.

This would be better without the white space and with the configuration URL
first (so that its position relative to the sig separator is always the same,
which minimizes the number of things that change about the signature from email
to email):

--%space%
Configure bugmail: %urlbase%userprefs.cgi?tab=email'
%reasonsbody%

The latter point is a nit--I think it makes the most sense, but it's a minor
issue, and there are reasons for placing it after the reasons as well--but the
whitespace should certainly be removed.  Fix (at least) that, and I'll r= it,
but I'd be very interested in polling installations to find out how many of
them use reasons, since we need to decide at some point whether trailing
signatures in bug mails is good UI at all.

Comment 24

[Roland Mainz]

What about making the footer configurable by the user ?

Comment 25

[Gervase Markham [:gerv]]

Attachment: Patch v.3

With Myk's rearrangement.

Gerv

Comment 26

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

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

r=myk

Comment 27

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

a=myk

Comment 28

[Gervase Markham [:gerv]]

Fixed.

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.112; previous revision: 1.111
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.3; previous revision: 1.2
done

Gerv

Comment 29

[Dave Miller [:justdave]]

This broke checksetup.pl

Operator or semicolon missing before %reasonsbody at defparams.pl line 504.
Ambiguous use of % resolved as operator % at defparams.pl line 504.
Couldn't parse defparams.pl: syntax error at defparams.pl line 505, near "}"
Compilation failed in require at ./checksetup.pl line 320.

Comment 30

[Dave Miller [:justdave]]

Getting tired of the hourly emails from landfill telling me checksetup.pl
crashed, so I fixed it myself :)

Index: defparams.pl
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v
retrieving revision 1.112
diff -u -r1.112 defparams.pl
--- defparams.pl        8 Apr 2003 23:29:33 -0000       1.112
+++ defparams.pl        9 Apr 2003 06:29:01 -0000
@@ -500,8 +500,8 @@
 %diffs%
 
 --%space%
-Configure bugmail: %urlbase%userprefs.cgi?tab=email'
-%reasonsbody%
+Configure bugmail: %urlbase%userprefs.cgi?tab=email
+%reasonsbody%'
   },
 
   {


Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.113; previous revision: 1.112
done

Comment 31

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

Urk, sorry Dave, as reviewer I should have been on top of that.

Comment 32

[Gervase Markham [:gerv]]

Oh %$&^#.

Sorry, guys. You would have thought I could swap two lines in a file without
breaking things. Obviously not.

Gerv

Comment 33

[Bradley Baetz (:bbaetz)]

Err, weren't we going to wait two weeks or something?

Comment 34

[Gervase Markham [:gerv]]

Er... well, we can wait two weeks for b.m.o. and, if people really don't like
it, revert the change there. But I haven't heard anyone complain apart from you. :-)

Gerv