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

fix: prevent editing money request in case create failure #46376

Merged
merged 2 commits into from
Jul 31, 2024
Merged

fix: prevent editing money request in case create failure #46376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91de470
fix: prevent editing money request in case create failure
dominictb Jul 28, 2024
f34d68a
fix: update boolean logic
dominictb Jul 30, 2024
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
31 changes: 12 additions & 19 deletions src/components/ReportActionItem/MoneyRequestView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@ import type {TransactionPendingFieldsKey} from '@src/types/onyx/Transaction';
import ReportActionItemImage from './ReportActionItemImage';

type MoneyRequestViewTransactionOnyxProps = {
/** The transaction associated with the transactionThread */
transaction: OnyxEntry<OnyxTypes.Transaction>;

/** Violations detected in this transaction */
transactionViolations: OnyxEntry<OnyxTypes.TransactionViolations>;
};
Expand Down Expand Up @@ -109,7 +106,6 @@ function MoneyRequestView({
parentReport,
parentReportActions,
policyCategories,
transaction,
policyTagList,
policy,
transactionViolations,
Expand All @@ -132,6 +128,12 @@ function MoneyRequestView({
const isTrackExpense = ReportUtils.isTrackExpenseReport(report);
const {canUseViolations, canUseP2PDistanceRequests} = usePermissions(isTrackExpense ? CONST.IOU.TYPE.TRACK : undefined);
const moneyRequestReport = parentReport;
const linkedTransactionID = useMemo(() => {
const originalMessage = parentReportAction && ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction) : undefined;
return originalMessage?.IOUTransactionID ?? '-1';
}, [parentReportAction]);
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${linkedTransactionID}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: To match the previous behavior using withOnyx should we render null if the transaction status is loading from Onyx?


const {
created: transactionDate,
amount: transactionAmount,
Expand Down Expand Up @@ -169,7 +171,7 @@ function MoneyRequestView({
const isCancelled = moneyRequestReport && moneyRequestReport?.isCancelledIOU;

// Used for non-restricted fields such as: description, category, tag, billable, etc.
const canEdit = ReportActionsUtils.isMoneyRequestAction(parentReportAction) && ReportUtils.canEditMoneyRequest(parentReportAction);
const canEdit = ReportActionsUtils.isMoneyRequestAction(parentReportAction) && ReportUtils.canEditMoneyRequest(parentReportAction, transaction);
const canEditTaxFields = canEdit && !isDistanceRequest;

const canEditAmount = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.AMOUNT);
Expand Down Expand Up @@ -214,7 +216,7 @@ function MoneyRequestView({
let amountDescription = `${translate('iou.amount')}`;

const hasRoute = TransactionUtils.hasRoute(transaction, isDistanceRequest);
const rateID = transaction?.comment.customUnit?.customUnitRateID ?? '-1';
const rateID = transaction?.comment?.customUnit?.customUnitRateID ?? '-1';

const currency = policy ? policy.outputCurrency : PolicyUtils.getPersonalPolicy()?.outputCurrency ?? CONST.CURRENCY.USD;

Expand Down Expand Up @@ -430,18 +432,18 @@ function MoneyRequestView({
errors={errors}
errorRowStyles={[styles.mh4]}
onClose={() => {
if (!transaction?.transactionID) {
if (!transaction?.transactionID && linkedTransactionID === '-1') {
return;
}

const isCreateChatErrored = !!report?.errorFields?.createChat;
if ((isCreateChatErrored || !!report?.isOptimisticReport) && parentReportAction) {
const urlToNavigateBack = IOU.cleanUpMoneyRequest(transaction.transactionID, parentReportAction, true);
const urlToNavigateBack = IOU.cleanUpMoneyRequest(transaction?.transactionID ?? linkedTransactionID, parentReportAction, true);
Navigation.goBack(urlToNavigateBack);
return;
}

if (transaction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
if (transaction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
if (chatReport?.reportID && ReportUtils.getAddWorkspaceRoomOrChatReportErrors(chatReport)) {
Report.navigateToConciergeChatAndDeleteReport(chatReport.reportID, true, true);
return;
Expand All @@ -450,7 +452,7 @@ function MoneyRequestView({
deleteTransaction(parentReport, parentReportAction);
}
}
Transaction.clearError(transaction.transactionID);
Transaction.clearError(transaction?.transactionID ?? linkedTransactionID);
ReportActions.clearAllRelatedReportActionErrors(report?.reportID ?? '-1', parentReportAction);
}}
>
Expand Down Expand Up @@ -675,15 +677,6 @@ export default withOnyx<MoneyRequestViewPropsWithoutTransaction, MoneyRequestVie
},
})(
withOnyx<MoneyRequestViewProps, MoneyRequestViewTransactionOnyxProps>({
transaction: {
key: ({report, parentReportActions}) => {
const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1'];
const originalMessage =
parentReportAction && ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction) : undefined;
const transactionID = originalMessage?.IOUTransactionID ?? -1;
return `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`;
},
},
transactionViolations: {
key: ({report, parentReportActions}) => {
const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1'];
Expand Down
7 changes: 6 additions & 1 deletion src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2718,7 +2718,7 @@ function getTransactionCommentObject(transaction: OnyxEntry<Transaction>): Comme
* This is used in conjunction with canEditRestrictedField to control editing of specific fields like amount, currency, created, receipt, and distance.
* On its own, it only controls allowing/disallowing navigating to the editing pages or showing/hiding the 'Edit' icon on report actions
*/
function canEditMoneyRequest(reportAction: OnyxInputOrEntry<ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.IOU>>): boolean {
function canEditMoneyRequest(reportAction: OnyxInputOrEntry<ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.IOU>>, linkedTransaction?: OnyxEntry<Transaction>): boolean {
const isDeleted = ReportActionsUtils.isDeletedAction(reportAction);

if (isDeleted) {
Expand All @@ -2733,6 +2733,11 @@ function canEditMoneyRequest(reportAction: OnyxInputOrEntry<ReportAction<typeof
return false;
}

const transaction = linkedTransaction ?? getLinkedTransaction(reportAction ?? undefined);
if (!transaction?.transactionID || (transaction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD && !isEmptyObject(transaction.errors))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: It would capture the intent better and be easier to read if we extract the if statement condition into a variable called transactionFailedToCreate. Alternatively we could explain with a comment.

return false;
}

const moneyRequestReportID = originalMessage?.IOUReportID ?? -1;

if (!moneyRequestReportID) {
Expand Down
Loading