333648 - Activity log and bugmail doesn't show a flag change when only requester/setter changes

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

This is an interesting bug.

Summary: I replaced a bzbarsky: review+ with cbiesinger: review+, and no bugmail was sent, and activity log is unchanged.

Steps to reproduce:
1. go to bug 263213
2. edit the "Like so-ish?" attachment
3 [review]. choose the + in the "addl. review" dropdown
4. clear the "review" dropdown
5. click submit

Result: No mail is sent. Activity log does not record this activity.

Proof:
According to https://bugzilla.mozilla.org/show_activity.cgi?id=263213 the last review flag change was by bzbarsky to review+
The bug itself does not have any review+ from bzbarsky though.

Comment 1

[Frédéric Buclin]

The reason is that Flag::snapshot(), which looks at flags associated with a bug or an attachment, only cares about statuses and requestees, but not requesters/flag setters. So it first sees: review+, then review+ => no change => no entry in the activity table => no email.

The fix would be to make it take care of the requester/flag setter too, so that he would read: bz: review+, then biesi: review+ => change => entry in the activity table (with the flag setter displayed) => email generated.

The fix is pretty trivial. That's just very low in my priority list. :)

Comment 2

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 3

[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 4

[Marc Bejarano]

retargeted

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> retargeted

Do you plan to work on it? Else why retargetting it?

Comment 6

[Marc Bejarano]

back to target "---".  i think i took the "If you believe this is a mistake" differently than intended ;)

Comment 7

[Teemu Mannermaa (:wicked)]

Attachment: Add setter to activity log and bugmail, V1

This can't be this simple, can't it? This patch does seem to fix the problem but it also adds setter information to all activity log flag entries. There's also no space after setter and colon because that would break diff_strings logic.

Comment 8

[Frédéric Buclin]

Comment on attachment 348910 [details] [diff] [review]
Add setter to activity log and bugmail, V1

I don't want the requester/flag setter to be displayed all the time. The only case where it would make sense is when going from bz:review+ to biesi:review+, because going from review+ to review+ would be too confusing. In all other cases, omit it.

So your fix makes sense, but is a partial fix and update_activity() should skip the requester/flag setter before updating the bugs_activity table.

Comment 9

[Teemu Mannermaa (:wicked)]

Attachment: Add flag change to activity log and bugmail when only setter changes, V2

Here's a patch that removes setter information before flag changes are written to the activity log. Unfortunately I couldn't find an easy way to preserve setter during the rare case of going from review+ to review+ due to the funky way removed and added strings contain the changes.

Comment 10

[Frédéric Buclin]

Comment on attachment 355215 [details] [diff] [review]
Add flag change to activity log and bugmail when only setter changes, V2

>     $old_summaries = join(", ", @$old_summaries);
>     $new_summaries = join(", ", @$new_summaries);
>     my ($removed, $added) = diff_strings($old_summaries, $new_summaries);

This is the single place where we use diff_strings(). This subroutine should go away (remove it from Util.pm) and we should use diff_arrays() instead, for the reason below.


>+        $added =~ s/\S+://g;
>+        $removed =~ s/\S+://g;

If a flag name contains ":", the regexp will break everything. With the usage of diff_arrays() above, we can parse each list item individually, letting us use /^\S+:/, which will correctly catch the setter name. Otherwise, looks good.

Comment 11

[Frédéric Buclin]

Bah, too late for 3.4, despite it was easy to fix. :(

Comment 12

[Teemu Mannermaa (:wicked)]

Attachment: Add flag change to activity log and bugmail when only setter changes, V3

This gets rid of diff_strings() utility function in addition previous changes.

Comment 13

[Frédéric Buclin]

Comment on attachment 361702 [details] [diff] [review]
Add flag change to activity log and bugmail when only setter changes, V3

Looks good. I hope you tested it because I didn't. r=LpSolit

Comment 14

[Frédéric Buclin]

OK, it had a patch right before the soft freeze, and wicked just fixed the remaining review comments. Let's have it for 3.4 (this should save us some trouble when maintaining the 3.4 branch for the next 2 years). Commit asap!

Comment 15

[Teemu Mannermaa (:wicked)]

I did test this and flag changes still seem to work. Including setter only changes even though setter is never shown.

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.100; previous revision: 1.99
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.85; previous revision: 1.84
done

Comment 16

[Frédéric Buclin]

Attachment: fix regexp

The regexp is incorrect and mangles flags having ":" in their name. This patch fixes this problem.

Comment 17

[Frédéric Buclin]

tip:

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.101; previous revision: 1.100
done

3.3.4:

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.100.2.1; previous revision: 1.100
done

Comment 18

[Max Kanat-Alexander]

FWIW, you also could have done \S+? which would have done the same as the regex you changed it to.