fix: Report purchase events to mParticle when bundling ecommerce data #50
Conversation
src/BrazeKit-dev.js
Outdated
| @@ -129,6 +129,8 @@ var constructor = function () { | |||
| quantity, | |||
| eventAttributes | |||
| ); | |||
|
|
|||
| return reportEvent; | |||
There was a problem hiding this comment.
wouldn't this be return reportEvent === true? thats how at least other log events functions have it as and I think it makes sense in case its not true? but also the video you shared for testing seems to be working as expected
There was a problem hiding this comment.
Good attention to detail! Yes so it seems that Braze returns true from when you call their login method already, so, so doing just return reportEvent is fine. I'm not sure why we do return reportEvent === true since it is essentially the same thing. For now, to be consistent, I will just follow that format even though it's unnecessary, but I can make a ticket to refactor in the future to just return the boolean instead of the equivalence.
test/tests.js
Outdated
| @@ -1756,6 +1756,7 @@ user.getUserIdentities is not a function,\n`; | |||
| baz: 'bar', | |||
| }; | |||
|
|
|||
| debugger; | |||
There was a problem hiding this comment.
noobie question, but wouldn't this pause the process of the test running and should be removed? I might be wrong
There was a problem hiding this comment.
Yes this is a mistake. I left it here. We should never have debugger in live code.
test/tests.js
Outdated
| @@ -1832,6 +1833,8 @@ user.getUserIdentities is not a function,\n`; | |||
| var purchaseEventProperties = window.braze.purchaseEventProperties[0]; | |||
|
|
|||
| purchaseEventProperties.should.eql(expectedPurchaseEvent); | |||
|
|
|||
| reportService.event.should.be.ok(); | |||
There was a problem hiding this comment.
I feel like this should be more like
reportService.event.should.have.property( 'EventName', 'eCommerce - Purchase' );
There was a problem hiding this comment.
I think being more specific is a good call. Will update.
Instructions
developmentSummary
Testing Plan
Link to test app showing the fix
https://share.zight.com/kpuvJZvD
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)