Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ function TransactionGroupListItem<TItem extends ListItem>({
const [oneTransactionThreadReport] = originalUseOnyx(`${ONYXKEYS.COLLECTION.REPORT}${oneTransactionItem?.reportAction?.childReportID}`, {canBeMissing: true});
const [oneTransaction] = originalUseOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${oneTransactionItem?.transactionID}`, {canBeMissing: true});
const parentReportActionSelector = useCallback(
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${oneTransactionItem?.moneyRequestReportActionID}`],
[oneTransactionItem?.moneyRequestReportActionID],
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${oneTransactionItem?.reportAction?.reportActionID}`],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential runtime error: When oneTransactionItem?.reportAction is undefined, the expression oneTransactionItem?.reportAction?.reportActionID evaluates to undefined. Inside the template literal \${undefined}`, this becomes the **string** "undefined", causing the selector to look up reportActions["undefined"]instead of returningundefined`.

Suggested fix: Add a null check before the template literal:

(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => {
    const reportActionID = oneTransactionItem?.reportAction?.reportActionID;
    return reportActionID ? reportActions?.[reportActionID] : undefined;
},

Or use a more concise approach:

(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => 
    oneTransactionItem?.reportAction?.reportActionID 
        ? reportActions?.[oneTransactionItem.reportAction.reportActionID] 
        : undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to what we currently have. That being said, I like the suggestion because it's more explicit. @DylanDylann let's implement it this way

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DylanDylann we can use the suggested approach to avoid reportActions["undefined"]. Also @luacmartins agrees to it.

[oneTransactionItem?.reportAction?.reportActionID],
);
const [parentReportAction] = originalUseOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${oneTransactionItem?.reportID}`, {selector: parentReportActionSelector, canBeMissing: true}, [
oneTransactionItem,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ function TransactionListItem<TItem extends ListItem>({
const [transactionThreadReport] = originalUseOnyx(`${ONYXKEYS.COLLECTION.REPORT}${transactionItem?.reportAction?.childReportID}`, {canBeMissing: true});
const [transaction] = originalUseOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionItem.transactionID}`, {canBeMissing: true});
const parentReportActionSelector = useCallback(
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${transactionItem?.moneyRequestReportActionID}`],
[transactionItem?.moneyRequestReportActionID],
(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => reportActions?.[`${transactionItem?.reportAction?.reportActionID}`],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential runtime error: When transactionItem?.reportAction is undefined, the expression transactionItem?.reportAction?.reportActionID evaluates to undefined. Inside the template literal \${undefined}`, this becomes the **string** "undefined", causing the selector to look up reportActions["undefined"]instead of returningundefined`.

Suggested fix: Add a null check before the template literal:

(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => {
    const reportActionID = transactionItem?.reportAction?.reportActionID;
    return reportActionID ? reportActions?.[reportActionID] : undefined;
},

Or use a more concise approach:

(reportActions: OnyxEntry<ReportActions>): OnyxEntry<ReportAction> => 
    transactionItem?.reportAction?.reportActionID 
        ? reportActions?.[transactionItem.reportAction.reportActionID] 
        : undefined,

[transactionItem?.reportAction?.reportActionID],
);
const [parentReportAction] = originalUseOnyx(
`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${getNonEmptyStringOnyxID(transactionItem.reportID)}`,
Expand Down
3 changes: 0 additions & 3 deletions src/components/SelectionListWithSections/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,6 @@ type TransactionListItemType = ListItem &
/** The display name of the purchaser card, if any */
cardName?: string;

/** Parent report action id */
moneyRequestReportActionID?: string;

/** The available actions that can be performed for the transaction */
allActions: SearchTransactionAction[];

Expand Down
3 changes: 1 addition & 2 deletions src/libs/SearchUIUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1388,7 +1388,7 @@ function createAndOpenSearchTransactionThread(
shouldNavigate = true,
) {
const iouReportAction = getIOUActionForReportID(item.reportID, item.transactionID);
const moneyRequestReportActionID = item.moneyRequestReportActionID !== '0' ? item.moneyRequestReportActionID : undefined;
const moneyRequestReportActionID = item.reportAction?.reportActionID ?? undefined;
Comment thread
DylanDylann marked this conversation as resolved.
const previewData = transactionPreviewData
? {...transactionPreviewData, hasTransactionThreadReport: true}
: {hasTransaction: false, hasParentReport: false, hasParentReportAction: false, hasTransactionThreadReport: true};
Expand Down Expand Up @@ -2704,7 +2704,6 @@ function getTransactionFromTransactionListItem(item: TransactionListItemType): O
isTaxAmountColumnWide,
violations,
hash,
moneyRequestReportActionID,
canDelete,
accountID,
policyID,
Expand Down
3 changes: 2 additions & 1 deletion src/libs/actions/Search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1105,7 +1105,8 @@ function getTotalFormattedAmount(selectedReports: SelectedReports[], selectedTra
* Note: we don't create anything new, we just optimistically generate the data that we know will be returned by API.
*/
function setOptimisticDataForTransactionThreadPreview(item: TransactionListItemType, transactionPreviewData: TransactionPreviewData, IOUTransactionID?: string) {
const {moneyRequestReportActionID, reportID, report, amount, currency, transactionID, created, policyID} = item;
const {reportID, report, amount, currency, transactionID, created, policyID} = item;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type safety issue: The destructuring attempts to extract moneyRequestReportActionID from item, but this field has been removed from the type definition. This line will always result in moneyRequestReportActionID being undefined.

Suggested fix: Remove the destructuring of moneyRequestReportActionID since it's now accessed via item.reportAction?.reportActionID on line 1109:

const {reportID, report, amount, currency, transactionID, created, policyID} = item;
const moneyRequestReportActionID = item.reportAction?.reportActionID;

This makes the code clearer and ensures the correct value is used.

const moneyRequestReportActionID = item?.reportAction?.reportActionID;
const {hasParentReport, hasParentReportAction, hasTransaction, hasTransactionThreadReport} = transactionPreviewData;
const onyxUpdates: OnyxUpdate[] = [];

Expand Down
3 changes: 0 additions & 3 deletions src/types/onyx/SearchResults.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,6 @@ type SearchTransaction = {
/** The modified MCC Group associated with the transaction */
modifiedMCCGroup?: ValueOf<typeof CONST.MCC_GROUPS>;

/** The ID of the money request reportAction associated with the transaction */
moneyRequestReportActionID?: string;

/** Whether the transaction has violations or errors */
errors?: OnyxCommon.Errors;

Expand Down
1 change: 0 additions & 1 deletion tests/unit/MoneyRequestReportUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ const transactionItemBaseMock: TransactionListItemType = {
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
errors: undefined,
filename: undefined,
violations: [],
Expand Down
60 changes: 4 additions & 56 deletions tests/unit/Search/SearchUIUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,6 @@ const searchResults: OnyxTypes.SearchResults = {
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
errors: undefined,
groupAmount: -5000,
groupCurrency: 'USD',
Expand Down Expand Up @@ -440,7 +439,6 @@ const searchResults: OnyxTypes.SearchResults = {
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
pendingAction: undefined,
errors: undefined,
groupAmount: -5000,
Expand Down Expand Up @@ -472,7 +470,6 @@ const searchResults: OnyxTypes.SearchResults = {
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
pendingAction: undefined,
errors: undefined,
groupAmount: -5000,
Expand Down Expand Up @@ -503,7 +500,6 @@ const searchResults: OnyxTypes.SearchResults = {
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
pendingAction: undefined,
errors: undefined,
groupAmount: -5000,
Expand Down Expand Up @@ -806,7 +802,6 @@ const transactionsListItems = [
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
errors: undefined,
violations: [],
groupAmount: -5000,
Expand Down Expand Up @@ -863,7 +858,6 @@ const transactionsListItems = [
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
pendingAction: undefined,
errors: undefined,
violations: [
Expand Down Expand Up @@ -926,7 +920,6 @@ const transactionsListItems = [
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
pendingAction: undefined,
errors: undefined,
violations: [],
Expand Down Expand Up @@ -984,7 +977,6 @@ const transactionsListItems = [
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
pendingAction: undefined,
errors: undefined,
violations: [],
Expand Down Expand Up @@ -1075,7 +1067,6 @@ const transactionReportGroupListItems = [
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
errors: undefined,
violations: [],
groupAmount: -5000,
Expand Down Expand Up @@ -1181,7 +1172,6 @@ const transactionReportGroupListItems = [
taxAmount: undefined,
mccGroup: undefined,
modifiedMCCGroup: undefined,
moneyRequestReportActionID: '789',
pendingAction: undefined,
errors: undefined,
groupAmount: -5000,
Expand Down Expand Up @@ -1716,7 +1706,7 @@ describe('SearchUIUtils', () => {
expect(distanceTransaction).toBeDefined();
expect(distanceTransaction?.iouRequestType).toBe(CONST.IOU.REQUEST_TYPE.DISTANCE);

const expectedPropertyCount = 47;
const expectedPropertyCount = 46;
expect(Object.keys(distanceTransaction ?? {}).length).toBe(expectedPropertyCount);
});

Expand Down Expand Up @@ -1749,7 +1739,7 @@ describe('SearchUIUtils', () => {
expect(distanceTransaction).toBeDefined();
expect(distanceTransaction?.iouRequestType).toBe(CONST.IOU.REQUEST_TYPE.DISTANCE);

const expectedPropertyCount = 47;
const expectedPropertyCount = 46;
expect(Object.keys(distanceTransaction ?? {}).length).toBe(expectedPropertyCount);
});

Expand Down Expand Up @@ -2811,49 +2801,7 @@ describe('SearchUIUtils', () => {
SearchUIUtils.createAndOpenSearchTransactionThread(transactionListItem, backTo, threadReportID, undefined, false);

expect(setOptimisticDataForTransactionThreadPreview).toHaveBeenCalled();
expect(createTransactionThreadReport).toHaveBeenCalledWith(report1, {reportActionID: transactionListItem.moneyRequestReportActionID}, undefined, undefined);
});

test('Should create transaction thread report for legacy transactions without IOU action (moneyRequestReportActionID = "0")', () => {
(createTransactionThreadReport as jest.Mock).mockReturnValue(threadReport);

// Create a legacy transaction item with moneyRequestReportActionID = '0'
const legacyTransactionItem = {
...transactionListItem,
moneyRequestReportActionID: '0',
};

SearchUIUtils.createAndOpenSearchTransactionThread(legacyTransactionItem, backTo, undefined, undefined, false);

// Extract the transaction by removing UI-specific and search-specific fields
const {
keyForList,
action,
allActions,
report,
from,
to,
formattedFrom,
formattedTo,
formattedTotal,
formattedMerchant,
date,
shouldShowMerchant,
shouldShowYear,
isAmountColumnWide,
isTaxAmountColumnWide,
violations,
hash: itemHash,
moneyRequestReportActionID,
canDelete,
accountID,
policyID: searchPolicyID,
...expectedTransaction
} = legacyTransactionItem;

// For legacy transactions (moneyRequestReportActionID = '0'), should pass transaction and violations
// reportActionID will be undefined since there's no IOU action
expect(createTransactionThreadReport).toHaveBeenCalledWith(report, {reportActionID: undefined}, expect.objectContaining(expectedTransaction), violations);
expect(createTransactionThreadReport).toHaveBeenCalledWith(report1, {reportActionID: '11111111'}, undefined, undefined);
});

test('Should not navigate if shouldNavigate = false', () => {
Expand Down Expand Up @@ -2899,7 +2847,7 @@ describe('SearchUIUtils', () => {
await waitForBatchedUpdates();

const parentReport = await getOnyxValue(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionListItem.reportID}`);
const parentReportAction = transactionListItem.moneyRequestReportActionID && parentReport?.[transactionListItem.moneyRequestReportActionID];
const parentReportAction = transactionListItem?.reportAction?.reportActionID && parentReport?.[transactionListItem?.reportAction?.reportActionID];

expect(parentReportAction).toBeTruthy();
});
Expand Down
Loading