Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Use sublocality when locality is missing - AddressSearch #5950

Merged
merged 12 commits into from
Oct 23, 2021

Conversation

aldo-expensify
Copy link
Contributor

Details

There are addresses that have the component "locality" missing. We use the "locality" as the city. This PR will make us use the "sublocality" as the city when the "locality" is missing.

Fixed Issues

$ #5867

Tests / QA Steps

  1. Create account in NewDot, verify it and log in
  2. Create Workspace
  3. Start add VBA flow and get to the "Company information" step (/bank-account/company)
  4. Input in the "Company address" field: 64 Noll Street, Brooklyn, NY, USA and select the first (and only) result
  5. Fill the rest of the fields
  6. You should be able to continue to the next step without errors.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@aldo-expensify aldo-expensify requested a review from a team as a code owner October 19, 2021 18:02
@MelvinBot MelvinBot requested review from danieldoglas and removed request for a team October 19, 2021 18:02
@aldo-expensify
Copy link
Contributor Author

I completed the validations in validateAddressComponents because if an address has some missing fields, this gives a little better feedback for the user.

Do console.debug logging eventually reach our logging servers?

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Do console.debug logging eventually reach our logging servers?

Nope, please use the Log library. See this PR.

@aldo-expensify
Copy link
Contributor Author

Nope, please use the Log library. See this PR.

Thanks, I would think these logs are interesting enough to be sent to the server. They may eventually reveal other problematic cases that we don't know about, sounds correct?

@aldo-expensify
Copy link
Contributor Author

Update: Changed console.debug for Log.hmmm

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Log changes look good. Let's add some unit tests for this!

src/components/AddressSearch.js Outdated Show resolved Hide resolved
@aldo-expensify
Copy link
Contributor Author

Log changes look good. Let's add some unit tests for this!

Thanks for the review, I'll be looking into the testing stuff tomorrow to finish this up!

types: ['postal_code'],
},
],
formatted_address: 'Quail Ridge Rd, Escondido, CA 92027, USA',
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 left this here even if it not used in the tests just as a way to remember these addresses that gave up problems.

@aldo-expensify
Copy link
Contributor Author

Update: Moved functions validateAddressComponents and getAddressComponent to lib/GooglePlacesUtils.js, added some tests for them in GooglePlacesUtilsTest.js

@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Oct 21, 2021

hmm, any ideas on why this could be happening?

Screen Shot 2021-10-20 at 5 54 49 PM

@TomatoToaster
Copy link
Contributor

Seems unrelated to your changes, @aldo-expensify, try merging the main branch into your PR, it might fix that issue.

@aldo-expensify
Copy link
Contributor Author

@TomatoToaster thanks!, that worked :)

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests, that's huge. Just had some minor suggestions.

src/components/AddressSearch.js Outdated Show resolved Hide resolved
src/components/AddressSearch.js Outdated Show resolved Hide resolved
src/libs/GooglePlacesUtils.js Outdated Show resolved Hide resolved
src/libs/GooglePlacesUtils.js Outdated Show resolved Hide resolved
@aldo-expensify
Copy link
Contributor Author

Update:

  • Reorganized export
  • Replace arrow functions for regular functions
  • Add jsDoc to functions in GooglePlacesUtils
  • Add [AddressSearch] tag in Log.hmmm

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Sorry, one last thing 😅

src/libs/GooglePlacesUtils.js Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor

Also lint failure ^

@aldo-expensify
Copy link
Contributor Author

Also lint failure ^

haha looking at that

@aldo-expensify
Copy link
Contributor Author

Also lint failure ^

Ok, seems like the automatic merge was leaving the import _ from 'underscore'; out 🤷

@roryabraham
Copy link
Contributor

Testing a feature of https://github.com/refined-github/refined-github:

image

Hopefully if E2E tests pass it will self-merge 🤞

@aldo-expensify
Copy link
Contributor Author

Hopefully if E2E tests pass it will self-merge 🤞

nice 🤞

@roryabraham
Copy link
Contributor

roryabraham commented Oct 22, 2021

If it doesn't, feel free to self-merge.

@aldo-expensify aldo-expensify merged commit e1ec61d into main Oct 23, 2021
@aldo-expensify aldo-expensify deleted the aldo_sublocality-for-missing-locality branch October 23, 2021 00:31
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @aldo-expensify in version: 1.1.8-11 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants