From 71c9a533806fb1701a131d6051a538847f4e24b7 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 18 Jul 2025 13:23:55 +0200 Subject: [PATCH] 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,