116863 - Add a user pref to paste all comments in each bugmail

Status: NEW

(Bugzilla :: Email Notifications, enhancement)

Description

[Max M. Stalnaker]

Add to email preferences an option for all comments, including the description.

Motivation:  In transistioning from an ad hoc email bug system, some users
prefer to get their information from the email, including history and original
description.  The tendency is increased if the bugzilla system is on a
non-routable host and some developers are remote.

Implementation:  I did the non-UI portion by adding an extra flag to the
GetLongDescriptonAsText call in processmail.  In globals.pl, I tested the flag
and modified the behavior transparently.

I did not do the UI portion.

Comment 1

[Dave Miller [:justdave]]

Changing default owner of Email Notifications component to JayPee, a.k.a.
J. Paul Reed ([e-mail]).  Jake will be offline for a few months.

Comment 2

[Max M. Stalnaker]

It is preferred that the order of display of the comments in the email be in the
order: most recent first.

Comment 3

[Albert Ting]

Now that we have user specific preferences, this can be supported on a per-user
basis.  

Comment 4

[Albert Ting]

Attachment: initial 2.19.2+ patch

This is my first cut.  It depends on bug #199048 to be checked in as I take
into account the reverse comment setting as part of collecting all the
comments.

It's not pretty, but it works.	It basically rebuilds the comment stack for any
users that don't follow the default setting of no-reverse/single-comment mode.

Comment 5

[Shane H. W. Travis]

If you're going to do this, it doesn't seem to me that it would be THAT much 
extra work to allow the user a gradation between 'just the newest comment' 
and 'every comment since the dawn of time'. Why not offer a range of choices, 
something like:

In bugmail with comments, I want to receive:
- the new comment only
- the new comment, and the most recent comment before that
- the new comment, and the two most recent coments
- the new comment, and the three most recent comments
- the new comment, and the five most recent comments
- the new comment, and the ten most recent comments
- the new comment, and all previous comments

Even if you do this, however, my opinion is that only the *extra* comments 
should be ordered in the setting->comment_sort_order->value order; the new 
comment itself should ALWAYS be at the top of the email, right under the 
listing of what else has changed... so it would look like this:

==================================

[e-mail] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Owner|[e-mail]                 |[e-mail]
             Status|NEW                         |ASSIGNED

------- Additional Comments From [e-mail] 2005-03-16 09:53 PST -------
Baz, I can handle this one; don't worry about it.


------- Previous comments --------

(Here is where you put the number of comments the user has chosen, in the order 
that the user has chosen.)


--
Configure bugmail: https://bugzilla.mozilla.org/userprefs.cgi?tab=email

==================================

This adds much more flexibility without changing the face of the mail too much.


Albert, are you wanting to take over this, or were you just providing a first-
cut patch with how you thought it might be implemented?

Comment 6

[Albert Ting]

Expanding the options and always making the new comment be at the front sounds
reasonable.  

But some other questions:

   - should we follow the comment_sort_order setting?
   - should we look at newest_to_oldest_desc_first?  If so, would it get placed
     before or after the new comment?  
   - expanding the options means rebuilding the comment stack for each user that
     does not use the default standard.  I'm going to assume that's ok.  It also
     implies I won't be hashing the various comment stack permutations.

Comment 7

[Dave Miller [:justdave]]

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

Comment 8

[Albert Ting]

Attachment: updated patch, v2

Shane, here's an updated patch.  I addressed all the concerns mentioned in
comment #5.  I agree all new comments should appear at the top, and only sort
the older comments based on the comment_sort_order. 

Some key things:
    - I only query the mysql database once for the longdescs, and only once to
      get the default user pref values.
    - I create a default comment result string based on the default user pref 
      values. 
    - I moved GetLongDescriptionAsText from globals.pl to BugMail.pm
    - GetLongDescriptionAsText now uses the comment array from GetComments

Comment 9

[Olav Vitters]

Note: moving of GetLongDescriptionAsText out of globals.pl is bug 279738

Comment 10

[Albert Ting]

Attachment: v3 patch

Shucks, the bugzilla tip changed quite a bit since I got this working.	
Here's an updated patch based on the 3/30 tip.

Comment 11

[Shane H. W. Travis]

Albert: I just changed companies/jobs, and have been *very* inactive in 
Bugzilla for the last two weeks. I'm hoping life settles down again a bit in 
the near future and I can at least keep an eye on things like this. This isn't 
getting in to 2.20 (I don't think) so I doubt it's a huge huge rush... but if 
you want it done faster than I'm getting to it I won't be hurt if you ask 
someone else.

Comment 12

[Albert Ting]

Attachment: v4 patch

No problem Shane, hope your new job goes well!

Will ask Gerv to take a look, based on the bugzilla reviewer list.  The updated
patch does a cleaner job fetching the default settings.

Comment 13

[Gervase Markham [:gerv]]

Would it be possible to have the pref be an integer, default 1, which is the
number of comments to send?

I think some people might want quite a few comments, but don't want a 100KB
email every time someone comments on an uber-bug. If they really want them all,
they can set it to 1000 or something.

Gerv

Comment 14

[Albert Ting]

(In reply to comment #13)
> Would it be possible to have the pref be an integer, default 1, which is the
> number of comments to send?

Hi Gerv,
  
   The current patch gives you discrete choices, per comment #5 (1, 2, 3, 5,
etc.).  I wouldn't mind making the param be an integer, but I don't think the
schema is setup to support a user definable integer, just a fixed set of choices.

Comment 15

[Gervase Markham [:gerv]]

That's a bit of a shame. Who did the current infrastructure? Can it be easily
expanded for other preference types?

Gerv

Comment 16

[Albert Ting]

(In reply to comment #15)
> That's a bit of a shame. Who did the current infrastructure? Can it be easily
> expanded for other preference types?

Shane wrote it (bug #98123).  But I'd like to request this bug be based on the
current infrastructure.  It should be trivial to upgrade it if/when bugzilla
supports other preference types.

Comment 17

[Gervase Markham [:gerv]]

Comment on attachment 179606 [details] [diff] [review]
v4 patch

>+# 2005-03-15 [e-mail] -- Bug 116863
>+add_setting ("email_comments", {"new_comments" => 1,
>+                                "new_comments_plus_one" => 2,
>+                                "new_comments_plus_two" => 3,
>+                                "new_comments_plus_three" => 4,
>+                                "new_comments_plus_five" => 5,
>+                                "new_comments_plus_ten" => 6,
>+                                "all_comments" => 7},
>+             "new_comments" );

I don't think we should have an "all_comments" option. I think this allows
people to put too much load on Bugzilla servers and email servers, for very
little gain. Very few people are going to read back more than a certain
distance, and I think it's reasonable to ask them to load the bug at that
point. Sending all comments also encourages bad practice - people read the
email to find the state of the bug instead of checking the bug itself, which
may have been updated.

I suggest we have new, new + two, new + five and new + ten.

In fact, this whole feature breaks the DRY principle. I don't like it. :-|

>+   "email_comments"                   => "When receiving a bugzilla email, include this much email",

"In email notifications of bug changes, include new comments and"

>+   "new_comments"                     => "Include just the new comments",

"nothing else"

>+   "new_comments_plus_two"            => "Include the new comments plus the two most recent comments",

"the next two most recent comments"

>+   "new_comments_plus_five"           => "Include the new comments plus the five most recent comments",

"the next five most recent comments"
etc.

Do the emails when sent differentiate between new comments and "repeated"
comments? If not, they need to. Otherwise it'll annoy users, who will find it
hard to work out if they've read a comment before, and so will re-read
comments, and slow themselves down.

The more I think about this feature, the more I think it's a backwards
usability step.

Gerv

Comment 18

[Albert Ting]

I can remove the "all_comments" line.  Will probably make the a custom feature
for my company.  

> In fact, this whole feature breaks the DRY principle. I don't like it. :-|

What does "DRY" mean?

> Do the emails when sent differentiate between new comments and "repeated"
> comments? If not, they need to. Otherwise it'll annoy users, who will find it
> hard to work out if they've read a comment before, and so will re-read
> comments, and slow themselves down.

What do you mean by "repeated" comments?  It's true multiple emails will mean
redundant comments.  But some people like that.

> The more I think about this feature, the more I think it's a backwards
> usability step.

I'm not a fan of this either, but I do have some folks who really value this.

Comment 19

[Max M. Stalnaker]

I am the reporter.  I report that in a small shop a reverse sort of the long
descriptions worked well.  There was no limit clause used.  

I suspect that in a large shop, if there was a performance hit, then another
index could be added to the database to support the reverse sort.  As I think
about it, I suspect databases are smart enough to read an index backwards.

The absence of an integer type seems to be sending people out on byways.  We
were fine with the equivalent of all_comments as our only choices and I am leary
of eliminating that from your options list.  

Comment 20

[Albert Ting]

> I suspect that in a large shop, if there was a performance hit, then another
> index could be added to the database to support the reverse sort.  As I think
> about it, I suspect databases are smart enough to read an index backwards.

The current patch only does 1 query, and then reverse sorts on the fly.  It was
my impression that the developers wanted to limit the amount of SQL queries. 
Granted, I could easily see a GetReverseComments() function so that you have two
sets to work from.  Would that be better?  

> We were fine with the equivalent of all_comments as our only choices and I am 
> leary of eliminating that from your options list.  

I guess the bugzilla team would have to decide.  bugzilla.mozilla.org is quite
large and having this additional feature could be problematic.  But the
user-pref can be figured to be hard coded to just the default "1 comment"
behavior.  So I don't see any harm supporting "all-comments".

Comment 21

[Gervase Markham [:gerv]]

> What does "DRY" mean?

Sorry. It means "Don't Repeat Yourself" - i.e. don't keep information in more
than one place.

> What do you mean by "repeated" comments?  It's true multiple emails will mean
> redundant comments.  But some people like that.

I understand that; but I think that there should be a line or break between new
comments and comments the user has been sent already. This means they don't have
to remember every bug they are CCed on to know how far back to read to make sure
they've read all the new stuff, if you see what I mean.

You could do this with two queries - one for the new comments, and one for the
extra ones. (I don't think we need an extra "GetReverseComments()" function -
you are right, reversing in place is fine.)

I'm not completely familiar with how the user pref system works. Are you saying
it's possible for an admin to disable this pref globally for, say, b.m.o.?

Gerv

Comment 22

[Albert Ting]

> I understand that; but I think that there should be a line or break between new
> comments and comments the user has been sent already. This means they don't have
> to remember every bug they are CCed on to know how far back to read to make sure
> they've read all the new stuff, if you see what I mean.

Oh, that's easy, I think it already does that... :).  Bugzilla currently emails
only the new comments.  All I'm doing is appending to that list and separating the
two sections with a "previous comments below" divider.  

> I'm not completely familiar with how the user pref system works. Are you saying
> it's possible for an admin to disable this pref globally for, say, b.m.o.?

Yes.

Comment 23

[Gervase Markham [:gerv]]

I won't be available for the next week, so please find another reviewer if you
are in a rush. Apologies :-)

Gerv

Comment 24

[Albert Ting]

Attachment: v5

Updated patch.
  - updated to 5-03 tip
  - cleaned up the text strings per comment #17

And regarding the other issues in comment #17
  - there are no repeat comments.  The patch differentiates between new
comments
    and old comments, with a divider in between, as noted in #22
  - the "all comments" still exists.  For large sites, this user preference can

    be disabled and hardcoded to "just new comments"
    - about the only thing I can see is the ability to disable "all comments", 

      but still allow the other fields.  I'm not sure how that can be done with

      the current API.	Perhaps a separate "all comments" flags that if turned
      off, defaults to "10".

Comment 25

[Max Kanat-Alexander]

Comment on attachment 182536 [details] [diff] [review]
v5

  Although it sounded like a good suggestion at the time, I really don't think
we have a need for anything other than "the latest comment" or "all comments."
I really can't picture a use for the other settings that justifies the
complexity. 

  I think having just those two would simplify the code, also.

  Also, please try to do CVS diffs. It makes life easier when using the
advanced features of PatchReader on bugzilla.mozilla.org.

>--- Bugzilla/BugMail.pm	2005-03-29 22:04:30.000000000 -0800
>+++ Bugzilla/BugMail.pm	2005-03-29 22:03:26.000000000 -0800
>@@ -318,8 +318,9 @@
>         push(@diffparts, $diffpart);
>     }
> 
>-
>-    my ($newcomments, $anyprivate) = GetLongDescriptionAsText($id, $start, $end);
>+    my $defaults = Bugzilla::User::Setting::get_defaults();

  Usually, you should never need to call that function anywhere but in
editsettings.cgi and userprefs.cgi. Is there perhaps a way to structure the
code so that it's not needed?

  Anyhow, it seems like you'd always need to get all the comments, in case any
user specified "all comments."

>+    my $commentRef = Bugzilla::Bug::GetComments($id, "oldest_to_newest", $start, $end);
>+    my ($newcomments, $anyprivate) = GetLongDescriptionAsText($id, $commentRef, $defaults);

  Also -- if a user has his setting *higher* than the default... he won't be
able to get the comments, right?

>@@ -422,6 +423,14 @@
>             # So the user exists, can see the bug, and wants mail in at least
>             # one role. But do we want to send it to them?
> 
>+            # check to see if specific user requested all comments
>+            my $email_comments = $user->settings->{'email_comments'}->{'value'};
>+            my $comment_sort_order = $user->settings->{'comment_sort_order'}->{'value'};
>+            if (($email_comments ne $defaults->{'email_comments'}) ||
>+                ($comment_sort_order ne $defaults->{'comment_sort_order'})) {
>+                ($newcomments, $anyprivate) = GetLongDescriptionAsText($id, $commentRef, $defaults, $comment_sort_order, $email_comments);

  We need a solution that doesn't involve calling GetLongDescriptionAsText
per-user under any circumstances. (It will be too slow, I suspect, and it also
just doesn't seem like a good idea.)

  Also, how can you tell which comments are private?

>@@ -609,6 +618,73 @@
>     return 1;
> }
> 
>+sub GetLongDescriptionAsText ($$$$$) {

  I actually already filed another bug for moving this function -- bug 279738.
Do the moving of the function there, and make it block this bug. We also need
to do things like rename the function and remove the SendSQL calls when we move
it.

> sub GetComments {
>-    my ($id, $comment_sort_order) = (@_);
>+    my ($id, $comment_sort_order, $start, $end) = (@_);
>     $comment_sort_order = $comment_sort_order ||
>         Bugzilla->user->settings->{'comment_sort_order'}->{'value'};
>+    $start = $start || "";

  Don't use an empty string for that, just make it undef if it should not be
defined.

>@@ -688,7 +689,8 @@
>                AS time, longdescs.thetext AS body, longdescs.work_time,
>                      isprivate, already_wrapped,
>             " . $dbh->sql_date_format('longdescs.bug_when', '%Y%m%d%H%i%s') . "
>-               AS bug_when
>+               AS bug_when,
>+            " . ($start eq "" ? "1" : "bug_when > '$start' and bug_when <= '$end'") . " as new" . "

  PostgreSQL doesn't allow boolean values like that in a SELECT statement. You
have to use a CASE WHEN statement.

>+
>+    my $count = ($sort_order eq "desc" ? $#comments + 1 : 0);
>+    foreach my $comment_ref (@comments) {
>+        $comment_ref->{'count'} = $count;
>+        $count = $count + ($sort_order eq "desc" ? -1 : 1);
>+    }

  I don't fully understand this code. Could you add a comment, here?

Comment 26

[Marc Schumann [:Wurblzap]]

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

Comment 27

[Jacob Høegh]

Please can you make a "vanille" configuration? What is the absolute simplest 
way of enableing all comment sorted by new -> old in the e-mail. Enabled for 
all e-mails sent. I'm a beginner in bugzilla. So please. :-)

Comment 28

[Jacob Høegh]

Please. i'm getteing desperate. I can not apply patch ver5. What i'm i doring 
wrong? 

Comment 29

[Albert Ting]

(In reply to comment #28)
> I can not apply patch ver5. What i'm i doing wrong? 

The patch is only for 2.20rc*.  I haven't had time yet to create an updated
patch that's suitable for the bugzilla code base.  Mainly need to resolve bug
279738.  Hope to address this within a month or so.

I don't know of a simple hack.  One possibility is to modify
globals.pl:GetLongDescriptionAsText() and force $start to always be the year
1990.  Or something like that.

Comment 30

[Jacob Høegh]

Albert. Is there a simpel hack that i can hardcode in order to force all e-mail 
sent to be including all comments. I can use this very much until the final 
release is ready. I understand that this i not development but more a 
workaround.

Comment 31

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Only bug fixes are accepted, no enhancement bugs. As this bug has a pretty low activity (especially from the assignee), it's retargetted to ---. If you want to work on it and you think you can have it fixed for 2.24, please retarget it accordingly (to 2.24).

Comment 32

[Frédéric Buclin]

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

Comment 33

[Albert Ting]

We've stopped using a long time ago and doesn't look like there's a high demand for this.  If somebody wants to take over, feel free.

Comment 34

[Frédéric Buclin]

Reverting the bug summary as the original request is to get all comments, not only the last X ones. And as said in comment 25, getting the last X comments doesn't really make sense. I'm not saying I'm in favor of the original RFE either. Many (most?) bugs have several tens of comments, and emailing them all all the time seems unnecessary to me.

Comment 35

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #34)
> Reverting the bug summary as the original request is to get all comments,
> not only the last X ones. And as said in comment 25, getting the last X
> comments doesn't really make sense. I'm not saying I'm in favor of the
> original RFE either. Many (most?) bugs have several tens of comments, and
> emailing them all all the time seems unnecessary to me.

I agreed that this would not be necessary, especially when the commenter can hit the reply link on the relevant/important comments which will include them that way.

dkl