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.5k stars 2.85k forks source link

[HOLD for payment 2024-08-26][$250] Report title is incorrect for IOU grouped expenses in search page #45414

Closed m-natarajan closed 2 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.6-6 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: @shawnborton Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1721056962485279

Action Performed:

Have IOU between 2 users

  1. Open app and go to search > shared page
  2. Verify the title for "IOU expenses"
  3. Click on the report and see the title

    Expected Result:

    Report title should match when viewing the grouped expenses and actually open up the report

    Actual Result:

    The report title used for the grouped expenses differs from what you see once you actually open up the report

    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
    • [ ] MacOS: Desktop

Screenshots/Videos

CleanShot 2024-07-15 at 17 23 00@2x CleanShot 2024-07-15 at 17 21 55@2x Snip - (2) New Expensify - Google Chrome

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @VictoriaExpensify
Issue OwnerCurrent Issue Owner: @getusha
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010b7fa06f0e22986d
  • Upwork Job ID: 1816886957940631192
  • Last Price Increase: 2024-08-02
  • Automatic offers:
    • DylanDylann | Contributor | 103415918
    • nkdengineer | Contributor | 103430472
melvin-bot[bot] commented 3 months 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.

shawnborton commented 3 months ago

cc @luacmartins in case you want to be assigned here

nyomanjyotisa commented 3 months ago

Proposal

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

Report title is incorrect for IOU grouped expenses in search page

What is the root cause of that problem?

In ReportDetailsPage we got the report title using ReportUtils.getReportName(report) here

But in Search Page, we don't use that and directly got the report title from reportName which is IOU for iou report

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

On getReportSections replace the reportName value with ReportUtils.getReportName(reportItem) to make it consistent

Use the following code here

const report = ReportUtils.getReport(reportItem.reportID);
const reportName = ReportUtils.getReportName(report);

reportIDToTransactions[reportKey] = {
    ...reportItem,
    reportName,
    keyForList: reportItem.reportID,
    from: data.personalDetailsList?.[reportItem.accountID],
    to,
    transactions,
};

RESULT

https://github.com/user-attachments/assets/dea137bb-1127-4efa-a916-942d15fed154

What alternative solutions did you explore? (Optional)

nkdengineer commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-06T14:20:00Z.

Proposal

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

The report title used for the grouped expenses differs from what you see once you actually open up the report

What is the root cause of that problem?

The iou report has reportName as IOU and we use this to display as the report title for iou grouped. While in header or report detail page we're using getReportName function to display the report title.

Screenshot 2024-07-16 at 12 25 49

https://github.com/Expensify/App/blob/7d58aa6694f8ef6eb298c35526a154f7e374fdd6/src/libs/SearchUtils.ts#L172

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

For the iou report, we can create a util to get the report name of this report based on the search data like this

function getIOUReportName(data: OnyxTypes.SearchResults['data'], reportItem: SearchTransaction & Record<string, SearchPersonalDetails> & SearchPolicyDetails & SearchReport) {
    const payerPersonalDetails = data.personalDetailsList?.[reportItem.managerID] as SearchAccountDetails;
    const payerName = payerPersonalDetails?.name || payerPersonalDetails?.displayName || payerPersonalDetails?.login || translateLocal('common.hidden');
    const formattedAmount = CurrencyUtils.convertToDisplayString(reportItem.total ?? 0, reportItem?.currency ?? CONST.CURRENCY.USD);
    if (reportItem.action === 'view') {
        return translateLocal('iou.payerOwesAmount', {payer: payerName, amount: formattedAmount});
    }

    return translateLocal('iou.payerPaidAmount', {
        payer: payerName,
        amount: formattedAmount,
    });
}

And then update the report name here for iou report

const isIOUReport === reportItem.type = CONST.REPORT.TYPE.IOU;
reportName: isIOUReport ? getIOUReportName(data, reportItem) : reportItem.reportName,

https://github.com/Expensify/App/blob/66ae37403897f907d26e5dcb3b1b3744ea1055da/src/libs/SearchUtils.ts#L186

What alternative solutions did you explore? (Optional)

Result

Screenshot 2024-08-05 at 15 24 44
luacmartins commented 3 months ago

@nkdengineer we can't reliably use ReportUtils.getReportName because the policy and report data might not be readily available in Onyx for Search results. We need to compute the reportName in the server.

melvin-bot[bot] commented 3 months ago

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

luacmartins commented 3 months ago

No updates yet.

luacmartins commented 3 months ago

This is a tricky one. We don't save the IOU reportName in the DB, we just save IOU and we compute it in the frontend as mentioned in the previous proposals. The issue is that because we can't rely on Onyx having the reports/policies returned by Search, we can't directly use the method. We also don't return the full report/policy object in Search so we can't run the same logic on Search data.

I think we need to implement a new simplified function to get the IOU report name, relying only on data available in Search

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~010b7fa06f0e22986d

melvin-bot[bot] commented 3 months ago

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

nkdengineer commented 3 months ago

Updated proposal https://github.com/Expensify/App/issues/45414#issuecomment-2230067269

nkdengineer commented 3 months ago

@luacmartins I just updated my proposal that will only use the data in search and the data that is returned in OpenApp API to get the report name of the money request report. What do you think about the required BE change and the solution?

luacmartins commented 3 months ago

and the data that is returned in OpenApp API to get the report name of the money request report

We can't rely on this since Search could return reports not returned by OpenApp. The solution needs to use only data from Search.

nkdengineer commented 3 months ago

@luacmartins My solution will only use search data and policy data, will not use the report data.

luacmartins commented 3 months ago

search data and policy data

What do you mean by policy data? That's also included in the search results, so we should use what we have in Search.

nkdengineer commented 3 months ago

@luacmartins Thanks, I checked and see it now in Search data. The policy and personal detail we can use the search data.

About these field that I needed in my proposal, does that makes sense and can we investigate this from BE side?

  1. hasTitleReportField
  2. oldPolicyName
  3. stateNum
  4. statusNum
  5. hasNonReimbursableTransactions
  6. isWaitingOnBankAccount
  7. ownerAccountID
melvin-bot[bot] commented 3 months ago

@luacmartins @VictoriaExpensify @getusha 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!

melvin-bot[bot] commented 3 months ago

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

getusha commented 3 months ago

Not overdue

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

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

luacmartins commented 2 months ago

I'm not sure if we should be adding those fields to the search response yet, since that's a lot of additional logic. We might need to discuss the solution further.

nkdengineer commented 2 months ago

@luacmartins These fields are necessary for expense report when the title field is not enabled. If we only want a solution for iou report, we don't need this. The iou report only has two cases: manager owns ... and manager paid ... and all necessary data already exist in search data.

luacmartins commented 2 months ago

I think we can go with a solution for iou reports only for now. We seem to save the expense report title in the DB, so hopefully those are in sync with the frontend.

nkdengineer commented 2 months ago

Let me update my proposal.

nkdengineer commented 2 months ago

@luacmartins I updated my proposal

luacmartins commented 2 months ago

Thanks! @getusha could you take a look at the proposal please?

getusha commented 2 months ago

@DylanDylann will take over, reassigning due to low bandwidth

melvin-bot[bot] commented 2 months ago

πŸ“£ @DylanDylann πŸŽ‰ 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 πŸ“–

DylanDylann commented 2 months ago

@nkdengineer How you can verify this point

The iou report only has two cases: manager owns ... and manager paid ...

I see an exception case when we request a scan request

Screenshot 2024-08-06 at 19 34 36
nkdengineer commented 2 months ago

@DylanDylann We're fixing the report name of the iou report. The image that you mentioned is the report name of the transaction thread report.

nkdengineer commented 2 months ago

The iou report only has two cases: manager owns ... and manager paid ...

I'm following the case in getMoneyRequestReportName and we only have these cases for the iou report.

DylanDylann commented 2 months ago

The image that you mentioned is the report name of the transaction thread report.

Ahh, my bad

DylanDylann commented 2 months ago

https://github.com/Expensify/App/blob/aae2e0164bf0aa3d1f672618ff78f3b166126fdc/src/libs/ReportUtils.ts#L2702-L2706

@nkdengineer How about this case?

nkdengineer commented 2 months ago

@DylanDylann I don't think p2p transactions will have non-reimbursable transaction.

@luacmartins Can you please confirm, thanks.

luacmartins commented 2 months ago

Agreed, all P2P transactions are reimbursable only

DylanDylann commented 2 months ago

@nyomanjyotisa As explained here, we should only the search data (snapshot_)in the search page

@nkdengineer's proposal looks good to me

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

melvin-bot[bot] commented 2 months ago

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

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

nkdengineer commented 2 months ago

@DylanDylann The PR is here.

luacmartins commented 2 months ago

PR was deployed to prod on Aug 19

VictoriaExpensify commented 2 months ago

Payment summary: Contributor: @nkdengineer paid $250 via Upwork Contributor+: @DylanDylann paid $250 via Upwork

Thanks for your work on this issue!