Feature flags: add subreddit filtering

Just like turning on a feature for only certain users can be useful, so can turning it on for only certain subreddits. In particular, this allows us to get a little bit of the functionality of a full A/B suite without doing terribly much work.
reddit-archive · Oct 1, 2014 · f41d0ad · f41d0ad
1 parent eb9f0ae
commit f41d0ad
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 28 deletions.
diff --git a/r2/r2/config/feature/README.md b/r2/r2/config/feature/README.md
@@ -57,10 +57,13 @@ feature_some_flag = {"url": "public_flag_name"}
 # On by group of users
 feature_some_flag = {"users": ["umbrae", "ajacksified"]}
 
+# On when viewing certain subreddits
+feature_some_flag = {"subreddits": ["wtf", "aww"]}
+
 # For both admin and a group of users
 feature_some_flag = {"admin": true, "users": ["user1", "user2"]}
 
-# Not yet available: rampups, variants for A/B, subreddit groups, etc
+# Not yet available: rampups, variants for A/B, etc.
 ```
 
 Since we're currently overloading live_config, each feature flag should be

diff --git a/r2/r2/config/feature/feature.py b/r2/r2/config/feature/feature.py
@@ -38,7 +38,9 @@ def is_enabled(name):
     :param name string - a given feature name
     :return bool
     """
-    return _get_featurestate(name).is_enabled(_world.current_user())
+    return _get_featurestate(name).is_enabled(
+               user=_world.current_user(),
+               subreddit=_world.current_subreddit())
 
 
 def is_enabled_for(name, user):

diff --git a/r2/r2/config/feature/state.py b/r2/r2/config/feature/state.py
@@ -75,7 +75,7 @@ def _parse_config(self, name):
 
         return config
 
-    def is_enabled(self, user=None):
+    def is_enabled(self, user=None, subreddit=None):
         cfg = self.config
         world = self.world
 
@@ -99,5 +99,9 @@ def is_enabled(self, user=None):
         if users and user and user.name in users:
             return True
 
+        subreddits = [s.lower() for s in cfg.get('subreddits', [])]
+        if subreddits and subreddit and subreddit.lower() in subreddits:
+            return True
+
         # Unknown value, default to off.
         return False
diff --git a/r2/r2/config/feature/world.py b/r2/r2/config/feature/world.py
@@ -32,6 +32,9 @@ class World(object):
     def current_user(self):
         return c.user
 
+    def current_subreddit(self):
+        return c.site.name
+
     def is_admin(self, user):
         if not user or not hasattr(user, 'name'):
             return False

diff --git a/r2/r2/tests/unit/config/feature_test.py b/r2/r2/tests/unit/config/feature_test.py
@@ -41,6 +41,7 @@ def world(cls):
         if not cls._world:
             cls._world = World()
             cls._world.current_user = mock.Mock(return_value='')
+            cls._world.current_subreddit = mock.Mock(return_value='')
 
         return cls._world
 
@@ -58,41 +59,41 @@ def test_enabled(self):
         cfg = {'enabled': 'on'}
         feature_state = self._make_state(cfg)
         self.assertTrue(feature_state.is_enabled())
-        self.assertTrue(feature_state.is_enabled(gary))
+        self.assertTrue(feature_state.is_enabled(user=gary))
 
     def test_disabled(self):
         cfg = {'enabled': 'off'}
         feature_state = self._make_state(cfg)
         self.assertFalse(feature_state.is_enabled())
-        self.assertFalse(feature_state.is_enabled(gary))
+        self.assertFalse(feature_state.is_enabled(user=gary))
 
     def test_admin_enabled(self):
         cfg = {'admin': True}
         mock_world = self.world()
         mock_world.is_admin = mock.Mock(return_value=True)
         feature_state = self._make_state(cfg, mock_world)
-        self.assertTrue(feature_state.is_enabled(gary))
+        self.assertTrue(feature_state.is_enabled(user=gary))
 
     def test_admin_disabled(self):
         cfg = {'admin': True}
         mock_world = self.world()
         mock_world.is_admin = mock.Mock(return_value=False)
         feature_state = self._make_state(cfg, mock_world)
-        self.assertFalse(feature_state.is_enabled(gary))
+        self.assertFalse(feature_state.is_enabled(user=gary))
 
     def test_employee_enabled(self):
         cfg = {'employee': True}
         mock_world = self.world()
         mock_world.is_employee = mock.Mock(return_value=True)
         feature_state = self._make_state(cfg, mock_world)
-        self.assertTrue(feature_state.is_enabled(gary))
+        self.assertTrue(feature_state.is_enabled(user=gary))
 
     def test_employee_disabled(self):
         cfg = {'employee': True}
         mock_world = self.world()
         mock_world.is_employee = mock.Mock(return_value=False)
         feature_state = self._make_state(cfg, mock_world)
-        self.assertFalse(feature_state.is_enabled(gary))
+        self.assertFalse(feature_state.is_enabled(user=gary))
 
     def test_url_enabled(self):
         mock_world = self.world()
@@ -101,13 +102,13 @@ def test_url_enabled(self):
         mock_world.url_features = mock.Mock(return_value={'test_state'})
         feature_state = self._make_state(cfg, mock_world)
         self.assertTrue(feature_state.is_enabled())
-        self.assertTrue(feature_state.is_enabled(gary))
+        self.assertTrue(feature_state.is_enabled(user=gary))
 
         cfg = {'url': 'test_state'}
         mock_world.url_features = mock.Mock(return_value={'x', 'test_state'})
         feature_state = self._make_state(cfg, mock_world)
         self.assertTrue(feature_state.is_enabled())
-        self.assertTrue(feature_state.is_enabled(gary))
+        self.assertTrue(feature_state.is_enabled(user=gary))
 
     def test_url_disabled(self):
         mock_world = self.world()
@@ -116,50 +117,68 @@ def test_url_disabled(self):
         mock_world.url_features = mock.Mock(return_value={})
         feature_state = self._make_state(cfg, mock_world)
         self.assertFalse(feature_state.is_enabled())
-        self.assertFalse(feature_state.is_enabled(gary))
+        self.assertFalse(feature_state.is_enabled(user=gary))
 
         cfg = {'url': 'test_state'}
         mock_world.url_features = mock.Mock(return_value={'x'})
         feature_state = self._make_state(cfg, mock_world)
         self.assertFalse(feature_state.is_enabled())
-        self.assertFalse(feature_state.is_enabled(gary))
+        self.assertFalse(feature_state.is_enabled(user=gary))
 
     def test_user_in(self):
         cfg = {'users': ['gary']}
-        mock_world = self.world()
-        feature_state = self._make_state(cfg, mock_world)
-        self.assertTrue(feature_state.is_enabled(gary))
+        feature_state = self._make_state(cfg)
+        self.assertTrue(feature_state.is_enabled(user=gary))
 
         cfg = {'users': ['dave', 'gary']}
-        mock_world = self.world()
-        feature_state = self._make_state(cfg, mock_world)
-        self.assertTrue(feature_state.is_enabled(gary))
+        feature_state = self._make_state(cfg)
+        self.assertTrue(feature_state.is_enabled(user=gary))
 
     def test_user_not_in(self):
         cfg = {'users': ['']}
-        mock_world = self.world()
-        featurestate = self._make_state(cfg, mock_world)
-        self.assertFalse(featurestate.is_enabled(gary))
+        featurestate = self._make_state(cfg)
+        self.assertFalse(featurestate.is_enabled(user=gary))
 
         cfg = {'users': ['dave', 'joe']}
-        mock_world = self.world()
-        featurestate = self._make_state(cfg, mock_world)
-        self.assertFalse(featurestate.is_enabled(gary))
+        featurestate = self._make_state(cfg)
+        self.assertFalse(featurestate.is_enabled(user=gary))
+
+    def test_subreddit_in(self):
+        cfg = {'subreddits': ['WTF']}
+        feature_state = self._make_state(cfg)
+        self.assertTrue(feature_state.is_enabled(subreddit='wtf'))
+
+        cfg = {'subreddits': ['wtf']}
+        feature_state = self._make_state(cfg)
+        self.assertTrue(feature_state.is_enabled(subreddit='WTF'))
+
+        cfg = {'subreddits': ['aww', 'wtf']}
+        feature_state = self._make_state(cfg)
+        self.assertTrue(feature_state.is_enabled(subreddit='wtf'))
+
+    def test_subreddit_not_in(self):
+        cfg = {'subreddits': []}
+        feature_state = self._make_state(cfg)
+        self.assertFalse(feature_state.is_enabled(subreddit='wtf'))
+
+        cfg = {'subreddits': ['aww', 'wtfoobar']}
+        feature_state = self._make_state(cfg)
+        self.assertFalse(feature_state.is_enabled(subreddit='wtf'))
 
     def test_multiple(self):
         # is_admin, globally off should still be False
         cfg = {'enabled': 'off', 'admin': True}
         mock_world = self.world()
         mock_world.is_admin = mock.Mock(return_value=True)
         featurestate = self._make_state(cfg, mock_world)
-        self.assertFalse(featurestate.is_enabled(gary))
+        self.assertFalse(featurestate.is_enabled(user=gary))
 
         # globally on but not admin should still be True
         cfg = {'enabled': 'on', 'admin': True}
         mock_world = self.world()
         mock_world.is_admin = mock.Mock(return_value=False)
         featurestate = self._make_state(cfg, mock_world)
-        self.assertTrue(featurestate.is_enabled(gary))
+        self.assertTrue(featurestate.is_enabled(user=gary))
         self.assertTrue(featurestate.is_enabled())
 
         # no URL but admin should still be True
@@ -168,4 +187,4 @@ def test_multiple(self):
         mock_world.url_features = mock.Mock(return_value={})
         mock_world.is_admin = mock.Mock(return_value=True)
         featurestate = self._make_state(cfg, mock_world)
-        self.assertTrue(featurestate.is_enabled(gary))
+        self.assertTrue(featurestate.is_enabled(user=gary))