-
Notifications
You must be signed in to change notification settings - Fork 2
Redesign/review scotch #305
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -918,7 +918,7 @@ | |
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -936,7 +936,7 @@ | |
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").FCMNotificationRequest" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").FCMNotificationRequest" | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -963,7 +963,7 @@ | |
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").UserNotification" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").UserNotification" | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -992,7 +992,7 @@ | |
| "schema": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/agreement/domain/UserAgreement\").Agreement" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/agreement/domain/UserAgreement\").Agreement" | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1010,7 +1010,7 @@ | |
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1035,7 +1035,7 @@ | |
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1087,7 +1087,7 @@ | |
| "schema": { | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1110,7 +1110,7 @@ | |
| "type": "null" | ||
| }, | ||
| { | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| "$ref": "#/components/schemas/import(\"/Users/panda/Desktop/develop/sparcs/otlplus-server/apps/server/src/modules/notification/domain/notification\").Notification" | ||
| } | ||
| ] | ||
| } | ||
|
|
@@ -2660,6 +2660,80 @@ | |
| } | ||
| } | ||
| }, | ||
| "/api/v2/reviews/{reviewId}": { | ||
| "put": { | ||
| "summary": "ReviewsV2Controller.updateReviewV2", | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/IReviewV2.UpdateResponseDto" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "parameters": [ | ||
| { | ||
| "name": "reviewId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| ], | ||
| "requestBody": { | ||
| "required": true, | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/IReviewV2.UpdateDto" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "/api/v2/reviews/{reviewId}/liked": { | ||
| "patch": { | ||
| "summary": "ReviewsV2Controller.patchReviewLike", | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/IReviewV2.UpdateResponseDto" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "parameters": [ | ||
| { | ||
| "name": "reviewId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| ], | ||
| "requestBody": { | ||
| "required": true, | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/IReviewV2.PatchLikedDto" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "/api/v2/timetables": { | ||
| "get": { | ||
| "summary": "TimetablesControllerV2.getTimetables", | ||
|
|
@@ -6240,9 +6314,6 @@ | |
| "direction": { | ||
| "type": "string", | ||
| "enum": ["ltr", "rtl"] | ||
| }, | ||
| "lang": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "required": [ | ||
|
|
@@ -6269,8 +6340,7 @@ | |
| "textBaseline", | ||
| "textAlign", | ||
| "canvas", | ||
| "direction", | ||
| "lang" | ||
| "direction" | ||
| ], | ||
| "additionalProperties": false | ||
| }, | ||
|
|
@@ -7702,6 +7772,56 @@ | |
| "required": ["lectureId", "content", "grade", "load", "speech"] | ||
| }, | ||
| "IReviewV2.CreateResponseDto": {}, | ||
| "IReviewV2.UpdateDto": { | ||
| "properties": { | ||
| "content": { | ||
| "minLength": 1, | ||
| "type": "string" | ||
| }, | ||
| "grade": { | ||
| "maximum": 5, | ||
| "type": "number", | ||
| "minimum": 0 | ||
| }, | ||
| "load": { | ||
| "maximum": 5, | ||
| "type": "number", | ||
| "minimum": 0 | ||
| }, | ||
| "speech": { | ||
| "maximum": 5, | ||
| "type": "number", | ||
| "minimum": 0 | ||
| } | ||
| }, | ||
| "type": "object", | ||
| "required": ["content", "grade", "load", "speech"] | ||
| }, | ||
| "IReviewV2.UpdateResponseDto": { | ||
| "properties": { | ||
| "id": { | ||
| "minLength": 1, | ||
| "type": "number" | ||
|
Comment on lines
+7803
to
+7804
|
||
| } | ||
| }, | ||
| "type": "object", | ||
| "required": ["id"] | ||
| }, | ||
| "IReviewV2.PatchLikedDto": { | ||
| "properties": { | ||
| "reviewId": { | ||
| "minLength": 1, | ||
| "type": "number" | ||
|
Comment on lines
+7813
to
+7814
|
||
| }, | ||
| "action": { | ||
| "type": "string", | ||
| "enum": ["like", "unlike"], | ||
| "minLength": 1 | ||
| } | ||
| }, | ||
| "type": "object", | ||
| "required": ["reviewId", "action"] | ||
| }, | ||
| "ITimetableV2.GetListQueryDto": { | ||
| "properties": { | ||
| "user_id": { | ||
paisano55 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,4 +121,56 @@ export namespace IReviewV2 { | |
| @ApiProperty({ type: Number }) | ||
| id!: number | ||
| } | ||
|
|
||
| export class UpdateDto { | ||
| @ApiProperty() | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| @Validate(StringStripLength) | ||
| content!: string | ||
|
|
||
| @ApiProperty() | ||
| @IsNumber() | ||
| @IsNotEmpty() | ||
| @Min(0) | ||
| @Max(5) | ||
| @Type(() => Number) | ||
| grade!: number | ||
|
|
||
| @ApiProperty() | ||
| @IsNumber() | ||
| @IsNotEmpty() | ||
| @Min(0) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 코드 변화는 특정 필드의 유효성 검사에 대한 수정으로 보입니다. 그러나 몇 가지 잠재적인 문제점과 개선 사항이 있습니다.
따라서, 병합하기 전에 위의 개선 사항을 고려하길 권장합니다. |
||
| @Max(5) | ||
| @Type(() => Number) | ||
| load!: number | ||
|
|
||
| @ApiProperty() | ||
| @IsNumber() | ||
| @IsNotEmpty() | ||
| @Min(0) | ||
| @Max(5) | ||
| @Type(() => Number) | ||
| speech!: number | ||
| } | ||
|
|
||
| export class UpdateResponseDto { | ||
| @ApiProperty() | ||
| @IsNumber() | ||
| @IsNotEmpty() | ||
| id!: number | ||
| } | ||
|
|
||
| export class PatchLikedDto { | ||
| @ApiProperty() | ||
| @IsNumber() | ||
| @IsNotEmpty() | ||
| reviewId!: number | ||
|
|
||
| @ApiProperty({ enum: ['like', 'unlike'] }) | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| @IsIn(['like', 'unlike']) | ||
| action!: 'like' | 'unlike' | ||
| } | ||
| } | ||
paisano55 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||||||||
| import { | ||||||||||||
| Body, Controller, Get, HttpException, HttpStatus, Post, Query, Req, | ||||||||||||
| Body, Controller, Get, HttpException, HttpStatus, Param, Patch, Post, Put, Query, Req, | ||||||||||||
| } from '@nestjs/common' | ||||||||||||
| import { GetLanguage } from '@otl/server-nest/common/decorators/get-language.decorator' | ||||||||||||
| import { GetUser } from '@otl/server-nest/common/decorators/get-user.decorator' | ||||||||||||
|
|
@@ -48,4 +48,29 @@ export class ReviewsV2Controller { | |||||||||||
| const result = await this.reviewsV2Service.createReviewV2(reviewBody, user) | ||||||||||||
| return { id: result.id } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @Put('/:reviewId') | ||||||||||||
| async updateReviewV2( | ||||||||||||
| @Param('reviewId') reviewId: number, | ||||||||||||
| @Body() reviewBody: IReviewV2.UpdateDto, | ||||||||||||
| @GetUser() user: session_userprofile, | ||||||||||||
| ): Promise<IReviewV2.UpdateResponseDto> { | ||||||||||||
| if (!user) { | ||||||||||||
| throw new HttpException('Unauthorized', HttpStatus.UNAUTHORIZED) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return await this.reviewsV2Service.updateReviewV2(reviewId, reviewBody, user) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| @Patch('/:reviewId/liked') | ||||||||||||
| async patchReviewLike( | ||||||||||||
| @Param('reviewId') reviewId: number, | ||||||||||||
| @Body() body: IReviewV2.PatchLikedDto, | ||||||||||||
| @GetUser() user: session_userprofile, | ||||||||||||
| ): Promise<IReviewV2.UpdateResponseDto> { | ||||||||||||
| if (reviewId !== body.reviewId) { | ||||||||||||
| throw new HttpException('Path param and body id not match', HttpStatus.BAD_REQUEST) | ||||||||||||
| } | ||||||||||||
| return await this.reviewsV2Service.updateReviewLiked(body, user) | ||||||||||||
|
Comment on lines
+71
to
+74
|
||||||||||||
| if (reviewId !== body.reviewId) { | |
| throw new HttpException('Path param and body id not match', HttpStatus.BAD_REQUEST) | |
| } | |
| return await this.reviewsV2Service.updateReviewLiked(body, user) | |
| return await this.reviewsV2Service.updateReviewLiked(reviewId, body, user) |
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.
이 코드 패치에 대해 몇 가지 잠재적 버그 및 개선 사항을 지적하겠습니다.
-
입력 검증 부족:
@Param('reviewId') reviewId: number로 받아오는 reviewId의 입력 검증이 없습니다. API에서 유효한 숫자가 아닌 값이 들어올 경우 'NaN'이 발생할 수 있습니다. 이에 대한 검증 로직을 추가하는 것이 좋습니다. -
HTTP 메서드 적절성:
updateReviewV2메서드는 PUT 메서드를 사용하는데, RESTful API 스타일에 따르면 영속적 리소스의 전체 수정에 적합합니다. 그러나 이 메서드가 일부 속성만 수정할 경우 PATCH 메서드를 사용하는 것이 더 적절할 수 있습니다. -
비동기 호출 처리:
await로 호출하는 서비스 메서드에서 오류가 발생할 경우, 적절한 에러 핸들링이 필요합니다. 예를 들어 try-catch 블록을 사용하여 예외를 잡고, 관련된 사용자에게 더 명확한 에러 메시지를 제공할 수 있습니다. -
리턴 타입 일관성:
updateReviewV2메서드는 항상IReviewV2.UpdateResponseDto를 반환한다고 가정하고 있는데, 이 과정에서 서비스 메서드가 무엇을 반환하는지 확인해야 합니다. 반환값의 일관성을 보장하기 위해 확인이 필요합니다. -
주석 및 문서화 부족: 각 메서드에 대한 주석이 부족합니다. 이 코드의 목적과 동작, 파라미터에 대한 설명이 있으면 이해하기 쉬워집니다.
위 사항들을 고려하여 코드를 개선하는 것이 좋습니다.
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.
Schema references contain absolute file paths that are specific to a developer's local machine. These should use relative paths or symbolic references to ensure the Swagger documentation works consistently across different development environments.