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

Improve creating expenses on Collect with Instant Submit #36388

Merged
merged 27 commits into from
Mar 7, 2024
Merged

Improve creating expenses on Collect with Instant Submit #36388

Show file tree
Hide file tree
Changes from 7 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
2 changes: 1 addition & 1 deletion src/libs/PolicyUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,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
8 changes: 4 additions & 4 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2921,11 +2921,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 = policy?.type === CONST.POLICY.TYPE.FREE || (PolicyUtils.isPaidGroupPolicy(policy) && PolicyUtils.isInstantSubmitEnabled(policy));
Beamanator marked this conversation as resolved.
Show resolved Hide resolved

// 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 @@ -4255,7 +4255,7 @@ function canRequestMoney(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>, o
if (isMoneyRequestReport(report)) {
const isOwnExpenseReport = isExpenseReport(report) && isOwnPolicyExpenseChat;
if (isOwnExpenseReport && PolicyUtils.isPaidGroupPolicy(policy)) {
return isDraftExpenseReport(report);
return isDraftExpenseReport(report) || (PolicyUtils.isInstantSubmitEnabled(policy) && isProcessingReport(report));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @youssef-lr is there any doc that helps me verify this logic? Or can you help to leave a comment in code to explain it? Thanks

Copy link
Contributor Author

@youssef-lr youssef-lr Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can add a comment. There is a doc, but I think you might not have access to 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.

I think this comment already explains this logic. If Instant Submit is turned on, we can add expenses to an already submitted report. If reporting frequency is set to 'Weekly', then we can only add expenses to draft reports (i.e. not submitted yet).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying @youssef-lr .

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe at this point you can revert this change after pulling main - since this was just added in #36997 - sorry!

Copy link
Contributor Author

@youssef-lr youssef-lr Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Beamanator I'm thinking of removing that helper function, it seems unnecessary and the naming is verbose and a bit confusing. imo PolicyUtils.isInstantSubmitEnabled(policy) && isProcessingReport(report) is more readable than isExpenseReportWithInstantSubmittedState, what is really InstantSubmittedState? I get that it means the report is submitted and under Instant Submit but it can be confusing for someone unfamiliar with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean & yeah maybe the function name is kinda gross, but don't we reuse the same check (PolicyUtils.isInstantSubmitEnabled(policy) && isProcessingReport(report)) in many places & won't we continue to do so? So wouldn't it be nice to make a util function for this? I slightly prefer a util function IFF we can come up with a good name for it that's super clear 😅

Copy link
Contributor Author

@youssef-lr youssef-lr Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good, I can't think of a way to improve that function name, so I think I'll create a new one which will instead decide if we can add transactions to a report or not, similar to how we do it in Auth. Will commit the update and let me know what you think! I think it looks cleaner and is more generic and reusable.

}

return (isOwnExpenseReport || isIOUReport(report)) && !isReportApproved(report) && !isSettled(report?.reportID);
Expand Down
17 changes: 12 additions & 5 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ 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));
let shouldCreateNewMoneyRequestReport = !moneyRequestReportID && (!chatReport.iouReportID || ReportUtils.hasIOUWaitingOnCurrentUserBankAccount(chatReport));
if (moneyRequestReportID) {
iouReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${moneyRequestReportID}`] ?? null;
} else if (!shouldCreateNewMoneyRequestReport) {
Expand All @@ -758,10 +758,17 @@ function getMoneyRequestInformation(
// If the scheduled submit is turned off on the policy, user needs to manually submit the report which is indicated by GBR in LHN
needsToBeManuallySubmitted = isFromPaidPolicy && !policy?.harvesting?.enabled;

// If the linked expense report on paid policy is not draft, we need to create a new draft expense report
if (iouReport && isFromPaidPolicy && !ReportUtils.isDraftExpenseReport(iouReport)) {
const isProcessingReportOnInstantSubmitPolicy =
isFromPaidPolicy && ReportUtils.isProcessingReport(iouReport) && PolicyUtils.isInstantSubmitEnabled(policy ?? ({} as OnyxEntry<OnyxTypes.Policy>));
Beamanator marked this conversation as resolved.
Show resolved Hide resolved

// If the linked expense report on paid policy is not draft, or is not processing in a policy with Instant Submit enabled, we need to create a new draft expense report
if (iouReport && isFromPaidPolicy && !ReportUtils.isDraftExpenseReport(iouReport) && !isProcessingReportOnInstantSubmitPolicy) {
iouReport = null;
}

if (isProcessingReportOnInstantSubmitPolicy) {
shouldCreateNewMoneyRequestReport = false;
}
}

if (iouReport) {
Expand Down Expand Up @@ -1728,7 +1735,7 @@ function createSplitsAndOnyxData(
// 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));
!oneOnOneIOUReport || (isOwnPolicyExpenseChat && ReportUtils.isPaidGroupPolicy(oneOnOneIOUReport) && ReportUtils.isReportApproved(oneOnOneIOUReport));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanna double check whether we should use ReportUtils.isPaidGroupPolicyExpenseReport instead of ReportUtils.isPaidGroupPolicy here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanna double check whether we should use ReportUtils.isPaidGroupPolicyExpenseReport instead of ReportUtils.isPaidGroupPolicy here.

It doesn't really matter, both should work fine.


if (!oneOnOneIOUReport || shouldCreateNewOneOnOneIOUReport) {
oneOnOneIOUReport = isOwnPolicyExpenseChat
Expand Down Expand Up @@ -2359,7 +2366,7 @@ 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));
!oneOnOneIOUReport || (isPolicyExpenseChat && ReportUtils.isPaidGroupPolicy(oneOnOneIOUReport) && ReportUtils.isReportApproved(oneOnOneIOUReport));

if (!oneOnOneIOUReport || shouldCreateNewOneOnOneIOUReport) {
oneOnOneIOUReport = isPolicyExpenseChat
Expand Down
Loading