-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
I completed the validations in Do |
There was a problem hiding this 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.
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? |
Update: Changed |
There was a problem hiding this 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!
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', |
There was a problem hiding this comment.
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.
Update: Moved functions |
Seems unrelated to your changes, @aldo-expensify, try merging the main branch into your PR, it might fix that issue. |
…-for-missing-locality
@TomatoToaster thanks!, that worked :) |
There was a problem hiding this 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.
Update:
|
There was a problem hiding this 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 😅
Also lint failure ^ |
haha looking at that |
Ok, seems like the automatic merge was leaving the |
Testing a feature of https://github.com/refined-github/refined-github: Hopefully if E2E tests pass it will self-merge 🤞 |
nice 🤞 |
If it doesn't, feel free to self-merge. |
🚀 Deployed to staging by @aldo-expensify in version: 1.1.8-11 🚀
|
🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀
|
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
64 Noll Street, Brooklyn, NY, USA
and select the first (and only) resultTested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android