From 4c358c88df8af84b6e902250510c476ad4a94109 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 18 Jul 2025 13:06:09 +0200 Subject: [PATCH 1/7] Propagate IConstraintWithId Makes it so that we require constraints with IDs in more places (to avoid accidentally mixing up constraint states). From c394800a1981e24e99344298c99a1d25635e1f31 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 18 Jul 2025 13:23:18 +0200 Subject: [PATCH 2/7] Remove IConstraintWithId type. The next move is to make id required on IConstraint. --- frontend/src/component/segments/SegmentForm.tsx | 4 ++-- frontend/src/component/segments/SegmentFormStepTwo.test.tsx | 4 ++-- frontend/src/component/segments/SegmentFormStepTwo.tsx | 4 ++-- frontend/src/component/segments/hooks/useSegmentForm.ts | 4 ++-- frontend/src/interfaces/strategy.ts | 4 ---- frontend/src/utils/createEmptyConstraint.ts | 6 ++---- 6 files changed, 10 insertions(+), 16 deletions(-) diff --git a/frontend/src/component/segments/SegmentForm.tsx b/frontend/src/component/segments/SegmentForm.tsx index 0d3cfcee14..8a1c38c7ad 100644 --- a/frontend/src/component/segments/SegmentForm.tsx +++ b/frontend/src/component/segments/SegmentForm.tsx @@ -1,4 +1,4 @@ -import type { IConstraint, IConstraintWithId } from 'interfaces/strategy'; +import type { IConstraint } from 'interfaces/strategy'; import { SegmentFormStepOne } from './SegmentFormStepOne.tsx'; import { SegmentFormStepTwo } from './SegmentFormStepTwo.tsx'; import type React from 'react'; @@ -14,7 +14,7 @@ interface ISegmentProps { name: string; description: string; project?: string; - constraints: IConstraintWithId[]; + constraints: IConstraint[]; setName: React.Dispatch>; setDescription: React.Dispatch>; setProject: React.Dispatch>; diff --git a/frontend/src/component/segments/SegmentFormStepTwo.test.tsx b/frontend/src/component/segments/SegmentFormStepTwo.test.tsx index 6a8efec383..3c8160d348 100644 --- a/frontend/src/component/segments/SegmentFormStepTwo.test.tsx +++ b/frontend/src/component/segments/SegmentFormStepTwo.test.tsx @@ -8,7 +8,7 @@ import { CREATE_SEGMENT, UPDATE_PROJECT_SEGMENT, } from 'component/providers/AccessProvider/permissions'; -import type { IConstraintWithId } from 'interfaces/strategy.ts'; +import type { IConstraint } from 'interfaces/strategy.ts'; const server = testServerSetup(); @@ -26,7 +26,7 @@ const setupRoutes = () => { const defaultProps = { project: undefined, - constraints: [] as IConstraintWithId[], + constraints: [] as IConstraint[], setConstraints: vi.fn(), setCurrentStep: vi.fn(), mode: 'create' as const, diff --git a/frontend/src/component/segments/SegmentFormStepTwo.tsx b/frontend/src/component/segments/SegmentFormStepTwo.tsx index abea3aeb9e..ddb84cf021 100644 --- a/frontend/src/component/segments/SegmentFormStepTwo.tsx +++ b/frontend/src/component/segments/SegmentFormStepTwo.tsx @@ -13,7 +13,7 @@ import { UPDATE_SEGMENT, } from 'component/providers/AccessProvider/permissions'; import useUnleashContext from 'hooks/api/getters/useUnleashContext/useUnleashContext'; -import type { IConstraint, IConstraintWithId } from 'interfaces/strategy'; +import type { IConstraint } from 'interfaces/strategy'; import { useNavigate } from 'react-router-dom'; import { EditableConstraintsList } from 'component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList'; import type { IEditableConstraintsListRef } from 'component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList'; @@ -33,7 +33,7 @@ import { GO_BACK } from 'constants/navigate'; interface ISegmentFormPartTwoProps { project?: string; - constraints: IConstraintWithId[]; + constraints: IConstraint[]; setConstraints: React.Dispatch>; setCurrentStep: React.Dispatch>; mode: SegmentFormMode; diff --git a/frontend/src/component/segments/hooks/useSegmentForm.ts b/frontend/src/component/segments/hooks/useSegmentForm.ts index 951619715d..e8e0d79992 100644 --- a/frontend/src/component/segments/hooks/useSegmentForm.ts +++ b/frontend/src/component/segments/hooks/useSegmentForm.ts @@ -1,4 +1,4 @@ -import type { IConstraint, IConstraintWithId } from 'interfaces/strategy'; +import type { IConstraint } from 'interfaces/strategy'; import { useEffect, useState } from 'react'; import { useSegmentValidation } from 'hooks/api/getters/useSegmentValidation/useSegmentValidation'; import { v4 as uuidv4 } from 'uuid'; @@ -17,7 +17,7 @@ export const useSegmentForm = ( [constraintId]: uuidv4(), ...constraint, })); - const [constraints, setConstraints] = useState( + const [constraints, setConstraints] = useState( initialConstraintsWithId, ); const [errors, setErrors] = useState({}); diff --git a/frontend/src/interfaces/strategy.ts b/frontend/src/interfaces/strategy.ts index 0f46621f79..b2aa226009 100644 --- a/frontend/src/interfaces/strategy.ts +++ b/frontend/src/interfaces/strategy.ts @@ -68,10 +68,6 @@ export interface IConstraint { [constraintId]?: string; } -export interface IConstraintWithId extends IConstraint { - [constraintId]: string; -} - export interface IFeatureStrategySortOrder { id: string; sortOrder: number; diff --git a/frontend/src/utils/createEmptyConstraint.ts b/frontend/src/utils/createEmptyConstraint.ts index 49e2887cbf..f05b19f1df 100644 --- a/frontend/src/utils/createEmptyConstraint.ts +++ b/frontend/src/utils/createEmptyConstraint.ts @@ -1,12 +1,10 @@ import { constraintId } from 'constants/constraintId'; import { isDateOperator } from 'constants/operators'; -import type { IConstraintWithId } from 'interfaces/strategy'; +import type { IConstraint } from 'interfaces/strategy'; import { operatorsForContext } from 'utils/operatorsForContext'; import { v4 as uuidv4 } from 'uuid'; -export const createEmptyConstraint = ( - contextName: string, -): IConstraintWithId => { +export const createEmptyConstraint = (contextName: string): IConstraint => { const operator = operatorsForContext(contextName)[0]; const value = isDateOperator(operator) ? new Date().toISOString() : ''; From 71c9a533806fb1701a131d6051a538847f4e24b7 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 18 Jul 2025 13:23:55 +0200 Subject: [PATCH 3/7] Make id required on IConstraint. Enforce setting [constraintId] on incoming constraints --- .../ConstraintsList/EditableConstraintsList.tsx | 2 -- .../editable-constraint-type.test.ts | 13 ------------- .../editable-constraint-type.ts | 4 ---- .../useEditableConstraint.test.tsx | 12 ++++++++++++ .../useRecentlyUsedConstraints.test.tsx | 10 ++++++++++ .../src/component/segments/hooks/useSegmentForm.ts | 3 --- .../useConstraintsValidation.test.tsx | 12 +++++++++++- frontend/src/interfaces/strategy.ts | 2 +- .../utils/api-payload-constraint-replacer.test.ts | 2 ++ .../src/utils/api-payload-constraint-replacer.ts | 7 +++++-- 10 files changed, 41 insertions(+), 26 deletions(-) diff --git a/frontend/src/component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList.tsx b/frontend/src/component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList.tsx index 0c37645762..c1cfbdae95 100644 --- a/frontend/src/component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList.tsx +++ b/frontend/src/component/common/NewConstraintAccordion/ConstraintsList/EditableConstraintsList.tsx @@ -9,7 +9,6 @@ import { ConstraintsList } from 'component/common/ConstraintsList/ConstraintsLis import { EditableConstraint } from 'component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/EditableConstraint'; import { createEmptyConstraint } from '../../../../utils/createEmptyConstraint.ts'; import { constraintId } from 'constants/constraintId.ts'; -import { v4 as uuidv4 } from 'uuid'; export interface IEditableConstraintsListRef { addConstraint?: (contextName: string) => void; } @@ -44,7 +43,6 @@ export const EditableConstraintsList = forwardRef< if (!constraints.every((constraint) => constraintId in constraint)) { setConstraints( constraints.map((constraint) => ({ - [constraintId]: uuidv4(), ...constraint, })), ); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.test.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.test.ts index 7eb54bdfaa..88630518b1 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.test.ts +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.test.ts @@ -1,7 +1,6 @@ import { createEmptyConstraint } from 'utils/createEmptyConstraint'; import { fromIConstraint, toIConstraint } from './editable-constraint-type.ts'; import { constraintId } from 'constants/constraintId'; -import type { IConstraint } from 'interfaces/strategy.ts'; test('mapping to and from retains the constraint id', () => { const constraint = createEmptyConstraint('context'); @@ -11,18 +10,6 @@ test('mapping to and from retains the constraint id', () => { ); }); -test('mapping to an editable constraint adds a constraint id if there is none', () => { - const constraint: IConstraint = createEmptyConstraint('context'); - delete constraint[constraintId]; - - const editableConstraint = fromIConstraint(constraint); - - expect(editableConstraint[constraintId]).toBeDefined(); - - const iConstraint = toIConstraint(editableConstraint); - expect(iConstraint[constraintId]).toEqual(editableConstraint[constraintId]); -}); - test('mapping from an empty constraint removes redundant value / values', () => { const constraint = { ...createEmptyConstraint('context'), value: '' }; expect(constraint).toHaveProperty('value'); diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.ts b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.ts index 80c798ba85..c5f14e2689 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.ts +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.ts @@ -1,4 +1,3 @@ -import { constraintId } from 'constants/constraintId'; import { type DateOperator, isDateOperator, @@ -11,7 +10,6 @@ import { isSemVerOperator, } from 'constants/operators'; import type { IConstraint } from 'interfaces/strategy'; -import { v4 as uuidv4 } from 'uuid'; type EditableConstraintBase = Omit< IConstraint, @@ -74,14 +72,12 @@ export const fromIConstraint = ( const { value, values, operator, ...rest } = constraint; if (isSingleValueOperator(operator)) { return { - [constraintId]: uuidv4(), ...rest, operator, value: value ?? '', }; } else { return { - [constraintId]: uuidv4(), ...rest, operator, values: new Set(values), diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx index ff63dae6c8..043304137d 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx @@ -13,6 +13,7 @@ import { vi } from 'vitest'; import { testServerRoute, testServerSetup } from 'utils/testServer'; import type { ContextFieldSchema } from 'openapi'; import { NUM_EQ } from '@server/util/constants'; +import { constraintId } from 'constants/constraintId.js'; const server = testServerSetup(); @@ -25,6 +26,7 @@ test('calls onUpdate with new state', async () => { contextName: 'context-field', operator: NOT_IN, values: ['A', 'B'], + [constraintId]: 'constraint id', }; const onUpdate = vi.fn(); @@ -71,6 +73,7 @@ describe('validators', () => { contextName: 'context-field', operator: operator, value: '', + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -94,6 +97,7 @@ describe('validators', () => { contextName: 'context-field', operator: operator, value: '', + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -117,6 +121,7 @@ describe('validators', () => { contextName: 'context-field', operator: operator, value: '', + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -140,6 +145,7 @@ describe('validators', () => { contextName: 'context-field', operator: operator, values: [], + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -162,6 +168,7 @@ describe('validators', () => { contextName: 'context-field', operator: operator, values: ['a', 'b'], + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -189,6 +196,7 @@ describe('legal values', () => { contextName: definition.name, operator: IN, values: [], + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -206,6 +214,7 @@ describe('legal values', () => { contextName: definition.name, operator: IN, values: [], + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -231,6 +240,7 @@ describe('legal values', () => { contextName: 'field-with-no-legal-values', operator: IN, values: [], + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -244,6 +254,7 @@ describe('legal values', () => { contextName: definition.name, operator: IN, values: ['A', 'B'], + [constraintId]: 'constraint id', }; const { result } = renderHook(() => @@ -260,6 +271,7 @@ describe('legal values', () => { contextName: definition.name, operator: NUM_EQ, values: [], + [constraintId]: 'constraint id', }; const { result } = renderHook(() => diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/RecentlyUsedConstraints/useRecentlyUsedConstraints.test.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/RecentlyUsedConstraints/useRecentlyUsedConstraints.test.tsx index ac2cee801e..96502eea17 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/RecentlyUsedConstraints/useRecentlyUsedConstraints.test.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/RecentlyUsedConstraints/useRecentlyUsedConstraints.test.tsx @@ -6,6 +6,7 @@ import { renderHook, act } from '@testing-library/react'; import type { IConstraint } from 'interfaces/strategy'; import { IN, STR_CONTAINS } from 'constants/operators'; import type { Operator } from 'constants/operators'; +import { constraintId } from 'constants/constraintId.ts'; const createTestConstraint = ( contextName: string, @@ -15,6 +16,7 @@ const createTestConstraint = ( contextName, operator, values, + [constraintId]: 'constraint id', }); describe('areConstraintsEqual', () => { @@ -24,6 +26,7 @@ describe('areConstraintsEqual', () => { operator: IN, values: ['user1', 'user2'], inverted: false, + [constraintId]: 'constraint id', }; const constraint2: IConstraint = { @@ -31,6 +34,7 @@ describe('areConstraintsEqual', () => { operator: IN, values: ['user1', 'user2'], inverted: false, + [constraintId]: 'constraint id', }; expect(areConstraintsEqual(constraint1, constraint2)).toBe(true); @@ -41,12 +45,14 @@ describe('areConstraintsEqual', () => { contextName: 'userId', operator: IN, values: ['user1', 'user2', 'user3'], + [constraintId]: 'constraint id', }; const constraint2: IConstraint = { contextName: 'userId', operator: IN, values: ['user2', 'user3', 'user1'], + [constraintId]: 'constraint id', }; expect(areConstraintsEqual(constraint1, constraint2)).toBe(true); @@ -57,12 +63,14 @@ describe('areConstraintsEqual', () => { contextName: 'userId', operator: IN, values: ['user1', 'user2'], + [constraintId]: 'constraint id', }; const constraint2: IConstraint = { contextName: 'userId', operator: IN, values: ['user1', 'user3'], + [constraintId]: 'constraint id', }; expect(areConstraintsEqual(constraint1, constraint2)).toBe(false); @@ -73,12 +81,14 @@ describe('areConstraintsEqual', () => { contextName: 'userId', operator: IN, values: ['user1'], + [constraintId]: 'constraint id', }; const constraint2: IConstraint = { contextName: 'userId', operator: STR_CONTAINS, values: ['user1'], + [constraintId]: 'constraint id', }; expect(areConstraintsEqual(constraint1, constraint2)).toBe(false); diff --git a/frontend/src/component/segments/hooks/useSegmentForm.ts b/frontend/src/component/segments/hooks/useSegmentForm.ts index e8e0d79992..bbbb89aa84 100644 --- a/frontend/src/component/segments/hooks/useSegmentForm.ts +++ b/frontend/src/component/segments/hooks/useSegmentForm.ts @@ -1,8 +1,6 @@ import type { IConstraint } from 'interfaces/strategy'; import { useEffect, useState } from 'react'; import { useSegmentValidation } from 'hooks/api/getters/useSegmentValidation/useSegmentValidation'; -import { v4 as uuidv4 } from 'uuid'; -import { constraintId } from 'constants/constraintId'; export const useSegmentForm = ( initialName = '', @@ -14,7 +12,6 @@ export const useSegmentForm = ( const [description, setDescription] = useState(initialDescription); const [project, setProject] = useState(initialProject); const initialConstraintsWithId = initialConstraints.map((constraint) => ({ - [constraintId]: uuidv4(), ...constraint, })); const [constraints, setConstraints] = useState( diff --git a/frontend/src/hooks/api/getters/useConstraintsValidation/useConstraintsValidation.test.tsx b/frontend/src/hooks/api/getters/useConstraintsValidation/useConstraintsValidation.test.tsx index f9a0d8a039..dd71b09799 100644 --- a/frontend/src/hooks/api/getters/useConstraintsValidation/useConstraintsValidation.test.tsx +++ b/frontend/src/hooks/api/getters/useConstraintsValidation/useConstraintsValidation.test.tsx @@ -3,6 +3,7 @@ import type { IConstraint } from 'interfaces/strategy'; // Assuming you have you import type { FC } from 'react'; import { useConstraintsValidation } from './useConstraintsValidation.ts'; import { testServerRoute, testServerSetup } from 'utils/testServer'; +import { constraintId } from 'constants/constraintId.ts'; const server = testServerSetup(); @@ -26,12 +27,14 @@ test('should display Valid when constraints are valid', async () => { values: ['test'], operator: 'IN', contextName: 'irrelevant', + [constraintId]: 'constraint id', }, { value: 'test', values: ['test'], operator: 'IN', contextName: 'irrelevant', + [constraintId]: 'constraint id 2', }, ]; @@ -42,12 +45,19 @@ test('should display Valid when constraints are valid', async () => { test('should display Invalid when constraints are invalid', async () => { const emptyValueAndValues: IConstraint[] = [ - { value: '', values: [], operator: 'IN', contextName: 'irrelevant' }, { value: '', values: [], operator: 'IN', contextName: 'irrelevant', + [constraintId]: 'constraint id 3', + }, + { + value: '', + values: [], + operator: 'IN', + contextName: 'irrelevant', + [constraintId]: 'constraint id 4', }, ]; diff --git a/frontend/src/interfaces/strategy.ts b/frontend/src/interfaces/strategy.ts index b2aa226009..702782ffbc 100644 --- a/frontend/src/interfaces/strategy.ts +++ b/frontend/src/interfaces/strategy.ts @@ -65,7 +65,7 @@ export interface IConstraint { caseInsensitive?: boolean; operator: Operator; contextName: string; - [constraintId]?: string; + [constraintId]: string; } export interface IFeatureStrategySortOrder { diff --git a/frontend/src/utils/api-payload-constraint-replacer.test.ts b/frontend/src/utils/api-payload-constraint-replacer.test.ts index 1790d2b6bd..4c70807020 100644 --- a/frontend/src/utils/api-payload-constraint-replacer.test.ts +++ b/frontend/src/utils/api-payload-constraint-replacer.test.ts @@ -9,6 +9,7 @@ test('keys are ordered in the expected order', () => { operator: 'STR_CONTAINS', contextName: 'context', caseInsensitive: true, + [constraintId]: 'constraint-id', }; const output = serializeConstraint(input); @@ -30,6 +31,7 @@ test('only value OR values is present, not both', () => { operator: 'IN', contextName: 'context', caseInsensitive: true, + [constraintId]: 'constraint-id', }; const noValue = serializeConstraint(input); diff --git a/frontend/src/utils/api-payload-constraint-replacer.ts b/frontend/src/utils/api-payload-constraint-replacer.ts index 244b207d74..6de5dae265 100644 --- a/frontend/src/utils/api-payload-constraint-replacer.ts +++ b/frontend/src/utils/api-payload-constraint-replacer.ts @@ -1,6 +1,9 @@ +import type { constraintId } from 'constants/constraintId'; import { isSingleValueOperator } from 'constants/operators'; import type { IConstraint } from 'interfaces/strategy'; +type SerializedConstraint = Omit; + export const serializeConstraint = ({ value, values, @@ -8,10 +11,10 @@ export const serializeConstraint = ({ operator, contextName, caseInsensitive, -}: IConstraint): IConstraint => { +}: IConstraint): SerializedConstraint => { const makeConstraint = ( valueProp: { value: string } | { values: string[] }, - ): IConstraint => { + ): SerializedConstraint => { return { contextName, operator, From 872c582eb9f5836b9e72e9a3bae05f07db5399a9 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 18 Jul 2025 13:52:19 +0200 Subject: [PATCH 4/7] Enforce [constraintId] on incoming constraints --- .../strategy-change-diff-calculation.test.ts | 6 ++ .../ProjectEnvironmentDefaultStrategy.tsx | 8 ++- .../useChangeRequest/useChangeRequest.ts | 58 ++++++++++++++++++- .../api/getters/useFeature/useFeature.ts | 44 +++++++++++++- .../getters/useFeature/useFeatureImmutable.ts | 8 ++- 5 files changed, 116 insertions(+), 8 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts index 3eecee1e84..ea83dbf387 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts @@ -9,6 +9,7 @@ import { getSegmentChangesThatWouldBeOverwritten, getStrategyChangesThatWouldBeOverwritten, } from './strategy-change-diff-calculation.js'; +import { constraintId } from 'constants/constraintId.js'; describe('Strategy change conflict detection', () => { const existingStrategy: IFeatureStrategy = { @@ -175,6 +176,7 @@ describe('Strategy change conflict detection', () => { operator: 'IN' as const, contextName: 'appName', caseInsensitive: false, + [constraintId]: 'id1', }, ], variants: [ @@ -230,6 +232,7 @@ describe('Strategy change conflict detection', () => { operator: 'IN' as const, contextName: 'appName', caseInsensitive: false, + [constraintId]: 'id2', }, ], }; @@ -249,6 +252,7 @@ describe('Strategy change conflict detection', () => { inverted: false, operator: 'IN' as const, values: ['blah'], + [constraintId]: 'id2', }, ], }, @@ -478,6 +482,7 @@ describe('Segment change conflict detection', () => { operator: 'IN' as const, contextName: 'appName', caseInsensitive: false, + [constraintId]: 'id3', }, ], }; @@ -494,6 +499,7 @@ describe('Segment change conflict detection', () => { operator: 'IN' as const, contextName: 'appName', caseInsensitive: false, + [constraintId]: 'id4', }, ], }, diff --git a/frontend/src/component/project/Project/ProjectSettings/ProjectDefaultStrategySettings/ProjectEnvironment/ProjectEnvironmentDefaultStrategy/ProjectEnvironmentDefaultStrategy.tsx b/frontend/src/component/project/Project/ProjectSettings/ProjectDefaultStrategySettings/ProjectEnvironment/ProjectEnvironmentDefaultStrategy/ProjectEnvironmentDefaultStrategy.tsx index fd6f23c2f7..35c561a83d 100644 --- a/frontend/src/component/project/Project/ProjectSettings/ProjectDefaultStrategySettings/ProjectEnvironment/ProjectEnvironmentDefaultStrategy/ProjectEnvironmentDefaultStrategy.tsx +++ b/frontend/src/component/project/Project/ProjectSettings/ProjectDefaultStrategySettings/ProjectEnvironment/ProjectEnvironmentDefaultStrategy/ProjectEnvironmentDefaultStrategy.tsx @@ -11,6 +11,8 @@ import { } from '@server/types/permissions'; import { StrategyItem } from 'component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyItem'; import type { IFeatureStrategy } from 'interfaces/strategy'; +import { constraintId } from 'constants/constraintId'; +import { v4 as uuidv4 } from 'uuid'; interface ProjectEnvironmentDefaultStrategyProps { environment: ProjectEnvironmentType; @@ -55,7 +57,11 @@ export const ProjectEnvironmentDefaultStrategy = ({ return { ...baseDefaultStrategy, disabled: false, - constraints: baseDefaultStrategy.constraints ?? [], + constraints: + baseDefaultStrategy.constraints?.map((constraint) => ({ + ...constraint, + [constraintId]: uuidv4(), + })) ?? [], title: baseDefaultStrategy.title ?? '', parameters: baseDefaultStrategy.parameters ?? {}, }; diff --git a/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts b/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts index 98e5f217ea..606c5e4d92 100644 --- a/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts +++ b/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts @@ -1,7 +1,22 @@ import useSWR from 'swr'; import { formatApiPath } from 'utils/formatPath'; import handleErrorResponses from '../httpErrorResponseHandler.js'; -import type { ChangeRequestType } from 'component/changeRequest/changeRequest.types'; +import type { + ChangeRequestType, + IChangeRequestAddStrategy, + IChangeRequestUpdateStrategy, + IFeatureChange, +} from 'component/changeRequest/changeRequest.types'; +import { useMemo } from 'react'; +import { constraintId } from 'constants/constraintId.js'; +import { v4 as uuidv4 } from 'uuid'; + +const isAddStrategyChange = ( + change: IFeatureChange, +): change is IChangeRequestAddStrategy => change.action === 'addStrategy'; +const isUpdateStrategyChange = ( + change: IFeatureChange, +): change is IChangeRequestUpdateStrategy => change.action === 'updateStrategy'; export const useChangeRequest = (projectId: string, id: string) => { const { data, error, mutate } = useSWR( @@ -10,8 +25,47 @@ export const useChangeRequest = (projectId: string, id: string) => { { refreshInterval: 15000 }, ); + const dataWithConstraintIds: ChangeRequestType | undefined = useMemo(() => { + if (!data) { + return data; + } + + const features = data.features.map((feature) => { + const changes: IFeatureChange[] = feature.changes.map((change) => { + if ( + isAddStrategyChange(change) || + isUpdateStrategyChange(change) + ) { + const { constraints, ...rest } = change.payload; + return { + ...change, + payload: { + ...rest, + constraints: constraints.map((constraint) => ({ + ...constraint, + [constraintId]: uuidv4(), + })), + }, + } as IFeatureChange; + } + return change; + }); + + return { + ...feature, + changes, + }; + }); + + const value: ChangeRequestType = { + ...data, + features, + }; + return value; + }, [data]); + return { - data, + data: dataWithConstraintIds, loading: !error && !data, refetchChangeRequest: () => mutate(), error, diff --git a/frontend/src/hooks/api/getters/useFeature/useFeature.ts b/frontend/src/hooks/api/getters/useFeature/useFeature.ts index e5b19a6a63..fa1540f719 100644 --- a/frontend/src/hooks/api/getters/useFeature/useFeature.ts +++ b/frontend/src/hooks/api/getters/useFeature/useFeature.ts @@ -1,9 +1,12 @@ import useSWR, { type SWRConfiguration } from 'swr'; -import { useCallback } from 'react'; +import { useCallback, useMemo } from 'react'; import { emptyFeature } from './emptyFeature.ts'; import handleErrorResponses from '../httpErrorResponseHandler.ts'; import { formatApiPath } from 'utils/formatPath'; import type { IFeatureToggle } from 'interfaces/featureToggle'; +import { constraintId } from 'constants/constraintId.ts'; +import { v4 as uuidv4 } from 'uuid'; +import type { IFeatureStrategy } from 'interfaces/strategy.ts'; export interface IUseFeatureOutput { feature: IFeatureToggle; @@ -35,8 +38,10 @@ export const useFeature = ( mutate().catch(console.warn); }, [mutate]); + const feature = useMemo(enrichConstraintsWithIds(data), [data?.body]); + return { - feature: data?.body || emptyFeature, + feature, refetchFeature, loading: !error && !data, status: data?.status, @@ -63,6 +68,41 @@ export const featureFetcher = async ( }; }; +export const enrichConstraintsWithIds = (data?: IFeatureResponse) => () => { + if (!data?.body) { + return emptyFeature; + } + + const { strategies, environments, ...rest } = data.body; + + const addConstraintIds = (strategy: IFeatureStrategy) => { + const { constraints, ...strategyRest } = strategy; + return { + ...strategyRest, + constraints: constraints?.map((constraint) => ({ + ...constraint, + [constraintId]: uuidv4(), + })), + }; + }; + + const strategiesWithConstraintIds = strategies?.map(addConstraintIds); + + const environmentsWithStrategyIds = environments?.map((environment) => { + const { strategies, ...environmentRest } = environment; + return { + ...environmentRest, + strategies: strategies?.map(addConstraintIds), + }; + }); + + return { + ...rest, + strategies: strategiesWithConstraintIds, + environments: environmentsWithStrategyIds, + }; +}; + export const formatFeatureApiPath = ( projectId: string, featureId: string, diff --git a/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts b/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts index b5c3656bfe..1e17c3f2ea 100644 --- a/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts +++ b/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts @@ -1,12 +1,12 @@ import useSWRImmutable from 'swr/immutable'; -import { useCallback } from 'react'; -import { emptyFeature } from './emptyFeature.js'; +import { useCallback, useMemo } from 'react'; import { type IUseFeatureOutput, type IFeatureResponse, featureFetcher, formatFeatureApiPath, useFeature, + enrichConstraintsWithIds, } from 'hooks/api/getters/useFeature/useFeature'; // useFeatureImmutable is like useFeature, except it won't refetch data on @@ -29,8 +29,10 @@ export const useFeatureImmutable = ( await refetchFeature(); }, [mutate, refetchFeature]); + const feature = useMemo(enrichConstraintsWithIds(data), [data?.body]); + return { - feature: data?.body || emptyFeature, + feature, refetchFeature: refetch, loading: !error && !data, status: data?.status, From 6ee18b0fe0b1ec68864d9f3a5607f7e12fc77522 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 21 Jul 2025 11:24:30 +0200 Subject: [PATCH 5/7] JSON.stringify some stuff and extract functions --- .../useChangeRequest/useChangeRequest.ts | 70 +++++++++---------- .../api/getters/useFeature/useFeature.ts | 63 +++++++++-------- .../getters/useFeature/useFeatureImmutable.ts | 4 +- 3 files changed, 69 insertions(+), 68 deletions(-) diff --git a/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts b/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts index 606c5e4d92..4434f3372f 100644 --- a/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts +++ b/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts @@ -4,6 +4,7 @@ import handleErrorResponses from '../httpErrorResponseHandler.js'; import type { ChangeRequestType, IChangeRequestAddStrategy, + IChangeRequestFeature, IChangeRequestUpdateStrategy, IFeatureChange, } from 'component/changeRequest/changeRequest.types'; @@ -18,6 +19,23 @@ const isUpdateStrategyChange = ( change: IFeatureChange, ): change is IChangeRequestUpdateStrategy => change.action === 'updateStrategy'; +const addConstraintIdsToFeatureChange = (change: IFeatureChange) => { + if (isAddStrategyChange(change) || isUpdateStrategyChange(change)) { + const { constraints, ...rest } = change.payload; + return { + ...change, + payload: { + ...rest, + constraints: constraints.map((constraint) => ({ + ...constraint, + [constraintId]: uuidv4(), + })), + }, + } as IFeatureChange; + } + return change; +}; + export const useChangeRequest = (projectId: string, id: string) => { const { data, error, mutate } = useSWR( formatApiPath(`api/admin/projects/${projectId}/change-requests/${id}`), @@ -25,47 +43,25 @@ export const useChangeRequest = (projectId: string, id: string) => { { refreshInterval: 15000 }, ); - const dataWithConstraintIds: ChangeRequestType | undefined = useMemo(() => { - if (!data) { - return data; - } + const { features, ...dataProps } = data || {}; + const featuresWithConstraintIds: IChangeRequestFeature[] | undefined = + useMemo(() => { + return ( + features?.map((feature) => { + const changes: IFeatureChange[] = feature.changes.map( + addConstraintIdsToFeatureChange, + ); - const features = data.features.map((feature) => { - const changes: IFeatureChange[] = feature.changes.map((change) => { - if ( - isAddStrategyChange(change) || - isUpdateStrategyChange(change) - ) { - const { constraints, ...rest } = change.payload; return { - ...change, - payload: { - ...rest, - constraints: constraints.map((constraint) => ({ - ...constraint, - [constraintId]: uuidv4(), - })), - }, - } as IFeatureChange; - } - return change; - }); - - return { - ...feature, - changes, - }; - }); - - const value: ChangeRequestType = { - ...data, - features, - }; - return value; - }, [data]); + ...feature, + changes, + }; + }) ?? [] + ); + }, [JSON.stringify(features)]); return { - data: dataWithConstraintIds, + data: { ...dataProps, features: featuresWithConstraintIds }, loading: !error && !data, refetchChangeRequest: () => mutate(), error, diff --git a/frontend/src/hooks/api/getters/useFeature/useFeature.ts b/frontend/src/hooks/api/getters/useFeature/useFeature.ts index fa1540f719..876955805e 100644 --- a/frontend/src/hooks/api/getters/useFeature/useFeature.ts +++ b/frontend/src/hooks/api/getters/useFeature/useFeature.ts @@ -38,7 +38,9 @@ export const useFeature = ( mutate().catch(console.warn); }, [mutate]); - const feature = useMemo(enrichConstraintsWithIds(data), [data?.body]); + const feature = useMemo(enrichConstraintsWithIds(data), [ + JSON.stringify(data?.body), + ]); return { feature, @@ -68,41 +70,42 @@ export const featureFetcher = async ( }; }; -export const enrichConstraintsWithIds = (data?: IFeatureResponse) => () => { - if (!data?.body) { - return emptyFeature; - } +export const enrichConstraintsWithIds = + (data?: IFeatureResponse) => (): IFeatureToggle => { + if (!data?.body) { + return emptyFeature; + } - const { strategies, environments, ...rest } = data.body; + const { strategies, environments, ...rest } = data.body; + + const addConstraintIds = (strategy: IFeatureStrategy) => { + const { constraints, ...strategyRest } = strategy; + return { + ...strategyRest, + constraints: constraints?.map((constraint) => ({ + ...constraint, + [constraintId]: uuidv4(), + })), + }; + }; + + const strategiesWithConstraintIds = strategies?.map(addConstraintIds); + + const environmentsWithStrategyIds = environments?.map((environment) => { + const { strategies, ...environmentRest } = environment; + return { + ...environmentRest, + strategies: strategies?.map(addConstraintIds), + }; + }); - const addConstraintIds = (strategy: IFeatureStrategy) => { - const { constraints, ...strategyRest } = strategy; return { - ...strategyRest, - constraints: constraints?.map((constraint) => ({ - ...constraint, - [constraintId]: uuidv4(), - })), + ...rest, + strategies: strategiesWithConstraintIds, + environments: environmentsWithStrategyIds, }; }; - const strategiesWithConstraintIds = strategies?.map(addConstraintIds); - - const environmentsWithStrategyIds = environments?.map((environment) => { - const { strategies, ...environmentRest } = environment; - return { - ...environmentRest, - strategies: strategies?.map(addConstraintIds), - }; - }); - - return { - ...rest, - strategies: strategiesWithConstraintIds, - environments: environmentsWithStrategyIds, - }; -}; - export const formatFeatureApiPath = ( projectId: string, featureId: string, diff --git a/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts b/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts index 1e17c3f2ea..e60f187be9 100644 --- a/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts +++ b/frontend/src/hooks/api/getters/useFeature/useFeatureImmutable.ts @@ -29,7 +29,9 @@ export const useFeatureImmutable = ( await refetchFeature(); }, [mutate, refetchFeature]); - const feature = useMemo(enrichConstraintsWithIds(data), [data?.body]); + const feature = useMemo(enrichConstraintsWithIds(data), [ + JSON.stringify(data?.body), + ]); return { feature, From c9efa2636548816b0a7e51676cc62c71ffe5eb37 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 21 Jul 2025 11:54:46 +0200 Subject: [PATCH 6/7] TODO: add constraint ids for default strategy which is in the project overview. --- .../useProjectOverview/useProjectOverview.ts | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/frontend/src/hooks/api/getters/useProjectOverview/useProjectOverview.ts b/frontend/src/hooks/api/getters/useProjectOverview/useProjectOverview.ts index 80f1615dd9..50a11dea67 100644 --- a/frontend/src/hooks/api/getters/useProjectOverview/useProjectOverview.ts +++ b/frontend/src/hooks/api/getters/useProjectOverview/useProjectOverview.ts @@ -1,5 +1,5 @@ import useSWR, { type SWRConfiguration } from 'swr'; -import { useCallback } from 'react'; +import { useCallback, useMemo } from 'react'; import { getProjectOverviewFetcher } from './getProjectOverviewFetcher.js'; import type { ProjectOverviewSchema } from 'openapi'; @@ -41,8 +41,26 @@ const useProjectOverview = (id: string, options: SWRConfiguration = {}) => { mutate(); }, [mutate]); + const overriddenData = useMemo(() => { + if (!data) return undefined; + return { + ...data, + environments: data.environments?.map((env) => { + return env.defaultStrategy + ? { + ...env, + defaultStrategy: { + ...env.defaultStrategy, + title: 'custom title override', + }, + } + : env; + }), + }; + }, [data]); + return { - project: data || fallbackProject, + project: overriddenData || fallbackProject, loading: !error && !data, error, refetch, From 2916ab3c4007d490cac68e5083699f0dc8d52cbe Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 21 Jul 2025 11:38:02 +0200 Subject: [PATCH 7/7] Propagate change request constraint id mapper --- .../useChangeRequest/useChangeRequest.ts | 38 +++++-------------- .../usePendingChangeRequests.ts | 30 ++++++++++++++- .../utils/addConstraintIdsToFeatureChange.ts | 31 +++++++++++++++ 3 files changed, 68 insertions(+), 31 deletions(-) create mode 100644 frontend/src/utils/addConstraintIdsToFeatureChange.ts diff --git a/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts b/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts index 4434f3372f..eafd015db6 100644 --- a/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts +++ b/frontend/src/hooks/api/getters/useChangeRequest/useChangeRequest.ts @@ -3,38 +3,11 @@ import { formatApiPath } from 'utils/formatPath'; import handleErrorResponses from '../httpErrorResponseHandler.js'; import type { ChangeRequestType, - IChangeRequestAddStrategy, IChangeRequestFeature, - IChangeRequestUpdateStrategy, IFeatureChange, } from 'component/changeRequest/changeRequest.types'; import { useMemo } from 'react'; -import { constraintId } from 'constants/constraintId.js'; -import { v4 as uuidv4 } from 'uuid'; - -const isAddStrategyChange = ( - change: IFeatureChange, -): change is IChangeRequestAddStrategy => change.action === 'addStrategy'; -const isUpdateStrategyChange = ( - change: IFeatureChange, -): change is IChangeRequestUpdateStrategy => change.action === 'updateStrategy'; - -const addConstraintIdsToFeatureChange = (change: IFeatureChange) => { - if (isAddStrategyChange(change) || isUpdateStrategyChange(change)) { - const { constraints, ...rest } = change.payload; - return { - ...change, - payload: { - ...rest, - constraints: constraints.map((constraint) => ({ - ...constraint, - [constraintId]: uuidv4(), - })), - }, - } as IFeatureChange; - } - return change; -}; +import { addConstraintIdsToFeatureChange } from 'utils/addConstraintIdsToFeatureChange.js'; export const useChangeRequest = (projectId: string, id: string) => { const { data, error, mutate } = useSWR( @@ -60,8 +33,15 @@ export const useChangeRequest = (projectId: string, id: string) => { ); }, [JSON.stringify(features)]); + const mappedData = data + ? { + ...dataProps, + features: featuresWithConstraintIds, + } + : data; + return { - data: { ...dataProps, features: featuresWithConstraintIds }, + data: mappedData, loading: !error && !data, refetchChangeRequest: () => mutate(), error, diff --git a/frontend/src/hooks/api/getters/usePendingChangeRequests/usePendingChangeRequests.ts b/frontend/src/hooks/api/getters/usePendingChangeRequests/usePendingChangeRequests.ts index 0c8334993a..40c98de2bd 100644 --- a/frontend/src/hooks/api/getters/usePendingChangeRequests/usePendingChangeRequests.ts +++ b/frontend/src/hooks/api/getters/usePendingChangeRequests/usePendingChangeRequests.ts @@ -1,7 +1,12 @@ import { formatApiPath } from 'utils/formatPath'; import handleErrorResponses from '../httpErrorResponseHandler.js'; -import type { ChangeRequestType } from 'component/changeRequest/changeRequest.types'; +import type { + ChangeRequestType, + IFeatureChange, +} from 'component/changeRequest/changeRequest.types'; import { useEnterpriseSWR } from '../useEnterpriseSWR/useEnterpriseSWR.js'; +import { useMemo } from 'react'; +import { addConstraintIdsToFeatureChange } from 'utils/addConstraintIdsToFeatureChange.js'; const fetcher = (path: string) => { return fetch(path) @@ -16,8 +21,29 @@ export const usePendingChangeRequests = (project: string) => { fetcher, ); + const mappedData: typeof data = useMemo( + () => + data?.map((changeRequest) => { + const { features, ...rest } = changeRequest || {}; + const featuresWithConstraintIds = + features?.map((feature) => { + const changes: IFeatureChange[] = feature.changes.map( + addConstraintIdsToFeatureChange, + ); + + return { + ...feature, + changes, + }; + }) ?? []; + + return { ...rest, features: featuresWithConstraintIds }; + }), + [JSON.stringify(data)], + ); + return { - data, + mappedData, loading: !error && !data, refetch: mutate, error, diff --git a/frontend/src/utils/addConstraintIdsToFeatureChange.ts b/frontend/src/utils/addConstraintIdsToFeatureChange.ts new file mode 100644 index 0000000000..6e398b754a --- /dev/null +++ b/frontend/src/utils/addConstraintIdsToFeatureChange.ts @@ -0,0 +1,31 @@ +import type { + IFeatureChange, + IChangeRequestAddStrategy, + IChangeRequestUpdateStrategy, +} from 'component/changeRequest/changeRequest.types'; +import { constraintId } from 'constants/constraintId'; +import { v4 as uuidv4 } from 'uuid'; + +const isAddStrategyChange = ( + change: IFeatureChange, +): change is IChangeRequestAddStrategy => change.action === 'addStrategy'; +const isUpdateStrategyChange = ( + change: IFeatureChange, +): change is IChangeRequestUpdateStrategy => change.action === 'updateStrategy'; + +export const addConstraintIdsToFeatureChange = (change: IFeatureChange) => { + if (isAddStrategyChange(change) || isUpdateStrategyChange(change)) { + const { constraints, ...rest } = change.payload; + return { + ...change, + payload: { + ...rest, + constraints: constraints.map((constraint) => ({ + ...constraint, + [constraintId]: uuidv4(), + })), + }, + } as IFeatureChange; + } + return change; +};