-
Notifications
You must be signed in to change notification settings - Fork 29
GH-62: adding jwt-enabled API keys #67
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
38e446d
20fcb9a
d8b4de1
f3f5bfd
30538f4
45eacc4
027c715
a77b3e3
73a3139
08f326b
7997a8c
aeb9dd7
49c999a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,5 @@ | ||
| example/ios/Podfile.lock | ||
| example/pubspec.lock | ||
| example/pubspec.lock | ||
| .dart_tool/ | ||
| build/ | ||
| pubspec.lock |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,9 +52,9 @@ class IterableFlutterPlugin : FlutterPlugin, MethodCallHandler { | |
| result.success(null) | ||
| } | ||
| "setEmail" -> { | ||
| val userEmail = call.arguments as String | ||
| IterableApi.getInstance().setEmail(userEmail) | ||
| IterableApi.getInstance().registerForPush() | ||
| val email = call.argument<String>("email") ?: "" | ||
| val jwt = call.argument<String>("jwt") ?: "" | ||
| IterableApi.getInstance().setEmail(email, jwt) | ||
| result.success(null) | ||
| } | ||
| "setUserId" -> { | ||
|
|
@@ -107,11 +107,10 @@ class IterableFlutterPlugin : FlutterPlugin, MethodCallHandler { | |
| notifyPushNotificationOpened() | ||
| false | ||
| } | ||
|
|
||
| if (activeLogDebug) { | ||
| configBuilder.setLogLevel(Log.DEBUG) | ||
| configBuilder.setLogLevel(Log.VERBOSE) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably not necessary to include in the PR and probably makes sense to change locally when you need to check logs for something. |
||
| } | ||
|
|
||
| IterableApi.initialize(context, apiKey, configBuilder.build()) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,14 @@ class IterableFlutter { | |
| _channel.setMethodCallHandler(nativeMethodCallHandler); | ||
| } | ||
|
|
||
| static Future<void> setEmail(String email) async { | ||
| await _channel.invokeMethod('setEmail', email); | ||
| static Future<void> setEmail(String email, String jwt) async { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make jwt nullable in order to communicate that it isn't strictly required. Some implementations either don't care about having a JWT, or just want to get something quick put together to ensure the package works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No support for setUserId? Seems easy enough to add. |
||
| await _channel.invokeMethod( | ||
| 'setEmail', | ||
| { | ||
| 'email': email, | ||
| 'jwt': jwt, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| static Future<void> setUserId(String userId) async { | ||
|
|
||
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.
I think PRs should be scoped specifically to what they're trying to do - support JWT in this case. Is is necessary to change this in order to do that? Maybe consider creating another PR for whatever you need these changes for.
Imagine if something in these new versions broke and needed to revert this PR - now we lose JWTs! Or have to spend extra time cherrypicking exactly what we need to revert.