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

Add checks for editing APPROVED reports #33633

Merged
merged 36 commits into from
Jan 5, 2024
Merged

Add checks for editing APPROVED reports #33633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
ba930e4
Add support for editing approved reports
youssef-lr Dec 25, 2023
bcb8dc4
Cleanup
youssef-lr Dec 25, 2023
c41b554
Fix bugs
youssef-lr Dec 25, 2023
0459190
Lint
youssef-lr Dec 26, 2023
394c82b
Merge branch 'main' into youssef_edit_approveed_reports
youssef-lr Dec 29, 2023
d273fc6
Fix typescript commplaining
youssef-lr Dec 29, 2023
7ba5146
Add safety check
youssef-lr Dec 29, 2023
d85421d
Remove useMemo
youssef-lr Dec 29, 2023
07e2148
Remove useMemo
youssef-lr Dec 29, 2023
9a2ef5c
Remove unnecessary call to canEditFieldOfMoneyRequest
youssef-lr Dec 31, 2023
bdfa319
Address comments
youssef-lr Dec 31, 2023
de8ea54
Remove unneeded check
youssef-lr Dec 31, 2023
982ba48
Remove code causing regression
youssef-lr Dec 31, 2023
cfaac6f
Add comment
youssef-lr Dec 31, 2023
f672b91
Update src/components/ReportActionItem/MoneyRequestView.js
youssef-lr Jan 1, 2024
31e6a15
Update comment
youssef-lr Jan 1, 2024
ba8506d
Remove transaction dependency from canEditFieldOfMoneyRequest
youssef-lr Jan 1, 2024
c51f250
Update comment
youssef-lr Jan 1, 2024
6ebe315
Fix typescript errors
youssef-lr Jan 2, 2024
eda03f7
Conflicts
youssef-lr Jan 2, 2024
c5e9c58
Fix condition
youssef-lr Jan 2, 2024
1bea72c
Update comment
youssef-lr Jan 3, 2024
a5397e5
Merge branch 'main' into youssef_edit_approveed_reports
youssef-lr Jan 3, 2024
22ec1dd
Fi comment
youssef-lr Jan 3, 2024
7021d27
Add policy to proptypes with default
youssef-lr Jan 3, 2024
a5cdf15
Use isGroupPolicy
youssef-lr Jan 3, 2024
7c894e2
Conflicts
youssef-lr Jan 4, 2024
53e263a
Cleanup
youssef-lr Jan 4, 2024
7de81d4
Cleanup
youssef-lr Jan 4, 2024
4123a31
Remove unneeded check
youssef-lr Jan 4, 2024
a130cb3
Update src/libs/ReportUtils.ts
youssef-lr Jan 5, 2024
b6ba955
use isDraftExpenseReport
youssef-lr Jan 5, 2024
1707e03
Move variables into the scope where they're needed
youssef-lr Jan 5, 2024
8491e86
Rename methods
youssef-lr Jan 5, 2024
3525c5e
Add code removed by mistake
youssef-lr Jan 5, 2024
316b718
Make code clearer
youssef-lr Jan 5, 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
15 changes: 7 additions & 8 deletions src/components/AttachmentModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import fileDownload from '@libs/fileDownload';
import * as FileUtils from '@libs/fileDownload/FileUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
import useNativeDriver from '@libs/useNativeDriver';
import reportPropTypes from '@pages/reportPropTypes';
Expand Down Expand Up @@ -95,6 +94,9 @@ const propTypes = {

/** Whether it is a receipt attachment or not */
isReceiptAttachment: PropTypes.bool,

/** Whether the receipt can be replaced */
canEditReceipt: PropTypes.bool,
};

const defaultProps = {
Expand All @@ -113,6 +115,7 @@ const defaultProps = {
onCarouselAttachmentChange: () => {},
isWorkspaceAvatar: false,
isReceiptAttachment: false,
canEditReceipt: false,
};

function AttachmentModal(props) {
Expand Down Expand Up @@ -372,13 +375,9 @@ function AttachmentModal(props) {
if (!props.isReceiptAttachment || !props.parentReport || !props.parentReportActions) {
return [];
}
const menuItems = [];
const parentReportAction = props.parentReportActions[props.report.parentReportActionID];

const canEdit =
ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, props.parentReport.reportID, CONST.EDIT_REQUEST_FIELD.RECEIPT, props.transaction) &&
!TransactionUtils.isDistanceRequest(props.transaction);
if (canEdit) {
const menuItems = [];
if (props.canEditReceipt) {
menuItems.push({
icon: Expensicons.Camera,
text: props.translate('common.replace'),
Expand All @@ -393,7 +392,7 @@ function AttachmentModal(props) {
text: props.translate('common.download'),
onSelected: () => downloadAttachment(source),
});
if (TransactionUtils.hasReceipt(props.transaction) && !TransactionUtils.isReceiptBeingScanned(props.transaction) && canEdit) {
if (TransactionUtils.hasReceipt(props.transaction) && !TransactionUtils.isReceiptBeingScanned(props.transaction) && props.canEditReceipt) {
menuItems.push({
icon: Expensicons.Trashcan,
text: props.translate('receipt.deleteReceipt'),
Expand Down
8 changes: 4 additions & 4 deletions src/components/MoneyReportHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ function MoneyReportHeader({session, personalDetails, policy, chatReport, nextSt
const isSettled = ReportUtils.isSettled(moneyRequestReport.reportID);
const policyType = lodashGet(policy, 'type');
const isPolicyAdmin = policyType !== CONST.POLICY.TYPE.PERSONAL && lodashGet(policy, 'role') === CONST.POLICY.ROLE.ADMIN;
const isGroupPolicy = _.contains([CONST.POLICY.TYPE.CORPORATE, CONST.POLICY.TYPE.TEAM], policyType);
const isPaidGroupPolicy = ReportUtils.isPaidGroupPolicy(moneyRequestReport);
const isManager = ReportUtils.isMoneyRequestReport(moneyRequestReport) && lodashGet(session, 'accountID', null) === moneyRequestReport.managerID;
const isPayer = isGroupPolicy
const isPayer = isPaidGroupPolicy
? // In a group policy, the admin approver can pay the report directly by skipping the approval step
isPolicyAdmin && (isApproved || isManager)
: isPolicyAdmin || (ReportUtils.isMoneyRequestReport(moneyRequestReport) && isManager);
Expand All @@ -99,11 +99,11 @@ function MoneyReportHeader({session, personalDetails, policy, chatReport, nextSt
[isPayer, isDraft, isSettled, moneyRequestReport, reimbursableTotal, chatReport],
);
const shouldShowApproveButton = useMemo(() => {
if (!isGroupPolicy) {
if (!isPaidGroupPolicy) {
return false;
}
return isManager && !isDraft && !isApproved && !isSettled;
}, [isGroupPolicy, isManager, isDraft, isApproved, isSettled]);
}, [isPaidGroupPolicy, isManager, isDraft, isApproved, isSettled]);
const shouldShowSettlementButton = shouldShowPayButton || shouldShowApproveButton;
const shouldShowSubmitButton = isDraft && reimbursableTotal !== 0;
const isFromPaidPolicy = policyType === CONST.POLICY.TYPE.TEAM || policyType === CONST.POLICY.TYPE.CORPORATE;
Expand Down
2 changes: 1 addition & 1 deletion src/components/ReportActionItem/MoneyRequestPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ function MoneyRequestPreview(props) {
}

let message = translate('iou.cash');
if (ReportUtils.isGroupPolicyExpenseReport(props.iouReport) && ReportUtils.isReportApproved(props.iouReport) && !ReportUtils.isSettled(props.iouReport)) {
if (ReportUtils.isPaidGroupPolicyExpenseReport(props.iouReport) && ReportUtils.isReportApproved(props.iouReport) && !ReportUtils.isSettled(props.iouReport)) {
message += ` • ${translate('iou.approved')}`;
} else if (props.iouReport.isWaitingOnBankAccount) {
message += ` • ${translate('iou.pending')}`;
Expand Down
44 changes: 25 additions & 19 deletions src/components/ReportActionItem/MoneyRequestView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import lodashGet from 'lodash/get';
import lodashValues from 'lodash/values';
import PropTypes from 'prop-types';
import React, {useCallback, useMemo} from 'react';
import React, {useCallback} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import categoryPropTypes from '@components/categoryPropTypes';
Expand Down Expand Up @@ -37,6 +37,7 @@ import AnimatedEmptyStateBackground from '@pages/home/report/AnimatedEmptyStateB
import reportActionPropTypes from '@pages/home/report/reportActionPropTypes';
import iouReportPropTypes from '@pages/iouReportPropTypes';
import reportPropTypes from '@pages/reportPropTypes';
import {policyDefaultProps, policyPropTypes} from '@pages/workspace/withPolicy';
import * as IOU from '@userActions/IOU';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
Expand Down Expand Up @@ -83,6 +84,9 @@ const propTypes = {
/** The actions from the parent report */
parentReportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)),

/** The policy the report is tied to */
...policyPropTypes,

/** Collection of categories attached to a policy */
policyCategories: PropTypes.objectOf(categoryPropTypes),

Expand All @@ -101,14 +105,15 @@ const propTypes = {
const defaultProps = {
parentReport: {},
parentReportActions: {},
policyCategories: {},
transaction: {
amount: 0,
currency: CONST.CURRENCY.USD,
comment: {comment: ''},
},
transactionViolations: [],
policyCategories: {},
policyTags: {},
...policyDefaultProps,
};

function MoneyRequestView({report, parentReport, parentReportActions, policyCategories, shouldShowHorizontalRule, transaction, policyTags, policy, transactionViolations}) {
Expand Down Expand Up @@ -147,12 +152,18 @@ function MoneyRequestView({report, parentReport, parentReportActions, policyCate
// Flags for allowing or disallowing editing a money request
const isSettled = ReportUtils.isSettled(moneyRequestReport.reportID);
const isCancelled = moneyRequestReport && moneyRequestReport.isCancelledIOU;

// Used for non-restricted fields such as: description, category, tag, billable, etc.
const canEdit = ReportUtils.canEditMoneyRequest(parentReportAction);
const canEditAmount = ReportUtils.canEditMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.AMOUNT, transaction) && !isSettled && !isCardTransaction;
const canEditReceipt = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, moneyRequestReport.reportID, CONST.EDIT_REQUEST_FIELD.RECEIPT);
const canEditAmount = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.AMOUNT);
const canEditMerchant = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.MERCHANT);
const canEditDate = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.DATE);
const canEditReceipt = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.RECEIPT);
const canEditDistance = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.DISTANCE);

// A flag for verifying that the current report is a sub-report of a workspace chat
const isPolicyExpenseChat = useMemo(() => ReportUtils.isPolicyExpenseChat(ReportUtils.getRootParentReport(report)), [report]);
// if the policy of the report is either Collect or Control, then this report must be tied to workspace chat
const isPolicyExpenseChat = ReportUtils.isGroupPolicy(report);
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 woops this doesn't take into account Free policies, I'll revert this back to testing the policy is not personal

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also rename it to isPaidGroupPolicy?

And create a new one name isGroupPolicy checking for free too? it can be done in a follow up too

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 that sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, though I just found out we have the same method in PolicyUtils

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 we're creating a lot of redundant helper methods we should probably some day clean things up :D


// Fetches only the first tag, for now
const policyTag = PolicyUtils.getTag(policyTags);
Expand Down Expand Up @@ -193,18 +204,12 @@ function MoneyRequestView({report, parentReport, parentReportActions, policyCate
}
}

// A temporary solution to hide the transaction detail
// This will be removed after we properly add the transaction as a prop
if (ReportActionsUtils.isDeletedAction(parentReportAction)) {
return null;
}

const hasReceipt = TransactionUtils.hasReceipt(transaction);
let receiptURIs;
let hasErrors = false;
if (hasReceipt) {
receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(transaction);
hasErrors = canEdit && TransactionUtils.hasMissingSmartscanFields(transaction);
hasErrors = canEditReceipt && TransactionUtils.hasMissingSmartscanFields(transaction);
}

const pendingAction = lodashGet(transaction, 'pendingAction');
Expand All @@ -223,11 +228,12 @@ function MoneyRequestView({report, parentReport, parentReportActions, policyCate
isLocalFile={receiptURIs.isLocalFile}
transaction={transaction}
enablePreviewModal
canEditReceipt={canEditReceipt}
/>
</View>
</OfflineWithFeedback>
)}
{!hasReceipt && canEditReceipt && !isSettled && canUseViolations && (
joelbettner marked this conversation as resolved.
Show resolved Hide resolved
{!hasReceipt && canEditReceipt && canUseViolations && (
<ReceiptEmptyState
hasError={hasErrors}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.RECEIPT))}
Expand Down Expand Up @@ -269,8 +275,8 @@ function MoneyRequestView({report, parentReport, parentReportActions, policyCate
<MenuItemWithTopDescription
description={translate('common.distance')}
title={hasPendingWaypoints ? transactionMerchant.replace(CONST.REGEX.FIRST_SPACE, translate('common.tbd')) : transactionMerchant}
interactive={canEdit && !isSettled}
shouldShowRightIcon={canEdit && !isSettled}
interactive={canEditDistance}
shouldShowRightIcon={canEditDistance}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DISTANCE))}
/>
Expand All @@ -280,8 +286,8 @@ function MoneyRequestView({report, parentReport, parentReportActions, policyCate
<MenuItemWithTopDescription
description={translate('common.merchant')}
title={isEmptyMerchant ? '' : transactionMerchant}
interactive={canEdit}
shouldShowRightIcon={canEdit}
interactive={canEditMerchant}
shouldShowRightIcon={canEditMerchant}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.MERCHANT))}
brickRoadIndicator={hasViolations('merchant') || (hasErrors && isEmptyMerchant) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
Expand All @@ -294,8 +300,8 @@ function MoneyRequestView({report, parentReport, parentReportActions, policyCate
<MenuItemWithTopDescription
description={translate('common.date')}
title={transactionDate}
interactive={canEdit && !isSettled}
shouldShowRightIcon={canEdit && !isSettled}
interactive={canEditDate}
shouldShowRightIcon={canEditDate}
titleStyle={styles.flex1}
onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DATE))}
brickRoadIndicator={hasViolations('date') || (hasErrors && transactionDate === '') ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
Expand Down
7 changes: 6 additions & 1 deletion src/components/ReportActionItem/ReportActionItemImage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,17 @@ const propTypes = {

/** whether thumbnail is refer the local file or not */
isLocalFile: PropTypes.bool,

/** whether the receipt can be replaced */
canEditReceipt: PropTypes.bool,
};

const defaultProps = {
thumbnail: null,
transaction: {},
enablePreviewModal: false,
isLocalFile: false,
canEditReceipt: false,
};

/**
Expand All @@ -46,7 +50,7 @@ const defaultProps = {
* and optional preview modal as well.
*/

function ReportActionItemImage({thumbnail, image, enablePreviewModal, transaction, isLocalFile}) {
function ReportActionItemImage({thumbnail, image, enablePreviewModal, transaction, canEditReceipt, isLocalFile}) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const imageSource = tryResolveUrlFromApiRoot(image || '');
Expand Down Expand Up @@ -88,6 +92,7 @@ function ReportActionItemImage({thumbnail, image, enablePreviewModal, transactio
isAuthTokenRequired={!isLocalFile}
report={report}
isReceiptAttachment
canEditReceipt={canEditReceipt}
allowToDownload
originalFileName={transaction.filename}
>
Expand Down
10 changes: 5 additions & 5 deletions src/components/ReportActionItem/ReportPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,22 +243,22 @@ function ReportPreview(props) {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const isGroupPolicy = ReportUtils.isGroupPolicyExpenseChat(props.chatReport);
const isPaidGroupPolicy = ReportUtils.isPaidGroupPolicyExpenseChat(props.chatReport);
const isPolicyAdmin = policyType !== CONST.POLICY.TYPE.PERSONAL && lodashGet(props.policy, 'role') === CONST.POLICY.ROLE.ADMIN;
const isPayer = isGroupPolicy
? // In a group policy, the admin approver can pay the report directly by skipping the approval step
const isPayer = isPaidGroupPolicy
? // In a paid group policy, the admin approver can pay the report directly by skipping the approval step
isPolicyAdmin && (isApproved || isCurrentUserManager)
: isPolicyAdmin || (isMoneyRequestReport && isCurrentUserManager);
const shouldShowPayButton = useMemo(
() => isPayer && !isDraftExpenseReport && !iouSettled && !props.iouReport.isWaitingOnBankAccount && reimbursableSpend !== 0 && !iouCanceled,
[isPayer, isDraftExpenseReport, iouSettled, reimbursableSpend, iouCanceled, props.iouReport],
);
const shouldShowApproveButton = useMemo(() => {
if (!isGroupPolicy) {
if (!isPaidGroupPolicy) {
return false;
}
return isCurrentUserManager && !isDraftExpenseReport && !isApproved && !iouSettled;
}, [isGroupPolicy, isCurrentUserManager, isDraftExpenseReport, isApproved, iouSettled]);
}, [isPaidGroupPolicy, isCurrentUserManager, isDraftExpenseReport, isApproved, iouSettled]);
const shouldShowSettlementButton = shouldShowPayButton || shouldShowApproveButton;
return (
<OfflineWithFeedback pendingAction={lodashGet(props, 'iouReport.pendingFields.preview')}>
Expand Down
Loading
Loading