-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add new api with additional field for response #69
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
|
Claude finished @eric-jy-park's task โโ View job ์ฝ๋ ๋ฆฌ๋ทฐ ์๋ฃ์์ ๋ชฉ๋ก
๋ณ๊ฒฝ์ฌํญ ์์ฝํ์ฌ ์งํ์ค์ธ ์ธ์
์กฐํ API( ์ฝ๋ ๋ฆฌ๋ทฐUserSessionController.java:112 - issue: ๋ถํ์ํ UserService ํธ์ถ๋ก ์ฑ๋ฅ ์ ํ - ์ด๋ฏธ ์ธ์ ์์ userId ๊ฒ์ฆ ํ ๋ณ๋ DB ์กฐํ๊ฐ ์ค๋ณต๋จ, UserSessionService์์ User ์ ๋ณด๋ฅผ ํจ๊ป ๋ฐํํ๋๋ก ์์ ๊ถ์ฅ UserSessionResponses.java:67 - question: SR ๊ณ์ฐ ๋ก์ง์ด ๋ถ๋ถ๋ช ํจ - currentSr = user.getSr() + session.getSrGained() ๊ณ์ฐ์ด ์ฌ๋ฐ๋ฅธ์ง ํ์ธ ํ์, ์ธ์ ์ข ๋ฃ ์์ ์ ์ด๋ฏธ user.getSr()์ ๋ฐ์๋์์ ๊ฐ๋ฅ์ฑ ๊ฒํ UserSessionServiceImpl.java:100 - suggestion: @transactional ์ด๋ ธํ ์ด์ ์ด ๋ถํ์ํจ - ๋จ์ ์กฐํ ๋ฉ์๋์์ ์ฝ๊ธฐ ์ ์ฉ ํธ๋์ญ์ ์ฌ์ฉ ๊ถ์ฅ, @transactional(readOnly = true) ๋๋ ์ ๊ฑฐ |
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.
Pull Request Overview
This PR adds a new API endpoint to fetch the currently active user session and enhances the existing session response to include additional SR (Skill Rating) fields. The changes implement functionality to return ongoing sessions and provide more detailed SR information for completed sessions.
Key changes:
- Added new
getActiveSessionmethod to retrieve the user's currently active session - Enhanced the session state response with
currentSrandpreviousSrfields - Updated API documentation paths and added comprehensive documentation for the new endpoint
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| UserSessionServiceImpl.java | Implements getActiveSession method to find active sessions by user ID |
| UserSessionService.java | Adds interface method declaration for getActiveSession |
| UserSessionResponses.java | Adds new response DTO for active sessions and enhances existing DTO with SR fields |
| UserSessionController.java | Adds new GET endpoint for active sessions and updates existing endpoint to include user data |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @RequiredArgsConstructor | ||
| public class UserSessionController { | ||
| private final UserSessionService userSessionService; | ||
| private final UserRepository userRepository; |
Copilot
AI
Aug 16, 2025
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.
The UserRepository field is injected but never used in this controller. Consider removing it since UserService is already available and provides the necessary user operations.
| private final UserRepository userRepository; |
| response.totalDuration = session.getTotalDuration(); | ||
| response.srGained = session.getSrGained(); | ||
| response.currentSr = user.getSr() + session.getSrGained(); | ||
| response.previousSr = user.getSr(); |
Copilot
AI
Aug 16, 2025
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.
The currentSr calculation adds srGained to the user's current SR, but this logic appears incorrect. If the session is finished, the user's SR should already include the gained SR. This would result in double-counting the SR gain.
| response.previousSr = user.getSr(); | |
| response.currentSr = user.getSr(); | |
| response.previousSr = user.getSr() - session.getSrGained(); |
| response.totalDuration = session.getTotalDuration(); | ||
| response.srGained = session.getSrGained(); | ||
| response.currentSr = user.getSr() + session.getSrGained(); | ||
| response.previousSr = user.getSr(); |
Copilot
AI
Aug 16, 2025
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.
The previousSr is set to the user's current SR, but this should represent the SR before the session. If the user's SR has already been updated after the session, this would be incorrect. Consider storing the previous SR value or calculating it as user.getSr() - session.getSrGained().
| response.previousSr = user.getSr(); | |
| response.previousSr = user.getSr() - session.getSrGained(); |
|
Claude finished @eric-jy-park's task โโ View job ์ฝ๋ ๋ฆฌ๋ทฐ ์๋ฃ์์ ๋ชฉ๋ก
๋ณ๊ฒฝ์ฌํญ ์์ฝํ์ฌ ์งํ์ค์ธ ์ธ์
์กฐํ API( ์ฝ๋ ๋ฆฌ๋ทฐUserSessionController.java:111 - issue: ๋ถํ์ํ UserService ํธ์ถ๋ก ์ฑ๋ฅ ์ ํ - ์ด๋ฏธ ์ธ์ ์์ userId ๊ฒ์ฆ ํ ๋ณ๋ DB ์กฐํ๊ฐ ์ค๋ณต๋จ, UserSessionService์์ User ์ ๋ณด๋ฅผ ํจ๊ป ๋ฐํํ๋๋ก ์์ ๊ถ์ฅ UserSessionResponses.java:67 - question: SR ๊ณ์ฐ ๋ก์ง์ด ๋ถ๋ถ๋ช ํจ - currentSr = user.getSr() + session.getSrGained() ๊ณ์ฐ์ด ์ฌ๋ฐ๋ฅธ์ง ํ์ธ ํ์, ์ธ์ ์ข ๋ฃ ์์ ์ ์ด๋ฏธ user.getSr()์ ๋ฐ์๋์์ ๊ฐ๋ฅ์ฑ ๊ฒํ UserSessionServiceImpl.java:100 - suggestion: @transactional ์ด๋ ธํ ์ด์ ์ด ๋ถํ์ํจ - ๋จ์ ์กฐํ ๋ฉ์๋์์ ์ฝ๊ธฐ ์ ์ฉ ํธ๋์ญ์ ์ฌ์ฉ ๊ถ์ฅ, @transactional(readOnly = true) ๋๋ ์ ๊ฑฐ |
๐ PR ์ ๋ชฉ
๐ ์์ ๋ด์ฉ
โ ์ฒดํฌ๋ฆฌ์คํธ
๐จ ์ฃผ์์ฌํญ