Implement myfeatures page boxes for starred-by:me and owner:me (#1533)

* wrote code

* Added unit tests and removed unwanted CSS.

* Use warning() because warn() is deprecated.

* Update internals/models.py

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

* Incorporated review comments.

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

api/features_api.py

@@ -23,6 +23,8 @@ from framework import permissions
 from framework import ramcache
 from framework import users
 from internals import models
+from internals import search
+
 
 class FeaturesAPI(basehandlers.APIHandler):
   """Features are the the main records that we track."""
@@ -37,11 +39,15 @@ class FeaturesAPI(basehandlers.APIHandler):
       try:
         milestone = int(self.request.args.get('milestone'))
         feature_list = models.Feature.get_in_milestone(
-          show_unlisted=show_unlisted_features, 
+          show_unlisted=show_unlisted_features,
           milestone=milestone)
       except ValueError:
         self.abort(400, msg='Invalid  Milestone')
 
+    user_query = self.request.args.get('q')
+    if user_query:
+      feature_list = search.process_query(user_query)
+
     # No Query-string parameter is provided
     if feature_list is None:
       feature_list = models.Feature.get_chronological(

index.yaml

@@ -22,6 +22,12 @@ indexes:
   - name: "shipped_android_milestone"
     direction: desc
   - name: "name"
+- kind: "Feature"
+  properties:
+  - name: "deleted"
+  - name: "owner"
+  - name: "updated"
+    direction: desc
 - kind: "Feature"
   properties:
   - name: "impl_status_chrome"

internals/models.py

@@ -776,7 +776,10 @@ class Feature(DictModel):
 
       # TODO(ericbidelman): Support more than one filter.
       if filterby:
-        query = query.filter(Feature.category == filterby[1])
+        if filterby[0] == 'category':
+          query = query.filter(Feature.category == filterby[1])
+        elif filterby[0] == 'owner':
+          query = query.filter(Feature.owner == filterby[1])
 
       features = query.fetch(limit)
 
@@ -825,6 +828,32 @@ class Feature(DictModel):
 
     return feature
 
+  @classmethod
+  def get_by_ids(self, feature_ids, update_cache=False):
+    result_dict = {}
+    futures = []
+
+    for feature_id in feature_ids:
+      KEY = '%s|%s' % (Feature.DEFAULT_CACHE_KEY, feature_id)
+      feature = ramcache.get(KEY)
+
+      if feature is None or update_cache:
+        futures.append(Feature.get_by_id_async(feature_id))
+
+    for future in futures:
+      unformatted_feature = future.get_result()
+      if unformatted_feature and not unformatted_feature.deleted:
+        feature = unformatted_feature.format_for_template()
+        feature['updated_display'] = unformatted_feature.updated.strftime("%Y-%m-%d")
+        feature['new_crbug_url'] = unformatted_feature.new_crbug_url()
+        ramcache.set(KEY, feature)
+        result_dict[unformatted_feature.key.integer_id()] = feature
+
+    result_list = [
+        result_dict.get(feature_id) for feature_id in feature_ids
+        if feature_id in result_dict]
+    return result_list
+
   @classmethod
   def get_chronological(
       self, limit=None, update_cache=False, version=None, show_unlisted=False):

internals/models_test.py

@@ -112,6 +112,58 @@ class FeatureTest(testing_config.CustomTestCase):
     self.feature_4.key.delete()
     ramcache.flush_all()
 
+  def test_get_all__normal(self):
+    """We can retrieve a list of all features with no filter."""
+    actual = models.Feature.get_all(update_cache=True)
+    names = [f['name'] for f in actual]
+    self.assertEqual(
+        ['feature c', 'feature d', 'feature a', 'feature b'],
+        names)
+
+    self.feature_1.summary = 'revised summary'
+    self.feature_1.put()  # Changes updated field.
+    actual = models.Feature.get_all(update_cache=True)
+    names = [f['name'] for f in actual]
+    self.assertEqual(
+        ['feature a', 'feature c', 'feature d', 'feature b'],
+        names)
+
+  def test_get_all__category(self):
+    """We can retrieve a list of all features of a given category."""
+    actual = models.Feature.get_all(
+        filterby=('category', models.CSS), update_cache=True)
+    names = [f['name'] for f in actual]
+    self.assertEqual(
+        [],
+        names)
+
+    self.feature_1.category = models.CSS
+    self.feature_1.put()  # Changes updated field.
+    actual = models.Feature.get_all(
+        filterby=('category', models.CSS), update_cache=True)
+    names = [f['name'] for f in actual]
+    self.assertEqual(
+        ['feature a'],
+        names)
+
+  def test_get_all__owner(self):
+    """We can retrieve a list of all features with a given owner."""
+    actual = models.Feature.get_all(
+        filterby=('owner', 'owner@example.com'), update_cache=True)
+    names = [f['name'] for f in actual]
+    self.assertEqual(
+        [],
+        names)
+
+    self.feature_1.owner = ['owner@example.com']
+    self.feature_1.put()  # Changes updated field.
+    actual = models.Feature.get_all(
+        filterby=('owner', 'owner@example.com'), update_cache=True)
+    names = [f['name'] for f in actual]
+    self.assertEqual(
+        ['feature a'],
+        names)
+
   def test_get_chronological__normal(self):
     """We can retrieve a list of features."""
     actual = models.Feature.get_chronological()
@@ -326,4 +378,4 @@ class UserPrefTest(testing_config.CustomTestCase):
               for i in range(100)]
     user_prefs = models.UserPref.get_prefs_for_emails(emails)
     self.assertEqual(100, len(user_prefs))
-    self.assertEqual('user_0@example.com', user_prefs[0].email)
+    self.assertEqual('user_0@example.com', user_prefs[0].email)

internals/notifier.py

@@ -146,8 +146,8 @@ def make_email_tasks(feature, is_update=False, changes=[]):
   for component_name in feature.blink_components:
     component = models.BlinkComponent.get_by_name(component_name)
     if not component:
-      logging.warn('Blink component "%s" not found.'
-                   'Not sending email to subscribers' % component_name)
+      logging.warning('Blink component "%s" not found.'
+                      'Not sending email to subscribers' % component_name)
       continue
 
     accumulate_reasons(
@@ -214,7 +214,7 @@ class FeatureStar(models.DictModel):
     logging.info('found %d stars for %r', len(feature_stars), email)
     feature_ids = [fs.feature_id for fs in feature_stars]
     logging.info('returning %r', feature_ids)
-    return feature_ids
+    return sorted(feature_ids, reverse=True)
 
   @classmethod
   def get_feature_starrers(self, feature_id):

internals/notifier_test.py

@@ -266,10 +266,15 @@ class FeatureStarTest(testing_config.CustomTestCase):
         name='feature two', summary='sum', category=1, visibility=1,
         standardization=1, web_dev_views=1, impl_status_chrome=1)
     self.feature_2.put()
+    self.feature_3 = models.Feature(
+        name='feature three', summary='sum', category=1, visibility=1,
+        standardization=1, web_dev_views=1, impl_status_chrome=1)
+    self.feature_3.put()
 
   def tearDown(self):
     self.feature_1.key.delete()
     self.feature_2.key.delete()
+    self.feature_3.key.delete()
 
   def test_get_star__no_existing(self):
     """User has never starred the given feature."""
@@ -309,12 +314,16 @@ class FeatureStarTest(testing_config.CustomTestCase):
     email = 'user5@example.com'
     feature_1_id = self.feature_1.key.integer_id()
     feature_2_id = self.feature_2.key.integer_id()
+    feature_3_id = self.feature_3.key.integer_id()
+    # Note intermixed order
     notifier.FeatureStar.set_star(email, feature_1_id)
+    notifier.FeatureStar.set_star(email, feature_3_id)
     notifier.FeatureStar.set_star(email, feature_2_id)
 
     actual = notifier.FeatureStar.get_user_stars(email)
-    self.assertItemsEqual(
-        [feature_1_id, feature_2_id],
+    self.assertEqual(
+        sorted([feature_3_id, feature_2_id, feature_1_id],
+               reverse=True),
         actual)
 
   def test_get_feature_starrers__no_stars(self):

internals/search.py

@@ -0,0 +1,63 @@
+# -*- coding: utf-8 -*-
+# Copyright 2021 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.
+
+from __future__ import division
+from __future__ import print_function
+
+import logging
+
+from framework import users
+from internals import models
+from internals import notifier
+
+
+def process_pending_approval_me_query():
+  """Return a list of features needing approval by current user."""
+  # TODO(jrobbins): write this
+  return []
+
+
+def process_starred_me_query():
+  """Return a list of features starred by the current user."""
+  user = users.get_current_user()
+  if not user:
+    return []
+
+  feature_ids = notifier.FeatureStar.get_user_stars(user.email())
+  features = models.Feature.get_by_ids(feature_ids)
+  return features
+
+
+def process_owner_me_query():
+  """Return features that the current user owns."""
+  user = users.get_current_user()
+  if not user:
+    return []
+  features = models.Feature.get_all(filterby=('owner', user.email()))
+  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.
+  if user_query == 'pending-approval-by:me':
+    return process_pending_approval_me_query()
+  if user_query == 'starred-by:me':
+    return process_starred_me_query()
+  if user_query == 'owner:me':
+    return process_owner_me_query()
+
+  logging.warning('Unexpected query: %r', user_query)
+  return []

internals/search_test.py

@@ -0,0 +1,107 @@
+# Copyright 2021 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.
+
+from __future__ import division
+from __future__ import print_function
+
+import testing_config  # Must be imported before the module under test.
+
+import mock
+
+from internals import models
+from internals import notifier
+from internals import search
+
+
+class SearchFunctionsTest(testing_config.CustomTestCase):
+
+  def setUp(self):
+    self.feature_1 = models.Feature(
+        name='feature a', 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()
+    notifier.FeatureStar.set_star(
+        'starrer@example.com', self.feature_1.key.integer_id())
+
+  def tearDown(self):
+    notifier.FeatureStar.set_star(
+        'starrer@example.com', self.feature_1.key.integer_id(),
+        starred=False)
+    self.feature_1.key.delete()
+
+  def test_process_pending_approval_me_query(self):
+    """We can return a list of features pending approval."""
+    # TODO(jrobbins): write this
+    pass
+
+  def test_process_starred_me_query__anon(self):
+    """Anon always has an empty list of starred features."""
+    testing_config.sign_out()
+    actual = search.process_starred_me_query()
+    self.assertEqual(actual, [])
+
+  def test_process_starred_me_query__none(self):
+    """We can return a list of features starred by the user."""
+    testing_config.sign_in('visitor@example.com', 111)
+    actual = search.process_starred_me_query()
+    self.assertEqual(actual, [])
+
+  def test_process_starred_me_query__some(self):
+    """We can return a list of features starred by the user."""
+    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')
+
+  def test_process_owner_me_query__none(self):
+    """We can return a list of features owned by the user."""
+    testing_config.sign_in('visitor@example.com', 111)
+    actual = search.process_owner_me_query()
+    self.assertEqual(actual, [])
+
+  def test_process_owner_me_query__some(self):
+    """We can return a list of features owned by the user."""
+    testing_config.sign_in('owner@example.com', 111)
+    actual = search.process_owner_me_query()
+    self.assertEqual(len(actual), 1)
+    self.assertEqual(actual[0]['summary'], 'sum')
+
+  @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')
+  def test_process_query(
+      self, mock_own_me, mock_star_me, mock_pend_me, mock_warn):
+    """We can match predefined queries."""
+    mock_own_me.return_value = 'fake owner result'
+    mock_star_me.return_value = 'fake star result'
+    mock_pend_me.return_value = 'fake pend result'
+
+    self.assertEqual(
+        search.process_query('pending-approval-by:me'),
+        'fake pend result')
+
+    self.assertEqual(
+        search.process_query('starred-by:me'),
+        'fake star result')
+
+    self.assertEqual(
+        search.process_query('owner:me'),
+        'fake owner result')
+
+    self.assertEqual(
+        search.process_query('anything else'),
+        [])
+    mock_warn.assert_called_once()

pages/featurelist.py

@@ -95,7 +95,7 @@ class FeatureListXMLHandler(basehandlers.FlaskHandler):
         for k,v in models.FEATURE_CATEGORIES.iteritems():
           normalized = utils.normalized_name(v)
           if category == normalized:
-            filterby = ('category =', k)
+            filterby = ('category', k)
             break
 
       feature_list = models.Feature.get_all( # cached

static/elements/chromedash-feature-table.js

@@ -1,11 +1,13 @@
 import {LitElement, css, html} from 'lit-element';
-// import {nothing} from 'lit-html';
+import {nothing} from 'lit-html';
 import SHARED_STYLES from '../css/shared.css';
 
 class ChromedashFeatureTable extends LitElement {
   static get properties() {
     return {
       query: {type: String},
+      features: {type: Array},
+      loading: {type: Boolean},
       rows: {type: Number},
       columns: {type: String},
       signedIn: {type: Boolean},
@@ -16,9 +18,18 @@ class ChromedashFeatureTable extends LitElement {
 
   constructor() {
     super();
+    this.loading = true;
     this.starredFeatures = new Set();
-    // TODO(jrobbins): use query to fetch features from server
-    this.features = ['One', 'Two', 'Three', 'Four'];
+    this.features = [];
+    this.noResultsMessage = 'No results';
+  }
+
+  connectedCallback() {
+    super.connectedCallback();
+    window.csClient.searchFeatures(this.query).then((features) => {
+      this.features = features;
+      this.loading = false;
+    });
   }
 
   static get styles() {
@@ -38,11 +49,25 @@ class ChromedashFeatureTable extends LitElement {
     `];
   }
 
+  renderMessages() {
+    if (this.loading) {
+      return html`
+        <tr><td>Loading...</td></tr>
+      `;
+    }
+    if (this.features.length == 0) {
+      return html`
+        <tr><td>${this.noResultsMessage}</td></tr>
+      `;
+    }
+    return nothing;
+  }
+
   renderFeature(feature) {
     // TODO(jrobbins): Add correct links and icons
     return html`
       <tr>
-        <td><a href="#">${feature}</a></td>
+        <td><a href="/feature/${feature.id}">${feature.name}</a></td>
       </tr>
     `;
   }
@@ -51,6 +76,7 @@ class ChromedashFeatureTable extends LitElement {
     return html`
       <table>
         ${this.features.map(this.renderFeature)}
+        ${this.renderMessages()}
       </table>
     `;
   }

static/js-src/cs-client.js

@@ -200,6 +200,10 @@ class ChromeStatusClient {
     return this.doGet(`/features?milestone=${milestone}`);
   }
 
+  searchFeatures(userQuery) {
+    return this.doGet(`/features?q=${userQuery}`);
+  }
+
   // Channels API
   getChannels() {
     return this.doGet('/channels');

templates/myfeatures.html

@@ -10,14 +10,6 @@
 {% endblock %}
 
 {% block content %}
-<style>
-  ul li {
-      background: white;
-      border: 1px solid gray;
-      margin: 16px;
-      padding: 16px;
-  }
-</style>
 
 <chromedash-accordion
   id="features_pending_my_approval"
@@ -36,8 +28,8 @@
   title="Features I starred">
 
   <chromedash-feature-table
-    query="pending-approval-by:me"
-    rows="10">
+    query="starred-by:me"
+    rows=10>
   </chromedash-feature-table>
 </chromedash-accordion>
 
@@ -47,8 +39,8 @@
   title="Features I own">
 
   <chromedash-feature-table
-    query="pending-approval-by:me"
-    rows="10">
+    query="owner:me"
+    rows=10>
   </chromedash-feature-table>
 </chromedash-accordion>