fix: sort strategy parameters payload (#1218)

* refactor: improve useStrategy fetch hook

* fix: sort strategy parameters payload

* refactor: move React import to the top

* refactor: fix refetchStrategy name

frontend/src/component/feature/FeatureStrategy/FeatureStrategyCreate/FeatureStrategyCreate.tsx

@@ -16,7 +16,6 @@ import {
     createStrategyPayload,
     featureStrategyDocsLinkLabel,
 } from '../FeatureStrategyEdit/FeatureStrategyEdit';
-import { useStrategies } from 'hooks/api/getters/useStrategies/useStrategies';
 import { CREATE_FEATURE_STRATEGY } from 'component/providers/AccessProvider/permissions';
 import { ISegment } from 'interfaces/segment';
 import { useSegmentsApi } from 'hooks/api/actions/useSegmentsApi/useSegmentsApi';
@@ -24,6 +23,7 @@ import { formatStrategyName } from 'utils/strategyNames';
 import { useFeatureImmutable } from 'hooks/api/getters/useFeature/useFeatureImmutable';
 import { useFormErrors } from 'hooks/useFormErrors';
 import { createFeatureStrategy } from 'utils/createFeatureStrategy';
+import { useStrategy } from 'hooks/api/getters/useStrategy/useStrategy';
 
 export const FeatureStrategyCreate = () => {
     const projectId = useRequiredPathParam('projectId');
@@ -32,7 +32,7 @@ export const FeatureStrategyCreate = () => {
     const strategyName = useRequiredQueryParam('strategyName');
     const [strategy, setStrategy] = useState<Partial<IFeatureStrategy>>({});
     const [segments, setSegments] = useState<ISegment[]>([]);
-    const { strategies } = useStrategies();
+    const { strategyDefinition } = useStrategy(strategyName);
     const errors = useFormErrors();
 
     const { addStrategyToFeature, loading } = useFeatureStrategyApi();
@@ -47,10 +47,6 @@ export const FeatureStrategyCreate = () => {
         featureId
     );
 
-    const strategyDefinition = strategies.find(strategy => {
-        return strategy.name === strategyName;
-    });
-
     useEffect(() => {
         if (strategyDefinition) {
             setStrategy(createFeatureStrategy(featureId, strategyDefinition));

frontend/src/component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit.test.tsx

@@ -1,20 +1,53 @@
 import { formatUpdateStrategyApiCode } from 'component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit';
+import { IFeatureStrategy, IStrategy } from 'interfaces/strategy';
 
 test('formatUpdateStrategyApiCode', () => {
+    const strategy: IFeatureStrategy = {
+        id: 'a',
+        name: 'b',
+        parameters: {
+            c: 1,
+            b: 2,
+            a: 3,
+        },
+        constraints: [],
+    };
+
+    const strategyDefinition: IStrategy = {
+        name: 'c',
+        displayName: 'd',
+        description: 'e',
+        editable: false,
+        deprecated: false,
+        parameters: [
+            { name: 'a', description: '', type: '', required: false },
+            { name: 'b', description: '', type: '', required: false },
+            { name: 'c', description: '', type: '', required: false },
+        ],
+    };
+
     expect(
         formatUpdateStrategyApiCode(
             'projectId',
             'featureId',
             'environmentId',
-            { id: 'strategyId' },
+            strategy,
+            strategyDefinition,
             'unleashUrl'
         )
     ).toMatchInlineSnapshot(`
-      "curl --location --request PUT 'unleashUrl/api/admin/projects/projectId/features/featureId/environments/environmentId/strategies/strategyId' \\\\
+      "curl --location --request PUT 'unleashUrl/api/admin/projects/projectId/features/featureId/environments/environmentId/strategies/a' \\\\
           --header 'Authorization: INSERT_API_KEY' \\\\
           --header 'Content-Type: application/json' \\\\
           --data-raw '{
-        \\"id\\": \\"strategyId\\"
+        \\"id\\": \\"a\\",
+        \\"name\\": \\"b\\",
+        \\"parameters\\": {
+          \\"a\\": 3,
+          \\"b\\": 2,
+          \\"c\\": 1
+        },
+        \\"constraints\\": []
       }'"
     `);
 });

frontend/src/component/feature/FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit.tsx

@@ -8,7 +8,11 @@ import useFeatureStrategyApi from 'hooks/api/actions/useFeatureStrategyApi/useFe
 import { formatUnknownError } from 'utils/formatUnknownError';
 import { useNavigate } from 'react-router-dom';
 import useToast from 'hooks/useToast';
-import { IFeatureStrategy, IFeatureStrategyPayload } from 'interfaces/strategy';
+import {
+    IFeatureStrategy,
+    IFeatureStrategyPayload,
+    IStrategy,
+} from 'interfaces/strategy';
 import { UPDATE_FEATURE_STRATEGY } from 'component/providers/AccessProvider/permissions';
 import { ISegment } from 'interfaces/segment';
 import { useSegmentsApi } from 'hooks/api/actions/useSegmentsApi/useSegmentsApi';
@@ -16,6 +20,8 @@ import { useSegments } from 'hooks/api/getters/useSegments/useSegments';
 import { formatStrategyName } from 'utils/strategyNames';
 import { useFeatureImmutable } from 'hooks/api/getters/useFeature/useFeatureImmutable';
 import { useFormErrors } from 'hooks/useFormErrors';
+import { useStrategy } from 'hooks/api/getters/useStrategy/useStrategy';
+import { sortStrategyParameters } from 'utils/sortStrategyParameters';
 
 export const FeatureStrategyEdit = () => {
     const projectId = useRequiredPathParam('projectId');
@@ -27,6 +33,7 @@ export const FeatureStrategyEdit = () => {
     const [segments, setSegments] = useState<ISegment[]>([]);
     const { updateStrategyOnFeature, loading } = useFeatureStrategyApi();
     const { setStrategySegments } = useSegmentsApi();
+    const { strategyDefinition } = useStrategy(strategy.name);
     const { setToastData, setToastApiError } = useToast();
     const errors = useFormErrors();
     const { uiConfig } = useUiConfig();
@@ -85,8 +92,7 @@ export const FeatureStrategyEdit = () => {
         }
     };
 
-    // Wait until the strategy has loaded before showing the form.
-    if (!strategy.id) {
+    if (!strategy.id || !strategyDefinition) {
         return null;
     }
 
@@ -103,6 +109,7 @@ export const FeatureStrategyEdit = () => {
                     featureId,
                     environmentId,
                     strategy,
+                    strategyDefinition,
                     unleashUrl
                 )
             }
@@ -156,14 +163,25 @@ export const formatUpdateStrategyApiCode = (
     featureId: string,
     environmentId: string,
     strategy: Partial<IFeatureStrategy>,
+    strategyDefinition: IStrategy,
     unleashUrl?: string
 ): string => {
     if (!unleashUrl) {
         return '';
     }
 
+    // Sort the strategy parameters payload so that they match
+    // the order of the input fields in the form, for usability.
+    const sortedStrategy = {
+        ...strategy,
+        parameters: sortStrategyParameters(
+            strategy.parameters ?? {},
+            strategyDefinition
+        ),
+    };
+
     const url = `${unleashUrl}/api/admin/projects/${projectId}/features/${featureId}/environments/${environmentId}/strategies/${strategy.id}`;
-    const payload = JSON.stringify(strategy, undefined, 2);
+    const payload = JSON.stringify(sortedStrategy, undefined, 2);
 
     return `curl --location --request PUT '${url}' \\
     --header 'Authorization: INSERT_API_KEY' \\

frontend/src/component/feature/FeatureStrategy/FeatureStrategyForm/FeatureStrategyForm.tsx

@@ -25,8 +25,8 @@ import PermissionButton from 'component/common/PermissionButton/PermissionButton
 import { FeatureStrategySegment } from 'component/feature/FeatureStrategy/FeatureStrategySegment/FeatureStrategySegment';
 import { ISegment } from 'interfaces/segment';
 import { IFormErrors } from 'hooks/useFormErrors';
-import { useStrategies } from 'hooks/api/getters/useStrategies/useStrategies';
 import { validateParameterValue } from 'utils/validateParameterValue';
+import { useStrategy } from 'hooks/api/getters/useStrategy/useStrategy';
 
 interface IFeatureStrategyFormProps {
     feature: IFeatureToggle;
@@ -60,13 +60,9 @@ export const FeatureStrategyForm = ({
     const hasValidConstraints = useConstraintsValidation(strategy.constraints);
     const enableProdGuard = useFeatureStrategyProdGuard(feature, environmentId);
     const { hasAccess } = useContext(AccessContext);
-    const { strategies } = useStrategies();
+    const { strategyDefinition } = useStrategy(strategy?.name);
     const navigate = useNavigate();
 
-    const strategyDefinition = strategies.find(definition => {
-        return definition.name === strategy.name;
-    });
-
     const {
         uiConfig,
         error: uiConfigError,

frontend/src/component/strategies/EditStrategy/EditStrategy.tsx

@@ -1,3 +1,4 @@
+import React from 'react';
 import { useNavigate } from 'react-router-dom';
 import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
 import useToast from 'hooks/useToast';
@@ -8,7 +9,7 @@ import { UPDATE_STRATEGY } from 'component/providers/AccessProvider/permissions'
 import useStrategiesApi from 'hooks/api/actions/useStrategiesApi/useStrategiesApi';
 import { useStrategies } from 'hooks/api/getters/useStrategies/useStrategies';
 import { formatUnknownError } from 'utils/formatUnknownError';
-import useStrategy from 'hooks/api/getters/useStrategy/useStrategy';
+import { useStrategy } from 'hooks/api/getters/useStrategy/useStrategy';
 import { UpdateButton } from 'component/common/UpdateButton/UpdateButton';
 import { useRequiredPathParam } from 'hooks/useRequiredPathParam';
 import { GO_BACK } from 'constants/navigate';
@@ -18,7 +19,7 @@ export const EditStrategy = () => {
     const { uiConfig } = useUiConfig();
     const navigate = useNavigate();
     const name = useRequiredPathParam('name');
-    const { strategy } = useStrategy(name);
+    const { strategyDefinition } = useStrategy(name);
     const {
         strategyName,
         strategyDesc,
@@ -32,9 +33,9 @@ export const EditStrategy = () => {
         setErrors,
         errors,
     } = useStrategyForm(
-        strategy?.name,
-        strategy?.description,
-        strategy?.parameters
+        strategyDefinition?.name,
+        strategyDefinition?.description,
+        strategyDefinition?.parameters
     );
     const { updateStrategy, loading } = useStrategiesApi();
     const { refetchStrategies } = useStrategies();

frontend/src/hooks/api/getters/useStrategy/defaultStrategy.ts

@@ -1,10 +0,0 @@
-import { IStrategy } from 'interfaces/strategy';
-
-export const defaultStrategy: IStrategy = {
-    name: '',
-    description: '',
-    displayName: '',
-    editable: false,
-    deprecated: false,
-    parameters: [],
-};

frontend/src/hooks/api/getters/useStrategy/useStrategy.ts

@@ -1,30 +1,40 @@
-import useSWR, { mutate, SWRConfiguration } from 'swr';
+import useSWR from 'swr';
+import { useCallback } from 'react';
 import { formatApiPath } from 'utils/formatPath';
+import { IStrategy } from 'interfaces/strategy';
 import handleErrorResponses from '../httpErrorResponseHandler';
-import { defaultStrategy } from './defaultStrategy';
 
-const useStrategy = (strategyName: string, options: SWRConfiguration = {}) => {
-    const STRATEGY_CACHE_KEY = `api/admin/strategies/${strategyName}`;
-    const path = formatApiPath(STRATEGY_CACHE_KEY);
+interface IUseStrategyOutput {
+    strategyDefinition?: IStrategy;
+    refetchStrategy: () => void;
+    loading: boolean;
+    error?: Error;
+}
 
-    const fetcher = () => {
-        return fetch(path)
-            .then(handleErrorResponses(`${strategyName} strategy`))
-            .then(res => res.json());
-    };
+export const useStrategy = (
+    strategyName: string | undefined
+): IUseStrategyOutput => {
+    const { data, error, mutate } = useSWR(
+        strategyName
+            ? formatApiPath(`api/admin/strategies/${strategyName}`)
+            : null, // Don't fetch until we have a strategyName.
+        fetcher
+    );
 
-    const { data, error } = useSWR(STRATEGY_CACHE_KEY, fetcher, options);
-
-    const refetchStrategy = () => {
-        mutate(STRATEGY_CACHE_KEY);
-    };
+    const refetchStrategy = useCallback(() => {
+        mutate().catch(console.warn);
+    }, [mutate]);
 
     return {
-        strategy: data || defaultStrategy,
-        error,
-        loading: !error && !data,
+        strategyDefinition: data,
         refetchStrategy,
+        loading: !error && !data,
+        error,
     };
 };
 
-export default useStrategy;
+const fetcher = (path: string): Promise<IStrategy> => {
+    return fetch(path)
+        .then(handleErrorResponses('Strategy'))
+        .then(res => res.json());
+};

frontend/src/utils/sortStrategyParameters.test.ts

@@ -0,0 +1,29 @@
+import { sortStrategyParameters } from 'utils/sortStrategyParameters';
+
+test('sortStrategyParameters', () => {
+    expect(
+        sortStrategyParameters(
+            {
+                c: 1,
+                b: 2,
+                a: 3,
+            },
+            {
+                name: '',
+                displayName: '',
+                description: '',
+                editable: false,
+                deprecated: false,
+                parameters: [
+                    { name: 'a', description: '', type: '', required: false },
+                    { name: 'b', description: '', type: '', required: false },
+                    { name: 'c', description: '', type: '', required: false },
+                ],
+            }
+        )
+    ).toEqual({
+        a: 3,
+        b: 2,
+        c: 1,
+    });
+});

frontend/src/utils/sortStrategyParameters.ts

@@ -0,0 +1,24 @@
+import {
+    IFeatureStrategyParameters,
+    IStrategy,
+    IFeatureStrategy,
+} from 'interfaces/strategy';
+
+// Sort the keys in a parameters payload object by the
+// order of the parameters in the strategy definition.
+export const sortStrategyParameters = (
+    parameters: IFeatureStrategyParameters,
+    strategyDefinition: IStrategy
+): Partial<IFeatureStrategy> => {
+    const sortedParameterNames = strategyDefinition.parameters.map(
+        parameter => parameter.name
+    );
+
+    return Object.fromEntries(
+        Object.entries(parameters).sort(
+            (a, b) =>
+                sortedParameterNames.indexOf(a[0]) -
+                sortedParameterNames.indexOf(b[0])
+        )
+    );
+};