Skip to content
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

114 학생 활동 상세 조회 api 개발 #122

Merged
merged 15 commits into from
Nov 12, 2023

Conversation

KimTaeO
Copy link
Collaborator

@KimTaeO KimTaeO commented Nov 10, 2023

💡 개요

학생 활동 상세 조회 api를 작성하였습니다

📃 작업내용

학생 활동 api를 통해 학생 활동을 상세 조회할 수 있도록 하였습니다
해당 학생 활동과 관련된 역할과 어드민만 조회할 수 있도록 하였습니다

@KimTaeO KimTaeO self-assigned this Nov 10, 2023
@KimTaeO KimTaeO linked an issue Nov 10, 2023 that may be closed by this pull request
@KimTaeO KimTaeO added 1️⃣ Priority: High 우선순위 - 상 ✨ Feature 신규 기능 labels Nov 10, 2023
Comment on lines 80 to 84
@GetMapping("/{id}/detail")
fun queryStudentActivityDetail(@PathVariable id: UUID): ResponseEntity<StudentActivityDetailResponse> {
val response = studentActivityService.queryStudentActivityDetail(id)
return ResponseEntity.status(HttpStatus.OK).body(response)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detail 붙인 이유가 무엇인가요? id에 GET 하면 되는거 아닌가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

학생 활동을 학생 단위로 조회하는 api와 endpoint가 겹쳐서 detail을 붙이게 되었습니다

Comment on lines 45 to 47
data class StudentActivitiesByStudentResponse(
val activities: Page<StudentActivityResponse>
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 ByStudentResponse 말고 StudentActivitiesResponse 하나로 써주시는게 좋을 것 같아요 StudentsReponse처럼요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변경하였습니다
40dff6d

Comment on lines +246 to +254
when(entity) {
is Student -> {
if(entity != studentActivity.student)
throw ForbiddenStudentActivityException("해당 학생 활동에 대한 권한이 없습니다. info : [ userId = ${user.id} ]")
}
is Teacher -> {
if(entity != studentActivity.teacher)
throw ForbiddenStudentActivityException("해당 학생 활동에 대한 권한이 없습니다. info : [ userId = ${user.id} ]")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else일때도 처리해줘야하지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else 분기도 처리하였는데 검증로직이 따로 필요 없는 어드민의 경우에는 아무것도 수행하지 않도록 하였는데 어떻게 처리할지 좋은 생각이 있으시다면 피드백 부탁드립니다
427d8e2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else일때 말고 그럼 GOVERNMENT, COMPANY_INSTRUCTOR .... -> forbidden 이런식으로 가도 되고 when 대신 assert나 require, check 사용해서 해결해도 될거같네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0de932d
변경했는데 현재 InvalidRoleException이 401에러를 띄워주고 있는 상태인데 403이 더 적절한 상태코드 인것 같아서 수정하려고 하는데 괜찮을까요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidRoleException을 던지는 것 보단 ForbiddenStudentActivityException을 던지는게 맞지 않을까 싶어요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변경했습니다
a19ccb7

@JuuuuHong
Copy link
Collaborator

다른 도메인의 로직에서는 details 로 표현했는데 이번에는 detail 로 표현했네요! 이것도 한 번 정리하고 가는 게 좋을 것 같아요

KimTaeO and others added 4 commits November 11, 2023 18:50
@KimTaeO KimTaeO merged commit 1bfb6e8 into master Nov 12, 2023
@esperar esperar deleted the 114-feat/student-activity-detail-api branch November 13, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1️⃣ Priority: High 우선순위 - 상 ✨ Feature 신규 기능
Projects
None yet
Development

Successfully merging this pull request may close these issues.

학생 활동 상세 조회 API
4 participants