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.55k stars 2.89k forks source link

[$250] When a receipt matches with a personal card transaction, it still shows Amount • Cash above the amount #46554

Open m-natarajan opened 3 months ago

m-natarajan commented 3 months 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.14-5 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: @puneetlath Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722282008985029

Action Performed:

  1. Submit a card expense report with receipt
  2. Open the report as approver (This is report 6565029153383101)

Expected Result:

The expense is mentioned as card expense

Actual Result:

Shown as Amount • Cash above the amount

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence Screenshot 2024-07-29 at 3 38 34 PM

Screenshot 2024-07-29 at 3 38 51 PM

Snip - (518) New Expensify - Google Chrome

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0184082e81b7b23b49
  • Upwork Job ID: 1820474809466444194
  • Last Price Increase: 2024-11-11
Issue OwnerCurrent Issue Owner: @Ollyws
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @abekkala (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 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0184082e81b7b23b49

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

@Ollyws, @abekkala Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

Chefkj commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-10 19:01:43 UTC.

Proposal

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

Amount type is cash when receipt is processed.

What is the root cause of that problem?

/App/src/libs/TransactionUtils/index.ts

function isCardTransaction(transaction: OnyxEntry<Transaction>): boolean {
    const cardID = transaction?.cardID ?? -1;
    return isCorporateCard(cardID);
}`

in App/src/components/ReportActionItem/MoneyRequestView.tsx

if (isCardTransaction) {
        if (formattedOriginalAmount) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.original')} ${formattedOriginalAmount}`;
        }
        if (isCancelled) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.canceled')}`;
        }
    } else {
        if (!isDistanceRequest) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.cash')}`;

Since the isCardTransaction function is only affected by a corporate card id being present, the result will always be cash.

This led me to out of scope details, for instance, when scanning a receipt, the svg graphic isn't updating from: src/components/SelectionList/Search/TransactionListItemRow.tsx

const getTypeIcon = (type?: SearchTransactionType) => {
    switch (type) {
        case CONST.SEARCH.TRANSACTION_TYPE.CASH:
            return Expensicons.Cash;
        case CONST.SEARCH.TRANSACTION_TYPE.CARD:
            return Expensicons.CreditCard;
        case CONST.SEARCH.TRANSACTION_TYPE.DISTANCE:
            return Expensicons.Car;
        default:
            return Expensicons.Cash;
    }
};

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

In the scope of the ticket, I would suggest adding boolean logic to show card text: in src/components/ReportActionItem/MoneyRequestView.tsx

if (isCardTransaction) {
        if (formattedOriginalAmount) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.original')} ${formattedOriginalAmount}`;
        }
        if (isCancelled) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.canceled')}`;
        }
    } else {
        if (!isDistanceRequest && !didReceiptScanSucceed) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.cash')}`;
        } else if (didReceiptScanSucceed) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.card')}`;
        }

This adds a path for card to show on receipt scan success, but this doesn't update the svg from the TransactionListRow, and won't show cash if it's a cash receipt.

Result:

Screenshot 2024-08-08 at 6 14 37 PM Screenshot 2024-08-08 at 6 15 15 PM

since the transaction object is being built from src/libs/actions/IOU.ts

function buildOnyxDataForMoneyRequest(..
onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}

and in src/ONYXKEYS.ts

[ONYXKEYS.COLLECTION.TRANSACTION]: OnyxTypes.Transaction;

we would need to build a new type value in the Transaction class to update the transaction as either cash or card. There are two ways to achieve this, one is with the current OCR provider providing data classification on your OCR provider team, the other is through a node OCR package which we would take the image file from receipt class and then regex for cash or card and update the isCard boolean with the result.

at which point the last line would look as so

else if (didReceiptScanSucceed) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${getTypeAsText(transaction)}';

What alternative solutions did you explore? (Optional)

Using SearchTransactionType

function getTypeAsText(transactionItem: SearchTransaction): String | undefined {
        const isCardT = transactionItem?.transactionType === CONST.SEARCH.TRANSACTION_TYPE.CARD;
        const isCashT = transactionItem?.transactionType=== CONST.SEARCH.TRANSACTION_TYPE.CASH;
        const isDistanceT = transactionItem?.transactionType === CONST.SEARCH.TRANSACTION_TYPE.DISTANCE;

        if (isCardT) {
            return CONST.SEARCH.TRANSACTION_TYPE.CARD;
        } else if (isCashT) {
            return CONST.SEARCH.TRANSACTION_TYPE.CASH;
        } else if (isDistanceT) {
            return CONST.SEARCH.TRANSACTION_TYPE.DISTANCE;
        }
    }

determined SearchTransactionType is unlinked from SearchTransaction

Ollyws commented 3 months ago

@Chefkj how did you achieve the Submit a card expense report with receipt step?

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Chefkj commented 3 months ago

I'm submitting an expense with the + button list, "submit expense" per the error video above.

Ollyws commented 3 months ago

@puneetlath Any chance you could explain the reproduction steps for this flow? I'm not aquainted with it and the repro steps aren't very clear. Thanks!

Chefkj commented 3 months ago

No problem, I just went off the pictures.

click FAB click submit expense click scan add receipt choose workspace submit expense after receipt is scanned, amount still shows cash if it's not a corporate card

Chefkj commented 3 months ago

Additionally, when you go to search and expenses, the svg graphic has not updated to the card svg

Chefkj commented 3 months ago

I've been looking into it pretty heavily, and I don't believe I can access the receipt scanning software to add the card/cash regex to the text output, as it's probably setup through aws or azure or gcp. Alternately I could install a node package to handle the image to text conversion, but I don't know if that's allowed, this would be my first ticket, I'm really just trying to get a push up so I can apply for permanent open source positions.

Ollyws commented 3 months ago

@m-natarajan Can we get some more detailed reproduction steps please?

melvin-bot[bot] commented 3 months ago

@Ollyws @abekkala this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

puneetlath commented 2 months ago

@puneetlath Any chance you could explain the reproduction steps for this flow? I'm not aquainted with it and the repro steps aren't very clear. Thanks!

  1. Import a personal card (this can be done on OldDot)
  2. Scan a receipt for an expense that was incurred on the imported card
  3. Wait for the card expense and the receipt to merge
Ollyws commented 2 months ago

@Chefkj The problem with your solution is that it will mark all successfully scanned receipts as card reciepts.

It seems the issue here lies in the fact that isCardTransaction is using isCorporateCard which checks if the card is present in ONYXKEYS.CARDLIST and this data doesn't seem to reflect imported credit cards or transactions.

Chefkj commented 2 months ago

@Ollyws Sorry I added further information in a comment, I updated the proposal with the information from the comment I made regarding the solution to the issue.

Ollyws commented 2 months ago

@Chefkj

Additionally, when you go to search and expenses, the svg graphic has not updated to the card svg

This seems to be successfully showing the card icon for me:

Screenshot 2024-08-19 at 16 36 21

The transaction.cardID seems to exist, it's just doesn't exist in ONYXKEYS.CARD_LIST so isCorporateCard returns false, which is causing cash to show instead of card.

Chefkj commented 2 months ago

Then this should work, is there a test card I can use to test it? or do I need to add my card and find a receipt?

 if (isCardTransaction || undefined ? transaction?.cardID != null: true) {
        if (formattedOriginalAmount) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.original')} ${formattedOriginalAmount}`;
        }
        if (isCancelled) {
            amountDescription += ` ${CONST.DOT_SEPARATOR} ${translate('iou.canceled')}`;
        }
melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

abekkala commented 2 months ago

@Ollyws can you check the comment from @Chefkj above?

Ollyws commented 2 months ago

Asked about testing this in slack.

Chefkj commented 2 months ago

This is my first ticket, I submitted a request for slack channel access but still haven't received any response

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 months ago

@Ollyws, @abekkala Eep! 4 days overdue now. Issues have feelings too...

abekkala commented 2 months ago

@Ollyws are you still waiting for testing access?

melvin-bot[bot] commented 2 months ago

@Ollyws @abekkala this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 2 months ago

@Ollyws, @abekkala Still overdue 6 days?! Let's take care of this!

Ollyws commented 2 months ago

Yeah still discussing how to set up a test card for this one.

Chefkj commented 2 months ago

@Ollyws My kiddo just got a gift card for his birthday and spent it on pokemon cards I have a receipt for, do you think that'd work?

Ollyws commented 2 months ago

It's ok, a solution for testing cards is being worked on internally as C+ need to test alot of issues like this.

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 months ago

@Ollyws, @abekkala Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 months ago

@Ollyws, @abekkala Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 months ago

@Ollyws, @abekkala 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Ollyws commented 2 months ago

Testing solution still being worked on internally.

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 months ago

@Ollyws, @abekkala Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

@Ollyws, @abekkala Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 1 month ago

@Ollyws, @abekkala Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 1 month ago

@Ollyws, @abekkala 10 days overdue. Is anyone even seeing these? Hello?

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 14 days. @Ollyws, @abekkala eroding to Weekly issue.

abekkala commented 1 month ago

@Ollyws do you have a test card for this one?

Ollyws commented 1 month ago

Not yet, it's still being discussed in slack

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

abekkala commented 1 month ago

as I'm going ooo until Oct 20, Corrine will be watching this one for me until I return.

@OfstadC This one is old, but we're waiting on some cards for testing. Slack thread: https://expensify.slack.com/archives/C02NK2DQWUX/p1728655992233659?thread_ts=1724811067.380789&cid=C02NK2DQWUX

At that point, we'll be able to review/select a viable proposal for fix.

melvin-bot[bot] commented 4 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸