From 69d050a70ffe001612f0bb9b9ae804423566af26 Mon Sep 17 00:00:00 2001 From: andreas-unleash Date: Mon, 23 Oct 2023 15:12:15 +0300 Subject: [PATCH 1/6] fix: grey out text and icons for disabled strategies in playground (#5113) What it says on the tin Closes # [1-1512](https://linear.app/unleash/issue/1-1512/grey-out-everything-icons-labels-etc-when-strategy-is-disabled) Screenshot 2023-10-20 at 12 25 51 Screenshot 2023-10-20 at 14 52 30 --------- Signed-off-by: andreas-unleash --- .../ConstraintAccordionView.tsx | 3 ++ .../ConstraintAccordionViewHeader.tsx | 5 +- .../ConstraintAccordionViewHeaderInfo.tsx | 17 +++++- ...raintAccordionViewHeaderMultipleValues.tsx | 12 ++++- ...nstraintAccordionViewHeaderSingleValue.tsx | 5 ++ .../ConstraintViewHeaderOperator.tsx | 3 ++ .../ConstraintAccordion/ConstraintIcon.tsx | 14 +++-- .../ConstraintOperator/ConstraintOperator.tsx | 17 ++++-- .../DisabledPercentageCircle.tsx | 46 ++++++++++++++++ .../common/SegmentItem/SegmentItem.tsx | 18 ++++++- .../ConstraintExecutionWithoutResults.tsx | 8 +-- .../DisabledStrategyExecution.tsx | 19 +++++-- .../PlaygroundParameterItem.tsx | 22 ++++++-- .../SegmentExecutionWithoutResult.tsx | 1 + .../StrategyExecution/StrategyExecution.tsx | 7 ++- .../StrategyExecutionParameters.tsx | 52 ++++++++++++++++--- 16 files changed, 213 insertions(+), 36 deletions(-) create mode 100644 frontend/src/component/common/PercentageCircle/DisabledPercentageCircle.tsx diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionView.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionView.tsx index 9d209f2b4a..d66cfd2eb3 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionView.tsx +++ b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionView.tsx @@ -23,6 +23,7 @@ interface IConstraintAccordionViewProps { onEdit?: () => void; sx?: SxProps; compact?: boolean; + disabled?: boolean; renderAfter?: JSX.Element; } @@ -68,6 +69,7 @@ export const ConstraintAccordionView = ({ onDelete, sx = undefined, compact = false, + disabled = false, renderAfter, }: IConstraintAccordionViewProps) => { const [expandable, setExpandable] = useState(true); @@ -102,6 +104,7 @@ export const ConstraintAccordionView = ({ onDelete={onDelete} singleValue={singleValue} allowExpand={setExpandable} + disabled={disabled} expanded={expanded} compact={compact} /> diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeader.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeader.tsx index 96576d8052..65145ce7c0 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeader.tsx +++ b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeader.tsx @@ -13,6 +13,7 @@ interface IConstraintAccordionViewHeaderProps { expanded: boolean; allowExpand: (shouldExpand: boolean) => void; compact?: boolean; + disabled?: boolean; } const StyledContainer = styled('div')(({ theme }) => ({ @@ -34,6 +35,7 @@ export const ConstraintAccordionViewHeader = ({ allowExpand, expanded, compact, + disabled, }: IConstraintAccordionViewHeaderProps) => { const { context } = useUnleashContext(); const { contextName } = constraint; @@ -44,12 +46,13 @@ export const ConstraintAccordionViewHeader = ({ return ( - + void; + disabled?: boolean; maxLength?: number; } @@ -58,23 +59,34 @@ export const ConstraintAccordionViewHeaderInfo = ({ singleValue, allowExpand, expanded, + disabled = false, maxLength = 112, //The max number of characters in the values text for NOT allowing expansion }: ConstraintAccordionViewHeaderMetaInfoProps) => { return ( - + ({ + color: disabled + ? theme.palette.text.secondary + : 'inherit', + })} + > {constraint.contextName} - + } elseShow={ @@ -83,6 +95,7 @@ export const ConstraintAccordionViewHeaderInfo = ({ expanded={expanded} allowExpand={allowExpand} maxLength={maxLength} + disabled={disabled} /> } /> diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeaderMultipleValues.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeaderMultipleValues.tsx index 6f70f0dfa5..f819e8052c 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeaderMultipleValues.tsx +++ b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintAccordionViewHeaderMultipleValues.tsx @@ -22,6 +22,7 @@ interface ConstraintSingleValueProps { expanded: boolean; maxLength: number; allowExpand: (shouldExpand: boolean) => void; + disabled?: boolean; } const StyledHeaderValuesContainerWrapper = styled('div')(({ theme }) => ({ @@ -55,6 +56,7 @@ export const ConstraintAccordionViewHeaderMultipleValues = ({ expanded, allowExpand, maxLength, + disabled = false, }: ConstraintSingleValueProps) => { const [expandable, setExpandable] = useState(false); @@ -72,7 +74,15 @@ export const ConstraintAccordionViewHeaderMultipleValues = ({ return ( - {text} + ({ + color: disabled + ? theme.palette.text.secondary + : 'inherit', + })} + > + {text} + ({ interface ConstraintSingleValueProps { constraint: IConstraint; allowExpand: (shouldExpand: boolean) => void; + disabled?: boolean; } const StyledHeaderValuesContainerWrapper = styled('div')(({ theme }) => ({ @@ -26,6 +27,7 @@ const StyledHeaderValuesContainerWrapper = styled('div')(({ theme }) => ({ export const ConstraintAccordionViewHeaderSingleValue = ({ constraint, allowExpand, + disabled = false, }: ConstraintSingleValueProps) => { const { locationSettings } = useLocationSettings(); @@ -36,6 +38,9 @@ export const ConstraintAccordionViewHeaderSingleValue = ({ return ( ({ + color: disabled ? theme.palette.text.secondary : 'inherit', + })} label={formatConstraintValue(constraint, locationSettings)} /> diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintViewHeaderOperator.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintViewHeaderOperator.tsx index 2a5d3e6c9d..5ba0ab66d7 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintViewHeaderOperator.tsx +++ b/frontend/src/component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionViewHeader/ConstraintViewHeaderOperator.tsx @@ -10,6 +10,7 @@ import { oneOf } from 'utils/oneOf'; interface ConstraintViewHeaderOperatorProps { constraint: IConstraint; + disabled?: boolean; } const StyledHeaderValuesContainerWrapper = styled('div')(({ theme }) => ({ @@ -28,6 +29,7 @@ const StyledHeaderConstraintContainer = styled('div')(({ theme }) => ({ export const ConstraintViewHeaderOperator = ({ constraint, + disabled = false, }: ConstraintViewHeaderOperatorProps) => { return ( @@ -47,6 +49,7 @@ export const ConstraintViewHeaderOperator = ({ = ({ compact }) => ( +export const ConstraintIcon: VFC = ({ + compact, + disabled, +}) => ( ({ + backgroundColor: disabled + ? theme.palette.neutral.border + : 'primary.light', p: compact ? '1px' : '2px', borderRadius: '50%', width: compact ? '18px' : '24px', height: compact ? '18px' : '24px', marginRight: '13px', - }} + })} > ({ diff --git a/frontend/src/component/common/ConstraintAccordion/ConstraintOperator/ConstraintOperator.tsx b/frontend/src/component/common/ConstraintAccordion/ConstraintOperator/ConstraintOperator.tsx index b7bd3fff05..0e05726d33 100644 --- a/frontend/src/component/common/ConstraintAccordion/ConstraintOperator/ConstraintOperator.tsx +++ b/frontend/src/component/common/ConstraintAccordion/ConstraintOperator/ConstraintOperator.tsx @@ -6,6 +6,7 @@ import { styled } from '@mui/material'; interface IConstraintOperatorProps { constraint: IConstraint; hasPrefix?: boolean; + disabled?: boolean; } const StyledContainer = styled('div')(({ theme }) => ({ @@ -15,19 +16,25 @@ const StyledContainer = styled('div')(({ theme }) => ({ lineHeight: 1.25, })); -const StyledName = styled('div')(({ theme }) => ({ +const StyledName = styled('div', { + shouldForwardProp: (prop) => prop !== 'disabled', +})<{ disabled: boolean }>(({ theme, disabled }) => ({ fontSize: theme.fontSizes.smallBody, lineHeight: 17 / 14, + color: disabled ? theme.palette.text.secondary : theme.palette.text.primary, })); -const StyledText = styled('div')(({ theme }) => ({ +const StyledText = styled('div', { + shouldForwardProp: (prop) => prop !== 'disabled', +})<{ disabled: boolean }>(({ theme, disabled }) => ({ fontSize: theme.fontSizes.smallerBody, - color: theme.palette.neutral.main, + color: disabled ? theme.palette.text.secondary : theme.palette.neutral.main, })); export const ConstraintOperator = ({ constraint, hasPrefix, + disabled = false, }: IConstraintOperatorProps) => { const operatorName = constraint.operator; const operatorText = formatOperatorDescription(constraint.operator); @@ -40,8 +47,8 @@ export const ConstraintOperator = ({ paddingLeft: hasPrefix ? 0 : undefined, }} > - {operatorName} - {operatorText} + {operatorName} + {operatorText} ); }; diff --git a/frontend/src/component/common/PercentageCircle/DisabledPercentageCircle.tsx b/frontend/src/component/common/PercentageCircle/DisabledPercentageCircle.tsx new file mode 100644 index 0000000000..9f8105ac15 --- /dev/null +++ b/frontend/src/component/common/PercentageCircle/DisabledPercentageCircle.tsx @@ -0,0 +1,46 @@ +import { useTheme } from '@mui/material'; +import { CSSProperties } from 'react'; + +interface IPercentageCircleProps { + percentage: number; + size?: `${number}rem`; +} + +const PercentageCircle = ({ + percentage, + size = '4rem', +}: IPercentageCircleProps) => { + const theme = useTheme(); + + const style: CSSProperties = { + display: 'block', + borderRadius: '100%', + transform: 'rotate(-90deg)', + height: size, + width: size, + background: theme.palette.background.elevation2, + }; + + // The percentage circle used to be drawn by CSS with a conic-gradient, + // but the result was either jagged or blurry. SVG seems to look better. + // See https://stackoverflow.com/a/70659532. + const radius = 100 / (2 * Math.PI); + const diameter = 2 * radius; + + return ( + + A circle progress bar with {percentage}% completion. + + + ); +}; + +export default PercentageCircle; diff --git a/frontend/src/component/common/SegmentItem/SegmentItem.tsx b/frontend/src/component/common/SegmentItem/SegmentItem.tsx index b085e80af6..061ee6dacf 100644 --- a/frontend/src/component/common/SegmentItem/SegmentItem.tsx +++ b/frontend/src/component/common/SegmentItem/SegmentItem.tsx @@ -16,6 +16,7 @@ import { ConditionallyRender } from 'component/common/ConditionallyRender/Condit interface ISegmentItemProps { segment: Partial; isExpanded?: boolean; + disabled?: boolean; constraintList?: JSX.Element; headerContent?: JSX.Element; } @@ -49,20 +50,33 @@ const StyledLink = styled(Link)(({ theme }) => ({ textDecoration: 'underline', }, })); +const StyledText = styled('span', { + shouldForwardProp: (prop) => prop !== 'disabled', +})<{ disabled: boolean }>(({ theme, disabled }) => ({ + color: disabled ? theme.palette.text.secondary : 'inherit', +})); export const SegmentItem: VFC = ({ segment, isExpanded, headerContent, constraintList, + disabled = false, }) => { const [isOpen, setIsOpen] = useState(isExpanded || false); return ( - - Segment: + ({ + mr: 1, + color: disabled + ? theme.palette.neutral.border + : theme.palette.secondary.main, + })} + /> + Segment: {segment.name} diff --git a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/ConstraintExecution/ConstraintExecutionWithoutResults.tsx b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/ConstraintExecution/ConstraintExecutionWithoutResults.tsx index bcfbc342c0..862270d1f0 100644 --- a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/ConstraintExecution/ConstraintExecutionWithoutResults.tsx +++ b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/ConstraintExecution/ConstraintExecutionWithoutResults.tsx @@ -9,8 +9,6 @@ import { ConditionallyRender } from 'component/common/ConditionallyRender/Condit import { StrategySeparator } from 'component/common/StrategySeparator/StrategySeparator'; import { styled } from '@mui/material'; import { ConstraintAccordionView } from 'component/common/ConstraintAccordion/ConstraintAccordionView/ConstraintAccordionView'; -import { ConstraintError } from './ConstraintError/ConstraintError'; -import { ConstraintOk } from './ConstraintOk/ConstraintOk'; interface IConstraintExecutionWithoutResultsProps { constraints?: PlaygroundConstraintSchema[]; @@ -35,7 +33,11 @@ export const ConstraintExecutionWithoutResults: VFC< condition={index > 0} show={} /> - + ))} diff --git a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/DisabledStrategyExecution.tsx b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/DisabledStrategyExecution.tsx index f195bd8f0f..391b964920 100644 --- a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/DisabledStrategyExecution.tsx +++ b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/DisabledStrategyExecution.tsx @@ -52,6 +52,7 @@ export const DisabledStrategyExecution: VFC = parameters={parameters} constraints={constraints} input={input} + disabled /> ), hasCustomStrategyParameters && ( @@ -61,9 +62,14 @@ export const DisabledStrategyExecution: VFC = /> ), name === 'default' && ( - - The standard strategy is ON{' '} - for all users. + ({ + width: '100%', + color: theme.palette.text.secondary, + })} + > + The standard strategy is{' '} + ON for all users. ), ].filter(Boolean); @@ -74,7 +80,12 @@ export const DisabledStrategyExecution: VFC = // biome-ignore lint/suspicious/noArrayIndexKey: 0} + condition={ + index > 0 && + (strategyResult.name === 'flexibleRollout' + ? index < items.length + : index < items.length - 1) + } show={} /> {item} diff --git a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/PlaygroundParameterItem/PlaygroundParameterItem.tsx b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/PlaygroundParameterItem/PlaygroundParameterItem.tsx index a722cef287..5c2bb84a94 100644 --- a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/PlaygroundParameterItem/PlaygroundParameterItem.tsx +++ b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/PlaygroundParameterItem/PlaygroundParameterItem.tsx @@ -8,6 +8,7 @@ interface IConstraintItemProps { text: string; input?: string | number | boolean | 'no value'; showReason?: boolean; + disabled?: boolean; } const StyledDivContainer = styled('div', { @@ -34,12 +35,15 @@ const StyledChip = styled(Chip)(({ theme }) => ({ margin: theme.spacing(0.5), })); -const StyledParagraph = styled('p')(({ theme }) => ({ +const StyledParagraph = styled('p', { + shouldForwardProp: (prop) => prop !== 'disabled', +})<{ disabled: boolean }>(({ theme, disabled }) => ({ display: 'inline', margin: theme.spacing(0.5, 0), maxWidth: '95%', textAlign: 'center', wordBreak: 'break-word', + color: disabled ? theme.palette.text.secondary : 'inherit', })); export const PlaygroundParameterItem = ({ @@ -47,10 +51,11 @@ export const PlaygroundParameterItem = ({ text, input, showReason = false, + disabled = false, }: IConstraintItemProps) => { const theme = useTheme(); - const color = input === 'no value' ? 'error' : 'neutral'; + const color = input === 'no value' && !disabled ? 'error' : 'neutral'; const reason = `value does not match any ${text}`; return ( @@ -64,7 +69,11 @@ export const PlaygroundParameterItem = ({ show={ {reason} @@ -75,7 +84,7 @@ export const PlaygroundParameterItem = ({ show={

No {text}s added yet.

} elseShow={
- + {value.length}{' '} {value.length > 1 ? `${text}s` : text} will get access. @@ -83,6 +92,7 @@ export const PlaygroundParameterItem = ({ {value.map((v: string | number) => ( } + show={ + + } elseShow={
} /> diff --git a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/SegmentExecution/SegmentExecutionWithoutResult.tsx b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/SegmentExecution/SegmentExecutionWithoutResult.tsx index 33e40ac84c..aadc0b12c6 100644 --- a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/SegmentExecution/SegmentExecutionWithoutResult.tsx +++ b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/SegmentExecution/SegmentExecutionWithoutResult.tsx @@ -29,6 +29,7 @@ export const SegmentExecutionWithoutResult: VFC< /> } isExpanded + disabled /> = ({ // biome-ignore lint/suspicious/noArrayIndexKey: 0} + condition={ + index > 0 && + (strategyResult.name === 'flexibleRollout' + ? index < items.length + : index < items.length - 1) + } show={} /> {item} diff --git a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/StrategyExecutionParameters/StrategyExecutionParameters.tsx b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/StrategyExecutionParameters/StrategyExecutionParameters.tsx index cc5606da08..d96c0a5895 100644 --- a/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/StrategyExecutionParameters/StrategyExecutionParameters.tsx +++ b/frontend/src/component/playground/Playground/PlaygroundResultsTable/FeatureResultInfoPopoverCell/FeatureStrategyList/StrategyList/StrategyItem/StrategyExecution/StrategyExecutionParameters/StrategyExecutionParameters.tsx @@ -2,24 +2,34 @@ import { parseParameterNumber, parseParameterStrings, } from 'utils/parseParameter'; -import { Box } from '@mui/material'; +import { Box, styled } from '@mui/material'; import PercentageCircle from 'component/common/PercentageCircle/PercentageCircle'; import { PlaygroundParameterItem } from '../PlaygroundParameterItem/PlaygroundParameterItem'; import { StyledBoxSummary } from '../StrategyExecution.styles'; import { PlaygroundConstraintSchema, PlaygroundRequestSchema } from 'openapi'; import { getMappedParam } from '../helpers'; import { Badge } from 'component/common/Badge/Badge'; +import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; +import DisabledPercentageCircle from 'component/common/PercentageCircle/DisabledPercentageCircle'; export interface PlaygroundResultStrategyExecutionParametersProps { parameters: { [key: string]: string }; constraints: PlaygroundConstraintSchema[]; input?: PlaygroundRequestSchema; + disabled?: boolean; } +const StyledText = styled('div', { + shouldForwardProp: (prop) => prop !== 'disabled', +})<{ disabled: boolean }>(({ theme, disabled }) => ({ + color: disabled ? theme.palette.text.secondary : theme.palette.neutral.main, +})); + export const PlaygroundResultStrategyExecutionParameters = ({ parameters, constraints, input, + disabled = false, }: PlaygroundResultStrategyExecutionParametersProps) => { return ( <> @@ -35,20 +45,44 @@ export const PlaygroundResultStrategyExecutionParameters = ({ key={key} sx={{ display: 'flex', alignItems: 'center' }} > - - ({ + mr: '1rem', + color: disabled + ? theme.palette.neutral.border + : theme.palette.text.secondary, + })} + > + + } + elseShow={ + + } /> -
- {percentage}%{' '} + + + {percentage}% + {' '} of your base{' '} {constraints.length > 0 ? 'who match constraints' : ''}{' '} is included. -
+ ); } @@ -87,6 +121,7 @@ export const PlaygroundResultStrategyExecutionParameters = ({ text={'host'} input={'no value'} showReason={undefined} + disabled={disabled} /> ); } @@ -97,6 +132,7 @@ export const PlaygroundResultStrategyExecutionParameters = ({ key={key} value={IPs} text={'IP'} + disabled={disabled} input={ input?.context?.[getMappedParam(key)] ? input?.context?.[getMappedParam(key)] From 314a08b4e69fc4e25a19b03b374839e11618772b Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Mon, 23 Oct 2023 14:32:15 +0200 Subject: [PATCH 2/6] feat: Make multiple roles per group/user GA by removing the flag (#5109) To prepare for 5.6 GA, I've done a find through both Frontend and Backend here to remove the usages of the flag. Seems like the flag was only in use in the frontend. @nunogois can you confirm? --- .../ProjectAccessAssign.tsx | 27 ++++--------------- .../__snapshots__/create-config.test.ts.snap | 2 -- src/lib/types/experimental.ts | 5 ---- 3 files changed, 5 insertions(+), 29 deletions(-) diff --git a/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx b/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx index 02ed00a5de..d1c3eeb0c4 100644 --- a/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx +++ b/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx @@ -441,28 +441,11 @@ export const ProjectAccessAssign = ({ Select the role to assign for this project - ( - - )} - elseShow={() => ( - - setRoles(role ? [role] : []) - } - /> - )} +
diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 487a3d6ec1..180600cfed 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -106,7 +106,6 @@ exports[`should create default config 1`] = ` }, }, "migrationLock": true, - "multipleRoles": false, "personalAccessTokensKillSwitch": false, "playgroundImprovements": false, "privateProjects": false, @@ -151,7 +150,6 @@ exports[`should create default config 1`] = ` }, }, "migrationLock": true, - "multipleRoles": false, "personalAccessTokensKillSwitch": false, "playgroundImprovements": false, "privateProjects": false, diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 5cd800932f..41da53b747 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -23,7 +23,6 @@ export type IFlagKey = | 'filterInvalidClientMetrics' | 'lastSeenByEnvironment' | 'customRootRolesKillSwitch' - | 'multipleRoles' | 'featureNamingPattern' | 'doraMetrics' | 'variantTypeNumber' @@ -117,10 +116,6 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_CUSTOM_ROOT_ROLES_KILL_SWITCH, false, ), - multipleRoles: parseEnvVarBoolean( - process.env.UNLEASH_EXPERIMENTAL_MULTIPLE_ROLES, - false, - ), featureNamingPattern: parseEnvVarBoolean( process.env.UNLEASH_EXPERIMENTAL_FEATURE_NAMING_PATTERN, false, From 1d1aa27ca34927b00791b413f60c6d1b293b30b1 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 23 Oct 2023 15:11:38 +0200 Subject: [PATCH 3/6] refactor: proxy service scheduler (#5125) --- .../middleware/cors-origin-middleware.test.ts | 35 ++++++------------- src/lib/server-impl.ts | 1 - src/lib/services/index.ts | 7 ++++ src/lib/services/proxy-service.ts | 16 +-------- 4 files changed, 19 insertions(+), 40 deletions(-) diff --git a/src/lib/middleware/cors-origin-middleware.test.ts b/src/lib/middleware/cors-origin-middleware.test.ts index bc760eeabf..2e81ebdbda 100644 --- a/src/lib/middleware/cors-origin-middleware.test.ts +++ b/src/lib/middleware/cors-origin-middleware.test.ts @@ -4,7 +4,12 @@ import { createTestConfig } from '../../test/config/test-config'; import FakeEventStore from '../../test/fixtures/fake-event-store'; import { randomId } from '../util/random-id'; import FakeProjectStore from '../../test/fixtures/fake-project-store'; -import { EventService, ProxyService, SettingService } from '../../lib/services'; +import { + EventService, + ProxyService, + SchedulerService, + SettingService, +} from '../../lib/services'; import { ISettingStore } from '../../lib/types'; import { frontendSettingsKey } from '../../lib/types/settings/frontend-settings'; import { minutesToMilliseconds } from 'date-fns'; @@ -55,7 +60,6 @@ test('corsOriginMiddleware origin validation', async () => { userName, ), ).rejects.toThrow('Invalid origin: a'); - proxyService.destroy(); }); test('corsOriginMiddleware without config', async () => { @@ -82,7 +86,6 @@ test('corsOriginMiddleware without config', async () => { expect(await proxyService.getFrontendSettings(false)).toEqual({ frontendApiOrigins: [], }); - proxyService.destroy(); }); test('corsOriginMiddleware with config', async () => { @@ -109,12 +112,9 @@ test('corsOriginMiddleware with config', async () => { expect(await proxyService.getFrontendSettings(false)).toEqual({ frontendApiOrigins: ['*'], }); - proxyService.destroy(); }); test('corsOriginMiddleware with caching enabled', async () => { - jest.useFakeTimers(); - const { proxyService } = createSettingService([]); const userName = randomId(); @@ -133,24 +133,11 @@ test('corsOriginMiddleware with caching enabled', async () => { frontendApiOrigins: [], }); - jest.advanceTimersByTime(minutesToMilliseconds(2)); + await proxyService.fetchFrontendSettings(); // called by the scheduler service - jest.useRealTimers(); + const settings = await proxyService.getFrontendSettings(); - /* - This is needed because it is not enough to fake time to test the - updated cache, we also need to make sure that all promises are - executed and completed, in the right order. - */ - await new Promise((resolve) => - process.nextTick(async () => { - const settings = await proxyService.getFrontendSettings(); - - expect(settings).toEqual({ - frontendApiOrigins: ['*'], - }); - resolve(); - }), - ); - proxyService.destroy(); + expect(settings).toEqual({ + frontendApiOrigins: ['*'], + }); }); diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index b705c142c1..bc90403645 100644 --- a/src/lib/server-impl.ts +++ b/src/lib/server-impl.ts @@ -60,7 +60,6 @@ async function createApp( metricsMonitor.stopMonitoring(); stores.clientInstanceStore.destroy(); services.clientMetricsServiceV2.destroy(); - services.proxyService.destroy(); services.addonService.destroy(); await db.destroy(); }; diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index b508e67bed..213093f9aa 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -118,6 +118,7 @@ export const scheduleServices = async ( featureToggleService, versionService, lastSeenService, + proxyService, } = services; if (await maintenanceService.isMaintenanceMode()) { @@ -205,6 +206,12 @@ export const scheduleServices = async ( hoursToMilliseconds(48), 'checkLatestVersion', ); + + schedulerService.schedule( + proxyService.fetchFrontendSettings.bind(proxyService), + minutesToMilliseconds(2), + 'fetchFrontendSettings', + ); }; export const createServices = ( diff --git a/src/lib/services/proxy-service.ts b/src/lib/services/proxy-service.ts index 324b1de210..2e3bf55922 100644 --- a/src/lib/services/proxy-service.ts +++ b/src/lib/services/proxy-service.ts @@ -53,18 +53,11 @@ export class ProxyService { private cachedFrontendSettings?: FrontendSettings; - private timer: NodeJS.Timeout | null; - constructor(config: Config, stores: Stores, services: Services) { this.config = config; this.logger = config.getLogger('services/proxy-service.ts'); this.stores = stores; this.services = services; - - this.timer = setInterval( - () => this.fetchFrontendSettings(), - minutesToMilliseconds(2), - ).unref(); } async getProxyFeatures( @@ -181,7 +174,7 @@ export class ProxyService { ); } - private async fetchFrontendSettings(): Promise { + async fetchFrontendSettings(): Promise { try { this.cachedFrontendSettings = await this.services.settingService.get(frontendSettingsKey, { @@ -201,11 +194,4 @@ export class ProxyService { } return this.fetchFrontendSettings(); } - - destroy(): void { - if (this.timer) { - clearInterval(this.timer); - this.timer = null; - } - } } From 8d8a975c6c63b445933c6f94d209290b5c20a375 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 23 Oct 2023 15:22:30 +0200 Subject: [PATCH 4/6] Client instance service (#5126) --- .../tests/client-feature-toggle.e2e.test.ts | 9 --- src/lib/routes/admin-api/config.test.ts | 9 --- src/lib/routes/admin-api/context.test.ts | 9 --- src/lib/routes/admin-api/metrics.test.ts | 9 --- .../routes/admin-api/public-signup.test.ts | 9 --- src/lib/routes/admin-api/strategy.test.ts | 10 --- src/lib/routes/admin-api/tag.test.ts | 8 --- src/lib/routes/backstage.test.ts | 1 - src/lib/routes/client-api/metrics.test.ts | 13 ++-- src/lib/routes/client-api/register.test.ts | 6 -- src/lib/routes/health-check.test.ts | 6 -- src/lib/routes/public-invite.test.ts | 9 --- .../client-metrics/instance-service.test.ts | 62 +++---------------- .../client-metrics/instance-service.ts | 24 ------- src/lib/services/index.ts | 12 ++++ .../services/public-signup-token-service.ts | 7 --- .../client-metrics-service.e2e.test.ts | 9 +-- 17 files changed, 32 insertions(+), 180 deletions(-) diff --git a/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts b/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts index 5b817dc51b..c2ec3f4150 100644 --- a/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts +++ b/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts @@ -24,9 +24,6 @@ async function getSetup() { base, clientFeatureToggleStore: stores.clientFeatureToggleStore, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } @@ -43,7 +40,6 @@ const callGetAll = async (controller: FeatureController) => { let base; let request; -let destroy; let flagResolver; @@ -51,7 +47,6 @@ beforeEach(async () => { const setup = await getSetup(); base = setup.base; request = setup.request; - destroy = setup.destroy; flagResolver = { isEnabled: () => { return false; @@ -59,10 +54,6 @@ beforeEach(async () => { }; }); -afterEach(() => { - destroy(); -}); - test('should get empty getFeatures via client', () => { expect.assertions(1); return request diff --git a/src/lib/routes/admin-api/config.test.ts b/src/lib/routes/admin-api/config.test.ts index 99a14438cc..a73025a887 100644 --- a/src/lib/routes/admin-api/config.test.ts +++ b/src/lib/routes/admin-api/config.test.ts @@ -28,25 +28,16 @@ async function getSetup() { return { base, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let request; let base; -let destroy; beforeEach(async () => { const setup = await getSetup(); request = setup.request; base = setup.base; - destroy = setup.destroy; -}); - -afterEach(() => { - destroy(); }); test('should get ui config', async () => { diff --git a/src/lib/routes/admin-api/context.test.ts b/src/lib/routes/admin-api/context.test.ts index c3b40594a0..0a584ad332 100644 --- a/src/lib/routes/admin-api/context.test.ts +++ b/src/lib/routes/admin-api/context.test.ts @@ -20,25 +20,16 @@ async function getSetup() { return { base, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let base; let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); base = setup.base; request = setup.request; - destroy = setup.destroy; -}); - -afterEach(async () => { - await destroy(); }); test('should get all context definitions', () => { diff --git a/src/lib/routes/admin-api/metrics.test.ts b/src/lib/routes/admin-api/metrics.test.ts index ef56d0876d..2b704b7249 100644 --- a/src/lib/routes/admin-api/metrics.test.ts +++ b/src/lib/routes/admin-api/metrics.test.ts @@ -19,25 +19,16 @@ async function getSetup() { stores, perms, config, - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let stores; let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); stores = setup.stores; request = setup.request; - destroy = setup.destroy; -}); - -afterEach(() => { - destroy(); }); test('/api/admin/metrics/seen-toggles is deprecated', () => { diff --git a/src/lib/routes/admin-api/public-signup.test.ts b/src/lib/routes/admin-api/public-signup.test.ts index 342181bbbc..be0bb80135 100644 --- a/src/lib/routes/admin-api/public-signup.test.ts +++ b/src/lib/routes/admin-api/public-signup.test.ts @@ -33,16 +33,11 @@ describe('Public Signup API', () => { request: supertest(app), stores, perms, - destroy: () => { - services.clientInstanceService.destroy(); - services.publicSignupTokenService.destroy(); - }, }; } let stores; let request; - let destroy; const user = { username: 'some-username', @@ -55,12 +50,8 @@ describe('Public Signup API', () => { const setup = await getSetup(); stores = setup.stores; request = setup.request; - destroy = setup.destroy; }); - afterEach(() => { - destroy(); - }); const expireAt = (addDays: number = 7): Date => { const now = new Date(); now.setDate(now.getDate() + addDays); diff --git a/src/lib/routes/admin-api/strategy.test.ts b/src/lib/routes/admin-api/strategy.test.ts index f8538ec62a..d21225f098 100644 --- a/src/lib/routes/admin-api/strategy.test.ts +++ b/src/lib/routes/admin-api/strategy.test.ts @@ -5,8 +5,6 @@ import permissions from '../../../test/fixtures/permissions'; import getApp from '../../app'; import { createServices } from '../../services'; -let destroy; - async function getSetup() { const randomBase = `/random${Math.round(Math.random() * 1000)}`; const perms = permissions(); @@ -18,10 +16,6 @@ async function getSetup() { const services = createServices(stores, config); const app = await getApp(config, stores, services); - destroy = () => { - services.clientInstanceService.destroy(); - }; - return { base: randomBase, strategyStore: stores.strategyStore, @@ -30,10 +24,6 @@ async function getSetup() { }; } -afterEach(() => { - destroy(); -}); - test('add version numbers for /strategies', async () => { const { request, base } = await getSetup(); return request diff --git a/src/lib/routes/admin-api/tag.test.ts b/src/lib/routes/admin-api/tag.test.ts index ad183a62eb..5a0b2169ca 100644 --- a/src/lib/routes/admin-api/tag.test.ts +++ b/src/lib/routes/admin-api/tag.test.ts @@ -21,26 +21,18 @@ async function getSetup() { perms, tagStore: stores.tagStore, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let base; let tagStore; let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); base = setup.base; tagStore = setup.tagStore; request = setup.request; - destroy = setup.destroy; -}); -afterEach(() => { - destroy(); }); test('should get empty getTags via admin', () => { diff --git a/src/lib/routes/backstage.test.ts b/src/lib/routes/backstage.test.ts index 23572d5665..fd77cc9624 100644 --- a/src/lib/routes/backstage.test.ts +++ b/src/lib/routes/backstage.test.ts @@ -19,5 +19,4 @@ test('should enable prometheus', async () => { .get('/internal-backstage/prometheus') .expect('Content-Type', /text/) .expect(200); - services.clientInstanceService.destroy(); }); diff --git a/src/lib/routes/client-api/metrics.test.ts b/src/lib/routes/client-api/metrics.test.ts index 1fd8cea491..fb48c324f1 100644 --- a/src/lib/routes/client-api/metrics.test.ts +++ b/src/lib/routes/client-api/metrics.test.ts @@ -19,10 +19,7 @@ async function getSetup(opts?: IUnleashOptions) { request: supertest(app), stores: db.stores, services, - destroy: async () => { - services.clientInstanceService.destroy(); - await db.destroy(); - }, + destroy: db.destroy, }; } @@ -31,7 +28,7 @@ let stores: IUnleashStores; let services: IUnleashServices; let destroy; -beforeEach(async () => { +beforeAll(async () => { const setup = await getSetup(); request = setup.request; stores = setup.stores; @@ -39,10 +36,14 @@ beforeEach(async () => { services = setup.services; }); -afterEach(() => { +afterAll(() => { destroy(); }); +afterEach(async () => { + await stores.featureToggleStore.deleteAll(); +}); + test('should validate client metrics', () => { return request .post('/api/client/metrics') diff --git a/src/lib/routes/client-api/register.test.ts b/src/lib/routes/client-api/register.test.ts index 64704e7513..70e0dd21a5 100644 --- a/src/lib/routes/client-api/register.test.ts +++ b/src/lib/routes/client-api/register.test.ts @@ -14,20 +14,14 @@ async function getSetup() { return { request: supertest(app), stores, - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); request = setup.request; - destroy = setup.destroy; }); afterEach(() => { - destroy(); getLogger.setMuteError(false); }); diff --git a/src/lib/routes/health-check.test.ts b/src/lib/routes/health-check.test.ts index 1149f8e8f6..a9b0ec657d 100644 --- a/src/lib/routes/health-check.test.ts +++ b/src/lib/routes/health-check.test.ts @@ -15,21 +15,15 @@ async function getSetup() { return { request: supertest(app), stores, - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); request = setup.request; - destroy = setup.destroy; }); afterEach(() => { - destroy(); getLogger.setMuteError(false); }); diff --git a/src/lib/routes/public-invite.test.ts b/src/lib/routes/public-invite.test.ts index b55dc6ddd3..c76bf6aff4 100644 --- a/src/lib/routes/public-invite.test.ts +++ b/src/lib/routes/public-invite.test.ts @@ -39,16 +39,11 @@ describe('Public Signup API', () => { request: supertest(app), stores, perms, - destroy: () => { - services.clientInstanceService.destroy(); - services.publicSignupTokenService.destroy(); - }, }; } let stores; let request; - let destroy; const user = { username: 'some-username', @@ -61,12 +56,8 @@ describe('Public Signup API', () => { const setup = await getSetup(); stores = setup.stores; request = setup.request; - destroy = setup.destroy; }); - afterEach(() => { - destroy(); - }); const expireAt = (addDays: number = 7): Date => { const now = new Date(); now.setDate(now.getDate() + addDays); diff --git a/src/lib/services/client-metrics/instance-service.test.ts b/src/lib/services/client-metrics/instance-service.test.ts index dcb53880dc..e706b7a488 100644 --- a/src/lib/services/client-metrics/instance-service.test.ts +++ b/src/lib/services/client-metrics/instance-service.test.ts @@ -5,45 +5,11 @@ import FakeEventStore from '../../../test/fixtures/fake-event-store'; import { createTestConfig } from '../../../test/config/test-config'; import { FakePrivateProjectChecker } from '../../features/private-project/fakePrivateProjectChecker'; -/** - * A utility to wait for any pending promises in the test subject code. - * For instance, if the test needs to wait for a timeout/interval handler, - * and that handler does something async, advancing the timers is not enough: - * We have to explicitly wait for the second promise. - * For more info, see https://stackoverflow.com/a/51045733/2868829 - * - * Usage in test code after advancing timers, but before making assertions: - * - * test('hello', async () => { - * jest.useFakeTimers('modern'); - * - * // Schedule a timeout with a callback that does something async - * // before calling our spy - * const spy = jest.fn(); - * setTimeout(async () => { - * await Promise.resolve(); - * spy(); - * }, 1000); - * - * expect(spy).not.toHaveBeenCalled(); - * - * jest.advanceTimersByTime(1500); - * await flushPromises(); // this is required to make it work! - * - * expect(spy).toHaveBeenCalledTimes(1); - * - * jest.useRealTimers(); - * }); - */ -function flushPromises() { - return Promise.resolve(setImmediate); -} let config; beforeAll(() => { config = createTestConfig({}); }); test('Multiple registrations of same appname and instanceid within same time period should only cause one registration', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -75,8 +41,8 @@ test('Multiple registrations of same appname and instanceid within same time per await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); - jest.advanceTimersByTime(7000); - await flushPromises(); + + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService expect(appStoreSpy).toHaveBeenCalledTimes(1); expect(bulkSpy).toHaveBeenCalledTimes(1); @@ -93,7 +59,6 @@ test('Multiple registrations of same appname and instanceid within same time per }); test('Multiple unique clients causes multiple registrations', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -136,16 +101,14 @@ test('Multiple unique clients causes multiple registrations', async () => { await clientMetrics.registerClient(client2, '127.0.0.1'); await clientMetrics.registerClient(client2, '127.0.0.1'); - jest.advanceTimersByTime(7000); - await flushPromises(); + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService const registrations = appStoreSpy.mock.calls[0][0]; expect(registrations.length).toBe(2); - jest.useRealTimers(); }); + test('Same client registered outside of dedup interval will be registered twice', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -155,8 +118,6 @@ test('Same client registered outside of dedup interval will be registered twice' bulkUpsert: bulkSpy, }; - const bulkInterval = secondsToMilliseconds(2); - const clientMetrics = new ClientInstanceService( { clientMetricsStoreV2: null, @@ -168,7 +129,6 @@ test('Same client registered outside of dedup interval will be registered twice' }, config, new FakePrivateProjectChecker(), - bulkInterval, ); const client1 = { appName: 'test_app', @@ -181,14 +141,13 @@ test('Same client registered outside of dedup interval will be registered twice' await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); - jest.advanceTimersByTime(3000); + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); - jest.advanceTimersByTime(3000); - await flushPromises(); + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService expect(appStoreSpy).toHaveBeenCalledTimes(2); expect(bulkSpy).toHaveBeenCalledTimes(2); @@ -198,11 +157,9 @@ test('Same client registered outside of dedup interval will be registered twice' expect(firstRegistrations.appName).toBe(secondRegistrations.appName); expect(firstRegistrations.instanceId).toBe(secondRegistrations.instanceId); - jest.useRealTimers(); }); test('No registrations during a time period will not call stores', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -211,7 +168,7 @@ test('No registrations during a time period will not call stores', async () => { const clientInstanceStore: any = { bulkUpsert: bulkSpy, }; - new ClientInstanceService( + const clientMetrics = new ClientInstanceService( { clientMetricsStoreV2: null, strategyStore: null, @@ -223,8 +180,9 @@ test('No registrations during a time period will not call stores', async () => { config, new FakePrivateProjectChecker(), ); - jest.advanceTimersByTime(6000); + + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService + expect(appStoreSpy).toHaveBeenCalledTimes(0); expect(bulkSpy).toHaveBeenCalledTimes(0); - jest.useRealTimers(); }); diff --git a/src/lib/services/client-metrics/instance-service.ts b/src/lib/services/client-metrics/instance-service.ts index 9af1bb4193..7b31d891a6 100644 --- a/src/lib/services/client-metrics/instance-service.ts +++ b/src/lib/services/client-metrics/instance-service.ts @@ -29,8 +29,6 @@ export default class ClientInstanceService { seenClients: Record = {}; - private timers: NodeJS.Timeout[] = []; - private clientMetricsStoreV2: IClientMetricsStoreV2; private strategyStore: IStrategyStore; @@ -47,10 +45,6 @@ export default class ClientInstanceService { private flagResolver: IFlagResolver; - private bulkInterval: number; - - private announcementInterval: number; - constructor( { clientMetricsStoreV2, @@ -73,8 +67,6 @@ export default class ClientInstanceService { flagResolver, }: Pick, privateProjectChecker: IPrivateProjectChecker, - bulkInterval = secondsToMilliseconds(5), - announcementInterval = minutesToMilliseconds(5), ) { this.clientMetricsStoreV2 = clientMetricsStoreV2; this.strategyStore = strategyStore; @@ -87,18 +79,6 @@ export default class ClientInstanceService { this.logger = getLogger( '/services/client-metrics/client-instance-service.ts', ); - - this.bulkInterval = bulkInterval; - this.announcementInterval = announcementInterval; - this.timers.push( - setInterval(() => this.bulkAdd(), this.bulkInterval).unref(), - ); - this.timers.push( - setInterval( - () => this.announceUnannounced(), - this.announcementInterval, - ).unref(), - ); } public async registerInstance( @@ -248,8 +228,4 @@ export default class ClientInstanceService { async removeInstancesOlderThanTwoDays(): Promise { return this.clientInstanceStore.removeInstancesOlderThanTwoDays(); } - - destroy(): void { - this.timers.forEach(clearInterval); - } } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 213093f9aa..e5638cf7a9 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -165,6 +165,18 @@ export const scheduleServices = async ( 'removeInstancesOlderThanTwoDays', ); + schedulerService.schedule( + clientInstanceService.bulkAdd.bind(clientInstanceService), + secondsToMilliseconds(5), + 'bulkAddInstances', + ); + + schedulerService.schedule( + clientInstanceService.announceUnannounced.bind(clientInstanceService), + minutesToMilliseconds(5), + 'announceUnannounced', + ); + schedulerService.schedule( projectService.statusJob.bind(projectService), hoursToMilliseconds(24), diff --git a/src/lib/services/public-signup-token-service.ts b/src/lib/services/public-signup-token-service.ts index 3fd855cdb5..5701029797 100644 --- a/src/lib/services/public-signup-token-service.ts +++ b/src/lib/services/public-signup-token-service.ts @@ -30,8 +30,6 @@ export class PublicSignupTokenService { private logger: Logger; - private timer: NodeJS.Timeout; - private readonly unleashBase: string; constructor( @@ -146,9 +144,4 @@ export class PublicSignupTokenService { private getMinimumDate(date1: Date, date2: Date): Date { return date1 < date2 ? date1 : date2; } - - destroy(): void { - clearInterval(this.timer); - this.timer = null; - } } diff --git a/src/test/e2e/services/client-metrics-service.e2e.test.ts b/src/test/e2e/services/client-metrics-service.e2e.test.ts index 2f258e19a8..e9088285dc 100644 --- a/src/test/e2e/services/client-metrics-service.e2e.test.ts +++ b/src/test/e2e/services/client-metrics-service.e2e.test.ts @@ -12,7 +12,7 @@ const { APPLICATION_CREATED } = require('../../../lib/types/events'); let stores; let db; -let clientInstanceService; +let clientInstanceService: ClientInstanceService; let config: IUnleashConfig; beforeAll(async () => { db = await dbInit('client_metrics_service_serial', getLogger); @@ -25,13 +25,10 @@ beforeAll(async () => { stores, config, new FakePrivateProjectChecker(), - bulkInterval, - announcementInterval, ); }); afterAll(async () => { - await clientInstanceService.destroy(); await db.destroy(); }); test('Apps registered should be announced', async () => { @@ -58,11 +55,11 @@ test('Apps registered should be announced', async () => { }; await clientInstanceService.registerClient(clientRegistration, '127.0.0.1'); await clientInstanceService.registerClient(differentClient, '127.0.0.1'); - await new Promise((res) => setTimeout(res, 1200)); + await clientInstanceService.bulkAdd(); // in prod called by a SchedulerService const first = await stores.clientApplicationsStore.getUnannounced(); expect(first.length).toBe(2); await clientInstanceService.registerClient(clientRegistration, '127.0.0.1'); - await new Promise((res) => setTimeout(res, secondsToMilliseconds(2))); + await clientInstanceService.announceUnannounced(); // in prod called by a SchedulerService const second = await stores.clientApplicationsStore.getUnannounced(); expect(second.length).toBe(0); const events = await stores.eventStore.getEvents(); From 8bc04c59f34ab8e40130e85e2fdc7a6010828ceb Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 23 Oct 2023 16:28:19 +0200 Subject: [PATCH 5/6] refactor: move metrics service scheduling (#5129) --- src/lib/db/client-instance-store.ts | 6 +---- src/lib/server-impl.ts | 2 -- .../last-seen/last-seen-service.ts | 6 ----- .../client-metrics/metrics-service-v2.ts | 22 +++---------------- src/lib/services/index.ts | 17 ++++++++++++++ src/test/e2e/helpers/database-init.ts | 2 -- .../services/last-seen-service.e2e.test.ts | 6 ----- 7 files changed, 21 insertions(+), 40 deletions(-) diff --git a/src/lib/db/client-instance-store.ts b/src/lib/db/client-instance-store.ts index 515544d9b6..bafe0ab053 100644 --- a/src/lib/db/client-instance-store.ts +++ b/src/lib/db/client-instance-store.ts @@ -51,8 +51,6 @@ export default class ClientInstanceStore implements IClientInstanceStore { private metricTimer: Function; - private timer: Timeout; - constructor(db: Db, eventBus: EventEmitter, getLogger: LogProvider) { this.db = db; this.eventBus = eventBus; @@ -197,7 +195,5 @@ export default class ClientInstanceStore implements IClientInstanceStore { return this.db(TABLE).where('app_name', appName).del(); } - destroy(): void { - clearInterval(this.timer); - } + destroy(): void {} } diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index bc90403645..ae7d3da7d3 100644 --- a/src/lib/server-impl.ts +++ b/src/lib/server-impl.ts @@ -58,8 +58,6 @@ async function createApp( } services.schedulerService.stop(); metricsMonitor.stopMonitoring(); - stores.clientInstanceStore.destroy(); - services.clientMetricsServiceV2.destroy(); services.addonService.destroy(); await db.destroy(); }; diff --git a/src/lib/services/client-metrics/last-seen/last-seen-service.ts b/src/lib/services/client-metrics/last-seen/last-seen-service.ts index 555b5eb930..4bb6d5a282 100644 --- a/src/lib/services/client-metrics/last-seen/last-seen-service.ts +++ b/src/lib/services/client-metrics/last-seen/last-seen-service.ts @@ -11,8 +11,6 @@ export type LastSeenInput = { }; export class LastSeenService { - private timers: NodeJS.Timeout[] = []; - private lastSeenToggles: Map = new Map(); private logger: Logger; @@ -79,8 +77,4 @@ export class LastSeenService { async cleanLastSeen() { await this.lastSeenStore.cleanLastSeen(); } - - destroy(): void { - this.timers.forEach(clearInterval); - } } diff --git a/src/lib/services/client-metrics/metrics-service-v2.ts b/src/lib/services/client-metrics/metrics-service-v2.ts index db950e342f..4a83f659a0 100644 --- a/src/lib/services/client-metrics/metrics-service-v2.ts +++ b/src/lib/services/client-metrics/metrics-service-v2.ts @@ -25,8 +25,6 @@ import { nameSchema } from '../../schema/feature-schema'; export default class ClientMetricsServiceV2 { private config: IUnleashConfig; - private timers: NodeJS.Timeout[] = []; - private unsavedMetrics: IClientMetricsEnv[] = []; private clientMetricsStoreV2: IClientMetricsStoreV2; @@ -41,7 +39,6 @@ export default class ClientMetricsServiceV2 { { clientMetricsStoreV2 }: Pick, config: IUnleashConfig, lastSeenService: LastSeenService, - bulkInterval = secondsToMilliseconds(5), ) { this.clientMetricsStoreV2 = clientMetricsStoreV2; this.lastSeenService = lastSeenService; @@ -50,18 +47,10 @@ export default class ClientMetricsServiceV2 { '/services/client-metrics/client-metrics-service-v2.ts', ); this.flagResolver = config.flagResolver; + } - this.timers.push( - setInterval(() => { - this.bulkAdd().catch(console.error); - }, bulkInterval).unref(), - ); - - this.timers.push( - setInterval(() => { - this.clientMetricsStoreV2.clearMetrics(48).catch(console.error); - }, hoursToMilliseconds(12)).unref(), - ); + async clearMetrics(hoursAgo: number) { + return this.clientMetricsStoreV2.clearMetrics(hoursAgo); } async filterValidToggleNames(toggleNames: string[]): Promise { @@ -245,9 +234,4 @@ export default class ClientMetricsServiceV2 { } return 'default'; } - - destroy(): void { - this.timers.forEach(clearInterval); - this.lastSeenService.destroy(); - } } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index e5638cf7a9..d1823cdde3 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -119,6 +119,7 @@ export const scheduleServices = async ( versionService, lastSeenService, proxyService, + clientMetricsServiceV2, } = services; if (await maintenanceService.isMaintenanceMode()) { @@ -224,6 +225,22 @@ export const scheduleServices = async ( minutesToMilliseconds(2), 'fetchFrontendSettings', ); + + schedulerService.schedule( + () => { + clientMetricsServiceV2.bulkAdd().catch(console.error); + }, + secondsToMilliseconds(5), + 'bulkAddMetrics', + ); + + schedulerService.schedule( + () => { + clientMetricsServiceV2.clearMetrics(48).catch(console.error); + }, + hoursToMilliseconds(12), + 'clearMetrics', + ); }; export const createServices = ( diff --git a/src/test/e2e/helpers/database-init.ts b/src/test/e2e/helpers/database-init.ts index a5b7f93121..2a51f9dba2 100644 --- a/src/test/e2e/helpers/database-init.ts +++ b/src/test/e2e/helpers/database-init.ts @@ -117,9 +117,7 @@ export default async function init( await setupDatabase(stores); }, destroy: async () => { - const { clientInstanceStore } = stores; return new Promise((resolve, reject) => { - clientInstanceStore.destroy(); testDb.destroy((error) => (error ? reject(error) : resolve())); }); }, diff --git a/src/test/e2e/services/last-seen-service.e2e.test.ts b/src/test/e2e/services/last-seen-service.e2e.test.ts index e1745a8b1d..49ad347aac 100644 --- a/src/test/e2e/services/last-seen-service.e2e.test.ts +++ b/src/test/e2e/services/last-seen-service.e2e.test.ts @@ -56,8 +56,6 @@ test('Should update last seen for known toggles', async () => { const t1 = await stores.featureToggleStore.get('ta1'); expect(t1.lastSeenAt.getTime()).toBeGreaterThan(time); - - service.destroy(); }); test('Should not update last seen toggles with 0 metrics', async () => { @@ -102,8 +100,6 @@ test('Should not update last seen toggles with 0 metrics', async () => { expect(t2.lastSeenAt).toBeNull(); expect(t1.lastSeenAt.getTime()).toBeGreaterThanOrEqual(time); - - service.destroy(); }); test('Should not update anything for 0 toggles', async () => { @@ -144,6 +140,4 @@ test('Should not update anything for 0 toggles', async () => { const count = await service.store(); expect(count).toBe(0); - - service.destroy(); }); From ab390dbaabe5a32b6a396552b244a854bb4e4edc Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 23 Oct 2023 17:14:41 +0200 Subject: [PATCH 6/6] test: silent migration test (#5131) --- src/test/e2e/migrator.e2e.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/e2e/migrator.e2e.test.ts b/src/test/e2e/migrator.e2e.test.ts index 9e7d6d2751..297b4870f0 100644 --- a/src/test/e2e/migrator.e2e.test.ts +++ b/src/test/e2e/migrator.e2e.test.ts @@ -1,9 +1,12 @@ import { getDbConfig } from './helpers/database-config'; import { createTestConfig } from '../config/test-config'; import { getInstance } from 'db-migrate'; +import { log } from 'db-migrate-shared'; import { Client } from 'pg'; import { IDBOption } from 'lib/types'; +log.setLogLevel('error'); + async function initSchema(db: IDBOption): Promise { const client = new Client(db); await client.connect(); @@ -30,6 +33,8 @@ test('Up & down migrations work', async () => { connectionTimeoutMillis: 2000, }; + // disable Intellij/WebStorm from setting verbose CLI argument to db-migrator + process.argv = process.argv.filter((it) => !it.includes('--verbose')); const dbm = getInstance(true, { cwd: `${__dirname}/../../`, // relative to src/test/e2e config: { e2e },