feat(eventprocessor): Datamodel for event processor #192
Conversation
msohailhussain
changed the title
| # limitations under the License. | ||
| from .. import version | ||
|
|
||
| SDK_VERSION = 'python-sdk' |
There was a problem hiding this comment.
This is not SDK_VERSION
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| from .. import version |
There was a problem hiding this comment.
I'd say
from optimizely import version
| from .user_event import UserEvent | ||
|
|
||
|
|
||
| class ImpressionEvent(UserEvent): |
There was a problem hiding this comment.
This classification under entity is vague.
ImpressionEvent ideally belongs under event/ and not under event/entity.
Similarly ConversionEvent and UserEvent
There was a problem hiding this comment.
I'd recommend putting all 3 of them in a single fine in optimizely/event/user_event.py
optimizely/event/entity/decision.py
Outdated
| # limitations under the License. | ||
|
|
||
|
|
||
| class Decision(object): |
There was a problem hiding this comment.
Would recommend putting all classes related to event payload in a single file. I would recommend calling the file optimizely/event/payload.py or something.
|
@aliabbasrizvi I will get back to you on this PR today. |
optimizely/event/event_payload.py
Outdated
| @@ -0,0 +1,76 @@ | |||
| # Copyright 2019 Optimizely | |||
There was a problem hiding this comment.
The word event in event_payload is redundant as it is already under event package. Perhaps just say payload.py
optimizely/event/user_event.py
Outdated
|
|
||
| from optimizely import version | ||
|
|
||
| SDK_TYPE = 'python-sdk' |
There was a problem hiding this comment.
Rename this to CLIENT_NAME
optimizely/event/event_payload.py
Outdated
| def __init__(self, entity_id, key, event_type, value): | ||
| self.entity_id = entity_id | ||
| self.key = key | ||
| self.type = event_type |
There was a problem hiding this comment.
I'd refrain from using a variable called type as it is a keyword in Python.
May be just say attribute_type.
There was a problem hiding this comment.
attribute type is expected as 'type' keyword on server side and even if changed to 'attribute_type', it will have to be converted to 'type' eventually before sending to server.
Followed C# and Java SDKs implementation here and both of these SDKs are also using 'type' as attribute type key.
optimizely/event/event_payload.py
Outdated
| self.visitor_id = visitor_id | ||
|
|
||
|
|
||
| class VisitorAttribute(object): |
There was a problem hiding this comment.
Let's just call this Attribute and all references to it as attribute
optimizely/event/event_payload.py
Outdated
| class VisitorAttribute(object): | ||
| """ Class representing Visitor Attribute. """ | ||
|
|
||
| def __init__(self, entity_id, key, event_type, value): |
There was a problem hiding this comment.
This should be attribute_type
tests/test_event_payload.py
Outdated
|
|
||
|
|
||
| class EventPayloadTest(base.BaseTest): | ||
| def _validate_event_object(self, expected_params, event_obj): |
There was a problem hiding this comment.
You should ideally override __eq__ method on the event object and not worry about the logic here.
|
@aliabbasrizvi .. This is ready for another review. It would be great if you can take another look and provide feedback. |
optimizely/event/payload.py
Outdated
| self.client_version = client_version | ||
| self.anonymize_ip = anonymize_ip | ||
| self.enrich_decisions = enrich_decisions | ||
| self.visitors = visitors |
There was a problem hiding this comment.
This should be self.visitors = visitors or []. That will make it valid.
| class ImpressionEvent(UserEvent): | ||
| """ Class representing Impression Event. """ | ||
|
|
||
| def __init__(self, event_context, user_id, experiment, visitor_attributes, variation, bot_filtering=None): |
There was a problem hiding this comment.
We can probably move user_id, bot_filtering and attributes to UserEvent as well
There was a problem hiding this comment.
Feedback not addressed. Is there a reason for not moving this?
tests/test_event_payload.py
Outdated
| # limitations under the License. | ||
|
|
||
| from optimizely import version | ||
| from optimizely.event.payload import Decision, EventBatch, Snapshot, SnapshotEvent, Visitor, VisitorAttribute |
There was a problem hiding this comment.
nit. Why not import payload and reference each of them directly?
Summary
Test plan
Impression Event
Conversion Event