Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@ import createDynamicRoute from '@libs/Navigation/helpers/dynamicRoutesUtils/crea
import Navigation from '@libs/Navigation/Navigation';
import Parser from '@libs/Parser';
import {getReportName} from '@libs/ReportNameUtils';
import {generateReportID, getOutstandingReportsForUser, isMoneyRequestReport, isReportOutstanding, sortOutstandingReportsBySelected} from '@libs/ReportUtils';
import {
canAddTransaction,
generateReportID,
getNewestOutstandingReportForUser,
getOutstandingReportsForUser,
isMoneyRequestReport,
isReportOutstanding,
sortOutstandingReportsBySelected,
} from '@libs/ReportUtils';

import CONST from '@src/CONST';
import type {IOUAction, IOUType} from '@src/CONST';
Expand Down Expand Up @@ -69,6 +77,7 @@ function ReportField({selectedParticipants, iouType, reportID, reportActionID, a
const [iouReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${iouReportIDFromMain}`);

const isUnreported = transactionReportID === CONST.REPORT.UNREPORTED_REPORT_ID;
const isCreatingExpense = action === CONST.IOU.ACTION.CREATE;

/**
* We need to check if the transaction report exists first in order to prevent the outstanding reports from being used.
Expand All @@ -81,7 +90,11 @@ function ReportField({selectedParticipants, iouType, reportID, reportActionID, a
(report1, report2) => sortOutstandingReportsBySelected(report1, report2, undefined, localeCompare),
);

const outstandingReportID = isPolicyExpenseChat ? (iouReportIDFromMain ?? availableOutstandingReports.at(0)?.reportID) : reportID;
// Match the builder's automatic destination before checking eligibility, rather than falling back to a report it would not use.
const defaultOutstandingReport = isCreatingExpense
? getNewestOutstandingReportForUser(policyID, ownerAccountID, rules, reportNameValuePairs, outstandingReportsForPolicy ?? {})
: availableOutstandingReports.at(0);
const outstandingReportID = isPolicyExpenseChat ? (iouReportIDFromMain ?? defaultOutstandingReport?.reportID) : reportID;

const [selectedReportID, selectedReport] = (() => {
const reportIDToUse = shouldUseTransactionReport ? transactionReportID : outstandingReportID;
Expand All @@ -100,6 +113,9 @@ function ReportField({selectedParticipants, iouType, reportID, reportActionID, a
} else {
reportToUse = availableOutstandingReports.find((r) => r?.reportID === reportIDToUse);
}
if (isCreatingExpense && reportToUse && !canAddTransaction(reportToUse, rules)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MelvinBot main reverted #105939 in 9eb336b, so the reason for my earlier comment about dropping the threshold is gone. The issue expects the Report field to be disabled when every report holds only non-reimbursable expenses. Right now it stays enabled, because line 135 checks editability against every outstanding report, including the ones the create picker hides. Example: with 2 retracted non-reimbursable-only reports, the field shows "New report" and can be pressed, and the list has only "Create report". In create mode, please check editability against the reports that pass canAddTransaction(report, rules). A single eligible report should still keep the field editable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 3848247. Creation now passes only reports accepted by canAddTransaction into the existing editability selector. Zero eligible reports disables New report; a single eligible alternative remains interactive. No new count threshold was added, and other actions retain their existing report list. All four report suites pass (38 tests).


view run

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Already addressed in 3848247:

  • Creation editability considers only canAddTransaction(report, rules)-eligible reports.
  • No eligible reports → disabled New report.
  • One eligible alternative → enabled field, without a > 1 override.

Regression tests cover both workspace and global creation. All four report suites passed: 38 tests.


view run

return [generateReportID(), undefined] as const;
}
return [reportIDToUse, reportToUse ?? undefined] as const;
})();

Expand All @@ -114,12 +130,17 @@ function ReportField({selectedParticipants, iouType, reportID, reportActionID, a
})();

const outstandingReports = useOutstandingReports(undefined, isFromGlobalCreate && !isPerDiemRequest ? undefined : policyID, ownerAccountID, false, ownerPersonalDetail?.login);
// The list offers a new report and, for a new expense, no report at all, so there is always a choice to make.
// Editing an expense that is already on a report can hide both (someone else's expense, edited by a non-admin),
// so there the row needs another report to choose from. Creating from inside an expense report locks the row,
// since the expense can only go on that report.
const eligibleOutstandingReports = isCreatingExpense ? outstandingReports.filter((report) => canAddTransaction(report, rules)) : outstandingReports;
// Edits on a report can hide the new-report option; other non-creation flows always offer a choice.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MelvinBot This comment uses a semicolon to join two clauses (CONSISTENCY-15). Restoring main's comment fixes it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restored main's original comment alongside its editability logic in 501323b.


view run

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Already addressed in 501323b: main’s original comment is restored, and the semicolon-containing replacement is gone.


view run

const mayHideListOptions = action === CONST.IOU.ACTION.EDIT && !isUnreported;
const shouldReportBeEditable = (!mayHideListOptions || shouldReportFieldBeEditable(outstandingReports, selectedReportID, isUnreported)) && !isMoneyRequestReport(reportID);
let hasSelectableReports = (!isCreatingExpense && !mayHideListOptions) || shouldReportFieldBeEditable(eligibleOutstandingReports, selectedReportID, isUnreported);
if (isCreatingExpense && !isUnreported) {
// Keep a single eligible alternative read-only when the automatic destination is "New report".
hasSelectableReports = eligibleOutstandingReports.length > 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MelvinBot This brings back the > 1 threshold for create, and that undoes #105939. #105939 made the field always editable on create, because the picker always offers "Create report" and "None". With this change, a workspace with 0 or 1 open reports shows a read-only field, so the user can't pick "None" or "Create report". Please remove eligibleOutstandingReports, hasSelectableReports and the isCreatingExpense block, and restore main's shouldReportBeEditable line and its comment. The default-value check and the picker filter already fix the bug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 501323b. Restored main's editability logic and comment, removing the report-count threshold. The default-value check and creation-only picker filter remain intact.


view run

}
// When creating an expense in an individual report, the report field becomes read-only
// since the destination is already determined and there's no need to show a selectable list.
const shouldReportBeEditable = hasSelectableReports && !isMoneyRequestReport(reportID);
// Editing a saved expense's report and moving a per diem expense both navigate on from the list (to the report, or to
// the destination step when the workspace changes), so those keep the full page.
const shouldOpenInDropdown = !!transactionID && shouldReportBeEditable && action !== CONST.IOU.ACTION.EDIT && !isPerDiemRequest;
Expand Down
6 changes: 6 additions & 0 deletions src/pages/iou/request/step/IOURequestEditReportCommon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ type IOURequestEditReportCommonProps = {
selectReport: (item: TransactionGroupListItem, report?: OnyxEntry<Report>) => void;
removeFromReport?: () => void;
isEditing?: boolean;
/** Uses creation eligibility instead of the admin/manager exceptions for moving saved expenses. Defaults to false. */
isCreatingExpense?: boolean;
Comment on lines +76 to +77

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Compose creation eligibility instead of adding a mode flag

When this shared picker is used for expense creation, correctness now depends on every caller remembering the optional isCreatingExpense flag; omitting it silently restores the admin/manager move exceptions and can expose destinations that the creation save path rejects. This adds behavioral boolean configuration to an already broad component API, contrary to CLEAN-REACT-PATTERNS-1; split creation and move eligibility into composed wrappers/blocks so each flow selects the correct behavior structurally rather than through an optional unsafe default.

AGENTS.md reference: AGENTS.md:L7-L10

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@MelvinBot investigate and fix if required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Investigated; no code change is required for this finding.

Re-ran the relevant suite: 17 tests passed, including real-hook CREATE/EDIT eligibility coverage.


view run

isUnreported?: boolean;
shouldShowNotFoundPage?: boolean;
createReport?: () => void;
Expand Down Expand Up @@ -123,6 +125,7 @@ function IOURequestEditReportCommon({
transactionPolicyID,
removeFromReport,
isEditing = false,
isCreatingExpense = false,
isUnreported,
shouldShowNotFoundPage: shouldShowNotFoundPageFromProps,
createReport,
Expand Down Expand Up @@ -241,6 +244,9 @@ function IOURequestEditReportCommon({
if (isPerDiemRequest && !canSubmitPerDiemExpenseFromWorkspace(policy)) {
return false;
}
if (isCreatingExpense) {
return canAddTransaction(report, rules);
}
if (canAddTransaction(report, rules, undefined, true)) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ type ReportPickerListProps = Pick<
| 'transactionPolicyID'
| 'removeFromReport'
| 'isEditing'
| 'isCreatingExpense'
| 'isUnreported'
| 'isPerDiemRequest'
| 'isTimeRequest'
Expand Down Expand Up @@ -321,6 +322,7 @@ function useReportPickerActions({
transactionPolicyID: targetExpensePolicyID,
removeFromReport,
isEditing,
isCreatingExpense: isCreateReport,
isUnreported,
isPerDiemRequest: transaction ? isPerDiemTransaction : false,
isTimeRequest: isTimeTransaction,
Expand Down
67 changes: 67 additions & 0 deletions tests/ui/IOURequestEditReportCommonTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ const renderIOURequestEditReportCommon = ({
isOdometerDistanceRequest = false,
isDistanceRequest = false,
isPerDiemRequest = false,
isCreatingExpense = false,
isEditing = false,
selectReport = jest.fn(),
createReport,
selectNoReport,
Expand All @@ -58,6 +60,8 @@ const renderIOURequestEditReportCommon = ({
isOdometerDistanceRequest?: boolean;
isDistanceRequest?: boolean;
isPerDiemRequest?: boolean;
isCreatingExpense?: boolean;
isEditing?: boolean;
selectReport?: jest.Mock;
createReport?: jest.Mock;
selectNoReport?: jest.Mock;
Expand All @@ -83,6 +87,8 @@ const renderIOURequestEditReportCommon = ({
onNavigateAway={onNavigateAway}
backTo=""
isPerDiemRequest={isPerDiemRequest}
isCreatingExpense={isCreatingExpense}
isEditing={isEditing}
/>
</ComposeProviders>
</NavigationContainer>,
Expand Down Expand Up @@ -155,6 +161,67 @@ describe('IOURequestEditReportCommon', () => {
expect(dotIndicators).toHaveLength(0);
});

it.each([
{isCreatingExpense: true, isEditing: false, shouldShowReport: false},
{isCreatingExpense: false, isEditing: true, shouldShowReport: true},
{isCreatingExpense: false, isEditing: false, shouldShowReport: true},
])('preserves move permissions but uses save eligibility for creation: %j', async ({isCreatingExpense, isEditing, shouldShowReport}) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These tests set isCreatingExpense directly, so nothing checks that useReportPickerActions maps action === CREATE to it. Please add one test that renders through the hook, or through ReportFieldDropdown with action: CREATE, and checks that an ineligible report is hidden.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added coverage through the real shared hook and rendered picker in 501323b. CREATE hides the ineligible report while retaining an eligible positive control; EDIT preserves the admin bypass. Neither case supplies isCreatingExpense directly, and both pass.


view run

// Given an admin's retracted report containing only non-reimbursable expenses
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${FAKE_POLICY_ID}`, {
role: CONST.POLICY.ROLE.ADMIN,
autoReporting: true,
autoReportingFrequency: CONST.POLICY.AUTO_REPORTING_FREQUENCIES.INSTANT,
approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL,
reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO,
pendingAction: undefined,
});
await Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${FAKE_TRANSACTION_ID}`, {
transactionID: FAKE_TRANSACTION_ID,
reportID: FAKE_REPORT_ID,
amount: -100,
currency: 'USD',
created: '2026-01-01',
merchant: 'Merchant',
reimbursable: false,
});
});
await waitForBatchedUpdatesWithAct();

// When the same picker opens for creation, editing, or a caller that does not set isEditing
renderIOURequestEditReportCommon({selectedReportID: FAKE_REPORT_ID, selectedPolicyID: FAKE_POLICY_ID, isCreatingExpense, isEditing});
await waitForBatchedUpdatesWithAct();

// Then only creation hides the rejected destination, preserving all existing move callers
expect(!!screen.queryByText('Expense Report')).toBe(shouldShowReport);
});

it('does not apply moving eligibility to submitted reports during creation', async () => {
// Given an instant-submit workspace with payments off and a submitted reimbursable report
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${FAKE_POLICY_ID}`, {
role: CONST.POLICY.ROLE.ADMIN,
autoReporting: true,
autoReportingFrequency: CONST.POLICY.AUTO_REPORTING_FREQUENCIES.INSTANT,
approvalMode: CONST.POLICY.APPROVAL_MODE.OPTIONAL,
reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO,
pendingAction: undefined,
});
await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${FAKE_REPORT_ID}`, {
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
statusNum: CONST.REPORT.STATUS_NUM.SUBMITTED,
});
});
await waitForBatchedUpdatesWithAct();

// When the create-flow picker lists destinations
renderIOURequestEditReportCommon({selectedReportID: FAKE_REPORT_ID, selectedPolicyID: FAKE_POLICY_ID, isCreatingExpense: true});
await waitForBatchedUpdatesWithAct();

// Then a report accepted only for moving transactions is not offered for creation
expect(screen.queryByText('Expense Report')).toBeNull();
});

const setUpCommuterExclusionTest = async () => {
const currentReport: Report = {
reportID: 'currentReport',
Expand Down
26 changes: 17 additions & 9 deletions tests/ui/MoneyRequestReportFooter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,14 @@ describe('MoneyRequestConfirmationListFooter', () => {
return waitForBatchedUpdatesWithAct();
});

beforeEach(async () => {
// Creation eligibility requires the signed-in user to be the report's submitter.
await act(async () => {
await Onyx.set(ONYXKEYS.SESSION, {accountID: FAKE_ACCOUNT_ID, email: 'reportfooter@example.com'});
});
await waitForBatchedUpdatesWithAct();
});

afterEach(async () => {
await act(async () => {
await Onyx.clear();
Expand Down Expand Up @@ -199,7 +207,7 @@ describe('MoneyRequestConfirmationListFooter', () => {
expect(reportItem.props.accessibilityState).toEqual(expect.objectContaining({disabled: false}));
});

it('should allow editing report field when there is only 1 outstanding report and creating from policy chat', async () => {
it('should disable report field when there is only 1 outstanding report and creating from policy chat', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#105939 added these tests for the behavior we want, so they weren't stale. Please revert these 3 tests and the beforeEach session setup to main, unless the other tests still need the session.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restored all three tests and removed the added session setup in 501323b. The footer test suite now exactly matches the merge base, and all five tests pass without that setup.


view run

// Given an expense created in a workspace chat whose workspace has one outstanding report, the one it goes on
const mockTransactionReport: Transaction = {
...mockTransaction,
Expand Down Expand Up @@ -228,12 +236,12 @@ describe('MoneyRequestConfirmationListFooter', () => {

await waitForBatchedUpdatesWithAct();

// Then the field can be changed, since the list always offers a new report next to that one
// Then creation keeps the field disabled because there is only one eligible report
const reportItem = screen.getByTestId('menu-item-Report');
expect(reportItem.props.accessibilityState).toEqual(expect.objectContaining({disabled: false}));
expect(reportItem.props.accessibilityState).toEqual(expect.objectContaining({disabled: true}));
});

it('should allow editing report field when there are no reports available', async () => {
it('should disable report field when there are no reports available', async () => {
// Given an expense being created on a workspace with no outstanding report
const mockTransactionReport: Transaction = {
...mockTransaction,
Expand All @@ -253,12 +261,12 @@ describe('MoneyRequestConfirmationListFooter', () => {

await waitForBatchedUpdatesWithAct();

// Then the field can be changed, since the list offers a new report or no report at all
// Then creation keeps the field disabled because there are no eligible reports
const reportItem = screen.getByTestId('menu-item-Report');
expect(reportItem.props.accessibilityState).toEqual(expect.objectContaining({disabled: false}));
expect(reportItem.props.accessibilityState).toEqual(expect.objectContaining({disabled: true}));
});

it('should allow editing report field when transaction has reportID and creating from FAB with only 1 outstanding report', async () => {
it('should disable report field when transaction has reportID and creating from FAB with only 1 outstanding report', async () => {
// Given an expense created from the global create button that already sits on the only outstanding report
const mockTransactionReport: Transaction = {
...mockTransaction,
Expand Down Expand Up @@ -287,9 +295,9 @@ describe('MoneyRequestConfirmationListFooter', () => {

await waitForBatchedUpdatesWithAct();

// Then the field can be changed, since the list always offers a new report next to that one
// Then creation keeps the field disabled because a reported expense requires more than one eligible report
const reportItem = screen.getByTestId('menu-item-Report');
expect(reportItem.props.accessibilityState).toEqual(expect.objectContaining({disabled: false}));
expect(reportItem.props.accessibilityState).toEqual(expect.objectContaining({disabled: true}));
});

it('should allow editing report field when transaction is unReported and creating from FAB with only 1 outstanding report', async () => {
Expand Down
Loading
Loading