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

Pendo sdk #164

Closed
wants to merge 16 commits into from
Closed

Pendo sdk #164

wants to merge 16 commits into from

Conversation

Shivam4415
Copy link
Contributor

@Shivam4415 Shivam4415 commented Nov 24, 2020

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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 56.97 KB (-0.38% 🔽) 1.2 s (-0.38% 🔽) 333 ms (-2.84% 🔽) 1.5 s

identify(rudderElement) {
let visitorObj = {};
visitorObj.id =
rudderElement.message.userId || rudderElement.message.anonymousId;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is correct.. What segment does is I user id is not present they append the anonymous id with PENDO_T . We can implement this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will do that, and what u are suggesting is actually correct, it will be easy to understand anonymous user by looking at Prefix :: PENDO_T

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

if (rudderElement.message.context.traits) {
visitorObj = {
...visitorObj,
...rudderElement.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.

please check if nested objects (the traits) are sent if it is being handled by panda or we have to send it as flattened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flattened the traits object and sending it to PendoAgent.
Pendo has an agent section that accepts traits that are sent for that visitor or account.

}

track(rudderElement) {
window.pendoCli.track(rudderElement.message.event);
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked track events are sent like this in panda sdk --- >
pendo.track("User Registered", {
userId: user.id,
plan: user.plan,
accountType: "Facebook"
});
});

So shouldn't the properties go along with the name of the event?
Here also check about the nested properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sending properties along with track calls.
We don't need to flatten this properties. Pendo take care of nested objects.
Fixed

@@ -40,7 +41,7 @@ const integrations = {
OPTIMIZELY: Optimizely.default,
BUGSNAG: Bugsnag.default,
FULLSTORY: Fullstory.default,
TVSQUARED: TVSquared.default,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.53 KB (+0.6% 🔺) 1.2 s (+0.6% 🔺) 348 ms (+14.5% 🔺) 1.5 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.77 KB (+1.02% 🔺) 1.2 s (+1.02% 🔺) 332 ms (-9.64% 🔽) 1.5 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.78 KB (+1.03% 🔺) 1.2 s (+1.03% 🔺) 411 ms (+25.89% 🔺) 1.6 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.81 KB (+1.08% 🔺) 1.2 s (+1.08% 🔺) 336 ms (+32.69% 🔺) 1.5 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.75 KB (+1% 🔺) 1.2 s (+1% 🔺) 319 ms (+37.87% 🔺) 1.5 s

Copy link
Contributor

@ruchiramoitra ruchiramoitra left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Few comments on functionality.
Add a PR to production-staging branch too

integrations/Pendo/browser.js Outdated Show resolved Hide resolved
integrations/Pendo/browser.js Outdated Show resolved Hide resolved
integrations/Pendo/browser.js Outdated Show resolved Hide resolved
utils/utils.js Show resolved Hide resolved
integrations/Pendo/browser.js Outdated Show resolved Hide resolved
integrations/Pendo/browser.js Outdated Show resolved Hide resolved
group(rudderElement) {
const obj = {};
let accountObj = {};
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.

our group level traits are present at top level.
user traits are present inside context.
I think we need to send both visitor and account object by reading traits from top level and under context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sending both object

if (traits) {
accountObj = {
...accountObj,
...flattenJsonPayload(traits),
Copy link
Contributor

Choose a reason for hiding this comment

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

also is flatten necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing flatten as Pendo cannot recognize property with dot operator

...accountObj,
...flattenJsonPayload(traits),
};
obj.account = accountObj;
Copy link
Contributor

Choose a reason for hiding this comment

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

for account there seems to be a concept of parentAccounts. Can you check that

@Shivam4415
Copy link
Contributor Author

Few comments on functionality.
Add a PR to production-staging branch too

@Shivam4415 Shivam4415 closed this Nov 26, 2020
@Shivam4415 Shivam4415 reopened this Nov 26, 2020
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.75 KB (+1% 🔺) 1.2 s (+1% 🔺) 381 ms (+28.82% 🔺) 1.6 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.71 KB (+0.91% 🔺) 1.2 s (+0.91% 🔺) 346 ms (+9.6% 🔺) 1.5 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.77 KB (+1.02% 🔺) 1.2 s (+1.02% 🔺) 318 ms (-0.05% 🔽) 1.5 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.73 KB (+0.95% 🔺) 1.2 s (+0.95% 🔺) 226 ms (-51.89% 🔽) 1.4 s

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.71 KB (+0.92% 🔺) 1.2 s (+0.92% 🔺) 347 ms (+7.89% 🔺) 1.6 s

@sayan-mitra sayan-mitra changed the base branch from master to production-staging November 26, 2020 17:15
@sayan-mitra sayan-mitra changed the base branch from production-staging to master November 26, 2020 18:06
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

size-limit report

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/rudder-analytics.min.js 57.69 KB (+0.88% 🔺) 1.2 s (+0.88% 🔺) 345 ms (-5.27% 🔽) 1.5 s

@sayan-mitra
Copy link
Contributor

closing this as #167 addresses this

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