-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(deriv_auth): add log in user tracking events. #620
feat(deriv_auth): add log in user tracking events. #620
Conversation
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.
Modify the tests
import 'package:deriv_auth/deriv_auth.dart'; | ||
import 'package:deriv_auth/core/analytics/data/auth_tracking_repository.dart'; | ||
|
||
/// |
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.
add a comment
default: | ||
break; |
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.
is this needed?
switch (socialAuthProvider) { | ||
case SocialAuthProvider.google: | ||
trackLoginWithGoogle(); | ||
break; |
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.
breaks can be removed
BaseAuthService({ | ||
required this.authRepository, | ||
required this.jwtService, | ||
required this.connectionInfo, | ||
required this.tokenService, |
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.
i believe this change would break the example app in this package
@@ -1,4 +1,7 @@ | |||
import 'package:analytics/sdk/rudderstack/sdk/deriv_rudderstack_sdk.dart'; |
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.
I don't see analytics package added to pubspec.yaml
|
||
/// | ||
mixin AuthTrackingMixin { | ||
final AuthTrackingRepository _repository = AuthTrackingRepository.instance; |
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.
please make this late. Otherwise it will be executed always before the repository's init function.
@@ -136,6 +151,8 @@ class DerivAuthCubit extends Cubit<DerivAuthState> implements DerivAuthIO { | |||
await authService.getLandingCompany(authorizeEntity.country); | |||
_isUserMigrated = _checkUserMigrated(authorizeEntity); | |||
|
|||
trackLoginFinished(); |
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.
can we add this to _loginRequest() as well?
Clickup link:
Fixes issue: #
This PR contains the following changes:
Developers Note (Optional)
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)