Make pending review and recent review queries use Gate.state (#2515)

* Progress

* partly fixed tests

* Updated unit tests

* Put back old search specs for open SPA tabs.

* Added some more indexes that NDB says are needed

* Add indexes needed by NDB

client-src/elements/chromedash-myfeatures-page.js

@@ -87,10 +87,10 @@ export class ChromedashMyFeaturesPage extends LitElement {
   renderPendingAndRecentApprovals() {
     const pendingBox = this.renderBox(
       'Features pending my approval', 'pending-approval-by:me', 'approvals',
-      'approvals.requested_on');
+      'gate.requested_on');
     const recentBox = this.renderBox(
       'Recently reviewed features', 'is:recently-reviewed', 'normal',
-      '-approvals.reviewed_on', false);
+      '-gate.reviewed_on', false);
     return [pendingBox, recentBox];
   }
 

index.yaml

@@ -431,3 +431,23 @@ indexes:
   - name: stage_type
   - name: milestones.webview_last
   - name: feature_id
+- kind: Gate
+  properties:
+  - name: state
+  - name: feature_id
+- kind: Gate
+  properties:
+  - name: gate_type
+  - name: state
+  - name: feature_id
+- kind: Gate
+  properties:
+  - name: state
+  - name: requested_on
+    direction: desc
+  - name: feature_id
+- kind: Gate
+  properties:
+  - name: state
+  - name: requested_on
+  - name: feature_id

internals/approval_defs.py

@@ -248,5 +248,7 @@ def update_gate_approval_state(gate: Gate) -> int:
   votes = Vote.get_votes(gate_id=gate.key.integer_id())
   afd = APPROVAL_FIELDS_BY_ID[gate.gate_type]
   gate.state = _calc_gate_state(votes, afd.rule)
+  if votes:
+    gate.requested_on = min(v.set_on for v in votes)
   gate.put()
   return gate.state

internals/review_models.py

@@ -199,42 +199,6 @@ class OwnersFile(ndb.Model):
     return owners_file.raw_content
 
 
-class Gate(ndb.Model):  # copy from ApprovalConfig
-  """Gates regulate the completion of a stage."""
-
-  PREPARING = 0
-
-  feature_id = ndb.IntegerProperty(required=True)
-  stage_id = ndb.IntegerProperty(required=True)
-  gate_type = ndb.IntegerProperty(required=True)  # copy from field_id
-
-  # Can one of Vote states or PREPARING.
-  state = ndb.IntegerProperty(required=True)  # calc from Approval
-
-  owners = ndb.StringProperty(repeated=True)
-  next_action = ndb.DateProperty()
-  additional_review = ndb.BooleanProperty(default=False)
-
-  # TODO(jrobbins): implement request_review()
-
-  def is_resolved(self) -> bool:
-    """Return if the Gate's outcome has been decided."""
-    return self.state == Vote.APPROVED or self.state == Vote.DENIED
-
-  def is_approved(self) -> bool:
-    """Return if the Gate approval requirements have been met."""
-    return self.state == Vote.APPROVED
-
-  @classmethod
-  def get_feature_gates(cls, feature_id: int) -> dict[int, list[Gate]]:
-    """Return a dictionary of stages associated with a given feature."""
-    gates: list[Gate] = Gate.query(Gate.feature_id == feature_id).fetch()
-    gates_dict = collections.defaultdict(list)
-    for gate in gates:
-      gates_dict[gate.gate_type].append(gate)
-    return gates_dict
-
-
 class Vote(ndb.Model):  # copy from Approval
   """One approver's vote on what the state of a gate should be."""
 
@@ -295,8 +259,46 @@ class Vote(ndb.Model):  # copy from Approval
   # Note: set_vote() moved to approval_defs.py
 
 
+class Gate(ndb.Model):  # copy from ApprovalConfig
+  """Gates regulate the completion of a stage."""
+
+  PREPARING = 0
+  PENDING_STATES = [Vote.REVIEW_REQUESTED, Vote.REVIEW_STARTED, Vote.NEEDS_WORK]
+  FINAL_STATES = [Vote.NA, Vote.APPROVED, Vote.DENIED]
+
+  feature_id = ndb.IntegerProperty(required=True)
+  stage_id = ndb.IntegerProperty(required=True)
+  gate_type = ndb.IntegerProperty(required=True)  # copy from field_id
+
+  # Calculated from Vote states.  Can be one of Vote states or PREPARING.
+  state = ndb.IntegerProperty(required=True)
+  # The datetime of the first vote on this gate.
+  requested_on = ndb.DateTimeProperty()
+
+  owners = ndb.StringProperty(repeated=True)
+  next_action = ndb.DateProperty()
+  additional_review = ndb.BooleanProperty(default=False)
+
+  # TODO(jrobbins): implement request_review()
+
+  def is_resolved(self) -> bool:
+    """Return if the Gate's outcome has been decided."""
+    return self.state == Vote.APPROVED or self.state == Vote.DENIED
+
+  def is_approved(self) -> bool:
+    """Return if the Gate approval requirements have been met."""
+    return self.state == Vote.APPROVED
+
+  @classmethod
+  def get_feature_gates(cls, feature_id: int) -> dict[int, list[Gate]]:
+    """Return a dictionary of stages associated with a given feature."""
+    gates: list[Gate] = Gate.query(Gate.feature_id == feature_id).fetch()
+    gates_dict = collections.defaultdict(list)
+    for gate in gates:
+      gates_dict[gate.gate_type].append(gate)
+    return gates_dict
+
 
-# Note: This class is not used yet.
 class Amendment(ndb.Model):
   """Activity log entries can record changes to fields."""
   field_name = ndb.StringProperty()  # from QUERIABLE_FIELDS
@@ -304,9 +306,6 @@ class Amendment(ndb.Model):
   new_value = ndb.TextProperty()
 
 
-# Note: This class is not used yet.
-# TODO(jrobbins): Decide on either copying to this new class or adding
-# and removing fields from the existing Comment class.
 class Activity(ndb.Model):  # copy from Comment
   """An activity log entry (comment + amendments) on a gate or feature."""
   feature_id = ndb.IntegerProperty(required=True)

internals/search.py

@@ -35,29 +35,20 @@ MAX_TERMS = 6
 DEFAULT_RESULTS_PER_PAGE = 100
 
 
-def _get_referenced_feature_ids(
-    approvals: list[review_models.Approval], reverse=False) -> list[int]:
-  """Retrieve the features being approved, withuot duplicates."""
-  logging.info('approvals is %r', [(a.feature_id, a.state) for a in approvals])
-  feature_ids = utils.dedupe(a.feature_id for a in approvals)
-
-  return feature_ids
-
-
 def process_pending_approval_me_query() -> list[int]:
   """Return a list of features needing approval by current user."""
   user = users.get_current_user()
   if not user:
     return []
 
-  approvable_fields_ids = approval_defs.fields_approvable_by(user)
-  pending_approvals = review_models.Approval.get_approvals(
-      states=[review_models.Approval.REVIEW_REQUESTED])
-  pending_approvals = [pa for pa in pending_approvals
-                       if pa.field_id in approvable_fields_ids]
-
-  feature_ids = _get_referenced_feature_ids(pending_approvals)
-  return feature_ids
+  approvable_gate_types = approval_defs.fields_approvable_by(user)
+  if not approvable_gate_types:
+    return []
+  query = review_models.Gate.query(
+      review_models.Gate.state.IN(review_models.Gate.PENDING_STATES),
+      review_models.Gate.gate_type.IN(approvable_gate_types))
+  future_feature_ids = query.fetch_async(projection=['feature_id'])
+  return future_feature_ids
 
 
 def process_starred_me_query() -> list[int]:
@@ -72,15 +63,10 @@ def process_starred_me_query() -> list[int]:
 
 def process_recent_reviews_query() -> list[int]:
   """Return features that were reviewed recently."""
-  user = users.get_current_user()
-  if not user:
-    return []
-
-  recent_approvals = review_models.Approval.get_approvals(
-      states=review_models.Approval.FINAL_STATES, limit=40)
-
-  feature_ids = _get_referenced_feature_ids(recent_approvals, reverse=True)
-  return feature_ids
+  query = review_models.Gate.query(
+      review_models.Gate.state.IN(review_models.Gate.FINAL_STATES))
+  future_feature_ids = query.fetch_async(projection=['feature_id'], limit=40)
+  return future_feature_ids
 
 
 def parse_query_value(val_str: str) -> Union[bool, datetime.datetime, int, str]:

internals/search_queries.py

@@ -176,17 +176,17 @@ def _sorted_by_joined_model(
 def sorted_by_pending_request_date(descending: bool) -> list[int]:
   """Return feature_ids of pending approvals sorted by request date."""
   return _sorted_by_joined_model(
-      review_models.Approval,
-      review_models.Approval.state == review_models.Approval.REVIEW_REQUESTED,
-      descending, review_models.Approval.set_on)
+      review_models.Gate,
+      review_models.Gate.state.IN(review_models.Gate.PENDING_STATES),
+      descending, review_models.Gate.requested_on)
 
 
 def sorted_by_review_date(descending: bool) -> list[int]:
   """Return feature_ids of reviewed approvals sorted by last review."""
   return _sorted_by_joined_model(
-      review_models.Approval,
-      review_models.Approval.state.IN(review_models.Approval.FINAL_STATES),
-      descending, review_models.Approval.set_on)
+      review_models.Gate,
+      review_models.Gate.state.IN(review_models.Gate.FINAL_STATES),
+      descending, review_models.Gate.requested_on)
 
 
 QUERIABLE_FIELDS: dict[str, Property] = {
@@ -336,6 +336,10 @@ STAGE_TYPES_BY_QUERY_FIELD: dict[str, dict[int, Optional[int]]] = {
 
 SORTABLE_FIELDS: dict[str, Union[Property, Callable]] = QUERIABLE_FIELDS.copy()
 SORTABLE_FIELDS.update({
+    # TODO(jrobbins): remove the 'approvals.*' items after 2023-01-01.
     'approvals.requested_on': sorted_by_pending_request_date,
     'approvals.reviewed_on': sorted_by_review_date,
+
+    'gate.requested_on': sorted_by_pending_request_date,
+    'gate.reviewed_on': sorted_by_review_date,
     })

internals/search_queries_test.py

@@ -54,41 +54,63 @@ class SearchFeaturesTest(testing_config.CustomTestCase):
     self.feature_3.put()
     self.feature_3_id = self.feature_3.key.integer_id()
 
-    self.approval_1_1 = review_models.Approval(
-        feature_id=self.feature_1_id, field_id=1,
-        state=review_models.Approval.REVIEW_REQUESTED,
+    self.gate_1 = review_models.Gate(
+        feature_id=self.feature_1_id, stage_id=1,
+        gate_type=core_models.GATE_PROTOTYPE,
+        state=review_models.Vote.APPROVED,
+        requested_on=datetime.datetime(2022, 7, 1))
+    self.gate_1.put()
+    self.gate_1_id = self.gate_1.key.integer_id()
+
+    self.vote_1_1 = review_models.Vote(
+        feature_id=self.feature_1_id, gate_type=core_models.GATE_PROTOTYPE,
+        gate_id=self.gate_1_id,
+        state=review_models.Vote.REVIEW_REQUESTED,
         set_on=datetime.datetime(2022, 7, 1),
         set_by='feature_owner@example.com')
-    self.approval_1_1.put()
+    self.vote_1_1.put()
 
-    self.approval_1_2 = review_models.Approval(
-        feature_id=self.feature_1_id, field_id=1,
-        state=review_models.Approval.APPROVED,
+    self.vote_1_2 = review_models.Vote(
+        feature_id=self.feature_1_id, gate_type=core_models.GATE_PROTOTYPE,
+        gate_id=self.gate_1_id,
+        state=review_models.Vote.APPROVED,
         set_on=datetime.datetime(2022, 7, 2),
         set_by='reviewer@example.com')
-    self.approval_1_2.put()
+    self.vote_1_2.put()
 
-    self.approval_2_1 = review_models.Approval(
-        feature_id=self.feature_2_id, field_id=1,
-        state=review_models.Approval.REVIEW_REQUESTED,
+    self.gate_2 = review_models.Gate(
+        feature_id=self.feature_2_id, stage_id=1,
+        gate_type=core_enums.GATE_SHIP,
+        state=review_models.Vote.REVIEW_REQUESTED,
+        requested_on=datetime.datetime(2022, 8, 1))
+    self.gate_2.put()
+    self.gate_2_id = self.gate_2.key.integer_id()
+
+    self.vote_2_1 = review_models.Vote(
+        feature_id=self.feature_2_id, gate_type=core_enums.GATE_SHIP,
+        gate_id=self.gate_2_id,
+        state=review_models.Vote.REVIEW_REQUESTED,
         set_on=datetime.datetime(2022, 8, 1),
         set_by='feature_owner@example.com')
-    self.approval_2_1.put()
+    self.vote_2_1.put()
 
-    self.approval_2_2 = review_models.Approval(
-        feature_id=self.feature_2_id, field_id=1,
-        state=review_models.Approval.APPROVED,
+    self.vote_2_2 = review_models.Vote(
+        feature_id=self.feature_2_id, gate_type=core_enums.GATE_SHIP,
+        gate_id=self.gate_2_id,
+        state=review_models.Vote.APPROVED,
         set_on=datetime.datetime(2022, 8, 2),
         set_by='reviewer@example.com')
-    self.approval_2_2.put()
+    self.vote_2_2.put()
 
   def tearDown(self):
     self.feature_1.key.delete()
     self.feature_2.key.delete()
     self.feature_3.key.delete()
     self.stage_1_ship.key.delete()
-    for appr in review_models.Approval.query():
-      appr.key.delete()
+    for gate in review_models.Gate.query():
+      gate.key.delete()
+    for vote in review_models.Vote.query():
+      vote.key.delete()
 
   def test_single_field_query_async__normal(self):
     """We get a promise to run the DB query, which produces results."""
@@ -234,17 +256,17 @@ class SearchFeaturesTest(testing_config.CustomTestCase):
         [self.feature_3_id, self.feature_2_id, self.feature_1_id], actual)
 
   def test_total_order_query_async__requested_on(self):
-    """We can get feature IDs sorted by approval review requests."""
-    actual = search_queries.total_order_query_async('approvals.requested_on')
+    """We can get feature IDs sorted by gate review requests."""
+    actual = search_queries.total_order_query_async('gate.requested_on')
     self.assertEqual(
-        [self.feature_1_id, self.feature_2_id],
+        [self.feature_2_id],
         actual)
 
   def test_total_order_query_async__reviewed_on(self):
-    """We can get feature IDs sorted by approval granting times."""
-    actual = search_queries.total_order_query_async('approvals.reviewed_on')
+    """We can get feature IDs sorted by gate resolution times."""
+    actual = search_queries.total_order_query_async('gate.reviewed_on')
     self.assertEqual(
-        [self.feature_1_id, self.feature_2_id],
+        [self.feature_1_id],
         actual)
 
   def test_stage_fields_have_join_conditions(self):
@@ -252,7 +274,7 @@ class SearchFeaturesTest(testing_config.CustomTestCase):
     self.assertCountEqual(
         search_queries.STAGE_QUERIABLE_FIELDS.keys(),
         search_queries.STAGE_TYPES_BY_QUERY_FIELD.keys())
-  
+
   def test_negate_operator(self):
     """We can get correct negated operators"""
     actual = search_queries.negate_operator('=')

internals/search_test.py

@@ -146,8 +146,8 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
     self.featureentry_1.key.delete()
     self.featureentry_2.key.delete()
     self.featureentry_3.key.delete()
-    for appr in review_models.Approval.query():
-      appr.key.delete()
+    for gate in review_models.Gate.query():
+      gate.key.delete()
 
   @mock.patch('internals.approval_defs.fields_approvable_by')
   def test_process_pending_approval_me_query__none(
@@ -155,9 +155,10 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
     """Nothing is pending."""
     testing_config.sign_in('oner@example.com', 111)
     now = datetime.datetime.now()
-    mock_approvable_by.return_value = set()
+    mock_approvable_by.return_value = set([1, 2, 3])
 
-    feature_ids = search.process_pending_approval_me_query()
+    future = search.process_pending_approval_me_query()
+    feature_ids = search._resolve_promise_to_id_list(future)
 
     self.assertEqual(0, len(feature_ids))
 
@@ -168,12 +169,13 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
     testing_config.sign_in('visitor@example.com', 111)
     now = datetime.datetime.now()
     mock_approvable_by.return_value = set()
-    review_models.Approval(
-        feature_id=self.feature_1.key.integer_id(),
-        field_id=1, state=review_models.Approval.REVIEW_REQUESTED,
-        set_by='feature_owner@example.com', set_on=now).put()
+    review_models.Gate(
+        feature_id=self.feature_1.key.integer_id(), stage_id=1,
+        gate_type=1, state=review_models.Vote.REVIEW_REQUESTED,
+        requested_on=now).put()
 
-    feature_ids = search.process_pending_approval_me_query()
+    future = search.process_pending_approval_me_query()
+    feature_ids = search._resolve_promise_to_id_list(future)
 
     self.assertEqual(0, len(feature_ids))
 
@@ -185,19 +187,20 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
     time_1 = datetime.datetime.now() - datetime.timedelta(days=4)
     time_2 = datetime.datetime.now()
     mock_approvable_by.return_value = set([1, 2, 3])
-    review_models.Approval(
-        feature_id=self.feature_2.key.integer_id(),
-        field_id=1, state=review_models.Approval.REVIEW_REQUESTED,
-        set_by='feature_owner@example', set_on=time_1).put()
-    review_models.Approval(
-        feature_id=self.feature_1.key.integer_id(),
-        field_id=1, state=review_models.Approval.REVIEW_REQUESTED,
-        set_by='feature_owner@example.com', set_on=time_2).put()
+    review_models.Gate(
+        feature_id=self.feature_2.key.integer_id(), stage_id=1,
+        gate_type=1, state=review_models.Vote.REVIEW_REQUESTED,
+        requested_on=time_1).put()
+    review_models.Gate(
+        feature_id=self.feature_1.key.integer_id(), stage_id=1,
+        gate_type=1, state=review_models.Vote.REVIEW_REQUESTED,
+        requested_on=time_2).put()
 
-    feature_ids = search.process_pending_approval_me_query()
+    future = search.process_pending_approval_me_query()
+    feature_ids = search._resolve_promise_to_id_list(future)
     self.assertEqual(2, len(feature_ids))
-    # Results are sorted by set_on timestamp.
-    self.assertEqual(
+    # Results are not sorted.
+    self.assertCountEqual(
         [self.feature_2.key.integer_id(),
          self.feature_1.key.integer_id()],
         feature_ids)
@@ -205,23 +208,24 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
   @mock.patch('internals.approval_defs.fields_approvable_by')
   def test_process_pending_approval_me_query__mixed__approver(
       self, mock_approvable_by):
-    """Only REVIEW_REQUESTED is considered a pending approval."""
+    """We can find gates that are pending approval."""
     testing_config.sign_in('owner@example.com', 111)
     time_1 = datetime.datetime.now() - datetime.timedelta(days=4)
     time_2 = datetime.datetime.now()
     mock_approvable_by.return_value = set([1, 2, 3])
-    review_models.Approval(
-        feature_id=self.feature_2.key.integer_id(),
-        field_id=1, state=review_models.Approval.REVIEW_REQUESTED,
-        set_by='feature_owner@example', set_on=time_1).put()
-    review_models.Approval(
-        feature_id=self.feature_1.key.integer_id(),
-        field_id=1, state=review_models.Approval.NEEDS_WORK,
-        set_by='feature_owner@example.com', set_on=time_2).put()
+    review_models.Gate(
+        feature_id=self.feature_2.key.integer_id(), stage_id=1,
+        gate_type=1, state=review_models.Vote.REVIEW_REQUESTED,
+        requested_on=time_1).put()
+    review_models.Gate(
+        feature_id=self.feature_1.key.integer_id(), stage_id=1,
+        gate_type=1, state=review_models.Vote.APPROVED,
+        requested_on=time_2).put()
 
-    feature_ids = search.process_pending_approval_me_query()
+    future = search.process_pending_approval_me_query()
+    feature_ids = search._resolve_promise_to_id_list(future)
     self.assertEqual(1, len(feature_ids))
-    # Results are sorted by set_on timestamp, but there is only one.
+    # Results are not sorted.
     self.assertEqual(
         [self.feature_2.key.integer_id()],
         feature_ids)
@@ -245,40 +249,41 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
     self.assertEqual(len(actual), 1)
     self.assertEqual(actual[0], self.feature_1.key.integer_id())
 
-  @mock.patch('internals.review_models.Approval.get_approvals')
   @mock.patch('internals.approval_defs.fields_approvable_by')
   def test_process_recent_reviews_query__none(
-      self, mock_approvable_by, mock_get_approvals):
+      self, mock_approvable_by):
     """Nothing has been reviewed recently."""
     mock_approvable_by.return_value = set({1, 2, 3})
-    mock_get_approvals.return_value = []
 
-    actual = search.process_recent_reviews_query()
+    future = search.process_recent_reviews_query()
+    feature_ids = search._resolve_promise_to_id_list(future)
 
-    self.assertEqual(0, len(actual))
+    self.assertEqual(0, len(feature_ids))
 
-  @mock.patch('internals.review_models.Approval.get_approvals')
   @mock.patch('internals.approval_defs.fields_approvable_by')
   def test_process_recent_reviews_query__some(
-      self, mock_approvable_by, mock_get_approvals):
+      self, mock_approvable_by):
     """Some features have been reviewed recently."""
     mock_approvable_by.return_value = set({1, 2, 3})
     time_1 = datetime.datetime.now() - datetime.timedelta(days=4)
     time_2 = datetime.datetime.now()
-    mock_get_approvals.return_value = [
-        review_models.Approval(
-            feature_id=self.feature_1.key.integer_id(),
-            field_id=1, state=review_models.Approval.NA, set_on=time_2),
-        review_models.Approval(
-            feature_id=self.feature_2.key.integer_id(),
-            field_id=1, state=review_models.Approval.APPROVED, set_on=time_1),
-    ]
+    review_models.Gate(
+        feature_id=self.feature_1.key.integer_id(), stage_id=1,
+        gate_type=1, state=review_models.Vote.NA,
+        requested_on=time_2).put()
+    review_models.Gate(
+        feature_id=self.feature_2.key.integer_id(), stage_id=1,
+        gate_type=1, state=review_models.Vote.APPROVED,
+        requested_on=time_1).put()
 
-    actual = search.process_recent_reviews_query()
+    future = search.process_recent_reviews_query()
+    feature_ids = search._resolve_promise_to_id_list(future)
 
-    self.assertEqual(2, len(actual))
-    self.assertEqual(actual[0], self.feature_1.key.integer_id())  # Most recent
-    self.assertEqual(actual[1], self.feature_2.key.integer_id())
+    self.assertEqual(2, len(feature_ids))
+    self.assertEqual(
+        feature_ids[0], self.feature_1.key.integer_id())  # Most recent
+    self.assertEqual(
+        feature_ids[1], self.feature_2.key.integer_id())
 
   def test_sort_by_total_order__empty(self):
     """Sorting an empty list works."""

pages/guide.py

@@ -119,7 +119,7 @@ class FeatureCreateHandler(basehandlers.FlaskHandler):
         if gate_type == core_enums.GATE_ORIGIN_TRIAL:
           ot_stage_id = stage.key.integer_id()
         gate = Gate(feature_id=feature_id, stage_id=stage.key.integer_id(),
-            gate_type=gate_type, state=Vote.NA)
+                    gate_type=gate_type, state=Gate.PREPARING)
 
         # If we are creating a trial extension gate,
         # then the trial extension stage was just created.