feat: 스터디 출석체크 V2 API 작성 #942
Conversation
📝 WalkthroughWalkthrough이번 PR에서는 스터디 출석 관리 기능이 추가되고 개선되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Student
participant Controller as StudentAttendanceControllerV2
participant Service as StudentAttendanceServiceV2
participant StudyRepo as StudyV2Repository/StudyHistoryV2Repository
participant AttendanceRepo as AttendanceV2Repository
participant Validator as AttendanceValidatorV2
Student->>Controller: POST /v2/attendances/attend?studySessionId\n(AttendanceCreateRequest)
Controller->>Service: attend(studySessionId, request)
Service->>StudyRepo: findFetchBySessionId(studySessionId)
StudyRepo-->>Service: StudyV2 (with StudySessionV2)
Service->>StudyRepo: existsByStudentAndStudy(currentMember, StudyV2)
Service->>AttendanceRepo: existsByStudentAndStudySession(currentMember, StudySessionV2)
Service->>Validator: validateAttendance(StudySessionV2, attendanceNumber, isApplied, isAttended)
Validator-->>Service: (pass or throw exception)
Service->>AttendanceRepo: save(new AttendanceV2)
Service->>Controller: return ResponseEntity OK
Controller-->>Student: HTTP 200 OK
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @kckc0608, 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!
Summary of Changes
This pull request implements the Study Attendance Check V2 API. It includes a new controller, service, repository interfaces, a validator, and a request DTO. The changes involve validating attendance based on the StudySession's LessonPeriod instead of the StudyDetail's attendance date, as per the updated specifications. It also includes checks for study application status and existing attendance records.
Highlights
- New API Endpoint: A new API endpoint
/v2/studies/{studyId}/attendis created to handle student attendance checks for studies. - Attendance Validation: Attendance validation is now based on
StudySessionand itsLessonPeriod, ensuring checks for attendance period, attendance number, study application status, and existing attendance records. - Data Access Layers: New repository interfaces (
AttendanceV2Repository,StudyHistoryV2Repository) are added to handle data access for attendance and study history.
Changelog
Click here to see the changelog
- src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentStudyControllerV2.java
- Created a new
StudentStudyControllerV2to handle the attendance check API endpoint. - Implemented the
attendmethod to receivestudyId,studySessionId, andAttendanceCreateRequest. - The controller uses the
StudentStudyServiceV2to process the attendance check.
- Created a new
- src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java
- Created a new
StudentStudyServiceV2to handle the business logic for attendance checks. - Implemented the
attendmethod to validate and record student attendance. - The service retrieves study and session information, validates attendance, and saves the attendance record.
- Created a new
- src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/AttendanceV2Repository.java
- Created
AttendanceV2Repositoryinterface to manageAttendanceV2entities. - Added a method to check if a student has already attended a specific study session.
- Created
- src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/StudyHistoryV2Repository.java
- Added
existsByStudentAndStudymethod to check if a student has applied to a study.
- Added
- src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java
- Created
AttendanceValidatorV2to encapsulate attendance validation logic. - Implemented
validateAttendancemethod to check attendance period, number, study application, and existing attendance.
- Created
- src/main/java/com/gdschongik/gdsc/domain/studyv2/dto/request/AttendanceCreateRequest.java
- Created
AttendanceCreateRequestrecord to encapsulate the attendance number. - Added validation constraints to ensure the attendance number matches the expected format.
- Created
- src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java
- Added
ATTENDANCE_PERIOD_INVALIDerror code for invalid attendance periods.
- Added
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Trivia time!
What is the purpose of the @Transactional annotation in Spring?
Click here for the answer
The `@Transactional` annotation in Spring is used to define the scope of a transaction. When a method is annotated with `@Transactional`, Spring automatically begins a new transaction before the method execution and commits the transaction upon successful completion of the method. In the event of an exception, the transaction is rolled back.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The code changes introduce a new API endpoint for student attendance check-in. The implementation is generally well-structured, but I've left a few suggestions for improvement. It's recommended to address these comments before requesting a review from someone else. Feel free to request another review from Gemini via /gemini review when you have addressed these comments and I'll take another look! Remember to have others review and approve this code before merging.
Summary of Findings
- Exception Handling: Consider centralizing exception handling or providing more specific error messages for better debugging.
- Code Readability: Improve code readability by adding comments to explain complex logic or non-obvious operations.
- Logging: Ensure logging is consistent and provides sufficient information for monitoring and troubleshooting.
Assessment
The code introduces a new API endpoint for student attendance check-in, which is a valuable addition to the application. The implementation appears to be well-structured and includes necessary validations. However, there are a few areas where improvements can be made to enhance code clarity and maintainability. Addressing these comments before merging is recommended, and it's crucial to have another team member review and approve this code before merging.
| .filter(studySession -> Objects.equals(studySession.getId(), studySessionId)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new CustomException(STUDY_NOT_UPDATABLE_SESSION_NOT_FOUND)); |
There was a problem hiding this comment.
Consider adding a more descriptive message to the CustomException to provide better context when the StudySession is not found. This can aid in debugging and troubleshooting.
For example, include the studyId and studySessionId in the error message.
.orElseThrow(() -> new CustomException(STUDY_NOT_UPDATABLE_SESSION_NOT_FOUND, "StudySession not found for studyId: " + studyId + ", studySessionId: " + studySessionId));| @Pattern(regexp = ATTENDANCE_NUMBER, message = "출석번호는 " + ATTENDANCE_NUMBER + " 형식이어야 합니다.") | ||
| @Schema(description = "출석번호") | ||
| String attendanceNumber) {} |
There was a problem hiding this comment.
Consider extracting the message "출석번호는 " + ATTENDANCE_NUMBER + " 형식이어야 합니다." to a constant to avoid duplication and improve maintainability. This also makes it easier to update the message in one place if needed.
import static com.gdschongik.gdsc.global.common.constant.RegexConstant.ATTENDANCE_NUMBER;
import io.swagger.v3.oas.annotations.media.Schema;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.Pattern;
public record AttendanceCreateRequest(
@NotBlank
@Pattern(regexp = ATTENDANCE_NUMBER, message = AttendanceCreateRequest.ATTENDANCE_NUMBER_MESSAGE)
@Schema(description = "출석번호")
String attendanceNumber) {
private static final String ATTENDANCE_NUMBER_MESSAGE = "출석번호는 " + ATTENDANCE_NUMBER + " 형식이어야 합니다.";
}
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentStudyControllerV2.java (1)
17-33: 컨트롤러 구현이 잘 되어 있습니다.컨트롤러 구현이 RESTful 원칙을 잘 따르고 있으며, 관심사 분리가 명확합니다. 컨트롤러는 HTTP 요청 처리만 담당하고 비즈니스 로직은 서비스 계층에 위임하고 있습니다.
한 가지 개선점으로는 Swagger 문서에 발생 가능한 오류 응답에 대한 정보를 추가하는 것이 좋겠습니다. API 사용자가 어떤 에러코드를 기대할 수 있는지 알 수 있도록
@ApiResponse애노테이션을 추가하는 것을 고려해보세요.src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java (1)
14-35: 검증 로직이 명확하게 구현되어 있습니다.검증 메서드가 책임 분리 원칙에 따라 명확하게 구현되어 있으며 필요한 모든 검증을 수행하고 있습니다.
다만 26번 라인의 주석이 실제 검증 내용과 일치하지 않습니다. 해당 부분은 출석체크 번호를 검증하는 것이 아니라 출석 중복 여부를 검증하는 로직입니다.
주석을 다음과 같이 수정하는 것이 좋겠습니다:
- // 출석체크 번호 검증 + // 출석 중복 검증 if (isAlreadyAttended) { throw new CustomException(STUDY_DETAIL_ALREADY_ATTENDED); }src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java (1)
39-42: 스터디 세션 조회 방식 개선 제안현재 코드는 스터디를 조회한 후 메모리에서 특정 세션을 필터링하는 방식으로 구현되어 있습니다. 스터디 세션이 많은 경우 성능 저하가 발생할 수 있습니다.
레포지토리에 스터디 ID와 세션 ID로 직접 세션을 조회하는 메서드를 추가하여 DB 수준에서 필터링하는 것이 더 효율적일 수 있습니다.
// StudySessionV2Repository에 다음 메서드 추가 제안 Optional<StudySessionV2> findByIdAndStudyId(Long sessionId, Long studyId);그리고 서비스 코드를 다음과 같이 수정:
- StudySessionV2 session = study.getStudySessions().stream() - .filter(studySession -> Objects.equals(studySession.getId(), studySessionId)) - .findFirst() - .orElseThrow(() -> new CustomException(STUDY_NOT_UPDATABLE_SESSION_NOT_FOUND)); + StudySessionV2 session = studySessionV2Repository.findByIdAndStudyId(studySessionId, studyId) + .orElseThrow(() -> new CustomException(STUDY_NOT_UPDATABLE_SESSION_NOT_FOUND));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentStudyControllerV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/AttendanceV2Repository.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/StudyHistoryV2Repository.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/dto/request/AttendanceCreateRequest.java(1 hunks)src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java(1 hunks)
🔇 Additional comments (5)
src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java (1)
140-140: 적절한 오류 코드 추가.
ATTENDANCE_PERIOD_INVALID오류 코드 추가는StudySession의LessonPeriod를 기반으로 출석 체크의 유효성을 검증하는 새로운 로직을 잘 지원합니다. 기존의ATTENDANCE_DATE_INVALID와 함께 출석 체크에 대한 보다 세밀한 검증이 가능해졌습니다.src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/StudyHistoryV2Repository.java (1)
12-12: 메서드 추가 적절.
existsByStudentAndStudy메서드는 Spring Data JPA 명명 규칙을 잘 따르고 있으며, 기존findByStudentAndStudy메서드와 일관성을 유지합니다. 특정 학생과 스터디의 연결 여부를 확인하기 위한 효율적인 방법을 제공합니다.src/main/java/com/gdschongik/gdsc/domain/studyv2/dto/request/AttendanceCreateRequest.java (1)
1-13: 유효성 검증이 포함된 적절한 요청 클래스.출석번호에 대한 유효성 검증(
@NotBlank,@Pattern)을 포함하고 있어 잘못된 입력을 방지할 수 있습니다.@Schema어노테이션을 통해 API 문서화도 잘 지원하고 있습니다.src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/AttendanceV2Repository.java (1)
8-11: 적절한 저장소 인터페이스 구현.
AttendanceV2Repository인터페이스는 Spring Data JPA 규칙을 잘 따르고 있으며,existsByStudentAndStudySession메서드는 특정 학생과 스터디 세션에 대한 출석 여부를 효율적으로 확인할 수 있게 해줍니다.src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java (1)
34-57: 트랜잭션 처리 및 비즈니스 로직 구현이 잘 되어 있습니다.
attend메서드의 전반적인 구현이 좋습니다. 적절한 트랜잭션 경계 설정, 예외 처리, 로깅 등이 잘 구현되어 있습니다. 특히 검증 로직을 별도의 클래스로 분리하여 단일 책임 원칙을 잘 지키고 있습니다.API의 핵심 목표인 "출석체크 날짜를
StudyDetail에서StudySession의LessonPeriod로 전환하여 검증하는 기능"이 잘 구현되어 있습니다.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java (2)
44-45: 변수명 및 로직 의도 명확화 권장
isAppliedToStudy와isAlreadyAttended라는 변수명은 간결하지만, 로직 의도를 더 명확하게 드러내려면hasJoinedStudy나hasAttendedSession등으로 표현을 고려해 보세요.
50-51: 새 엔티티 생성 후 반환값 처리 고려
출석 정보를 생성한 뒤 바로 저장만 하고 있습니다. 이후 작업을 위해 필요한 경우save()이후 생성된 엔티티(예: 출석 ID)를 반환하는 방안을 고려해 보시면 좋겠습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentStudyControllerV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java
- src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentStudyControllerV2.java
🔇 Additional comments (3)
src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java (3)
39-39: 에러 메시지에 식별자를 포함하는 것 고려하기
이미 이전 리뷰에서 제안되었듯이,CustomException(STUDY_NOT_FOUND)를 던질 때studySessionId또는studyId를 포함한 구체적인 에러 메시지를 제공하면 문제 파악에 유리합니다.
40-40:getStudySession결과 검증 필요성 확인
study.getStudySession(studySessionId)호출 시, 내부적으로 세션이 존재하지 않는 경우 예외가 던져지는지, 또는 null이 반환되는지 확인이 필요합니다. null 반환 시 예외 처리가 누락될 수 있습니다.
47-48: 동시성 처리 여부 확인
attendanceValidatorV2.validateAttendance()내부에서 이미 출석 여부를 검사하더라도, 동시에 여러 요청이 들어올 경우 중복 출석을 막는 로직이 충분히 처리되는지 재확인이 필요합니다.
|
|
||
| // 출석체크 번호 검증 | ||
| if (isAlreadyAttended) { | ||
| throw new CustomException(STUDY_DETAIL_ALREADY_ATTENDED); |
There was a problem hiding this comment.
STUDY_DETAIL_ALREADY_ATTENDED도 수정이 필요해 보입니다!
| Optional<StudyHistoryV2> optionalStudyHistory = | ||
| studyHistoryV2Repository.findByStudentAndStudy(currentMember, study); | ||
|
|
||
| boolean isAppliedToStudy = optionalStudyHistory.isPresent(); |
There was a problem hiding this comment.
이 부분은 exists 메서드로 처리하지 않은 이유가 있나요?
There was a problem hiding this comment.
원래 exists 로 처리했었는데, #938 로직과 비슷하게 가져가는 게 가독성이 좋을 것 같아서 통일했습니다
There was a problem hiding this comment.
938에서 exists를 사용하지 않은 이유는 존재 여부를 검증한 뒤 -> 내부 엔티티 정보를 나중에 get() 으로 가져와야 했기 때문입니다. 여기서는 엔티티 내부 정보가 필요없고 존재성만 검증하며 되기 때문에 exists를 사용하는 게 좋을 듯 합니다.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java (3)
26-27: 중복된 주석을 수정해주세요.두번째 검증 로직의 주석이 "출석체크 번호 검증"으로 첫번째 검증 로직과 동일합니다. 실제로는 출석 중복 여부를 확인하는 로직이므로 해당 주석을 "출석체크 중복 검증"과 같이 수정하는 것이 좋겠습니다.
- // 출석체크 번호 검증 + // 출석체크 중복 검증 if (isAlreadyAttended) { throw new CustomException(STUDY_SESSION_ALREADY_ATTENDED); }
14-15: 파라미터 명명 개선 고려출석 유효성 검증에 사용되는 불리언 파라미터들의 이름(
isAppliedToStudy,isAlreadyAttended)이 의미를 잘 전달하고 있습니다만, 더 일관된 네이밍을 위해isEnrolledInStudy와 같이 수정하는 것도 고려해 볼 수 있습니다.
14-35: 검증 로직 순서 개선 고려현재 검증 순서는 논리적이지만, 성능 최적화를 위해 가장 빠르게 실패할 수 있는 검증을 먼저 수행하는 것이 좋습니다. 예를 들어, 데이터베이스 조회 없이 검증할 수 있는
isAlreadyAttended나attendanceNumber검증을 먼저 수행하면 성능상 이점이 있을 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/gdschongik/gdsc/global/exception/ErrorCode.java
🔇 Additional comments (2)
src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java (2)
1-36: 코드 구조가 잘 정리되어 있습니다.도메인 서비스로서 출석 유효성 검증 로직이 캡슐화되어 있고, 각 검증 단계마다 적절한 예외 처리가 되어 있어 좋습니다.
DomainService어노테이션을 통해 레이어 구분이 명확합니다.
27-29: ErrorCode 수정 필요이전 리뷰에서 언급된 것처럼
STUDY_DETAIL_ALREADY_ATTENDED를STUDY_SESSION_ALREADY_ATTENDED로 수정해야 합니다. 현재 코드에서는 이미STUDY_SESSION_ALREADY_ATTENDED를 사용하고 있으므로, 이와 관련된 ErrorCode 클래스의 상수 정의도 확인해주세요.
| ATTENDANCE_NUMBER_MISMATCH(HttpStatus.CONFLICT, "출석번호가 일치하지 않습니다."), | ||
| STUDY_DETAIL_ALREADY_ATTENDED(HttpStatus.CONFLICT, "이미 출석 처리된 스터디입니다."), | ||
| STUDY_SESSION_ALREADY_ATTENDED(HttpStatus.CONFLICT, "이미 출석 처리된 스터디입니다."), |
There was a problem hiding this comment.
| STUDY_SESSION_ALREADY_ATTENDED(HttpStatus.CONFLICT, "이미 출석 처리된 스터디입니다."), | |
| STUDY_SESSION_ALREADY_ATTENDED(HttpStatus.CONFLICT, "이미 출석 처리된 스터디 회차입니다."), |
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
| } | ||
|
|
||
| // 스터디 신청 여부 검증 | ||
| if (!isAppliedToStudy) { |
There was a problem hiding this comment.
로직 상 변경은 아니지만, 신청여부 -> 중복출석 -> 출석가능여부 -> 출석번호 순으로 검증하는 게 의미 상으로 더 적절할 것 같습니다
| public void validateAttendance( | ||
| StudySessionV2 studySession, String attendanceNumber, boolean isAppliedToStudy, boolean isAlreadyAttended) { | ||
| // 출석체크 기간 검증 | ||
| if (!studySession.getLessonPeriod().isOpen()) { |
There was a problem hiding this comment.
isOpen 은 deprecation 처리하는 게 좋아보입니다.
대신 isWithin 같이 유즈케이스로부터 독립적인 메서드를 추가하고, now를 인자로 받아서 처리하면 좋을 것 같습니다. '주어진 값이 시작일시와 종료일시 사이에 있다' 라는 걸 '기간이 열려있다' 라고 하지는 않잖아요?
'열려있다' 라는 건 Recruitment 도메인 구현하면서 잘못 추출된 워딩입니다.
해당 모집회차가 '열려있다' 라는 표현이 여기로 넘어온 것이죠.
'기한'은 '열려있다' 라는 표현보다는 '이내에 있다' 라는 표현이 적절하고, 따라서 isWithin 이 적절한 네이밍으로 보입니다. 사실 전에 assignmentPeriod 구현하면서 별도로 이슈 파서 추가할까 고민했는데 선반영하면 좋을 것 같아서 남깁니다.
그리고 기존 isOpen 로직은 오른쪽 끝에 대해서 inclusive하게 동작하는데, exclusive하게 구현을 변경하는 게 더 올바를 것 같아요. 1일 00시 제출 건을 받게 되어버리면 일자가 바뀌기 때문에...
한편 도메인 서비스도 마찬가지로 now를 바깥에서 받아야 합니다.
과제 제출하기 로직 참고해주세요.
| StudySessionV2 studySession, String attendanceNumber, boolean isAppliedToStudy, boolean isAlreadyAttended) { | ||
| // 출석체크 기간 검증 | ||
| if (!studySession.getLessonPeriod().isOpen()) { | ||
| throw new CustomException(ATTENDANCE_PERIOD_INVALID); |
There was a problem hiding this comment.
이제 봤는데 에러코드 네이밍이 조금 이상하네요.
출석기한 이내에 있지 않는데, 이 에러코드만 보면 출석기한 자체가 invalid한 것처럼 네이밍이 되어있어요. 여기서 처리할 내용은 아닌 것 같고 투두 남기고 이슈 별도로 파면 좋을 것 같습니다.
-> 저번 과제 제출 PR 네이밍 참고해주세요. 네이밍이 스터디 쪽 API스럽지 않은 문제는 앞에
-> 스터디회차는 스터디 애그리거트의 자식 엔티티이므로, 단독으로 노출할 수 없습니다.
-> 앞에서 말했듯 (일반적으로) 애그리거트 하나 당 리소스 하나로 보고 있으니 해당 컨벤션을 따르면 될 것 같습니다. 출석이력은 수강이력과 별개입니다. |
| Optional<StudyHistoryV2> optionalStudyHistory = | ||
| studyHistoryV2Repository.findByStudentAndStudy(currentMember, study); | ||
|
|
||
| boolean isAppliedToStudy = optionalStudyHistory.isPresent(); |
There was a problem hiding this comment.
938에서 exists를 사용하지 않은 이유는 존재 여부를 검증한 뒤 -> 내부 엔티티 정보를 나중에 get() 으로 가져와야 했기 때문입니다. 여기서는 엔티티 내부 정보가 필요없고 존재성만 검증하며 되기 때문에 exists를 사용하는 게 좋을 듯 합니다.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
|
앞에 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java (1)
14-35: 🛠️ Refactor suggestion검증 순서 및 메서드 개선 필요
검증 순서가 의미적으로 더 자연스러운 흐름을 가지도록 재배열하는 것이 좋겠습니다:
- 스터디 신청 여부
- 중복 출석 여부
- 출석 가능 기간
- 출석 번호 일치
또한 다음과 같은 개선이 필요합니다:
public void validateAttendance( StudySessionV2 studySession, String attendanceNumber, boolean isAppliedToStudy, boolean isAlreadyAttended) { // 스터디 신청 여부 검증 if (!isAppliedToStudy) { throw new CustomException(STUDY_HISTORY_NOT_FOUND); } // 스터디 중복 출석체크 여부 검증 if (isAlreadyAttended) { throw new CustomException(STUDY_SESSION_ALREADY_ATTENDED); } // 출석체크 가능 기간 검증 - if (!studySession.getLessonPeriod().isOpen()) { + // TODO: isOpen() 메서드를 isWithin()으로 대체하는 리팩토링 필요 + if (!studySession.getLessonPeriod().isOpen()) { throw new CustomException(ATTENDANCE_PERIOD_INVALID); } // 출석체크 번호 검증 if (!studySession.getLessonAttendanceNumber().equals(attendanceNumber)) { throw new CustomException(ATTENDANCE_NUMBER_MISMATCH); } }이전 리뷰에서도 언급되었듯이,
isOpen()메서드는 deprecated 처리하고 대신isWithin()과 같이 유스케이스에 독립적인 메서드로 교체하는 것이 좋겠습니다. 또한 에러 코드 네이밍(ATTENDANCE_PERIOD_INVALID)도 더 명확한 것으로 변경을 고려해보세요.
🧹 Nitpick comments (2)
src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentAttendanceControllerV2.java (2)
16-32: API 엔드포인트 경로 설계 재고려 필요현재 설계된
/v2/attendances/attend경로는 중복적인 표현이 있고, PR 목표에서 언급된 대로 엔드포인트 명명 규칙에 대한 논의가 있었습니다. 리소스 중심 설계 원칙을 따르면, 다음과 같은 대안을 고려해볼 수 있습니다:
/v2/studies/{studyId}/sessions/{sessionId}/attendances- RESTful한 계층 구조/v2/study-sessions/{sessionId}/attendances- StudySession 중심/v2/attendances(현재) - 출석 중심PR 코멘트에서 언급된 것처럼 스터디 집계(aggregate)의 일관성을 고려하여 적절한 명명 규칙을 선택하는 것이 좋겠습니다.
24-31: 요청 구조 최적화 가능현재
studySessionId를@RequestParam으로 받고 있는데, 이는 REST API 설계 원칙에 따르면 URL 경로의 일부로 포함되는 것이 더 적절할 수 있습니다.-@PostMapping("/attend") -public ResponseEntity<Void> attend( - @RequestParam(name = "studySessionId") Long studySessionId, - @Valid @RequestBody AttendanceCreateRequest request) { +@PostMapping("/sessions/{studySessionId}") +public ResponseEntity<Void> attend( + @PathVariable Long studySessionId, + @Valid @RequestBody AttendanceCreateRequest request) {이렇게 변경하면 URL이
/v2/attendances/sessions/{studySessionId}가 되어 리소스 식별이 더 명확해집니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentAttendanceControllerV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/StudyHistoryV2Repository.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/domain/AttendanceValidatorV2.java(1 hunks)
🔇 Additional comments (5)
src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/StudyHistoryV2Repository.java (1)
12-12: 메서드 추가 적절합니다.학생과 스터디를 기준으로 존재 여부를 확인하는 메서드로, Spring Data JPA 네이밍 규칙을 잘 따르고 있습니다. 출석 체크 전에 해당 학생이 스터디에 신청했는지 검증하는 데 사용될 것으로 보입니다.
src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentStudyServiceV2.java (4)
35-38: 예외 메시지 개선 필요예외 발생 시 더 자세한 정보를 제공하면 디버깅과 문제 해결에 도움이 됩니다. studySessionId를 포함한 메시지를 추가하는 것이 좋겠습니다.
StudyV2 study = studyV2Repository .findFetchBySessionId(studySessionId) - .orElseThrow(() -> new CustomException(STUDY_NOT_FOUND)); + .orElseThrow(() -> new CustomException(STUDY_NOT_FOUND, + String.format("studySessionId=%d에 해당하는 스터디를 찾을 수 없습니다.", studySessionId)));
40-41: 효율적인 쿼리 사용
existsByStudentAndStudy와existsByStudentAndStudySession메서드를 사용하여 효율적으로 존재 여부를 확인하고 있습니다. 이는 전체 엔티티를 조회하지 않고 존재 여부만 확인하기 때문에 성능상 좋은 접근법입니다.
46-47: 정적 팩토리 메서드 사용 적절
AttendanceV2.create(currentMember, studySession)정적 팩토리 메서드를 사용하여 객체 생성을 추상화하고 있습니다. 이는 객체 생성 로직을 캡슐화하고 가독성을 높이는 좋은 패턴입니다.
49-53: 로깅 구현 적절중요한 비즈니스 이벤트(출석 체크)에 대한 로깅이 잘 구현되어 있습니다. 출석 ID와 사용자 ID를 포함하여 추적 가능성을 높였습니다.
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentAttendanceServiceV2.java (4)
49-52: 로그 메시지의 클래스명 불일치로그 메시지에 사용된 클래스명
[StudentStudyServiceV2]가 실제 클래스명StudentAttendanceServiceV2와 일치하지 않습니다.- log.info( - "[StudentStudyServiceV2] 스터디 출석체크: attendanceId={}, memberId={}", - attendance.getId(), - currentMember.getId()); + log.info( + "[StudentAttendanceServiceV2] 스터디 출석체크: attendanceId={}, memberId={}", + attendance.getId(), + currentMember.getId());
32-53: 메소드 반환값 개선 제안현재
attend메소드가 void를 반환하고 있어, 출석 생성 결과에 대한 정보를 호출자에게 제공하지 않습니다. 생성된AttendanceV2객체나 관련 정보를 반환하는 것이 좋을 수 있습니다.- @Transactional - public void attend(Long studySessionId, AttendanceCreateRequest request) { + @Transactional + public AttendanceV2 attend(Long studySessionId, AttendanceCreateRequest request) { // 기존 코드 유지 AttendanceV2 attendance = AttendanceV2.create(currentMember, studySession); attendanceV2Repository.save(attendance); log.info( "[StudentAttendanceServiceV2] 스터디 출석체크: attendanceId={}, memberId={}", attendance.getId(), currentMember.getId()); + + return attendance; }
43-44: 매개변수 명확성 개선
validateAttendance메소드에 전달되는 인자들의 의미가 메소드 호출만으로는 명확하지 않습니다. 각 매개변수의 의미를 더 명확히 하기 위해 Java 17의 레코드나 빌더 패턴을 고려해 볼 수 있습니다.현재 방식 대신, 다음과 같이 더 명시적으로 호출하는 것을 고려해 보세요:
attendanceValidatorV2.validateAttendance( studySession, attendanceNumber: request.attendanceNumber(), isStudentApplied: isAppliedToStudy, isAlreadyAttended: isAlreadyAttended);또는
AttendanceValidateRequest같은 별도의 값 객체를 만드는 것도 고려해볼 수 있습니다.
40-41: 유효성 검사 명확화
isAppliedToStudy와isAlreadyAttended변수는 의미는 명확하지만, 이 두 조건이 어떤 오류 상황과 연결되는지 주석으로 설명하면 코드 이해가 더 쉬울 것입니다.+ // 학생이 해당 스터디에 신청했는지 확인 (신청하지 않은 학생은 출석 불가) boolean isAppliedToStudy = studyHistoryV2Repository.existsByStudentAndStudy(currentMember, study); + // 이미 해당 세션에 출석했는지 확인 (중복 출석 방지) boolean isAlreadyAttended = attendanceV2Repository.existsByStudentAndStudySession(currentMember, studySession);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentAttendanceControllerV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentAttendanceServiceV2.java(1 hunks)src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/StudyHistoryV2Repository.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/gdschongik/gdsc/domain/studyv2/dao/StudyHistoryV2Repository.java
- src/main/java/com/gdschongik/gdsc/domain/studyv2/api/StudentAttendanceControllerV2.java
🔇 Additional comments (2)
src/main/java/com/gdschongik/gdsc/domain/studyv2/application/StudentAttendanceServiceV2.java (2)
39-39: StudySession이 존재하지 않는 경우 예외 처리 추가 필요
studySession변수가 null인 경우에 대한 예외 처리가 없습니다.getStudySession메소드가 세션을 찾지 못할 경우 발생할 수 있는 문제입니다.// 이런 식으로 명시적인 예외 처리를 추가하는 것이 좋습니다 if (studySession == null) { throw new CustomException(ErrorCode.STUDY_SESSION_NOT_FOUND); }
21-24: 클래스 역할과 책임 검토이 클래스는 이름과 함께 출석체크 기능만 담당하고 있어 단일 책임 원칙(SRP)을 잘 따르고 있습니다. 다만, 이후 추가될 출석 관련 기능(수정, 삭제, 조회 등)에 대한 계획도 미리 고려해보는 것이 좋겠습니다.
| attendanceV2Repository.save(attendance); | ||
|
|
||
| log.info( | ||
| "[StudentStudyServiceV2] 스터디 출석체크: attendanceId={}, memberId={}", |
There was a problem hiding this comment.
| "[StudentStudyServiceV2] 스터디 출석체크: attendanceId={}, memberId={}", | |
| "[StudentAttendanceServiceV2] 스터디 출석체크: attendanceId={}, memberId={}", |
🌱 관련 이슈
📌 작업 내용 및 특이사항
📝 참고사항
StudyDetail에 있는 출석체크 날짜를 기반으로 검증했는데, 바뀐 명세에 따라StudySession에 있는LessonPeriod기반으로 출석 가능 기간 검증을 진행했습니다./studies로 뒀는데, studyId 없이 바로/attend로 끝내고 study session id 를 쿼리로 받으니까 어색한 것 같더라구요./study-sessions로 해볼까 해도 그때는 쿼리 파라미터보다 패스 파라미터로 받아서/attend로 끝내는 게 더 자연스러울 것 같고,/study-histories로 빼면 과거 수강 이력에 대한 api 와 섞이는 것 같아 고민되는데 좋은 네이밍 아이디어 있을까요..📚 기타
Summary by CodeRabbit