Bug 751862 - Make it so people inactive in last 60 days can't be asked for review and aren't suggested. r=glob

3 files changed, 47 insertions, 8 deletions

diff --git a/extensions/Review/Extension.pm b/extensions/Review/Extension.pm
index eda12f734..4e5fd35a6 100644
--- a/extensions/Review/Extension.pm
+++ b/extensions/Review/Extension.pm
@@ -21,10 +21,15 @@ use Bugzilla::Install::Filesystem;
 use Bugzilla::Search;
 use Bugzilla::User;
 use Bugzilla::User::Setting;
-use Bugzilla::Util qw(clean_text);
+use Bugzilla::Util qw(clean_text datetime_from);
 
 use constant UNAVAILABLE_RE => qr/\b(?:unavailable|pto|away)\b/i;
 
+# This extension prevents you from assigning reviews to inactive reviewers,
+# with this constant defining 'inactive'. Set the value to 0 to disable this
+# prevention.
+use constant MAX_REVIEWER_LAST_SEEN_DAYS_AGO => 60;
+
 #
 # monkey-patched methods
 #
@@ -42,6 +47,7 @@ BEGIN {
     *Bugzilla::Bug::set_bug_mentors       = \&_bug_set_bug_mentors;
     *Bugzilla::User::review_count         = \&_user_review_count;
     *Bugzilla::User::reviews_blocked      = \&_user_reviews_blocked;
+    *Bugzilla::User::is_active            = \&_user_is_active;
 }
 
 #
@@ -54,12 +60,12 @@ sub _component_reviewers       { _reviewers($_[0],      'component', $_[1]) }
 sub _component_reviewers_objs  { _reviewers_objs($_[0], 'component', $_[1]) }
 
 sub _reviewers {
-    my ($object, $type, $include_disabled) = @_;
-    return join(', ', map { $_->login } @{ _reviewers_objs($object, $type, $include_disabled) });
+    my ($object, $type, $unfiltered) = @_;
+    return join(', ', map { $_->login } @{ _reviewers_objs($object, $type, $unfiltered) });
 }
 
 sub _reviewers_objs {
-    my ($object, $type, $include_disabled) = @_;
+    my ($object, $type, $unfiltered) = @_;
     if (!$object->{reviewers}) {
         my $dbh = Bugzilla->dbh;
         my $user_ids = $dbh->selectcol_arrayref(
@@ -72,9 +78,10 @@ sub _reviewers_objs {
         my $users = Bugzilla::User->new_from_list($user_ids);
         my %user_map = map { $_->id => $_ } @$users;
         my @reviewers = map { $user_map{$_} } @$user_ids;
-        if (!$include_disabled) {
+        if (!$unfiltered) {
             @reviewers = grep {
                             $_->is_enabled
+                            && $_->is_active
                             && $_->name !~ UNAVAILABLE_RE
                             && !$_->reviews_blocked
                         } @reviewers;
@@ -84,6 +91,18 @@ sub _reviewers_objs {
     return $object->{reviewers};
 }
 
+sub _user_is_active {
+    my ($self) = @_;
+
+    return 0 if !defined($self->last_seen_date);
+    return 1 if MAX_REVIEWER_LAST_SEEN_DAYS_AGO == 0; # feature disabled
+
+    my $dt = datetime_from($self->last_seen_date);
+    my $days_ago = $dt->delta_days(DateTime->now())->in_units('days');
+
+    return $days_ago <= MAX_REVIEWER_LAST_SEEN_DAYS_AGO;
+}
+
 sub _user_review_count {
     my ($self) = @_;
     if (!exists $self->{review_count}) {
@@ -576,7 +595,6 @@ sub flag_end_of_update {
     my ($self, $args) = @_;
     my ($object, $new_flags) = @$args{qw(object new_flags)};
     my $bug = $object->isa('Bugzilla::Attachment') ? $object->bug : $object;
-    return unless $bug->product_obj->reviewer_required;
 
     foreach my $orig_change (@$new_flags) {
         my $change = $orig_change; # work on a copy
@@ -587,8 +605,22 @@ sub flag_end_of_update {
         }
         my ($name, $value) = $change =~ /^(.+)(.)$/;
 
-        if ($name eq 'review' && $value eq '?' && $reviewer eq '') {
-            ThrowUserError('reviewer_required');
+        if ($name eq 'review' && $value eq '?') {
+            if ($reviewer eq '' && $bug->product_obj->reviewer_required) {
+                ThrowUserError('reviewer_required');
+            }
+
+            my $reviewer_obj = Bugzilla::User->new({
+                name => $reviewer,
+                cache => 1
+            });
+
+            if (!$reviewer_obj->is_active) {
+                ThrowUserError('reviewer_inactive', {
+                    reviewer => $reviewer_obj,
+                    timeout  => MAX_REVIEWER_LAST_SEEN_DAYS_AGO
+                });
+            }
         }
     }
 }
diff --git a/extensions/Review/template/en/default/hook/global/header-start.html.tmpl b/extensions/Review/template/en/default/hook/global/header-start.html.tmpl
index a1ced8108..8bc72ecdb 100644
--- a/extensions/Review/template/en/default/hook/global/header-start.html.tmpl
+++ b/extensions/Review/template/en/default/hook/global/header-start.html.tmpl
@@ -43,6 +43,7 @@
   review_suggestions = {
     _mentors: [
       [% FOREACH u = mentors %]
+        [% NEXT IF NOT u.is_active %]
         [% PROCESS reviewer %][% "," UNLESS loop.last %]
       [% END %]
     ],
diff --git a/extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl b/extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl
index 112751076..aafdb5445 100644
--- a/extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl
+++ b/extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl
@@ -10,6 +10,12 @@
   [% title = "Reviewer Required" %]
   You must provide a reviewer for review requests.
 
+[% ELSIF error == "reviewer_inactive" %]
+  [% title = "Reviewer Is Inactive" %]
+  You can't ask <em>[% reviewer.identity FILTER html %]</em> for a review
+  because they have not logged in to [% terms.Bugzilla %] within the last
+  [%+ timeout FILTER none %] days.
+
 [% ELSIF error == "reviewer_suggestions_param_required" %]
   [% title = "Parameter Required" %]
   You must provide either a bug_id, or a product (and optionally a