Repository navigation
Show the Upgrade step when copying rules to a Collect workspace - #101710
Conversation
Copying the rules or codingRules parts writes Control-only policy fields, so a Collect target must be upgraded first. Gate those parts in the copy flow instead of deferring to canPolicyAccessFeature, which correctly reports that Collect can enable Rules on its own. Co-authored-by: daledah <daledah@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
🤖 Web test steps verification results: ✅ pass Copying Rules from a Control workspace to a Collect workspace now routes through the Upgrade step ("Some features require a Control plan"), the upgrade completes with a success view, and no copy-failure error appears anywhere in the flow. Steps came from the linked issue Auto - Workspace - Unable to "Copy settings" from Control to Collect workspace, since this PR's
|
|
🤖 Android test steps verification results: ✅ pass Copying the Control-only "Rules" setting from a Control workspace to a Collect workspace now routes through the Upgrade step, the upgrade succeeds, and the copy completes with no failure error. Steps came from the linked issue Auto - Workspace - Unable to "Copy settings" from Control to Collect workspace, since this PR's
|
|
@MelvinBot please merge main |
|
@MelvinBot please open this PR |
…lesUpgradeStep Co-authored-by: daledah <daledah@users.noreply.github.com>
|
🤖 Merged Merge commit: Neither of this PR's two files — I could not re-run the unit tests or verify in a browser in this run: the test command was blocked by the sandbox and the web session was unavailable. CI on this push will run the suites. |
|
🤖 Done — the PR is out of draft and open for review.
Two things still need a human before review can finish:
I could not verify the change in a browser in this run — the web session was unavailable. CI will run the unit suites on the pushed commit. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeScreen.Recording.2026-09-23.at.15.46.53.moviOS: HybridAppios.moviOS: mWeb Safariios-mweb.movMacOS: Chrome / Safariweb.mov |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /** | ||
| * The selected parts that the Collect (Team) targets can't receive on their current plan. Most parts | ||
| * defer to `canPolicyAccessFeature` (the single source of truth for which features require a Control | ||
| * plan); `CONTROL_ONLY_COPY_PARTS` covers the parts that need Control to be copied even when the |
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
This JSDoc block joins two independent sentences with a semicolon (...require a Control plan); \CONTROL_ONLY_COPY_PARTS` covers the parts...`). Comments should read as plain sentences, so use two separate sentences instead of a semicolon.
Suggested fix:
/**
* The selected parts that the Collect (Team) targets can't receive on their current plan. Most parts
* defer to `canPolicyAccessFeature` (the single source of truth for which features require a Control
* plan). `CONTROL_ONLY_COPY_PARTS` covers the parts that need Control to be copied even when the
* target could enable the feature itself. Returns empty when there are no Collect targets.
*/Reviewed at: 3571add | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
🤖 Valid finding — fixed in 724104e.
The JSDoc did join two independent sentences with a semicolon, which CONSISTENCY-15 flags. I applied the suggested wording verbatim — plan); → plan). at src/libs/CopyPolicySettingsUtils.ts:321. Comment text only, one character; no logic, types, or tests touched.
I couldn't run lint/spellcheck locally in this run (the sandbox blocked the Node tooling) and browser verification wasn't available, but CI runs both on this push.
|
🤖 Nothing to fix — no check is failing on the code. Every build, lint, type, and test check passed on the latest commit ( The one red check is Check independent approval, which fails only because the PR has no approving review yet ( Check-by-check status
The failing job's only failed step is I could not verify anything in a browser on this run — the web session backend was unavailable — but no browser check was needed here, since nothing in the code is failing. |
Co-authored-by: daledah <daledah@users.noreply.github.com>
|
@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] |
|
🤖 Both red checks are stale process gates, not code failures. No code fix exists to push. Every code check passes on the current head 1. You posted the Reviewer Checklist at 08:36:36 with every box unchecked, the job ran at 08:42:03 and correctly reported It will not re-run on a push: the workflow triggers only on 2.
Full check status on
|
| Check | Result |
|---|---|
test (jobs 1–8) |
✅ pass |
typecheck |
✅ pass |
ESLint check |
✅ pass |
Oxfmt check |
✅ pass |
Compare knip issues against main |
✅ pass |
spellcheck |
✅ pass |
Storybook tests / Bun tests |
✅ pass |
React Compiler Compliance |
✅ pass |
Check for unused styles |
✅ pass |
baseline-perf-tests / branch-perf-tests / validate-perf-tests |
✅ pass |
build (Android + iOS, all variants) |
✅ pass |
codecov/patch, codecov/project |
✅ pass |
verifySignedCommits, CLA |
✅ pass |
checklist (PR Reviewer Checklist) |
❌ stale — re-run it |
Check independent approval |
❌ needs a human approval |
The only commit since my last comment is 724104e ("Split JSDoc semicolon into two sentences per CONSISTENCY-15"), touching only src/libs/CopyPolicySettingsUtils.ts — a comment-only change, and every code check passed on it.
I could not verify anything in a browser on this run — the web session backend was unavailable — but no browser check was needed here, since nothing in the code is failing.
|
@MelvinBot i've completed my checklist, please complete the author checklist and fill the |
|
🤖 Filled in Tests, Offline tests, and QA Steps, and ticked the author checklist items I can stand behind. The test steps follow the repro from Auto - Workspace - Unable to "Copy settings" from Control to Collect workspace and add the three regression cases this change could plausibly break: a Control target (must still skip Upgrade), a Collect target with only non-Control parts selected (must still skip Upgrade), and the Rules toggle on a Collect workspace's More features page (unchanged, because this PR gates the copy in Seven boxes are left for you — they all attest to testing I can't do or can't see evidence of:
I could not verify any of this in a browser in this run — the web session was unavailable. Everything I ticked is backed by the diff, the unit tests, or static analysis of the flow. |
|
🚧 marcochavezf has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.4.93-0 🚀
|
|
🤖 Yes, a help site change is required. Draft PR: Docs updates for E/A#101710 Rules copy requires a Control destination workspace This PR changes user-visible behavior in the Copy settings flow: selecting Rules or Merchant rules with a Collect destination workspace now shows the Some features require a Control plan page instead of running a copy that fails. docs/articles/new-expensify/workspaces/Copy-Workspace-Settings.md was the only affected article, and it only said "some settings require the destination workspaces to be on the Control plan" without naming any. Its numbered steps also went straight from Select settings to the summary, so a reader who hit the upgrade prompt had nothing to match it against. What the docs PR changes (that one file, nothing else):
Label verification and style complianceEvery UI label in the article was checked against the current app rather than written from memory:
The Rules and Merchant rules rows could not be exercised in the live session, because the test account has no Control workspace with rules enabled. Those two labels were confirmed from the translation source instead. Written against @daledah, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #102184 was identified to be related to this PR. |
|
Deploy Blocker #102210 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.93-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Copying the Rules feature from a Control workspace to a Collect workspace ended in a copy failure instead of showing the Upgrade step.
shouldShowCopyPolicySettingsUpgradeStepdelegates the whole decision tocanPolicyAccessFeature, which returnstruefor Rules on a Collect policy. Soruleswas filtered out of the Control-only set, the Select settings step skipped Upgrade and went straight to Confirm, and the copy fired — and the backend rejected it.That behaviour changed in Remove rulesRevamp beta.:
isCollectPolicy(policy) && isRulesRevampEnabled(false in production, so Rules was treated as Control-only and the Upgrade step did show) became an unconditionalisCollectPolicy(policy).The fix: gate
rulesandcodingRulesinsidegetControlOnlySelectedPartsrather than changingcanPolicyAccessFeature. The two questions are different:canPolicyAccessFeatureanswers, and its three other consumers (More Features page, workspace menu items) depend on that answer.rulespart copies ~17 policy fields at once (maxExpenseAmount,eReceipts,preventSelfApproval,glCodes, …), most of them Control-only.codingRules(Merchant rules) maps to the same feature, so it has the same hole.With the gate in place, Select settings routes to the existing Upgrade step (route, page and copy already exist) before anything is sent.
This also flips the assertion added by
Expensify/App#98662inCopyPolicySettingsUtilsTest.ts, which locked in the broken behaviour — that test is updated, and coverage is added forcodingRulesand for the Control-target case.Full analysis: the approved proposal on the issue.
AI Tests run by Melvin
npm test tests/unit/CopyPolicySettingsUtilsTest.tsnpm test(all 6 copy-settings suites)npm run typechecknpm run spell-changedFixed Issues
$ #101456
PROPOSAL: #101456 (comment)
Tests
Preconditions — sign in as an admin of two paid workspaces:
1. Rules → Collect target shows the Upgrade step (the bug)
2. Merchant rules behaves the same
3. Regression — Upgrade must still be skipped when it isn't needed
4. Regression — the Rules toggle on Collect is unchanged
canPolicyAccessFeature, which drives that toggle.)Offline tests
QA Steps
Preconditions — on staging, use an Expensifail account that is an admin of two paid workspaces:
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari