From 77ab03bdddd23e44ff7cc53667ae8cadca538b24 Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Fri, 11 Aug 2023 19:49:33 +0600 Subject: [PATCH 1/3] Address Code Review feedback. --- .../googlesitekit/modules/create-gathering-data-store.js | 5 ++++- .../modules/create-gathering-data-store.test.js | 7 +++++-- assets/js/modules/analytics-4/datastore/report.js | 6 +++--- assets/js/modules/analytics-4/datastore/report.test.js | 3 ++- assets/js/modules/analytics/datastore/report.js | 6 +++--- assets/js/modules/analytics/datastore/report.test.js | 3 ++- assets/js/modules/search-console/datastore/report.js | 6 +++--- assets/js/modules/search-console/datastore/report.test.js | 3 ++- 8 files changed, 24 insertions(+), 15 deletions(-) diff --git a/assets/js/googlesitekit/modules/create-gathering-data-store.js b/assets/js/googlesitekit/modules/create-gathering-data-store.js index 976f44098c2..28b82d40644 100644 --- a/assets/js/googlesitekit/modules/create-gathering-data-store.js +++ b/assets/js/googlesitekit/modules/create-gathering-data-store.js @@ -36,6 +36,9 @@ const { createRegistryControl } = Data; const RECEIVE_GATHERING_DATA = 'RECEIVE_GATHERING_DATA'; const WAIT_FOR_DATA_AVAILABILITY_STATE = 'WAIT_FOR_DATA_AVAILABILITY_STATE'; +export const ERROR_DETERMINING_GATHERING_DATA_STATE = + 'error_determining_gathering_data_state'; + /** * Creates a store object that includes actions and selectors for gathering data state for a module. * @@ -186,7 +189,7 @@ export const createGatheringDataStore = ( if ( dataAvailability === null ) { yield receiveError( - { message: 'Unable to determine gathering data state.' }, + { message: ERROR_DETERMINING_GATHERING_DATA_STATE }, 'isGatheringData', [] ); diff --git a/assets/js/googlesitekit/modules/create-gathering-data-store.test.js b/assets/js/googlesitekit/modules/create-gathering-data-store.test.js index 3a23ea2330f..8e43e134348 100644 --- a/assets/js/googlesitekit/modules/create-gathering-data-store.test.js +++ b/assets/js/googlesitekit/modules/create-gathering-data-store.test.js @@ -26,7 +26,10 @@ import { untilResolved, waitForDefaultTimeouts, } from '../../../../tests/js/utils'; -import { createGatheringDataStore } from './create-gathering-data-store'; +import { + ERROR_DETERMINING_GATHERING_DATA_STATE, + createGatheringDataStore, +} from './create-gathering-data-store'; import { createErrorStore } from '../data/create-error-store'; const MODULE_SLUG = 'test-slug'; @@ -343,7 +346,7 @@ describe( 'createGatheringDataStore', () => { expect( error ).not.toBeUndefined(); expect( error.message ).toBe( - 'Unable to determine gathering data state.' + ERROR_DETERMINING_GATHERING_DATA_STATE ); } ); } ); diff --git a/assets/js/modules/analytics-4/datastore/report.js b/assets/js/modules/analytics-4/datastore/report.js index 96297555342..61d640fc05b 100644 --- a/assets/js/modules/analytics-4/datastore/report.js +++ b/assets/js/modules/analytics-4/datastore/report.js @@ -57,7 +57,7 @@ const { createRegistrySelector } = Data; * @param {Function} select The select function of the registry. * @return {Object} Report args. */ -const getZeroDataReportArgs = ( select ) => { +const getSampleReportArgs = ( select ) => { const { startDate, endDate } = select( CORE_USER ).getDateRangeDates( { offsetDays: DATE_RANGE_OFFSET, } ); @@ -153,7 +153,7 @@ const gatheringDataStore = createGatheringDataStore( 'analytics-4', { // eslint-disable-next-line @wordpress/no-unused-vars-before-return const hasZeroData = select( MODULES_ANALYTICS_4 ).hasZeroData(); - const args = getZeroDataReportArgs( select ); + const args = getSampleReportArgs( select ); const hasResolvedReport = select( MODULES_ANALYTICS_4 @@ -362,7 +362,7 @@ const baseSelectors = { * @return {boolean|undefined} Returns `true` if the report is zero, otherwise `false`. Returns `undefined` while resolving. */ hasZeroData: createRegistrySelector( ( select ) => () => { - const args = getZeroDataReportArgs( select ); + const args = getSampleReportArgs( select ); // Disable reason: select needs to be called here or it will never run. // eslint-disable-next-line @wordpress/no-unused-vars-before-return diff --git a/assets/js/modules/analytics-4/datastore/report.test.js b/assets/js/modules/analytics-4/datastore/report.test.js index a1ae452fe9e..a8b0c30ddc9 100644 --- a/assets/js/modules/analytics-4/datastore/report.test.js +++ b/assets/js/modules/analytics-4/datastore/report.test.js @@ -34,6 +34,7 @@ import { import { DAY_IN_SECONDS } from '../../../util'; import { isZeroReport } from '../utils'; import * as fixtures from './__fixtures__'; +import { ERROR_DETERMINING_GATHERING_DATA_STATE } from '../../../googlesitekit/modules/create-gathering-data-store'; describe( 'modules/analytics-4 report', () => { let registry; @@ -275,7 +276,7 @@ describe( 'modules/analytics-4 report', () => { expect( error ).not.toBeUndefined(); expect( error.message ).toBe( - 'Unable to determine gathering data state.' + ERROR_DETERMINING_GATHERING_DATA_STATE ); expect( isGatheringData() ).toBe( true ); diff --git a/assets/js/modules/analytics/datastore/report.js b/assets/js/modules/analytics/datastore/report.js index 8d607cbb5af..3ab052a5bd8 100644 --- a/assets/js/modules/analytics/datastore/report.js +++ b/assets/js/modules/analytics/datastore/report.js @@ -62,7 +62,7 @@ const { createRegistrySelector } = Data; * @param {Function} select The select function of the registry. * @return {Object} Report args. */ -const getZeroDataReportArgs = ( select ) => { +const getSampleReportArgs = ( select ) => { const { startDate, endDate } = select( CORE_USER ).getDateRangeDates( { offsetDays: DATE_RANGE_OFFSET, } ); @@ -154,7 +154,7 @@ const gatheringDataStore = createGatheringDataStore( 'analytics', { dataAvailable: global._googlesitekitModulesData?.[ 'data_available_analytics' ], selectDataAvailability: createRegistrySelector( ( select ) => () => { - const args = getZeroDataReportArgs( select ); + const args = getSampleReportArgs( select ); // Disable reason: select needs to be called here or it will never run. // eslint-disable-next-line @wordpress/no-unused-vars-before-return @@ -366,7 +366,7 @@ const baseSelectors = { return true; } - const args = getZeroDataReportArgs( select ); + const args = getSampleReportArgs( select ); // Disable reason: select needs to be called here or it will never run. // eslint-disable-next-line @wordpress/no-unused-vars-before-return diff --git a/assets/js/modules/analytics/datastore/report.test.js b/assets/js/modules/analytics/datastore/report.test.js index e8057ed645a..2bfc060cc60 100644 --- a/assets/js/modules/analytics/datastore/report.test.js +++ b/assets/js/modules/analytics/datastore/report.test.js @@ -32,6 +32,7 @@ import { } from '../../../../../tests/js/utils'; import * as fixtures from './__fixtures__'; import { isZeroReport } from '../util'; +import { ERROR_DETERMINING_GATHERING_DATA_STATE } from '../../../googlesitekit/modules/create-gathering-data-store'; describe( 'modules/analytics report', () => { let registry; @@ -391,7 +392,7 @@ describe( 'modules/analytics report', () => { expect( error ).not.toBeUndefined(); expect( error.message ).toBe( - 'Unable to determine gathering data state.' + ERROR_DETERMINING_GATHERING_DATA_STATE ); expect( console ).toHaveErroredWith( ...consoleError ); diff --git a/assets/js/modules/search-console/datastore/report.js b/assets/js/modules/search-console/datastore/report.js index fcdbce60b66..0ee1b97ba31 100644 --- a/assets/js/modules/search-console/datastore/report.js +++ b/assets/js/modules/search-console/datastore/report.js @@ -48,7 +48,7 @@ const { createRegistrySelector } = Data; * @param {Function} select The select function of the registry. * @return {Object} Report args. */ -const getZeroDataReportArgs = ( select ) => { +const getSampleReportArgs = ( select ) => { const url = select( CORE_SITE ).getCurrentEntityURL(); const { compareStartDate: startDate, endDate } = select( CORE_USER @@ -118,7 +118,7 @@ const gatheringDataStore = createGatheringDataStore( 'search-console', { dataAvailable: global._googlesitekitModulesData?.[ 'data_available_search-console' ], selectDataAvailability: createRegistrySelector( ( select ) => () => { - const reportArgs = getZeroDataReportArgs( select ); + const reportArgs = getSampleReportArgs( select ); // Disable reason: select needs to be called here or it will never run. // eslint-disable-next-line @wordpress/no-unused-vars-before-return const report = select( MODULES_SEARCH_CONSOLE ).getReport( reportArgs ); @@ -211,7 +211,7 @@ const baseSelectors = { return true; } - const args = getZeroDataReportArgs( select ); + const args = getSampleReportArgs( select ); // Disable reason: select needs to be called here or it will never run. // eslint-disable-next-line @wordpress/no-unused-vars-before-return diff --git a/assets/js/modules/search-console/datastore/report.test.js b/assets/js/modules/search-console/datastore/report.test.js index 9e031f42819..6fc4d35f359 100644 --- a/assets/js/modules/search-console/datastore/report.test.js +++ b/assets/js/modules/search-console/datastore/report.test.js @@ -31,6 +31,7 @@ import { waitForTimeouts, } from '../../../../../tests/js/utils'; import * as fixtures from './__fixtures__'; +import { ERROR_DETERMINING_GATHERING_DATA_STATE } from '../../../googlesitekit/modules/create-gathering-data-store'; describe( 'modules/search-console report', () => { const searchAnalyticsRegexp = new RegExp( @@ -205,7 +206,7 @@ describe( 'modules/search-console report', () => { expect( error ).not.toBeUndefined(); expect( error.message ).toBe( - 'Unable to determine gathering data state.' + ERROR_DETERMINING_GATHERING_DATA_STATE ); expect( console ).toHaveErroredWith( ...consoleError ); From 0a1124a58ee7993251522ed6f0d44f0ab8293e67 Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Sat, 12 Aug 2023 02:00:19 +0600 Subject: [PATCH 2/3] Do not dispatch error for isGatheringData selector. --- .../modules/create-gathering-data-store.js | 14 -------------- .../modules/create-gathering-data-store.test.js | 16 ++-------------- .../modules/analytics-4/datastore/report.test.js | 10 ---------- .../modules/analytics/datastore/report.test.js | 10 ---------- .../search-console/datastore/report.test.js | 10 ---------- 5 files changed, 2 insertions(+), 58 deletions(-) diff --git a/assets/js/googlesitekit/modules/create-gathering-data-store.js b/assets/js/googlesitekit/modules/create-gathering-data-store.js index 28b82d40644..59fbf29653d 100644 --- a/assets/js/googlesitekit/modules/create-gathering-data-store.js +++ b/assets/js/googlesitekit/modules/create-gathering-data-store.js @@ -27,18 +27,12 @@ import invariant from 'invariant'; import API from 'googlesitekit-api'; import Data from 'googlesitekit-data'; import { createFetchStore } from '../data/create-fetch-store'; -import { actions as errorStoreActions } from '../data/create-error-store'; - -const { receiveError } = errorStoreActions; const { createRegistryControl } = Data; const RECEIVE_GATHERING_DATA = 'RECEIVE_GATHERING_DATA'; const WAIT_FOR_DATA_AVAILABILITY_STATE = 'WAIT_FOR_DATA_AVAILABILITY_STATE'; -export const ERROR_DETERMINING_GATHERING_DATA_STATE = - 'error_determining_gathering_data_state'; - /** * Creates a store object that includes actions and selectors for gathering data state for a module. * @@ -187,14 +181,6 @@ export const createGatheringDataStore = ( yield actions.receiveIsGatheringData( ! dataAvailability ); - if ( dataAvailability === null ) { - yield receiveError( - { message: ERROR_DETERMINING_GATHERING_DATA_STATE }, - 'isGatheringData', - [] - ); - } - if ( dataAvailability ) { yield fetchSaveDataAvailableStateStore.actions.fetchSaveDataAvailableState(); } diff --git a/assets/js/googlesitekit/modules/create-gathering-data-store.test.js b/assets/js/googlesitekit/modules/create-gathering-data-store.test.js index 8e43e134348..d0c4761f38b 100644 --- a/assets/js/googlesitekit/modules/create-gathering-data-store.test.js +++ b/assets/js/googlesitekit/modules/create-gathering-data-store.test.js @@ -26,10 +26,7 @@ import { untilResolved, waitForDefaultTimeouts, } from '../../../../tests/js/utils'; -import { - ERROR_DETERMINING_GATHERING_DATA_STATE, - createGatheringDataStore, -} from './create-gathering-data-store'; +import { createGatheringDataStore } from './create-gathering-data-store'; import { createErrorStore } from '../data/create-error-store'; const MODULE_SLUG = 'test-slug'; @@ -312,7 +309,7 @@ describe( 'createGatheringDataStore', () => { ); } ); - it( 'should set gathering data state and dispatch an error when selectDataAvailability returns NULL', async () => { + it( 'should set gathering data state to TRUE when selectDataAvailability returns NULL', async () => { selectDataAvailability.mockReturnValue( null ); registry.registerStore( @@ -339,15 +336,6 @@ describe( 'createGatheringDataStore', () => { expect( registry.select( STORE_NAME ).isGatheringData() ).toBe( true ); - - const error = registry - .select( STORE_NAME ) - .getErrorForSelector( 'isGatheringData' ); - - expect( error ).not.toBeUndefined(); - expect( error.message ).toBe( - ERROR_DETERMINING_GATHERING_DATA_STATE - ); } ); } ); } ); diff --git a/assets/js/modules/analytics-4/datastore/report.test.js b/assets/js/modules/analytics-4/datastore/report.test.js index a8b0c30ddc9..689e9e4da3d 100644 --- a/assets/js/modules/analytics-4/datastore/report.test.js +++ b/assets/js/modules/analytics-4/datastore/report.test.js @@ -34,7 +34,6 @@ import { import { DAY_IN_SECONDS } from '../../../util'; import { isZeroReport } from '../utils'; import * as fixtures from './__fixtures__'; -import { ERROR_DETERMINING_GATHERING_DATA_STATE } from '../../../googlesitekit/modules/create-gathering-data-store'; describe( 'modules/analytics-4 report', () => { let registry; @@ -270,15 +269,6 @@ describe( 'modules/analytics-4 report', () => { // Wait for resolvers to run. await waitForTimeouts( 30 ); - const error = registry - .select( MODULES_ANALYTICS_4 ) - .getErrorForSelector( 'isGatheringData' ); - - expect( error ).not.toBeUndefined(); - expect( error.message ).toBe( - ERROR_DETERMINING_GATHERING_DATA_STATE - ); - expect( isGatheringData() ).toBe( true ); expect( console ).toHaveErrored(); expect( fetchMock ).not.toHaveFetched( dataAvailableRegexp ); diff --git a/assets/js/modules/analytics/datastore/report.test.js b/assets/js/modules/analytics/datastore/report.test.js index 2bfc060cc60..ad4441b8579 100644 --- a/assets/js/modules/analytics/datastore/report.test.js +++ b/assets/js/modules/analytics/datastore/report.test.js @@ -32,7 +32,6 @@ import { } from '../../../../../tests/js/utils'; import * as fixtures from './__fixtures__'; import { isZeroReport } from '../util'; -import { ERROR_DETERMINING_GATHERING_DATA_STATE } from '../../../googlesitekit/modules/create-gathering-data-store'; describe( 'modules/analytics report', () => { let registry; @@ -386,15 +385,6 @@ describe( 'modules/analytics report', () => { // Wait for resolvers to run. await waitForTimeouts( 30 ); - const error = registry - .select( MODULES_ANALYTICS ) - .getErrorForSelector( 'isGatheringData' ); - - expect( error ).not.toBeUndefined(); - expect( error.message ).toBe( - ERROR_DETERMINING_GATHERING_DATA_STATE - ); - expect( console ).toHaveErroredWith( ...consoleError ); expect( isGatheringData() ).toBe( true ); expect( fetchMock ).not.toHaveFetched( dataAvailableRegexp ); diff --git a/assets/js/modules/search-console/datastore/report.test.js b/assets/js/modules/search-console/datastore/report.test.js index 6fc4d35f359..6c8bd8364a7 100644 --- a/assets/js/modules/search-console/datastore/report.test.js +++ b/assets/js/modules/search-console/datastore/report.test.js @@ -31,7 +31,6 @@ import { waitForTimeouts, } from '../../../../../tests/js/utils'; import * as fixtures from './__fixtures__'; -import { ERROR_DETERMINING_GATHERING_DATA_STATE } from '../../../googlesitekit/modules/create-gathering-data-store'; describe( 'modules/search-console report', () => { const searchAnalyticsRegexp = new RegExp( @@ -200,15 +199,6 @@ describe( 'modules/search-console report', () => { // Wait for resolvers to run. await waitForTimeouts( 30 ); - const error = registry - .select( MODULES_SEARCH_CONSOLE ) - .getErrorForSelector( 'isGatheringData' ); - - expect( error ).not.toBeUndefined(); - expect( error.message ).toBe( - ERROR_DETERMINING_GATHERING_DATA_STATE - ); - expect( console ).toHaveErroredWith( ...consoleError ); expect( isGatheringData() ).toBe( true ); expect( fetchMock ).not.toHaveFetched( dataAvailableRegexp ); From b324474a7f37c2fcd8ce06a3005a52b97bc5431f Mon Sep 17 00:00:00 2001 From: Arafat Zahan Date: Sat, 12 Aug 2023 02:10:48 +0600 Subject: [PATCH 3/3] Update `getSampleReportArgs` function description. --- assets/js/modules/analytics-4/datastore/report.js | 2 +- assets/js/modules/analytics/datastore/report.js | 2 +- assets/js/modules/search-console/datastore/report.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/assets/js/modules/analytics-4/datastore/report.js b/assets/js/modules/analytics-4/datastore/report.js index 61d640fc05b..ee66aae0d00 100644 --- a/assets/js/modules/analytics-4/datastore/report.js +++ b/assets/js/modules/analytics-4/datastore/report.js @@ -50,7 +50,7 @@ import { createGatheringDataStore } from '../../../googlesitekit/modules/create- const { createRegistrySelector } = Data; /** - * Returns report args for the zero data report. + * Returns report args for a sample report. * * @since 1.107.0 * diff --git a/assets/js/modules/analytics/datastore/report.js b/assets/js/modules/analytics/datastore/report.js index 3ab052a5bd8..ed338a25932 100644 --- a/assets/js/modules/analytics/datastore/report.js +++ b/assets/js/modules/analytics/datastore/report.js @@ -55,7 +55,7 @@ import { createGatheringDataStore } from '../../../googlesitekit/modules/create- const { createRegistrySelector } = Data; /** - * Returns report args for the zero data report. + * Returns report args for a sample report. * * @since 1.107.0 * diff --git a/assets/js/modules/search-console/datastore/report.js b/assets/js/modules/search-console/datastore/report.js index 0ee1b97ba31..c1c373c95e5 100644 --- a/assets/js/modules/search-console/datastore/report.js +++ b/assets/js/modules/search-console/datastore/report.js @@ -41,7 +41,7 @@ import { createGatheringDataStore } from '../../../googlesitekit/modules/create- const { createRegistrySelector } = Data; /** - * Returns report args for the zero data report. + * Returns report args for a sample report. * * @since 1.107.0 *