Update Address search - show all inputs #6569

aldo-expensify · 2021-12-02T22:10:11Z

Added reviewers from previous PR because you already have some context and saw most of these changes :)

Similar to the previous PR, but without moving the error handling to the form's state. I did a work around to batch clearing errors and kept the errors in Onyx.

Details

Allow the user to manually complete address information and all fields are visible:

Added state, city and zip code fields back in the following steps: "Company Step", "Personal information (requestor)" and "Additional information (beneficial owners)"
Added state and zip code to AddDebitCardPage
When we select a result from the address autocomplete, we use the information that is available. If some part is missing (i.e. city), we just ignore that.
Deleted unused validation method for Google places address components

Fixed Issues

$ #6309

Tests / QA steps

Create NewDot account, verify it.
Create workspace and start the add VBA flow
In "Company information" step:
1. You should see the inputs: Company address, City, Zip Code, and State:
2. Click "Save & Continue", they should get inline errors
3. Typing in any of the field should clear the error.
4. Type and address in Company address and select a result. The other address fields (City, Zip Code, and State) should get updated with the information from the selected result.
5. Focus on the Company address field and type #12334 at the end (adding the apt number).
6. Click "Save & Continue", you should be able to proceed to the next step.
Do the same steps (sub-steps in step (3)) for "Personal information"
Do the same steps (sub-steps in step (3)) for "Additional information" adding beneficial owners
Navigate to settings/payments/add-debit-card and do the same steps (sub-steps in step (3)). The only difference in this view is that the City field is not available.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

onChange callback gets called with an object, and only once when a result is selected

Had to add again `skippedFirstOnChangeText` state to work around the library's feature that is clearing the input at the beginning.

Update IndentityForm input names, now it reads and writes using the same input names for address related fields.

src/components/AddressSearch.js

aldo-expensify · 2021-12-02T22:52:41Z

src/libs/ValidationUtils.js

@@ -206,7 +206,7 @@ function isValidURL(url) {
 * @returns {Object}
 */
 function validateIdentity(identity) {
-    const requiredFields = ['firstName', 'lastName', 'street', 'city', 'zipCode', 'state', 'ssnLast4', 'dob'];
+    const requiredFields = ['firstName', 'lastName', 'addressStreet', 'addressCity', 'addressZipCode', 'addressState', 'ssnLast4', 'dob'];


This change is because I changed these names in the IdentityForm.js for consistency, see lines:

App/src/pages/ReimbursementAccount/IdentityForm.js

Lines 155 to 156 in 970c2d5

value={props.values.state}

onChange={value => props.onFieldChange('addressState', value)}

IdentityForm is reading values using one key (i.e. state) and then calling onChangeText with a different key (i.e. addressState). This just makes it harder to follow and update the logic where we already have some key renaming (i.e. requestor step, beneficial owners).

This just makes it harder to follow and update the logic where we already have some key renaming (i.e. requestor step, beneficial owners).

Unsure what is hard to follow or update, but won't we have to "rename" again if something else decides to use this method which is meant to be generic? Could be better to just leave it generic and then pass in fields we want to validate regardless of what they are named.

Well, looking at it again, this function could really stay as it is. The thing that was confusing is the IdentityForm reading an input with one key and then calling "onFieldChange" with another key.

src/pages/ReimbursementAccount/IdentityForm.js

aldo-expensify · 2021-12-02T23:01:46Z

Two more things:

@luacmartins commented that typing the Apt # in the street field (autocomplete) causing the autocomplete to show results is not the best. I agree, but I would say lets do that in a separate PR.
Since there is not "ref" in the AddressSearch, I think it can be converted to class component. Let me know if I should do that here.

luacmartins

While testing, I've seen the following behavior:

Selecting an address from the dropdown does not clear City and Zip Code errors (see video below).
Just leaving this one here for the record, as we might want to fix it in a different PR. When the dropdown is open, tabbing out will cycle through all dropdown options (without highlighting them) until we finally land on the City field after 6 tabs. The only way to close the dropdown then is to focus on it again and click away or selecting one of the values. Maybe we should close the dropdown on tab out?

error.mov

src/components/AddressSearch.js

aldo-expensify · 2021-12-03T13:30:18Z

Selecting an address from the dropdown does not clear City and Zip Code errors (see video below).

Thanks 😬 , will have a look at it!

Just leaving this one here for the record, as we might want to fix it in a different PR. When the dropdown is open, tabbing out will cycle through all dropdown options (without highlighting them) until we finally land on the City field after 6 tabs. The only way to close the dropdown then is to focus on it again and click away or selecting one of the values. Maybe we should close the dropdown on tab out?

There is this separate GH issue for that: #6235

aldo-expensify · 2021-12-03T14:26:14Z

Selecting an address from the dropdown does not clear City and Zip Code errors (see video below).

Fixed!

…-all-inputs2

aldo-expensify · 2021-12-03T22:36:41Z

I'm going on vacations for two weeks starting on Monday, so I won't be able to continue and finish this during that time.

If anyone wants to use this PR and finish it, feel free to do it. Also feel free to pick the original issue and do a new PR if this doesn't seem to be the right path :)

If the issue is still there when I come back, I can start working on it. I don't think it is of high priority because the user currently can enter any address manually.

marcaaron

Left a few more comments and ideas for how to improve a few things.

I think it would be good to maybe try and just wrap this one up and really try to do the bare minimum simplest things we can to make this work - then revisit things like perfecting errors and validation once the forms doc refactors start.

src/components/AddressSearch.js

src/libs/ReimbursementAccountUtils.js

marcaaron · 2021-12-02T23:27:18Z

src/libs/ValidationUtils.js

@@ -206,7 +206,7 @@ function isValidURL(url) {
 * @returns {Object}
 */
 function validateIdentity(identity) {
-    const requiredFields = ['firstName', 'lastName', 'street', 'city', 'zipCode', 'state', 'ssnLast4', 'dob'];
+    const requiredFields = ['firstName', 'lastName', 'addressStreet', 'addressCity', 'addressZipCode', 'addressState', 'ssnLast4', 'dob'];


This just makes it harder to follow and update the logic where we already have some key renaming (i.e. requestor step, beneficial owners).

Unsure what is hard to follow or update, but won't we have to "rename" again if something else decides to use this method which is meant to be generic? Could be better to just leave it generic and then pass in fields we want to validate regardless of what they are named.

marcaaron · 2021-12-04T00:18:37Z

src/components/AddressSearch.js

+    // We use `skippedFirstOnChangeText` to work around a feature of the library:
+    // The library is calling onChangeText with '' at the start and we don't need this
+    // https://github.com/FaridSafi/react-native-google-places-autocomplete/blob/47d7223dd48f85da97e80a0729a985bbbcee353f/GooglePlacesAutocomplete.js#L148
+    const [skippedFirstOnChangeText, setSkippedFirstOnChangeText] = useState(false);


Not saying this is likely but example of how this could go wrong in the future...

Someone updates this library and the bug gets fixed

They are now stuck having to figure out why the first onChangeText never fires

Maybe they find this code eventually 😂

opened an issue here FaridSafi/react-native-google-places-autocomplete#786

src/components/AddressSearch.js

src/pages/ReimbursementAccount/ACHContractStep.js

marcaaron · 2021-12-04T00:38:23Z

src/pages/ReimbursementAccount/ACHContractStep.js

@@ -233,14 +252,20 @@ class ACHContractStep extends React.Component {
                                    </ExpensifyText>
                                    <IdentityForm
                                        style={[styles.mb2]}
-                                        onFieldChange={(inputKey, value) => this.clearErrorAndSetBeneficialOwnerValue(index, inputKey, value)}
+                                        onFieldChange={(inputKeyOrValues, value) => {


Having onFieldChange return one of two possible things is a bit strange.

What if we just return an object and have the validation and error clearing handled all at the same time for the various fields?

Sorry to sound like a broken record, but the way we validate these will be changing soon in pretty drastic ways so maybe we should not spend much time perfecting this.

Having onFieldChange return one of two possible things is a bit strange.

Agreed, didn't liked it, but it was what I came up with to solve the clearing multiple errors in a batch.

What if we just return an object and have the validation and error clearing handled all at the same time for the various fields?

the idea is to ALWAYS pass an object to the onFieldChange callback, even if we are just changing a single field like firstName, right?

Update: onFieldChange gets called with an object every time.

src/pages/ReimbursementAccount/ACHContractStep.js

marcaaron · 2021-12-04T00:51:31Z

src/pages/ReimbursementAccount/IdentityForm.js

+            />
+            <ExpensifyText style={[styles.mutedTextLabel, styles.mt1]}>{props.translate('common.noPO')}</ExpensifyText>
+            <View style={[styles.flexRow, styles.mt4]}>
+                <View style={[styles.flex2, styles.mr2]}>


opportunity to DRY up the code perhaps?

Looks very similar to the lines here:

App/src/pages/ReimbursementAccount/CompanyStep.js

Lines 223 to 227 in b0fe744

<ExpensifyText style={[styles.mutedTextLabel, styles.mt1]}>{this.props.translate('common.noPO')}</ExpensifyText>

<View style={[styles.flexRow, styles.mt4]}>

<View style={[styles.flex2, styles.mr2]}>

<ExpensiTextInput

label={this.props.translate('common.city')}

Did some refactoring for this. Added AddressForm component, the name was based on the name for IndentityForm, which is also a "partial" form.

marcaaron · 2021-12-23T06:39:08Z

@aldo-expensify just checking on this one. Are you actively working on it and if not should we close it?

aldo-expensify · 2021-12-23T13:38:57Z

@aldo-expensify just checking on this one. Are you actively working on it and if not should we close it?

I'll try to revive this PR and address your comments, there are a bunch of conflicts now :P

…-all-inputs2

aldo-expensify · 2021-12-23T17:04:16Z

Update:

(AddressSearch) Using ref instead of state to keep track of skipping the first time the library calls onTextChange with ''
Use street, city, zipCode and state without "address" prefix in IdentityForm and ValidationUtils. This removed a few input key renaming (maps) but added new ones in other places. Overall, I think it reduced the number of changes and I personally prefer it without the "address" prefix everywhere.

aldo-expensify · 2021-12-23T17:17:23Z

Update: IdentityForm now always calls onFieldChange passing an object with shape {[inputName]: value}. The callbacks look less ugly now.

marcaaron

Tests well, nice job and thanks for sticking with it!

marcaaron · 2021-12-23T19:48:30Z

src/components/AddressSearch.js

+
+        const values = {};
+        if (street && street.length > props.value.length) {
+            // Don't replace if the user has typed something longer. I.e. maybe the user entered the Apt #


Thanks for the change!

NAB, this comment does help explain why we might not want to use the street number and street name provided by Google Places. I do have a lingering concern that we are not quite painting the exact picture of why we need this logic. Here's a suggestion that is more verbose, but explains the situation in greater detail

We are only passing the street number and name if the combined length is longer than the value that was initially passed to the autocomplete component. Google Places can truncate details like Apt # and this is the best way we have to tell that the new value it's giving us is less specific than the one the user entered manually.

marcaaron · 2021-12-23T19:49:11Z

src/components/AddressSearch.js

+        const state = GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name');
+
+        const values = {};
+        if (street && street.length > props.value.length) {


NAB, should we trim props.value.length in case there is whitespace that makes it longer but less accurate?

Also, I wonder if we need to worry about a case like this where someone enters some very long address into the street input. I feel like in trying to optimize for the Apartment # case we are maybe introducing the possibility of bad street info instead of trusting the correctness of what Google Places returns.

Here's an example of what I mean:

I think we can maybe wait and see if this is a problem...

NAB, should we trim props.value.length in case there is whitespace that makes it longer but less accurate?

Also, I wonder if we need to worry about a case like this where someone enters some very long address into the street input. I feel like in trying to optimize for the Apartment # case we are maybe introducing the possibility of bad street info instead of trusting the correctness of what Google Places returns.

I think we can maybe wait and see if this is a problem...

I honestly don't like it either, I think the ideal case is to have the Apt number in a separate field. As @luacmartins , this also causes the autocomplete to start showing results while the user may be only trying to input the apt number and it looks weird :P. Having said that, if we wanted to do a separate input for it, I think that should be done in a separate pr/issue.

src/components/AddressSearch.js

marcaaron · 2021-12-23T19:58:13Z

src/pages/ReimbursementAccount/AddressForm.js

+    errors: {},
+};
+
+const AddressForm = props => (


Thanks for refactoring this!

marcaaron · 2021-12-23T20:02:13Z

src/pages/ReimbursementAccount/CompanyStep.js

+                            this.setValue(renamedValues);
+                            this.clearErrors(_.keys(renamedValues));
+                        }}
+                    />


So much cleaner than what we had before 👏

marcaaron · 2021-12-23T20:06:52Z

src/pages/ReimbursementAccount/CompanyStep.js

+                        onFieldChange={(values) => {
+                            const renamedValues = {};
+                            _.each(values, (value, inputKey) => {
+                                const renamedInputKey = lodashGet(this.renamedFields, inputKey, inputKey);


NAB, if we are not using this.renamedFields again we could just declare them in this method. Only thought to mention it because we are doing that in the RequestorStep and could keep it consistent.

OSBotify · 2021-12-23T20:40: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.

OSBotify · 2021-12-29T00:50:43Z

🚀 Deployed to staging by @marcaaron in version: 1.1.23-2 🚀

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

marcaaron · 2022-01-03T20:27:09Z

src/pages/settings/Payments/AddDebitCardPage.js

+                                    _.each(values, (value, inputKey) => {
+                                        if (inputKey === 'city') {
+                                            return;
+                                        }


Just noticed this, but don't we want to update the city field?

I saw that the city field wasn't properly added in any of these places in the component: state, requiredFields and errorTranslationKeys.

And, most importantly, we didn't really use it when sending the information to the backend here:

App/src/libs/actions/PaymentMethods.js

Lines 43 to 71 in 87f41cc

function addBillingCard(params) {

const cardMonth = CardUtils.getMonthFromExpirationDateString(params.expirationDate);

const cardYear = CardUtils.getYearFromExpirationDateString(params.expirationDate);

Onyx.merge(ONYXKEYS.ADD_DEBIT_CARD_FORM, {submitting: true});

API.AddBillingCard({

cardNumber: params.cardNumber,

cardYear,

cardMonth,

cardCVV: params.securityCode,

addressName: params.nameOnCard,

addressZip: params.addressZipCode,

currency: CONST.CURRENCY.USD,

}).then(((response) => {

let errorMessage = '';

if (response.jsonCode === 200) {

const cardObject = {

additionalData: {

isBillingCard: false,

isP2PDebitCard: true,

},

addressName: params.nameOnCard,

addressState: params.addressState,

addressStreet: params.addressStreet,

addressZip: params.addressZipCode,

cardMonth,

cardNumber: CardUtils.maskCardNumber(params.cardNumber),

cardYear,

currency: 'USD',

I thought that it was added by mistake when the "manual" switch was added. It made sense to me that it wasn't being sent to the back because I have seen many card forms that don't have a "City" field.

ah, not used by the backend hmm OK then thanks!

OSBotify · 2022-01-04T01:34:43Z

🚀 Deployed to production by @francoisl in version: 1.1.24-8 🚀

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

aldo-expensify added 8 commits December 2, 2021 11:24

Add clearErrors to clear multiple errors on one call

2865a78

Replace onChangeText with onChange.

8f8d148

onChange callback gets called with an object, and only once when a result is selected

Update CompanyStep with new AddressSearch

32a9eb9

Add two-way binding for AddressSearch text input

91920d6

Had to add again `skippedFirstOnChangeText` state to work around the library's feature that is clearing the input at the beginning.

Update requestor step with new address fields

bf63468

Update IndentityForm input names, now it reads and writes using the same input names for address related fields.

Update ACHContract step handling of IdentityForm

0ea709a

Update AddDebitCardPage with new AddressSearch

9295c47

Remove unused validation

27b0d84

aldo-expensify commented Dec 2, 2021

View reviewed changes

src/components/AddressSearch.js Show resolved Hide resolved

aldo-expensify commented Dec 2, 2021

View reviewed changes

src/pages/ReimbursementAccount/IdentityForm.js Outdated Show resolved Hide resolved

aldo-expensify self-assigned this Dec 2, 2021

aldo-expensify requested review from nickmurray47, sketchydroide, marcaaron, Jag96 and luacmartins December 2, 2021 22:55

aldo-expensify mentioned this pull request Dec 2, 2021

Enable again all address fields and allow the user to manually fill them / Move form errors to component state #6200

Closed

5 tasks

Don't replace street if user typed something longer

3f73b08

aldo-expensify marked this pull request as ready for review December 2, 2021 23:08

aldo-expensify requested a review from a team as a code owner December 2, 2021 23:08

MelvinBot requested review from deetergp and removed request for a team December 2, 2021 23:09

aldo-expensify removed the request for review from deetergp December 2, 2021 23:09

Update variable name

5adf0c2

luacmartins reviewed Dec 3, 2021

View reviewed changes

src/components/AddressSearch.js Outdated Show resolved Hide resolved

src/components/AddressSearch.js Outdated Show resolved Hide resolved

aldo-expensify added 2 commits December 3, 2021 11:11

Fix CompanyStep not clearing errors

7d44cd8

Resolve conclifts

d7ac198

aldo-expensify mentioned this pull request Dec 3, 2021

VBA: always show all address fields #6309

Closed

Merge branch 'main' of github.com:Expensify/App into aldo_vba-address…

b0fe744

…-all-inputs2

marcaaron requested changes Dec 4, 2021

View reviewed changes

aldo-expensify added 5 commits December 23, 2021 12:26

Merge branch 'main' of github.com:Expensify/App into aldo_vba-address…

f8377ad

…-all-inputs2

Use ref instead of state to skip first onChangeText('')

aed7b8c

Remove unnecesary new line

e7ff88a

Remove unnecesary empty placeholder

fd5316e

Remove "address" prefix from address related input

3fd626a

IdentityForm calls onChange always passing an object

c30f08a

aldo-expensify added 3 commits December 23, 2021 14:32

Cleanup contract step

ec758cc

Remove unused function

8466121

Refactored address related fields into new AddressForm component

3cddfcf

marcaaron approved these changes Dec 23, 2021

View reviewed changes

marcaaron merged commit 4b7f78a into main Dec 23, 2021

marcaaron deleted the aldo_vba-address-all-inputs2 branch December 23, 2021 20:40

aldo-expensify mentioned this pull request Dec 23, 2021

[NO QA] Minor refactor - polish #6897

Merged

OSBotify mentioned this pull request Dec 29, 2021

Deploy Checklist: New Expensify 2021-12-29 #6920

Closed

44 tasks

marcaaron reviewed Jan 3, 2022

View reviewed changes

marcaaron mentioned this pull request Jan 3, 2022

Return promise in payIOUReport / Fix AdditionalDetailsStep #6992

Merged

5 tasks

This was referenced Mar 29, 2023

[$4000] Address - After selecting address from drop-down Address Line is not correctly filled #15784

Closed

Better handling of additional address fragments (e.g. Apt #) in AddressSearch #16745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Address search - show all inputs #6569

Update Address search - show all inputs #6569

aldo-expensify commented Dec 2, 2021 •

edited

Loading

aldo-expensify Dec 2, 2021

marcaaron Dec 2, 2021

aldo-expensify Dec 23, 2021

aldo-expensify commented Dec 2, 2021 •

edited

Loading

luacmartins left a comment

aldo-expensify commented Dec 3, 2021

aldo-expensify commented Dec 3, 2021

aldo-expensify commented Dec 3, 2021

marcaaron left a comment

marcaaron Dec 2, 2021

marcaaron Dec 4, 2021

marcaaron Dec 4, 2021

marcaaron Dec 4, 2021

aldo-expensify Dec 23, 2021 •

edited

Loading

aldo-expensify Dec 23, 2021

marcaaron Dec 4, 2021

aldo-expensify Dec 23, 2021

marcaaron commented Dec 23, 2021

aldo-expensify commented Dec 23, 2021

aldo-expensify commented Dec 23, 2021 •

edited

Loading

aldo-expensify commented Dec 23, 2021

marcaaron left a comment

marcaaron Dec 23, 2021

marcaaron Dec 23, 2021

aldo-expensify Dec 23, 2021

marcaaron Dec 23, 2021

marcaaron Dec 23, 2021

marcaaron Dec 23, 2021

OSBotify commented Dec 23, 2021

OSBotify commented Dec 29, 2021

marcaaron Jan 3, 2022

aldo-expensify Jan 3, 2022 •

edited

Loading

marcaaron Jan 3, 2022

OSBotify commented Jan 4, 2022

	value={props.values.state}
	onChange={value => props.onFieldChange('addressState', value)}

	<ExpensifyText style={[styles.mutedTextLabel, styles.mt1]}>{this.props.translate('common.noPO')}</ExpensifyText>
	<View style={[styles.flexRow, styles.mt4]}>
	<View style={[styles.flex2, styles.mr2]}>
	<ExpensiTextInput
	label={this.props.translate('common.city')}

	function addBillingCard(params) {
	const cardMonth = CardUtils.getMonthFromExpirationDateString(params.expirationDate);
	const cardYear = CardUtils.getYearFromExpirationDateString(params.expirationDate);

	Onyx.merge(ONYXKEYS.ADD_DEBIT_CARD_FORM, {submitting: true});
	API.AddBillingCard({
	cardNumber: params.cardNumber,
	cardYear,
	cardMonth,
	cardCVV: params.securityCode,
	addressName: params.nameOnCard,
	addressZip: params.addressZipCode,
	currency: CONST.CURRENCY.USD,
	}).then(((response) => {
	let errorMessage = '';
	if (response.jsonCode === 200) {
	const cardObject = {
	additionalData: {
	isBillingCard: false,
	isP2PDebitCard: true,
	},
	addressName: params.nameOnCard,
	addressState: params.addressState,
	addressStreet: params.addressStreet,
	addressZip: params.addressZipCode,
	cardMonth,
	cardNumber: CardUtils.maskCardNumber(params.cardNumber),
	cardYear,
	currency: 'USD',

Update Address search - show all inputs #6569

Update Address search - show all inputs #6569

Conversation

aldo-expensify commented Dec 2, 2021 • edited Loading

Details

Fixed Issues

Tests / QA steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify commented Dec 2, 2021 • edited Loading

luacmartins left a comment

Choose a reason for hiding this comment

aldo-expensify commented Dec 3, 2021

aldo-expensify commented Dec 3, 2021

aldo-expensify commented Dec 3, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Dec 23, 2021

aldo-expensify commented Dec 23, 2021

aldo-expensify commented Dec 23, 2021 • edited Loading

aldo-expensify commented Dec 23, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Dec 23, 2021

OSBotify commented Dec 29, 2021

Choose a reason for hiding this comment

aldo-expensify Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jan 4, 2022

aldo-expensify commented Dec 2, 2021 •

edited

Loading

aldo-expensify commented Dec 2, 2021 •

edited

Loading

aldo-expensify Dec 23, 2021 •

edited

Loading

aldo-expensify commented Dec 23, 2021 •

edited

Loading

aldo-expensify Jan 3, 2022 •

edited

Loading