Implement a "Recently reviewed features" box on the myfeatures page (#1600)

* Implement a "Recenty reviewed features" box on the myfeatures page. * removed some logging
2021-11-09 18:00:07 -08:00 · 2021-11-09 18:00:07 -08:00 · 03b814da8b
--- a/index.yaml
+++ b/index.yaml
@ -7,16 +7,6 @@ indexes:
 # index.yaml file manually, remove the "# AUTOGENERATED" marker line above.
 # If you want to manage some indexes manually, move them above the marker line.

- kind: "Feature"
-  properties:
-  - name: "deleted"
-  - name: "owner"
-  - name: "updated"
-    direction: desc
- kind: "Comment"
-  properties:
-  - name: "feature_id"
-  - name: "created"
 - kind: "Feature"
  properties:
  - name: "impl_status_chrome"
@ -32,6 +22,29 @@ indexes:
  - name: "shipped_milestone"
    direction: desc
  - name: "name"
+- kind: "Approval"
+  properties:
+  - name: "state"
+  - name: "set_on"
+- kind: "Approval"
+  properties:
+  - name: "state"
+  - name: "set_on"
+    direction: desc
+- kind: "Feature"
+  properties:
+  - name: "deleted"
+  - name: "owner"
+  - name: "updated"
+    direction: desc
+- kind: "Approval"
+  properties:
+  - name: "feature_id"
+  - name: "set_on"
+- kind: "Comment"
+  properties:
+  - name: "feature_id"
+  - name: "created"
 - kind: "Feature"
  properties:
  - name: "impl_status_chrome"
--- a/internals/models.py
+++ b/internals/models.py
@ -1351,9 +1351,12 @@ class Approval(DictModel):

  @classmethod
  def get_approvals(
-      cls, feature_id=None, field_id=None, states=None, set_by=None):
+      cls, feature_id=None, field_id=None, states=None, set_by=None,
+      order=None, limit=None):
    """Return the requested approvals."""
-    query = Approval.query()
+    if order is None:
+      order = Approval.set_on
+    query = Approval.query().order(order)
    if feature_id is not None:
      query = query.filter(Approval.feature_id == feature_id)
    if field_id is not None:
@ -1362,7 +1365,7 @@ class Approval(DictModel):
      query = query.filter(Approval.state.IN(states))
    if set_by is not None:
      query = query.filter(Approval.set_by == set_by)
-    approvals = query.fetch(None)
+    approvals = query.fetch(limit)
    return approvals

  @classmethod
--- a/internals/models_test.py
+++ b/internals/models_test.py
@ -340,10 +340,16 @@ class ApprovalTest(testing_config.CustomTestCase):
    self.feature_1_id = self.feature_1.key.integer_id()
    self.appr_1 = models.Approval(
        feature_id=self.feature_1_id, field_id=1,
-        state=models.Approval.REVIEW_STARTED,
-        set_on=datetime.datetime.now(),
+        state=models.Approval.REVIEW_REQUESTED,
+        set_on=datetime.datetime.now() - datetime.timedelta(1),
        set_by='one@example.com')
    self.appr_1.put()
+    self.appr_2 = models.Approval(
+        feature_id=self.feature_1_id, field_id=1,
+        state=models.Approval.APPROVED,
+        set_on=datetime.datetime.now(),
+        set_by='two@example.com')
+    self.appr_2.put()

  def tearDown(self):
    self.feature_1.key.delete()
@ -353,18 +359,39 @@ class ApprovalTest(testing_config.CustomTestCase):
  def test_get_approvals(self):
    """We can retrieve Approval entities."""
    actual = models.Approval.get_approvals(feature_id=self.feature_1_id)
-    self.assertEqual(1, len(actual))
-    self.assertEqual(self.feature_1_id, actual[0].feature_id)
+    self.assertEqual(2, len(actual))
+    self.assertEqual(models.Approval.REVIEW_REQUESTED, actual[0].state)
+    self.assertEqual(models.Approval.APPROVED, actual[1].state)

    actual = models.Approval.get_approvals(field_id=1)
-    self.assertEqual(1, len(actual))
+    self.assertEqual(models.Approval.REVIEW_REQUESTED, actual[0].state)
+    self.assertEqual(models.Approval.APPROVED, actual[1].state)

    actual = models.Approval.get_approvals(
-        states={models.Approval.REVIEW_STARTED, models.Approval.NEED_INFO})
+        states={models.Approval.REVIEW_REQUESTED,
+                models.Approval.REVIEW_STARTED})
    self.assertEqual(1, len(actual))

    actual = models.Approval.get_approvals(set_by='one@example.com')
    self.assertEqual(1, len(actual))
+    self.assertEqual(models.Approval.REVIEW_REQUESTED, actual[0].state)
+
+    actual = models.Approval.get_approvals(
+        order=-models.Approval.set_on, limit=10)
+    self.assertEqual(2, len(actual))
+    self.assertEqual(models.Approval.APPROVED, actual[0].state)
+    self.assertEqual(models.Approval.REVIEW_REQUESTED, actual[1].state)
+
+    actual = models.Approval.get_approvals(
+        order=-models.Approval.set_on, limit=1)
+    self.assertEqual(1, len(actual))
+    self.assertEqual(models.Approval.APPROVED, actual[0].state)
+
+    actual = models.Approval.get_approvals(
+        order=-models.Approval.set_on, states=[models.Approval.APPROVED],
+        limit=10)
+    self.assertEqual(1, len(actual))
+    self.assertEqual(models.Approval.APPROVED, actual[0].state)

  def test_is_valid_state(self):
    """We know what approval states are valid."""
@ -380,7 +407,7 @@ class ApprovalTest(testing_config.CustomTestCase):
        self.feature_1_id, 2, models.Approval.REVIEW_REQUESTED,
        'owner@example.com')
    self.assertEqual(
-        2,
+        3,
        len(models.Approval.query().fetch(None)))

  def test_clear_request(self):
--- a/internals/search.py
+++ b/internals/search.py
@ -24,6 +24,23 @@ from internals import notifier
 PENDING_STATES = [
    models.Approval.REVIEW_REQUESTED, models.Approval.REVIEW_STARTED,
    models.Approval.NEED_INFO]
+FINAL_STATES = [
+    models.Approval.NA, models.Approval.APPROVED,
+    models.Approval.NOT_APPROVED]
+
+def _get_referenced_features(approvals, reverse=False):
+  """Retrieve the features being approved, withuot duplicates."""
+  logging.info('approvals is %r', [(a.feature_id, a.state) for a in approvals])
+  feature_ids = []
+  seen = set()
+  for appr in approvals:
+    if appr.feature_id not in seen:
+      seen.add(appr.feature_id)
+      feature_ids.append(appr.feature_id)
+
+  features = models.Feature.get_by_ids(feature_ids)
+  return features
+

 def process_pending_approval_me_query():
  """Return a list of features needing approval by current user."""
@ -33,21 +50,10 @@ def process_pending_approval_me_query():

  approvable_fields_ids = approval_defs.fields_approvable_by(user)
  pending_approvals = models.Approval.get_approvals(states=PENDING_STATES)
-  logging.info('pending_approvals is %r', pending_approvals)
  pending_approvals = [pa for pa in pending_approvals
                       if pa.field_id in approvable_fields_ids]
-  pending_approvals.sort(key=lambda pa: pa.set_on)
-
-  # De-duplicate feature_ids while maintaining set_on order.
-  feature_ids = []
-  seen = set()
-  for pa in pending_approvals:
-    if pa.feature_id not in seen:
-      seen.add(pa.feature_id)
-      feature_ids.append(pa.feature_id)
-
-  features = models.Feature.get_by_ids(feature_ids)

+  features = _get_referenced_features(pending_approvals)
  return features


@ -71,6 +77,19 @@ def process_owner_me_query():
  return features


+def process_recent_reviews_query():
+  """Return features that were reviewed recently."""
+  user = users.get_current_user()
+  if not user:
+    return []
+
+  recent_approvals = models.Approval.get_approvals(
+      states=FINAL_STATES, order=-models.Approval.set_on, limit=10)
+
+  features = _get_referenced_features(recent_approvals, reverse=True)
+  return features
+
+
 def process_query(user_query):
  """Parse and run a user-supplied query, if we can handle it."""
  # TODO(jrobbins): Replace this with a more general approach.
@ -80,6 +99,8 @@ def process_query(user_query):
    return process_starred_me_query()
  if user_query == 'owner:me':
    return process_owner_me_query()
+  if user_query == 'is:recently-reviewed':
+    return process_recent_reviews_query()

  logging.warning('Unexpected query: %r', user_query)
  return []
--- a/internals/search_test.py
+++ b/internals/search_test.py
@ -26,7 +26,7 @@ class SearchFunctionsTest(testing_config.CustomTestCase):

  def setUp(self):
    self.feature_1 = models.Feature(
-        name='feature a', summary='sum', category=1, visibility=1,
+        name='feature 1', summary='sum', category=1, visibility=1,
        standardization=1, web_dev_views=1, impl_status_chrome=3)
    self.feature_1.owner = ['owner@example.com']
    self.feature_1.put()
@ -86,12 +86,12 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
    time_2 = datetime.datetime.now()
    mock_approvable_by.return_value = set([1, 2, 3])
    mock_get_approvals.return_value = [
-        models.Approval(
-            feature_id=self.feature_1.key.integer_id(),
-            field_id=1, state=0, set_on=time_2),
        models.Approval(
            feature_id=self.feature_2.key.integer_id(),
            field_id=1, state=0, set_on=time_1),
+        models.Approval(
+            feature_id=self.feature_1.key.integer_id(),
+            field_id=1, state=0, set_on=time_2),
    ]

    features = search.process_pending_approval_me_query()
@ -119,7 +119,7 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
    testing_config.sign_in('starrer@example.com', 111)
    actual = search.process_starred_me_query()
    self.assertEqual(len(actual), 1)
-    self.assertEqual(actual[0]['summary'], 'sum')
+    self.assertEqual(actual[0]['name'], 'feature 1')

  def test_process_owner_me_query__none(self):
    """We can return a list of features owned by the user."""
@ -132,15 +132,53 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
    testing_config.sign_in('owner@example.com', 111)
    actual = search.process_owner_me_query()
    self.assertEqual(len(actual), 2)
-    self.assertEqual(actual[0]['summary'], 'sum')
+    self.assertEqual(actual[0]['name'], 'feature 1')
+    self.assertEqual(actual[1]['name'], 'feature 2')
+
+  @mock.patch('internals.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):
+    """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()
+
+    self.assertEqual(0, len(actual))
+
+  @mock.patch('internals.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):
+    """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 = [
+        models.Approval(
+            feature_id=self.feature_1.key.integer_id(),
+            field_id=1, state=models.Approval.NA, set_on=time_2),
+        models.Approval(
+            feature_id=self.feature_2.key.integer_id(),
+            field_id=1, state=models.Approval.APPROVED, set_on=time_1),
+    ]
+
+    actual = search.process_recent_reviews_query()
+
+    self.assertEqual(2, len(actual))
+    self.assertEqual(actual[0]['name'], 'feature 1')  # Most recent
+    self.assertEqual(actual[1]['name'], 'feature 2')

  @mock.patch('logging.warning')
  @mock.patch('internals.search.process_pending_approval_me_query')
  @mock.patch('internals.search.process_starred_me_query')
  @mock.patch('internals.search.process_owner_me_query')
+  @mock.patch('internals.search.process_recent_reviews_query')
  def test_process_query(
-      self, mock_own_me, mock_star_me, mock_pend_me, mock_warn):
+      self, mock_recent, mock_own_me, mock_star_me, mock_pend_me, mock_warn):
    """We can match predefined queries."""
+    mock_recent.return_value = 'fake recent result'
    mock_own_me.return_value = 'fake owner result'
    mock_star_me.return_value = 'fake star result'
    mock_pend_me.return_value = 'fake pend result'
@ -157,6 +195,10 @@ class SearchFunctionsTest(testing_config.CustomTestCase):
        search.process_query('owner:me'),
        'fake owner result')

+    self.assertEqual(
+        search.process_query('is:recently-reviewed'),
+        'fake recent result')
+
    self.assertEqual(
        search.process_query('anything else'),
        [])
--- a/package.json
+++ b/package.json
@ -12,7 +12,7 @@
    "do-tests": "curl -X POST 'http://localhost:15606/reset' && python3 -m unittest discover -p '*_test.py'  -b",
    "start-emulator": "gcloud beta emulators datastore start --host-port=:15606 --no-store-on-disk --consistency=1.0",
    "stop-emulator": "curl -X POST 'http://localhost:15606/shutdown'",
-    "test": "(npm run start-emulator > /dev/null 2>&1 &); sleep 3; curl --retry 4 http://localhost:15606/ --retry-connrefused; npm run do-tests; status=$?; npm run stop-emulator; exit $status",
+    "test": "(npm run start-emulator > /dev/null 2>&1 &); sleep 6; curl --retry 4 http://localhost:15606/ --retry-connrefused; npm run do-tests; status=$?; npm run stop-emulator; exit $status",
    "test2": "python2 -m unittest discover -p '*_py2test.py'  -b",
    "do-coverage": "coverage3 erase && coverage3 run -m unittest discover -p '*_test.py' -b && coverage3 html",
    "coverage": "(npm run start-emulator > /dev/null 2>&1 &); sleep 3; curl --retry 4 http://localhost:15606/ --retry-connrefused; npm run do-coverage;  npm run stop-emulator",
--- a/static/elements/chromedash-myfeatures.js
+++ b/static/elements/chromedash-myfeatures.js
@ -58,11 +58,11 @@ class ChromedashMyFeatures extends LitElement {
    dialog.openWithFeature(featureId);
  }

-  renderBox(title, query, columns) {
+  renderBox(title, query, columns, opened=true) {
    return html`
      <chromedash-accordion
        title="${title}"
-        opened>
+        ?opened=${opened}>

        <chromedash-feature-table
          query="${query}"
@ -78,9 +78,12 @@ class ChromedashMyFeatures extends LitElement {
    `;
  }

-  renderPendingApprovals() {
-    return this.renderBox(
+  renderPendingAndRecentApprovals() {
+    const pendingBox = this.renderBox(
      'Features pending my approval', 'pending-approval-by:me', 'approvals');
+    const recentBox = this.renderBox(
+      'Recently reviewed features', 'is:recently-reviewed', 'approvals', false);
+    return [pendingBox, recentBox];
  }

  renderIStarred() {
@ -95,7 +98,7 @@ class ChromedashMyFeatures extends LitElement {

  render() {
    return html`
-      ${this.canApprove ? this.renderPendingApprovals() : nothing}
+      ${this.canApprove ? this.renderPendingAndRecentApprovals() : nothing}
      ${this.renderIOwn()}
      ${this.renderIStarred()}
      <chromedash-approvals-dialog