Permit assigned reviewer to vote on reviews. (#3278)

api/converters.py

@@ -602,7 +602,7 @@ def gate_value_to_json_dict(gate: Gate) -> dict[str, Any]:
       'state': gate.state,
       'requested_on': requested_on,  # YYYY-MM-DD HH:MM:SS or None
       'responded_on': responded_on,  # YYYY-MM-DD HH:MM:SS or None
-      'owners': gate.owners,
+      'assignee_emails': gate.assignee_emails,
       'next_action': next_action,  # YYYY-MM-DD or None
       'additional_review': gate.additional_review,
       'slo_initial_response': slo_initial_response,

api/converters_test.py

@@ -512,7 +512,7 @@ class GateConvertersTest(testing_config.CustomTestCase):
       'state': 4,
       'requested_on': None,
       'responded_on': None,
-      'owners': [],
+      'assignee_emails': [],
       'next_action': None,
       'additional_review': False,
       'slo_initial_response': appr_def.slo_initial_response,
@@ -527,7 +527,7 @@ class GateConvertersTest(testing_config.CustomTestCase):
     gate = Gate(
         feature_id=1, stage_id=2, gate_type=34, state=4,
         requested_on=datetime(2022, 12, 14, 1, 2, 3), # Wednesday
-        owners=['appr1@example.com', 'appr2@example.com'],
+        assignee_emails=['appr1@example.com', 'appr2@example.com'],
         next_action=datetime(2022, 12, 25),
         additional_review=True)
     gate.put()
@@ -546,7 +546,7 @@ class GateConvertersTest(testing_config.CustomTestCase):
       'state': 4,
       'requested_on': '2022-12-14 01:02:03',
       'responded_on': None,
-      'owners': ['appr1@example.com', 'appr2@example.com'],
+      'assignee_emails': ['appr1@example.com', 'appr2@example.com'],
       'next_action': '2022-12-25',
       'additional_review': True,
       'slo_initial_response': appr_def.slo_initial_response,
@@ -561,7 +561,7 @@ class GateConvertersTest(testing_config.CustomTestCase):
         feature_id=1, stage_id=2, gate_type=3, state=4,
         requested_on=datetime(2022, 12, 14, 1, 2, 3),
         responded_on=datetime(2022, 12, 20, 1, 2, 3),
-        owners=['appr1@example.com', 'appr2@example.com'],
+        assignee_emails=['appr1@example.com', 'appr2@example.com'],
         next_action=datetime(2022, 12, 25),
         additional_review=True)
     gate.put()

api/permissions_api.py

@@ -32,8 +32,6 @@ class PermissionsAPI(basehandlers.APIHandler):
     # get user permission data if signed in
     user = self.get_current_user()
     if user:
-      field_id = approval_defs.ShipApproval.field_id
-      approvers = approval_defs.get_approvers(field_id)
       user_data = {
         'can_create_feature': permissions.can_create_feature(user),
         'approvable_gate_types': sorted(

api/reviews_api.py

@@ -73,7 +73,7 @@ class VotesAPI(basehandlers.APIHandler):
     is_requesting_review = (new_state == Vote.REVIEW_REQUESTED)
     is_editor = permissions.can_edit_feature(user, feature.key.integer_id())
     approvers = approval_defs.get_approvers(gate.gate_type)
-    is_approver = permissions.can_approve_feature(user, feature, approvers)
+    is_approver = permissions.can_review_gate(user, feature, gate, approvers)
 
     if is_requesting_review and is_editor:
       return
@@ -99,16 +99,16 @@ class GatesAPI(basehandlers.APIHandler):
     if len(gates) == 0:
       return {
           'gates': [],
-          'possible_owners': {}
+          'possible_assignee_emails': {}
           }
 
     dicts = [converters.gate_value_to_json_dict(g) for g in gates]
-    possible_owners_by_gate_type: dict[int, list[str]] = {
+    possible_assignees_by_gate_type: dict[int, list[str]] = {
         gate_type: approval_defs.get_approvers(gate_type)
         for gate_type in approval_defs.APPROVAL_FIELDS_BY_ID
         }
 
     return {
         'gates': dicts,
-        'possible_owners': possible_owners_by_gate_type
+        'possible_assignee_emails': possible_assignees_by_gate_type
         }

api/reviews_api_test.py

@@ -325,7 +325,7 @@ class GatesAPITest(testing_config.CustomTestCase):
                 "state": 1,
                 "requested_on": None,
                 "responded_on": None,
-                "owners": [],
+                "assignee_emails": [],
                 "next_action": None,
                 "additional_review": False,
                 'slo_initial_response': 5,
@@ -333,7 +333,7 @@ class GatesAPITest(testing_config.CustomTestCase):
                 'slo_initial_response_remaining': None,
             },
         ],
-        "possible_owners": {
+        "possible_assignee_emails": {
             1: ["reviewer1@example.com"],
             2: ["reviewer1@example.com"],
             3: ["reviewer1@example.com"],
@@ -360,6 +360,6 @@ class GatesAPITest(testing_config.CustomTestCase):
 
     expected = {
         'gates': [],
-        'possible_owners': {}
+        'possible_assignee_emails': {}
     }
     self.assertEqual(actual, expected)

framework/basehandlers.py

@@ -357,8 +357,6 @@ class FlaskHandler(BaseHandler):
       user_pref = user_models.UserPref.get_signed_in_user_pref()
       common_data['user'] = {
         'can_create_feature': permissions.can_create_feature(user),
-        'can_approve': permissions.can_approve_feature(
-            user, None, approvers),
         'can_edit_all': permissions.can_edit_any_feature(user),
         'is_admin': permissions.can_admin_site(user),
         'editable_features': [],

framework/permissions.py

@@ -22,6 +22,7 @@ import settings
 from framework.users import User
 from internals import feature_helpers
 from internals.core_models import FeatureEntry
+from internals.review_models import Gate
 from internals.user_models import AppUser
 
 
@@ -126,15 +127,18 @@ def can_edit_feature(user: User, feature_id: int) -> bool:
       email == feature.creator_email)
 
 
-def can_approve_feature(user: User, feature: FeatureEntry, approvers) -> bool:
-  """Return True if the user is allowed to approve the given feature."""
-  # TODO(jrobbins): make this per-feature
+def can_review_gate(
+    user: User, feature: FeatureEntry, gate: Gate | None,
+    approvers: list[str]) -> bool:
+  """Return True if the user is allowed to review the given gate."""
   if not can_view_feature(user, feature):
     return False
   if can_admin_site(user):
     return True
   is_approver = user is not None and user.email() in approvers
-  return is_approver
+  is_assigned = (user is not None and gate is not None and
+                 user.email() in gate.assignee_emails)
+  return is_approver or is_assigned
 
 
 def _maybe_redirect_to_login(handler_obj):
@@ -142,7 +146,7 @@ def _maybe_redirect_to_login(handler_obj):
   if 'current_path' in common_data and 'loginStatus=False' in common_data['current_path']:
     return {}
   return handler_obj.redirect(settings.LOGIN_PAGE_URL)
-    
+
 
 def _reject_or_proceed(
     handler_obj, handler_method, handler_args, handler_kwargs,

framework/permissions_test.py

@@ -230,16 +230,16 @@ class PermissionFunctionTests(testing_config.CustomTestCase):
       creator=True, site_editor=True, admin=True, spec_mentor=True
     )
 
-  def test_can_approve_feature(self):
+  def test_can_review_gate(self):
     approvers = []
     self.check_function_results(
-        permissions.can_approve_feature, (None, approvers),
+        permissions.can_review_gate, (None, None, approvers),
         unregistered=False, registered=False,
         special=False, site_editor=False, admin=True, anon=False)
 
     approvers = ['registered@example.com']
     self.check_function_results(
-        permissions.can_approve_feature, (None, approvers),
+        permissions.can_review_gate, (None, None, approvers),
         unregistered=False, registered=True,
         special=False, site_editor=False, admin=True, anon=False)
 

internals/detect_intent.py

@@ -142,7 +142,8 @@ def is_lgtm_allowed(from_addr, feature, approval_field):
   """Return true if the user is allowed to approve this feature."""
   user = users.User(email=from_addr)
   approvers = approval_defs.get_approvers(approval_field.field_id)
-  allowed = permissions.can_approve_feature(user, feature, approvers)
+  gate = None  # TODO(jrobbins): Detect assignee who is not an approver.
+  allowed = permissions.can_review_gate(user, feature, gate, approvers)
   return allowed
 
 

internals/review_models.py

@@ -165,7 +165,7 @@ class Gate(ndb.Model):
   # The first comment or vote on this gate from a reviewer after the request.
   responded_on = ndb.DateTimeProperty()
 
-  owners = ndb.StringProperty(repeated=True)
+  assignee_emails = ndb.StringProperty(repeated=True)
   next_action = ndb.DateProperty()
   additional_review = ndb.BooleanProperty(default=False)
 

internals/slo.py

@@ -85,7 +85,7 @@ def record_comment(
   elif gate.responded_on is not None:
     return False  # We already recorded the time of the initial response.
   else:
-    is_approver = permissions.can_approve_feature(user, feature, approvers)
+    is_approver = permissions.can_review_gate(user, feature, gate, approvers)
     if is_approver:
       logging.info('SLO: Got reviewer comment as initial response')
       gate.responded_on = now_utc()

internals/slo_test.py

@@ -184,35 +184,35 @@ class SLORecordingTests(testing_config.CustomTestCase):
     self.assertEqual(self.a_date, self.gate.requested_on)
     self.assertEqual(self.a_date, self.gate.responded_on)
 
-  @mock.patch('framework.permissions.can_approve_feature')
-  def test_record_comment__not_started(self, mock_caf):
+  @mock.patch('framework.permissions.can_review_gate')
+  def test_record_comment__not_started(self, mock_crg):
     """Comments posted before the review starts don't count."""
     feature, user, approvers = 'fake feature', 'fake user', ['fake approvers']
     # Note that self.gate.requested_on is None.
-    mock_caf.return_value = False
+    mock_crg.return_value = False
     self.assertFalse(slo.record_comment(feature, self.gate, user, approvers))
-    mock_caf.return_value = True
+    mock_crg.return_value = True
     self.assertFalse(slo.record_comment(feature, self.gate, user, approvers))
     self.assertIsNone(self.gate.requested_on)
     self.assertIsNone(self.gate.responded_on)
 
-  @mock.patch('framework.permissions.can_approve_feature')
-  def test_record_comment__non_appover(self, mock_caf):
+  @mock.patch('framework.permissions.can_review_gate')
+  def test_record_comment__non_appover(self, mock_crg):
     """Comments posted during the review by non-approvers don't count."""
     feature, user, approvers = 'fake feature', 'fake user', ['fake approvers']
     self.gate.requested_on = self.a_date
-    mock_caf.return_value = False
+    mock_crg.return_value = False
     self.assertFalse(slo.record_comment(feature, self.gate, user, approvers))
     self.assertEqual(self.a_date, self.gate.requested_on)
     self.assertIsNone(self.gate.responded_on)
 
   @mock.patch('internals.slo.now_utc')
-  @mock.patch('framework.permissions.can_approve_feature')
-  def test_record_comment__appover(self, mock_caf, mock_now):
+  @mock.patch('framework.permissions.can_review_gate')
+  def test_record_comment__appover(self, mock_crg, mock_now):
     """Comments posted during the review by an approver do count."""
     feature, user, approvers = 'fake feature', 'fake user', ['fake approvers']
     self.gate.requested_on = self.a_date
-    mock_caf.return_value = True
+    mock_crg.return_value = True
     mock_now.return_value = self.a_date
     self.assertTrue(slo.record_comment(feature, self.gate, user, approvers))
     self.assertEqual(self.a_date, self.gate.requested_on)

pages/testdata/featurelist_test/test_html_rendering.html

@@ -141,11 +141,11 @@ limitations under the License.
         </div>
       </div>
       <chromedash-featurelist
-         user="{&#34;can_create_feature&#34;: true, &#34;can_approve&#34;: true, &#34;can_edit_all&#34;: true, &#34;is_admin&#34;: true, &#34;editable_features&#34;: [], &#34;email&#34;: &#34;admin@example.com&#34;, &#34;dismissed_cues&#34;: &#34;[]&#34;}" 
+         user="{&#34;can_create_feature&#34;: true, &#34;can_edit_all&#34;: true, &#34;is_admin&#34;: true, &#34;editable_features&#34;: [], &#34;email&#34;: &#34;admin@example.com&#34;, &#34;dismissed_cues&#34;: &#34;[]&#34;}" 
          signedInUser="admin@example.com" 
         isSiteEditor
          editableFeatures="[]" 
-        canApprove>
+        >
       </chromedash-featurelist>
     </div>
   </div>