From edaea80f0c887b9de44138dd9ee7d7cbaa2d3241 Mon Sep 17 00:00:00 2001 From: Tymoteusz Czech <2625371+Tymek@users.noreply.github.com> Date: Tue, 9 Sep 2025 09:22:43 +0200 Subject: [PATCH] refactor: new endpoint for global impact metrics saving (#10631) Use `impact-metrics/config` for saving global charts. --- .../FeatureMetrics/FeatureImpactMetrics.tsx | 2 +- .../impact-metrics/GridLayoutWrapper.tsx | 4 +- .../impact-metrics/ImpactMetrics.tsx | 25 +--- .../hooks/useImpactMetricsState.test.tsx | 127 ++++++------------ .../hooks/useImpactMetricsState.ts | 120 +++++++---------- .../useImpactMetricsApi.ts | 4 +- .../useImpactMetricsSettingsApi.ts | 32 ----- .../useImpactMetricsConfig.ts | 21 +++ .../useImpactMetricsSettings.ts | 18 --- 9 files changed, 112 insertions(+), 241 deletions(-) rename frontend/src/hooks/api/actions/{useImpactMetricsSettingsApi => useImpactMetricsApi}/useImpactMetricsApi.ts (92%) delete mode 100644 frontend/src/hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsSettingsApi.ts create mode 100644 frontend/src/hooks/api/getters/useImpactMetricsConfig/useImpactMetricsConfig.ts delete mode 100644 frontend/src/hooks/api/getters/useImpactMetricsSettings/useImpactMetricsSettings.ts diff --git a/frontend/src/component/feature/FeatureView/FeatureMetrics/FeatureImpactMetrics.tsx b/frontend/src/component/feature/FeatureView/FeatureMetrics/FeatureImpactMetrics.tsx index 07b70ff89f..54798557a7 100644 --- a/frontend/src/component/feature/FeatureView/FeatureMetrics/FeatureImpactMetrics.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureMetrics/FeatureImpactMetrics.tsx @@ -5,7 +5,7 @@ import Add from '@mui/icons-material/Add'; import { useImpactMetricsMetadata } from 'hooks/api/getters/useImpactMetricsMetadata/useImpactMetricsMetadata.ts'; import { type FC, useMemo, useState } from 'react'; import { ChartConfigModal } from '../../../impact-metrics/ChartConfigModal/ChartConfigModal.tsx'; -import { useImpactMetricsApi } from 'hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsApi.ts'; +import { useImpactMetricsApi } from 'hooks/api/actions/useImpactMetricsApi/useImpactMetricsApi.ts'; import { useRequiredPathParam } from 'hooks/useRequiredPathParam.ts'; import { useFeatureImpactMetrics } from 'hooks/api/getters/useFeatureImpactMetrics/useFeatureImpactMetrics.ts'; import { ChartItem } from '../../../impact-metrics/ChartItem.tsx'; diff --git a/frontend/src/component/impact-metrics/GridLayoutWrapper.tsx b/frontend/src/component/impact-metrics/GridLayoutWrapper.tsx index 8b1f3ef4ca..4f25296a63 100644 --- a/frontend/src/component/impact-metrics/GridLayoutWrapper.tsx +++ b/frontend/src/component/impact-metrics/GridLayoutWrapper.tsx @@ -134,8 +134,8 @@ export const GridLayoutWrapper: FC = ({ Number.parseInt(theme.spacing(2)), ]} containerPadding={[0, 0]} - isDraggable={!isMobileBreakpoint} - isResizable={!isMobileBreakpoint} + isDraggable={false} + isResizable={false} onLayoutChange={handleLayoutChange} resizeHandles={['se']} draggableHandle='.grid-item-drag-handle' diff --git a/frontend/src/component/impact-metrics/ImpactMetrics.tsx b/frontend/src/component/impact-metrics/ImpactMetrics.tsx index 4e7c2eba6a..6eea82453c 100644 --- a/frontend/src/component/impact-metrics/ImpactMetrics.tsx +++ b/frontend/src/component/impact-metrics/ImpactMetrics.tsx @@ -2,14 +2,13 @@ import type { FC } from 'react'; import { useMemo, useState, useCallback } from 'react'; import { Typography, Button, Paper, styled, Box } from '@mui/material'; import Add from '@mui/icons-material/Add'; -import DragHandle from '@mui/icons-material/DragHandle'; import { PageHeader } from 'component/common/PageHeader/PageHeader.tsx'; import { useImpactMetricsMetadata } from 'hooks/api/getters/useImpactMetricsMetadata/useImpactMetricsMetadata'; import { ChartConfigModal } from './ChartConfigModal/ChartConfigModal.tsx'; import { ChartItem } from './ChartItem.tsx'; import { GridLayoutWrapper, type GridItem } from './GridLayoutWrapper.tsx'; import { useImpactMetricsState } from './hooks/useImpactMetricsState.ts'; -import type { ChartConfig, LayoutItem } from './types.ts'; +import type { ChartConfig } from './types.ts'; import useToast from 'hooks/useToast'; import { formatUnknownError } from 'utils/formatUnknownError'; import PermissionButton from 'component/common/PermissionButton/PermissionButton.tsx'; @@ -49,7 +48,6 @@ export const ImpactMetrics: FC = () => { addChart, updateChart, deleteChart, - updateLayout, } = useImpactMetricsState(); const { @@ -91,17 +89,6 @@ export const ImpactMetrics: FC = () => { } }; - const handleLayoutChange = useCallback( - async (layout: any[]) => { - try { - await updateLayout(layout as LayoutItem[]); - } catch (error) { - setToastApiError(formatUnknownError(error)); - } - }, - [updateLayout, setToastApiError], - ); - const handleDeleteChart = useCallback( async (id: string) => { try { @@ -126,11 +113,6 @@ export const ImpactMetrics: FC = () => { config={config} onEdit={handleEditChart} onDelete={handleDeleteChart} - dragHandle={ - - - - } /> ), w: existingLayout?.w ?? 6, @@ -194,10 +176,7 @@ export const ImpactMetrics: FC = () => { ) : charts.length > 0 ? ( - + ) : null} - updateChart(charts[0].id, { title: 'Updated Chart' }) + updateChart(charts[0].id, { + metricName: charts[0].metricName, + timeRange: charts[0].timeRange, + yAxisMin: charts[0].yAxisMin, + aggregationMode: charts[0].aggregationMode, + labelSelectors: charts[0].labelSelectors, + title: 'Updated Chart', + }) } > Update Chart @@ -110,11 +117,9 @@ describe('useImpactMetricsState', () => { }); it('loads settings from API', async () => { - testServerRoute( - server, - '/api/admin/impact-metrics/settings', - mockSettings, - ); + testServerRoute(server, '/api/admin/impact-metrics/config', { + configs: mockSettings.charts, + }); render(); @@ -127,11 +132,9 @@ describe('useImpactMetricsState', () => { }); it('handles empty settings', async () => { - testServerRoute( - server, - '/api/admin/impact-metrics/settings', - emptySettings, - ); + testServerRoute(server, '/api/admin/impact-metrics/config', { + configs: emptySettings.charts, + }); render(); @@ -146,7 +149,7 @@ describe('useImpactMetricsState', () => { it('handles API errors', async () => { testServerRoute( server, - '/api/admin/impact-metrics/settings', + '/api/admin/impact-metrics/config', { message: 'Server error' }, 'get', 500, @@ -160,11 +163,9 @@ describe('useImpactMetricsState', () => { }); it('adds a chart successfully', async () => { - testServerRoute( - server, - '/api/admin/impact-metrics/settings', - emptySettings, - ); + testServerRoute(server, '/api/admin/impact-metrics/config', { + configs: emptySettings.charts, + }); render(); @@ -174,65 +175,27 @@ describe('useImpactMetricsState', () => { testServerRoute( server, - '/api/admin/impact-metrics/settings', - { - charts: [ - { - id: 'new-chart-id', - metricName: 'test-series', - timeRange: 'day', - yAxisMin: 'zero', - mode: 'count', - labelSelectors: {}, - title: 'Test Chart', - }, - ], - layout: [ - { - i: 'new-chart-id', - x: 0, - y: 0, - w: 6, - h: 4, - minW: 4, - minH: 2, - maxW: 12, - maxH: 8, - }, - ], - }, - 'put', - 200, + '/api/admin/impact-metrics/config', + 'Created', + 'post', + 201, ); testServerRoute( server, - '/api/admin/impact-metrics/settings', + '/api/admin/impact-metrics/config', { - charts: [ + configs: [ { id: 'new-chart-id', metricName: 'test-series', timeRange: 'day', yAxisMin: 'zero', - mode: 'count', + aggregationMode: 'count', labelSelectors: {}, title: 'Test Chart', }, ], - layout: [ - { - i: 'new-chart-id', - x: 0, - y: 0, - w: 6, - h: 4, - minW: 4, - minH: 2, - maxW: 12, - maxH: 8, - }, - ], }, 'get', 200, @@ -252,25 +215,15 @@ describe('useImpactMetricsState', () => { }); it('updates a chart successfully', async () => { - testServerRoute( - server, - '/api/admin/impact-metrics/settings', - mockSettings, - ); + testServerRoute(server, '/api/admin/impact-metrics/config', { + configs: mockSettings.charts, + }); testServerRoute( server, - '/api/admin/impact-metrics/settings', - { - charts: [ - { - ...mockSettings.charts[0], - title: 'Updated Chart', - }, - ], - layout: mockSettings.layout, - }, - 'put', + '/api/admin/impact-metrics/config', + 'Updated', + 'post', 200, ); @@ -289,11 +242,9 @@ describe('useImpactMetricsState', () => { }); it('deletes a chart successfully', async () => { - testServerRoute( - server, - '/api/admin/impact-metrics/settings', - mockSettings, - ); + testServerRoute(server, '/api/admin/impact-metrics/config', { + configs: mockSettings.charts, + }); render(); @@ -303,16 +254,16 @@ describe('useImpactMetricsState', () => { testServerRoute( server, - '/api/admin/impact-metrics/settings', - emptySettings, - 'put', + '/api/admin/impact-metrics/config/test-chart', + 'Deleted', + 'delete', 200, ); testServerRoute( server, - '/api/admin/impact-metrics/settings', - emptySettings, + '/api/admin/impact-metrics/config', + { configs: emptySettings.charts }, 'get', 200, ); diff --git a/frontend/src/component/impact-metrics/hooks/useImpactMetricsState.ts b/frontend/src/component/impact-metrics/hooks/useImpactMetricsState.ts index 55e427ea44..b3452acdb5 100644 --- a/frontend/src/component/impact-metrics/hooks/useImpactMetricsState.ts +++ b/frontend/src/component/impact-metrics/hooks/useImpactMetricsState.ts @@ -1,7 +1,7 @@ -import { useCallback } from 'react'; -import { useImpactMetricsSettings } from 'hooks/api/getters/useImpactMetricsSettings/useImpactMetricsSettings.js'; -import { useImpactMetricsSettingsApi } from 'hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsSettingsApi.js'; -import type { ChartConfig, ImpactMetricsState, LayoutItem } from '../types.ts'; +import { useCallback, useMemo } from 'react'; +import type { ChartConfig } from '../types.ts'; +import { useImpactMetricsConfig } from 'hooks/api/getters/useImpactMetricsConfig/useImpactMetricsConfig.ts'; +import { useImpactMetricsApi } from 'hooks/api/actions/useImpactMetricsApi/useImpactMetricsApi.ts'; /** * "Select all" represents all current and future labels. @@ -11,106 +11,76 @@ export const METRIC_LABELS_SELECT_ALL = '*'; export const useImpactMetricsState = () => { const { - settings, - loading: settingsLoading, - error: settingsError, + configs, + loading: configLoading, + error: configError, refetch, - } = useImpactMetricsSettings(); + } = useImpactMetricsConfig(); + + const { layout, charts } = useMemo( + () => ({ + layout: configs.map((config, index) => { + const column = index % 2; + const row = Math.floor(index / 2); + + return { + i: config.id, + x: column * 6, + y: row * 2, + w: 6, + h: 2, + minW: 4, + minH: 2, + maxW: 12, + maxH: 8, + }; + }), + charts: configs, + }), + [configs], + ); const { - updateSettings, + createImpactMetric, + deleteImpactMetric, loading: actionLoading, errors: actionErrors, - } = useImpactMetricsSettingsApi(); + } = useImpactMetricsApi(); const addChart = useCallback( async (config: Omit) => { - const newChart: ChartConfig = { - ...config, - id: `chart-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, - }; - - const maxY = - settings.layout.length > 0 - ? Math.max( - ...settings.layout.map((item) => item.y + item.h), - ) - : 0; - - const newState: ImpactMetricsState = { - charts: [...settings.charts, newChart], - layout: [ - ...settings.layout, - { - i: newChart.id, - x: 0, - y: maxY, - w: 6, - h: 4, - minW: 4, - minH: 2, - maxW: 12, - maxH: 8, - }, - ], - }; - - await updateSettings(newState); + await createImpactMetric(config); refetch(); }, - [settings, updateSettings, refetch], + [createImpactMetric, refetch], ); const updateChart = useCallback( - async (id: string, updates: Partial) => { - const updatedCharts = settings.charts.map((chart) => - chart.id === id ? { ...chart, ...updates } : chart, - ); - const newState: ImpactMetricsState = { - charts: updatedCharts, - layout: settings.layout, - }; - await updateSettings(newState); + async (id: string, updates: Omit) => { + await createImpactMetric({ ...updates, id }); refetch(); }, - [settings, updateSettings, refetch], + [configs, createImpactMetric, refetch], ); const deleteChart = useCallback( async (id: string) => { - const newState: ImpactMetricsState = { - charts: settings.charts.filter((chart) => chart.id !== id), - layout: settings.layout.filter((item) => item.i !== id), - }; - await updateSettings(newState); + await deleteImpactMetric(id); refetch(); }, - [settings, updateSettings, refetch], - ); - - const updateLayout = useCallback( - async (newLayout: LayoutItem[]) => { - const newState: ImpactMetricsState = { - charts: settings.charts, - layout: newLayout, - }; - await updateSettings(newState); - refetch(); - }, - [settings, updateSettings, refetch], + [configs, deleteImpactMetric, refetch], ); return { - charts: settings.charts || [], - layout: settings.layout || [], - loading: settingsLoading || actionLoading, + charts, + layout, + loading: configLoading || actionLoading, error: - settingsError || Object.keys(actionErrors).length > 0 + configError || Object.keys(actionErrors).length > 0 ? actionErrors : undefined, addChart, updateChart, deleteChart, - updateLayout, }; }; diff --git a/frontend/src/hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsApi.ts b/frontend/src/hooks/api/actions/useImpactMetricsApi/useImpactMetricsApi.ts similarity index 92% rename from frontend/src/hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsApi.ts rename to frontend/src/hooks/api/actions/useImpactMetricsApi/useImpactMetricsApi.ts index 5bbe66b408..f0a5938942 100644 --- a/frontend/src/hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsApi.ts +++ b/frontend/src/hooks/api/actions/useImpactMetricsApi/useImpactMetricsApi.ts @@ -9,7 +9,7 @@ type UseImpactMetricsApiParams = } | undefined; -export const useImpactMetricsApi = (params: UseImpactMetricsApiParams) => { +export const useImpactMetricsApi = (params?: UseImpactMetricsApiParams) => { const basePath = params ? `api/admin/projects/${params.projectId}/features/${params.featureName}/impact-metrics/config` : `api/admin/impact-metrics/config`; @@ -46,7 +46,7 @@ export const useImpactMetricsApi = (params: UseImpactMetricsApiParams) => { return makeRequest(req.caller, req.id); }, - [makeRequest, createRequest], + [makeRequest, createRequest, basePath], ); return { diff --git a/frontend/src/hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsSettingsApi.ts b/frontend/src/hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsSettingsApi.ts deleted file mode 100644 index a0e562313d..0000000000 --- a/frontend/src/hooks/api/actions/useImpactMetricsSettingsApi/useImpactMetricsSettingsApi.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { useCallback } from 'react'; -import useAPI from '../useApi/useApi.js'; -import type { ImpactMetricsState } from 'component/impact-metrics/types.ts'; - -export const useImpactMetricsSettingsApi = () => { - const { makeRequest, createRequest, errors, loading } = useAPI({ - propagateErrors: true, - }); - - const updateSettings = useCallback( - async (settings: ImpactMetricsState) => { - const path = `api/admin/impact-metrics/settings`; - const req = createRequest( - path, - { - method: 'PUT', - body: JSON.stringify(settings), - }, - 'updateImpactMetricsSettings', - ); - - return makeRequest(req.caller, req.id); - }, - [makeRequest, createRequest], - ); - - return { - updateSettings, - errors, - loading, - }; -}; diff --git a/frontend/src/hooks/api/getters/useImpactMetricsConfig/useImpactMetricsConfig.ts b/frontend/src/hooks/api/getters/useImpactMetricsConfig/useImpactMetricsConfig.ts new file mode 100644 index 0000000000..b8186f0344 --- /dev/null +++ b/frontend/src/hooks/api/getters/useImpactMetricsConfig/useImpactMetricsConfig.ts @@ -0,0 +1,21 @@ +import type { ImpactMetricsConfigListSchema } from 'openapi/index.js'; +import { fetcher, useApiGetter } from '../useApiGetter/useApiGetter.js'; +import { formatApiPath } from 'utils/formatPath'; + +/** + * Get all impact metrics configurations now associated with any feature flag. + */ +export const useImpactMetricsConfig = () => { + const PATH = `api/admin/impact-metrics/config`; + const { data, refetch, loading, error } = + useApiGetter(formatApiPath(PATH), () => + fetcher(formatApiPath(PATH), 'impactMetricsConfig'), + ); + + return { + configs: data?.configs || [], + refetch, + loading, + error, + }; +}; diff --git a/frontend/src/hooks/api/getters/useImpactMetricsSettings/useImpactMetricsSettings.ts b/frontend/src/hooks/api/getters/useImpactMetricsSettings/useImpactMetricsSettings.ts deleted file mode 100644 index 3faa89a942..0000000000 --- a/frontend/src/hooks/api/getters/useImpactMetricsSettings/useImpactMetricsSettings.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { fetcher, useApiGetter } from '../useApiGetter/useApiGetter.js'; -import { formatApiPath } from 'utils/formatPath'; -import type { DisplayImpactMetricsState } from 'component/impact-metrics/types.ts'; - -export const useImpactMetricsSettings = () => { - const PATH = `api/admin/impact-metrics/settings`; - const { data, refetch, loading, error } = - useApiGetter(formatApiPath(PATH), () => - fetcher(formatApiPath(PATH), 'Impact metrics settings'), - ); - - return { - settings: data || { charts: [], layout: [] }, - refetch, - loading, - error, - }; -};