Skip to content

Commit f9894b8

Browse files
emkhalidemkhalid
committed
Fix optimistic next-step approver without affecting unapprove flow
1 parent cd19088 commit f9894b8

4 files changed

Lines changed: 103 additions & 6 deletions

File tree

‎src/libs/ReportUtils.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ import addTrailingForwardSlash from './UrlUtils';
309309
import type {AvatarSource} from './UserAvatarUtils';
310310
import {getDefaultAvatarURL} from './UserAvatarUtils';
311311
import {generateAccountID} from './UserUtils';
312-
import {isInvalidMerchantValue, isValidAccountRoute} from './ValidationUtils';
312+
import {isInvalidMerchantValue} from './ValidationUtils';
313313
import ViolationsUtils from './Violations/ViolationsUtils';
314314

315315
type AvatarRange = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18;
@@ -4792,7 +4792,7 @@ function getNextApproverAccountID(report: OnyxEntry<Report>, isUnapproved = fals
47924792
return deprecatedCurrentUserAccountID;
47934793
}
47944794

4795-
return isValidAccountRoute(submitToAccountID) ? submitToAccountID : report?.managerID;
4795+
return report?.managerID ?? submitToAccountID;
47964796
}
47974797

47984798
if (approvalChain.length === 0) {

‎src/libs/actions/IOU/ReportWorkflow.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ import {
6363
isScanning,
6464
isScanningTransaction,
6565
} from '@libs/TransactionUtils';
66+
import {isValidAccountRoute} from '@libs/ValidationUtils';
6667
import CONST from '@src/CONST';
6768
import ONYXKEYS from '@src/ONYXKEYS';
6869
import ROUTES from '@src/ROUTES';
@@ -1301,6 +1302,7 @@ function submitReport({
13011302
const adminAccountID = policy?.role === CONST.POLICY.ROLE.ADMIN ? currentUserAccountIDParam : undefined;
13021303
const parentReport = getReportOrDraftReport(expenseReport.parentReportID);
13031304
const managerID = getSubmitReportManagerAccountID(policy, expenseReport);
1305+
const optimisticNextStepApproverID = !isSubmitAndClosePolicy && isValidAccountRoute(managerID ?? CONST.DEFAULT_NUMBER_ID) ? managerID : undefined;
13041306
const isCurrentUserManager = currentUserAccountIDParam === managerID;
13051307
const optimisticSubmittedReportAction = buildOptimisticSubmittedReportAction(
13061308
expenseReport?.total ?? 0,
@@ -1326,6 +1328,7 @@ function submitReport({
13261328
hasViolations,
13271329
isASAPSubmitBetaEnabled,
13281330
isUnapprove: true,
1331+
bypassNextApproverID: optimisticNextStepApproverID,
13291332
});
13301333
const optimisticNextStep = isDEWPolicy
13311334
? null
@@ -1338,6 +1341,7 @@ function submitReport({
13381341
hasViolations,
13391342
isASAPSubmitBetaEnabled,
13401343
isUnapprove: true,
1344+
bypassNextApproverID: optimisticNextStepApproverID,
13411345
});
13421346
const optimisticData: Array<
13431347
OnyxUpdate<typeof ONYXKEYS.COLLECTION.REPORT_ACTIONS | typeof ONYXKEYS.COLLECTION.REPORT | typeof ONYXKEYS.COLLECTION.NEXT_STEP | typeof ONYXKEYS.COLLECTION.REPORT_METADATA>

‎tests/actions/IOUTest/ReportWorkflowTest.ts‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import * as API from '@src/libs/API';
3030
import DateUtils from '@src/libs/DateUtils';
3131
import ONYXKEYS from '@src/ONYXKEYS';
3232
import ROUTES from '@src/ROUTES';
33-
import type {Policy, Report, ReportNameValuePairs} from '@src/types/onyx';
33+
import type {Policy, Report, ReportNameValuePairs, ReportNextStepDeprecated} from '@src/types/onyx';
3434
import type ReportAction from '@src/types/onyx/ReportAction';
3535
import type {ReportActions} from '@src/types/onyx/ReportAction';
3636
import type {OnyxData} from '@src/types/onyx/Request';
@@ -1389,6 +1389,100 @@ describe('actions/IOU/ReportWorkflow', () => {
13891389
const optimisticReportUpdate = onyxData.optimisticData?.find((update) => update.key === `${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`);
13901390
expect((optimisticReportUpdate?.value as Report | undefined)?.managerID).toBe(adminAccountID);
13911391
});
1392+
1393+
it('uses the rule approver in the optimistic next step when the existing report manager is stale', async () => {
1394+
// eslint-disable-next-line rulesdir/no-multiple-api-calls -- Inspecting API.write calls to verify submit payload and optimistic data.
1395+
const apiWriteSpy = jest.spyOn(API, 'write').mockImplementation(() => Promise.resolve());
1396+
const policyID = '1';
1397+
const submitterAccountID = 100;
1398+
const defaultApproverAccountID = 101;
1399+
const ruleApproverAccountID = 102;
1400+
const submitterEmail = 'submitter@example.com';
1401+
const defaultApproverEmail = 'default-approver@example.com';
1402+
const ruleApproverEmail = 'rule-approver@example.com';
1403+
1404+
await Onyx.set(ONYXKEYS.PERSONAL_DETAILS_LIST, {
1405+
[submitterAccountID]: {accountID: submitterAccountID, login: submitterEmail},
1406+
[defaultApproverAccountID]: {accountID: defaultApproverAccountID, login: defaultApproverEmail},
1407+
[ruleApproverAccountID]: {accountID: ruleApproverAccountID, login: ruleApproverEmail},
1408+
});
1409+
1410+
const policy: Policy = {
1411+
...createRandomPolicy(Number(policyID)),
1412+
id: policyID,
1413+
type: CONST.POLICY.TYPE.CORPORATE,
1414+
approvalMode: CONST.POLICY.APPROVAL_MODE.ADVANCED,
1415+
approver: defaultApproverEmail,
1416+
owner: defaultApproverEmail,
1417+
employeeList: {
1418+
[submitterEmail]: {
1419+
email: submitterEmail,
1420+
submitsTo: defaultApproverEmail,
1421+
},
1422+
},
1423+
rules: {
1424+
approvalRules: [
1425+
{
1426+
id: 'travel-rule',
1427+
applyWhen: [
1428+
{
1429+
field: CONST.POLICY.FIELDS.CATEGORY,
1430+
condition: CONST.POLICY.RULE_CONDITIONS.MATCHES,
1431+
value: 'Travel',
1432+
},
1433+
],
1434+
approver: ruleApproverEmail,
1435+
},
1436+
],
1437+
},
1438+
};
1439+
const expenseReport: Report = {
1440+
...createRandomReport(Number(policyID), undefined),
1441+
reportID: '1',
1442+
policyID,
1443+
type: CONST.REPORT.TYPE.EXPENSE,
1444+
ownerAccountID: submitterAccountID,
1445+
managerID: defaultApproverAccountID,
1446+
stateNum: CONST.REPORT.STATE_NUM.OPEN,
1447+
statusNum: CONST.REPORT.STATUS_NUM.OPEN,
1448+
total: 1000,
1449+
currency: CONST.CURRENCY.USD,
1450+
};
1451+
const transaction: Transaction = {
1452+
...createRandomTransaction(1),
1453+
reportID: expenseReport.reportID,
1454+
category: 'Travel',
1455+
};
1456+
1457+
await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction);
1458+
await waitForBatchedUpdates();
1459+
1460+
submitReport({
1461+
expenseReport,
1462+
policy,
1463+
currentUserAccountIDParam: submitterAccountID,
1464+
currentUserEmailParam: submitterEmail,
1465+
hasViolations: false,
1466+
isASAPSubmitBetaEnabled: false,
1467+
expenseReportCurrentNextStepDeprecated: undefined,
1468+
userBillingGracePeriodEnds: undefined,
1469+
amountOwed: 0,
1470+
ownerBillingGracePeriodEnd: undefined,
1471+
delegateEmail: undefined,
1472+
});
1473+
1474+
const [, parameters, onyxData] = apiWriteSpy.mock.calls.at(-1) as [unknown, {managerAccountID?: number}, OnyxData<typeof ONYXKEYS.COLLECTION.REPORT>];
1475+
expect(parameters.managerAccountID).toBe(ruleApproverAccountID);
1476+
1477+
const optimisticReportUpdate = onyxData.optimisticData?.find((update) => update.key === `${ONYXKEYS.COLLECTION.REPORT}${expenseReport.reportID}`);
1478+
expect((optimisticReportUpdate?.value as Report | undefined)?.managerID).toBe(ruleApproverAccountID);
1479+
expect((optimisticReportUpdate?.value as Report | undefined)?.nextStep?.actorAccountID).toBe(ruleApproverAccountID);
1480+
1481+
const optimisticDeprecatedNextStepUpdate = onyxData.optimisticData?.find((update) => update.key === `${ONYXKEYS.COLLECTION.NEXT_STEP}${expenseReport.reportID}`);
1482+
const optimisticDeprecatedNextStep = optimisticDeprecatedNextStepUpdate?.value as ReportNextStepDeprecated | undefined;
1483+
expect(optimisticDeprecatedNextStep?.message?.find((message) => message.type === 'strong')?.text).toBe(ruleApproverEmail);
1484+
});
1485+
13921486
it('keeps the workspace chat outstanding when an admin submits after approver changes', async () => {
13931487
// eslint-disable-next-line rulesdir/no-multiple-api-calls -- Inspecting optimistic parent chat data after submit from workspace chat.
13941488
const apiWriteSpy = jest.spyOn(API, 'write').mockImplementation(() => Promise.resolve());

‎tests/unit/ReportUtilsTest.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,6 @@ const rules = {
452452
const employeeAccountID = 2;
453453
const policyAdminAccountID = 1;
454454
const categoryApprover1Email = 'categoryapprover1@test.com';
455-
const categoryApprover1AccountID = 3;
456455
const categoryApprover2Email = 'categoryapprover2@test.com';
457456
const tagApprover1Email = 'tagapprover1@test.com';
458457
const tagApprover2Email = 'tagapprover2@test.com';
@@ -8018,7 +8017,7 @@ describe('ReportUtils', () => {
80188017
await waitForBatchedUpdates();
80198018
});
80208019

8021-
it('should prefer a valid submit target over a stale report manager when unapproving', async () => {
8020+
it('should use the report manager when unapproving even if the submit target differs', async () => {
80228021
const policyID = 'next-approver-policy';
80238022
const policyTest: Policy = {
80248023
...createRandomPolicy(0),
@@ -8047,7 +8046,7 @@ describe('ReportUtils', () => {
80478046
await Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transaction);
80488047
await waitForBatchedUpdates();
80498048

8050-
expect(getNextApproverAccountID(expenseReport, true)).toBe(categoryApprover1AccountID);
8049+
expect(getNextApproverAccountID(expenseReport, true)).toBe(policyAdminAccountID);
80518050
});
80528051

80538052
it('should fall back to report manager when submit target is invalid', async () => {

0 commit comments

Comments
 (0)