feat: Add support to forward subscriptionGroupIds#57
Conversation
| function setSubscriptionGroups(key, value) { | ||
| const subscriptionGroupId = parsedSubscriptionGroupMapping[key]; | ||
| if (!(typeof value === 'boolean')) { | ||
| kitLogger("Can't call setSubscriptionGroups on forwarder " + | ||
| name + | ||
| ', setSubscriptionGroups must set this value to a boolean'); | ||
| return; | ||
| } else { | ||
| if (!value) { | ||
| kitLogger('braze.getUser().removeFromSubscriptionGroup', subscriptionGroupId); | ||
|
|
||
| braze.getUser().removeFromSubscriptionGroup(subscriptionGroupId); | ||
| } else { | ||
| kitLogger('braze.getUser().addToSubscriptionGroup', subscriptionGroupId); | ||
|
|
||
| braze.getUser().addToSubscriptionGroup(subscriptionGroupId); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think the nested if/else statements makes this function a bit hard to read.
Also typeof value !== 'boolean' reads better than !(typeof value === 'boolean').
P.S. Nit: Set up your linter!
| function setSubscriptionGroups(key, value) { | |
| const subscriptionGroupId = parsedSubscriptionGroupMapping[key]; | |
| if (!(typeof value === 'boolean')) { | |
| kitLogger("Can't call setSubscriptionGroups on forwarder " + | |
| name + | |
| ', setSubscriptionGroups must set this value to a boolean'); | |
| return; | |
| } else { | |
| if (!value) { | |
| kitLogger('braze.getUser().removeFromSubscriptionGroup', subscriptionGroupId); | |
| braze.getUser().removeFromSubscriptionGroup(subscriptionGroupId); | |
| } else { | |
| kitLogger('braze.getUser().addToSubscriptionGroup', subscriptionGroupId); | |
| braze.getUser().addToSubscriptionGroup(subscriptionGroupId); | |
| } | |
| } | |
| } | |
| function setSubscriptionGroups(key, value) { | |
| var subscriptionGroupId = parsedSubscriptionGroupMapping[key]; | |
| if (typeof value !== 'boolean') { | |
| kitLogger("Can't call setSubscriptionGroups on forwarder " + | |
| name + | |
| ', setSubscriptionGroups must set this value to a boolean'); | |
| return; | |
| } | |
| var action = value ? 'addToSubscriptionGroup' : 'removeFromSubscriptionGroup'; | |
| kitLogger('braze.getUser().' + action, subscriptionGroupId); | |
| braze.getUser()[action](subscriptionGroupId); | |
| } |
src/BrazeKit-dev.js
Outdated
| if (parsedSubscriptionGroupMapping[key]) { | ||
| setSubscriptionGroups(key, value) | ||
| } else { | ||
| var sanitizedKey = getSanitizedValueForBraze(key); | ||
| var sanitizedValue = getSanitizedValueForBraze(value); | ||
| if (value != null && sanitizedValue == null) { | ||
| return 'Value did not pass validation for ' + key; | ||
| } | ||
|
|
||
| kitLogger( | ||
| 'braze.getUser().setCustomUserAttribute', | ||
| sanitizedKey, | ||
| sanitizedValue | ||
| ); | ||
| kitLogger( | ||
| 'braze.getUser().setCustomUserAttribute', | ||
| sanitizedKey, | ||
| sanitizedValue | ||
| ); | ||
|
|
||
| braze | ||
| .getUser() | ||
| .setCustomUserAttribute(sanitizedKey, sanitizedValue); | ||
| braze | ||
| .getUser() | ||
| .setCustomUserAttribute(sanitizedKey, sanitizedValue); | ||
| } |
There was a problem hiding this comment.
Same here. I would rewrite this whole function to inverse the guards and make it cleaner.
function setUserAttribute(key, value) {
if (key in DefaultAttributeMethods) {
return setDefaultAttribute(key, value);
}
if (parsedSubscriptionGroupMapping[key]) {
setSubscriptionGroups(key, value);
return;
}
var sanitizedKey = getSanitizedValueForBraze(key);
var sanitizedValue = getSanitizedValueForBraze(value);
if (value != null && sanitizedValue == null) {
return 'Value did not pass validation for ' + key;
}
kitLogger(
'braze.getUser().setCustomUserAttribute',
sanitizedKey,
sanitizedValue
);
braze
.getUser()
.setCustomUserAttribute(sanitizedKey, sanitizedValue);
}
src/BrazeKit-dev.js
Outdated
| let subscriptionGroupIds = {}; | ||
| const decodedSetting = subscriptionGroupSetting.replace(/"/g, '"'); | ||
| const parsedSetting = JSON.parse(decodedSetting) | ||
| for (let subscriptionGroupMap of parsedSetting) { | ||
| const key = subscriptionGroupMap.map | ||
| const value = subscriptionGroupMap.value | ||
| subscriptionGroupIds[key] = value | ||
| } | ||
| return subscriptionGroupIds | ||
| } |
There was a problem hiding this comment.
As much as I want to, we should avoid using const and let in kits since we don't have transpiling set up.
| let subscriptionGroupIds = {}; | |
| const decodedSetting = subscriptionGroupSetting.replace(/"/g, '"'); | |
| const parsedSetting = JSON.parse(decodedSetting) | |
| for (let subscriptionGroupMap of parsedSetting) { | |
| const key = subscriptionGroupMap.map | |
| const value = subscriptionGroupMap.value | |
| subscriptionGroupIds[key] = value | |
| } | |
| return subscriptionGroupIds | |
| } | |
| var subscriptionGroupIds = {}; | |
| var decodedSetting = subscriptionGroupSetting.replace(/"/g, '"'); | |
| var parsedSetting = JSON.parse(decodedSetting) | |
| for (let subscriptionGroupMap of parsedSetting) { | |
| var key = subscriptionGroupMap.map | |
| var value = subscriptionGroupMap.value | |
| subscriptionGroupIds[key] = value | |
| } | |
| return subscriptionGroupIds | |
| } |
| window.braze.getUser().subscriptionGroup[mappedSubscriptionGroupId1].should.equal(true); | ||
|
|
||
| // set attribute subscriptionGroupTest2 with boolean value false should call Braze's removeFromSubscriptionGroup since it's mapped | ||
| mParticle.forwarder.setUserAttribute('subscriptionGroupTest2', false); |
There was a problem hiding this comment.
I think this should be a separate test. You should have one test for the "positive" case, a separate one for the "negative" case, and then additional tests for specific edge cases.
test/tests.js
Outdated
| mParticle.forwarder.msg = msg; | ||
| }, | ||
| }; | ||
| mParticle.forwarder.setUserAttribute('subscriptionGroupTest1', 'test'); |
There was a problem hiding this comment.
I would consider making this a separate test case as well.
test/tests.js
Outdated
| (window.braze.getUser().yearOfBirth === null).should.equal(true); | ||
| }); | ||
|
|
||
| it('it should set subscription group for mapped attributes when value is boolean', function() { |
There was a problem hiding this comment.
I would consider making specific unit tests for the new functions you've introduced if possible. I see this as an integration test, which is super helpful, but having tests for specific functions allows us to spot failures in specific functions makes it much easier to troubleshoot and catch bugs.
rmi22186
left a comment
There was a problem hiding this comment.
agree with all of Alex's points. otherwise lgtm
test/tests.js
Outdated
| (window.braze.getUser().yearOfBirth === null).should.equal(true); | ||
| }); | ||
|
|
||
| it('it should set subscription group for mapped attributes when value is boolean', function() { |
There was a problem hiding this comment.
no need for the line to start with it
| it('it should set subscription group for mapped attributes when value is boolean', function() { | |
| it('should set subscription group for mapped attributes when value is boolean', function() { |
test/tests.js
Outdated
| // get the decoded mapped subscriptionGroup | ||
| const parsedSubscriptionGroupMapping = mParticle.forwarder.decodeSubscriptionGroupMappings(subscriptionGroupMapping); |
There was a problem hiding this comment.
this should be its own test to determine that decodeSubscriptionGroupMappings is working properly
|
@rmi22186 @alexs-mparticle - thanks all for the review, just changed per your recommendations, let me know how it looks |
test/tests.js
Outdated
| mParticle.forwarder.init( | ||
| { | ||
| apiKey: '123456', | ||
| subscriptionGroupMapping: subscriptionGroupMapping, | ||
| }, | ||
| reportService.cb, | ||
| true, | ||
| null | ||
| ); |
There was a problem hiding this comment.
pretty sure this can be removed since you are only testing the method and you don't need to do a full initialization fo the forwarder to do so
| mParticle.forwarder.init( | |
| { | |
| apiKey: '123456', | |
| subscriptionGroupMapping: subscriptionGroupMapping, | |
| }, | |
| reportService.cb, | |
| true, | |
| null | |
| ); |
There was a problem hiding this comment.
@rmi22186 - just removed it, thanks for the feedback!
| var decodedSetting = subscriptionGroupSetting.replace(/"/g, '"'); | ||
| var parsedSetting = JSON.parse(decodedSetting); | ||
| for (let subscriptionGroupMap of parsedSetting) { | ||
| var key = subscriptionGroupMap.map; | ||
| var value = subscriptionGroupMap.value; | ||
| subscriptionGroupIds[key] = value; | ||
| } | ||
| return subscriptionGroupIds; |
There was a problem hiding this comment.
@mmustafa-tse I realize, we should actually put a try/catch block here just to be extra safe.
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)