Introduce Bot Filtering #121

oakbani · 2018-05-31T11:32:41Z

No description provided.

coveralls · 2018-05-31T11:35:38Z

Coverage increased (+0.005%) to 99.653% when pulling fe607b4 on oakbani/bot-filtering into 6ed11d7 on master.

msohailhussain · 2018-05-31T18:06:20Z

optimizely/event_builder.py

@@ -172,19 +182,39 @@ def _get_attributes(self, attributes):
    if not attributes:
      return []

-    for attribute_key in attributes.keys():
+    for attribute_key in sorted(attributes.keys()):


why do we need to sort?

attributes is a dict. Order of keys is not guaranteed to be same for all python versions. Hence for eventpayload unit tests to pass for all versions, we need to ensure that they get appended in the same order.

Why does ordering matter? Let us not do this. It seems unnecessary.

@aliabbasrizvi attributes is a dict. Order of keys is not guaranteed to be same for all python versions. Hence for eventpayload unit tests to pass for all versions, we need to ensure that they get appended in the same order.
https://travis-ci.org/optimizely/python-sdk/builds/389076889

msohailhussain · 2018-05-31T18:09:20Z

optimizely/event_builder.py

-        attribute = self.config.get_attribute(attribute_key)
-        if attribute:
+        # Check for reserved attributes
+        reserved_attrs = [ReservedAttributes.BUCKETING_ID, ReservedAttributes.USER_AGENT]


this must be outside, as it will be initialized again and again.

msohailhussain · 2018-05-31T18:13:18Z

optimizely/event_builder.py

+
+    # Append Bot Filtering Attribute
+    attribute_key = ReservedAttributes.BOT_FILTERING
+    params.append({


Please add condition which was placed in javascript code. If no botFiltering is given or old datafile is used, in that case it shouldn't send the value of botFiltering.
https://github.com/optimizely/javascript-sdk/pull/99/files#diff-1fbd9d0f02956a420aeef6959fc24caaR73

msohailhussain · 2018-05-31T18:15:08Z

optimizely/project_config.py

@@ -56,6 +56,7 @@ def __init__(self, datafile, logger, error_handler):
    self.feature_flags = config.get('featureFlags', [])
    self.rollouts = config.get('rollouts', [])
    self.anonymize_ip = config.get('anonymizeIP', False)
+    self.bot_filtering = config.get('botFiltering', False)


default is NULL, not false. Please recheck it.

msohailhussain · 2018-05-31T18:16:54Z

optimizely/event_builder.py

          })

+        else:
+          attribute = self.config.get_attribute(attribute_key)
+          if attribute:


in case attribute is not found, pruning that attribute but where's the log? Add log or I would suggest revise it as per javascript code.

msohailhussain · 2018-05-31T18:38:50Z

tests/test_config.py

+
+    # Assert bot filtering is False when not provided in data file
+    self.assertTrue('botFiltering' not in self.config_dict)
+    self.assertEqual(False, self.project_config.get_bot_filtering_value())


I need to check, when botFiltering is not in dictionary what it's supposed to return? it MUST be NULL, i haven't seen such condition in JS.

msohailhussain · 2018-05-31T18:43:13Z

tests/base.py

@@ -150,6 +150,7 @@ def setUp(self):
      'accountId': '12001',
      'projectId': '111111',
      'version': '4',
+      'botFiltering': False,


It may be included or not included, don't add it here. Let's discuss tonight.

msohailhussain · 2018-05-31T18:45:31Z

First revise the code and align with JS & then correct test cases. Right now, not reviewing test cases anymore.

…-filtering # Conflicts: # tests/test_config.py

aliabbasrizvi · 2018-06-07T03:30:40Z

optimizely/event_builder.py

@@ -18,6 +18,7 @@

 from . import version
 from .helpers import event_tag_utils
+from .helpers.enums import ControlAttributes


nit. I would usually just import enums and refer to enums.ControlAttributes

aliabbasrizvi · 2018-06-07T03:32:31Z

optimizely/event_builder.py

+    """ Get bot filtering bool
+
+    Returns:
+      'botFiltering' value in the datafile.


nit. Boolean representing whether bot filtering is enabled or not.

I would similarly also change the comment for IP anonymization in the method above to

Boolean representing whether IP anonymization is enabled or not.

aliabbasrizvi · 2018-06-07T03:33:04Z

optimizely/event_builder.py

@@ -172,19 +182,39 @@ def _get_attributes(self, attributes):
    if not attributes:
      return []

-    for attribute_key in attributes.keys():
+    for attribute_key in sorted(attributes.keys()):


Why does ordering matter? Let us not do this. It seems unnecessary.

aliabbasrizvi · 2018-06-07T03:34:26Z

optimizely/event_builder.py

            'key': attribute_key,
            'type': self.EventParams.CUSTOM,
            'value': attribute_value,
          })

+    # Append Bot Filtering Attribute
+    if isinstance(self._get_bot_filtering(), bool):


aliabbasrizvi · 2018-06-07T03:34:59Z

optimizely/event_builder.py

+          'entity_id': ControlAttributes.BOT_FILTERING,
+          'key': ControlAttributes.BOT_FILTERING,
+          'type': self.EventParams.CUSTOM,
+          'value': self._get_bot_filtering(),


Lets call self._get_bot_filtering at line 198 and use that.

aliabbasrizvi · 2018-06-12T05:16:40Z

optimizely/event_builder.py

-            'value': attribute_value,
-          })
+    if isinstance(attributes, dict):
+      for attribute_key in sorted(attributes.keys()):


Do no sort these.

aliabbasrizvi · 2018-06-12T05:19:07Z

optimizely/project_config.py

+
+      return attribute.id
+
+    if has_reserved_prefix and attribute_key != enums.ControlAttributes.BOT_FILTERING:


Why the not condition? I do not thing it is needed. Lets remove the second condition.

aliabbasrizvi · 2018-06-12T05:20:25Z

tests/test_config.py

+    # Assert bot filtering is retrieved as provided in the data file
+    opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
+    project_config = opt_obj.config
+    self.assertEqual(self.config_dict_with_features['botFiltering'],


nit. This styling seems wrong

self.assertEqual( self.config_dict_with_features['botFiltering'], project_config.get_bot_filtering_value() )

aliabbasrizvi · 2018-06-12T05:21:11Z

tests/test_config.py

+    """ Test that Attribute Key is returned as ID when provided attribute key is not
+    present in the datafile but has $opt prefix. """
+    self.assertEqual('$opt_interesting',
+                       self.project_config.get_attribute_id('$opt_interesting'))


nit. Indentation seems off. s of self should align with the '

oakbani · 2018-06-13T05:40:40Z

@aliabbasrizvi Please review the changes.

aliabbasrizvi

LGTM

oakbani added 2 commits May 31, 2018 15:18

🖊️ Bot Filtering changes + Fix previous unit tests

46c964e

🖊️ Add unit test for bot filtering

6b48a7b

msohailhussain reviewed May 31, 2018

View reviewed changes

oakbani added 3 commits June 1, 2018 17:03

🖊️ JS Sync impl

ca14331

🖊️ PEP8 fix

c458c08

Merge remote-tracking branch 'remotes/origin/master' into oakbani/bot…

0a7e74f

…-filtering # Conflicts: # tests/test_config.py

aliabbasrizvi suggested changes Jun 7, 2018

View reviewed changes

oakbani added 3 commits June 7, 2018 09:20

🖊️ Changes

9e05758

🖊️ sort attribute keys

b938de3

Add BotFiltering attr even when attributes are empty/None

d94231a

aliabbasrizvi suggested changes Jun 12, 2018

View reviewed changes

oakbani added 2 commits June 12, 2018 16:49

🖊️ Changes

cd7b448

🖊️ Sort in unit tests

c4359d1

oakbani added 3 commits June 20, 2018 17:07

Merge branch 'master' into oakbani/bot-filtering

e3295cd

Merge branch 'master' into oakbani/bot-filtering

aabb4a5

Update testcase overwritten in merge

fe607b4

aliabbasrizvi approved these changes Jun 27, 2018

View reviewed changes

aliabbasrizvi merged commit fc7bf6c into master Jun 28, 2018

aliabbasrizvi deleted the oakbani/bot-filtering branch June 28, 2018 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Bot Filtering #121

Introduce Bot Filtering #121

oakbani commented May 31, 2018

coveralls commented May 31, 2018 •

edited

Loading

msohailhussain May 31, 2018

oakbani Jun 1, 2018

aliabbasrizvi Jun 7, 2018

oakbani Jun 7, 2018

msohailhussain May 31, 2018

msohailhussain May 31, 2018

msohailhussain May 31, 2018

msohailhussain May 31, 2018

msohailhussain May 31, 2018

msohailhussain May 31, 2018

msohailhussain commented May 31, 2018

aliabbasrizvi Jun 7, 2018

aliabbasrizvi Jun 7, 2018

aliabbasrizvi Jun 7, 2018

aliabbasrizvi Jun 7, 2018

aliabbasrizvi Jun 7, 2018

aliabbasrizvi Jun 12, 2018

aliabbasrizvi Jun 12, 2018

aliabbasrizvi Jun 12, 2018

aliabbasrizvi Jun 12, 2018

oakbani commented Jun 13, 2018

aliabbasrizvi left a comment


		return attribute.id

		if has_reserved_prefix and attribute_key != enums.ControlAttributes.BOT_FILTERING:

Introduce Bot Filtering #121

Introduce Bot Filtering #121

Conversation

oakbani commented May 31, 2018

coveralls commented May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msohailhussain commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oakbani commented Jun 13, 2018

aliabbasrizvi left a comment

Choose a reason for hiding this comment

coveralls commented May 31, 2018 •

edited

Loading