Skip to content

Commit adea01a

Browse files
authored
Merge pull request Expensify#92174 from daledah/fix/66415
refactor canModifyHoldStatus and getReportFieldsByPolicyID functions
2 parents 67dcd9b + 1b68a56 commit adea01a

5 files changed

Lines changed: 162 additions & 10 deletions

File tree

src/libs/ReportUtils.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4569,12 +4569,12 @@ function getReportFieldKey(reportFieldId: string | undefined) {
45694569
/**
45704570
* Get the report fields attached to the policy given policyID
45714571
*/
4572-
function getReportFieldsByPolicyID(policyID: string | undefined): Policy['fieldList'] {
4572+
function getReportFieldsByPolicyID(policy: OnyxEntry<Policy>): Policy['fieldList'] {
4573+
const policyID = policy?.id;
45734574
if (!policyID) {
45744575
return {};
45754576
}
45764577

4577-
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`];
45784578
const policyDraft = allPolicyDrafts?.[`${ONYXKEYS.COLLECTION.POLICY_DRAFTS}${policyID}`];
45794579
const fieldList = (policy ?? policyDraft)?.fieldList;
45804580

@@ -5113,11 +5113,10 @@ function canEditReportAction(reportAction: OnyxInputOrEntry<ReportAction>, linke
51135113
);
51145114
}
51155115

5116-
function canModifyHoldStatus(report: Report, reportAction: ReportAction, currentUserAccountID: number | undefined): boolean {
5116+
function canModifyHoldStatus(report: Report, reportAction: ReportAction, currentUserAccountID: number | undefined, isAdmin: boolean): boolean {
51175117
if (!isMoneyRequestReport(report) || isTrackExpenseReport(report)) {
51185118
return false;
51195119
}
5120-
const isAdmin = isPolicyAdmin(allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`]);
51215120
const isActionOwner = isActionCreator(reportAction);
51225121
const isManager = isMoneyRequestReport(report) && report?.managerID !== null && currentUserAccountID === report?.managerID;
51235122

@@ -5160,7 +5159,7 @@ function canHoldUnholdReportAction(
51605159
const isOnHold = isOnHoldTransactionUtils(transaction);
51615160
const isClosed = isClosedReport(report);
51625161

5163-
const canModifyStatus = canModifyHoldStatus(report, reportAction, currentUserAccountID);
5162+
const canModifyStatus = canModifyHoldStatus(report, reportAction, currentUserAccountID, isAdmin);
51645163
const canModifyUnholdStatus = !isTrackExpenseMoneyReport && (isAdmin || (isActionOwner && isHoldActionCreator) || isApprover);
51655164

51665165
const canHoldOrUnholdRequest = !isRequestSettled && !isApproved && !isClosed && !isDeletedParentAction(reportAction);
@@ -6736,7 +6735,7 @@ function computeOptimisticReportName(report: Report, policy: OnyxEntry<Policy>,
67366735
return null;
67376736
}
67386737

6739-
const titleReportField = getTitleReportField(getReportFieldsByPolicyID(policyID) ?? {});
6738+
const titleReportField = getTitleReportField(getReportFieldsByPolicyID(policy) ?? {});
67406739
const formulaContext: FormulaContext = {
67416740
report,
67426741
policy,
@@ -13634,6 +13633,7 @@ export {
1363413633
isSortableColumnName,
1363513634
getLinkedIOUTransaction,
1363613635
hasHeldExpensesFromTransactions,
13636+
canModifyHoldStatus,
1363713637
};
1363813638

1363913639
export type {

src/pages/workspace/copyPolicySettings/CopyPolicySettingsSelectFeaturesPage.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ function CopyPolicySettingsSelectFeaturesPage() {
7878
)
7979
: 0;
8080
const taxesCount = Object.values(sourcePolicy?.taxRates?.taxes ?? {}).filter((tax) => tax.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length;
81-
const reportFieldsCount = Object.values(getReportFieldsByPolicyID(sourcePolicyID) ?? {}).filter((field) => field.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length;
81+
const reportFieldsCount = Object.values(getReportFieldsByPolicyID(sourcePolicy) ?? {}).filter((field) => field.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length;
8282
const codingRulesCount = Object.values(sourcePolicy?.rules?.codingRules ?? {}).filter((rule) => rule.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length;
8383
const connectedIntegration = getAllValidConnectedIntegration(sourcePolicy, CONST.POLICY.CONNECTIONS.ACCOUNTING_CONNECTION_NAMES);
8484
const distanceRatesCount = Object.values(getDistanceRateCustomUnit(sourcePolicy)?.rates ?? {}).filter((rate) => rate.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length;

src/pages/workspace/duplicate/WorkspaceDuplicateSelectFeaturesForm.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ function WorkspaceDuplicateSelectFeaturesForm({policyID}: WorkspaceDuplicateForm
4545
const categoriesCount = Object.values(policyCategories ?? {}).filter((category) => category.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length;
4646
const codingRulesCount = Object.values(policy?.rules?.codingRules ?? {}).filter((rule) => rule.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length;
4747
const [selectedItems, setSelectedItems] = useState<string[]>([]);
48-
const reportFields = Object.values(getReportFieldsByPolicyID(policyID) ?? {}).filter((field) => field.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length ?? 0;
48+
const reportFields = Object.values(getReportFieldsByPolicyID(policy) ?? {}).filter((field) => field.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length ?? 0;
4949
const customUnits = getPerDiemCustomUnit(policy);
5050
const customUnitRates: Record<string, Rate> = customUnits?.rates ?? {};
5151
const allRates = Object.values(customUnitRates)?.filter((rate) => rate.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE).length ?? 0;

tests/unit/ReportSecondaryActionUtilsTest.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1790,8 +1790,7 @@ describe('getSecondaryAction', () => {
17901790
} as unknown as Transaction;
17911791
const policy = {} as unknown as Policy;
17921792

1793-
jest.spyOn(ReportUtils, 'isAwaitingFirstLevelApproval').mockReturnValueOnce(false);
1794-
jest.spyOn(ReportUtils, 'isActionCreator').mockReturnValueOnce(true);
1793+
jest.spyOn(ReportUtils, 'canHoldUnholdReportAction').mockReturnValueOnce({canHoldRequest: false, canUnholdRequest: false});
17951794
jest.spyOn(ReportActionsUtils, 'getOneTransactionThreadReportID').mockReturnValueOnce(originalMessageR14932.IOUTransactionID);
17961795
const result = getSecondaryReportActions({
17971796
currentUserLogin: EMPLOYEE_EMAIL,

tests/unit/ReportUtilsTest.ts

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ import {
6565
canHoldUnholdReportAction,
6666
canJoinChat,
6767
canLeaveChat,
68+
canModifyHoldStatus,
6869
canRejectReportAction,
6970
canSeeDefaultRoom,
7071
canUserPerformWriteAction,
@@ -107,6 +108,7 @@ import {
107108
getPolicyName,
108109
getReasonAndReportActionThatRequiresAttention,
109110
getReportActionWithSmartscanError,
111+
getReportFieldsByPolicyID,
110112
getReportIDFromLink,
111113
getReportOrDraftReport,
112114
getReportPreviewMessage,
@@ -18297,6 +18299,157 @@ describe('ReportUtils', () => {
1829718299
});
1829818300
});
1829918301

18302+
describe('getReportFieldsByPolicyID', () => {
18303+
const mockFieldList = {
18304+
expensify_title: {
18305+
fieldID: 'title',
18306+
name: 'title',
18307+
type: 'formula',
18308+
defaultValue: '{report:type}',
18309+
},
18310+
expensify_custom: {
18311+
fieldID: 'custom',
18312+
name: 'custom field',
18313+
type: 'text',
18314+
defaultValue: '',
18315+
},
18316+
} as unknown as NonNullable<Policy['fieldList']>;
18317+
18318+
it('returns empty object when policy is undefined', () => {
18319+
expect(getReportFieldsByPolicyID(undefined)).toEqual({});
18320+
});
18321+
18322+
it('returns empty object when policy has no id', () => {
18323+
const testPolicy = {fieldList: mockFieldList} as unknown as Policy;
18324+
expect(getReportFieldsByPolicyID(testPolicy)).toEqual({});
18325+
});
18326+
18327+
it('returns fieldList from policy when policy has fieldList', () => {
18328+
const testPolicy = {id: 'policyA', fieldList: mockFieldList} as unknown as Policy;
18329+
expect(getReportFieldsByPolicyID(testPolicy)).toEqual(mockFieldList);
18330+
});
18331+
18332+
it('returns empty object when policy has no fieldList', () => {
18333+
const testPolicy = {id: 'policyA'} as unknown as Policy;
18334+
expect(getReportFieldsByPolicyID(testPolicy)).toEqual({});
18335+
});
18336+
18337+
it('prefers policy over policyDraft when both exist', async () => {
18338+
const testPolicyID = 'bothExist';
18339+
const draftFieldList = {
18340+
expensify_draft_field: {
18341+
fieldID: 'draft_field',
18342+
name: 'draft field',
18343+
type: 'text',
18344+
defaultValue: 'draft',
18345+
},
18346+
} as unknown as NonNullable<Policy['fieldList']>;
18347+
18348+
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_DRAFTS}${testPolicyID}`, {
18349+
id: testPolicyID,
18350+
fieldList: draftFieldList,
18351+
} as unknown as Policy);
18352+
await waitForBatchedUpdates();
18353+
18354+
const testPolicy = {id: testPolicyID, fieldList: mockFieldList} as unknown as Policy;
18355+
expect(getReportFieldsByPolicyID(testPolicy)).toEqual(mockFieldList);
18356+
18357+
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_DRAFTS}${testPolicyID}`, null);
18358+
});
18359+
});
18360+
18361+
describe('canModifyHoldStatus', () => {
18362+
const makeReport = (overrides: Partial<Report> = {}): Report =>
18363+
({
18364+
reportID: '1',
18365+
type: CONST.REPORT.TYPE.EXPENSE,
18366+
ownerAccountID: currentUserAccountID,
18367+
...overrides,
18368+
}) as Report;
18369+
18370+
const makeReportAction = (overrides: Partial<ReportAction> = {}): ReportAction =>
18371+
({
18372+
reportActionID: '100',
18373+
actionName: CONST.REPORT.ACTIONS.TYPE.IOU,
18374+
actorAccountID: currentUserAccountID,
18375+
...overrides,
18376+
}) as ReportAction;
18377+
18378+
it('returns false for non-money-request reports', () => {
18379+
const chatReport = makeReport({type: CONST.REPORT.TYPE.CHAT});
18380+
expect(canModifyHoldStatus(chatReport, makeReportAction(), currentUserAccountID, true)).toBe(false);
18381+
});
18382+
18383+
it('returns true for IOU report when user is action owner', () => {
18384+
const iouReport = makeReport({type: CONST.REPORT.TYPE.IOU});
18385+
const reportAction = makeReportAction({actorAccountID: currentUserAccountID});
18386+
expect(canModifyHoldStatus(iouReport, reportAction, currentUserAccountID, false)).toBe(true);
18387+
});
18388+
18389+
it('returns true for IOU report when user is manager', () => {
18390+
const iouReport = makeReport({type: CONST.REPORT.TYPE.IOU, managerID: currentUserAccountID});
18391+
const reportAction = makeReportAction({actorAccountID: 999});
18392+
expect(canModifyHoldStatus(iouReport, reportAction, currentUserAccountID, false)).toBe(true);
18393+
});
18394+
18395+
it('returns false for IOU report when user is neither owner nor manager', () => {
18396+
const iouReport = makeReport({type: CONST.REPORT.TYPE.IOU, managerID: 888});
18397+
const reportAction = makeReportAction({actorAccountID: 999});
18398+
expect(canModifyHoldStatus(iouReport, reportAction, currentUserAccountID, false)).toBe(false);
18399+
});
18400+
18401+
it('returns true for open expense report when user is action owner', () => {
18402+
const openExpenseReport = makeReport({
18403+
type: CONST.REPORT.TYPE.EXPENSE,
18404+
stateNum: CONST.REPORT.STATE_NUM.OPEN,
18405+
statusNum: CONST.REPORT.STATUS_NUM.OPEN,
18406+
});
18407+
const reportAction = makeReportAction({actorAccountID: currentUserAccountID});
18408+
expect(canModifyHoldStatus(openExpenseReport, reportAction, currentUserAccountID, false)).toBe(true);
18409+
});
18410+
18411+
it('returns false for open expense report when user is not action owner', () => {
18412+
const openExpenseReport = makeReport({
18413+
type: CONST.REPORT.TYPE.EXPENSE,
18414+
stateNum: CONST.REPORT.STATE_NUM.OPEN,
18415+
statusNum: CONST.REPORT.STATUS_NUM.OPEN,
18416+
});
18417+
const reportAction = makeReportAction({actorAccountID: 999});
18418+
expect(canModifyHoldStatus(openExpenseReport, reportAction, currentUserAccountID, false)).toBe(false);
18419+
});
18420+
18421+
it('returns true for admin on processing expense report', () => {
18422+
const processingReport = makeReport({
18423+
type: CONST.REPORT.TYPE.EXPENSE,
18424+
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
18425+
statusNum: CONST.REPORT.STATUS_NUM.SUBMITTED,
18426+
});
18427+
const reportAction = makeReportAction({actorAccountID: 999});
18428+
expect(canModifyHoldStatus(processingReport, reportAction, currentUserAccountID, true)).toBe(true);
18429+
});
18430+
18431+
it('returns true for manager on processing expense report', () => {
18432+
const processingReport = makeReport({
18433+
type: CONST.REPORT.TYPE.EXPENSE,
18434+
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
18435+
statusNum: CONST.REPORT.STATUS_NUM.SUBMITTED,
18436+
managerID: currentUserAccountID,
18437+
});
18438+
const reportAction = makeReportAction({actorAccountID: 999});
18439+
expect(canModifyHoldStatus(processingReport, reportAction, currentUserAccountID, false)).toBe(true);
18440+
});
18441+
18442+
it('returns false for admin on non-processing expense report', () => {
18443+
const approvedReport = makeReport({
18444+
type: CONST.REPORT.TYPE.EXPENSE,
18445+
stateNum: CONST.REPORT.STATE_NUM.APPROVED,
18446+
statusNum: CONST.REPORT.STATUS_NUM.APPROVED,
18447+
});
18448+
const reportAction = makeReportAction({actorAccountID: 999});
18449+
expect(canModifyHoldStatus(approvedReport, reportAction, currentUserAccountID, true)).toBe(false);
18450+
});
18451+
});
18452+
1830018453
describe('getBankAccountRoute', () => {
1830118454
it('returns the policy bank account setup route when the report is a policy expense chat', () => {
1830218455
const policyID = 'POLICY_EXP_1';

0 commit comments

Comments
 (0)