Update "Additional details" form in KYC wallet upgrade flow by marcaaron · Pull Request #6752 · Expensify/App

marcaaron · 2021-12-14T04:47:21Z

Details

Improves the "Additional details" step of the KYC Wallet Upgrade flow so it behaves like the VBA.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/188531

Tests

Login with an account that has a SILVER wallet
Trigger the KYC flow by attempting to send money to another user via the FAB + button
Upload doc and video in the Onfido screen
Test the "Additional details" form on the next screen
Make sure submitting the form without any details will highlight which fields are required
Test that you cannot add an address with a PO Box
Test that server errors are presented above the "Save & continue" button and validation errors have a "Fix the errors" link
Close form and re-trigger. Verify that the user details are not cleared out and "remembered".

QA Steps

Internal QA - perform tests above with a new account on the "Send Money" beta

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

add FormScrollView component to DRY things up Fix up Additional Details step in wallet flow undo bad change

marcaaron · 2021-12-14T17:25:32Z

+
+        const newErrors = lodashCloneDeep(errors);
+        lodashUnset(newErrors, fieldName);
+        Wallet.setAdditionalDetailsErrors(newErrors);


I thought about maybe taking the time to abstract some of the logic out of ReimbursementAccountUtils to reuse here, but decided not to since this will all be replaced eventually by our new forms system whenever it is ready.

App/src/libs/ReimbursementAccountUtils.js

Lines 32 to 43 in 9f2b4f0

function clearError(props, path) {

const errors = getErrors(props);

if (!lodashGet(errors, path, false)) {

// No error found for this path

return;

}

// Clear the existing errors

const newErrors = lodashCloneDeep(errors);

lodashUnset(newErrors, path);

BankAccounts.setBankAccountFormValidationErrors(newErrors);

}

I will create a follow up to address this if it looks like the forms project will take a long time.

…rver errors for fields

nickmurray47

Code looks good to me! Had a couple basic comments to add

nickmurray47 · 2021-12-14T21:20:40Z

-
+        setAdditionalDetailsLoading(true);
+        setAdditionalDetailsErrors(null);
+        setAdditionalDetailsErrorMessage();


Even though the default argument is set to '', I think it'd be clearer here to have setAdditionalDetailsErrorMessage('') to make it more obvious that we are clearing the error message. Otherwise, I'm thinking that some other message is getting set.

Thoughts?

Sure I can get behind that reasoning.

nickmurray47 · 2021-12-14T21:24:49Z

    WALLET_ADDITIONAL_DETAILS: 'walletAdditionalDetails',

+    // Stores values put into the additional details step of the wallet KYC flow
+    WALLET_ADDITIONAL_DETAILS_DRAFT: 'walletAdditionalDetailsDraft',


A little bit confused by this key name, why DRAFT over something like WALLET_ADDITIONAL_DETAILS_KYC

Is it a DRAFT because the user might not finish but will need to come back to it?

I'm following the pattern we initially set up here:

App/src/ONYXKEYS.js

Lines 133 to 134 in 1c80150

// Stores draft information about the active reimbursement account being set up

REIMBURSEMENT_ACCOUNT_DRAFT: 'reimbursementAccountDraft',

And yeah they are drafts since if the user bails on the flow and returns later they can pick up where they left off.

This hopefully will be a detail of all forms eventually but for now we have to store it somewhere.

nickmurray47 · 2021-12-14T21:29:37Z

+                        // object with field names as keys and boolean for values
+                        const errorFields = _.reduce(response.data.fieldNames, (errors, fieldName) => ({
+                            ...errors,
+                            [fieldName]: true,


+1 like the change to booleans for values, will keep this refactor in mind

nickmurray47 · 2021-12-14T21:38:00Z

+                                onFixTheErrorsLinkPressed={() => {
+                                    this.form.scrollTo({y: 0, animated: true});
                                }}


marcaaron · 2021-12-14T23:14:20Z

Updated!

nickmurray47

LGTM

MelvinBot · 2021-12-14T23:32:15Z

Triggered auto assignment to @marcaaron (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

botify · 2021-12-14T23:32:17Z

@nickmurray47 looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2021-12-14T23:32:51Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

nickmurray47 · 2021-12-14T23:34:03Z

looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

This doesn't look right to me, going to remove the Emergency label unless you think it's necessary @marcaaron

marcaaron · 2021-12-14T23:43:04Z

Ya I think this is a bug and this did pass all the tests.

OSBotify · 2021-12-16T01:26:08Z

🚀 Deployed to staging by @nickmurray47 in version: 1.1.20-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

Julesssss · 2021-12-17T16:07:55Z

I tried to verify this to unblock the deploy checklist, but unfortunately I'm blocked by this bug in the Onfido flow for Android mWeb 😞

marcaaron · 2021-12-17T16:33:03Z

Ah thanks @Julesssss! I think we can check this off if everything else is working - nobody should be triggering the KYC flow in product yet and we're going to need to E2E test the entire flow on all platforms eventually.

Julesssss · 2021-12-17T16:39:12Z

Ah that's great, thanks for confirming 👍

OSBotify · 2021-12-20T12:08:22Z

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron added 4 commits December 13, 2021 17:53

Add fix the errors to additional details step

f7171b6

add FormScrollView component to DRY things up Fix up Additional Details step in wallet flow undo bad change

Use DatePicker and add more validation

79cff94

fix containerStyles in DatePicker

36bfc7e

undo unnecessary changes

b1ce75f

marcaaron self-assigned this Dec 14, 2021

reset error message on submit

a694ff0

marcaaron changed the title ~~[WIP] Add fix the errors to additional details step~~ Update "Additional details" form in KYC wallet upgrade flow Dec 14, 2021

marcaaron added the InternalQA This pull request required internal QA label Dec 14, 2021

marcaaron commented Dec 14, 2021

View reviewed changes

marcaaron added 2 commits December 14, 2021 07:27

restore usages of scrollview for now.. add comment where we handle se…

0a0580d

…rver errors for fields

Break up the setAdditionalDetailsStep method

31bdc6d

marcaaron marked this pull request as ready for review December 14, 2021 20:23

marcaaron requested a review from a team as a code owner December 14, 2021 20:23

MelvinBot requested review from nickmurray47 and removed request for a team December 14, 2021 20:23

nickmurray47 reviewed Dec 14, 2021

View reviewed changes

explicitly set error message insstead of using default

8007892

nickmurray47 approved these changes Dec 14, 2021

View reviewed changes

nickmurray47 merged commit c993662 into main Dec 14, 2021

nickmurray47 deleted the marcaaron-additionalDetailsFormFix branch December 14, 2021 23:32

botify added the Emergency label Dec 14, 2021

nickmurray47 removed the Emergency label Dec 14, 2021

OSBotify mentioned this pull request Dec 15, 2021

Deploy Checklist: New Expensify 2021-12-15 #6778

Closed

37 tasks

	function clearError(props, path) {
	const errors = getErrors(props);
	if (!lodashGet(errors, path, false)) {
	// No error found for this path
	return;
	}

	// Clear the existing errors
	const newErrors = lodashCloneDeep(errors);
	lodashUnset(newErrors, path);
	BankAccounts.setBankAccountFormValidationErrors(newErrors);
	}

	// Stores draft information about the active reimbursement account being set up
	REIMBURSEMENT_ACCOUNT_DRAFT: 'reimbursementAccountDraft',

Uh oh!

Conversation

marcaaron commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickmurray47 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Dec 14, 2021

Uh oh!

nickmurray47 left a comment

Choose a reason for hiding this comment

Uh oh!

MelvinBot commented Dec 14, 2021

Uh oh!

botify commented Dec 14, 2021

Uh oh!

OSBotify commented Dec 14, 2021

Uh oh!

nickmurray47 commented Dec 14, 2021

Uh oh!

marcaaron commented Dec 14, 2021

Uh oh!

OSBotify commented Dec 16, 2021

Uh oh!

Julesssss commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron commented Dec 17, 2021

Uh oh!

Julesssss commented Dec 17, 2021

Uh oh!

OSBotify commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marcaaron commented Dec 14, 2021 •

edited

Loading

Julesssss commented Dec 17, 2021 •

edited

Loading