chore: code review changes

Signed-off-by: blam <ben@blam.sh>

plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts

@@ -93,7 +93,9 @@ export class DatabaseTaskStore implements TaskStore {
     this.db = options.database;
   }
 
-  async list(options: { createdBy?: string }): Promise<SerializedTask[]> {
+  async list(options: {
+    createdBy?: string;
+  }): Promise<{ tasks: SerializedTask[] }> {
     const queryBuilder = this.db<RawDbTaskRow>('tasks');
 
     if (options.createdBy) {
@@ -104,7 +106,7 @@ export class DatabaseTaskStore implements TaskStore {
 
     const results = await queryBuilder.orderBy('created_at', 'desc').select();
 
-    return results.map(result => ({
+    const tasks = results.map(result => ({
       id: result.id,
       spec: JSON.parse(result.spec),
       status: result.status,
@@ -112,6 +114,8 @@ export class DatabaseTaskStore implements TaskStore {
       lastHeartbeatAt: parseSqlDateToIsoString(result.last_heartbeat_at),
       createdAt: parseSqlDateToIsoString(result.created_at),
     }));
+
+    return { tasks };
   }
 
   async getTask(taskId: string): Promise<SerializedTask> {

plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.test.ts

@@ -208,13 +208,13 @@ describe('StorageTaskBroker', () => {
     const { taskId } = await broker.dispatch({ spec: {} as TaskSpec });
 
     const promise = broker.list();
-    await expect(promise).resolves.toEqual(
-      expect.arrayContaining([
+    await expect(promise).resolves.toEqual({
+      tasks: expect.arrayContaining([
         expect.objectContaining({
           id: taskId,
         }),
       ]),
-    );
+    });
   });
 
   it('should list only tasks createdBy a specific user', async () => {
@@ -227,6 +227,6 @@ describe('StorageTaskBroker', () => {
     const task = await storage.getTask(taskId);
 
     const promise = broker.list({ createdBy: 'user:default/foo' });
-    await expect(promise).resolves.toEqual([task]);
+    await expect(promise).resolves.toEqual({ tasks: [task] });
   });
 });

plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts

@@ -151,7 +151,14 @@ export class StorageTaskBroker implements TaskBroker {
     private readonly logger: Logger,
   ) {}
 
-  async list(options?: { createdBy?: string }): Promise<SerializedTask[]> {
+  async list(options?: {
+    createdBy?: string;
+  }): Promise<{ tasks: SerializedTask[] }> {
+    if (!this.storage.list) {
+      throw new Error(
+        'TaskStore does not implement the list method. Please implement the list method to be able to list tasks',
+      );
+    }
     return await this.storage.list({ createdBy: options?.createdBy });
   }
 

plugins/scaffolder-backend/src/scaffolder/tasks/types.ts

@@ -133,7 +133,7 @@ export interface TaskBroker {
     after: number | undefined;
   }): Observable<{ events: SerializedTaskEvent[] }>;
   get(taskId: string): Promise<SerializedTask>;
-  list(options?: { createdBy?: string }): Promise<SerializedTask[]>;
+  list?(options?: { createdBy?: string }): Promise<{ tasks: SerializedTask[] }>;
 }
 
 /**
@@ -194,7 +194,7 @@ export interface TaskStore {
   listStaleTasks(options: { timeoutS: number }): Promise<{
     tasks: { taskId: string }[];
   }>;
-  list(options: { createdBy?: string }): Promise<SerializedTask[]>;
+  list?(options: { createdBy?: string }): Promise<{ tasks: SerializedTask[] }>;
 
   emitLogEvent({ taskId, body }: TaskStoreEmitOptions): Promise<void>;
   listEvents({

plugins/scaffolder-backend/src/service/router.test.ts

@@ -292,42 +292,52 @@ describe('createRouter', () => {
 
   describe('GET /v2/tasks', () => {
     it('return all tasks', async () => {
-      (taskBroker.list as jest.Mocked<TaskBroker>['list']).mockResolvedValue([
-        {
-          id: 'a-random-id',
-          spec: {} as any,
-          status: 'completed',
-          createdAt: '',
-          createdBy: '',
-        },
-      ]);
+      (
+        taskBroker.list as jest.Mocked<Required<TaskBroker>>['list']
+      ).mockResolvedValue({
+        tasks: [
+          {
+            id: 'a-random-id',
+            spec: {} as any,
+            status: 'completed',
+            createdAt: '',
+            createdBy: '',
+          },
+        ],
+      });
 
       const response = await request(app).get(`/v2/tasks`);
       expect(taskBroker.list).toBeCalledWith({
         createdBy: undefined,
       });
       expect(response.status).toEqual(200);
-      expect(response.body).toStrictEqual([
-        {
-          id: 'a-random-id',
-          spec: {} as any,
-          status: 'completed',
-          createdAt: '',
-          createdBy: '',
-        },
-      ]);
+      expect(response.body).toStrictEqual({
+        tasks: [
+          {
+            id: 'a-random-id',
+            spec: {} as any,
+            status: 'completed',
+            createdAt: '',
+            createdBy: '',
+          },
+        ],
+      });
     });
 
     it('return filtered tasks', async () => {
-      (taskBroker.list as jest.Mocked<TaskBroker>['list']).mockResolvedValue([
-        {
-          id: 'a-random-id',
-          spec: {} as any,
-          status: 'completed',
-          createdAt: '',
-          createdBy: 'user:default/foo',
-        },
-      ]);
+      (
+        taskBroker.list as jest.Mocked<Required<TaskBroker>>['list']
+      ).mockResolvedValue({
+        tasks: [
+          {
+            id: 'a-random-id',
+            spec: {} as any,
+            status: 'completed',
+            createdAt: '',
+            createdBy: 'user:default/foo',
+          },
+        ],
+      });
 
       const response = await request(app).get(
         `/v2/tasks?createdBy=user:default/foo`,
@@ -337,15 +347,17 @@ describe('createRouter', () => {
       });
 
       expect(response.status).toEqual(200);
-      expect(response.body).toStrictEqual([
-        {
-          id: 'a-random-id',
-          spec: {} as any,
-          status: 'completed',
-          createdAt: '',
-          createdBy: 'user:default/foo',
-        },
-      ]);
+      expect(response.body).toStrictEqual({
+        tasks: [
+          {
+            id: 'a-random-id',
+            spec: {} as any,
+            status: 'completed',
+            createdAt: '',
+            createdBy: 'user:default/foo',
+          },
+        ],
+      });
     });
   });
 

plugins/scaffolder-backend/src/service/router.ts

@@ -253,7 +253,20 @@ export async function createRouter(
       res.status(201).json({ id: result.taskId });
     })
     .get('/v2/tasks', async (req, res) => {
-      const userEntityRef = req.query.createdBy?.toString();
+      const [userEntityRef] = [req.query.createdBy].flat();
+
+      if (
+        typeof userEntityRef !== 'string' &&
+        typeof userEntityRef !== 'undefined'
+      ) {
+        throw new InputError('createdBy query parameter must be a string');
+      }
+
+      if (!taskBroker.list) {
+        throw new Error(
+          'TaskBroker does not support listing tasks, please implement the list method on the TaskBroker.',
+        );
+      }
 
       const tasks = await taskBroker.list({
         createdBy: userEntityRef,

plugins/scaffolder/src/api.test.ts

@@ -347,7 +347,7 @@ describe('api', () => {
         }),
       );
 
-      const result = await apiClient.listTasks({ createdBy: 'all' });
+      const result = await apiClient.listTasks({ filterByOwnership: 'all' });
       expect(result).toHaveLength(2);
     });
     it('should list task using the current user as owner', async () => {
@@ -357,23 +357,27 @@ describe('api', () => {
 
           if (createdBy) {
             return res(
-              ctx.json([
-                {
-                  createdBy,
-                },
-              ]),
+              ctx.json({
+                tasks: [
+                  {
+                    createdBy,
+                  },
+                ],
+              }),
             );
           }
 
           return res(
-            ctx.json([
-              {
-                createdBy: null,
-              },
-              {
-                createdBy: null,
-              },
-            ]),
+            ctx.json({
+              tasks: [
+                {
+                  createdBy: null,
+                },
+                {
+                  createdBy: null,
+                },
+              ],
+            }),
           );
         }),
       );
@@ -382,9 +386,9 @@ describe('api', () => {
         userEntityRef: 'user:default/foo',
       });
 
-      const result = await apiClient.listTasks({ createdBy: 'owned' });
+      const result = await apiClient.listTasks({ filterByOwnership: 'owned' });
       expect(identityApi.getBackstageIdentity).toBeCalled();
-      expect(result).toHaveLength(1);
+      expect(result.tasks).toHaveLength(1);
     });
   });
 });

plugins/scaffolder/src/api.ts

@@ -37,7 +37,6 @@ import {
   ScaffolderGetIntegrationsListOptions,
   ScaffolderGetIntegrationsListResponse,
   ScaffolderTask,
-  TasksOwnerFilterKind,
   ScaffolderDryRunOptions,
   ScaffolderDryRunResponse,
 } from './types';
@@ -61,13 +60,13 @@ export class ScaffolderClient implements ScaffolderApi {
   private readonly discoveryApi: DiscoveryApi;
   private readonly scmIntegrationsApi: ScmIntegrationRegistry;
   private readonly fetchApi: FetchApi;
-  private readonly identityApi: IdentityApi;
+  private readonly identityApi?: IdentityApi;
   private readonly useLongPollingLogs: boolean;
 
   constructor(options: {
     discoveryApi: DiscoveryApi;
     fetchApi: FetchApi;
-    identityApi: IdentityApi;
+    identityApi?: IdentityApi;
     scmIntegrationsApi: ScmIntegrationRegistry;
     useLongPollingLogs?: boolean;
   }) {
@@ -79,18 +78,21 @@ export class ScaffolderClient implements ScaffolderApi {
   }
 
   async listTasks(options: {
-    createdBy: TasksOwnerFilterKind;
-  }): Promise<ScaffolderTask[]> {
+    filterByOwnership: 'owned' | 'all';
+  }): Promise<{ tasks: ScaffolderTask[] }> {
+    if (!this.identityApi) {
+      throw new Error(
+        'IdentityApi is not available in the ScaffolderClient, please pass through the IdentityApi to the ScaffolderClient constructor in order to use the listTasks method',
+      );
+    }
     const baseUrl = await this.discoveryApi.getBaseUrl('scaffolder');
     const { userEntityRef } = await this.identityApi.getBackstageIdentity();
 
     const query = queryString.stringify(
-      options.createdBy === 'owned' ? { createdBy: userEntityRef } : {},
+      options.filterByOwnership === 'owned' ? { createdBy: userEntityRef } : {},
     );
 
-    const url = `${baseUrl}/v2/tasks?${query}`;
-
-    const response = await this.fetchApi.fetch(url);
+    const response = await this.fetchApi.fetch(`${baseUrl}/v2/tasks?${query}`);
     if (!response.ok) {
       throw await ResponseError.fromResponse(response);
     }

plugins/scaffolder/src/components/ListTasksPage/ListTaskPage.test.tsx

@@ -62,7 +62,7 @@ describe('<ListTasksPage />', () => {
     };
     catalogApi.getEntityByRef.mockResolvedValue(entity);
 
-    scaffolderApiMock.listTasks.mockResolvedValue([]);
+    scaffolderApiMock.listTasks.mockResolvedValue({ tasks: [] });
 
     const { getByText } = await renderInTestApp(
       <TestApiProvider
@@ -101,20 +101,22 @@ describe('<ListTasksPage />', () => {
       },
     };
     catalogApi.getEntityByRef.mockResolvedValue(entity);
-    scaffolderApiMock.listTasks.mockResolvedValue([
-      {
-        id: 'a-random-id',
-        spec: {
-          user: { ref: 'user:default/foo' },
-          templateInfo: {
-            entityRef: 'template:default/test',
-          },
-        } as any,
-        status: 'completed',
-        createdAt: '',
-        lastHeartbeatAt: '',
-      },
-    ]);
+    scaffolderApiMock.listTasks.mockResolvedValue({
+      tasks: [
+        {
+          id: 'a-random-id',
+          spec: {
+            user: { ref: 'user:default/foo' },
+            templateInfo: {
+              entityRef: 'template:default/test',
+            },
+          } as any,
+          status: 'completed',
+          createdAt: '',
+          lastHeartbeatAt: '',
+        },
+      ],
+    });
 
     scaffolderApiMock.getTemplateParameterSchema.mockResolvedValue({
       title: 'One Template',
@@ -139,7 +141,9 @@ describe('<ListTasksPage />', () => {
       },
     );
 
-    expect(scaffolderApiMock.listTasks).toBeCalledWith({ createdBy: 'owned' });
+    expect(scaffolderApiMock.listTasks).toBeCalledWith({
+      filterByOwnership: 'owned',
+    });
     expect(getByText('List template tasks')).toBeInTheDocument();
     expect(getByText('All tasks that have been started')).toBeInTheDocument();
     expect(getByText('Tasks')).toBeInTheDocument();
@@ -173,36 +177,40 @@ describe('<ListTasksPage />', () => {
       });
 
     scaffolderApiMock.listTasks
-      .mockResolvedValue([
-        {
-          id: 'a-random-id',
-          spec: {
-            user: { ref: 'user:default/foo' },
-            templateInfo: {
-              entityRef: 'template:default/mock',
-            },
-          } as any,
-          status: 'completed',
-          createdAt: '',
-          lastHeartbeatAt: '',
-        },
-      ])
-      .mockResolvedValue([
-        {
-          id: 'b-random-id',
-          spec: {
-            templateInfo: {
-              entityRef: 'template:default/mock',
-            },
-            user: {
-              ref: 'user:default/boo',
-            },
-          } as any,
-          status: 'completed',
-          createdAt: '',
-          lastHeartbeatAt: '',
-        },
-      ]);
+      .mockResolvedValue({
+        tasks: [
+          {
+            id: 'a-random-id',
+            spec: {
+              user: { ref: 'user:default/foo' },
+              templateInfo: {
+                entityRef: 'template:default/mock',
+              },
+            } as any,
+            status: 'completed',
+            createdAt: '',
+            lastHeartbeatAt: '',
+          },
+        ],
+      })
+      .mockResolvedValue({
+        tasks: [
+          {
+            id: 'b-random-id',
+            spec: {
+              templateInfo: {
+                entityRef: 'template:default/mock',
+              },
+              user: {
+                ref: 'user:default/boo',
+              },
+            } as any,
+            status: 'completed',
+            createdAt: '',
+            lastHeartbeatAt: '',
+          },
+        ],
+      });
 
     scaffolderApiMock.getTemplateParameterSchema.mockResolvedValue({
       title: 'One Template',
@@ -232,7 +240,9 @@ describe('<ListTasksPage />', () => {
       fireEvent.click(allButton);
     });
 
-    expect(scaffolderApiMock.listTasks).toBeCalledWith({ createdBy: 'all' });
+    expect(scaffolderApiMock.listTasks).toBeCalledWith({
+      filterByOwnership: 'all',
+    });
     expect(await findByText('One Template')).toBeInTheDocument();
     expect(await findByText('OtherUser')).toBeInTheDocument();
   });

plugins/scaffolder/src/components/ListTasksPage/ListTasksPage.tsx

@@ -31,7 +31,6 @@ import React, { useState } from 'react';
 import { scaffolderApiRef } from '../../api';
 import { rootRouteRef } from '../../routes';
 import { OwnerListPicker } from './OwnerListPicker';
-import { TasksOwnerFilterKind } from '../../types';
 import {
   CreatedAtColumn,
   OwnerEntityColumn,
@@ -40,7 +39,7 @@ import {
 } from './columns';
 
 export interface MyTaskPageProps {
-  initiallySelectedFilter?: TasksOwnerFilterKind;
+  initiallySelectedFilter?: 'owned' | 'all';
 }
 
 const ListTaskPageContent = (props: MyTaskPageProps) => {
@@ -52,14 +51,15 @@ const ListTaskPageContent = (props: MyTaskPageProps) => {
   const [ownerFilter, setOwnerFilter] = useState(initiallySelectedFilter);
   const { value, loading, error } = useAsync(() => {
     if (scaffolderApi.listTasks) {
-      return scaffolderApi.listTasks?.({ createdBy: ownerFilter });
+      return scaffolderApi.listTasks?.({ filterByOwnership: ownerFilter });
     }
 
     // eslint-disable-next-line no-console
     console.warn(
       'listTasks is not implemented in the scaffolderApi, please make sure to implement this method.',
     );
-    return Promise.resolve([]);
+
+    return Promise.resolve({ tasks: [] });
   }, [scaffolderApi, ownerFilter]);
 
   if (loading) {
@@ -89,7 +89,7 @@ const ListTaskPageContent = (props: MyTaskPageProps) => {
       </CatalogFilterLayout.Filters>
       <CatalogFilterLayout.Content>
         <MaterialTable
-          data={value!}
+          data={value?.tasks ?? []}
           title="Tasks"
           columns={[
             {

plugins/scaffolder/src/components/ListTasksPage/OwnerListPicker.tsx

@@ -29,8 +29,6 @@ import React, { Fragment } from 'react';
 
 import AllIcon from '@material-ui/icons/FontDownload';
 
-import { TasksOwnerFilterKind } from '../../types';
-
 const useStyles = makeStyles<Theme>(
   theme => ({
     root: {
@@ -91,7 +89,7 @@ function getFilterGroups(): ButtonGroup[] {
 
 export const OwnerListPicker = (props: {
   filter: string;
-  onSelectOwner: (id: TasksOwnerFilterKind) => void;
+  onSelectOwner: (id: 'owned' | 'all') => void;
 }) => {
   const { filter, onSelectOwner } = props;
   const classes = useStyles();
@@ -111,7 +109,7 @@ export const OwnerListPicker = (props: {
                   key={item.id}
                   button
                   divider
-                  onClick={() => onSelectOwner(item.id as TasksOwnerFilterKind)}
+                  onClick={() => onSelectOwner(item.id as 'owned' | 'all')}
                   selected={item.id === filter}
                   className={classes.menuItem}
                   data-testid={`owner-picker-${item.id}`}

plugins/scaffolder/src/index.ts

@@ -37,7 +37,6 @@ export type {
   ScaffolderTaskOutput,
   ScaffolderTaskStatus,
   TemplateParameterSchema,
-  TasksOwnerFilterKind,
 } from './types';
 export {
   createScaffolderFieldExtension,

plugins/scaffolder/src/types.ts

@@ -102,13 +102,6 @@ export type LogEvent = {
   taskId: string;
 };
 
-/**
- * The status of each task you can filter from `ScaffolderClient`
- *
- * @public
- */
-export type TasksOwnerFilterKind = 'owned' | 'all';
-
 /**
  * The input options to the `scaffold` method of the `ScaffolderClient`.
  *
@@ -200,10 +193,10 @@ export interface ScaffolderApi {
   getTask(taskId: string): Promise<ScaffolderTask>;
 
   listTasks?({
-    createdBy,
+    filterByOwnership,
   }: {
-    createdBy: TasksOwnerFilterKind;
-  }): Promise<ScaffolderTask[]>;
+    filterByOwnership: 'owned' | 'all';
+  }): Promise<{ tasks: ScaffolderTask[] }>;
 
   getIntegrationsList(
     options: ScaffolderGetIntegrationsListOptions,