Updated country and state searches to fix extra spaces + support searching with/without non alphabet chars in country name by huzaifa-99 · Pull Request #24140 · Expensify/App

huzaifa-99 · 2023-08-04T00:30:54Z

Details

Fixed Issues

$ #23727
PROPOSAL: #23727 (comment)

Tests

Go to settings > profile > personal details > home address
Go to the country selector
Try out different searches with/without 1 or more than 1 spaces in the start, middle or end of the search term -- Verify that it works
Try out different searches with a search term with/without brackets ex: Myanmar (Burma), Cocos (Keeling) Islands -- Verify that it works
Try out different searches with a search term with/without dots ex: St. Lucia, U.S Virgin Islands -- Verify that it works
Try out different searches with a search term with/without dashes ex: Congo - Kinshasa, Congo - Brazzaville, Timor-Leste -- Verify that it works
Try out different searches with a search term with/without ampersand ex: St. Kitts & Nevis, St. Vincent & Grenadines -- Verify that it works
Try out different searches with a search term with/without single quote ex: Côte d'Ivoire -- Verify that it works
Try out different searches with country code ex: AF -> Afganistan, US -> United States, DZ -> Argeliz -- Verify that it works
Now go back and go to state selector
Try out different searches with/without 1 or more than 1 spaces in the start, middle or end of the search term -- Verify that it works
Try out different searches with state code ex: NY -> New York, DC -> District Of Columbia -- Verify that it works

Verify that no errors appear in the JS console

Offline tests

Same as "Tests" section above

QA Steps

Same as "Tests" section above

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Chrome:

Web.Chrome.mp4

Safari:

Web.Safari.mp4

Mobile Web - Chrome

mWeb.Chrome.mp4

Mobile Web - Safari

mWeb.Safari.mp4

Desktop

Desktop.Native.mp4

iOS

IOS.Native.mp4

Android

Android.Native.mp4

…phabet chars from country name

melvin-bot · 2023-08-04T00:30:58Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

melvin-bot · 2023-08-04T00:30:59Z

@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

huzaifa-99 · 2023-08-04T00:31:43Z

    }

-    return _.filter(data, (country) => country.text.toLowerCase().includes(searchValue.toLowerCase()));
+    return _.filter(data, (country) => country.searchValue.includes(searchValueWithOnlyAlphabets) || country.value.toLowerCase().includes(searchValueWithOnlyAlphabets));


From here

I think we could also add the ability to search for a country by entering a country code, it's reasonable for users to expect this to work in a country picker

Searching for both country name and code now

huzaifa-99 · 2023-08-04T00:35:28Z

 function filterOptions(searchValue, data) {
-    const trimmedSearchValue = searchValue.trim();
-    if (trimmedSearchValue.length === 0) {
+    const searchValueWithOnlyAlphabets = searchValue.toLowerCase().replaceAll(' ', '');


The USA States only had spaces in names and no other special char (in my tests), so only replaced empty spaces. Plz LMK if there is anything missing here

I think we can use the same logic here too, in essence, it should work even if there are states with special characters

Sounds good, will update that.

Done, now both country/state search work the same (we can probably make a utility out of it)

huzaifa-99 · 2023-08-04T00:36:17Z

    }

-    return _.filter(data, (country) => country.text.toLowerCase().includes(searchValue.toLowerCase()));
+    return _.filter(data, (countryState) => countryState.searchValue.includes(searchValueWithOnlyAlphabets) || countryState.value.toLowerCase().includes(searchValueWithOnlyAlphabets));


Searching for both state name and code, same reason as this

huzaifa-99 · 2023-08-04T16:24:51Z

Gentle bump for review @eVoloshchak

huzaifa-99 · 2023-08-05T07:36:30Z

Bump for review @eVoloshchak

huzaifa-99 · 2023-08-06T11:10:37Z

Gentle bump for review @eVoloshchak

huzaifa-99 · 2023-08-07T05:18:15Z

Bump for review @eVoloshchak

eVoloshchak · 2023-08-07T15:05:23Z

Reviewing today

eVoloshchak · 2023-08-07T18:43:55Z

        CARD_EXPIRATION_DATE: /^(0[1-9]|1[0-2])([^0-9])?([0-9]{4}|([0-9]{2}))$/,
        PAYPAL_ME_USERNAME: /^[a-zA-Z0-9]{1,20}$/,
        ROOM_NAME: /^#[a-z0-9à-ÿ-]{1,80}$/,
+        COUNTRY_NAME_WITH_ONLY_ALPHABETS: /[-'().&\s]/g,


Let's do this the other way around and exclude all non-alphabetic characters. We have to also remember there's a Spanish locale

Suggested change

COUNTRY_NAME_WITH_ONLY_ALPHABETS: /[-'().&\s]/g,

NON_ALPHABETIC_AND_NON_LATIN_CHARS: /[^a-zA-ZÀ-ÿ]/g,

Thanks for this, Implemented

eVoloshchak · 2023-08-07T18:49:12Z

    }

-    return _.filter(data, (country) => country.text.toLowerCase().includes(searchValue.toLowerCase()));
+    return _.filter(data, (country) => country.searchValue.includes(searchValueWithOnlyAlphabets) || country.value.toLowerCase().includes(searchValueWithOnlyAlphabets));


This can be reduced to

Suggested change

return _.filter(data, (country) => country.searchValue.includes(searchValueWithOnlyAlphabets) || country.value.toLowerCase().includes(searchValueWithOnlyAlphabets));

return _.filter(data, (country) => `${country.value}${country.searchValue}`.toLowerCase().includes(trimmedSearchValue));

But even then, I think we can improve the search itself. How can we "prioritize" searching by contry code so that, for instance, United States would be the first item when you enter US?

We can sort it after filtering

_.sortBy(filteredData, (country) => country.value.toLowerCase() === searchValueWithOnlyAlphabets ? -1 : 1);

eVoloshchak · 2023-08-07T18:54:04Z

                keyForList: countryISO,
                text: countryName,
                isSelected: currentCountry === countryISO,
+                searchValue: countryName.toLowerCase().replaceAll(CONST.REGEX.COUNTRY_NAME_WITH_ONLY_ALPHABETS, ''),


This isn't needed

Apologies, nevermind, this is necessary
Removing it would break cases with countries that have special characters in their name
(but toLowerCase() isn't needed anymore)

Actually, I added it here because if we store it in the state we won't have to recalculate it. But I will remove it if you think it's not necessary.

I added it here because if we store it in the state we won't have to recalculate it.

Fair enough, let's leave it here

eVoloshchak · 2023-08-07T18:55:20Z

 function filterOptions(searchValue, data) {
-    const trimmedSearchValue = searchValue.trim();
-    if (trimmedSearchValue.length === 0) {
+    const searchValueWithOnlyAlphabets = searchValue.toLowerCase().replaceAll(' ', '');


I think we can use the same logic here too, in essence, it should work even if there are states with special characters

eVoloshchak · 2023-08-07T19:01:56Z

 function filterOptions(searchValue, data) {
-    const trimmedSearchValue = searchValue.trim();
-    if (trimmedSearchValue.length === 0) {
+    const searchValueWithOnlyAlphabets = searchValue.toLowerCase().replaceAll(CONST.REGEX.COUNTRY_NAME_WITH_ONLY_ALPHABETS, '');


I think trimmedSearchValue is fine

Hmmm, I think we have to keep it since most searches won't work if we remove ex: U.S Virgin Islands -> no results

I mean just the name, so it looks like this

const trimmedSearchValue = searchValue.toLowerCase().replaceAll(CONST.REGEX.NON_ALPHABETIC_AND_NON_LATIN_CHARS, '');

Oh okay, sry I misunderstood.

huzaifa-99 · 2023-08-07T20:19:48Z

~~@eVoloshchak Applied the feedback. Should I update test videos (we updated the search to prioritize country/state code)?~~

I updated test videos to include the new search to prioritize country/state code case

eVoloshchak · 2023-08-07T20:48:44Z

-function filterOptions(searchValue, data) {
-    const trimmedSearchValue = searchValue.trim();
-    if (trimmedSearchValue.length === 0) {
+function searchOptions(searchValue, data) {


Are searchOptions identical for CountrySelectorModal and StateSelectorModal? If so, we could move this function to libs/CountrySelectorUtils

Yes, searchOptions logic is the same for both CountrySelectorModal and StateSelectorModal (other than some variable name and comments), I will move it to libs/CountrySelectorUtils

eVoloshchak · 2023-08-07T21:22:11Z

Since this exports a single item, let's just call it searchCountryOptions.js (a single file, no need for a folder)

eVoloshchak · 2023-08-07T21:22:50Z

+import CONST from '../../CONST';
+
+/**
+ * Searches the countriesData and returns sorted results based on country code


Suggested change

* Searches the countriesData and returns sorted results based on country code

* Searches the countries or states data and returns sorted results based on the search query

eVoloshchak · 2023-08-07T21:23:29Z

+ * Searches the countriesData and returns sorted results based on country code
+ * @param {String} searchValue
+ * @param {Object[]} countriesData - An array of country data objects
+ * @returns {Object[]} An array of sorted country data based on country code


Suggested change

* @returns {Object[]} An array of sorted country data based on country code

* @returns {Object[]} An array of countries/states sorted based on the search query

eVoloshchak · 2023-08-07T21:25:19Z

+ * @param {Object[]} countriesData - An array of country data objects
+ * @returns {Object[]} An array of sorted country data based on country code
+ */
+function searchOptions(searchValue, countriesData) {


Suggested change

function searchOptions(searchValue, countriesData) {

function searchCountryOptions(searchValue, countriesData) {

huzaifa-99 · 2023-08-07T21:34:01Z

Applied the change @eVoloshchak

eVoloshchak · 2023-08-07T21:39:29Z

 import HeaderWithBackButton from '../HeaderWithBackButton';
 import SelectionListRadio from '../SelectionListRadio';
 import Modal from '../Modal';
+import searchOptions from '../../libs/searchCountryOptions';


Suggested change

import searchOptions from '../../libs/searchCountryOptions';

import searchCountryOptions from '../../libs/searchCountryOptions';

eVoloshchak · 2023-08-07T21:39:46Z

 import HeaderWithBackButton from '../HeaderWithBackButton';
 import SelectionListRadio from '../SelectionListRadio';
 import useLocalize from '../../hooks/useLocalize';
+import searchOptions from '../../libs/searchCountryOptions';


Suggested change

import searchOptions from '../../libs/searchCountryOptions';

import searchCountryOptions from '../../libs/searchCountryOptions';

I had a thought about this too but searchCountryOptions in StateSelectorModal.js sounded a little misleading 😅. Anyways updating it as well.

huzaifa-99 · 2023-08-07T21:46:34Z

Updated @eVoloshchak

eVoloshchak · 2023-08-07T21:50:24Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-08-08.at.00.55.15.mov

Mobile Web - Chrome

screen-20230808-010553.mp4

Mobile Web - Safari

Screen.Recording.2023-08-08.at.00.59.25.mov

Desktop

Screen.Recording.2023-08-08.at.00.57.10.mov

iOS

There is an issue preventing me from logging in on iOS (endless spinner after entering the magic code). There is no platform-specific code, should work the same on iOS

Android

screen-20230808-010359.mp4

eVoloshchak

LGTM!
@huzaifa-99, thanks for the patience and quick turnaround time!

huzaifa-99 · 2023-08-07T22:21:27Z

LGTM! @huzaifa-99, thanks for the patience and quick turnaround time!

Always welcomed @eVoloshchak. And thank you for the nice suggestions and quick feedback!

OSBotify · 2023-08-07T22:30:02Z

✋ 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 · 2023-08-09T09:11:50Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀

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

OSBotify · 2023-08-09T09:50:33Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀

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

OSBotify · 2023-08-10T20:08:59Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀

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

huzaifa-99 added 4 commits August 4, 2023 02:33

Update country search to work with leading/trailing spaces and non-al…

7260fa1

…phabet chars from country name

Allow countries to be searched by country code

3d5d105

Update state search to work with leading/trailing spaces

75e5d78

Allow country states to be searched by state code

1a37a3e

huzaifa-99 requested a review from a team as a code owner August 4, 2023 00:30

melvin-bot bot requested review from eVoloshchak and removed request for a team August 4, 2023 00:30

huzaifa-99 commented Aug 4, 2023

View reviewed changes

huzaifa-99 mentioned this pull request Aug 4, 2023

[HOLD for payment 2023-08-17] [$1000] Web - Country suggestion does not work with head or trail space #23727

Closed

6 tasks

eVoloshchak suggested changes Aug 7, 2023

View reviewed changes

eVoloshchak reviewed Aug 7, 2023

View reviewed changes

Update state+country search to prioritize state code

536048a

Updated variable name

9013f9d

eVoloshchak reviewed Aug 7, 2023

View reviewed changes

huzaifa-99 added 2 commits August 8, 2023 02:04

Moved country/state search options to utils

c0d7504

Fix lint

3873f13

eVoloshchak suggested changes Aug 7, 2023

View reviewed changes

Moved CountrySelectorUtils to searchCountryOptions

28e5f9c

eVoloshchak reviewed Aug 7, 2023

View reviewed changes

Updated import name

09f3579

eVoloshchak approved these changes Aug 7, 2023

View reviewed changes

melvin-bot bot requested a review from marcaaron August 7, 2023 22:17

marcaaron assigned huzaifa-99 Aug 7, 2023

marcaaron approved these changes Aug 7, 2023

View reviewed changes

marcaaron merged commit 0dcb699 into Expensify:main Aug 7, 2023

OSBotify mentioned this pull request Aug 9, 2023

Deploy Checklist: New Expensify 2023-08-09 #24307

Closed

87 tasks

lanitochka17 mentioned this pull request Aug 9, 2023

[HOLD for payment 2023-08-28] [$1000] Profile - Entering country without a diacritic for Côte D'ivoire returns no suggestions #24344

Closed

6 tasks

	COUNTRY_NAME_WITH_ONLY_ALPHABETS: /[-'().&\s]/g,
	NON_ALPHABETIC_AND_NON_LATIN_CHARS: /[^a-zA-ZÀ-ÿ]/g,

	return _.filter(data, (country) => country.searchValue.includes(searchValueWithOnlyAlphabets) \|\| country.value.toLowerCase().includes(searchValueWithOnlyAlphabets));
	return _.filter(data, (country) => `${country.value}${country.searchValue}`.toLowerCase().includes(trimmedSearchValue));

	* Searches the countriesData and returns sorted results based on country code
	* Searches the countries or states data and returns sorted results based on the search query

	* @returns {Object[]} An array of sorted country data based on country code
	* @returns {Object[]} An array of countries/states sorted based on the search query

	function searchOptions(searchValue, countriesData) {
	function searchCountryOptions(searchValue, countriesData) {

	import searchOptions from '../../libs/searchCountryOptions';
	import searchCountryOptions from '../../libs/searchCountryOptions';

Conversation

huzaifa-99 commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Aug 4, 2023

Uh oh!

melvin-bot bot commented Aug 4, 2023

Uh oh!

huzaifa-99 Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

huzaifa-99 commented Aug 4, 2023

Uh oh!

huzaifa-99 commented Aug 5, 2023

Uh oh!

huzaifa-99 commented Aug 6, 2023

Uh oh!

huzaifa-99 commented Aug 7, 2023

Uh oh!

eVoloshchak commented Aug 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huzaifa-99 Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eVoloshchak Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

eVoloshchak Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

huzaifa-99 commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huzaifa-99 commented Aug 4, 2023 •

edited

Loading

huzaifa-99 Aug 4, 2023 •

edited

Loading

huzaifa-99 Aug 7, 2023 •

edited

Loading

eVoloshchak Aug 7, 2023 •

edited

Loading

eVoloshchak Aug 7, 2023 •

edited

Loading

huzaifa-99 commented Aug 7, 2023 •

edited

Loading

eVoloshchak commented Aug 7, 2023 •

edited

Loading

huzaifa-99 commented Aug 7, 2023 •

edited

Loading