Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Fix tracks connected info property naming#26

Merged
malinajirka merged 4 commits intodevelopfrom
merge/woocommerce-android/1287
Aug 7, 2019
Merged

Fix tracks connected info property naming#26
malinajirka merged 4 commits intodevelopfrom
merge/woocommerce-android/1287

Conversation

@AmandaRiu
Copy link
Copy Markdown
Contributor

@AmandaRiu AmandaRiu commented Aug 1, 2019

This PR merges the changes defined in this WCAndroid PR. The changes are minor, I just needed to fix the property names of the _login_connected_site_info_succeeded event so the properties would get properly saved to the event.

This Draft PR can be used for testing these changes in WPAndroid.

AmandaRiu added 3 commits August 1, 2019 16:29
# Conflicts:
#	WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginSiteAddressFragment.java
@malinajirka
Copy link
Copy Markdown
Contributor

Thanks @AmandaRiu! The changes look good. My only concerns is if it's a good idea to rename the properties. In general changing names of events or properties isn't a good idea as it will break already existing funnels/queries. This is especially true when the events are related to login/signup/sitecreation as we have some cross-platform funnels which we regularly check. I'm not saying we shouldn't make this change, I just wanted to raise this point. I want to be 100% this change is safe to do. Wdyt?

@malinajirka
Copy link
Copy Markdown
Contributor

ok, just ignore my comment:D. I forgot you actually checked the previous properties were being completely ignored. I'll double check the subtree and merge this PR ;).

Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

@malinajirka malinajirka merged commit 8b6384c into develop Aug 7, 2019
@malinajirka malinajirka deleted the merge/woocommerce-android/1287 branch August 7, 2019 08:05
@AmandaRiu
Copy link
Copy Markdown
Contributor Author

thanks @malinajirka !! 💃

wzieba pushed a commit that referenced this pull request Oct 15, 2024
…d/1287

Fix tracks connected info property naming
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.

2 participants