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.

Sign up for GitHub

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

Jump to bottom

fix: Set MPID before open session #41

Merged
merged 10 commits into from
Jul 25, 2023
Merged

fix: Set MPID before open session #41

merged 10 commits into from
Jul 25, 2023

Conversation

SbDove
Copy link

@SbDove SbDove commented Jul 6, 2023 •
edited
Loading

Summary

  • checks forwarder settings to see if the identity type is set to MPID on initializations.
  • checks if MPID has already been established on kit initialization.
  • if both checks are successful, calls changeUser via onUserIdentified before openSession

Testing Plan

  • Testing was done by enabling local overrides to test kit initialization with the proper settings - no issues arose from the initialization process. but confirmation that the duplication sessions were corrected was not done.

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

@SbDove
Copy link
Author

SbDove commented Jul 6, 2023

@alexs-mparticl

rk-mp
rk-mp reviewed Jul 6, 2023
View reviewed changes
src/BrazeKit-dev.js Outdated Show resolved Hide resolved
alexs-mparticle
alexs-mparticle reviewed Jul 7, 2023
View reviewed changes
src/BrazeKit-dev.js Outdated Show resolved Hide resolved
@SbDove
Copy link
Author

SbDove commented Jul 13, 2023

@alexs-mparticle Pushed the tests. Let me know if there's anything else that needs to be addressed on this.

alexs-mparticle
alexs-mparticle requested changes Jul 14, 2023
View reviewed changes
Copy link
Collaborator

@alexs-mparticle alexs-mparticle left a comment

Choose a reason for hiding this comment

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

The changes look good. Please remove the changes to dist and and package-lock.json. These will be handled as part of our release job.

dist/BrazeKit.common.js Outdated
@@ -9172,7 +9172,7 @@ window.braze = require$$0;
var name = 'Appboy',
suffix = 'v4',
moduleId = 28,
version = '4.1.0',
version = '4.1.1',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please undo this change. Our process is to update the version number and distribution as a separate commit when we release.

Copy link
Author

Choose a reason for hiding this comment

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

This change seems to already be in the master-v4 branch. I don't think it was a change that I made. Not sure I know how to roll it back in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, you may need to rebase your branch against master-v4. I'll see if I can help you out with that.

dist/BrazeKit.iife.js Outdated
@@ -9172,7 +9172,7 @@ var mpBrazeKitV4 = (function (exports) {
var name = 'Appboy',
suffix = 'v4',
moduleId = 28,
version = '4.1.0',
version = '4.1.1',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please undo all changes in this file as well.

Copy link
Author

Choose a reason for hiding this comment

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

This change is also already in the master-v4 branch.

package-lock.json Outdated
@@ -6,7 +6,7 @@
"packages": {
"": {
"name": "@mparticle/web-braze-kit",
"version": "4.0.1",
"version": "4.1.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please roll this back.

Copy link
Author

Choose a reason for hiding this comment

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

Looking at the git blame it says that you made the change in the master-v4 branch a few days ago. I could very well be confused about these changes.

src/BrazeKit-dev.js Outdated Show resolved Hide resolved
test/tests.js
getMPID: function () {
return 'MPID123';
},
getUserIdentities: function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is getUserIdentities a necessary addition?

Copy link
Author

Choose a reason for hiding this comment

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

Currently, it is necessary, yes. The getUserIdentities function is needed in the onUserIdentified call which sets the Braze identity. In retrospect, I'm calling the onUserIdentified function which in turn calls the Braze changeUser function. Seems like I could skip the secondary function and call the Braze function directly - would it make sense to make that change to eliminate the getUserIdentities and the extra processing?

test/tests.js Outdated Show resolved Hide resolved
test/tests.js Outdated Show resolved Hide resolved
@SbDove SbDove force-pushed the fix/GOC-0004937-set-mpid-before-open_session branch from 61bdbed to 26b4093 Compare July 20, 2023 17:36
@alexs-mparticle alexs-mparticle changed the title Fix/goc-0004937, set MPID before open session fix: Set MPID before open session Jul 25, 2023
@alexs-mparticle alexs-mparticle merged commit c416392 into mparticle-integrations:master-v4 Jul 25, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rk-mp rk-mp rk-mp left review comments

@alexs-mparticle alexs-mparticle alexs-mparticle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SbDove @alexs-mparticle @rk-mp