Skip to content

Commit

Permalink
[Security Solution][Detections] Pre-refactoring for the rule manageme…
Browse files Browse the repository at this point in the history
…nt table (#91302) (#91777)

**Related to:** #89877

## Summary

This is based on #89877 and the kind of pre-refactoring that has been done there.

Mainly this:

- consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc)
- removes some of the redundant state, leverages the reducer as the source of truth for state
- makes it easier to dispatch events, removes some of the noise

While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code).

### Checklist

- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
  • Loading branch information
kibanamachine and banderror authored Feb 18, 2021
1 parent 14282cc commit 4a79869
Show file tree
Hide file tree
Showing 15 changed files with 522 additions and 301 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ import styled from 'styled-components';
import React, { useMemo, useCallback, useState, useEffect } from 'react';

import * as i18n from '../../../pages/detection_engine/rules/translations';
import { enableRules } from '../../../containers/detection_engine/rules';
import { enableRules, RulesTableAction } from '../../../containers/detection_engine/rules';
import { enableRulesAction } from '../../../pages/detection_engine/rules/all/actions';
import { Action } from '../../../pages/detection_engine/rules/all/reducer';
import { useStateToaster, displayErrorToast } from '../../../../common/components/toasters';
import { bucketRulesResponse } from '../../../pages/detection_engine/rules/all/helpers';

Expand All @@ -33,7 +32,7 @@ const StaticSwitch = styled(EuiSwitch)`
StaticSwitch.displayName = 'StaticSwitch';

export interface RuleSwitchProps {
dispatch?: React.Dispatch<Action>;
dispatch?: React.Dispatch<RulesTableAction>;
id: string;
enabled: boolean;
isDisabled?: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ export * from './use_update_rule';
export * from './use_create_rule';
export * from './types';
export * from './use_rule';
export * from './use_rules';
export * from './rules_table';
export * from './use_pre_packaged_rules';
export * from './use_rule_status';
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export * from './rules_table_facade';
export * from './rules_table_reducer';
export * from './use_rules';
export * from './use_rules_table';
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import type { Dispatch } from 'react';
import { Rule, FilterOptions, PaginationOptions } from '../types';
import { RulesTableAction, LoadingRuleAction } from './rules_table_reducer';

export interface RulesTableFacade {
setRules(newRules: Rule[], newPagination: Partial<PaginationOptions>): void;
updateRules(rules: Rule[]): void;
updateOptions(filter: Partial<FilterOptions>, pagination: Partial<PaginationOptions>): void;
actionStarted(actionType: LoadingRuleAction, ruleIds: string[]): void;
actionStopped(): void;
setShowIdleModal(show: boolean): void;
setLastRefreshDate(): void;
setAutoRefreshOn(on: boolean): void;
}

export const createRulesTableFacade = (dispatch: Dispatch<RulesTableAction>): RulesTableFacade => {
return {
setRules: (newRules: Rule[], newPagination: Partial<PaginationOptions>) => {
dispatch({
type: 'setRules',
rules: newRules,
pagination: newPagination,
});
},

updateRules: (rules: Rule[]) => {
dispatch({
type: 'updateRules',
rules,
});
},

updateOptions: (filter: Partial<FilterOptions>, pagination: Partial<PaginationOptions>) => {
dispatch({
type: 'updateFilterOptions',
filterOptions: filter,
pagination,
});
},

actionStarted: (actionType: LoadingRuleAction, ruleIds: string[]) => {
dispatch({
type: 'loadingRuleIds',
actionType,
ids: ruleIds,
});
},

actionStopped: () => {
dispatch({
type: 'loadingRuleIds',
actionType: null,
ids: [],
});
},

setShowIdleModal: (show: boolean) => {
dispatch({
type: 'setShowIdleModal',
show,
});
},

setLastRefreshDate: () => {
dispatch({
type: 'setLastRefreshDate',
});
},

setAutoRefreshOn: (on: boolean) => {
dispatch({
type: 'setAutoRefreshOn',
on,
});
},
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@
* 2.0.
*/

import { FilterOptions, PaginationOptions } from '../../../../containers/detection_engine/rules';
import { mockRule } from '../../../../pages/detection_engine/rules/all/__mocks__/mock';
import { FilterOptions, PaginationOptions } from '../types';
import { RulesTableAction, RulesTableState, createRulesTableReducer } from './rules_table_reducer';

import { Action, State, allRulesReducer } from './reducer';
import { mockRule } from './__mocks__/mock';

const initialState: State = {
exportRuleIds: [],
const initialState: RulesTableState = {
rules: [],
pagination: {
page: 1,
perPage: 20,
total: 0,
},
filterOptions: {
filter: '',
sortField: 'enabled',
Expand All @@ -20,29 +24,24 @@ const initialState: State = {
showCustomRules: false,
showElasticRules: false,
},
loadingRuleIds: [],
loadingRulesAction: null,
pagination: {
page: 1,
perPage: 20,
total: 0,
},
rules: [],
loadingRuleIds: [],
selectedRuleIds: [],
exportRuleIds: [],
lastUpdated: 0,
showIdleModal: false,
isRefreshOn: false,
showIdleModal: false,
};

describe('allRulesReducer', () => {
let reducer: (state: State, action: Action) => State;
let reducer: (state: RulesTableState, action: RulesTableAction) => RulesTableState;

beforeEach(() => {
jest.useFakeTimers();
jest
.spyOn(global.Date, 'now')
.mockImplementationOnce(() => new Date('2020-10-31T11:01:58.135Z').valueOf());
reducer = allRulesReducer({ current: undefined });
reducer = createRulesTableReducer({ current: undefined });
});

afterEach(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import type React from 'react';
import { EuiBasicTable } from '@elastic/eui';
import { FilterOptions, PaginationOptions, Rule } from '../types';

export type LoadingRuleAction =
| 'load'
| 'duplicate'
| 'enable'
| 'disable'
| 'export'
| 'delete'
| null;

export interface RulesTableState {
rules: Rule[];
pagination: PaginationOptions;
filterOptions: FilterOptions;
loadingRulesAction: LoadingRuleAction;
loadingRuleIds: string[];
selectedRuleIds: string[];
exportRuleIds: string[];
lastUpdated: number;
isRefreshOn: boolean;
showIdleModal: boolean;
}

export type RulesTableAction =
| { type: 'setRules'; rules: Rule[]; pagination: Partial<PaginationOptions> }
| { type: 'updateRules'; rules: Rule[] }
| {
type: 'updateFilterOptions';
filterOptions: Partial<FilterOptions>;
pagination: Partial<PaginationOptions>;
}
| { type: 'loadingRuleIds'; ids: string[]; actionType: LoadingRuleAction }
| { type: 'selectedRuleIds'; ids: string[] }
| { type: 'exportRuleIds'; ids: string[] }
| { type: 'setLastRefreshDate' }
| { type: 'setAutoRefreshOn'; on: boolean }
| { type: 'setShowIdleModal'; show: boolean }
| { type: 'failure' };

export const createRulesTableReducer = (
tableRef: React.MutableRefObject<EuiBasicTable<unknown> | undefined>
) => {
const rulesTableReducer = (state: RulesTableState, action: RulesTableAction): RulesTableState => {
switch (action.type) {
case 'setRules': {
if (
tableRef != null &&
tableRef.current != null &&
tableRef.current.changeSelection != null
) {
// for future devs: eui basic table is not giving us a prop to set the value, so
// we are using the ref in setTimeout to reset on the next loop so that we
// do not get a warning telling us we are trying to update during a render
window.setTimeout(() => tableRef?.current?.changeSelection([]), 0);
}

return {
...state,
rules: action.rules,
selectedRuleIds: [],
loadingRuleIds: [],
loadingRulesAction: null,
pagination: {
...state.pagination,
...action.pagination,
},
};
}
case 'updateRules': {
const ruleIds = state.rules.map((r) => r.id);
const updatedRules = action.rules.reduce((rules, updatedRule) => {
let newRules = rules;
if (ruleIds.includes(updatedRule.id)) {
newRules = newRules.map((r) => (updatedRule.id === r.id ? updatedRule : r));
} else {
newRules = [...newRules, updatedRule];
}
return newRules;
}, state.rules);
const updatedRuleIds = action.rules.map((r) => r.id);
const newLoadingRuleIds = state.loadingRuleIds.filter((id) => !updatedRuleIds.includes(id));
return {
...state,
rules: updatedRules,
loadingRuleIds: newLoadingRuleIds,
loadingRulesAction: newLoadingRuleIds.length === 0 ? null : state.loadingRulesAction,
};
}
case 'updateFilterOptions': {
return {
...state,
filterOptions: {
...state.filterOptions,
...action.filterOptions,
},
pagination: {
...state.pagination,
...action.pagination,
},
};
}
case 'loadingRuleIds': {
return {
...state,
loadingRuleIds: action.actionType == null ? [] : [...state.loadingRuleIds, ...action.ids],
loadingRulesAction: action.actionType,
};
}
case 'selectedRuleIds': {
return {
...state,
selectedRuleIds: action.ids,
};
}
case 'exportRuleIds': {
return {
...state,
loadingRuleIds: action.ids,
loadingRulesAction: 'export',
exportRuleIds: action.ids,
};
}
case 'setLastRefreshDate': {
return {
...state,
lastUpdated: Date.now(),
};
}
case 'setAutoRefreshOn': {
return {
...state,
isRefreshOn: action.on,
};
}
case 'setShowIdleModal': {
return {
...state,
showIdleModal: action.show,
isRefreshOn: !action.show,
};
}
case 'failure': {
return {
...state,
rules: [],
};
}
default: {
return state;
}
}
};

return rulesTableReducer;
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@

import { renderHook, act } from '@testing-library/react-hooks';
import { useRules, UseRules, ReturnRules } from './use_rules';
import * as api from './api';
import * as api from '../api';

jest.mock('./api');
jest.mock('../api');

describe('useRules', () => {
beforeEach(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@

import { useEffect, useState, useRef } from 'react';

import { FetchRulesResponse, FilterOptions, PaginationOptions, Rule } from './types';
import { errorToToaster, useStateToaster } from '../../../../common/components/toasters';
import { fetchRules } from './api';
import * as i18n from './translations';
import { FetchRulesResponse, FilterOptions, PaginationOptions, Rule } from '../types';
import { errorToToaster, useStateToaster } from '../../../../../common/components/toasters';
import { fetchRules } from '../api';
import * as i18n from '../translations';

export type ReturnRules = [boolean, FetchRulesResponse | null, () => Promise<void>];

Expand Down
Loading

0 comments on commit 4a79869

Please sign in to comment.