301656 - Making a flag request of me (like review) should add me to the cc list, as my preference

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

When someone asks me to review  a patch attached to a bug, 
if I am on the CC list, I generally get two emails with the review request.
But if I'm not on the CC list, I may or may not get *any* emails.  And, not 
being on the cc list, I miss all the discussion going on about the bug I'm 
supposed to be reviewing.

So, I want bugzilla to always add me to the cc list of a bug when someone
asks me to review a patch on that bug.  I'm not suggesting that bugzilla
always do this for all users, but rather that bugzilla give me an option
to specify that for my account, whenever anyone asks me to review or super
review or "additional review" a patch, then I get added to the CC list
automatically (if I'm not already).

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This is particularly relevant to bugs in the "security group".  I've seen 
bugs sit waiting for reviews, where the reviewer (not in the security group)
could not see the bug or the attachments to review them.

Comment 3

[Max Kanat-Alexander]

  Implementation, from the dupe:

  Add a preference called "Add me to the cc list of a bug when somebody
makes a request of me" or something like that. It would be on by default.

  I think that this is nice, because it allows people to opt-out of the feature
for installations where it doesn't make sense, or where people don't want the
behavior, but for most people in most situations, it handles the problem
nicely. The only remaining problem is when the requester doesn't know that the
requestee has turned off this preference, but that's the requestee's
preference, so they probably have some reason for setting it that way and don't
want to be CC'ed. They will know to CC themselves, because they turned off the
preference.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We really need this fixed for Mozilla's Bugzilla. I regularly get review requests, provide some comments, then someone responds to my comments in the bug, but I forgot to CC myself and they didn't notice I'm not CCed so they wait for days or weeks wondering why I'm not responding.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I have the same problem.  Hearty +1 from me.

Comment 7

[:glob ✱]

Attachment: patch v1

here's a simple patch, works for both bug and attachment flags.


the setting's description is a bit wordy and i'm not totally happy with it:

"Automatically add me to the CC list of bugs I am requested to review"

Comment 8

[Frédéric Buclin]

Comment on attachment 566158 [details] [diff] [review]
patch v1

CC the requestee from within Bugzilla::Flag::_validate(), which is called by $bug->set_flags() and $attachment->set_flags(). This way, you don't need to move the code in Bug.pm at all and the hooks in Bugzilla::Object::update() have a chance to catch this change on time.

Comment 9

[:glob ✱]

(In reply to Frédéric Buclin from comment #8)
> CC the requestee from within Bugzilla::Flag::_validate()

i think a method called validate should perform data validation only, and no other actions.

Comment 10

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #9)
> i think a method called validate should perform data validation only, and no
> other actions.

Well, so let's put it at the end of Bugzilla::Flag::set_flag(). This has the same effect anyway. And it's fine to set it there. :)

Comment 11

[:glob ✱]

Attachment: patch v2

i have no idea why i didn't do this in the first revision :)

Comment 12

[Frédéric Buclin]

Comment on attachment 576512 [details] [diff] [review]
patch v2

>=== modified file 'Bugzilla/Flag.pm'

>+        foreach my $obj_flag (@{$obj_flagtype->{flags}}) {
>+            if ($obj_flag->requestee_id
>+                && $obj_flag->requestee->setting('requestee_cc') eq 'on')
>+            {
>+                $bug->add_cc($obj_flag->requestee);
>+            }
>+        }

No, put this code at the very end of set_flag(), after the last ELSE block. Else this code will only be triggered with newly created flags, not existing ones (such as request from the wind -> request with requestee, or r- -> r?).

Also, this code is wrong as you are looping over all existing flags again and again. You should only look at the edited one (set_flag() handles only one bug at a time). Simply make _validate() return $obj_flag, which is the flag which has been created/edited.

Comment 13

[:glob ✱]

Attachment: patch v3

Comment 14

[Frédéric Buclin]

Comment on attachment 576875 [details] [diff] [review]
patch v3

>=== modified file 'Bugzilla/Flag.pm'

>-        $class->_validate(undef, $obj_flagtype, $params, $bug, $attachment);
>+        $obj_flag = $class->_validate(undef, $obj_flagtype, $params, $bug, $attachment);

Here, you get $obj_flag from _validate() for new flags, which is right. But if a user cancels an existing flag request, then you also want $obj_flag be set to undef when checking if the requestee must be CC'ed or not, which you currently doesn't do. Your code for existing flags works only because requestee_id is set to undef when cancelling a flag request, but it would be more robust to get the return value of _validate() for existing flags too. Please fix that.


Now the reason of my r-: While playing with your patch, I realized that once a user is set as a requestee, this user can no longer unCC himself from the bug, because your code will bring it back immediately. As a reviewer, there are some bugs which I really don't want to be CC'ed to because they are too noisy and I don't have the time to review the patch immediately (or because the patch is super low priority). In that case, I want to be able to unCC myself while still being the requestee. So $bug->add_cc() should only be triggered when the requestee of the flag changes. There is already code in _validate() which tells you if the requestee change (to know if we should change the flag setter or not). You could define a variable $requestee_changed = 1 there and return it to set_flag(), maybe using wantarray ? ($obj_flag, $requestee_changed) : $obj_flag.

Comment 15

[:glob ✱]

(In reply to Frédéric Buclin from comment #14)
> [..] it would be more robust to get the return value of _validate() for existing
> flags too.

thanks for pointing that out, it's an easy fix :)

> Now the reason of my r-: While playing with your patch, I realized that once
> a user is set as a requestee, this user can no longer unCC himself from the
> bug, because your code will bring it back immediately.

oh, that's bad, will fix.

Comment 16

[:glob ✱]

Attachment: patch v4

fixes review points.

Comment 17

[Frédéric Buclin]

Comment on attachment 577947 [details] [diff] [review]
patch v4

>=== modified file 'Bugzilla/Flag.pm'

>+    if ($obj_flag
>+        && $requestee_changed
>+        && $obj_flag->status eq '?'
>+        && $obj_flag->requestee_id

If $obj_flag->requestee_id is defined, then we already know the status is '?'. So you can safely remove

  && $obj_flag->status eq '?'

from the check above; it's useless.


r=LpSolit with this fix on checkin.

Comment 18

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Flag.pm
modified Bugzilla/Install.pm
modified template/en/default/global/setting-descs.none.tmpl
Committed revision 8023.

Comment 19

[Frédéric Buclin]

Added to relnotes for 4.4.