1082887 - comments made when setting a flag from the attachment details page are not included in the "flag updated" email

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Something changed recently that caused review-granted emails to no longer include the review comments made as part of the grant.

This is bad, as I often filter my mountain of bugmail by a subject of "review granted" in order to find the things that I need to land, and if I can't see the review comments I may land so I've landed patches without making the changes requested. I've already done this twice in the past few days.

Please restore the old behaviour.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Just to be clear:  this is a really major regression, and is almost certainly going to cause code to end up in the tree that shouldn't be in the tree, because people weren't aware there were review comments.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: example email with problem (with Received: headers trimmed)

Comment 3

[:glob ✱]

likely caused by bug 1073336; investigating.

Comment 4

[:glob ✱]

oops - likely caused by bug 1064140; investigating.

Comment 5

[:glob ✱]

Attachment: 1082887_1.patch

this patch:
- calls $bug->update prior to $attachment->update to ensure the comments are in the database before we read them
- adds Bugzilla::Bug::update_delta_ts()
- updates the bug's delta_ts from Bugzilla::Flag::update()
- fixes the indentation of insert() and edit() in attachment.cgi (indentation no longer changes to 2 spaces mid-function!)

will backport this patch (without indentation fixes) for older branches following an r+.

Comment 6

[Frédéric Buclin]

Comment on attachment 8505348 [details] [diff] [review]
1082887_1.patch

>+++ b/Bugzilla/Bug.pm

>-    my $sth_bug_time = $dbh->prepare('UPDATE bugs SET delta_ts = ? WHERE bug_id = ?');

>+sub update_delta_ts {
>+    my ($invocant, $bug_id, $timestamp) = @_;
>+    Bugzilla->dbh->do(

The goal to prepare() the statement was for performance reasons. Now you call do() all the time. You should use prepare_cached() instead, see bug 340160 comment 14.


Also, there is no POD for this method, so I'm pretty sure this doesn't pass tests.



>+++ b/Bugzilla/Flag.pm

>+        Bugzilla::Bug->update_delta_ts($self->bug_id, $timestamp);

Why this call here? This is supposed to be done by the caller when calling $bug->update. If you edit several flags at once, which is pretty common, it doesn't make sense to call update_delta_ts() again and again. And this is inconsistent with create() which doesn't do it. IMO, this worths a r-.



>+++ b/attachment.cgi

>+    # We have to update the attachment update the bug

I don't understand this sentence. Did you mean "We have to update the attachment after the bug"??


>+    # Commit the comment, if any.
>+    $bug->update($timestamp);
>+
>     if ($can_edit) {
>         my $changes = $attachment->update($timestamp);

You should add a comment here too that you must update the bug object before the attachment object, to correctly send comments. Else it's very likely that someone moves $bug->update in the future because he didn't pay attention to this problem.

Comment 7

[:glob ✱]

Comment on attachment 8505348 [details] [diff] [review]
1082887_1.patch

thanks for the swift feedback lpsolit; i'll work on an updated patch.

Comment 8

[:glob ✱]

Attachment: 1082887_2.patch

- addresses lpsolit's comments

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8505497 [details] [diff] [review]
1082887_2.patch

Review of attachment 8505497 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 10

[Dave Miller [:justdave]]

what about the other branches?  The comments and flags lead me to believe this isn't 4.0-only...

Comment 11

[Frédéric Buclin]

Comment on attachment 8505497 [details] [diff] [review]
1082887_2.patch

>+++ b/Bugzilla/Bug.pm

>+sub update_delta_ts {
>+    my ($invocant, $bug_id, $timestamp) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->prepare_cached("UPDATE bugs SET delta_ts=? WHERE bug_id=?");

Nit: add spaces around =.



>+++ b/attachment.cgi

>+    $bug->update($timestamp);
>+
>     if ($can_edit) {
>         my $changes = $attachment->update($timestamp);
>+        if (scalar keys %$changes) {
>+            # If there are changes, we have to update delta_ts in the DB as
>+            # well as the current bug object.
>+            Bugzilla::Bug->update_delta_ts($bug->id, $timestamp);

Err, this doesn't make sense. $bug->update above already updated delta_ts.

Comment 12

[:glob ✱]

Attachment: 1082887_3.patch

- addresses comment 11 concerns

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 8505538 [details] [diff] [review]
1082887_3.patch

Review of attachment 8505538 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 14

[:glob ✱]

Attachment: 1082887_4.patch

while working on the backports, i realised that since i missed that Bugzilla::Attachment::update() already touches the bug's delta_ts, there's no need for all the delta_ts related changes, which simplifies this patch significantly.

sorry about the high churn rate.

Comment 15

[:glob ✱]

Attachment: patch for 4.0, 4.2, and 4.4

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 8505606 [details] [diff] [review]
1082887_4.patch

Review of attachment 8505606 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 8505607 [details] [diff] [review]
patch for 4.0, 4.2, and 4.4

Review of attachment 8505607 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 19

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   42d5e45..940d3fa  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   de4e564..17803aa  4.0 -> 4.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   b443d14..04407b8  4.2 -> 4.2

To ssh://[e-mail]/bugzilla/bugzilla.git
   8e3d2de..bad9ac7  4.4 -> 4.4

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   e8a33a1..aa74918  master -> master