Skip to content

Hide deleted tags in merchant rule preview - #103408

Merged
mollfpr merged 2 commits into
Expensify:mainfrom
situchan:fix-102229
Oct 12, 2026
Merged

mollfpr merged 2 commits into
Expensify:mainfrom
situchan:fix-102229

Conversation

@situchan

@situchan situchan commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

The backend keeps a deleted tag on a merchant rule so the rule applies again if the tag is recreated. The Rules table printed that tag without checking it still exists.
#98662 removed the old isTagInPolicy check added in #86864 along with MerchantRulesSection.

The merchant rule summary now drops the tag when isTagInPolicy returns false, so a rule left with only a merchant rename shows as "Rename".
Tags aren't hidden while policy tags are still loading.

Fixed Issues

$ #102229
PROPOSAL:

Tests

Precondition: control workspace with Rules and Tags enabled, and a tag Tag 1.

  1. Go to Workspace settings > Rules and add a merchant rule: merchant Apple, tag Tag 1, description Test.
  2. Verify the rule row in the table shows Update tag to "Tag 1".
  3. Go to Tags and delete Tag 1.
  4. Go back to Rules.
  5. Verify the rule preview no longer shows the tag but still shows the description.
  6. Recreate Tag 1, go back to Rules, and verify the tag shows again in the preview.
  7. Add a merchant rule that renames the merchant and sets tag Tag 1, then delete Tag 1.
  8. Verify that rule's type changes to "Rename" and only the rename shows.
  • Verify that no errors appear in the JS console

Offline tests

  1. Go offline and delete the tag used in a merchant rule.
  2. Go to Rules and verify the tag is already hidden from the rule row in the table.
  3. Click on the rule, then its Tag row.
  4. Verify that the deleted tag isn't in the list and nothing is selected.

QA Steps

Same as Tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
demo.mov

@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/libs/MerchantTypeRulesUtils.ts 65.51% <100.00%> (+3.88%) ⬆️
...s/workspace/rules/tabs/useRulesTableBulkActions.ts 61.22% <100.00%> (+0.39%) ⬆️
...s/workspace/rules/tabs/RulesExpenseDefaultsTab.tsx 0.00% <0.00%> (ø)
...pages/workspace/rules/MerchantRules/AddTagPage.tsx 0.00% <0.00%> (ø)
... and 8 files with indirect coverage changes

@situchan

situchan commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Review

The fix looks right for single-level tags, and CI passes. I found one issue with multi-level tags. It's worth fixing before merge.

1. Multi-level tags: deleting one level hides the whole tag

MerchantTypeRulesUtils.ts:150-152 drops the whole tag field when isTagInPolicy returns false. isTagInPolicy uses every(), so it returns false if any single level is missing.

The rule page checks each level on its own instead (MerchantRulePageBase.tsx:565-571).

Example with independent tag lists:

  • A rule sets Engineering:NYC.
  • An admin deletes NYC.
  • The rule page still shows Engineering in the first tag row.
  • The table drops the tag entirely, even though the rule still sets Engineering.

Suggested fix: check each level with findPolicyTagAtLevel, the same way the rule page does. Replace missing or pending-delete levels with ''. getCommaSeparatedTagNameWithSanitizedColons already skips empty parts, so the table would show Update tag to "Engineering". Drop the field only when every level is gone. Please add a multi-level case to MerchantRuleTagSummaryTest.ts.

2. Minor: tag-only rule shows an empty description

When a rule's only default is a deleted tag, the row shows "Update" with a blank description. The test at the end of MerchantRuleTagSummaryTest.ts treats this as intended. Please confirm it's OK with design.

Checked and fine
  • Loading state: when policyTags is undefined, the tag is kept, so it won't flicker.
  • Pending-delete tags are hidden right away offline, in both the table and AddTagPage.
  • Dependent tags: isTagInPolicy matches each level by its parent path.
  • Both callers of getExpenseDefaultsTableData now pass policyTags.

@situchan

situchan commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

@MelvinBot your both issues (or maybe not bugs) existed in #86864 (original PR which fixed the linked issue). The root cause already exists. Can you confirm?

@MelvinBot

Copy link
Copy Markdown
Contributor

Yes, confirmed. Both behaviors already existed in fix: System message with no tag is shown. This PR restores that logic rather than adding new problems, so neither one should block it.

  1. Multi-level tags: fix: System message with no tag is shown #86864 added the same isTagInPolicy check with every(). It also dropped the whole tag when any level was missing (MerchantRulesSection.tsx:72).
  2. Empty description: The old getRuleDescription also returned '' for a tag-only rule with a deleted tag (MerchantRulesSection.tsx:95).

Per-level handling would be a separate follow-up if you want it.

@situchan
situchan marked this pull request as ready for review October 8, 2026 04:31
@situchan
situchan requested review from a team as code owners October 8, 2026 04:31
@melvin-bot
melvin-bot Bot requested review from dylanexpensify and thesahindia and removed request for a team October 8, 2026 04:31
@melvin-bot

melvin-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

@thesahindia 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]

@dylanexpensify dylanexpensify left a comment

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.

Looks good from a product perspective 👍

@thesahindia

Copy link
Copy Markdown
Member

@situchan, the selected tag doesn't disappear if disabled. I think it should have the same behaviour as deleted?

Screen.Recording.2026-10-10.at.2.56.11.AM.mov

@situchan

situchan commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

the selected tag doesn't disappear if disabled.

I believe it's expected as per rule will keep deleted values, but the FE won't display it if the value doesn't exist in the policy anymore.

The disabled tag still exists in the policy so makes sense to show

cc: @luacmartins

@thesahindia

Copy link
Copy Markdown
Member

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified any copy / text that was added to the app is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If the PR adds or modifies the UI:
    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp
Screen.Recording.2026-10-12.at.12.14.55.AM.mov
Android: mWeb Chrome
Screen.Recording.2026-10-12.at.12.28.17.AM.mov
iOS: HybridApp
Screen.Recording.2026-10-11.at.10.00.04.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-10-11.at.11.45.00.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-10-12.at.12.57.16.AM.mov

@thesahindia thesahindia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good and tested well

@melvin-bot

melvin-bot Bot commented Oct 11, 2026

Copy link
Copy Markdown

We did not find an internal engineer to review this PR, trying to assign a random engineer to #102229 as well as to this PR... Please reach out for help on Slack if no one gets assigned!

@melvin-bot
melvin-bot Bot requested a review from mollfpr October 11, 2026 19:41
@mollfpr
mollfpr merged commit 4445fe2 into Expensify:main Oct 12, 2026
42 of 44 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🚧 mollfpr has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/mollfpr in version: 9.5.8-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site update needed. This bug fix restores behavior that Workspace Merchant Rules already describes: "If a tag a rule applies is deleted from the workspace, the rule stops applying that tag and stops showing it."


view run

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/cristipaval in version: 9.5.8-5 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants