Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.11k stars 2.6k forks source link

[$250] [Payment card / Subscription] limit input field to 16 digits in `add-payment-card` screen field for card number #44111

Open blimpich opened 1 week ago

blimpich commented 1 week ago

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


Issue reported by: @blimpich

Action Performed:

Break down in numbered steps

  1. create a new account
  2. create a workspace on that account
  3. go to https://staging.new.expensify.com/settings/subscription/add-payment-card
  4. look at the card number field and type "abcdef01234567891234567"

Expected Result:

Describe what you think should've happened

Alphabet characters should not be allowed into the input field and the input field should be limited to 16 digits

Actual Result:

Describe what actually happened

"abcdef01234567891234567" shows up in the input field as typed

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0117d7a6e5f63d9c2e
  • Upwork Job ID: 1803868245221381580
  • Last Price Increase: 2024-06-20
  • Automatic offers:
    • fedirjh | Reviewer | 102858281
    • nkdengineer | Contributor | 102858284
Issue OwnerCurrent Issue Owner: @fedirjh
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~0117d7a6e5f63d9c2e

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)

gijoe0295 commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Alphabet characters should not be allowed into the input field and the input field should be limited to 16 digits

What is the root cause of that problem?

This is a feature request.

What changes do you think we should make in order to solve the problem?

  1. Create a cardNumber state and use it for the card number input's value
value={cardNumber}
  1. Implement onValueChange for card number input where we remove all non-digit characters:
onValueChange={(value) => {
    if (typeof value !== 'string') {
        return;
    }
    setCardNumber(value.replace(/\D/g, ''));
}}
  1. Set maxLength={16}

By this, when we pasted a string with non-digit characters, the input would only keep the digits and we couldn't type non-digit characters as well.

What alternative solutions did you explore? (Optional)

We can optionally format the card number to the space-separated form (i.e. 0123 4567 8910) using the above approach. Additionally, we should write a util function extractCardNumber to extract the plain digits without spaces so the submit handler could use the minified value.

let formattedText = value.replace(/\D/g, '');
if (formattedText.length > 0) {
    formattedText = formattedText.match(/.{1,4}/g)?.join(' ') || formattedText;
}
setCardNumber(formattedText);
nkdengineer commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

limit input field to 16 digits in add-payment-card screen field for card number

What is the root cause of that problem?

This is a new requirement

What changes do you think we should make in order to solve the problem?

We need to transform the card number entered by the user such that the whitespaces will be removed and any digits that are beyond 16 digits allowed for the card, will be removed

So first here define a new state cardNumber and a method onChangeCardNumber to transform the cardNumber correctly

const [cardNumber, setCardNumber] = useState('');

const onChangeCardNumber = useCallback((newValue: string) => {
    // replace all characters that are not spaces or digits
    let validCardNumber = newValue.replace(/[^\d ]/g, '');

    // gets only the first 16 digits if the inputted number have more digits than that
    validCardNumber = validCardNumber.match(/(?:\d *){1,16}/)?.[0] ?? '';

    setCardNumber(validCardNumber);
}, []);

We need to replace the card redundant digits in here, we cannot set maxLength={16} as others suggested above because the input/pasted by the user could have arbitrary format. For example it could be "1234 1234 1234 1234" that has 19 digits (very commonly used), or "1234 1234 1234 1234", or "Payment card: 1234 1234 1234 1234". If we set maxLength, the digits at the end will be stripped. So "1234 1234 1234 1234" will become "1234 1234 1234 1" which is not good from user perspecive.

That's why in my solution we'll be finding the first 16 digits of the inputted card number, and stripped anything beyond that.

Next, in https://github.com/Expensify/App/blob/633e708dfe3d2aba47dbb2091ba1f0f8683bfaaf/src/components/AddPaymentCard/PaymentCardForm.tsx#L206 we need to update the input to use the above defined cardNumber

onChangeText={onChangeCardNumber}
value={cardNumber}

We can optionally use keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD} for this form.

IMO the current behavior where we preserve the spaces entered by the user is fine because we want to allow users to format the card number however they want. But if we'd like to always format it with spaces every 4 digits like "1234 1234 1234 1234", we can add a simple Regex and transform it in the onChangeCardNumber. Also this was not highlighted as an issue by @blimpich so I think it's fine as is.

validCardNumber = validCardNumber.replace(/ /g, '').match(/.{1,4}/g)?.join(' ') ?? '';

What alternative solutions did you explore? (Optional)

Setting maxLength={19} could also be fine if we want to enforce users to use the "1234 1234 1234 1234" format. But this still fails to get the correct digits in case the number pasted by user has spaces at the start of the number, or more than 3 spaces between the number, or has any non-digit character. So the main solution is still better.

nkdengineer commented 1 week ago

Proposal updated

fedirjh commented 1 week ago

@gijoe0295 Your proposal fails to address the scenario of copying and pasting text. If you were to copy this text abcdef01234567891234567, it would only copy the initial 16 characters and then eliminate any alphabetical characters, resulting in 0123456789 which is not expected.

fedirjh commented 1 week ago

@nkdengineer Your proposal looks good and makes sense to me. As for the formatting, I believe it would be best to consistently use spaces every 4 digits, such as "1234 1234 1234 1234", to ensure a uniform format for all text (whether typed or copied).

cc @blimpich Let's proceed with this proposal from @nkdengineer

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 week ago

Current assignee @blimpich is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 1 week ago

πŸ“£ @fedirjh πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

πŸ“£ @nkdengineer πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

blimpich commented 1 week ago

Sounds good, lets do the spacing between every 4 digits, since that is what we do in Expensify Classic right now as well.

nkdengineer commented 1 week ago

@fedirjh this PR is ready for review

blimpich commented 5 days ago

Problem: this PR changes the value that is being sent to the api call. We use to be sending 4242424242424242 and now we are sending 4242 4242 4242 4242. This breaks the add payment card flow. Can we make a follow up PR to make it so that it strips the spaces away before sending it to the api call?

Sorry I didn't catch this till now, I should have more thoroughly tested this or provided my ngrok for you to test against.

nkdengineer commented 5 days ago

@blimpich i'll raise PR soon

nkdengineer commented 5 days ago

@blimpich we have a follow up PR here

blimpich commented 23 hours ago

Follow-up PR merged and works as intended πŸŽ‰