Conversation
optimizely/event_builder.py
Outdated
| return [] | ||
|
|
||
| for attribute_key in attributes.keys(): | ||
| for attribute_key in sorted(attributes.keys()): |
There was a problem hiding this comment.
why do we need to sort?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why does ordering matter? Let us not do this. It seems unnecessary.
There was a problem hiding this comment.
@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
optimizely/event_builder.py
Outdated
| attribute = self.config.get_attribute(attribute_key) | ||
| if attribute: | ||
| # Check for reserved attributes | ||
| reserved_attrs = [ReservedAttributes.BUCKETING_ID, ReservedAttributes.USER_AGENT] |
There was a problem hiding this comment.
this must be outside, as it will be initialized again and again.
optimizely/event_builder.py
Outdated
|
|
||
| # Append Bot Filtering Attribute | ||
| attribute_key = ReservedAttributes.BOT_FILTERING | ||
| params.append({ |
There was a problem hiding this comment.
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
optimizely/project_config.py
Outdated
| 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) |
There was a problem hiding this comment.
default is NULL, not false. Please recheck it.
optimizely/event_builder.py
Outdated
|
|
||
| else: | ||
| attribute = self.config.get_attribute(attribute_key) | ||
| if attribute: |
There was a problem hiding this comment.
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.
tests/test_config.py
Outdated
|
|
||
| # 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()) |
There was a problem hiding this comment.
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.
tests/base.py
Outdated
| 'accountId': '12001', | ||
| 'projectId': '111111', | ||
| 'version': '4', | ||
| 'botFiltering': False, |
There was a problem hiding this comment.
It may be included or not included, don't add it here. Let's discuss tonight.
|
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
optimizely/event_builder.py
Outdated
|
|
||
| from . import version | ||
| from .helpers import event_tag_utils | ||
| from .helpers.enums import ControlAttributes |
There was a problem hiding this comment.
nit. I would usually just import enums and refer to enums.ControlAttributes
optimizely/event_builder.py
Outdated
| """ Get bot filtering bool | ||
|
|
||
| Returns: | ||
| 'botFiltering' value in the datafile. |
There was a problem hiding this comment.
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.
optimizely/event_builder.py
Outdated
| return [] | ||
|
|
||
| for attribute_key in attributes.keys(): | ||
| for attribute_key in sorted(attributes.keys()): |
There was a problem hiding this comment.
Why does ordering matter? Let us not do this. It seems unnecessary.
optimizely/event_builder.py
Outdated
| }) | ||
|
|
||
| # Append Bot Filtering Attribute | ||
| if isinstance(self._get_bot_filtering(), bool): |
optimizely/event_builder.py
Outdated
| 'entity_id': ControlAttributes.BOT_FILTERING, | ||
| 'key': ControlAttributes.BOT_FILTERING, | ||
| 'type': self.EventParams.CUSTOM, | ||
| 'value': self._get_bot_filtering(), |
There was a problem hiding this comment.
Lets call self._get_bot_filtering at line 198 and use that.
optimizely/event_builder.py
Outdated
| 'value': attribute_value, | ||
| }) | ||
| if isinstance(attributes, dict): | ||
| for attribute_key in sorted(attributes.keys()): |
optimizely/project_config.py
Outdated
|
|
||
| return attribute.id | ||
|
|
||
| if has_reserved_prefix and attribute_key != enums.ControlAttributes.BOT_FILTERING: |
There was a problem hiding this comment.
Why the not condition? I do not thing it is needed. Lets remove the second condition.
tests/test_config.py
Outdated
| # 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'], |
There was a problem hiding this comment.
nit. This styling seems wrong
self.assertEqual(
self.config_dict_with_features['botFiltering'],
project_config.get_bot_filtering_value()
)
tests/test_config.py
Outdated
| """ 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')) |
There was a problem hiding this comment.
nit. Indentation seems off. s of self should align with the '
|
@aliabbasrizvi Please review the changes. |
No description provided.