feat(api): Decision Notification Listener for activate and getVariation APIs. #244
Conversation
| @@ -392,14 +395,30 @@ describe('lib/optimizely', function() { | |||
| }); | |||
|
|
|||
| describe('#activate', function() { | |||
| it('should call bucketer and dispatchEvent with proper args and return variation key', function() { | |||
| it('should call bucketer, send notifications and dispatchEvent with proper args and return variation key', function() { | |||
There was a problem hiding this comment.
lets split this test up, please add a new #activate for these notification center tests.
it's important to keep the base case of these tests without notificationCenter.
| ACTIVATE: 'ACTIVATE:experiment, user_id,attributes, variation, event', | ||
| TRACK: 'TRACK:event_key, user_id, attributes, event_tags, event', | ||
| ACTIVATE: 'ACTIVATE:experiment, userId, attributes, variation, logEvent', | ||
| TRACK: 'TRACK:eventKey, userId, attributes, eventTags, logEvent', |
There was a problem hiding this comment.
We can't change these enums, customers are already using them.
Please revert the NOTIFICATION_TYPES back to the way they were and make them use snake_case
| sinon.assert.calledWith(onDecisionListener, { | ||
| type: DECISION_INFO_TYPES.EXPERIMENT, | ||
| userId: 'testUser', | ||
| attributes: undefined, |
There was a problem hiding this comment.
this listener should be called with attributes: {} even if no attributes are provided.
Please change the logic where you send the notification
| { | ||
| type: DECISION_INFO_TYPES.EXPERIMENT, | ||
| userId: userId, | ||
| attributes: attributes, |
There was a problem hiding this comment.
linking to comment below, this should default to empty object if attribute isn't passed through
attributes: attributes || {},
There was a problem hiding this comment.
Done for decision listener. I just want to confirm that should we change it for activate and track listeners also?
There was a problem hiding this comment.
I confirmed with Ali, for Decision Listener it's fine.
| @@ -2287,7 +2288,7 @@ describe('lib/optimizely', function() { | |||
| browser_type: 'firefox', | |||
| }; | |||
| optlyInstance.notificationCenter.addNotificationListener( | |||
| enums.NOTIFICATION_TYPES.ACTIVATE, | |||
There was a problem hiding this comment.
Also, why make this change. It seems we are pretty consistent about using enums.XXX
Please change it back since this seems unnecessary and not the conventional way we access enums.
|
LGTM. @jordangarcia may want to take another look. |
| */ | ||
| exports.NOTIFICATION_TYPES = { | ||
| ACTIVATE: 'ACTIVATE:experiment, user_id,attributes, variation, event', | ||
| TRACK: 'TRACK:event_key, user_id, attributes, event_tags, event', | ||
| ON_DECISION: 'ON_DECISION:type, userId, attributes, decisionInfo', |
There was a problem hiding this comment.
Similar to feedback in other PR. This should be DECISION and not ON_DECISION.
There was a problem hiding this comment.
Yes, it has already been renamed. Please see the latest changes.
Summary
Test plan