Skip to content

Removed nested objects from Braze identify and tracking calls #153

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

Merged
merged 7 commits into from
Aug 2, 2019

Conversation

NikoRoberts
Copy link
Contributor

What does this PR do?
Stops the plugin from erroring when analytics.track or identify calls are made with incompatible nested object data for Braze.

Are there breaking changes in this PR?
No

Any background context you want to provide?
This is related to Segment tech support ticket 353525

Is there parity with the server-side/android/iOS integration components (if applicable)?
Yes, at least iOS does similar sanitization before attempting to send data to Braze
https://github.com/segmentio/analytics.js-integrations/blob/master/integrations/appboy/lib/index.js#L256

Does this require a new integration setting? If so, please explain how the new setting works
No

Links to helpful docs and other external resources
Segment's own docs cover several times that nested data is not accepted by Braze in these calls.
https://segment.com/docs/destinations/braze/#identify

@NikoRoberts NikoRoberts requested a review from a team July 17, 2019 12:40
@@ -407,19 +408,23 @@ describe('Appboy', function() {
analytics.called(window.appboy.logCustomEvent, 'event');
});

it('should send all properties', function() {
it('should send all properties except nested arrays and hashes', function() {
Copy link

Choose a reason for hiding this comment

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

Suggested change
it('should send all properties except nested arrays and hashes', function() {
it('should send all properties except nested arrays and objects', function() {

@@ -335,10 +335,11 @@ describe('Appboy', function() {
);
});

it('should handle custom traits of all types', function() {
it('should handle custom traits of acceptable types and excludes nested hashes', function() {
Copy link

Choose a reason for hiding this comment

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

Suggested change
it('should handle custom traits of acceptable types and excludes nested hashes', function() {
it('should handle custom traits of acceptable types and excludes nested objects', function() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

arrays are not objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typeof([])
"object"

Copy link
Contributor

@gpsamson gpsamson left a comment

Choose a reason for hiding this comment

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

All looks good except some of the type checking conditionals. I'll be making it one of my priorities this week to start the release process with you guys. 😄

First, by releasing this PR to Segment's internal test sources and Braze's test sources. After we can confirm all is working in our production test sources, we'll begin rolling it out to the rest of our mutual customers.

Cheers!

GPS

@NikoRoberts
Copy link
Contributor Author

Thanks @gpsamson let us know how we can help!

Copy link
Contributor

@gpsamson gpsamson left a comment

Choose a reason for hiding this comment

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

🚀

@gpsamson
Copy link
Contributor

One more thing @NikoRoberts. Can you bump the version to 1.10.1 here please: https://github.com/NikoRoberts/analytics.js-integrations/blob/braze-fixes/integrations/appboy/package.json#L4

I'll start validating the changes on our end, but if you want to test them out yourself let me know and I can reach out to you via email.

Thanks!

GPS

@NikoRoberts
Copy link
Contributor Author

@gpsamson done
Yes we are definitely interested in testing, please email me at niko.roberts (a) airtasker.com

@gpsamson gpsamson merged commit a4c8a79 into segmentio:master Aug 2, 2019
patrickotoole pushed a commit to rockerbox/analytics.js-integrations that referenced this pull request Feb 27, 2020
…tio#153)

* Removed nested objects from Braze identify and tracking calls

* Apply suggestions from code review

* bump braze integration version to v1.10.1

* Update integrations/appboy/lib/index.js
marinhero pushed a commit to Wootric/analytics.js-integrations-1 that referenced this pull request Apr 24, 2020
…tio#153)

* Removed nested objects from Braze identify and tracking calls

* Apply suggestions from code review

* bump braze integration version to v1.10.1

* Update integrations/appboy/lib/index.js
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