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

refactor: osiv 옵션 해제 및 swagger 문서 정리 #70

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

kth990303
Copy link
Member

구현사항

  1. osiv 옵션을 해제해도 비밀번호 변경이 가능하도록 변경 (https://kth990303.tistory.com/427)

image

  1. AttendanceServiceTest에서 불필요한 @Transactional 제거
  1. 관리자 페이지에서 작동되는 동작임에도 불구하고 swagger operation에 [관리자 페이지]가 붙어있지 않던 명세 수정

@kth990303 kth990303 added 문서화 문서 작성 또는 API 문서 자동화 관련 이슈 테스트 테스트 추가 및 수정 관련 이슈 리팩터링 코드 스멜 개선 labels Mar 1, 2023
@kth990303 kth990303 self-assigned this Mar 1, 2023
val token = AuthorizationHeaderUtils.extractBearerToken(webRequest)
val userEmail = jwtTokenProvider.getPayload(token)
return memberService.getByEmail(userEmail)
return LoggedInMemberRequest(userEmail)
Copy link
Member Author

Choose a reason for hiding this comment

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

AS-IS
LoginUserResolver에서 Entity 직접 반환

TO-DO
LoginUserResolver에서 payload 값을 담는 DTO를 반환

osiv를 끄면 영속성 컨텍스트 라이프사이클이 비즈니스 레이어(service layer)이외에는 꺼져있으므로 DTO를 반환하게 했습니다. 이후 서비스 로직에서 엔티티를 찾아와 영속화시켜 비밀번호 변경 등, 변경 로직을 성공하게 했습니다.

@@ -18,23 +18,23 @@ class GenerationController(
private val generationService: GenerationService
) {

@Operation(summary = "현재 기수 조회")
@Operation(summary = "[관리자 페이지] 현재 기수 조회")
Copy link
Member Author

Choose a reason for hiding this comment

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

GenerationController에서 [관리자 페이지]문구가 모두 누락되어 있어 추가해주었습니다.

Comment on lines +182 to +186
val updatedAttendance = attendanceRepository.findByIdOrNull(attendance.id)
val updatedGenerationMember = generationMemberRepository.findByIdOrNull(generationMember.id)
updatedAttendance?.attendanceStatus shouldBe AttendanceStatus.TARDY
updatedAttendance?.scoreChanged shouldBe AttendanceStatus.TARDY.penaltyScore
updatedGenerationMember?.score shouldBe MAX_SCORE + AttendanceStatus.TARDY.penaltyScore
Copy link
Member Author

Choose a reason for hiding this comment

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

테스트 코드에서 @Transactional을 제거하면 영속성 컨텍스트 관리 대상에서 제거되므로 영속 상태로 끌어올려 변경 여부를 제대로 테스트할 수 있도록 했습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

고치려다가 깨지는 테스트 고칠 시간이 없어서 보류했었는데 잘했소

Comment on lines +182 to +186
val updatedAttendance = attendanceRepository.findByIdOrNull(attendance.id)
val updatedGenerationMember = generationMemberRepository.findByIdOrNull(generationMember.id)
updatedAttendance?.attendanceStatus shouldBe AttendanceStatus.TARDY
updatedAttendance?.scoreChanged shouldBe AttendanceStatus.TARDY.penaltyScore
updatedGenerationMember?.score shouldBe MAX_SCORE + AttendanceStatus.TARDY.penaltyScore
Copy link
Collaborator

Choose a reason for hiding this comment

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

고치려다가 깨지는 테스트 고칠 시간이 없어서 보류했었는데 잘했소

Comment on lines 27 to 28
@LoggedInMember member: LoggedInMemberRequest,
@RequestBody @Valid request: ValidateQrCodeRequest,
Copy link
Collaborator

Choose a reason for hiding this comment

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

어노테이션명과 매개변수명이 아직 좀 entity스러운데 적절한 네이밍이 떠오른다면 수정하는게 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

이거 진짜 고민되더라고요ㅋㅋㅋ 결국 request를 붙이긴 했는데 뭐가 좋을까요..?

Copy link
Member Author

Choose a reason for hiding this comment

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

dto 제거해서 네이밍 고민할 필요가 사라졌습니다~

Comment on lines 4 to 5

data class LoggedInMemberRequest(val email: String)
Copy link
Collaborator

Choose a reason for hiding this comment

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

LoggedInMemberRequest라는 클래스로 감싸지 않고 email 문자열을 그대로 반환하는건 어떨까요?

getter를 써야하는 자바가 아니라 코틀린이라서 충분히 가독성이 높은 것 같긴한데, 그냥 이 클래스가 꼭 필요한가?라는 생각이 들었어요.

Copy link
Member Author

Choose a reason for hiding this comment

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

일리 있는 의견이라 생각합니다. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
리팩터링 코드 스멜 개선 문서화 문서 작성 또는 API 문서 자동화 관련 이슈 테스트 테스트 추가 및 수정 관련 이슈
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants