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

Improve creating expenses on Collect with Instant Submit #36388

Merged
merged 27 commits into from
Mar 7, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
9bc5b38
Allow requesting money from processing reports in Collect with Instan…
youssef-lr Feb 12, 2024
34917e5
Add new expenses to processing reports
youssef-lr Feb 13, 2024
99d1542
Set optimisitc report as submitted if instant submit is turned on
youssef-lr Feb 13, 2024
4fc82e7
Do not create a new report preview if we have a processing report alr…
youssef-lr Feb 13, 2024
f165755
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr Feb 17, 2024
771eef6
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr Feb 22, 2024
4eb5cbf
Add support for splitBill
youssef-lr Feb 23, 2024
9587d19
Add help method for deciding if we can add transactions to a report
youssef-lr Feb 28, 2024
a2675fe
Fix conflicts and mini refactor on how we decide to create new reports
youssef-lr Feb 29, 2024
72ddbc8
Add comment in function definition
youssef-lr Feb 29, 2024
f5a5959
Clean up code in MoneyRequestHeader
youssef-lr Feb 29, 2024
5be0b3f
DRY up code in shouldBuildOptimisticMoneyRequestReport
youssef-lr Feb 29, 2024
206cd78
Lint
youssef-lr Feb 29, 2024
f0a99eb
Fix test
youssef-lr Feb 29, 2024
933e0d8
Fix test for real now
youssef-lr Feb 29, 2024
902f45c
Apply suggestions from code review
youssef-lr Feb 29, 2024
9cccb7e
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr Feb 29, 2024
267de9d
Update tests
youssef-lr Mar 1, 2024
4e9ccc9
Fix test
youssef-lr Mar 1, 2024
c0371b2
Rename variables
youssef-lr Mar 3, 2024
9eb9b97
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr Mar 3, 2024
8496f5d
Fix calls of functions I missed renaming
youssef-lr Mar 3, 2024
ee17782
Apply suggestions from code review
youssef-lr Mar 7, 2024
6a464b3
Fix bug, use isMoneyRequestReport
youssef-lr Mar 7, 2024
1777c68
Cleanup
youssef-lr Mar 7, 2024
9d9caf6
Rename variable
youssef-lr Mar 7, 2024
3fcd191
Fix wrong helper method used
youssef-lr Mar 7, 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
16 changes: 5 additions & 11 deletions src/components/MoneyRequestHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as HeaderUtils from '@libs/HeaderUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
Expand Down Expand Up @@ -79,16 +78,11 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction,
const isScanning = TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction);
const isPending = TransactionUtils.isExpensifyCardTransaction(transaction) && TransactionUtils.isPending(transaction);

const isRequestModifiable = !isSettled && !isApproved && !ReportActionsUtils.isDeletedAction(parentReportAction);
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
const canModifyRequest = isActionOwner && !isSettled && !isApproved && !ReportActionsUtils.isDeletedAction(parentReportAction);
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 also removed this one, because we aren't really modifying the request in the header. We just need to check for permission to hold/unhold, and delete.

Copy link
Contributor

@hoangzinh hoangzinh Mar 4, 2024

Choose a reason for hiding this comment

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

@youssef-lr quick question here, it doesn't need to be a isActionOwner to hold or unhold a request, does it? nvm about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup it doesn't. We used isRequestModifiable for that before, it doesn't have isActionOwner.

let canDeleteRequest = canModifyRequest;
const isDeletedParentAction = ReportActionsUtils.isDeletedAction(parentReportAction);
const canHoldOrUnholdRequest = !isSettled && !isApproved && !isDeletedParentAction;

if (ReportUtils.isPaidGroupPolicyExpenseReport(moneyRequestReport)) {
// If it's a paid policy expense report, only allow deleting the request if it's in draft state or instantly submitted state or the user is the policy admin
canDeleteRequest =
canDeleteRequest &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed in Slack that the admin should not be able to delete requests, the backend also throws an error if we try to. This check was also never true PolicyUtils.isPolicyAdmin(policy)); , as canDeleteRequest assumes the actor is the owner of the action, so it contradicted isPolicyAdmin.

Copy link
Contributor

Choose a reason for hiding this comment

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

As confirmed here #36202 (comment), the current logic of delete action is: user is not only an admin but also an action/request owner. If we remove the check of admin, we probably revert the login in this PR #35744

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That PR is outdated given the new logic of instant submit. The backend will not throw if the policy has it enabled.

(ReportUtils.isDraftExpenseReport(moneyRequestReport) || ReportUtils.isExpenseReportWithInstantSubmittedState(moneyRequestReport) || PolicyUtils.isPolicyAdmin(policy));
}
// If the report supports adding transactions to it, then it also supports deleting transactions from it.
const canDeleteRequest = isActionOwner && ReportUtils.canAddTransactionsToMoneyRequest(moneyRequestReport) && !isDeletedParentAction;

const changeMoneyRequestStatus = () => {
if (isOnHold) {
Expand All @@ -108,7 +102,7 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction,
}, [canDeleteRequest]);

const threeDotsMenuItems = [HeaderUtils.getPinMenuItem(report)];
if (isRequestModifiable) {
if (canHoldOrUnholdRequest) {
const isRequestIOU = parentReport?.type === 'iou';
const isHoldCreator = ReportUtils.isHoldCreator(transaction, report?.reportID) && isRequestIOU;
const canModifyStatus = isPolicyAdmin || isActionOwner || isApprover;
Expand Down
2 changes: 1 addition & 1 deletion src/libs/PolicyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ function isPaidGroupPolicy(policy: OnyxEntry<Policy> | EmptyObject): boolean {
* Checks if policy's scheduled submit / auto reporting frequency is "instant".
* Note: Free policies have "instant" submit always enabled.
*/
function isInstantSubmitEnabled(policy: OnyxEntry<Policy>): boolean {
function isInstantSubmitEnabled(policy: OnyxEntry<Policy> | EmptyObject): boolean {
return policy?.autoReportingFrequency === CONST.POLICY.AUTO_REPORTING_FREQUENCIES.INSTANT || policy?.type === CONST.POLICY.TYPE.FREE;
}

Expand Down
64 changes: 43 additions & 21 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -947,14 +947,6 @@ function isProcessingReport(report: OnyxEntry<Report> | EmptyObject): boolean {
return report?.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && report?.statusNum === CONST.REPORT.STATUS_NUM.SUBMITTED;
}

/**
* Returns true if the policy has `instant` reporting frequency and if the report is still being processed (i.e. submitted state)
*/
function isExpenseReportWithInstantSubmittedState(report: OnyxEntry<Report> | EmptyObject): boolean {
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`] ?? null;
return isExpenseReport(report) && isProcessingReport(report) && PolicyUtils.isInstantSubmitEnabled(policy);
}

/**
* Check if the report is a single chat report that isn't a thread
* and personal detail of participant is optimistic data
Expand Down Expand Up @@ -1244,6 +1236,29 @@ function getChildReportNotificationPreference(reportAction: OnyxEntry<ReportActi
return isActionCreator(reportAction) ? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS : CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
}

/**
* Checks whether the supplied report supports adding more transactions to it.
* Return true if:
* - report is a non-settled IOU
* - report is a draft
* - report is processing and policy's on Instant Submit
youssef-lr marked this conversation as resolved.
Show resolved Hide resolved
*/
function canAddTransactionsToMoneyRequest(report: OnyxEntry<Report>): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Rename to canAddOrDeleteTransactionFromMoneyRequest or something like that so we don't have to add this comment // If the report supports adding transactions to it, then it also supports deleting transactions from it..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True true, on it

if (!isIOUReport(report) && !isExpenseReport(report)) {
return false;
}

if (isReportApproved(report) || isSettled(report?.reportID)) {
return false;
}

if (isGroupPolicy(report) && isProcessingReport(report) && !PolicyUtils.isInstantSubmitEnabled(getPolicy(report?.policyID))) {
return false;
}

return true;
}

/**
* Can only delete if the author is this user and the action is an ADDCOMMENT action or an IOU action in an unsettled report, or if the user is a
* policy admin
Expand All @@ -1264,8 +1279,8 @@ function canDeleteReportAction(reportAction: OnyxEntry<ReportAction>, reportID:

if (isActionOwner) {
if (!isEmptyObject(report) && isPaidGroupPolicyExpenseReport(report)) {
// If it's a paid policy expense report, only allow deleting the request if it's a draft or is instantly submitted or the user is the policy admin
return isDraftExpenseReport(report) || isExpenseReportWithInstantSubmittedState(report) || PolicyUtils.isPolicyAdmin(policy);
// If the report supports adding transactions to it, then it also supports deleting transactions from it.
return canAddTransactionsToMoneyRequest(report);
}
return true;
}
Expand Down Expand Up @@ -2871,11 +2886,11 @@ function buildOptimisticExpenseReport(chatReportID: string, policyID: string, pa
const formattedTotal = CurrencyUtils.convertToDisplayString(storedTotal, currency);
const policy = getPolicy(policyID);

const isFree = policy?.type === CONST.POLICY.TYPE.FREE;
const isInstantSubmitEnabled = PolicyUtils.isInstantSubmitEnabled(policy);

// Define the state and status of the report based on whether the policy is free or paid
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this comment, or update it to match the new condition.

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 the code is self-explanatory, so we can remove

const stateNum = isFree ? CONST.REPORT.STATE_NUM.SUBMITTED : CONST.REPORT.STATE_NUM.OPEN;
const statusNum = isFree ? CONST.REPORT.STATUS_NUM.SUBMITTED : CONST.REPORT.STATUS_NUM.OPEN;
const stateNum = isInstantSubmitEnabled ? CONST.REPORT.STATE_NUM.SUBMITTED : CONST.REPORT.STATE_NUM.OPEN;
const statusNum = isInstantSubmitEnabled ? CONST.REPORT.STATUS_NUM.SUBMITTED : CONST.REPORT.STATUS_NUM.OPEN;

const expenseReport: OptimisticExpenseReport = {
reportID: generateReportID(),
Expand Down Expand Up @@ -4190,7 +4205,6 @@ function canRequestMoney(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>, o
return false;
}

// In case of expense reports, we have to look at the parent workspace chat to get the isOwnPolicyExpenseChat property
let isOwnPolicyExpenseChat = report?.isOwnPolicyExpenseChat ?? false;
if (isExpenseReport(report) && getParentReport(report)) {
isOwnPolicyExpenseChat = Boolean(getParentReport(report)?.isOwnPolicyExpenseChat);
Expand All @@ -4204,12 +4218,8 @@ function canRequestMoney(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>, o
// User can request money in any IOU report, unless paid, but user can only request money in an expense report
// which is tied to their workspace chat.
if (isMoneyRequestReport(report)) {
const isOwnExpenseReport = isExpenseReport(report) && isOwnPolicyExpenseChat;
if (isOwnExpenseReport && PolicyUtils.isPaidGroupPolicy(policy)) {
return isDraftExpenseReport(report) || isExpenseReportWithInstantSubmittedState(report);
}

return (isOwnExpenseReport || isIOUReport(report)) && !isReportApproved(report) && !isSettled(report?.reportID);
const canAddTransactions = canAddTransactionsToMoneyRequest(report);
return isGroupPolicy(report) ? isOwnPolicyExpenseChat && canAddTransactions : canAddTransactions;
}

// In case of policy expense chat, users can only request money from their own policy expense chat
Expand Down Expand Up @@ -4977,6 +4987,17 @@ function canBeAutoReimbursed(report: OnyxEntry<Report>, policy: OnyxEntry<Policy
return isAutoReimbursable;
}

/**
* Used from money request actions to decide if we need to build an optimistic money request report.
Create a new report if:
- we don't have an iouReport set in the chatReport
- we have one, but it's waiting on the payee adding a bank account
- we have one but we can't add more transactions to it due to: report is approved or settled, or, report is processing and policy isn't under Instant Submit
youssef-lr marked this conversation as resolved.
Show resolved Hide resolved
*/
function shouldBuildOptimisticMoneyRequestReport(existingIOUReport: OnyxEntry<Report> | undefined | null, chatReport: OnyxEntry<Report> | null): boolean {
youssef-lr marked this conversation as resolved.
Show resolved Hide resolved
return !existingIOUReport || hasIOUWaitingOnCurrentUserBankAccount(chatReport) || !canAddTransactionsToMoneyRequest(existingIOUReport);
}

export {
getReportParticipantsTitle,
isReportMessageAttachment,
Expand Down Expand Up @@ -5008,7 +5029,6 @@ export {
isPublicAnnounceRoom,
isConciergeChatReport,
isProcessingReport,
isExpenseReportWithInstantSubmittedState,
isCurrentUserTheOnlyParticipant,
hasAutomatedExpensifyAccountIDs,
hasExpensifyGuidesEmails,
Expand Down Expand Up @@ -5179,6 +5199,8 @@ export {
canEditRoomVisibility,
canEditPolicyDescription,
getPolicyDescriptionText,
canAddTransactionsToMoneyRequest,
shouldBuildOptimisticMoneyRequestReport,
};

export type {
Expand Down
48 changes: 17 additions & 31 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -828,37 +828,26 @@ function getMoneyRequestInformation(
// STEP 2: Get the money request report. If the moneyRequestReportID has been provided, we want to add the transaction to this specific report.
// If no such reportID has been provided, let's use the chatReport.iouReportID property. In case that is not present, build a new optimistic money request report.
let iouReport: OnyxEntry<OnyxTypes.Report> = null;
const shouldCreateNewMoneyRequestReport = !moneyRequestReportID && (!chatReport.iouReportID || ReportUtils.hasIOUWaitingOnCurrentUserBankAccount(chatReport));
if (moneyRequestReportID) {
iouReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${moneyRequestReportID}`] ?? null;
} else if (!shouldCreateNewMoneyRequestReport) {
} else {
iouReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${chatReport.iouReportID}`] ?? null;
}

let isFromPaidPolicy = false;
if (isPolicyExpenseChat) {
isFromPaidPolicy = PolicyUtils.isPaidGroupPolicy(policy ?? null);
const shouldCreateNewMoneyRequestReport = ReportUtils.shouldBuildOptimisticMoneyRequestReport(iouReport, chatReport);

// If the linked expense report on paid policy is not draft and not instantly submitted, we need to create a new draft expense report
if (iouReport && isFromPaidPolicy && !ReportUtils.isDraftExpenseReport(iouReport) && !ReportUtils.isExpenseReportWithInstantSubmittedState(iouReport)) {
iouReport = null;
}
}

if (iouReport) {
if (isPolicyExpenseChat) {
iouReport = {...iouReport};
if (iouReport?.currency === currency && typeof iouReport.total === 'number') {
// Because of the Expense reports are stored as negative values, we subtract the total from the amount
iouReport.total -= amount;
}
} else {
iouReport = IOUUtils.updateIOUOwnerAndTotal(iouReport, payeeAccountID, amount, currency);
}
} else {
if (!iouReport || shouldCreateNewMoneyRequestReport) {
iouReport = isPolicyExpenseChat
? ReportUtils.buildOptimisticExpenseReport(chatReport.reportID, chatReport.policyID ?? '', payeeAccountID, amount, currency)
: ReportUtils.buildOptimisticIOUReport(payeeAccountID, payerAccountID, amount, chatReport.reportID, currency);
} else if (isPolicyExpenseChat) {
iouReport = {...iouReport};
if (iouReport?.currency === currency && typeof iouReport.total === 'number') {
// Because of the Expense reports are stored as negative values, we subtract the total from the amount
iouReport.total -= amount;
}
} else {
iouReport = IOUUtils.updateIOUOwnerAndTotal(iouReport, payeeAccountID, amount, currency);
}

// STEP 3: Build optimistic receipt and transaction
Expand Down Expand Up @@ -1843,12 +1832,10 @@ function createSplitsAndOnyxData(
}

// STEP 2: Get existing IOU/Expense report and update its total OR build a new optimistic one
// For Control policy expense chats, if the report is already approved, create a new expense report
let oneOnOneIOUReport: OneOnOneIOUReport = oneOnOneChatReport.iouReportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${oneOnOneChatReport.iouReportID}`] : null;
const shouldCreateNewOneOnOneIOUReport =
!oneOnOneIOUReport || (isOwnPolicyExpenseChat && ReportUtils.isControlPolicyExpenseReport(oneOnOneIOUReport) && ReportUtils.isReportApproved(oneOnOneIOUReport));
const shouldCreateNewMoneyRequestReport = ReportUtils.shouldBuildOptimisticMoneyRequestReport(oneOnOneIOUReport, oneOnOneChatReport);

if (!oneOnOneIOUReport || shouldCreateNewOneOnOneIOUReport) {
if (!oneOnOneIOUReport || shouldCreateNewMoneyRequestReport) {
oneOnOneIOUReport = isOwnPolicyExpenseChat
? ReportUtils.buildOptimisticExpenseReport(oneOnOneChatReport.reportID, oneOnOneChatReport.policyID ?? '', currentUserAccountID, splitAmount, currency)
: ReportUtils.buildOptimisticIOUReport(currentUserAccountID, accountID, splitAmount, oneOnOneChatReport.reportID, currency);
Expand Down Expand Up @@ -1951,7 +1938,7 @@ function createSplitsAndOnyxData(
isNewOneOnOneChatReport,
optimisticTransactionThread,
optimisticCreatedActionForTransactionThread,
shouldCreateNewOneOnOneIOUReport,
shouldCreateNewMoneyRequestReport,
);

const individualSplit = {
Expand Down Expand Up @@ -2484,10 +2471,9 @@ function completeSplitBill(chatReportID: string, reportAction: OnyxTypes.ReportA
}

let oneOnOneIOUReport: OneOnOneIOUReport = oneOnOneChatReport?.iouReportID ? allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${oneOnOneChatReport.iouReportID}`] : null;
const shouldCreateNewOneOnOneIOUReport =
!oneOnOneIOUReport || (isPolicyExpenseChat && ReportUtils.isControlPolicyExpenseReport(oneOnOneIOUReport) && ReportUtils.isReportApproved(oneOnOneIOUReport));
const shouldCreateNewOneOnOneMoneyRequestReport = ReportUtils.shouldBuildOptimisticMoneyRequestReport(oneOnOneIOUReport, oneOnOneChatReport);

if (!oneOnOneIOUReport || shouldCreateNewOneOnOneIOUReport) {
if (!oneOnOneIOUReport || shouldCreateNewOneOnOneMoneyRequestReport) {
oneOnOneIOUReport = isPolicyExpenseChat
? ReportUtils.buildOptimisticExpenseReport(oneOnOneChatReport?.reportID ?? '', participant.policyID ?? '', sessionAccountID, splitAmount, currency ?? '')
: ReportUtils.buildOptimisticIOUReport(sessionAccountID, participant.accountID ?? -1, splitAmount, oneOnOneChatReport?.reportID ?? '', currency ?? '');
Expand Down Expand Up @@ -2554,7 +2540,7 @@ function completeSplitBill(chatReportID: string, reportAction: OnyxTypes.ReportA
isNewOneOnOneChatReport,
optimisticTransactionThread,
optimisticCreatedActionForTransactionThread,
shouldCreateNewOneOnOneIOUReport,
shouldCreateNewOneOnOneMoneyRequestReport,
);

splits.push({
Expand Down
Loading