From ef3ffc4d944a383bc296fa2bd2b5fdda289f8219 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 18 Jun 2025 13:47:37 +0200 Subject: [PATCH] fix: Order properties correctly when mapping from Editable Constraint to IConstraint (#10163) Prevents the property order from changing when constraints are set from the editable constraint component. When we render out the API command, we don't specify the order of properties in the objects, which means that it can change dramatically, which can be a little jarring. As it is right now, it first renders in one order when you open the strategy form: image And when you navigate to the targeting section of the strategy form, it changes to: image This also applies to constraints in segments. With this change, the order will remain the same before and after. Additionally, there's some extra tests around constraint ids being kept the same and being set if it doesn't exist. ## Further work This came about as part of the issue we had with constraint editing in segments being broken as of now. As part of that, I would like to make some further improvements (such as making the ID required when you use a list of editable constraints), but that will be in a follow-up. There are some complications that might not make it a viable option, sadly. We could also try to stabilize the property order in the API rendering methods (which I think might be a good idea), but because there's multiple different ones, this seems to be a faster solution. --- .../editable-constraint-type.test.ts | 44 +++++++++++++++++++ .../editable-constraint-type.ts | 30 ++++++++++--- .../useEditableConstraint.test.tsx | 12 ++--- 3 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.test.ts 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 new file mode 100644 index 0000000000..234e6955ed --- /dev/null +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/editable-constraint-type.test.ts @@ -0,0 +1,44 @@ +import { createEmptyConstraint } from 'utils/createEmptyConstraint'; +import { fromIConstraint, toIConstraint } from './editable-constraint-type.ts'; +import { constraintId } from 'constants/constraintId'; + +test('mapping to and from retains the constraint id', () => { + const constraint = createEmptyConstraint('context'); + + expect(toIConstraint(fromIConstraint(constraint))[constraintId]).toEqual( + constraint[constraintId], + ); +}); + +test('mapping to an editable constraint adds a constraint id if there is none', () => { + const constraint = 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'); + expect(constraint).toHaveProperty('value'); + + const transformed = toIConstraint(fromIConstraint(constraint)); + expect(transformed).not.toHaveProperty('value'); +}); + +test('mapping to constraint returns properties in expected order', () => { + const constraint = createEmptyConstraint('context'); + const transformed = toIConstraint(fromIConstraint(constraint)); + + expect(Object.keys(transformed)).toEqual([ + 'values', + 'inverted', + 'operator', + 'contextName', + 'caseInsensitive', + ]); +}); 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 c55c546cfb..a0e2498346 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,3 +1,4 @@ +import { constraintId } from 'constants/constraintId'; import { type DateOperator, isDateOperator, @@ -10,6 +11,7 @@ import { isSemVerOperator, } from 'constants/operators'; import type { IConstraint } from 'interfaces/strategy'; +import { v4 as uuidv4 } from 'uuid'; type EditableConstraintBase = Omit< IConstraint, @@ -72,12 +74,14 @@ 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), @@ -86,13 +90,29 @@ export const fromIConstraint = ( }; export const toIConstraint = (constraint: EditableConstraint): IConstraint => { + const { + inverted, + operator, + contextName, + caseInsensitive, + [constraintId]: id, + } = constraint; + const baseValues = { + inverted, + operator, + contextName, + caseInsensitive, + [constraintId]: id, + }; if ('value' in constraint) { - return constraint; - } else { - const { values, ...rest } = constraint; return { - ...rest, - values: Array.from(values), + value: constraint.value, + ...baseValues, + }; + } else { + return { + values: Array.from(constraint.values), + ...baseValues, }; } }; 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 fa2bab70f2..bdc671ea45 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 @@ -40,11 +40,13 @@ test('calls onUpdate with new state', async () => { // gets called by useEffect, so we need to wait for the next render. await waitFor(() => { - expect(onUpdate).toHaveBeenCalledWith({ - contextName: 'context-field', - operator: IN, - values: [], - }); + expect(onUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + contextName: 'context-field', + operator: IN, + values: [], + }), + ); }); });