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
Make several logging improvements for FC #8380
Conversation
Diffuse output:
APK
|
e925f37
to
ced3750
Compare
@@ -67,7 +67,7 @@ internal class ErrorViewModel @AssistedInject constructor( | |||
ErrorState::payload, | |||
onFail = { error -> | |||
eventTracker.logError( | |||
extraMessage = "Error linking more accounts", | |||
extraMessage = "Error loading the error screen payload", |
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.
🥴
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.
looking good! left a suggestion
private fun computeSessionCompletionStatus( | ||
session: FinancialConnectionsSession, | ||
earlyTerminationCause: EarlyTerminationCause?, | ||
closeAuthFlowError: Throwable?, | ||
): String { | ||
return earlyTerminationCause?.analyticsValue ?: session.completionStatus(closeAuthFlowError) | ||
} |
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.
(opinionated, feel free to ignore) - to prevent the viewmodel from growing on relatively complex pieces of logic, we can move this to a ComputeSessionCompletionStatus
usecase and add some docs / tests for it : )
- Fix `extraMessage` in `ErrorViewModel` - Report `pane` in various events - Add `status` to complete event
- Move computation of completion status to use case - Add unit tests for use case
ced3750
to
9adcd54
Compare
Summary
This pull request makes a few improvements to our analytics:
extraMessage
inErrorViewModel
pane
to various events that were missing it (the pane could usually be derived from the event, but it makes the analytics harder to follow)status
field to the complete event. Same logic as iOS and easier to query(cc @kgaidis-stripe for the iOS angle)
Motivation
Testing
Screenshots
Changelog