-
Notifications
You must be signed in to change notification settings - Fork 243
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
Amplify analytics #4
Conversation
...in/kotlin/com/amazonaws/amplify/amplify_analytics_pinpoint/AmplifyAnalyticsPinpointPlugin.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/com/amazonaws/amplify/amplify_analytics_pinpoint/AmplifyAnalyticsPinpointPlugin.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/com/amazonaws/amplify/amplify_analytics_pinpoint/AmplifyAnalyticsPinpointPlugin.kt
Outdated
Show resolved
Hide resolved
packages/amplify_analytics_pinpoint/test/amplify_analytics_pinpoint_test.dart
Show resolved
Hide resolved
packages/amplify_analytics_pinpoint/ios/Classes/SwiftAmplifyAnalyticsPinpointPlugin.swift
Show resolved
Hide resolved
packages/amplify_analytics_pinpoint/ios/Classes/SwiftAmplifyAnalyticsPinpointPlugin.swift
Show resolved
Hide resolved
packages/amplify_analytics_pinpoint/ios/Classes/SwiftAmplifyAnalyticsPinpointPlugin.swift
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.
Looks good overall! See inline comments for suggestions/questions.
Ship it! Just be sure to squash and merge, and try following conventional commit style in the PR title. |
* Initial commit for amplify-analytics. - amplify_analytics_pinpoint : Pinpoint plugin package implementing analytics interface - amplify_analytics_plugin_interface : Analytics interface - various updates to amplify_core and amplify_core_plugin_interface to support analytics.
a27219d
to
b942c72
Compare
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.
Sorry I know I'm late to the party, but just got an opportunity to take a look. There are some minor comments which we can discuss offline, just wanted to leave them here.
.../main/kotlin/com/amazonaws/amplify/amplify_analytics_pinpoint/AmplifyAnalyticsConstructor.kt
Show resolved
Hide resolved
channel = MethodChannel(flutterPluginBinding.getFlutterEngine().getDartExecutor(), "com.amazonaws.amplify/analytics_pinpoint") | ||
channel.setMethodCallHandler(this); | ||
|
||
Amplify.addPlugin(AWSCognitoAuthPlugin()) |
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.
We don't need this here, right?
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.
Yes that's correct. The one issue is that the auth category hasn't been implemented into master branch yet, so if I remove this code, the example app would break.
I guess we can leave this comment here as a note to remove this line once Auth is properly merged into master and can be included as a dependency here?
break | ||
} else { | ||
context = context.applicationContext | ||
Log.e(TAG, "Failed to resolve Application from Context, AWS Pinpoint not initialized") |
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.
- But we haven't failed yet, we are still in while loop, right?
- Is there a guarantee that the while loop will break?
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.
- That's correct, thanks for catching that. I've moved the fail case outside of the while loop.
- I'm no Android expert, but I believe the idea behind context is that it's the encapsulating state of your current location, so at some point you'll reach the root context and have no where else to go.
|
||
@JvmStatic | ||
fun registerWith(registrar: Registrar) { | ||
val channel = MethodChannel(registrar.messenger(), "com.amazonaws.amplify/analytics") |
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.
Wrong channel name. Also we need to confirm if we want to support pre-Flutter-1.12 Android projects officially.
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 I changed the channel name.
Sounds like a nice to have though not too sure on customer impact here. Do you know where I can look to find what steps are needed to support pre-Flutter-1.12 Android projects? I've seen update steps but no explicit information on what we might need to do to officially support this. I think we might need to manually create a pre-Flutter-1.12 project and test to see what might break. Perhaps this is something we should mention in our next team meeting?
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.
Do you know where I can look to find what steps are needed to support pre-Flutter-1.12 Android projects?
No, I reckon that we will just check the usage of pre-Flutter-1.12 Android projects and if minimal, call out explicitly that we support Flutter X versions onward.
...in/kotlin/com/amazonaws/amplify/amplify_analytics_pinpoint/AmplifyAnalyticsPinpointPlugin.kt
Show resolved
Hide resolved
packages/amplify_analytics_plugin_interface/lib/analytics_plugin_interface.dart
Show resolved
Hide resolved
if(plan != null){ | ||
allProperties["plan"] = plan; | ||
} | ||
|
||
|
||
|
||
if (location != null) { | ||
allProperties["location"] = location.getAllProperties(); | ||
} |
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.
There are some extra new lines throughout the code. We should clean it up.
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, removed the extra lines. I think this might be a bigger discussion. I know in Jetbrains Rider there is a way to enforce automatic code styling / spacing. I think we should look into this and perhaps have a discussion as a team to get that setup.
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.
Yes that would be a life saver and a good early investment.
packages/amplify_core_plugin_interface/lib/amplify_analytics_category.dart
Show resolved
Hide resolved
packages/amplify_core_plugin_interface/lib/amplify_analytics_category.dart
Show resolved
Hide resolved
Before ``` flutter: ERROR | SignUpStateMachine | Emitted error flutter: ERROR | Authenticator | Error in AuthBloc ``` After ``` flutter: ERROR | SignUpStateMachine | Emitted error: InvalidParameterException [Attributes did not conform to the schema: email: The attribute is required, null, null] #0 HttpOperation.deserializeOutput package:smithy/…/http/http_operation.dart:278 <asynchronous suspension> #1 AWSRetryer.retry package:smithy_aws/…/retry/aws_retryer.dart:160 <asynchronous suspension> #2 WrappedCognitoIdentityProviderClient.signUp package:amplify_auth_cognito_dart/…/sdk/sdk_bridge.dart:516 <asynchronous suspension> #3 SignUpStateMachine.onInitiate package:amplify_auth_cognito_dart/…/machines/sign_up_state_machine.dart:42 <asynchronous suspension> #4 SignUpStateMachineBase.resolve package:amplify_auth_cognito_dart/…/generated/sign_up_state_machine_base.dart:39 <asynchronous suspension> #5 StateMachine._listenForEvents package:amplify_core/…/state_machine/state_machine.dart:171 <asynchronous suspension> flutter: ERROR | Authenticator | Error in AuthBloc: InvalidParameterException [Attributes did not conform to the schema: email: The attribute is required, null, null] ```
* fix: Proper enum fixes * chore: Update goldens * Add test * Remove orElse from enum helpers * Update goldens
* fix: Proper enum fixes * chore: Update goldens * Add test * Remove orElse from enum helpers * Update goldens
* fix: Proper enum fixes * chore: Update goldens * Add test * Remove orElse from enum helpers * Update goldens
Update Log Metric TEMP Ensure PR Trigger try fix fix formatting issues fix error 3 fix ending ) Pass github token fix syntax error remove smart quote BS Ensure Run temp update 2 temp Hopefully this fixes it Let’s try again Fix Attempt #4 fix failing step pipe fix improper job status fix dart script namings #7 fix freaking typo #8 fix mappings again #9 Print for Debug #10 Remove Quotes? aws-amplify#11 fixi failing trigger debug - print dart outputs fix syntax error try to get output force github actions to run fix syntax stop integ tests trynig to get output
Description of changes:
This PR includes '''amplify_analytics_plugin_interface"' and '''amplify_analytics_pinpoint''' plugin as well as minor changes needed in code.
The plugin implements all analytics methods present in the equivalent ios and android plugin.
Equivalent DART data structures were created to match ones present for ios and android.
The amplify_analytics_pinpoint folder's example app contains a basic DART app integrating the plugin with several UI buttons for calling each of the analytics methods to verify the API is working. You will need to change amplifyconfiguration.dart to point to your own AWS Pinpoint setup to properly receive those events.
Remaining points to work on:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.