[NoQA] Feature: Travel Invoicing - Release 2.4: Update Settlement Frequency Flow#79810
[NoQA] Feature: Travel Invoicing - Release 2.4: Update Settlement Frequency Flow#79810ikevin127 wants to merge 13 commits intoExpensify:mainfrom
Conversation
|
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 |
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.
|
melvin-bot
bot
requested review from
joekaufmanexpensify and
parasharrajat
and removed request for
a team
|
@parasharrajat 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b58d69d992
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/pages/workspace/travel/WorkspaceTravelSettlementFrequencyPage.tsx
Outdated
Show resolved
Hide resolved
|
🟢 Ready for review! |
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Design doc implementation
ikevin127
dismissed
joekaufmanexpensify’s stale review
The merge-base changed after approval.
|
@ikevin127 can you update this branch and fix the merge conflicts? |
|
Conflicts @ikevin127 |
|
@ikevin127 Can you please merge main here? There are duplicate changes here. |
|
No worries - conflicts are expected since these PRs are branched out from each other in order - I'll always make sure to resolve them. Conflicts shouldn't block review though. |
…7-travel-invoicing-24
|
🟢 Conflicts resolved - synced with latest main! |
src/libs/actions/TravelInvoicing.ts
Outdated
| frequency: ValueOf<typeof CONST.EXPENSIFY_CARD.FREQUENCY_SETTING>, | ||
| currentMonthlySettlementDate?: Date, | ||
| ) { | ||
| const cardSettingsKey = |
There was a problem hiding this comment.
You created a function for this getTravelInvoicingCardSettingsKey..
src/libs/actions/TravelInvoicing.ts
Outdated
| pendingFields: { | ||
| monthlySettlementDate: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE, | ||
| }, | ||
| errors: null, |
There was a problem hiding this comment.
We don't need the error key to be set here if dealing with errorFields
|
Noticed a bug:
31.01.2026_17.27.52_REC.mp4 |
Screenshots🔲 iOS / Safari31.01.2026_17.35.37_REC.mp4🔲 MacOS / Chrome31.01.2026_17.33.36_REC.mp4🔲 Android / native |
|
BUG: Errors are not cleared on the settlement account field. 31.01.2026_17.43.49_REC.mp4 |
|
Bump @ikevin127 |
|
Will get to this one soon - currently attention is directed towards #81084 to allow testing for @blimpich and because this testing PR will pring about some changes that will affect all currently opened PRs we should hold the currently opened PRs until that one is merged 👍 |
ikevin127
changed the title
|
Any updates? @ikevin127 |
✅ Resolved, after #81084 was merged now the flow works as follows:
@parasharrajat 🟢 Ready for review! Updated PR description tests since testing is easier after the latest PR was merged. |
ikevin127
changed the title
|
❌ ESLint fail is not caused by our PRs changes, there's no errors just warnings coming from other PRs changes which overflow the warning limit causing workflow to fail. |
Why are we using a Settlement account for this purpose?
This contradicts your point 2 ⬇️
|
| /> | ||
| <OfflineWithFeedback | ||
| errors={settlementFrequencyErrors} | ||
| pendingAction={cardSettings?.pendingFields?.monthlySettlementDate} |
There was a problem hiding this comment.
Shouldn't the field name be settlementFrequency? having monthly in the name while the values can be montly and other, does not seem right.
There was a problem hiding this comment.
I agree with that point, but the reason I used monthlySettlementDate is due to design doc specs - because BE is using the Expensify Card model for Travel cards which name the variable like that, here's ref from docs section:
// Example of private_expensifyCardSettings NVP
const example = {
US: {
paymentBankAccountID: 101,
limit: 30000,
marqetaBusinessToken: 1,
ownerEmail: "owner@example.com",
monthlySettlementDate: 15,
country: "US",
currency: "USD"
}
}Before changing the variable name to settlementFrequency, we would have to ensure that this aligns with BE logic. @blimpich For input on whether we should change this or leave as is 🙌
If anything, I noticed that the type I used for monthlySettlementDate should be changed to number instead of Date since that's what BE seems to expect.
Edit: I checked Expensify Card types and it was monthlySettlementDate: Date; before any of Travel Invoicing logic so I guess it should remain Date ?
There was a problem hiding this comment.
Besides what I referenced above, in design doc we also have this section which conflicts with previous reference:
- 1.10 Create UpdateTravelInvoiceSettlementFrequency command which does indeed reference
settlementFrequencyas param:
} elseif ($command === 'UpdateTravelInvoiceSettlementFrequency') {
Auth::updateExpensifyCardDomainSettings(
$authToken,
$domainName,
Request::getInt('domainAccountID'),
Request::getString('settlementFrequency'),
Card::PROGRAM_TRAVEL_US // Use travel feed type
);
}There was a problem hiding this comment.
Looks like:
UpdateTravelInvoiceSettlementFrequencytakessettlementFrequencyparam asStringaccepting:
FREQUENCY_SETTING: {
DAILY: 'daily',
MONTHLY: 'monthly',
},monthlySettlementDateis a separate param existing on Expensify Card object that from BE side seems to acceptNumber(day of month) instead of date / but is typed asDatein FE
For Travel Invoicing - Release 2.4: Update Settlement Frequency Flow current logic uses this logic to send the FE choice to BE UpdateTravelInvoiceSettlementFrequency API:
@blimpich We have to sort out if this is the expected logic / flow from FE <-> BE or we should change things at this point in this PR.
There was a problem hiding this comment.
- Lets change the type of monthlySettlementDate from date to number. It should be number
monthlySettlementDateis how we reference it on the backend, so lets keep the naming 👍
This is slightly confusing because the backend does some inference, but Kevin is right that the backend data model uses monthlySettlementDate not settlementFrequency. The backend assumes that if there is a monthlySettlementDate then it is monthly, and if not then it is daily. The settlementFrequency parameter for the UpdateTravelInvoiceSettlementFrequency command is an abstraction over that logic.
There was a problem hiding this comment.
Sounds good. Thanks for confirming both.
…7-travel-invoicing-24
That's how we agreed on the flow behaving for PR #81084 / Slack thread discussion.
It doesn't contradict because we're talking about 2 specific cases:
|
4 participants
Explanation of Change
2.4 Update Settlement Frequency Flow
UX Flow
Default Value: When Travel Invoicing is first enabled, the settlement frequency defaults to Monthly (matching
CONST.EXPENSIFY_CARD.FREQUENCY_SETTING.MONTHLY). This aligns with typical business billing cycles and mirrors Expensify Card behavior. Users can change to daily if preferred.Data & Onyx
Current frequency comes from
getTravelSettlementFrequency(policy)(e.g., 'monthly', 'weekly').We map backend values to localized labels via a pure helper:
getLocalizedSettlementFrequencyLabel(frequency, translate).Actions
New function in
src/libs/actions/TravelInvoicing.ts:updateTravelInvoiceSettlementFrequency({policyID, frequency}):UpdateTravelInvoiceSettlementFrequency.Promiseand surfaces failures as localized errors.Components & reuse
Frequency selector:
SelectionList+RadioListItem, same as other simple settings pickers.WorkspaceTravelInvoicingSection:MenuItemwith a right‑aligned label for the localized frequency.Testing
Component tests:
updateTravelInvoiceSettlementFrequencywith the correct payload.Fixed Issues
$ #78676
PROPOSAL:
Tests
Preconditions:
Test 1: View Settlement Frequency Row
Steps:
Expected Results:
Test 2: Open Frequency Selection Screen
Steps:
Expected Results:
Test 3: Change Frequency (Online)
Steps:
Expected Results:
✅ Selection immediately updates to "Daily"
✅ Screen navigates back to Travel Invoicing section
✅ Settlement frequency row now displays "Daily"
Verify that no errors appear in the JS console
Offline tests
Test 5: Change Frequency While Offline (Optimistic Update)
Steps:
Expected Results:
Test 6: Reconnect After Offline Change (Success)
Steps:
Expected Results:
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
web.mov
ios-hybrid.mov