[BE] 질문 체크 기능 구현 #188
[BE] 질문 체크 기능 구현 #188
Conversation
- 프론트엔드의 요구사항에 맞게 수정
- questionId를 바탕으로 course 조회 및 validation 검사 - isComplete를 true로 변경 - SSE로 학생에게 전송
- 학생 사용자가 질문 체크 시 교수에게 SSE 메시지로 전송
- 보편성을 위해 String 타입의 id를 받아 전송할 수 있도록 파라미터 타입 수정
WalkthroughThe changes in this pull request involve refactoring the SSE messaging system, including renaming methods and updating their parameters from Long to String. New functionality has been introduced for checking questions by both professors and students, with corresponding endpoints and service methods. The Changes
Sequence Diagram(s)Professor Question Check FlowsequenceDiagram
participant ProfCtrl as ProfessorQuestionController
participant ProfService as ProfessorQuestionService
participant Repo as QuestionRepository
participant SSE as SseMessageSender
ProfCtrl->>ProfService: checkQuestion(questionId)
ProfService->>Repo: getQuestion(questionId)
Repo-->>ProfService: Question
ProfService->>Repo: update question status (mark complete)
ProfService->>SSE: sendMessage(QuestionCheckSseRequest)
ProfService-->>ProfCtrl: HTTP 204 No Content
Student Question Check FlowsequenceDiagram
participant StudCtrl as StudentQuestionController
participant StudService as StudentQuestionService
participant Repo as QuestionRepository
participant SSE as SseMessageSender
StudCtrl->>StudService: checkQuestion(questionId, request)
StudService->>Repo: updateQuestion(studentId, courseId, questionId)
alt update successful
StudService->>SSE: sendMessage(QuestionCheckSseRequest)
else update failed
StudService-->>StudCtrl: error (Question not found)
end
StudService-->>StudCtrl: HTTP 204 No Content
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
back-end/reacton/src/main/java/com/softeer/reacton/domain/reaction/StudentReactionService.java (1)
23-35: Consider enhancing error handling for SSE message delivery.While the code handles course-related errors well, it might benefit from handling SSE message delivery failures to ensure notifications are reliably delivered.
Consider:
- Adding a retry mechanism for failed SSE message deliveries
- Implementing a fallback notification method (e.g., WebSocket or polling)
- Monitoring SSE connection health
- Adding metrics for tracking message delivery success/failure rates
Would you like me to provide an example implementation of these enhancements?
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/dto/QuestionCheckSseRequest.java (1)
8-10: Improve field naming and add validation.The field name
idis ambiguous. Consider renaming it to be more specific (e.g.,questionId). Also, since this is a request DTO, consider adding validation annotations.public class QuestionCheckSseRequest { - private Long id; + @NotNull(message = "Question ID cannot be null") + private Long questionId; }back-end/reacton/src/main/java/com/softeer/reacton/global/exception/code/QuestionErrorCode.java (1)
9-11: Consider adding more specific error codes.Given the question-checking feature's requirements, consider adding more specific error codes for scenarios like:
- Question already checked
- Invalid course or student ID
- Unauthorized access to question
public enum QuestionErrorCode implements ErrorCode { - QUESTION_NOT_FOUND(HttpStatus.NOT_FOUND, "해당 질문 정보를 찾을 수 없습니다."); + QUESTION_NOT_FOUND(HttpStatus.NOT_FOUND, "해당 질문 정보를 찾을 수 없습니다."), + QUESTION_ALREADY_CHECKED(HttpStatus.BAD_REQUEST, "이미 확인된 질문입니다."), + INVALID_COURSE_OR_STUDENT(HttpStatus.BAD_REQUEST, "유효하지 않은 수업 또는 학생 정보입니다."), + UNAUTHORIZED_QUESTION_ACCESS(HttpStatus.FORBIDDEN, "질문에 대한 접근 권한이 없습니다.");back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java (1)
14-17: Consider using JPA's built-in methods instead of custom query.The custom update query could be replaced with JPA's built-in methods for better maintainability:
- Use
findByStudentIdAndCourseAndIdto fetch the question- Update the entity using the setter
- Save using
savemethodThis approach would:
- Leverage JPA's entity management
- Trigger entity lifecycle events
- Maintain audit trails (if using
@EntityListeners)- @Modifying - @Query("UPDATE Question q SET q.isComplete = true " + - "WHERE q.studentId = :studentId AND q.course = :course AND q.id = :questionId") - int updateQuestion(@Param("studentId") String studentId, @Param("course") Course course, @Param("questionId") Long questionId); + Optional<Question> findByStudentIdAndCourseAndId(String studentId, Course course, Long id);back-end/reacton/src/main/java/com/softeer/reacton/domain/question/Question.java (3)
24-26: Use primitive boolean for non-null field.Since the field is marked as
nullable=false, use primitivebooleaninstead of wrapperBoolean.@Setter @Column(nullable = false) - private Boolean isComplete; + private boolean isComplete;
30-30: Translate Korean comment to English.For consistency and better international collaboration, consider translating the Korean comment to English.
- private Course course; // 수업 정보 (외래 키) + private Course course; // Course information (foreign key)
24-24: Consider using a more controlled approach to state changes.Instead of using
@Setter, consider using a more explicit method to control state changes.- @Setter @Column(nullable = false) private boolean isComplete; + + public void markAsComplete() { + this.isComplete = true; + }back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionController.java (1)
36-47: Consider adding request validation.While the implementation is correct, consider adding validation for the
questionIdparameter to ensure it's positive and within valid range.public ResponseEntity<Void> checkQuestion( - @PathVariable("questionId") Long questionId) { + @PathVariable("questionId") @Positive @NotNull Long questionId) {back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionController.java (2)
81-92: Enhance API documentation with more specific error descriptions.The API responses could be more descriptive to help API consumers better understand and handle errors:
responses = { - @ApiResponse(responseCode = "200", description = "성공적으로 전송했습니다."), - @ApiResponse(responseCode = "404", description = "수업을 찾을 수 없습니다."), - @ApiResponse(responseCode = "404", description = "질문을 찾을 수 없습니다."), - @ApiResponse(responseCode = "409", description = "아직 수업이 시작되지 않았습니다."), - @ApiResponse(responseCode = "500", description = "서버와의 연결에 실패했습니다.") + @ApiResponse(responseCode = "204", description = "질문이 성공적으로 체크되었습니다."), + @ApiResponse(responseCode = "404", description = "Course not found: 해당 수업을 찾을 수 없습니다."), + @ApiResponse(responseCode = "404", description = "Question not found: 해당 질문을 찾을 수 없거나 접근 권한이 없습니다."), + @ApiResponse(responseCode = "409", description = "Course not active: 수업이 아직 시작되지 않았습니다."), + @ApiResponse(responseCode = "500", description = "Internal server error: 서버 처리 중 오류가 발생했습니다.")
93-108: Add validation for questionId path parameter.Consider adding validation to ensure questionId is positive and not null.
public ResponseEntity<Void> checkQuestion( - @PathVariable("questionId") Long questionId, + @PathVariable("questionId") @Min(1) @NotNull Long questionId, HttpServletRequest request) {Also, the success response code should be 204 (NO_CONTENT) in the API documentation since that's what the implementation returns.
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java (2)
73-92: Add unit tests for the new sendQuestionCheck method.The new method requires test coverage to verify:
- Successful question check update
- Error handling for non-existent questions
- SSE message sending
Would you like me to generate the unit test cases for this new functionality?
79-83: Consider adding more context to error messages.When the question update fails, include more details in the error message to aid debugging.
if (updatedRows == 0) { - log.debug("질문 체크를 처리하는 과정에서 발생한 에러입니다. : Question does not exist."); + log.debug("질문 체크를 처리하는 과정에서 발생한 에러입니다. : Question not found or access denied. questionId={}, studentId={}, courseId={}", questionId, studentId, courseId); throw new BaseException(QuestionErrorCode.QUESTION_NOT_FOUND); }back-end/reacton-classroom/src/main/java/com/softeer/reacton_classroom/domain/sse/service/SseService.java (2)
46-59: Document the parameter type change and enhance error messages.The method signature change from courseId to id needs better documentation and error handling.
+ /** + * Sends a message to a specific recipient identified by their ID. + * @param id The recipient's identifier (can be either courseId or studentId) + * @param message The message to be sent + * @throws BaseException if the recipient is not found or message sending fails + */ public void sendMessage(String id, MessageResponse<?> message) { Sinks.Many<MessageResponse<?>> sink = sinks.get(id); if (sink == null) { - log.debug("전송 대상을 찾지 못했습니다. : Receiver not found."); + log.debug("전송 대상을 찾지 못했습니다. : Receiver not found for id={}", id); throw new BaseException(SseErrorCode.USER_NOT_FOUND); } Sinks.EmitResult result = sink.tryEmitNext(message); if (result.isFailure()) { - log.info("메시지 전송에 실패했습니다."); + log.info("메시지 전송에 실패했습니다. : Failed to send message to id={}, result={}", id, result); throw new BaseException(SseErrorCode.MESSAGE_SEND_FAILURE); } - log.info("메시지 전송에 성공했습니다."); + log.info("메시지 전송에 성공했습니다. : Successfully sent message to id={}", id); }
66-67: Consider implementing auto-reconnect functionality.The TODO comment indicates a need for auto-reconnect functionality on the frontend.
Would you like me to help create an issue to track the implementation of auto-reconnect functionality for SSE connections?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
back-end/reacton-classroom/src/main/java/com/softeer/reacton_classroom/domain/sse/controller/SseMessageController.java(1 hunks)back-end/reacton-classroom/src/main/java/com/softeer/reacton_classroom/domain/sse/service/SseService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/course/StudentCourseController.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionController.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/Question.java(2 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionController.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java(2 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/dto/QuestionCheckSseRequest.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/reaction/StudentReactionService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/request/StudentRequestService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/global/exception/code/QuestionErrorCode.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/global/sse/SseMessageSender.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java (1)
Learnt from: sunohkim
PR: softeer5th/Team3-PowerPenguin#149
File: back-end/reacton/src/main/java/com/softeer/reacton/domain/course/CourseRepository.java:29-29
Timestamp: 2025-02-18T03:09:12.819Z
Learning: In Spring Boot applications with JPA, prefer using built-in methods from JpaRepository (e.g. findById, save, delete) instead of creating custom methods with identical functionality to avoid redundancy.
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/Question.java (2)
Learnt from: sunohkim
PR: softeer5th/Team3-PowerPenguin#149
File: back-end/reacton/src/main/java/com/softeer/reacton/domain/course/CourseRepository.java:29-29
Timestamp: 2025-02-18T03:09:12.819Z
Learning: In Spring Boot applications with JPA, prefer using built-in methods from JpaRepository (e.g. findById, save, delete) instead of creating custom methods with identical functionality to avoid redundancy.
Learnt from: uri010
PR: softeer5th/Team3-PowerPenguin#69
File: back-end/reacton/src/main/java/com/softeer/reacton/domain/course/Course.java:97-104
Timestamp: 2025-02-07T00:57:09.147Z
Learning: The project uses Java 17, which supports modern language features like Stream's `.toList()` method.
🔇 Additional comments (10)
back-end/reacton/src/main/java/com/softeer/reacton/domain/reaction/StudentReactionService.java (1)
34-34: LGTM! The update aligns with the new SSE messaging API.The change correctly adapts to the new
sendMessageAPI by converting thecourseIdto a string.back-end/reacton/src/main/java/com/softeer/reacton/domain/course/StudentCourseController.java (1)
83-83:Details
✅ Verification successful
Verify frontend route compatibility for the new redirect URL.
The Location header URL has been changed from "students/classroom" to "student/course". This is a breaking change that requires corresponding updates in frontend routing.
Also note that while the controller's base path uses plural form ("/students/courses"), the redirect URL uses singular form ("student/course"). Consider maintaining consistent URL patterns across the application.
Run the following script to check for any other occurrences of these URL patterns and ensure consistency:
🏁 Script executed:
#!/bin/bash # Search for URL patterns in both frontend and backend code echo "Searching for URL patterns in the codebase..." rg -g '!*.{class,jar}' -i "student/course|students/classroom" # Check frontend route definitions echo "Checking frontend route definitions..." fd -e ts -e tsx -e js -e jsx --exec rg -l "Route|router.push|navigate|redirect"Length of output: 1105
Frontend routing compatibility confirmed.
The new redirect URL "student/course" is correctly aligned with the front-end route definitions, as confirmed in the following:
- Backend: The Location header now sets the URL to
FRONTEND_BASE_URL + "student/course"inStudentCourseController.java.- Frontend: The route corresponding to
/student/courseis used consistently (e.g., infront-end/src/pages/student/home/StudentHome.tsxand the import infront-end/src/App.tsx).Although the controller's base path uses the plural form ("/students/courses"), the update to a singular redirect URL appears intentional given the front-end usage. Please ensure that this naming difference is deliberate and that all team members are aware of the convention to avoid potential confusion in the future.
back-end/reacton/src/main/java/com/softeer/reacton/domain/request/StudentRequestService.java (1)
45-45:Details
✅ Verification successful
LGTM! Consider adding null checks.
The change to use a more generic
sendMessagemethod with string identifiers aligns well with the PR objectives for implementing SSE notifications for both students and professors.Consider adding null checks for
courseIdbefore conversion to string:- sseMessageSender.sendMessage(courseId.toString(), sseMessage); + if (courseId != null) { + sseMessageSender.sendMessage(courseId.toString(), sseMessage); + } else { + log.error("courseId is null"); + throw new BaseException(CourseErrorCode.COURSE_NOT_FOUND); + }Let's verify that this change is consistently applied across the codebase:
🏁 Script executed:
#!/bin/bash # Search for all usages of sendMessage and sendMessageToProfessor rg -A 2 'sendMessage(To(Professor|Student)?)?\(' --type javaLength of output: 4295
SSE Messaging Update: Approved with a Minor Suggestion
The refactor to use the generic
sendMessagemethod with string IDs is consistent with the refactored pattern seen in other parts of the codebase and aligns with the SSE notification objectives. The usage is uniform (e.g., in StudentQuestionService and StudentReactionService), which confirms the consistency of the change.
Verified Usage:
StudentRequestService.javanow convertscourseIdto a string, similar to patterns found in other modules.- Other related files (e.g., StudentQuestionService.java, StudentReactionService.java) follow the same approach.
Suggestion for Improvement:
Consider adding a null check forcourseIdbefore conversion. This will safeguard against potential null pointer issues. For example:- sseMessageSender.sendMessage(courseId.toString(), sseMessage); + if (courseId != null) { + sseMessageSender.sendMessage(courseId.toString(), sseMessage); + } else { + log.error("courseId is null"); + throw new BaseException(CourseErrorCode.COURSE_NOT_FOUND); + }Overall, the change is LGTM. Please review the null-safety suggestion to ensure robust error handling.
back-end/reacton-classroom/src/main/java/com/softeer/reacton_classroom/domain/sse/controller/SseMessageController.java (2)
20-27: LGTM! Method rename improves clarity.The rename from
sendMessagetosendMessageToCoursebetter reflects the method's purpose and target recipient.
29-36: LGTM! New method for student messaging.The new
sendMessageToStudentmethod follows consistent patterns with the course messaging and properly handles student-specific message routing.back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionController.java (1)
24-35: LGTM! Well-documented API endpoint.The API documentation is thorough and clearly describes all possible response scenarios.
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java (2)
44-47: LGTM! Clean error handling.The
getQuestionmethod properly handles the not-found scenario with a custom exception.
49-53: LGTM! Clear course status validation.The
checkIfOpenmethod effectively validates course status before proceeding with question check.back-end/reacton/src/main/java/com/softeer/reacton/global/sse/SseMessageSender.java (1)
36-46: LGTM! Robust retry mechanism.The retry logic with exponential backoff is well implemented and properly handles WebClient exceptions.
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java (1)
66-66:Details
❓ Verification inconclusive
Verify the impact of the sendMessage signature change.
The method call has been updated to use toString() on courseId. Ensure all consumers of this API are updated to handle String IDs.
🏁 Script executed:
#!/bin/bash # Search for other usages of sendMessage to verify they've been updated rg -l "sendMessage.*courseId" --type javaLength of output: 445
Action Required: Verify Consistent Handling of String IDs in SSE API
Please ensure that the change of converting
courseIdto a string usingtoString()is reflected consistently across all usages of thesendMessageAPI. In addition to the update inStudentQuestionService.java, verify that the following files also correctly handle the new String type for thecourseIdargument:
back-end/reacton-classroom/src/main/java/com/softeer/reacton_classroom/domain/sse/controller/SseMessageController.javaback-end/reacton/src/main/java/com/softeer/reacton/domain/reaction/StudentReactionService.javaback-end/reacton/src/main/java/com/softeer/reacton/domain/request/StudentRequestService.javaDouble-check that each consumer converts the course ID to a string if needed, and update the unit tests accordingly.
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java
Show resolved
Hide resolved
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java
Show resolved
Hide resolved
...-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java
Show resolved
Hide resolved
back-end/reacton/src/main/java/com/softeer/reacton/global/sse/SseMessageSender.java
Outdated
Show resolved
Hide resolved
- '질문 체크 데이터를 database에 저장하는 과정'만을 트랜잭션 처리했고, 비동기 방식으로 SSE 요청을 보내는 기능은 트랜잭션에서 제외
📝 추가 작업 내용
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java (1)
23-41: 🛠️ Refactor suggestionConsider separating DB operations into a dedicated transaction.
The current implementation includes both DB update and SSE message sending within the same transaction boundary. This could lead to unnecessary transaction rollbacks if SSE message sending fails. Consider extracting the DB operations into a separate transactional method:
- public void sendQuestionCheck(Long questionId) { + public void sendQuestionCheck(Long questionId) { + Question question = updateQuestionStatus(questionId); + sendSseMessage(question); + } + + @Transactional + private Question updateQuestionStatus(Long questionId) { log.debug("질문 체크 처리를 시작합니다. : questionId = {}", questionId); Question question = getQuestion(questionId); Course course = question.getCourse(); checkIfOpen(course); - String studentId = question.getStudentId(); question.setIsComplete(true); - questionService.save(question); + return questionService.save(question); + } + private void sendSseMessage(Question question) { + String studentId = question.getStudentId(); QuestionCheckSseRequest questionCheckSseRequest = new QuestionCheckSseRequest(questionId); log.debug("SSE 서버에 질문 체크 전송을 요청합니다."); SseMessage<QuestionCheckSseRequest> sseMessage = new SseMessage<>("QUESTION_CHECK", questionCheckSseRequest); sseMessageSender.sendMessage(studentId, sseMessage); log.debug("질문 체크 처리가 완료되었습니다."); - }
🧹 Nitpick comments (3)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionService.java (2)
3-3: Consider using Spring's @transactional annotation.Replace
jakarta.transaction.Transactionalwithorg.springframework.transaction.annotation.Transactionalfor better integration with Spring's transaction management and access to additional features.-import jakarta.transaction.Transactional; +import org.springframework.transaction.annotation.Transactional;
13-16: Enhance method robustness with validation and logging.The save method could benefit from:
- Input validation to ensure the Question object is valid
- Exception handling for database operations
- Logging for tracking save operations
@Transactional public void save(Question question) { + if (question == null) { + throw new IllegalArgumentException("Question cannot be null"); + } + log.info("Saving question: {}", question.getId()); + try { questionRepository.save(question); + log.debug("Successfully saved question: {}", question.getId()); + } catch (Exception e) { + log.error("Failed to save question: {}", question.getId(), e); + throw e; + } }Don't forget to add the lombok @slf4j annotation to the class:
@Service @RequiredArgsConstructor +@Slf4j public class QuestionService {back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java (1)
31-32: Consider adding optimistic locking for concurrent updates.The current implementation might face race conditions if multiple professors attempt to check the same question simultaneously. Consider adding version-based optimistic locking to prevent concurrent modifications:
question.setIsComplete(true); +// Add @Version field to Question entity to enable optimistic locking questionService.save(question);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java(2 hunks)back-end/reacton/src/main/java/com/softeer/reacton/global/sse/SseMessageSender.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- back-end/reacton/src/main/java/com/softeer/reacton/global/sse/SseMessageSender.java
🧰 Additional context used
🧠 Learnings (1)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java (1)
Learnt from: sunohkim
PR: softeer5th/Team3-PowerPenguin#149
File: back-end/reacton/src/main/java/com/softeer/reacton/domain/course/CourseRepository.java:29-29
Timestamp: 2025-02-18T03:09:12.819Z
Learning: In Spring Boot applications with JPA, prefer using built-in methods from JpaRepository (e.g. findById, save, delete) instead of creating custom methods with identical functionality to avoid redundancy.
🔇 Additional comments (2)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionService.java (1)
7-8: LGTM! Good use of Lombok for constructor injection.The combination of
@Serviceand@RequiredArgsConstructorfollows Spring best practices for dependency injection.back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java (1)
15-19: LGTM! Well-structured update query with proper error handling.The implementation correctly uses
@Modifyingand@Transactionalfor the update operation, and returns the number of affected rows to handle the case where no matching question is found.
| public class QuestionService { | ||
|
|
||
| private final QuestionRepository questionRepository; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding methods for question checking functionality.
Based on the PR objectives, this service should handle question checking for both students and professors. Consider adding methods like:
checkQuestionByStudent(Long questionId, Long studentId, Long courseId)checkQuestionByProfessor(Long questionId)
These methods would encapsulate the different database access patterns mentioned in the PR description.
| log.debug("SSE 서버에 질문 전송을 요청합니다."); | ||
| SseMessage<CourseQuestionResponse> sseMessage = new SseMessage<>("QUESTION", courseQuestionResponse); | ||
| sseMessageSender.sendMessageToProfessor(courseId, sseMessage); | ||
| sseMessageSender.sendMessage(courseId.toString(), sseMessage); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the impact of type change in sendMessage.
The parameter type for sseMessageSender.sendMessage has changed from Long to String. Ensure that all callers of this method have been updated accordingly.
🏁 Script executed:
#!/bin/bash
# Search for all calls to sendMessage to verify type consistency
rg -A 2 'sseMessageSender\.sendMessage\(' Length of output: 1962
Action Required: Ensure consistent type conversion for sendMessage calls
The updated sendMessage method now expects a String rather than a Long. In the StudentQuestionService (and in the StudentRequestService and StudentReactionService), the course ID is correctly converted to a String using toString(). However, notice that in ProfessorQuestionService the call still passes studentId without conversion. Please verify if studentId should also be converted to a String and update the call accordingly to maintain consistency across all invocations.
| public void sendQuestionCheck(String studentId, Long courseId, Long questionId) { | ||
| log.debug("질문 체크 처리를 시작합니다. : questionId = {}", questionId); | ||
|
|
||
| Course course = getCourse(courseId); | ||
| checkIfOpen(course); | ||
|
|
||
| int updatedRows = questionRepository.updateQuestion(studentId, course, questionId); | ||
| if (updatedRows == 0) { | ||
| log.debug("질문 체크를 처리하는 과정에서 발생한 에러입니다. : Question does not exist."); | ||
| throw new BaseException(QuestionErrorCode.QUESTION_NOT_FOUND); | ||
| } | ||
|
|
||
| QuestionCheckSseRequest questionCheckSseRequest = new QuestionCheckSseRequest(questionId); | ||
|
|
||
| log.debug("SSE 서버에 질문 체크 전송을 요청합니다."); | ||
| SseMessage<QuestionCheckSseRequest> sseMessage = new SseMessage<>("QUESTION_CHECK", questionCheckSseRequest); | ||
| sseMessageSender.sendMessage(courseId.toString(), sseMessage); | ||
|
|
||
| log.debug("질문 체크 처리가 완료되었습니다."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction boundary for DB operations.
The sendQuestionCheck method should be annotated with @Transactional to ensure that the DB operations are executed within a transaction:
+ @Transactional
public void sendQuestionCheck(String studentId, Long courseId, Long questionId) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void sendQuestionCheck(String studentId, Long courseId, Long questionId) { | |
| log.debug("질문 체크 처리를 시작합니다. : questionId = {}", questionId); | |
| Course course = getCourse(courseId); | |
| checkIfOpen(course); | |
| int updatedRows = questionRepository.updateQuestion(studentId, course, questionId); | |
| if (updatedRows == 0) { | |
| log.debug("질문 체크를 처리하는 과정에서 발생한 에러입니다. : Question does not exist."); | |
| throw new BaseException(QuestionErrorCode.QUESTION_NOT_FOUND); | |
| } | |
| QuestionCheckSseRequest questionCheckSseRequest = new QuestionCheckSseRequest(questionId); | |
| log.debug("SSE 서버에 질문 체크 전송을 요청합니다."); | |
| SseMessage<QuestionCheckSseRequest> sseMessage = new SseMessage<>("QUESTION_CHECK", questionCheckSseRequest); | |
| sseMessageSender.sendMessage(courseId.toString(), sseMessage); | |
| log.debug("질문 체크 처리가 완료되었습니다."); | |
| } | |
| @Transactional | |
| public void sendQuestionCheck(String studentId, Long courseId, Long questionId) { | |
| log.debug("질문 체크 처리를 시작합니다. : questionId = {}", questionId); | |
| Course course = getCourse(courseId); | |
| checkIfOpen(course); | |
| int updatedRows = questionRepository.updateQuestion(studentId, course, questionId); | |
| if (updatedRows == 0) { | |
| log.debug("질문 체크를 처리하는 과정에서 발생한 에러입니다. : Question does not exist."); | |
| throw new BaseException(QuestionErrorCode.QUESTION_NOT_FOUND); | |
| } | |
| QuestionCheckSseRequest questionCheckSseRequest = new QuestionCheckSseRequest(questionId); | |
| log.debug("SSE 서버에 질문 체크 전송을 요청합니다."); | |
| SseMessage<QuestionCheckSseRequest> sseMessage = new SseMessage<>("QUESTION_CHECK", questionCheckSseRequest); | |
| sseMessageSender.sendMessage(courseId.toString(), sseMessage); | |
| log.debug("질문 체크 처리가 완료되었습니다."); | |
| } |
uri010
left a comment
uri010
left a comment
There was a problem hiding this comment.
조회와 업데이트를 한 트랜잭션에서 관리하지 않아도 되는 기능일 수도 있으나 같이 고민해봤으면 좋겠어서 리뷰로 남겨놨습니다. 선오님 의견도 공유해주세요~!
|
|
||
| question.setIsComplete(true); | ||
| questionService.save(question); | ||
|
|
||
| QuestionCheckSseRequest questionCheckSseRequest = new QuestionCheckSseRequest(questionId); |
There was a problem hiding this comment.
혹시 트랜잭션 범위 수정이 된건가요? 조회랑 업데이트 부분이 같이 한 트랜잭션으로 묶여야 하지 않나요?
There was a problem hiding this comment.
생각해보니 그렇네요. 단순히 '업데이트'만을 하나의 트랜잭션으로 생각했는데, 조회 후 업데이트할 경우에는 이 과정을 모두 트랜잭션으로 묶어야 할 필요가 있었습니다.
| Course course = getCourse(courseId); | ||
| checkIfOpen(course); | ||
|
|
||
| int updatedRows = questionRepository.updateQuestion(studentId, course, questionId); | ||
| if (updatedRows == 0) { | ||
| log.debug("질문 체크를 처리하는 과정에서 발생한 에러입니다. : Question does not exist."); | ||
| throw new BaseException(QuestionErrorCode.QUESTION_NOT_FOUND); | ||
| } |
There was a problem hiding this comment.
이 부분도 업데이트는 트랜잭션이 보장될 거 같은데 조회랑 같이 한 트랜잭션으로 묶어주지 않아도 괜찮을까요?
uri010
left a comment
uri010
left a comment
There was a problem hiding this comment.
프론트분들이 테스트 먼저 하고 싶다고 하셔서 Approve 드립니다!
|
트랜잭션 관련 기능에 대해 코드를 보완했습니다. |
[BE] 질문 체크 기능 구현
#️⃣ 연관된 이슈
#182
📝 작업 내용
학생과 교수 모두 질문을 체크하고, 이를 바탕으로 SSE 메시지를 보내는 기능
💬 리뷰 요구사항(선택)
학생이 체크 버튼을 눌렀을 때와 교수가 체크 버튼을 눌렀을 때 database 접근 방식을 조금 다르게 구성했습니다.
더 좋은 방식이 있으면 제안해 주셔도 됩니다.
Summary by CodeRabbit
New Features
Improvements