From af817b4ba5e8d8b30d37bc4b1b22c79727d97c2f Mon Sep 17 00:00:00 2001 From: Yara Tercero Date: Mon, 2 Mar 2020 14:35:44 -0500 Subject: [PATCH 1/4] Fixed minor UI bug on all rules table pagination --- .../rules/all/helpers.test.tsx | 100 +++++++++++++++++- .../detection_engine/rules/all/helpers.ts | 16 +++ .../detection_engine/rules/all/index.tsx | 8 +- 3 files changed, 121 insertions(+), 3 deletions(-) diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx index c60933733587d..5bc2997d572db 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { bucketRulesResponse } from './helpers'; +import { bucketRulesResponse, getRulesTotal, showRulesTable } from './helpers'; import { mockRule, mockRuleError } from './__mocks__/mock'; import uuid from 'uuid'; import { Rule, RuleError } from '../../../../containers/detection_engine/rules'; @@ -44,4 +44,102 @@ describe('AllRulesTable Helpers', () => { }); }); }); + + describe('showRulesTable', () => { + test('returns false when rulesCustomInstalled and rulesInstalled are null', () => { + const result = showRulesTable({ + rulesCustomInstalled: null, + rulesInstalled: null, + }); + expect(result).toBeFalsy(); + }); + + test('returns false when rulesCustomInstalled and rulesInstalled are 0', () => { + const result = showRulesTable({ + rulesCustomInstalled: 0, + rulesInstalled: 0, + }); + expect(result).toBeFalsy(); + }); + + test('returns false when both rulesCustomInstalled and rulesInstalled checks return false', () => { + const result = showRulesTable({ + rulesCustomInstalled: 0, + rulesInstalled: null, + }); + expect(result).toBeFalsy(); + }); + + test('returns true if rulesCustomInstalled is not null or 0', () => { + const result = showRulesTable({ + rulesCustomInstalled: 5, + rulesInstalled: null, + }); + expect(result).toBeTruthy(); + }); + + test('returns true if rulesInstalled is not null or 0', () => { + const result = showRulesTable({ + rulesCustomInstalled: null, + rulesInstalled: 5, + }); + expect(result).toBeTruthy(); + }); + }); + + describe('getRulesTotal', () => { + test('returns 0 when rulesCustomInstalled and rulesInstalled are null', () => { + const result = getRulesTotal({ + rulesCustomInstalled: null, + rulesInstalled: null, + filterOptions: { showCustomRules: false, showElasticRules: false }, + }); + expect(result).toEqual(0); + }); + + test('returns rulesCustomInstalled when rulesInstalled is null', () => { + const result = getRulesTotal({ + rulesCustomInstalled: 5, + rulesInstalled: null, + filterOptions: { showCustomRules: false, showElasticRules: false }, + }); + expect(result).toEqual(5); + }); + + test('returns rulesInstalled when rulesCustomInstalled is null', () => { + const result = getRulesTotal({ + rulesCustomInstalled: null, + rulesInstalled: 5, + filterOptions: { showCustomRules: false, showElasticRules: false }, + }); + expect(result).toEqual(5); + }); + + test('returns sum of rulesInstalled and rulesCustomInstalled when no filter', () => { + const result = getRulesTotal({ + rulesCustomInstalled: 6, + rulesInstalled: 5, + filterOptions: { showCustomRules: false, showElasticRules: false }, + }); + expect(result).toEqual(11); + }); + + test('returns rulesCustomInstalled when showCustomRules is true', () => { + const result = getRulesTotal({ + rulesCustomInstalled: 6, + rulesInstalled: 5, + filterOptions: { showCustomRules: true, showElasticRules: false }, + }); + expect(result).toEqual(6); + }); + + test('returns rulesInstalled when showElasticRules is true', () => { + const result = getRulesTotal({ + rulesCustomInstalled: 6, + rulesInstalled: 5, + filterOptions: { showCustomRules: false, showElasticRules: true }, + }); + expect(result).toEqual(5); + }); + }); }); diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts index 5ce26144a4d9c..5db9067b9cb1b 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts @@ -8,6 +8,7 @@ import { Rule, RuleError, RuleResponseBuckets, + FilterOptions, } from '../../../../containers/detection_engine/rules'; /** @@ -34,3 +35,18 @@ export const showRulesTable = ({ }) => (rulesCustomInstalled != null && rulesCustomInstalled > 0) || (rulesInstalled != null && rulesInstalled > 0); + +export const getRulesTotal = ({ + rulesCustomInstalled, + rulesInstalled, + filterOptions, +}: { + rulesCustomInstalled: number | null; + rulesInstalled: number | null; + filterOptions: Partial; +}): number => { + const showAllRules = !filterOptions.showCustomRules && !filterOptions.showElasticRules; + const customRules = filterOptions.showCustomRules || showAllRules ? rulesCustomInstalled ?? 0 : 0; + const installedRules = filterOptions.showElasticRules || showAllRules ? rulesInstalled ?? 0 : 0; + return customRules + installedRules; +}; diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx index 79fec526faf48..725fd9b5d5f3a 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx @@ -40,7 +40,7 @@ import * as i18n from '../translations'; import { EuiBasicTableOnChange } from '../types'; import { getBatchItems } from './batch_actions'; import { getColumns } from './columns'; -import { showRulesTable } from './helpers'; +import { showRulesTable, getRulesTotal } from './helpers'; import { allRulesReducer, State } from './reducer'; import { RulesTableFilters } from './rules_table_filters/rules_table_filters'; @@ -324,7 +324,11 @@ export const AllRules = React.memo( pagination={{ pageIndex: pagination.page - 1, pageSize: pagination.perPage, - totalItemCount: pagination.total, + totalItemCount: getRulesTotal({ + rulesCustomInstalled, + rulesInstalled, + filterOptions, + }), pageSizeOptions: [5, 10, 20, 50, 100, 200, 300], }} ref={tableRef} From 537b9cdb21a6734778557a6358986fd03a77b062 Mon Sep 17 00:00:00 2001 From: Yara Tercero Date: Mon, 2 Mar 2020 16:43:17 -0500 Subject: [PATCH 2/4] Updated code after finding bug with previous solution, needed to update at reducer level --- .../detection_engine/rules/use_rules.tsx | 4 +- .../rules/all/helpers.test.tsx | 58 +------------------ .../detection_engine/rules/all/helpers.ts | 16 ----- .../detection_engine/rules/all/index.tsx | 11 ++-- .../detection_engine/rules/all/reducer.ts | 6 +- 5 files changed, 12 insertions(+), 83 deletions(-) diff --git a/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx b/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx index d05d59d15802d..4c0bf86e0c227 100644 --- a/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx +++ b/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx @@ -23,7 +23,7 @@ export interface UseRules { pagination: PaginationOptions; filterOptions: FilterOptions; refetchPrePackagedRulesStatus?: () => void; - dispatchRulesInReducer?: (rules: Rule[]) => void; + dispatchRulesInReducer?: (rules: Rule[], rulesTotal: number) => void; } /** @@ -59,7 +59,7 @@ export const useRules = ({ if (isSubscribed) { setRules(fetchRulesResult); if (dispatchRulesInReducer != null) { - dispatchRulesInReducer(fetchRulesResult.data); + dispatchRulesInReducer(fetchRulesResult.data, fetchRulesResult.total); } } } catch (error) { diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx index 5bc2997d572db..062d7967bf301 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.test.tsx @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { bucketRulesResponse, getRulesTotal, showRulesTable } from './helpers'; +import { bucketRulesResponse, showRulesTable } from './helpers'; import { mockRule, mockRuleError } from './__mocks__/mock'; import uuid from 'uuid'; import { Rule, RuleError } from '../../../../containers/detection_engine/rules'; @@ -86,60 +86,4 @@ describe('AllRulesTable Helpers', () => { expect(result).toBeTruthy(); }); }); - - describe('getRulesTotal', () => { - test('returns 0 when rulesCustomInstalled and rulesInstalled are null', () => { - const result = getRulesTotal({ - rulesCustomInstalled: null, - rulesInstalled: null, - filterOptions: { showCustomRules: false, showElasticRules: false }, - }); - expect(result).toEqual(0); - }); - - test('returns rulesCustomInstalled when rulesInstalled is null', () => { - const result = getRulesTotal({ - rulesCustomInstalled: 5, - rulesInstalled: null, - filterOptions: { showCustomRules: false, showElasticRules: false }, - }); - expect(result).toEqual(5); - }); - - test('returns rulesInstalled when rulesCustomInstalled is null', () => { - const result = getRulesTotal({ - rulesCustomInstalled: null, - rulesInstalled: 5, - filterOptions: { showCustomRules: false, showElasticRules: false }, - }); - expect(result).toEqual(5); - }); - - test('returns sum of rulesInstalled and rulesCustomInstalled when no filter', () => { - const result = getRulesTotal({ - rulesCustomInstalled: 6, - rulesInstalled: 5, - filterOptions: { showCustomRules: false, showElasticRules: false }, - }); - expect(result).toEqual(11); - }); - - test('returns rulesCustomInstalled when showCustomRules is true', () => { - const result = getRulesTotal({ - rulesCustomInstalled: 6, - rulesInstalled: 5, - filterOptions: { showCustomRules: true, showElasticRules: false }, - }); - expect(result).toEqual(6); - }); - - test('returns rulesInstalled when showElasticRules is true', () => { - const result = getRulesTotal({ - rulesCustomInstalled: 6, - rulesInstalled: 5, - filterOptions: { showCustomRules: false, showElasticRules: true }, - }); - expect(result).toEqual(5); - }); - }); }); diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts index 5db9067b9cb1b..5ce26144a4d9c 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/helpers.ts @@ -8,7 +8,6 @@ import { Rule, RuleError, RuleResponseBuckets, - FilterOptions, } from '../../../../containers/detection_engine/rules'; /** @@ -35,18 +34,3 @@ export const showRulesTable = ({ }) => (rulesCustomInstalled != null && rulesCustomInstalled > 0) || (rulesInstalled != null && rulesInstalled > 0); - -export const getRulesTotal = ({ - rulesCustomInstalled, - rulesInstalled, - filterOptions, -}: { - rulesCustomInstalled: number | null; - rulesInstalled: number | null; - filterOptions: Partial; -}): number => { - const showAllRules = !filterOptions.showCustomRules && !filterOptions.showElasticRules; - const customRules = filterOptions.showCustomRules || showAllRules ? rulesCustomInstalled ?? 0 : 0; - const installedRules = filterOptions.showElasticRules || showAllRules ? rulesInstalled ?? 0 : 0; - return customRules + installedRules; -}; diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx index 725fd9b5d5f3a..2982a034aa401 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx @@ -40,7 +40,7 @@ import * as i18n from '../translations'; import { EuiBasicTableOnChange } from '../types'; import { getBatchItems } from './batch_actions'; import { getColumns } from './columns'; -import { showRulesTable, getRulesTotal } from './helpers'; +import { showRulesTable } from './helpers'; import { allRulesReducer, State } from './reducer'; import { RulesTableFilters } from './rules_table_filters/rules_table_filters'; @@ -118,10 +118,11 @@ export const AllRules = React.memo( const history = useHistory(); const [, dispatchToaster] = useStateToaster(); - const setRules = useCallback((newRules: Rule[]) => { + const setRules = useCallback((newRules: Rule[], rulesTotal: number) => { dispatch({ type: 'setRules', rules: newRules, + total: rulesTotal, }); }, []); @@ -324,11 +325,7 @@ export const AllRules = React.memo( pagination={{ pageIndex: pagination.page - 1, pageSize: pagination.perPage, - totalItemCount: getRulesTotal({ - rulesCustomInstalled, - rulesInstalled, - filterOptions, - }), + totalItemCount: pagination.total, pageSizeOptions: [5, 10, 20, 50, 100, 200, 300], }} ref={tableRef} diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts index 54da43efd66d9..99694623da2ea 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts @@ -26,7 +26,7 @@ export type Action = | { type: 'exportRuleIds'; ids: string[] } | { type: 'loadingRuleIds'; ids: string[]; actionType: LoadingRuleAction } | { type: 'selectedRuleIds'; ids: string[] } - | { type: 'setRules'; rules: Rule[] } + | { type: 'setRules'; rules: Rule[]; total: number } | { type: 'updateRules'; rules: Rule[] } | { type: 'updatePagination'; pagination: Partial } | { @@ -76,6 +76,10 @@ export const allRulesReducer = ( selectedRuleIds: [], loadingRuleIds: [], loadingRulesAction: null, + pagination: { + ...state.pagination, + total: action.total, + }, }; } case 'updateRules': { From 34ddd408536c26ae67d92995404a12125ea286c9 Mon Sep 17 00:00:00 2001 From: Yara Tercero Date: Mon, 2 Mar 2020 17:05:18 -0500 Subject: [PATCH 3/4] re-re-did solution. noticed that update-pagination action was never being called, moved pagination update to that action --- .../containers/detection_engine/rules/use_rules.tsx | 10 +++++++--- .../public/pages/detection_engine/rules/all/index.tsx | 8 ++++++-- .../public/pages/detection_engine/rules/all/reducer.ts | 6 +----- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx b/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx index 4c0bf86e0c227..81b8b04ed6648 100644 --- a/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx +++ b/x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/use_rules.tsx @@ -23,7 +23,7 @@ export interface UseRules { pagination: PaginationOptions; filterOptions: FilterOptions; refetchPrePackagedRulesStatus?: () => void; - dispatchRulesInReducer?: (rules: Rule[], rulesTotal: number) => void; + dispatchRulesInReducer?: (rules: Rule[], pagination: Partial) => void; } /** @@ -59,14 +59,18 @@ export const useRules = ({ if (isSubscribed) { setRules(fetchRulesResult); if (dispatchRulesInReducer != null) { - dispatchRulesInReducer(fetchRulesResult.data, fetchRulesResult.total); + dispatchRulesInReducer(fetchRulesResult.data, { + page: fetchRulesResult.page, + perPage: fetchRulesResult.perPage, + total: fetchRulesResult.total, + }); } } } catch (error) { if (isSubscribed) { errorToToaster({ title: i18n.RULE_FETCH_FAILURE, error, dispatchToaster }); if (dispatchRulesInReducer != null) { - dispatchRulesInReducer([]); + dispatchRulesInReducer([], {}); } } } diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx index 2982a034aa401..8418641b9260e 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx @@ -21,6 +21,7 @@ import { CreatePreBuiltRules, FilterOptions, Rule, + PaginationOptions, } from '../../../../containers/detection_engine/rules'; import { HeaderSection } from '../../../../components/header_section'; import { @@ -118,11 +119,14 @@ export const AllRules = React.memo( const history = useHistory(); const [, dispatchToaster] = useStateToaster(); - const setRules = useCallback((newRules: Rule[], rulesTotal: number) => { + const setRules = useCallback((newRules: Rule[], newPagination: Partial) => { dispatch({ type: 'setRules', rules: newRules, - total: rulesTotal, + }); + dispatch({ + type: 'updatePagination', + pagination: newPagination, }); }, []); diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts index 99694623da2ea..54da43efd66d9 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts @@ -26,7 +26,7 @@ export type Action = | { type: 'exportRuleIds'; ids: string[] } | { type: 'loadingRuleIds'; ids: string[]; actionType: LoadingRuleAction } | { type: 'selectedRuleIds'; ids: string[] } - | { type: 'setRules'; rules: Rule[]; total: number } + | { type: 'setRules'; rules: Rule[] } | { type: 'updateRules'; rules: Rule[] } | { type: 'updatePagination'; pagination: Partial } | { @@ -76,10 +76,6 @@ export const allRulesReducer = ( selectedRuleIds: [], loadingRuleIds: [], loadingRulesAction: null, - pagination: { - ...state.pagination, - total: action.total, - }, }; } case 'updateRules': { From c60c8638295acb80b000b0be1ccceaedb85687b3 Mon Sep 17 00:00:00 2001 From: Yara Tercero Date: Tue, 3 Mar 2020 12:06:21 -0500 Subject: [PATCH 4/4] refactored to update pagination at the same time as rules, avoiding unnecessary re-renders --- .../pages/detection_engine/rules/all/index.tsx | 3 --- .../pages/detection_engine/rules/all/reducer.ts | 16 +++++----------- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx index 8418641b9260e..9676b83a26f55 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/index.tsx @@ -123,9 +123,6 @@ export const AllRules = React.memo( dispatch({ type: 'setRules', rules: newRules, - }); - dispatch({ - type: 'updatePagination', pagination: newPagination, }); }, []); diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts index 54da43efd66d9..0a4d169d13154 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/reducer.ts @@ -26,9 +26,8 @@ export type Action = | { type: 'exportRuleIds'; ids: string[] } | { type: 'loadingRuleIds'; ids: string[]; actionType: LoadingRuleAction } | { type: 'selectedRuleIds'; ids: string[] } - | { type: 'setRules'; rules: Rule[] } + | { type: 'setRules'; rules: Rule[]; pagination: Partial } | { type: 'updateRules'; rules: Rule[] } - | { type: 'updatePagination'; pagination: Partial } | { type: 'updateFilterOptions'; filterOptions: Partial; @@ -76,6 +75,10 @@ export const allRulesReducer = ( selectedRuleIds: [], loadingRuleIds: [], loadingRulesAction: null, + pagination: { + ...state.pagination, + ...action.pagination, + }, }; } case 'updateRules': { @@ -101,15 +104,6 @@ export const allRulesReducer = ( } return state; } - case 'updatePagination': { - return { - ...state, - pagination: { - ...state.pagination, - ...action.pagination, - }, - }; - } case 'updateFilterOptions': { return { ...state,