Merge pull request woocommerce/woocommerce-admin#565 from woocommerce/try/fix-dry-queries-2nd-round

Reports: create queries for data requests in one place
This commit is contained in:
Paul Sealock 2018-10-19 09:33:12 +13:00 committed by GitHub
commit 71b2435d96
5 changed files with 59 additions and 108 deletions

View File

@ -21,7 +21,6 @@ import {
getPreviousDate, getPreviousDate,
} from 'lib/date'; } from 'lib/date';
import { getReportChartData } from 'store/reports/utils'; import { getReportChartData } from 'store/reports/utils';
import { MAX_PER_PAGE } from 'store/constants';
import ReportError from 'analytics/components/report-error'; import ReportError from 'analytics/components/report-error';
class ReportChart extends Component { class ReportChart extends Component {
@ -100,32 +99,8 @@ ReportChart.propTypes = {
export default compose( export default compose(
withSelect( ( select, props ) => { withSelect( ( select, props ) => {
const { query, endpoint } = props; const { query, endpoint } = props;
const interval = getIntervalForQuery( query ); const primaryData = getReportChartData( endpoint, 'primary', query, select );
const datesFromQuery = getCurrentDates( query ); const secondaryData = getReportChartData( endpoint, 'primary', query, select );
const baseArgs = {
order: 'asc',
interval: interval,
per_page: MAX_PER_PAGE,
};
const primaryData = getReportChartData(
endpoint,
{
...baseArgs,
after: datesFromQuery.primary.after,
before: datesFromQuery.primary.before,
},
select
);
const secondaryData = getReportChartData(
endpoint,
{
...baseArgs,
after: datesFromQuery.secondary.after,
before: datesFromQuery.secondary.before,
},
select
);
return { return {
primaryData, primaryData,
secondaryData, secondaryData,

View File

@ -15,7 +15,7 @@ import PropTypes from 'prop-types';
import { formatCurrency } from 'lib/currency'; import { formatCurrency } from 'lib/currency';
import { getNewPath } from 'lib/nav-utils'; import { getNewPath } from 'lib/nav-utils';
import { SummaryList, SummaryListPlaceholder, SummaryNumber } from '@woocommerce/components'; import { SummaryList, SummaryListPlaceholder, SummaryNumber } from '@woocommerce/components';
import { getCurrentDates, getDateParamsFromQuery } from 'lib/date'; import { getDateParamsFromQuery } from 'lib/date';
import { getSummaryNumbers } from 'store/reports/utils'; import { getSummaryNumbers } from 'store/reports/utils';
import ReportError from 'analytics/components/report-error'; import ReportError from 'analytics/components/report-error';
class ReportSummary extends Component { class ReportSummary extends Component {
@ -93,15 +93,7 @@ ReportSummary.propTypes = {
export default compose( export default compose(
withSelect( ( select, props ) => { withSelect( ( select, props ) => {
const { query, endpoint } = props; const { query, endpoint } = props;
const datesFromQuery = getCurrentDates( query ); const summaryNumbers = getSummaryNumbers( endpoint, query, select );
const summaryNumbers = getSummaryNumbers(
endpoint,
{
primary: datesFromQuery.primary,
secondary: datesFromQuery.secondary,
},
select
);
return { return {
summaryNumbers, summaryNumbers,

View File

@ -15,9 +15,8 @@ import { get } from 'lodash';
import { EmptyContent, ReportFilters } from '@woocommerce/components'; import { EmptyContent, ReportFilters } from '@woocommerce/components';
import { filters, advancedFilterConfig } from './config'; import { filters, advancedFilterConfig } from './config';
import { getAdminLink } from 'lib/nav-utils'; import { getAdminLink } from 'lib/nav-utils';
import { appendTimestamp, getCurrentDates, getIntervalForQuery } from 'lib/date'; import { appendTimestamp, getCurrentDates } from 'lib/date';
import { getReportChartData } from 'store/reports/utils'; import { getReportChartData } from 'store/reports/utils';
import { MAX_PER_PAGE } from 'store/constants';
import OrdersReportChart from './chart'; import OrdersReportChart from './chart';
import OrdersReportTable from './table'; import OrdersReportTable from './table';
@ -98,23 +97,8 @@ OrdersReport.propTypes = {
export default compose( export default compose(
withSelect( ( select, props ) => { withSelect( ( select, props ) => {
const { query } = props; const { query } = props;
const interval = getIntervalForQuery( query );
const datesFromQuery = getCurrentDates( query ); const datesFromQuery = getCurrentDates( query );
const baseArgs = { const primaryData = getReportChartData( 'orders', 'primary', query, select );
order: 'asc',
interval: interval,
per_page: MAX_PER_PAGE,
};
const primaryData = getReportChartData(
'orders',
{
...baseArgs,
after: datesFromQuery.primary.after,
before: datesFromQuery.primary.before,
},
select
);
const { getOrders, isGetOrdersError, isGetOrdersRequesting } = select( 'wc-admin' ); const { getOrders, isGetOrdersError, isGetOrdersRequesting } = select( 'wc-admin' );
const tableQuery = { const tableQuery = {

View File

@ -76,7 +76,7 @@ describe( 'getReportChartData()', () => {
setIsReportStatsRequesting( () => { setIsReportStatsRequesting( () => {
return true; return true;
} ); } );
const result = getReportChartData( 'revenue', {}, select ); const result = getReportChartData( 'revenue', 'primary', {}, select );
expect( result ).toEqual( { ...response, isRequesting: true } ); expect( result ).toEqual( { ...response, isRequesting: true } );
} ); } );
@ -87,7 +87,7 @@ describe( 'getReportChartData()', () => {
setIsReportStatsError( () => { setIsReportStatsError( () => {
return true; return true;
} ); } );
const result = getReportChartData( 'revenue', {}, select ); const result = getReportChartData( 'revenue', 'primary', {}, select );
expect( result ).toEqual( { ...response, isError: true } ); expect( result ).toEqual( { ...response, isError: true } );
} ); } );
@ -122,7 +122,7 @@ describe( 'getReportChartData()', () => {
}; };
} ); } );
const result = getReportChartData( 'revenue', {}, select ); const result = getReportChartData( 'revenue', 'primary', {}, select );
expect( result ).toEqual( { ...response, data: { ...data } } ); expect( result ).toEqual( { ...response, data: { ...data } } );
} ); } );
@ -168,7 +168,7 @@ describe( 'getReportChartData()', () => {
}; };
} ); } );
const actualResponse = getReportChartData( 'revenue', {}, select ); const actualResponse = getReportChartData( 'revenue', 'primary', {}, select );
const expectedResponse = { const expectedResponse = {
...response, ...response,
data: { data: {
@ -191,7 +191,7 @@ describe( 'getReportChartData()', () => {
return false; return false;
} ); } );
const result = getReportChartData( 'revenue', {}, select ); const result = getReportChartData( 'revenue', 'primary', {}, select );
expect( result ).toEqual( { ...response, isRequesting: true } ); expect( result ).toEqual( { ...response, isRequesting: true } );
} ); } );
@ -205,7 +205,7 @@ describe( 'getReportChartData()', () => {
} }
return false; return false;
} ); } );
const result = getReportChartData( 'revenue', {}, select ); const result = getReportChartData( 'revenue', 'primary', {}, select );
expect( result ).toEqual( { ...response, isError: true } ); expect( result ).toEqual( { ...response, isError: true } );
} ); } );
@ -223,7 +223,7 @@ describe( 'getReportChartData()', () => {
}; };
} ); } );
const result = getReportChartData( 'revenue', {}, select ); const result = getReportChartData( 'revenue', 'primary', {}, select );
expect( result ).toEqual( { ...response, isEmpty: true } ); expect( result ).toEqual( { ...response, isEmpty: true } );
} ); } );
} ); } );
@ -239,15 +239,11 @@ describe( 'getSummaryNumbers()', () => {
}, },
}; };
const dates = { const query = {
primary: { after: '2018-10-10',
after: '2018-10-10', before: '2018-10-10',
before: '2018-10-10', period: 'custom',
}, compare: 'previous_period',
secondary: {
after: '2018-10-09',
before: '2018-10-09',
},
}; };
beforeAll( () => { beforeAll( () => {
@ -280,7 +276,7 @@ describe( 'getSummaryNumbers()', () => {
setIsReportStatsRequesting( () => { setIsReportStatsRequesting( () => {
return true; return true;
} ); } );
const result = getSummaryNumbers( 'revenue', dates, select ); const result = getSummaryNumbers( 'revenue', query, select );
expect( result ).toEqual( { ...response, isRequesting: true } ); expect( result ).toEqual( { ...response, isRequesting: true } );
} ); } );
@ -291,7 +287,7 @@ describe( 'getSummaryNumbers()', () => {
setIsReportStatsError( () => { setIsReportStatsError( () => {
return true; return true;
} ); } );
const result = getSummaryNumbers( 'revenue', dates, select ); const result = getSummaryNumbers( 'revenue', query, select );
expect( result ).toEqual( { ...response, isError: true } ); expect( result ).toEqual( { ...response, isError: true } );
} ); } );
@ -319,8 +315,8 @@ describe( 'getSummaryNumbers()', () => {
}; };
} ); } );
setGetReportStats( ( endpoint, query ) => { setGetReportStats( ( endpoint, _query ) => {
if ( '2018-10-10T00:00:00+00:00' === query.after ) { if ( '2018-10-10T00:00:00+00:00' === _query.after ) {
return { return {
data: { data: {
totals: totals.primary, totals: totals.primary,
@ -336,7 +332,7 @@ describe( 'getSummaryNumbers()', () => {
}; };
} ); } );
const result = getSummaryNumbers( 'revenue', dates, select ); const result = getSummaryNumbers( 'revenue', query, select );
expect( result ).toEqual( { ...response, totals } ); expect( result ).toEqual( { ...response, totals } );
} ); } );
} ); } );

View File

@ -9,7 +9,7 @@ import { forEach, isNull } from 'lodash';
* Internal dependencies * Internal dependencies
*/ */
import { MAX_PER_PAGE } from 'store/constants'; import { MAX_PER_PAGE } from 'store/constants';
import { appendTimestamp } from 'lib/date'; import { appendTimestamp, getCurrentDates, getIntervalForQuery } from 'lib/date';
/** /**
* Returns true if a report object is empty. * Returns true if a report object is empty.
@ -33,15 +33,34 @@ export function isReportDataEmpty( report ) {
return false; return false;
} }
/**
* Constructs and returns a query associated with a Report data request.
*
* @param {String} dataType 'primary' or 'secondary'.
* @param {Object} query query parameters in the url.
* @returns {Object} data request query parameters.
*/
function getRequestQuery( dataType, query ) {
const datesFromQuery = getCurrentDates( query );
const interval = getIntervalForQuery( query );
return {
order: 'asc',
interval,
per_page: MAX_PER_PAGE,
after: appendTimestamp( datesFromQuery[ dataType ].after, 'start' ),
before: appendTimestamp( datesFromQuery[ dataType ].before, 'end' ),
};
}
/** /**
* Returns summary number totals needed to render a report page. * Returns summary number totals needed to render a report page.
* *
* @param {String} endpoint Report API Endpoint * @param {String} endpoint Report API Endpoint
* @param {Object} dates Primary and secondary dates. * @param {Object} query query parameters in the url
* @param {object} select Instance of @wordpress/select * @param {object} select Instance of @wordpress/select
* @return {Object} Object containing summary number responses. * @return {Object} Object containing summary number responses.
*/ */
export function getSummaryNumbers( endpoint, dates, select ) { export function getSummaryNumbers( endpoint, query, select ) {
const { getReportStats, isReportStatsRequesting, isReportStatsError } = select( 'wc-admin' ); const { getReportStats, isReportStatsRequesting, isReportStatsError } = select( 'wc-admin' );
const response = { const response = {
isRequesting: false, isRequesting: false,
@ -52,16 +71,7 @@ export function getSummaryNumbers( endpoint, dates, select ) {
}, },
}; };
const baseQuery = { const primaryQuery = getRequestQuery( 'primary', query );
interval: 'day',
per_page: 1, // We only need the `totals` part of the response.
};
const primaryQuery = {
...baseQuery,
after: appendTimestamp( dates.primary.after, 'start' ),
before: appendTimestamp( dates.primary.before, 'end' ),
};
const primary = getReportStats( endpoint, primaryQuery ); const primary = getReportStats( endpoint, primaryQuery );
if ( isReportStatsRequesting( endpoint, primaryQuery ) ) { if ( isReportStatsRequesting( endpoint, primaryQuery ) ) {
return { ...response, isRequesting: true }; return { ...response, isRequesting: true };
@ -71,12 +81,7 @@ export function getSummaryNumbers( endpoint, dates, select ) {
const primaryTotals = ( primary && primary.data && primary.data.totals ) || null; const primaryTotals = ( primary && primary.data && primary.data.totals ) || null;
const secondaryQuery = { const secondaryQuery = getRequestQuery( 'secondary', query );
...baseQuery,
per_page: 1,
after: appendTimestamp( dates.secondary.after, 'start' ),
before: appendTimestamp( dates.secondary.before, 'end' ),
};
const secondary = getReportStats( endpoint, secondaryQuery ); const secondary = getReportStats( endpoint, secondaryQuery );
if ( isReportStatsRequesting( endpoint, secondaryQuery ) ) { if ( isReportStatsRequesting( endpoint, secondaryQuery ) ) {
return { ...response, isRequesting: true }; return { ...response, isRequesting: true };
@ -93,11 +98,12 @@ export function getSummaryNumbers( endpoint, dates, select ) {
* Returns all of the data needed to render a chart with summary numbers on a report page. * Returns all of the data needed to render a chart with summary numbers on a report page.
* *
* @param {String} endpoint Report API Endpoint * @param {String} endpoint Report API Endpoint
* @param {Object} query API arguments * @param {String} dataType 'primary' or 'secondary'
* @param {Object} query query parameters in the url
* @param {object} select Instance of @wordpress/select * @param {object} select Instance of @wordpress/select
* @return {Object} Object containing API request information (response, fetching, and error details) * @return {Object} Object containing API request information (response, fetching, and error details)
*/ */
export function getReportChartData( endpoint, query, select ) { export function getReportChartData( endpoint, dataType, query, select ) {
const { getReportStats, isReportStatsRequesting, isReportStatsError } = select( 'wc-admin' ); const { getReportStats, isReportStatsRequesting, isReportStatsError } = select( 'wc-admin' );
const response = { const response = {
@ -110,14 +116,12 @@ export function getReportChartData( endpoint, query, select ) {
}, },
}; };
query.after = appendTimestamp( query.after, 'start' ); const requestQuery = getRequestQuery( dataType, query );
query.before = appendTimestamp( query.before, 'end' ); const stats = getReportStats( endpoint, requestQuery );
const stats = getReportStats( endpoint, query ); if ( isReportStatsRequesting( endpoint, requestQuery ) ) {
if ( isReportStatsRequesting( endpoint, query ) ) {
return { ...response, isRequesting: true }; return { ...response, isRequesting: true };
} else if ( isReportStatsError( endpoint, query ) ) { } else if ( isReportStatsError( endpoint, requestQuery ) ) {
return { ...response, isError: true }; return { ...response, isError: true };
} else if ( isReportDataEmpty( stats ) ) { } else if ( isReportDataEmpty( stats ) ) {
return { ...response, isEmpty: true }; return { ...response, isEmpty: true };
@ -135,17 +139,17 @@ export function getReportChartData( endpoint, query, select ) {
const totalPages = Math.ceil( stats.totalResults / MAX_PER_PAGE ); const totalPages = Math.ceil( stats.totalResults / MAX_PER_PAGE );
for ( let i = 2; i <= totalPages; i++ ) { for ( let i = 2; i <= totalPages; i++ ) {
const _query = { ...query, page: i }; const nextQuery = { ...requestQuery, page: i };
const _data = getReportStats( endpoint, _query ); const _data = getReportStats( endpoint, nextQuery );
if ( isReportStatsRequesting( endpoint, _query ) ) { if ( isReportStatsRequesting( endpoint, nextQuery ) ) {
continue; continue;
} }
if ( isReportStatsError( endpoint, _query ) ) { if ( isReportStatsError( endpoint, nextQuery ) ) {
isError = true; isError = true;
isFetching = false; isFetching = false;
break; break;
} }
if ( ! isReportStatsRequesting( endpoint, _query ) ) { if ( ! isReportStatsRequesting( endpoint, nextQuery ) ) {
pagedData.push( _data ); pagedData.push( _data );
if ( i === totalPages ) { if ( i === totalPages ) {
isFetching = false; isFetching = false;