Implement server-side changes for review SLOs (#3062)

* progress

* Progress

* progress

* Added tests

* Actually add new test file

* Another test

* Update internals/slo.py

Co-authored-by: Kyle Ju <kyleju@google.com>

---------

Co-authored-by: Kyle Ju <kyleju@google.com>

api/comments_api.py

@@ -18,7 +18,10 @@ from typing import Any
 from framework import basehandlers
 from framework import permissions
 from internals.review_models import Activity, Amendment, Gate
-from internals import notifier, notifier_helpers
+from internals import approval_defs
+from internals import notifier
+from internals import notifier_helpers
+from internals import slo
 
 
 def amendment_to_json_dict(amendment: Amendment) -> dict[str, Any]:
@@ -92,13 +95,16 @@ class CommentsAPI(basehandlers.APIHandler):
       comment_activity.put()
 
     # Notify subscribers of new comments when user posts a comment
-    # via the gate column.
+    # via the gate column.  Also, record SLO initial response time.
     if gate_id:
       gate = Gate.get_by_id(gate_id)
       if not gate:
         self.abort(404, msg='Gate not found; notifications abort.')
       notifier_helpers.notify_subscribers_of_new_comments(
           feature, gate, user.email(), comment_content)
+      approvers = approval_defs.get_approvers(gate.gate_type)
+      if slo.record_comment(feature, gate, user, approvers):
+        gate.put()
 
     # We can only be certain which intent thread we want to post to with
     # a relevant gate ID in order to get the intent_thread_url field from

api/converters.py

@@ -22,6 +22,7 @@ from internals.core_models import FeatureEntry, MilestoneSet, Stage
 from internals.data_types import StageDict, VerboseFeatureDict
 from internals.review_models import Vote, Gate
 from internals import approval_defs
+from internals import slo
 
 
 SIMPLE_TYPES = frozenset((int, float, bool, dict, str, list))
@@ -529,7 +530,21 @@ def vote_value_to_json_dict(vote: Vote) -> dict[str, Any]:
 def gate_value_to_json_dict(gate: Gate) -> dict[str, Any]:
   next_action = str(gate.next_action) if gate.next_action else None
   requested_on = str(gate.requested_on) if gate.requested_on else None
+  responded_on = str(gate.responded_on) if gate.responded_on else None
   appr_def = approval_defs.APPROVAL_FIELDS_BY_ID.get(gate.gate_type)
+  slo_initial_response = approval_defs.DEFAULT_SLO_LIMIT
+  if appr_def:
+    slo_initial_response = appr_def.slo_initial_response
+  slo_initial_response_remaining = None
+  slo_initial_response_took = None
+  if requested_on:
+    if responded_on:
+      slo_initial_response_took = slo.weekdays_between(
+          gate.requested_on, gate.responded_on)
+    else:
+      slo_initial_response_remaining = slo.remaining_days(
+          gate.requested_on, slo_initial_response)
+
   return {
       'id': gate.key.integer_id(),
       'feature_id': gate.feature_id,
@@ -539,7 +554,11 @@ def gate_value_to_json_dict(gate: Gate) -> dict[str, Any]:
       'gate_name': appr_def.name if appr_def else 'Gate',
       '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,
       'next_action': next_action,  # YYYY-MM-DD or None
-      'additional_review': gate.additional_review
+      'additional_review': gate.additional_review,
+      'slo_initial_response': slo_initial_response,
+      'slo_initial_response_took': slo_initial_response_took,
+      'slo_initial_response_remaining': slo_initial_response_remaining,
       }

api/converters_test.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import testing_config  # Must be imported before the module under test.
+from unittest import mock
 
 from datetime import datetime
 
@@ -429,7 +430,7 @@ class FeatureConvertersTest(testing_config.CustomTestCase):
         },
         'other': {
           'view': {
-            'notes': 'other notes',                                      
+            'notes': 'other notes',
             'text': None,
             'url': None,
             'val': None,
@@ -508,21 +509,29 @@ class GateConvertersTest(testing_config.CustomTestCase):
       'gate_name': appr_def.name,
       'state': 4,
       'requested_on': None,
+      'responded_on': None,
       'owners': [],
       'next_action': None,
       'additional_review': False,
+      'slo_initial_response': 2,
+      'slo_initial_response_took': None,
+      'slo_initial_response_remaining': None,
       }
     self.assertEqual(expected, actual)
 
-  def test_maxmimal(self):
+  @mock.patch('internals.slo.now_utc')
+  def test_maxmimal(self, mock_now):
     """If a Gate has all fields set, we can convert it to JSON."""
     gate = Gate(
         feature_id=1, stage_id=2, gate_type=3, state=4,
-        requested_on=datetime(2022, 12, 14, 1, 2, 3),
+        requested_on=datetime(2022, 12, 14, 1, 2, 3), # Wednesday
         owners=['appr1@example.com', 'appr2@example.com'],
         next_action=datetime(2022, 12, 25),
         additional_review=True)
     gate.put()
+    # The review weas due on Friday 2022-12-16.
+    mock_now.return_value = datetime(2022, 12, 20, 1, 2, 3)  # Tuesday after.
+
     actual = converters.gate_value_to_json_dict(gate)
     appr_def = approval_defs.APPROVAL_FIELDS_BY_ID[gate.gate_type]
     expected = {
@@ -534,8 +543,27 @@ class GateConvertersTest(testing_config.CustomTestCase):
       'gate_name': appr_def.name,
       'state': 4,
       'requested_on': '2022-12-14 01:02:03',
+      'responded_on': None,
       'owners': ['appr1@example.com', 'appr2@example.com'],
       'next_action': '2022-12-25',
       'additional_review': True,
+      'slo_initial_response': 2,
+      'slo_initial_response_took': None,  # Review is still in-progress.
+      'slo_initial_response_remaining': -2,  # Two weekdays overdue.
       }
     self.assertEqual(expected, actual)
+
+  def test_slo_complete_review(self):
+    """If a Gate review was completed, response includes the number of days."""
+    gate = Gate(
+        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'],
+        next_action=datetime(2022, 12, 25),
+        additional_review=True)
+    gate.put()
+    actual = converters.gate_value_to_json_dict(gate)
+
+    self.assertEqual(4, actual['slo_initial_response_took'])
+    self.assertEqual(None, actual['slo_initial_response_remaining'])

api/reviews_api_test.py

@@ -324,9 +324,13 @@ class GatesAPITest(testing_config.CustomTestCase):
                 "gate_name": "Intent to Prototype",
                 "state": 1,
                 "requested_on": None,
+                "responded_on": None,
                 "owners": [],
                 "next_action": None,
                 "additional_review": False,
+                'slo_initial_response': 2,
+                'slo_initial_response_took': None,
+                'slo_initial_response_remaining': None,
             },
         ],
         "possible_owners": {

internals/approval_defs.py

@@ -22,6 +22,7 @@ import requests
 
 from framework import permissions
 from internals import core_enums
+from internals import slo
 from internals.review_models import Gate, OwnersFile, Vote
 import settings
 
@@ -56,6 +57,8 @@ TESTING_APPROVERS = [
     'vivianz@google.com',
 ]
 
+DEFAULT_SLO_LIMIT = 2  # Two weekdays in the Pacific timezone.
+
 @dataclass(eq=True, frozen=True)
 class ApprovalFieldDef:
   name: str
@@ -64,6 +67,8 @@ class ApprovalFieldDef:
   rule: str
   approvers: str | list[str]
   team_name: str
+  slo_initial_response: int = DEFAULT_SLO_LIMIT
+
 
 # Note: This can be requested manually through the UI, but it is not
 # triggered by a blink-dev thread because i2p intents are only FYIs to
@@ -154,7 +159,7 @@ APPROVAL_FIELDS_BY_ID = {
     }
 
 
-def fetch_owners(url):
+def fetch_owners(url) -> list[str]:
   """Load a list of email addresses from an OWNERS file."""
   raw_content = OwnersFile.get_raw_owner_file(url)
   if raw_content:
@@ -170,7 +175,7 @@ def fetch_owners(url):
   return decode_raw_owner_content(response.content)
 
 
-def decode_raw_owner_content(raw_content):
+def decode_raw_owner_content(raw_content) -> list[str]:
   owners = []
   decoded = base64.b64decode(raw_content).decode()
   for line in decoded.split('\n'):
@@ -184,7 +189,7 @@ def decode_raw_owner_content(raw_content):
   return owners
 
 
-def get_approvers(field_id):
+def get_approvers(field_id) -> list[str]:
   """Return a list of email addresses of users allowed to approve."""
   afd = APPROVAL_FIELDS_BY_ID[field_id]
 
@@ -279,8 +284,12 @@ def set_vote(feature_id: int,  gate_type: int | None, new_state: int,
         gate_type=gate_type, state=new_state, set_on=now, set_by=set_by_email)
     new_vote.put()
 
-  if gate and update_gate_approval_state(gate):
-    gate.put()
+  if gate:
+    votes = Vote.get_votes(gate_id=gate.key.integer_id())
+    state_was_updated = update_gate_approval_state(gate, votes)
+    slo_was_updated = slo.record_vote(gate, votes)
+    if state_was_updated or slo_was_updated:
+      gate.put()
 
 
 def get_gate_by_type(feature_id: int, gate_type: int):
@@ -323,9 +332,8 @@ def _calc_gate_state(votes: list[Vote], rule: str) -> int:
   return Gate.PREPARING
 
 
-def update_gate_approval_state(gate: Gate) -> int:
+def update_gate_approval_state(gate: Gate, votes: list[Vote]) -> bool:
   """Change the Gate state in RAM based on its votes. Return True if changed."""
-  votes = Vote.get_votes(gate_id=gate.key.integer_id())
   afd = APPROVAL_FIELDS_BY_ID.get(gate.gate_type)
   # Assume any gate of a type that is not currently supported is ONE_LGTM.
   rule = afd.rule if afd else ONE_LGTM
@@ -335,4 +343,9 @@ def update_gate_approval_state(gate: Gate) -> int:
   gate.state = new_state
   if votes:
     gate.requested_on = min(v.set_on for v in votes)
+
+  # Starting a review resets responded_on.
+  if new_state == Vote.REVIEW_REQUESTED:
+    gate.responded_on = None
+
   return True

internals/approval_defs_test.py

@@ -369,21 +369,24 @@ class UpdateTest(testing_config.CustomTestCase):
   def test_update_approval_stage__needs_update(self):
     """Gate's approval state will be updated based on votes."""
     # Gate 1 should evaluate to approved after updating.
+    votes = Vote.get_votes(gate_id=self.gate_1.key.integer_id())
     self.assertTrue(
-        approval_defs.update_gate_approval_state(self.gate_1))
+        approval_defs.update_gate_approval_state(self.gate_1, votes))
     self.assertEqual(self.gate_1.state, Vote.APPROVED)
 
   def test_update_approval_state__no_change(self):
     """Gate's approval state does not change unless it needs to."""
     # Gate 2 is already marked as approved and should not change.
+    votes = Vote.get_votes(gate_id=self.gate_2.key.integer_id())
     self.assertFalse(
-        approval_defs.update_gate_approval_state(self.gate_2))
+        approval_defs.update_gate_approval_state(self.gate_2, votes))
     self.assertEqual(self.gate_2.state, Vote.APPROVED)
 
   def test_update_approval_state__unsupported_gate_type(self):
     """If we don't recognize the gate type, assume rule ONE_LGTM."""
     self.gate_1.gate_type = 999
     # Gate 1 should evaluate to approved after updating.
+    votes = Vote.get_votes(gate_id=self.gate_1.key.integer_id())
     self.assertTrue(
-        approval_defs.update_gate_approval_state(self.gate_1))
+        approval_defs.update_gate_approval_state(self.gate_1, votes))
     self.assertEqual(self.gate_1.state, Vote.APPROVED)

internals/processes_test.py

@@ -36,6 +36,7 @@ BAKE_APPROVAL_DEF_DICT = collections.OrderedDict([
     ('field_id', 9),
     ('rule', approval_defs.ONE_LGTM),
     ('approvers', ['chef@example.com']),
+    ('slo_initial_response', 2),
     ])
 
 PI_COLD_DOUGH = processes.ProgressItem('Cold dough', 'dough')

internals/review_models.py

@@ -136,6 +136,8 @@ class Gate(ndb.Model):
   state = ndb.IntegerProperty(required=True)
   # The datetime of the first vote on this gate.
   requested_on = ndb.DateTimeProperty()
+  # The first comment or vote on this gate from a reviewer after the request.
+  responded_on = ndb.DateTimeProperty()
 
   owners = ndb.StringProperty(repeated=True)
   next_action = ndb.DateProperty()

internals/slo.py

@@ -0,0 +1,99 @@
+# Copyright 2023 Google Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License")
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import datetime
+import logging
+import pytz
+
+from framework import permissions
+from framework.users import User
+from internals.core_models import FeatureEntry
+from internals.review_models import Gate, Vote
+
+PACIFIC_TZ = pytz.timezone('US/Pacific')
+MAX_DAYS = 9999
+
+
+def is_weekday(d: datetime.datetime) -> bool:
+  """Return True if d is a weekday: Monday through Friday."""
+  return d.weekday() < 5
+
+
+def weekdays_between(start: datetime.datetime, end: datetime.datetime) -> int:
+  """Return the number of Pacific timezone weekdays between two UTC dates."""
+  d_ptz = start.astimezone(PACIFIC_TZ)
+  # The day of the request does not count.
+  d_ptz = d_ptz.replace(hour=23, minute=59, second=59)
+  end_ptz = end.astimezone(tz=PACIFIC_TZ)
+  weekday_counter = 0
+  while d_ptz < end_ptz and weekday_counter < MAX_DAYS:
+    d_ptz = d_ptz + datetime.timedelta(days=1)
+    if is_weekday(d_ptz):
+      weekday_counter += 1
+
+  return weekday_counter
+
+
+def now_utc() -> datetime.datetime:
+  """A mockable version of datetime.datetime.now()."""
+  return datetime.datetime.now()
+
+
+def remaining_days(requested_on: datetime.datetime, slo_limit: int) -> int:
+  """Return the number of weekdays before the SLO deadline is reached."""
+  # Positive: There are days remaining.
+  # Zero: The review is due today.
+  # Negative: The review is overdue.
+  return slo_limit - weekdays_between(requested_on, now_utc())
+
+
+def is_overdue(requested_on: datetime.datetime, slo_limit: int) -> bool:
+  """Return True if a review is overdue."""
+  return remaining_days(requested_on, slo_limit) < 0
+
+
+def record_vote(gate: Gate, votes: list[Vote]) -> bool:
+  """Record a Gate SLO response time if needed.  Return True if changed."""
+  if gate.requested_on is None:
+    return False  # Review has not been requested yet.
+  elif gate.responded_on is not None:
+    return False  # We already recorded the time of the initial response.
+  elif not votes:
+    return False  # No votes yet
+  else:
+    recent_vote_time = max(v.set_on for v in votes)
+    if recent_vote_time > gate.requested_on:
+      logging.info('SLO: Got reviewer vote as initial response')
+      gate.responded_on = recent_vote_time
+      return True
+
+  return False
+
+
+def record_comment(
+    feature: FeatureEntry, gate: Gate, user: User,
+    approvers: list[str]) -> bool:
+  """Record Gate SLO response time if needed. Return True if changed."""
+  if gate.requested_on is None:
+    return False  # Review has not been requested yet.
+  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)
+    if is_approver:
+      logging.info('SLO: Got reviewer comment as initial response')
+      gate.responded_on = now_utc()
+      return True
+
+  return False

internals/slo_test.py

@@ -0,0 +1,239 @@
+# Copyright 2023 Google Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License")
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import testing_config  # Must be imported before the module under test.
+
+import datetime
+from unittest import mock
+
+from framework import permissions
+from framework.users import User
+from internals.core_models import FeatureEntry
+from internals.review_models import Gate, Vote
+from internals import slo
+
+
+class SLOFunctionTests(testing_config.CustomTestCase):
+
+  def test_is_weekday(self):
+    """We can tell if a day is a weekday or weekend."""
+    self.assertFalse(slo.is_weekday(datetime.datetime(2023, 6, 4)))  # Sun
+    self.assertTrue(slo.is_weekday(datetime.datetime(2023, 6, 5)))  # Mon
+    self.assertTrue(slo.is_weekday(datetime.datetime(2023, 6, 6)))  # Tue
+    self.assertTrue(slo.is_weekday(datetime.datetime(2023, 6, 7)))  # Wed
+    self.assertTrue(slo.is_weekday(datetime.datetime(2023, 6, 8)))  # Thu
+    self.assertTrue(slo.is_weekday(datetime.datetime(2023, 6, 9)))  # Fri
+    self.assertFalse(slo.is_weekday(datetime.datetime(2023, 6, 10)))  # Sat
+
+  def test_weekdays_between__same_day(self):
+    """There are zero days between different times on the same day."""
+    start = datetime.datetime(2023, 6, 7, 12, 30, 0)
+    end = datetime.datetime(2023, 6, 7, 14, 15, 10)
+    actual = slo.weekdays_between(start, end)
+    self.assertEqual(0, actual)
+
+  def test_weekdays_between__normal(self):
+    """We can count the weekdays between dates."""
+    start = datetime.datetime(2023, 6, 7, 12, 30, 0)
+    end = datetime.datetime(2023, 6, 8, 14, 15, 10)
+    actual = slo.weekdays_between(start, end)
+    self.assertEqual(1, actual)
+
+    start = datetime.datetime(2023, 6, 7, 12, 30, 0)
+    end = datetime.datetime(2023, 6, 9, 14, 15, 10)
+    actual = slo.weekdays_between(start, end)
+    self.assertEqual(2, actual)
+
+  def test_weekdays_between__friday_into_weekend(self):
+    """We can count the weekdays between dates."""
+    start = datetime.datetime(2023, 6, 9, 12, 30, 0)  # Fri
+    end = datetime.datetime(2023, 6, 10, 14, 15, 10)  # Sat
+    actual = slo.weekdays_between(start, end)
+    # Friday does not count because it is the day of the request,
+    # and Saturday is a weekend.
+    self.assertEqual(0, actual)
+
+  def test_weekdays_between__backwards(self):
+    """If end is before start, that counts as zero."""
+    start = datetime.datetime(2023, 6, 7, 12, 30, 0)
+    end = datetime.datetime(2023, 6, 1, 14, 15, 10)
+    actual = slo.weekdays_between(start, end)
+    self.assertEqual(0, actual)
+
+  def test_weekdays_between__huge(self):
+    """We can stop counting at 9999 days."""
+    start = datetime.datetime(1970, 6, 7, 12, 30, 0)
+    end = datetime.datetime(2111, 6, 9, 14, 15, 10)
+    actual = slo.weekdays_between(start, end)
+    self.assertEqual(9999, actual)
+
+  def test_now_utc(self):
+    """This function returns a datetime."""
+    actual = slo.now_utc()
+    self.assertEqual(datetime.datetime, type(actual))
+
+  @mock.patch('internals.slo.now_utc')
+  def test_remaining_days__starting_midweek(self, mock_now):
+    """We can calculate days remaining in the SLO limit."""
+    start = datetime.datetime(2023, 6, 7, 12, 30, 0)  # Wed
+    mock_now.return_value = datetime.datetime(2023, 6, 7, 12, 30, 0)  # Wed
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(2, actual)
+
+    mock_now.return_value = datetime.datetime(2023, 6, 8, 12, 30, 0)  # Thu
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(1, actual)
+
+    mock_now.return_value = datetime.datetime(2023, 6, 9, 12, 30, 0)  # Fri
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(0, actual)
+
+    mock_now.return_value = datetime.datetime(2023, 6, 10, 12, 30, 0)  # Sat
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(0, actual)
+
+    mock_now.return_value = datetime.datetime(2023, 6, 11, 12, 30, 0)  # Sun
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(0, actual)
+
+    mock_now.return_value = datetime.datetime(2023, 6, 12, 12, 30, 0)  # Mon
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(-1, actual)
+
+  @mock.patch('internals.slo.now_utc')
+  def test_remaining_days__starting_weekend(self, mock_now):
+    """We can calculate days remaining in the SLO limit."""
+    start = datetime.datetime(2023, 6, 10, 12, 30, 0)  # Sat
+    mock_now.return_value = datetime.datetime(2023, 6, 11, 12, 30, 0)  # Sun
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(2, actual)
+
+    mock_now.return_value = datetime.datetime(2023, 6, 12, 12, 30, 0)  # Mon
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(1, actual)
+
+  @mock.patch('internals.slo.now_utc')
+  def test_remaining_days__backwards(self, mock_now):
+    """If the end is before the start, we count zero days used."""
+    start = datetime.datetime(2023, 6, 7, 12, 30, 0)  # Wed
+    mock_now.return_value = datetime.datetime(2023, 6, 7, 12, 30, 0)  # Tue
+    actual = slo.remaining_days(start, 2)
+    self.assertEqual(2, actual)
+
+  @mock.patch('internals.slo.now_utc')
+  def test_is_overdue(self, mock_now):
+    """We can calculate days remaining in the SLO limit."""
+    start = datetime.datetime(2023, 6, 7, 12, 30, 0)  # Wed
+    mock_now.return_value = datetime.datetime(2023, 6, 7, 12, 30, 0)  # Wed
+
+    mock_now.return_value = datetime.datetime(2023, 6, 8, 12, 30, 0)  # Thu
+    self.assertFalse(slo.is_overdue(start, 2))
+
+    mock_now.return_value = datetime.datetime(2023, 6, 9, 12, 30, 0)  # Fri
+    self.assertFalse(slo.is_overdue(start, 2))
+
+    mock_now.return_value = datetime.datetime(2023, 6, 10, 12, 30, 0)  # Sat
+    self.assertFalse(slo.is_overdue(start, 2))
+
+    mock_now.return_value = datetime.datetime(2023, 6, 11, 12, 30, 0)  # Sun
+    self.assertFalse(slo.is_overdue(start, 2))
+
+    mock_now.return_value = datetime.datetime(2023, 6, 12, 12, 30, 0)  # Mon
+    self.assertTrue(slo.is_overdue(start, 2))
+
+
+class SLORecordingTests(testing_config.CustomTestCase):
+
+  def setUp(self):
+    self.gate = Gate(feature_id=1, stage_id=2, gate_type=3, state=4)
+    self.vote_review_requested = Vote(
+        feature_id=1, gate_id=2, gate_type=3, state=Vote.REVIEW_REQUESTED,
+        set_on=datetime.datetime(2023, 6, 7, 1, 2, 3),  # Wed
+        set_by='feature-owner@example.com')
+    self.vote_needs_work = Vote(
+        feature_id=1, gate_id=2, gate_type=3, state=Vote.NEEDS_WORK,
+        set_on=datetime.datetime(2023, 6, 12, 1, 2, 3),  # Mon
+        set_by='reviewer@example.com')
+    self.a_date = datetime.datetime(2023, 6, 17, 1, 2, 3)
+
+  def test_record_vote__not_started(self):
+    """If this somehow gets called before the review starts, it's a no-op."""
+    # Note that self.gate.requested_on is None.
+    self.assertFalse(slo.record_vote(self.gate, []))
+    self.assertFalse(slo.record_vote(self.gate, [self.vote_review_requested]))
+    self.assertFalse(slo.record_vote(
+        self.gate, [self.vote_review_requested, self.vote_needs_work]))
+    self.assertIsNone(self.gate.requested_on)
+    self.assertIsNone(self.gate.responded_on)
+
+  def test_record_vote__just_started(self):
+    """If checked after the request but before the response, it's a no-op."""
+    self.gate.requested_on = self.vote_review_requested.set_on
+    self.assertFalse(slo.record_vote(self.gate, []))
+    self.assertFalse(slo.record_vote(self.gate, [self.vote_review_requested]))
+    self.assertEqual(self.vote_review_requested.set_on, self.gate.requested_on)
+    self.assertIsNone(self.gate.responded_on)
+
+  def test_record_vote__got_response(self):
+    """If called with the reviewer's response, that is recorded."""
+    self.gate.requested_on = self.vote_review_requested.set_on
+    self.assertTrue(slo.record_vote(
+        self.gate, [self.vote_review_requested, self.vote_needs_work]))
+    self.assertEqual(self.vote_review_requested.set_on, self.gate.requested_on)
+    self.assertEqual(self.vote_needs_work.set_on, self.gate.responded_on)
+
+  def test_record_vote__already_finished(self):
+    """If this gets called after the review is done, it's a no-op."""
+    self.gate.requested_on = self.a_date
+    self.gate.responded_on = self.a_date
+    self.assertFalse(slo.record_vote(self.gate, []))
+    self.assertFalse(slo.record_vote(self.gate, [self.vote_review_requested]))
+    self.assertFalse(slo.record_vote(
+        self.gate, [self.vote_review_requested, self.vote_needs_work]))
+    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):
+    """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
+    self.assertFalse(slo.record_comment(feature, self.gate, user, approvers))
+    mock_caf.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):
+    """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
+    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):
+    """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_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)
+    self.assertEqual(self.a_date, self.gate.responded_on)

requirements.dev.txt

@@ -12,3 +12,4 @@ types-protobuf
 types-redis
 types-requests
 types-setuptools
+types-pytz