Biometrics button on login screen #18106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (37)
|
flexsurfer
requested changes
Dec 7, 2023
| @@ -180,18 +180,22 @@ | |||
| {:keychain/get-auth-method [key-uid | |||
| #(rf/dispatch [:profile.login/get-auth-method-success % key-uid])]}) | |||
|
|
|||
| (rf/reg-event-fx | |||
There was a problem hiding this comment.
could you elaborate? this looks strange
There was a problem hiding this comment.
When logging in, we need :biometric/authenticate with the same success/fail callbacks in two different places, so the dispatch was moved in a separate event so it can be re-used. Removed it in favor of using :biometric/authenticate directly in the view.
| (rf/reg-event-fx | ||
| :profile/switch-profile | ||
| (fn [{:keys [db]} [key-uid]] | ||
| {:db db |
| :profile/switch-profile | ||
| (fn [{:keys [db]} [key-uid]] | ||
| {:db db | ||
| :fx [[:dispatch [:profile/profile-selected key-uid]] |
There was a problem hiding this comment.
same here, its better just dispatch in the view no need in the :profile/switch-profile event
J-Son89
reviewed
Dec 7, 2023
| @@ -29,19 +30,30 @@ | |||
| (defn- view-internal | |||
| [{:keys [default-password theme shell?]}] | |||
| (let [{:keys [error processing]} (rf/sub [:profile/login]) | |||
| auth-method (rf/sub [:auth-method]) | |||
There was a problem hiding this comment.
In my pr I was passing this in as a prop 🤔 No strong preferences, we could keep this input purer however and pass everything in as a prop 🤔 https://github.com/status-im/status-mobile/pull/17992/files
There was a problem hiding this comment.
I'd say unless it's used higher in the tree, mught be better to use the sub directly where it's used. I see very often 'subscription' maps passed as props all over the place, even when a sub is needed in one single child somewhere down the tree. What do you think?
There was a problem hiding this comment.
For me it's fine either way 👍
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 11, 2023
| @@ -49,6 +49,7 @@ | |||
| {:container-style style/auth-button | |||
| :on-press on-press-biometrics | |||
| :icon-only? true | |||
| :background :blur | |||
There was a problem hiding this comment.
this should probably be passed as a prop and defaulted to false. wdyt? I think in some flows this could be incorrect 🤔
There was a problem hiding this comment.
The text input has :blur? enabled as well. Do we have cases where the input is used without the blur?
There was a problem hiding this comment.
here is the designs for syncing auth - https://www.figma.com/file/V6nlpAWIf2e1XU8RJ9yQPe/Syncing-for-Mobile?type=design&node-id=1112-146720&mode=design&t=ioE4htigtApMxrtn-4
which is in blur and in this it has it.
in wallet it is not using blur
J-Son89
reviewed
Dec 11, 2023
| {:shell? true | ||
| :default-password password}]] | ||
| {:shell? true | ||
| :on-press-biometrics (fn [] |
There was a problem hiding this comment.
is this correct? Have you handled the case where biometrics fails -> password input shows, then you press biometric authenticate again and let it fail -> password input shows then you press biometric authenticate again ... (to infinity :) )
There was a problem hiding this comment.
Not sure I understand the flow 😄
There was a problem hiding this comment.
I might be wrong let me test out your branch 😀
flexsurfer
approved these changes
Dec 11, 2023
| @@ -0,0 +1,6 @@ | |||
| (ns status-im2.common.biometric.constants) | |||
There was a problem hiding this comment.
we have status-im2.constants for all constants
siddarthkay
approved these changes
Dec 11, 2023
78% of end-end tests have passedFailed tests (4)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (38)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
@clauxx please, rebase your PR to current develop, thanks! |
|
@churik sorry I found some issues in this PR. Removing it from QA for now. |
|
@churik fixed the issues and aligned with develop. It's moved to E2E now. |
55% of end-end tests have passedFailed tests (14)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (27)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
73% of end-end tests have passedFailed tests (7)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (6)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (35)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
|
|
@clauxx Thank you for your work! e2e failures are not related to particular PR. @Francesca-G
|
clauxx
force-pushed
the
17996-cl-biometry-login
branch
from
92425d6 to
e7ecf7d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17996
Summary
When logging in, there should be a biometry button (if enabled) next to the password input. Also, when switching to a profile with enabled biometry, the biometric check will be triggered automatically as shown in the Figma flow.
Platforms
Areas that maybe impacted
Functional
status: ready