fix tests

api/converters_test.py

@@ -114,6 +114,10 @@ def test_feature_entry_to_json_basic__normal(self):
         'samples': ['https://example.com/samples'],
         'docs': ['https://example.com/docs'],
       },
+      'confidential': False,
+      'creator': '[email protected]',
+      'editors': ['[email protected]', '[email protected]'],
+      'owners': ['[email protected]'],
       'created': {
         'by': '[email protected]',
         'when': expected_date
@@ -201,6 +205,10 @@ def test_feature_entry_to_json_basic__feature_release(self):
         'samples': ['https://example.com/samples'],
         'docs': ['https://example.com/docs'],
       },
+      'confidential': False,
+      'creator': '[email protected]',
+      'editors': ['[email protected]', '[email protected]'],
+      'owners': ['[email protected]'],
       'created': {
         'by': '[email protected]',
         'when': expected_date
@@ -398,6 +406,7 @@ def test_feature_entry_to_json_verbose__normal(self):
       'editors': ['[email protected]', '[email protected]'],
       'creator': '[email protected]',
       'comments': 'notes',
+      'confidential': False,
       'browsers': {
         'chrome': {
           'bug': 'https://example.com/bug',

api/features_api_test.py

@@ -141,6 +141,19 @@ def setUp(self):
         stage_type=360, milestones=MilestoneSet(desktop_first=2))
     self.ship_stage_3.put()
 
+    self.feature_confidential = FeatureEntry(
+        name='feature confidential', summary='sum A', feature_type=2,
+        owner_emails=['[email protected]'], category=1,
+        editor_emails=['[email protected]'],
+        creator_email='confidential_creator@examplecom',
+        intent_stage=core_enums.INTENT_IMPLEMENT,
+        unlisted=True, confidential=True)
+    self.feature_confidential.put()
+    self.feature_confidential_id = self.feature_3.key.integer_id()
+    self.ship_stage_3 = Stage(feature_id=self.feature_3_id,
+        stage_type=360, milestones=MilestoneSet(desktop_first=1))
+    self.ship_stage_3.put()
+
     self.request_path = '/api/v0/features'
     self.handler = features_api.FeaturesAPI()
 
@@ -180,9 +193,9 @@ def test_get__all_listed_feature_names(self):
     # as it only returns feature names.
     self.assertEqual(2, len(actual['features']))
     self.assertEqual(2, actual['total_count'])
-    self.assertEqual(2, len(actual['features'][0]))
+    self.assertEqual(3, len(actual['features'][0]))
     self.assertEqual('feature two', actual['features'][0]['name'])
-    self.assertEqual(2, len(actual['features'][1]))
+    self.assertEqual(3, len(actual['features'][1]))
     self.assertEqual('feature one', actual['features'][1]['name'])
 
   def test_get__all_listed__pagination(self):
@@ -320,11 +333,12 @@ def test_get__all_unlisted_can_edit(self):
     testing_config.sign_in('[email protected]', 123567890)
     with test_app.test_request_context(self.request_path):
       actual = self.handler.do_get()
-    self.assertEqual(3, len(actual['features']))
-    self.assertEqual(3, actual['total_count'])
-    self.assertEqual('feature three', actual['features'][0]['name'])
-    self.assertEqual('feature two', actual['features'][1]['name'])
-    self.assertEqual('feature one', actual['features'][2]['name'])
+    self.assertEqual(4, len(actual['features']))
+    self.assertEqual(4, actual['total_count'])
+    self.assertEqual('feature confidential', actual['features'][0]['name'])
+    self.assertEqual('feature three', actual['features'][1]['name'])
+    self.assertEqual('feature two', actual['features'][2]['name'])
+    self.assertEqual('feature one', actual['features'][3]['name'])
 
   def test_get__user_query_no_sort__signed_out(self):
     """Get all features with a specified owner, unlisted not shown."""
@@ -361,8 +375,8 @@ def test_get__user_query_with_sort__with_perms(self):
     url = self.request_path + '?sort=-summary'
     with test_app.test_request_context(url):
       actual = self.handler.do_get()
-    self.assertEqual(3, len(actual['features']))
-    self.assertEqual(3, actual['total_count'])
+    self.assertEqual(4, len(actual['features']))
+    self.assertEqual(4, actual['total_count'])
     self.assertEqual('sum Z', actual['features'][0]['summary'])
     self.assertEqual('sum K', actual['features'][1]['summary'])
     self.assertEqual('sum A', actual['features'][2]['summary'])

framework/permissions.py

@@ -49,9 +49,9 @@ def can_view_feature_formatted(user: User, feature: dict) -> bool:
   if not feature:
     return False
 
-  return not feature['confidential'] or can_view_confidential_feature_formatted(user, feature)
+  return not feature['confidential'] or _can_view_confidential_feature_formatted(user, feature)
 
-def can_view_confidential_feature_formatted(user: User, feature: dict):
+def _can_view_confidential_feature_formatted(user: User, feature: dict):
   ''' Check if the user is an owner, editor, spec mentor, or creator
   for this feature or has a google.com or chromium.org account.
   If yes, they feature can be viewed, otherwise they cannot view
@@ -77,14 +77,14 @@ def can_view_feature(user: User, feature: FeatureEntry) -> bool:
   if not feature:
     return False
 
-  return not feature.confidential or can_view_confidential_feature(user, feature)
+  return not feature.confidential or _can_view_confidential_feature(user, feature)
 
-def can_view_confidential_feature(user: User, feature: FeatureEntry):
+def _can_view_confidential_feature(user: User, feature: FeatureEntry):
   ''' Check if the user is an owner, editor, spec mentor, or creator
   for this feature or has a google.com or chromium.org account.
   If yes, they feature can be viewed, otherwise they cannot view
   confidential features.'''
-  if not user:
+  if not user or not feature:
     return False
 
   if is_google_or_chromium_account(user) or can_admin_site(user):

framework/permissions_test.py

@@ -94,10 +94,22 @@ def setUp(self):
     self.feature_1.put()
     self.feature_id = self.feature_1.key.integer_id()
 
+    # Feature for checking permissions against
+    self.feature_confidential = core_models.FeatureEntry(
+        name='feature one', summary='sum',
+        creator_email="[email protected]",
+        owner_emails=['[email protected]'],
+        editor_emails=['[email protected]'],
+        spec_mentor_emails=['[email protected]'], category=1,
+        confidential=True)
+    self.feature_confidential.put()
+    self.feature_confidential_id = self.feature_confidential.key.integer_id()
+
   def tearDown(self):
     for user in self.users:
       user.delete()
     self.feature_1.key.delete()
+    self.feature_confidential.key.delete()
 
   def check_function_results(
       self, func, additional_args,
@@ -194,16 +206,23 @@ def test_can_admin_site(self):
   def test_can_view_feature(self):
     self.check_function_results(
         permissions.can_view_feature, (None,),
-        unregistered=True, registered=True,
-        special=True, site_editor=True, admin=True, anon=True)
+        unregistered=False, registered=False,
+        special=False, site_editor=False, admin=False, anon=False)
 
     self.check_function_results_with_feature(
-      permissions.can_view_feature, (self.feature_id,),
+      permissions.can_view_feature, (self.feature_1,),
       unregistered=True, registered=True,
       feature_owner=True, feature_editor=True,
       creator=True, site_editor=True, admin=True, spec_mentor=True,
     )
 
+    self.check_function_results_with_feature(
+      permissions.can_view_feature, (self.feature_confidential,),
+      unregistered=False, registered=False,
+      feature_owner=True, feature_editor=True,
+      creator=True, site_editor=False, admin=True, spec_mentor=False,
+    )
+
   def test_can_create_feature(self):
     self.check_function_results(
         permissions.can_create_feature, tuple(),
@@ -233,13 +252,13 @@ def test_can_edit_feature(self):
   def test_can_review_gate(self):
     approvers = []
     self.check_function_results(
-        permissions.can_review_gate, (None, None, approvers),
+        permissions.can_review_gate, (self.feature_1, None, approvers),
         unregistered=False, registered=False,
         special=False, site_editor=False, admin=True, anon=False)
 
     approvers = ['[email protected]']
     self.check_function_results(
-        permissions.can_review_gate, (None, None, approvers),
+        permissions.can_review_gate, (self.feature_1, None, approvers),
         unregistered=False, registered=True,
         special=False, site_editor=False, admin=True, anon=False)
 

internals/feature_helpers.py

@@ -376,6 +376,7 @@ def get_feature_names_by_ids(feature_ids: list[int],
       feature_name_dict = {
         'id': feature_id,
         'name': fe.name,
+        'confidential': fe.confidential
       }
       result_dict[feature_id] = feature_name_dict
 

internals/feature_helpers_test.py

@@ -192,7 +192,8 @@ def test_get_by_ids__cache_hit(self):
     cached_feature = {
       'name': 'fake cached_feature',
       'id': self.feature_1.key.integer_id(),
-      'unlisted': False
+      'unlisted': False,
+      'confidential': False
     }
     rediscache.set(cache_key, cached_feature)
 
@@ -255,9 +256,9 @@ def test_get_feature_names_by_ids__cache_miss(self):
         self.feature_2.key.integer_id()])
 
     self.assertEqual(2, len(actual))
-    self.assertEqual(2, len(actual[0]))
+    self.assertEqual(3, len(actual[0]))
     self.assertEqual('feature a', actual[0]['name'])
-    self.assertEqual(2, len(actual[1]))
+    self.assertEqual(3, len(actual[1]))
     self.assertEqual('feature b', actual[1]['name'])
 
     lookup_key_1 = '%s|%s' % (FeatureEntry.FEATURE_NAME_CACHE_KEY,
@@ -273,14 +274,15 @@ def test_get_feature_names_by_ids__cache_hit(self):
         FeatureEntry.FEATURE_NAME_CACHE_KEY, self.feature_1.key.integer_id())
     cached_feature = {
       'name': 'fake cached_feature',
-      'id': self.feature_1.key.integer_id()
+      'id': self.feature_1.key.integer_id(),
+      'confidential': False
     }
     rediscache.set(cache_key, cached_feature)
 
     actual = feature_helpers.get_feature_names_by_ids([self.feature_1.key.integer_id()])
 
     self.assertEqual(1, len(actual))
-    self.assertEqual(2, len(actual[0]))
+    self.assertEqual(3, len(actual[0]))
     self.assertEqual(cached_feature, actual[0])
 
   def test_get_feature_names_by_ids__batch_order(self):
@@ -293,7 +295,7 @@ def test_get_feature_names_by_ids__batch_order(self):
     ])
 
     self.assertEqual(4, len(actual))
-    self.assertEqual(2, len(actual[0]))
+    self.assertEqual(3, len(actual[0]))
     self.assertEqual('feature d', actual[0]['name'])
     self.assertEqual('feature a', actual[1]['name'])
     self.assertEqual('feature c', actual[2]['name'])
@@ -321,7 +323,7 @@ def test_get_feature_names_by_ids__cached_correctly(self):
     ])
 
     self.assertEqual(4, len(actual))
-    self.assertEqual(2, len(actual[0]))
+    self.assertEqual(3, len(actual[0]))
     self.assertEqual('feature d', actual[0]['name'])
     self.assertEqual('feature a', actual[1]['name'])
     self.assertEqual('feature c', actual[2]['name'])
@@ -459,7 +461,7 @@ def test_get_in_milestone__cached(self):
     """If there is something in the cache, we use it."""
     cache_key = '%s|%s|%s' % (
         FeatureEntry.DEFAULT_CACHE_KEY, 'milestone', 1)
-    cached_test_feature = {'test': [{'name': 'test_feature', 'unlisted': False}]}
+    cached_test_feature = {'test': [{'name': 'test_feature', 'unlisted': False, 'confidential': False}]}
     rediscache.set(cache_key, cached_test_feature)
 
     actual = feature_helpers.get_in_milestone(milestone=1)