-
Notifications
You must be signed in to change notification settings - Fork 1
134 feat: 복수 전공, 편입생 예외 처리 추가 #147
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
Conversation
- 편입생 판별법: 학법 앞 2자리, 입학년도 뒷 2자리 비교
Summary of ChangesHello @pp8817, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 졸업 요건 조회 시스템의 견고성과 확장성을 크게 개선합니다. 복수전공 학생들의 졸업 요건 계산을 정확하게 지원하고, 편입생에 대한 명시적인 예외 처리를 통해 현재 지원 범위를 명확히 합니다. 또한, 졸업 요건 데이터 누락과 같은 중요한 오류 발생 시 상세한 진단 정보를 제공하여 문제 해결 시간을 단축하고, 서비스 코드의 구조를 개선하여 향후 기능 추가 및 유지보수를 용이하게 합니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
이 pull request는 복수 전공 및 편입생에 대한 졸업 요건 조회 로직을 개선하고, 관련 예외 처리 및 로깅을 강화하는 훌륭한 변경입니다. 특히 GraduationService를 여러 책임 단위의 메서드로 분리하여 코드 가독성과 유지보수성을 크게 향상했습니다. 또한 MdcCleanupFilter를 추가하여 MDC 컨텍스트를 안정적으로 관리하도록 개선한 점도 좋습니다. 몇 가지 코드 명확성과 일관성을 더 높일 수 있는 부분을 제안합니다.
| try { | ||
| return graduationQueryRepository.getDualMajorAreaProgress( | ||
| studentId, primaryMajorId, secondaryMajorId, admissionYear | ||
| ); | ||
| } catch (CommonException e) { | ||
| if (ErrorCode.GRADUATION_REQUIREMENTS_DATA_NOT_FOUND.code().equals(e.getCode())) { | ||
| throwGraduationRequirementNotFound( | ||
| student, | ||
| primaryMajorId, | ||
| secondaryMajorId, | ||
| admissionYear | ||
| ); | ||
| } | ||
| throw e; | ||
| } |
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.
getDualMajorProgressOrThrow 메서드 내 catch 블록의 구조가 다소 혼란을 줄 수 있습니다. throwGraduationRequirementNotFound 메서드는 항상 예외를 던지지만, 반환 타입이 void이기 때문에 코드 흐름을 파악하기 어렵습니다. 이로 인해 if 블록 다음에 오는 throw e;가 어떤 조건에서 실행되는지 즉시 이해하기 힘듭니다.
코드의 명확성을 높이기 위해 throwGraduationRequirementNotFound 메서드가 예외 객체를 생성하여 반환하고(newGraduationRequirementNotFoundException 등으로 이름 변경), 호출하는 쪽에서 throw 하도록 수정하는 것을 제안합니다. 이렇게 하면 제어 흐름이 명확해지고, 메서드의 역할(예외 생성)이 이름에 더 잘 드러나 유지보수성이 향상됩니다.
아래는 getDualMajorProgressOrThrow 메서드에 대한 제안이며, throwGraduationRequirementNotFound 메서드와 getSingleMajorProgressOrThrow의 호출 부분도 함께 수정되어야 합니다.
| try { | |
| return graduationQueryRepository.getDualMajorAreaProgress( | |
| studentId, primaryMajorId, secondaryMajorId, admissionYear | |
| ); | |
| } catch (CommonException e) { | |
| if (ErrorCode.GRADUATION_REQUIREMENTS_DATA_NOT_FOUND.code().equals(e.getCode())) { | |
| throwGraduationRequirementNotFound( | |
| student, | |
| primaryMajorId, | |
| secondaryMajorId, | |
| admissionYear | |
| ); | |
| } | |
| throw e; | |
| } | |
| try { | |
| return graduationQueryRepository.getDualMajorAreaProgress( | |
| studentId, primaryMajorId, secondaryMajorId, admissionYear | |
| ); | |
| } catch (CommonException e) { | |
| if (ErrorCode.GRADUATION_REQUIREMENTS_DATA_NOT_FOUND.code().equals(e.getCode())) { | |
| throw newGraduationRequirementNotFoundException( | |
| student, | |
| primaryMajorId, | |
| secondaryMajorId, | |
| admissionYear | |
| ); | |
| } | |
| throw e; | |
| } |
| private List<AreaProgressDto> getSingleMajorProgressOrThrow( | ||
| Student student, | ||
| UUID studentId, | ||
| Long departmentId, |
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.
getSingleMajorProgressOrThrow 메서드의 파라미터 이름인 departmentId를 primaryMajorId로 변경하는 것을 제안합니다. 이 메서드를 호출하는 resolveAreaProgress에서 primaryMajorId를 전달하고 있으며, 다른 메서드들(getDualMajorProgressOrThrow 등)에서도 일관되게 primaryMajorId라는 이름을 사용하고 있어 코드의 통일성과 가독성을 높일 수 있습니다.
| Long departmentId, | |
| Long primaryMajorId, |
|
|
||
| /** | ||
| * 졸업 요건 부재 예외 처리 메서드 | ||
| * MDC 정리는 GlobalExceptionHandler에서 수행됨 |
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.
Javadoc 주석에 "MDC 정리는 GlobalExceptionHandler에서 수행됨"이라고 명시되어 있지만, 이번 변경으로 추가된 MdcCleanupFilter가 실제 MDC 정리 작업을 수행합니다. 주석의 내용이 현재 구현과 일치하지 않으므로, MdcCleanupFilter에서 처리된다는 내용으로 수정하거나, 구체적인 구현을 명시하기보다 "요청 처리 마지막에 정리됨"과 같이 좀 더 일반적인 내용으로 변경하는 것이 좋겠습니다.
| * MDC 정리는 GlobalExceptionHandler에서 수행됨 | |
| * MDC는 요청 처리 마지막에 MdcCleanupFilter에 의해 정리됨 |
| STUDENT_NOT_FOUND("S01", "해당 학생이 존재하지 않습니다.", HttpStatus.NOT_FOUND), | ||
| INVALID_TARGET_GPA("S02", "유효하지 않은 목표 학점입니다.", HttpStatus.BAD_REQUEST), | ||
| STUDENT_ID_REQUIRED("S03", "Student ID는 필수입니다.", HttpStatus.BAD_REQUEST), | ||
| TRANSFER_STUDENT_UNSUPPORTED("T13", "편입생 학적 정보는 현재 지원되지 않습니다.", HttpStatus.UNPROCESSABLE_ENTITY), |
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.
TRANSFER_STUDENT_UNSUPPORTED 오류 코드의 prefix가 T로 시작하여 토큰 관련 오류(Token)와 혼동될 수 있습니다. 학생(Student) 관련 오류 코드들은 S prefix를 사용하고 있으므로, 일관성을 위해 S04와 같이 S prefix를 사용하는 코드로 변경하는 것을 권장합니다. 이는 오류 코드를 관리하고 식별하는 데 도움이 됩니다.
| TRANSFER_STUDENT_UNSUPPORTED("T13", "편입생 학적 정보는 현재 지원되지 않습니다.", HttpStatus.UNPROCESSABLE_ENTITY), | |
| TRANSFER_STUDENT_UNSUPPORTED("S04", "편입생 학적 정보는 현재 지원되지 않습니다.", HttpStatus.UNPROCESSABLE_ENTITY), |
hoooonshub
left a comment
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.
일단 피드백은 남겼는데 바로 approve는 해둘게!!
| if (primaryFiltered.isEmpty()) { | ||
| throw new CommonException(ErrorCode.GRADUATION_REQUIREMENTS_DATA_NOT_FOUND); | ||
| } | ||
|
|
||
| // 복수전공 졸업 요건 조회 | ||
| List<AreaRequirementDto> dualMajorReqs = getDualMajorRequirementsWithCache(primaryMajorId, secondaryMajorId, admissionYear); | ||
|
|
||
| // 복수 전공 졸업 요건 데이터 부재 시 404 예외 처리 | ||
| if (dualMajorReqs == null || dualMajorReqs.isEmpty()) { | ||
| throw new CommonException(ErrorCode.GRADUATION_REQUIREMENTS_DATA_NOT_FOUND); | ||
| } | ||
|
|
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.
여기서 주전공이랑 복수전공이랑 예외 똑같이 놔두면 이슈에서 작성해준 '복수전공 정보 누락인 422 예외'는 못 던지는거 아니야? 여기서는 다른 상황이야?
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.
이슈에 있는 복수전공 정보 누락 422 예외는 운영 서버에만 적용되는 사항입니다.
dev에는 복수전공에 대한 처리가 되어 있기 때문에, 졸업 요건 분석 로직에서
단일 전공, 복수 전공 각각의 경우로 예외 처리를 나누지 않고, 동일한 예외 처리를 한 후
졸업 요건 부재 예외 처리 메서드인 GraduationService.throwGraduationRequirementNotFound에서 단일/복수 전공 별 분기 처리를 통해 MDC 값을 다르게 주입하여 판별 가능하도록 했습니다.
| // 졸업 요건 관련 | ||
| GRADUATION_REQUIREMENTS_NOT_FOUND("G01", "졸업 요건 정보를 찾을 수 없습니다.", HttpStatus.NOT_FOUND), | ||
| GRADUATION_REQUIREMENTS_DATA_NOT_FOUND("G02", "사용자에게 맞는 졸업 요건 데이터가 존재하지 않습니다.", HttpStatus.NOT_FOUND), | ||
|
|
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.
기존에 사용하던게 G01인데, 혹시 몰라서 유지하긴 했어요
이제 사용 안하는 G01을 제거하겠습니다
| STUDENT_NOT_FOUND("S01", "해당 학생이 존재하지 않습니다.", HttpStatus.NOT_FOUND), | ||
| INVALID_TARGET_GPA("S02", "유효하지 않은 목표 학점입니다.", HttpStatus.BAD_REQUEST), | ||
| STUDENT_ID_REQUIRED("S03", "Student ID는 필수입니다.", HttpStatus.BAD_REQUEST), | ||
| TRANSFER_STUDENT_UNSUPPORTED("T13", "편입생 학적 정보는 현재 지원되지 않습니다.", HttpStatus.UNPROCESSABLE_ENTITY), |
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.
제미나이 피드백처럼 T13으로 둔 이유가 있어? 학생 예외면 S로 두는 게 더 직관적이 않을까 싶은데, 어떻게 생각해?
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.
저도 그 피드백을 받고 S로 두는게 더 좋을것 같긴 한데, 클라이언트에서 처리한 다음이라서 마음대로 바꿨다가 문제 생길 것 같아서 우선 건드리지는 않았어요.
프론트 쪽과 소통한 후 문제가 없다면 S04 정도로 변경하는게 좋아보입니다!
ISSUE #134
변경 사항
를 MDC에 기록하고 Sentry 태그로 승격
변경 의도
영향 범위
참고 사항
P1: 꼭 반영해주세요 (Request changes)
P2: 적극적으로 고려해주세요 (Request changes)
P3: 웬만하면 반영해 주세요 (Comment)
P4: 반영해도 좋고 넘어가도 좋습니다 (Approve)
P5: 그냥 사소한 의견입니다 (Approve)
🔗 Related Issue
Closes #134