From 31804aeba36e55c79910889ad92d21146308b960 Mon Sep 17 00:00:00 2001 From: Joel Thiessen <444632+joelclimbsthings@users.noreply.github.com> Date: Mon, 26 Jul 2021 12:11:44 -0700 Subject: [PATCH] Refactoring report table withSelect to fix issues with the table data populating correctly (https://github.com/woocommerce/woocommerce-admin/pull/7355) --- plugins/woocommerce-admin/changelogs/fix-6820 | 4 +++ .../components/report-chart/index.js | 8 +++-- .../components/report-table/index.js | 33 +++++++++-------- .../components/report-table/utils.js | 14 ++++---- .../client/analytics/report/index.js | 7 ++-- .../client/analytics/report/products/index.js | 8 +++-- .../client/analytics/report/products/table.js | 9 +++-- .../packages/data/src/items/utils.js | 6 ++-- .../packages/data/src/reports/utils.js | 35 ++++++++++++------- 9 files changed, 78 insertions(+), 46 deletions(-) create mode 100644 plugins/woocommerce-admin/changelogs/fix-6820 diff --git a/plugins/woocommerce-admin/changelogs/fix-6820 b/plugins/woocommerce-admin/changelogs/fix-6820 new file mode 100644 index 00000000000..060dfe5f89e --- /dev/null +++ b/plugins/woocommerce-admin/changelogs/fix-6820 @@ -0,0 +1,4 @@ +Significance: patch +Type: Fix + +Fixing issues with ReportTable component data not populating correctly #7355 diff --git a/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js b/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js index 542fd9c72b0..f5f8ab33495 100644 --- a/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js +++ b/plugins/woocommerce-admin/client/analytics/components/report-chart/index.js @@ -13,6 +13,7 @@ import { getReportChartData, getTooltipValueFormat, SETTINGS_STORE_NAME, + REPORTS_STORE_NAME, } from '@woocommerce/data'; import { getAllowedIntervalsForQuery, @@ -360,6 +361,9 @@ export default compose( SETTINGS_STORE_NAME ).getSetting( 'wc_admin', 'wcAdminSettings' ); + /* eslint @wordpress/no-unused-vars-before-return: "off" */ + const reportStoreSelector = select( REPORTS_STORE_NAME ); + const newProps = { mode: chartMode, filterParam, @@ -387,7 +391,7 @@ export default compose( endpoint, dataType: 'primary', query, - select, + selector: reportStoreSelector, limitBy, filters, advancedFilters, @@ -406,7 +410,7 @@ export default compose( endpoint, dataType: 'secondary', query, - select, + selector: reportStoreSelector, limitBy, filters, advancedFilters, diff --git a/plugins/woocommerce-admin/client/analytics/components/report-table/index.js b/plugins/woocommerce-admin/client/analytics/components/report-table/index.js index 8214f877770..55cc8be80b1 100644 --- a/plugins/woocommerce-admin/client/analytics/components/report-table/index.js +++ b/plugins/woocommerce-admin/client/analytics/components/report-table/index.js @@ -27,6 +27,7 @@ import { getReportTableData, EXPORT_STORE_NAME, SETTINGS_STORE_NAME, + REPORTS_STORE_NAME, useUserPreferences, QUERY_DEFAULTS, } from '@woocommerce/data'; @@ -583,27 +584,32 @@ export default compose( filters, advancedFilters, summaryFields, + extendedItemsStoreName, } = props; - if ( - isRequesting || - ( query.search && - ! ( query[ endpoint ] && query[ endpoint ].length ) ) - ) { - return EMPTY_OBJECT; - } + /* eslint @wordpress/no-unused-vars-before-return: "off" */ + const reportStoreSelector = select( REPORTS_STORE_NAME ); + + const extendedStoreSelector = extendedItemsStoreName + ? select( extendedItemsStoreName ) + : null; + const { woocommerce_default_date_range: defaultDateRange } = select( SETTINGS_STORE_NAME ).getSetting( 'wc_admin', 'wcAdminSettings' ); + if ( isRequesting ) { + return EMPTY_OBJECT; + } + // Category charts are powered by the /reports/products/stats endpoint. const chartEndpoint = endpoint === 'categories' ? 'products' : endpoint; const primaryData = getSummary ? getReportChartData( { endpoint: chartEndpoint, + selector: reportStoreSelector, dataType: 'primary', query, - select, filters, advancedFilters, defaultDateRange, @@ -615,17 +621,16 @@ export default compose( getReportTableData( { endpoint, query, - select, + selector: reportStoreSelector, tableQuery, filters, advancedFilters, defaultDateRange, } ); - const extendedTableData = extendTableData( - select, - props, - queriedTableData - ); + + const extendedTableData = extendedStoreSelector + ? extendTableData( extendedStoreSelector, props, queriedTableData ) + : queriedTableData; return { primaryData, diff --git a/plugins/woocommerce-admin/client/analytics/components/report-table/utils.js b/plugins/woocommerce-admin/client/analytics/components/report-table/utils.js index c5a311db83b..86e0f0dae13 100644 --- a/plugins/woocommerce-admin/client/analytics/components/report-table/utils.js +++ b/plugins/woocommerce-admin/client/analytics/components/report-table/utils.js @@ -3,12 +3,12 @@ */ import { first } from 'lodash'; -export function extendTableData( select, props, queriedTableData ) { - const { - extendItemsMethodNames, - extendedItemsStoreName, - itemIdField, - } = props; +export function extendTableData( + extendedStoreSelector, + props, + queriedTableData +) { + const { extendItemsMethodNames, itemIdField } = props; const itemsData = queriedTableData.items.data; if ( ! Array.isArray( itemsData ) || @@ -23,7 +23,7 @@ export function extendTableData( select, props, queriedTableData ) { [ extendItemsMethodNames.getError ]: getErrorMethod, [ extendItemsMethodNames.isRequesting ]: isRequestingMethod, [ extendItemsMethodNames.load ]: loadMethod, - } = select( extendedItemsStoreName ); + } = extendedStoreSelector; const extendQuery = { include: itemsData.map( ( item ) => item[ itemIdField ] ).join( ',' ), per_page: itemsData.length, diff --git a/plugins/woocommerce-admin/client/analytics/report/index.js b/plugins/woocommerce-admin/client/analytics/report/index.js index 84483545217..683e9696971 100644 --- a/plugins/woocommerce-admin/client/analytics/report/index.js +++ b/plugins/woocommerce-admin/client/analytics/report/index.js @@ -7,7 +7,7 @@ import { withSelect } from '@wordpress/data'; import PropTypes from 'prop-types'; import { find } from 'lodash'; import { getQuery, getSearchWords } from '@woocommerce/navigation'; -import { searchItemsByString } from '@woocommerce/data'; +import { searchItemsByString, ITEMS_STORE_NAME } from '@woocommerce/data'; /** * Internal dependencies @@ -89,6 +89,9 @@ export default compose( const query = getQuery(); const { search } = query; + /* eslint @wordpress/no-unused-vars-before-return: "off" */ + const itemsSelector = select( ITEMS_STORE_NAME ); + if ( ! search ) { return {}; } @@ -101,7 +104,7 @@ export default compose( ? 'products' : report; const itemsResult = searchItemsByString( - select, + itemsSelector, mappedReport, searchWords ); diff --git a/plugins/woocommerce-admin/client/analytics/report/products/index.js b/plugins/woocommerce-admin/client/analytics/report/products/index.js index c2373abdd18..f943e92745a 100644 --- a/plugins/woocommerce-admin/client/analytics/report/products/index.js +++ b/plugins/woocommerce-admin/client/analytics/report/products/index.js @@ -142,6 +142,11 @@ export default compose( ! query.search && query.products && query.products.split( ',' ).length === 1; + + const { getItems, isResolving, getItemsError } = select( + ITEMS_STORE_NAME + ); + if ( isRequesting ) { return { query: { @@ -152,9 +157,6 @@ export default compose( }; } - const { getItems, isResolving, getItemsError } = select( - ITEMS_STORE_NAME - ); if ( isSingleProductView ) { const productId = parseInt( query.products, 10 ); const includeArgs = { include: productId }; diff --git a/plugins/woocommerce-admin/client/analytics/report/products/table.js b/plugins/woocommerce-admin/client/analytics/report/products/table.js index 4dbffffa82e..1991a80affc 100644 --- a/plugins/woocommerce-admin/client/analytics/report/products/table.js +++ b/plugins/woocommerce-admin/client/analytics/report/products/table.js @@ -145,6 +145,7 @@ class ProductsReportTable extends Component { const productCategories = ( categoryIds && + categories && categoryIds .map( ( categoryId ) => categories.get( categoryId ) ) .filter( Boolean ) ) || @@ -375,6 +376,11 @@ ProductsReportTable.contextType = CurrencyContext; export default compose( withSelect( ( select, props ) => { const { query, isRequesting } = props; + + const { getItems, getItemsError, isResolving } = select( + ITEMS_STORE_NAME + ); + if ( isRequesting || ( query.search && ! ( query.products && query.products.length ) ) @@ -382,9 +388,6 @@ export default compose( return {}; } - const { getItems, getItemsError, isResolving } = select( - ITEMS_STORE_NAME - ); const tableQuery = { per_page: -1, }; diff --git a/plugins/woocommerce-admin/packages/data/src/items/utils.js b/plugins/woocommerce-admin/packages/data/src/items/utils.js index d0576453eda..bd21c284c9b 100644 --- a/plugins/woocommerce-admin/packages/data/src/items/utils.js +++ b/plugins/woocommerce-admin/packages/data/src/items/utils.js @@ -63,13 +63,13 @@ export function getLeaderboard( options ) { /** * Returns items based on a search query. * - * @param {Object} select Instance of @wordpress/select + * @param {Object} selector Instance of @wordpress/select response * @param {string} endpoint Report API Endpoint * @param {string[]} search Array of search strings. * @return {Object} Object containing API request information and the matching items. */ -export function searchItemsByString( select, endpoint, search ) { - const { getItems, getItemsError, isResolving } = select( STORE_NAME ); +export function searchItemsByString( selector, endpoint, search ) { + const { getItems, getItemsError, isResolving } = selector; const items = {}; let isRequesting = false; diff --git a/plugins/woocommerce-admin/packages/data/src/reports/utils.js b/plugins/woocommerce-admin/packages/data/src/reports/utils.js index 90ffd42b2e6..1092c022043 100644 --- a/plugins/woocommerce-admin/packages/data/src/reports/utils.js +++ b/plugins/woocommerce-admin/packages/data/src/reports/utils.js @@ -342,16 +342,18 @@ const getReportChartDataResponse = memoize( * @param {string} options.endpoint Report API Endpoint * @param {string} options.dataType 'primary' or 'secondary' * @param {Object} options.query Query parameters in the url - * @param {Object} options.select Instance of @wordpress/select + * @param {Object} options.selector Instance of @wordpress/select response * @param {Array} options.limitBy Properties used to limit the results. It will be used in the API call to send the IDs. * @param {string} options.defaultDateRange User specified default date range. * @return {Object} Object containing API request information (response, fetching, and error details) */ export function getReportChartData( options ) { - const { endpoint, select } = options; - const { getReportStats, getReportStatsError, isResolving } = select( - STORE_NAME - ); + const { endpoint } = options; + const { + getReportStats, + getReportStatsError, + isResolving, + } = options.selector; const requestQuery = getRequestQuery( options ); // Disable eslint rule requiring `stats` to be defined below because the next two if statements @@ -489,16 +491,18 @@ export function getReportTableQuery( options ) { * @param {Object} options arguments * @param {string} options.endpoint Report API Endpoint * @param {Object} options.query Query parameters in the url - * @param {Object} options.select Instance of @wordpress/select + * @param {Object} options.selector Instance of @wordpress/select response * @param {Object} options.tableQuery Query parameters specific for that endpoint * @param {string} options.defaultDateRange User specified default date range. * @return {Object} Object Table data response */ export function getReportTableData( options ) { - const { endpoint, select } = options; - const { getReportItems, getReportItemsError, isResolving } = select( - STORE_NAME - ); + const { endpoint } = options; + const { + getReportItems, + getReportItemsError, + hasFinishedResolution, + } = options.selector; const tableQuery = reportsUtils.getReportTableQuery( options ); const response = { @@ -516,9 +520,16 @@ export function getReportTableData( options ) { // eslint-disable-next-line @wordpress/no-unused-vars-before-return const items = getReportItems( endpoint, tableQuery ); - if ( isResolving( 'getReportItems', [ endpoint, tableQuery ] ) ) { + const queryResolved = hasFinishedResolution( 'getReportItems', [ + endpoint, + tableQuery, + ] ); + + if ( ! queryResolved ) { return { ...response, isRequesting: true }; - } else if ( getReportItemsError( endpoint, tableQuery ) ) { + } + + if ( getReportItemsError( endpoint, tableQuery ) ) { return { ...response, isError: true }; }