From 48ec0b72e23e075211537bfafaae56bf757596ba Mon Sep 17 00:00:00 2001
From: Kyle Watson <kyle.watson@devoteam.com>
Date: Mon, 4 Dec 2023 10:18:37 +0100
Subject: [PATCH] refactor(backstage-plugin): Refactor data service (#116)

Reuse request logic in the data service.
Rename data service from GroupDataService to DoraDataService.
Reorganise tests to describe them better.
Rename test to describe what is being tested.

Addresses #71
---
 .../plugins/open-dora/dev/index.tsx           |  12 +-
 .../DashboardComponent.test.tsx               |  10 +-
 .../src/hooks/MetricBenchmarkHook.ts          |   8 +-
 .../open-dora/src/hooks/MetricDataHook.ts     |   8 +-
 .../plugins/open-dora/src/plugin.ts           |  10 +-
 .../src/services/DoraDataService.test.ts      | 201 ++++++++++++++++++
 ...GroupDataService.ts => DoraDataService.ts} |  46 ++--
 .../src/services/GroupDataService.test.ts     | 199 -----------------
 8 files changed, 242 insertions(+), 252 deletions(-)
 create mode 100644 backstage-plugin/plugins/open-dora/src/services/DoraDataService.test.ts
 rename backstage-plugin/plugins/open-dora/src/services/{GroupDataService.ts => DoraDataService.ts} (62%)
 delete mode 100644 backstage-plugin/plugins/open-dora/src/services/GroupDataService.test.ts

diff --git a/backstage-plugin/plugins/open-dora/dev/index.tsx b/backstage-plugin/plugins/open-dora/dev/index.tsx
index 6294fdb..3c1c562 100644
--- a/backstage-plugin/plugins/open-dora/dev/index.tsx
+++ b/backstage-plugin/plugins/open-dora/dev/index.tsx
@@ -1,11 +1,11 @@
 import { createDevApp } from '@backstage/dev-utils';
+import { MockConfigApi } from '@backstage/test-utils';
 import React from 'react';
 import { openDoraPlugin, OpenDoraPluginPage } from '../src';
-import { MockConfigApi } from '@backstage/test-utils';
 import {
-  GroupDataService,
-  groupDataServiceApiRef,
-} from '../src/services/GroupDataService';
+  DoraDataService,
+  doraDataServiceApiRef,
+} from '../src/services/DoraDataService';
 
 const mockConfig = new MockConfigApi({
   'open-dora': {
@@ -16,9 +16,9 @@ const mockConfig = new MockConfigApi({
 createDevApp()
   .registerPlugin(openDoraPlugin)
   .registerApi({
-    api: groupDataServiceApiRef,
+    api: doraDataServiceApiRef,
     deps: {},
-    factory: () => new GroupDataService({ configApi: mockConfig }),
+    factory: () => new DoraDataService({ configApi: mockConfig }),
   })
   .addPage({
     element: <OpenDoraPluginPage />,
diff --git a/backstage-plugin/plugins/open-dora/src/components/DashboardComponent/DashboardComponent.test.tsx b/backstage-plugin/plugins/open-dora/src/components/DashboardComponent/DashboardComponent.test.tsx
index 1bf5dbb..0415afa 100644
--- a/backstage-plugin/plugins/open-dora/src/components/DashboardComponent/DashboardComponent.test.tsx
+++ b/backstage-plugin/plugins/open-dora/src/components/DashboardComponent/DashboardComponent.test.tsx
@@ -11,9 +11,9 @@ import { rest } from 'msw';
 import React from 'react';
 import { baseUrl, metricUrl } from '../../../testing/mswHandlers';
 import {
-  GroupDataService,
-  groupDataServiceApiRef,
-} from '../../services/GroupDataService';
+  DoraDataService,
+  doraDataServiceApiRef,
+} from '../../services/DoraDataService';
 import { server } from '../../setupTests';
 import {
   DashboardComponent,
@@ -26,8 +26,8 @@ async function renderComponentWithApis(component: JSX.Element) {
   });
 
   const apiRegistry = TestApiRegistry.from([
-    groupDataServiceApiRef,
-    new GroupDataService({ configApi: mockConfig }),
+    doraDataServiceApiRef,
+    new DoraDataService({ configApi: mockConfig }),
   ]);
 
   return await renderInTestApp(
diff --git a/backstage-plugin/plugins/open-dora/src/hooks/MetricBenchmarkHook.ts b/backstage-plugin/plugins/open-dora/src/hooks/MetricBenchmarkHook.ts
index b3679d5..23a382a 100644
--- a/backstage-plugin/plugins/open-dora/src/hooks/MetricBenchmarkHook.ts
+++ b/backstage-plugin/plugins/open-dora/src/hooks/MetricBenchmarkHook.ts
@@ -1,18 +1,18 @@
 import { useApi } from '@backstage/core-plugin-api';
 import { useEffect, useState } from 'react';
 import { dfBenchmarkKey } from '../models/DfBenchmarkData';
-import { groupDataServiceApiRef } from '../services/GroupDataService';
+import { doraDataServiceApiRef } from '../services/DoraDataService';
 
 export const useMetricBenchmark = (type: string) => {
-  const groupDataService = useApi(groupDataServiceApiRef);
+  const doraDataService = useApi(doraDataServiceApiRef);
   const [benchmark, setDfBenchmark] = useState<dfBenchmarkKey | undefined>();
   const [error, setError] = useState<Error | undefined>();
 
   useEffect(() => {
-    groupDataService.retrieveBenchmarkData({ type: type }).then(response => {
+    doraDataService.retrieveBenchmarkData({ type: type }).then(response => {
       setDfBenchmark(response.key);
     }, setError);
-  }, [groupDataService, type]);
+  }, [doraDataService, type]);
 
   return { error: error, benchmark: benchmark };
 };
diff --git a/backstage-plugin/plugins/open-dora/src/hooks/MetricDataHook.ts b/backstage-plugin/plugins/open-dora/src/hooks/MetricDataHook.ts
index 99c2a5d..ba4de67 100644
--- a/backstage-plugin/plugins/open-dora/src/hooks/MetricDataHook.ts
+++ b/backstage-plugin/plugins/open-dora/src/hooks/MetricDataHook.ts
@@ -1,17 +1,17 @@
 import { useApi } from '@backstage/core-plugin-api';
 import { useContext, useEffect, useState } from 'react';
 import { MetricData } from '../models/MetricData';
-import { groupDataServiceApiRef } from '../services/GroupDataService';
+import { doraDataServiceApiRef } from '../services/DoraDataService';
 import { MetricContext } from '../services/MetricContext';
 
 export const useMetricData = (type: string) => {
-  const groupDataService = useApi(groupDataServiceApiRef);
+  const doraDataService = useApi(doraDataServiceApiRef);
   const [chartData, setChartData] = useState<MetricData | undefined>();
   const [error, setError] = useState<Error | undefined>();
   const { aggregation, team, project } = useContext(MetricContext);
 
   useEffect(() => {
-    groupDataService
+    doraDataService
       .retrieveMetricDataPoints({
         type: type,
         team: team,
@@ -25,7 +25,7 @@ export const useMetricData = (type: string) => {
           setError(new Error('No data found'));
         }
       }, setError);
-  }, [aggregation, team, project, groupDataService, type]);
+  }, [aggregation, team, project, doraDataService, type]);
 
   return { error: error, chartData: chartData };
 };
diff --git a/backstage-plugin/plugins/open-dora/src/plugin.ts b/backstage-plugin/plugins/open-dora/src/plugin.ts
index 014ab1a..21acf70 100644
--- a/backstage-plugin/plugins/open-dora/src/plugin.ts
+++ b/backstage-plugin/plugins/open-dora/src/plugin.ts
@@ -6,9 +6,9 @@ import {
 } from '@backstage/core-plugin-api';
 import { rootRouteRef } from './routes';
 import {
-  GroupDataService,
-  groupDataServiceApiRef,
-} from './services/GroupDataService';
+  DoraDataService,
+  doraDataServiceApiRef,
+} from './services/DoraDataService';
 
 export const openDoraPlugin = createPlugin({
   id: 'opendora',
@@ -17,9 +17,9 @@ export const openDoraPlugin = createPlugin({
   },
   apis: [
     createApiFactory({
-      api: groupDataServiceApiRef,
+      api: doraDataServiceApiRef,
       deps: { configApi: configApiRef },
-      factory: ({ configApi }) => new GroupDataService({ configApi }),
+      factory: ({ configApi }) => new DoraDataService({ configApi }),
     }),
   ],
 });
diff --git a/backstage-plugin/plugins/open-dora/src/services/DoraDataService.test.ts b/backstage-plugin/plugins/open-dora/src/services/DoraDataService.test.ts
new file mode 100644
index 0000000..02236fc
--- /dev/null
+++ b/backstage-plugin/plugins/open-dora/src/services/DoraDataService.test.ts
@@ -0,0 +1,201 @@
+import { MockConfigApi } from '@backstage/test-utils';
+import { rest } from 'msw';
+import { baseUrl, benchmarkUrl, metricUrl } from '../../testing/mswHandlers';
+import { server } from '../setupTests';
+import { DoraDataService } from './DoraDataService';
+
+function createService() {
+  server.use(
+    rest.get(benchmarkUrl, (req, res, ctx) => {
+      const params = req.url.searchParams;
+      const type = params.get('type');
+
+      switch (type) {
+        case 'df': {
+          return res(
+            ctx.json({
+              key: 'lt-6month',
+            }),
+          );
+        }
+        default: {
+          return res(ctx.status(400));
+        }
+      }
+    }),
+    rest.get(metricUrl, (req, res, ctx) => {
+      const params = req.url.searchParams;
+      const type = params.get('type');
+      const aggregation = params.get('aggregation');
+      const project = params.get('project');
+      const team = params.get('team');
+
+      return res(
+        ctx.json({
+          aggregation: aggregation || 'weekly',
+          dataPoints: [
+            {
+              key: `${project}_${team}_${aggregation}_${type}_first_key`,
+              value: 2.3,
+            },
+          ],
+        }),
+      );
+    }),
+  );
+  const mockConfig = new MockConfigApi({
+    'open-dora': { apiBaseUrl: baseUrl },
+  });
+
+  return new DoraDataService({ configApi: mockConfig });
+}
+
+describe('DoraDataService', () => {
+  describe('retriveMetricDataPoints', () => {
+    it('should return data from the server', async () => {
+      const service = createService();
+
+      expect(
+        await service.retrieveMetricDataPoints({ type: 'df_count' }),
+      ).toEqual({
+        aggregation: 'weekly',
+        dataPoints: [{ key: 'null_null_null_df_count_first_key', value: 2.3 }],
+      });
+    });
+
+    it('should use provided details in the query parameters', async () => {
+      const service = createService();
+
+      expect(
+        await service.retrieveMetricDataPoints({
+          type: 'df_count',
+          aggregation: 'monthly',
+          project: 'project1',
+          team: 'team1',
+        }),
+      ).toEqual({
+        aggregation: 'monthly',
+        dataPoints: [
+          { key: 'project1_team1_monthly_df_count_first_key', value: 2.3 },
+        ],
+      });
+    });
+
+    it('should throw an error if the response does not contain metric data', async () => {
+      const service = createService();
+
+      server.use(
+        rest.get(metricUrl, (_, res, ctx) => {
+          return res(ctx.json({ other: 'data' }));
+        }),
+      );
+      await expect(
+        service.retrieveMetricDataPoints({
+          type: 'df_count',
+        }),
+      ).rejects.toEqual(new Error('Unexpected response'));
+    });
+
+    it('should throw an error when the server is unreachable', async () => {
+      const service = createService();
+
+      server.use(
+        rest.get(metricUrl, (_, res) => {
+          return res.networkError('Host unreachable');
+        }),
+      );
+      await expect(
+        service.retrieveMetricDataPoints({
+          type: 'df_count',
+        }),
+      ).rejects.toEqual(new Error('Failed to fetch'));
+    });
+
+    it('should throw an error when the server returns a non-ok status', async () => {
+      const service = createService();
+
+      server.use(
+        rest.get(metricUrl, (_, res, ctx) => {
+          return res(ctx.status(401));
+        }),
+      );
+      await expect(
+        service.retrieveMetricDataPoints({
+          type: 'df_count',
+        }),
+      ).rejects.toEqual(new Error('Unauthorized'));
+
+      server.use(
+        rest.get(metricUrl, (_, res, ctx) => {
+          return res(ctx.status(500));
+        }),
+      );
+      await expect(
+        service.retrieveMetricDataPoints({
+          type: 'df_count',
+        }),
+      ).rejects.toEqual(new Error('Internal Server Error'));
+    });
+  });
+
+  describe('retrieveBenchmarkData', () => {
+    it('should return deployment frequency overall data from the server', async () => {
+      const service = createService();
+
+      expect(await service.retrieveBenchmarkData({ type: 'df' })).toEqual({
+        key: 'lt-6month',
+      });
+    });
+
+    it('should throw an error if the response does not contain metric data', async () => {
+      const service = createService();
+
+      server.use(
+        rest.get(benchmarkUrl, (_, res, ctx) => {
+          return res(ctx.json({ other: 'data' }));
+        }),
+      );
+      await expect(
+        service.retrieveBenchmarkData({
+          type: 'df',
+        }),
+      ).rejects.toEqual(new Error('Unexpected response'));
+    });
+
+    it('should send the params in the request', async () => {
+      const service = createService();
+
+      await expect(
+        service.retrieveBenchmarkData({
+          type: 'invalid_type',
+        }),
+      ).rejects.toEqual(new Error('Bad Request'));
+    });
+
+    it('should throw an error when the server returns a non-ok status', async () => {
+      const service = createService();
+
+      server.use(
+        rest.get(benchmarkUrl, (_, res, ctx) => {
+          return res(ctx.status(401));
+        }),
+      );
+      await expect(
+        service.retrieveBenchmarkData({
+          type: 'df',
+        }),
+      ).rejects.toEqual(new Error('Unauthorized'));
+
+      server.use(
+        rest.get(benchmarkUrl, (_, res, ctx) => {
+          return res(ctx.status(500));
+        }),
+      );
+      await expect(
+        service.retrieveBenchmarkData({
+          type: 'df',
+        }),
+      ).rejects.toEqual(new Error('Internal Server Error'));
+    });
+  });
+});
diff --git a/backstage-plugin/plugins/open-dora/src/services/GroupDataService.ts b/backstage-plugin/plugins/open-dora/src/services/DoraDataService.ts
similarity index 62%
rename from backstage-plugin/plugins/open-dora/src/services/GroupDataService.ts
rename to backstage-plugin/plugins/open-dora/src/services/DoraDataService.ts
index 15b6496..d4fd5c1 100644
--- a/backstage-plugin/plugins/open-dora/src/services/GroupDataService.ts
+++ b/backstage-plugin/plugins/open-dora/src/services/DoraDataService.ts
@@ -1,24 +1,19 @@
 import { ConfigApi, createApiRef } from '@backstage/core-plugin-api';
-import { MetricData } from '../models/MetricData';
 import { dfBenchmarkData } from '../models/DfBenchmarkData';
+import { MetricData } from '../models/MetricData';
 
-export const groupDataServiceApiRef = createApiRef<GroupDataService>({
-  id: 'plugin.open-dora.group-data',
+export const doraDataServiceApiRef = createApiRef<DoraDataService>({
+  id: 'plugin.open-dora.data-service',
 });
 
-export class GroupDataService {
+export class DoraDataService {
   constructor(private options: { configApi: ConfigApi }) {}
 
-  async retrieveMetricDataPoints(params: {
-    type: string;
-    team?: string;
-    project?: string;
-    aggregation?: string;
-  }) {
+  private async retrieveData(params: Record<string, string>, path: string) {
     const baseUrl = this.options.configApi.getString('open-dora.apiBaseUrl');
 
     const url = new URL(baseUrl);
-    url.pathname = 'dora/api/metric';
+    url.pathname = path;
     for (const [key, value] of Object.entries(params)) {
       if (value) {
         url.searchParams.append(key, value);
@@ -32,7 +27,16 @@ export class GroupDataService {
       throw new Error(data.statusText);
     }
 
-    const response = await data.json();
+    return await data.json();
+  }
+
+  async retrieveMetricDataPoints(params: {
+    type: string;
+    team?: string;
+    project?: string;
+    aggregation?: string;
+  }) {
+    const response = await this.retrieveData(params, 'dora/api/metric');
     if (
       response.aggregation === undefined ||
       response.dataPoints === undefined
@@ -44,24 +48,8 @@ export class GroupDataService {
   }
 
   async retrieveBenchmarkData(params: { type: string }) {
-    const baseUrl = this.options.configApi.getString('open-dora.apiBaseUrl');
-
-    const url = new URL(baseUrl);
-    url.pathname = 'dora/api/benchmark';
-    for (const [key, value] of Object.entries(params)) {
-      if (value) {
-        url.searchParams.append(key, value);
-      }
-    }
-    const data = await fetch(url.toString(), {
-      method: 'GET',
-    });
-
-    if (!data.ok) {
-      throw new Error(data.statusText);
-    }
+    const response = await this.retrieveData(params, 'dora/api/benchmark');
 
-    const response = await data.json();
     if (response.key === undefined) {
       throw new Error('Unexpected response');
     }
diff --git a/backstage-plugin/plugins/open-dora/src/services/GroupDataService.test.ts b/backstage-plugin/plugins/open-dora/src/services/GroupDataService.test.ts
deleted file mode 100644
index 2e3a67a..0000000
--- a/backstage-plugin/plugins/open-dora/src/services/GroupDataService.test.ts
+++ /dev/null
@@ -1,199 +0,0 @@
-import { MockConfigApi } from '@backstage/test-utils';
-import { rest } from 'msw';
-import { baseUrl, benchmarkUrl, metricUrl } from '../../testing/mswHandlers';
-import { server } from '../setupTests';
-import { GroupDataService } from './GroupDataService';
-
-function createService() {
-  server.use(
-    rest.get(benchmarkUrl, (req, res, ctx) => {
-      const params = req.url.searchParams;
-      const type = params.get('type');
-
-      switch (type) {
-        case 'df': {
-          return res(
-            ctx.json({
-              key: 'lt-6month',
-            }),
-          );
-        }
-        default: {
-          return res(ctx.status(400));
-        }
-      }
-    }),
-    rest.get(metricUrl, (req, res, ctx) => {
-      const params = req.url.searchParams;
-      const type = params.get('type');
-      const aggregation = params.get('aggregation');
-      const project = params.get('project');
-      const team = params.get('team');
-
-      return res(
-        ctx.json({
-          aggregation: aggregation || 'weekly',
-          dataPoints: [
-            {
-              key: `${project}_${team}_${aggregation}_${type}_first_key`,
-              value: 2.3,
-            },
-          ],
-        }),
-      );
-    }),
-  );
-  const mockConfig = new MockConfigApi({
-    'open-dora': { apiBaseUrl: baseUrl },
-  });
-
-  return new GroupDataService({ configApi: mockConfig });
-}
-
-describe('GroupDataService', () => {
-  it('should return data from the server', async () => {
-    const service = createService();
-
-    expect(
-      await service.retrieveMetricDataPoints({ type: 'df_count' }),
-    ).toEqual({
-      aggregation: 'weekly',
-      dataPoints: [{ key: 'null_null_null_df_count_first_key', value: 2.3 }],
-    });
-  });
-
-  it('should use provided details in the query parameters', async () => {
-    const service = createService();
-
-    expect(
-      await service.retrieveMetricDataPoints({
-        type: 'df_count',
-        aggregation: 'monthly',
-        project: 'project1',
-        team: 'team1',
-      }),
-    ).toEqual({
-      aggregation: 'monthly',
-      dataPoints: [
-        { key: 'project1_team1_monthly_df_count_first_key', value: 2.3 },
-      ],
-    });
-  });
-
-  it('should throw an error if the response does not contain metric data', async () => {
-    const service = createService();
-
-    server.use(
-      rest.get(metricUrl, (_, res, ctx) => {
-        return res(ctx.json({ other: 'data' }));
-      }),
-    );
-    await expect(
-      service.retrieveMetricDataPoints({
-        type: 'df_count',
-      }),
-    ).rejects.toEqual(new Error('Unexpected response'));
-  });
-
-  it('should throw an error when the server is unreachable', async () => {
-    const service = createService();
-
-    server.use(
-      rest.get(metricUrl, (_, res) => {
-        return res.networkError('Host unreachable');
-      }),
-    );
-    await expect(
-      service.retrieveMetricDataPoints({
-        type: 'df_count',
-      }),
-    ).rejects.toEqual(new Error('Failed to fetch'));
-  });
-
-  it('should throw an error when the server returns a non-ok status', async () => {
-    const service = createService();
-
-    server.use(
-      rest.get(metricUrl, (_, res, ctx) => {
-        return res(ctx.status(401));
-      }),
-    );
-    await expect(
-      service.retrieveMetricDataPoints({
-        type: 'df_count',
-      }),
-    ).rejects.toEqual(new Error('Unauthorized'));
-
-    server.use(
-      rest.get(metricUrl, (_, res, ctx) => {
-        return res(ctx.status(500));
-      }),
-    );
-    await expect(
-      service.retrieveMetricDataPoints({
-        type: 'df_count',
-      }),
-    ).rejects.toEqual(new Error('Internal Server Error'));
-  });
-});
-
-describe('BenchmarkService', () => {
-  it('should return deployment frequency overall data from the server', async () => {
-    const service = createService();
-
-    expect(await service.retrieveBenchmarkData({ type: 'df' })).toEqual({
-      key: 'lt-6month',
-    });
-  });
-
-  it('should throw an error if the response does not contain metric data', async () => {
-    const service = createService();
-
-    server.use(
-      rest.get(benchmarkUrl, (_, res, ctx) => {
-        return res(ctx.json({ other: 'data' }));
-      }),
-    );
-    await expect(
-      service.retrieveBenchmarkData({
-        type: 'df',
-      }),
-    ).rejects.toEqual(new Error('Unexpected response'));
-  });
-
-  it('should return 404 for invalid types', async () => {
-    const service = createService();
-
-    await expect(
-      service.retrieveBenchmarkData({
-        type: 'invalid_type',
-      }),
-    ).rejects.toEqual(new Error('Bad Request'));
-  });
-
-  it('should throw an error when the server returns a non-ok status', async () => {
-    const service = createService();
-
-    server.use(
-      rest.get(benchmarkUrl, (_, res, ctx) => {
-        return res(ctx.status(401));
-      }),
-    );
-    await expect(
-      service.retrieveBenchmarkData({
-        type: 'df',
-      }),
-    ).rejects.toEqual(new Error('Unauthorized'));
-
-    server.use(
-      rest.get(benchmarkUrl, (_, res, ctx) => {
-        return res(ctx.status(500));
-      }),
-    );
-    await expect(
-      service.retrieveBenchmarkData({
-        type: 'df',
-      }),
-    ).rejects.toEqual(new Error('Internal Server Error'));
-  });
-});