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
2.99k stars 2.5k forks source link

[$250] No indication of scan in progress when clicking in to the expense when the receipt is scanning #40828

Open m-natarajan opened 3 weeks ago

m-natarajan commented 3 weeks 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: 1.4.64-0 Reproducible in staging?: yes Reproducible in production?: yes 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/p1713883047268849

Action Performed:

  1. Create scan receipt expense as employee
  2. Open the report when scanning is in progress as approver or employee

Expected Result:

Should have some indication for scanning

Actual Result:

No indication as receipt is scanning

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/ae2628c5-24e0-4b2e-8328-e6451a897840

Screenshot 2024-04-23 at 10 35 01 AM

Screenshot 2024-04-23 at 10 34 55 AM

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016bf55aab1c62849f
  • Upwork Job ID: 1785313474230407168
  • Last Price Increase: 2024-04-30
  • Automatic offers:
    • rojiphil | Reviewer | 0
    • bernhardoj | Contributor | 0
melvin-bot[bot] commented 3 weeks ago

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

bernhardoj commented 3 weeks ago

Proposal

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

There is no indication of the receipt is scanning in one transaction page.

What is the root cause of that problem?

In one transaction page, we use MoneyReportHeader. In MoneyReportHeader, we don't have a scanning information component like we do in MoneyRequestHeader. https://github.com/Expensify/App/blob/915a1b64400995032829004245d6dbf7aeeb4424/src/components/MoneyRequestHeader.tsx#L193-L199

Also, the header title is taken from the money report name, not from the transaction thread name. If we open a transaction thread, then we will see the title is Scan in progress.. along with the scanning information component.

image

That's becuase in getMoneyRequestReportName, there is no case to return Scan in progress.. string, while in getTransactionReportName, we get the linked transaction and check it's scan status. https://github.com/Expensify/App/blob/915a1b64400995032829004245d6dbf7aeeb4424/src/libs/ReportUtils.ts#L2645-L2647

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

We need to add the scan information component to MoneyReportHeader too. First, we need to get the transaction data. To get the transaction data, we need the transaction parent report action. We already have it here, https://github.com/Expensify/App/blob/915a1b64400995032829004245d6dbf7aeeb4424/src/components/MoneyReportHeader.tsx#L64-L69

but since it's not a props, we need to create a new component (let's call it MoneyRequestHeaderScanStatusBar) which will accept a parent action then get the transaction data from it.

<MoneyRequestHeaderScanStatusBar parentReportAction={requestParentReportAction ?? null} />

...

function MoneyRequestHeaderScanStatusBar({transaction}: MoneyRequestHeaderScanStatusBarProps) {
    const {translate} = useLocalize();
    const isScanning = TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction);

    return isScanning && (
        <MoneyRequestHeaderStatusBar
            title={translate('iou.receiptStatusTitle')}
            description={translate('iou.receiptStatusText')}
            shouldShowBorderBottom={false}
        />
    );
}

export default withOnyx<MoneyRequestHeaderScanStatusBarProps, MoneyRequestHeaderScanStatusBarOnyxProps>({
    transaction: {
        key: ({parentReportAction}) => `${ONYXKEYS.COLLECTION.TRANSACTION}${parentReportAction?.originalMessage?.IOUTransactionID}`
    }
})(MoneyRequestHeaderScanStatusBar);
Screenshot 2024-04-24 at 15 27 23

If we want to show the header title as "Scan in progress..", then we need to pass transactionThreadReport from MoneyReportHeader down to AvatarWithDisplayName and use transactionThreadReport to get the report name when availlable.

const title =  ReportUtils.getReportName(transactionThreadReport ?? report);

However, this will show the title differently then what we have now, main - using money report header title logic

image

new changes - using transaction thread title logic

image

If we only want to show Scan in progress.. and Receipt missing details and follow the rest of the money report header title logic, then we need to pass transactionThreadReport as a new param and if transactionThreadReport exists and it's scanning, then return Scan in progress..

melvin-bot[bot] commented 2 weeks ago

@miljakljajic Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~016bf55aab1c62849f

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

rojiphil commented 2 weeks ago

Will review the proposal tomorrow and update.

rojiphil commented 1 week ago

@bernhardoj Thanks for the proposal. I agree we need to add support for Scan in progress title and scanning status bar in MoneyReportHeader. However, I am curious to know why we need to show scanning and next steps status bar together. Instead, don’t we need to show scanning status bar only if all the requests are being scanned? Otherwise, we can show Next steps status bar as it is currently shown. What do you think? Also, let us call MoneyReportHeaderScanStatusBar instead of MoneyRequestHeaderScanStatusBar.

bernhardoj commented 1 week ago

Instead, don’t we need to show scanning status bar only if all the requests are being scanned?

The scanning status bar will only show if there is one request and it's scanning.

I also like to hide the Next Step when the scan status bar is shown, but that depends on the team decision.

rojiphil commented 1 week ago

The scanning status bar will only show if there is one request and it's scanning.

Since the Report Preview shows Scanning in progress when all requests are being scanned, we can expect the same in Expense Report too. So, let us do it this way unless we have good reasoning to do otherwise at the PR stage.

rojiphil commented 1 week ago

I also like to hide the Next Step when the scan status bar is shown, but that depends on the team decision.

Showing Next Step does not make sense when scanning is in progress. So, let us hide this while displaying Scanning in progress unless we have good reasoning to do otherwise at the PR stage.

bernhardoj commented 1 week ago

I think the reason this issue is created is because we don't know whether a one transaction report is in scanning or not. If we have multiple requests, then we don't need to write Scan in progress in the header anymore IMO.

image
rojiphil commented 1 week ago

If we have multiple requests, then we don't need to write Scan in progress in the header anymore IMO.

As seen here, we show Scan in progress in Report Preview when all the requests are being smart scanned. So, it is expected to see scan in progress status in the Expense Report in this scenario. no?

rojiphil commented 1 week ago

I think the reason this issue is created is because we don't know whether a one transaction report is in scanning or not.

hmm.. Not sure if I understood your comment. However, my understanding of the issue is that there is no indication of scan in progress in Expense Report when the Report Preview does. Are we on the same page on this?

bernhardoj commented 1 week ago

My thought is, that in the transaction thread, you can see that the transaction receipt scan is in progress from the header.

image

But if you view the transaction from the (IOU/expense) one transaction report, the scan indication in the header doesn't appears. It's like an inconsistency since one transaction report looks like a transaction thread.

image

Btw, the scanning indication is now exist in the Amount and Merchant field.

I'm fine if we want to show Scan in progress when all transactions are scanning, but I think we should confirm with the team again.

rojiphil commented 1 week ago

I'm fine if we want to show Scan in progress when all transactions are scanning, but I think we should confirm with the team again.

That’s correct. The internal engineer/design team would also weigh in with their inputs. But, let’s deal with all this at the PR implementation stage: As per my understanding, the following needs to be done: 1) Show Scan in progress title in Expense Report when all requests are being smart scanned. Reasoning: The Report Preview in chat report shows as Scan in progress when all the requests are being smart scanned. Also, we display this title whenever the scanning... status bar is displayed. 2) Show Scanning… status bar instead of Next steps when all requests are being smart scanned. Reasoning: Showing Next steps does not seem right when all the requests are being smart scanned. Instead show scanning… status bar.

@bernhardoj proposal LGTM as indication of scanning can be shown via status bar and title in Expense Report. Further comments as mentioned above can be confirmed during the implementation. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

mvtglobally commented 1 week ago

Issue not reproducible during KI retests. (First week)

thienlnam commented 1 week ago

Looks good, seems like this current issue is no longer reproducible but seems like there's some additional improvements that are discussed here

melvin-bot[bot] commented 1 week ago

📣 @rojiphil 🎉 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

📣 @bernhardoj 🎉 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 📖

melvin-bot[bot] commented 1 week ago

@rojiphil @miljakljajic @thienlnam @bernhardoj 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!

bernhardoj commented 1 week ago

I just realized that if we show Scan in progress for the one transaction report title, then we will have both title and subtitle as Scan in progress

image

Is that fine?

rojiphil commented 1 week ago

I just realized that if we show Scan in progress for the one transaction report title, then we will have both title and subtitle as Scan in progress. Is this fine?

@bernhardoj I think this one on the Sidebar may be intentional. But how is it connected to our issue related to the status bar/title on the Expense Report?

bernhardoj commented 1 week ago

But how is it connected to our issue related to the status bar/title on the Expense Report?

because both LHN title and report header title use ReportUtils.getReportName, so if we change it to show Scan in progress if all transactions are scanning, then that double Scan in progress... will happen.

rojiphil commented 1 week ago

because both LHN title and report header title use ReportUtils.getReportName, so if we change it to show Scan in progress if all transactions are scanning, then that double Scan in progress... will happen

@bernhardoj Ah! Got it. Thanks and that may make sense. Are you saying that showing Scanning in progress as title does not seem right? i.e. Point mentioned here is not required.

melvin-bot[bot] commented 1 week ago

@rojiphil @miljakljajic @thienlnam @bernhardoj 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!

bernhardoj commented 1 week ago

@rojiphil yep, I think we should just add the "Scanning" status bar for a one-transaction report. What do you think?

rojiphil commented 1 week ago

Sounds cool. So, point (1) here does not make sense. But I am unclear on what you meant by one-transaction report. Can you please bring the PR up? We can discuss further there.

bernhardoj commented 1 week ago

PR is ready

cc: @rojiphil