From 90915cfdd7bea9c24de9a15ece0d2caf2539e2b5 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 27 Nov 2023 11:23:10 +0100 Subject: [PATCH] Chore: add strategy sorting algorithm (#5406) This PR adds a strategy sorting algorithm to be used for the segment deletion dialog. It assumes that you have a list of existing strategies and a list of change request strategies. Based on the content of these two lists, it will create one unified list sorted after a number of criteria (as listed in the test). # Discussion point: This impl does the sorting on the front end, but could we do it on the back end? Instead of adding a new property to the segment data, could we simply fold the change request strategies in with the existing segment strategies and return it using the old property? If the only place we do that is in this view, then that might be a good suggestion. Response: I'll leave this in the front end for now. The reason is that we can't add change request strategies to the existing `strategies` property of the API payload without it being a breaking change. The OpenAPI schema says that `id` is a required field on a strategy, and that field doesn't exist on strategies that have only been added in change requests, but not yet applied. --- .../sort-strategies.test.ts | 137 ++++++++++++++++++ .../sort-strategies.ts | 92 ++++++++++++ 2 files changed, 229 insertions(+) create mode 100644 frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.test.ts create mode 100644 frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.ts diff --git a/frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.test.ts b/frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.test.ts new file mode 100644 index 0000000000..c8b1be72ca --- /dev/null +++ b/frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.test.ts @@ -0,0 +1,137 @@ +import { sortStrategiesByFeature } from './sort-strategies'; + +describe('sorting strategies by feature', () => { + test('strategies with the same id are sorted: existing first, then change requests', () => { + const strategies = [{ id: 'a', featureName: 'feature1' }]; + const changeRequestStrategies = [ + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + const expected = [ + { id: 'a', featureName: 'feature1' }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + expect( + sortStrategiesByFeature(strategies, changeRequestStrategies), + ).toStrictEqual(expected); + }); + + test('the same strategy used in multiple change requests is sorted by change request id', () => { + const changeRequestStrategies = [ + { id: 'a', featureName: 'feature1', changeRequest: { id: 2 } }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + const expected = [ + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 2 } }, + ]; + + expect( + sortStrategiesByFeature([], changeRequestStrategies), + ).toStrictEqual(expected); + }); + + test('strategies are sorted by id, with change requests strategies being listed before existing strategies if their ids would indicate that', () => { + const strategies = [{ id: 'b', featureName: 'feature1' }]; + const changeRequestStrategies = [ + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + const expected = [ + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { id: 'b', featureName: 'feature1' }, + ]; + + expect( + sortStrategiesByFeature(strategies, changeRequestStrategies), + ).toStrictEqual(expected); + }); + + test('strategies without ids (new strategies) are sorted by change request id', () => { + const changeRequestStrategies = [ + { featureName: 'feature1', changeRequest: { id: 2 } }, + { featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + const expected = [ + { featureName: 'feature1', changeRequest: { id: 1 } }, + { featureName: 'feature1', changeRequest: { id: 2 } }, + ]; + + expect( + sortStrategiesByFeature([], changeRequestStrategies), + ).toStrictEqual(expected); + }); + + test('if new strategies have the same change request id, they should be listed in the same order as in the input', () => { + const changeRequestStrategies = [ + { key: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { key: 'b', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + const expected = [ + { key: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { key: 'b', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + expect( + sortStrategiesByFeature([], changeRequestStrategies), + ).toStrictEqual(expected); + }); + + test('all the various sorts work together', () => { + const strategies = [ + { id: 'a', featureName: 'feature1' }, + { id: 'b', featureName: 'feature1' }, + { id: 'd', featureName: 'feature1' }, + ]; + const changeRequestStrategies = [ + { id: 'a', featureName: 'feature1', changeRequest: { id: 2 } }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { id: 'c', featureName: 'feature1', changeRequest: { id: 1 } }, + { key: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { key: 'b', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + const expected = [ + { id: 'a', featureName: 'feature1' }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 2 } }, + { id: 'b', featureName: 'feature1' }, + { id: 'c', featureName: 'feature1', changeRequest: { id: 1 } }, + { id: 'd', featureName: 'feature1' }, + { key: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { key: 'b', featureName: 'feature1', changeRequest: { id: 1 } }, + ]; + + expect( + sortStrategiesByFeature(strategies, changeRequestStrategies), + ).toStrictEqual(expected); + }); + + test('when multiple flag names are provided, the list will be sorted on flag name first', () => { + const strategies = [ + { id: 'b', featureName: 'feature2' }, + { id: 'a', featureName: 'feature1' }, + ]; + const changeRequestStrategies = [ + { id: 'a', featureName: 'feature1', changeRequest: { id: 2 } }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { id: 'b', featureName: 'feature2', changeRequest: { id: 2 } }, + ]; + + const expected = [ + { id: 'a', featureName: 'feature1' }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 1 } }, + { id: 'a', featureName: 'feature1', changeRequest: { id: 2 } }, + { id: 'b', featureName: 'feature2' }, + { id: 'b', featureName: 'feature2', changeRequest: { id: 2 } }, + ]; + + expect( + sortStrategiesByFeature(strategies, changeRequestStrategies), + ).toStrictEqual(expected); + }); +}); diff --git a/frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.ts b/frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.ts new file mode 100644 index 0000000000..9fbc5de0e9 --- /dev/null +++ b/frontend/src/component/segments/SegmentDelete/SegmentDeleteUsedSegment/sort-strategies.ts @@ -0,0 +1,92 @@ +export const sortStrategiesByFeature = < + ExistingStrategy extends { id: string; featureName?: string }, + UpdatedStrategy extends { + id: string; + featureName: string; + changeRequest: { id: number }; + }, + NewStrategy extends { + featureName: string; + changeRequest: { id: number }; + }, +>( + strategies: ExistingStrategy[], + changeRequestStrategies: (UpdatedStrategy | NewStrategy)[], +): (ExistingStrategy | UpdatedStrategy | NewStrategy)[] => { + const strategiesByFlag = [...strategies, ...changeRequestStrategies].reduce( + (acc, strategy) => { + if (!strategy.featureName) { + // this shouldn't ever happen here, but because the + // type system allows it to, we need to handle it. If + // a strategy doesn't have a feature name, discard it. + return acc; + } + const registered = acc[strategy.featureName]; + if (registered) { + registered.push(strategy); + } else { + acc[strategy.featureName] = [strategy]; + } + + return acc; + }, + {} as { + [flagName: string]: ( + | ExistingStrategy + | UpdatedStrategy + | NewStrategy + )[]; + }, + ); + + const flagToStrategiesList = Object.entries(strategiesByFlag); + + flagToStrategiesList.sort(([flagA], [flagB]) => { + return flagA.localeCompare(flagB); + }); + + return flagToStrategiesList.flatMap(([_, strategies]) => { + const isExistingStrategy = ( + strategy: ExistingStrategy | UpdatedStrategy | NewStrategy, + ): strategy is ExistingStrategy | UpdatedStrategy => 'id' in strategy; + + const isChangeRequest = ( + strategy: ExistingStrategy | UpdatedStrategy | NewStrategy, + ): strategy is UpdatedStrategy | NewStrategy => + 'changeRequest' in strategy; + + strategies.sort((a, b) => { + if (isExistingStrategy(a) && isExistingStrategy(b)) { + // both strategies exist already + const idComp = a.id.localeCompare(b.id); + if (idComp === 0) { + const crA = isChangeRequest(a); + const crB = isChangeRequest(b); + + if (crA && !crB) { + return 1; + } else if (!crA && crB) { + return -1; + } else if (crA && crB) { + return a.changeRequest.id - b.changeRequest.id; + } else { + return 0; + } + } else { + return idComp; + } + } else if (isExistingStrategy(a)) { + // strategy b is new + return -1; + } else if (isExistingStrategy(b)) { + // strategy a is new + return 1; + } else { + // both strategies are new + return a.changeRequest.id - b.changeRequest.id; + } + }); + + return strategies; + }); +};