From 6d17019827586246f6be34710385461e3622a3f7 Mon Sep 17 00:00:00 2001 From: Eason Date: Mon, 8 Jul 2024 11:15:12 +0800 Subject: [PATCH] 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 --- .../data/top-selling-products-mock-data.js | 4 +- .../analytics/components/leaderboard/index.js | 56 +++++-- .../components/leaderboard/test/index.js | 149 +++++++++++++----- ...48-analytics-dashboard-currency-formatting | 4 + .../src/Admin/API/Leaderboards.php | 16 ++ .../woocommerce-admin/api/leaderboards.php | 3 +- 6 files changed, 180 insertions(+), 52 deletions(-) create mode 100644 plugins/woocommerce/changelog/fix-36748-analytics-dashboard-currency-formatting diff --git a/plugins/woocommerce-admin/client/analytics/components/leaderboard/data/top-selling-products-mock-data.js b/plugins/woocommerce-admin/client/analytics/components/leaderboard/data/top-selling-products-mock-data.js index 584b9903933..8f878b2e09b 100644 --- a/plugins/woocommerce-admin/client/analytics/components/leaderboard/data/top-selling-products-mock-data.js +++ b/plugins/woocommerce-admin/client/analytics/components/leaderboard/data/top-selling-products-mock-data.js @@ -23,8 +23,8 @@ And as such will require data layer logic for products to fully build the table export default [ { product_id: 20, - items_sold: 1000, - net_revenue: 999.99, + items_sold: 123456789, + net_revenue: 9876543.215, orders_count: 54, name: 'awesome shirt', _links: { diff --git a/plugins/woocommerce-admin/client/analytics/components/leaderboard/index.js b/plugins/woocommerce-admin/client/analytics/components/leaderboard/index.js index 7ee8b89db32..98e9bdbd9b3 100644 --- a/plugins/woocommerce-admin/client/analytics/components/leaderboard/index.js +++ b/plugins/woocommerce-admin/client/analytics/components/leaderboard/index.js @@ -14,6 +14,8 @@ import { getLeaderboard, SETTINGS_STORE_NAME, } from '@woocommerce/data'; +import { CurrencyContext } from '@woocommerce/currency'; +import { formatValue } from '@woocommerce/number'; import { Text } from '@woocommerce/experimental'; /** @@ -23,7 +25,46 @@ import ReportError from '../report-error'; import sanitizeHTML from '../../../lib/sanitize-html'; import './style.scss'; +const formattable = new Set( [ 'currency', 'number' ] ); + 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: ( +
+ ), + value: column.value, + }; + }; + getFormattedHeaders() { return this.props.headers.map( ( header, i ) => { return { @@ -38,18 +79,7 @@ export class Leaderboard extends Component { getFormattedRows() { return this.props.rows.map( ( row ) => { - return row.map( ( column ) => { - return { - display: ( -
- ), - value: column.value, - }; - } ); + return row.map( this.getFormattedColumn ); } ); } @@ -151,6 +181,8 @@ Leaderboard.defaultProps = { isRequesting: false, }; +Leaderboard.contextType = CurrencyContext; + export default compose( withSelect( ( select, props ) => { const { id, query, totalRows, filters } = props; diff --git a/plugins/woocommerce-admin/client/analytics/components/leaderboard/test/index.js b/plugins/woocommerce-admin/client/analytics/components/leaderboard/test/index.js index c5c9cacd89d..582da723b6d 100644 --- a/plugins/woocommerce-admin/client/analytics/components/leaderboard/test/index.js +++ b/plugins/woocommerce-admin/client/analytics/components/leaderboard/test/index.js @@ -1,18 +1,14 @@ /** * External dependencies */ -import { render } from '@testing-library/react'; -import { numberFormat } from '@woocommerce/number'; -import { CurrencyFactory } from '@woocommerce/currency'; +import { render, screen } from '@testing-library/react'; +import { CurrencyFactory, CurrencyContext } from '@woocommerce/currency'; /** * Internal dependencies */ import { Leaderboard } from '../'; import mockData from '../data/top-selling-products-mock-data'; -import { CURRENCY } from '~/utils/admin-settings'; - -const { formatAmount, formatDecimal } = CurrencyFactory( CURRENCY ); const headers = [ { @@ -42,23 +38,26 @@ const rows = mockData.map( ( row ) => { value: name, }, { - display: numberFormat( CURRENCY, itemsSold ), + display: itemsSold.toString(), value: itemsSold, + format: 'number', }, { - display: numberFormat( CURRENCY, ordersCount ), - value: ordersCount, + display: ordersCount.toString(), + value: ordersCount.toString(), + format: 'number', }, { - display: formatAmount( netRevenue ), - value: formatDecimal( netRevenue ), + display: `${ netRevenue }`, + value: netRevenue, + format: 'currency', }, ]; } ); describe( 'Leaderboard', () => { test( 'should render empty message when there are no rows', () => { - const { queryByText } = render( + render( { ); expect( - queryByText( 'No data recorded for the selected time period.' ) + screen.getByText( 'No data recorded for the selected time period.' ) ).toBeInTheDocument(); } ); test( 'should render the headers', () => { - const { queryByText } = render( - - ); - - 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( + render( { /> ); - 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 ); - expect( queryByText( 'awesome shirt' ) ).toBeInTheDocument(); - expect( queryByText( '1,000.00' ) ).toBeInTheDocument(); - expect( queryByText( '54.00' ) ).toBeInTheDocument(); - expect( queryByText( '$999.99' ) ).toBeInTheDocument(); + test( 'should render formatted data in the table', () => { + render( + + ); + + 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( + + + + ); + + 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( + ( { + 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(); } ); } ); diff --git a/plugins/woocommerce/changelog/fix-36748-analytics-dashboard-currency-formatting b/plugins/woocommerce/changelog/fix-36748-analytics-dashboard-currency-formatting new file mode 100644 index 00000000000..70dd09fea89 --- /dev/null +++ b/plugins/woocommerce/changelog/fix-36748-analytics-dashboard-currency-formatting @@ -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. diff --git a/plugins/woocommerce/src/Admin/API/Leaderboards.php b/plugins/woocommerce/src/Admin/API/Leaderboards.php index e859b834d69..f40d2470196 100644 --- a/plugins/woocommerce/src/Admin/API/Leaderboards.php +++ b/plugins/woocommerce/src/Admin/API/Leaderboards.php @@ -130,10 +130,12 @@ class Leaderboards extends \WC_REST_Data_Controller { array( 'display' => wc_admin_number_format( $coupon['orders_count'] ), 'value' => $coupon['orders_count'], + 'format' => 'number', ), array( 'display' => wc_price( $coupon['amount'] ), 'value' => $coupon['amount'], + 'format' => 'currency', ), ); } @@ -199,10 +201,12 @@ class Leaderboards extends \WC_REST_Data_Controller { array( 'display' => wc_admin_number_format( $category['items_sold'] ), 'value' => $category['items_sold'], + 'format' => 'number', ), array( 'display' => wc_price( $category['net_revenue'] ), 'value' => $category['net_revenue'], + 'format' => 'currency', ), ); } @@ -266,10 +270,12 @@ class Leaderboards extends \WC_REST_Data_Controller { array( 'display' => wc_admin_number_format( $customer['orders_count'] ), 'value' => $customer['orders_count'], + 'format' => 'number', ), array( 'display' => wc_price( $customer['total_spend'] ), 'value' => $customer['total_spend'], + 'format' => 'currency', ), ); } @@ -335,10 +341,12 @@ class Leaderboards extends \WC_REST_Data_Controller { array( 'display' => wc_admin_number_format( $product['items_sold'] ), 'value' => $product['items_sold'], + 'format' => 'number', ), array( 'display' => wc_price( $product['net_revenue'] ), 'value' => $product['net_revenue'], + 'format' => 'currency', ), ); } @@ -578,6 +586,14 @@ class Leaderboards extends \WC_REST_Data_Controller { 'context' => array( 'view', 'edit' ), 'readonly' => true, ), + 'format' => array( + 'description' => __( 'Table cell format.', 'woocommerce' ), + 'type' => 'string', + 'context' => array( 'view' ), + 'enum' => array( 'currency', 'number' ), + 'readonly' => true, + 'required' => false, + ), ), ), ), diff --git a/plugins/woocommerce/tests/legacy/unit-tests/woocommerce-admin/api/leaderboards.php b/plugins/woocommerce/tests/legacy/unit-tests/woocommerce-admin/api/leaderboards.php index 9018ffb3463..30eada7f88d 100644 --- a/plugins/woocommerce/tests/legacy/unit-tests/woocommerce-admin/api/leaderboards.php +++ b/plugins/woocommerce/tests/legacy/unit-tests/woocommerce-admin/api/leaderboards.php @@ -140,9 +140,10 @@ class WC_Admin_Tests_API_Leaderboards extends WC_REST_Unit_Test_Case { $this->assertArrayHasKey( 'label', $header_properties ); $row_properties = $schema['rows']['items']['properties']; - $this->assertCount( 2, $row_properties ); + $this->assertCount( 3, $row_properties ); $this->assertArrayHasKey( 'display', $row_properties ); $this->assertArrayHasKey( 'value', $row_properties ); + $this->assertArrayHasKey( 'format', $row_properties ); } /**