Commit 8e022cf12a9 for woocommerce
commit 8e022cf12a97eec3448d14445690f69ba757a783
Author: Chi-Hsuan Huang <chihsuan.tw@gmail.com>
Date: Tue Aug 25 11:13:45 2026 +0800
fix: keep Analytics settings retryable after a failed save and clear stale error (#67868)
* fix: keep Analytics settings retryable after a failed save and clear stale error
* fix: narrow settings error state carry-over and guard clearing unloaded groups
* fix: clear the settings error before a save reports as finished
diff --git a/packages/js/data/changelog/fix-wooairr-102-settings-stale-error b/packages/js/data/changelog/fix-wooairr-102-settings-stale-error
new file mode 100644
index 00000000000..bf953c12262
--- /dev/null
+++ b/packages/js/data/changelog/fix-wooairr-102-settings-stale-error
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Keep unsaved settings retryable after a failed save and clear the stored error once a save succeeds.
diff --git a/packages/js/data/src/settings/action-types.ts b/packages/js/data/src/settings/action-types.ts
index c8c60aa01bc..7e97d8a7291 100644
--- a/packages/js/data/src/settings/action-types.ts
+++ b/packages/js/data/src/settings/action-types.ts
@@ -4,6 +4,7 @@ const TYPES = {
CLEAR_SETTINGS: 'CLEAR_SETTINGS',
SET_IS_REQUESTING: 'SET_IS_REQUESTING',
CLEAR_IS_DIRTY: 'CLEAR_IS_DIRTY',
+ CLEAR_ERROR_FOR_GROUP: 'CLEAR_ERROR_FOR_GROUP',
} as const;
export default TYPES;
diff --git a/packages/js/data/src/settings/actions.ts b/packages/js/data/src/settings/actions.ts
index 397714b300f..f29e5a46c8f 100644
--- a/packages/js/data/src/settings/actions.ts
+++ b/packages/js/data/src/settings/actions.ts
@@ -56,6 +56,13 @@ export function setIsRequesting( group: string, isRequesting: boolean ) {
};
}
+export function clearErrorForGroup( group: string ) {
+ return {
+ type: TYPES.CLEAR_ERROR_FOR_GROUP,
+ group,
+ };
+}
+
export function clearIsDirty( group: string ) {
return {
type: TYPES.CLEAR_IS_DIRTY,
@@ -106,8 +113,6 @@ export function* persistSettingsForGroup( group: string ) {
data: { update },
} );
- yield setIsRequesting( group, false );
-
if ( ! results ) {
throw new Error(
__(
@@ -117,8 +122,15 @@ export function* persistSettingsForGroup( group: string ) {
);
}
+ // Clear any error left over from a previous failed save, otherwise the
+ // stale error keeps being reported for every subsequent success.
+ yield clearErrorForGroup( group );
// remove dirtyKeys from map - note we're only doing this if there is no error.
yield clearIsDirty( group );
+ // Marked as finished last so that consumers watching `isRequesting`
+ // for the end of a save never see it flip while the stale error is
+ // still in state.
+ yield setIsRequesting( group, false );
} catch ( e ) {
yield updateErrorForGroup( group, null, e );
yield setIsRequesting( group, false );
@@ -148,6 +160,7 @@ export type Actions = ReturnType<
| typeof updateErrorForGroup
| typeof setIsRequesting
| typeof clearIsDirty
+ | typeof clearErrorForGroup
| typeof clearSettings
>;
diff --git a/packages/js/data/src/settings/reducer.ts b/packages/js/data/src/settings/reducer.ts
index fc1f6d8e9fa..02c05706300 100644
--- a/packages/js/data/src/settings/reducer.ts
+++ b/packages/js/data/src/settings/reducer.ts
@@ -50,6 +50,18 @@ const reducer: Reducer< SettingsState, Actions > = ( state = {}, action ) => {
},
};
break;
+ case TYPES.CLEAR_ERROR_FOR_GROUP:
+ if ( ! state[ action.group ] ) {
+ break;
+ }
+ state = {
+ ...state,
+ [ action.group ]: {
+ ...state[ action.group ],
+ error: null,
+ },
+ };
+ break;
case TYPES.CLEAR_IS_DIRTY:
state = {
...state,
@@ -72,6 +84,12 @@ const reducer: Reducer< SettingsState, Actions > = ( state = {}, action ) => {
...state,
[ group ]: {
data: state[ group ] ? state[ group ].data : [],
+ // Keep the dirty keys so a failed save can be retried.
+ // Nothing else is carried over on purpose: the getSettings
+ // resolver sets isRequesting and never resets it, so
+ // preserving it here would leave the group stuck in a
+ // requesting state after a failed fetch.
+ dirty: state[ group ]?.dirty,
error,
lastReceived: time,
},
diff --git a/packages/js/data/src/settings/test/actions.ts b/packages/js/data/src/settings/test/actions.ts
new file mode 100644
index 00000000000..d629d597828
--- /dev/null
+++ b/packages/js/data/src/settings/test/actions.ts
@@ -0,0 +1,117 @@
+/**
+ * Internal dependencies
+ */
+import { persistSettingsForGroup } from '../actions';
+import TYPES from '../action-types';
+
+const GROUP = 'wc_admin';
+const DIRTY_KEYS = [ 'wcAdminSettings' ];
+const DIRTY_DATA = {
+ wcAdminSettings: { woocommerce_default_date_range: 'period=month' },
+};
+
+// The step at which the generator yields the batch request. Everything before
+// it is `setIsRequesting` and the two `resolveSelect` controls.
+const API_FETCH_STEP = 3;
+
+type YieldedAction = Record< string, unknown > & { type: string };
+
+const OWN_TYPES: string[] = Object.values( TYPES );
+
+/**
+ * Drives persistSettingsForGroup to completion, answering each control with a
+ * canned value, and returns the actions it dispatched along the way.
+ *
+ * @param options What the batch request should do.
+ * @param options.apiResult Value the request resolves to.
+ * @param options.apiError Error the request rejects with instead.
+ */
+const runPersist = ( {
+ apiResult,
+ apiError,
+}: {
+ apiResult?: unknown;
+ apiError?: Error;
+} ) => {
+ const replies: unknown[] = [ undefined, DIRTY_KEYS, DIRTY_DATA ];
+ const yielded: YieldedAction[] = [];
+ const generator = persistSettingsForGroup( GROUP );
+
+ let step = generator.next();
+ let index = 0;
+ let thrown: unknown;
+
+ while ( ! step.done ) {
+ yielded.push( step.value as YieldedAction );
+ try {
+ if ( index === API_FETCH_STEP ) {
+ step = apiError
+ ? generator.throw( apiError )
+ : generator.next( apiResult );
+ } else {
+ step = generator.next( replies[ index ] );
+ }
+ } catch ( e ) {
+ thrown = e;
+ break;
+ }
+ index++;
+ }
+
+ return {
+ thrown,
+ // Controls (resolveSelect, apiFetch) are not dispatched, so only the
+ // store's own actions are relevant to the order under test.
+ actions: yielded.filter( ( action ) =>
+ OWN_TYPES.includes( action.type )
+ ),
+ };
+};
+
+describe( 'persistSettingsForGroup', () => {
+ it( 'clears the stale error and the dirty keys before it stops requesting', () => {
+ const { actions } = runPersist( { apiResult: { update: [] } } );
+
+ // The Analytics settings screen decides between the success and the
+ // error notice on the `isRequesting` true -> false transition, so that
+ // transition has to come last.
+ expect( actions.map( ( action ) => action.type ) ).toEqual( [
+ TYPES.SET_IS_REQUESTING,
+ TYPES.CLEAR_ERROR_FOR_GROUP,
+ TYPES.CLEAR_IS_DIRTY,
+ TYPES.SET_IS_REQUESTING,
+ ] );
+ expect( actions[ 0 ] ).toEqual( {
+ type: TYPES.SET_IS_REQUESTING,
+ group: GROUP,
+ isRequesting: true,
+ } );
+ expect( actions[ 3 ] ).toEqual( {
+ type: TYPES.SET_IS_REQUESTING,
+ group: GROUP,
+ isRequesting: false,
+ } );
+ } );
+
+ it( 'records the error before it stops requesting when the save fails', () => {
+ const apiError = new Error( 'Nope.' );
+ const { actions, thrown } = runPersist( { apiError } );
+
+ expect( actions.map( ( action ) => action.type ) ).toEqual( [
+ TYPES.SET_IS_REQUESTING,
+ TYPES.UPDATE_ERROR_FOR_GROUP,
+ TYPES.SET_IS_REQUESTING,
+ ] );
+ expect( actions[ 1 ] ).toMatchObject( {
+ group: GROUP,
+ data: null,
+ error: apiError,
+ } );
+ expect( actions[ 2 ] ).toEqual( {
+ type: TYPES.SET_IS_REQUESTING,
+ group: GROUP,
+ isRequesting: false,
+ } );
+ expect( thrown ).toBe( apiError );
+ } );
+} );
diff --git a/packages/js/data/src/settings/test/reducer.ts b/packages/js/data/src/settings/test/reducer.ts
new file mode 100644
index 00000000000..c6058f6445c
--- /dev/null
+++ b/packages/js/data/src/settings/test/reducer.ts
@@ -0,0 +1,116 @@
+/**
+ * Internal dependencies
+ */
+import reducer from '../reducer';
+import {
+ clearErrorForGroup,
+ clearIsDirty,
+ setIsRequesting,
+ updateErrorForGroup,
+ updateSettingsForGroup,
+} from '../actions';
+import {
+ getDirtyKeys,
+ getLastSettingsErrorForGroup,
+ isUpdateSettingsRequesting,
+} from '../selectors';
+import { SettingsState } from '../types';
+
+const GROUP = 'wc_admin';
+
+const edit = ( state: SettingsState, value: unknown ) =>
+ reducer(
+ state,
+ updateSettingsForGroup( GROUP, {
+ wcAdminSettings: { woocommerce_default_date_range: value },
+ } )
+ );
+
+describe( 'settings reducer', () => {
+ describe( 'UPDATE_ERROR_FOR_GROUP', () => {
+ it( 'keeps the dirty keys so a failed save can be retried', () => {
+ let state = edit( {}, 'period=month' );
+ expect( getDirtyKeys( state, GROUP ) ).toEqual( [
+ 'wcAdminSettings',
+ ] );
+
+ state = reducer(
+ state,
+ updateErrorForGroup( GROUP, null, new Error( 'Nope.' ) )
+ );
+
+ expect( getDirtyKeys( state, GROUP ) ).toEqual( [
+ 'wcAdminSettings',
+ ] );
+ } );
+
+ it( 'does not leave the group stuck in a requesting state', () => {
+ // Mirrors the getSettings resolver, which flags the group as
+ // requesting and never resets it when the fetch fails.
+ let state = reducer( {}, setIsRequesting( GROUP, true ) );
+ state = reducer(
+ state,
+ updateErrorForGroup( GROUP, null, new Error( 'Nope.' ) )
+ );
+
+ expect( getLastSettingsErrorForGroup( state, GROUP ) ).toBeTruthy();
+ expect( isUpdateSettingsRequesting( state, GROUP ) ).toBe( false );
+ } );
+ } );
+
+ describe( 'CLEAR_ERROR_FOR_GROUP', () => {
+ it( 'clears an error left over from a previous failed save', () => {
+ let state = edit( {}, 'period=month' );
+ state = reducer(
+ state,
+ updateErrorForGroup( GROUP, null, new Error( 'Nope.' ) )
+ );
+ expect( getLastSettingsErrorForGroup( state, GROUP ) ).toBeTruthy();
+
+ state = reducer( state, clearErrorForGroup( GROUP ) );
+
+ expect( getLastSettingsErrorForGroup( state, GROUP ) ).toBe(
+ false
+ );
+ } );
+
+ it( 'leaves the dirty keys alone', () => {
+ let state = edit( {}, 'period=month' );
+ state = reducer( state, clearErrorForGroup( GROUP ) );
+
+ expect( getDirtyKeys( state, GROUP ) ).toEqual( [
+ 'wcAdminSettings',
+ ] );
+ } );
+
+ it( 'does not create a group that was never loaded', () => {
+ const state = reducer( {}, clearErrorForGroup( GROUP ) );
+
+ expect( state ).toEqual( {} );
+ expect( getLastSettingsErrorForGroup( state, GROUP ) ).toBe(
+ false
+ );
+ } );
+ } );
+
+ it( 'reports no error once a retried save succeeds', () => {
+ // Edit a setting, fail the save, then retry it successfully.
+ let state = edit( {}, 'period=month' );
+ state = reducer(
+ state,
+ updateErrorForGroup( GROUP, null, new Error( 'Nope.' ) )
+ );
+ state = reducer( state, setIsRequesting( GROUP, false ) );
+
+ // The retry still has something to send.
+ expect( getDirtyKeys( state, GROUP ) ).toEqual( [ 'wcAdminSettings' ] );
+
+ state = reducer( state, clearErrorForGroup( GROUP ) );
+ state = reducer( state, clearIsDirty( GROUP ) );
+ state = reducer( state, setIsRequesting( GROUP, false ) );
+
+ expect( getLastSettingsErrorForGroup( state, GROUP ) ).toBe( false );
+ expect( getDirtyKeys( state, GROUP ) ).toEqual( [] );
+ expect( isUpdateSettingsRequesting( state, GROUP ) ).toBe( false );
+ } );
+} );
diff --git a/plugins/woocommerce/changelog/fix-wooairr-102-settings-stale-error b/plugins/woocommerce/changelog/fix-wooairr-102-settings-stale-error
new file mode 100644
index 00000000000..ad1a6d2776f
--- /dev/null
+++ b/plugins/woocommerce/changelog/fix-wooairr-102-settings-stale-error
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Fix Analytics settings so a failed save can be retried and no longer reports an error after it succeeds.