-
Notifications
You must be signed in to change notification settings - Fork 985
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
[#21476] Rework login and logout flow to always use biometrics #21628
[#21476] Rework login and logout flow to always use biometrics #21628
Conversation
Jenkins BuildsClick to see older builds (36)
|
(rf/reg-event-fx | ||
:profile.logout/logout | ||
(fn [_] | ||
;; We need to disable notifications before starting the logout process |
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.
Inherited comment from the previous implementation. I tested the app without the dispatch-later
and everything seemed to work. But I decided to preserve the behaviour.
@flexsurfer It'd be great if you are able to provide some context on this comment.
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'm afraid i can't help. might be @Parveshdhull could help
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.
thanks, left a few comments
(rf/reg-event-fx | ||
:profile.logout/logout | ||
(fn [_] | ||
;; We need to disable notifications before starting the logout process |
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'm afraid i can't help. might be @Parveshdhull could help
25% of end-end tests have passed
Failed tests (5)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Hi @yevh-berdnyk, could you take a look at the E2E tests in this PR? The flow has changed, which seems to have broken our E2E runs. |
unassigning myself from this PR. Face id has broken on my ios :( |
25% of end-end tests have passed
Failed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
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.
LGTM
9c44fd6
to
7be1be9
Compare
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.
Switching profiles with biometrics is much better now with this PR.
But I did notice one problem and I'm sharing the iOS screencasts @ulisesmac. The first video is 2.31 from the production build, the second video is the PR's build. In the PR build I sometimes see a loading spinner after logout, something I've never seen before after logout. When the spinner doesn't appear, I also get the impression the initial animation showing the list of profiles is clunky, unlike what's in the production build.
Prod build
https://github.com/user-attachments/assets/f9e02fdf-49ec-408f-8807-f6a54fd5bb79
PR build
https://github.com/user-attachments/assets/23fec410-55d8-4488-b9f0-2b68d9bbfc91
50% of end-end tests have passed
Failed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
70f7f64
to
ca3973b
Compare
75% of end-end tests have passed
Failed tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
|
ca3973b
to
b3da606
Compare
88% of end-end tests have passed
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
@ulisesmac Thank you for your PR. The PR is ready to be merged, but this comment is still unresolved. I need to understand whether a re-test will be required or not. Will there be any changes to this? |
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.
Looks good, just left some small comments 👍
@@ -183,22 +182,21 @@ | |||
:render-fn profile-card}]])) | |||
|
|||
(defn password-input | |||
[processing error] | |||
(let [auth-method (rf/sub [:auth-method]) |
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.
why lift the auth-method
sub to the parent if it's only used in this component?
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.
Hi @clauxx, thanks for noticing.
The behaviour is a bit different, this subscription has been replaced by another one that calculates the auth-method
for all the profiles instead of calculating it for the pressed profile. this change let us have a better UX when the user switches profiles.
It comes from the parent because there the auth-method
is stored in the profile.
LMK if you have another question!
b3da606
to
04b10bf
Compare
I'm investigating this issue now. Thank you so much @ilmotta for paying attention to this detail. I'll check how to improve it, btw on Android the logout process is faster. I'll post a follow up comment with the results. |
a263581
to
6080cef
Compare
The implementation has been fixed and the comments solved, here's the final result of logging out. iOS: Screen.Recording.2024-11-19.at.2.16.56.p.m.movAndroid: video_2024-11-19_14-25-32.mp4 |
@ulisesmac Thank you for your fix! |
- Move logout logic to a new namespace, the implementation no longer uses `rf/defn`
037af86
to
368bfbe
Compare
fixes #21476
Summary
This PR implements the new flow defined in figma
Important changes
Moved the ns
legacy.status-im.multiaccounts.logout.core
tostatus-im.contexts.profile.logout.events
and renamed its events consistently and analogous to our login events, additionally, it doesn't userf/defn
since it's deprecated.This PR uses
dispatch-sync
to properly trigger updates on the react state when a user presses a profile to log-in. Here are videos showing the difference, look at the profile card:dispatch
:Screen_Recording_20241113_235854_Status.Debug.mp4
dispatch-sync
:Screen_Recording_20241113_235826_Status.Debug.mp4
Additional fixes
To make the review easier, the changes not fully related to the feature had been split into two PRs:
Small screen issues in the Onboarding #21241
in:
[#21476] Part 3 - Extra fixes #21630
Testing notes
Please test the authentication flows, mainly logging in and logging out, but also changing the password and enabling/disabling biometrics on both Android and iOS.
Platforms
status: ready