Show the debited and reimbursed amounts on the reconciliation dashboard - #97167
Show the debited and reimbursed amounts on the reconciliation dashboard#97167ishpaul777 wants to merge 15 commits into
Conversation
…s until a settlement converts
|
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 |
|
@MelvinBot review |
|
@codex review |
SortableTableHeader only makes a header pressable when isColumnSortable is set, so without it the columns rendered as plain labels and the sort Auth supports could not be reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getSortedData re-sorts the page by the mapped property, which for these two columns is a raw amount in whichever currency that settlement used. Ranking GBP 10250 against USD 12000 that way puts them in the wrong order. Unmapped columns fall through with the data untouched, which is the order Auth already computed from a common-currency sort key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
typecheck and TranslateTest both fail on a key that only exists in en.ts. The generateTranslations bot skips pull requests from forks, so these are written by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 Reviewed the diff. The visibility gating, types, styles, parser regen, and the two column-availability tests all look clean and match the existing patterns. One blocker on the sorting behavior, plus a design question and a minor nit. 🔴 The two new columns aren't actually sortableThe PR description says "Both columns are sortable" and Test step 8 asks to click the
In 🟡 Once sortable, the client sorts by the raw per-currency amountThe description argues raw amounts can't be ranked across currencies and that the server ranks via a hidden converted key. But grouped results are re-sorted client-side — 🟢 Minor: falsy guard hides a zero amount
Everything else looks good — nice touch adding the |
![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: 3d7bcaccf9
ℹ️ 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".
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.
|
The list is built at module scope, so the name resolves the moment the file loads and grouping by category throws instead of rendering.
The helper answered "does this column have data" for every column, which read as a double negative for the ones it knows nothing about. Bail out on a transaction array first, then ask the question only of the two conversion columns, matching the expense report branch above.
The server communicates its order through orderedKeys, which the client never reads, so a column with no sorting property leaves the rows in the order the response object happened to be keyed in and clicking the header does nothing. Point both columns at their amount, the way the total column already works.
|
@MelvinBot final review this |
Final reviewLGTM — the change is well-scoped and complete. The two currency-aware columns are wired through the whole stack consistently, and the edge cases you'd worry about are covered. A couple of non-blocking observations below. What I verified
Observations (non-blocking)
Neither blocks merge — nice, clean PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6f26227d3
ℹ️ 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".
| return result; | ||
| } | ||
|
|
||
| return result.filter((column) => !isConversionAmountGroupColumn(column) || hasGroupWithConversionAmount(column, data)); |
There was a problem hiding this comment.
When a withdrawal search has more pages and the currently loaded page contains only domestic settlements, this filter removes both conversion columns even if later pages contain cross-border groups. This also makes both fields unavailable in the Sort By popup; after sorting a populated amount column ascending, the backend can return all empty domestic groups first, causing the active column and its header to disappear until enough additional pages are loaded. Defer the absence-based hiding until all results are loaded, or use result-level metadata rather than inspecting only the accumulated page data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@ishpaul777 Could you take a look at this comment?
There was a problem hiding this comment.
Fixed in 3e0d2ec - the column the results are sorted by now stays visible even when no loaded group carries that amount, so the header and the Sort by option survive an all-domestic first page.
|
Let us know when there are screenshots to review. |
|
sure |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@ishpaul777 Could you record a video and attach it to checklist? |
Sorting ascending puts the groups without that amount first, so an all-domestic first page would drop the column, taking the header and the Sort by option with it.
done! |
|
@ishpaul777 Could you sync with main to resolve the conflict? |
4 participants
Explanation of Change
Grouped by Withdrawal ID, the dashboard only showed the report total. A cross-border settlement debits the company and credits the employee in different currencies, so one number cannot describe it.
Adds
Amount debitedandAmount reimbursedbetweenExpensesandTotal, each in its own currency, from Auth #23263.achAccount, while Auth derives global reimbursement from the policy's reimbursement countries. Gating on the data lands in the same place.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/655931
PROPOSAL: N/A - internal issue
Tests
Amount debitedandAmount reimbursedbetweenExpensesandTotal.Amount debited, and verify the column and its header disappear and the rest of the table stays aligned.Amount debitedheader and verify the rows reorder by that amount, and that clicking it again flips the order. Repeat forAmount reimbursed.Withdrawn,ExpensesandTotalstill works.Offline tests
QA Steps
Same as tests, using a staging account that is an admin of a workspace with global reimbursements enabled and a settled cross-border reimbursement.
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
Screen.Recording.2026-08-03.at.8.03.34.PM.mov