Auto-create imported merchant rule categories that don't exist on the policy#96408
Auto-create imported merchant rule categories that don't exist on the policy#96408MelvinBot wants to merge 2 commits into
Conversation
… policy Co-authored-by: Nicolás Bonet <NicolasBonet@users.noreply.github.com>
Co-authored-by: Nicolás Bonet <NicolasBonet@users.noreply.github.com>
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.
|
|
I checked all the checks on this PR and there's no code fix to make — every substantive check is passing. The only failing check is Full check breakdown
Since there's no failing check caused by the code, I haven't pushed anything. If the perf tests end up failing, re-ping me and I'll investigate. |
|
Re-checked now that the perf tests finished. There's still no code fix to make — both remaining failures are unrelated to this PR's code. 1. This is a GitHub Actions artifact-storage hiccup. On top of that, 2. Every substantive check passes — Notes
|
|
I looked at both failing checks — neither is caused by this PR's code, so there's nothing to fix in the diff. 1. The job only failed afterward on the artifact upload step: That's a GitHub artifact-service hiccup, not a test/code issue. On top of that, 2. Everything else (all 8 |
|
Still no code fix to push — both failing checks are infra/human, not caused by this PR. 1. This is a GitHub Actions/blob-storage upload flake against the 2. Every other check passes (typecheck, all 8 test jobs, ESLint, Storybook, React Compiler, Bun tests, builds, |
|
@NicolasBonet Could you assign me to review this? |
Is this something you can review within the next hour? Would really like to unblock the deployment soon. |
Yes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9a9bda7c9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .filter((category): category is string => !!category && !policyCategories?.[category]), | ||
| ), | ||
| ]; | ||
| createPolicyCategories(policyID, missingCategories); |
There was a problem hiding this comment.
Serialize category creation before importing rules
When the spreadsheet references a category that is not already on the policy, this enqueues CREATE_WORKSPACE_CATEGORIES and then immediately continues to API.makeRequestWithSideEffects for the merchant-rule import. API.write only puts the create request on the sequential write queue, while side-effect requests run right away, so the import can reach the backend before the category exists server-side; in that case the imported rule can still lose or reject the new category despite the optimistic Onyx update. Please make the import wait for the category write to finish, or have the server-side import create the missing categories atomically.
Useful? React with 👍 / 👎.
|
The perf tests have now finished, and there's still no code fix to make — both remaining failures are non-code issues. 1.
That's the 2. Everything else is green (typecheck, all test jobs, ESLint, Storybook, |
|
@NicolasBonet, do you think the current approach violates our 1:1:1 rules? Also, we're calling What happens if the second request fails? We won't be able to roll back the categories created by the first request, which could leave the data in an inconsistent state. |
|
@NicolasBonet I think everything should be handled on the BE side via |
|
Closing this PR — two reasons:
We'll settle the desired behavior in #96389 first (e.g. skip/warn at import time, surface "these categories will be created" in the import preview, or auto-create if that's what product actually wants) before reintroducing a fix. |
Explanation of Change
When importing merchant rules from a spreadsheet, an
Updated categorycell was passed straight through as free text with no check against the workspace's category list, and no category was ever created for it. A rule only applies its category to a matching expense if that category actually exists on the policy, so importing a rule whose category didn't exist left the rule showing a category that never got applied to expenses (deploy blocker from #95105).This change makes the import auto-create any imported category that doesn't already exist on the workspace, so the imported category is added to the category list instead of being silently dropped — matching the reporter's expected behavior.
createPolicyCategories(policyID, categoryNames)helper inPolicy/Category.tsthat reuses the existingbuildOptimisticPolicyCategoriesoptimistic data and theCREATE_WORKSPACE_CATEGORIEScommand to create several enabled categories at once.importMerchantRulesSpreadsheetnow receives the policy's existing categories, computes the distinct imported categories that aren't already on the policy, and creates them before the rules import.ImportedMerchantRulesPagereadsPOLICY_CATEGORIESfor the policy and passes them to the action.This is an App-only fix that relies on the already-deployed
CreateWorkspaceCategoriesbackend command, so no new backend work is required for the category to exist when a matching expense is later created.AI tests run by MelvinBot (human co-author must still complete Tests/QA)
npm run typecheck-tsgo— passednpm run lint-changed— passednpm run react-compiler-compliance-check check src/pages/workspace/rules/MerchantRules/ImportedMerchantRulesPage.tsx— passedNot run in this environment: the full
npm testsuite andnpm run storybook --smoke-test(there is no existing unit test for this action). Please run these before marking ready.Fixed Issues
$ #96389
PROPOSAL: #96389 (comment)
Tests
merchant-rules-categories.csv
Team Lunch,Airfare Deluxe,Travel).Team LunchandAirfare Deluxenow exist and are enabled.Travelwas not duplicated in the category list.Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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
MacOS: Chrome / Safari