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

Allow spaces and special chars in phone number username #6293

Merged
merged 6 commits into from
Nov 18, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Nov 14, 2021

Details

#6227 (comment)

Fixed Issues

$ #6227

Tests | QA Steps

  1. Navigate to NewDot
  2. Input a phone number as login using any of symbols -, (, ) or spaces (e.g. +1 (222) 222 2222)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner November 14, 2021 12:04
@MelvinBot MelvinBot requested review from chiragsalian and removed request for a team November 14, 2021 12:04
Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

Code looks good to me. But let's not forget to place this same logic in the request a call modal,
image

Steps: Go to Concierge chat -> Click the phone icon on the top right -> Enter a valid phone number with spaces and special characters. Confirm it works fine.

@parasharrajat
Copy link
Member Author

@chiragsalian Added the same for RequestCallPage as well.

Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

Code LGTM and works well. Left a minor suggestion for improvement.

@@ -106,13 +106,13 @@ class RequestCallPage extends Component {
Growl.error(this.props.translate('requestCallPage.growlMessageNoPersonalPolicy'), 3000);
return;
}

const phoneNumber = this.state.phoneNumber.replace(/((?!\n)[()-\s\t])/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

The regex in this file has been repeated 4 times. It seems pretty useful as well so how about moving it to a lib file and calling it via a method like getPhoneNumberWithoutSpecialChars that would do the replace as well, or moving the regex to CONST. This way we can DRY up the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

Nice, thank you for the changes.

@chiragsalian chiragsalian merged commit e1a8662 into Expensify:main Nov 18, 2021
@OSBotify
Copy link
Contributor

✋ 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
Copy link
Contributor

🚀 Deployed to staging by @chiragsalian in version: 1.1.15-18 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

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.

3 participants