[CP Staging] Fix imported merchant rule tag corruption for X: Y (multi-level) tag format#96407
Conversation
Co-authored-by: Nicolás Bonet <NicolasBonet@users.noreply.github.com>
Co-authored-by: Nicolás Bonet <NicolasBonet@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@NicolasBonet do I need to review this? |
please |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-18.at.01.01.51.mov |
|
@Pujan92 Have you started reviewing this? If not, I can take over since it comes from the original PR where I am reviewer |
|
TBH, I have started it. |
|
@NicolasBonet It looks like the Melvin solution doesn't fix the issue: Screen.Recording.2026-07-18.at.00.20.18.mov |
|
Looks QA issue expectation is incorrect, I just mentioned here #96391 (comment) |
|
@MelvinBot for a single-level tag, I think we are allowing a colon in the value. We can do a conditional change like below const {policy, tags: policyTags} = usePolicy(policyID);
const tag = isMultiLevelTags(policyTags) ? normalizeImportedTag(getCellValue(tagColumn, rowIndex)) : escapeTagName(getCellValue(tagColumn, rowIndex)); |
|
@MelvinBot instead of isMultiLevelTags, we can use policy.hasMultipleTagLists too. |
|
Good catch — the concern is valid. One adjustment to the snippet: // usePolicy already returns `policy`; add the tag lists from Onyx
const [policyTags] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`, {canBeMissing: true});
// import {isMultiLevelTags, escapeTagName} from '@libs/PolicyUtils';
const tag = isMultiLevelTags(policyTags)
? normalizeImportedTag(getCellValue(tagColumn, rowIndex))
: escapeTagName(getCellValue(tagColumn, rowIndex));This would also want the unit test extended to cover the single-level path (a colon in a single-level value stays escaped/verbatim rather than being split). I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author should reply with a request tagging me — |
|
Agreed — const tag = policy?.hasMultipleTagLists
? normalizeImportedTag(getCellValue(tagColumn, rowIndex))
: escapeTagName(getCellValue(tagColumn, rowIndex));No new imports beyond I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author should reply with a request tagging me — |
|
@NicolasBonet can you check #96407 (comment) and ask Melvin to apply if that makes sense to you? @truph01 in case you can give a look too Policy with Single Level Tag Screen.Recording.2026-07-17.at.23.57.14.movPolicy with Multi-Level Tag Screen.Recording.2026-07-17.at.23.56.23.mov |
|
@Pujan92 What about the case where the policy doesn't have any tags and users try importing a rule with tags? |
|
I am not sure what the requirement is for it, but I believe it is fine if that BE changes of adding the non-existent tag later as at the moment the user won't be blocked by adding the rule. |
@NicolasBonet What do you think about it? |
|
@MelvinBot trimming the space is required for both cases #96407 (comment). So consider assigning the value of normalizeImportedTag to one variable and using that for both ternary expressions. |
|
Agreed — since the space-trimming should apply to both branches, computing const normalizedTag = normalizeImportedTag(getCellValue(tagColumn, rowIndex));
const tag = policy?.hasMultipleTagLists ? normalizedTag : escapeTagName(normalizedTag);
One edge case to keep in mind: I can only make code changes when directed by the PR author, an assignee, or an existing co-author. |
|
@gijoe0295 @NicolasBonet One of you needs to 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] |
I pushed the changes to handle both cases, care to double check, please? |
|
@NicolasBonet I tested both cases, and everything looks good except that we don't automatically create a new tag when importing a rule with a tag that doesn't already exist. |
|
We need to update the test steps to clarify what needs to be tested according to the policy tags option(whether it has multiple tag lists or not) |
|
@truph01 That's expected and intentionally out of scope here — we don't auto-create tags anywhere in the app when a flow references a nonexistent one (tags are only created via the explicit Tags > Add tag and Tags > Import routes; personal expense rules and the manual merchant rule flow are selection-only over existing tags). We also closed the category equivalent (PR 96408 and its BE fix) because (a) the creation call goes on the sequential write queue while the import fires immediately as a side-effect request, so the client can't guarantee the tag exists server-side before the rules land, and (b) auto-creating from a rules import is new product behavior — everywhere else a rule referencing a nonexistent tag/category is simply skipped at apply time. Tags are even trickier (multi-level tag lists, integration-synced tags). We'll settle the desired behavior in #96389 first; this PR only fixes the colon encoding. |
|
🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
…ColonFormat Fix imported merchant rule tag corruption for X: Y (multi-level) tag format (cherry picked from commit 8f7cef0) (cherry-picked to staging by roryabraham)
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.4.36-2 🚀
|
|
I reviewed the changes in this PR against Why: This PR is a behind-the-scenes bug fix — it normalizes how a The only relevant article, Merchant Rules, documents the manual Add merchant rule flow and doesn't cover the spreadsheet-import path this fix touches, so there's nothing in it to update. No other article references this behavior. Since no changes are required, I did not create a draft docs PR. |
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.4.38-0 🚀
|
|
🤖 Help site review: no changes required. This PR is an internal bug fix, not a user-facing behavior or feature change, so no updates to Why no docs update is needed
I searched Since no help site changes are required, I did not create a draft PR. @NicolasBonet, please confirm this assessment — if you believe the spreadsheet-import behavior should be documented, let me know and I'll draft the help site PR. |
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.4.40-0 🚀
|
Help site review — no changes requiredI reviewed the changes in this PR against the help site files under Why:
Since the fix restores expected behavior without introducing any new user-facing feature or workflow, there is no draft help site PR to create. @NicolasBonet, please confirm you agree no help site changes are needed here. If you'd like me to document the "Import merchant rules" flow as a separate improvement (independent of this bug fix), let me know and I'll draft it. |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.40-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/roryabraham in version: 9.4.41-0 🚀
|
Help site review — docs PR createdOn re-review, I found a genuine documentation gap this PR relates to: the Import merchant rules spreadsheet flow (and how Tag cells are interpreted) is not documented anywhere. The existing Merchant Rules article only covers creating rules manually, so the behavior this PR fixes ( I've created a draft help site PR adding a new article for it: #96544 — Docs updates for E/A#96407 Document importing merchant rules via spreadsheet It documents:
All feature names, menu items, and buttons were verified against the current UI strings and follow @NicolasBonet, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
Explanation of Change
When importing merchant/expense rules via spreadsheet, a Tag cell typed in
X: Yformat (a multi-level/dependent tag) was stored verbatim, including the space after the colon. Multi-level tags are canonically encoded as levels joined by a bare colon with no surrounding spaces (X:Y) — the same encoding the manual "Add tag" flow produces viatrimTag(newTags.join(':')). Because the import skipped this normalization, the stored valueX: Ywas later split on:into["X", " Y"]on display, so the rule row renderedX, Yand the tag field only resolved the first levelX.Conversely, on a policy with a single-level tag list, a colon in the Tag cell is part of the literal tag name, not a level separator. Storing it unescaped meant the value was later split on
:and the tag wasn't persisted as one name (e.g.ab:cdresolved as two levels).This adds a
normalizeImportedTaghelper inImportedMerchantRulesPagethat branches onpolicy.hasMultipleTagLists::(respecting escaped colons viagetTagArrayFromName), trims each level, and re-joins with:(viatrimTag), soX: Yis stored asX:Y— the canonical encoding the manual "Add tag" flow produces — and displays correctly in both the rule row and the tag field.escapeTagName, soab:cdis stored asab\:cd— matching how manually created tag names are stored on the policy — and the full value persists as one literal tag name.Unit tests cover both paths.
Fixed Issues
$ #96391
PROPOSAL: #96391 (comment)
Tests
Unit test:
npx jest tests/unit/ImportedMerchantRulesPageTest.tsand verify all tests pass.Manual:
The import interprets the Tag cell according to the workspace's tag setup at the time of import (multi-level tags vs. a single-level tag list), so both scenarios below must be tested — set up the tags before importing the rules.
Scenario A: Workspace with multi-level tags (colon = level separator)
Update tag to "Parent, Child"andUpdate tag to "North America, New York"(no space-prefixed or missing second level).Level1: Parent,Level2: Child) instead of only the first level.Scenario B: Workspace with a single-level tag list (colon is part of the tag name)
ab:cd.Update tag to "ab:cd"(the full value persists as one tag name, not split or truncated toab).ab:cdtag selected.Offline tests
N/A
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