-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adobe Analytics #250
Adobe Analytics #250
Conversation
"Adobe Analytics": "ADOBE_ANALYITCS", | ||
ADOBE_ANALYTICS: "ADOBE_ANALYTICS", | ||
AdobeAnalytics: "ADOBE_ANALYTICS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's support all lowercases
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
integrations/AdobeAnalytics/util.js
Outdated
* @param {} return true or false accordingly if value is empty | ||
*/ | ||
|
||
function isEmpty(value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not mix ES5 and ES6 and fix on ES6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
integrations/AdobeAnalytics/util.js
Outdated
value === undefined || | ||
value === null || | ||
(typeof value === "object" && Object.keys(value).length === 0) || | ||
(typeof value === "string" && value.trim().length === 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we handle array
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,69 @@ | |||
/* eslint-disable no-param-reassign */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to move this file to a common util file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
// Begin heartbeat implementation | ||
|
||
populatHeartbeat(rudderElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
populatHeartbeat(rudderElement) { | |
populateHeartbeat(rudderElement) { |
typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
campaign = properties.campaign; | ||
} | ||
const channel = rudderElement.message.channel || properties.channel; | ||
const { state, zip } = properties; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take from traits
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const key = rudderProp.productMerchProperties.split("."); | ||
// take the keys after products. and find the value in properties | ||
const value = _.get(properties, key[1]); | ||
if (value && value !== "undefined") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check similar implementations and figure out if we can move it to a util method
Description of the change
Type of change
Related issues
Checklists
Development
Code review
This change is