-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(firebase_analytics): support complex data structures like list and map on Android #4394
fix(firebase_analytics): support complex data structures like list and map on Android #4394
Conversation
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I fixed it. |
Sorry for the bump, but this seems critical to me and I was about to make a similar PR |
When can we expect this to be merged? We really need this fix for our application |
Hey @lColinDl, it would be great if you could have add a test to confirm an array and map type works here for us to consider this. Something like: await FirebaseAnalytics().logEvent(
name: 'test_complex_data',
parameters: <String, dynamic>{
'items': []// complex types here
},
)
I wasn't able to log complex data types using your changes in my own preliminary investigation. It was an invalid type error from the native side. |
Agreed with Russell, we'll want tests. I also believe we'll want iOS and web support too. |
iOS worked out of the box and if I look at the code for web it should also work. It is just Android that was not working. I wrote the test for the change now in the same way as the existing tests. Hope its fine now. I'm not sure why it's not working for you @russellwheatley and what exactly you tested, but the implementation works for me for all enhanced ecommerce events that I had to implement by now. If there is a specific case that is not working, please specify it for me, so I can test and adjust the code. |
@@ -30,5 +30,23 @@ void main() { | |||
expect( | |||
FirebaseAnalytics().setCurrentScreen(screenName: 'testing'), completes); | |||
expect(FirebaseAnalytics().logEvent(name: 'testing'), completes); |
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.
Could you add a test for maps inside maps?
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.
great stuff so far, @lColinDl 🎉 . I agree with @rrousselGit, we should probably test a nested map as well 👍
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.
Adjusted the tests. Now there is an Array
containing a Map
containing a Map
. Hope that is enough :)
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!
Could you also add a test for the throw IllegalArgumentException
?
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.
Done
Looks like there's a failing Web test, could you take a look please? Thanks? |
packages/firebase_analytics/firebase_analytics/example/test_driver/firebase_analytics_e2e.dart
Outdated
Show resolved
Hide resolved
…iver/firebase_analytics_e2e.dart
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.
LGTM
Description
The native Android implementation of the firebase_analytics plugin does not support passing complex data structures (e.g. lists) as parameters into the
logEvent
method. This is for example necessary for ecommerce events like "view_item_list".The native plugin implementation for iOS supports this already.
This PR is based on this already open, but outdated PR (#1458). I don't care which PR gets merged, but as this is blocking the usage of ecommerce events, this is really important for me.
Related Issues
This PR fixes:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?