Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PAID] [$250] IOU - Fields in transaction thread appear at the top of the page in combined IOU report #39824

Closed
5 of 6 tasks
izarutskaya opened this issue Apr 8, 2024 · 30 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@izarutskaya
Copy link

izarutskaya commented Apr 8, 2024

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.61-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to 1:1 DM with user that has no unsettled IOU.
  3. Send an IOU to the user.
  4. Click on the preview.

Expected Result:

The IOU transaction thread is placed correctly.

Actual Result:

The IOU fields in transaction thread appear at the top of the page. This issue only happens in combined 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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6442196_1712579374631.one_transaction_report_view.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01323cb2c1a74d7f5e
  • Upwork Job ID: 1777640937893277696
  • Last Price Increase: 2024-04-09
  • Automatic offers:
    • ishpaul777 | Contributor | 0
Issue OwnerCurrent Issue Owner: @strepanier03
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

Triggered auto assignment to @pecanoro (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Apr 8, 2024

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Apr 8, 2024
Copy link
Contributor

github-actions bot commented Apr 8, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@izarutskaya
Copy link
Author

@strepanier03 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.

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@pecanoro
Copy link
Contributor

pecanoro commented Apr 8, 2024

Trying to figure out which PR caused this

@pecanoro
Copy link
Contributor

pecanoro commented Apr 8, 2024

It's interesting because it shows the total in the wrong place for a bit and then it gets updated to show the proper layout.

@pecanoro
Copy link
Contributor

pecanoro commented Apr 8, 2024

I could not figure out where the error is coming from so I will continue tomorrow

@nkdengineer
Copy link
Contributor

Proposal

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

The IOU fields in transaction thread appear at the top of the page. This issue only happens in combined report.

What is the root cause of that problem?

For combine report, we will render both MoneyRequestView and MoneyReportView if the transaction currency is not the same with report currency.

{transactionCurrency !== report.currency && (

But the transaction in ReportActionItem for combine report is always undefined. The reason is for combine report, we filtered out the iouPreviewAction here and then action here is undefined which leading the transaction is always undefined.

const action = reportActions?.find((reportAction) => reportAction.reportActionID === parentReportActionID);

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

We should pass parentReportActionForTransactionThread prop which is the money request action of combine report from ReportActionView to ReportActionItem and then use it here to get the transactionID

key: ({parentReportActionForTransactionThread}) => {
    const transactionID = (parentReportActionForTransactionThread as OnyxTypes.OriginalMessageIOU)?.originalMessage.IOUTransactionID ? (parentReportActionForTransactionThread as OnyxTypes.OriginalMessageIOU).originalMessage.IOUTransactionID : 0;
    return `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`;
},

const action = reportActions?.find((reportAction) => reportAction.reportActionID === parentReportActionID);

To calculate parentReportActionForTransactionThread, we can do it here when we get the combineReportActions

const [combinedReportActions, parentReportActionForTransactionThread] = useMemo(() => {
        if (isEmptyObject(transactionThreadReportActions)) {
            return [allReportActions, null];
        }

       ...
        const parentReportActionForTransactionThread = allReportActions.find((action) => {
            const actionType = (action as OnyxTypes.OriginalMessageIOU).originalMessage?.type ?? '';
            return actionType === CONST.IOU.REPORT_ACTION_TYPE.CREATE || actionType === CONST.IOU.REPORT_ACTION_TYPE.TRACK || !ReportActionsUtils.isSentMoneyReportAction(action);
        })

        ...
        return [ReportActionsUtils.getSortedReportActions(filteredReportActions, true), parentReportActionForTransactionThread];
}, [allReportActions, transactionThreadReportActions]); 

const combinedReportActions = useMemo(() => {

What alternative solutions did you explore? (Optional)

Or we can simply pass the transactionID from ReportActionView to ReportActionItem. The way to get transactionID is the same as we do for parentReportActionForTransactionThread above.

@pecanoro
Copy link
Contributor

pecanoro commented Apr 9, 2024

I think I found the PR that caused the issue: #39465. I tried reverting the PR on my dev and it fixes the bug. However, there is a bug that also exists on prod and it's the total appearing and disappearing when the action stops being pending.

@pecanoro
Copy link
Contributor

pecanoro commented Apr 9, 2024

I am going to open this to external and see if we can fix both bugs in one PR.

@pecanoro pecanoro added the External Added to denote the issue can be worked on by a contributor label Apr 9, 2024
@melvin-bot melvin-bot bot changed the title IOU - Fields in transaction thread appear at the top of the page in combined IOU report [$250] IOU - Fields in transaction thread appear at the top of the page in combined IOU report Apr 9, 2024
Copy link

melvin-bot bot commented Apr 9, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01323cb2c1a74d7f5e

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 9, 2024
Copy link

melvin-bot bot commented Apr 9, 2024

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

@pecanoro pecanoro assigned pecanoro and unassigned pecanoro Apr 9, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Hourly KSv2 labels Apr 9, 2024
@nkdengineer
Copy link
Contributor

@ishpaul777 The PR is here.

@Julesssss
Copy link
Contributor

@nkdengineer it sounds like the fix might not work. As this is blocking our deploy we'll need to see if anyone else has an idea for an alternative solution.

@nkdengineer
Copy link
Contributor

@Julesssss I updated the PR, the main idea is correct, just a mistake in a check.

@Julesssss
Copy link
Contributor

Checed this one off as we reverted the offending PR

@pecanoro pecanoro removed the DeployBlockerCash This issue or pull request should block deployment label Apr 10, 2024
@Julesssss
Copy link
Contributor

Copy link

melvin-bot bot commented May 2, 2024

This issue has not been updated in over 15 days. @pecanoro, @strepanier03, @ishpaul777, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@nkdengineer
Copy link
Contributor

@strepanier03 The PR was deployed to production last week, please help to add Weekly label again.

@pecanoro
Copy link
Contributor

pecanoro commented May 7, 2024

I am not sure what's going on with automatization to update issues for payment

@pecanoro pecanoro added Weekly KSv2 Monthly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 and removed Monthly KSv2 Weekly KSv2 Reviewing Has a PR in review labels May 7, 2024
@pecanoro pecanoro changed the title [$250] IOU - Fields in transaction thread appear at the top of the page in combined IOU report [Awaiting payment] [$250] IOU - Fields in transaction thread appear at the top of the page in combined IOU report May 7, 2024
@strepanier03
Copy link
Contributor

Thanks @pecanoro - automation has been hit or miss lately for sure.

@strepanier03
Copy link
Contributor

@nkdengineer - Invited you to the job in Upwork.
@ishpaul777 - I've finished paying you via Upwork.

@nkdengineer
Copy link
Contributor

@strepanier03 Thank you 🙇 I accepted the offer

@strepanier03
Copy link
Contributor

You're welcome!

This is all paid and contracts closed, thanks everyone!

@strepanier03 strepanier03 changed the title [Awaiting payment] [$250] IOU - Fields in transaction thread appear at the top of the page in combined IOU report [PAID] [$250] IOU - Fields in transaction thread appear at the top of the page in combined IOU report May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants