-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/backend/room user relation #41
Conversation
add to User table Room column add to Room table User id column
Your existing walkthrough is comprehensive and aligns with the provided instructions. Therefore, it doesn't require any updates. Here's the poem to complement the changes:
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- backend/prisma/migrations/20231111115551_add_oweer_id/migration.sql (1 hunks)
- backend/prisma/schema.prisma (1 hunks)
- backend/prisma/seed.ts (1 hunks)
- backend/src/room/dto/create-room-request.dto.ts (1 hunks)
- backend/src/room/dto/create-room.dto.ts (1 hunks)
- backend/src/room/entities/room.entity.ts (1 hunks)
- backend/src/room/room.controller.ts (1 hunks)
- backend/src/room/room.service.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- backend/src/room/room.service.ts
Additional comments: 14
backend/src/room/entities/room.entity.ts (2)
5-7: The constructor allows for partial object assignment, which can be useful for creating new instances of
RoomEntity
with only a subset of properties. This is a good practice for flexibility.14-14: The
ownerId
property has been added to theRoomEntity
class. Ensure that all references toRoomEntity
in the codebase are updated to handle this new property.backend/prisma/seed.ts (1)
- 62-75: The changes in this hunk are consistent with the pull request summary. The
ownerId
field is added to thecreate
calls forroom1
androom2
, and theid
ofuser1
anduser2
are assigned respectively. This establishes the many-to-one relationship betweenUser
andRoom
as described in the pull request summary.backend/prisma/schema.prisma (2)
15-19: The
User
model now includes aroom
field, establishing a many-to-one relationship with theRoom
model. Ensure that this change is reflected in all parts of the codebase where theUser
model is used.22-26: The
Room
model now includes anownerId
field and a corresponding relation to theUser
model. This establishes a one-to-many relationship fromUser
toRoom
. Ensure that this change is reflected in all parts of the codebase where theRoom
model is used.backend/prisma/migrations/20231111115551_add_oweer_id/migration.sql (4)
1-7: The warnings indicate potential issues with the migration. Ensure that the
Room
table is empty before running this migration, and that there are no duplicateownerId
values.9-9: Adding a non-nullable
ownerId
column to theRoom
table. Ensure that all futureRoom
entries will have anownerId
.12-12: Creating a unique index on
ownerId
. This will help with performance when querying byownerId
.15-15: Adding a foreign key constraint on
ownerId
referencingUser
(id
). This ensures data integrity betweenRoom
andUser
tables. TheON DELETE RESTRICT
clause prevents a user from being deleted if they own a room, and theON UPDATE CASCADE
clause updates theownerId
in theRoom
table if theid
in theUser
table is updated.backend/src/room/dto/create-room-request.dto.ts (1)
- 9-13: The
CreateRoomRequestDto
class is well defined with appropriate decorators for validation and API documentation. Thename
property is correctly typed asstring
.backend/src/room/room.controller.ts (3)
10-11: The addition of
UseGuards
andReq
imports from@nestjs/common
is appropriate for the changes made in thecreate
method ofRoomController
.19-19: The import of
JwtAuthGuard
from'src/auth/jwt-auth.guard'
is necessary for thecreate
method to useJwtAuthGuard
.26-32: The
create
method inRoomController
has been updated to useJwtAuthGuard
andApiBearerAuth
, and its parameters have been changed fromcreateRoomDto
tocreateRoomRequestDto
and@Req() request: Request
. This change is consistent with the pull request summary and seems to be a part of the effort to establish a many-to-one relationship betweenUser
andRoom
.backend/src/room/dto/create-room.dto.ts (1)
- 4-13: The
CreateRoomDto
class has been updated to remove theid
property and add anownerId
property with aIsNumber
validator. Ensure that all instances where this DTO is used have been updated to reflect these changes.
a room have many users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (7)
- backend/prisma/migrations/20231113020147_many_to_many_userroom/migration.sql (1 hunks)
- backend/prisma/migrations/20231113023444_add_role_column_user/migration.sql (1 hunks)
- backend/prisma/schema.prisma (1 hunks)
- backend/prisma/seed.ts (1 hunks)
- backend/src/room/dto/create-room.dto.ts (1 hunks)
- backend/src/room/room.controller.ts (1 hunks)
- backend/src/room/room.service.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- backend/prisma/seed.ts
Additional comments: 9
backend/src/room/dto/create-room.dto.ts (1)
- 1-13: The
CreateRoomDto
class has been updated to include auserId
property. This change is in line with the updates to theRoom
andUser
entities. Ensure that all instances whereCreateRoomDto
is used have been updated to include theuserId
property. Also, verify that theuserId
is being validated and sanitized before use.backend/prisma/schema.prisma (3)
15-19: The
User
model now includes arooms
field to represent the many-to-many relationship with theRoom
model. Ensure that all references to theUser
model in the codebase are updated to handle this new field.22-25: The
Room
model now includes ausers
field to represent the many-to-many relationship with theUser
model. Ensure that all references to theRoom
model in the codebase are updated to handle this new field.27-34: The new
useronroom
model establishes a many-to-many relationship betweenUser
andRoom
with an additionalrole
field. Ensure that all references to theUser
andRoom
models in the codebase are updated to handle this new relationship.backend/prisma/migrations/20231113020147_many_to_many_userroom/migration.sql (4)
1-6: Ensure that the data in the
ownerId
column is not needed anymore or has been migrated to the new structure before applying this migration.7-14: The
ownerId
column is being dropped from theRoom
table. Make sure that this change is reflected in the data model and all the places where this column is being used.17-23: A new table
useronroom
is being created. Ensure that this table is properly integrated into the data model and that all necessary relationships are established.26-29: Foreign key constraints are being added to the
useronroom
table. Make sure that these constraints are properly handled in the application logic to avoid database errors.backend/src/room/room.controller.ts (1)
- 26-32: The
create
method now requires authentication and the request object is used to extract theuserId
. Ensure that all calls to this endpoint are updated to include the necessary authentication token in the request header.
backend/prisma/migrations/20231113023444_add_role_column_user/migration.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- backend/prisma/seed.ts (1 hunks)
- backend/src/room/dto/create-room-request.dto.ts (1 hunks)
- backend/src/room/entities/room.entity.ts (1 hunks)
- backend/src/room/room.controller.ts (1 hunks)
- backend/src/room/room.service.ts (1 hunks)
Additional comments: 8
backend/prisma/seed.ts (1)
- 62-84: The changes look good. The code is connecting users to rooms as expected. However, ensure that the
users
field and theconnect
method are correctly implemented in the Prisma client and that theid
field is available in the user objects.backend/src/room/dto/create-room-request.dto.ts (1)
- 1-9: The
CreateRoomRequestDto
class is well defined with appropriate validation decorators and API property decorator for thename
property. Ensure that thename
property is being used correctly in the service layer and that the validation decorators are working as expected.backend/src/room/entities/room.entity.ts (2)
5-7: The constructor allows for partial assignment of properties, which can be useful for creating instances with only a subset of properties. However, be aware that this approach can lead to instances with missing required properties. Ensure that all required properties are assigned before using the instance.
14-14: The
ownerId
property has been added to theRoomEntity
class. Ensure that this change is reflected in all places whereRoomEntity
instances are created or used.backend/src/room/room.controller.ts (4)
7-12: The import statements are well-organized and only import necessary modules.
26-28: The
RoomController
class is properly decorated with@Controller
and@ApiTags
, which is good for routing and API documentation.31-43: The
create
method is now guarded withJwtAuthGuard
and requires a bearer token for authentication. This is a good security practice. However, ensure that theJwtAuthGuard
is correctly implemented and that therequest['user']['id']
is always available after successful authentication.- userId: request['user']['id'], + userId: request.user?.id,
- 45-45: The
findAll
method is not guarded. If this is intentional and the method should be publicly accessible, then it's fine. Otherwise, consider adding appropriate guards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
レビューしました!わからないけど、結構コメント書いてしまた!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
口頭でも言いましたが、一つの機能(PR)の中でmigrationファイルが3つに分かれているのはわかりづらいので、(経過としてはいいですが)、一回migrationファイルを消してからprisma migrate dev
しなおすとかしてからgit rebaseし直すなどして欲しかったですね!
やり方はわかりますかね?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
分からない! todo
return this.roomService.create({ | ||
...createRoomRequestDto, | ||
userId: request['user']['id'], | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controllerにロジックは記入するのはいいのかな〜?よくわからぬ
create-room.dto
とcreate-room-request.dto
の二つあることに違和感を持ちました。
serviceファイルにロジックを記入するのがいいかな?こちらを参考に!
https://github.com/usatie/pong/blob/main/backend/src/user/user.service.ts#L17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こんな感じ?
this.roomService.create(createRoomDto, req.user);
参考になるのかどうかも全然怪しいけど
https://tkssharma.com/nestjs-playing-with-query-param-dto/
Summary by CodeRabbit