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

added for moengage and tested #153

Merged
merged 8 commits into from
Nov 12, 2020
Merged

added for moengage and tested #153

merged 8 commits into from
Nov 12, 2020

Conversation

ruchiramoitra
Copy link
Contributor

@ruchiramoitra ruchiramoitra commented Nov 6, 2020

Description of the change

Description here

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • [ yes ] 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

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

@ruchiramoitra ruchiramoitra changed the title added for moengage and tested [WIP] added for moengage and tested Nov 6, 2020
@ruchiramoitra ruchiramoitra requested a review from arnab-p November 6, 2020 10:12
Copy link
Contributor

@sayan-mitra sayan-mitra left a comment

Choose a reason for hiding this comment

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

add code comments heavily.
change the PR description and other fields specific to this one. (fixes #1) etc is not helpful here,

import each from "@ndhoule/each";
import logger from "../../utils/logUtil";

class MoEngage {
Copy link
Contributor

Choose a reason for hiding this comment

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

please include comments involving the destination documentation
any special case handling and why it is required and where it's mentioned in destination docs etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

window,
document,
"script",
"https://cdn.moengage.com/webpush/moe_webSdk.min.latest.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add this with Scriptloader? Also include the check that which protocol the website site is opened with (http/https etc) and depending on that get the destination sdk min file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (rudderElement.message) {
const { anonymousId, event, properties } = rudderElement.message;
if (anonymousId) {
if (this.initialAnonId !== anonymousId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

also reinitialise this.initialAnonId to new anonymousID?
also, it's almost expected that anonId will not change, is it better to perform this using userID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to user id

this.moeClient.track_event(event);
}
} else {
logger.error("Event name not present");
Copy link
Contributor

Choose a reason for hiding this comment

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

perform this and below check earlier to reduce if/else condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not get this.

reset() {
logger.debug("inside reset");
// reset the anonymous id
this.initialAnonId = window.rudderanalytics.getAnonymousId();
Copy link
Contributor

Choose a reason for hiding this comment

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

also, it's almost expected that anonId will not change, is it better to perform this using userID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

this.moeClient.add_unique_user_id(userId);
}
// custom traits mapping context.traits --> moengage properties
const traitsMap = {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this const outside of the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// if any of the custom properties are present it will be sent
// For example : email is present in username then the method will be add_user_name

if (Object.prototype.hasOwnProperty.call(traitsMap, key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can the below 3 loops be merged into one?
perform lookup to traitsMap, if present use add${key_name} else use add_user_attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but we have to loop through traits too right? to send the remaining attributes?

@@ -33,8 +33,9 @@
})(method);
}

rudderanalytics.load("1cA1w20Fe34TdZrtGshutNY8Fm1", "http://localhost:8080", {
Copy link
Contributor

Choose a reason for hiding this comment

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

include this test file inside the moengage dest folder.
also remove so many commented lines and use the final tests that were run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a test folder with different cases which I finally ran including the results.

identify(rudderElement) {
const self = this;
const { anonymousId, userId } = rudderElement.message;
const { traits } = rudderElement.message.context;
Copy link
Contributor

Choose a reason for hiding this comment

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

put a check if context is present or not.
in general, whenever there is more than one indirection, it's better to check for existence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ruchiramoitra ruchiramoitra changed the base branch from production to production-staging November 11, 2020 12:43
email: "email",
phone: "mobile",
name: "user_name",
username: "user_name",
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're supporting camelCase, should we support userName as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@arnab-p arnab-p left a comment

Choose a reason for hiding this comment

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

LGTM

@sayan-mitra sayan-mitra merged commit 949db6a into production-staging Nov 12, 2020
@akashrpo akashrpo deleted the moengageProd branch January 20, 2022 07:22
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.

3 participants