Exclude deleted features from API responses. (#4619)

* Exclude deleted features from API responses.

* Addressed review comment

api/features_api.py

@@ -352,10 +352,7 @@ def do_delete(self, **kwargs) -> dict[str, str]:
     """Delete the specified feature."""
     # TODO(jrobbins): implement undelete UI.  For now, use cloud console.
     feature_id = kwargs.get('feature_id', None)
-    feature = self.get_specified_feature(feature_id=feature_id)
-    if feature is None:
-      return {'message': 'ID does not match any feature.'}
-
+    feature: FeatureEntry = self.get_specified_feature(feature_id=feature_id)
     user = users.get_current_user()
     app_user = AppUser.get_app_user(user.email())
     if ((app_user is None or not app_user.is_admin)
@@ -366,11 +363,4 @@ def do_delete(self, **kwargs) -> dict[str, str]:
     rediscache.delete_keys_with_prefix(FeatureEntry.DEFAULT_CACHE_KEY)
     rediscache.delete_keys_with_prefix(FeatureEntry.SEARCH_CACHE_KEY)
 
-    # Write for new FeatureEntry entity.
-    feature_entry: FeatureEntry | None = (
-        FeatureEntry.get_by_id(feature_id))
-    if feature_entry:
-      feature_entry.deleted = True
-      feature_entry.put()
-
     return {'message': 'Done'}

api/reviews_api.py

@@ -133,13 +133,11 @@ class GatesAPI(basehandlers.APIHandler):
   def do_get(self, **kwargs) -> dict[str, Any]:
     """Return a list of all gates associated with the given feature."""
     feature_id = kwargs.get('feature_id', None)
-    gates: list[Gate] = Gate.query(Gate.feature_id == feature_id).fetch()
+    feature: FeatureEntry = self.get_specified_feature(feature_id=feature_id)
+    gates: list[Gate] = []
 
-    # No gates associated with this feature.
-    if len(gates) == 0:
-      return {
-          'gates': [],
-          }
+    if not feature.deleted or self.get_bool_arg('include_deleted'):
+      gates = Gate.query(Gate.feature_id == feature_id).fetch()
 
     dicts = [converters.gate_value_to_json_dict(g) for g in gates]
     for g in dicts:

api/reviews_api_test.py

@@ -352,15 +352,44 @@ def test_do_get__success(self, mock_get_approvers):
   def test_do_get__empty_gates(self, mock_get_approvers):
     """Handler cannnot find any gates."""
     mock_get_approvers.return_value = ['[email protected]']
+    gateless_feature = core_models.FeatureEntry(
+        name='gateless feature', summary='sum', category=1,
+        owner_emails=['[email protected]'])
+    gateless_feature.put()
+    gateless_feature_id = gateless_feature.key.integer_id()
+
+    with test_app.test_request_context(self.request_path):
+      actual = self.handler.do_get(feature_id=gateless_feature_id)
+
+    expected = {
+        'gates': [],
+    }
+    self.assertEqual(actual, expected)
+
+  def test_do_get__deleted(self):
+    """If a feature is deleted, don't return any gates."""
+    self.feature_1.deleted = True
+    self.feature_1.put()
 
     with test_app.test_request_context(self.request_path):
-      actual = self.handler.do_get(feature_id=999)
+      actual = self.handler.do_get(feature_id=self.feature_id)
 
     expected = {
         'gates': [],
     }
     self.assertEqual(actual, expected)
 
+  def test_do_get__include_deleted(self):
+    """If a feature is deleted, return gates if include_deleted=1."""
+    self.feature_1.deleted = True
+    self.feature_1.put()
+
+    with test_app.test_request_context(
+        self.request_path + '?include_deleted=1'):
+      actual = self.handler.do_get(feature_id=self.feature_id)
+
+    self.assertEqual(1, len(actual['gates']))
+
 
 class XfnGatesAPITest(testing_config.CustomTestCase):
 

client-src/js-src/cs-client.js

@@ -530,7 +530,7 @@ export class ChromeStatusClient {
   }
 
   getGates(featureId) {
-    return this.doGet(`/features/${featureId}/gates`);
+    return this.doGet(`/features/${featureId}/gates?include_deleted=1`);
   }
 
   getPendingGates() {

internals/feature_helpers.py

@@ -476,6 +476,7 @@ def get_features_by_impl_status(limit: int | None=None, update_cache: bool=False
     feature_list = []
     for section in query_results:
       if len(section) > 0:
+        section = [f for f in section if not f.deleted]
         section = [f for f in section if f.feature_type != FEATURE_TYPE_ENTERPRISE_ID]
         section = [converters.feature_entry_to_json_basic(
             f, all_stages[f.key.integer_id()]) for f in section]

internals/feature_helpers_test.py

@@ -572,3 +572,25 @@ def test_get_in_milestone__non_enterprise_features(self):
     rediscache.delete(cache_key)
     self.assertEqual(cached_result, features)
 
+  def test_get_features_by_impl_status__normal(self):
+    """We can get JSON dicts for /features_v2.json."""
+    features = feature_helpers.get_features_by_impl_status()
+    self.assertEqual(4, len(features))
+    self.assertEqual({'feature a', 'feature b', 'feature c', 'feature d'},
+                     set(f['name'] for f in features))
+    self.assertEqual('feature a', features[2]['name'])
+    self.assertEqual('feature b', features[3]['name'])
+
+
+  def test_get_features_by_impl_status__deleted(self):
+    """Deleted features are not included in /features_v2.json."""
+    self.feature_3.deleted = True
+    self.feature_3.put()
+
+    features = feature_helpers.get_features_by_impl_status()
+
+    self.assertEqual(3, len(features))
+    self.assertEqual({'feature a', 'feature b', 'feature d'},
+                     set(f['name'] for f in features))
+    self.assertEqual('feature a', features[1]['name'])
+    self.assertEqual('feature b', features[2]['name'])