fix(event-tags): do not exclude falsy revenue and event values in the event payload #213
Conversation
|
I am running @nchilada's compat suite tests here: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/builds/98673737 |
|
It passes @nchilada's tests here: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/builds/98673737 |
| if (eventTags) { | ||
| var revenue = eventTagUtils.getRevenueValue(eventTags, logger); | ||
| if (revenue) { | ||
| if (revenue !== null) { |
There was a problem hiding this comment.
Should we check for undefined?
There was a problem hiding this comment.
Nah because the getRevenueValue function will always return either null or the parsed value
There was a problem hiding this comment.
The most robust (redundant) solution would be to check whether the returned value is a number, but I'm not sure it matters
nchilada
left a comment
nchilada
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
| if (eventTags) { | ||
| var revenue = eventTagUtils.getRevenueValue(eventTags, logger); | ||
| if (revenue) { | ||
| if (revenue !== null) { |
There was a problem hiding this comment.
The most robust (redundant) solution would be to check whether the returned value is a number, but I'm not sure it matters
| assert.deepEqual(actualParams, expectedParams); | ||
| }); | ||
|
|
||
| it('should include falsy revenue values in the event object', function() { |
There was a problem hiding this comment.
Maybe just mention 0 directly? The current test name incorrectly implies that NaN and non-numeric falsy values like '' would also be sent.
Summary
nullinstead of performing a truthy check because the utility method will returnnullif the revenue or event values cannot be parsed from the tags.Test plan