Skip to content

fix: update Android logEvent to support parameters of type List<Map<String, dynamic>> to match iOS#1458

Closed
tp wants to merge 7 commits into
firebase:masterfrom
tp:master
Closed

fix: update Android logEvent to support parameters of type List<Map<String, dynamic>> to match iOS#1458
tp wants to merge 7 commits into
firebase:masterfrom
tp:master

Conversation

@tp

@tp tp commented Nov 19, 2019

Copy link
Copy Markdown

Description

The logEvent params on Android so far only support primitive values (numbers, Strings, boolean).

This change adapts the Java implementation to also support sending Lists with Map<String, dynamic> from the Flutter side.

This worked for us already on iOS, so we just needed this adaptation for Android to reach feature parity.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@tp

tp commented Nov 19, 2019

Copy link
Copy Markdown
Author

Still have to add tests for this PR.

Just wanted to post this early, so I could get some feedback if anyone sees any issue with this approach. Works fine for us in production.

@tp

tp commented Nov 19, 2019

Copy link
Copy Markdown
Author

@googlebot I signed it!

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@tp

tp commented Nov 20, 2019

Copy link
Copy Markdown
Author

My PR includes unit or integration tests for all changed/updated/fixed behaviors

Sadly there wasn't a test of the Java implementation so far, and I don't know yet how to best add one. Any pointers would be appreciated.

@tp tp marked this pull request as ready for review November 20, 2019 16:14
@tp

tp commented Nov 20, 2019

Copy link
Copy Markdown
Author

Not sure what happened to build-ipas+drive-examples. As this didn't change anything on iOS it should pass just like on master before.

@tp

tp commented Nov 27, 2019

Copy link
Copy Markdown
Author

@collinjackson Can you have a look here please if what we're doing is OK?

We've been running in production with this and it's working fine for us, not the lists are available via Google Tag Manager just like on iOS.

@alfietham

alfietham commented Jul 28, 2020

Copy link
Copy Markdown

I ran into this exact need, was about to create a PR but glad to find there's already one open!

Any ideas what else might be holding this up @tp ? Any pointers @kroikie ? Happy to help where I can

Btw, I've cloned this fork and rebased onto today's upstream/master and checked the diffs. Only conflicts to address are versioning-related in the CHANGELOG.md and pubspec.yaml's version number. I can PR to your fork if you'd like @tp.

Also, this fixes: #1036, #2835

@GursheeshSingh

GursheeshSingh commented Aug 19, 2020

Copy link
Copy Markdown

Would this throw any error if Value is too long?

I tried using the custom event with string and it throws an error for lists too long.

@tp tp changed the title [firebase_analytics] Support lists with maps fix: [firebase_analytics] Support lists with maps Mar 22, 2021
@tp

tp commented Mar 22, 2021

Copy link
Copy Markdown
Author

Updated to the latest release version.

Now there is "only" an issue with iOS builds, but that also affects other PRs here and seems unrelated to my changes.

@collinjackson @kroikie Could you help our here moving this forward?

@Salakar Salakar changed the title fix: [firebase_analytics] Support lists with maps fix: update Android logEvent to support parameters of type List<Map<String, dynamic>> to match iOS Apr 1, 2021
@Salakar

Salakar commented Apr 1, 2021

Copy link
Copy Markdown
Contributor

Closing this in favour of #4394 which is slightly further along in the review process and has tests. Thanks for sending this up though.

@Salakar Salakar closed this Apr 1, 2021
@firebase firebase locked and limited conversation to collaborators May 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[firebase_analytics] List type paramters [firebase_analytics] handle nested json in event params

6 participants