[Wallet] Add resend button to VerificationInputScreen #5134

i1skn · 2020-09-22T16:36:43Z

Description

Remove "I already received three codes" and merge its logic into "Resume" button on the VerificationEducation screen.
Add "Resend N messages" on the VerificationInput screen. Button is only available after a minute since the last reveal. When pressed we will try to reveal already existing attestations first. If HTTP call to attestation-service returns anything, but OK we request another issuer and try to reveal the number to it. Those we increase number of actionable attestations till the limit 5, after which user need to wait until some attestations expire to request more.
Updated text for "Do I need to confirm" dialog in English.

See figma for more details.

Tested

iOS & Android with actual requesting of SMSs

Related issues

Backwards compatibility

Yes

Screenshots

aslawson · 2020-09-22T22:42:01Z

packages/mobile/src/identity/verification.ts

+    : getPhoneHash(e164Number)
+  const account: string = yield call(getConnectedUnlockedAccount)
+
+  const actionableAttestations: ActionableAttestation[] = yield call(


So this appears to only try to re-reveal the already selected issuers (unless I'm missing something in the PR).

We also wanted to request an additional issuer for the case that the SMS could not be received. I think without a "successful response cache" or Tim's new /get_attestations endpoint yet, we could accomplish this by re-revealing to the original issuer, and if it returns an error then request a new issuer. WDYT?

@aslawson Yeap, you got it right!

In this case it's pretty hard to predict if user has sufficient balance in advance. So, after user presses this button and we try to request another issuer with insufficient balance - we would show an error (or should we silently handle this and do nothing if balance is insufficient?). If we show an error, how can we explain to user why suddenly they need more funds?

i1skn · 2020-09-27T12:34:28Z

There is a vulnerability which does not have a fix yet https://www.npmjs.com/advisories/1560/versions and it blocks the build

gnardini

I get a bit lost on the details of packages/mobile/src/identity/verification.ts but it looks good to me.

gnardini · 2020-09-28T13:22:01Z

packages/mobile/src/identity/types.ts

@@ -12,8 +12,7 @@ export enum VerificationStatus {
  Prepping = 1,
  GettingStatus = 2,
  RequestingAttestations = 3,
-  RevealingNumber = 4,
-  RevealAttemptFailed = 5,
+  CompletingAttestations = 4,
  Done = 6,


Maybe this is on purpose and this is just a nit but there is a jump between 4 and 6.

I did it mainly for back compatibility reasons, so I would not need right now to check do we persist this status in redux-storage and potentially creating migrations.

gnardini · 2020-09-28T13:46:17Z

packages/mobile/src/identity/verification.ts

+    Logger.debug(TAG, 'Verification has been restarted')
+  }
+}
+
 export function* doVerificationFlow(withoutRevealing: boolean = false) {


This method looks really important. I didn't have previous knowledge of it so I appreciate the code comments, but it's still 150 lines long. I know this was already like this before but I wonder if some of the logic could be extracted to other methods to make this a bit easier to read. No need to change anything now, just thinking out loud.

I agree, the whole saga related logic needs a refactoring 👍

packages/mobile/src/verify/VerificationInputScreen.tsx

packages/mobile/src/identity/verification.ts

tarikbellamine · 2020-09-28T16:44:22Z

packages/mobile/src/identity/verification.ts

+  let isRestarted = false
+  while (true) {
+    const withoutRevealing = !isRestarted && initialWithoutRevealing
+    yield call(navigate, Screens.VerificationLoadingScreen, {


Are there any negative repercussions to calling navigate in a loop like this? What happens to the screen stack if VerificationLoadingScreen is already at the top when it's called? I'm not sure but maybe replace is more appropriate in that instance?

As far as I understand, if you call navigate with the current screen it would not result into any modification to the stack. If this is not true, we should re-think our approach here

celo-monorepo/packages/mobile/src/import/saga.ts

Line 64 in 22cd844

navigate(Screens.ImportWallet, { clean: false, showZeroBalanceModal: true })

as well, where we navigate to the same screen just with different params.

If I am understanding this thread correctly, it looks like this method will continue to push the same screen onto the top of the stack with new params.

If so, this probably has negative implications on performance as well as creating some unwanted consequences of breaking the "back" carat. Maybe you can use the setParams method instead?

From here https://reactnavigation.org/docs/navigating/#navigate-to-a-route-multiple-times

... The navigate function roughly means "go to this screen", and if you are already on that screen then it makes sense that it would do nothing.

Also, in our case we will navigate to the same screen only if we are stuck on VerificationLoadingScreen.

From the same page:

Each time you call push we add a new route to the navigation stack. When you call navigate it first tries to find an existing route with that name, and only pushes a new route if there isn't yet one on the stack.

So, I assume it does not add to the stack already existing routes when you navigate to them.

Lmk if that makes sense for you?

My understanding based on this comment in the thread was that if you're passing new params, then it would create a new screen.

To be honest, I'm not too sure about this either way. If you didn't notice any adverse effects when you were testing, I am ok with merging as is given we do this in other parts of the code base.

So, I've logged route.key when navigating to the route with {withoutRevealing: true} and then again with {withoutRevealing: false} and key stays the same. I assume it does not create a new screen, but I might be wrong...

packages/mobile/src/identity/verification.ts

tarikbellamine

Awesome work!

jeanregisser · 2020-09-28T13:45:17Z

packages/mobile/locales/en-US/onboarding.json

  "verificationLearnMoreDialog": {
-    "title": "Phone Number Confirmation",
-    "body": "Phone number confirmation works by associating your account with your phone number.\n\n<0>Do I need to complete this?</0>\n\nConfirmation is not required, however, unconfirmed accounts can only send payments using Celo addresses or QR codes.\n\n<1>Security and Privacy</1>\n\nTo protect your privacy, only an obfuscated version of your phone number is stored on the Celo blockchain.",
+    "title": "Phone Numbers and Valora",


Should be {{appName}} instead of Valora

Just out of curiosity, why is that necessary?

jeanregisser · 2020-09-28T13:45:36Z

packages/mobile/locales/es-419/onboarding.json

    "title": "Confirmación de número de teléfono",
    "body": "La confirmación del número de teléfono funciona asociando tu cuenta con tu número de teléfono.\n\n<0>¿Necesito completar esto?</0>\n\nNo se requiere confirmación, sin embargo, las cuentas no confirmadas solo pueden enviar pagos utilizando direcciones Celo o códigos QR.\n\n<1>Seguridad y privacidad</1>\n\nPara proteger tu privacidad, solo una versión ofuscada de tu número de teléfono se almacena en la cadena de bloques de Celo.",


Looks like these need to be updated too.

jeanregisser · 2020-09-28T13:59:35Z

packages/mobile/src/verify/VerificationEducationScreen.test.tsx

@@ -45,39 +45,6 @@ describe('VerificationEducationScreen', () => {
    expect(queryByTestId('VerificationEducationAlready')).toBeFalsy()
  })

-  it('shows the `continue` and `already received` buttons when there are actionable attestations', () => {


Shouldn't we update the test instead of deleting it, for the state when there are actionable attestations?

Visually there is no difference in rendering, so I've decided to drop this test. The only difference that button click dispatch a slightly different event. Do you think it makes sense to test it here?

### Description * Remove "I already received three codes" and merge its logic into "Resume" button on the VerificationEducation screen. * Add "Resend N messages" on the VerificationInput screen. Button is only available after a minute since the last reveal. When pressed we will try to reveal already existing attestations first. If HTTP call to attestation-service returns anything, but `OK` we request another issuer and try to reveal the number to it. Those we increase number of actionable attestations till the limit 5, after which user need to wait until some attestations expire to request more. * Updated text for "Do I need to confirm" dialog in English. See [figma](https://www.figma.com/file/zt7aTQ5wuXycIwxq5oAmF9/Wallet-Refresh?node-id=7420%3A2384) for more details. ### Tested iOS & Android with actual requesting of SMSs ### Related issues - Part of celo-org#4563 - Closes celo-org#5216 ### Backwards compatibility Yes ### Screenshots ![Simulator Screen Shot - iPhone 11 Pro - 2020-09-22 at 16 57 36](https://user-images.githubusercontent.com/6160124/93912316-e4963580-fd03-11ea-8a05-e2680e6a9008.png) ![Simulator Screen Shot - iPhone 11 Pro - 2020-09-22 at 16 25 06](https://user-images.githubusercontent.com/6160124/93912318-e52ecc00-fd03-11ea-84c2-3624c3c28935.png) ![Simulator Screen Shot - iPhone 11 Pro - 2020-09-22 at 16 22 08](https://user-images.githubusercontent.com/6160124/93912320-e5c76280-fd03-11ea-9f8b-22ae5f735541.png)

### Description Fix to the issue reported in Slack: > I found an issue on the invite redemption flow: After redeeming an invite code, if I try to verify my number I get a "Verification Failed" error immediately. If I skip verification but then verify by tapping on the notification box or the settings menu verification works well. Attaching logs, the relevant line seems to be identity/verification :: Error occured during verification flow :: invalid bytes32 value (arg="identifier", coderType="bytes32", value="") in throwError@http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false:299691:26 This is reproduced 100% on iOS emulator, trying now on Android device. ### Other changes Address Jean's comments in #5134 ### Related Issues - Closes #5228 ### Tested iOS simulator ### Backwards compatibility Yes

Lss-Ankit · 2020-10-01T13:17:41Z

Hi @tarikbellamine Can you please give access to below id for figma screen (https://www.figma.com/) to verify this task
Account: [email protected]

Lss-Ankit · 2020-10-02T14:12:32Z

Hi @tarikbellamine I have verified this task test flight build (25) & Android internal build Android internal build 1.2.0(1004294313) and observe the following
Device: iPhone 11 (14iOS), Samsung Galaxy A70(9.0)

Test plan link: https://docs.google.com/spreadsheets/d/1phHo7MUMQNLjuJOic1TlZwoHM2T0iecT07VLoKtiqMg/edit#gid=1211519342

Need Clarification:

Blank fields is shown on First & second Confirm code fields when user go back and open confirm page
"Verification Error " message is shown when user paste third code from Resend messages
can you please confirm that i need file issues for the same

[Wallet] Add resend button to VerificationInputScreen

4ae24d5

i1skn requested review from cmcewen, jeanregisser and jmrossy as code owners September 22, 2020 16:36

aslawson reviewed Sep 22, 2020

View reviewed changes

jeanregisser added the wallet label Sep 23, 2020

i1skn added the do not merge label Sep 23, 2020

i1skn added 3 commits September 24, 2020 08:44

Merge branch 'master' into i1skn/verification-resend-button

7a5beba

Implement resend attestations with requesting it from new issuers

4b696df

Introduce a limit of actionable attestation

3f32da8

i1skn removed the do not merge label Sep 27, 2020

Merge branch 'master' into i1skn/verification-resend-button

22cd844

gnardini approved these changes Sep 28, 2020

View reviewed changes