-
Notifications
You must be signed in to change notification settings - Fork 160
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
Session falsely displayed as 'verified' with no internet connection #2884
Session falsely displayed as 'verified' with no internet connection #2884
Conversation
Remove the need to wait for `isReady` for `SessionVerificationService.canVerifySessionFlow` to fix this.
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2884 +/- ##
===========================================
- Coverage 74.30% 74.30% -0.01%
===========================================
Files 1530 1530
Lines 36524 36521 -3
Branches 7054 7054
===========================================
- Hits 27140 27137 -3
Misses 5698 5698
Partials 3686 3686 ☔ View full report in Codecov by Sentry. |
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.
OK, but as you said, it's hard to see if they can have some side effects...
@@ -39,7 +39,7 @@ interface SessionVerificationService { | |||
val sessionVerifiedStatus: StateFlow<SessionVerifiedStatus> | |||
|
|||
/** | |||
* Returns whether the current session needs to be verified and the SDK is ready to start the verification. | |||
* Returns whether the current session needs to be verified. |
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.
Maybe take the opportunity to rename canVerifySessionFlow
to mustVerifySessionFlow
or needToVerifySessionFlow
for clarity?
Also SessionVerificationService.isReady
is not used externally, so maybe remove it from the interface?
override val canVerifySessionFlow = combine(sessionVerifiedStatus, isReady) { verificationStatus, isReady -> | ||
isReady && verificationStatus == SessionVerifiedStatus.NotVerified | ||
override val canVerifySessionFlow = sessionVerifiedStatus.map { verificationStatus -> | ||
verificationStatus == SessionVerifiedStatus.NotVerified |
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 understand that if the verificationStatus
get the value SessionVerifiedStatus.NotVerified
, it means that the sliding sync is ready, but I am not 100% sure.
- Rename `SessionVerificationService.canVerifySessionFlow` to `needsSessionVerification`. - Make `isReady` private.
Quality Gate passedIssues Measures |
Remove the need to wait for
isReady
forSessionVerificationService.canVerifySessionFlow
to fix this.Type of change
Content
Motivation and context
When no internet connection was found or the sliding sync process wasn't running, sessions were falsely being displayed in UI as verified although they were unverified.
Screenshots / GIFs
auto-verified.mp4
Tests
I couldn't find any side effects, but it might be worth it double checking those.
Tested devices
Checklist