Query-String Parameter for milestone in features api (GoogleChrome#1424)

* Added query-string support for milestones in feature API

* Added tests

* Factored out the call to permissions.can_edit_feature(user, None) to the top of the method

* Resolved error in comment

* Added method for annotating first of each implementation status

* Added tests

* Made version 2 as the default version for api template

* Renamed versions to statuses

* Removed reverse argument

Author: shivamag00

api/features_api.py

@@ -29,9 +29,25 @@ class FeaturesAPI(basehandlers.APIHandler):
 
   def do_get(self):
     user = users.get_current_user()
-    feature_list = models.Feature.get_chronological(
-        version=2,
-        show_unlisted=permissions.can_edit_feature(user, None))
+    show_unlisted_features = permissions.can_edit_feature(user, None)
+    feature_list = None
+
+    # Query-string parameter 'milestone' is provided
+    if self.request.args.get('milestone') is not None:
+      try:
+        milestone = int(self.request.args.get('milestone'))
+        feature_list = models.Feature.get_in_milestone(
+          show_unlisted=show_unlisted_features, 
+          milestone=milestone)
+      except ValueError:
+        self.abort(400, msg='Invalid  Milestone')
+
+    # No Query-string parameter is provided
+    if feature_list is None:
+      feature_list = models.Feature.get_chronological(
+          version=2,
+          show_unlisted=show_unlisted_features)
+
     return feature_list
 
   # TODO(jrobbins): do_post

api/features_api_test.py

@@ -27,7 +27,7 @@
 from framework import ramcache
 
 
-class FeaturesAPITest(testing_config.CustomTestCase):
+class FeaturesAPITestDelete(testing_config.CustomTestCase):
 
   def setUp(self):
     self.feature_1 = models.Feature(
@@ -95,6 +95,31 @@ def test_delete__not_found(self):
     revised_feature = models.Feature.get_by_id(self.feature_id)
     self.assertFalse(revised_feature.deleted)
 
+
+class FeaturesAPITestGet(testing_config.CustomTestCase):
+
+  def setUp(self):
+    self.feature_1 = models.Feature(
+        name='feature one', summary='sum', category=1, visibility=1,
+        standardization=1, web_dev_views=1, impl_status_chrome=5,
+        intent_stage=models.INTENT_IMPLEMENT, shipped_milestone=1)
+    self.feature_1.put()
+    self.feature_id = self.feature_1.key.integer_id()
+
+    self.request_path = '/api/v0/features'
+    self.handler = features_api.FeaturesAPI()
+
+    self.app_admin = models.AppUser(email='admin@example.com')
+    self.app_admin.is_admin = True
+    self.app_admin.put()
+
+  def tearDown(self):
+    self.feature_1.key.delete()
+    self.app_admin.key.delete()
+    testing_config.sign_out()
+    ramcache.flush_all()
+    ramcache.check_for_distributed_invalidation()
+
   def test_get__all_listed(self):
     """Get all features that are listed."""
     with register.app.test_request_context(self.request_path):
@@ -105,16 +130,14 @@ def test_get__all_listed(self):
     self.assertEqual(1, len(actual_response))
     self.assertEqual('feature one', actual_response[0]['name'])
 
-  def test_get__unlisted_no_perms(self):
+  def test_get__all_unlisted_no_perms(self):
     """JSON feed does not include unlisted features for users who can't edit."""
     self.feature_1.unlisted = True
     self.feature_1.put()
 
     # No signed-in user
     with register.app.test_request_context(self.request_path):
-      actual_response = self.handler.do_get()
-    # Comparing only the total number of features and name of the feature 
-    # as certain fields like `updated` cannot be compared 
+      actual_response = self.handler.do_get() 
     self.assertEqual(0, len(actual_response))
 
     # Signed-in user with no permissions
@@ -123,15 +146,73 @@ def test_get__unlisted_no_perms(self):
       actual_response = self.handler.do_get()
     self.assertEqual(0, len(actual_response))
 
-  def test_get__unlisted_can_edit(self):
+  def test_get__all_unlisted_can_edit(self):
     """JSON feed includes unlisted features for users who may edit."""
     self.feature_1.unlisted = True
     self.feature_1.put()
 
+    # Signed-in user with permissions
+    testing_config.sign_in('admin@example.com', 123567890)
+    with register.app.test_request_context(self.request_path):
+      actual_response = self.handler.do_get()
+    self.assertEqual(1, len(actual_response))
+    self.assertEqual('feature one', actual_response[0]['name'])
+
+  def test_get__in_milestone_listed(self):
+    """Get all features in a specific milestone that are listed."""
+    # Atleast one feature is present in milestone
+    with register.app.test_request_context(self.request_path+'?milestone=1'):
+      actual_response = self.handler.do_get()
+    self.assertEqual(1, len(actual_response))
+    self.assertEqual('feature one', actual_response[0]['name'])
+    self.assertEqual(1, actual_response[0]['browsers']['chrome']['status']['milestone_str'])
+
+    # No Feature is present in milestone
+    with register.app.test_request_context(self.request_path+'?milestone=2'):
+      actual_response = self.handler.do_get()
+    self.assertEqual(0, len(actual_response))
+
+  def test_get__in_milestone_unlisted_no_perms(self):
+    """JSON feed does not include unlisted features for users who can't edit."""
+    self.feature_1.unlisted = True
+    self.feature_1.put()
+
     # No signed-in user
+    with register.app.test_request_context(self.request_path+'?milestone=1'):
+      actual_response = self.handler.do_get()
+    self.assertEqual(0, len(actual_response))
+
     # Signed-in user with no permissions
+    testing_config.sign_in('one@example.com', 123567890)
+    with register.app.test_request_context(self.request_path+'?milestone=1'):
+      actual_response = self.handler.do_get()
+    self.assertEqual(0, len(actual_response))
+
+  def test_get__in_milestone_unlisted_can_edit(self):
+    """JSON feed includes unlisted features for users who may edit."""
+    self.feature_1.unlisted = True
+    self.feature_1.put()
+
+    # Signed-in user with permissions
     testing_config.sign_in('admin@example.com', 123567890)
-    with register.app.test_request_context(self.request_path):
+
+    # Feature is present in milestone
+    with register.app.test_request_context(self.request_path+'?milestone=1'):
       actual_response = self.handler.do_get()
     self.assertEqual(1, len(actual_response))
-    self.assertEqual('feature one', actual_response[0]['name'])
+    self.assertEqual('feature one', actual_response[0]['name'])
+    self.assertEqual(1, actual_response[0]['browsers']['chrome']['status']['milestone_str'])
+
+    # Feature is not present in milestone
+    with register.app.test_request_context(self.request_path+'?milestone=2'):
+      actual_response = self.handler.do_get()
+    self.assertEqual(0, len(actual_response))
+
+  @mock.patch('flask.abort')
+  def test_get__in_milestone_invalid_query(self, mock_abort):
+    """Invalid value of milestone should not be processed."""
+
+    # Feature is present in milestone
+    with register.app.test_request_context(self.request_path+'?milestone=chromium'):
+      actual_response = self.handler.do_get()
+    mock_abort.assert_called_once_with(400, description='Invalid  Milestone')

internals/models.py

@@ -554,6 +554,30 @@ def _annotate_first_of_milestones(self, feature_list, version=None):
     except Exception as e:
       logging.error(e)
 
+  @classmethod
+  def _annotate_first_of_impl_status_in_milestones(self, feature_list, version=2):
+    try:
+      statuses = [
+        IMPLEMENTATION_STATUS[BEHIND_A_FLAG],
+        IMPLEMENTATION_STATUS[ENABLED_BY_DEFAULT],
+        IMPLEMENTATION_STATUS[REMOVED],
+        IMPLEMENTATION_STATUS[ORIGIN_TRIAL],
+        IMPLEMENTATION_STATUS[INTERVENTION],
+        IMPLEMENTATION_STATUS[ON_HOLD]
+      ]
+      first_of_milestone_func = Feature._first_of_milestone
+      if version == 2:
+        first_of_milestone_func = Feature._first_of_milestone_v2
+
+      last_good_idx = 0
+      for i, status in enumerate(statuses):
+        idx = first_of_milestone_func(feature_list, status, start=last_good_idx)
+        if idx != -1:
+          feature_list[idx]['first_of_milestone'] = True
+          last_good_idx = idx
+    except Exception as e:
+      logging.error(e)
+
   def migrate_views(self):
     """Migrate obsolete values for views on first edit."""
     if self.ff_views == MIXED_SIGNALS:
@@ -571,7 +595,7 @@ def migrate_views(self):
     if self.safari_views == PUBLIC_SKEPTICISM:
       self.safari_views = OPPOSED
 
-  def format_for_template(self, version=None):
+  def format_for_template(self, version=2):
     self.migrate_views()
     d = self.to_dict()
     is_released = self.impl_status_chrome in RELEASE_IMPL_STATES
@@ -894,6 +918,48 @@ def getSortingMilestone(feature):
 
     return allowed_feature_list
 
+  @classmethod
+  def get_in_milestone(
+      self, show_unlisted=False, milestone=None):
+    if milestone == None:
+      return None
+
+    logging.info('Getting chronological feature list in milestone %d', milestone)
+    q = Feature.query()
+    q = q.order(Feature.name)
+    q = q.filter(Feature.shipped_milestone == milestone)
+    desktop_shipping_features = q.fetch(None)
+
+    # Features with an android shipping milestone but no desktop milestone.
+    q = Feature.query()
+    q = q.order(Feature.name)
+    q = q.filter(Feature.shipped_android_milestone == milestone)
+    q = q.filter(Feature.shipped_milestone == None)
+    android_only_shipping_features = q.fetch(None)
+
+    # Constructor the proper ordering.
+    all_features = []
+    all_features.extend(desktop_shipping_features)
+    all_features.extend(android_only_shipping_features)
+
+    # Feature list must be first sorted by implementation status and then by name. 
+    # The implementation may seem to be counter-intuitive using sort() method.
+    all_features.sort(key=lambda f: f.name)
+    all_features.sort(key=lambda f: f.impl_status_chrome)
+
+    all_features = [f for f in all_features if not f.deleted]
+
+    feature_list = [f.format_for_template() for f in all_features]
+
+    self._annotate_first_of_impl_status_in_milestones(feature_list)
+
+    allowed_feature_list = [
+        f for f in feature_list
+        if show_unlisted or not f['unlisted']]
+
+    return allowed_feature_list
+
+  
   @classmethod
   def get_shipping_samples(self, limit=None, update_cache=False):
     cache_key = '%s|%s|%s' % (Feature.DEFAULT_CACHE_KEY, 'samples', limit)