Fix the inconsistent formatting of the numeric columns in the Leaderboards on the Analytics > Dashboard page (#49097)

* Add the `format` property to the items of the `rows` array in Analytics' Leaderboards API response to indicate how it's formatted.

* Make the `Leaderboard` component within Analytics aware of the currency context and format the numeric columns accordingly.

* Avoid a warning from the test of `Leaderboard` component when running JS tests.

File: client/analytics/components/leaderboard/test/index.js
Ref: https://github.com/woocommerce/woocommerce/blob/9.0.2/packages/js/components/src/pagination/pagination.tsx#L44

* Apply better practice to the test of `Leaderboard` component.

File: client/analytics/components/leaderboard/test/index.js

* Add changelog.

* Make the `Leaderboard` component not create a new function for every `row` rendering.

Address: https://github.com/woocommerce/woocommerce/pull/49097#discussion_r1667009910
This commit is contained in:
Eason 2024-07-08 11:15:12 +08:00 committed by GitHub
parent e62f28b3ca
commit 6d17019827
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 180 additions and 52 deletions

View File

@ -23,8 +23,8 @@ And as such will require data layer logic for products to fully build the table
export default [ export default [
{ {
product_id: 20, product_id: 20,
items_sold: 1000, items_sold: 123456789,
net_revenue: 999.99, net_revenue: 9876543.215,
orders_count: 54, orders_count: 54,
name: 'awesome shirt', name: 'awesome shirt',
_links: { _links: {

View File

@ -14,6 +14,8 @@ import {
getLeaderboard, getLeaderboard,
SETTINGS_STORE_NAME, SETTINGS_STORE_NAME,
} from '@woocommerce/data'; } from '@woocommerce/data';
import { CurrencyContext } from '@woocommerce/currency';
import { formatValue } from '@woocommerce/number';
import { Text } from '@woocommerce/experimental'; import { Text } from '@woocommerce/experimental';
/** /**
@ -23,7 +25,46 @@ import ReportError from '../report-error';
import sanitizeHTML from '../../../lib/sanitize-html'; import sanitizeHTML from '../../../lib/sanitize-html';
import './style.scss'; import './style.scss';
const formattable = new Set( [ 'currency', 'number' ] );
export class Leaderboard extends Component { export class Leaderboard extends Component {
getFormattedColumn = ( column ) => {
const { format } = column;
/*
* The format property is used for numeric columns and is optional.
* The `value` property type is specified as string in the API schema
* and it's extensible from other extensions. Therefore, even if the
* actual type of numeric columns returned by WooCoomerce's own API is
* number, there is no guarantee the value will be a number.
*/
if ( formattable.has( column.format ) && isFinite( column.value ) ) {
const value = parseFloat( column.value );
if ( ! Number.isNaN( value ) ) {
const { formatAmount, getCurrencyConfig } = this.context;
const display =
format === 'currency'
? formatAmount( value )
: formatValue( getCurrencyConfig(), format, value );
return {
display,
value,
};
}
}
return {
display: (
<div
dangerouslySetInnerHTML={ sanitizeHTML( column.display ) }
/>
),
value: column.value,
};
};
getFormattedHeaders() { getFormattedHeaders() {
return this.props.headers.map( ( header, i ) => { return this.props.headers.map( ( header, i ) => {
return { return {
@ -38,18 +79,7 @@ export class Leaderboard extends Component {
getFormattedRows() { getFormattedRows() {
return this.props.rows.map( ( row ) => { return this.props.rows.map( ( row ) => {
return row.map( ( column ) => { return row.map( this.getFormattedColumn );
return {
display: (
<div
dangerouslySetInnerHTML={ sanitizeHTML(
column.display
) }
/>
),
value: column.value,
};
} );
} ); } );
} }
@ -151,6 +181,8 @@ Leaderboard.defaultProps = {
isRequesting: false, isRequesting: false,
}; };
Leaderboard.contextType = CurrencyContext;
export default compose( export default compose(
withSelect( ( select, props ) => { withSelect( ( select, props ) => {
const { id, query, totalRows, filters } = props; const { id, query, totalRows, filters } = props;

View File

@ -1,18 +1,14 @@
/** /**
* External dependencies * External dependencies
*/ */
import { render } from '@testing-library/react'; import { render, screen } from '@testing-library/react';
import { numberFormat } from '@woocommerce/number'; import { CurrencyFactory, CurrencyContext } from '@woocommerce/currency';
import { CurrencyFactory } from '@woocommerce/currency';
/** /**
* Internal dependencies * Internal dependencies
*/ */
import { Leaderboard } from '../'; import { Leaderboard } from '../';
import mockData from '../data/top-selling-products-mock-data'; import mockData from '../data/top-selling-products-mock-data';
import { CURRENCY } from '~/utils/admin-settings';
const { formatAmount, formatDecimal } = CurrencyFactory( CURRENCY );
const headers = [ const headers = [
{ {
@ -42,23 +38,26 @@ const rows = mockData.map( ( row ) => {
value: name, value: name,
}, },
{ {
display: numberFormat( CURRENCY, itemsSold ), display: itemsSold.toString(),
value: itemsSold, value: itemsSold,
format: 'number',
}, },
{ {
display: numberFormat( CURRENCY, ordersCount ), display: ordersCount.toString(),
value: ordersCount, value: ordersCount.toString(),
format: 'number',
}, },
{ {
display: formatAmount( netRevenue ), display: `<span class="woocommerce-Price-currencySymbol">${ netRevenue }</span>`,
value: formatDecimal( netRevenue ), value: netRevenue,
format: 'currency',
}, },
]; ];
} ); } );
describe( 'Leaderboard', () => { describe( 'Leaderboard', () => {
test( 'should render empty message when there are no rows', () => { test( 'should render empty message when there are no rows', () => {
const { queryByText } = render( render(
<Leaderboard <Leaderboard
id="products" id="products"
title={ '' } title={ '' }
@ -69,29 +68,12 @@ describe( 'Leaderboard', () => {
); );
expect( expect(
queryByText( 'No data recorded for the selected time period.' ) screen.getByText( 'No data recorded for the selected time period.' )
).toBeInTheDocument(); ).toBeInTheDocument();
} ); } );
test( 'should render the headers', () => { test( 'should render the headers', () => {
const { queryByText } = render( render(
<Leaderboard
id="products"
title={ '' }
headers={ headers }
rows={ rows }
totalRows={ 0 }
/>
);
expect( queryByText( 'Name' ) ).toBeInTheDocument();
expect( queryByText( 'Items sold' ) ).toBeInTheDocument();
expect( queryByText( 'Orders' ) ).toBeInTheDocument();
expect( queryByText( 'Net sales' ) ).toBeInTheDocument();
} );
test( 'should render formatted data in the table', () => {
const { container, queryByText } = render(
<Leaderboard <Leaderboard
id="products" id="products"
title={ '' } title={ '' }
@ -101,12 +83,105 @@ describe( 'Leaderboard', () => {
/> />
); );
const tableRows = container.querySelectorAll( 'tr' ); expect( screen.getByText( 'Name' ) ).toBeInTheDocument();
expect( screen.getByText( 'Items sold' ) ).toBeInTheDocument();
expect( screen.getByText( 'Orders' ) ).toBeInTheDocument();
expect( screen.getByText( 'Net sales' ) ).toBeInTheDocument();
} );
expect( tableRows.length ).toBe( 6 ); test( 'should render formatted data in the table', () => {
expect( queryByText( 'awesome shirt' ) ).toBeInTheDocument(); render(
expect( queryByText( '1,000.00' ) ).toBeInTheDocument(); <Leaderboard
expect( queryByText( '54.00' ) ).toBeInTheDocument(); id="products"
expect( queryByText( '$999.99' ) ).toBeInTheDocument(); title={ '' }
headers={ headers }
rows={ rows }
totalRows={ 5 }
/>
);
expect( screen.getAllByRole( 'row' ) ).toHaveLength( 6 );
expect( screen.getByText( 'awesome shirt' ) ).toBeInTheDocument();
expect( screen.getByText( '123,456,789' ) ).toBeInTheDocument();
expect( screen.getByText( '54' ) ).toBeInTheDocument();
expect( screen.getByText( '$9,876,543.22' ) ).toBeInTheDocument();
} );
test( 'should format data according to the currency context', () => {
const currencySetting = {
code: 'PLN',
decimalSeparator: ',',
precision: 3,
priceFormat: '%1$s %2$s',
symbol: 'zł',
thousandSeparator: '.',
};
render(
<CurrencyContext.Provider
value={ new CurrencyFactory( currencySetting ) }
>
<Leaderboard
id="products"
title={ '' }
headers={ headers }
rows={ rows }
totalRows={ 5 }
/>
</CurrencyContext.Provider>
);
expect( screen.getByText( 'awesome shirt' ) ).toBeInTheDocument();
expect( screen.getByText( '123.456.789' ) ).toBeInTheDocument();
expect( screen.getByText( '54' ) ).toBeInTheDocument();
expect( screen.getByText( 'zł 9.876.543,215' ) ).toBeInTheDocument();
} );
test( `should not format data that is not specified in a format or doesn't conform to a number value`, () => {
const columns = [
{
display: 'awesome shirt',
value: '$123.456', // Not a pure numeric string
format: 'currency',
},
{
display: 'awesome pants',
value: '123,456', // Not a pure numeric string
format: 'number',
},
{
display: 'awesome hat',
value: '', // Not a number
format: 'number',
},
{
display: 'awesome sticker',
value: 123,
format: 'product', // Invalid format
},
{
// Not specified format
display: 'awesome button',
value: 123,
},
];
render(
<Leaderboard
id="products"
title={ '' }
headers={ columns.map( ( _, i ) => ( {
label: i.toString(),
} ) ) }
rows={ [ columns ] }
totalRows={ 5 }
/>
);
expect( screen.getByText( 'awesome shirt' ) ).toBeInTheDocument();
expect( screen.getByText( 'awesome pants' ) ).toBeInTheDocument();
expect( screen.getByText( 'awesome hat' ) ).toBeInTheDocument();
expect( screen.getByText( 'awesome sticker' ) ).toBeInTheDocument();
expect( screen.getByText( 'awesome button' ) ).toBeInTheDocument();
} ); } );
} ); } );

View File

@ -0,0 +1,4 @@
Significance: patch
Type: fix
Make the Leaderboards on the Analytics > Dashboard page use consistent currency and number formatting across the page, and perceive the currency setting comes from the relevant filter.

View File

@ -130,10 +130,12 @@ class Leaderboards extends \WC_REST_Data_Controller {
array( array(
'display' => wc_admin_number_format( $coupon['orders_count'] ), 'display' => wc_admin_number_format( $coupon['orders_count'] ),
'value' => $coupon['orders_count'], 'value' => $coupon['orders_count'],
'format' => 'number',
), ),
array( array(
'display' => wc_price( $coupon['amount'] ), 'display' => wc_price( $coupon['amount'] ),
'value' => $coupon['amount'], 'value' => $coupon['amount'],
'format' => 'currency',
), ),
); );
} }
@ -199,10 +201,12 @@ class Leaderboards extends \WC_REST_Data_Controller {
array( array(
'display' => wc_admin_number_format( $category['items_sold'] ), 'display' => wc_admin_number_format( $category['items_sold'] ),
'value' => $category['items_sold'], 'value' => $category['items_sold'],
'format' => 'number',
), ),
array( array(
'display' => wc_price( $category['net_revenue'] ), 'display' => wc_price( $category['net_revenue'] ),
'value' => $category['net_revenue'], 'value' => $category['net_revenue'],
'format' => 'currency',
), ),
); );
} }
@ -266,10 +270,12 @@ class Leaderboards extends \WC_REST_Data_Controller {
array( array(
'display' => wc_admin_number_format( $customer['orders_count'] ), 'display' => wc_admin_number_format( $customer['orders_count'] ),
'value' => $customer['orders_count'], 'value' => $customer['orders_count'],
'format' => 'number',
), ),
array( array(
'display' => wc_price( $customer['total_spend'] ), 'display' => wc_price( $customer['total_spend'] ),
'value' => $customer['total_spend'], 'value' => $customer['total_spend'],
'format' => 'currency',
), ),
); );
} }
@ -335,10 +341,12 @@ class Leaderboards extends \WC_REST_Data_Controller {
array( array(
'display' => wc_admin_number_format( $product['items_sold'] ), 'display' => wc_admin_number_format( $product['items_sold'] ),
'value' => $product['items_sold'], 'value' => $product['items_sold'],
'format' => 'number',
), ),
array( array(
'display' => wc_price( $product['net_revenue'] ), 'display' => wc_price( $product['net_revenue'] ),
'value' => $product['net_revenue'], 'value' => $product['net_revenue'],
'format' => 'currency',
), ),
); );
} }
@ -578,6 +586,14 @@ class Leaderboards extends \WC_REST_Data_Controller {
'context' => array( 'view', 'edit' ), 'context' => array( 'view', 'edit' ),
'readonly' => true, 'readonly' => true,
), ),
'format' => array(
'description' => __( 'Table cell format.', 'woocommerce' ),
'type' => 'string',
'context' => array( 'view' ),
'enum' => array( 'currency', 'number' ),
'readonly' => true,
'required' => false,
),
), ),
), ),
), ),

View File

@ -140,9 +140,10 @@ class WC_Admin_Tests_API_Leaderboards extends WC_REST_Unit_Test_Case {
$this->assertArrayHasKey( 'label', $header_properties ); $this->assertArrayHasKey( 'label', $header_properties );
$row_properties = $schema['rows']['items']['properties']; $row_properties = $schema['rows']['items']['properties'];
$this->assertCount( 2, $row_properties ); $this->assertCount( 3, $row_properties );
$this->assertArrayHasKey( 'display', $row_properties ); $this->assertArrayHasKey( 'display', $row_properties );
$this->assertArrayHasKey( 'value', $row_properties ); $this->assertArrayHasKey( 'value', $row_properties );
$this->assertArrayHasKey( 'format', $row_properties );
} }
/** /**