Analytics - Send different values when checkoutAttemptId is not available or it failed to fetch #1812
Conversation
araratthehero
added
the
Chore
araratthehero
changed the title
araratthehero
force-pushed
the
feature/analytics_send_checkout_attempt_id_not_yet_fetched
branch
2 times, most recently
from
17180b0
to
8e5facd
Compare
araratthehero
force-pushed
the
feature/analytics_send_checkout_attempt_id_not_yet_fetched
branch
from
8e5facd
to
23a9e1c
Compare
araratthehero
changed the title
...rc/main/java/com/adyen/checkout/components/core/internal/analytics/CheckoutAttemptIdState.kt
Show resolved
Hide resolved
| }.fold( | ||
| onSuccess = { /* Not necessary */ }, | ||
| onFailure = { throwable -> adyenLog(AdyenLogLevel.WARN, throwable) { "Failed sending analytics events" } }, | ||
| ) | ||
| } | ||
|
|
||
| override fun getCheckoutAttemptId(): String? = checkoutAttemptId | ||
| override fun getCheckoutAttemptIdState() = checkoutAttemptIdState |
There was a problem hiding this comment.
I think the state should be something private to DefaultAnalyticsManager and then this method can stay as it was
There was a problem hiding this comment.
That was my initial implementation, but I changed it, since checkoutAttemptId-not-fetched and fetch-checkoutAttemptId-failed values are not really related to the analytics. They are only necessary to be sent in the /payments call, based on if the checkoutAttemptId is fetched or not. Those two values are not the actual values of the checkoutAttemptId, that is why I separated them, to make sure that the Analytics manager is not aware of them and if they change in the future, they will not change the logic of the analytics manager. They are relevant only for the /payments call.
Do you think there is a better way to handle this and keep them separate from the analytics manager?
There was a problem hiding this comment.
Hmm I see your point, but I think it actually is related to analytics. Currently, the getCheckoutAttemptId method is only used by delegates for the /payments call. The only other place we use it is to make analytics requests, but we don't make analytics requests if we have no attempt id. Making the not fetched and failed values valid attempt ids imo
There was a problem hiding this comment.
I was thinking of exposing a state and whenever necessary we can assign custom values to the checkoutAttemptId, since those are not values which are returned from backend. But I guess it is just thinking about cases which we do not have now.
I can move everything to the analytics manager and later we can make changes if necessary.
There was a problem hiding this comment.
Yes, I think that's a good idea
| import com.adyen.checkout.components.core.internal.analytics.CheckoutAttemptIdState | ||
|
|
||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
| const val CHECKOUT_ATTEMPT_ID_NOT_FETCHED = "checkoutAttemptId-not-fetched" |
There was a problem hiding this comment.
All these fields and methods should be internal or even moved to DefaultAnalyticsManager IMO. These two vals are now used in tests, but I think if you make the change I suggested earlier and hardcode a value in TestAnalyticsManager then tests don't have to know about these
araratthehero
force-pushed
the
feature/analytics_send_checkout_attempt_id_not_yet_fetched
branch
3 times, most recently
from
7f92856
to
0954120
Compare
…eckoutAttemptId state. COAND-982
araratthehero
force-pushed
the
feature/analytics_send_checkout_attempt_id_not_yet_fetched
branch
from
0954120
to
7503f8b
Compare
✅ No public API changes |
|
araratthehero
deleted the
feature/analytics_send_checkout_attempt_id_not_yet_fetched
branch
Description
Send different values, based on if
checkoutAttemptIdis fetched, not fetched or failed to fetch.Checklist
COAND-982