Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/libs/SearchUIUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5891,9 +5891,9 @@ function getColumnsToShow({
[CONST.SEARCH.TABLE_COLUMNS.ORIGINAL_AMOUNT]: false,
[CONST.SEARCH.TABLE_COLUMNS.REIMBURSABLE]: shouldShowReimbursableColumn,
[CONST.SEARCH.TABLE_COLUMNS.BILLABLE]: shouldShowBillableColumn,
[CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT]: false,
[CONST.SEARCH.TABLE_COLUMNS.COMMENTS]: shouldShowCommentsColumn,
[CONST.SEARCH.TABLE_COLUMNS.TOTAL]: true,
[CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT]: true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you reorder the columns here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

App/src/libs/SearchUIUtils.ts

Lines 5927 to 5934 in c8d6035

if (shouldShowCommentsColumn && !addedColumns.has(CONST.SEARCH.TABLE_COLUMNS.COMMENTS)) {
const totalIndex = result.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
if (totalIndex === -1) {
result.push(CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
} else {
result.splice(totalIndex, 0, CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Image Image

[CONST.SEARCH.TABLE_COLUMNS.TOTAL]: false,
}
: {
[CONST.SEARCH.TABLE_COLUMNS.AVATAR]: true,
Expand Down Expand Up @@ -5961,11 +5961,11 @@ function getColumnsToShow({
}

if (shouldShowCommentsColumn && !addedColumns.has(CONST.SEARCH.TABLE_COLUMNS.COMMENTS)) {
const totalIndex = result.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
if (totalIndex === -1) {
const totalAmountIndex = result.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT);
if (totalAmountIndex === -1) {
result.push(CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
} else {
result.splice(totalIndex, 0, CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
result.splice(totalAmountIndex, 0, CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
}
}

Expand Down Expand Up @@ -6065,7 +6065,7 @@ function getColumnsToShow({
columns[CONST.SEARCH.TABLE_COLUMNS.EXCHANGE_RATE] = true;
}
if (hasExchangeRate && isExpenseReportView) {
columns[CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT] = true;
columns[CONST.SEARCH.TABLE_COLUMNS.TOTAL] = true;
}

if (!Array.isArray(data)) {
Expand Down
4 changes: 2 additions & 2 deletions src/pages/settings/Report/ReportDetailsColumnsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const REPORT_DETAILS_DEFAULT_COLUMNS: SearchCustomColumnIds[] = [
CONST.SEARCH.TABLE_COLUMNS.MERCHANT,
CONST.SEARCH.TABLE_COLUMNS.CATEGORY,
CONST.SEARCH.TABLE_COLUMNS.TAG,
CONST.SEARCH.TABLE_COLUMNS.TOTAL,
CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT,
];

function ReportDetailsColumnsPage() {
Expand Down Expand Up @@ -95,7 +95,7 @@ function ReportDetailsColumnsPage() {
return visibleColumns.filter((col) => allTypeCustomColumns.includes(col as SearchCustomColumnIds)) as SearchCustomColumnIds[];
}, [reportDetailsColumns, reportTransactions, currentUserDetails?.accountID, report, policy, allTypeCustomColumns]);

const requiredColumns = new Set<SearchCustomColumnIds>([CONST.SEARCH.TABLE_COLUMNS.TOTAL]);
const requiredColumns = new Set<SearchCustomColumnIds>([CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT]);

const handleSave = (selectedColumnIds: SearchCustomColumnIds[]) => {
// Skip saving if columns haven't changed from the effective state, to avoid
Expand Down
76 changes: 61 additions & 15 deletions tests/unit/Search/SearchUIUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9238,7 +9238,7 @@ describe('SearchUIUtils', () => {
// Column order: after Tag and before Amount (and before Comments)
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TAG)).toBeLessThan(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.REIMBURSABLE));
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.REIMBURSABLE)).toBeLessThan(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.BILLABLE));
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.BILLABLE)).toBeLessThan(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL));
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.BILLABLE)).toBeLessThan(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT));
});

test('Should not include Reimbursable column in expense report view when all expenses are reimbursable', () => {
Expand Down Expand Up @@ -9303,16 +9303,19 @@ describe('SearchUIUtils', () => {
expect(columns).not.toContain(CONST.SEARCH.TABLE_COLUMNS.TAG);
});

test('Should place COMMENTS right before TOTAL in expense report view with custom columns', () => {
test('Should place COMMENTS right before AMOUNT in expense report view with custom columns', () => {
const baseTransaction = searchResults.data[`transactions_${transactionID}`];
const testTransaction = {
...baseTransaction,
transactionID: 'test',
merchant: 'Test Merchant',
groupExchangeRate: undefined,
currencyConversionRate: undefined,
convertedAmount: undefined,
};

// Use custom columns to trigger the custom columns path
const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.TOTAL];
const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT];
const columns = SearchUIUtils.getColumnsToShow({
currentAccountID: submitterAccountID,
data: [testTransaction],
Expand All @@ -9322,17 +9325,20 @@ describe('SearchUIUtils', () => {
});

expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
// TOTAL should be at the end, with COMMENTS immediately before it
// AMOUNT should be at the end, with COMMENTS immediately before it
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.COMMENTS)).toBe(columns.length - 2);
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL)).toBe(columns.length - 1);
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT)).toBe(columns.length - 1);
});

test('Should place COMMENTS right before TOTAL in default expense report view', () => {
test('Should place COMMENTS right before AMOUNT in default expense report view', () => {
const baseTransaction = searchResults.data[`transactions_${transactionID}`];
const testTransaction = {
...baseTransaction,
transactionID: 'test',
merchant: 'Test Merchant',
groupExchangeRate: undefined,
currencyConversionRate: undefined,
convertedAmount: undefined,
};

const columns = SearchUIUtils.getColumnsToShow({
Expand All @@ -9343,7 +9349,7 @@ describe('SearchUIUtils', () => {
});

expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.COMMENTS)).toBe(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL) - 1);
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.COMMENTS)).toBe(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT) - 1);
});

test('Should not duplicate COMMENTS column when already in visible columns', () => {
Expand Down Expand Up @@ -9384,7 +9390,7 @@ describe('SearchUIUtils', () => {

// EXCHANGE_RATE is selected, so it shows even when no transaction has exchange rate data
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.EXCHANGE_RATE);
// Always-shown columns should still be present
// Selected custom columns should still be present
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.DATE);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
});
Expand Down Expand Up @@ -9635,23 +9641,42 @@ describe('SearchUIUtils', () => {
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TAX_AMOUNT);
});

test('Should show empty AMOUNT column in expense report view when selected and no conversion', () => {
test('Should show AMOUNT by default and hide TOTAL in expense report view when no conversion', () => {
const baseTransaction = searchResults.data[`transactions_${transactionID}`];
const testTransaction = {
...baseTransaction,
transactionID: 'test',
merchant: 'Test Merchant',
groupExchangeRate: undefined,
currencyConversionRate: undefined,
convertedAmount: undefined,
};

const columns = SearchUIUtils.getColumnsToShow({currentAccountID: submitterAccountID, data: [testTransaction], visibleColumns: [], isExpenseReportView: true});

expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT);
expect(columns).not.toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
});

test('Should show selected TOTAL from custom expense report columns when there is no conversion', () => {
const baseTransaction = searchResults.data[`transactions_${transactionID}`];
const testTransaction = {
...baseTransaction,
transactionID: 'test',
merchant: 'Test Merchant',
groupExchangeRate: undefined,
currencyConversionRate: undefined,
convertedAmount: undefined,
};

const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.TOTAL, CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT];
const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT, CONST.SEARCH.TABLE_COLUMNS.TOTAL];
const columns = SearchUIUtils.getColumnsToShow({currentAccountID: submitterAccountID, data: [testTransaction], visibleColumns, isExpenseReportView: true});

// TOTAL_AMOUNT is selected, so it shows even when no transaction has a conversion
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
});

test('Should show AMOUNT column when transaction has a real conversion (currencies differ)', () => {
test('Should show TOTAL from custom expense report columns when there is a conversion', () => {
const baseTransaction = searchResults.data[`transactions_${transactionID}`];
const testTransaction = {
...baseTransaction,
Expand All @@ -9663,10 +9688,31 @@ describe('SearchUIUtils', () => {
convertedAmount: 2500,
};

const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.TOTAL, CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT];
const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT, CONST.SEARCH.TABLE_COLUMNS.TOTAL];
const columns = SearchUIUtils.getColumnsToShow({currentAccountID: submitterAccountID, data: [testTransaction], visibleColumns, isExpenseReportView: true});

expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT)).toBeLessThan(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL));
});

test('Should show AMOUNT and TOTAL by default when transaction has a real conversion (currencies differ)', () => {
const baseTransaction = searchResults.data[`transactions_${transactionID}`];
const testTransaction = {
...baseTransaction,
transactionID: 'test',
merchant: 'Test Merchant',
currency: 'EUR',
groupCurrency: 'USD',
groupExchangeRate: 1.1,
convertedAmount: 2500,
};

const columns = SearchUIUtils.getColumnsToShow({currentAccountID: submitterAccountID, data: [testTransaction], visibleColumns: [], isExpenseReportView: true});

expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
expect(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT)).toBeLessThan(columns.indexOf(CONST.SEARCH.TABLE_COLUMNS.TOTAL));
});

test('Should show selected columns regardless of whether any transaction has data for them', () => {
Expand Down Expand Up @@ -9705,7 +9751,7 @@ describe('SearchUIUtils', () => {
modifiedMerchant: '',
};

const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.RECEIPT, CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.MERCHANT, CONST.SEARCH.TABLE_COLUMNS.TOTAL];
const visibleColumns = [CONST.SEARCH.TABLE_COLUMNS.RECEIPT, CONST.SEARCH.TABLE_COLUMNS.DATE, CONST.SEARCH.TABLE_COLUMNS.MERCHANT, CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT];
const columns = SearchUIUtils.getColumnsToShow({
currentAccountID: submitterAccountID,
data: [testTransaction],
Expand All @@ -9717,7 +9763,7 @@ describe('SearchUIUtils', () => {
// Always-shown columns should be present
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.RECEIPT);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.DATE);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TOTAL_AMOUNT);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.TYPE);
expect(columns).toContain(CONST.SEARCH.TABLE_COLUMNS.COMMENTS);
// MERCHANT is selected, so it shows even with no data
Expand Down
Loading