chore: prioritize milestone strategies in sorting (#9081)

https://linear.app/unleash/issue/2-3100/sorting-issues-with-milestone-strategies-and-regular-strategies

This PR ensures that milestone strategies are always prioritized and
evaluated before regular strategies. It ensures that the order displayed
in the UI matches the internal evaluation order.

src/lib/features/client-feature-toggles/client-feature-toggle-store.ts

@@ -22,6 +22,7 @@ import type EventEmitter from 'events';
 import FeatureToggleStore from '../feature-toggle/feature-toggle-store';
 import type { Db } from '../../db/db';
 import Raw = Knex.Raw;
+import { sortStrategies } from '../../util/sortStrategies';
 
 export interface IGetAllFeatures {
     featureQuery?: IFeatureToggleQuery;
@@ -95,6 +96,7 @@ export default class FeatureToggleClientStore
             'fs.parameters as parameters',
             'fs.constraints as constraints',
             'fs.sort_order as sort_order',
+            'fs.milestone_id as milestone_id',
             'fs.variants as strategy_variants',
             'segments.id as segment_id',
             'segments.constraints as segment_constraints',
@@ -244,16 +246,8 @@ export default class FeatureToggleClientStore
         const cleanedFeatures = features.map(({ strategies, ...rest }) => ({
             ...rest,
             strategies: strategies
-                ?.sort((strategy1, strategy2) => {
-                    if (
-                        typeof strategy1.sortOrder === 'number' &&
-                        typeof strategy2.sortOrder === 'number'
-                    ) {
-                        return strategy1.sortOrder - strategy2.sortOrder;
-                    }
-                    return 0;
-                })
-                .map(({ id, title, sortOrder, ...strategy }) => ({
+                ?.sort(sortStrategies)
+                .map(({ id, title, sortOrder, milestoneId, ...strategy }) => ({
                     ...strategy,
 
                     ...(isPlayground && title ? { title } : {}),
@@ -275,6 +269,7 @@ export default class FeatureToggleClientStore
             constraints: row.constraints || [],
             parameters: mapValues(row.parameters || {}, ensureStringValue),
             sortOrder: row.sort_order,
+            milestoneId: row.milestone_id,
         };
         strategy.variants = row.strategy_variants || [];
         return strategy;

src/lib/features/client-feature-toggles/delta/client-feature-toggle-delta-read-model.ts

@@ -17,6 +17,7 @@ import {
 } from '../../../internals';
 import metricsHelper from '../../../util/metrics-helper';
 import FeatureToggleStore from '../../feature-toggle/feature-toggle-store';
+import { sortStrategies } from '../../../util/sortStrategies';
 
 export default class ClientFeatureToggleDeltaReadModel
     implements IClientFeatureToggleDeltaReadModel
@@ -58,6 +59,7 @@ export default class ClientFeatureToggleDeltaReadModel
             'fs.parameters as parameters',
             'fs.constraints as constraints',
             'fs.sort_order as sort_order',
+            'fs.milestone_id as milestone_id',
             'fs.variants as strategy_variants',
             'segments.id as segment_id',
             'segments.constraints as segment_constraints',
@@ -175,16 +177,8 @@ export default class ClientFeatureToggleDeltaReadModel
         const cleanedFeatures = features.map(({ strategies, ...rest }) => ({
             ...rest,
             strategies: strategies
-                ?.sort((strategy1, strategy2) => {
-                    if (
-                        typeof strategy1.sortOrder === 'number' &&
-                        typeof strategy2.sortOrder === 'number'
-                    ) {
-                        return strategy1.sortOrder - strategy2.sortOrder;
-                    }
-                    return 0;
-                })
-                .map(({ id, title, sortOrder, ...strategy }) => ({
+                ?.sort(sortStrategies)
+                .map(({ id, title, sortOrder, milestoneId, ...strategy }) => ({
                     ...strategy,
                 })),
         }));
@@ -216,6 +210,7 @@ export default class ClientFeatureToggleDeltaReadModel
             constraints: row.constraints || [],
             parameters: mapValues(row.parameters || {}, ensureStringValue),
             sortOrder: row.sort_order,
+            milestoneId: row.milestone_id,
         };
         strategy.variants = row.strategy_variants || [];
         return strategy;

src/lib/features/export-import-toggles/export-import-service.ts

@@ -949,6 +949,7 @@ export default class ExportImportService
                     projectId,
                     environment,
                     strategyName,
+                    milestoneId,
                     ...rest
                 } = item;
                 return {

src/lib/features/feature-toggle/converters/feature-toggle-row-converter.ts

@@ -9,6 +9,7 @@ import type {
 } from '../../../types';
 
 import { mapValues, ensureStringValue } from '../../../util';
+import { sortStrategies } from '../../../util/sortStrategies';
 import type { FeatureConfigurationClient } from '../types/feature-toggle-strategies-store-type';
 
 export class FeatureToggleRowConverter {
@@ -106,6 +107,7 @@ export class FeatureToggleRowConverter {
             constraints: row.constraints || [],
             parameters: mapValues(row.parameters || {}, ensureStringValue),
             sortOrder: row.sort_order,
+            milestoneId: row.milestone_id,
             disabled: row.strategy_disabled,
             variants: row.strategy_variants || [],
         };
@@ -128,16 +130,8 @@ export class FeatureToggleRowConverter {
         Object.values(result).map(({ strategies, ...rest }) => ({
             ...rest,
             strategies: strategies
-                ?.sort((strategy1, strategy2) => {
-                    if (
-                        typeof strategy1.sortOrder === 'number' &&
-                        typeof strategy2.sortOrder === 'number'
-                    ) {
-                        return strategy1.sortOrder - strategy2.sortOrder;
-                    }
-                    return 0;
-                })
-                .map(({ title, sortOrder, ...strategy }) => ({
+                ?.sort(sortStrategies)
+                .map(({ title, sortOrder, milestoneId, ...strategy }) => ({
                     ...strategy,
                     ...(title ? { title } : {}),
                 })),

src/lib/features/feature-toggle/feature-toggle-controller.ts

@@ -856,6 +856,7 @@ export default class ProjectFeaturesController extends Controller {
                     projectId: project,
                     environment: environmentId,
                     createdAt,
+                    milestoneId,
                     ...rest
                 } = strategy;
                 return { ...rest, name: strategyName };

src/lib/features/feature-toggle/feature-toggle-service.ts

@@ -112,6 +112,7 @@ import type { IFeatureLifecycleReadModel } from '../feature-lifecycle/feature-li
 import type { ResourceLimitsSchema } from '../../openapi';
 import { throwExceedsLimitError } from '../../error/exceeds-limit-error';
 import type { Collaborator } from './types/feature-collaborators-read-model-type';
+import { sortStrategies } from '../../util/sortStrategies';
 
 interface IFeatureContext {
     featureName: string;
@@ -596,15 +597,7 @@ class FeatureToggleService {
                 environment,
             )
         )
-            .sort((strategy1, strategy2) => {
-                if (
-                    typeof strategy1.sortOrder === 'number' &&
-                    typeof strategy2.sortOrder === 'number'
-                ) {
-                    return strategy1.sortOrder - strategy2.sortOrder;
-                }
-                return 0;
-            })
+            .sort(sortStrategies)
             .map((strategy) => strategy.id);
 
         const eventPreData: StrategyIds = { strategyIds: existingOrder };
@@ -624,15 +617,7 @@ class FeatureToggleService {
                 environment,
             )
         )
-            .sort((strategy1, strategy2) => {
-                if (
-                    typeof strategy1.sortOrder === 'number' &&
-                    typeof strategy2.sortOrder === 'number'
-                ) {
-                    return strategy1.sortOrder - strategy2.sortOrder;
-                }
-                return 0;
-            })
+            .sort(sortStrategies)
             .map((strategy) => strategy.id);
 
         const eventData: StrategyIds = { strategyIds: newOrder };
@@ -1042,6 +1027,7 @@ class FeatureToggleService {
                     title: strat.title,
                     disabled: strat.disabled,
                     sortOrder: strat.sortOrder,
+                    milestoneId: strat.milestoneId,
                     segments,
                 });
             }

src/lib/features/feature-toggle/feature-toggle-store.ts

@@ -152,6 +152,7 @@ export default class FeatureToggleStore implements IFeatureToggleStore {
             'fs.parameters as parameters',
             'fs.constraints as constraints',
             'fs.sort_order as sort_order',
+            'fs.milestone_id as milestone_id',
             'fs.variants as strategy_variants',
             'segments.id as segment_id',
             'segments.constraints as segment_constraints',

src/lib/features/feature-toggle/feature-toggle-strategies-store.ts

@@ -62,6 +62,7 @@ interface IFeatureStrategiesTable {
     constraints: string;
     variants: string;
     sort_order: number;
+    milestone_id?: string;
     created_at?: Date;
     disabled?: boolean | null;
 }
@@ -86,6 +87,7 @@ function mapRow(row: IFeatureStrategiesTable): IFeatureStrategy {
         variants: (row.variants as unknown as IStrategyVariant[]) || [],
         createdAt: row.created_at,
         sortOrder: row.sort_order,
+        milestoneId: row.milestone_id,
         disabled: row.disabled,
     };
 }
@@ -326,6 +328,9 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
                 feature_name: featureName,
                 environment,
             })
+            .orderByRaw(
+                'CASE WHEN milestone_id IS NOT NULL THEN 0 ELSE 1 END ASC',
+            )
             .orderBy([
                 {
                     column: 'sort_order',

src/lib/types/model.ts

@@ -38,6 +38,7 @@ export interface IStrategyConfig {
     segments?: number[];
     parameters?: { [key: string]: string };
     sortOrder?: number;
+    milestoneId?: string;
     title?: string | null;
     disabled?: boolean | null;
 }
@@ -49,6 +50,7 @@ export interface IFeatureStrategy {
     strategyName: string;
     parameters: { [key: string]: string };
     sortOrder?: number;
+    milestoneId?: string;
     constraints: IConstraint[];
     variants?: IStrategyVariant[];
     createdAt?: Date;

src/lib/util/sortStrategies.test.ts

@@ -0,0 +1,68 @@
+import { sortStrategies } from './sortStrategies';
+
+describe('sortStrategies', () => {
+    it('should prioritize strategies with milestoneId over those without', () => {
+        const strategies = [
+            { sortOrder: 2 },
+            { milestoneId: 'm1', sortOrder: 1 },
+            { sortOrder: 1 },
+            { milestoneId: 'm2', sortOrder: 2 },
+        ];
+
+        const sorted = strategies.sort(sortStrategies);
+
+        expect(sorted).toEqual([
+            { milestoneId: 'm1', sortOrder: 1 },
+            { milestoneId: 'm2', sortOrder: 2 },
+            { sortOrder: 1 },
+            { sortOrder: 2 },
+        ]);
+    });
+
+    it('should sort by sortOrder when both have milestoneId', () => {
+        const strategies = [
+            { milestoneId: 'm1', sortOrder: 2 },
+            { milestoneId: 'm2', sortOrder: 1 },
+        ];
+
+        const sorted = strategies.sort(sortStrategies);
+
+        expect(sorted).toEqual([
+            { milestoneId: 'm2', sortOrder: 1 },
+            { milestoneId: 'm1', sortOrder: 2 },
+        ]);
+    });
+
+    it('should sort by sortOrder when neither has milestoneId', () => {
+        const strategies = [
+            { sortOrder: 3 },
+            { sortOrder: 1 },
+            { sortOrder: 2 },
+        ];
+
+        const sorted = strategies.sort(sortStrategies);
+
+        expect(sorted).toEqual([
+            { sortOrder: 1 },
+            { sortOrder: 2 },
+            { sortOrder: 3 },
+        ]);
+    });
+
+    it('should return 0 if both milestoneId and sortOrder are equal', () => {
+        const strategy1 = { milestoneId: 'm1', sortOrder: 1 };
+        const strategy2 = { milestoneId: 'm1', sortOrder: 1 };
+
+        const result = sortStrategies(strategy1, strategy2);
+
+        expect(result).toBe(0);
+    });
+
+    it('should handle cases where sortOrder is not a number', () => {
+        const strategies = [{ sortOrder: undefined }, { sortOrder: 1 }];
+
+        const sorted = strategies.sort(sortStrategies);
+
+        expect(sorted).toEqual([{ sortOrder: undefined }, { sortOrder: 1 }]);
+    });
+});

src/lib/util/sortStrategies.ts

@@ -0,0 +1,23 @@
+import type { IStrategyConfig } from '../types';
+
+type SortableStrategy = Pick<IStrategyConfig, 'milestoneId' | 'sortOrder'>;
+
+export const sortStrategies = (
+    strategy1: SortableStrategy,
+    strategy2: SortableStrategy,
+): number => {
+    if (strategy1.milestoneId && !strategy2.milestoneId) {
+        return -1;
+    }
+    if (!strategy1.milestoneId && strategy2.milestoneId) {
+        return 1;
+    }
+
+    if (
+        typeof strategy1.sortOrder === 'number' &&
+        typeof strategy2.sortOrder === 'number'
+    ) {
+        return strategy1.sortOrder - strategy2.sortOrder;
+    }
+    return 0;
+};