Shopper insights button analytics -- add new params #1187
Conversation
…into shopper-insights-button-analytics
...perInsights/src/main/java/com/braintreepayments/api/shopperinsights/ShopperInsightsClient.kt
Show resolved
Hide resolved
...perInsights/src/main/java/com/braintreepayments/api/shopperinsights/ShopperInsightsClient.kt
Outdated
Show resolved
Hide resolved
...perInsights/src/main/java/com/braintreepayments/api/shopperinsights/ShopperInsightsClient.kt
Outdated
Show resolved
Hide resolved
| val experiment: String? = null, | ||
| val buttonRank: Int? = null |
There was a problem hiding this comment.
Curious - was there an issue with using delegation or inheritance to not have the base analytics class have module specific properties?
also is there a good way we can document these properties even though it's internal to the SDK?
There was a problem hiding this comment.
was there an issue with using delegation or inheritance to not have the base analytics class have module specific properties?
No, I didn't want to do that as part of this ticket. After looking at the way it is setup, I think grouping the events into modules within this AnalyticsEvent class should be sufficient. We use putOpt and only add the relevant parameters to the events, so there isn't extra bloat.
IMO The delegation pattern example you've shared would probably be overly verbose for what we are doing here. If there's a cleaner way and more concise way, I'd prefer that.
There was a problem hiding this comment.
also is there a good way we can document these properties even though it's internal to the SDK?
I'd think we'd want to break the properties logically into module based properties. Then it'd probably be easier to document them.
Let me address these issues one by one.
Good callouts!
There was a problem hiding this comment.
Actually, some of these events are shared by multiple modules. So, there isn't necessarily a proper separation. This is probably the best we'll do on this.
For the documentation though, buttonRank could be module specific, but experiment, though currently limited to one module, could be anything and could be used by any module. Not sure what the best course of action is. I'll probably just keep it as is.
|
@tdchow I don't want to hold up this PR for the refactor since works and that'd would be considered an improvement. It'll give me time to try a couple different options to figure out which works best. Let me know if you are ok with me moving the refactor to a separate PR. |
I think we can refactor later down the line when the number of analytic params increases. No need to hold up your PR or even continue with the refactor in a follow-up. However, we still might want to document the new properties somehow. My only worry is that it'll add confusion for new folks who join the team who do not have context on this work. |
BraintreeCore/src/main/java/com/braintreepayments/api/core/AnalyticsEvent.kt
Outdated
Show resolved
Hide resolved
| @@ -2,12 +2,30 @@ package com.braintreepayments.api.core | |||
|
|
|||
| import androidx.annotation.RestrictTo | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
@tdchow does this look good?
Summary of changes
Checklist
Authors