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

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpearce, Assigned: glob)

Tracking

({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)

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)
Assignee: nobody → glob
Comment hidden (typo)
oops - likely caused by bug 1064140; investigating.
Component: General → Attachments & Requests
Depends on: CVE-2014-1571
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
Posted patch 1082887_1.patch (obsolete) — Splinter Review
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

5 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

5 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.
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)
Posted patch 1082887_2.patch (obsolete) — Splinter Review
- 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+
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+
Flags: approval4.0? → approval?
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.
Posted patch 1082887_3.patch (obsolete) — Splinter Review
- 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+
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)

Updated

5 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+
Flags: approval4.4?
Flags: approval4.2?
Flags: approval4.0?
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: 5 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.