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.

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
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
4 changes: 4 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,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
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: 7 additions & 4 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,15 @@ import {
getTag,
getTaxAmount,
getTaxCode,
getTransaction,
getAmount as getTransactionAmount,
getWaypoints,
hasMissingSmartscanFields as hasMissingSmartscanFieldsTransactionUtils,
hasNoticeTypeViolation,
hasReceipt as hasReceiptTransactionUtils,
hasViolation,
hasWarningTypeViolation,
isCardTransaction,
isCardTransaction as isCardTransactionTransactionUtils,
isDistanceRequest,
isDuplicate,
isExpensifyCardTransaction,
Expand Down Expand Up @@ -2158,9 +2159,11 @@ function canDeleteTransaction(moneyRequestReport: OnyxEntry<Report>): boolean {
*/
function canDeleteReportAction(reportAction: OnyxInputOrEntry<ReportAction>, reportID: string): boolean {
const report = getReportOrDraftReport(reportID);

const isActionOwner = reportAction?.actorAccountID === currentUserAccountID;
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`] ?? null;
const iouTransactionID = isMoneyRequestAction(reportAction) ? getOriginalMessage(reportAction)?.IOUTransactionID : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, just feel that if we can move this below isMoneyRequestAction(reportAction) , it could help us avoid some unnecessary fetches. I'll start filling the checklist in a few hours. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@narefyev91 might be good to follow up later

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

if (isMoneyRequestAction(reportAction)) {
// For now, users cannot delete split actions
Expand All @@ -2172,7 +2175,7 @@ function canDeleteReportAction(reportAction: OnyxInputOrEntry<ReportAction>, rep

if (isActionOwner) {
if (!isEmptyObject(report) && (isMoneyRequestReport(report) || isInvoiceReport(report))) {
return canDeleteTransaction(report);
return canDeleteTransaction(report) && (!isCardTransaction || (isCardTransaction && transaction?.comment?.liabilityType === CONST.TRANSACTION.LIABILITY_TYPE.ALLOW));
}
return true;
}
Expand Down Expand Up @@ -3600,7 +3603,7 @@ function canEditFieldOfMoneyRequest(reportAction: OnyxInputOrEntry<ReportAction>

if (
(fieldToEdit === CONST.EDIT_REQUEST_FIELD.AMOUNT || fieldToEdit === CONST.EDIT_REQUEST_FIELD.CURRENCY || fieldToEdit === CONST.EDIT_REQUEST_FIELD.DATE) &&
isCardTransaction(transaction)
isCardTransactionTransactionUtils(transaction)
) {
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/libs/TransactionUtils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1183,7 +1183,7 @@ function getTaxName(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transactio
return Object.values(transformedTaxRates(policy, transaction)).find((taxRate) => taxRate.code === (transaction?.taxCode ?? defaultTaxCode))?.modifiedName;
}

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

Expand Down
12 changes: 9 additions & 3 deletions src/pages/ReportDetailsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ import {
shouldUseFullTitleToDisplay,
} from '@libs/ReportUtils';
import StringUtils from '@libs/StringUtils';
import {isCardTransaction as isCardTransactionTransactionUtils} from '@libs/TransactionUtils';
import {
canCancelPayment,
cancelPayment as cancelPaymentAction,
Expand Down Expand Up @@ -294,7 +295,14 @@ 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 : '';

/* eslint-disable @typescript-eslint/prefer-nullish-coalescing */
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${iouTransactionID || CONST.DEFAULT_NUMBER_ID}`);
const isCardTransaction = isCardTransactionTransactionUtils(transaction);

const shouldShowDeleteButton =
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also wrap the (!isCardTransaction || (isCardTransaction && transaction?.comment?.liabilityType === CONST.TRANSACTION.LIABILITY_TYPE.ALLOW)) condition to some helper method or variable? I feel like this is hard to read and its used in at least two places

shouldShowTaskDeleteButton || (canDeleteRequest && (!isCardTransaction || (isCardTransaction && transaction?.comment?.liabilityType === CONST.TRANSACTION.LIABILITY_TYPE.ALLOW)));

useEffect(() => {
if (canDeleteRequest) {
Expand Down Expand Up @@ -366,8 +374,6 @@ function ReportDetailsPage({policies, report, route, reportMetadata}: ReportDeta
const shouldShowCancelPaymentButton = caseID === CASES.MONEY_REPORT && canCancelPayment(moneyRequestReport, session);
const [chatReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${moneyRequestReport?.chatReportID}`);

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

const cancelPayment = useCallback(() => {
if (!chatReport) {
return;
Expand Down
14 changes: 9 additions & 5 deletions src/pages/home/report/ContextMenu/ContextMenuActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -753,18 +753,22 @@ const ContextMenuActions: ContextMenuAction[] = [
isAnonymousAction: false,
textTranslateKey: 'reportActionContextMenu.deleteAction',
icon: Expensicons.Trashcan,
shouldShow: ({type, reportAction, isArchivedRoom, isChronosReport, reportID}) =>
shouldShow: ({type, reportAction, isArchivedRoom, isChronosReport, reportID, moneyRequestAction}) =>
// Until deleting parent threads is supported in FE, we will prevent the user from deleting a thread parent
type === CONST.CONTEXT_MENU_TYPES.REPORT_ACTION && canDeleteReportAction(reportAction, reportID) && !isArchivedRoom && !isChronosReport && !isMessageDeleted(reportAction),
onPress: (closePopover, {reportID, reportAction}) => {
type === CONST.CONTEXT_MENU_TYPES.REPORT_ACTION &&
canDeleteReportAction(moneyRequestAction ?? reportAction, reportID) &&
!isArchivedRoom &&
!isChronosReport &&
!isMessageDeleted(reportAction),
onPress: (closePopover, {reportID, reportAction, moneyRequestAction}) => {
if (closePopover) {
// Hide popover, then call showDeleteConfirmModal
hideContextMenu(false, () => showDeleteModal(reportID, reportAction));
hideContextMenu(false, () => showDeleteModal(reportID, moneyRequestAction ?? reportAction));
return;
}

// No popover to hide, call showDeleteConfirmModal immediately
showDeleteModal(reportID, reportAction);
showDeleteModal(reportID, moneyRequestAction ?? reportAction);
},
getDescription: () => {},
},
Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/Transaction.ts
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
4 changes: 4 additions & 0 deletions tests/ui/ReportDetailsPageTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type SCREENS from '@src/SCREENS';
import type {Report} from '@src/types/onyx';
import createRandomReportAction from '../utils/collections/reportActions';
import createRandomReport from '../utils/collections/reports';
import createRandomTransaction from '../utils/collections/transaction';

jest.mock('@react-navigation/native', () => {
const actualNav = jest.requireActual<typeof Navigation>('@react-navigation/native');
Expand Down Expand Up @@ -42,6 +43,8 @@ describe('ReportDetailsPage', () => {
const selfDMReportID = '1';
const trackExpenseReportID = '2';
const trackExpenseActionID = '123';
const transactionID = '3';
const transaction = createRandomTransaction(1);
const trackExpenseReport: Report = {
...createRandomReport(Number(trackExpenseReportID)),
chatType: '' as Report['chatType'],
Expand All @@ -53,6 +56,7 @@ describe('ReportDetailsPage', () => {
chatType: CONST.REPORT.CHAT_TYPE.SELF_DM,
});
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${trackExpenseReportID}`, trackExpenseReport);
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, transaction);
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${selfDMReportID}`, {
[trackExpenseActionID]: {
...createRandomReportAction(Number(trackExpenseActionID)),
Expand Down
59 changes: 59 additions & 0 deletions tests/unit/ReportUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
buildOptimisticIOUReportAction,
buildParticipantsFromAccountIDs,
buildTransactionThread,
canDeleteReportAction,
canEditWriteCapability,
getAllAncestorReportActions,
getApprovalChain,
Expand Down Expand Up @@ -1857,6 +1858,64 @@ describe('ReportUtils', () => {
});
});

describe('canDeleteReportAction', () => {
it('should return false for delete button visibility if transaction is not allowed to be deleted', () => {
const parentReport = LHNTestUtils.getFakeReport();
const report = LHNTestUtils.getFakeReport();
const parentReportAction: ReportAction = {
...LHNTestUtils.getFakeReportAction(),
message: [
{
type: 'COMMENT',
html: 'hey',
text: 'hey',
isEdited: false,
whisperedTo: [],
isDeletedParentAction: false,
moderationDecision: {
decision: CONST.MODERATION.MODERATOR_DECISION_PENDING_REMOVE,
},
},
],
childReportID: report.reportID,
};
report.parentReportID = parentReport.reportID;
report.parentReportActionID = parentReportAction.reportActionID;
const currentReportId = '';
const transactionID = 1;
const moneyRequestAction = {
...parentReportAction,
actorAccountID: currentUserAccountID,
actionName: CONST.REPORT.ACTIONS.TYPE.IOU,
originalMessage: {
IOUReportID: '1',
IOUTransactionID: '1',
amount: 100,
participantAccountID: 1,
currency: CONST.CURRENCY.USD,
type: CONST.IOU.REPORT_ACTION_TYPE.PAY,
paymentType: CONST.IOU.PAYMENT_TYPE.EXPENSIFY,
},
};

const transaction: Transaction = {
...createRandomTransaction(transactionID),
category: '',
tag: '',
created: testDate,
reportID: currentReportId,
managedCard: true,
comment: {
liabilityType: CONST.TRANSACTION.LIABILITY_TYPE.RESTRICT,
},
};

Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, transaction).then(() => {
expect(canDeleteReportAction(moneyRequestAction, currentReportId)).toBe(false);
});
});
});

describe('getApprovalChain', () => {
describe('submit and close policy', () => {
it('should return empty array', () => {
Expand Down
Loading