fix: Show editable Amount by default in report view#94929
Conversation
|
@gijoe0295 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] |
trjExpensify
left a comment
There was a problem hiding this comment.
I still think we should keep the orientation we had before whereby the Total column is after the Amount column so the totals on the report line up better:
What do you think @iwiznia @shawnborton?
|
Yup, makes sense to me 👍 |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@trjExpensify Thanks for confirming. Updated the ordering so when both are visible it stays Could you please check this ?
|
|
@gijoe0295 The PR is ready for review. Could you please take a look ? |
Yep! 👍 |
|
@gijoe0295, Gentle bump. thanks. |
| [CONST.SEARCH.TABLE_COLUMNS.COMMENTS]: shouldShowCommentsColumn, | ||
| [CONST.SEARCH.TABLE_COLUMNS.TOTAL]: true, | ||
| [CONST.SEARCH.TABLE_COLUMNS.TOTAL]: false, | ||
| [CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT]: true, |
There was a problem hiding this comment.
Why did you reorder the columns here?
There was a problem hiding this comment.
I reordered it to keep Comments immediately before the money columns, consistent with the behavior from #90301.
Without this change, same-currency reports render as Amount | Comments, while multi-currency reports render as Amount | Comments | Total. Moving COMMENTS before TOTAL_AMOUNT keeps the layout consistent:
- same currency:
Comments | Amount - multi-currency:
Comments | Amount | Total
This also preserves the latest design feedback that Total should remain after Amount when both are visible.
Please let me know if you think we should handle this differently.
There was a problem hiding this comment.
This also preserves the latest design feedback that Total should remain after Amount when both are visible.
Without the reordering, Amount was already to the left of Total.
multi-currency reports render as Amount | Comments | Total
The Comments column reordering is already done manually:
Lines 5927 to 5934 in c8d6035
There was a problem hiding this comment.
The Comments column reordering is already done manually
The manual Comments insertion handles the custom-columns path, but it does not run for the default path when visibleColumns is empty. In the default path, the order comes from Object.keys(columns).
So if we keep the previous order, default reports with comments render as Amount | Comments / Amount | Comments | Total.
Since the expected layout is Comments | Amount and Comments | Amount | Total, I think we still need to move COMMENTS before TOTAL_AMOUNT in the default map as well.
| if (shouldShowCommentsColumn && !addedColumns.has(CONST.SEARCH.TABLE_COLUMNS.COMMENTS)) { | ||
| const totalIndex = result.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL); | ||
| if (totalIndex === -1) { | ||
| const moneyColumnIndexes = [result.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT), result.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL)].filter((index) => index !== -1); |
There was a problem hiding this comment.
I think we only need to change TOTAL to TOTAL_AMOUNT here.
When there're both TOTAL and TOTAL_AMOUNT, TOTAL_AMOUNT is always before TOTAL. Wdyt?
There was a problem hiding this comment.
That makes sense. Since TOTAL_AMOUNT is now the default money column and should always come before TOTAL, anchoring COMMENTS directly before TOTAL_AMOUNT is enough. I simplified the logic. Thanks
| // Expense report view: TOTAL_AMOUNT (transaction amount) is shown by default. TOTAL | ||
| // (workspace currency) is added when a conversion exists. ORIGINAL_AMOUNT never | ||
| // renders by default in report view because it must be explicitly selected. | ||
| // Search page: ORIGINAL_AMOUNT stays data-driven (shown whenever a conversion exists). |
There was a problem hiding this comment.
Search page: ORIGINAL_AMOUNT stays data-driven (shown whenever a conversion exists).
Where does this come from? Does not seem right.
On Search page, we always show TOTAL_AMOUNT alone. We don't show the converted amount.
Also ORIGINAL_AMOUNT is not the converted amount.
Update: This is a stale logic that was removed in #93146. Please merge main and remove it.
|
|
||
| if (customResult) { | ||
| return customResult; | ||
| return customResult.filter((col) => col !== CONST.SEARCH.TABLE_COLUMNS.TOTAL || columns[CONST.SEARCH.TABLE_COLUMNS.TOTAL]); |
There was a problem hiding this comment.
I traced the PR that removes the nonDataColumns predicate. They intentionally want to always show the selected columns even when they have no data. See #92692 (comment).
So let's remove this change, returning customResult is correct here.
|
@gijoe0295 I’ve addressed the feedback comments. Could you please take another look when you get a chance? |
| // Expense report view: TOTAL_AMOUNT (transaction amount) is shown by default. | ||
| // TOTAL (workspace currency) is added when a conversion exists. |
There was a problem hiding this comment.
| // Expense report view: TOTAL_AMOUNT (transaction amount) is shown by default. | |
| // TOTAL (workspace currency) is added when a conversion exists. |
Not needed. Code is self-explanatory.
|
@x-dev90 Please update this test step:
to:
|
Reviewer Checklist
Screenshots/Videos |
|
@trjExpensify One question. Previously
Even if user explicitly selected it in columns settings, it wouldn't show. But you decided to remove that "dynamic" column settings here (i.e., columns will show when user selects it, regardless of data existence). That'd cause
Would you mind confirming if you want to keep |
|
@trjExpensify, could you please take a look at this comment, when you get a chance. thanks. |
|
I think it depends on what you mean by "dynamic". The behaviour we discovered over there was confusing whereby someone selected a column and it wouldn't show. What I think we should do here is:
@shawnborton what do you think? |
|
That sounds good to me, let's do it. |
|
@gijoe0295 I think the expected behavior is clarified now. Could you please take a look at this comment and confirm whether the PR looks good from your side? thanks |
|
🚧 cristipaval 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! 🧪🧪
|











Explanation of Change
Updates the expense report table defaults so the editable Amount column is shown by default instead of the read-only Total column. This also keeps the existing multi-currency behavior by showing Total only when a conversion exists, and updates custom-column handling so Comments stay positioned before the active money column.
Fixed Issues
$ #93014
PROPOSAL: #93014 (comment)
Tests
Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Not applicable
Android: mWeb Chrome
Not applicable
iOS: Native
Not applicable
iOS: mWeb Safari
Not applicable
MacOS: Chrome / Safari
Screen.Recording.2026-06-30.at.12.06.23.PM.mov