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

Fix - Domain Card - User can edit the date in expenses related to the assigned domain card #39244

Merged
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
6 changes: 3 additions & 3 deletions src/ROUTES.ts
Original file line number Diff line number Diff line change
Expand Up @@ -352,9 +352,9 @@ const ROUTES = {
getUrlWithBackToParam(`${action as string}/${iouType as string}/currency/${transactionID}/${reportID}/${pageIndex}?currency=${currency}`, backTo),
},
MONEY_REQUEST_STEP_DATE: {
route: ':action/:iouType/date/:transactionID/:reportID',
getRoute: (action: IOUAction, iouType: IOUType, transactionID: string, reportID: string, backTo = '') =>
getUrlWithBackToParam(`${action as string}/${iouType as string}/date/${transactionID}/${reportID}`, backTo),
route: ':action/:iouType/date/:transactionID/:reportID/:reportActionID?',
getRoute: (action: IOUAction, iouType: IOUType, transactionID: string, reportID: string, backTo = '', reportActionID?: string) =>
getUrlWithBackToParam(`${action as string}/${iouType as string}/date/${transactionID}/${reportID}${reportActionID ? `/${reportActionID}` : ''}`, backTo),
},
MONEY_REQUEST_STEP_DESCRIPTION: {
route: ':action/:iouType/description/:transactionID/:reportID/:reportActionID?',
Expand Down
2 changes: 1 addition & 1 deletion src/components/MoneyRequestConfirmationList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ function MoneyRequestConfirmationList({
style={[styles.moneyRequestMenuItem]}
titleStyle={styles.flex1}
onPress={() => {
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_DATE.getRoute(action, iouType, transactionID, reportID, Navigation.getActiveRouteWithoutParams()));
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_DATE.getRoute(action, iouType, transactionID, reportID, Navigation.getActiveRouteWithoutParams(), reportActionID));
}}
disabled={didConfirm}
interactive={!isReadOnly}
Expand Down
1 change: 1 addition & 0 deletions src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ function MoneyRequestView({
description={translate('iou.card')}
title={cardProgramName}
titleStyle={styles.flex1}
interactive={false}
/>
</OfflineWithFeedback>
)}
Expand Down
1 change: 1 addition & 0 deletions src/libs/Navigation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,7 @@ type MoneyRequestNavigatorParamList = {
transactionID: string;
reportID: string;
backTo: Routes;
reportActionID?: string;
};
[SCREENS.MONEY_REQUEST.STEP_DESCRIPTION]: {
action: IOUAction;
Expand Down
21 changes: 11 additions & 10 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2615,18 +2615,19 @@ function canEditFieldOfMoneyRequest(reportAction: OnyxEntry<ReportAction>, field
return false;
}

if (fieldToEdit === CONST.EDIT_REQUEST_FIELD.AMOUNT || fieldToEdit === CONST.EDIT_REQUEST_FIELD.CURRENCY) {
if (TransactionUtils.isCardTransaction(transaction)) {
return false;
}
if (
(fieldToEdit === CONST.EDIT_REQUEST_FIELD.AMOUNT || fieldToEdit === CONST.EDIT_REQUEST_FIELD.CURRENCY || fieldToEdit === CONST.EDIT_REQUEST_FIELD.DATE) &&
TransactionUtils.isCardTransaction(transaction)
) {
return false;
}

if (TransactionUtils.isDistanceRequest(transaction)) {
const policy = getPolicy(moneyRequestReport?.reportID ?? '');
const isAdmin = isExpenseReport(moneyRequestReport) && policy.role === CONST.POLICY.ROLE.ADMIN;
const isManager = isExpenseReport(moneyRequestReport) && currentUserAccountID === moneyRequestReport?.managerID;
if ((fieldToEdit === CONST.EDIT_REQUEST_FIELD.AMOUNT || fieldToEdit === CONST.EDIT_REQUEST_FIELD.CURRENCY) && TransactionUtils.isDistanceRequest(transaction)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to change this condition @FitseTLT?

Copy link
Contributor Author

@FitseTLT FitseTLT May 3, 2024

Choose a reason for hiding this comment

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

This was taken from here

App/src/libs/ReportUtils.ts

Lines 2623 to 2624 in 004b3af

if (TransactionUtils.isDistanceRequest(transaction)) {
const policy = getPolicy(moneyRequestReport?.reportID ?? '');

As we took this

App/src/libs/ReportUtils.ts

Lines 2619 to 2621 in 004b3af

if (TransactionUtils.isCardTransaction(transaction)) {
return false;
}

out of its code block

I had to centralize the condition in one if

const policy = getPolicy(moneyRequestReport?.reportID ?? '');
const isAdmin = isExpenseReport(moneyRequestReport) && policy.role === CONST.POLICY.ROLE.ADMIN;
const isManager = isExpenseReport(moneyRequestReport) && currentUserAccountID === moneyRequestReport?.managerID;

return isAdmin || isManager;
}
return isAdmin || isManager;
}

if (fieldToEdit === CONST.EDIT_REQUEST_FIELD.RECEIPT) {
Expand Down
44 changes: 40 additions & 4 deletions src/pages/iou/request/step/IOURequestStepDate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import * as IOUUtils from '@libs/IOUUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportUtils from '@libs/ReportUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
import * as IOU from '@userActions/IOU';
import CONST from '@src/CONST';
Expand All @@ -26,6 +27,12 @@ type IOURequestStepDateOnyxProps = {
/** The draft transaction that holds data to be persisted on the current transaction */
splitDraftTransaction: OnyxEntry<OnyxTypes.Transaction>;

/** The actions from the parent report */
reportActions: OnyxEntry<OnyxTypes.ReportActions>;

/** Session info for the currently logged in user. */
session: OnyxEntry<OnyxTypes.Session>;

/** The policy of the report */
policy: OnyxEntry<OnyxTypes.Policy>;

Expand All @@ -37,27 +44,39 @@ type IOURequestStepDateOnyxProps = {
};

type IOURequestStepDateProps = IOURequestStepDateOnyxProps &
WithWritableReportOrNotFoundProps<typeof SCREENS.MONEY_REQUEST.STEP_WAYPOINT> & {
/** Holds data related to Expense view state, rather than the underlying Expense data. */
WithWritableReportOrNotFoundProps<typeof SCREENS.MONEY_REQUEST.STEP_DATE> & {
/** Holds data related to Money Request view state, rather than the underlying Money Request data. */
transaction: OnyxEntry<OnyxTypes.Transaction>;

/** The report linked to the transaction */
report: OnyxEntry<Report>;
};

function IOURequestStepDate({
route: {
params: {action, iouType, reportID, backTo},
params: {action, iouType, reportID, backTo, reportActionID},
},
transaction,
splitDraftTransaction,
policy,
policyTags,
policyCategories,
reportActions,
report,
session,
}: IOURequestStepDateProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const isEditing = action === CONST.IOU.ACTION.EDIT;
// In the split flow, when editing we use SPLIT_TRANSACTION_DRAFT to save draft value
const isEditingSplitBill = iouType === CONST.IOU.TYPE.SPLIT && isEditing;
const currentCreated = isEditingSplitBill && !lodashIsEmpty(splitDraftTransaction) ? TransactionUtils.getCreated(splitDraftTransaction) : TransactionUtils.getCreated(transaction);
const parentReportAction = reportActions?.[(isEditingSplitBill ? reportActionID : report?.parentReportActionID) ?? 0];
const canEditingSplitBill =
isEditingSplitBill && session && parentReportAction && session.accountID === parentReportAction.actorAccountID && TransactionUtils.areRequiredFieldsEmpty(transaction);
const canEditMoneyRequest = isEditing && ReportUtils.canEditFieldOfMoneyRequest(parentReportAction ?? null, CONST.EDIT_REQUEST_FIELD.DATE);
// eslint-disable-next-line rulesdir/no-negated-variables
const shouldShowNotFound = !IOUUtils.isValidMoneyRequestType(iouType) || (isEditing && !canEditMoneyRequest && !canEditingSplitBill);
Comment on lines +74 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change required @FitseTLT? I think this is getting way too complicated.

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 I know. But we need to do it if we want to fix deep link access u mentioned. This is what we did to tag and category pages. It is basically displaying not found page when users are not allowed to edit the date and the logic for permission to edit the field, I took it the logic we use to enable the clicking of the date field in the money request view.

const canEditDate = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.DATE);
const canEditReceipt = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.RECEIPT);

But an additional case we need to consider is for SplitBillDetailsPage (When we create split request with receipt), and for that the logic is taken from
const isEditingSplitBill = session?.accountID === reportAction?.actorAccountID && TransactionUtils.areRequiredFieldsEmpty(transaction);

Copy link
Contributor

Choose a reason for hiding this comment

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

@FitseTLT Isn't ReportUtils.canEditFieldOfMoneyRequest function enough to do the job? Can you share the relevant checks on tags and categories page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const isEditing = action === CONST.IOU.ACTION.EDIT;
const isSplitBill = iouType === CONST.IOU.TYPE.SPLIT;
const isEditingSplitBill = isEditing && isSplitBill;
const currentTransaction = isEditingSplitBill && !isEmptyObject(splitDraftTransaction) ? splitDraftTransaction : transaction;
const transactionTag = TransactionUtils.getTag(currentTransaction);
const tag = TransactionUtils.getTag(currentTransaction, tagListIndex);
const reportAction = reportActions?.[report?.parentReportActionID ?? reportActionID];
const canEditSplitBill = isSplitBill && reportAction && session?.accountID === reportAction.actorAccountID && TransactionUtils.areRequiredFieldsEmpty(transaction);
const policyTagLists = useMemo(() => PolicyUtils.getTagLists(policyTags), [policyTags]);
const shouldShowTag = ReportUtils.isGroupPolicy(report) && (transactionTag || OptionsListUtils.hasEnabledTags(policyTagLists));
// eslint-disable-next-line rulesdir/no-negated-variables
const shouldShowNotFoundPage = !shouldShowTag || (isEditing && (isSplitBill ? !canEditSplitBill : reportAction && !canEditMoneyRequest(reportAction)));

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't ReportUtils.canEditFieldOfMoneyRequest function enough to do the job

Can you please answer above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope it is not enough because we have to include the case of split detail page that is what I have included and I have given you a perfect similar example done for tag.


const navigateBack = () => {
Navigation.goBack(backTo);
Expand Down Expand Up @@ -94,7 +113,7 @@ function IOURequestStepDate({
<StepScreenWrapper
headerTitle={translate('common.date')}
onBackButtonPress={navigateBack}
shouldShowNotFoundPage={!IOUUtils.isValidMoneyRequestType(iouType)} // TODO: Check why is this even here
shouldShowNotFoundPage={shouldShowNotFound}
shouldShowWrapper
testID={IOURequestStepDate.displayName}
>
Expand Down Expand Up @@ -127,6 +146,20 @@ const IOURequestStepDateWithOnyx = withOnyx<IOURequestStepDateProps, IOURequestS
return `${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${transactionID}`;
},
},
reportActions: {
key: ({
route: {
params: {action, iouType},
},
report,
}) => {
let reportID;
if (action === CONST.IOU.ACTION.EDIT) {
reportID = iouType === CONST.IOU.TYPE.SPLIT ? report?.reportID : report?.parentReportID;
}
return `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID ?? '0'}`;
},
},
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
},
Expand All @@ -136,6 +169,9 @@ const IOURequestStepDateWithOnyx = withOnyx<IOURequestStepDateProps, IOURequestS
policyTags: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_TAGS}${report ? report.policyID : '0'}`,
},
session: {
key: ONYXKEYS.SESSION,
},
})(IOURequestStepDate);

// eslint-disable-next-line rulesdir/no-negated-variables
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type MoneyRequestRouteName =
| typeof SCREENS.MONEY_REQUEST.STEP_AMOUNT
| typeof SCREENS.MONEY_REQUEST.STEP_WAYPOINT
| typeof SCREENS.MONEY_REQUEST.STEP_DESCRIPTION
| typeof SCREENS.MONEY_REQUEST.STEP_DATE
| typeof SCREENS.MONEY_REQUEST.STEP_TAX_AMOUNT
| typeof SCREENS.MONEY_REQUEST.STEP_PARTICIPANTS
| typeof SCREENS.MONEY_REQUEST.STEP_MERCHANT
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type WithWritableReportOrNotFoundOnyxProps = {
type MoneyRequestRouteName =
| typeof SCREENS.MONEY_REQUEST.STEP_WAYPOINT
| typeof SCREENS.MONEY_REQUEST.STEP_DESCRIPTION
| typeof SCREENS.MONEY_REQUEST.STEP_DATE
| typeof SCREENS.MONEY_REQUEST.STEP_CATEGORY
| typeof SCREENS.MONEY_REQUEST.STEP_DISTANCE_RATE
| typeof SCREENS.MONEY_REQUEST.STEP_CONFIRMATION
Expand Down
Loading