310450 - Bugzilla should send an email when a comment becomes private or un-private

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Dim Kurt]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

Hello.

Steps to repro:
1) Create a private comment in existed bug. Bugzilla will send a email to people
who can see private commnets of this bug only.
2) Open the bug again, clear the checkbox "Private" near the comment and commit
changes. Bugzilla doesn't send email to anyone. So people did not get a
notification about the bug changes.

Bugzilla should send a notification by email when comment become a non-private.
People should get the email like the nonprivate comment was submitted at the
first time.

Reproducible: Always

Actual Results:  
People did not get a notification about the bug changes when clear the checkbox
"Private" near the comment.

Comment 1

[Frédéric Buclin]

(un)setting a comment as private doesn't update bugs.delta_ts and no entry in
the bugs_activity table is added. This should be fixed. I can reproduce the bug;
confirming.

Comment 2

[Joel Peshkin]

I dont think we should send mail to everyone... only to the people who can
normally see private comments.

Comment 3

[Joel Peshkin]

That also applies to GetBugActivity() so that changes to isprivate are only
shown to people who can see private comments.

Comment 4

[Frédéric Buclin]

Per discussion with joel, this won't go into 2.20.

Comment 5

[Max Kanat-Alexander]

I think that when a comment becomes un-private, that change should be seen by
all users.

When a comment becomes private, it should work just like a bug becoming private.

Comment 6

[Dim Kurt]

(In reply to comment #5)
> I think that when a comment becomes un-private, that change should be seen by
> all users.
> 
> When a comment becomes private, it should work just like a bug becoming private.

that's right.


I'm going to hack my 2.18 to implement this. Could you please indicate me the
primary points in the code.

Comment 7

[Joel Peshkin]

It requires a schema change.  Not a good newbie project.

Comment 8

[Dim Kurt]

(In reply to comment #7)
> It requires a schema change.
> 
Why?

Need to catch a condition of un-private comment and f.e. process bugmail
template with all recipients.
It is in process_bug.cgi:
=====================================
if ($::FORM{'id'} &&
    (Param("insidergroup") && UserInGroup(Param("insidergroup")))) {
    detaint_natural($::FORM{'id'});
    foreach my $field (keys %::FORM) {
        if ($field =~ /when-([0-9]+)/) {
            my $sequence = $1;
            my $private = $::FORM{"isprivate-$sequence"} ? 1 : 0 ;
            if ($private != $::FORM{"oisprivate-$sequence"}) {
                detaint_natural($::FORM{"$field"});
                SendSQL("UPDATE longdescs SET isprivate = $private
                    WHERE bug_id = $::FORM{'id'} AND bug_when = " .
$::FORM{"$field"});
 
### Added the condition of un-private comment
if ($private == 0){
 
    $vars->{'mailrecipients'} = { 'cc' => \@ccRemoved,
                                  'owner' => $testmail,
                                  'qa' => $origQaContact,
                                  'changer' => $::COOKIE{'Bugzilla_login'} };

    $vars->{'id'} = $::FORM{'id'};
 
$template->process("bug/process/results.html.tmpl", $vars)
      || ThrowTemplateError($template->error());
}
###
 
            }
        }
    }
}
=====================================

But in this sample i get processed bugmail template twice and all recipients are
excluded. I can't understand the way of exluding users.

Comment 9

[Dim Kurt]

Anybody work on this?

Comment 10

[Frédéric Buclin]

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

Comment 11

[Frédéric Buclin]

There is no patch to approve...

Comment 12

[Frédéric Buclin]

We are freezing the code for 3.0 in two weeks and we don't expect this bug to be fixed on time.

Comment 13

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.

Comment 14

[Max Kanat-Alexander]

It's definitely too late for this to block 3.2, particularly with no patch.

Comment 15

[Dim Kurt]

Was it realized in any other release?

Comment 16

[David Lawrence [:dkl]]

Attachment: Patch to send email notifications about comment privacy changes (v1)

Attaching patch to include comment privacy changes in email notifications.

Please review

Comment 17

[Max Kanat-Alexander]

Comment on attachment 412376 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v1)

>=== modified file 'Bugzilla/Bug.pm'
>+        elsif ($fieldname eq 'longdescs.isprivate'
>+                && !Bugzilla->user->is_insider 
>+                && $added) { 

Nit: Open-brace goes on the next line.

>+sub GetCommentNumber{

  The existence of this subroutine is unnecessary now, I think, as $bug->longdescs now returns Bugzilla::Comment objects which have a count. For objects that don't have a count, probably the best thing to do would be to create a count() accessor, for now.

>=== modified file 'Bugzilla/BugMail.pm'
>+        if ($fieldname eq 'longdescs.isprivate') {
>+            my $comment_num = Bugzilla::Bug::GetCommentNumber($id, $comment_id) || "";
>+            $what = "Comment $comment_num is private";

  Hmmm. I do hate adding a hard-coded string here. For attachments, we do it with a regex, at least, so "is private" would still be customizable. Could you just modify the field name as it exists in the DB?

>=== modified file 'template/en/default/bug/activity/table.html.tmpl'
>--- template/en/default/bug/activity/table.html.tmpl	2009-10-26 11:28:48 +0000
>+++ template/en/default/bug/activity/table.html.tmpl	2009-11-14 06:03:23 +0000
>@@ -72,7 +72,12 @@
>+              [% IF change.fieldname == 'longdescs.isprivate' %]
>+                 <a href="show_bug.cgi?id=[% bug.bug_id FILTER url_quote %]#c[% change.comment_num FILTER url_quote %]">

  Instead of that, you can use bug_link and it will actually handle things right if you pass a comment object to it.

Comment 18

[David Lawrence [:dkl]]

Attachment: Patch to send email notifications about comment privacy changes (v2)

Thanks for the review Max. I have updated the patch with your suggested changed.

Please review.
Dave

Comment 19

[Max Kanat-Alexander]

Comment on attachment 413992 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v2)

>=== modified file 'Bugzilla/Bug.pm'
>+        my ($from, $to) 
>+            = $self->{comment_isprivate}->{$comment_id}
>+            ? (0, 1)
>+            : (1, 0);


  Nit: I think that could be formatted more compactly.

>@@ -3159,6 +3173,13 @@
>+            if ($comment_id) {
>+                my $comments = Bugzilla::Comment->match({ bug_id     => $bug_id, 
>+                                                           comment_id => $comment_id });

  You don't need to use match()--$comment_id is unique. You can use new().

>+                $change{'comment_num'} = $comments->[0]->count();

  Nit: Accessors don't get () on the end.

>=== modified file 'Bugzilla/BugMail.pm'
>+        if ($fieldname eq 'longdescs.isprivate') {
>+            my $comments = Bugzilla::Comment->match({ bug_id     => $id,
>+                                                      comment_id => $comment_id });

  Same note here about match().

>+            my $comment_num = $comments->[0]->count();

  Same nit about the accessor.

>=== modified file 'Bugzilla/Comment.pm'
>+sub count {
>+    my ($self) = @_;
>+
>+    return $self->{'count'} if $self->{'count'};

  $self->{'count'} can validly be 0.

>+    ($self->{'count'}) = $dbh->selectrow_array("SELECT COUNT(*)
>+                                                  FROM longdescs 
>+                                                 WHERE bug_id = ? 
>+                                                       AND bug_when <= 
>+                                                           (SELECT bug_when 
>+                                                              FROM longdescs 
>+                                                             WHERE comment_id = ?)",

  Nit: I suspect the formatting here would get better if you started the SELECT on the next line.

>+                                               undef, $self->{'bug_id'}, $self->{'comment_id'});

  Those should be accessors, not hash accesses.

>+    return --$self->{'count'};

>=== modified file 'Bugzilla/Install/DB.pm'
> [snip]

  This bit is OK, but the Schema.pm change seems to be missing.

>=== modified file 'template/en/default/bug/activity/table.html.tmpl'

>+              [% IF change.fieldname == 'longdescs.isprivate' %]

  Maybe that should just check if the string starts with "longdesc"? That way we could (or customizers could) add other comment-specific fields, yeah?

Comment 20

[David Lawrence [:dkl]]

Attachment: Patch to send email notifications about comment privacy changes (v3)

(In reply to comment #19)
> (From update of attachment 413992 [details] [diff] [review])
> >=== modified file 'Bugzilla/Bug.pm'
> >+        my ($from, $to) 
> >+            = $self->{comment_isprivate}->{$comment_id}
> >+            ? (0, 1)
> >+            : (1, 0);
> 

Cleaned up.
 
>   Nit: I think that could be formatted more compactly.
> 
> >@@ -3159,6 +3173,13 @@
> >+            if ($comment_id) {
> >+                my $comments = Bugzilla::Comment->match({ bug_id     => $bug_id, 
> >+                                                           comment_id => $comment_id });
> 
>   You don't need to use match()--$comment_id is unique. You can use new().

Duh. Not sure why I didn't think of that but I fixed it now. Much cleaner.
 
> >+                $change{'comment_num'} = $comments->[0]->count();
> 
>   Nit: Accessors don't get () on the end.

Fixed.

> >=== modified file 'Bugzilla/Comment.pm'
> >+sub count {
> >+    my ($self) = @_;
> >+
> >+    return $self->{'count'} if $self->{'count'};
> 
>   $self->{'count'} can validly be 0.

Fixed. I changed to check for defined $self->{'count'} instead.
 
> >+    ($self->{'count'}) = $dbh->selectrow_array("SELECT COUNT(*)
> >+                                                  FROM longdescs 
> >+                                                 WHERE bug_id = ? 
> >+                                                       AND bug_when <= 
> >+                                                           (SELECT bug_when 
> >+                                                              FROM longdescs 
> >+                                                             WHERE comment_id = ?)",
> 
>   Nit: I suspect the formatting here would get better if you started the SELECT
> on the next line.

Done

> >+                                               undef, $self->{'bug_id'}, $self->{'comment_id'});
> 
>   Those should be accessors, not hash accesses.


Changed. Had to add a comment_id accessor to Bugzilla::Comment as well.

> >+    return --$self->{'count'};
> 
> >=== modified file 'Bugzilla/Install/DB.pm'
> > [snip]
> 
>   This bit is OK, but the Schema.pm change seems to be missing.
> 
> >=== modified file 'template/en/default/bug/activity/table.html.tmpl'
> 
> >+              [% IF change.fieldname == 'longdescs.isprivate' %]
> 
>   Maybe that should just check if the string starts with "longdesc"? That way
> we could (or customizers could) add other comment-specific fields, yeah?

Yeah. I changed to just look for change.comment_num.defined which should help with customizers.

Please review
Dave

Comment 21

[Max Kanat-Alexander]

> Changed. Had to add a comment_id accessor to Bugzilla::Comment as well.

  Oh, you don't need that. Look at how Bugzilla::Object::id actually works.

Comment 22

[Max Kanat-Alexander]

Comment on attachment 414329 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v3)

>+            if ($comment_id) {
>+                my $comment = Bugzilla::Comment->new($comment_id);
>+                $change{'comment_num'} = $comment->count;

  Since you have to create a comment object anyhow, why not just pass the whole comment object to the template?

>=== modified file 'Bugzilla/Comment.pm'
>+sub comment_id  { return $_[0]->{'comment_id'}; }

  That's unnecessary, as I mentioned in my previous comment.

>+sub count {
>+    my ($self) = @_;
>+
>+    return $self->{'count'} if defined $self->{'count'};
>+
>+    my $dbh = Bugzilla->dbh;
>+    ($self->{'count'}) = $dbh->selectrow_array(
>+        "SELECT COUNT(*)
>+           FROM longdescs 
>+          WHERE bug_id = ? 
>+                AND bug_when <= 
>+                (SELECT bug_when 
>+                   FROM longdescs 
>+                  WHERE comment_id = ?)",
>+        undef, $self->bug_id, $self->comment_id);

  Wait, since this is an object, don't you already have bug_when?

> __END__
>@@ -166,7 +186,19 @@
> 
> =back
> 
>-
>+=item C<count>
>+
>+=over
>+
>+=item B<Description>
>+
>+The position this comment is located in the full list of comments for a bug.

  Mention "starting with 0". Also, you don't need Description/Returns for this, I think, since it's just a simple accessor.

>=== modified file 'Bugzilla/DB/Schema.pm'
>+            comment_id => {TYPE => 'INT3'},

  That needs a foreign key.

>=== modified file 'template/en/default/bug/activity/table.html.tmpl'
bug_link(bug.bug_id, comment_num => change.comment_num) %]

  That second argument doesn't need to be wrapped with {}?

Comment 23

[David Lawrence [:dkl]]

Attachment: Patch to send email notifications about comment privacy changes (v4)

Thanks for the review. I have made the suggested changes.
As for the hash reference question in Template-Toolkit, I got that from
http://template-toolkit.org/docs/manual/Variables.html#section_Passing_Parameters_and_Returning_Values

Please review
Dave

Comment 24

[Max Kanat-Alexander]

Comment on attachment 416999 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v4)

>+C<int> The position this comment is located in the full list of comments for a bug starting from 0.

  Hmm, probably need a comma there to make things clearer. I suspect this line is longer than 80 chars, too.

>=== modified file 'Bugzilla/DB/Schema.pm'
>+            comment_id => {TYPE => 'INT3', 
>+                           REFERENCES => { TABLE  => 'longdescs',
>+                                           COLUMN => 'comment_id'}},

  I think we should probably do DELETE => 'CASCADE'. If the comment is deleted, there's no reason to track history for it, right?

  That can all be fixed on checkin, though.

  r+ pending testing.

Comment 25

[Max Kanat-Alexander]

Comment on attachment 416999 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v4)

Okay, actually, this patch doesn't seem to be working. No email is generated and no history update is performed when changing the privacy of a comment.

Comment 26

[David Lawrence [:dkl]]

(In reply to comment #25)
> (From update of attachment 416999 [details] [diff] [review])
> Okay, actually, this patch doesn't seem to be working. No email is generated
> and no history update is performed when changing the privacy of a comment.

Hmm. I am starting from a empty database, ran checksetup.pl using updated code from HEAD with patch applied. I have mail_delivery_method set to 'Test'. I had to of course set the insider group to a new private_comment group. I also created a test user that was not in the insider group.

I am not able to get this to fail on my test system. I see the see the emails being generated in data/mailer.testfile to the non-privileged user when the comment is made public when was private previously. I also see the comment privacy changes in the bug history properly. 

So it seems to be working fine for me. I am curious what the different could be in your environment.

Comment 27

[David Lawrence [:dkl]]

Hey Max. Any new information on this as to what could be different on your environment? Would love to wrap this one up.

Thanks
Dave

Comment 28

[Max Kanat-Alexander]

Comment on attachment 416999 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v4)

Oh, thanks for the ping. I'll put this back in my queue to check.

Comment 29

[Max Kanat-Alexander]

Comment on attachment 416999 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v4)

Looks great, and works. :-)

Comment 30

[Max Kanat-Alexander]

Comment on attachment 416999 [details] [diff] [review]
Patch to send email notifications about comment privacy changes (v4)

One thing needs to be fixed on checkin: The new FK on bugs_activity needs to have DELETE => 'CASCADE'.

Comment 31

[David Lawrence [:dkl]]

modified Bugzilla/Bug.pm
modified Bugzilla/BugMail.pm
modified Bugzilla/Comment.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/bug/activity/table.html.tmpl
Committed revision 6980.

Comment 32

[Max Kanat-Alexander]

Added to the release notes in bug 604256.