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.34k stars 2.77k forks source link

Invoices - Hold option is present in context menu before invoice report is opened #47570

Open IuliiaHerets opened 1 month ago

IuliiaHerets 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: v9.0.21-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4864172 Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to FAB > Send invoice.
  3. Enter amount, select any user and send the invoice.
  4. Without clicking on the invoice preview (important), right click on the invoice preview in invoice chat.
  5. Note that Hold option is present.
  6. Click on the invoice preview.
  7. Return to invoice chat.
  8. Right click on the invoice preview.
  9. Note that Hold option disappears.

Expected Result:

In Step 5, Hold option should not appear for invoice because invoices cannot be held.

Actual Result:

In Step 5, Hold option appears for invoice. In Step 9, Hold option disappears after user opens the invoice report.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/c9742567-929c-47e0-bdd2-3d8841f59354

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @abdulrahuman5196
melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

We think that this bug might be related to #vip-bills

IuliiaHerets commented 1 month ago

@VictoriaExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

codewaseem commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-17 16:16:35 UTC.

Proposal

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

The hold option appears in the context menu which should not happen for invoices.

What is the root cause of that problem?

The areHoldRequirementsMet condition doesn't exclude invoice type here:

https://github.com/Expensify/App/blob/6aa873d9eb69c48719f9915c63fc9113a586b2f3/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L178-L180

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

Making sure areHoldRequirementsMet returns false for invoice type solves this problem. Here are the changes to be made

   const isMoneyRequestOrReport = isMoneyRequestReport || isSingleTransactionView;

    const areHoldRequirementsMet =
        !isInvoiceReport && isMoneyRequestOrReport && !ReportUtils.isArchivedRoom(transactionThreadReportID ? childReport : parentReport, parentReportNameValuePairs);

Added !isInvoiceReport to areHoldRequirementsMet and removed isInvoiceReport from isMoneyRequestOrReport which is not need.

To hide the hold option from ReportDetails view, need to update the ReportUtils.canHoldUnholdReportAction function with the following


function canHoldUnholdReportAction(reportAction: OnyxInputOrEntry<ReportAction>): {canHoldRequest: boolean; canUnholdRequest: boolean} {
    if (!ReportActionsUtils.isMoneyRequestAction(reportAction)) {
        return {canHoldRequest: false, canUnholdRequest: false};
    }

    const moneyRequestReportID = ReportActionsUtils.getOriginalMessage(reportAction)?.IOUReportID ?? 0;
    const moneyRequestReport = getReportOrDraftReport(String(moneyRequestReportID));

    if (!moneyRequestReportID || !moneyRequestReport) {
        return {canHoldRequest: false, canUnholdRequest: false};
    }

    if (isInvoiceReport(moneyRequestReport)) {
        return {
            canHoldRequest: false,
            canUnholdRequest: false,
        };
    }
   ...

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

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

VictoriaExpensify commented 1 month ago

I agree this is an issue and we should fix it!

melvin-bot[bot] commented 1 month ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 3 weeks ago

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

abdulrahuman5196 commented 3 weeks ago

Checking now

codewaseem commented 3 weeks ago

Friendly bump @abdulrahuman5196.

abdulrahuman5196 commented 3 weeks ago

Sorry checking again

abdulrahuman5196 commented 3 weeks ago

@VictoriaExpensify / @IuliiaHerets I don't understand the issue? Are we not supposed to show the Hold in the context menu, because if I go to the invoice view I can see the Hold option. If we shouldn't show the Hold option in context, under what conditions we shouldn't show?

https://github.com/user-attachments/assets/29d875cd-507c-4c32-8416-b6ada79be83b

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

@abdulrahuman5196, @VictoriaExpensify Still overdue 6 days?! Let's take care of this!

VictoriaExpensify commented 2 weeks ago

Posting to get some clarification on this one

VictoriaExpensify commented 2 weeks ago

https://expensify.slack.com/archives/CSL3XBCCR/p1725609816349509

VictoriaExpensify commented 2 weeks ago

Not overdue

VictoriaExpensify commented 1 week ago

Still getting clarification on this one!

VictoriaExpensify commented 1 week ago

Ok chatted about this one in #wave_Collect. We are not allowing Invoices to be held (if that changes, it will be way down the track), so yes - we should proceed with a fix for this one.

@abdulrahuman5196 - we don't really want the Hold option to appear at all for Invoices. I'm really sorry, but is the video you've linked there showing anything different to what was posted in the initial bug report? It looks like it's the same thing but, if I've missed something and you are seeing the Hold option for Invoices somewhere else, then a bug report should be created for it

VictoriaExpensify commented 1 week ago

bump @abdulrahuman5196

melvin-bot[bot] commented 3 days ago

@abdulrahuman5196, @VictoriaExpensify Eep! 4 days overdue now. Issues have feelings too...

abdulrahuman5196 commented 3 days ago

Checking now.

abdulrahuman5196 commented 3 days ago

Ok chatted about this one in #wave_Collect. We are not allowing Invoices to be held (if that changes, it will be way down the track), so yes - we should proceed with a fix for this one.

Got it.

we don't really want the Hold option to appear at all for Invoices. I'm really sorry, but is the video you've linked there showing anything different to what was posted in the initial bug report? It looks like it's the same thing but, if I've missed something and you are seeing the Hold option for Invoices somewhere else, then a bug report should be created for it

@VictoriaExpensify

This original OP issue is happening and the bug is also happening at a different place as pointed in the below video. Should we handle it part of this issue or create separate issue for it?

https://github.com/user-attachments/assets/897ebc58-f99a-4121-985c-2e39d204bbad

codewaseem commented 2 days ago

Proposal Updated To hide hold option from ReportDetails view.

@abdulrahuman5196