Skip to content
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

onboarding clevertap #219

Merged
merged 4 commits into from
Mar 17, 2021
Merged

onboarding clevertap #219

merged 4 commits into from
Mar 17, 2021

Conversation

utsabc
Copy link
Member

@utsabc utsabc commented Mar 8, 2021

Description of the change

Description here

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fix #1

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

This change is Reviewable

@utsabc utsabc requested a review from sayan-mitra March 8, 2021 11:40
? "https://d2r1yp2w7bby2u.cloudfront.net/js/a.js"
: "http://static.clevertap.com/js/a.js";

ScriptLoader("clevertap-integration", sourceUrl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do this after the below part.
though same thing but still :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


isLoaded() {
logger.debug("in clevertap isLoaded");
return !!window.clevertap && window.clevertap.logout !== "undefined";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"undefined" --> undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed

logger.debug("in clevertap identify");

const { message } = rudderElement;
if (!(message.context && message.context.traits)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is payload with only identity invalid?
i.e not traits but only userId/anonymousId?

Copy link
Member Author

@utsabc utsabc Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wehn identify is called without traits or id i.e rudderanalytics.identify() by default, SDK is generating an empty object for traits. I don't think this block will be executed, still, should we keep it for any kind of future SDK changes fail-safety?

Screenshot 2021-03-12 at 5 31 16 PM

logger.debug(`Error occured at extractCustomFields ${err}`);
}

window.clevertap.onUserLogin.push({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use the window.clevertap.profile.push ? not sure I think onUserLogin is what is documented.
still check once

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think something to do. with the clevertap native sdk behaviour we observed

logger.debug("in clevertap track");
const { event, properties } = rudderElement.message;
if (properties) {
window.clevertap.event.push(event, properties);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try sending inner objects or arrays. check if that as property is supported

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is only supported in charged events we should use a validator for other events for checking if arrays or objects are are present

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added validator

@utsabc utsabc requested a review from sayan-mitra March 12, 2021 12:07
utils/utils.js Outdated
* To check if a variable is storing object or not
*/
const isObject = (obj) => {
return Object.prototype.toString.call(obj) === '[object Object]';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use the type method present in the utils

utils/utils.js Outdated
* To check if a variable is storing object or not
*/
const isArray = (obj) => {
return Object.prototype.toString.call(obj) === '[object Array]';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@sayan-mitra sayan-mitra merged commit 9dfa286 into production-staging Mar 17, 2021
@akashrpo akashrpo deleted the clevertap branch March 5, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants