[Due for payment 2025-05-22] [$125] Do not create optimistic transaction thread report on RequestMoney

[iwiznia]

Related to https://github.com/Expensify/Expensify/issues/433103

Conversation https://expensify.slack.com/archives/C08CZDJFJ77/p1742395185178209

The main goal of the project is to stop creating the transaction thread reports optimistically and instead have those be created when they are needed.

I have already deployed the PRs in the backend that make it so that we do not create the report in RequestMoney api request if we don't pass a transactionThreadReport and have this draft PR #58523 that stops sending it.

Issue Owner

Current Issue Owner: @sakluger / @abekkala

[VickyStash]

Re-posting updates for today:
I've opened a new Draft PR with updates, did a brief testing and it works as expected - no transaction thread is created.
Tomorrow I'm going to test my PR over the PR that creates transactionThread when needed, also I'm going to do some testing with offline flows to check how it works.

[VickyStash]

Today, during testing, I have found some issues related to not creating optimistic transaction threads.
Through the app, we have a lot of logic relying on checking if the report has only one transaction thread:
isOneTransactionThread
getOneTransactionThreadReportID

Example, of side effects:

1. I create 3 expenses in 1-1 chat. This way I have a report with several transaction.
2. I open one of the transactions -> transaction thread report is created for 1 out of 3 transactions.
3. Then, if I from the chat try to open the expenses overview, the app thinks that I have one-transaction-report and just shows that specific transaction thread (see the video below).

bug_example1.mp4

I'll need to look through every case in the app where it checks for one-transaction-thread and think about the alternative, maybe checking for transactions on its own.

[VickyStash]

I'm not sure that we can replace all of the one-transaction-thread checks (see my message above) through the app with checks for just transactions since transactions are not returned in the OpenApp response.

@iwiznia @mountiny Was it discussed before?

[VickyStash]

Another thing is offline behavior.

When I test my updates over the PR that creates a transaction thread when needed , in the offline opening, the transaction thread gives endless loading.
Is it the expected behavior?

offline_example2.mp4

[mountiny]

The OpenApp will definitely not return all the transactions for you, it probably should not return any once we have all the violations migrated to Auth. OpenReport will load those you need for the report.

[VickyStash]

The OpenApp will definitely not return all the transactions for you, it probably should not return any once we have all the violations migrated to Auth. OpenReport will load those you need for the report.

Okay, that means the app doesn't have information about report transactions before the report is opened.

Right now, the app uses transaction threads to identify the number of transactions in the report and what logic to apply.
If we update the app logic not to create transaction threads right away, which data can I use to handle some checks in case I haven't opened the report?

For example, 3 weeks ago some updates were merged, they includes checking for one-transaction-tread reports from the LHN level.

[iwiznia]

Is it the expected behavior?

No.

Right now, the app uses transaction threads to identify the number of transactions in the report and what logic to apply.
If we update the app logic not to create transaction threads right away, which data can I use to handle some checks in case I haven't opened the report?

I don't think I am following, I see that getOneTransactionThreadReportID only uses the reportActions not the transactions.

[VickyStash]

I don't think I am following, I see that getOneTransactionThreadReportID only uses the reportActions not the transactions.

I mean since we won't create transaction threads right away, all of the checks that use getOneTransactionThreadReportID/isOneTransactionThread won't work as expected

[iwiznia]

Maybe we can assume that if there are no actions is because we will optimistically create the transaction thread?

[VickyStash]

Okay, it seems like I was able to fix the one-thread-transactions checks just by suppressing the action.childReportID checks inside getOneTransactionThreadReportID function. I've tested and it stills works as expected thanks to other checks we have there.
Thank you for thoughts sharing! 🙌

@iwiznia What about the offline flow expected behaviour? What should users see if they are offline -> create expense -> try to open a transaction thread?

[iwiznia]

@iwiznia What about the offline flow expected behaviour? What should users see if they are offline -> create expense -> try to open a transaction thread?

They should see the correct page, same as if they were online (but greyed out like in your video)

[VickyStash]

They should see the correct page, same as if they were online (but greyed out like in your video)

Hm, so should I generate it optimistically? User didn't open it before online, so it wasn't generated on the BE.

[iwiznia]

Yeah, it should be created optimistically when the user opens the report and ensure we pass the correct parameters when the API call is done so that the backend creates it. I think some of that is already being done in #58097 so we can probably reuse some of that code.

[VickyStash]

Got it! just to confirm, does the OpenReport API call support this logic? I don't see an option to pass optimisticTransactionThreadID to the OpenReport API call, otherwise the BE will create the thread with another ID and there will be duplicates

[iwiznia]

I assume it works, since it seems that's what this is doing? @rlinoz can you double check?

[rlinoz]

Yeah, I think we are using the same mechanism as thread creation, so the BE will handle duplicate reports as long as you pass the same reportActionID.

[rlinoz]

Also, to be considered, in the linked PR I didn't create the offline state, currently it will just keep in a loading state until you go online, so if we could create the optimistic state here it would be great

[VickyStash]

Updates:

• Fixed tests (it was failing with the timeout)
• Tested my PR over Make ReportScreen create transactionThread when needed.

Also, to be considered, in the linked PR I didn't create the offline state, currently it will just keep in a loading state until you go online, so if we could create the optimistic state here it would be great

Yep, I can handle it in the PR I'm working on! I have already stashed the necessary updates!

@rlinoz Just one thing I have noticed. Your PR doesn't handle the case when there is only one expense in the 1-1 chat, so when a user taps ReportPreview he directly navigates to the transaction thread report chat. You handle the thread creation in the MoneyRequestAction component, but not in the ReportPreview. I can cover it in my PR tough!

For the next steps (final PR polishing and preparing for the review), we need to wait till #58097 is merged!

[rlinoz]

Ah yes, I am mainly fixing the cases for the selfDM, thanks for covering it!

[VickyStash]

Note: I'll be OOO 1-5th of May 🌴

[melvin-bot]

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.

[sakluger]

I will be OOO from May 5 - 16, so I have added another BZ member to watch over the issue while I'm out.

Notes: This was completed, but there was a regression, so I halved the price, and we are waiting for a follow-up PR.

[abekkala]

@VickyStash Can you catch me up on this issue status?
Has the follow-up PR been created yet? If so, can you link it here please.

Thanks!

[hungvu193]

@VickyStash Can you catch me up on this issue status? Has the follow-up PR been created yet? If so, can you link it here please.

Thanks!

The follow-up is up (#60292). I'm still working on it

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[SzymczakJ]

Hi, coming from [$250] Expense - Expense report does not show transaction details and it only shows the total which was also regression from the reverted PR.
Could you add this transaction thread creation(that happens in ReportScreen) to SearchMoneyRequestReportPage? That screen bases 90% of it's logic on ReportScreen and it should also create optimistic transaction thread, if it's not already present. If you have any questions about that feel free to ask me here or on Slack 😄

[VickyStash]

UPD: I've started to prepare a new PR with fixes!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.45-21 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Stop creating the transaction thread reports optimistically for RequestMoney #60292

If no regressions arise, payment will be issued on 2025-05-22. 🎊

For reference, here are some details about the assignees on this issue:

• @hungvu193 requires payment through NewDot Manual Requests
• @VickyStash does not require payment (Contractor)

[melvin-bot]

@hungvu193 @sakluger / @abekkala @hungvu193 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[VickyStash]

The issue shouldn't be closed as the PR was reverted! cc @iwiznia

[VickyStash]

UPD: I needed to switch to another issue today, I'm going to focus on this one on Monday!