Skip to content
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

#32 Feat: 회원 수정 #33

Merged
merged 2 commits into from
Jul 18, 2024
Merged

#32 Feat: 회원 수정 #33

merged 2 commits into from
Jul 18, 2024

Conversation

KoungQ
Copy link
Member

@KoungQ KoungQ commented Jul 18, 2024

1. 개발내용

내 정보 조회 + 회원 수정

2. 구현 세부사항

3. 메모

@KoungQ KoungQ added the Feature 기능 개발 label Jul 18, 2024
@KoungQ KoungQ self-assigned this Jul 18, 2024
@KoungQ KoungQ linked an issue Jul 18, 2024 that may be closed by this pull request
Copy link
Member

@hyxklee hyxklee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 깔끔하네요 고생하셨습니다!

Copy link
Member

@jiixon jiixon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니당~

Comment on lines +76 to +79
public void update(Long userId, UserDTO.Update dto) {
User user = mapper.update(userId, dto);
userRepository.save(user);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 findById(userId)로 사용자 존재 여부를 확인 해봐야 하지 않을지 궁금합니다!

Copy link
Member Author

@KoungQ KoungQ Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로직상 Jwt 토큰에서 추출하는 아이디기 때문에 존재하지 않는 시나리오는 불가능해요
그리고 새로 올린 PR에서 정지된 유저 막는 로직 중 해당 토큰의 이메일로 유저를 찾아서 유저 정보가 없다면 거기서도 한번 더 예외처리를 하기 때문에 모든 케이스가 막혔다고 봐도 과언이 아니라 굳이 필요하지 않다고 느꼈어요
리뷰 감사합니당

Comment on lines +76 to +79
public void update(Long userId, UserDTO.Update dto) {
User user = mapper.update(userId, dto);
userRepository.save(user);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update 메서드에도 Transaction을 고려해야하지 않을까용?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 사용하고 있는 save 메서드는 SimpleJpaRepository의 save 메서드인데,
이는 이미 메서드에 @transactional 처리가 되어있으므로 고려하지 않아도 괜찮습니다
리뷰 감사합니당

참고: https://wangtak.tistory.com/2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇다면, update라서 save()를 호출하지 않아도 영속성으로 값이 변경되면 DB에도 반영이 되는걸로 알고 있어요. 새로운 entity가 추가되지 않는 거라면, @transactional을 붙이고, save를 지우는 것이 객제지향 관점에서 좋다고 생각해서 리뷰 남겼었습니다! (save를 말씀안드렸네여.. )

@KoungQ KoungQ merged commit 135d1cd into main Jul 18, 2024
1 check passed
@KoungQ KoungQ deleted the feature-#32 branch July 26, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#32 Feat: 회원 수정
3 participants