-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor/#220 회원 탈퇴 로직 리팩터링 #227
base: dev
Are you sure you want to change the base?
Conversation
Test Results 96 files 96 suites 17s ⏱️ Results for commit 39b4833. ♻️ This comment has been updated with latest results. |
@Column(name = "privacy_date_time") | ||
private LocalDateTime privacyDateTime; | ||
|
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.
이거 필드 추가하지 말고 updatedAt으로 하는거 어떤가요? isDeleted가 update되면서 더 이상 변경되지 않을 것 같은데
if (user.isRegistered()) { | ||
// TODO: 12/21/23 회원가입 중간에 나갈 때 예외 터지는 오류 잡기 | ||
List<Keyword> interests = interestQuery.getKeywordsByUserId(user.getId()); | ||
Certification certification = certificationQuery.getCertificationByUserId(user.getId()); | ||
AuthTokens authTokens = authTokensGenerator.generate(user.getId()); | ||
return LoginDetailsDto.of(user, interests, certification, authTokens); | ||
if (user.isDeleted()) { | ||
if (user.getPrivacyDateTime().isBefore(LocalDateTime.now())) { | ||
user.deletedWithdraw(); | ||
userCommand.updateUser(user); | ||
List<Keyword> interests = interestQuery.getKeywordsByUserId(user.getId()); | ||
Certification certification = certificationQuery.getCertificationByUserId(user.getId()); | ||
AuthTokens authTokens = authTokensGenerator.generate(user.getId()); | ||
return LoginDetailsDto.of(user, interests, certification, authTokens); | ||
} else { | ||
userCommand.deleteUser(user.getId()); | ||
} | ||
} else { | ||
List<Keyword> interests = interestQuery.getKeywordsByUserId(user.getId()); | ||
Certification certification = certificationQuery.getCertificationByUserId(user.getId()); | ||
AuthTokens authTokens = authTokensGenerator.generate(user.getId()); | ||
return LoginDetailsDto.of(user, interests, certification, authTokens); | ||
} | ||
} | ||
userCommand.saveUser(user); |
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.
if가 너무 많아서 가독성이 좋지 않습니다. 그리고 User Entity 위에 붙은 where 때문에 User를 조회 할 때는 isDeleted가 true인 것은 조회 되지 않습니다. if (user.isDeleted()) 는 없어도 될 것 같네요.
Service에서는 비즈니스 로직이 잘 보여야 하는데 구현에 대한 내용이 많아서 비즈니스 로직을 파악하기가 너무 힘듭니다. 적절하게 객체 만들어서 리팩터링 해주세요.
도메인 모델 패턴과 트랜잭션 스크립트 패턴을 참고해보세요.
List interests = interestQuery.getKeywordsByUserId(user.getId());
Certification certification = certificationQuery.getCertificationByUserId(user.getId());
AuthTokens authTokens = authTokensGenerator.generate(user.getId());
return LoginDetailsDto.of(user, interests, certification, authTokens);
이 부분도 전부 중복입니다.
User user = userQuery.getUserById(userId); | ||
if (nickname != null) { | ||
userCommand.updateUserInfo(user, nickname); | ||
} | ||
if (keywords != null && !keywords.isEmpty()) { | ||
interestCommand.updateInterests(user, keywords); | ||
} | ||
user.delete(); |
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.
UserCommand에 deleteUser() 로 묶어주는게 좋겠네요
public void deleteUser(Long userId) { | ||
userCommand.deleteUser(userId); | ||
List<User> deletedUsers = users.stream() | ||
.filter(u -> u.getPrivacyDateTime() != 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.
u 로 줄이지 말고 user로 해주세요
return MyProfileDto.of(user, keywords, certification); | ||
} | ||
|
||
public void updateProfileImage(Long userId, File file) { |
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.
프로필 관련 서비스 따로 나눴습니다 ~
} | ||
|
||
@Transactional | ||
public void updateProfileInfo(Long userId, String nickname, |
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.
이거 왜 지우셨나요?
AuthTokens authTokens = authTokensGenerator.generate(user.getId()); | ||
return LoginDetailsDto.of(user, interests, certification, authTokens); | ||
if (user.isDeleted()) { | ||
if (user.getPrivacyDateTime().isBefore(LocalDateTime.now())) { |
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.
저도 이거 맨날 햇갈리는데 현재보다 getPrivacyDateTime이 과거면 이미 삭제된 사용자입니다.
isAfter가 맞는 것 같습니다.
|
||
@SpringBootApplication | ||
@EnableScheduling |
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.
ScheduleConfig에 붙여주세요
@@ -25,4 +25,6 @@ public interface UserRepository extends JpaRepository<User, Long> { | |||
|
|||
List<User> findAllByChattingRoom(ChattingRoom chattingRoom); | |||
|
|||
List<User> findAllByIsDeletedIsTrue(); |
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.
User entity 위에 where 가 있어서 nativeQuery로 조회하셔야 합니다.
|
||
@Transactional | ||
public void unlink(Long userId, String accessToken, OAuthProvider oAuthProvider) { | ||
userService.deleteUser(userId, accessToken, oAuthProvider); |
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.
service에서 service를 참조하면 안됩니다.
implement 계층을 참조하도록 변경해주세요
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.
수고하셨습니다!
이슈번호
close: #220
작업 내용 설명
리뷰어에게 한마디