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

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
Attachments & Requests
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: glob)

Tracking

({regression})

4.2.11
Bugzilla 4.0
regression
Bug Flags:
approval +
approval4.4 +
blocking4.4.7 +
approval4.2 +
blocking4.2.12 +
approval4.0 +
blocking4.0.16 +

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.
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.
Severity: normal → critical
Summary: review-granted emails don't include review comments → review-granted emails don't include review comments (and other request-granted emails)
Created attachment 8505254 [details]
example email with problem (with Received: headers trimmed)
(Assignee)

Updated

3 years ago
Assignee: nobody → glob
Comment hidden (typo)
(Assignee)

Comment 4

3 years ago
oops - likely caused by bug 1064140; investigating.
(Assignee)

Updated

3 years ago
Component: General → Attachments & Requests
Depends on: 1064140
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Summary: review-granted emails don't include review comments (and other request-granted emails) → comments made when setting a flag from the attachment details page are not included in the "flag updated" email
Target Milestone: --- → Bugzilla 5.0
Version: Production → unspecified
(Assignee)

Comment 5

3 years ago
Created attachment 8505348 [details] [diff] [review]
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+.
Attachment #8505348 - Flags: review?(dkl)

Updated

3 years ago
Flags: blocking4.4.7?
Flags: blocking4.2.12?
Flags: blocking4.0.16?
Keywords: regression
Target Milestone: Bugzilla 5.0 → Bugzilla 4.0
Version: unspecified → 4.2.11

Comment 6

3 years ago
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.
(Assignee)

Comment 7

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

thanks for the swift feedback lpsolit; i'll work on an updated patch.
Attachment #8505348 - Attachment is obsolete: true
Attachment #8505348 - Flags: review?(dkl)
(Assignee)

Comment 8

3 years ago
Created attachment 8505497 [details] [diff] [review]
1082887_2.patch

- addresses lpsolit's comments
Attachment #8505497 - Flags: review?(dkl)
Comment on attachment 8505497 [details] [diff] [review]
1082887_2.patch

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

r=dkl
Attachment #8505497 - Flags: review?(dkl) → review+

Updated

3 years ago
Flags: approval4.0?
what about the other branches?  The comments and flags lead me to believe this isn't 4.0-only...
Flags: blocking4.4.7?
Flags: blocking4.4.7+
Flags: blocking4.2.12?
Flags: blocking4.2.12+
Flags: blocking4.0.16?
Flags: blocking4.0.16+

Updated

3 years ago
Flags: approval4.0? → approval?

Comment 11

3 years ago
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.
(Assignee)

Comment 12

3 years ago
Created attachment 8505538 [details] [diff] [review]
1082887_3.patch

- addresses comment 11 concerns
Attachment #8505497 - Attachment is obsolete: true
Attachment #8505538 - Flags: review?(dkl)
Comment on attachment 8505538 [details] [diff] [review]
1082887_3.patch

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

r=dkl
Attachment #8505538 - Flags: review?(dkl) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8505606 [details] [diff] [review]
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.
Attachment #8505538 - Attachment is obsolete: true
Attachment #8505606 - Flags: review?(dkl)
(Assignee)

Comment 15

3 years ago
Created attachment 8505607 [details] [diff] [review]
patch for 4.0, 4.2, and 4.4
Attachment #8505607 - Flags: review?(dkl)

Updated

3 years ago
Duplicate of this bug: 1081019
Comment on attachment 8505606 [details] [diff] [review]
1082887_4.patch

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

r=dkl
Attachment #8505606 - Flags: review?(dkl) → review+
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
Attachment #8505607 - Flags: review?(dkl) → review+

Updated

3 years ago
Flags: approval4.4?
Flags: approval4.2?
Flags: approval4.0?
(Assignee)

Comment 19

3 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   42d5e45..940d3fa  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   de4e564..17803aa  4.0 -> 4.0

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   b443d14..04407b8  4.2 -> 4.2

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   8e3d2de..bad9ac7  4.4 -> 4.4

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   e8a33a1..aa74918  master -> master
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Flags: approval+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.