Disallow duplicates in blacklist and whitelist

feature_ramp/Feature.py

@@ -36,8 +36,8 @@ def __init__(self, feature_name, feature_group_name=None, default_percentage=0):
         redis_raw = redis.get(key)
         redis_data = self._deserialize(redis_raw)
 
-        self.whitelist = redis_data.get('whitelist', [])
-        self.blacklist = redis_data.get('blacklist', [])
+        self.whitelist = set(redis_data.get('whitelist', []))
+        self.blacklist = set(redis_data.get('blacklist', []))
         self.percentage = redis_data.get('percentage', default_percentage)
 
     def is_visible(self, identifier):
@@ -119,8 +119,8 @@ def reset_settings(self):
         """
 
         self.percentage = 0
-        self.whitelist = []
-        self.blacklist = []
+        self.whitelist = set([])
+        self.blacklist = set([])
         self._save()
 
     def delete(self):
@@ -148,7 +148,7 @@ def set_percentage(self, percentage):
     def add_to_whitelist(self, identifier):
         """ Whitelist the given identifier to always see the feature regardless of ramp. """
 
-        self.whitelist.append(identifier)
+        self.whitelist.add(identifier)
         self._save()
 
     def remove_from_whitelist(self, identifier):
@@ -160,7 +160,7 @@ def remove_from_whitelist(self, identifier):
     def add_to_blacklist(self, identifier):
         """ Blacklist the given identifier to never see the feature regardless of ramp. """
 
-        self.blacklist.append(identifier)
+        self.blacklist.add(identifier)
         self._save()
 
     def remove_from_blacklist(self, identifier):
@@ -235,8 +235,8 @@ def _get_redis_data(self):
         """ Returns the dictionary representation of this object for storage in Redis. """
 
         return {
-            'whitelist': self.whitelist,
-            'blacklist': self.blacklist,
+            'whitelist': list(self.whitelist),
+            'blacklist': list(self.blacklist),
             'percentage': self.percentage
         }
 
@@ -253,4 +253,8 @@ def _deserialize(self, redis_obj):
     def __str__(self):
         """ Pretty print the feature and some stats """
         stats = self._get_redis_data()
-        return "Feature: {0}\nwhitelisted: {1}\nblacklisted: {2}\npercentage: {3}\n".format(self.feature_name, stats['whitelist'], stats['blacklist'], stats['percentage'])
+        return "Feature: {0}\nwhitelisted: {1}\nblacklisted: {2}\npercentage: {3}\n".format(
+            self.feature_name,
+            stats['whitelist'],
+            stats['blacklist'],
+            stats['percentage'])

tests/test_feature.py

@@ -20,7 +20,7 @@ def tearDown(self):
     def test_initialize_feature_with_default_percentage(self):
         feature_test = Feature("testing", default_percentage=100)
 
-        self.assertEquals(feature_test.percentage, 100)
+        self.assertEqual(feature_test.percentage, 100)
 
     def test_reset_settings(self):
         """ Tests calling reset_settings resets the correct
@@ -82,7 +82,7 @@ def test_all_features_with_data(self):
         all_features = Feature.all_features(include_data=True)
         self.assertEqual(len(all_features), 4)
 
-        for key in ['looktest1','looktest2','looktest3','looktest4']:
+        for key in ['looktest1', 'looktest2', 'looktest3', 'looktest4']:
             self.assertTrue(key in all_features)
             if not key == 'looktest1':
                 self.assertEqual(all_features[key]['percentage'], 100)
@@ -92,17 +92,17 @@ def test_all_features_with_data(self):
         self.assertFalse('blacklist' in all_features['looktest1'])
 
         self.assertTrue('whitelist' in all_features['looktest2'])
-        self.assertEqual(all_features['looktest2']['whitelist'], [3])
+        self.assertEqual(all_features['looktest2']['whitelist'], set([3]))
         self.assertFalse('blacklist' in all_features['looktest2'])
 
         self.assertFalse('whitelist' in all_features['looktest3'])
         self.assertTrue('blacklist' in all_features['looktest3'])
-        self.assertEqual(all_features['looktest3']['blacklist'], [4,5])
+        self.assertEqual(all_features['looktest3']['blacklist'], set([4, 5]))
 
         self.assertTrue('whitelist' in all_features['looktest4'])
-        self.assertEqual(all_features['looktest4']['whitelist'], [3, 5])
+        self.assertEqual(all_features['looktest4']['whitelist'], set([3, 5]))
         self.assertTrue('blacklist' in all_features['looktest4'])
-        self.assertEqual(all_features['looktest4']['blacklist'], [4])
+        self.assertEqual(all_features['looktest4']['blacklist'], set([4]))
 
     def test_set_add(self):
         """ Tests that creating a feature stores its key in a Redis set. """
@@ -152,6 +152,16 @@ def test_add_to_whitelist_with_string(self):
         self.feature_test.add_to_whitelist(email)
         self.assertTrue(email in Feature("testing").whitelist)
 
+    def test_add_to_whitelist_with_duplicate(self):
+        """ Tests calling add_to_whitelist doesn't set duplicate
+        data. """
+
+        self.feature_test.add_to_whitelist(3)
+        self.feature_test.add_to_whitelist(3)
+        self.assertEqual(
+            len([id for id in Feature("testing").whitelist if id == 3]),
+            1)
+
     def test_remove_from_whitelist(self):
         """ Tests calling remove_from_whitelist sets the correct
         data in Redis. """
@@ -182,6 +192,16 @@ def test_add_to_blacklist_with_string(self):
         self.feature_test.add_to_blacklist(email)
         self.assertTrue(email in Feature("testing").blacklist)
 
+    def test_add_to_blacklist_with_duplicate(self):
+        """ Tests calling add_to_blacklist doesn't set duplicate
+        data. """
+
+        self.feature_test.add_to_blacklist(3)
+        self.feature_test.add_to_blacklist(3)
+        self.assertEqual(
+            len([id for id in Feature("testing").blacklist if id == 3]),
+            1)
+
     def test_remove_from_blacklist(self):
         """ Tests calling remove_from_blacklist sets the correct
         data in Redis. """
@@ -296,7 +316,6 @@ def test_is_ramped_using_int(self):
         self.feature_test.set_percentage(100)
         self.assertTrue(self.feature_test._is_ramped(5))
 
-
     def test_is_ramped_using_string(self):
         """Tests that _is_ramped accepts strings as identifier."""
         self.feature_test.set_percentage(100)
@@ -324,9 +343,9 @@ def test_group_names(self):
         identifiers = range(1, 10001)
 
         visibility_test_one = [id for id in identifiers
-                                if feature_one.is_visible(id)]
+                               if feature_one.is_visible(id)]
 
         visibility_test_two = [id for id in identifiers
-                                if feature_two.is_visible(id)]
+                               if feature_two.is_visible(id)]
 
         self.assertEqual(visibility_test_one, visibility_test_two)