Closed kavimuru closed 1 year ago
Job added to Upwork: https://www.upwork.com/jobs/~011ac1ab2abb35dff9
Triggered auto assignment to Contributor Plus for review of internal employee PR - @mollfpr (Internal
)
I want to keep this internal for now, it's possibly due to a bug in our backend logic for detecting emails.
This isn't a bug. We use the standard PHP function for detecting correct-looking emails, and this fails that check which to be fair is expected.
gbfjdgjbjfdgbjfdjgbjbfdjbgjkbdfjkgkbjfkdbgkbdfkbgkbfdkgbkfdbdfbgkbfdkb@fafffffffffff.com
@davidcardoza you might need to close that job, but I'm not exactly sure how the automated jobs work.
Actually, the error should be improved. But this will still be an internal issue.
Cool, I’ll keep the job open.
@davidcardoza, @Julesssss Whoops! This issue is 2 days overdue. Let's get this updated quick!
This isn't overdue, it's an internal issue.
@davidcardoza @Julesssss this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
Lowering the priority as it is not that important. I'll get to it soon
@ctkochan22 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
Just took over issue.
So as Jules says, even though the email in the example has a @
and .com
format. Its not recognized by PHP's filter that its a legitimate email.
When that fails, we essentially assume its an SMS login. And as a result, Twillio confirms that is indeed not a valid phone number. Which is the wrong message to send to the user.
This is pretty edge case. Not sure if we want to do a lot to handle this case. But the solutions that come to mind are.
15005550006@expensify.sms
the only style of "phonenumber" that still looks like an email? Based on the code it seems like it. We can add an else if statement where if the $login
is not an expensify sms, we look for a @
. If by that point in the code, it has a @
we can assume its not going to be a valid numbercreateChatReport()
and make the error more generic. I noted where this is happening and wrote out the issue with the logic above.
Specifically the file issue is in User_Utils::getNormalizedCanonicalLogin();
Assigning this back out to the engineering pool
Triggered auto assignment to @techievivek (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
@techievivek this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks!
Not overdue, I will look into it today/tomorrow.
I looked into it based on the direction given by Kosuke, going to bring this to #eng-chat for a discussion on how to handle long email addresses.
Ok, it looks like we don't want to allow such a long email address in our system, so I feel like the filter seems to be working correctly. But we want to show some meaningful errors related to email validation rather than related to the phone number.
@techievivek Whoops! This issue is 2 days overdue. Let's get this updated quick!
@techievivek Huh... This is 4 days overdue. Who can take care of this?
@techievivek 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
@techievivek 10 days overdue. I'm getting more depressed than Marvin.
This is already deployed and I am not sure why the issue didn't get closed automatically.
@techievivek can you please reopen the job as reporting bonus is pending
Sorry @gadhiyamanan, I didn't realize this was reported by contributors.
@gadhiyamanan mind applying here? TY!
applied @dylanexpensify
offer sent!
Paid, contract ended!
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
should show validation error message for wrong email address
Actual Result:
shows validation error message for wrong phone number (i couldn’t validate phone number … )
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number:v1.2.64-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/216395185-6c061560-6275-465e-96d8-dbff5017bff0.mov
https://user-images.githubusercontent.com/43996225/216395204-de200e07-814f-4a18-8b66-0dfebf1aa94f.mp4
Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675313582030769
View all open jobs on GitHub
Upwork Automation - Do Not Edit