implement 10 and 11 issues #15

murtagh27 · 2024-05-22T22:36:42Z

No description provided.

package.json

apps/backend/src/drink-action/entities/drink-action.entity.ts

LeventeFarkashazi · 2024-05-23T11:21:41Z

apps/backend/src/drink-action/dto/create-drink-action.dto.ts

@@ -0,0 +1,4 @@
+import { OmitType } from '@nestjs/swagger';


You can fix this by running yarn lint:fix. If you have the ESLint extension in VSCode, it should show the error and may even fix it when saving the file.

i dont know whats the problem, i have eslint...

I have no idea why it doesn't find the apps directory. Maybe try running it manually: npx eslint apps/* --ext .ts,.tsx --fix

can you try running from a git bash terminal? It's possible that powershell doesn't like forward slashes

i tried both and it still says the same error message:l

The command was invalid on windows, I'll push to master to fix this. You can do the linting with yarn eslint apps --ext .ts,.tsx --fix until then.

Co-authored-by: Farkasházi Levente <[email protected]>

apps/backend/src/drink-action/entities/drink-action.entity.ts

LeventeFarkashazi · 2024-05-23T11:49:26Z

apps/backend/src/drink-action/drink-action.service.ts

+  async create(createDrinkActionDto: CreateDrinkActionDto) {
+    return this.prisma.drinkAction.create({ data: createDrinkActionDto });
+  }


If the drinkId or the eventId is invalid, we get an internal server error. There should be some error handling for these cases.
e.g. :

Suggested change

async create(createDrinkActionDto: CreateDrinkActionDto) {

return this.prisma.drinkAction.create({ data: createDrinkActionDto });

}

async create(createDrinkActionDto: CreateDrinkActionDto) {

const { drinkId, eventId } = createDrinkActionDto;

const [drinkExists, eventExists] = await Promise.all([

this.prisma.drink.findUnique({ where: { id: drinkId } }),

this.prisma.event.findUnique({ where: { id: eventId } }),

]);

if (!drinkExists) {

throw new BadRequestException(`Drink with ID ${drinkId} does not exist.`);

}

if (!eventExists) {

throw new BadRequestException(`Event with ID ${eventId} does not exist.`);

}

return this.prisma.drinkAction.create({ data: createDrinkActionDto });

}

}

but there are other (maybe more efficient ) ways to do it.

It's probably more efficient to just attempt the update and catch any exceptions. We might not be able to tell which ID was invalid, but since this API won't be public, it's not essential to give a very detailed error message

LeventeFarkashazi

Please fix these, and update the branch. I think there should be no merge conflicts so you can simply do it on the PR-s github page (there is a button under the checks)

Tschonti · 2024-05-24T15:26:16Z

apps/backend/src/drink-action/drink-action.controller.ts

+  async findAll(): Promise<DrinkAction[]> {
+    return this.drinkActionsService.findAll();
+  }
+
+  @Get(':id')
+  async findOne(@Param('id', new ParseUUIDPipe()) id: string): Promise<DrinkAction> {
+    return this.drinkActionsService.findOne(id);
+  }


These should return a different DTO, that also includes the event and drink fields

Tschonti · 2024-05-24T15:26:49Z

apps/backend/src/drink-action/drink-action.service.ts

+  async findAll(): Promise<DrinkAction[]> {
+    return this.prisma.drinkAction.findMany({
+      include: { drink: true, event: true },
+    });
+  }
+
+  async findOne(id: string): Promise<DrinkAction> {
+    const drinkAction = await this.prisma.drinkAction.findUnique({
+      where: { id },
+      include: { drink: true, event: true },
+    });
+    if (!drinkAction) {
+      throw new NotFoundException('DrinkAction not found');
+    }
+    return drinkAction;
+  }


these should also return with that new DTO

Tschonti · 2024-05-24T15:27:55Z

apps/backend/src/drink-action/dto/update-drink-action.dto.ts

+
+import { CreateDrinkActionDto } from './create-drink-action.dto';
+
+export class UpdateDrinkActionDto extends PartialType(CreateDrinkActionDto) {}


drinkId and eventId should not be updateable, so remove those from this class with OmitType

Tschonti · 2024-05-24T15:28:56Z

package-lock.json

please delete this file. This is generated when you use a command like npm install .... We should be using yarn instead (e.g. yarn add ...)

…opefully

Tschonti

getting there, just some minor issues

Tschonti · 2024-07-02T15:46:17Z

apps/backend/src/drink-action/drink-action.service.ts

+        data: {
+          ...rest,
+          drink: { connect: { id: drinkId } },
+          event: { connect: { id: eventId } },
+        },


Connecting drink and event like this is probably not needed, since both the dto and the schema have drinkId and eventId fields, so the connection is established just by passing the dto to the data property. But it also doesn't hurt, at least this way the connection is more apparent just from looking at the code . So this can stay

Tschonti · 2024-07-02T15:47:22Z

apps/backend/src/drink-action/drink-action.service.ts

+          drink: { connect: { id: drinkId } },
+          event: { connect: { id: eventId } },
+        },
+        include: { drink: true, event: true },


This is not necessary here. We don't usually do anything with the return object of a POST request, so there's no need to add extra joins. Returning just the standard DrinkAction is fine

Tschonti · 2024-07-02T15:48:15Z

apps/backend/src/drink-action/drink-action.service.ts

+    } catch (error) {
+      if (error instanceof PrismaClientKnownRequestError) {
+        if (error.code === 'P2003') {
+          throw new BadRequestException('drinkId or eventId does not exist');
+        }
+      }
+      throw error;
+    }


When I tested this with invalid event or drink Ids, I got a different Prisma error code, so the response was 500 instead of 400.

i dont know how to fix this, can you tell me?

if you console.log the error inside the catch block and send a request with an invalid event or drink id, you'll see the error that prisma throws. You can use the code from that instead of P2003

Tschonti · 2024-07-02T15:48:42Z

apps/backend/src/drink-action/dto/update-drink-action.dto.ts

+export class UpdateDrinkActionDto extends OmitType(PartialType(CreateDrinkActionDto), [
+  'drinkId',
+  'eventId',
+] as const) {}


as const is not needed here

implement 10 and 11 issues

ae96b84

murtagh27 requested review from a team, Tschonti, LeventeFarkashazi, pem00 and VarMattew and removed request for a team May 22, 2024 22:36

LeventeFarkashazi reviewed May 23, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

LeventeFarkashazi reviewed May 23, 2024

View reviewed changes

apps/backend/src/drink-action/entities/drink-action.entity.ts Outdated Show resolved Hide resolved

corrected package.json

165621f

LeventeFarkashazi reviewed May 23, 2024

View reviewed changes

Update apps/backend/src/drink-action/entities/drink-action.entity.ts

b31d097

Co-authored-by: Farkasházi Levente <[email protected]>

LeventeFarkashazi reviewed May 23, 2024

View reviewed changes

apps/backend/src/drink-action/entities/drink-action.entity.ts Outdated Show resolved Hide resolved

extra validation added in drink-action.entity

16609b3

LeventeFarkashazi reviewed May 23, 2024

View reviewed changes

LeventeFarkashazi requested changes May 23, 2024

View reviewed changes

Tschonti reviewed May 24, 2024

View reviewed changes

This was linked to issues Jun 24, 2024

Create DrinkAction controller and DTO #10

Closed

Create DrinkAction service #11

Closed

i made a new dto and also fixed some other things connected to that h…

0897090

…opefully

murtagh27 requested review from LeventeFarkashazi and Tschonti July 1, 2024 21:16

Tschonti requested changes Jul 2, 2024

View reviewed changes

murtagh27 and others added 2 commits July 3, 2024 20:32

error handling corrected

d04d514

Merge branch 'main' into lilla

39eaae0

Tschonti approved these changes Jul 3, 2024

View reviewed changes

Tschonti merged commit 4c052c7 into main Jul 3, 2024
3 checks passed

Tschonti deleted the lilla branch July 3, 2024 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement 10 and 11 issues #15

implement 10 and 11 issues #15

murtagh27 commented May 22, 2024

LeventeFarkashazi May 23, 2024

murtagh27 May 23, 2024

LeventeFarkashazi May 23, 2024

Tschonti May 23, 2024

murtagh27 May 23, 2024

Tschonti May 24, 2024

LeventeFarkashazi May 23, 2024

Tschonti May 23, 2024

LeventeFarkashazi left a comment

Tschonti May 24, 2024

Tschonti May 24, 2024

Tschonti May 24, 2024

Tschonti May 24, 2024

Tschonti left a comment

Tschonti Jul 2, 2024

Tschonti Jul 2, 2024

Tschonti Jul 2, 2024

murtagh27 Jul 2, 2024

Tschonti Jul 2, 2024

Tschonti Jul 2, 2024

-  async create(createDrinkActionDto: CreateDrinkActionDto) {
-    return this.prisma.drinkAction.create({ data: createDrinkActionDto });
-  }
+  async create(createDrinkActionDto: CreateDrinkActionDto) {
+    const { drinkId, eventId } = createDrinkActionDto;
+    const [drinkExists, eventExists] = await Promise.all([
+      this.prisma.drink.findUnique({ where: { id: drinkId } }),
+      this.prisma.event.findUnique({ where: { id: eventId } }),
+    ]);
+    if (!drinkExists) {
+      throw new BadRequestException(`Drink with ID ${drinkId} does not exist.`);
+    }
+    if (!eventExists) {
+      throw new BadRequestException(`Event with ID ${eventId} does not exist.`);
+    }
+    return this.prisma.drinkAction.create({ data: createDrinkActionDto });
+  }
+  }


		import { CreateDrinkActionDto } from './create-drink-action.dto';

		export class UpdateDrinkActionDto extends PartialType(CreateDrinkActionDto) {}

implement 10 and 11 issues #15

implement 10 and 11 issues #15

Conversation

murtagh27 commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeventeFarkashazi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tschonti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment