-
Notifications
You must be signed in to change notification settings - Fork 0
[Release] v2.1.3 login v2 적용 & Slack 채널명 변경 #323
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
* fix: 리뷰 리스트 속 아이템의 별점이 평점이 아닌 그 리뷰의 별점을 반환하도록 수정 * fix: 리뷰 리스트가 메뉴에 대한 리뷰 + 밀에 대한 리뷰를 합쳐서 반환하도록 수정 * fix: menu reviewlist에서 rating이 없을시 mainrating을 내보내도록 수정 * fix: meal list에서도 rating, 없을시 mainrating을 내보내도록 수정
* fix: [POST] /reviews/write/{menuID} List<String>으로 변경
* fix: List String으로 바꾸는 것 불요하여 원상 복귀
* feat: 기능 구현 * chore: enum 대문자화 * fix: 오타 정정 --------- Co-authored-by: 나용준 <[email protected]>
Co-authored-by: 나용준 <[email protected]>
Co-authored-by: 나용준 <[email protected]>
Co-authored-by: 나용준 <[email protected]>
Summary of ChangesHello @yongjun0511, 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! 이 풀 리퀘스트는 카카오 및 애플 로그인 시스템의 V2 버전을 도입하여 기기 유형 정보를 통합하고 사용자 관리를 개선합니다. 또한, 식사 관련 리뷰 표시 로직에 대한 중요한 리팩토링 및 개선 사항을 포함하여 더 정확한 평점 계산 및 데이터 검색을 보장합니다. 추가적으로, 슬랙 채널 이름이 업데이트되었고 기존 문의 시스템은 사용 중단되었습니다. 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
이번 PR은 로그인 v2를 도입하여 사용자의 디바이스 타입(IOS/ANDROID)을 수집하고, 이를 인증 및 로깅 과정에 통합하는 변경 사항을 포함하고 있습니다. 또한, 리뷰 관련 로직을 리팩토링하고 Slack 채널명을 변경하는 등 여러 개선 사항이 있습니다. 전반적으로 기능 확장을 위한 좋은 시도이지만, 몇 가지 코드 중복, 스타일 가이드 위반, 잠재적 버그 등이 발견되었습니다. 아래 리뷰 코멘트를 확인하고 수정하여 코드 품질을 더욱 향상시키는 것을 권장합니다.
| public static MealReviewResponse from(Review review, | ||
| Long userId, | ||
| List<ValidMenuForViewResponse.MenuDto> validMenus,Double rating) { | ||
| List<ValidMenuForViewResponse.MenuDto> validMenus, Integer rating) { |
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.
from 메소드의 rating 파라미터가 사용되지 않고 있습니다. 메소드 내부에서 resolvedRating이라는 변수를 새로 계산하여 사용하고 있어, rating 파라미터는 혼란을 야기할 수 있습니다. 이 파라미터를 메소드 시그니처에서 제거하고, 호출하는 부분(ReviewServiceV2:216)도 함께 수정하는 것을 권장합니다.
| public static MealReviewResponse from(Review review, | |
| Long userId, | |
| List<ValidMenuForViewResponse.MenuDto> validMenus,Double rating) { | |
| List<ValidMenuForViewResponse.MenuDto> validMenus, Integer rating) { | |
| public static MealReviewResponse from(Review review, | |
| Long userId, | |
| List<ValidMenuForViewResponse.MenuDto> validMenus) { |
| } | ||
|
|
||
| Integer likeCount = Optional.ofNullable(menus) | ||
| Integer likeCount = Optional.of(menus) |
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.
Optional.ofNullable을 Optional.of로 변경하면 menus가 null일 경우 NullPointerException이 발생할 수 있습니다. ofNullable이 더 안전한 선택입니다. 더 좋은 방법은 mealMenuRepository가 null을 반환하지 않는다고 가정하고 Optional 래퍼를 완전히 제거하는 것입니다. 하지만 현재 변경 사항만 고려했을 때는 ofNullable로 되돌리는 것이 좋습니다.
| Integer likeCount = Optional.of(menus) | |
| Integer likeCount = Optional.ofNullable(menus) |
| import ssu.eatssu.domain.auth.dto.AppleLoginRequest; | ||
| import ssu.eatssu.domain.auth.dto.KakaoLoginRequest; | ||
| import ssu.eatssu.domain.auth.dto.ValidRequest; | ||
| import ssu.eatssu.domain.auth.dto.*; |
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.
리포지토리 스타일 가이드 3번 규칙에 따라 와일드카드(*)를 사용한 임포트는 지양해야 합니다. 필요한 클래스를 명시적으로 임포트하여 코드의 가독성과 명확성을 높이는 것이 좋습니다.
| import ssu.eatssu.domain.auth.dto.*; | |
| import ssu.eatssu.domain.auth.dto.AppleLoginRequest; | |
| import ssu.eatssu.domain.auth.dto.AppleLoginRequestV2; | |
| import ssu.eatssu.domain.auth.dto.KakaoLoginRequest; | |
| import ssu.eatssu.domain.auth.dto.KakaoLoginRequestV2; | |
| import ssu.eatssu.domain.auth.dto.ValidRequest; |
References
- Do not use wildcard when importing libraries (link)
|
|
||
| private final OAuthService oauthService; | ||
|
|
||
| // TODO : 로그인 & 회원 가입 마이그레이션 이후에 지울 것. |
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.
리포지토리 스타일 가이드 7, 8, 9번 규칙에 따라 TODO 주석을 작성해야 합니다. 주석은 한 줄로 작성하고, 작성자 이름과 날짜를 포함하며, FIX ME 형식을 사용하는 것을 권장합니다. 예를 들어, // FIXME(author, yyyy-MM-dd): 로그인 & 회원 가입 마이그레이션 이후에 지울 것.과 같이 수정할 수 있습니다. 69번 라인의 TODO 주석도 동일하게 수정이 필요합니다.
| long startTime = System.currentTimeMillis(); | ||
| Tokens tokens = oauthService.kakaoLoginV2(request); | ||
| long endTime = System.currentTimeMillis(); | ||
| long duration = endTime - startTime; | ||
| log.info("OAuthWarmupRunner 완료 - 소요 시간: {} ms", duration); |
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.
System.currentTimeMillis()를 사용한 성능 측정 코드가 포함되어 있습니다. 이러한 코드는 보통 개발 및 디버깅 단계에서 임시로 사용되며, 프로덕션 코드에 포함되는 것은 바람직하지 않습니다. 만약 성능 측정이 필요하다면 AOP 등을 활용하여 공통 로직으로 분리하는 것을 고려해볼 수 있습니다. appleLoginV2 메소드에는 해당 코드가 없는 것으로 보아 일관성도 부족해 보입니다. 이 코드를 제거하는 것을 제안합니다.
| long startTime = System.currentTimeMillis(); | |
| Tokens tokens = oauthService.kakaoLoginV2(request); | |
| long endTime = System.currentTimeMillis(); | |
| long duration = endTime - startTime; | |
| log.info("OAuthWarmupRunner 완료 - 소요 시간: {} ms", duration); | |
| Tokens tokens = oauthService.kakaoLoginV2(request); |
| if (user.getDeviceType() == null) { | ||
| user.updateDeviceType(request.deviceType()); | ||
| } |
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.
| // TODO : 회원 가입 V2 마이그레이션 이후 삭제. | ||
| // 기본 생성자 : DeviceType을 Null로 받고 있는데 마이그레이션 이후 기본 생성자도 NotNull 붙이기. | ||
| // admin 정적 펙토리 : 여기도 null 넣는 부분 바꿔주기. |
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.
리포지토리 스타일 가이드 7, 8, 9번 규칙에 따라 TODO 주석을 작성해야 합니다. 주석은 한 줄로 작성하고, 작성자 이름과 날짜를 포함하며, FIX ME 형식을 사용하는 것을 권장합니다. 여러 줄에 걸쳐 작성된 TODO를 규칙에 맞게 수정해주세요.
| // .filter(menu -> !MenuFilterUtil.isExcludedFromReview( | ||
| // menu.getName())) |
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.
| ADDMENU_CHANNEL("#메뉴_추가"), | ||
| ERROR_CHANNEL("#장애"), | ||
| REPORT_CHANNEL("#01-신고"), | ||
| // TODO: 회의에서 제안 후 삭제한다 |
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.
| private String getUserIdFromSecurityContext() { | ||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | ||
| if (authentication != null && authentication.isAuthenticated() && authentication.getPrincipal() instanceof CustomUserDetails) { | ||
| CustomUserDetails userDetails = (CustomUserDetails) authentication.getPrincipal(); | ||
| if (authentication != null && authentication.isAuthenticated() && | ||
| authentication.getPrincipal() instanceof CustomUserDetails userDetails) { | ||
| return String.valueOf(userDetails.getId()); | ||
| } | ||
| return "anonymous"; | ||
| } | ||
|
|
||
| private String getDeviceTypeFromSecurityContext() { | ||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | ||
| if (authentication != null && authentication.isAuthenticated() && | ||
| authentication.getPrincipal() instanceof CustomUserDetails userDetails) { | ||
|
|
||
| return userDetails.getDeviceType() != null | ||
| ? userDetails.getDeviceType().name() | ||
| : "null"; | ||
| } | ||
| return "unknown"; | ||
| } |
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.
getUserIdFromSecurityContext 메소드와 getDeviceTypeFromSecurityContext 메소드에서 SecurityContextHolder로부터 Authentication 객체를 가져오고 CustomUserDetails로 캐스팅하는 로직이 중복됩니다. 이 중복을 제거하기 위해 CustomUserDetails를 반환하는 private 헬퍼 메소드를 만드는 것을 고려해 보세요. 예를 들어, private Optional<CustomUserDetails> getCustomUserDetails()와 같은 메소드를 만들어 사용할 수 있습니다.
No description provided.