-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/#100 오픈채팅 프로필 url 등록 기능 구현 #110
The head ref may contain hidden characters: "Feature/#100-\uC624\uD508\uCC44\uD305_\uD504\uB85C\uD544_URL_\uB4F1\uB85D_\uAE30\uB2A5_\uAD6C\uD604"
Conversation
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.
수고하셨습니다!
몇 가지 누락된 것으로 보이는 코드가 있어서 코멘트 남겼습니다!
반영 부탁드려요~~!
</div> | ||
</div> | ||
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.15.6/styles/github.min.css"> | ||
<script src="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.15.6/highlight.min.js"></script> | ||
<script>hljs.initHighlighting()</script> | ||
</body> | ||
</html> | ||
</html> |
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.
'career', '커리어'로 검색하니 수정되지 않은 부분이 많이 남아있는 것 같아요!
당장 급한 건은 아니지만 이왕 바꾸는 거 마저 수정해주시면 좋을 것 같아요.
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.
이 부분은 제가 이 이슈에서 수정할 부분이 아닌 것 같아요^^;
나중에 이슈를 파서 수정해야 할 것 같습니다.
@RequestBody @Valid final OpenProfileUrlRequest openProfileUrlRequest | ||
) { | ||
memberUpdateService.updateOpenProfileUrl(member, openProfileUrlRequest); | ||
return ResponseEntity.ok().build(); |
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.
ok() 대신 noContent()를 사용해도 좋을 것 같아요!
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.
ㅇㅈ
@MockBean | ||
private MemberActivityService memberActivityService; | ||
@MockBean | ||
private MemberUpdateService memberUpdateService; |
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.
MemberApi
클래스를 빈으로 실행시키기 위해 필요합니다.
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.
고생하셨습니다~~
테스트 하실 때, 테스트 코드도 물론 중요하지만 로컬에서 해당 API 를 한번 호출해보고 직접 눈으로 확인하는 것도 좋은 방법이지 않을까 생각합니다!
낼 현장 리뷰에서 뵙죠~~
수정하고 다시 request 주십셔
@RequestBody @Valid final OpenProfileUrlRequest openProfileUrlRequest | ||
) { | ||
memberUpdateService.updateOpenProfileUrl(member, openProfileUrlRequest); | ||
return ResponseEntity.ok().build(); |
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.
ㅇㅈ
@Getter | ||
public class OpenProfileUrlRequest { | ||
|
||
@Pattern(regexp = "(https://open.kakao.com/).*") |
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 | ||
public class MemberUpdateService { | ||
|
||
public void updateOpenProfileUrl( |
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.
JPA에서는 변경감지를 통해서 데이터를 수정해주는데 @Transactinoal 이 없으면 변경 감지가 동작 안하는 것으로 알고 있습니다,,
혹시 http 테스트를 하고 데이터가 변경되었는지 확인하셨는지 궁금합니다 !!
저도 트랜잭션 어노테이션 사용안해서 수정 안됐던 기억이,,
memberUpdateService.updateOpenProfileUrl(member, request); | ||
|
||
// then | ||
assertThat(member.getOpenProfileUrl()).isEqualTo(newOpenProfileUrl); |
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 부분 말고는 걸리는 부분이 없네요. 수고하셨습니다.
import org.springframework.stereotype.Service; | ||
|
||
@Service | ||
public class MemberUpdateService { |
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.
혹시 MemberUpdateService라고 이름을 지은 이유가 있을까요?
궁금해서요
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.
개인적으로 클래스의 역할을 파악하기 쉽다고 느껴서 위와 같이 지었습니다.
- MemberUpdateService의 updateOpenProfileUrl 메서드에서 Member 파라미터가 영속화가 되지 않아 변경 감지가 일어나지 않는 오류 해결 #100
#️⃣연관된 이슈
close: #100
📝작업 내용
예상 소요 시간 및 실제 소요 시간
2시간 / 3시간