Skip to content
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

Move session verification to FTUE flow, make it mandatory #2594

Merged

Conversation

jmartinesp
Copy link
Contributor

@jmartinesp jmartinesp commented Mar 22, 2024

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Moves the session verification flow to be the first step of the FTUE flow.
  • Makes it mandatory.
  • Removes session verification banner.
  • Implements new designs from Figma.
  • Fixes bug with verification cancellation.

Motivation and context

Closes #2580.

Screenshots / GIFs

verification-in-ftue.webm

Tests

  • Do a fresh log in.
  • The FTUE flow should now start with the session verification. It shouldn't be skippable.

Tested devices

  • Physical
  • Emulator
  • OS version(s): 14

Checklist

@jmartinesp jmartinesp force-pushed the misc/jme/2580-move-session-verification-to-ftue-flow branch from 9b69e62 to 6dac601 Compare April 2, 2024 10:19
Copy link
Contributor

github-actions bot commented Apr 2, 2024

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/CNTnxv

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 84.66667% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 73.37%. Comparing base (f4076dc) to head (525eb67).
Report is 6 commits behind head on develop.

Files Patch % Lines
...atures/verifysession/impl/VerifySelfSessionView.kt 85.18% 3 Missing and 9 partials ⚠️
...erifysession/impl/VerifySelfSessionStateMachine.kt 88.46% 0 Missing and 3 partials ⚠️
...atures/preferences/impl/tasks/ClearCacheUseCase.kt 0.00% 2 Missing ⚠️
...securebackup/impl/DefaultSecureBackupEntryPoint.kt 0.00% 2 Missing ⚠️
...oid/features/ftue/impl/state/DefaultFtueService.kt 94.11% 0 Missing and 1 partial ⚠️
...impl/timeline/components/TimelineItemVirtualRow.kt 0.00% 1 Missing ⚠️
...s/verifysession/impl/VerifySelfSessionPresenter.kt 66.66% 0 Missing and 1 partial ⚠️
...rix/api/verification/SessionVerificationService.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2594      +/-   ##
===========================================
- Coverage    73.37%   73.37%   -0.01%     
===========================================
  Files         1459     1458       -1     
  Lines        35177    35151      -26     
  Branches      6752     6746       -6     
===========================================
- Hits         25812    25792      -20     
+ Misses        5851     5848       -3     
+ Partials      3514     3511       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmartinesp jmartinesp force-pushed the misc/jme/2580-move-session-verification-to-ftue-flow branch from 67fc5ef to 3c9bf50 Compare April 2, 2024 11:25
@jmartinesp jmartinesp marked this pull request as ready for review April 2, 2024 12:13
@jmartinesp jmartinesp requested a review from a team as a code owner April 2, 2024 12:13
@jmartinesp jmartinesp requested review from ganfra and removed request for a team April 2, 2024 12:13
Copy link
Contributor

@ganfra ganfra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, I just have some remarks.

val pushProvider = pushService.getAvailablePushProviders().firstOrNull() ?: return@LaunchedEffect
val distributor = pushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect
pushService.registerWith(matrixClient, pushProvider, distributor)
val verifiedStatus by sessionVerificationService.sessionVerifiedStatus.collectAsState()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better like this?

val isVerified by remember {
            verificationService.sessionVerifiedStatus.map { it == SessionVerifiedStatus.Verified }
        }.collectAsState(initial = false)
        LaunchedEffect(isVerified) {

@@ -398,20 +393,23 @@ class LoggedInFlowNode @AssistedInject constructor(
}
}

suspend fun attachRoot(): Node {
return attachChild {
suspend fun attachRoot() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should rename Root to RoomList

// Attach the root node as soon as we know the first session verification status and the FTUE shouldn't be displayed.
// This prevents the room list from being displayed while the session is not verified.
combine(
sessionVerificationService.sessionVerifiedStatus,
Copy link
Contributor

@ganfra ganfra Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit strange to do that. I'd let the FtueState exposes a Loading state instead.
(maybe we should rename FtueState to be FtueService and have a proper FtueState because it's a bit misleading currently).

You could replace everything by something like this afterwards

        ftueFlow
            .onEach { ftueState ->
                when(ftueState){
                    FtueState.Loading -> Unit // don't do anything, Placeholder is the default node. If app is killed we don't want to loose our current navigation state.
                    is FtueState.Loaded -> {
                        if(ftueState.isCompleted){
                            backstack.safeRoot(NavTarget.RoomList)
                        } else {
                            backstack.safeRoot(NavTarget.Ftue)
                        }
                    }
                }
            }

- Renamed `FtueState` to `FtueService`, created an actual `FtueState`.
- Minor improvements.
@jmartinesp
Copy link
Contributor Author

Thanks for the review! I believe ad4ffb2 should fix all those issues.

@jmartinesp jmartinesp requested a review from ganfra April 3, 2024 08:43
Copy link
Contributor

@ganfra ganfra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, LGTM!

@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 3, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 3, 2024
@jmartinesp jmartinesp enabled auto-merge (squash) April 3, 2024 09:13
@jmartinesp jmartinesp force-pushed the misc/jme/2580-move-session-verification-to-ftue-flow branch from 116ad5a to 48670e1 Compare April 3, 2024 12:33
@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 3, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 3, 2024
@jmartinesp jmartinesp force-pushed the misc/jme/2580-move-session-verification-to-ftue-flow branch from 48670e1 to 525eb67 Compare April 3, 2024 14:25
@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 3, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 3, 2024
Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jmartinesp jmartinesp merged commit 41287c5 into develop Apr 3, 2024
19 checks passed
@jmartinesp jmartinesp deleted the misc/jme/2580-move-session-verification-to-ftue-flow branch April 3, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] FTUE - Move session verification to the login flow
3 participants