From 9235eed26cbe13b99ecca6e3f7e4552ac72aa219 Mon Sep 17 00:00:00 2001 From: budzanowski Date: Fri, 2 Dec 2016 13:48:43 +0100 Subject: [PATCH 1/2] Add themeRequestError, getThemeRequestError and their tests. --- client/state/themes/reducer.js | 21 +++++ client/state/themes/selectors.js | 16 ++++ client/state/themes/test/reducer.js | 107 ++++++++++++++++++++++++++ client/state/themes/test/selectors.js | 31 ++++++++ 4 files changed, 175 insertions(+) diff --git a/client/state/themes/reducer.js b/client/state/themes/reducer.js index b1ab7c972f35e..3b8815a6ab73a 100644 --- a/client/state/themes/reducer.js +++ b/client/state/themes/reducer.js @@ -161,6 +161,26 @@ export function themeRequests( state = {}, action ) { return state; } +/** + * Returns the updated site theme requests error state after an action has been + * dispatched. The state reflects a mapping of site ID, theme ID pairing to a + * object describing request error. If there is no error null is storred. + * + * @param {Object} state Current state + * @param {Object} action Action payload + * @return {Object} Updated state + */ +export const themeRequestsError = createReducer( {}, { + [ THEME_REQUEST_FAILURE ]: ( state, { siteId, themeId, error } ) => ( { + ...state, + [ siteId ]: Object.assign( {}, state[ siteId ], { [ themeId ]: { error } } ) + } ), + [ THEME_REQUEST_SUCCESS ]: ( state, { siteId, themeId } ) => ( { + ...state, + [ siteId ]: Object.assign( {}, state[ siteId ], { [ themeId ]: null } ) + } ) +} ); + /** * Returns the updated theme query requesting state after an action has been * dispatched. The state reflects a mapping of serialized query to whether a @@ -267,6 +287,7 @@ export default combineReducers( { // queryRequests, // lastQuery themeRequests, + // themeRequestsError activeThemes, activeThemeRequests, activationRequests, diff --git a/client/state/themes/selectors.js b/client/state/themes/selectors.js index 160093d915767..9ecf00351c200 100644 --- a/client/state/themes/selectors.js +++ b/client/state/themes/selectors.js @@ -59,6 +59,22 @@ export const getTheme = createSelector( ( state ) => state.themes.queries ); +/** + * Returns theme request error object + * + * @param {Object} state Global state tree + * @param {String} themeId Theme ID + * @param {Number} siteId Site ID + * @return {Object} error object if present or null otherwise + */ +export function getThemeRequestsError( state, themeId, siteId ) { + const themes = get( state.themes.themeRequestsError, siteId, null ); + if ( ! themes ) { + return null; + } + return get( themes, themeId, null ); +} + /** * Returns an array of normalized themes for the themes query, or null if no * themes have been received. diff --git a/client/state/themes/test/reducer.js b/client/state/themes/test/reducer.js index 6fc8af32127c7..db3735374c836 100644 --- a/client/state/themes/test/reducer.js +++ b/client/state/themes/test/reducer.js @@ -30,6 +30,7 @@ import reducer, { queries, lastQuery, themeRequests, + themeRequestsError, activeThemes, activationRequests, activeThemeRequests, @@ -464,6 +465,112 @@ describe( 'reducer', () => { } ); } ); + describe( '#themeRequestsError()', () => { + it( 'should default to an empty object', () => { + const state = themeRequestsError( undefined, {} ); + + expect( state ).to.deep.equal( {} ); + } ); + + it( 'should map site ID, theme ID to null value if request was successful', () => { + const state = themeRequestsError( deepFreeze( {} ), { + type: THEME_REQUEST_SUCCESS, + siteId: 2916284, + themeId: 841 + } ); + + expect( state ).to.deep.equal( { + 2916284: { + 841: null + } + } ); + } ); + + it( 'should map site ID, theme ID to error object if request finishes with failure', () => { + const state = themeRequestsError( deepFreeze( {} ), { + type: THEME_REQUEST_FAILURE, + siteId: 2916284, + themeId: 'vivaro', + error: 'Request error' + } ); + + expect( state ).to.deep.equal( { + 2916284: { + vivaro: { + error: 'Request error' + } + } + } ); + } ); + + it( 'should switch from error to null after successful request after a failure', () => { + const state = themeRequestsError( deepFreeze( { + 2916284: { + pinboard: { + error: 'Request Error' + } + } + } ), { + type: THEME_REQUEST_SUCCESS, + siteId: 2916284, + themeId: 'pinboard' + } ); + + expect( state ).to.deep.equal( { + 2916284: { + pinboard: null + } + } ); + } ); + + it( 'should accumulate mappings', () => { + const state = themeRequestsError( deepFreeze( { + 2916284: { + twentysixteennnnn: { + error: 'No such theme!' + } + } + } ), { + type: THEME_REQUEST_SUCCESS, + siteId: 2916284, + themeId: 'twentysixteen' + } ); + + expect( state ).to.deep.equal( { + 2916284: { + twentysixteen: null, + twentysixteennnnn: { + error: 'No such theme!' + } + } + } ); + } ); + + it( 'never persists state', () => { + const state = themeRequestsError( deepFreeze( { + 2916284: { + twentysixteen: null + } + } ), { + type: SERIALIZE + } ); + + expect( state ).to.deep.equal( {} ); + } ); + + it( 'never loads persisted state', () => { + const state = themeRequestsError( deepFreeze( { + 2916284: { + twentysixteen: null + } + } ), { + type: DESERIALIZE + } ); + + expect( state ).to.deep.equal( {} ); + } ); + } ); + describe( '#activeThemes()', () => { it( 'should default to an empty object', () => { const state = activeThemes( undefined, {} ); diff --git a/client/state/themes/test/selectors.js b/client/state/themes/test/selectors.js index 9a0896f328bdd..310758be24a7c 100644 --- a/client/state/themes/test/selectors.js +++ b/client/state/themes/test/selectors.js @@ -10,6 +10,7 @@ import { values } from 'lodash'; import { getThemes, getTheme, + getThemeRequestsError, isRequestingTheme, getThemesForQuery, getLastThemeQuery, @@ -130,6 +131,36 @@ describe( 'themes selectors', () => { } ); } ); + describe( '#getThemesRequestError()', () => { + it( 'should return null if thre is not request error storred for that theme on site', () => { + const error = getThemeRequestsError( { + themes: { + themeRequestsError: {} + } + }, 'twentysixteen', 413 ); + + expect( error ).to.be.null; + } ); + + it( 'should return the error object for the site ID, theme ID pair', () => { + const error = getThemeRequestsError( { + themes: { + themeRequestsError: { + 2916284: { + twentysixteen: { + error: 'Request error' + } + } + } + } + }, 'twentysixteen', 2916284, ); + + expect( error ).to.deep.equal( { + error: 'Request error' + } ); + } ); + } ); + describe( '#isRequestingTheme()', () => { it( 'should return false if there are no active requests for site', () => { const isRequesting = isRequestingTheme( { From b88b09debc0ce2c92416b58df8a566f8f154a7ef Mon Sep 17 00:00:00 2001 From: budzanowski Date: Fri, 2 Dec 2016 18:49:09 +0100 Subject: [PATCH 2/2] Address review findings. --- client/state/themes/reducer.js | 13 ++++--- client/state/themes/selectors.js | 8 +--- client/state/themes/test/reducer.js | 53 +++++++++++---------------- client/state/themes/test/selectors.js | 18 ++++----- 4 files changed, 39 insertions(+), 53 deletions(-) diff --git a/client/state/themes/reducer.js b/client/state/themes/reducer.js index 3b8815a6ab73a..8861dbd4f6ac7 100644 --- a/client/state/themes/reducer.js +++ b/client/state/themes/reducer.js @@ -2,7 +2,7 @@ * External dependencies */ import { combineReducers } from 'redux'; -import { mapValues } from 'lodash'; +import { mapValues, omit } from 'lodash'; /** * Internal dependencies @@ -170,14 +170,17 @@ export function themeRequests( state = {}, action ) { * @param {Object} action Action payload * @return {Object} Updated state */ -export const themeRequestsError = createReducer( {}, { +export const themeRequestErrors = createReducer( {}, { [ THEME_REQUEST_FAILURE ]: ( state, { siteId, themeId, error } ) => ( { ...state, - [ siteId ]: Object.assign( {}, state[ siteId ], { [ themeId ]: { error } } ) + [ siteId ]: { + ...state[ siteId ], + [ themeId ]: error + } } ), [ THEME_REQUEST_SUCCESS ]: ( state, { siteId, themeId } ) => ( { ...state, - [ siteId ]: Object.assign( {}, state[ siteId ], { [ themeId ]: null } ) + [ siteId ]: omit( state[ siteId ], themeId ), } ) } ); @@ -287,7 +290,7 @@ export default combineReducers( { // queryRequests, // lastQuery themeRequests, - // themeRequestsError + // themeRequestErrors activeThemes, activeThemeRequests, activationRequests, diff --git a/client/state/themes/selectors.js b/client/state/themes/selectors.js index 9ecf00351c200..96f1f8a3fc719 100644 --- a/client/state/themes/selectors.js +++ b/client/state/themes/selectors.js @@ -67,12 +67,8 @@ export const getTheme = createSelector( * @param {Number} siteId Site ID * @return {Object} error object if present or null otherwise */ -export function getThemeRequestsError( state, themeId, siteId ) { - const themes = get( state.themes.themeRequestsError, siteId, null ); - if ( ! themes ) { - return null; - } - return get( themes, themeId, null ); +export function getThemeRequestErrors( state, themeId, siteId ) { + return get( state.themes.themeRequestErrors, [ siteId, themeId ], null ); } /** diff --git a/client/state/themes/test/reducer.js b/client/state/themes/test/reducer.js index db3735374c836..bac0268f47bb9 100644 --- a/client/state/themes/test/reducer.js +++ b/client/state/themes/test/reducer.js @@ -30,7 +30,7 @@ import reducer, { queries, lastQuery, themeRequests, - themeRequestsError, + themeRequestErrors, activeThemes, activationRequests, activeThemeRequests, @@ -465,29 +465,27 @@ describe( 'reducer', () => { } ); } ); - describe( '#themeRequestsError()', () => { + describe( '#themeRequestErrors()', () => { it( 'should default to an empty object', () => { - const state = themeRequestsError( undefined, {} ); + const state = themeRequestErrors( undefined, {} ); expect( state ).to.deep.equal( {} ); } ); - it( 'should map site ID, theme ID to null value if request was successful', () => { - const state = themeRequestsError( deepFreeze( {} ), { + it( 'should create empyt mapping on success if previous state was empty', () => { + const state = themeRequestErrors( deepFreeze( {} ), { type: THEME_REQUEST_SUCCESS, siteId: 2916284, - themeId: 841 + themeId: 'twentysixteen' } ); expect( state ).to.deep.equal( { - 2916284: { - 841: null - } + 2916284: {} } ); } ); - it( 'should map site ID, theme ID to error object if request finishes with failure', () => { - const state = themeRequestsError( deepFreeze( {} ), { + it( 'should map site ID, theme ID to error if request finishes with failure', () => { + const state = themeRequestErrors( deepFreeze( {} ), { type: THEME_REQUEST_FAILURE, siteId: 2916284, themeId: 'vivaro', @@ -496,15 +494,13 @@ describe( 'reducer', () => { expect( state ).to.deep.equal( { 2916284: { - vivaro: { - error: 'Request error' - } + vivaro: 'Request error' } } ); } ); - it( 'should switch from error to null after successful request after a failure', () => { - const state = themeRequestsError( deepFreeze( { + it( 'should switch from error to no mapping after successful request after a failure', () => { + const state = themeRequestErrors( deepFreeze( { 2916284: { pinboard: { error: 'Request Error' @@ -517,37 +513,32 @@ describe( 'reducer', () => { } ); expect( state ).to.deep.equal( { - 2916284: { - pinboard: null - } + 2916284: {} } ); } ); it( 'should accumulate mappings', () => { - const state = themeRequestsError( deepFreeze( { + const state = themeRequestErrors( deepFreeze( { 2916284: { - twentysixteennnnn: { - error: 'No such theme!' - } + twentysixteennnnn: 'No such theme!' } } ), { - type: THEME_REQUEST_SUCCESS, + type: THEME_REQUEST_FAILURE, siteId: 2916284, - themeId: 'twentysixteen' + themeId: 'twentysixteen', + error: 'System error' } ); expect( state ).to.deep.equal( { 2916284: { - twentysixteen: null, - twentysixteennnnn: { - error: 'No such theme!' - } + twentysixteennnnn: 'No such theme!', + twentysixteen: 'System error' } } ); } ); it( 'never persists state', () => { - const state = themeRequestsError( deepFreeze( { + const state = themeRequestErrors( deepFreeze( { 2916284: { twentysixteen: null } @@ -559,7 +550,7 @@ describe( 'reducer', () => { } ); it( 'never loads persisted state', () => { - const state = themeRequestsError( deepFreeze( { + const state = themeRequestErrors( deepFreeze( { 2916284: { twentysixteen: null } diff --git a/client/state/themes/test/selectors.js b/client/state/themes/test/selectors.js index 310758be24a7c..8e982e792407b 100644 --- a/client/state/themes/test/selectors.js +++ b/client/state/themes/test/selectors.js @@ -10,7 +10,7 @@ import { values } from 'lodash'; import { getThemes, getTheme, - getThemeRequestsError, + getThemeRequestErrors, isRequestingTheme, getThemesForQuery, getLastThemeQuery, @@ -133,9 +133,9 @@ describe( 'themes selectors', () => { describe( '#getThemesRequestError()', () => { it( 'should return null if thre is not request error storred for that theme on site', () => { - const error = getThemeRequestsError( { + const error = getThemeRequestErrors( { themes: { - themeRequestsError: {} + themeRequestErrors: {} } }, 'twentysixteen', 413 ); @@ -143,21 +143,17 @@ describe( 'themes selectors', () => { } ); it( 'should return the error object for the site ID, theme ID pair', () => { - const error = getThemeRequestsError( { + const error = getThemeRequestErrors( { themes: { - themeRequestsError: { + themeRequestErrors: { 2916284: { - twentysixteen: { - error: 'Request error' - } + twentysixteen: 'Request error' } } } }, 'twentysixteen', 2916284, ); - expect( error ).to.deep.equal( { - error: 'Request error' - } ); + expect( error ).to.equal( 'Request error' ); } ); } );