Add a user pref to paste all comments in each bugmail

NEW
Unassigned

Status

()

--
enhancement
17 years ago
6 years ago

People

(Reporter: astar, Unassigned)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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.
Changing default owner of Email Notifications component to JayPee, a.k.a.
J. Paul Reed (preed@sigkill.com).  Jake will be offline for a few months.
Assignee: jake → preed

Updated

16 years ago
Severity: normal → enhancement
(Reporter)

Comment 2

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

Comment 3

14 years ago
Now that we have user specific preferences, this can be supported on a per-user
basis.  
Depends on: 98123

Updated

14 years ago
Depends on: 199048

Comment 4

14 years ago
Created attachment 177577 [details] [diff] [review]
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

14 years ago
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:

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

foo@bar.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Owner|baz@bar.com                 |foo@bar.com
             Status|NEW                         |ASSIGNED

------- Additional Comments From foo@bar.com 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

14 years ago
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.
*** Bug 271412 has been marked as a duplicate of this bug. ***

Comment 8

14 years ago
Created attachment 179083 [details] [diff] [review]
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
Attachment #177577 - Attachment is obsolete: true

Updated

14 years ago
Attachment #179083 - Flags: review?(shane.h.w.travis)

Updated

14 years ago
Blocks: 285368

Comment 9

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

Comment 10

14 years ago
Created attachment 179116 [details] [diff] [review]
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.
Attachment #179083 - Attachment is obsolete: true

Updated

14 years ago
Attachment #179083 - Flags: review?(shane.h.w.travis)

Updated

14 years ago
Blocks: 279738

Comment 11

14 years ago
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.
Assignee: preed → altlst

Comment 12

14 years ago
Created attachment 179606 [details] [diff] [review]
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.
Attachment #179116 - Attachment is obsolete: true
Attachment #179606 - Flags: review?(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

14 years ago
(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.
That's a bit of a shame. Who did the current infrastructure? Can it be easily
expanded for other preference types?

Gerv

Comment 16

14 years ago
(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.

Updated

14 years ago
Depends on: 286357
Comment on attachment 179606 [details] [diff] [review]
v4 patch

>+# 2005-03-15 altlst@sonic.net -- 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
Attachment #179606 - Flags: review?(gerv) → review-

Comment 18

14 years ago
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.
(Reporter)

Comment 19

14 years ago
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

14 years ago
> 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".
> 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

14 years ago
> 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.

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

Gerv

Updated

14 years ago
No longer blocks: 279738
Depends on: 279738

Comment 24

14 years ago
Created attachment 182536 [details] [diff] [review]
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".
Attachment #179606 - Attachment is obsolete: true
Attachment #182536 - Flags: review?(mkanat)

Updated

14 years ago
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Summary: add email preference "All comments" → Add User Setting of how many comments to send in each email
Target Milestone: --- → Bugzilla 2.22
Version: unspecified → 2.19.2

Comment 25

14 years ago
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?
Attachment #182536 - Flags: review?(mkanat) → review-
*** Bug 303530 has been marked as a duplicate of this bug. ***

Comment 27

14 years ago
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

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

Comment 29

14 years ago
(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

14 years ago
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

13 years ago
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).
Target Milestone: Bugzilla 2.22 → ---

Updated

13 years ago
QA Contact: mattyt-bugzilla → default-qa

Comment 32

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

Comment 33

10 years ago
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.
Assignee: altlist → administration

Updated

9 years ago
Status: ASSIGNED → NEW

Comment 34

6 years ago
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.
Summary: Add User Setting of how many comments to send in each email → Add a user pref to paste all comments in each bugmail
(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
You need to log in before you can comment on or make changes to this bug.