refactor: Refactor custom-widget deletion

Vizzuality · Nov 13, 2024 · 5235af0 · 5235af0
1 parent 51efe94
commit 5235af0
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 20 deletions.
diff --git a/api/src/modules/widgets/custom-widgets.controller.ts b/api/src/modules/widgets/custom-widgets.controller.ts
@@ -67,11 +67,8 @@ export class CustomWidgetsController {
   public async deleteCustomWidget(): Promise<ControllerResponse> {
     return tsRestHandler(c.deleteCustomWidget, async ({ params }) => {
       const { userId, id } = params;
-      const data = await this.customWidgetsService.deleteCustomWidget(
-        userId,
-        id,
-      );
-      return { body: { data }, status: 200 };
+      await this.customWidgetsService.deleteCustomWidget(userId, id);
+      return { body: null, status: 204 };
     });
   }
 }
diff --git a/api/src/modules/widgets/custom-widgets.service.ts b/api/src/modules/widgets/custom-widgets.service.ts
@@ -190,26 +190,19 @@ export class CustomWidgetService extends AppBaseService<
     id: number,
     info?: AppInfoDTO,
   ) {
-    const customWidget = await this.customWidgetRepository.findOneBy({
+    const result = await this.customWidgetRepository.delete({
       id,
       user: { id: userId },
     });
-    if (customWidget === null) {
+
+    if (result.affected === 0) {
       const exception = new NotFoundException('CustomWidget not found');
       this.logger.error(
-        `CustomWidget with id ${id} not found`,
+        `CustomWidget with id ${id} for user ${info.authenticatedUser.id} not found`,
         null,
         exception.stack,
       );
       throw exception;
     }
-
-    // TODO: There are differences between remove() and delete() methods of the repository. Main differences:
-    //       1- similarly as save() vs insert(), remove first loads the entity, effectively making two queries, while delete does not.
-    //       2- it has it's usages (you can check), but I don't see any reasons right now to use remove right now. I am not saying we should not use it here
-    //          I just want you to be aware of the differences.
-    //       3- Following the previous point, remove mutates the original object, removing the id field, so we should be careful with the mutability
-    //       4- Last point, we should not return to the consumer the deleted entity, as it is not available anymore. Right now there is no usage for that
-    return this.customWidgetRepository.remove(customWidget);
   }
 }
diff --git a/api/test/e2e/users/custom-widgets/custom-widget-crud.spec.ts b/api/test/e2e/users/custom-widgets/custom-widget-crud.spec.ts
@@ -1,6 +1,7 @@
 import { DataSourceManager } from '@api/infrastructure/data-source-manager';
 import { User } from '@shared/dto/users/user.entity';
 import { BaseWidget } from '@shared/dto/widgets/base-widget.entity';
+import { CustomWidget } from '@shared/dto/widgets/custom-widget.entity';
 import { TestManager } from 'api/test/utils/test-manager';
 
 describe('Custom Widgets API', () => {
@@ -429,8 +430,17 @@ describe('Custom Widgets API', () => {
         .set('Authorization', `Bearer ${authToken}`);
 
       // Then
-      expect(res.status).toBe(200);
-      expect(res.body.data.name).toBe(customWidget.name);
+      expect(res.status).toBe(204);
+      const customWidgetRepository = testManager
+        .getDataSource()
+        .getRepository(CustomWidget);
+
+      expect(
+        await customWidgetRepository.findOneBy({
+          id: customWidget.id,
+          user: { id: customWidget.user.id },
+        }),
+      ).toBe(null);
     });
 
     it("Shouldn't allow authenticated users to delete other user's custom widgets", async () => {

diff --git a/...lizations/table/columns/actions-button/actions-menu/delete-visualization-button/index.tsx b/...lizations/table/columns/actions-button/actions-menu/delete-visualization-button/index.tsx
@@ -41,7 +41,7 @@ const DeleteVisualizationButton: FC<{ id: number }> = ({ id }) => {
       },
     });
 
-    if (response.status === 200) {
+    if (response.status === 204) {
       queryClient.invalidateQueries(
         queryKeys.users.userCharts(session?.user.id as string, {}).queryKey,
         {

diff --git a/shared/contracts/users.contract.ts b/shared/contracts/users.contract.ts
@@ -171,7 +171,7 @@ export const usersContract = contract.router({
     pathParams: z.object({ userId: z.string().uuid(), id: z.coerce.number() }),
     body: null,
     responses: {
-      200: contract.type<ApiResponse<CustomWidget>>(),
+      204: contract.type<null>(),
       400: contract.type<JSONAPIError>(),
       404: contract.type<JSONAPIError>(),
       500: contract.type<JSONAPIError>(),