Remove getTransaction

[luacmartins]

Problem

It breaks the reactivity of the application because the method itself is not subscribed to Onyx changes. So when a page uses the method to display information about the transaction, e.g. here and the transaction changes, the view is not updated with that change.

Solution

Remove getTransaction and pass transaction as argument instead

Issue Owner

Current Issue Owner: @

[melvin-bot]

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

[melvin-bot]

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

[FitseTLT]

🚨 Edited by proposal-police: This proposal was edited at 2025-05-13 14:11:11 UTC.

Proposal

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

Remove getTransaction

What is the root cause of that problem?

Refactor

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

Remove getTransaction usages

1..tsx files and hooks like useSelectedTransactionsActions - get transaction subscribing to useOnyx transaction key in all instances we are using it in tsx files
For instance in here

App/src/pages/TransactionDuplicate/Review.tsx

Line 41 in 074a9fd

const transactions = transactionIDs.map((item) => getTransaction(item)).sort((a, b) => new Date(a?.created ?? '').getTime() - new Date(b?.created ?? '').getTime());

 
 const [transactions] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}`, {
        selector: (trans) =>
            transactionIDs.map((item) => trans?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${item}`]).sort((a, b) => new Date(a?.created ?? '').getTime() - new Date(b?.created ?? '').getTime()),
    });

2. .ts files - We will remove the instances we use getTransaction to get transaction and we will make the transaction data to be passed as a param for instance for this case
   

   App/src/libs/ReportUtils.ts

   Line 2387 in 074a9fd

   const transaction = getTransaction(iouTransactionID ?? CONST.DEFAULT_NUMBER_ID);

   
   We will change iouTransactionID param with iouTransaction and pass it wherever we use the function, for instance,
   

   App/src/pages/ReportDetailsPage.tsx

   Line 336 in 074a9fd

   const isCardTransactionCanBeDeleted = canDeleteCardTransactionByLiabilityType(iouTransactionID);

 const [iouTransaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${iouTransactionID}`, )
    const isCardTransactionCanBeDeleted = canDeleteCardTransactionByLiabilityType(iouTransaction);
   

function canDeleteCardTransactionByLiabilityType(iouTransaction?: OnyxEntry<Transaction>): boolean {

BTW, we should apply the above changes for cases we are using the transaction data to display and for other cases, especially in util .ts files, that do not need reactivity we can add an Onyx.connect to the actions file and get the transaction directly from it. The perfect example is here

App/src/libs/actions/IOU.ts

Line 8199 in 074a9fd

const transaction = getTransaction(transactionID);

It is used in actions like pay and approve money request so no need for reactivity as the data is fetched when the action is triggered by the user and used immediately

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N / A

What alternative solutions did you explore? (Optional)

[PiyushChandra17]

Proposal

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

Remove getTransaction

What is the root cause of that problem?

Improvement

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

We should remove getTransaction here and pass argument instead

App/src/libs/TransactionUtils/index.ts

Lines 1256 to 1258 in c7d6a67

	function getTransaction(transactionID: string | number | undefined): OnyxEntry<Transaction> {
	return allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
	}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

Result

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[nabi-ebrahimi]

Proposal

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

This component uses getTransaction() to retrieve transaction data imperatively. However, getTransaction() is not reactive — meaning any updates to the transaction in Onyx will not automatically re-render this view. As a result, if the transaction data changes (e.g., it's updated or removed), the user interface becomes outdated and misleading.

What is the root cause of that problem?

The root cause is the use of a non-reactive function (getTransaction()) to retrieve transaction data outside the Onyx subscription system. Since getTransaction() does not subscribe to Onyx updates, changes to any transaction object are not reflected in the component, breaking the reactivity model expected across the app.

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

We should remove the use of getTransaction() entirely and instead subscribe reactively to the needed transactions using useOnyx. Here's how we can do that in this component:

Code Change Summary

1. Remove this line:

   const transactions = transactionIDs.map((item) => getTransaction(item)).sort(...);

2. Replace it with a useOnyx call that subscribes to all relevant transactions:

   const [transactionsMap] = useOnyx(transactionIDs.map(id => `${ONYXKEYS.COLLECTION.TRANSACTION}${id}`));

3. Transform the map into an array of transactions, preserving sort order:

   const transactions = transactionIDs
       .map(id => transactionsMap?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`])
       .filter(Boolean)
       .sort((a, b) => new Date(a?.created ?? '').getTime() - new Date(b?.created ?? '').getTime());

4. All references to transactions in this component will continue to work without change, but now they will update reactively if any transaction is modified.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

[dominictb]

Thanks for proposals everyone. We should clean up all getTransaction usages.

I have this specific case and wonder how you're gonna clean it up:

App/src/libs/actions/IOU.ts

Line 8202 in 12eb120

return {holdReportActions, holdTransactions};

Also please create test branch and attach evidence that performance test won't fail.

[FitseTLT]

Thanks for proposals everyone. We should clean up all getTransaction usages.

I have this specific case and wonder how you're gonna clean it up:

App/src/libs/actions/IOU.ts

Line 8202 in 12eb120

return {holdReportActions, holdTransactions};
Also please create test branch and attach evidence that performance test won't fail.

@dominictb This case is used for payMoneRequest and approveMoneyRequest which use the transaction data on its trigger via user's action there is no need for reactivity we can use getTransaction for these cases. There are other examples we use similar functions like here

App/src/libs/actions/IOU.ts

Line 9141 in 58ea580

const transactions = getReportTransactions(expenseReport.reportID).filter((transaction) => isDuplicate(transaction.transactionID, true));

cc @luacmartins

[luacmartins]

@dominictb This case is used for payMoneRequest and approveMoneyRequest which use the transaction data on its trigger via user's action there is no need for reactivity we can use getTransaction for these cases. There are other examples we use similar functions like here

I think that for actions like the one above, we can add an Onyx.connect to the actions file and get the data from it directly (ideally we make the function pure though). Let's not keep a method like getTransactions around, because as we've seen, it starts being used in incorrect places.

[FitseTLT]

@dominictb This case is used for payMoneRequest and approveMoneyRequest which use the transaction data on its trigger via user's action there is no need for reactivity we can use getTransaction for these cases. There are other examples we use similar functions like here

I think that for actions like the one above, we can add an Onyx.connect to the actions file and get the data from it directly (ideally we make the function pure though). Let's not keep a method like getTransactions around, because as we've seen, it starts being used in incorrect places.

Makes Sense. So I have already mentioned all different types of cases to handle in my proposal Other details left should be handled in the PR phase WDYT @dominictb @luacmartins

[dominictb]

Okay I think @FitseTLT proposal is the most detailed. Let's go with them.

Let's remove all getTransaction usages. If we really need to use it, make it pure and only use locally (without exporting), and add unit test to prevent exporting it in the future:

App/tests/actions/EnforceActionExportRestrictions.ts

Lines 67 to 72 in c36fc9d

	describe('OptionsListUtils', () => {
	it('does not export getReport', () => {
	// @ts-expect-error the test is asserting that it's undefined, so the TS error is normal
	expect(OptionsListUtils.getReportOrDraftReport).toBeUndefined();
	});
	});

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[dominictb]

@FitseTLT Ping me once PR is ready.