Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[KED-2181] Deprecate pipelines flag #294

Merged
merged 1 commit into from
Nov 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/actions/pipelines.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { resetData } from './index';
* 4. Whether localStorage has an active pipeline already defined.
* 5. If so, whether it exists in the current dataset.
* 6. Whether the requested endpoint is the 'main' one, or not.
* 7. Whether the pipeline flag is disabled.
*
* These are mostly handled either within this file, in the preparePipelineState
* utility, or in the getNodeDisabledPipeline selector. Please see their tests
Expand Down Expand Up @@ -62,14 +61,10 @@ export const getPipelineUrl = pipeline => {
* Check whether another async data pipeline request is needed on first page-load.
* A second request is typically only required when an active pipeline is set in
* localStorage, and it's not the 'main' pipeline endpoint.
* @param {object} flags Flag state
* @param {object} pipeline Pipeline state
* @return {boolean} True if another request is needed
*/
export const requiresSecondRequest = (flags, pipeline) => {
// Pipelines are disabled via flags
// TODO: Delete this line when removing flags.pipeline
if (!flags.pipelines) return false;
export const requiresSecondRequest = pipeline => {
// Pipelines are not present in the data
if (!pipeline.ids.length || !pipeline.main) return false;
// There is no active pipeline set
Expand Down Expand Up @@ -99,7 +94,7 @@ export function loadInitialPipelineData() {
preparePipelineState(data, true)
);
// If the active pipeline isn't 'main' then request data from new URL
if (requiresSecondRequest(state.flags, newState.pipeline)) {
if (requiresSecondRequest(newState.pipeline)) {
const url = getPipelineUrl(newState.pipeline);
newState = await loadJsonData(url).then(preparePipelineState);
}
Expand Down
25 changes: 4 additions & 21 deletions src/actions/pipelines.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { createStore } from 'redux';
import reducer from '../reducers';
import { changeFlag } from './index';
import { mockState } from '../utils/state.mock';
import { saveState } from '../store/helpers';
import {
Expand Down Expand Up @@ -56,30 +55,24 @@ describe('pipeline actions', () => {
});

describe('requiresSecondRequest', () => {
it('should return false if the pipelines flag is false', () => {
expect(requiresSecondRequest({ pipelines: false }, {})).toBe(false);
});

const flags = { pipelines: true };

it('should return false if pipelines are not present in the data', () => {
const pipeline = { ids: [], active: 'a' };
expect(requiresSecondRequest(flags, pipeline)).toBe(false);
expect(requiresSecondRequest(pipeline)).toBe(false);
});

it('should return false if there is no active pipeline', () => {
const pipeline = { ids: ['a', 'b'], main: 'a' };
expect(requiresSecondRequest(flags, pipeline)).toBe(false);
expect(requiresSecondRequest(pipeline)).toBe(false);
});

it('should return false if the main pipeline is active', () => {
const pipeline = { ids: ['a', 'b'], main: 'a', active: 'a' };
expect(requiresSecondRequest(flags, pipeline)).toBe(false);
expect(requiresSecondRequest(pipeline)).toBe(false);
});

it('should return true if the main pipeline is not active', () => {
const pipeline = { ids: ['a', 'b'], main: 'a', active: 'b' };
expect(requiresSecondRequest(flags, pipeline)).toBe(true);
expect(requiresSecondRequest(pipeline)).toBe(true);
});
});

Expand Down Expand Up @@ -142,16 +135,6 @@ describe('pipeline actions', () => {
expect(state.node).toEqual(mockState.animals.node);
});

it("shouldn't make a second data request if the pipeline flag is false", async () => {
const { pipeline } = mockState.animals;
const active = pipeline.ids.find(id => id !== pipeline.main);
saveState({ pipeline: { active } });
const state = reducer(mockState.json, changeFlag('pipelines', false));
const store = createStore(reducer, state);
await loadInitialPipelineData()(store.dispatch, store.getState);
expect(store.getState().node).toEqual(mockState.animals.node);
});

it("shouldn't make a second data request if the dataset doesn't support pipelines", async () => {
window.deletePipelines = true; // pass option to load-data mock
const { pipeline } = mockState.animals;
Expand Down
6 changes: 2 additions & 4 deletions src/components/sidebar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ import './sidebar.css';

/**
* Main app container. Handles showing/hiding the sidebar nav, and theme classes.
* @param {object} props.flags Feature flags from URL/localStorage
* @param {boolean} props.visible Whether the sidebar is open/closed
*/
export const Sidebar = ({ flags, visible }) => {
export const Sidebar = ({ visible }) => {
const [pipelineIsOpen, togglePipeline] = useState(false);

return (
Expand All @@ -23,7 +22,7 @@ export const Sidebar = ({ flags, visible }) => {
'pipeline-sidebar--visible': visible
})}>
<div className="pipeline-ui">
{flags.pipelines && <PipelineList onToggleOpen={togglePipeline} />}
<PipelineList onToggleOpen={togglePipeline} />
<NodeList faded={pipelineIsOpen} />
</div>
<nav className="pipeline-toolbar">
Expand All @@ -37,7 +36,6 @@ export const Sidebar = ({ flags, visible }) => {
};

const mapStateToProps = state => ({
flags: state.flags,
visible: state.visible.sidebar
});

Expand Down
5 changes: 0 additions & 5 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ export const flags = {
default: false,
icon: '📈'
},
pipelines: {
description: 'Select from multiple pipelines',
default: typeof jest !== 'undefined',
icon: '🔀'
},
meta: {
description: 'Show the metadata panel',
default: false,
Expand Down
13 changes: 3 additions & 10 deletions src/selectors/pipeline.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,14 @@ const getNodePipelines = state => state.node.pipelines;
const getActivePipeline = state => state.pipeline.active;
const getNodeTags = state => state.node.tags;
const getAsyncDataSource = state => state.asyncDataSource;
const getPipelineFlag = state => state.flags.pipelines;

/**
* Calculate whether nodes should be disabled based on their tags
*/
export const getNodeDisabledPipeline = createSelector(
[
getNodeIDs,
getNodePipelines,
getActivePipeline,
getAsyncDataSource,
getPipelineFlag
],
(nodeIDs, nodePipelines, activePipeline, asyncDataSource, pipelineFlag) => {
if (asyncDataSource || !activePipeline || !pipelineFlag) {
[getNodeIDs, getNodePipelines, getActivePipeline, getAsyncDataSource],
(nodeIDs, nodePipelines, activePipeline, asyncDataSource) => {
if (asyncDataSource || !activePipeline) {
return {};
}
return arrayToObject(
Expand Down
9 changes: 0 additions & 9 deletions src/selectors/pipeline.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
getPipelineTagIDs
} from './pipeline';
import reducer from '../reducers';
import { changeFlag } from '../actions';
import { updateActivePipeline } from '../actions/pipelines';

const getNodeIDs = state => state.node.ids;
Expand Down Expand Up @@ -85,14 +84,6 @@ describe('Selectors', () => {
);
expect(getNodeDisabledPipeline(newMockState)).toEqual({});
});

it('does not disable any nodes if the pipelines flag is false', () => {
const newMockState = reducer(
mockState.animals,
changeFlag('pipelines', false)
);
expect(getNodeDisabledPipeline(newMockState)).toEqual({});
});
});

describe('getPipelineNodeIDs', () => {
Expand Down
3 changes: 1 addition & 2 deletions src/store/initial-state.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ describe('prepareNonPipelineState', () => {
// In this case, location.href is not provided
expect(prepareNonPipelineState({ data: animals })).toMatchObject({
flags: {
newgraph: expect.any(Boolean),
pipelines: expect.any(Boolean)
newgraph: expect.any(Boolean)
}
});
});
Expand Down