feat(eventprocessor): Datamodel for event processor by msohailhussain · Pull Request #192 · optimizely/python-sdk

msohailhussain · 2019-07-23T21:11:58Z

Summary

Data models to represent Impression/Conversion event on client/server side.

Test plan

Unit tests to verify after serialization datamodel is converted into expected server payload
Impression Event
Conversion Event

coveralls · 2019-07-23T21:24:26Z

Coverage decreased (-2.0%) to 96.424% when pulling a6ff8fe on sohail/pr-189 into f5b0d0c on master.

aliabbasrizvi · 2019-08-15T16:15:48Z

optimizely/event/entity/event_context.py

+# limitations under the License.
+from .. import version
+
+SDK_VERSION = 'python-sdk'


This is not SDK_VERSION

aliabbasrizvi · 2019-08-15T16:17:28Z

optimizely/event/entity/event_context.py

+# 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


I'd say

from optimizely import version

aliabbasrizvi · 2019-08-15T16:19:03Z

optimizely/event/entity/impression_event.py

+from .user_event import UserEvent
+
+
+class ImpressionEvent(UserEvent):


This classification under entity is vague.

ImpressionEvent ideally belongs under event/ and not under event/entity.

Similarly ConversionEvent and UserEvent

I'd recommend putting all 3 of them in a single fine in optimizely/event/user_event.py

aliabbasrizvi · 2019-08-15T16:21:04Z

optimizely/event/entity/decision.py

+# limitations under the License.
+
+
+class Decision(object):


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.

msohailhussain · 2019-08-19T17:25:29Z

@aliabbasrizvi I will get back to you on this PR today.

aliabbasrizvi

More feedback.

aliabbasrizvi · 2019-08-21T05:43:51Z

optimizely/event/event_payload.py

@@ -0,0 +1,76 @@
+# Copyright 2019 Optimizely


The word event in event_payload is redundant as it is already under event package. Perhaps just say payload.py

aliabbasrizvi · 2019-08-21T05:44:27Z

optimizely/event/user_event.py

+
+from optimizely import version
+
+SDK_TYPE = 'python-sdk'


Rename this to CLIENT_NAME

aliabbasrizvi · 2019-08-21T05:50:14Z

optimizely/event/event_payload.py

+  def __init__(self, entity_id, key, event_type, value):
+    self.entity_id = entity_id
+    self.key = key
+    self.type = event_type


I'd refrain from using a variable called type as it is a keyword in Python.

May be just say attribute_type.

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.

aliabbasrizvi · 2019-08-21T05:51:59Z

optimizely/event/event_payload.py

+    self.visitor_id = visitor_id
+
+
+class VisitorAttribute(object):


Let's just call this Attribute and all references to it as attribute

aliabbasrizvi · 2019-08-21T05:52:52Z

optimizely/event/event_payload.py

+class VisitorAttribute(object):
+  """ Class representing Visitor Attribute. """
+
+  def __init__(self, entity_id, key, event_type, value):


This should be attribute_type

optimizely/event/user_event.py

tests/test_event_payload.py

aliabbasrizvi · 2019-08-21T05:56:29Z

tests/test_event_payload.py

+
+
+class EventPayloadTest(base.BaseTest):
+  def _validate_event_object(self, expected_params, event_obj):


You should ideally override __eq__ method on the event object and not worry about the logic here.

zashraf1985 · 2019-08-28T05:09:01Z

@aliabbasrizvi .. This is ready for another review. It would be great if you can take another look and provide feedback.

optimizely/event/payload.py

aliabbasrizvi · 2019-08-23T18:42:35Z

optimizely/event/payload.py

+    self.client_version = client_version
+    self.anonymize_ip = anonymize_ip
+    self.enrich_decisions = enrich_decisions
+    self.visitors = visitors


This should be self.visitors = visitors or []. That will make it valid.

aliabbasrizvi · 2019-08-23T18:57:34Z

optimizely/event/user_event.py

+class ImpressionEvent(UserEvent):
+  """ Class representing Impression Event. """
+
+  def __init__(self, event_context, user_id, experiment, visitor_attributes, variation, bot_filtering=None):


We can probably move user_id, bot_filtering and attributes to UserEvent as well

Feedback not addressed. Is there a reason for not moving this?

aliabbasrizvi

Looks good. Minor feedback.

aliabbasrizvi · 2019-08-28T05:33:24Z

tests/test_event_payload.py

+# limitations under the License.
+
+from optimizely import version
+from optimizely.event.payload import Decision, EventBatch, Snapshot, SnapshotEvent, Visitor, VisitorAttribute


nit. Why not import payload and reference each of them directly?

… sohail/pr-189

aliabbasrizvi

Some feedback was missed. Can you take a look?

… sohail/pr-189

mjamal@folio3.com and others added 3 commits July 19, 2019 12:16

feat: Add event_batch datamodels.

232f0a0

fix: unit test fixes.

2320939

Merge branch 'master' into epmodels

fbb5a68

msohailhussain changed the title ~~test PR - Sohail/pr 189~~ feat(eventprocessor): Datamodel for event processor Jul 23, 2019

msohailhussain requested review from a team and mikeproeng37 July 24, 2019 05:24

msohailhussain mentioned this pull request Jul 24, 2019

feat(EventFactory): EventFactory and UserEventFactory #193

Closed

msohailhussain requested a review from aliabbasrizvi July 24, 2019 18:00

Merge branch 'master' into sohail/pr-189

8a5f531

jamesopti added the 🤖 24 hrs label Aug 15, 2019

aliabbasrizvi suggested changes Aug 15, 2019

View reviewed changes

jamesopti removed the 🤖 24 hrs label Aug 15, 2019

jamesopti assigned msohailhussain Aug 15, 2019

Merge branch 'master' into sohail/pr-189

c829fca

Addressed feedback. Restructured classes.

c3c9d46

msohailhussain requested a review from aliabbasrizvi August 20, 2019 18:01

aliabbasrizvi suggested changes Aug 21, 2019

View reviewed changes

mjamal@folio3.com and others added 2 commits August 21, 2019 17:49

fix: addressed more feedback

23ab6ce

update: removed print statements.

6474e98

msohailhussain requested a review from aliabbasrizvi August 22, 2019 05:59

Merge branch 'master' into sohail/pr-189

d4f2c66

aliabbasrizvi reviewed Aug 28, 2019

View reviewed changes

aliabbasrizvi approved these changes Aug 28, 2019

View reviewed changes

mjamal@folio3.com added 3 commits August 28, 2019 11:30

fix: addressed minor feedback.

80b0963

fix: addressed minor feedback.

071460a

Merge branch 'sohail/pr-189' of github.com:optimizely/python-sdk into…

900d96d

… sohail/pr-189

aliabbasrizvi reviewed Sep 5, 2019

View reviewed changes

mjamal@folio3.com and others added 2 commits September 6, 2019 09:41

Merge branch 'sohail/pr-189' of github.com:optimizely/python-sdk into…

8fabcdb

… sohail/pr-189

Merge branch 'sohail/pr-189' of github.com:optimizely/python-sdk into…

a6ff8fe

… sohail/pr-189

aliabbasrizvi merged commit 96b4d55 into master Sep 12, 2019

oakbani deleted the sohail/pr-189 branch September 24, 2019 19:16

		from .user_event import UserEvent


		class ImpressionEvent(UserEvent):



		class EventPayloadTest(base.BaseTest):
		def _validate_event_object(self, expected_params, event_obj):

Comments

Conversation

msohailhussain commented Jul 23, 2019 • edited by mnoman09 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain commented Aug 19, 2019

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zashraf1985 commented Aug 28, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

msohailhussain commented Jul 23, 2019 •

edited by mnoman09

Loading

coveralls commented Jul 23, 2019 •

edited

Loading