Skip to content

Delete the expense/iou report when deleting the last transaction #60523

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.

Sign up for GitHub

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

Merged
blimpich merged 3 commits into from
Apr 22, 2025
Merged

Delete the expense/iou report when deleting the last transaction #60523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
132a7ce
delete the expense/iou report when deleting the last transaction
bernhardoj Apr 19, 2025
b31f450
lint
bernhardoj Apr 19, 2025
c2235c9
update test
bernhardoj Apr 19, 2025
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
8 changes: 4 additions & 4 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ import {
getTrackExpenseActionableWhisper,
isActionableTrackExpense,
isCreatedAction,
isDeletedParentAction,
isMoneyRequestAction,
isReportPreviewAction,
} from '@libs/ReportActionsUtils';
Expand Down Expand Up @@ -7019,9 +7018,8 @@ function prepareToCleanUpMoneyRequest(transactionID: string, reportAction: OnyxT
if (chatReport) {
canUserPerformWriteAction = !!canUserPerformWriteActionReportUtils(chatReport);
}
const lastVisibleAction = getLastVisibleAction(iouReport?.reportID, canUserPerformWriteAction, updatedReportAction);
const iouReportLastMessageText = getLastVisibleMessage(iouReport?.reportID, canUserPerformWriteAction, updatedReportAction).lastMessageText;
const shouldDeleteIOUReport = iouReportLastMessageText.length === 0 && !isDeletedParentAction(lastVisibleAction) && (!transactionThreadID || shouldDeleteTransactionThread);
// If we are deleting the last transaction on a report, then delete the report too
const shouldDeleteIOUReport = getReportTransactions(iouReportID).length === 1;

// STEP 4: Update the iouReport and reportPreview with new totals and messages if it wasn't deleted
let updatedIOUReport: OnyxInputValue<OnyxTypes.Report>;
Expand Down Expand Up @@ -7063,6 +7061,8 @@ function prepareToCleanUpMoneyRequest(transactionID: string, reportAction: OnyxT
}

if (updatedIOUReport) {
const lastVisibleAction = getLastVisibleAction(iouReport?.reportID, canUserPerformWriteAction, updatedReportAction);
const iouReportLastMessageText = getLastVisibleMessage(iouReport?.reportID, canUserPerformWriteAction, updatedReportAction).lastMessageText;
updatedIOUReport.lastMessageText = iouReportLastMessageText;
updatedIOUReport.lastVisibleActionCreated = lastVisibleAction?.created;
}
Expand Down
113 changes: 37 additions & 76 deletions tests/actions/IOUTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2813,7 +2813,7 @@ describe('actions/IOU', () => {
expect(tr).toBeFalsy();
});

it('delete the IOU report when there are no visible comments left in the IOU report', async () => {
it('delete the IOU report when there are no expenses left in the IOU report', async () => {

@dukenv0307 dukenv0307 Apr 21, 2025

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.

We should add some visible comments in IOU report then make sure it's deleted totally

@bernhardoj bernhardoj Apr 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's unnecesarry since we don't delete based on the comment counts anymore.

@dukenv0307 dukenv0307 Apr 21, 2025

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.

yes, but we need to make sure if there are some visible comments, we still delete the iou report. What do you think?

@bernhardoj bernhardoj Apr 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I get what you mean, but I personally think it's not needed since it's now 100% unrelated to the test (the existence of the comments doesn't affect the test result)

@dukenv0307 dukenv0307 Apr 22, 2025

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.

ok, let's skip it for now

// Given an IOU report and a paused fetch state
mockFetch?.pause?.();

Expand Down Expand Up @@ -2856,41 +2856,27 @@ describe('actions/IOU', () => {
expect(report).toBeFalsy();
});

it('does not delete the IOU report when there are visible comments left in the IOU report', async () => {
// Given the initial setup is completed
await waitForBatchedUpdates();

Onyx.connect({
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${IOU_REPORT_ID}`,
callback: (val) => (reportActions = val),
it('does not delete the IOU report when there are expenses left in the IOU report', async () => {
// Given multiple expenses on an IOU report
requestMoney({
report: chatReport,
participantParams: {
payeeEmail: TEST_USER_LOGIN,
payeeAccountID: TEST_USER_ACCOUNT_ID,
participant: {login: RORY_EMAIL, accountID: RORY_ACCOUNT_ID},
},
transactionParams: {
amount,
attendees: [],
currency: CONST.CURRENCY.USD,
created: '',
merchant: '',
comment,
},
});
await waitForBatchedUpdates();

jest.advanceTimersByTime(10);

// When a comment is added to the IOU report
if (IOU_REPORT_ID) {
addComment(IOU_REPORT_ID, 'Testing a comment');
}
await waitForBatchedUpdates();

// Then verify that the comment is correctly added
const resultAction = Object.values(reportActions ?? {}).find((reportAction) => reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT);
reportActionID = resultAction?.reportActionID;

expect(resultAction?.message).toEqual(REPORT_ACTION.message);
expect(resultAction?.person).toEqual(REPORT_ACTION.person);
expect(resultAction?.pendingAction).toBeUndefined();

await waitForBatchedUpdates();

// Verify there are three actions (created + iou + addcomment) and our optimistic comment has been removed
expect(Object.values(reportActions ?? {}).length).toBe(3);

// Then check the loading state of our action
const resultActionAfterUpdate = reportActionID ? reportActions?.[reportActionID] : undefined;
expect(resultActionAfterUpdate?.pendingAction).toBeUndefined();

// When we attempt to delete an expense from the IOU report
mockFetch?.pause?.();
if (transaction && createIOUAction) {
Expand Down Expand Up @@ -2918,7 +2904,7 @@ describe('actions/IOU', () => {
expect(iouReport).toHaveProperty('chatReportID');

// Given the resumed fetch state
mockFetch?.resume?.();
await mockFetch?.resume?.();

@bernhardoj bernhardoj Apr 19, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

resume is a promise and we need to wait for it.


allReports = await new Promise<OnyxCollection<Report>>((resolve) => {
const connection = Onyx.connect({
Expand Down Expand Up @@ -3449,49 +3435,31 @@ describe('actions/IOU', () => {
});

it('navigate the user correctly to the iou Report when appropriate', async () => {
await waitForBatchedUpdates();

Onyx.connect({
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${IOU_REPORT_ID}`,
callback: (val) => (reportActions = val),
// Given multiple expenses on an IOU report
requestMoney({
report: chatReport,
participantParams: {
payeeEmail: TEST_USER_LOGIN,
payeeAccountID: TEST_USER_ACCOUNT_ID,
participant: {login: RORY_EMAIL, accountID: RORY_ACCOUNT_ID},
},
transactionParams: {
amount,
attendees: [],
currency: CONST.CURRENCY.USD,
created: '',
merchant: '',
comment,
},
});
await waitForBatchedUpdates();

// Given an added comment to the iou report

jest.advanceTimersByTime(10);

if (IOU_REPORT_ID) {
addComment(IOU_REPORT_ID, 'Testing a comment');
}
await waitForBatchedUpdates();

const resultAction = Object.values(reportActions ?? {}).find((reportAction) => reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT);
reportActionID = resultAction?.reportActionID;

expect(resultAction?.message).toEqual(REPORT_ACTION.message);
expect(resultAction?.person).toEqual(REPORT_ACTION.person);

await waitForBatchedUpdates();

// Verify there are three actions (created + iou + addcomment) and our optimistic comment has been removed
expect(Object.values(reportActions ?? {}).length).toBe(3);

await waitForBatchedUpdates();

// Given a thread report

jest.advanceTimersByTime(10);
thread = buildTransactionThread(createIOUAction, iouReport);

expect(thread.participants).toStrictEqual({[CARLOS_ACCOUNT_ID]: {notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, role: CONST.REPORT.ROLE.ADMIN}});

@bernhardoj bernhardoj Apr 19, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure why, but this test case (navigate the user correctly to the iou Report when appropriate) is testing unrelated cases, such as testing the transaction thread data. Looks like someone added it here instead of creating a new test case.

Onyx.connect({
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${thread.reportID}`,
callback: (val) => (reportActions = val),
});
await waitForBatchedUpdates();

jest.advanceTimersByTime(10);
const participantAccountIDs = Object.keys(thread.participants ?? {}).map(Number);
const userLogins = getLoginsByAccountIDs(participantAccountIDs);
Expand All @@ -3515,17 +3483,13 @@ describe('actions/IOU', () => {
);
expect(createIOUAction?.childReportID).toBe(thread.reportID);

await waitForBatchedUpdates();

// When we delete the expense in SingleTransactionView and we should not delete the IOU report

// When we delete the expense, we should not delete the IOU report
mockFetch?.pause?.();

let navigateToAfterDelete;
if (transaction && createIOUAction) {
navigateToAfterDelete = deleteMoneyRequest(transaction.transactionID, createIOUAction, true);
}
await waitForBatchedUpdates();

let allReports = await new Promise<OnyxCollection<Report>>((resolve) => {
const connection = Onyx.connect({
Expand All @@ -3538,14 +3502,12 @@ describe('actions/IOU', () => {
});
});

await waitForBatchedUpdates();

@bernhardoj bernhardoj Apr 19, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deleting unneeded waitForBatchedUpdates because nothing to wait.


iouReport = Object.values(allReports ?? {}).find((report) => isIOUReport(report));
expect(iouReport).toBeTruthy();
expect(iouReport).toHaveProperty('reportID');
expect(iouReport).toHaveProperty('chatReportID');

mockFetch?.resume?.();
await mockFetch?.resume?.();

allReports = await new Promise<OnyxCollection<Report>>((resolve) => {
const connection = Onyx.connect({
Expand All @@ -3564,7 +3526,6 @@ describe('actions/IOU', () => {
expect(iouReport).toHaveProperty('chatReportID');

// Then we expect to navigate to the iou report

expect(IOU_REPORT_ID).not.toBeUndefined();
if (IOU_REPORT_ID) {
expect(navigateToAfterDelete).toEqual(ROUTES.REPORT_WITH_ID.getRoute(IOU_REPORT_ID));
Expand Down