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

IOU Invalid Phone number error handled #8019

Merged
merged 5 commits into from
Mar 18, 2022

Conversation

Santhosh-Sellavel
Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel commented Mar 5, 2022

Details

Fixed Issues

$ #7688

Tests

  1. Click on FAB ➕ Button -> Split Bill
  2. Enter Amount and press Next
  3. Search and select a three-digit phone number
  4. Click Next and press the Split Bill button
  5. You should see the following error message for the phone number
    Please enter a valid phone number, with the country code (e.g. +1234567890)

Success case:

  1. Now press next.
  2. Deselect the invalid number.
  3. Search and select a valid phone number (Ex: +1234567890)
  4. Click Next and press the Split Bill button
  5. Split should happen successfully and open to the chat!
  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I verified the PR has a small number of commits behind main
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I included screenshots or videos for tests on all platforms
  • I ran the tests & verify they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
    • I followed the JSDocs style guidelines (in STYLE.md)
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I corroborated the UI performance was not affected (the performance is the same than main branch)
  • If I created a new component I verified that a similar component doesn't exist in the codebase

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • 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 testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

QA Steps

  1. Click on FAB ➕ Button -> Split Bill
  2. Enter Amount and press Next
  3. Search and select a three-digit phone number
  4. Click Next and press the Split Bill button
  5. You should see the following error message for the phone number
    Please enter a valid phone number, with the country code (e.g. +1234567890)
  • Verify that no errors appear in the JS console

Success case:

  1. Now press next.
  2. Deselect the invalid number.
  3. Search and select a valid phone number (Ex: +1234567890)
  4. Click Next and press the Split Bill button
  5. Split should happen successfully and open to the chat!

Screenshots

Web

IOU_Error_Web.mov

Mobile Web

IOU_Error_mWeb.mp4

Desktop

IOU_Error_Desktop.mov

iOS

IOU_Error_iOS.mp4

Android

IOU_Error_Android.mov

@Santhosh-Sellavel Santhosh-Sellavel marked this pull request as ready for review March 5, 2022 17:38
@Santhosh-Sellavel Santhosh-Sellavel requested a review from a team as a code owner March 5, 2022 17:38
@MelvinBot MelvinBot removed the request for review from a team March 5, 2022 17:38
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

@Santhosh-Sellavel Looks great, some minor formatting requests

src/libs/API.js Outdated Show resolved Hide resolved
src/libs/API.js Outdated Show resolved Hide resolved
src/libs/API.js Outdated Show resolved Hide resolved
Typo

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

@iwiznia LGTM! 🎉

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 7, 2022

Commenting since I can't edit the PR

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • 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 testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

@Santhosh-Sellavel
Copy link
Collaborator Author

@iwiznia @rushatgabhane bump!

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 15, 2022

@Santhosh-Sellavel PR is already approved, let's merge main into your branch for good measure

@Santhosh-Sellavel
Copy link
Collaborator Author

@iwiznia @rushatgabhane updated with main brach

@iwiznia
Copy link
Contributor

iwiznia commented Mar 17, 2022

image

Can you test the success case (successfully adding a phone number), add it to the list of tests and mark that checkbox please @Santhosh-Sellavel?

@Santhosh-Sellavel
Copy link
Collaborator Author

Updated Tests @iwiznia.

@iwiznia
Copy link
Contributor

iwiznia commented Mar 18, 2022

Awesome thanks!

@iwiznia iwiznia merged commit 75e6dbd into Expensify:main Mar 18, 2022
@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.

@@ -179,8 +179,8 @@ Network.registerResponseHandler((queuedRequest, response) => {
return;
}

if (response.jsonCode === 405 || response.jsonCode === 404) {
// IOU Split & Request money transactions failed due to invalid amount(405) or unable to split(404)
if (response.jsonCode === 405 || response.jsonCode === 404 || response.jsonCode === 402) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Santhosh-Sellavel We must stop rejecting the promise here. ANY API command that returns this error code will lead to an unhandled promise rejection unless it is caught...

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixing it in this PR #8087 already but got conflicts because the bad practice continues 😅

Copy link
Collaborator Author

@Santhosh-Sellavel Santhosh-Sellavel Mar 22, 2022

Choose a reason for hiding this comment

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

Sorry, I thought they were specific to this IOU Flow. Will keep a note on this @marcaaron.

@@ -66,6 +66,8 @@ function getIOUErrorMessage(error) {
return Localize.translateLocal('common.error.invalidAmount');
} if (error.jsonCode === 404) {
return Localize.translateLocal('iou.error.invalidSplit');
} if (error.jsonCode === 402) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Put if statements on new lines please

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @iwiznia in version: 1.1.46-0 🚀

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

@roryabraham
Copy link
Contributor

Is there a reason we don't validate phone numbers in the front-end?

@rushatgabhane
Copy link
Member

@roryabraham it'll require using some lib like google/libphonenumber. But it isn't really required because the backend already validates it for us.

all details here - #7688 (comment)

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @timszot in version: 1.1.46-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 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.

6 participants