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

Onboarding flow coordinator and FTUE changes #2578

Merged
merged 18 commits into from
Mar 21, 2024

Conversation

stefanceriu
Copy link
Member

@stefanceriu stefanceriu commented Mar 15, 2024

This PR introduces the following significant changes:

  • session verification is mandatory for all users, the app shouldn't be usable without a verified session
  • session verification / recovery, the (optional) app pin lock setup, the analytics consent prompt and the push notifications permissions requesting are now handled by the newly introduced OnboardingFlowCoordinator
  • all traces of these flows have been removed from the AuthenticationFlowCoordinator and the main AppCoordinator, and consolidated in the Onboarding

Designs here https://www.figma.com/file/qjs3RKuofeKzBRPnOmLo2a/OIDC-sign-in-with-QR-code?type=design&node-id=19-144144&mode=design&t=7MXZzQhzq8C3h2uv-0

Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-03-21.at.14.09.37.mp4

@stefanceriu stefanceriu requested a review from a team as a code owner March 15, 2024 13:12
@stefanceriu stefanceriu requested review from pixlwave and Velin92 and removed request for a team March 15, 2024 13:12
@stefanceriu stefanceriu force-pushed the stefan/onboardingFlowCoordinatorAndFTUEChanges branch from dbc256f to ae35a65 Compare March 15, 2024 13:38
Copy link

github-actions bot commented Mar 15, 2024

Warnings
⚠️ This pull request seems relatively large. Please consider splitting it into multiple smaller ones.
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.
⚠️ You seem to have made changes to some resource images. Please consider using an SVG or PDF.

Generated by 🚫 Danger Swift against 5d6edc0

@stefanceriu stefanceriu force-pushed the stefan/onboardingFlowCoordinatorAndFTUEChanges branch from 3aadb0c to 10e1ea7 Compare March 18, 2024 12:59
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

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

Project coverage is 75.15%. Comparing base (f9f163f) to head (10caf46).
Report is 1 commits behind head on develop.

❗ Current head 10caf46 differs from pull request most recent head 5d6edc0. Consider uploading reports for the commit 5d6edc0 to get more accurate results

Files Patch % Lines
...s/FlowCoordinators/OnboardingFlowCoordinator.swift 0.00% 265 Missing ⚠️
.../FlowCoordinators/UserSessionFlowCoordinator.swift 0.00% 32 Missing ⚠️
...Screen/IdentityConfirmationScreenCoordinator.swift 0.00% 29 Missing ⚠️
...onScreen/IdentityConfirmationScreenViewModel.swift 55.17% 26 Missing ⚠️
...medScreen/IdentityConfirmedScreenCoordinator.swift 0.00% 25 Missing ⚠️
...een/NotificationPermissionsScreenCoordinator.swift 0.00% 23 Missing ⚠️
ElementX/Sources/Application/AppCoordinator.swift 0.00% 21 Missing ⚠️
...ne/TimelineController/RoomTimelineController.swift 0.00% 14 Missing ⚠️
...mationScreen/View/IdentityConfirmationScreen.swift 81.94% 13 Missing ⚠️
...creen/NotificationPermissionsScreenViewModel.swift 31.57% 13 Missing ⚠️
... and 15 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2578      +/-   ##
===========================================
- Coverage    75.94%   75.15%   -0.79%     
===========================================
  Files          545      566      +21     
  Lines        37693    38806    +1113     
  Branches     17078    16940     -138     
===========================================
+ Hits         28627    29166     +539     
- Misses        9066     9640     +574     
Flag Coverage Δ
unittests 60.51% <38.61%> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Wooo, finally an onboarding flow 👏

@stefanceriu stefanceriu force-pushed the stefan/onboardingFlowCoordinatorAndFTUEChanges branch from 10e1ea7 to 1fcbf5d Compare March 19, 2024 12:43
@stefanceriu stefanceriu requested a review from pixlwave March 20, 2024 16:49
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

This looks great, although I think you're missing the commit with the changes you were showing me to the flow?

@stefanceriu
Copy link
Member Author

although I think you're missing the commit with the changes you were showing me to the flow

Oh that's mega weird, the push failed and I didn't notice 🤔 It's up now.

…the flows

- consolidate app lock in the onboarding flow coordinator
- cleanup the old session verification flows
- allow the recovery key screen to be presented both modally and not
- add bottom background to certain screens
- change hero images to support green styles and fix default colors
- switch new onboarding screns to the FullScreenDialog
- disable interactive dismissal
- update snapshots
- present the onboarding flow as a full screen model and faster
- speed up how soon we show onboarding for identity confirmation based on stored settings
- store the notification permissions onboarding user choice
- push onboarding screens onto the stack instead of having them replace the root
@stefanceriu stefanceriu force-pushed the stefan/onboardingFlowCoordinatorAndFTUEChanges branch from eef7618 to 998d358 Compare March 21, 2024 08:17
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@stefanceriu stefanceriu enabled auto-merge (squash) March 21, 2024 12:00
@stefanceriu stefanceriu disabled auto-merge March 21, 2024 12:00
@stefanceriu stefanceriu merged commit a62c96f into develop Mar 21, 2024
5 checks passed
@stefanceriu stefanceriu deleted the stefan/onboardingFlowCoordinatorAndFTUEChanges branch March 21, 2024 12:01
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.

2 participants