-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[$125] Error message displayed when adding a UK zip code to a GBP payment card #52642
Comments
Triggered auto assignment to @sakluger ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Adding a UK zip code to a GBP payment card errors out incorrectly What is the root cause of that problem?We only check valid zip code and not check valid us post code App/src/components/AddPaymentCard/PaymentCardForm.tsx Lines 168 to 170 in 1b9e302
What changes do you think we should make in order to solve the problem?
Notes: we should add What alternative solutions did you explore? (Optional)Simply we only need to update |
This seems like a pretty easy change. I'm going to set the initial price at $125. If this needs to be done for several countries, and each one requires specific treatment, then we can consider a higher price, but it doesn't seem like that's necessary for now. |
Job added to Upwork: https://www.upwork.com/jobs/~021857542773027117579 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha ( |
Upwork job price has been updated to $125 |
Edited by proposal-police: This proposal was edited at 2024-11-18 19:58:48 UTC. ProposalPlease re-state the problem that we are trying to solve in this issueError message displayed when adding a UK zip code to a GBP payment card. What is the root cause of that problem?The current App/src/components/AddPaymentCard/PaymentCardForm.tsx Lines 168 to 170 in 99bf554
What changes do you think we should make in order to solve the problem?Adjust the validate logic similarly to what we do in Address / AddressForm which is Zip Code validation based on the selected country, in our case for a more comprehensive solution we'll validate Zip Code based on selected Currency. To do this we will add / adjust the following:
/**
* Get the country code for a certain currency(ISO 4217) Code
*/
function getCurrencyCountryCodes(currencyCode: keyof typeof CONST.CURRENCY): Array<keyof typeof CONST.ALL_COUNTRIES> {
const currencyCountryMap: Record<keyof typeof CONST.CURRENCY, Array<keyof typeof CONST.ALL_COUNTRIES>> = {
[CONST.CURRENCY.USD]: ['US'],
[CONST.CURRENCY.AUD]: ['AU'],
[CONST.CURRENCY.CAD]: ['CA'],
[CONST.CURRENCY.GBP]: ['GB'],
[CONST.CURRENCY.NZD]: ['NZ'],
[CONST.CURRENCY.EUR]: ['AT', 'BE', 'CY', 'EE', 'FI', 'FR', 'DE', 'GR', 'IE', 'IT', 'LV', 'LT', 'LU', 'MT', 'NL', 'PT', 'SK', 'SI', 'ES'],
};
return currencyCountryMap[currencyCode] ?? [];
}
// Use provided currency or default to USD
const currency = data?.currency ?? CONST.PAYMENT_CARD_CURRENCY.USD;
// Get countries associated with the currency
const countries = CurrencyUtils.getCurrencyCountryCodes(currency);
// Check if the addressZipCode matches any country-specific regex
if (values.addressZipCode) {
const zipCode = values.addressZipCode.trim();
const isValidZip = countries.some((countryCode) => {
const regexDetails = CONST.COUNTRY_ZIP_REGEX_DATA[countryCode];
// Optionally: check for empty string after trim
if (!zipCode) {
return false;
}
return 'regex' in regexDetails && regexDetails.regex.test(zipCode);
});
if (!isValidZip) {
errors.addressZipCode = translate(label.error.addressZipCode);
}
} this will ensure that the Zip Code is validated based on the currency selected in the Add payment card, defaulting to USD just like the Currency field. Here's a test branch: ikevin127-cardZipCodeValidation for testing convenience or the diff if preferred. Alternative solution 1Map over all Alternative solution 2Remove any specific Zip Code validation, allowing any input as long as the field is not empty. This one would be the most performant (since this seems to be a very important criteria 🙂). Alternative solution 3Use Result video (before / after)MacOS: Chrome
|
@sakluger In the proposed solution I made sure to include logic for each specific currency / country and also for the I'd say that justifies the base $250 bounty for this issue in case the proposed solution will be the one selected. Thank you! |
@ikevin127 that sounds fair to me! If your solution is selected, then I will increase the price to $250. |
@ikevin127 @daledah we already have |
@getusha Are you sure you read my proposal ? Asking because I'm confused by your note since I did use |
mb, i overlooked that. reviewing your solution |
@ikevin127 after a second look i am not sure if mapping currencies to countries is the right idea. as there could be for example, someone from |
ProposalPlease re-state the problem that we are trying to solve in this issue.Error message displayed when adding a UK zip code to a GBP payment card What is the root cause of that problem?We only check valid US zip code and not check valid UK zip code App/src/components/AddPaymentCard/PaymentCardForm.tsx Lines 168 to 170 in 8e1b0fd
What changes do you think we should make in order to solve the problem?we must handle UK zip code when the GBP currency is selected
What alternative solutions did you explore? (Optional) |
📣 @YahyaDalbah! 📣
|
@getusha If by generic regex you mean simply allowing any postcode then we might as well remove any validation as we're not interested in actually validating based on each country's regex. The reason why I propose validating based on currency is because especially in the US, if you don't input the Zip Code matching the card - the bank will reject authorization / payment. Updated proposalAdded alternative solution where we map over all Hopefully this will satisfy, otherwise I need more specificity on what exactly you're expecting in terms of solution here. |
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
We are selecting a currency not a country, i don't see how that's relevant.
Having a
It might cause performance implications, which could make the solution not ideal. |
@getusha You are not making any sense honestly, first you say:
Then when I suggested 2 different solutions using I don't understand what are your expectations in this case, can you clarify ? Also please test the solutions and come back with solid proof that the solution does introduce performance issues, because "might" means nothing. Let me know if you have the capacity to do that and want to review this issue properly, otherwise I will propose re-assigning another reviewer. Thanks! |
@sakluger what about USD cards? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
We have four billing currencies - USD, GBP, AUD, and NZD. I'm going to ask how we validate the billing card postal code field in OldDot, because I assume we should just reuse that logic. |
FYI I asked here in Slack. I'm still waiting for a response so I bumped my question. |
I heard back from the team and discovered that there is no postal code validation in OldDot (none in frontend or backend). I also checked with the people who lead the project to add billing cards to NewDot, and they think that we should copy OldDot. That means that we should remove all postal code validation from NewDot. @daledah @ikevin127 @YahyaDalbah could you please update your proposals with the new guidance if you'd like to be considered? Thanks! |
@getusha I already suggested removing any specific Zip Code validation in my proposal, Alternative solution 2. |
@ikevin127's proposal looks good to me. they were the first to suggest removing any zip code validation on their alternative solution. based on the decision making from the internal team 🎀 👀 🎀 C+ Reviewed! |
Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
♻️ Update regarding the issue: I drafted the PR already, awaiting to be assigned so I can open it. While testing the fix, I noticed that on mWeb platforms the keyboard was locked to numeric because of this line:
As part of the PR I removed that line as well since UK Zip Codes contain both characters and numbers, therefore we don't want to restrict users to numeric input only. More details in this PR #53178 (comment). |
📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
PR #53178 is ready for review! 🚀 |
♻️ Status update: PR has been open and ready for review 2 days ago, awaiting @getusha's review. |
♻️ Status update: PR has been open and ready for review for 5 days, still awaiting @getusha's review. |
Just as a bit of a follow-up here if that PR is almost done and dusted, but I think we should change the label name to |
@trjExpensify Sure, will change the label before the PR gets merged - linking to your comment for the reason. ✅ Done |
Excellent! |
Updated postcode input label to match the label found in the form on Settings > Profile > Private > Address , as requested in Expensify#52642 (comment).
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.63-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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
Expensify/Expensify Issue URL:
Issue reported by: @muttmuure
Slack conversation (hyperlinked to channel name): ts_external_expensify_bugs
Action Performed:
Expected Result:
A UK post code is accepted
Actual Result:
The Zip Code field shows an error.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: