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

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Attachments & Requests
P2
enhancement
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: glob)

Tracking

2.19.1
Bugzilla 4.4
Bug Flags:
approval +
testcase ?

Details

(Whiteboard: [wanted-bmo])

Attachments

(1 attachment, 3 obsolete attachments)

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).
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.

Updated

8 years ago
Priority: -- → P5

Updated

8 years ago
Duplicate of this bug: 549463

Comment 3

8 years ago
  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.
Priority: P5 → P2
Summary: asking me for a review should add me to the cc list → Asking me for a review should add me to the cc list, as my preference

Updated

8 years ago
Summary: Asking me for a review should add me to the cc list, as my preference → Making a flag request of me (like review) should add me to the cc list, as my preference

Updated

7 years ago
Duplicate of this bug: 628681
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.
I have the same problem.  Hearty +1 from me.
Whiteboard: [wanted-bmo]
(Assignee)

Comment 7

6 years ago
Created attachment 566158 [details] [diff] [review]
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"
Assignee: attach-and-request → glob
Status: NEW → ASSIGNED
Attachment #566158 - Flags: review?(LpSolit)

Comment 8

6 years ago
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.
Attachment #566158 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 9

6 years ago
(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

6 years ago
(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. :)
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Comment 11

6 years ago
Created attachment 576512 [details] [diff] [review]
patch v2

i have no idea why i didn't do this in the first revision :)
Attachment #566158 - Attachment is obsolete: true
Attachment #576512 - Flags: review?(LpSolit)

Comment 12

6 years ago
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.
Attachment #576512 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 13

6 years ago
Created attachment 576875 [details] [diff] [review]
patch v3
Attachment #576512 - Attachment is obsolete: true
Attachment #576875 - Flags: review?(LpSolit)

Comment 14

6 years ago
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.
Attachment #576875 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 15

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

Comment 16

6 years ago
Created attachment 577947 [details] [diff] [review]
patch v4

fixes review points.
Attachment #576875 - Attachment is obsolete: true
Attachment #577947 - Flags: review?(LpSolit)

Comment 17

6 years ago
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.
Attachment #577947 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval+
Keywords: relnote
(Assignee)

Comment 18

6 years ago
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Flag.pm
modified Bugzilla/Install.pm
modified template/en/default/global/setting-descs.none.tmpl
Committed revision 8023.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Flags: testcase?

Comment 19

5 years ago
Added to relnotes for 4.4.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.