feat(notification-center): Added LogEvent notification and integration. #197
Conversation
| NotificationCenter::NOTIFICATION_TYPES[:TRACK], | ||
| event_key, user_id, attributes, event_tags, log_event | ||
| ) | ||
| if @notification_center.notification_count(NotificationCenter::NOTIFICATION_TYPES[:TRACK]).positive? |
There was a problem hiding this comment.
What do we gain by checking notification count, as opposed to just sending notifications regardless of count?
There was a problem hiding this comment.
This validates that the listener has already been added before sending the notification.
Please view java-sdk as a reference: https://github.com/optimizely/java-sdk/blob/master/core-api/src/main/java/com/optimizely/ab/Optimizely.java#L304
| @@ -36,10 +36,11 @@ class BatchEventProcessor < EventProcessor | |||
|
|
|||
| def initialize( | |||
| event_queue: SizedQueue.new(DEFAULT_QUEUE_CAPACITY), | |||
| event_dispatcher:, | |||
| event_dispatcher: Optimizely::EventDispatcher.new, | |||
There was a problem hiding this comment.
Why are we adding a default value here?
There was a problem hiding this comment.
This is not related to LogEvent, therefore moved to PR: #200
| @@ -144,6 +151,16 @@ def run | |||
|
|
|||
| add_to_batch(item) if item.is_a? Optimizely::UserEvent | |||
| end | |||
| rescue SignalException | |||
There was a problem hiding this comment.
This seems like an important change, but it isn't discussed in the summary. Could you explain this change? If this isn't related to LogEvent notification, then I think it should be in its own separate PR.
There was a problem hiding this comment.
Moved changes into PR: #200.
Summary
LogEventnotification innotification_center.rb.batch_event_dispatcher.rbandforwarding_event_processor.rb.notification_countinnotification_center.rband integrated intoOptimizelyfornotification typesACTIVATE and TRACK.Test plan
LogEventinnotification_center_spec.rb,batch_event_dispatcher_spec.rbandforwarding_event_processor_spec.rb.Optimizely'sunit tests forLogEventandnotification_count.