-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Autofill code from SMS magic code #15081
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@alex-mechler Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
I have read the CLA Document and I hereby sign the CLA |
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.
@narefyev91 Looking great, thanks!
Can you also link native iOS video to the OP? I have also adjusted the test section a bit so its clear to QA what are the pre-requisites and what to expect.
Additionally do you know if this can work in the Electron Desktop build on mac as well? I know this was not specified in the issue originally, it can be a follow up but it would be great to get this feature in there too
@mountiny for IOS native - it's unfortunately not possible to add any video - I could not run app on real device - to do it - you need to add my device in provision profile and also add myself as developer in apple dev program. I can attach video - how the same field will work on other apps |
Oh that is very good point I have not realized. In that case I think we are fine. |
Reviewer ChecklistThis is tricky to test locally it will be easier to perform tests on staging and changes are not harmful
Screenshots/VideosI have tested this with the build below but I am not being asked for the verification on iOS if I use my phone number (primary login). I think this might still be behind beta so it might be best to test internally. WebMobile Web - ChromeMobile Web - SafariDesktopN/A iOSAndroidN/A |
@mountiny for making it working on Desktop app - i did not really find any correct source that it may work - apple have a lot of restriction to prevent phishing from sms - and for now as i know it's really possible to have autofill for some sensitive data like first name, last name, password - which can be stored in keychain and automatically taken from it |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪
|
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.
Changes look good to me, I think we can QA this on staging easier. Asked internally whats the state of the passwordless beta
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.71-0 🚀
|
@narefyev91 @mountiny to clarify, it sounds like for the internal QA, for now we only want to test:
but not |
@francoisl Thats correct! |
🚀 Deployed to production by https://github.com/francoisl in version: 1.2.71-1 🚀
|
Details
Implemented new props for React Native TextInput
autoComplete
andtextContentType
. Combinations of this props will allow us to auto fetch verification code from SMS.For android we will need to make some modification for SMS payload, all information can be find under proposal link
To generate hash you can use this sh file - command to execute
./sms_retriever_hash_v9.sh --package com.expensify.chat --keystore ./android/app/debug.keystore
(for debug keystorehash = h7MoDsTCzjw
)Fixed Issues
$ #14853
PROPOSAL: 14853(COMMENT)
Tests
Note for Safari Mac, you need to have your phone number linked to Messages on Mac so you get the SMS message on the same device you are testing on.
Offline tests
This feature doesn't change or is impacted by offline mode.
QA Steps
Same as above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
android_web.mp4
Mobile Web - Safari
ios_web.MOV
Desktop
iOS
Android
android.MOV