From e58ecca164e05538a9d3b7fb37995e467227ed9c Mon Sep 17 00:00:00 2001 From: Eason Su Date: Thu, 22 Jun 2023 17:32:04 +0800 Subject: [PATCH 1/5] Add the `handleError` utils for handling API errors --- js/src/utils/handleError.js | 83 ++++++++++++++++++ js/src/utils/handleError.test.js | 146 +++++++++++++++++++++++++++++++ 2 files changed, 229 insertions(+) create mode 100644 js/src/utils/handleError.js create mode 100644 js/src/utils/handleError.test.js diff --git a/js/src/utils/handleError.js b/js/src/utils/handleError.js new file mode 100644 index 0000000000..77125f1908 --- /dev/null +++ b/js/src/utils/handleError.js @@ -0,0 +1,83 @@ +/** + * External dependencies + */ +import { dispatch } from '@wordpress/data'; +import { __, _x } from '@wordpress/i18n'; + +/** + * Internal dependencies + */ +import { NOTICES_STORE_KEY } from '.~/data/constants'; + +/** + * @typedef {Object} ApiError + * @property {string} [message] The error reason. + * @property {number} [statusCode] The HTTP response status code. + */ + +// Functions in this module use optional chaining to access `error` because the +// thrown error has various possibilities and there is no way to ensure +// that it's always an object type or an error instance. + +/** + * Resolves the final error message. + * + * @param {ApiError} error The error data or instance. + * @param {string} [leadingMessage] Optional leading message. + * @param {string} [fallbackMessage] Optional fallback message if API error message is not a string. + */ +export function resolveErrorMessage( error, leadingMessage, fallbackMessage ) { + const messages = []; + const apiMessage = error?.message; + + if ( leadingMessage ) { + messages.push( leadingMessage ); + } + + if ( apiMessage && typeof apiMessage === 'string' ) { + messages.push( apiMessage ); + } else if ( fallbackMessage ) { + messages.push( fallbackMessage ); + } + + if ( messages.length === 0 ) { + messages.push( + __( 'Unknown error occurred.', 'google-listings-and-ads' ) + ); + } + + return messages.join( + _x( + ' ', + `The spacing between sentences. It's a space in English. Please use an empty string if no spacing is needed in that language.`, + 'google-listings-and-ads' + ) + ); +} + +/** + * Handles the API error by showing the error message via a notification for users + * and also via `console.error` for developers. + * + * Please note that authorization errors will be only printed by `console.error`. + * + * @param {ApiError} error The error got from an API response. + * @param {string} [leadingMessage] Optional leading message. + * @param {string} [fallbackMessage] Optional fallback message if API error message is not available. + */ +export function handleApiError( error, leadingMessage, fallbackMessage ) { + // In this extension, errors with the 401 `statusCode` are used to indicate + // authorization errors, and they were already handled in the middleware of + // the `apiFetch` when runtime reaches here. + // + // Ref: The call of `apiFetch.use`` in js/src/data/index.js + if ( error?.statusCode !== 401 ) { + const args = [ error, leadingMessage, fallbackMessage ]; + const message = resolveErrorMessage( ...args ); + + dispatch( NOTICES_STORE_KEY ).createNotice( 'error', message ); + } + + // eslint-disable-next-line no-console + console.error( error ); +} diff --git a/js/src/utils/handleError.test.js b/js/src/utils/handleError.test.js new file mode 100644 index 0000000000..c8425bc6b1 --- /dev/null +++ b/js/src/utils/handleError.test.js @@ -0,0 +1,146 @@ +/** + * External dependencies + */ +import { dispatch } from '@wordpress/data'; + +/** + * Internal dependencies + */ +import { resolveErrorMessage, handleApiError } from './handleError'; + +jest.mock( '@wordpress/data', () => { + return { + dispatch: jest.fn().mockName( 'dispatch' ), + }; +} ); + +describe( 'resolveErrorMessage', () => { + it( 'When the arguments do not have any available messages, it should return "Unknown error occurred."', () => { + const message = resolveErrorMessage( {}, null, null ); + + expect( message ).toBe( 'Unknown error occurred.' ); + } ); + + it( 'Should place the `leadingMessage` in front and `error.message` in the back', () => { + const message = resolveErrorMessage( + { message: 'Something happened.' }, + 'Oh no.' + ); + + expect( message ).toBe( 'Oh no. Something happened.' ); + } ); + + it( 'Only when the `error.message` is a string, it should be used', () => { + let message = resolveErrorMessage( { message: 'Something happened.' } ); + + expect( message ).toBe( 'Something happened.' ); + + message = resolveErrorMessage( new Error( 'Something happened.' ) ); + + expect( message ).toBe( 'Something happened.' ); + + message = resolveErrorMessage( null, 'a' ); + + expect( message ).toBe( 'a' ); + + message = resolveErrorMessage( undefined, 'b' ); + + expect( message ).toBe( 'b' ); + + message = resolveErrorMessage( {}, 'c' ); + + expect( message ).toBe( 'c' ); + + message = resolveErrorMessage( { message: NaN }, 'd' ); + + expect( message ).toBe( 'd' ); + + message = resolveErrorMessage( { message: 123 }, 'e' ); + + expect( message ).toBe( 'e' ); + } ); + + it( 'When the `error.message` is a string, it should not use the `fallbackMessage`', () => { + const message = resolveErrorMessage( + { message: 'Something happened.' }, + null, + 'The code works well on my machine!' + ); + + expect( message ).toBe( 'Something happened.' ); + } ); + + it( 'When the `error.message` is not available, it should replace it with the `fallbackMessage`', () => { + let message = resolveErrorMessage( + null, + null, + 'The code works well on my machine!' + ); + + expect( message ).toBe( 'The code works well on my machine!' ); + + message = resolveErrorMessage( + null, + 'Hmm.', + 'The code works well on my machine!' + ); + + expect( message ).toBe( 'Hmm. The code works well on my machine!' ); + } ); +} ); + +describe( 'handleApiError', () => { + let createNotice; + let consoleError; + + beforeEach( () => { + createNotice = jest.fn().mockName( 'createNotice' ); + dispatch.mockImplementation( () => ( { createNotice } ) ); + + consoleError = jest.spyOn( global.console, 'error' ); + } ); + + afterEach( () => { + consoleError.mockReset(); + } ); + + it( 'Should call to `console.error` with the given error', () => { + let error = new Error( 'Oops!' ); + + handleApiError( error ); + + expect( consoleError ).toHaveBeenCalledWith( error ); + + error = { statusCode: 401, message: 'Oops!' }; + + handleApiError( error ); + + expect( consoleError ).toHaveBeenCalledWith( error ); + } ); + + it( `Should use the 'core/notices' wp-data store to dispatch notification actions`, () => { + handleApiError(); + + expect( dispatch ).toHaveBeenCalledWith( 'core/notices' ); + } ); + + it( 'When the error is not related to authorization, it should dispatch a notification action', () => { + handleApiError( { message: 'Oops!' } ); + + expect( createNotice ).toHaveBeenCalledWith( 'error', 'Oops!' ); + + handleApiError( { statusCode: 404, message: 'Oops!' } ); + + expect( createNotice ).toHaveBeenCalledWith( 'error', 'Oops!' ); + + handleApiError( { statusCode: 500, message: 'Oops!' } ); + + expect( createNotice ).toHaveBeenCalledWith( 'error', 'Oops!' ); + } ); + + it( 'When the error is related to authorization, it should not dispatch any notification actions', () => { + handleApiError( { statusCode: 401, message: 'Oops!' } ); + + expect( createNotice ).toHaveBeenCalledTimes( 0 ); + } ); +} ); From a0bf2436f48ccadbc783a74dcc6293da20b14b67 Mon Sep 17 00:00:00 2001 From: Eason Su Date: Thu, 22 Jun 2023 17:58:11 +0800 Subject: [PATCH 2/5] Show API error messages when errors occur in ads campaign modifications --- js/src/data/actions.js | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/js/src/data/actions.js b/js/src/data/actions.js index 131091ab55..6a45e89f8a 100644 --- a/js/src/data/actions.js +++ b/js/src/data/actions.js @@ -14,6 +14,7 @@ import { REQUEST_ACTIONS, EMPTY_ASSET_ENTITY_GROUP, } from './constants'; +import { handleApiError } from '.~/utils/handleError'; import { adaptAdsCampaign } from './adapters'; export function handleFetchError( error, message ) { @@ -785,13 +786,7 @@ export function* createAdsCampaign( amount, countryCodes ) { createdCampaign: adaptAdsCampaign( createdCampaign ), }; } catch ( error ) { - yield handleFetchError( - error, - __( - 'Unable to create your paid ads campaign. Please try again later.', - 'google-listings-and-ads' - ) - ); + handleApiError( error ); throw error; } @@ -820,18 +815,18 @@ export function* updateAdsCampaign( id, data ) { data, }; } catch ( error ) { - yield handleFetchError( - error, - __( - 'Unable to update your paid ads campaign. Please try again later.', - 'google-listings-and-ads' - ) - ); + handleApiError( error ); throw error; } } +/** + * Delete an ads campaign by ID. + * + * @param {number} id The ID of the ads campaign to be deleted. + * @throws Will throw an error if the request failed. + */ export function* deleteAdsCampaign( id ) { try { yield apiFetch( { @@ -844,13 +839,8 @@ export function* deleteAdsCampaign( id ) { id, }; } catch ( error ) { - yield handleFetchError( - error, - __( - 'Unable to delete your paid ads campaign. Please try again later.', - 'google-listings-and-ads' - ) - ); + handleApiError( error ); + throw error; } } From 8ba91f06548aee1fb159d3e03a45e96f2aef1e4c Mon Sep 17 00:00:00 2001 From: Eason Su Date: Thu, 22 Jun 2023 18:06:39 +0800 Subject: [PATCH 3/5] Replace the rest `handleFetchError` calls with `handleApiError` and remove most "Please try again later." from messages in the actions.js --- js/src/data/actions.js | 86 ++++++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 45 deletions(-) diff --git a/js/src/data/actions.js b/js/src/data/actions.js index 6a45e89f8a..1fe3d91f6f 100644 --- a/js/src/data/actions.js +++ b/js/src/data/actions.js @@ -144,7 +144,7 @@ export function* fetchShippingRates() { shippingRates, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading shipping rates.', @@ -235,7 +235,7 @@ export function* fetchShippingTimes() { shippingTimes, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading shipping times.', @@ -309,7 +309,7 @@ export function* fetchSettings() { settings: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading merchant center settings.', @@ -349,7 +349,7 @@ export function* fetchJetpackAccount() { account: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading Jetpack account info.', @@ -370,7 +370,7 @@ export function* fetchGoogleAccount() { account: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading Google account info.', @@ -398,7 +398,7 @@ export function* fetchGoogleMCAccount() { account: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading Google Merchant Center account info.', @@ -419,7 +419,7 @@ export function* fetchExistingGoogleMCAccounts() { accounts: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error getting your Google Merchant Center accounts.', @@ -440,7 +440,7 @@ export function* fetchGoogleAdsAccount() { account: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading Google Ads account info.', @@ -461,10 +461,10 @@ export function* disconnectGoogleAccount() { type: TYPES.DISCONNECT_ACCOUNTS_GOOGLE, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( - 'Unable to disconnect your Google account. Please try again later.', + 'Unable to disconnect your Google account.', 'google-listings-and-ads' ) ); @@ -490,10 +490,10 @@ export function* disconnectGoogleAdsAccount( invalidateRelatedState = false ) { invalidateRelatedState, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( - 'Unable to disconnect your Google Ads account. Please try again later.', + 'Unable to disconnect your Google Ads account.', 'google-listings-and-ads' ) ); @@ -512,10 +512,10 @@ export function* disconnectAllAccounts() { type: TYPES.DISCONNECT_ACCOUNTS_ALL, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( - 'Unable to disconnect all your accounts. Please try again later.', + 'Unable to disconnect all your accounts.', 'google-listings-and-ads' ) ); @@ -531,7 +531,7 @@ export function* fetchGoogleAdsAccountBillingStatus() { return receiveGoogleAdsAccountBillingStatus( response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error getting the billing status of your Google Ads account.', @@ -552,7 +552,7 @@ export function* fetchExistingGoogleAdsAccounts() { accounts: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error getting your Google Ads accounts.', @@ -582,10 +582,10 @@ export function* updateGoogleMCContactInformation() { yield receiveGoogleMCContactInformation( response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( - 'Unable to update your Google Merchant Center contact information. Please try again later.', + 'Unable to update your Google Merchant Center contact information.', 'google-listings-and-ads' ) ); @@ -633,10 +633,10 @@ export function* requestPhoneVerificationCode( country, phoneNumber, method ) { }; } - yield handleFetchError( + handleApiError( error, __( - 'Unable to request the phone verification code. Please try again later.', + 'Unable to request the phone verification code.', 'google-listings-and-ads' ) ); @@ -691,10 +691,10 @@ export function* verifyPhoneNumber( verificationId, code, method ) { } } - yield handleFetchError( + handleApiError( error, __( - 'Unable to verify your phone number. Please try again later.', + 'Unable to verify your phone number.', 'google-listings-and-ads' ) ); @@ -712,7 +712,7 @@ export function* fetchTargetAudience() { target_audience: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading target audience.', @@ -869,14 +869,12 @@ export function* createCampaignAssetGroup( campaignId ) { }, }; } catch ( error ) { - const message = - error.message || - __( - 'There was an error creating the assets of the campaign.', - 'google-listings-and-ads' - ); + const fallbackMessage = __( + 'There was an error creating the assets of the campaign.', + 'google-listings-and-ads' + ); - yield handleFetchError( error, message ); + handleApiError( error, null, fallbackMessage ); throw error; } } @@ -902,14 +900,12 @@ export function* updateCampaignAssetGroup( assetGroupId, body ) { assetGroupId, }; } catch ( error ) { - const message = - error.message || - __( - 'There was an error updating the assets of the campaign.', - 'google-listings-and-ads' - ); + const fallbackMessage = __( + 'There was an error updating the assets of the campaign.', + 'google-listings-and-ads' + ); - yield handleFetchError( error, message ); + handleApiError( error, null, fallbackMessage ); throw error; } } @@ -930,7 +926,7 @@ export function* fetchMCSetup() { return receiveMCSetup( response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading your merchant center setup status.', @@ -1006,10 +1002,10 @@ export function* updateMCProductVisibility( ids, visible ) { type: TYPES.UPDATE_MC_PRODUCTS_VISIBILITY, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( - 'Unable to update the channel visibility of products. Please try again later.', + 'Unable to update the channel visibility of products.', 'google-listings-and-ads' ) ); @@ -1029,7 +1025,7 @@ export function* sendMCReviewRequest() { return yield receiveMCReviewRequest( response ); } catch ( error ) { - yield handleFetchError( error, error?.message ); + handleApiError( error ); throw error; } } @@ -1092,7 +1088,7 @@ export function* createMappingRule( rule ) { rule: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error creating the rule.', @@ -1121,7 +1117,7 @@ export function* updateMappingRule( rule ) { rule: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error updating the rule.', @@ -1150,7 +1146,7 @@ export function* deleteMappingRule( rule ) { rule: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error deleting the rule.', @@ -1217,7 +1213,7 @@ export function* upsertTour( tour, upsertingClientStoreFirst = false ) { yield action; } } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error updating the tour.', From b913ee78afb8d78e92dfa8fcd72050b4a454900d Mon Sep 17 00:00:00 2001 From: Eason Su Date: Thu, 22 Jun 2023 18:01:26 +0800 Subject: [PATCH 4/5] Replace all `handleFetchError` calls with `handleApiError` in the resolvers.js --- js/src/data/resolvers.js | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/js/src/data/resolvers.js b/js/src/data/resolvers.js index efb6432f0d..37f4a564d5 100644 --- a/js/src/data/resolvers.js +++ b/js/src/data/resolvers.js @@ -16,11 +16,11 @@ import { import TYPES from './action-types'; import { API_NAMESPACE } from './constants'; import { getReportKey } from './utils'; +import { handleApiError } from '.~/utils/handleError'; import { adaptAdsCampaign, adaptAssetGroup } from './adapters'; import { fetchWithHeaders, awaitPromise } from './controls'; import { - handleFetchError, fetchShippingRates, fetchShippingTimes, fetchSettings, @@ -79,7 +79,7 @@ export function* getGoogleAccountAccess() { yield receiveGoogleAccountAccess( data ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading Google account access info.', @@ -134,7 +134,7 @@ export function* getGoogleMCContactInformation() { } ); yield receiveGoogleMCContactInformation( data ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading Google Merchant Center contact information.', @@ -159,7 +159,7 @@ export function* getMCCountriesAndContinents() { data, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading supported country details.', @@ -183,7 +183,7 @@ export function* getPolicyCheck() { data: response, }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading policy check details.', @@ -209,7 +209,7 @@ export function* getAdsCampaigns( query ) { adsCampaigns: campaigns.map( adaptAdsCampaign ), }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading ads campaigns.', @@ -242,7 +242,7 @@ export function* getCampaignAssetGroups( campaignId ) { assetGroups: assetGroups.map( adaptAssetGroup ), }; } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading the assets of the campaign.', @@ -264,7 +264,7 @@ export function* getMCProductStatistics() { yield receiveMCProductStatistics( response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading your merchant center product statistics.', @@ -282,7 +282,7 @@ export function* getMCReviewRequest() { yield receiveMCReviewRequest( response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading your merchant center product review request status.', @@ -307,7 +307,7 @@ export function* getMCIssues( query ) { yield receiveMCIssues( query, response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading issues to resolve.', @@ -329,7 +329,7 @@ export function* getMCProductFeed( query ) { yield receiveMCProductFeed( query, response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading product feed.', @@ -367,7 +367,7 @@ export function* getReportByApiQuery( category, type, reportQuery ) { const reportKey = getReportKey( category, type, reportQuery ); yield receiveReport( reportKey, data ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading report.', @@ -385,7 +385,7 @@ export function* getMappingAttributes() { yield receiveMappingAttributes( response.data ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading the mapping attributes.', @@ -409,7 +409,7 @@ export function* getMappingSources( attributeKey ) { yield receiveMappingSources( response.data, attributeKey ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading the mapping sources for the selected attribute.', @@ -444,7 +444,7 @@ export function* getMappingRules( pagination ) { pages, } ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error loading the mapping rules.', @@ -478,7 +478,7 @@ export function* getStoreCategories() { yield receiveStoreCategories( response ); } catch ( error ) { - yield handleFetchError( + handleApiError( error, __( 'There was an error getting the store categories.', @@ -510,7 +510,7 @@ export function* getTour( tourId ) { const bodyPromise = response?.json() || response?.text(); const error = yield awaitPromise( bodyPromise ); - yield handleFetchError( + handleApiError( error, __( 'There was an error getting the tour.', From f3f027683d404dbfe5e0295e501554f7accf5958 Mon Sep 17 00:00:00 2001 From: Eason Su Date: Thu, 22 Jun 2023 18:01:56 +0800 Subject: [PATCH 5/5] Remove `handleFetchError` from the actions.js --- js/src/data/actions.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/js/src/data/actions.js b/js/src/data/actions.js index 1fe3d91f6f..64dfd5fd10 100644 --- a/js/src/data/actions.js +++ b/js/src/data/actions.js @@ -2,7 +2,6 @@ * External dependencies */ import { apiFetch } from '@wordpress/data-controls'; -import { dispatch } from '@wordpress/data'; import { __ } from '@wordpress/i18n'; /** @@ -17,18 +16,6 @@ import { import { handleApiError } from '.~/utils/handleError'; import { adaptAdsCampaign } from './adapters'; -export function handleFetchError( error, message ) { - const { createNotice } = dispatch( 'core/notices' ); - - // Only show errors that are not authorization issues. - if ( error.statusCode !== 401 ) { - createNotice( 'error', message ); - } - - // eslint-disable-next-line no-console - console.log( error ); -} - /** * @typedef {import('.~/data/types.js').AssetEntityGroupUpdateBody} AssetEntityGroupUpdateBody * @typedef {import('./selectors').Tour} Tour