[HOLD for payment 2023-02-23] [$1000] Zipcode doesn’t update with a newly chosen location and remains unchanged

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open web app
2. Create a new workspace
3. Click on connect a bank account
4. Fill in the routing number and account details
5. Go to company address and search in for a location that has zip code. (For example, Search for Hunstville road and select any one from the suggestions. You’ll see City, state and zipcode populates properly)

Expected Result:

Zipcode value should have been updated with the newly chosen location or it should have been empty in cases where no zipcode value is present in the database

Actual Result:

Zipcode value doesn’t update with the newly chosen location in some cases like texas, Missouri (These are just a few location examples, there are a lot more). But now remove the company address and type in Texas and select any one from the suggestions. You’ll notice that the zip code doesn’t change and has the same zipcode for the previous set location (in our case Huntsville). It should have shown texas zipcode or it should have shown blank in the zipcode field.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] MacOS / Chrome / Safari

Version Number: 1.2.54-2 Reproducible in staging?: Y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/212739789-912bd9fe-a595-45ec-b49f-ecec22e230f5.mp4

https://user-images.githubusercontent.com/43996225/212739803-c255c37d-23f8-4cd2-ac0d-0425e057263f.mp4

Expensify/Expensify Issue URL: Issue reported by: @priya-zha Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673774316512329

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0164a7b7c772a0bdbb
• Upwork Job ID: 1615340851475869696
• Last Price Increase: 2023-01-17

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0164a7b7c772a0bdbb

[melvin-bot[bot]]

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

[melvin-bot[bot]]

Triggered auto assignment to @bondydaa (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[bernhardoj]

Proposal

This happen because we check if the zipCode has a value, then we change the text field. To always update it, we can remove the condition and set an empty string as the default value.

diff --git a/src/components/AddressSearch.js b/src/components/AddressSearch.js
index 4d465c990..d40c86f5b 100644
--- a/src/components/AddressSearch.js
+++ b/src/components/AddressSearch.js
@@ -113,9 +113,7 @@ const AddressSearch = (props) => {
         if (city) {
             values.city = city;
         }
-        if (zipCode) {
-            values.zipCode = zipCode;
-        }
+        values.zipCode = zipCode || '';
         if (state) {
             values.state = state;
         }

Maybe we can do this to all field.

Or we can move the default value to this function

diff --git a/src/libs/GooglePlacesUtils.js b/src/libs/GooglePlacesUtils.js
index 5a875e7f1..3fb1fb30f 100644
--- a/src/libs/GooglePlacesUtils.js
+++ b/src/libs/GooglePlacesUtils.js
@@ -18,7 +18,7 @@ function getAddressComponent(addressComponents, type, key) {
     return _.chain(addressComponents)
         .find(component => _.contains(component.types, type))
         .get(key)
-        .value();
+        .value() || '';
 }

 export {

By the way, we have an issue which if we update the text field value props to empty, the label does not deactivate because it checks the current state value which is not updated yet to empty string.

https://github.com/Expensify/App/blob/aff83ef1895d48603f9610bf56bd06722b8e10c2/src/components/TextInput/BaseTextInput.js#L75-L95

https://github.com/Expensify/App/blob/aff83ef1895d48603f9610bf56bd06722b8e10c2/src/components/TextInput/BaseTextInput.js#L162-L169

To solve this (to prevent future issue report), we can also check the props value length.

diff --git a/src/components/TextInput/BaseTextInput.js b/src/components/TextInput/BaseTextInput.js
index 1788476ed..048934267 100644
--- a/src/components/TextInput/BaseTextInput.js
+++ b/src/components/TextInput/BaseTextInput.js
@@ -160,7 +160,9 @@ class BaseTextInput extends Component {
     }

     deactivateLabel() {
-        if (this.props.forceActiveLabel || this.state.value.length !== 0 || this.props.prefixCharacter) {
+        if (this.props.forceActiveLabel 
+            || (this.state.value.length !== 0 && this.props.value.length !== 0) 
+            || this.props.prefixCharacter) {
             return;
         }

or we can use the setState callback to activate/deactivate the label.

diff --git a/src/components/TextInput/BaseTextInput.js b/src/components/TextInput/BaseTextInput.js
index 1788476ed..4ab260668 100644
--- a/src/components/TextInput/BaseTextInput.js
+++ b/src/components/TextInput/BaseTextInput.js
@@ -80,18 +80,18 @@ class BaseTextInput extends Component {
         }

         // eslint-disable-next-line react/no-did-update-set-state
-        this.setState({value: inputValue, selection: this.props.selection});
+        this.setState({value: inputValue, selection: this.props.selection}, () => {
+            if (inputValue) {
+                this.activateLabel();
+            } else if (!this.state.isFocused) {
+                this.deactivateLabel();
+            }
+        });

         // In some cases, When the value prop is empty, it is not properly updated on the TextInput due to its uncontrolled nature, thus manually clearing the TextInput.
         if (inputValue === '') {
             this.input.clear();
         }
-
-        if (inputValue) {
-            this.activateLabel();
-        } else if (!this.state.isFocused) {
-            this.deactivateLabel();
-        }
     }

[s77rt]

Proposal

diff --git a/src/components/AddressSearch.js b/src/components/AddressSearch.js
index 4d465c990a..c2952f2de5 100644
--- a/src/components/AddressSearch.js
+++ b/src/components/AddressSearch.js
@@ -103,21 +103,21 @@ const AddressSearch = (props) => {
         const values = {
             street: props.value ? props.value.trim() : '',
         };
+        let shouldUpdateValues = false;
         if (street && street.length >= values.street.length) {
             // 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.
             values.street = street;
+            shouldUpdateValues = true;
+        } else if (city || zipCode || state) {
+            shouldUpdateValues = true;
         }
-        if (city) {
-            values.city = city;
-        }
-        if (zipCode) {
-            values.zipCode = zipCode;
-        }
-        if (state) {
-            values.state = state;
+        if (shouldUpdateValues) {
+            values.city = city || '';
+            values.zipCode = zipCode || '';
+            values.state = state || '';
         }
         if (_.size(values) === 0) {
             return;

RCA

As @bernhardoj pointed we only update the values if they are not empty. But the real issue is the partial update. We should not do partial updates, we should either update everything or keep everything (with the exception of the street value which may not be updated in some cases - check the comments)

To do that I have defined a new variable shouldUpdateValues. I think the code is self-explanatory.

---

I'm not sure about the issue mentioned by @bernhardoj but since it seems out of scope, I'm not handling this for now.

[Pujan92]

Proposal

We can simply set the values for city, zipcode, and state with the availability otherwise empty string whenever the user presses the autocomplete option. Reason is to we should update all details when he selects option from the list.

diff --git a/src/components/AddressSearch.js b/src/components/AddressSearch.js
index 4d465c990..a61558993 100644
--- a/src/components/AddressSearch.js
+++ b/src/components/AddressSearch.js
@@ -110,15 +110,10 @@ const AddressSearch = (props) => {
             // specific than the one the user entered manually.
             values.street = street;
         }
-        if (city) {
-            values.city = city;
-        }
-        if (zipCode) {
-            values.zipCode = zipCode;
-        }
-        if (state) {
-            values.state = state;
-        }
+        values.city = city || '';
+        values.zipCode = zipCode || '';
+        values.state = state || '';

         if (_.size(values) === 0) {
             return;
         }

https://user-images.githubusercontent.com/14358475/213181832-3072d6a5-c1b9-4878-a3f0-3973f6c960ed.mov

[mananjadhav]

Thank you folks for the proposals here.

I think @s77rt's proposal is simple and good here.

@Pujan92 While your proposal is also similar, I can see we're doing a conditional update to street, and if that isn't getting updated, then I don't see a reason why we should be updating other values?

@bernhardoj I think your proposal is too complex to solve something so trivial.

@bondydaa what do you think?

[Pujan92]

@mananjadhav my intention is to update all fields when the user selects any option from the list, for the street we have conditions to update or not but logically updating all other fields seems a clean way to me. 1 usecase maybe we have the same street number/name but the city or state should be different, on choosing the different option won't update the city/state/zipcode.

[bernhardoj]

@mananjadhav I think you can ignore the 2nd part of my proposal (the textinput changes). I provided that because when we set the zip code with an empty string, the label does not get deactivated.

[mananjadhav]

same street number/name but the city or state should be different

That's a good point @Pujan92. I am not sure how it works for the US, but I think this is a possibility that the same street name might exist for different countries/state. If I look at the change, and the comment made I think the idea is that if the user is manually changing the value only then it updates the street. I think the same street name logic should cater to that.

I am going to tag @aldo-expensify if they can shed some light on the use case.

---

@bernhardoj Thanks for the comment, but I think the other proposals do cover all the fields compared to zipCode.

[bernhardoj]

Maybe we can do this to all field.

@mananjadhav I have pointed out on my proposal that maybe we want to do it for the other fields because the issue only initially stated about zip code.

Also, I would like to answer for the street conditional updates. It will not update the value in case the value we type is longer than the value from the google places autocomplete. For example, we can type Huntsville Road, Huntsville, AR, USA, 123 and select it from the autocomplete. Because the length of what we type is longer, thus the street value does not get updated, but the others field still get auto filled.

[Pujan92]

If we only update other fields on street name update then as I mentioned the same name issue can arise.

https://user-images.githubusercontent.com/14358475/213227718-bbbcbc69-08c7-463c-af87-693fb4df02bd.mov

It will be fixed with @s77rt or my proposal but only concern is to why to put extra checks, as long as user selects new option we should update the fields based on the new data.

[s77rt]

@mananjadhav I agree with @Pujan92 we should always update the values, in my proposal shouldUpdateValues will probably always be true so it's redundant anyway. I missed the fact that we are doing those changes onPress callback and since it's the case we should always update all the values on press.

[Snehal-Techforce]

Proposal

Root Cause: For each location does not have Zip Code in the system. Hence, during location changes, if location does not have zip code, then it keeps remains previous Zip Code. Resolution: "If" condition needs to remove and let update the Zip Code with new location Zip Code or blank during location change.

Preventive Step: At present, each location contains value of City and State. Hence, it always getting true and showing the updated City and State. However, as an improvement values of those 2 fields can also be placed underneath of zipCode in above change.

[aldo-expensify]

I am going to tag @aldo-expensify if they can shed some light on the use case.

Hey @mananjadhav , sorry I'm a bit swamped at the time with work to have a look here. If you need a second opinion please ask in the slack contributor channel 🙇

[mananjadhav]

No worries @aldo-expensify. I checked for the side effects.

Thanks for the comment @s77rt and @Pujan92. I saw we can have same street names across different cities. While I don't see any downsides with @s77rt proposal with how our street values change, but I think it is safe to always update the city instead of being dependent on the street value. I think if we do a conditional updates, we could miss out on any side effects/manual updates.

That being said @Pujan92 your proposal is good to go.

C+ reviewed 🎀👀🎀

[bernhardoj]

@mananjadhav hi, I don't mean to disregard your decision, but I feel it's not fair to choose @Pujan92 proposal over mine (no hard feeling ✌️) as my proposal is the first one to suggest the empty string default value.

I am aware that I only provide the code for zip code only, however

1. The issue only stated about zip code
2. I wrote in my proposal that maybe we want to apply the changes to the other field

I would love to hear some opinion maybe from @bondydaa (because I see you are the engineer assigned here, sorry if I'm wrong). @s77rt Maybe you want to give your opinion too because I see you like to give your opinion on other proposal 😀.

Thank you! ✌️

[s77rt]

@bernhardoj

I see you like to give your opinion on other proposal

😂😂

Honestly, I don't know who should get hired, You wrote "Maybe we can do this to all field." so technically you didn't forget about other fields but you seems uncertain as you are asking and not suggesting, if you said "We should do this to all fields." it would be more convincing to go with your proposal. But that's a really minor thing and perhaps we should not take things so literally that way. That being said, maybe this is a case where you should just split the compensation or even better give it up to @s77rt 😂

[bondydaa]

hi sorry just catching up on this one now.

[bondydaa]

Alrighty think I've wrapped my head around this one.

So I think I'd actually prefer what @bernhardoj suggested in the first proposal about fixing this in the src/libs/GooglePlacesUtils.js itself.

This is because it allows us to greatly clean up the rest of src/components/AddressSearch.js b/c basically everything that is calling GooglePlacesUtils.getAddressComponent is doing the || '' check then if we do the other proposed solutions.

Then we basically update src/components/AddressSearch.js to build the values object like so:

diff --git a/src/components/AddressSearch.js b/src/components/AddressSearch.js
index 4d465c990a..9fc5a600bb 100644
--- a/src/components/AddressSearch.js
+++ b/src/components/AddressSearch.js
@@ -89,20 +89,21 @@ const AddressSearch = (props) => {
         }

         // Gather the values from the Google details
-        const streetNumber = GooglePlacesUtils.getAddressComponent(addressComponents, 'street_number', 'long_name') || '';
-        const streetName = GooglePlacesUtils.getAddressComponent(addressComponents, 'route', 'long_name') || '';
+        const streetNumber = GooglePlacesUtils.getAddressComponent(addressComponents, 'street_number', 'long_name');
+        const streetName = GooglePlacesUtils.getAddressComponent(addressComponents, 'route', 'long_name');
         const street = `${streetNumber} ${streetName}`.trim();
         let city = GooglePlacesUtils.getAddressComponent(addressComponents, 'locality', 'long_name');
         if (!city) {
             city = GooglePlacesUtils.getAddressComponent(addressComponents, 'sublocality', 'long_name');
             Log.hmmm('[AddressSearch] Replacing missing locality with sublocality: ', {address: details.formatted_address, sublocality: city});
         }
-        const zipCode = GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name');
-        const state = GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name');
-
         const values = {
             street: props.value ? props.value.trim() : '',
+            city,
+            zipCode: GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name'),
+            state = GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name'),
         };
+
         if (street && street.length >= values.street.length) {
             // 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
@@ -110,15 +111,6 @@ const AddressSearch = (props) => {
             // specific than the one the user entered manually.
             values.street = street;
         }
-        if (city) {
-            values.city = city;
-        }
-        if (zipCode) {
-            values.zipCode = zipCode;
-        }
-        if (state) {
-            values.state = state;
-        }
         if (_.size(values) === 0) {
             return;
         }

We could probably simplify city and street as well in there but my brain is getting tired for the day 😅.

This shouldn't introduce any other regressions b/c the GooglePlacesUtils.getAddressComponent method is only used in this component

Expensidev/App (main) $ ack getAddressComponent
tests/unit/GooglePlacesUtilsTest.js
4:    describe('getAddressComponent', () => {
33:            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'sublocality', 'long_name')).toStrictEqual('Brooklyn');
34:            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name')).toStrictEqual('NY');
35:            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name')).toStrictEqual('11206');
36:            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'doesn-exist', 'long_name')).toStrictEqual(undefined);

src/libs/GooglePlacesUtils.js
17:function getAddressComponent(addressComponents, type, key) {
26:    getAddressComponent,

src/components/AddressSearch.js
92:        const streetNumber = GooglePlacesUtils.getAddressComponent(addressComponents, 'street_number', 'long_name') || '';
93:        const streetName = GooglePlacesUtils.getAddressComponent(addressComponents, 'route', 'long_name') || '';
95:        let city = GooglePlacesUtils.getAddressComponent(addressComponents, 'locality', 'long_name');
97:            city = GooglePlacesUtils.getAddressComponent(addressComponents, 'sublocality', 'long_name');
100:        const zipCode = GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name');
101:        const state = GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name');

What do you think @mananjadhav? cc @roryabraham and @aldo-expensify (I know you already got pinged) but you both introduced most of this on this PR https://github.com/Expensify/App/pull/5950 so just want to make sure I'm not missing a reason why we wouldn't want to fix this in the GooglePlacesUtils lib.

[s77rt]

Proposal 2

diff --git a/src/components/AddressSearch.js b/src/components/AddressSearch.js
index 4d465c990a..c782669e0d 100644
--- a/src/components/AddressSearch.js
+++ b/src/components/AddressSearch.js
@@ -8,7 +8,6 @@ import CONFIG from '../CONFIG';
 import withLocalize, {withLocalizePropTypes} from './withLocalize';
 import styles from '../styles/styles';
 import TextInput from './TextInput';
-import Log from '../libs/Log';
 import * as GooglePlacesUtils from '../libs/GooglePlacesUtils';

 // The error that's being thrown below will be ignored until we fork the
@@ -89,20 +88,30 @@ const AddressSearch = (props) => {
         }

         // Gather the values from the Google details
-        const streetNumber = GooglePlacesUtils.getAddressComponent(addressComponents, 'street_number', 'long_name') || '';
-        const streetName = GooglePlacesUtils.getAddressComponent(addressComponents, 'route', 'long_name') || '';
-        const street = `${streetNumber} ${streetName}`.trim();
-        let city = GooglePlacesUtils.getAddressComponent(addressComponents, 'locality', 'long_name');
-        if (!city) {
-            city = GooglePlacesUtils.getAddressComponent(addressComponents, 'sublocality', 'long_name');
-            Log.hmmm('[AddressSearch] Replacing missing locality with sublocality: ', {address: details.formatted_address, sublocality: city});
-        }
-        const zipCode = GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name');
-        const state = GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name');
+        const {
+            street_number: streetNumber,
+            route: streetName,
+            locality: city,
+            sublocality: cityFallback,
+            postal_code: zipCode,
+            administrative_area_level_1: state,
+        } = GooglePlacesUtils.getAddressComponent(addressComponents, {
+            street_number: 'long_name',
+            route: 'long_name',
+            locality: 'long_name',
+            sublocality: 'long_name',
+            postal_code: 'long_name',
+            administrative_area_level_1: 'short_name',
+        });

         const values = {
             street: props.value ? props.value.trim() : '',
+            city: city || cityFallback,
+            zipCode,
+            state,
         };
+
+        const street = `${streetNumber} ${streetName}`.trim();
         if (street && street.length >= values.street.length) {
             // 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
@@ -110,18 +119,7 @@ const AddressSearch = (props) => {
             // specific than the one the user entered manually.
             values.street = street;
         }
-        if (city) {
-            values.city = city;
-        }
-        if (zipCode) {
-            values.zipCode = zipCode;
-        }
-        if (state) {
-            values.state = state;
-        }
-        if (_.size(values) === 0) {
-            return;
-        }
+
         if (props.inputID) {
             _.each(values, (value, key) => {
                 const inputKey = lodashGet(props.renamedInputKeys, key, key);
diff --git a/src/libs/GooglePlacesUtils.js b/src/libs/GooglePlacesUtils.js
index 5a875e7f10..8d4b00771b 100644
--- a/src/libs/GooglePlacesUtils.js
+++ b/src/libs/GooglePlacesUtils.js
@@ -9,16 +9,34 @@ import _ from 'underscore';
  *   types: [ "locality", "political" ]
  * }
  *
+ * The param types is an object where the key is the type to look for and the value is the format (long_name or short_name), e.g:
+ * {
+ *   country: 'long_name',
+ *   postal_code: 'short_name'
+ * }
+ *
  * @param {Array} addressComponents
- * @param {String} type
- * @param {String} key
- * @returns {String|undefined}
+ * @param {Object} types
+ * @param {Boolean} initWithEmpty
+ * @returns {Object}
  */
-function getAddressComponent(addressComponents, type, key) {
-    return _.chain(addressComponents)
-        .find(component => _.contains(component.types, type))
-        .get(key)
-        .value();
+function getAddressComponent(addressComponents, types, initWithEmpty = true) {
+    const address = {};
+
+    if (initWithEmpty) {
+        _.each(_.keys(types), type => address[type] = '');
+    }
+
+    const typesToFind = {...types};
+    _.each(addressComponents, (component) => {
+        const intersection = _.intersection(component.types, _.keys(typesToFind));
+        _.each(intersection, (key) => {
+            address[key] = component[typesToFind[key]];
+            delete typesToFind[key];
+        });
+    });
+
+    return address;
 }

 export {
diff --git a/tests/unit/GooglePlacesUtilsTest.js b/tests/unit/GooglePlacesUtilsTest.js
index 2993254a49..952b6ecd1d 100644
--- a/tests/unit/GooglePlacesUtilsTest.js
+++ b/tests/unit/GooglePlacesUtilsTest.js
@@ -30,10 +30,12 @@ describe('GooglePlacesUtilsTest', () => {
                     types: ['postal_code'],
                 },
             ];
-            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'sublocality', 'long_name')).toStrictEqual('Brooklyn');
-            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'administrative_area_level_1', 'short_name')).toStrictEqual('NY');
-            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'postal_code', 'long_name')).toStrictEqual('11206');
-            expect(GooglePlacesUtils.getAddressComponent(addressComponents, 'doesn-exist', 'long_name')).toStrictEqual(undefined);
+            expect(GooglePlacesUtils.getAddressComponent(addressComponents, {sublocality: 'long_name'})).toStrictEqual({sublocality: 'Brooklyn'});
+            expect(GooglePlacesUtils.getAddressComponent(addressComponents, {administrative_area_level_1: 'short_name'})).toStrictEqual({administrative_area_level_1: 'NY'});
+            expect(GooglePlacesUtils.getAddressComponent(addressComponents, {postal_code: 'long_name'})).toStrictEqual({postal_code: '11206'});
+            expect(GooglePlacesUtils.getAddressComponent(addressComponents, {doesnt_exist: 'long_name'})).toStrictEqual({doesnt_exist: ''});
+            expect(GooglePlacesUtils.getAddressComponent(addressComponents, {doesnt_exist: 'long_name'}, false)).toStrictEqual({});
+            expect(GooglePlacesUtils.getAddressComponent(addressComponents, {postal_code: 'long_name', country: 'short_name'})).toStrictEqual({postal_code: '11206', country: 'US'});
         });
     });
 });

Details

• Refactored the GooglePlacesUtils.getAddressComponent function so we can avoid the multiple functions calls and instead we make only one call and be done with it.
• Updated the tests

[bondydaa]

can we please hold off on any further proposals until @mananjadhav and I get on the same page? I think the existing solutions both will work and once we come to an agreement on which one to implement we'll assign it.

[mananjadhav]

Thanks for the comments @bondydaa. TL;DR, I am fine with both the approaches.

I am aware @bernhardoj's proposal will work, I've only two minor concerns:

1. We're assuming that the type of all fields will be string (At the moment they're all!)
2. To avoid transforming values from the library. Because Google Places is actually not sending the value, and we're transforming it.

As I mentioned, these are minor concerns, and considering we're using AddressSearch component for all places covers our needs for all the forms.

[roryabraham]

Sorry for the delay in responding @bondydaa.

just want to make sure I'm not missing a reason why we wouldn't want to fix this in the GooglePlacesUtils lib

None that I'm aware of.

[roryabraham]

FWIW I agree with @bondydaa's evaluation that this should be fixed by providing a default value in GooglePlacesUtils. @s77rt's last proposal looks pretty good to me because it simplifies AddressSearch nicely, but I think that the refactor in GooglePlacesUtils looks like it could be simplified to something like this:

/**
 * Finds an address component by type, and returns the value associated to key. Each address component object
 * inside the addressComponents array has the following structure:
 * {
 *   long_name: "New York",
 *   short_name: "New York",
 *   types: [ "locality", "political" ]
 * }
 *
 * @param {Array} addressComponents
 * @param {Object} fieldsToExtract – has shape: {fieldName: 'keyToUse'}
 * @returns {Object}
 */
function getAddressComponents(addressComponents, fieldsToExtract) {
    const result = _.mapObject(fieldsToExtract, () => '');
    _.each(addressComponents, (addressComponent) => {
        if (!_.isEmpty(result[addressComponent])) {
            return;
        }
        result[addressComponent] = lodashGet(addressComponent, fieldsToExtract[addressComponent], '');
    });
    return result;
}

[s77rt]

@roryabraham I didn't test your proposal but I don't see types being used anywhere so I'm not sure how would that work. I just tried to keep the same behaviour in case a type exists in many items such as political. The current behaviour is to return only the first one. Also we don't have _.mapObject included our underscore bundle.

[roryabraham]

I don't see types being used anywhere so I'm not sure how would that work

Oh yeah, looks like you're right and there are some problems with my refactor. Sorry about that. Still it be great to simplify this a bit if possible, but now I see why you were using _.intersection.

Also we don't have _.mapObject included our underscore bundle.

What makes you say that? It seems to work for me:

[s77rt]

underscore has mapObject but our bundle does not (maybe a diff version or we are using a slim version). You can test on web open console and type _.mapObject you will get undefined.

[bondydaa]

I see we use mapObject here https://github.com/Expensify/App/blob/17edc8253c992391c6937e9ba788679b898e99aa/src/libs/requireParameters.js#L22 and requireParameters is used a in few spots throughout the app so I'm sure mapObject exists, it may not be exposed on the window object though.

[s77rt]

Yes confirmed, we have it. Not sure why only some methods are available in the window though...

[melvin-bot[bot]]

📣 @bernhardoj You have been assigned to this job by @bondydaa! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[bondydaa]

okay given that we're now just somewhat discussing various ways to fix the GooglePlacesUtils lib I am going to assign @bernhardoj since they had initially proposed the fix there.

I agree with doing something like what Rory suggested here https://github.com/Expensify/App/issues/14324#issuecomment-1398821657

[s77rt]

@bondydaa You didn't check my proposal?

[bernhardoj]

@bondydaa hi, so should I open the PR with your suggestion here based on my proposal or doing the refactor based on @s77rt proposal?

If we want to do the refactor, I think @s77rt should be the one to take this.

[bondydaa]

I think Rory's suggestion comes closest to what we want (even if it needs some tweaking to work) https://github.com/Expensify/App/issues/14324#issuecomment-1398821657

The problem with the proposal here https://github.com/Expensify/App/issues/14324#issuecomment-1398468379 is that it has nested for-loops which will eventually be a bottleneck because as you add more items into either of the first 2 arguments it will take longer to process (O(n^2) I believe) and slow everything down.

That's why I assigned you since you were first to come up with the idea to fix this in the lib though we'll need to adjust any proposal/suggestion thus far during code reviews anyways.

If you don't want the job I can assign to @s77rt but whoever is assigned I would based your PR off of https://github.com/Expensify/App/issues/14324#issuecomment-1398821657 and add unit tests that call the getAddressComponents with a multi-key/value object as well as single key/value objects so we know it works both ways and also so we can eventually add a timer to monitor any execution speed regressions.

[s77rt]

@bondydaa; @roryabraham suggestion won't work as he forget the types as that's exactly why we need another nested loop. FWIW the loop will get smaller once we start finding some keys.

[bernhardoj]

Thanks for the answer @bondydaa, I really appreciate it. Obviously, I want the job 😅, but I don't think it is possible to do the refactor with only one loop because we need to iterate over the addressComponent types array and eventually we will use @s77rt refactor solution. Actually, our current code already "do" the nested loop.

We call getAddressComponent for each field, so the complexity will be addressComponentLength * fieldsToExtractLength (also O(n^2)). So, with or without refactor, the complexity will still be O(n^2). CMIIW.

If the refactor is not needed, I will open the PR straight away because it's been taking a few days after I've been assigned (sorry.) Otherwise, I am fine to give it to @s77rt to do the refactor.

[bondydaa]

Hmm I'll have to take another look tomorrow my brain is fried today.

If the choice is between using nested loops so we can pass an object of keys we want OR just going with your first proposal to default to an empty string and calling it multiple times, I'd prefer the latter though I do see your point that execution wise that maybe it's all the same.

[roryabraham]

Sorry about my last snippet not being correct and not accounting for types. I've tested this one and it seems to work as expected. It's basically the same as @s77rt's proposal, but I tried to make it a bit more readable (of course, readability subjective)...

/**
 * Finds an address component by type, and returns the value associated to key. Each address component object
 * inside the addressComponents array has the following structure:
 * {
 *   long_name: "New York",
 *   short_name: "New York",
 *   types: [ "locality", "political" ]
 * }
 *
 * @param {Array} addressComponents
 * @param {Object} fieldsToExtract – has shape: {addressType: 'keyToUse'}
 * @returns {Object}
 */
function getAddressComponents(addressComponents, fieldsToExtract) {
    const result = _.mapObject(fieldsToExtract, () => '');
    _.each(addressComponents, (addressComponent) => {
        _.each(addressComponent.types, (addressType) => {
            if (!_.has(fieldsToExtract, addressType) || !_.isEmpty(result[addressType])) {
                return;
            }
            result[addressType] = lodashGet(addressComponent, fieldsToExtract[addressType], '');
        });
    });
    return result;
}

Tests:

import * as GooglePlacesUtils from '../../src/libs/GooglePlacesUtils';

describe('GooglePlacesUtilsTest', () => {
    describe('getAddressComponents', () => {
        it('should find address components by type', () => {
            const addressComponents = [
                {
                    long_name: 'Bushwick',
                    short_name: 'Bushwick',
                    types: ['neighborhood', 'political'],
                },
                {
                    long_name: 'Brooklyn',
                    short_name: 'Brooklyn',
                    types: ['sublocality_level_1', 'sublocality', 'political'],
                },
                {
                    long_name: 'New York',
                    short_name: 'NY',
                    types: ['administrative_area_level_1', 'political'],
                },
                {
                    long_name: 'United States',
                    short_name: 'US',
                    types: ['country', 'political'],
                },
                {
                    long_name: '11206',
                    short_name: '11206',
                    types: ['postal_code'],
                },
            ];
            expect(GooglePlacesUtils.getAddressComponents(addressComponents, {
                sublocality: 'long_name',
                administrative_area_level_1: 'short_name',
                postal_code: 'long_name',
                'doesn-exist': 'long_name',
            })).toStrictEqual({
                sublocality: 'Brooklyn',
                administrative_area_level_1: 'NY',
                postal_code: '11206',
                'doesn-exist': '',
            });
        });
    });
});

---

If the choice is between using nested loops so we can pass an object of keys we want OR just going with your first proposal to default to an empty string and calling it multiple times

I agree this is prettymuch the choice we've got here 👍🏼

do see your point that execution wise that maybe it's all the same

yeah, I think this is true too. There might be slight differences but probably none that are really significant

[s77rt]

@bernhardoj Thanks for backing me up here

@roryabraham Thanks for the update, but I think my proposal will lead to less iterations over time due to the use of intersection and deleting keys that we found. Also isn't the use of lodashGet redundant here or maybe I'm missing something? Either way this will clean up AddressSearch significantly for little to no performance drop/(gain?), a good trade-off I would say.

[bondydaa]

Okay I implemented this without nested loops and then used performance.now() to roughly try and calculate execution time across the proposals and existing code.

built-ins (My PR)	nested loops (Proposals)	existing code
0.10000000894069672ms	0.19999998807907104ms	0.20000000298023224ms
0.09999999403953552ms	0.09999999403953552ms	0.09999999403953552ms
0.09999999403953552ms	0.19999998807907104ms	0.10000000894069672ms
0.10000000894069672ms	0.09999999403953552ms	0.09999999403953552ms
avg 0.100000001490116ms	avg 0.149999991059303ms	avg 0.125ms

You can see how I generated the values in this commit https://github.com/Expensify/App/pull/14577/commits/32b5f7e17bfdafee23c9f9862350199e2a0712b3

I just commented these lines to get the built-ins results https://github.com/Expensify/App/blob/2bfb3b771278b5271ef149074cc36228376a4471/src/libs/GooglePlacesUtils.js#L27-L35

and then commented out these lines to get the "proposals" times and uncommented the ones above https://github.com/Expensify/App/blob/2bfb3b771278b5271ef149074cc36228376a4471/src/libs/GooglePlacesUtils.js#L37-L52

I ran 4 different address look ups with each one, added them up and divided by 4 to get the avgs. It's pretty basic but gives us a good enough idea I think. (I actually had to run the built-ins more than that b/c more than once it was quick enough that it out put 0ms for execution so I tried to make it as fair as possible)

Ultimately we're talking about like half a ms here so I don't think it's the end of the world but I think it's important to note that we should avoid nested loops whenever possible and only allow them in the code base sparingly.

Another thing I wasn't aware of that @tgolen mentioned to me 1:1 was

⁦for sure avoid using lodashget in nested forloops. The thing is SLOW anytime I do nested stuff like that, I always go straight vanilla JS. Even those _.each() are slow

So I would also advice to be weary of those underscore methods too.

Since I've basically already written this code I'm just going to make this internal and finish off the PR.

[melvin-bot[bot]]

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

[bondydaa]

Okay updated this code a bit more after talking with Tim 1:1, decided to update the tests a little bit and run each different solution 100 times to see how quick they'd execute.

TL:DR I was wrong 🤦 . Sorry to doubt ya @s77rt @roryabraham @bernhardoj, I'm going to update my PR to implement Rory's solution and I think we should still pay @s77rt out since his solution was based on your proposal here https://github.com/Expensify/App/issues/14324#issuecomment-1398468379

Explanation of tests: I compared a standard object which had 4 key/values https://github.com/Expensify/App/blob/0dd824e525687af658d54e08846c1c07904739f7/tests/unit/GooglePlacesUtilsTest.js#L2-L7 and a big object which had 84 key/values https://github.com/Expensify/App/blob/0dd824e525687af658d54e08846c1c07904739f7/tests/unit/GooglePlacesUtilsTest.js#L9-L94

To my surprise the code using underscore is actually quicker with the big object (probably because it's using some memorization under the hood) though it's not as quick on the standard object but again it's so negligible that it probably doesn't matter.

    addressComponents standard object
    Call to looping 100 times took 0.17708299999981136ms

    addressComponents Big Object
    Call to looping 100 times took 65.26604199999997ms

    addressComponentsNested standard object
    Call to looping 100 times took 0.19320799999991323ms

    addressComponentsNested Big Object
    Call to looping 100 times took 2.5833749999999327ms

    addressComponentsUnderscore standard object
    Call to looping 100 times took 0.45087499999999636ms

    addressComponentsUnderscore Big Object
    Call to looping 100 times took 1.211708999999928ms

Here's the code for how I ran this https://github.com/Expensify/App/blob/0dd824e525687af658d54e08846c1c07904739f7/tests/unit/GooglePlacesUtilsTest.js#L171-L184

the addressComponents method is the current iteration trying to get rid of nested loops all together https://github.com/Expensify/App/blob/0dd824e525687af658d54e08846c1c07904739f7/src/libs/GooglePlacesUtils.js#L65-L80

addressComponentsNested is the code I'd pushed yesterday: https://github.com/Expensify/App/blob/0dd824e525687af658d54e08846c1c07904739f7/src/libs/GooglePlacesUtils.js#L41-L63

And addressComponentsUnderscore is Rory's code (which was based on @s77rt's proposal): https://github.com/Expensify/App/blob/0dd824e525687af658d54e08846c1c07904739f7/src/libs/GooglePlacesUtils.js#L25-L39

I could probably keep trying to tweak addressComponents as I've written it to use memorization as well and maybe it'd eventually get it to execute quicker but ultimately that doesn't seem like a good use of time.

[bondydaa]

discussing this here https://expensify.slack.com/archives/C01GTK53T8Q/p1675200087387539

[bondydaa]

updated and published my PR after we came to a resolution on that thread above

[mananjadhav]

Just getting back from OOO, I can get this reviewed by tomorrow

[mananjadhav]

Just getting back from OOO, I can get this reviewed by tomorrow

[MelvinBot]

@mananjadhav, @bondydaa, @CortneyOfstad Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[bondydaa]

PR was deployed to staging yesterday