-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Add GDPR and CCPA Consent #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together.. I left some feedback. Note that I only commented on one occurrence of each item but changes will need to be made in two places for all except the first.
plugin/src/android/src/main/java/com/mparticle/cordova/MParticleCordovaPlugin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look good, thanks.
Hey @BrandonStalnaker , it's been a while since I've done anything with this project, but don't we need to update the |
@willpassidomo You would be correct So it would be really helpful if I had included those changes in the commit initially... oops. Added them now thanks for noticing that. |
Hahaha, that's happened to me more time than I would want to admit, checking it out now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Read through my comments, there are a couple things I pointed out in there that might be worth fixing now or not, depending on the urgency of getting this PR out :)
@@ -326,6 +338,56 @@ public void getUserIdentities(final JSONArray args, final CallbackContext callba | |||
callbackContext.sendPluginResult(pluginResult); | |||
} | |||
|
|||
public void addGDPRConsentState(final JSONArray args) throws JSONException { | |||
MParticleUser user = MParticle.getInstance().Identity().getCurrentUser(); | |||
final JSONObject map = args.getJSONObject(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I'm not going to block the PR because it looks like we already do this in a bunch of other methods, but the JSONArray.getJSONObject(int)
method (or any JSONObject/JSONArray get{type}()
method) will throw an exception if the value is not a JSONObject
(or {type}
), meaning our null-check on this value won't every help us.
The most defensive way to handle this parsing would be like:
JSONObject map = null;
if (args.length > 0) {
map = args.optJSONObject(0)
}
String purpose = null;
if (args.length > 1) {
purpose = args.optString(1)
}
if (user != null && map != null && purpose != null) {
//do the stuff
}
Since your implementation is consistent with how we do it elsewhere in this file, it'd be fine to get this out and fast-follow with a broader update
@@ -698,5 +760,35 @@ private static String ConvertPromotionActionType(int promotionActionType) { | |||
return Promotion.CLICK; | |||
} | |||
} | |||
|
|||
private static GDPRConsent ConvertGDPRConsent(JSONObject map) throws JSONException { | |||
String dateStr = obj.getString("timestamp"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, using obj.optString("timestamp")
then null-checking will allow us to gracefully handle malformed input without crashing..right now this will generate a JSONException if the user leaves out the "timestamp" value
@@ -375,6 +391,52 @@ var mparticle = { | |||
'modify', | |||
[IdentityRequest]) | |||
} | |||
}, | |||
|
|||
GDPRConsent: function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since these are identical objects, we could instead just merge them into a single Consent
class and use that on both add methods
We're going to spin out Will's add-on suggestions to another PR. |
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
6 similar comments
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [2.1.0](2.0.6...2.1.0) (2022-05-06) ### Features * Add GDPR and CCPA Consent ([#13](#13)) ([60368be](60368be))
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [2.1.0](2.0.6...2.1.0) (2022-05-06) ### Features * Add GDPR and CCPA Consent ([#13](#13)) ([60368be](60368be))
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Update Cordova SDK to support consent state. Thankfully ATTStatus was already added