add the ability for users to prevent review/feedback/needinfo requests

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

5 years ago
when developers are not available (eg. due to taking time off) it doesn't always make sense for review/feedback/needinfo requests to be targeted at them.  this extends the turn-around time for reviews both immediately for the requester, and when the reviewer returns from leave and has to deal with a backlog of reviews.

we should add user preferences that allows users to prevent themselves from being set as the requestee for selected flags, and expose those preferences on the 'account information' tab as well as 'general preferences'.

we probably just need two user prefs:
  'block review and feedback requests'  yes/no
  'block needinfo requests'             yes/no

if 'block review/feedback' is set, they should be excluded from 'suggested reviewer' lists.

the error message displayed if someone set a requestee to someone who is blocking requests needs to include the user's real name, as this is where most users put information about when they are unavailable.
I feel a bit more granularity would be helpful. I don't want to block review/feedbacks/needinfos when I'm out of office. I just want to inform people that they should probably find someone else if they want a timely answer. But if they really want an info from me, I want them to be able to set the flag.
Assignee

Comment 2

5 years ago
(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #1)
> I feel a bit more granularity would be helpful. I don't want to block
> review/feedbacks/needinfos when I'm out of office. I just want to inform
> people that they should probably find someone else if they want a timely
> answer. But if they really want an info from me, I want them to be able to
> set the flag.

please file a new bug for that.  that's a larger amount of work and outside of the scope of what i think we should do here.
Assignee

Comment 3

5 years ago
Posted patch 1003701_1.patch (obsolete) — Splinter Review
- adds "block_needinfo" parameter to needinfo extension
- adds "block_reviews" (and feedback) parameter to review extension
- exposes both these parameters on the 'account information' tab of user prefs
- some minor semi-related cosmetic changes
Attachment #8417396 - Flags: review?(dkl)
Comment on attachment 8417396 [details] [diff] [review]
1003701_1.patch

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

Comments:

1. Would like to see the needinfo drop down below the comment text area to either 1) remove or 2) disable the choice if the role is someone whose needinfo requests are blocked. Disregard this if it is too much of a performance hit.

2. The changes to the account prefs tab is much nicer than the upstream but are a major change to our form which we will need to maintain going forward.
I would like to see this refactoring go upsteam and the needinfo and review blocker form elements to be placed in an extension hook such as 

[% Hook.process('non_password_form') %]

and then further down a new hook for changes that require password to be entered for extensions that might need it:

[% Hook.process('password_form') %]

3. Otherwise works well from a technical standpoint.

dkl

::: template/en/default/account/prefs/account.html.tmpl
@@ +64,5 @@
> +      <td><hr></td>
> +    </tr>
> +    <tr>
> +      <td colspan="3">
> +        Your current password is required to confirm [% can_change.join(' or ') %] changes.

FILTER html (t/008filter.t fails)

::: template/en/default/account/prefs/email.html.tmpl
@@ +60,5 @@
>    }
>  }
>  
> +document.write('<input type="button" value="Enable All Bugmail" onclick="SetCheckboxes(true); return false;">\n');
> +document.write('<input type="button" value="Disable All Bugmail" onclick="SetCheckboxes(false); return false;">\n');

This should also go upstream.
Attachment #8417396 - Flags: review?(dkl) → review-
Assignee

Comment 5

4 years ago
Posted patch 1003701_2.patch (obsolete) — Splinter Review
- moves the 'field' hook in prefs/account to a more suitable location
- uses that hook to display the needinfo and review checkboxes
- adds a 'start' hook to prefs/account and updates userprofile to use that
- needinfo entries for reporter, assignee, qa_contact, myself skipped is that
  account has needinfo blocked
- no longer updating the needinfo setting from the review extension (oops)

as per irc will file upstream bugs after this lands.
Attachment #8417396 - Attachment is obsolete: true
Attachment #8559217 - Flags: review?(dkl)
Assignee

Updated

4 years ago
Duplicate of this bug: 1135064
Comment on attachment 8559217 [details] [diff] [review]
1003701_2.patch

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

Overall looks good. Couple issues to address:

1) When a review flag is set to +/- and then changed to ? with a blocked requestee, it allows the requestee without generating an error. Otherwise it throws an error properly if the flag is not already set.
2) IMO, the suggested reviewers should omit someone that has review requests blocked (or disable but still visible) from the list next to the review/feedback flag.

After those, should be short next review.

dkl

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +182,1 @@
>              [% FOREACH mentor = bug.mentors %]

[% NEXT IF mentor.needinfo_blocked %]

::: extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +25,5 @@
>  
> +[% ELSIF error == "reviews_blocked" %]
> +  [% title = "Request Blocked" %]
> +  [% user.identity FILTER html %] is not currently accepting
> +  "[% flagtype FILTER html %]" requests.

nit: use single quotes
Attachment #8559217 - Flags: review?(dkl) → review-
Assignee

Comment 9

4 years ago
Posted patch 1003701_3.patch (obsolete) — Splinter Review
(In reply to David Lawrence [:dkl] from comment #8)
> 1) When a review flag is set to +/- and then changed to ? with a blocked
> requestee, it allows the requestee without generating an error. Otherwise it
> throws an error properly if the flag is not already set.

fixed

> 2) IMO, the suggested reviewers should omit someone that has review requests
> blocked (or disable but still visible) from the list next to the
> review/feedback flag.

this is already implemented.

> ::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
> @@ +182,1 @@
> >              [% FOREACH mentor = bug.mentors %]
> 
> [% NEXT IF mentor.needinfo_blocked %]

in my defence, the original patch pre-dates bug.mentors :)
Attachment #8559217 - Attachment is obsolete: true
Attachment #8573028 - Flags: review?(dkl)
Comment on attachment 8573028 [details] [diff] [review]
1003701_3.patch

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

>> 2) IMO, the suggested reviewers should omit someone that has review requests
>> blocked (or disable but still visible) from the list next to the
>> review/feedback flag.
>
> this is already implemented.

I see what is happening. I have a test bug with myself and you set as Mentors. I have my account set to reviews_blocked = 1. When I do review? for an attachment, it still shows my name in the suggested list (because I am a mentor) even though I have reviews_blocked set. This is because Review.suggestions does not filter the mentors like _reviewer_objs does.

dkl

::: extensions/Needinfo/template/en/default/bug/needinfo.html.tmpl
@@ +132,4 @@
>                  } else if (role == 'user') {
>                    identity = '[% user.realname || user.login FILTER html FILTER js %]';
>                  [% FOREACH mentor = bug.mentors %]
> +                  [% NEXT IF mentor.needinfo_blocked %]

You also need this in the part below that generates the <options> as I still see a mentor in the needinfo drop down that has their needinfo_blocked set.
Attachment #8573028 - Flags: review?(dkl) → review-
Assignee

Comment 11

4 years ago
Attachment #8573028 - Attachment is obsolete: true
Attachment #8574562 - Flags: review?(dkl)
Comment on attachment 8574562 [details] [diff] [review]
1003701_4.patch

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

Cool. r=dkl
Attachment #8574562 - Flags: review?(dkl) → review+
Assignee

Comment 13

4 years ago
\o/

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   9250c96..ef96ae1  master -> master
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1189500
You need to log in before you can comment on or make changes to this bug.