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

[HOLD for payment 2021-09-22] Concierge - Request call without phone number leads to infinite loading. In mobile, app crashes #4861

Closed
isagoico opened this issue Aug 26, 2021 · 20 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Aug 26, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Upwork Post: https://www.upwork.com/jobs/~0198bddf350b2411a7


Action Performed:

  1. Go to https://staging.new.expensify.com
  2. Log in with any account
  3. Search for Concierge
  4. Click on Call icon
  5. Don't write anything on phone number and click call

Expected Result:

Error message only should appears

Actual Result:

Request call without phone number leads for infinite loading

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.0.88-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation
In mobile, app crashes.

Bug5210709_Recording__304.mp4

image

Expensify/Expensify Issue URL:

View all open jobs on GitHub

@MelvinBot
Copy link

Triggered auto assignment to @MariaHCD (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Aug 27, 2021

Proposal

We need to add a validation here for the phone number also and show here a user-friendly message using growl like we already do for the firstName & lastName!

onSubmit() {
this.setState({isLoading: true});
if (!this.state.firstName.length || !this.state.lastName.length) {
Growl.success(this.props.translate('requestCallPage.growlMessageEmptyName'));
this.setState({isLoading: false});
return;
}
const personalPolicy = _.find(this.props.policies, policy => policy.type === CONST.POLICY.TYPE.PERSONAL);
if (!personalPolicy) {
Growl.error(this.props.translate('requestCallPage.growlMessageNoPersonalPolicy'), 3000);
return;
}

Also, we should allow users to type only Numbers and plus. So we should use CONST.REGEX.DIGITS_AND_PLUS regex and validate onChange.

<ExpensiTextInput
label={this.props.translate('common.phoneNumber')}
autoCompleteType="off"
autoCorrect={false}
value={this.state.phoneNumber}
placeholder="+14158675309"
onChangeText={phoneNumber => this.setState({phoneNumber})}
/>

Demo of an error Message:

PleaseEnterPhoneNumber.mov

Used success type growl notification which is already used for name validation also, we can change both to error type if needed thanks!

@MariaHCD MariaHCD added the External Added to denote the issue can be worked on by a contributor label Aug 27, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Jag96 (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MariaHCD MariaHCD removed their assignment Aug 27, 2021
@parasharrajat
Copy link
Member

I tested out this component. I didn't find infinite loading. On clicking save without phone number, just throw the backend error which seems fine to me.

But looking at the video, it seems the error is at the

const personalPolicy = _.find(this.props.policies, policy => policy.type === CONST.POLICY.TYPE.PERSONAL);
.

It is possible that the policy is empty for the user.

Proposal

Not making this simple component complicated as we already have a backend validation message in place when the phone number is not present or invalid, I would suggest

  1. Disable the Call me button until all the fields are filled.
  2. Add the check for the policy before checking policy.type so that it does not throw an error as shown in the video i.e. just using lodash.get.
  3. If we want to validate the number in the front end before submitting. we can show the error/red outline on the phone input if the number is invalid using the errorText prop in ExpensiInput.

@Jag96 Jag96 added the Exported label Aug 27, 2021
@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 27, 2021
@Jag96
Copy link
Contributor

Jag96 commented Aug 27, 2021

Posted to upwork here: https://www.upwork.com/jobs/~0198bddf350b2411a7

@parasharrajat your proposal looks good, good eye on the policy error in the video. I've invited you to the Upwork job, once you accept feel free to start on a PR and I'll assign you.

@parasharrajat
Copy link
Member

Please assign this issue to me. Thanks.

@isagoico
Copy link
Author

Issue reproducible during KI retests

@MelvinBot
Copy link

@Jag96 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 30, 2021
@Jag96 Jag96 added Weekly KSv2 and removed Daily KSv2 labels Aug 30, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 30, 2021
@isagoico
Copy link
Author

isagoico commented Sep 5, 2021

Issue reproducible during KI retests

@parasharrajat
Copy link
Member

I am on vacation. back on 8.

@botify
Copy link

botify commented Sep 10, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@Jag96 Jag96 added the Reviewing Has a PR in review label Sep 10, 2021
@isagoico
Copy link
Author

Issue reproducible during KI retests.

@parasharrajat
Copy link
Member

parasharrajat commented Sep 13, 2021

@isagoico Could you please a screenshot of the error? Oops, it is not yet moved to staging. Sorry.

@botify
Copy link

botify commented Sep 13, 2021

🚀 Deployed to staging by @Jag96 in version: 1.0.97-1 🚀

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

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Sep 15, 2021
@botify botify changed the title Concierge - Request call without phone number leads to infinite loading. In mobile, app crashes [HOLD for payment 2021-09-22] Concierge - Request call without phone number leads to infinite loading. In mobile, app crashes Sep 15, 2021
@botify
Copy link

botify commented Sep 15, 2021

🚀 Deployed to production by @francoisl in version: 1.0.98-1 🚀

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

@isagoico
Copy link
Author

Issue not reproducible during KI retests. (Fixed 🎉)

@botify botify added Weekly KSv2 and removed Weekly KSv2 labels Sep 20, 2021
@botify botify changed the title [HOLD for payment 2021-09-22] Concierge - Request call without phone number leads to infinite loading. In mobile, app crashes [HOLD for payment 2021-09-27] [HOLD for payment 2021-09-22] Concierge - Request call without phone number leads to infinite loading. In mobile, app crashes Sep 20, 2021
@botify
Copy link

botify commented Sep 20, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.0.98-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2021-09-27. 🎊

@mountiny mountiny changed the title [HOLD for payment 2021-09-27] [HOLD for payment 2021-09-22] Concierge - Request call without phone number leads to infinite loading. In mobile, app crashes [HOLD for payment 2021-09-22] Concierge - Request call without phone number leads to infinite loading. In mobile, app crashes Sep 20, 2021
@mountiny
Copy link
Contributor

This message is a bug, caused by today's deploy and some weird GH API behaviour. Updated the title back to what it was 🙇

@parasharrajat
Copy link
Member

Ping for
image

@Jag96
Copy link
Contributor

Jag96 commented Sep 22, 2021

Paid!

@Jag96 Jag96 closed this as completed Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants