Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable short verification codes remotely and related UI facelift #16

Merged
merged 7 commits into from
Feb 25, 2021

Conversation

i1skn
Copy link
Contributor

@i1skn i1skn commented Feb 18, 2021

Description

Enable short verification codes remotely and related UI facelift

Tested

iOS & Android both with enabled and disabled feature

Related issues

Backwards compatibility

Yes

Screenshots

Simulator Screen Shot - iPhone 11 - 2021-02-23 at 18 32 49
Simulator Screen Shot - iPhone 11 - 2021-02-23 at 18 32 43
Simulator Screen Shot - iPhone 11 - 2021-02-23 at 18 32 11

Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Looking great! 👍 🚀

A couple of points:

  • can you test on Android too to be sure it doesn't break anything?
  • could you add screenshots so it's easier to review the UI changes?

Platform.OS === 'ios' && numberOfLines
? LINE_HEIGHT * numberOfLines
: undefined,
}}
autoCapitalize="none"
testID={testID}
/>
Copy link
Member

Choose a reason for hiding this comment

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

I think we also want keyboardType to be number-pad when shortVerificationCodesEnabled is true, right?

@i1skn
Copy link
Contributor Author

i1skn commented Feb 23, 2021

@jeanregisser great suggestions!
I've tested it on Android and also changed keyboard to numeric on iOS.
Also have added screenshot to the description!

Comment on lines 136 to 140
Platform.OS === 'android'
? 'visible-password'
: shortVerificationCodesEnabled
? 'number-pad'
: undefined
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want number-pad too on Android?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm slightly worried about the comment above autoCorrect={false}, which starts with // This disables keyboard suggestions on iOS, but unfortunately NOT on Android .... So, I'm hesitant to replacevisible-password with number-pad. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I was fighting to disable autoCorrect on Android.
But here using number-pad should be safe on both iOS and Android when shortVerificationCodesEnabled is true.
And we can keep the existing logic when it's false.

@i1skn i1skn merged commit a9f5354 into main Feb 25, 2021
@ValoraQA
Copy link

ValoraQA commented Mar 23, 2021

Hi @i1skn we have verified this task using latest Android Internal Build V1.12.0(1004294340) & Test flight build V1.12.0(49) and observed the followings.

  • User is able to receive 8 digit verification codes and able to compete the verification flow.

User is able to receive 8 digit verification codes and able to complete the verification flow when user go through below scenarios:

  • Able to receive 8 digits verification codes & able to complete verification flow on-Boarding flow.
  • Able to complete verification flow when user go through restore my account key.
  • Able to complete verification flow when user go through Confirm phone number in-App notification.
  • Able to complete verification flow when user go through settings> Confirm phone number
  • Able to complete verification flow when user go through Earn rewards> Confirm phone number.

Observation: Confirmation error message is shown on confirming screen when user keep the app in background while receiving codes,
Can you please let us know is this expected behavior Or we need to raise an issue.

Please let us know if we need to test anything else in this task.

@i1skn
Copy link
Contributor Author

i1skn commented Mar 23, 2021

@Celoqa Thanks for checking it, can you please open an issue with attached logs/video, so I can check is that an expected behaviour or not. Thanks!

@ValoraQA
Copy link

Sure @i1skn We will raise a new issue with attached logs.

@ValoraQA
Copy link

Hi @i1skn We have raised a new issue with attached logs #146

@i1skn
Copy link
Contributor Author

i1skn commented Mar 23, 2021

Thanks!

@ValoraQA
Copy link

ValoraQA commented Mar 24, 2021

Hi @i1skn Can you please confirm below issue is expected Or not if not we can raise a new issue.

  • Confirmation failed message is shown when user change the phone number after receiving attestation.
  • After receiving codes we are able to see all same alphabets so, can you please confirm it is expected behavior.Attachment
  • Also we have observed text difference in code for iOS & Android. Let me know it is expected behavior. Attachment

Build: Android Internal Build V1.12.0(1004294340) & Test flight build V1.12.0(49)
Devices: iPhone 6+ (12.4.5), Samsung M30(10)
Repro steps:

  • Install the app
  • Go through on-boarding flow
  • Enter phone number
  • Tap on start button & complete captcha
  • Make sure user should receive attestation
  • Go back to Phone number page
  • Change the phone number
  • Tap on resume button
  • Observe
    Attachment:
  • Confirmation Failed.mp4
  • ConfirmationFailedLogs

@silasbw silasbw deleted the i1skn/short-codes-remote-enabling branch May 26, 2021 15:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8-digit attestation code
3 participants