From 5615db1f941618fffa54d268ede43f25ce72efbb Mon Sep 17 00:00:00 2001 From: "Shubham Agrawal (via MelvinBot)" Date: Sun, 25 Oct 2026 13:36:47 +0000 Subject: [PATCH 1/6] Show report field save errors on the field's row in workspace settings Co-authored-by: Shubham Agrawal --- src/libs/ReportUtils.ts | 8 +- src/libs/actions/Policy/ReportField.ts | 50 +++++++++-- .../fields/WorkspaceFieldsSection.tsx | 42 ++++++--- tests/actions/ReportFieldTest.ts | 88 +++++++++++++++++++ tests/unit/ReportUtilsTest.ts | 34 +++++++ 5 files changed, 202 insertions(+), 20 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index b1d299082bb8..0826bec2b01e 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -5638,7 +5638,7 @@ function getReportFieldsByPolicyID(policy: OnyxEntry): Policy['fieldList * Get the report fields that we should display a MoneyReportView gets opened */ -function getAvailableReportFields(report: OnyxEntry, policyReportFields: PolicyReportField[]): PolicyReportField[] { +function getAvailableReportFields(report: OnyxEntry, policyReportFields: Array): PolicyReportField[] { // Get the report fields that are attached to a report. These will persist even if a field is deleted from the policy. const reportFields = Object.values(report?.fieldList ?? {}); const reportIsSettled = isSettled(report?.reportID); @@ -5650,7 +5650,11 @@ function getAvailableReportFields(report: OnyxEntry, policyReportFields: // If the report is unsettled, we want to merge the new fields that get added to the policy with the fields that // are attached to the report. - const mergedFieldIds = Array.from(new Set([...policyReportFields.map(({fieldID}) => fieldID), ...reportFields.map(({fieldID}) => fieldID)])); + // Skip policy fields pending ADD that aren't on the report yet. Optimistic adds are written to the report too, so this + // keeps fields that failed to be created (kept on the policy until their error is dismissed) off reports. + const reportFieldIDs = new Set(reportFields.map(({fieldID}) => fieldID)); + const availablePolicyReportFields = policyReportFields.filter(({fieldID, pendingAction}) => pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD || reportFieldIDs.has(fieldID)); + const mergedFieldIds = Array.from(new Set([...availablePolicyReportFields.map(({fieldID}) => fieldID), ...reportFieldIDs])); const fields = mergedFieldIds.map((id) => { const field = report?.fieldList?.[getReportFieldKey(id)] ?? report?.fieldList?.[id]; diff --git a/src/libs/actions/Policy/ReportField.ts b/src/libs/actions/Policy/ReportField.ts index 01cd2864f232..f5f8fc41f07a 100644 --- a/src/libs/actions/Policy/ReportField.ts +++ b/src/libs/actions/Policy/ReportField.ts @@ -95,6 +95,18 @@ type UpdateReportFieldInitialValueParams = { policy: OnyxEntry; }; +/** + * Returns the errorFields update that clears a single report field's errors without wiping other policy errors (such as the report title error). + */ +function getReportFieldErrorFieldsToClear(fieldKey: string) { + return { + [fieldKey]: null, + fieldList: { + [fieldKey]: null, + }, + }; +} + function openPolicyReportFieldsPage(policyID: string) { if (!policyID) { Log.warn('openPolicyReportFieldsPage invalid params', {policyID}); @@ -237,7 +249,7 @@ function createReportField({name, type, initialValue, listValues, disabledListVa fieldList: { [fieldKey]: {...optimisticReportFieldDataForPolicy, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}, }, - errorFields: null, + errorFields: getReportFieldErrorFieldsToClear(fieldKey), }, }, ...(policyReportIDs ?? []).map( @@ -258,8 +270,9 @@ function createReportField({name, type, initialValue, listValues, disabledListVa key: `${ONYXKEYS.COLLECTION.POLICY}${policy?.id}`, onyxMethod: Onyx.METHOD.MERGE, value: { + // Keep the failed field in the list so its error shows on its own row until the user dismisses it fieldList: { - [fieldKey]: null, + [fieldKey]: {...optimisticReportFieldDataForPolicy, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}, }, errorFields: { [fieldKey]: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('workspace.reportFields.genericFailureMessage'), @@ -289,7 +302,7 @@ function createReportField({name, type, initialValue, listValues, disabledListVa fieldList: { [fieldKey]: {pendingAction: null}, }, - errorFields: null, + errorFields: getReportFieldErrorFieldsToClear(fieldKey), }, }, ], @@ -427,7 +440,7 @@ function updateReportFieldInitialValue({policy, reportFieldID, newInitialValue}: fieldList: { [fieldKey]: {...updatedReportField, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}, }, - errorFields: null, + errorFields: getReportFieldErrorFieldsToClear(fieldKey), }, }, ], @@ -439,7 +452,7 @@ function updateReportFieldInitialValue({policy, reportFieldID, newInitialValue}: fieldList: { [fieldKey]: {pendingAction: null}, }, - errorFields: null, + errorFields: getReportFieldErrorFieldsToClear(fieldKey), }, }, ], @@ -626,9 +639,36 @@ function removeReportFieldListValue({policy, reportFieldID, valueIndexes}: Remov API.write(removeCommand, parameters, onyxData); } +/** + * Clears a report field's errors. A field that failed to be created is removed from the list. + */ +function clearReportFieldErrors(policy: OnyxEntry, reportFieldID: string) { + if (!policy?.id) { + return; + } + + const fieldKey = ReportUtils.getReportFieldKey(reportFieldID); + const reportField = policy.fieldList?.[fieldKey]; + + if (reportField?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) { + Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { + fieldList: { + [fieldKey]: null, + }, + errorFields: getReportFieldErrorFieldsToClear(fieldKey), + }); + return; + } + + Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { + errorFields: getReportFieldErrorFieldsToClear(fieldKey), + }); +} + export type {CreateReportFieldParams}; export { + clearReportFieldErrors, setInitialCreateReportFieldsForm, createReportFieldsListValue, renameReportFieldsListValue, diff --git a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx index 153549291517..6594bc48d106 100644 --- a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx +++ b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx @@ -16,15 +16,17 @@ import usePolicyFeatureWriteAccess from '@hooks/usePolicyFeatureWriteAccess'; import useThemeStyles from '@hooks/useThemeStyles'; import {isConnectionInProgress, isConnectionUnverified} from '@libs/actions/connections'; +import {getLatestErrorField} from '@libs/ErrorUtils'; import Navigation from '@libs/Navigation/Navigation'; import {getConnectedIntegration, isControlPolicy, shouldShowSyncError} from '@libs/PolicyUtils'; import type {PolicyFeature} from '@libs/PolicyUtils'; +import {getReportFieldKey} from '@libs/ReportUtils'; import {getReportFieldTypeTranslationKey, isReportFieldImportedFromIntegration} from '@libs/WorkspaceReportFieldUtils'; import {getCurrentAccountingIntegrationName} from '@pages/workspace/accounting/utils'; import ToggleSettingOptionRow from '@pages/workspace/workflows/ToggleSettingsOptionRow'; -import {setInitialCreateReportFieldsForm} from '@userActions/Policy/ReportField'; +import {clearReportFieldErrors, setInitialCreateReportFieldsForm} from '@userActions/Policy/ReportField'; import CONST from '@src/CONST'; import type {TranslationPaths} from '@src/languages/types'; @@ -32,7 +34,8 @@ import ONYXKEYS from '@src/ONYXKEYS'; import ROUTES from '@src/ROUTES'; import type {Route} from '@src/ROUTES'; import type {Policy, PolicyConnectionName} from '@src/types/onyx'; -import type {PendingAction} from '@src/types/onyx/OnyxCommon'; +import type {Errors, PendingAction} from '@src/types/onyx/OnyxCommon'; +import {isEmptyObject} from '@src/types/utils/EmptyObject'; import type {ListRenderItemInfo} from '@shopify/flash-list'; import type {OnyxEntry} from 'react-native-onyx'; @@ -46,6 +49,7 @@ type FieldListValue = NonNullable[string]; type FieldListItem = { fieldID: string; + errors: Errors; isDisabled: boolean; keyForList: string; pendingAction?: PendingAction; @@ -117,6 +121,7 @@ function WorkspaceFieldsSection({ const isConnectionVerified = connectedIntegration && !isConnectionUnverified(policy, connectedIntegration); const currentConnectionName = getCurrentAccountingIntegrationName(policy, translate); const fieldList = policy?.fieldList; + const errorFields = policy?.errorFields; const hasImportedField = useMemo( () => Object.values(fieldList ?? {}).some((field) => fieldFilter(field) && isReportFieldImportedFromIntegration(field, policy)), [fieldFilter, fieldList, policy], @@ -141,15 +146,21 @@ function WorkspaceFieldsSection({ return Object.values(fieldList) .filter(fieldFilter) .sort((a, b) => localeCompare(a.name, b.name)) - .map((field) => ({ - text: field.name, - keyForList: String(field.fieldID), - fieldID: field.fieldID, - pendingAction: field.pendingAction, - isDisabled: field.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, - rightLabel: Str.recapitalize(translate(getReportFieldTypeTranslationKey(field.type ?? CONST.REPORT_FIELD_TYPES.TEXT))), - })); - }, [fieldFilter, fieldList, localeCompare, translate]); + .map((field) => { + const fieldKey = getReportFieldKey(field.fieldID); + const defaultValueErrors = getLatestErrorField({errorFields: errorFields?.fieldList?.[fieldKey] ?? {}}, 'defaultValue'); + + return { + text: field.name, + keyForList: String(field.fieldID), + fieldID: field.fieldID, + pendingAction: field.pendingAction, + errors: isEmptyObject(defaultValueErrors) ? getLatestErrorField({errorFields}, fieldKey) : defaultValueErrors, + isDisabled: field.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, + rightLabel: Str.recapitalize(translate(getReportFieldTypeTranslationKey(field.type ?? CONST.REPORT_FIELD_TYPES.TEXT))), + }; + }); + }, [errorFields, fieldFilter, fieldList, localeCompare, translate]); const navigateToFieldSettings = useCallback( (item: FieldListItem) => { @@ -164,7 +175,12 @@ function WorkspaceFieldsSection({ const renderItem = useCallback( ({item}: ListRenderItemInfo) => ( - + clearReportFieldErrors(policy, item.fieldID)} + > navigateToFieldSettings(item)} @@ -177,7 +193,7 @@ function WorkspaceFieldsSection({ /> ), - [canWrite, navigateToFieldSettings, cardPaddingHorizontal, styles.popoverMenuText, styles.textStrong], + [canWrite, navigateToFieldSettings, cardPaddingHorizontal, policy, styles.mb2, styles.popoverMenuText, styles.textStrong], ); const headerText = diff --git a/tests/actions/ReportFieldTest.ts b/tests/actions/ReportFieldTest.ts index 1e8639df010c..f4fea2f6e557 100644 --- a/tests/actions/ReportFieldTest.ts +++ b/tests/actions/ReportFieldTest.ts @@ -230,6 +230,94 @@ describe('actions/ReportField', () => { // Check if the policy pending action was cleared expect(policy?.fieldList?.[reportFieldKey].pendingAction).toBeFalsy(); }); + + it('keeps the failed report field with an error until the error is dismissed', async () => { + // Given a workspace with a report title error + const reportFieldName = 'Test Field'; + const reportFieldID = generateFieldID(reportFieldName); + const reportFieldKey = ReportUtils.getReportFieldKey(reportFieldID); + const titleErrors = {[CONST.POLICY.FIELDS.FIELD_LIST_TITLE]: 'Title error'}; + const fakePolicy: PolicyType = { + ...createRandomPolicy(0), + fieldList: {}, + errorFields: {fieldList: titleErrors}, + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${fakePolicy.id}`, fakePolicy); + + // When creating a report field fails on the server + mockFetch.pause(); + ReportField.createReportField({ + policy: fakePolicy, + name: reportFieldName, + type: CONST.REPORT_FIELD_TYPES.TEXT, + initialValue: '{field:DoesNotExist}', + listValues: [], + disabledListValues: [], + policyReportIDs: [], + }); + await waitForBatchedUpdates(); + mockFetch.fail(); + mockFetch.resume(); + await waitForBatchedUpdates(); + + // Then the field stays in the list with its error so the user can see which field failed, and the title error is kept + let policy = await connectToFetchPolicy(fakePolicy.id); + expect(policy?.fieldList?.[reportFieldKey]?.pendingAction).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD); + expect(policy?.errorFields?.[reportFieldKey]).toBeTruthy(); + expect(policy?.errorFields?.fieldList).toBeTruthy(); + + // When the user dismisses the error + ReportField.clearReportFieldErrors(policy, reportFieldID); + await waitForBatchedUpdates(); + + // Then the failed field and its errors are removed, and the title error is untouched + policy = await connectToFetchPolicy(fakePolicy.id); + expect(policy?.fieldList?.[reportFieldKey]).toBeUndefined(); + expect(policy?.errorFields?.[reportFieldKey]).toBeUndefined(); + expect(policy?.errorFields?.fieldList).toEqual(titleErrors); + }); + }); + + describe('clearReportFieldErrors', () => { + it('clears the errors of an existing report field without removing it', async () => { + // Given an existing report field whose initial value update was rejected by the server + const reportFieldName = 'Test Field'; + const reportFieldID = generateFieldID(reportFieldName); + const reportFieldKey = ReportUtils.getReportFieldKey(reportFieldID); + const reportField: PolicyReportField = { + name: reportFieldName, + type: CONST.REPORT_FIELD_TYPES.TEXT, + defaultValue: 'Default Value', + values: [], + disabledOptions: [], + fieldID: reportFieldID, + orderWeight: 1, + deletable: false, + keys: [], + externalIDs: [], + isTax: false, + }; + const errorKey = DateUtils.getMicroseconds(); + const fakePolicy: PolicyType = { + ...createRandomPolicy(0), + fieldList: {[reportFieldKey]: reportField}, + errorFields: { + [reportFieldKey]: {[errorKey]: 'Generic error'}, + fieldList: {[reportFieldKey]: 'Formula field {field:DoesNotExist} not recognized'}, + }, + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${fakePolicy.id}`, fakePolicy); + + // When the user dismisses the error + ReportField.clearReportFieldErrors(fakePolicy, reportFieldID); + await waitForBatchedUpdates(); + + // Then both errors are cleared and the field is kept, because it already exists on the server + const policy = await connectToFetchPolicy(fakePolicy.id); + expect(policy?.fieldList?.[reportFieldKey]).toEqual(reportField); + expect(policy?.errorFields?.[reportFieldKey]).toBeUndefined(); + expect(policy?.errorFields?.fieldList?.[reportFieldKey]).toBeUndefined(); + }); }); it('optimistically treats Text type with formula-like initial value as Formula', async () => { diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 9e015a831925..cad7f7721d7f 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -22750,6 +22750,40 @@ describe('ReportUtils', () => { ]); expect(getAvailableReportFields(report, policyFieldList)).toEqual(expectedFieldList); }); + + it('should skip policy fields pending ADD that are not on the report', async () => { + // Given an unsettled report and a policy field that failed to be created, so it is still pending ADD on the policy but was removed from the report + const report = createMock({ + reportID: '3', + policyID: '1', + fieldList: fieldList1, + }); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); + const failedPolicyField = {...fieldList2.expensify_field_id_LIST_POLICY, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}; + + // When getting the available report fields + const availableFields = getAvailableReportFields(report, [failedPolicyField]); + + // Then the failed field is not shown on the report + expect(availableFields).toEqual(Object.values(fieldList1)); + }); + + it('should keep policy fields pending ADD that are already on the report', async () => { + // Given an unsettled report that has an optimistically added field, which is also pending ADD on the policy + const report = createMock({ + reportID: '4', + policyID: '1', + fieldList: fieldList1, + }); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); + const pendingPolicyField = {...fieldList1.expensify_field_id_LIST, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}; + + // When getting the available report fields + const availableFields = getAvailableReportFields(report, [pendingPolicyField]); + + // Then the optimistic field is still shown on the report + expect(availableFields.map(({fieldID}) => fieldID)).toEqual(['field_id_LIST']); + }); }); describe('getReportFieldMaps', () => { From f6b791088de804c741bdeb7f3f54250a9c1487fc Mon Sep 17 00:00:00 2001 From: "Shubham Agrawal (via MelvinBot)" Date: Sun, 25 Oct 2026 14:41:50 +0000 Subject: [PATCH 2/6] Set report field failure errors at errorFields.fieldList and drop redundant failure fieldList update The optimistic data already keeps the new field on the policy with pendingAction ADD, so the failure data no longer re-writes it. The generic failure error now goes to errorFields.fieldList..defaultValue, where the server returns its errors, so the row reads a single location. Co-authored-by: Shubham Agrawal --- src/libs/actions/Policy/ReportField.ts | 29 +++++++++++-------- .../fields/WorkspaceFieldsSection.tsx | 4 +-- tests/actions/ReportFieldTest.ts | 19 ++++++------ 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/libs/actions/Policy/ReportField.ts b/src/libs/actions/Policy/ReportField.ts index f5f8fc41f07a..ca3b9869ef23 100644 --- a/src/libs/actions/Policy/ReportField.ts +++ b/src/libs/actions/Policy/ReportField.ts @@ -25,7 +25,7 @@ import ONYXKEYS from '@src/ONYXKEYS'; import type {WorkspaceReportFieldForm} from '@src/types/form/WorkspaceReportFieldForm'; import INPUT_IDS from '@src/types/form/WorkspaceReportFieldForm'; import type {Policy, PolicyReportField} from '@src/types/onyx'; -import type {OnyxValueWithOfflineFeedback} from '@src/types/onyx/OnyxCommon'; +import type {ErrorFields, OnyxValueWithOfflineFeedback} from '@src/types/onyx/OnyxCommon'; import type {OnyxData} from '@src/types/onyx/Request'; import type {OnyxEntry, OnyxUpdate} from 'react-native-onyx'; @@ -100,13 +100,26 @@ type UpdateReportFieldInitialValueParams = { */ function getReportFieldErrorFieldsToClear(fieldKey: string) { return { - [fieldKey]: null, fieldList: { [fieldKey]: null, }, }; } +/** + * Returns the errorFields update that sets a report field's generic failure error at errorFields.fieldList..defaultValue, where the server also returns its errors. + */ +function getReportFieldFailureErrorFields(fieldKey: string) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- ErrorFields doesn't model the nested fieldList..defaultValue shape the server uses, same as the report title error + return { + fieldList: { + [fieldKey]: { + defaultValue: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('workspace.reportFields.genericFailureMessage'), + }, + }, + } as unknown as ErrorFields; +} + function openPolicyReportFieldsPage(policyID: string) { if (!policyID) { Log.warn('openPolicyReportFieldsPage invalid params', {policyID}); @@ -270,13 +283,7 @@ function createReportField({name, type, initialValue, listValues, disabledListVa key: `${ONYXKEYS.COLLECTION.POLICY}${policy?.id}`, onyxMethod: Onyx.METHOD.MERGE, value: { - // Keep the failed field in the list so its error shows on its own row until the user dismisses it - fieldList: { - [fieldKey]: {...optimisticReportFieldDataForPolicy, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}, - }, - errorFields: { - [fieldKey]: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('workspace.reportFields.genericFailureMessage'), - }, + errorFields: getReportFieldFailureErrorFields(fieldKey), }, }, ...(policyReportIDs ?? []).map( @@ -464,9 +471,7 @@ function updateReportFieldInitialValue({policy, reportFieldID, newInitialValue}: fieldList: { [fieldKey]: {...previousFieldList[fieldKey], pendingAction: null}, }, - errorFields: { - [fieldKey]: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('workspace.reportFields.genericFailureMessage'), - }, + errorFields: getReportFieldFailureErrorFields(fieldKey), }, }, ], diff --git a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx index 6594bc48d106..21213a857d20 100644 --- a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx +++ b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx @@ -35,7 +35,6 @@ import ROUTES from '@src/ROUTES'; import type {Route} from '@src/ROUTES'; import type {Policy, PolicyConnectionName} from '@src/types/onyx'; import type {Errors, PendingAction} from '@src/types/onyx/OnyxCommon'; -import {isEmptyObject} from '@src/types/utils/EmptyObject'; import type {ListRenderItemInfo} from '@shopify/flash-list'; import type {OnyxEntry} from 'react-native-onyx'; @@ -148,14 +147,13 @@ function WorkspaceFieldsSection({ .sort((a, b) => localeCompare(a.name, b.name)) .map((field) => { const fieldKey = getReportFieldKey(field.fieldID); - const defaultValueErrors = getLatestErrorField({errorFields: errorFields?.fieldList?.[fieldKey] ?? {}}, 'defaultValue'); return { text: field.name, keyForList: String(field.fieldID), fieldID: field.fieldID, pendingAction: field.pendingAction, - errors: isEmptyObject(defaultValueErrors) ? getLatestErrorField({errorFields}, fieldKey) : defaultValueErrors, + errors: getLatestErrorField({errorFields: errorFields?.fieldList?.[fieldKey] ?? {}}, 'defaultValue'), isDisabled: field.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, rightLabel: Str.recapitalize(translate(getReportFieldTypeTranslationKey(field.type ?? CONST.REPORT_FIELD_TYPES.TEXT))), }; diff --git a/tests/actions/ReportFieldTest.ts b/tests/actions/ReportFieldTest.ts index f4fea2f6e557..98dd7dae3778 100644 --- a/tests/actions/ReportFieldTest.ts +++ b/tests/actions/ReportFieldTest.ts @@ -13,7 +13,7 @@ import * as ReportUtils from '@src/libs/ReportUtils'; import ONYXKEYS from '@src/ONYXKEYS'; import INPUT_IDS from '@src/types/form/WorkspaceReportFieldForm'; import type {PolicyReportField, Policy as PolicyType} from '@src/types/onyx'; -import type {OnyxValueWithOfflineFeedback} from '@src/types/onyx/OnyxCommon'; +import type {ErrorFields, OnyxValueWithOfflineFeedback} from '@src/types/onyx/OnyxCommon'; import type {OnyxEntry} from 'react-native-onyx'; @@ -263,8 +263,8 @@ describe('actions/ReportField', () => { // Then the field stays in the list with its error so the user can see which field failed, and the title error is kept let policy = await connectToFetchPolicy(fakePolicy.id); expect(policy?.fieldList?.[reportFieldKey]?.pendingAction).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD); - expect(policy?.errorFields?.[reportFieldKey]).toBeTruthy(); - expect(policy?.errorFields?.fieldList).toBeTruthy(); + expect(policy?.errorFields?.fieldList?.[reportFieldKey]).toBeTruthy(); + expect(policy?.errorFields?.fieldList?.[CONST.POLICY.FIELDS.FIELD_LIST_TITLE]).toBe(titleErrors[CONST.POLICY.FIELDS.FIELD_LIST_TITLE]); // When the user dismisses the error ReportField.clearReportFieldErrors(policy, reportFieldID); @@ -273,7 +273,6 @@ describe('actions/ReportField', () => { // Then the failed field and its errors are removed, and the title error is untouched policy = await connectToFetchPolicy(fakePolicy.id); expect(policy?.fieldList?.[reportFieldKey]).toBeUndefined(); - expect(policy?.errorFields?.[reportFieldKey]).toBeUndefined(); expect(policy?.errorFields?.fieldList).toEqual(titleErrors); }); }); @@ -301,10 +300,11 @@ describe('actions/ReportField', () => { const fakePolicy: PolicyType = { ...createRandomPolicy(0), fieldList: {[reportFieldKey]: reportField}, + // The server returns report field errors at errorFields.fieldList..defaultValue, which ErrorFields doesn't model + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion errorFields: { - [reportFieldKey]: {[errorKey]: 'Generic error'}, - fieldList: {[reportFieldKey]: 'Formula field {field:DoesNotExist} not recognized'}, - }, + fieldList: {[reportFieldKey]: {defaultValue: {[errorKey]: 'Formula field {field:DoesNotExist} not recognized'}}}, + } as unknown as ErrorFields, }; await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${fakePolicy.id}`, fakePolicy); @@ -312,10 +312,9 @@ describe('actions/ReportField', () => { ReportField.clearReportFieldErrors(fakePolicy, reportFieldID); await waitForBatchedUpdates(); - // Then both errors are cleared and the field is kept, because it already exists on the server + // Then the error is cleared and the field is kept, because it already exists on the server const policy = await connectToFetchPolicy(fakePolicy.id); expect(policy?.fieldList?.[reportFieldKey]).toEqual(reportField); - expect(policy?.errorFields?.[reportFieldKey]).toBeUndefined(); expect(policy?.errorFields?.fieldList?.[reportFieldKey]).toBeUndefined(); }); }); @@ -568,7 +567,7 @@ describe('actions/ReportField', () => { [reportFieldKey]: reportField, }); // Check if the policy errors was set - expect(policy?.errorFields?.[reportFieldKey]).toBeTruthy(); + expect(policy?.errorFields?.fieldList?.[reportFieldKey]).toBeTruthy(); }); it('reverts optimistic type to text when new initial value is non-formula', async () => { From 73fc3e7497352bcba984c0a618c07d1f35eeb7ba Mon Sep 17 00:00:00 2001 From: "Shubham Agrawal (via MelvinBot)" Date: Sun, 25 Oct 2026 15:05:57 +0000 Subject: [PATCH 3/6] Use errorRowTextStyles for the report field error row spacing Co-authored-by: Shubham Agrawal --- src/pages/workspace/fields/WorkspaceFieldsSection.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx index 21213a857d20..cac16a11b997 100644 --- a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx +++ b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx @@ -176,7 +176,8 @@ function WorkspaceFieldsSection({ clearReportFieldErrors(policy, item.fieldID)} > ), - [canWrite, navigateToFieldSettings, cardPaddingHorizontal, policy, styles.mb2, styles.popoverMenuText, styles.textStrong], + [canWrite, navigateToFieldSettings, cardPaddingHorizontal, policy, styles.mv2, styles.popoverMenuText, styles.textStrong], ); const headerText = From 89286214c9a1f23a50d01099fb6354f98ac0219a Mon Sep 17 00:00:00 2001 From: "Shubham Agrawal (via MelvinBot)" Date: Sun, 25 Oct 2026 15:34:54 +0000 Subject: [PATCH 4/6] Hide only failed report field adds from reports, not pending offline adds Co-authored-by: Shubham Agrawal --- .../MoneyRequestViewReportFields.tsx | 2 +- .../ReportActionItem/MoneyReportView.tsx | 4 +-- src/libs/ReportUtils.ts | 22 ++++++++++----- .../ReportDetailsTitleSection.tsx | 2 +- tests/unit/ReportUtilsTest.ts | 27 ++++++++++++++++--- 5 files changed, 42 insertions(+), 15 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx index f86f158685b8..ea1028db4715 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx @@ -141,7 +141,7 @@ function MoneyRequestViewReportFields({report, policy, pendingAction, style, sho }; const sortedPolicyReportFields = useMemo((): EnrichedPolicyReportField[] => { - const {fieldValues, fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); + const {fieldValues, fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs, policy?.errorFields); const fields = Object.values(fieldsByName); return fields diff --git a/src/components/ReportActionItem/MoneyReportView.tsx b/src/components/ReportActionItem/MoneyReportView.tsx index a3ab9bc5a161..06a27867bf69 100644 --- a/src/components/ReportActionItem/MoneyReportView.tsx +++ b/src/components/ReportActionItem/MoneyReportView.tsx @@ -135,9 +135,9 @@ function MoneyReportView({ ]; const policyReportFields = useMemo(() => { - const {fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs); + const {fieldsByName} = getReportFieldMaps(report, policy?.fieldList ?? {}, reportNameValuePairs, policy?.errorFields); return Object.values(fieldsByName).filter((field) => isReportFieldTargetMatchingReport(report, field)); - }, [policy?.fieldList, report, reportNameValuePairs]); + }, [policy?.fieldList, policy?.errorFields, report, reportNameValuePairs]); const isOnlyTitleFieldEnabled = policyReportFields.every(shouldHideSingleReportField); const isClosedExpenseReportWithNoExpenses = isClosedExpenseReportWithNoExpensesReportUtils(report, transactions.length > 0); diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 0826bec2b01e..5ca2832f5f5c 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -5573,7 +5573,8 @@ function isReportFieldDisabledForUser( */ function canEditReportTitle(report: OnyxEntry, policy: OnyxEntry, currentUserAccountID: number | undefined, rules: OnyxCollection): boolean { const titleField = - getAvailableReportFields(report, Object.values(policy?.fieldList ?? {})).find((reportField) => isReportFieldOfTypeTitle(reportField)) ?? getTitleFieldWithFallback(policy); + getAvailableReportFields(report, Object.values(policy?.fieldList ?? {}), policy?.errorFields).find((reportField) => isReportFieldOfTypeTitle(reportField)) ?? + getTitleFieldWithFallback(policy); const isFieldDisabled = isReportFieldDisabled(report, titleField, policy, rules); return !isFieldDisabled && isAdminOwnerApproverOrReportOwner(report, policy, currentUserAccountID) && isExpenseReport(report) && isGroupPolicyPolicyUtils(policy); @@ -5638,7 +5639,11 @@ function getReportFieldsByPolicyID(policy: OnyxEntry): Policy['fieldList * Get the report fields that we should display a MoneyReportView gets opened */ -function getAvailableReportFields(report: OnyxEntry, policyReportFields: Array): PolicyReportField[] { +function getAvailableReportFields( + report: OnyxEntry, + policyReportFields: Array, + policyErrorFields?: ErrorFields, +): PolicyReportField[] { // Get the report fields that are attached to a report. These will persist even if a field is deleted from the policy. const reportFields = Object.values(report?.fieldList ?? {}); const reportIsSettled = isSettled(report?.reportID); @@ -5650,10 +5655,12 @@ function getAvailableReportFields(report: OnyxEntry, policyReportFields: // If the report is unsettled, we want to merge the new fields that get added to the policy with the fields that // are attached to the report. - // Skip policy fields pending ADD that aren't on the report yet. Optimistic adds are written to the report too, so this - // keeps fields that failed to be created (kept on the policy until their error is dismissed) off reports. + // Skip policy fields that failed to be created. They stay on the policy, pending ADD with an error, until the error is + // dismissed. Fields still pending ADD without an error (for example, created offline) are kept. const reportFieldIDs = new Set(reportFields.map(({fieldID}) => fieldID)); - const availablePolicyReportFields = policyReportFields.filter(({fieldID, pendingAction}) => pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD || reportFieldIDs.has(fieldID)); + const availablePolicyReportFields = policyReportFields.filter( + ({fieldID, pendingAction}) => pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD || !policyErrorFields?.fieldList?.[getReportFieldKey(fieldID)], + ); const mergedFieldIds = Array.from(new Set([...availablePolicyReportFields.map(({fieldID}) => fieldID), ...reportFieldIDs])); const fields = mergedFieldIds.map((id) => { @@ -10821,7 +10828,7 @@ function hasVisibleReportFieldViolations( return false; } - const {fieldsByName} = getReportFieldMaps(report, policy.fieldList, reportNameValuePairs); + const {fieldsByName} = getReportFieldMaps(report, policy.fieldList, reportNameValuePairs, policy.errorFields); return Object.values(fieldsByName).some((field) => { if (!isReportFieldTargetMatchingReport(report, field)) { @@ -15064,8 +15071,9 @@ function getReportFieldMaps( report: OnyxEntry, fieldList: Record, reportNameValuePairs: OnyxEntry, + policyErrorFields?: ErrorFields, ): {fieldValues: Record; fieldsByName: Record} { - const availableFields = getAvailableReportFields(report, Object.values(fieldList ?? {})); + const availableFields = getAvailableReportFields(report, Object.values(fieldList ?? {}), policyErrorFields); const invoiceReportFields = report?.type === CONST.REPORT.TYPE.INVOICE ? getInvoiceReportFieldsFromReportNameValuePairs(reportNameValuePairs) : []; const fieldsMatchingReport = [...availableFields, ...invoiceReportFields].filter((field) => isReportFieldTargetMatchingReport(report, field)); const fields = [...new Map(fieldsMatchingReport.map((field) => [field.fieldID, field])).values()]; diff --git a/src/pages/DynamicReportDetailsPage/ReportDetailsTitleSection.tsx b/src/pages/DynamicReportDetailsPage/ReportDetailsTitleSection.tsx index 45942fe1a3b5..3a9016856564 100644 --- a/src/pages/DynamicReportDetailsPage/ReportDetailsTitleSection.tsx +++ b/src/pages/DynamicReportDetailsPage/ReportDetailsTitleSection.tsx @@ -76,7 +76,7 @@ function ReportDetailsTitleSectionContent({report}: ReportDetailsTitleSectionCon const parentNavigationSubtitleData = getParentNavigationSubtitle(report, policy, conciergeReportID, translate, formatPhoneNumber, derivedParentReportName, isParentReportArchived); const titleField: OnyxTypes.PolicyReportField | undefined = (() => { - const fields = getAvailableReportFields(report, Object.values(policy?.fieldList ?? {})); + const fields = getAvailableReportFields(report, Object.values(policy?.fieldList ?? {}), policy?.errorFields); return fields.find((reportField) => isReportFieldOfTypeTitle(reportField)); })(); const fieldKey = getReportFieldKey(titleField?.fieldID); diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index cad7f7721d7f..253c85c44432 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -274,7 +274,7 @@ import type { Transaction, TransactionViolation, } from '@src/types/onyx'; -import type {Icon, OnyxValueWithOfflineFeedback} from '@src/types/onyx/OnyxCommon'; +import type {ErrorFields, Icon, OnyxValueWithOfflineFeedback} from '@src/types/onyx/OnyxCommon'; import type {ACHAccount, PolicyReportField} from '@src/types/onyx/Policy'; import type {Participant, Participants, ReportCollectionDataSet} from '@src/types/onyx/Report'; import type {ReportActionsCollectionDataSet} from '@src/types/onyx/ReportAction'; @@ -22751,8 +22751,8 @@ describe('ReportUtils', () => { expect(getAvailableReportFields(report, policyFieldList)).toEqual(expectedFieldList); }); - it('should skip policy fields pending ADD that are not on the report', async () => { - // Given an unsettled report and a policy field that failed to be created, so it is still pending ADD on the policy but was removed from the report + it('should skip policy fields that failed to be created', async () => { + // Given an unsettled report and a policy field that failed to be created, so it is still pending ADD on the policy with an error and was removed from the report const report = createMock({ reportID: '3', policyID: '1', @@ -22760,14 +22760,33 @@ describe('ReportUtils', () => { }); await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); const failedPolicyField = {...fieldList2.expensify_field_id_LIST_POLICY, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}; + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- ErrorFields doesn't model the nested fieldList..defaultValue shape the server uses + const policyErrorFields = {fieldList: {expensify_field_id_LIST_POLICY: {defaultValue: {1: 'Error'}}}} as unknown as ErrorFields; // When getting the available report fields - const availableFields = getAvailableReportFields(report, [failedPolicyField]); + const availableFields = getAvailableReportFields(report, [failedPolicyField], policyErrorFields); // Then the failed field is not shown on the report expect(availableFields).toEqual(Object.values(fieldList1)); }); + it('should keep policy fields pending ADD without an error that are not on the report', async () => { + // Given a report created after a field was added offline, so the field is pending ADD on the policy, has no error, and was never written to this report + const report = createMock({ + reportID: 'reportCreatedAfterOfflineFieldAdd', + policyID: '1', + fieldList: {}, + }); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report); + const pendingPolicyField = {...fieldList2.expensify_field_id_LIST_POLICY, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}; + + // When getting the available report fields + const availableFields = getAvailableReportFields(report, [pendingPolicyField], {}); + + // Then the pending field is still shown, because it hasn't failed + expect(availableFields.map(({fieldID}) => fieldID)).toEqual(['field_id_LIST_POLICY']); + }); + it('should keep policy fields pending ADD that are already on the report', async () => { // Given an unsettled report that has an optimistically added field, which is also pending ADD on the policy const report = createMock({ From 93fa674f21aacc2902bcee79b7ebd9aec0bc7955 Mon Sep 17 00:00:00 2001 From: "Shubham Agrawal (via MelvinBot)" Date: Tue, 27 Oct 2026 15:37:10 +0000 Subject: [PATCH 5/6] Treat report fields that failed to be created as missing outside their row Their row no longer opens the settings page, and the name check, the circular reference check, and the Search report field filters skip them. Co-authored-by: Shubham Agrawal --- .../MoneyRequestViewReportFields.tsx | 3 +- src/hooks/useAdvancedSearchFilters.ts | 3 +- src/libs/ReportUtils.ts | 27 +++++++++--- src/pages/DynamicEditReportFieldPage.tsx | 6 ++- .../workspace/fields/CreateFieldsPage.tsx | 7 +++- .../fields/FieldsInitialValuePage.tsx | 4 +- .../fields/WorkspaceFieldsSection.tsx | 11 +++-- src/selectors/Policy.ts | 5 ++- tests/unit/ReportUtilsTest.ts | 42 +++++++++++++++++++ 9 files changed, 89 insertions(+), 19 deletions(-) diff --git a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx index 70ecd751c17f..29eb587a9650 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx @@ -18,6 +18,7 @@ import { getFieldViolationTranslation, getReportFieldKey, getReportFieldMaps, + getReportFieldsWithoutFailedAdds, isReportFieldDisabledForUser, isReportFieldTargetMatchingReport, shouldDisplayReportFields as shouldDisplayReportFieldsUtils, @@ -97,7 +98,7 @@ function ReportFieldView( value={reportField.fieldValue} isDisabled={reportField.isFieldDisabled} errorText={reportField.violationTranslation} - fieldList={policy?.fieldList} + fieldList={getReportFieldsWithoutFailedAdds(policy?.fieldList, policy?.errorFields)} onSaveValue={(value) => onSaveValue(reportField, value)} onDelete={ reportField.isFieldDeletable diff --git a/src/hooks/useAdvancedSearchFilters.ts b/src/hooks/useAdvancedSearchFilters.ts index cb76d36a79b9..efe74fdebd54 100644 --- a/src/hooks/useAdvancedSearchFilters.ts +++ b/src/hooks/useAdvancedSearchFilters.ts @@ -1,5 +1,6 @@ import {isFilterableBankAccount} from '@libs/BankAccountUtils'; import {hasVendorFeatureOnAnyPolicy, isPolicyFeatureEnabled} from '@libs/PolicyUtils'; +import {getReportFieldsWithoutFailedAdds} from '@libs/ReportUtils'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; @@ -246,7 +247,7 @@ const policyDerivedSelector = (policies: OnyxCollection) => { isAttendeeTrackingEnabled = isPolicyFeatureEnabled(policy, CONST.POLICY.MORE_FEATURES.IS_ATTENDEE_TRACKING_ENABLED); } if (!hasReportFields) { - hasReportFields = Object.values(policy.fieldList ?? {}).some((val) => val.type !== CONST.POLICY.DEFAULT_FIELD_LIST_TYPE); + hasReportFields = Object.values(getReportFieldsWithoutFailedAdds(policy.fieldList, policy.errorFields)).some((val) => val.type !== CONST.POLICY.DEFAULT_FIELD_LIST_TYPE); } if (!hasAnyTaxRates && policy.taxRates?.taxes && Object.keys(policy.taxRates.taxes).length > 0) { hasAnyTaxRates = true; diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index c94f9f32c73f..9073fa8889e2 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -5667,6 +5667,24 @@ function getReportFieldKey(reportFieldId: string | undefined) { return `expensify_${reportFieldId}`; } +/** + * Whether a policy report field failed to be created. It stays on the policy, pending ADD with an error, until the error + * is dismissed. Fields still pending ADD without an error (for example, created offline) haven't failed. + */ +function isFailedReportFieldAdd(reportField: (PolicyReportField & {pendingAction?: PendingAction | null}) | undefined, policyErrorFields: ErrorFields | undefined): boolean { + return reportField?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD && !!policyErrorFields?.fieldList?.[getReportFieldKey(reportField.fieldID)]; +} + +/** + * Get the policy report fields without the ones that failed to be created + */ +function getReportFieldsWithoutFailedAdds( + fieldList: Record | undefined, + policyErrorFields: ErrorFields | undefined, +): Record { + return Object.fromEntries(Object.entries(fieldList ?? {}).filter(([, reportField]) => !isFailedReportFieldAdd(reportField, policyErrorFields))); +} + /** * Get the report fields attached to the policy given policyID */ @@ -5706,12 +5724,9 @@ function getAvailableReportFields( // If the report is unsettled, we want to merge the new fields that get added to the policy with the fields that // are attached to the report. - // Skip policy fields that failed to be created. They stay on the policy, pending ADD with an error, until the error is - // dismissed. Fields still pending ADD without an error (for example, created offline) are kept. + // Policy fields that failed to be created are skipped. const reportFieldIDs = new Set(reportFields.map(({fieldID}) => fieldID)); - const availablePolicyReportFields = policyReportFields.filter( - ({fieldID, pendingAction}) => pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD || !policyErrorFields?.fieldList?.[getReportFieldKey(fieldID)], - ); + const availablePolicyReportFields = policyReportFields.filter((policyReportField) => !isFailedReportFieldAdd(policyReportField, policyErrorFields)); const mergedFieldIds = Array.from(new Set([...availablePolicyReportFields.map(({fieldID}) => fieldID), ...reportFieldIDs])); const fields = mergedFieldIds.map((id) => { @@ -15530,6 +15545,8 @@ export { getReimbursementQueuedActionMessage, getReportDescription, getReportFieldKey, + getReportFieldsWithoutFailedAdds, + isFailedReportFieldAdd, getReportFieldMaps, getReportIDFromLink, getReportTransactions, diff --git a/src/pages/DynamicEditReportFieldPage.tsx b/src/pages/DynamicEditReportFieldPage.tsx index f24e212352c5..650e610aaed7 100644 --- a/src/pages/DynamicEditReportFieldPage.tsx +++ b/src/pages/DynamicEditReportFieldPage.tsx @@ -23,6 +23,7 @@ import {getReportName} from '@libs/ReportNameUtils'; import { getReportFieldFromReportNameValuePairs, getReportFieldKey, + getReportFieldsWithoutFailedAdds, getTitleFieldWithFallback, isGroupPolicyExpenseReport, isInvoiceReport, @@ -134,6 +135,7 @@ function DynamicEditReportFieldPage({route}: DynamicEditReportFieldPageProps) { const menuItems: PopoverMenuItem[] = []; const isReportFieldDeletable = reportField.deletable && reportField?.fieldID !== CONST.REPORT_FIELD_TITLE_FIELD_ID; + const policyFieldList = getReportFieldsWithoutFailedAdds(policy?.fieldList, policy?.errorFields); if (isReportFieldDeletable) { menuItems.push({ @@ -168,7 +170,7 @@ function DynamicEditReportFieldPage({route}: DynamicEditReportFieldPageProps) { fieldValue={fieldValue} isRequired={!isReportFieldDeletable} onSubmit={handleReportFieldChange} - fieldList={policy?.fieldList} + fieldList={policyFieldList} /> )} @@ -198,7 +200,7 @@ function DynamicEditReportFieldPage({route}: DynamicEditReportFieldPageProps) { fieldValue={fieldValue} isRequired={!isReportFieldDeletable} onSubmit={handleReportFieldChange} - fieldList={policy?.fieldList} + fieldList={policyFieldList} disabled /> )} diff --git a/src/pages/workspace/fields/CreateFieldsPage.tsx b/src/pages/workspace/fields/CreateFieldsPage.tsx index 36ae2013625e..8ffcd20eeb60 100644 --- a/src/pages/workspace/fields/CreateFieldsPage.tsx +++ b/src/pages/workspace/fields/CreateFieldsPage.tsx @@ -15,6 +15,7 @@ import {setDraftValues} from '@libs/actions/FormActions'; import {addErrorMessage} from '@libs/ErrorUtils'; import {hasCircularReferences} from '@libs/Formula'; import Navigation from '@libs/Navigation/Navigation'; +import {getReportFieldsWithoutFailedAdds} from '@libs/ReportUtils'; import {isRequiredFulfilled} from '@libs/ValidationUtils'; import {getExistingReportFieldByName, getReportFieldsForTarget, getUnsupportedReportFieldFormulaParts, hasFormulaPartsInInitialValue} from '@libs/WorkspaceReportFieldUtils'; @@ -56,6 +57,8 @@ function CreateFieldsPage({policy, policyID, isInvoiceField, listValuesRoute, ge const reportTypeForTarget = isInvoiceField ? CONST.REPORT.TYPE.INVOICE : CONST.REPORT.TYPE.EXPENSE; const fieldTarget = isInvoiceField ? CONST.REPORT_FIELD_TARGETS.INVOICE : CONST.REPORT_FIELD_TARGETS.EXPENSE; + // Fields that failed to be created are left out, so the user can retry with the same name without dismissing the error first. + const fieldList = getReportFieldsWithoutFailedAdds(policy?.fieldList, policy?.errorFields); const policyReportIDsSelector = (reports: OnyxCollection) => Object.values(reports ?? {}) @@ -83,7 +86,7 @@ function CreateFieldsPage({policy, policyID, isInvoiceField, listValuesRoute, ge }; const getExistingFieldNameError = (name: string) => { - const existingField = getExistingReportFieldByName(policy?.fieldList, name); + const existingField = getExistingReportFieldByName(fieldList, name); if (!existingField) { return undefined; } @@ -117,7 +120,7 @@ function CreateFieldsPage({policy, policyID, isInvoiceField, listValuesRoute, ge if ( (type === CONST.REPORT_FIELD_TYPES.TEXT || type === CONST.REPORT_FIELD_TYPES.FORMULA) && - hasCircularReferences(formInitialValue, name, getReportFieldsForTarget(policy?.fieldList, fieldTarget)) + hasCircularReferences(formInitialValue, name, getReportFieldsForTarget(fieldList, fieldTarget)) ) { errors[INPUT_IDS.INITIAL_VALUE] = translate('workspace.reportFields.circularReferenceError'); } diff --git a/src/pages/workspace/fields/FieldsInitialValuePage.tsx b/src/pages/workspace/fields/FieldsInitialValuePage.tsx index fc41a46e2bf1..2ed8a5b6759c 100644 --- a/src/pages/workspace/fields/FieldsInitialValuePage.tsx +++ b/src/pages/workspace/fields/FieldsInitialValuePage.tsx @@ -12,7 +12,7 @@ import useThemeStyles from '@hooks/useThemeStyles'; import {hasCircularReferences} from '@libs/Formula'; import Navigation from '@libs/Navigation/Navigation'; -import {getReportFieldKey, hasEnabledListValue} from '@libs/ReportUtils'; +import {getReportFieldKey, getReportFieldsWithoutFailedAdds, hasEnabledListValue} from '@libs/ReportUtils'; import {isRequiredFulfilled} from '@libs/ValidationUtils'; import {getReportFieldInitialValue, getReportFieldsForTarget, getUnsupportedReportFieldFormulaParts, isReportFieldTargetValid} from '@libs/WorkspaceReportFieldUtils'; @@ -75,7 +75,7 @@ function FieldsInitialValuePage({policy, policyID, reportFieldID, featureName, e if ( (reportField?.type === CONST.REPORT_FIELD_TYPES.TEXT || reportField?.type === CONST.REPORT_FIELD_TYPES.FORMULA) && - hasCircularReferences(formInitialValue, reportField?.name, getReportFieldsForTarget(policy?.fieldList, expectedTarget)) + hasCircularReferences(formInitialValue, reportField?.name, getReportFieldsForTarget(getReportFieldsWithoutFailedAdds(policy?.fieldList, policy?.errorFields), expectedTarget)) ) { errors[INPUT_IDS.INITIAL_VALUE] = translate('workspace.reportFields.circularReferenceError'); } diff --git a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx index 2d4026d1fbfc..14244a478cec 100644 --- a/src/pages/workspace/fields/WorkspaceFieldsSection.tsx +++ b/src/pages/workspace/fields/WorkspaceFieldsSection.tsx @@ -20,7 +20,7 @@ import {getLatestErrorField} from '@libs/ErrorUtils'; import Navigation from '@libs/Navigation/Navigation'; import {getConnectedIntegration, isControlPolicy, shouldShowSyncError} from '@libs/PolicyUtils'; import type {PolicyFeature} from '@libs/PolicyUtils'; -import {getReportFieldKey} from '@libs/ReportUtils'; +import {getReportFieldKey, isFailedReportFieldAdd} from '@libs/ReportUtils'; import {getReportFieldTypeTranslationKey, isReportFieldImportedFromIntegration} from '@libs/WorkspaceReportFieldUtils'; import {getCurrentAccountingIntegrationName} from '@pages/workspace/accounting/utils'; @@ -50,6 +50,7 @@ type FieldListItem = { fieldID: string; errors: Errors; isDisabled: boolean; + isFailedAdd: boolean; keyForList: string; pendingAction?: PendingAction; rightLabel: string; @@ -149,12 +150,14 @@ function WorkspaceFieldsSection({ pendingAction: field.pendingAction, errors: getLatestErrorField({errorFields: errorFields?.fieldList?.[fieldKey] ?? {}}, 'defaultValue'), isDisabled: field.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, + isFailedAdd: isFailedReportFieldAdd(field, errorFields), rightLabel: Str.recapitalize(translate(getReportFieldTypeTranslationKey(field.type ?? CONST.REPORT_FIELD_TYPES.TEXT))), }; }); const navigateToFieldSettings = (item: FieldListItem) => { - if (!canWrite) { + // A field that failed to be created doesn't exist on the server, so its settings can't be saved. The user dismisses the error instead. + if (!canWrite || item.isFailedAdd) { return; } @@ -174,8 +177,8 @@ function WorkspaceFieldsSection({ onPress={() => navigateToFieldSettings(item)} description={item.text} disabled={item.isDisabled} - shouldShowRightIcon={!item.isDisabled && canWrite} - interactive={!item.isDisabled && canWrite} + shouldShowRightIcon={!item.isDisabled && !item.isFailedAdd && canWrite} + interactive={!item.isDisabled && !item.isFailedAdd && canWrite} rightLabel={item.rightLabel} descriptionTextStyle={[styles.popoverMenuText, styles.textStrong]} /> diff --git a/src/selectors/Policy.ts b/src/selectors/Policy.ts index 5469c1a96a57..21d64f36dddd 100644 --- a/src/selectors/Policy.ts +++ b/src/selectors/Policy.ts @@ -23,6 +23,7 @@ import { isTimeTrackingEnabled, shouldShowPolicy, } from '@libs/PolicyUtils'; +import {getReportFieldsWithoutFailedAdds} from '@libs/ReportUtils'; import type {BillingRestrictionPolicy} from '@libs/SubscriptionUtils'; import {getDefaultAvatarURL} from '@libs/UserAvatarUtils'; @@ -267,12 +268,12 @@ const createHasWorkspaceToSubmitToSelector = getActivePoliciesWithExpenseChat(policies, currentUserLogin).length > 0; /** - * Creates a selector that aggregates all non-formula policy report fields from all policies, + * Creates a selector that aggregates all non-formula policy report fields from all policies, except the ones that failed to be created, * sorted alphabetically by field key using the provided locale compare function */ const createAllPolicyReportFieldsSelector = (policies: OnyxCollection, localeCompare: (a: string, b: string) => number) => { const allPolicyReportFields = Object.values(policies ?? {}).reduce>((acc, policy) => { - Object.assign(acc, policy?.fieldList ?? {}); + Object.assign(acc, getReportFieldsWithoutFailedAdds(policy?.fieldList, policy?.errorFields)); return acc; }, {}); diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 7355274445b8..43223c9ff968 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -146,6 +146,7 @@ import { getReportActionWithSmartscanError, getReportFieldMaps, getReportFieldsByPolicyID, + getReportFieldsWithoutFailedAdds, getReportForHeader, getReportIDFromLink, getReportNotificationPreference, @@ -22991,6 +22992,47 @@ describe('ReportUtils', () => { }); }); + describe('getReportFieldsWithoutFailedAdds', () => { + const baseField = createMock({type: 'text', values: [], disabledOptions: [], defaultValue: ''}); + const savedField = {...baseField, fieldID: 'field_id_SAVED', name: 'Saved'}; + const offlineField = {...baseField, fieldID: 'field_id_OFFLINE', name: 'Offline', pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}; + const failedField = {...baseField, fieldID: 'field_id_FAILED', name: 'Failed', pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}; + const updateFailedField = {...baseField, fieldID: 'field_id_UPDATE_FAILED', name: 'Update failed'}; + const fieldList = { + expensify_field_id_SAVED: savedField, + expensify_field_id_OFFLINE: offlineField, + expensify_field_id_FAILED: failedField, + expensify_field_id_UPDATE_FAILED: updateFailedField, + }; + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- ErrorFields doesn't model the nested fieldList..defaultValue shape the server uses + const policyErrorFields = { + fieldList: { + expensify_field_id_FAILED: {defaultValue: {1: 'Error'}}, + expensify_field_id_UPDATE_FAILED: {defaultValue: {2: 'Error'}}, + }, + } as unknown as ErrorFields; + + it('should leave out only the fields that failed to be created', () => { + // Given a policy with a saved field, a field added offline, a field whose creation failed, and an existing field whose update failed + + // When removing the failed adds + const fields = getReportFieldsWithoutFailedAdds(fieldList, policyErrorFields); + + // Then only the field whose creation failed is left out, because it doesn't exist on the server. The others do, or are still being created. + expect(Object.keys(fields)).toEqual(['expensify_field_id_SAVED', 'expensify_field_id_OFFLINE', 'expensify_field_id_UPDATE_FAILED']); + }); + + it('should keep every field when the policy has no errors', () => { + // Given a policy without report field errors + + // When removing the failed adds + const fields = getReportFieldsWithoutFailedAdds(fieldList, undefined); + + // Then no field is left out, because a pending ADD without an error hasn't failed + expect(fields).toEqual(fieldList); + }); + }); + describe('getReportFieldMaps', () => { it('returns persisted invoice fields when the receiver has no policy field list', () => { const report = createMock({ From bd0bb3c955c7d6011b32507b1fd70cbb6587125c Mon Sep 17 00:00:00 2001 From: "Shubham Agrawal (via MelvinBot)" Date: Wed, 28 Oct 2026 14:53:28 +0000 Subject: [PATCH 6/6] Keep report fields that failed to be created out of deletes and new reports Co-authored-by: Shubham Agrawal --- src/libs/ReportUtils.ts | 4 +- src/libs/actions/Policy/ReportField.ts | 10 ++++- tests/actions/ReportFieldTest.ts | 56 ++++++++++++++++++++++++++ tests/unit/ReportUtilsTest.ts | 30 ++++++++++++++ 4 files changed, 96 insertions(+), 4 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 9073fa8889e2..3e9f9779cb6c 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -8028,7 +8028,7 @@ function buildOptimisticIOUReport( parentReportID: chatReportID, created, lastVisibleActionCreated: created, - fieldList: policy?.fieldList, + fieldList: policy?.fieldList ? getReportFieldsWithoutFailedAdds(policy.fieldList, policy.errorFields) : undefined, parentReportActionID, }; } @@ -8253,7 +8253,7 @@ function buildOptimisticExpenseReport({ } // Otherwise, keep the default format: `${policyName} owes ${formattedTotal}` - expenseReport.fieldList = policy?.fieldList; + expenseReport.fieldList = policy?.fieldList ? getReportFieldsWithoutFailedAdds(policy.fieldList, policy.errorFields) : undefined; return expenseReport; } diff --git a/src/libs/actions/Policy/ReportField.ts b/src/libs/actions/Policy/ReportField.ts index ca3b9869ef23..e9db25c08ae1 100644 --- a/src/libs/actions/Policy/ReportField.ts +++ b/src/libs/actions/Policy/ReportField.ts @@ -343,7 +343,10 @@ function deleteReportFields({policy, reportFieldsToUpdate}: DeleteReportFieldsPa return; } - const updatedReportFields = Object.fromEntries(Object.entries(allReportFields).filter(([key]) => !resolvedReportFieldKeys.includes(key))); + // Fields that failed to be created don't exist on the server, so they're left out of the remaining fields we send + const updatedReportFields = Object.fromEntries( + Object.entries(ReportUtils.getReportFieldsWithoutFailedAdds(allReportFields, policy.errorFields)).filter(([key]) => !resolvedReportFieldKeys.includes(key)), + ); const optimisticReportFields = resolvedReportFieldKeys.reduce>>>((acc, reportFieldKey) => { acc[reportFieldKey] = {pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE}; return acc; @@ -354,6 +357,9 @@ function deleteReportFields({policy, reportFieldsToUpdate}: DeleteReportFieldsPa return acc; }, {}); + // Only the deleted fields' errors are cleared, so other fields keep theirs (for example, a field that failed to be created) + const successErrorFields = {fieldList: successReportFields}; + const failureReportFields = resolvedReportFieldKeys.reduce>>>((acc, reportFieldKey) => { acc[reportFieldKey] = {pendingAction: null}; return acc; @@ -375,7 +381,7 @@ function deleteReportFields({policy, reportFieldsToUpdate}: DeleteReportFieldsPa key: `${ONYXKEYS.COLLECTION.POLICY}${policy?.id}`, value: { fieldList: successReportFields, - errorFields: null, + errorFields: successErrorFields, }, }, ], diff --git a/tests/actions/ReportFieldTest.ts b/tests/actions/ReportFieldTest.ts index 98dd7dae3778..398a24089823 100644 --- a/tests/actions/ReportFieldTest.ts +++ b/tests/actions/ReportFieldTest.ts @@ -1,3 +1,4 @@ +import {WRITE_COMMANDS} from '@libs/API/types'; import DateUtils from '@libs/DateUtils'; import {generateFieldID} from '@libs/WorkspaceReportFieldUtils'; @@ -459,6 +460,61 @@ describe('actions/ReportField', () => { [reportFieldKey]: fakeReportField, }); }); + + it('leaves a report field that failed to be created out of the request and keeps its error', async () => { + // Given a workspace with a field to delete and a field that failed to be created, which is still pending ADD with its error + const reportFieldToDelete: PolicyReportField = { + name: 'Field To Delete', + type: CONST.REPORT_FIELD_TYPES.TEXT, + defaultValue: 'Default Value', + values: [], + disabledOptions: [], + fieldID: generateFieldID('Field To Delete'), + orderWeight: 1, + deletable: true, + keys: [], + externalIDs: [], + isTax: false, + }; + const failedReportField: OnyxValueWithOfflineFeedback = { + ...reportFieldToDelete, + name: 'Failed Field', + defaultValue: '{field:DoesNotExist}', + fieldID: generateFieldID('Failed Field'), + orderWeight: 2, + pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, + }; + const reportFieldToDeleteKey = ReportUtils.getReportFieldKey(reportFieldToDelete.fieldID); + const failedReportFieldKey = ReportUtils.getReportFieldKey(failedReportField.fieldID); + const failedReportFieldErrors = {defaultValue: {[DateUtils.getMicroseconds()]: 'Formula field {field:DoesNotExist} not recognized'}}; + const fakePolicy: PolicyType = { + ...createRandomPolicy(0), + fieldList: { + [reportFieldToDeleteKey]: reportFieldToDelete, + [failedReportFieldKey]: failedReportField, + }, + // The server returns report field errors at errorFields.fieldList..defaultValue, which ErrorFields doesn't model + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + errorFields: {fieldList: {[failedReportFieldKey]: failedReportFieldErrors}} as unknown as ErrorFields, + }; + await Onyx.set(`${ONYXKEYS.COLLECTION.POLICY}${fakePolicy.id}`, fakePolicy); + + // When the user deletes the other field before dismissing the error + ReportField.deleteReportFields({policy: fakePolicy, reportFieldsToUpdate: [reportFieldToDeleteKey]}); + await waitForBatchedUpdates(); + + // Then the failed field isn't sent to the server as a remaining field, because it was never created there + const body = TestHelper.getFetchMockCalls(WRITE_COMMANDS.DELETE_POLICY_REPORT_FIELD).at(0)?.[1]?.body; + const reportFields = body instanceof FormData ? body.get('reportFields') : null; + expect(reportFields).toBe(JSON.stringify([])); + + // Then the failed field keeps its error, so it's still treated as failed until the error is dismissed + const policy = await connectToFetchPolicy(fakePolicy.id); + expect(policy?.fieldList?.[reportFieldToDeleteKey]).toBeUndefined(); + expect(policy?.fieldList?.[failedReportFieldKey]?.pendingAction).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD); + expect(policy?.errorFields?.fieldList?.[failedReportFieldKey]).toEqual(failedReportFieldErrors); + expect(ReportUtils.isFailedReportFieldAdd(policy?.fieldList?.[failedReportFieldKey], policy?.errorFields)).toBe(true); + }); }); describe('updateReportFieldInitialValue', () => { diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 43223c9ff968..55c902f79dfe 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -19506,6 +19506,36 @@ describe('ReportUtils', () => { expect(expenseReport.reportName).toBe(`${fakePolicy.name} owes ${convertToDisplayString(-total, currency)}`); }); + it('should leave report fields that failed to be created out of the report field list', async () => { + // Given a policy with a saved field and a field that failed to be created, which is still pending ADD with its error + const policyID = '3'; + const baseField = createMock({type: CONST.REPORT_FIELD_TYPES.TEXT, values: [], disabledOptions: [], defaultValue: ''}); + const savedField = {...baseField, fieldID: 'field_id_SAVED', name: 'Saved'}; + const failedField = {...baseField, fieldID: 'field_id_FAILED', name: 'Failed', pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}; + const fakePolicy: Policy = { + ...createRandomPolicy(Number(policyID)), + fieldList: {expensify_field_id_SAVED: savedField, expensify_field_id_FAILED: failedField}, + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- ErrorFields doesn't model the nested fieldList..defaultValue shape the server uses + errorFields: {fieldList: {expensify_field_id_FAILED: {defaultValue: {1: 'Error'}}}} as unknown as ErrorFields, + }; + await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, fakePolicy); + + // When an expense report is built before the error is dismissed + const expenseReport = buildOptimisticExpenseReport({ + rules: undefined, + chatReportID: undefined, + policyID, + payeeAccountID: 1, + total: 100, + currency: CONST.CURRENCY.USD, + isASAPSubmitBetaEnabled: true, + getCurrencyDecimals: getCurrencyDecimalsLocal, + }); + + // Then only the saved field is copied to the report, because the failed field doesn't exist on the server + expect(Object.keys(expenseReport.fieldList ?? {})).toEqual(['expensify_field_id_SAVED']); + }); + it('should keep the English workspace fallback in the report name when another locale is loaded', async () => { // Given a chat report that cannot be resolved, so the report name falls back to the "unavailable workspace" copy const chatReportID = '9999';