diff --git a/RELEASE.md b/RELEASE.md index bfb4d2d918..c285a2b74e 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -15,6 +15,7 @@ Please follow the established format: - Refactor backend integration with Kedro by replacing bootstrap_project with configure_project. (#1796) - Enhance kedro-viz doc integration. (#1874) - Fix Kedro-Viz waiting for valid Kedro project. (#1871) +- Include expandAllPipelines in initial state. (#1896) - Enhance Kedro-Viz documentation by using Kedro-sphinx-theme. (#1898) - Remove default props from functional components. (#1906) - Fix for schema change in strawberry-graphql JSON scalar. (#1903) diff --git a/src/actions/actions.test.js b/src/actions/actions.test.js index 4778ed741b..fde1e1b162 100644 --- a/src/actions/actions.test.js +++ b/src/actions/actions.test.js @@ -16,6 +16,7 @@ import { TOGGLE_CODE, TOGGLE_MODULAR_PIPELINE_FOCUS_MODE, TOGGLE_HOVERED_FOCUS_MODE, + TOGGLE_EXPAND_ALL_PIPELINES, changeFlag, resetData, toggleIgnoreLargeWarning, @@ -32,6 +33,7 @@ import { updateChartSize, toggleFocusMode, toggleHoveredFocusMode, + toggleExpandAllPipelines, } from '../actions'; import { TOGGLE_NODE_CLICKED, @@ -75,7 +77,18 @@ describe('actions', () => { expect(toggleLayers(visible)).toEqual(expectedAction); }); - it('should create an action to toggle whether to show layers', () => { + it('should create an action to toggle whether to expand all modular pipelines or collapse', () => { + const shouldExpandAllPipelines = false; + const expectedAction = { + type: TOGGLE_EXPAND_ALL_PIPELINES, + shouldExpandAllPipelines, + }; + expect(toggleExpandAllPipelines(shouldExpandAllPipelines)).toEqual( + expectedAction + ); + }); + + it('should create an action to toggle whether to show minimap', () => { const visible = false; const expectedAction = { type: TOGGLE_MINIMAP, diff --git a/src/actions/index.js b/src/actions/index.js index 973e16d531..47e83197af 100644 --- a/src/actions/index.js +++ b/src/actions/index.js @@ -24,6 +24,19 @@ export function toggleLayers(visible) { }; } +export const TOGGLE_EXPAND_ALL_PIPELINES = 'TOGGLE_EXPAND_ALL_PIPELINES'; + +/** + * Toggle whether to expand all modular pipelines or collapse + * @param {Boolean} shouldExpandAllPipelines + */ +export function toggleExpandAllPipelines(shouldExpandAllPipelines) { + return { + type: TOGGLE_EXPAND_ALL_PIPELINES, + shouldExpandAllPipelines, + }; +} + export const TOGGLE_EXPORT_MODAL = 'TOGGLE_EXPORT_MODAL'; /** diff --git a/src/actions/pipelines.js b/src/actions/pipelines.js index 06e1bc30e6..8882b425d8 100644 --- a/src/actions/pipelines.js +++ b/src/actions/pipelines.js @@ -99,7 +99,7 @@ export function loadInitialPipelineData() { // obtain the status of expandAllPipelines to decide whether it needs to overwrite the // list of visible nodes const expandAllPipelines = - state.display.expandAllPipelines || state.flags.expandAllPipelines; + state.display.expandAllPipelines || state.expandAllPipelines; let newState = await loadJsonData(url).then((data) => preparePipelineState(data, true, expandAllPipelines) ); @@ -122,7 +122,7 @@ export function loadInitialPipelineData() { */ export function loadPipelineData(pipelineID) { return async function (dispatch, getState) { - const { dataSource, pipeline, display, flags } = getState(); + const { dataSource, pipeline, display, expandAllPipelines } = getState(); if (pipelineID && pipelineID === pipeline.active) { return; @@ -136,10 +136,10 @@ export function loadPipelineData(pipelineID) { active: pipelineID, }); - const expandAllPipelines = - display.expandAllPipelines || flags.expandAllPipelines; + const shouldExpandAllPipelines = + display.expandAllPipelines || expandAllPipelines; const newState = await loadJsonData(url).then((data) => - preparePipelineState(data, false, expandAllPipelines) + preparePipelineState(data, false, shouldExpandAllPipelines) ); // Set active pipeline here rather than dispatching two separate actions, diff --git a/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.js b/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.js index 844fc7ec90..d40e6ecfeb 100644 --- a/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.js +++ b/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.js @@ -5,7 +5,7 @@ import { toggleLayers, toggleSidebar, toggleTextLabels, - changeFlag, + toggleExpandAllPipelines, } from '../../actions'; import { loadInitialPipelineData } from '../../actions/pipelines'; import IconButton from '../ui/icon-button'; @@ -110,7 +110,7 @@ export const mapStateToProps = (state) => ({ textLabels: state.textLabels, visible: state.visible, visibleLayers: Boolean(getVisibleLayerIDs(state).length), - expandedPipelines: state.flags.expandAllPipelines, + expandedPipelines: state.expandAllPipelines, }); export const mapDispatchToProps = (dispatch) => ({ @@ -127,7 +127,7 @@ export const mapDispatchToProps = (dispatch) => ({ dispatch(toggleTextLabels(Boolean(value))); }, onToggleExpandAllPipelines: (isExpanded) => { - dispatch(changeFlag('expandAllPipelines', isExpanded)); + dispatch(toggleExpandAllPipelines(isExpanded)); dispatch(loadInitialPipelineData()); }, }); diff --git a/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.test.js b/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.test.js index 732389f33c..a6f328c8a5 100644 --- a/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.test.js +++ b/src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.test.js @@ -70,6 +70,7 @@ describe('PrimaryToolbar', () => { const expectedResult = { disableLayerBtn: expect.any(Boolean), textLabels: expect.any(Boolean), + expandedPipelines: expect.any(Boolean), displaySidebar: true, visible: expect.objectContaining({ exportBtn: expect.any(Boolean), @@ -127,9 +128,8 @@ describe('PrimaryToolbar', () => { const dispatch = jest.fn(); mapDispatchToProps(dispatch).onToggleExpandAllPipelines(true); expect(dispatch.mock.calls[0][0]).toEqual({ - name: 'expandAllPipelines', - type: 'CHANGE_FLAG', - value: true, + type: 'TOGGLE_EXPAND_ALL_PIPELINES', + shouldExpandAllPipelines: true, }); }); }); diff --git a/src/components/flowchart-wrapper/flowchart-wrapper.js b/src/components/flowchart-wrapper/flowchart-wrapper.js index b305bc73ea..02750f59fc 100644 --- a/src/components/flowchart-wrapper/flowchart-wrapper.js +++ b/src/components/flowchart-wrapper/flowchart-wrapper.js @@ -110,9 +110,9 @@ export const FlowChartWrapper = ({ disabledKeys && toSetQueryParam(params.types, mappedDisabledNodes); } }, - flags: (value) => { + expandAllPipelines: (value) => { if (!searchParams.has(params.expandAll)) { - toSetQueryParam(params.expandAll, value.expandAllPipelines); + toSetQueryParam(params.expandAll, value); } }, }; diff --git a/src/reducers/index.js b/src/reducers/index.js index af858fdc50..606077d8d9 100644 --- a/src/reducers/index.js +++ b/src/reducers/index.js @@ -20,6 +20,7 @@ import { TOGGLE_THEME, UPDATE_CHART_SIZE, UPDATE_ZOOM, + TOGGLE_EXPAND_ALL_PIPELINES, } from '../actions'; import { TOGGLE_PARAMETERS_HOVERED } from '../actions'; @@ -95,6 +96,11 @@ const combinedReducer = combineReducers({ TOGGLE_HOVERED_FOCUS_MODE, 'hoveredFocusMode' ), + expandAllPipelines: createReducer( + false, + TOGGLE_EXPAND_ALL_PIPELINES, + 'shouldExpandAllPipelines' + ), }); const rootReducer = (state, action) => diff --git a/src/reducers/reducers.test.js b/src/reducers/reducers.test.js index 48aeeb0409..5778001bff 100644 --- a/src/reducers/reducers.test.js +++ b/src/reducers/reducers.test.js @@ -19,6 +19,7 @@ import { TOGGLE_THEME, UPDATE_CHART_SIZE, TOGGLE_HOVERED_FOCUS_MODE, + TOGGLE_EXPAND_ALL_PIPELINES, } from '../actions'; import { TOGGLE_NODE_CLICKED, @@ -241,6 +242,16 @@ describe('Reducer', () => { }); }); + describe('TOGGLE_EXPAND_ALL_PIPELINES', () => { + it('should toggle whether to expand all modular pipelines or collapse', () => { + const newState = reducer(mockState.spaceflights, { + type: TOGGLE_EXPAND_ALL_PIPELINES, + shouldExpandAllPipelines: true, + }); + expect(newState.expandAllPipelines).toEqual(true); + }); + }); + describe('TOGGLE_SIDEBAR', () => { it('should toggle whether the sidebar is open', () => { const newState = reducer(mockState.spaceflights, { diff --git a/src/store/initial-state.js b/src/store/initial-state.js index 8bc03021ea..9e65f9497f 100644 --- a/src/store/initial-state.js +++ b/src/store/initial-state.js @@ -2,7 +2,7 @@ import deepmerge from 'deepmerge'; import { loadLocalStorage } from './helpers'; import normalizeData from './normalize-data'; import { getFlagsFromUrl, Flags } from '../utils/flags'; -import { mapNodeType } from '../utils'; +import { mapNodeType, isValidBoolean } from '../utils'; import { settings, sidebarWidth, @@ -21,6 +21,7 @@ export const createInitialState = () => ({ flags: Flags.defaults(), textLabels: true, theme: 'dark', + expandAllPipelines: false, isPrettyName: settings.isPrettyName.default, showFeatureHints: settings.showFeatureHints.default, ignoreLargeWarning: false, @@ -67,19 +68,20 @@ const parseUrlParameters = () => { nodeTagInUrl: search.get(params.tags) ? search.get(params.tags).split(',') : [], + expandAllPipelinesInUrl: search.get(params.expandAll), }; }; /** - * Applies URL parameters to the application state. - * This function modifies the state based on URL parameters such as + * Applies URL parameters to the application pipeline state. + * This function modifies the state based on the URL parameters such as * pipeline ID, node ID, node name, node type presence, and tag presence. * - * @param {Object} state The current application state. + * @param {Object} state The current application pipeline state. * @param {Object} urlParams An object containing parsed URL parameters. - * @returns {Object} The new state with modifications applied based on URL parameters. + * @returns {Object} The new state with modifications applied based on the URL parameters. */ -const applyUrlParametersToState = (state, urlParams) => { +const applyUrlParametersToPipelineState = (state, urlParams) => { const { pipelineIdFromURL, nodeIdFromUrl, @@ -130,6 +132,24 @@ const applyUrlParametersToState = (state, urlParams) => { return newState; }; +/** + * Applies URL parameters to the application non-pipeline state. + * This function modifies the state based on the URL parameters such as + * expandAllPipelines presence. + * + * @param {Object} state The current application non-pipeline state. + * @param {Object} urlParams An object containing parsed URL parameters. + * @returns {Object} The new state with modifications applied based on the URL parameters. + */ +const applyUrlParametersToNonPipelineState = (state, urlParams) => { + const { expandAllPipelinesInUrl } = urlParams; + let newState = { ...state }; + if (expandAllPipelinesInUrl && isValidBoolean(expandAllPipelinesInUrl)) { + newState.expandAllPipelines = JSON.parse(expandAllPipelinesInUrl); + } + return newState; +}; + /** * Load values from localStorage and combine with existing state, * but filter out any unused values from localStorage @@ -142,7 +162,7 @@ export const mergeLocalStorage = (state) => { localStorageRunsMetadata ); Object.keys(localStorageState).forEach((key) => { - if (!state[key]) { + if (!(key in state)) { delete localStorageState[key]; } }); @@ -162,10 +182,17 @@ export const mergeLocalStorage = (state) => { * Exactly when it runs depends on whether the data is loaded asynchronously or not. * @param {Object} data Data prop passed to App component * @param {Boolean} applyFixes Whether to override initialState + * @param {Boolean} expandAllPipelines Whether to expand all the modular pipelines + * @param {Object} urlParams An object containing parsed URL parameters. + * @returns {Object} The new pipeline state with modifications applied. */ -export const preparePipelineState = (data, applyFixes, expandAllPipelines) => { +export const preparePipelineState = ( + data, + applyFixes, + expandAllPipelines, + urlParams +) => { let state = mergeLocalStorage(normalizeData(data, expandAllPipelines)); - const urlParams = parseUrlParameters(); if (applyFixes) { // Use main pipeline if active pipeline from localStorage isn't recognised @@ -173,7 +200,10 @@ export const preparePipelineState = (data, applyFixes, expandAllPipelines) => { state.pipeline.active = state.pipeline.main; } } - state = applyUrlParametersToState(state, urlParams); + if (urlParams) { + state = applyUrlParametersToPipelineState(state, urlParams); + } + return state; }; @@ -182,17 +212,25 @@ export const preparePipelineState = (data, applyFixes, expandAllPipelines) => { * will persist if the pipeline data is reset. * Merge local storage and add custom state overrides from props etc * @param {object} props Props passed to App component - * @return {object} Updated initial state + * @param {Object} urlParams An object containing parsed URL parameters. + * @returns {Object} The new non-pipeline state with modifications applied. */ -export const prepareNonPipelineState = (props) => { - const state = mergeLocalStorage(createInitialState()); +export const prepareNonPipelineState = (props, urlParams) => { + let state = mergeLocalStorage(createInitialState()); let newVisibleProps = {}; + if (props.display?.sidebar === false || state.display.sidebar === false) { newVisibleProps['sidebar'] = false; } + if (props.display?.minimap === false || state.display.miniMap === false) { newVisibleProps['miniMap'] = false; } + + if (urlParams) { + state = applyUrlParametersToNonPipelineState(state, urlParams); + } + return { ...state, flags: { ...state.flags, ...getFlagsFromUrl() }, @@ -210,16 +248,18 @@ export const prepareNonPipelineState = (props) => { * @return {Object} Initial state */ const getInitialState = (props = {}) => { - const nonPipelineState = prepareNonPipelineState(props); + const urlParams = parseUrlParameters(); + const nonPipelineState = prepareNonPipelineState(props, urlParams); const expandAllPipelines = nonPipelineState.display.expandAllPipelines || - nonPipelineState.flags.expandAllPipelines; + nonPipelineState.expandAllPipelines; const pipelineState = preparePipelineState( props.data, props.data !== 'json', - expandAllPipelines + expandAllPipelines, + urlParams ); return { diff --git a/src/store/initial-state.test.js b/src/store/initial-state.test.js index 56b3458e93..2ac6a3fb5f 100644 --- a/src/store/initial-state.test.js +++ b/src/store/initial-state.test.js @@ -20,6 +20,7 @@ describe('mergeLocalStorage', () => { textLabels: false, theme: 'light', tag: { enabled: 'medium' }, + expandAllPipelines: true, }; saveLocalStorage(localStorageName, localStorageValues); expect( @@ -27,6 +28,7 @@ describe('mergeLocalStorage', () => { textLabels: true, theme: 'dark', tag: { enabled: 'large' }, + expandAllPipelines: false, }) ).toMatchObject(localStorageValues); window.localStorage.clear(); @@ -70,10 +72,13 @@ describe('preparePipelineState', () => { describe('prepareNonPipelineState', () => { it('applies localStorage values on top of initial state', () => { - const localStorageState = { theme: 'foo' }; + const localStorageState = { theme: 'foo', expandAllPipelines: true }; saveLocalStorage(localStorageName, localStorageState); const state = prepareNonPipelineState({}); expect(state.theme).toEqual(localStorageState.theme); + expect(state.expandAllPipelines).toEqual( + localStorageState.expandAllPipelines + ); window.localStorage.clear(); }); @@ -101,6 +106,13 @@ describe('prepareNonPipelineState', () => { prepareNonPipelineState({ data: spaceflights, ...props }) ).toMatchObject(props); }); + + it('overrides expandAllPipelines with values from URL', () => { + // In this case, location.href is not provided + expect(prepareNonPipelineState({ data: spaceflights })).toMatchObject({ + expandAllPipelines: expect.any(Boolean), + }); + }); }); describe('getInitialState', () => { @@ -119,6 +131,7 @@ describe('getInitialState', () => { chartSize: {}, textLabels: true, theme: 'dark', + expandAllPipelines: false, visible: { exportBtn: true, labelBtn: true, diff --git a/src/store/normalize-data.js b/src/store/normalize-data.js index 6ad17baf44..3fc3715855 100644 --- a/src/store/normalize-data.js +++ b/src/store/normalize-data.js @@ -299,7 +299,7 @@ const normalizeData = (data, expandAllPipelines) => { data.modular_pipelines ); - // Case for expandAllPipelines in component props or within flag + // Case for expandAllPipelines in component props or within state if (expandAllPipelines) { // assign all modular pipelines into expanded state state.modularPipeline.expanded = state.modularPipeline.ids; diff --git a/src/store/store.js b/src/store/store.js index bbac8d2722..01bf864cf8 100644 --- a/src/store/store.js +++ b/src/store/store.js @@ -58,6 +58,7 @@ const saveStateToLocalStorage = (state) => { isPrettyName: state.isPrettyName, showFeatureHints: state.showFeatureHints, flags: state.flags, + expandAllPipelines: state.expandAllPipelines, }); // Store Run's metadata to localstorage diff --git a/src/utils/index.js b/src/utils/index.js index b8cd1888bf..d82777c7d2 100644 --- a/src/utils/index.js +++ b/src/utils/index.js @@ -243,3 +243,12 @@ export const mapNodeType = (nodeType) => nodeTypeMapObj[nodeType] || nodeType; export const mapNodeTypes = (nodeTypes) => { return nodeTypes.replace(/task|data/g, (matched) => mapNodeType(matched)); }; + +/** + * Test if the passed string value is valid boolean + * @param {String} inputString + * @returns {Boolean} true if the inputString is a valid boolean + */ +export const isValidBoolean = (inputString) => { + return /^(true|false)$/i.test(inputString); +}; diff --git a/src/utils/utils.test.js b/src/utils/utils.test.js index 9f6c565a0c..d63b9f3e55 100644 --- a/src/utils/utils.test.js +++ b/src/utils/utils.test.js @@ -4,6 +4,7 @@ import { unique, replaceMatches, replaceAngleBracketMatches, + isValidBoolean, } from './index'; describe('utils', () => { @@ -77,4 +78,21 @@ describe('utils', () => { expect(replaceAngleBracketMatches('')).toEqual('<lambda>'); }); }); + + describe('isValidBoolean', () => { + it('validates if the inputString is valid boolean', () => { + // Valid booleans + expect(isValidBoolean('true')).toEqual(true); + expect(isValidBoolean('false')).toEqual(true); + expect(isValidBoolean(true)).toEqual(true); + expect(isValidBoolean(false)).toEqual(true); + + // Invalid booleans + expect(isValidBoolean('0')).toEqual(false); + expect(isValidBoolean('undefined')).toEqual(false); + expect(isValidBoolean(undefined)).toEqual(false); + expect(isValidBoolean('trueTesting')).toEqual(false); + expect(isValidBoolean('Testingfalse')).toEqual(false); + }); + }); });