-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Changes from 4 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 34917e5
Add new expenses to processing reports
youssef-lr 99d1542
Set optimisitc report as submitted if instant submit is turned on
youssef-lr 4fc82e7
Do not create a new report preview if we have a processing report alr…
youssef-lr f165755
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr 771eef6
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr 4eb5cbf
Add support for splitBill
youssef-lr 9587d19
Add help method for deciding if we can add transactions to a report
youssef-lr a2675fe
Fix conflicts and mini refactor on how we decide to create new reports
youssef-lr 72ddbc8
Add comment in function definition
youssef-lr f5a5959
Clean up code in MoneyRequestHeader
youssef-lr 5be0b3f
DRY up code in shouldBuildOptimisticMoneyRequestReport
youssef-lr 206cd78
Lint
youssef-lr f0a99eb
Fix test
youssef-lr 933e0d8
Fix test for real now
youssef-lr 902f45c
Apply suggestions from code review
youssef-lr 9cccb7e
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr 267de9d
Update tests
youssef-lr 4e9ccc9
Fix test
youssef-lr c0371b2
Rename variables
youssef-lr 9eb9b97
Merge branch 'main' into youssef_instant_submit_collect
youssef-lr 8496f5d
Fix calls of functions I missed renaming
youssef-lr ee17782
Apply suggestions from code review
youssef-lr 6a464b3
Fix bug, use isMoneyRequestReport
youssef-lr 1777c68
Cleanup
youssef-lr 9d9caf6
Rename variable
youssef-lr 3fcd191
Fix wrong helper method used
youssef-lr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1255,16 +1255,16 @@ function getChildReportNotificationPreference(reportAction: OnyxEntry<ReportActi | |
* - report is a draft | ||
* - report is a processing expense report and its policy has Instant reporting frequency | ||
*/ | ||
function canAddTransactionsToMoneyRequest(report: OnyxEntry<Report>): boolean { | ||
if (!isIOUReport(report) && !isExpenseReport(report)) { | ||
function canAddOrDeleteTransactions(moneyRequestReport: OnyxEntry<Report>): boolean { | ||
if (!isMoneyRequest(moneyRequestReport)) { | ||
return false; | ||
} | ||
|
||
if (isReportApproved(report) || isSettled(report?.reportID)) { | ||
if (isReportApproved(moneyRequestReport) || isSettled(moneyRequestReport?.reportID)) { | ||
return false; | ||
} | ||
|
||
if (isGroupPolicy(report) && isProcessingReport(report) && !PolicyUtils.isInstantSubmitEnabled(getPolicy(report?.policyID))) { | ||
if (isGroupPolicy(moneyRequestReport) && isProcessingReport(moneyRequestReport) && !PolicyUtils.isInstantSubmitEnabled(getPolicy(moneyRequestReport?.policyID))) { | ||
return false; | ||
} | ||
|
||
|
@@ -1285,14 +1285,13 @@ function canDeleteReportAction(reportAction: OnyxEntry<ReportAction>, reportID: | |
// For now, users cannot delete split actions | ||
const isSplitAction = reportAction?.originalMessage?.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT; | ||
|
||
if (isSplitAction || isSettled(String(reportAction?.originalMessage?.IOUReportID)) || (!isEmptyObject(report) && isReportApproved(report))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed this logic as it's already handled in |
||
if (isSplitAction) { | ||
return false; | ||
} | ||
|
||
if (isActionOwner) { | ||
if (!isEmptyObject(report) && isPaidGroupPolicyExpenseReport(report)) { | ||
// If the report supports adding transactions to it, then it also supports deleting transactions from it. | ||
return canAddTransactionsToMoneyRequest(report); | ||
if (!isEmptyObject(report) && isMoneyRequestReport(report)) { | ||
return canAddOrDeleteTransactions(report); | ||
} | ||
return true; | ||
} | ||
|
@@ -2974,7 +2973,6 @@ function buildOptimisticExpenseReport(chatReportID: string, policyID: string, pa | |
|
||
const isInstantSubmitEnabled = PolicyUtils.isInstantSubmitEnabled(policy); | ||
|
||
// Define the state and status of the report based on whether the policy is free or paid | ||
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; | ||
|
||
|
@@ -4315,7 +4313,7 @@ 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 canAddTransactions = canAddTransactionsToMoneyRequest(report); | ||
const canAddTransactions = canAddOrDeleteTransactions(report); | ||
return isGroupPolicy(report) ? isOwnPolicyExpenseChat && canAddTransactions : canAddTransactions; | ||
} | ||
|
||
|
@@ -5093,7 +5091,7 @@ function canBeAutoReimbursed(report: OnyxEntry<Report>, policy: OnyxEntry<Policy | |
- 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 on Instant submit reporting frequency | ||
*/ | ||
function shouldCreateNewMoneyRequestReport(existingIOUReport: OnyxEntry<Report> | undefined | null, chatReport: OnyxEntry<Report> | null): boolean { | ||
return !existingIOUReport || hasIOUWaitingOnCurrentUserBankAccount(chatReport) || !canAddTransactionsToMoneyRequest(existingIOUReport); | ||
return !existingIOUReport || hasIOUWaitingOnCurrentUserBankAccount(chatReport) || !canAddOrDeleteTransactions(existingIOUReport); | ||
} | ||
|
||
export { | ||
|
@@ -5298,7 +5296,7 @@ export { | |
canEditRoomVisibility, | ||
canEditPolicyDescription, | ||
getPolicyDescriptionText, | ||
canAddTransactionsToMoneyRequest, | ||
canAddOrDeleteTransactions, | ||
shouldCreateNewMoneyRequestReport, | ||
}; | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isMoneyRequest
only check for IOU reports, and I think this is making the button not show up inside the expense report (step 7 of the tests).I think you will need to update this to use
isMoneyRequestReport
as wellThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might fix the failing tests as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops just a mistake, will update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should only be
isMoneyRequestReport
here, because we can't add transactions to a transaction :D