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.

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

Show/hide delete expense button based on Liability type for card transaction #56877

Merged
merged 21 commits into from
Mar 5, 2025
Merged

Show/hide delete expense button based on Liability type for card transaction #56877

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e1586d2
Show/hide delete expense button based on feed settings for Company cards
narefyev91 Feb 14, 2025
b5600cd
Merge branch 'refs/heads/main' into hide-delete-expense-button-if-car…
narefyev91 Feb 27, 2025
ea2eb87
show/hide button based on transaction liability
narefyev91 Feb 27, 2025
db6e038
prettier
narefyev91 Feb 27, 2025
658078e
fix ts
narefyev91 Feb 27, 2025
f04c256
update condition
narefyev91 Feb 27, 2025
fb471bb
remove ability to show delete button in context menu
narefyev91 Feb 28, 2025
089e966
fix empty transaction
narefyev91 Feb 28, 2025
ba8fa7e
Merge branch 'refs/heads/main' into hide-delete-expense-button-if-car…
narefyev91 Feb 28, 2025
3cb14b6
fix jest
narefyev91 Feb 28, 2025
78a5666
fix jest
narefyev91 Feb 28, 2025
20f09c3
add unit test
narefyev91 Mar 3, 2025
5c5ffa0
show delete button for card transaction report
narefyev91 Mar 4, 2025
c60107a
update condition for report details page
narefyev91 Mar 4, 2025
1b4744c
update condition for context menu
narefyev91 Mar 4, 2025
5b32275
Merge branch 'refs/heads/main' into hide-delete-expense-button-if-car…
narefyev91 Mar 5, 2025
53e8d25
update after c+ review
narefyev91 Mar 5, 2025
0ef4703
prettier
narefyev91 Mar 5, 2025
e15f660
ts
narefyev91 Mar 5, 2025
dbca006
fix jest
narefyev91 Mar 5, 2025
2807ce8
add missing money request
narefyev91 Mar 5, 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
4 changes: 4 additions & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,6 +1480,10 @@ const CONST = {
DRAFT: 'draft',
BACKUP: 'backup',
},
LIABILITY_TYPE: {
RESTRICT: 'corporate',
ALLOW: 'personal',
},
},

MCC_GROUPS: {
Expand Down
2 changes: 2 additions & 0 deletions src/libs/DebugUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1041,6 +1041,7 @@ function validateTransactionDraftProperty(key: keyof Transaction, value: string)
linkedTrackedExpenseReportAction: CONST.RED_BRICK_ROAD_PENDING_ACTION,
linkedTrackedExpenseReportID: CONST.RED_BRICK_ROAD_PENDING_ACTION,
bank: CONST.RED_BRICK_ROAD_PENDING_ACTION,
liabilityType: CONST.RED_BRICK_ROAD_PENDING_ACTION,
cardName: CONST.RED_BRICK_ROAD_PENDING_ACTION,
cardNumber: CONST.RED_BRICK_ROAD_PENDING_ACTION,
managedCard: CONST.RED_BRICK_ROAD_PENDING_ACTION,
Expand Down Expand Up @@ -1077,6 +1078,7 @@ function validateTransactionDraftProperty(key: keyof Transaction, value: string)
customUnit: 'object',
source: 'string',
originalTransactionID: 'string',
liabilityType: CONST.TRANSACTION.LIABILITY_TYPE,
splits: 'array',
dismissedViolations: 'object',
});
Expand Down
11 changes: 8 additions & 3 deletions src/pages/ReportDetailsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ import {
shouldUseFullTitleToDisplay,
} from '@libs/ReportUtils';
import StringUtils from '@libs/StringUtils';
import {isCardTransaction as isCardTransactionTransactionUtils} from '@libs/TransactionUtils';
import {
cancelPayment as cancelPaymentAction,
deleteMoneyRequest,
Expand Down Expand Up @@ -302,7 +303,13 @@ function ReportDetailsPage({policies, report, route, reportMetadata}: ReportDeta
const shouldShowTaskDeleteButton =
isTaskReport && !isCanceledTaskReport && canWriteInReport(report) && report.stateNum !== CONST.REPORT.STATE_NUM.APPROVED && !isClosedReport(report) && canModifyTask && canActionTask;
const canDeleteRequest = isActionOwner && (canDeleteTransaction(moneyRequestReport) || isSelfDMTrackExpenseReport) && !isDeletedParentAction;
const shouldShowDeleteButton = shouldShowTaskDeleteButton || canDeleteRequest;
const iouTransactionID = isMoneyRequestAction(requestParentReportAction) ? getOriginalMessage(requestParentReportAction)?.IOUTransactionID : '';

const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${iouTransactionID ?? CONST.DEFAULT_NUMBER_ID}`);
const isCardTransaction = isCardTransactionTransactionUtils(transaction);

const isAllowToDeleteTransaction = !isCardTransaction || (isCardTransaction && transaction?.comment?.liabilityType === CONST.TRANSACTION.LIABILITY_TYPE.ALLOW);
const shouldShowDeleteButton = isCardTransaction ? isAllowToDeleteTransaction : shouldShowTaskDeleteButton || canDeleteRequest;
Copy link
Contributor

@ntdiary ntdiary Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

looks like the condition here is actually isCardTransaction ? transaction?.comment?.liabilityType === CONST.TRANSACTION.LIABILITY_TYPE.ALLOW : shouldShowTaskDeleteButton || canDeleteRequest ?
BTW, should we also update the Delete expense option in the context menu? or address that in a separate issue?

image

might need to add some similar logic here as well.

App/src/libs/ReportUtils.ts

Lines 2168 to 2181 in 5c4dc1c

if (isMoneyRequestAction(reportAction)) {
// For now, users cannot delete split actions
const isSplitAction = getOriginalMessage(reportAction)?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT;
if (isSplitAction) {
return false;
}
if (isActionOwner) {
if (!isEmptyObject(report) && (isMoneyRequestReport(report) || isInvoiceReport(report))) {
return canDeleteTransaction(report);
}
return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes i think we should update the logic there and the logic on Search page too - you can also delete expense from there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah from search page we using the same component "Details page" - which already has that logic:
Screenshot 2025-02-28 at 11 41 35
Let me know if that's not only place to take a look... did not find anything else in search area

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mountiny updated all places

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes i think we should update the logic there and the logic on Search page too - you can also delete expense from there

This Delete option?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@ntdiary @narefyev91 yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in this particular case Delete button reference on API data - for transaction it's coming prop - canDelete: boolean.
Screenshot 2025-03-03 at 14 50 29

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that liabilityType is being returned. maybe either fix it on the backend to ensure canDelete has the correct value, or consider adding a liabilityType sub-condition on the frontend? I don’t have a strong preference. :D

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if BE already knows that transaction type will be corporate - i think the flag canDelete should also follows the same pattern. @mountiny do you think it will be possible to add check on BE side?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I personally agree that it’s better for the backend to return the correct value and keep it as the source of truth. :)


const canUnapproveRequest = isExpenseReportUtil(report) && (isReportManagerUtil(report) || isPolicyAdmin) && isReportApprovedUtil({report}) && !isSubmitAndClose(policy);

Expand Down Expand Up @@ -380,8 +387,6 @@ function ReportDetailsPage({policies, report, route, reportMetadata}: ReportDeta
const shouldShowCancelPaymentButton = caseID === CASES.MONEY_REPORT && isPayer && isSettled && isExpenseReportUtil(moneyRequestReport);
const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${moneyRequestReport?.chatReportID}`);

const iouTransactionID = isMoneyRequestAction(requestParentReportAction) ? getOriginalMessage(requestParentReportAction)?.IOUTransactionID : '';

const cancelPayment = useCallback(() => {
if (!chatReport) {
return;
Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/Transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ type Comment = {

/** Violations that were dismissed */
dismissedViolations?: Partial<Record<ViolationName, Record<string, string | number>>>;

/** Defines the type of liability for the transaction */
liabilityType?: ValueOf<typeof CONST.TRANSACTION.LIABILITY_TYPE>;
};

/** Model of transaction custom unit */
Expand Down
Loading