[Payment due @shubham1206agra] Copy workspace settings: don't leak Spotnana identity on travel copy + capture terms consent#93320
Conversation
The optimistic Onyx patch for the "travel" part copied the entire source travelSettings onto each target, including the per-policy Spotnana identity fields (spotnanaCompanyID, associatedTravelDomainAccountID) and hasAcceptedTerms. successData never corrected them, so a target would show the source's provisioning state - including hasAcceptedTerms=true - hiding the "Get Started" flow until a full policy refetch. Copy only the non-identity autoAddTripName preference via a dedicated buildTravelSettingsPatch, preserving the target's own identity fields. The backend re-provisions each target with its own Spotnana entity. Mirrors Auth's TRAVEL_SETTINGS_COPYABLE_FIELDS allow-list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the travel part is selected and the source workspace is provisioned for Spotnana, the backend re-provisions each target with its own entity, which accepts Expensify Travel terms on the admin's behalf. Show the terms link and a consent checkbox on the confirm screen and disable Copy until it is checked, so the admin explicitly consents before provisioning runs. The backend already derives which targets need provisioning, so no extra request param is needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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.
|
Build travelSettings patches explicitly from the target and consolidate test helpers to stay within the eslint-seatbelt baseline.
| if (!update || !('value' in update)) { | ||
| return undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable-next-line comment lacks a justification. The other two similar disables in this file include explanations (e.g., -- SET value is a full Policy snapshot, -- MERGE value is a partial Policy patch), but this one does not.
Add a justification comment after --, for example:
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- value is a CopyPolicySettings lifecycle object
return update.value as CopyPolicySettingsStep;Reviewed at: 2464f97 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Ah totally missed it, Added the missing eslint-disable justification in 7fbe3d1.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2464f97e4d
ℹ️ 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".
| if (sourceAutoAddTripName === undefined || !targetTravelSettings) { | ||
| return undefined; |
There was a problem hiding this comment.
Copy travel preferences for unprovisioned targets
When Travel is copied to a target that has no existing travelSettings (the unprovisioned target case covered by the QA/offline flow), this guard skips the autoAddTripName patch entirely. If the source has autoAddTripName: false, the optimistic target only gets isTravelEnabled: true, and the travel settings UI treats a missing value as enabled (policy?.travelSettings?.autoAddTripName !== false), so the copied workspace shows the opposite preference until a server refresh. Please still write a travel settings patch containing the source autoAddTripName while omitting the per-policy identity fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch - fixed in 2a3e91e. buildTravelSettingsPatch now triggers whenever the source has autoAddTripName defined (dropped the !targetTravelSettings guard that skipped unprovisioned targets), and spreads the target's existing travelSettings while overlaying autoAddTripName - the same pattern as setWorkspaceTravelSettings. So an unprovisioned target with a source autoAddTripName: false now optimistically gets {autoAddTripName: false} and the UI shows the correct preference. No identity fields are fabricated (spotnanaCompanyID/associatedTravelDomainAccountID stay undefined), so the target is still not treated as provisioned. Added a test for the unprovisioned-target + autoAddTripName: false case.
Resolve conflicts in CopyPolicySettings by keeping both travel settings patch logic and receipt partners patch logic from their respective branches.
|
@ishpaul777 can you address codex comments? |
buildTravelSettingsPatch skipped the patch entirely when the target had no
existing travelSettings (the common unprovisioned case). A source with
autoAddTripName=false then left the optimistic target without the field, and
the travel settings UI treats a missing value as enabled - so the copied
workspace showed the opposite preference until a server refresh.
Spread the target's existing travelSettings and overlay autoAddTripName (the
setWorkspaceTravelSettings pattern). An unprovisioned target gets only
{autoAddTripName}, with no fabricated Spotnana identity fields, so it is still
not treated as provisioned.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…isioned" The autoAddTripName patch for a not-yet-provisioned target produces a travelSettings object without the Spotnana identity fields, which failed typecheck because spotnanaCompanyID/associatedTravelDomainAccountID/ hasAcceptedTerms were required. They are genuinely absent until the backend provisions the workspace (and every read already optional-chains them), so mark them optional - this removes the type error with no cast and no ripple. Also rephrase "unprovisioned" to "not yet provisioned" in comments and the test name to satisfy the spell check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Core design doc PR
…pes. Centralize the Spotnana provisioning check in PolicyUtils and use typed test helpers plus a Policy filter predicate on the confirm page.
|
PR looks good, no issues found, I'm ready to review as soon as the bot comment is resolved and we have a proper author checklist |
|
@Valforte bot comment is addressed and checklist is fixed! |
|
@Valforte Should I review this first? |
|
@shubham1206agra please! I'll remove the checklist |
Copying travel from a provisioned source re-provisions each target, which requires the admin to accept Expensify Travel terms (needs their legal name) and a company address per target for the Spotnana entity - the same prerequisites BookTravelButton enforces in the normal travel flow. - Hide the Travel feature when the source is provisioned and the current user has no legal name, mirroring areTravelPersonalDetailsMissing. - On the confirm screen, disable Copy and show a message when travel is being copied and any selected target has no company address. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…dress Replace the confirm-screen red error + disabled Copy button with the existing gray-out pattern used for accounting mismatches: disable the Travel feature row and show an explanatory subtitle on the selection step. Travel provisioning creates a Spotnana entity per target, which needs a company address. The source address only reaches a target when "overview" is also copied, so Travel is disabled when any target lacks an address - unless overview is selected and the source has an address to supply. When the source has no address either, Travel is hard-disabled since overview can't help. Moves the message from confirmSettings.travelAddressRequired to selectSettings.travelAddressMismatch across all locales. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Provisioning a target for travel does not require a legal name: AcceptSpotnanaTerms has no legal-name check, and ProvisionUserSpotnana uses firstName/lastName with an email fallback. A legal name is only required at trip-booking time, which BookTravelButton already enforces. Gating the copy on it blocked a valid setup flow, so remove the currentUserHasLegalName hiding of the Travel feature. Also removes the "unprovisioned" wording that the spell check flagged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-06-16.at.10.12.21.PM.mov
|
|
🎯 @shubham1206agra, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
| // Travel needs a company address on every target. When the source has no address either, | ||
| // copying "overview" can't supply one, so travel can never be provisioned - hard-disable it. | ||
| if (part === 'travel') { | ||
| return hasTargetWithoutAddress && !sourceHasAddress; | ||
| } |
There was a problem hiding this comment.
cc @yuwenmemon for vizzz Spotnana required company address to provision so if any WS dont have and user dont copy it from overview it silently skip provisioning, so i diabled the travel part with grayed out pattern for this edge case
|
@ishpaul777 there are conflicts, can you fix it so we can merge? |
…-spotnana-provisioning-clean
|
@ishpaul777 still conflicts. |
Resolve import conflict in CopyPolicySettingsConfirmPage.tsx by keeping isSourceProvisionedForTravel and Policy type alongside upstream's isLoadingOnyxValue addition.
|
fixed! @yuwenmemon |
|
🚧 @yuwenmemon 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/yuwenmemon in version: 9.4.15-0 🚀
|
|
🤖 Help-site review: no I reviewed the changes against the help site and applied What this PR changes (user-facing): within the Copy settings flow (three dots (⋮) on a workspace in Workspaces → Copy settings), when Travel is copied from a source already set up for Expensify Travel:
Why no docs change is needed:
Recommendation: When @ishpaul777, does that match your expectation? If you'd like me to draft a beta Copy settings article (or add a Travel note to the Duplicate Workspace article) anyway, reply with |
|
Hi @ishpaul777, can you please share clear QA steps for testing? |
|
Precondition:
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.15-3 🚀
|
|
🤖 Payment issue created: #94063 |


Explanation of Change
Part of the bulk workspace-settings copy feature. When the Travel part is copied, two things needed fixing on the client:
Optimistic data was leaking Spotnana identity fields. The
travelpart mapped to['isTravelEnabled', 'travelSettings'], so the optimistic Onyx update copied the source's entiretravelSettings— includingspotnanaCompanyID,associatedTravelDomainAccountID, andhasAcceptedTerms— onto each target.successDatanever corrected those fields, so a target would optimistically (and then persistently, until a full refetch) show the source's Spotnana entity andhasAcceptedTerms: true, hiding the "Get Started" travel-provisioning state. This now copies only the non-identityautoAddTripNamepreference via a dedicatedbuildTravelSettingsPatch, preserving the target's own identity fields. The backend re-provisions each target with its own Spotnana entity, so the identity fields must never be copied. Mirrors Auth'sTRAVEL_SETTINGS_COPYABLE_FIELDSallow-list.Travel terms consent. Copying travel from a provisioned source causes the backend to re-provision each target, which accepts Expensify Travel terms on the admin's behalf. The confirm screen now shows the Expensify Travel terms link and a consent checkbox whenever the Travel part is selected and the source workspace is provisioned for travel, and the Copy button is disabled until the admin checks it. No new request parameter is needed — the backend already derives which targets require provisioning.
Fixed Issues
$
PROPOSAL:
For https://github.com/Expensify/Expensify/issues/642749
Tests
travelSettings.spotnanaCompanyID/associatedTravelDomainAccountID), and at least one target workspace that is NOT provisioned for travel.Offline tests
autoAddTripNamepreference, but does NOT show the source's Spotnana identity fields orhasAcceptedTerms.QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Screen.Recording.2026-06-16.at.6.06.52.AM.mov
Android: mWeb Chrome
Screen.Recording.2026-06-16.at.6.01.44.AM.mov
iOS: Native
Screen.Recording.2026-06-16.at.6.04.10.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-06-16.at.5.55.25.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-06-16.at.5.53.13.AM.mov