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
| 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
|
|
||
| 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.
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.