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.

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

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

BrandonStalnaker
Copy link
Collaborator

Summary

Update Cordova SDK to support consent state. Thankfully ATTStatus was already added

Copy link
Contributor

@peterjenkins peterjenkins left a 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/ios/CDVMParticle.m Outdated Show resolved Hide resolved
plugin/src/ios/CDVMParticle.m Outdated Show resolved Hide resolved
Copy link
Contributor

@peterjenkins peterjenkins left a 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.

@willpassidomo
Copy link
Contributor

Hey @BrandonStalnaker , it's been a while since I've done anything with this project, but don't we need to update the www/mparticle.js file in order to actually add the setConsentState() etc APIs?

@BrandonStalnaker
Copy link
Collaborator Author

@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.

@willpassidomo
Copy link
Contributor

Hahaha, that's happened to me more time than I would want to admit, checking it out now :)

Copy link
Contributor

@willpassidomo willpassidomo left a 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);
Copy link
Contributor

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");
Copy link
Contributor

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 () {
Copy link
Contributor

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

@BrandonStalnaker BrandonStalnaker merged commit 60368be into master Apr 6, 2022
@BrandonStalnaker
Copy link
Collaborator Author

We're going to spin out Will's add-on suggestions to another PR.

@BrandonStalnaker BrandonStalnaker deleted the feat/3615-Add-GDPR-And-CCPA-Consent branch April 6, 2022 16:52
@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

6 similar comments
@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request May 6, 2022
# [2.1.0](2.0.6...2.1.0) (2022-05-06)

### Features

* Add GDPR and CCPA Consent ([#13](#13)) ([60368be](60368be))
@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request May 6, 2022
# [2.1.0](2.0.6...2.1.0) (2022-05-06)

### Features

* Add GDPR and CCPA Consent ([#13](#13)) ([60368be](60368be))
@mparticle-automation
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants