Closed izarutskaya closed 5 months ago
Triggered auto assignment to @mallenexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
@mallenexpensify I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.
The user can not enter the phone number as the placeholder shows.
We already have the logic to check the phone number is valid or not in here
but it's not really correct since it can cause some confused cases: https://github.com/Expensify/App/issues/34766
so we added the additional logic to check phone number in
but isValidPhone
just use the simple e164 regex
and that doesn't cover other format like (440)458-9784
We shouldn't update isValidPhone
logic since it's used in many places in the App, instead of that we should change its name to isValidE164Phone
then replace it in all places
Create other function isValidPhone
to handle all possible phone number
Here's the possible format:
significant: 4404589784 international: +1 440-458-9784 e164: +14404589784 national: (440) 458-978 123.456.7890
so we can use the following regex:
const phoneNumberRegex = /(\+\d{1,2}\s?)?(\(\d{3}\)|\d{3})[\s.-]?\d{3}[\s.-]?\d{4}/;
NA
Phone number in the format (440)458-9784 is marked as invalid
Issue is within the function isValidUSPhone
https://github.com/Expensify/App/blob/578769197ddd4039f46b949d83a7e85ad2476abc/src/libs/ValidationUtils.ts#L281C10-L293
When the input is in phormat like: (440)458-9784
The condition below:
https://github.com/Expensify/App/blob/578769197ddd4039f46b949d83a7e85ad2476abc/src/libs/ValidationUtils.ts#L287-L289
will turn:
regionCode: US
and !Str.isValidPhone(phone): true
Then the execution will exit without approving the phone number
We need to fix this condition https://github.com/Expensify/App/blob/578769197ddd4039f46b949d83a7e85ad2476abc/src/libs/ValidationUtils.ts#L285-L289
It's built to fix an issue as mentioned in the comment:
// When we pass regionCode as an option to parsePhoneNumber it wrongly assumes inputs like '=15123456789' as valid
Currently we use Str.isValidPhone(phone)
to check if the input is a valid phone number. isValidPhone
is a function from expensify-common. We can replace it with another function we create to check all possible format and return a true for valid US phone numbers.
We can fix the function isValidPhone
from expensify-common
to consider the format (xxx)xxx-xxxx
as a valid phone number
https://github.com/Expensify/expensify-common/blob/77d0b150ba6bfbe7a64b3c3e30b65592b2e58c4a/lib/str.js#L934-L936
I think this should be closed, as seen in the video, we don't enter space as specified in the placeholder and hence we face error as the regex doesn't match
Job added to Upwork: https://www.upwork.com/jobs/~01f2cb151e80502fdb
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 (External
)
Was able to reproduce and seems like we'd want to fix. I, personally, would find it frustrating. Let me find which wave to drop it into. @Pujan92 , can you also please try to repro? Bank account deets here
Checking for #wave-collect to add.
📣 @waleedj1! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
This issue is due to Validation For Phone Number Input Field Is Failing, Due To Some Issue In The Regex, We can solve this by improving our regex
Contributor details Expensify Account: waleedjaved308@gmail.com upwork Account: https://www.upwork.com/freelancers/~0124f16df0b524ada4
Yes @mallenexpensify , I am able to reproduce here and for enable wallet screen too as both uses the same common function.
Do we need to allow users to enter in placeholder format((xxx) xxx-xxxx
) or should it be like the way we allow for the login("Please enter a valid phone number, with the country code (e.g. +15005550006)")?
Asking above to decide whether a placeholder change is needed or the pattern support needed.
Thanks for testing @Pujan92 . I've added this to the wave-collect project as Polish.
Do we need to allow users to enter in placeholder format((xxx) xxx-xxxx) or should it be like the way we allow for the login
Are there other examples of where a phone number is entered and accepted in multiple formats? Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).
Do we need to allow users to enter in placeholder format((xxx) xxx-xxxx) or should it be like the way we allow for the login
Are there other examples of where a phone number is entered and accepted in multiple formats? Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).
Earlier it used to be working with this format((xxx) xxx-xxxx
) and many more. I think this problem arises after this PR. To allow multiple format case we need a solution which solves this issue as well as https://github.com/Expensify/App/issues/34766.
@Pujan92 my proposal allows multiple formats
Thanks @dragnoir , I will review the proposals tomorrow.
Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).
@Pujan92 I updated my proposal to support multiple formats:
All proposals RCA are correct, I like the @tienifr's proposal as it is more detailed and agree with the fact that isValidPhone
is used at multiple places and some places we are not supporting multiple formats(for eg. login). So renaming it to isValidE164Phone
and creating other function for supporting multiple formats makes more sense to me.
🎀👀🎀 C+ reviewed
Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@Pujan92 I suggested these updates first. Please check again. I also was the 1st to figure out the RCA
@dragnoir My original proposal also point the RCA, I think it's easy to figure out. I just want to update the proposal because of this discussion
@tienifr you changed everything after the discussion.
@dragnoir Regarding your proposed solution you have mentioned replacing isValidPhone
with other function which accepts all formats. But we need this function also to support only E164 format. Also the selected updated proposal provided the regex which seems fine to me.
Let's wait for @grgia's thoughts.
@Pujan92 there's a condition that checks if the phone number starts with = like (=5436789876) and turn false
it also turn false for numbers like (xxx) xxx-xxxx
Here I al the 1st who figured out this RCA.
Then I suggested fixing it so it catch format like (=xxxxxxx) as false and format like (xxx)xxx-xxxx as true
I also suggested to make the new function figure out all possible formats
@mallenexpensify and @grgia I am optimistic about your judicious decision-making. Please take a moment to review how significantly the proposal was transformed by tienifr, especially after mallenexpensifyr declined to simply adjust the placeholder. Additionally, it's important to note tienifr's mischaracterization of the Root Cause Analysis (RCA). He was not aware that the function isValidUSPhone is effectively operational when utilizing parsePhoneNumber, and the actual issue stemmed merely from a condition handling a specific case of a number format like xxxxxx. Furthermore, following the change, tienifr acknowledged the issue with isValidPhone, which was initially outlined in my proposal. This adjustment not only rectifies the oversight but also reinforces the original accuracy and foresight of my proposal.
@tienifr @dragnoir, thank you both for your time on this issue; I can see that you've both put effort into your proposals and have made efforts to cover all cases.
From the discussion, it seems that @tienifr's proposal covered the original expected result and was proposed first, whereas @dragnoir's original proposal was proposed later, but covered the other cases that we later deemed a requirement of this issue.
How about we split the bounty for this issue? $200 / $300 for @tienifr to open the PR/do screenshots/testing as I trust @Pujan92's C+ decision.
Does that work for both of you?
Thank you @grgia for reviewing the proposals. I will take the $200.
@grgia I don't think it's fair
all possible format
but did not mention what the formats arereplace it with another function
is not correct since we use this function in many places. His solution is too general.Additionally, I need to open the PR/do screenshots/testing so I believe 200/300 in this case is not fair
@tienifr, I appreciate your effort in reviewing the pricing strategy; however, it seems there has been a misunderstanding. The entirety of the proceeds are allocated to me, which contradicts your Root Cause Analysis (RCA) and the subsequent proposal you made for overhauling the function or modifying the input placeholder. It's important to note that the focus of our discussion has shifted significantly towards the isValidPhone function, diverging from the original scope of your proposal.
I have agreed to the $200 arrangement as a gesture of respect for @grgia's decision.
In my proposal I not only mentioned what the formats that we should support, but also found the regex to detect the phone number.
I agree that my original is not good, since I don't know what formats that we support. After this discussion here, I want to modified my proposal to match the requirement. We can modified the proposal to find the best one, right? And C+ also agree with that
Let's wait for response from @grgia and I just want her to check the bounty again. I'll be happy with her final decision. Thanks
@tienifr this is your initial proposal (updates later with my ideas)
You said about the RCA:
We are using isValidUSPhone to validate the phone number, and it doesn't consider the (xxx)xxx-xxx is the valid phone
and this is totally wrong. BECAUSE the function isValidUSPhone
considers all formats of phone numbers because we are using parsePhoneNumber
to check the phone format (we use the library awesome-phonenumber)
So what you said is wrong, isValidUSPhone consider the (xxx)xxx-xxx format.
In my proposal, I mentioned the correct RCA, we have a condition to turn false if the number is in this format (=xxxxxxxxxx) and suddenly this condition wrongly turn false for the format (xxx)xxx-xxx and this is the right RCA
Your solution was to fix isValidUSPhone with a new Regex and this is WRONG.
My solution was to fix the condition to only filter for mat like (=xxxxxxx) and not (xxx)xxx-xxxx and this the best way to do it
@dragnoir This logic
is in isValidUSPhone
right? And Str.isValidPhone(phone) doesn't consider (xxx)xxx-xxx
is valid format
Your solution was to fix isValidUSPhone with a new Regex and this is WRONG.
In my later solution I also create the new regex but I think we should add in other place, so it's normal. We can edit the proposal and I didn't use yours 🤣
Let's wait to hear internal decision
@tienifr my proposal and yours are totally different. You never mentioned isValidPhone. so recap:
You said isValidUSPhone
si wrong and we should fix it
==> WRONG
I said isValidUSPhone
is good and a condition inside it should be fixed
==> CORRECT
if your initial proposal was correct, why did you changed it totally with agreement and ideas from mine? Because you were pointing to the wrong RCA
@grgia BTW this is the second time @tienifr steal my idea, you can check here https://github.com/Expensify/App/issues/33709 also after he stole my idea, he was happily taking $250 without doing anything at all. And here he stole my RCA and the solution and he want all the price!!!
@dragnoir Just let you know that I worked on a lot of issues and I knew that we're building the supportive environment and try to find the best solution. We can have many proposals on a issue so of course the later C can read the information from the previous one and create the better proposal. The best solution will be chosen. In this issue and https://github.com/Expensify/App/issues/33709 your solution is not good, and my solution is chosen. I just wonder why your too general proposal is paid 200$ while I spend a lot of time to create the detail solution and open the PR just is paid 300$. But I will respect the final decision from @grgia
I don't agree when you say he stole my RCA and the solution
since your solution is not perfect. We can stop debate and wait for the final decision
@tienifr @dragnoir I'm going to discuss this internally and let @mallenexpensify give an update.
For now, let's pause for a minute and wait for an internal update from the pending conversation.
We've got more than enough issues to go around and if you're having trouble finding work in the meantime, I'm happy to help you find something to work on.
👋 , here to help! I don't have an immediate answer.
The first piece I'm wondering about it - @tienifr when you were making edits to your proposal, for any code that would have affected the solution, did you also post a comment in the GH? Asking because of the below in CONTRIBUTING.md
If you want to make an entirely new proposal or update an existing proposal, please go back and edit your original proposal, then post a new comment to the issue in this format to alert everyone that it has been updated:
## Proposal [Updated](link to proposal)
@mallenexpensify I posted in here, sorry because I didn't use the format ## Proposal
Thanks @tienifr , I'm seeing that is from March 6, 8:42pm.
Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).
@dragnoir , are you specifically saying that you addressed this before March 6 (my time) as part of you proposal?
Also.. let's all focus on getting the PR merged as we continue discussions about the process and potential compensation.
Also, @Pujan92 , can you review the above and provide your feedback and recommendation for how best to handle? Thx
Also.. let's all focus on getting the PR merged as we continue discussions about the process and potential compensation.
@grgia Can you help assign me so I can create the PR? Thanks
Ideally want input to be consistent with otheres (and... to also allow for multiple formats, if possible).
@dragnoir , are you specifically saying that you addressed this before March 6 (my time) as part of you proposal?
Also.. let's all focus on getting the PR merged as we continue discussions about the process and potential compensation.
@mallenexpensify Yes I addressed this before March 6. This is the original proposal from tienifr https://github.com/Expensify/App/issues/37723#issuecomment-1983272314 and he never mentioned that only after my proposal. Also his regex on his 1st proposal does not cover all phone formats. and this is what I mentioned before him:
We can replace it with another function we create to check all possible format and return a true for valid US phone numbers.
@Pujan92 as mentioned above and here https://github.com/Expensify/App/issues/37723#issuecomment-1983288015 please be fair.
Ready to handle the RP.
Whatever the decision, @Pujan92 PR ready https://github.com/Expensify/App/pull/38028 (Working on screenshots) and I provided a list of tests I have done with random phone numbers.
@mallenexpensify After re-reviewing the proposals I still think @tienifr's proposal is more accurate here.
Reason - In expensify-common we have a util function called isValidPhone
but it only considers the pattern for E164 format. So it is better to update that common lib with the 1) Rename it to the isValidE164Phone
and 2) Create a new isValidPhone
function to be more generic.
However, a detailed explanation has been added after @dragnoir's proposal and their proposal will solve the issue too.
I would suggest assigning @tienifr here and best to split the bounty as @grgia suggested earlier($300 for @tienifr & $200 for @dragnoir). For this specific case, I expect both contributors to mutually agree and proceed to fix the issue. With that, we have many other external issues for which we are looking for proposals.
Whatever the decision, @Pujan92 PR ready #38028
@dragnoir I think you should wait till the assignment here.
@Pujan92 I think you are talking about my proposal
We can fix the function isValidPhone from expensify-common to consider the format (xxx)xxx-xxxx as a valid phone number
Where did you find that tienifr mentioned about fixing the common lib or creating new function? (below in screenshot of his proposal) All what he mentioned is fixing the main function isValidUSPhone
We can fix the function isValidPhone from expensify-common to consider the format (xxx)xxx-xxxx as a valid phone number
We can't directly add this format check to expensify-common isValidPhone
function as we do need to support E164 format validation separately which is used in some places.
Where did you find that tienifr mentioned about fixing the common lib or creating new function? (below in screenshot of his proposal) All what he mentioned is fixing the main function isValidUSPhone
@dragnoir I have mentioned it is added in their updated proposal after your proposal, That's why we are deciding on splitting the bounty. Let's wait now for @grgia and @mallenexpensify for the final decision and assignment.
Thanks god you finally admitted that everything about fixing the common lib or creating new function allow for multiple formats is added AFTER my proposal.
@Pujan92 Where it's mentioned that updating a wrong proposal based on a second correct proposal leads to splitting the bounty. This is a violation of the contributions rules!!!!
Thanks for the patience here. Coming from here in CONTRIBUTING.md.
After you reproduce the issue, complete the proposal template here and post it as a comment in the corresponding GitHub issue (linked in the Upwork job). Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.
Since the issue persists that it's possible that proposals are edited and updated from the time their first posted, we've ben working on Proposal Police
to see if it can help with monitoring updates. From this post
- tienifr posted initial proposal - all good, no action performed
- dragnoir posted initial proposal - all good, no action performed
- dragnoir 1st update (added alternative solution) - minor proposal change detected, no action performed
- dragnoir 2nd update - significant proposal change detected - proposal police edited the proposal adding the following line at the top: Edited by proposal-police: This proposal was edited at 2024-03-07 16:31:55 UTC.
- tienifr 1st update - significant proposal change detected - proposal police edited the proposal adding the following line at the top: Edited by proposal-police: This proposal was edited at 2024-03-07 16:41:05 UTC.
So it looks like both proposals were significantly-enough updated to get to the point where @Pujan92 was able to review and recommend one.
For me, being the BugZero team member who manages the issues, and as a non-engineer, it's hard to judge the level of significance of the updates and changes. It appears where we're here now
Is the above correct? If so, @tienifr can you provide reasoning why you think @dragnoir isn't due compensation or propose an amount you feel is fair for the split for their contributions? Thx
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v1.4.47-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+vd_web030424@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team
Action Performed:
Pre-requisite: user must be logged in and have created a workspace.
Expected Result:
The user should be able to enter the phone number as the placeholder shows.
Actual Result:
The user can not enter the phone number as the placeholder shows.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
https://github.com/Expensify/App/assets/115492554/2f41a1a2-5ff6-4063-bfc4-50c69258884f
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @mallenexpensify