From 90d6c7c0ba5a36a6666d64218227e63e8c2c3f00 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 20 Nov 2023 12:42:24 +0100 Subject: [PATCH 01/11] chore: remove usage of feature naming pattern flag (#5364) In preparation for this feature going GA --- .../feature/FeatureForm/FeatureForm.tsx | 6 +- .../ProjectEnterpriseSettingsForm.tsx | 202 ++++++++---------- frontend/src/interfaces/uiConfig.ts | 1 - .../__snapshots__/create-config.test.ts.snap | 1 - .../export-import.e2e.test.ts | 1 - .../feature-toggle/feature-toggle-service.ts | 23 +- .../tests/feature-toggle-service.e2e.test.ts | 2 +- src/lib/types/experimental.ts | 5 - src/server-dev.ts | 1 - src/test/e2e/api/client/feature.e2e.test.ts | 1 - 10 files changed, 108 insertions(+), 135 deletions(-) diff --git a/frontend/src/component/feature/FeatureForm/FeatureForm.tsx b/frontend/src/component/feature/FeatureForm/FeatureForm.tsx index 2da886aa89..436c625e64 100644 --- a/frontend/src/component/feature/FeatureForm/FeatureForm.tsx +++ b/frontend/src/component/feature/FeatureForm/FeatureForm.tsx @@ -23,7 +23,6 @@ import React from 'react'; import { useAuthPermissions } from 'hooks/api/getters/useAuth/useAuthPermissions'; import { FeatureNamingType } from 'interfaces/project'; import { FeatureNamingPatternInfo } from '../FeatureNamingPatternInfo/FeatureNamingPatternInfo'; -import { useUiFlag } from 'hooks/useUiFlag'; interface IFeatureToggleForm { type: string; @@ -122,15 +121,12 @@ const FeatureForm: React.FC = ({ const navigate = useNavigate(); const { permissions } = useAuthPermissions(); const editable = mode !== 'Edit'; - const featureNamingPatternEnabled = useUiFlag('featureNamingPattern'); const renderToggleDescription = () => { return featureTypes.find((toggle) => toggle.id === type)?.description; }; - const displayFeatureNamingInfo = Boolean( - featureNamingPatternEnabled && featureNaming?.pattern, - ); + const displayFeatureNamingInfo = Boolean(featureNaming?.pattern); React.useEffect(() => { if (featureNaming?.pattern && validateToggleName && name) { diff --git a/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/ProjectEnterpriseSettingsForm.tsx b/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/ProjectEnterpriseSettingsForm.tsx index 559d04e590..41f54c664d 100644 --- a/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/ProjectEnterpriseSettingsForm.tsx +++ b/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/ProjectEnterpriseSettingsForm.tsx @@ -136,7 +136,6 @@ const ProjectEnterpriseSettingsForm: React.FC = clearErrors, }) => { const privateProjects = useUiFlag('privateProjects'); - const shouldShowFlagNaming = useUiFlag('featureNamingPattern'); const { setPreviousPattern, trackPattern } = useFeatureNamePatternTracking(); @@ -253,115 +252,104 @@ const ProjectEnterpriseSettingsForm: React.FC = options={projectModeOptions} /> - - - Feature flag naming pattern? - - - - -

- Define a{' '} - - JavaScript RegEx - {' '} - used to enforce feature flag names - within this project. The regex will be - surrounded by a leading ^{' '} - and a trailing $. -

-

- Leave it empty if you don’t want to add - a naming pattern. -

- - - - - ^ - - ), - endAdornment: ( - - $ - - ), - }} - type={'text'} - value={featureNamingPattern || ''} - error={Boolean(errors.featureNamingPattern)} - errorText={errors.featureNamingPattern} - onChange={(e) => - onSetFeatureNamingPattern( - e.target.value, - ) - } - /> - -

- The example and description will be - shown to users when they create a new - feature flag in this project. -

- + + + Feature flag naming pattern? + + + + +

+ Define a{' '} + + JavaScript RegEx + {' '} + used to enforce feature flag names within this + project. The regex will be surrounded by a + leading ^ and a trailing{' '} + $. +

+

+ Leave it empty if you don’t want to add a naming + pattern. +

+ + + + + ^ + + ), + endAdornment: ( + + $ + + ), + }} + type={'text'} + value={featureNamingPattern || ''} + error={Boolean(errors.featureNamingPattern)} + errorText={errors.featureNamingPattern} + onChange={(e) => + onSetFeatureNamingPattern(e.target.value) + } + /> + +

+ The example and description will be shown to + users when they create a new feature flag in + this project. +

+ - - onSetFeatureNamingExample( - e.target.value, - ) - } - /> - .. + + onSetFeatureNamingExample(e.target.value) + } + /> + .. The flag name should contain the project name, the feature name, and the ticket number, each separated by a dot.`} - multiline - minRows={5} - value={featureNamingDescription || ''} - onChange={(e) => - onSetFeatureNamingDescription( - e.target.value, - ) - } - /> - - - } - /> + multiline + minRows={5} + value={featureNamingDescription || ''} + onChange={(e) => + onSetFeatureNamingDescription(e.target.value) + } + /> + + {children} ); diff --git a/frontend/src/interfaces/uiConfig.ts b/frontend/src/interfaces/uiConfig.ts index ffb6884c5c..41be2d88d3 100644 --- a/frontend/src/interfaces/uiConfig.ts +++ b/frontend/src/interfaces/uiConfig.ts @@ -61,7 +61,6 @@ export type UiFlags = { customRootRolesKillSwitch?: boolean; strategyVariant?: boolean; lastSeenByEnvironment?: boolean; - featureNamingPattern?: boolean; doraMetrics?: boolean; variantTypeNumber?: boolean; privateProjects?: boolean; diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 6cb1885dc1..4ea4c88925 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -84,7 +84,6 @@ exports[`should create default config 1`] = ` "doraMetrics": false, "embedProxy": true, "embedProxyFrontend": true, - "featureNamingPattern": false, "featureSearchAPI": false, "featureSearchFrontend": false, "featuresExportImport": true, diff --git a/src/lib/features/export-import-toggles/export-import.e2e.test.ts b/src/lib/features/export-import-toggles/export-import.e2e.test.ts index 9095b48ac1..06f49a6103 100644 --- a/src/lib/features/export-import-toggles/export-import.e2e.test.ts +++ b/src/lib/features/export-import-toggles/export-import.e2e.test.ts @@ -159,7 +159,6 @@ beforeAll(async () => { experimental: { flags: { featuresExportImport: true, - featureNamingPattern: true, dependentFeatures: true, }, }, diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index 9e665cde69..6a42310171 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -1168,22 +1168,21 @@ class FeatureToggleService { projectId: string, featureNames: string[], ): Promise { - if (this.flagResolver.isEnabled('featureNamingPattern')) { - const project = await this.projectStore.get(projectId); - const patternData = project.featureNaming; - const namingPattern = patternData?.pattern; + const project = await this.projectStore.get(projectId); + const patternData = project.featureNaming; + const namingPattern = patternData?.pattern; - if (namingPattern) { - const result = checkFeatureFlagNamesAgainstPattern( - featureNames, - namingPattern, - ); + if (namingPattern) { + const result = checkFeatureFlagNamesAgainstPattern( + featureNames, + namingPattern, + ); - if (result.state === 'invalid') { - return { ...result, featureNaming: patternData }; - } + if (result.state === 'invalid') { + return { ...result, featureNaming: patternData }; } } + return { state: 'valid' }; } diff --git a/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts index 974c51c45e..0093f71b18 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts @@ -39,7 +39,7 @@ const irrelevantDate = new Date(); beforeAll(async () => { const config = createTestConfig({ experimental: { - flags: { featureNamingPattern: true, playgroundImprovements: true }, + flags: { playgroundImprovements: true }, }, }); db = await dbInit( diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index d6825d5eb7..7b5e15bccd 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -23,7 +23,6 @@ export type IFlagKey = | 'filterInvalidClientMetrics' | 'lastSeenByEnvironment' | 'customRootRolesKillSwitch' - | 'featureNamingPattern' | 'doraMetrics' | 'variantTypeNumber' | 'privateProjects' @@ -116,10 +115,6 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_CUSTOM_ROOT_ROLES_KILL_SWITCH, false, ), - featureNamingPattern: parseEnvVarBoolean( - process.env.UNLEASH_EXPERIMENTAL_FEATURE_NAMING_PATTERN, - false, - ), doraMetrics: parseEnvVarBoolean( process.env.UNLEASH_EXPERIMENTAL_DORA_METRICS, false, diff --git a/src/server-dev.ts b/src/server-dev.ts index 5755f862a3..37271519ee 100644 --- a/src/server-dev.ts +++ b/src/server-dev.ts @@ -38,7 +38,6 @@ process.nextTick(async () => { anonymiseEventLog: false, responseTimeWithAppNameKillSwitch: false, lastSeenByEnvironment: true, - featureNamingPattern: true, doraMetrics: true, variantTypeNumber: true, privateProjects: true, diff --git a/src/test/e2e/api/client/feature.e2e.test.ts b/src/test/e2e/api/client/feature.e2e.test.ts index b2fe54eec0..c68de4738a 100644 --- a/src/test/e2e/api/client/feature.e2e.test.ts +++ b/src/test/e2e/api/client/feature.e2e.test.ts @@ -21,7 +21,6 @@ beforeAll(async () => { experimental: { flags: { strictSchemaValidation: true, - featureNamingPattern: true, dependentFeatures: true, }, }, From 0ba99a6162a3f2d70e08d2e5db71cff0a53d89ef Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 20 Nov 2023 17:02:41 +0100 Subject: [PATCH 02/11] fix: handle check against non existing projects (#5368) Instead of throwing an error when the project doesn't exist, we say that the names are valid, because we have nothing to say that they're not. Presumably there is already something in place to prevent you from importing into a non-existent project. --- .../feature-toggle/feature-toggle-service.ts | 32 +++++++++++++------ .../tests/feature-toggle-service.e2e.test.ts | 14 ++++++++ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index 6a42310171..4bc977e6e8 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -1168,19 +1168,31 @@ class FeatureToggleService { projectId: string, featureNames: string[], ): Promise { - const project = await this.projectStore.get(projectId); - const patternData = project.featureNaming; - const namingPattern = patternData?.pattern; + try { + const project = await this.projectStore.get(projectId); - if (namingPattern) { - const result = checkFeatureFlagNamesAgainstPattern( - featureNames, - namingPattern, + const patternData = project.featureNaming; + const namingPattern = patternData?.pattern; + + if (namingPattern) { + const result = checkFeatureFlagNamesAgainstPattern( + featureNames, + namingPattern, + ); + + if (result.state === 'invalid') { + return { ...result, featureNaming: patternData }; + } + } + } catch (error) { + // the project doesn't exist, so there's nothing to + // validate against + this.logger.info( + "Got an error when trying to validate flag naming patterns. It is probably because the target project doesn't exist. Here's the error:", + error.message, ); - if (result.state === 'invalid') { - return { ...result, featureNaming: patternData }; - } + return { state: 'valid' }; } return { state: 'valid' }; diff --git a/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts index 0093f71b18..c27e36f686 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts @@ -641,6 +641,20 @@ describe('flag name validation', () => { ).resolves.toBeFalsy(); } }); + + test("should allow anything if the project doesn't exist", async () => { + const projectId = 'project-that-doesnt-exist'; + const validFeatures = ['testpattern-feature', 'testpattern-feature2']; + + for (const feature of validFeatures) { + await expect( + service.validateFeatureFlagNameAgainstPattern( + feature, + projectId, + ), + ).resolves.toBeFalsy(); + } + }); }); test('Should return last seen at per environment', async () => { From 11533bf97aad1e45d0073777abe18c33bdfeb6ab Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Tue, 21 Nov 2023 08:18:00 +0100 Subject: [PATCH 03/11] refactor: remove feature flag for Dora (#5367) --- .../ExperimentalFeedback.tsx} | 50 +++++++------------ .../src/component/project/Project/Project.tsx | 3 +- .../ProjectDoraMetrics/ProjectDoraMetrics.tsx | 2 - .../__snapshots__/create-config.test.ts.snap | 1 - src/lib/routes/admin-api/project/index.ts | 22 +++----- src/lib/types/experimental.ts | 5 -- src/server-dev.ts | 1 - 7 files changed, 28 insertions(+), 56 deletions(-) rename frontend/src/component/{project/Project/ProjectDoraMetrics/ProjectDoraFeedback/ProjectDoraFeedback.tsx => common/ExperimentalFeedback/ExperimentalFeedback.tsx} (75%) diff --git a/frontend/src/component/project/Project/ProjectDoraMetrics/ProjectDoraFeedback/ProjectDoraFeedback.tsx b/frontend/src/component/common/ExperimentalFeedback/ExperimentalFeedback.tsx similarity index 75% rename from frontend/src/component/project/Project/ProjectDoraMetrics/ProjectDoraFeedback/ProjectDoraFeedback.tsx rename to frontend/src/component/common/ExperimentalFeedback/ExperimentalFeedback.tsx index c5e8ede757..5827f55016 100644 --- a/frontend/src/component/project/Project/ProjectDoraMetrics/ProjectDoraFeedback/ProjectDoraFeedback.tsx +++ b/frontend/src/component/common/ExperimentalFeedback/ExperimentalFeedback.tsx @@ -1,7 +1,7 @@ import { useState, useEffect } from 'react'; import { Box, Button, Divider, Typography, styled } from '@mui/material'; import { PermMedia, Send } from '@mui/icons-material'; -import { usePlausibleTracker } from 'hooks/usePlausibleTracker'; +import { CustomEvents, usePlausibleTracker } from 'hooks/usePlausibleTracker'; import { createLocalStorage } from 'utils/createLocalStorage'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; @@ -61,12 +61,21 @@ const StyledLink = styled('a')(({ theme }) => ({ textDecoration: 'none', })); -export const ProjectDoraFeedback = () => { +interface IExperimentalFeedbackProps { + trackerKey: string; + eventKey: CustomEvents; + description: string; + sketchURL: string; +} + +export const ExperimentalFeedback: React.FC = ({ + trackerKey, + eventKey, + description, + sketchURL, +}) => { const { trackEvent } = usePlausibleTracker(); - const { value, setValue } = createLocalStorage( - `project:metrics:plausible`, - { sent: false }, - ); + const { value, setValue } = createLocalStorage(trackerKey, { sent: false }); const [metrics, setMetrics] = useState(value); useEffect(() => { @@ -75,7 +84,7 @@ export const ProjectDoraFeedback = () => { const onBtnClick = (type: string) => { try { - trackEvent('project-metrics', { + trackEvent(eventKey, { props: { eventType: type, }, @@ -91,7 +100,7 @@ export const ProjectDoraFeedback = () => { const recipientEmail = 'ux@getunleash.io'; const emailSubject = "I'd like to get involved"; - const emailBody = `Hello Unleash,\n\nI just saw the new metrics page you are experimenting with in Unleash. I'd like to be involved in user tests and give my feedback on this feature.\n\nRegards,\n`; + const emailBody = `Hello Unleash,\n\nI just saw your ${eventKey} experiment. I'd like to be involved in user tests and give my feedback on this feature.\n\nRegards,\n`; const mailtoURL = `mailto:${recipientEmail}?subject=${encodeURIComponent( emailSubject, @@ -102,31 +111,10 @@ export const ProjectDoraFeedback = () => { We are trying something experimental! - - We are considering adding project metrics to see how a project - performs. As a first step, we have added a{' '} - lead time for changes indicator that is calculated per - feature toggle based on the creation of the feature toggle and - when it was first turned on in an environment of type - production. - + {description}
- - DORA is a method for measuring the performance of your DevOps - teams. It measures four different metrics. You can read Google's - blog post about{' '} - - DORA metrics - {' '} - for more information. - - { diff --git a/frontend/src/component/project/Project/Project.tsx b/frontend/src/component/project/Project/Project.tsx index cf1ff921a9..58427523a0 100644 --- a/frontend/src/component/project/Project/Project.tsx +++ b/frontend/src/component/project/Project/Project.tsx @@ -102,8 +102,7 @@ export const Project = () => { title: 'Metrics', path: `${basePath}/metrics`, name: 'dora', - flag: 'doraMetrics', - new: true, + isEnterprise: true, }, { title: 'Event log', diff --git a/frontend/src/component/project/Project/ProjectDoraMetrics/ProjectDoraMetrics.tsx b/frontend/src/component/project/Project/ProjectDoraMetrics/ProjectDoraMetrics.tsx index 2ad4c61c18..5cfb217d61 100644 --- a/frontend/src/component/project/Project/ProjectDoraMetrics/ProjectDoraMetrics.tsx +++ b/frontend/src/component/project/Project/ProjectDoraMetrics/ProjectDoraMetrics.tsx @@ -15,7 +15,6 @@ import { PageContent } from 'component/common/PageContent/PageContent'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { PageHeader } from 'component/common/PageHeader/PageHeader'; import { Badge } from 'component/common/Badge/Badge'; -import { ProjectDoraFeedback } from './ProjectDoraFeedback/ProjectDoraFeedback'; import { useConditionallyHiddenColumns } from 'hooks/useConditionallyHiddenColumns'; import theme from 'themes/theme'; @@ -194,7 +193,6 @@ export const ProjectDoraMetrics = () => { return ( <> - , ): Promise { - if (this.config.flagResolver.isEnabled('doraMetrics')) { - const { projectId } = req.params; + const { projectId } = req.params; - const dora = await this.projectService.getDoraMetrics(projectId); + const dora = await this.projectService.getDoraMetrics(projectId); - this.openApiService.respondWithValidation( - 200, - res, - projectDoraMetricsSchema.$id, - dora, - ); - } else { - throw new InvalidOperationError( - 'Feature dora metrics is not enabled', - ); - } + this.openApiService.respondWithValidation( + 200, + res, + projectDoraMetricsSchema.$id, + dora, + ); } } diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 7b5e15bccd..f6810809a7 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -23,7 +23,6 @@ export type IFlagKey = | 'filterInvalidClientMetrics' | 'lastSeenByEnvironment' | 'customRootRolesKillSwitch' - | 'doraMetrics' | 'variantTypeNumber' | 'privateProjects' | 'dependentFeatures' @@ -115,10 +114,6 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_CUSTOM_ROOT_ROLES_KILL_SWITCH, false, ), - doraMetrics: parseEnvVarBoolean( - process.env.UNLEASH_EXPERIMENTAL_DORA_METRICS, - false, - ), dependentFeatures: parseEnvVarBoolean( process.env.UNLEASH_EXPERIMENTAL_DEPENDENT_FEATURES, false, diff --git a/src/server-dev.ts b/src/server-dev.ts index 37271519ee..7920390cf0 100644 --- a/src/server-dev.ts +++ b/src/server-dev.ts @@ -38,7 +38,6 @@ process.nextTick(async () => { anonymiseEventLog: false, responseTimeWithAppNameKillSwitch: false, lastSeenByEnvironment: true, - doraMetrics: true, variantTypeNumber: true, privateProjects: true, dependentFeatures: true, From e79e30de96d7435fa2671fbb6b4b1f51cbe9fa67 Mon Sep 17 00:00:00 2001 From: Jaanus Sellin Date: Tue, 21 Nov 2023 10:08:20 +0200 Subject: [PATCH 04/11] fix: total number should be correct now in search (#5355) The issue was that we all features were created exactly in same time, and our feature counter waas expecting time to be unique to feature, which was not the case. --- .../feature-toggle/feature-toggle-strategies-store.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts index 92a39ea0f5..c3a5674233 100644 --- a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts +++ b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts @@ -709,7 +709,7 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { const [, envName] = sortBy.split(':'); rankingSql += this.db .raw( - `CASE WHEN feature_environments.environment = ? THEN feature_environments.enabled ELSE NULL END ${validatedSortOrder} NULLS LAST, features.created_at asc`, + `CASE WHEN feature_environments.environment = ? THEN feature_environments.enabled ELSE NULL END ${validatedSortOrder} NULLS LAST, features.created_at asc, features.name asc`, [envName], ) .toString(); @@ -718,9 +718,9 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { .raw(`?? ${validatedSortOrder}`, [ sortByMapping[sortBy], ]) - .toString()}, features.created_at asc`; + .toString()}, features.created_at asc, features.name asc`; } else { - rankingSql += `features.created_at ${validatedSortOrder}`; + rankingSql += `features.created_at ${validatedSortOrder}, features.name asc`; } query From 7a8c8c8d29e6982807f0a5deeaec0f5546c13001 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Tue, 21 Nov 2023 10:24:35 +0100 Subject: [PATCH 05/11] docs: variants reassignment (#5372) --- website/docs/reference/strategy-variants.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/website/docs/reference/strategy-variants.md b/website/docs/reference/strategy-variants.md index f7367aaee3..ce62857256 100644 --- a/website/docs/reference/strategy-variants.md +++ b/website/docs/reference/strategy-variants.md @@ -75,6 +75,8 @@ Unleash currently supports these payload types: When you have only one variant in a strategy, stickiness does not matter. If you decide to add multiple variants to the strategy, then variant stickiness is derived from the strategy stickiness. Strategy stickiness is calculated on the received user and context, as described in [the stickiness chapter](./stickiness.md). This ensures that the same user will consistently see the same variant. If no context data is provided, the traffic will be spread randomly for each request. +If you would like to reassign users to different variants using existing stickiness parameter then you can change the groupId of the strategy. This will provide different input to the stickiness calculation. + ### Strategy variants vs feature toggle variants Strategy variants take precedence over the [feature toggle variants](./feature-toggle-variants.md). If your matching activation strategy doesn't have any variants configured you will fall back to the [feature toggle variants](./feature-toggle-variants.md). From 27252f7728866d84374da36ecd2aede449e353d8 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 21 Nov 2023 10:29:43 +0100 Subject: [PATCH 06/11] chore: find segment strategies in CRs (#5365) This PR adds the ability to detect which strategies use a specific segment in active change requests. It does not wire this functionality up to anything just yet. Follow-up PRs will integrate this with the segment service and eventually with the front end. --- ...e-request-segment-usage-read-model.test.ts | 75 ++++++++++++++++++- ...change-request-segment-usage-read-model.ts | 14 ++++ ...change-request-segment-usage-read-model.ts | 20 ++++- ...change-request-segment-usage-read-model.ts | 38 +++++++++- .../openapi/spec/admin-strategies-schema.ts | 3 +- src/test/e2e/api/admin/segment.e2e.test.ts | 2 +- 6 files changed, 144 insertions(+), 8 deletions(-) diff --git a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts index 26ae25c690..f2581c2208 100644 --- a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts +++ b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts @@ -113,7 +113,7 @@ describe.each([ ])('Should handle %s changes correctly', (_, addOrUpdateStrategy) => { test.each([ ['Draft', true], - ['In Review', true], + ['In review', true], ['Scheduled', true], ['Approved', true], ['Rejected', false], @@ -133,3 +133,76 @@ describe.each([ }, ); }); + +test.each([ + ['Draft', true], + ['In review', true], + ['Scheduled', true], + ['Approved', true], + ['Rejected', false], + ['Cancelled', false], + ['Applied', false], +])( + 'addStrategy events in %s CRs should show up only of the CR is active', + async (state, isActiveCr) => { + await createCR(state); + + const segmentId = 3; + + await addStrategyToCr(segmentId, FLAG_NAME); + + const result = await readModel.getStrategiesUsedInActiveChangeRequests( + segmentId, + ); + if (isActiveCr) { + expect(result).toStrictEqual([ + { + projectId: 'default', + strategyName: 'flexibleRollout', + environment: 'default', + featureName: FLAG_NAME, + }, + ]); + } else { + expect(result).toStrictEqual([]); + } + }, +); + +test.each([ + ['Draft', true], + ['In review', true], + ['Scheduled', true], + ['Approved', true], + ['Rejected', false], + ['Cancelled', false], + ['Applied', false], +])( + `updateStrategy events in %s CRs should show up only of the CR is active`, + async (state, isActiveCr) => { + await createCR(state); + + const segmentId = 3; + + const strategyId = randomId(); + await updateStrategyInCr(strategyId, segmentId, FLAG_NAME); + + const result = await readModel.getStrategiesUsedInActiveChangeRequests( + segmentId, + ); + + if (isActiveCr) { + expect(result).toMatchObject([ + { + id: strategyId, + projectId: 'default', + strategyName: 'flexibleRollout', + environment: 'default', + featureName: FLAG_NAME, + }, + ]); + } else { + expect(result).toStrictEqual([]); + } + }, +); diff --git a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts index 08ba2f3217..4b680fd006 100644 --- a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts +++ b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts @@ -1,3 +1,17 @@ +type NewStrategy = { + projectId: string; + featureName: string; + strategyName: string; + environment: string; +}; + +type ExistingStrategy = NewStrategy & { id: string }; + +export type ChangeRequestStrategy = NewStrategy | ExistingStrategy; + export interface IChangeRequestSegmentUsageReadModel { isSegmentUsedInActiveChangeRequests(segmentId: number): Promise; + getStrategiesUsedInActiveChangeRequests( + segmentId: number, + ): Promise; } diff --git a/src/lib/features/change-request-segment-usage-service/fake-change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/fake-change-request-segment-usage-read-model.ts index d5cb25554a..a5e0573b45 100644 --- a/src/lib/features/change-request-segment-usage-service/fake-change-request-segment-usage-read-model.ts +++ b/src/lib/features/change-request-segment-usage-service/fake-change-request-segment-usage-read-model.ts @@ -1,16 +1,32 @@ -import { IChangeRequestSegmentUsageReadModel } from './change-request-segment-usage-read-model'; +import { + ChangeRequestStrategy, + IChangeRequestSegmentUsageReadModel, +} from './change-request-segment-usage-read-model'; export class FakeChangeRequestSegmentUsageReadModel implements IChangeRequestSegmentUsageReadModel { private isSegmentUsedInActiveChangeRequestsValue: boolean; + strategiesUsedInActiveChangeRequests: ChangeRequestStrategy[]; - constructor(isSegmentUsedInActiveChangeRequests = false) { + constructor( + isSegmentUsedInActiveChangeRequests = false, + strategiesUsedInActiveChangeRequests = [], + ) { this.isSegmentUsedInActiveChangeRequestsValue = isSegmentUsedInActiveChangeRequests; + + this.strategiesUsedInActiveChangeRequests = + strategiesUsedInActiveChangeRequests; } public async isSegmentUsedInActiveChangeRequests(): Promise { return this.isSegmentUsedInActiveChangeRequestsValue; } + + public async getStrategiesUsedInActiveChangeRequests(): Promise< + ChangeRequestStrategy[] + > { + return this.strategiesUsedInActiveChangeRequests; + } } diff --git a/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts index 47dbf632ae..f33f5606cc 100644 --- a/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts +++ b/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts @@ -1,5 +1,8 @@ import { Db } from '../../db/db'; -import { IChangeRequestSegmentUsageReadModel } from './change-request-segment-usage-read-model'; +import { + ChangeRequestStrategy, + IChangeRequestSegmentUsageReadModel, +} from './change-request-segment-usage-read-model'; export class ChangeRequestSegmentUsageReadModel implements IChangeRequestSegmentUsageReadModel @@ -9,7 +12,6 @@ export class ChangeRequestSegmentUsageReadModel constructor(db: Db) { this.db = db; } - public async isSegmentUsedInActiveChangeRequests( segmentId: number, ): Promise { @@ -17,7 +19,7 @@ export class ChangeRequestSegmentUsageReadModel `SELECT events.* FROM change_request_events events JOIN change_requests cr ON events.change_request_id = cr.id - WHERE cr.state IN ('Draft', 'In Review', 'Scheduled', 'Approved') + WHERE cr.state IN ('Draft', 'In review', 'Scheduled', 'Approved') AND events.action IN ('updateStrategy', 'addStrategy');`, ); @@ -27,4 +29,34 @@ export class ChangeRequestSegmentUsageReadModel return isUsed; } + + mapRow = (row): ChangeRequestStrategy => { + const { payload, project, environment, feature } = row; + return { + projectId: project, + featureName: feature, + environment: environment, + strategyName: payload.name, + ...(payload.id ? { id: payload.id } : {}), + }; + }; + + public async getStrategiesUsedInActiveChangeRequests( + segmentId: number, + ): Promise { + const query = this.db.raw( + `SELECT events.*, cr.project, cr.environment + FROM change_request_events events + JOIN change_requests cr ON events.change_request_id = cr.id + WHERE cr.state NOT IN ('Applied', 'Cancelled', 'Rejected') + AND events.action IN ('updateStrategy', 'addStrategy');`, + ); + + const queryResult = await query; + const strategies = queryResult.rows + .filter((row) => row.payload?.segments?.includes(segmentId)) + .map(this.mapRow); + + return strategies; + } } diff --git a/src/lib/openapi/spec/admin-strategies-schema.ts b/src/lib/openapi/spec/admin-strategies-schema.ts index e6124d83e1..53cc4b74b2 100644 --- a/src/lib/openapi/spec/admin-strategies-schema.ts +++ b/src/lib/openapi/spec/admin-strategies-schema.ts @@ -26,7 +26,8 @@ export const segmentStrategiesSchema = { }, featureName: { type: 'string', - description: 'The ID of the strategy', + description: + 'The name of the feature flag that this strategy belongs to.', example: 'new-signup-flow', }, projectId: { diff --git a/src/test/e2e/api/admin/segment.e2e.test.ts b/src/test/e2e/api/admin/segment.e2e.test.ts index 90dccbfa0a..e4e6cf2236 100644 --- a/src/test/e2e/api/admin/segment.e2e.test.ts +++ b/src/test/e2e/api/admin/segment.e2e.test.ts @@ -249,7 +249,7 @@ test('should not delete segments used by strategies in CRs', async () => { await db.rawDatabase.table('change_requests').insert({ id: CR_ID, environment: 'default', - state: 'In Review', + state: 'In review', project: 'default', created_by: user.id, created_at: '2023-01-01 00:00:00', From ae375703d248b347756b35ef51ae0538ae2a2bdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 21 Nov 2023 10:06:38 +0000 Subject: [PATCH 07/11] fix: scheduler job runtime control (#5363) ## PR Description https://linear.app/unleash/issue/2-1645/address-post-mortem-action-point-all-flags-should-be-runtime Refactor with the goal of ensuring that flags are runtime controllable, mostly focused on the current scheduler logic. This includes the following changes: - Moves scheduler into its own "scheduler" feature folder - Reverts dependency: SchedulerService takes in the MaintenanceService, not the other way around - Scheduler now evaluates maintenance mode at runtime instead of relying only on its mode state (active / paused) - Favors flag checks to happen inside the scheduled methods, instead of controlling whether the method is scheduled at all (favor runtime over startup) - Moves "account last seen update" to scheduler - Updates tests accordingly - Boyscouting Here's a manual test showing this behavior, where my local instance was controlled by a remote instance. Whenever I toggle `maintenanceMode` through a flag remotely, my scheduled functions stop running: https://github.com/Unleash/unleash/assets/14320932/ae0a7fa9-5165-4c0b-9b0b-53b9fb20de72 Had a look through all of our current flags and it *seems to me* that they are all used in a runtime controllable way, but would still feel more comfortable if this was double checked, since it can be complex to ensure this. The only exception to this was `migrationLock`, which I believe is OK, since the migration only happens at the start anyways. ## Discussion / Questions ~~Scheduler `mode` (active / paused) is currently not *really* being used, along with its respective methods, except in tests. I think this could be a potential footgun. Should we remove it in favor of only controlling the scheduler state through maintenance mode?~~ Addressed in https://github.com/Unleash/unleash/commit/7c52e3f63826fe9a7c572b959252fc042a0a0cf2 ~~The config property `disableScheduler` is still a startup configuration, but perhaps that makes sense to leave as is?~~ [Answered](https://github.com/Unleash/unleash/pull/5363#issuecomment-1819005445) by @FredrikOseberg, leaving as is. Are there any other tests we should add? Is there anything I missed? Identified some `setInterval` and `setTimeout` that may make sense to leave as is instead of moving over to the scheduler service: - ~~`src/lib/metrics` - This is currently considered a `MetricsMonitor`. Should this be refactored to a service instead and adapt these setIntervals to use the scheduler instead? Is there anything special with this we need to take into account? @chriswk @ivarconr~~ [Answered](https://github.com/Unleash/unleash/pull/5363#issuecomment-1820501511) by @ivarconr, leaving as is. - ~~`src/lib/proxy/proxy-repository.ts` - This seems to have a complex and specific logic currently. Perhaps we should leave it alone for now? @FredrikOseberg~~ [Answered](https://github.com/Unleash/unleash/pull/5363#issuecomment-1819005445) by @FredrikOseberg, leaving as is. - `src/lib/services/user-service.ts` - This one also seems to be a bit more specific, where we generate new timeouts for each receiver id. Might not belong in the scheduler service. @Tymek --- .../features/scheduler/schedule-services.ts | 153 +++++++++++ .../scheduler/scheduler-service.test.ts | 239 ++++++++++++++++++ .../scheduler}/scheduler-service.ts | 36 ++- .../middleware/cors-origin-middleware.test.ts | 8 +- src/lib/server-impl.ts | 5 +- src/lib/services/account-service.ts | 13 - .../last-seen/last-seen-service.ts | 14 +- src/lib/services/index.ts | 167 +----------- src/lib/services/maintenance-service.test.ts | 67 ++--- src/lib/services/maintenance-service.ts | 29 +-- src/lib/services/scheduler-service.test.ts | 148 ----------- src/lib/types/services.ts | 2 +- 12 files changed, 468 insertions(+), 413 deletions(-) create mode 100644 src/lib/features/scheduler/schedule-services.ts create mode 100644 src/lib/features/scheduler/scheduler-service.test.ts rename src/lib/{services => features/scheduler}/scheduler-service.ts (60%) delete mode 100644 src/lib/services/scheduler-service.test.ts diff --git a/src/lib/features/scheduler/schedule-services.ts b/src/lib/features/scheduler/schedule-services.ts new file mode 100644 index 0000000000..0e276270c9 --- /dev/null +++ b/src/lib/features/scheduler/schedule-services.ts @@ -0,0 +1,153 @@ +import { + hoursToMilliseconds, + minutesToMilliseconds, + secondsToMilliseconds, +} from 'date-fns'; +import { IUnleashServices } from '../../server-impl'; + +/** + * Schedules service methods. + * + * In order to promote runtime control, you should **not use** a flagResolver inside this method. Instead, implement your flag usage inside the scheduled methods themselves. + * @param services + */ +export const scheduleServices = async ( + services: IUnleashServices, +): Promise => { + const { + accountService, + schedulerService, + apiTokenService, + instanceStatsService, + clientInstanceService, + projectService, + projectHealthService, + configurationRevisionService, + eventAnnouncerService, + featureToggleService, + versionService, + lastSeenService, + proxyService, + clientMetricsServiceV2, + } = services; + + schedulerService.schedule( + lastSeenService.cleanLastSeen.bind(lastSeenService), + hoursToMilliseconds(1), + 'cleanLastSeen', + ); + + schedulerService.schedule( + lastSeenService.store.bind(lastSeenService), + secondsToMilliseconds(30), + 'storeLastSeen', + ); + + schedulerService.schedule( + apiTokenService.fetchActiveTokens.bind(apiTokenService), + minutesToMilliseconds(1), + 'fetchActiveTokens', + ); + + schedulerService.schedule( + apiTokenService.updateLastSeen.bind(apiTokenService), + minutesToMilliseconds(3), + 'updateLastSeen', + ); + + schedulerService.schedule( + instanceStatsService.refreshStatsSnapshot.bind(instanceStatsService), + minutesToMilliseconds(5), + 'refreshStatsSnapshot', + ); + + schedulerService.schedule( + clientInstanceService.removeInstancesOlderThanTwoDays.bind( + clientInstanceService, + ), + hoursToMilliseconds(24), + '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), + 'statusJob', + ); + + schedulerService.schedule( + projectHealthService.setHealthRating.bind(projectHealthService), + hoursToMilliseconds(1), + 'setHealthRating', + ); + + schedulerService.schedule( + configurationRevisionService.updateMaxRevisionId.bind( + configurationRevisionService, + ), + secondsToMilliseconds(1), + 'updateMaxRevisionId', + ); + + schedulerService.schedule( + eventAnnouncerService.publishUnannouncedEvents.bind( + eventAnnouncerService, + ), + secondsToMilliseconds(1), + 'publishUnannouncedEvents', + ); + + schedulerService.schedule( + featureToggleService.updatePotentiallyStaleFeatures.bind( + featureToggleService, + ), + minutesToMilliseconds(1), + 'updatePotentiallyStaleFeatures', + ); + + schedulerService.schedule( + versionService.checkLatestVersion.bind(versionService), + hoursToMilliseconds(48), + 'checkLatestVersion', + ); + + schedulerService.schedule( + proxyService.fetchFrontendSettings.bind(proxyService), + minutesToMilliseconds(2), + 'fetchFrontendSettings', + ); + + schedulerService.schedule( + () => { + clientMetricsServiceV2.bulkAdd().catch(console.error); + }, + secondsToMilliseconds(5), + 'bulkAddMetrics', + ); + + schedulerService.schedule( + () => { + clientMetricsServiceV2.clearMetrics(48).catch(console.error); + }, + hoursToMilliseconds(12), + 'clearMetrics', + ); + + schedulerService.schedule( + accountService.updateLastSeen.bind(accountService), + minutesToMilliseconds(3), + 'updateAccountLastSeen', + ); +}; diff --git a/src/lib/features/scheduler/scheduler-service.test.ts b/src/lib/features/scheduler/scheduler-service.test.ts new file mode 100644 index 0000000000..55f0fdd7f7 --- /dev/null +++ b/src/lib/features/scheduler/scheduler-service.test.ts @@ -0,0 +1,239 @@ +import { SchedulerService } from './scheduler-service'; +import { LogProvider } from '../../logger'; +import MaintenanceService from '../../services/maintenance-service'; +import { createTestConfig } from '../../../test/config/test-config'; +import SettingService from '../../services/setting-service'; +import FakeSettingStore from '../../../test/fixtures/fake-setting-store'; +import EventService from '../../services/event-service'; + +function ms(timeMs) { + return new Promise((resolve) => setTimeout(resolve, timeMs)); +} + +const getLogger = () => { + const records: any[] = []; + const logger: LogProvider = () => ({ + error(...args: any[]) { + records.push(args); + }, + debug() {}, + info() {}, + warn() {}, + fatal() {}, + }); + const getRecords = () => records; + + return { logger, getRecords }; +}; + +const toggleMaintenanceMode = async ( + maintenanceService: MaintenanceService, + enabled: boolean, +) => { + await maintenanceService.toggleMaintenanceMode( + { enabled }, + 'irrelevant user', + ); +}; + +test('Schedules job immediately', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + + const job = jest.fn(); + + await schedulerService.schedule(job, 10, 'test-id'); + + expect(job).toBeCalledTimes(1); + schedulerService.stop(); +}); + +test('Does not schedule job immediately when paused', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + + const job = jest.fn(); + + await toggleMaintenanceMode(maintenanceService, true); + await schedulerService.schedule(job, 10, 'test-id-2'); + + expect(job).toBeCalledTimes(0); + schedulerService.stop(); +}); + +test('Can schedule a single regular job', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + + const job = jest.fn(); + + await schedulerService.schedule(job, 50, 'test-id-3'); + await ms(75); + + expect(job).toBeCalledTimes(2); + schedulerService.stop(); +}); + +test('Scheduled job ignored in a paused mode', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + + const job = jest.fn(); + + await toggleMaintenanceMode(maintenanceService, true); + await schedulerService.schedule(job, 50, 'test-id-4'); + await ms(75); + + expect(job).toBeCalledTimes(0); + schedulerService.stop(); +}); + +test('Can resume paused job', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + + const job = jest.fn(); + + await toggleMaintenanceMode(maintenanceService, true); + await schedulerService.schedule(job, 50, 'test-id-5'); + await toggleMaintenanceMode(maintenanceService, false); + await ms(75); + + expect(job).toBeCalledTimes(1); + schedulerService.stop(); +}); + +test('Can schedule multiple jobs at the same interval', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + + const job = jest.fn(); + const anotherJob = jest.fn(); + + await schedulerService.schedule(job, 50, 'test-id-6'); + await schedulerService.schedule(anotherJob, 50, 'test-id-7'); + await ms(75); + + expect(job).toBeCalledTimes(2); + expect(anotherJob).toBeCalledTimes(2); + schedulerService.stop(); +}); + +test('Can schedule multiple jobs at the different intervals', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + const job = jest.fn(); + const anotherJob = jest.fn(); + + await schedulerService.schedule(job, 100, 'test-id-8'); + await schedulerService.schedule(anotherJob, 200, 'test-id-9'); + await ms(250); + + expect(job).toBeCalledTimes(3); + expect(anotherJob).toBeCalledTimes(2); + schedulerService.stop(); +}); + +test('Can handle crash of a async job', async () => { + const { logger, getRecords } = getLogger(); + const config = { ...createTestConfig(), logger }; + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService(logger, maintenanceService); + + const job = async () => { + await Promise.reject('async reason'); + }; + + await schedulerService.schedule(job, 50, 'test-id-10'); + await ms(75); + + schedulerService.stop(); + expect(getRecords()).toEqual([ + ['scheduled job failed | id: test-id-10 | async reason'], + ['scheduled job failed | id: test-id-10 | async reason'], + ]); +}); + +test('Can handle crash of a sync job', async () => { + const { logger, getRecords } = getLogger(); + const config = { ...createTestConfig(), logger }; + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService(logger, maintenanceService); + + const job = () => { + throw new Error('sync reason'); + }; + + await schedulerService.schedule(job, 50, 'test-id-11'); + await ms(75); + + schedulerService.stop(); + expect(getRecords()).toEqual([ + ['scheduled job failed | id: test-id-11 | Error: sync reason'], + ['scheduled job failed | id: test-id-11 | Error: sync reason'], + ]); +}); diff --git a/src/lib/services/scheduler-service.ts b/src/lib/features/scheduler/scheduler-service.ts similarity index 60% rename from src/lib/services/scheduler-service.ts rename to src/lib/features/scheduler/scheduler-service.ts index c2f8ae0478..20f4a94a3c 100644 --- a/src/lib/services/scheduler-service.ts +++ b/src/lib/features/scheduler/scheduler-service.ts @@ -1,17 +1,19 @@ -import { Logger, LogProvider } from '../logger'; - -export type SchedulerMode = 'active' | 'paused'; +import { Logger, LogProvider } from '../../logger'; +import MaintenanceService from '../../services/maintenance-service'; export class SchedulerService { private intervalIds: NodeJS.Timer[] = []; - private mode: SchedulerMode; - private logger: Logger; - constructor(getLogger: LogProvider) { + private maintenanceService: MaintenanceService; + + constructor( + getLogger: LogProvider, + maintenanceService: MaintenanceService, + ) { this.logger = getLogger('/services/scheduler-service.ts'); - this.mode = 'active'; + this.maintenanceService = maintenanceService; } async schedule( @@ -22,7 +24,9 @@ export class SchedulerService { this.intervalIds.push( setInterval(async () => { try { - if (this.mode === 'active') { + const maintenanceMode = + await this.maintenanceService.isMaintenanceMode(); + if (!maintenanceMode) { await scheduledFunction(); } } catch (e) { @@ -33,7 +37,9 @@ export class SchedulerService { }, timeMs).unref(), ); try { - if (this.mode === 'active') { + const maintenanceMode = + await this.maintenanceService.isMaintenanceMode(); + if (!maintenanceMode) { await scheduledFunction(); } } catch (e) { @@ -44,16 +50,4 @@ export class SchedulerService { stop(): void { this.intervalIds.forEach(clearInterval); } - - pause(): void { - this.mode = 'paused'; - } - - resume(): void { - this.mode = 'active'; - } - - getMode(): SchedulerMode { - return this.mode; - } } diff --git a/src/lib/middleware/cors-origin-middleware.test.ts b/src/lib/middleware/cors-origin-middleware.test.ts index 2e81ebdbda..abd44622da 100644 --- a/src/lib/middleware/cors-origin-middleware.test.ts +++ b/src/lib/middleware/cors-origin-middleware.test.ts @@ -4,15 +4,9 @@ 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, - SchedulerService, - SettingService, -} from '../../lib/services'; +import { EventService, ProxyService, SettingService } from '../../lib/services'; import { ISettingStore } from '../../lib/types'; import { frontendSettingsKey } from '../../lib/types/settings/frontend-settings'; -import { minutesToMilliseconds } from 'date-fns'; import FakeFeatureTagStore from '../../test/fixtures/fake-feature-tag-store'; const createSettingService = ( diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index dd3fe82ec9..93d99c2750 100644 --- a/src/lib/server-impl.ts +++ b/src/lib/server-impl.ts @@ -5,7 +5,7 @@ import { migrateDb } from '../migrator'; import getApp from './app'; import { createMetricsMonitor } from './metrics'; import { createStores } from './db'; -import { createServices, scheduleServices } from './services'; +import { createServices } from './services'; import { createConfig } from './create-config'; import registerGracefulShutdown from './util/graceful-shutdown'; import { createDb } from './db/db-pool'; @@ -33,6 +33,7 @@ import * as permissions from './types/permissions'; import * as eventType from './types/events'; import { Db } from './db/db'; import { defaultLockKey, defaultTimeout, withDbLock } from './util/db-lock'; +import { scheduleServices } from './features/scheduler/schedule-services'; async function createApp( config: IUnleashConfig, @@ -45,7 +46,7 @@ async function createApp( const stores = createStores(config, db); const services = createServices(stores, config, db); if (!config.disableScheduler) { - await scheduleServices(services, config.flagResolver); + await scheduleServices(services); } const metricsMonitor = createMetricsMonitor(); diff --git a/src/lib/services/account-service.ts b/src/lib/services/account-service.ts index 2f13b3403b..78aecaf48c 100644 --- a/src/lib/services/account-service.ts +++ b/src/lib/services/account-service.ts @@ -18,8 +18,6 @@ export class AccountService { private accessService: AccessService; - private seenTimer: NodeJS.Timeout; - private lastSeenSecrets: Set = new Set(); constructor( @@ -32,7 +30,6 @@ export class AccountService { this.logger = getLogger('service/account-service.ts'); this.store = stores.accountStore; this.accessService = services.accessService; - this.updateLastSeen(); } async getAll(): Promise { @@ -63,19 +60,9 @@ export class AccountService { this.lastSeenSecrets = new Set(); await this.store.markSeenAt(toStore); } - - this.seenTimer = setTimeout( - async () => this.updateLastSeen(), - minutesToMilliseconds(3), - ).unref(); } addPATSeen(secret: string): void { this.lastSeenSecrets.add(secret); } - - destroy(): void { - clearTimeout(this.seenTimer); - this.seenTimer = null; - } } 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 4bb6d5a282..c211c4dbf9 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 @@ -1,9 +1,12 @@ -import { secondsToMilliseconds } from 'date-fns'; import { Logger } from '../../../logger'; import { IUnleashConfig } from '../../../server-impl'; import { IClientMetricsEnv } from '../../../types/stores/client-metrics-store-v2'; import { ILastSeenStore } from './types/last-seen-store-type'; -import { IFeatureToggleStore, IUnleashStores } from '../../../../lib/types'; +import { + IFeatureToggleStore, + IFlagResolver, + IUnleashStores, +} from '../../../../lib/types'; export type LastSeenInput = { featureName: string; @@ -21,6 +24,8 @@ export class LastSeenService { private config: IUnleashConfig; + private flagResolver: IFlagResolver; + constructor( { featureToggleStore, @@ -33,6 +38,7 @@ export class LastSeenService { this.logger = config.getLogger( '/services/client-metrics/last-seen-service.ts', ); + this.flagResolver = config.flagResolver; this.config = config; } @@ -75,6 +81,8 @@ export class LastSeenService { } async cleanLastSeen() { - await this.lastSeenStore.cleanLastSeen(); + if (this.flagResolver.isEnabled('useLastSeenRefactor')) { + await this.lastSeenStore.cleanLastSeen(); + } } } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 36c2cbb752..db7ef63b27 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -1,9 +1,4 @@ -import { - IUnleashConfig, - IUnleashStores, - IUnleashServices, - IFlagResolver, -} from '../types'; +import { IUnleashConfig, IUnleashStores, IUnleashServices } from '../types'; import FeatureTypeService from './feature-type-service'; import EventService from './event-service'; import HealthService from './health-service'; @@ -44,13 +39,8 @@ import { LastSeenService } from './client-metrics/last-seen/last-seen-service'; import { InstanceStatsService } from '../features/instance-stats/instance-stats-service'; import { FavoritesService } from './favorites-service'; import MaintenanceService from './maintenance-service'; -import { - hoursToMilliseconds, - minutesToMilliseconds, - secondsToMilliseconds, -} from 'date-fns'; import { AccountService } from './account-service'; -import { SchedulerService } from './scheduler-service'; +import { SchedulerService } from '../features/scheduler/scheduler-service'; import { Knex } from 'knex'; import { createExportImportTogglesService, @@ -109,149 +99,6 @@ import { } from '../features/feature-search/createFeatureSearchService'; import { FeatureSearchService } from '../features/feature-search/feature-search-service'; -// TODO: will be moved to scheduler feature directory -export const scheduleServices = async ( - services: IUnleashServices, - flagResolver: IFlagResolver, -): Promise => { - const { - schedulerService, - apiTokenService, - instanceStatsService, - clientInstanceService, - projectService, - projectHealthService, - configurationRevisionService, - maintenanceService, - eventAnnouncerService, - featureToggleService, - versionService, - lastSeenService, - proxyService, - clientMetricsServiceV2, - } = services; - - if (await maintenanceService.isMaintenanceMode()) { - schedulerService.pause(); - } - - if (flagResolver.isEnabled('useLastSeenRefactor')) { - schedulerService.schedule( - lastSeenService.cleanLastSeen.bind(lastSeenService), - hoursToMilliseconds(1), - 'cleanLastSeen', - ); - } - - schedulerService.schedule( - lastSeenService.store.bind(lastSeenService), - secondsToMilliseconds(30), - 'storeLastSeen', - ); - - schedulerService.schedule( - apiTokenService.fetchActiveTokens.bind(apiTokenService), - minutesToMilliseconds(1), - 'fetchActiveTokens', - ); - - schedulerService.schedule( - apiTokenService.updateLastSeen.bind(apiTokenService), - minutesToMilliseconds(3), - 'updateLastSeen', - ); - - schedulerService.schedule( - instanceStatsService.refreshStatsSnapshot.bind(instanceStatsService), - minutesToMilliseconds(5), - 'refreshStatsSnapshot', - ); - - schedulerService.schedule( - clientInstanceService.removeInstancesOlderThanTwoDays.bind( - clientInstanceService, - ), - hoursToMilliseconds(24), - '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), - 'statusJob', - ); - - schedulerService.schedule( - projectHealthService.setHealthRating.bind(projectHealthService), - hoursToMilliseconds(1), - 'setHealthRating', - ); - - schedulerService.schedule( - configurationRevisionService.updateMaxRevisionId.bind( - configurationRevisionService, - ), - secondsToMilliseconds(1), - 'updateMaxRevisionId', - ); - - schedulerService.schedule( - eventAnnouncerService.publishUnannouncedEvents.bind( - eventAnnouncerService, - ), - secondsToMilliseconds(1), - 'publishUnannouncedEvents', - ); - - schedulerService.schedule( - featureToggleService.updatePotentiallyStaleFeatures.bind( - featureToggleService, - ), - minutesToMilliseconds(1), - 'updatePotentiallyStaleFeatures', - ); - - schedulerService.schedule( - versionService.checkLatestVersion.bind(versionService), - hoursToMilliseconds(48), - 'checkLatestVersion', - ); - - schedulerService.schedule( - proxyService.fetchFrontendSettings.bind(proxyService), - 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 = ( stores: IUnleashStores, config: IUnleashConfig, @@ -438,13 +285,11 @@ export const createServices = ( db ? createGetProductionChanges(db) : createFakeGetProductionChanges(), ); - const schedulerService = new SchedulerService(config.getLogger); + const maintenanceService = new MaintenanceService(config, settingService); - const maintenanceService = new MaintenanceService( - stores, - config, - settingService, - schedulerService, + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, ); const eventAnnouncerService = new EventAnnouncerService(stores, config); diff --git a/src/lib/services/maintenance-service.test.ts b/src/lib/services/maintenance-service.test.ts index 4eab1c0adc..fc4be7e720 100644 --- a/src/lib/services/maintenance-service.test.ts +++ b/src/lib/services/maintenance-service.test.ts @@ -1,17 +1,40 @@ -import { SchedulerService } from './scheduler-service'; +import { SchedulerService } from '../features/scheduler/scheduler-service'; import MaintenanceService from './maintenance-service'; -import { IUnleashStores } from '../types'; import SettingService from './setting-service'; import { createTestConfig } from '../../test/config/test-config'; +import FakeSettingStore from '../../test/fixtures/fake-setting-store'; +import EventService from './event-service'; -test('Maintenance on should pause scheduler', async () => { +test('Scheduler should run scheduled functions if maintenance mode is off', async () => { const config = createTestConfig(); - const schedulerService = new SchedulerService(config.getLogger); - const maintenanceService = new MaintenanceService( - {} as IUnleashStores, - config, - { insert() {} } as unknown as SettingService, - schedulerService, + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, + ); + + const job = jest.fn(); + + await schedulerService.schedule(job, 10, 'test-id'); + + expect(job).toBeCalledTimes(1); + schedulerService.stop(); +}); + +test('Scheduler should not run scheduled functions if maintenance mode is on', async () => { + const config = createTestConfig(); + const settingStore = new FakeSettingStore(); + const settingService = new SettingService({ settingStore }, config, { + storeEvent() {}, + } as unknown as EventService); + const maintenanceService = new MaintenanceService(config, settingService); + const schedulerService = new SchedulerService( + config.getLogger, + maintenanceService, ); await maintenanceService.toggleMaintenanceMode( @@ -19,26 +42,10 @@ test('Maintenance on should pause scheduler', async () => { 'irrelevant user', ); - expect(schedulerService.getMode()).toBe('paused'); - schedulerService.stop(); -}); - -test('Maintenance off should resume scheduler', async () => { - const config = createTestConfig({ disableScheduler: false }); - const schedulerService = new SchedulerService(config.getLogger); - schedulerService.pause(); - const maintenanceService = new MaintenanceService( - {} as IUnleashStores, - config, - { insert() {} } as unknown as SettingService, - schedulerService, - ); - - await maintenanceService.toggleMaintenanceMode( - { enabled: false }, - 'irrelevant user', - ); - - expect(schedulerService.getMode()).toBe('active'); + const job = jest.fn(); + + await schedulerService.schedule(job, 10, 'test-id'); + + expect(job).toBeCalledTimes(0); schedulerService.stop(); }); diff --git a/src/lib/services/maintenance-service.ts b/src/lib/services/maintenance-service.ts index 21be7f1249..78c2ad7d1e 100644 --- a/src/lib/services/maintenance-service.ts +++ b/src/lib/services/maintenance-service.ts @@ -1,40 +1,20 @@ -import { IUnleashConfig, IUnleashStores } from '../types'; +import { IUnleashConfig } from '../types'; import { Logger } from '../logger'; -import { IPatStore } from '../types/stores/pat-store'; -import { IEventStore } from '../types/stores/event-store'; import SettingService from './setting-service'; import { maintenanceSettingsKey } from '../types/settings/maintenance-settings'; import { MaintenanceSchema } from '../openapi/spec/maintenance-schema'; -import { SchedulerService } from './scheduler-service'; export default class MaintenanceService { private config: IUnleashConfig; private logger: Logger; - private patStore: IPatStore; - - private eventStore: IEventStore; - private settingService: SettingService; - private schedulerService: SchedulerService; - - constructor( - { - patStore, - eventStore, - }: Pick, - config: IUnleashConfig, - settingService: SettingService, - schedulerService: SchedulerService, - ) { + constructor(config: IUnleashConfig, settingService: SettingService) { this.config = config; this.logger = config.getLogger('services/pat-service.ts'); - this.patStore = patStore; - this.eventStore = eventStore; this.settingService = settingService; - this.schedulerService = schedulerService; } async isMaintenanceMode(): Promise { @@ -56,11 +36,6 @@ export default class MaintenanceService { setting: MaintenanceSchema, user: string, ): Promise { - if (setting.enabled) { - this.schedulerService.pause(); - } else if (!this.config.disableScheduler) { - this.schedulerService.resume(); - } return this.settingService.insert( maintenanceSettingsKey, setting, diff --git a/src/lib/services/scheduler-service.test.ts b/src/lib/services/scheduler-service.test.ts deleted file mode 100644 index 0a03a7b0f8..0000000000 --- a/src/lib/services/scheduler-service.test.ts +++ /dev/null @@ -1,148 +0,0 @@ -import { SchedulerService } from './scheduler-service'; -import { LogProvider } from '../logger'; - -function ms(timeMs) { - return new Promise((resolve) => setTimeout(resolve, timeMs)); -} - -const getLogger = () => { - const records: any[] = []; - const logger: LogProvider = () => ({ - error(...args: any[]) { - records.push(args); - }, - debug() {}, - info() {}, - warn() {}, - fatal() {}, - }); - const getRecords = () => records; - - return { logger, getRecords }; -}; - -test('Schedules job immediately', async () => { - const { logger } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = jest.fn(); - - schedulerService.schedule(job, 10, 'test-id'); - - expect(job).toBeCalledTimes(1); - schedulerService.stop(); -}); - -test('Does not schedule job immediately when paused', async () => { - const { logger } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = jest.fn(); - - schedulerService.pause(); - schedulerService.schedule(job, 10, 'test-id-2'); - - expect(job).toBeCalledTimes(0); - schedulerService.stop(); -}); - -test('Can schedule a single regular job', async () => { - const { logger } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = jest.fn(); - - schedulerService.schedule(job, 50, 'test-id-3'); - await ms(75); - - expect(job).toBeCalledTimes(2); - schedulerService.stop(); -}); - -test('Scheduled job ignored in a paused mode', async () => { - const { logger } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = jest.fn(); - - schedulerService.pause(); - schedulerService.schedule(job, 50, 'test-id-4'); - await ms(75); - - expect(job).toBeCalledTimes(0); - schedulerService.stop(); -}); - -test('Can resume paused job', async () => { - const { logger } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = jest.fn(); - - schedulerService.pause(); - schedulerService.schedule(job, 50, 'test-id-5'); - schedulerService.resume(); - await ms(75); - - expect(job).toBeCalledTimes(1); - schedulerService.stop(); -}); - -test('Can schedule multiple jobs at the same interval', async () => { - const { logger } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = jest.fn(); - const anotherJob = jest.fn(); - - schedulerService.schedule(job, 50, 'test-id-6'); - schedulerService.schedule(anotherJob, 50, 'test-id-7'); - await ms(75); - - expect(job).toBeCalledTimes(2); - expect(anotherJob).toBeCalledTimes(2); - schedulerService.stop(); -}); - -test('Can schedule multiple jobs at the different intervals', async () => { - const { logger } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = jest.fn(); - const anotherJob = jest.fn(); - - schedulerService.schedule(job, 100, 'test-id-8'); - schedulerService.schedule(anotherJob, 200, 'test-id-9'); - await ms(250); - - expect(job).toBeCalledTimes(3); - expect(anotherJob).toBeCalledTimes(2); - schedulerService.stop(); -}); - -test('Can handle crash of a async job', async () => { - const { logger, getRecords } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = async () => { - await Promise.reject('async reason'); - }; - - schedulerService.schedule(job, 50, 'test-id-10'); - await ms(75); - - schedulerService.stop(); - expect(getRecords()).toEqual([ - ['scheduled job failed | id: test-id-10 | async reason'], - ['scheduled job failed | id: test-id-10 | async reason'], - ]); -}); - -test('Can handle crash of a sync job', async () => { - const { logger, getRecords } = getLogger(); - const schedulerService = new SchedulerService(logger); - const job = () => { - throw new Error('sync reason'); - }; - - schedulerService.schedule(job, 50, 'test-id-11'); - await ms(75); - - schedulerService.stop(); - expect(getRecords()).toEqual([ - ['scheduled job failed | id: test-id-11 | Error: sync reason'], - ['scheduled job failed | id: test-id-11 | Error: sync reason'], - ]); -}); diff --git a/src/lib/types/services.ts b/src/lib/types/services.ts index a5c15769c7..51ec2bf626 100644 --- a/src/lib/types/services.ts +++ b/src/lib/types/services.ts @@ -37,7 +37,7 @@ import { InstanceStatsService } from '../features/instance-stats/instance-stats- import { FavoritesService } from '../services/favorites-service'; import MaintenanceService from '../services/maintenance-service'; import { AccountService } from '../services/account-service'; -import { SchedulerService } from '../services/scheduler-service'; +import { SchedulerService } from '../features/scheduler/scheduler-service'; import { Knex } from 'knex'; import { IExportService, From d5049e61979c422e79cf4271477ebda9dea59aad Mon Sep 17 00:00:00 2001 From: Tymoteusz Czech <2625371+Tymek@users.noreply.github.com> Date: Tue, 21 Nov 2023 11:25:31 +0100 Subject: [PATCH 08/11] feat: useTableState hook (#5362) Simplified logic for handling interaction between URL (query), table state and localstorage. --- frontend/src/hooks/useTableState.test.ts | 291 +++++++++++++++++++++++ frontend/src/hooks/useTableState.ts | 82 +++++++ 2 files changed, 373 insertions(+) create mode 100644 frontend/src/hooks/useTableState.test.ts create mode 100644 frontend/src/hooks/useTableState.ts diff --git a/frontend/src/hooks/useTableState.test.ts b/frontend/src/hooks/useTableState.test.ts new file mode 100644 index 0000000000..ce2f716d4f --- /dev/null +++ b/frontend/src/hooks/useTableState.test.ts @@ -0,0 +1,291 @@ +import { vi, type Mock } from 'vitest'; +import { renderHook } from '@testing-library/react-hooks'; +import { useTableState } from './useTableState'; +import { createLocalStorage } from 'utils/createLocalStorage'; +import { useSearchParams } from 'react-router-dom'; +import { act } from 'react-test-renderer'; + +vi.mock('react-router-dom', () => ({ + useSearchParams: vi.fn(() => [new URLSearchParams(), vi.fn()]), +})); +vi.mock('../utils/createLocalStorage', () => ({ + createLocalStorage: vi.fn(() => ({ + value: {}, + setValue: vi.fn(), + })), +})); + +const mockStorage = createLocalStorage as Mock; +const mockQuery = useSearchParams as Mock; + +describe('useTableState', () => { + beforeEach(() => { + mockStorage.mockRestore(); + mockQuery.mockRestore(); + }); + + it('should return default params', () => { + const { result } = renderHook(() => + useTableState<{ + page: '0'; + pageSize: '10'; + }>({ page: '0', pageSize: '10' }, 'test', [], []), + ); + expect(result.current[0]).toEqual({ page: '0', pageSize: '10' }); + }); + + it('should return params from local storage', () => { + mockStorage.mockReturnValue({ + value: { pageSize: 25 }, + setValue: vi.fn(), + }); + + const { result } = renderHook(() => + useTableState({ pageSize: '10' }, 'test', [], []), + ); + + expect(result.current[0]).toEqual({ pageSize: 25 }); + }); + + it('should return params from url', () => { + mockQuery.mockReturnValue([new URLSearchParams('page=1'), vi.fn()]); + + const { result } = renderHook(() => + useTableState({ page: '0' }, 'test', [], []), + ); + + expect(result.current[0]).toEqual({ page: '1' }); + }); + + it('should use params from url over local storage', () => { + mockQuery.mockReturnValue([ + new URLSearchParams('page=2&pageSize=25'), + vi.fn(), + ]); + mockStorage.mockReturnValue({ + value: { pageSize: '10', sortOrder: 'desc' }, + setValue: vi.fn(), + }); + + const { result } = renderHook(() => + useTableState({ page: '1', pageSize: '5' }, 'test', [], []), + ); + + expect(result.current[0]).toEqual({ + page: '2', + pageSize: '25', + }); + }); + + it('sets local state', () => { + const { result } = renderHook(() => + useTableState({ page: '1' }, 'test', [], []), + ); + const setParams = result.current[1]; + + act(() => { + setParams({ page: '2' }); + }); + + expect(result.current[0]).toEqual({ page: '2' }); + }); + + it('keeps previous state', () => { + const { result } = renderHook(() => + useTableState({ page: '1', pageSize: '10' }, 'test', [], []), + ); + const setParams = result.current[1]; + + act(() => { + setParams({ page: '2' }); + }); + + expect(result.current[0]).toEqual({ page: '2', pageSize: '10' }); + }); + + it('removes params from previous state', () => { + const { result } = renderHook(() => + useTableState({ page: '1', pageSize: '10' }, 'test', [], []), + ); + const setParams = result.current[1]; + + act(() => { + setParams({ pageSize: undefined }); + }); + + expect(result.current[0]).toEqual({ page: '1' }); + + // ensure that there are no keys with undefined values + expect(Object.keys(result.current[0])).toHaveLength(1); + }); + + it('removes params from url', () => { + const querySetter = vi.fn(); + mockQuery.mockReturnValue([new URLSearchParams('page=2'), querySetter]); + + const { result } = renderHook(() => + useTableState( + { page: '1', pageSize: '10' }, + 'test', + ['page', 'pageSize'], + [], + ), + ); + const setParams = result.current[1]; + + expect(result.current[0]).toEqual({ page: '2', pageSize: '10' }); + + act(() => { + setParams({ page: '10', pageSize: undefined }); + }); + + expect(result.current[0]).toEqual({ page: '10' }); + + expect(querySetter).toHaveBeenCalledWith({ + page: '10', + }); + }); + + it('removes params from local storage', () => { + const storageSetter = vi.fn(); + mockStorage.mockReturnValue({ + value: { sortBy: 'type' }, + setValue: storageSetter, + }); + + const { result } = renderHook(() => + useTableState( + { sortBy: 'createdAt', pageSize: '10' }, + 'test', + [], + ['sortBy', 'pageSize'], + ), + ); + + expect(result.current[0]).toEqual({ sortBy: 'type', pageSize: '10' }); + + act(() => { + result.current[1]({ pageSize: undefined }); + }); + + expect(result.current[0]).toEqual({ sortBy: 'type' }); + + expect(storageSetter).toHaveBeenCalledWith({ + sortBy: 'type', + }); + }); + + test('saves default parameters if not explicitly provided', (key) => { + const querySetter = vi.fn(); + const storageSetter = vi.fn(); + mockQuery.mockReturnValue([new URLSearchParams(), querySetter]); + mockStorage.mockReturnValue({ + value: {}, + setValue: storageSetter, + }); + + const { result } = renderHook(() => useTableState({}, 'test')); + + act(() => { + result.current[1]({ + unspecified: 'test', + page: '2', + pageSize: '10', + search: 'test', + sortBy: 'type', + sortOrder: 'favorites', + favorites: 'false', + columns: ['test', 'id'], + }); + }); + + expect(storageSetter).toHaveBeenCalledTimes(1); + expect(storageSetter).toHaveBeenCalledWith({ + pageSize: '10', + search: 'test', + sortBy: 'type', + sortOrder: 'favorites', + favorites: 'false', + columns: ['test', 'id'], + }); + expect(querySetter).toHaveBeenCalledTimes(1); + expect(querySetter).toHaveBeenCalledWith({ + page: '2', + pageSize: '10', + search: 'test', + sortBy: 'type', + sortOrder: 'favorites', + favorites: 'false', + columns: ['test', 'id'], + }); + }); + + it("doesn't save default params if explicitly specified", () => { + const storageSetter = vi.fn(); + mockStorage.mockReturnValue({ + value: {}, + setValue: storageSetter, + }); + const querySetter = vi.fn(); + mockQuery.mockReturnValue([new URLSearchParams(), querySetter]); + + const { result } = renderHook(() => + useTableState<{ + [key: string]: string | string[]; + }>({}, 'test', ['saveOnlyThisToUrl'], ['page']), + ); + const setParams = result.current[1]; + + act(() => { + setParams({ + saveOnlyThisToUrl: 'test', + page: '2', + pageSize: '10', + search: 'test', + sortBy: 'type', + sortOrder: 'favorites', + favorites: 'false', + columns: ['test', 'id'], + }); + }); + + expect(querySetter).toHaveBeenCalledWith({ saveOnlyThisToUrl: 'test' }); + expect(storageSetter).toHaveBeenCalledWith({ page: '2' }); + }); + + it('can reset state to the default instead of overwriting', () => { + mockStorage.mockReturnValue({ + value: { pageSize: 25 }, + setValue: vi.fn(), + }); + mockQuery.mockReturnValue([new URLSearchParams('page=4'), vi.fn()]); + + const { result } = renderHook(() => + useTableState<{ + page: string; + pageSize?: string; + sortBy?: string; + }>({ page: '1', pageSize: '10' }, 'test'), + ); + + const setParams = result.current[1]; + + act(() => { + setParams({ sortBy: 'type' }); + }); + expect(result.current[0]).toEqual({ + page: '4', + pageSize: '10', + sortBy: 'type', + }); + + act(() => { + setParams({ pageSize: '50' }, true); + }); + + expect(result.current[0]).toEqual({ + page: '1', + pageSize: '50', + }); + }); +}); diff --git a/frontend/src/hooks/useTableState.ts b/frontend/src/hooks/useTableState.ts new file mode 100644 index 0000000000..8e2ad79351 --- /dev/null +++ b/frontend/src/hooks/useTableState.ts @@ -0,0 +1,82 @@ +import { useState } from 'react'; +import { useSearchParams } from 'react-router-dom'; +import { createLocalStorage } from '../utils/createLocalStorage'; + +const filterObjectKeys = >( + obj: T, + keys: Array, +) => + Object.fromEntries( + Object.entries(obj).filter(([key]) => keys.includes(key as keyof T)), + ) as T; + +const defaultStoredKeys = [ + 'pageSize', + 'search', + 'sortBy', + 'sortOrder', + 'favorites', + 'columns', +]; +const defaultQueryKeys = [...defaultStoredKeys, 'page']; + +/** + * There are 3 sources of params, in order of priority: + * 1. local state + * 2. search params from the url + * 3. stored params in local storage + * 4. default parameters + * + * `queryKeys` will be saved in the url + * `storedKeys` will be saved in local storage + * + * @param defaultParams initial state + * @param storageId identifier for the local storage + * @param queryKeys array of elements to be saved in the url + * @param storageKeys array of elements to be saved in local storage + */ +export const useTableState = >( + defaultParams: Params, + storageId: string, + queryKeys?: Array, + storageKeys?: Array, +) => { + const [searchParams, setSearchParams] = useSearchParams(); + const { value: storedParams, setValue: setStoredParams } = + createLocalStorage(`${storageId}:tableQuery`, defaultParams); + + const searchQuery = Object.fromEntries(searchParams.entries()); + const [params, setParams] = useState({ + ...defaultParams, + ...(Object.keys(searchQuery).length ? {} : storedParams), + ...searchQuery, + } as Params); + + const updateParams = (value: Partial, reset = false) => { + const newState: Params = reset + ? { ...defaultParams, ...value } + : { + ...params, + ...value, + }; + + // remove keys with undefined values + Object.keys(newState).forEach((key) => { + if (newState[key] === undefined) { + delete newState[key]; + } + }); + + setParams(newState); + setSearchParams( + filterObjectKeys(newState, queryKeys || defaultQueryKeys), + ); + setStoredParams( + filterObjectKeys(newState, storageKeys || defaultStoredKeys), + ); + + return params; + }; + + return [params, updateParams] as const; +}; From 83fe430a14f937b96249ac258a92d4e943c68353 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Tue, 21 Nov 2023 11:49:50 +0100 Subject: [PATCH 09/11] Feat/private project badge (#5373) Adds an icon with tooltip for private projects in ProjectCard and Project header: Skjermbilde 2023-11-21 kl 10 58 13 --- .../HiddenProjectIconWithTooltip.tsx | 17 +++++++++++++++++ .../src/component/project/Project/Project.tsx | 5 +++++ .../CollaborationModeTooltip.tsx | 9 +++++---- .../project/ProjectCard/ProjectCard.styles.ts | 5 +++++ .../project/ProjectCard/ProjectCard.tsx | 16 ++++++++++++---- .../project/ProjectList/ProjectList.tsx | 2 ++ .../project/ProjectList/loadingData.ts | 4 ++++ frontend/src/interfaces/project.ts | 1 + 8 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 frontend/src/component/project/Project/HiddenProjectIconWithTooltip/HiddenProjectIconWithTooltip.tsx diff --git a/frontend/src/component/project/Project/HiddenProjectIconWithTooltip/HiddenProjectIconWithTooltip.tsx b/frontend/src/component/project/Project/HiddenProjectIconWithTooltip/HiddenProjectIconWithTooltip.tsx new file mode 100644 index 0000000000..bb753b5caa --- /dev/null +++ b/frontend/src/component/project/Project/HiddenProjectIconWithTooltip/HiddenProjectIconWithTooltip.tsx @@ -0,0 +1,17 @@ +import { styled } from '@mui/material'; +import { VisibilityOff } from '@mui/icons-material'; +import { HtmlTooltip } from 'component/common/HtmlTooltip/HtmlTooltip'; + +export const StyledVisibilityIcon = styled(VisibilityOff)(({ theme }) => ({ + color: theme.palette.action.disabled, +})); + +export const HiddenProjectIconWithTooltip = () => ( + + + +); diff --git a/frontend/src/component/project/Project/Project.tsx b/frontend/src/component/project/Project/Project.tsx index 58427523a0..500e00c896 100644 --- a/frontend/src/component/project/Project/Project.tsx +++ b/frontend/src/component/project/Project/Project.tsx @@ -41,6 +41,7 @@ import { Badge } from 'component/common/Badge/Badge'; import { ProjectDoraMetrics } from './ProjectDoraMetrics/ProjectDoraMetrics'; import { UiFlags } from 'interfaces/uiConfig'; import { ExperimentalProjectFeatures } from './ExperimentalProjectFeatures/ExperimentalProjectFeatures'; +import { HiddenProjectIconWithTooltip } from './HiddenProjectIconWithTooltip/HiddenProjectIconWithTooltip'; const StyledBadge = styled(Badge)(({ theme }) => ({ position: 'absolute', @@ -189,6 +190,10 @@ export const Project = () => { isFavorite={project?.favorite} /> + } + /> {projectName} diff --git a/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/CollaborationModeTooltip.tsx b/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/CollaborationModeTooltip.tsx index 0afaab9e12..659ba8cdb1 100644 --- a/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/CollaborationModeTooltip.tsx +++ b/frontend/src/component/project/Project/ProjectEnterpriseSettingsForm/CollaborationModeTooltip.tsx @@ -23,13 +23,13 @@ export const CollaborationModeTooltip: FC = () => { open: - everyone can submit change requests + Everyone can submit change requests protected: - only admins and project members can submit change + Only admins and project members can submit change requests @@ -39,8 +39,9 @@ export const CollaborationModeTooltip: FC = () => { private: - only projects members can and access see the - project + Only admins, editors and project members can + see and access the project and associated + feature toggles } diff --git a/frontend/src/component/project/ProjectCard/ProjectCard.styles.ts b/frontend/src/component/project/ProjectCard/ProjectCard.styles.ts index bf78de9c48..397947a832 100644 --- a/frontend/src/component/project/ProjectCard/ProjectCard.styles.ts +++ b/frontend/src/component/project/ProjectCard/ProjectCard.styles.ts @@ -75,3 +75,8 @@ export const StyledParagraphInfo = styled('p')(({ theme }) => ({ color: theme.palette.primary.dark, fontWeight: 'bold', })); + +export const StyledIconBox = styled(Box)(() => ({ + display: 'flex', + justifyContent: 'center', +})); diff --git a/frontend/src/component/project/ProjectCard/ProjectCard.tsx b/frontend/src/component/project/ProjectCard/ProjectCard.tsx index 52ff4a5eac..f35d3b612b 100644 --- a/frontend/src/component/project/ProjectCard/ProjectCard.tsx +++ b/frontend/src/component/project/ProjectCard/ProjectCard.tsx @@ -1,4 +1,4 @@ -import { Menu, MenuItem } from '@mui/material'; +import { Menu, MenuItem, Tooltip, Box } from '@mui/material'; import MoreVertIcon from '@mui/icons-material/MoreVert'; import React, { SyntheticEvent, useContext, useState } from 'react'; import { useNavigate } from 'react-router-dom'; @@ -26,8 +26,10 @@ import { StyledDivInfo, StyledDivInfoContainer, StyledParagraphInfo, + StyledIconBox, } from './ProjectCard.styles'; import useToast from 'hooks/useToast'; +import { HiddenProjectIconWithTooltip } from '../Project/HiddenProjectIconWithTooltip/HiddenProjectIconWithTooltip'; interface IProjectCardProps { name: string; @@ -37,6 +39,7 @@ interface IProjectCardProps { id: string; onHover: () => void; isFavorite?: boolean; + mode: string; } export const ProjectCard = ({ @@ -46,6 +49,7 @@ export const ProjectCard = ({ memberCount, onHover, id, + mode, isFavorite = false, }: IProjectCardProps) => { const { hasAccess } = useContext(AccessContext); @@ -127,9 +131,13 @@ export const ProjectCard = ({ -
- -
+ + } + elseShow={} + /> + diff --git a/frontend/src/component/project/ProjectList/ProjectList.tsx b/frontend/src/component/project/ProjectList/ProjectList.tsx index a9f5c5ce67..a6d8d9279e 100644 --- a/frontend/src/component/project/ProjectList/ProjectList.tsx +++ b/frontend/src/component/project/ProjectList/ProjectList.tsx @@ -245,6 +245,7 @@ export const ProjectListNew = () => { key={project.id} name={project.name} id={project.id} + mode={project.mode} memberCount={2} health={95} featureCount={4} @@ -263,6 +264,7 @@ export const ProjectListNew = () => { handleHover(project.id) } name={project.name} + mode={project.mode} memberCount={ project.memberCount ?? 0 } diff --git a/frontend/src/component/project/ProjectList/loadingData.ts b/frontend/src/component/project/ProjectList/loadingData.ts index 04de3f2973..4e6f4d7033 100644 --- a/frontend/src/component/project/ProjectList/loadingData.ts +++ b/frontend/src/component/project/ProjectList/loadingData.ts @@ -7,6 +7,7 @@ const loadingData = [ featureCount: 4, createdAt: '', description: '', + mode: '', }, { id: 'loading2', @@ -16,6 +17,7 @@ const loadingData = [ featureCount: 4, createdAt: '', description: '', + mode: '', }, { id: 'loading3', @@ -25,6 +27,7 @@ const loadingData = [ featureCount: 4, createdAt: '', description: '', + mode: '', }, { id: 'loading4', @@ -34,6 +37,7 @@ const loadingData = [ featureCount: 4, createdAt: '', description: '', + mode: '', }, ]; diff --git a/frontend/src/interfaces/project.ts b/frontend/src/interfaces/project.ts index 06d4a3a9d2..b17f6fb86d 100644 --- a/frontend/src/interfaces/project.ts +++ b/frontend/src/interfaces/project.ts @@ -10,6 +10,7 @@ export interface IProjectCard { health: number; description: string; featureCount: number; + mode: string; memberCount?: number; favorite?: boolean; } From 9dc64659b769320387ff73b7f4bee97d68abf768 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 21 Nov 2023 12:53:35 +0100 Subject: [PATCH 10/11] Fix: add the right change --- ...e-request-segment-usage-read-model.test.ts | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts index f2581c2208..489900b128 100644 --- a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts +++ b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts @@ -100,40 +100,6 @@ const updateStrategyInCr = async ( }); }; -describe.each([ - [ - 'updateStrategy', - (segmentId: number) => - updateStrategyInCr(randomId(), segmentId, FLAG_NAME), - ], - [ - 'addStrategy', - (segmentId: number) => addStrategyToCr(segmentId, FLAG_NAME), - ], -])('Should handle %s changes correctly', (_, addOrUpdateStrategy) => { - test.each([ - ['Draft', true], - ['In review', true], - ['Scheduled', true], - ['Approved', true], - ['Rejected', false], - ['Cancelled', false], - ['Applied', false], - ])( - 'Changes in %s CRs should make it %s', - async (state, expectedOutcome) => { - await createCR(state); - - const segmentId = 3; - await addOrUpdateStrategy(segmentId); - - expect( - await readModel.isSegmentUsedInActiveChangeRequests(segmentId), - ).toBe(expectedOutcome); - }, - ); -}); - test.each([ ['Draft', true], ['In review', true], From f8db9098fca92e7a7bc165adeadc01e61d71788f Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 21 Nov 2023 12:54:45 +0100 Subject: [PATCH 11/11] Revert "Fix: add the right change" This reverts commit 9dc64659b769320387ff73b7f4bee97d68abf768. Wasn't intended to push to main. --- ...e-request-segment-usage-read-model.test.ts | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts index 489900b128..f2581c2208 100644 --- a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts +++ b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts @@ -100,6 +100,40 @@ const updateStrategyInCr = async ( }); }; +describe.each([ + [ + 'updateStrategy', + (segmentId: number) => + updateStrategyInCr(randomId(), segmentId, FLAG_NAME), + ], + [ + 'addStrategy', + (segmentId: number) => addStrategyToCr(segmentId, FLAG_NAME), + ], +])('Should handle %s changes correctly', (_, addOrUpdateStrategy) => { + test.each([ + ['Draft', true], + ['In review', true], + ['Scheduled', true], + ['Approved', true], + ['Rejected', false], + ['Cancelled', false], + ['Applied', false], + ])( + 'Changes in %s CRs should make it %s', + async (state, expectedOutcome) => { + await createCR(state); + + const segmentId = 3; + await addOrUpdateStrategy(segmentId); + + expect( + await readModel.isSegmentUsedInActiveChangeRequests(segmentId), + ).toBe(expectedOutcome); + }, + ); +}); + test.each([ ['Draft', true], ['In review', true],