Repository navigation
Conversation
… them Co-authored-by: Francois Laithier <[email protected]>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
The failing Details
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🤖 Web test steps verification results: ✅ pass Bulk "Duplicate expense" from an expense report now keeps the new copy on the source report (report 1), matching the single-expense duplicate path. The newer report (report 2) was unaffected in both the single and bulk duplicate flows.
|
|
I merged Details
|
|
🤖 Android test steps verification results: ❌ fail Testing was blocked. The Android app stopped responding to all input while the second test expense was being added, before any duplicate-expense step ran, so this run neither confirms nor disproves the fix.
|
|
@thelullabyy Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@francoisl Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9060b6cf6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (currentTargetReport && !isTargetExpenseReport) { | ||
| currentTargetReport = {...currentTargetReport, iouReportID: currentOptimisticIOUReportID}; |
There was a problem hiding this comment.
When the source policy uses instant submit plus submit-and-close and the copies are all non-reimbursable (or reimbursement is disabled), the first copy closes the source report because policyWillSplitReport prevents deferral. This branch then clears optimisticIOUReport but, for an expense-report target, leaves currentTargetReport pointing at that now-closed report; every later request therefore still sends the source report as its explicit moneyRequestReportID instead of creating a fresh report. Either defer submission while targeting an explicit source report or switch subsequent iterations to its parent chat.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Francois' agent)
Good catch, fixed in 564c46c. When the target is an expense report, the copies no longer split: they all land on the source report, and only the last one lets the server auto-submit. I didn't switch to the parent chat, because that would scatter the copies across new reports. I added a DuplicateTest case for this setup.
| const duplicateTargetPolicy = canDuplicateIntoSourceReport ? sourcePolicy : defaultExpensePolicy; | ||
| const duplicateTargetReport = canDuplicateIntoSourceReport ? sourceReport : activePolicyExpenseChat; |
There was a problem hiding this comment.
When bulk-duplicating from a non-default workspace whose billing is restricted while the default workspace remains usable, this now selects the restricted source policy, but the report-selection callers invoke the handler without the shouldRestrictUserBillableActions check that the single-expense duplicate applies to its target policy. The action consequently submits writes to the restricted workspace instead of navigating to the restricted-action screen, producing failed optimistic copies; the billing guard needs to evaluate this newly selected target policy before duplication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Francois' agent)
Agreed, fixed in 564c46c. useBulkDuplicateAction now checks billing restrictions on the chosen target policy and opens the restricted action page instead of duplicating, like single-expense duplicate. I added a test for it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Bulk duplicating into an open expense report on a workspace with instant submit and no approvals split the copies as if each one needed its own report. None of them deferred auto-submit, so the server closed the source report after the first copy. The later copies still targeted it, and the optimistic total left out the first copy. An expense report target now never splits: every copy lands on it, and only the last one lets the server auto-submit. Bulk duplicate from a report view never checked billing restrictions. Now that the target can be the source workspace, a restricted source workspace would produce failed copies. The handler now opens the restricted action page for the chosen target policy, like the single-expense duplicate does. Also drop two prop comments that only restated the prop names.
|
@marcochavezf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
The change makes sense to me from a product perspective.
|
@thelullabyy can you review when you have a chance please? Melvin opened the PR, but I made manual commits to address Codex's comments and fix conflicts. |
thelullabyy
left a comment
thelullabyy
left a comment
There was a problem hiding this comment.
Thanks, the approach matches what #103072 did for single-expense duplicate, and reusing canDuplicateExpenseIntoSourceReport keeps the two entry points consistent. A few questions and comments are inline. One more that isn't on a changed line:
Per diem stays hidden on non-default workspace reports. shouldShowBulkDuplicateOption (src/hooks/useSearchBulkActions.ts) still hides bulk duplicate for a per diem expense unless its policy is the default workspace. Single duplicate now checks duplicateTargetPolicy?.id !== policy?.id, so on Report 3 in Workspace B (step 7 of the tests), single duplicate of a per diem expense is allowed but bulk duplicate of the same expense is hidden. Should the visibility check also account for the source report target, or should that be a follow-up?
| currentReportPreviewActionID = NumberUtils.rand64(); | ||
| reportWasSplit = true; | ||
| if (currentTargetReport) { | ||
| if (currentTargetReport && !isTargetExpenseReport) { |
There was a problem hiding this comment.
With an expense report target, what happens if this split branch still runs? policyWillSplitReport is now false, but !canAddTransaction(optimisticIOUReport, rules) can still return true mid-loop (for example a rule that limits the report). Then optimisticIOUReport is reset to undefined, and the next iteration passes the source expense report again as targetReport. The builder then resolves it through moneyRequestReportID, which reads allReports. Onyx callbacks are deferred, so that copy is stale and doesn't include the previous iteration's total. That is the race the existingIOUReport comment above warns about.
For an expense report target, should we either keep passing optimisticIOUReport, or fall back to the default workspace chat for the remaining expenses? Either way, a test that covers a mid-loop canAddTransaction failure with an expense report target would help.
| const participantsPolicyTags = useParticipantsPolicyTags(participants); | ||
|
|
||
| const handleDuplicate = () => { | ||
| if (restrictedActionPolicyID) { |
There was a problem hiding this comment.
This restricted-action redirect is new for every bulk duplicate path, including the Search page, where duplicateTargetPolicy is the default workspace. The description says Search-page bulk duplicate is unchanged. The change looks right because single duplicate does the same check, but could you mention it in the description and QA steps, and add a test for it?
| const duplicateTargetPolicy = canDuplicateIntoSourceReport ? sourcePolicy : defaultExpensePolicy; | ||
| const duplicateTargetReport = canDuplicateIntoSourceReport ? sourceReport : activePolicyExpenseChat; | ||
|
|
||
| const [targetPolicyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${duplicateTargetPolicy?.id}`); |
There was a problem hiding this comment.
NAB: the single-duplicate code wraps the ID with getNonEmptyStringOnyxID(duplicateTargetPolicy?.id), so an undefined policy doesn't subscribe to policyCategories_undefined. Same for the tags key on the next line.
A copy can push a submitted report's total past an approval rule's limit, so the report stops accepting expenses partway through a bulk duplicate. The loop dropped the report's latest optimistic state at that point, so the builder reread an outdated copy from Onyx and still added the next copy to the same report with the wrong total. The loop now keeps that state for an expense report target, and the builder opens a new report in the same workspace chat. Also add a test for the restricted-action redirect when the copies fall back to the default workspace, and wrap the target policy ID with getNonEmptyStringOnyxID like single-expense duplicate does.
|
Updated the PR.
Let's keep that for a follow-up, the PR doesn't change that check. |
Main now reads the default workspace chat with a useOnyx selector (policyExpenseChatSelector) instead of getPolicyExpenseChat. Keep that read in useBulkDuplicateAction and keep this branch's choice of duplicate target, matching how main updated useExpenseActions.
|
Fixed the conflict, let's try to merge this before it conflicts again 🙏 |
The pre-commit hook rebuilt these two compiled actions during the last merge of main, using an outdated expensify-common from this worktree's node_modules. The output no longer matched a fresh build, which failed the Verify Javascript Action Builds check. Take main's versions again.
Brings in the main fix for the Oxlint failure: the unnecessary type assertions added in tests/unit/ReportActionsUtilsTest.ts and tests/unit/ReportUtilsTest.ts were removed in #105195.
Explanation of Change
Bulk duplicating expenses from an expense report always targeted the default workspace chat, so the copies landed on whatever report that chat's
iouReportIDpointed at, which is usually the newest report. The single-expense duplicate was already fixed to stay on the source report in #103072, but that PR left bulk duplicate out of scope, so the two entry points behaved differently.This change brings bulk duplicate in line with the single-expense duplicate:
MoneyReportHeaderSelectionDropdownandSelectionToolbarnow pass the report being viewed, its chat report, and its policy throughBulkDuplicateHandlerintouseBulkDuplicateAction.useBulkDuplicateActiontargets that source report and its policy (categories, tags, and participants included) when every selected expense lives on it andcanDuplicateExpenseIntoSourceReportreturns true. Otherwise it keeps the existing routing to the default workspace chat. Search-page bulk duplicate passes no source report, so it is unchanged.bulkDuplicateExpensesno longer patchesiouReportIDonto the target when it is an expense report. That patching assumes the target is a chat.Unit tests cover the new target selection in
useBulkDuplicateAction(source report used, and fallback when a selected expense is on another report) andbulkDuplicateExpenseswith an open expense report as the target.AI tests run locally:
npm run linton changed files,npm run typecheck,npm run fmt(oxfmt),npm run react-compiler-compliance-check checkon changed components, cspell (no issues in changed files), andDuplicateTest,useSearchBulkActionsDuplicateTest, andReportUtilsTest(1,684 tests passed). An automated web UI run of the issue's steps could not complete because the test session's page went blank during onboarding, so the UI flow still needs a manual check.Fixed Issues
$ #103866
PROPOSAL: #103866 (comment)
Tests
Setup
Steps
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari