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.36k stars 2.79k forks source link

[$250] Pasting a dollar amount a dollar symbol does not work in IOU flow manual #47270

Closed m-natarajan closed 1 week ago

m-natarajan commented 1 month 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!


Version Number: 9.0.19-0 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 Expensify/Expensify Issue URL: Issue reported by: @quinthar Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1723332467625059

Action Performed:

Precondition: Copy an amount (eg: $90.28)

  1. Click FAB > Submit expense
  2. In the BNP press CTRL+V

    Expected Result:

    90.28 should be pasted in, stripping/ignoring the $

    Actual Result:

    Nothing happens Note: Trying again with 90.28 works fine

    Workaround:

    unknown

    Platforms:

    Which of our officially supported platforms is this issue occurring on?

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [x] MacOS: Chrome / Safari
    • [x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/33437013-3015-4880-ae6c-84a6c686adb6

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bb35c100257d5f4c
  • Upwork Job ID: 1824206181504759627
  • Last Price Increase: 2024-08-15
  • Automatic offers:
    • nkdengineer | Contributor | 103599426
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @alexpensify (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.

nyomanjyotisa commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-13 00:03:03 UTC.

Proposal

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

Pasting an amount with dollar icon or any other non number doesn't work. When pasting $90.28 it should be 90.28

What is the root cause of that problem?

We don't strip or remove non number and dot character here https://github.com/Expensify/App/blob/4b30725055e42558ab381f1d64f6f0b25288839c/src/components/MoneyRequestAmountInput.tsx#L156-L188

So on validateAmount it return false and new amount not applied

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

Create new function on MoneyRequestUtils called keepNumbersAndDot

function keepNumbersAndDot(amount: string): string {
    return amount.replace(/[^0-9.]/g, '');
}

use that new function before validateAmount amount here

            const amount = newAmountWithoutSpaces.includes('.')
                ? MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces)
                : MoneyRequestUtils.replaceCommasWithPeriod(newAmountWithoutSpaces);

            const finalAmount = MoneyRequestUtils.keepNumbersAndDot(amount)

What alternative solutions did you explore? (Optional)

Only allow paste and strip the currency symbol if the currency symbol on copied text match the selected currency Create new function on MoneyRequestUtils called stripCurrencySymbol

function stripCurrencySymbol(amount: string, currencyCode: string): string {
    const currencySymbol = CurrencyUtils.getLocalizedCurrencySymbol(currencyCode);
    const escapedCurrencySymbol = currencySymbol?.replace(/[-/\\^$*+?.()|[\]{}]/g, '\\$&');
    return amount.replace(new RegExp(`^${escapedCurrencySymbol}`), '').trim();
}

use that new function before validateAmount amount here

            const amount = newAmountWithoutSpaces.includes('.')
                ? MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces)
                : MoneyRequestUtils.replaceCommasWithPeriod(newAmountWithoutSpaces);

            const finalAmount = MoneyRequestUtils.stripCurrencySymbol(amount, currency)
Krishna2323 commented 1 month ago

Proposal

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

Pasting a dollar amount a dollar symbol does not work in IOU flow manual

What is the root cause of that problem?

We are not stripping the currency symbol before validating it with validateAmount. https://github.com/Expensify/App/blob/4b30725055e42558ab381f1d64f6f0b25288839c/src/components/MoneyRequestAmountInput.tsx#L156-L188

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

We can strip anything before the first number.

    const setNewAmount = useCallback(
        (newAmount: string) => {
            // Regular expression to match valid patterns including decimals
            const result = newAmount.replace(/^[^\d]+/, '');

            // Remove spaces from the newAmount value because Safari on iOS adds spaces when pasting a copied value
            // More info: https://github.com/Expensify/App/issues/16974
            const newAmountWithoutSpaces = MoneyRequestUtils.stripSpacesFromAmount(result);
            const finalAmount = newAmountWithoutSpaces.includes('.')
                ? MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces)
                : MoneyRequestUtils.replaceCommasWithPeriod(newAmountWithoutSpaces);

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~01bb35c100257d5f4c

melvin-bot[bot] commented 1 month ago

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

alexpensify commented 1 month ago

@s77rt - when you get a chance, can you please review whether one of these proposals will fix this issue? Thanks!

s77rt commented 1 month ago

@nyomanjyotisa @Krishna2323 Thank you for the proposals. When pasting an amount with currency, we should change the selected currency to the one we pasted. Can you please update your proposals to account for that?

nkdengineer commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-08-17 01:38:37 UTC.

Proposal

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

Nothing happens Note: Trying again with 90.28 works fine

What is the root cause of that problem?

We get the validated amount without detecting the case newAmount has a currency symbol prefix.

https://github.com/Expensify/App/blob/5e5bff3630958264c22a34f083879792ff8ee6ab/src/components/MoneyRequestAmountInput.tsx#L156-L170

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

The test branch here: https://github.com/nkdengineer/App/tree/fix/47270

  1. We should create a function to exact the currency and amount from the new amount like this
function validateAmountWithCurrency(amountWithCurreny: string): ExactAmountWithCurrency | undefined {
    const regex = /^(\p{Sc})?(\d+)$/u;

    const match = amountWithCurreny.match(regex);

    if (match) {
        const currencySymbol = match[1];
        const amount = match[2];
        const currency = getCurrencyCode(currencySymbol);
        if (!currency) {
            return undefined;
        }
        return {currency, amount};
    }

    return undefined;
}
  1. Then, in the' setAmount' function, if we get the amount with currency, we will update both currency and amount. We will validate the exacted amount with the exacted currency by using validateAmount function, if it's valid we will update both the amount and currency. The logic update amount already exists, to update the currency we will need to pass a prop likeonPasteAmountWithCurrency from IOURequestStepAmount to MoneyRequestAmountInput. I think passing a prop from IOURequestStepAmount to MoneyRequestAmountInput is the best option because setMoneyRequestCurrency function needs some data that exists on IOURequestStepAmount and this data is unnecessary in MoneyRequestAmountInput
IOU.setMoneyRequestCurrency(transactionID, updateCurrency, action === CONST.IOU.ACTION.EDIT);
let finalAmount: string;
const exactAmountWithCurrency = MoneyRequestUtils.validateAmountWithCurrency(newAmount);
if (exactAmountWithCurrency) {
    const {currency: updateCurrency, amount: updateAmount} = exactAmountWithCurrency;
    const updateDecimals = CurrencyUtils.getCurrencyDecimals(updateCurrency);
    finalAmount = updateAmount.includes('.') ? MoneyRequestUtils.stripCommaFromAmount(updateAmount) : MoneyRequestUtils.replaceCommasWithPeriod(updateAmount);
    // Use a shallow copy of selection to trigger setSelection
    // More info: https://github.com/Expensify/App/issues/16385
    if (!MoneyRequestUtils.validateAmount(finalAmount, updateDecimals)) {
        setSelection((prevSelection) => ({...prevSelection}));
        return;
    }
    onPasteAmountWithCurrency?.(updateCurrency);
} else {
    finalAmount = newAmountWithoutSpaces.includes('.')
        ? MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces)
        : MoneyRequestUtils.replaceCommasWithPeriod(newAmountWithoutSpaces);
    // Use a shallow copy of selection to trigger setSelection
    // More info: https://github.com/Expensify/App/issues/16385
    if (!MoneyRequestUtils.validateAmount(finalAmount, decimals)) {
        setSelection((prevSelection) => ({...prevSelection}));
        return;
    }
}

https://github.com/Expensify/App/blob/5e5bff3630958264c22a34f083879792ff8ee6ab/src/components/MoneyRequestAmountInput.tsx#L156-L170

What alternative solutions did you explore? (Optional)

To update the currency, we can create a state in MoneyRequestAmountInput to control the currency value then in the case we copy and paste the amount with the currency above we will update the currency state. We will pass this state as selectedCurrencyCode here

https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/components/MoneyRequestAmountInput.tsx#L305

and pass the currency state as a param to onCurrencyButtonPress callback here then we can use this in IOURequestStepAmount here to open the currency selection page with the correct currency

https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/components/MoneyRequestAmountInput.tsx#L292

To update the currency when clicking on the submit button

  1. We should create a new method getCurrency for moneyRequestAmountInputRef

https://github.com/Expensify/App/blob/e903ec9f5eb1a185d2c85a9ca4fb821107d5d364/src/pages/iou/request/step/IOURequestStepAmount.tsx#L324

  1. Get the currency here and replace currency with the current currency in this function

https://github.com/Expensify/App/blob/8f9ca45f10c69d4431892b2feb2574c858940a33/src/pages/iou/MoneyRequestAmountForm.tsx#L213

  1. Then we will get this here and replace all use of currency in this function with the new currency that passed to this function and navigateToNextPage function https://github.com/Expensify/App/blob/e903ec9f5eb1a185d2c85a9ca4fb821107d5d364/src/pages/iou/request/step/IOURequestStepAmount.tsx#L268

    Result

https://github.com/user-attachments/assets/5f631dd6-70f3-46d1-a970-d9decac7c4e8

s77rt commented 1 month ago

@nkdengineer Thanks for the proposal. Can you please explain in English the process of updating the currency? (after extracting it from pasted text)

nkdengineer commented 1 month ago

@s77rt I updated my proposal with English explain.

s77rt commented 1 month ago

@nkdengineer Why do we need a callback to set the currency? Can't we just update it inside MoneyRequestAmountInput (the same way the amount is handled)

nkdengineer commented 1 month ago

Can't we just update it inside MoneyRequestAmountInput (the same way the amount is handled)

@s77rt Yeah checked again and we can do this.

nkdengineer commented 1 month ago

Updated my proposal.

s77rt commented 1 month ago

@nkdengineer Can you please cover how we will use that new updated currency when pressing the next button

https://github.com/user-attachments/assets/f468f183-8ffe-4741-8dfc-40e07e87159c

nkdengineer commented 1 month ago

@s77rt Updated alternative solution to cover the submit button case. And also updated the test branch for testing.

s77rt commented 1 month ago

@nkdengineer Still not using the new currency

https://github.com/user-attachments/assets/6d93c7ea-0b23-4b98-b45b-f5b6bfbec788

nkdengineer commented 1 month ago

I tested and it works well. Did you get the current currency in submitAndNavigateToNextPage function via moneyRequestAmountInputRef

https://github.com/user-attachments/assets/d765069d-051e-4bba-b849-128cbccb23d6

s77rt commented 1 month ago

@nkdengineer Yes, it's already logged in the recorded video

nkdengineer commented 1 month ago

@s77rt Did you pass this currency to navigateToNextPage function and in this function pass the currency param to setMoneyRequestAmount function here

https://github.com/Expensify/App/blob/e903ec9f5eb1a185d2c85a9ca4fb821107d5d364/src/pages/iou/request/step/IOURequestStepAmount.tsx#L172

s77rt commented 1 month ago

@nkdengineer Ah that what I was missing. Another problem is that I can't change the currency using the currency selector

https://github.com/user-attachments/assets/b3a6e532-9518-4291-9f53-ca8c1f1f567f

nkdengineer commented 1 month ago

@s77rt Follow the same way we do for amount we need to add a useEffect in MoneyRequestAmountInput to update currecy state when currency prop is changed. I already add this logic in the test branch of my proposal before.

s77rt commented 1 month ago

@nkdengineer Overall looks good to me.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed Link to proposal PS: Please do not add code-diff, branches, PRs.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

nkdengineer commented 1 month ago

PS: Please do not add code-diff, branches, PRs

Thanks, I noted.

melvin-bot[bot] commented 1 month 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 πŸ“–

alexpensify commented 1 month ago

Next Steps

This PR is going through the review process.

Heads up, I will be offline until Tuesday, September 3, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

alexpensify commented 3 weeks ago

Catching up from being OOO, I see the PR is moving along.

alexpensify commented 2 weeks ago

PR is still going through the review process.

s77rt commented 1 week ago

After working on this for about a month we think it's best to close the PR. Reasoning: the needed work turned out to be more complex than initially evaluated. The initial plan was to make the currency prop stateful the same way as the amount. However the currency is used in other places (namely the currency selection page) and it needs to be in sync e.g. if you change currency via paste, you should see that new currency in the currency page and vice-versa. But we kept running into bugs and decided not to continue with that approach.

Next steps: I think we can either:

  1. Reopen for more proposals and look for a different approach
  2. Close the issue (since it's a feature request, not a bug)

cc @Gonals thoughts on this one?

Gonals commented 1 week ago

Makes sense to me. @alexpensify, thoughts on this? Do we give it another try to find a different approach or do we call it a day?

alexpensify commented 1 week ago

I agree, let's close. Thanks!