-
Notifications
You must be signed in to change notification settings - Fork 4
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/#133-마이페이지_회원_정보_수정_기능을_제작한다 #159
The head ref may contain hidden characters: "Feature/#133-\uB9C8\uC774\uD398\uC774\uC9C0_\uD68C\uC6D0_\uC815\uBCF4_\uC218\uC815_\uAE30\uB2A5\uC744_\uC81C\uC791\uD55C\uB2E4"
Changes from 2 commits
bac8277
6a4405c
0e29cc5
6aa2662
d1c1c58
077c3f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,4 +100,16 @@ private Team validateExistTeam(Long teamId) { | |
private Study validateExistStudy(Long studyId) { | ||
return studyRepository.findById(studyId).orElseThrow(() -> new StudyException(NOT_FOUND_STUDY)); | ||
} | ||
|
||
public void updateMyPageName(Long memberId, String newName) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 앞 리뷰와 동일합니다~ 메서드명 수정하면 좋을 것 같아요! |
||
Member member = validateExistMember(memberId); | ||
Member updatedMember = Member.builder() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저희가 클라이언트로부터 받아온 memberId와 토큰의 memberId가 같은지(본인확인절차)가 필요할 것 같습니다~ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JJimini 프론트의 localStorage에서 저장한 것은 클라이언트에서 바꿀 수 있어서 그 값을 받아서 굳이 확인하기보다는 저희가 보낸 토큰을 통해 복호화하여 멤버의 권한을 확인 할 필요 없이 그 사람의 myPage를 보여주는 것이 괜찮을 것 같은데 어떻게 생각하시나요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jimini1026 우선 저희 두레는 회원(본인) 접근 api는 2가지 검증 절차를 거칩니다.
1번은 클라이언트에서 변경할 수 있기 때문에 확인하는 것이고, 그 값이 맞다면 2번 확인을 통해 본인이 맞는지 확인합니다. 예를 들어, 클라이언트로부터 memberId가 1이 들어왔고 token에 있는 memberId는 5라고 가정합시다. memberId 1이 데이터베이스에 존재하면 1번 검증을 통과하지만 본인이 아니기 때문에 2번 검증에서 걸러지는 것이죠. 요시님 말씀처럼 토큰의 memberId를 가지고 해당 API에 접근할 수 있도록 한다면 어떻게 API를 요청한 사람이 본인인지 확인할 수 있는지 잘 모르겠습니다,, |
||
.id(memberId) | ||
.name(newName) | ||
.googleId(member.getGoogleId()) | ||
.email(member.getEmail()) | ||
.imageUrl(member.getImageUrl()) | ||
.build(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 더 간단하게 작성할 수 있는 방법이 있을 것 같아용~ 업데이트 요청이 있을 때마다 객체를 생성하여 db에 저장하면 성능 측면에서 문제가 있을 것 같아요~! Study entity나 CurriculumItem entity, Team entity를 참고하시면 좋을 것 같습니다 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 네 좋습니다! |
||
memberRepository.save(updatedMember); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -227,4 +227,14 @@ void init() { | |
memberCommandService.transferStudyLeader(study.getId(), member.getId(), notStudyLeaderMember.getId()); | ||
}); | ||
} | ||
|
||
@Test | ||
@DisplayName("[성공] 프로필 이름 수정에 성공한다.") | ||
void updateMyPageName_프로필_이름_수정에_성공한다_성공() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 여기 메서드 이름이 안바뀐 것 같아요! |
||
String updateName = "요시"; | ||
memberCommandService.updateMyPageName(member.getId(), updateName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. request를 전달하고, request 내용대로 정보가 바뀌었는지 테스트하는 코드를 짜면 좋을 것 같아요~ |
||
|
||
Member findMember = memberRepository.findById(member.getId()).get(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기서 |
||
assertThat(findMember.getName()).isEqualTo(updateName); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
import org.springframework.http.HttpHeaders; | ||
import org.springframework.http.MediaType; | ||
import org.springframework.restdocs.mockmvc.RestDocumentationRequestBuilders; | ||
|
||
public class MemberApiDocsTest extends RestDocsTest { | ||
|
@@ -61,4 +62,19 @@ void setUp() { | |
.andDo(document("delete-member")); | ||
} | ||
|
||
//todo: docs code | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo는 진행하셨으면 삭제해주셔도 될 것 같아요~ |
||
@Test | ||
@DisplayName("[성공] 프로필 이름 수정에 성공한다.") | ||
void updateMyPageName_프로필_이름_수정에_성공한다() throws Exception { | ||
String requestJson = "{\"newName\":\"요시\"}"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
doNothing().when(memberCommandService).updateMyPageName(any(), any()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 위에서 미나님이 피드백 해주셨듯이 실제 service의 return값은 dto이니 String 대신 결과 응답 dto를 만들면 좋을 거 같아요~! |
||
|
||
mockMvc.perform(RestDocumentationRequestBuilders.patch("/profile/name") | ||
.contentType(MediaType.APPLICATION_JSON) | ||
.content(requestJson) | ||
.header(HttpHeaders.AUTHORIZATION, accessToken)) | ||
.andExpect(status().isNoContent()) | ||
.andDo(document("update-my-page-name")); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분도 request를 사용하면 코드가 바뀌겠죠?! 그리고 |
||
} |
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.
마이페이지는 회원 본인만 접근이 가능해야합니다! URI에 회원의 정보를 받아오면 좋을 것 같습니다!
"/profile/members/{memberId}"
로 수정하는 것은 어떨까 제안드려봅니다!또한 메서드명이 너무 상세합니다.
회원의 이름을 수정하는 메서드
가 아닌회원의 정보를 수정하는 메서드
가 되면 좋을 것 같습니다. ex)updateMyPage
이렇게 작성하면 추후 수정할 회원 정보가 확장되면 추가하기 쉽습니다~! (이름을 수정하는 메서드, 이메일을 수정하는 메서드 등등 수정 내용이 생길때마다 메서드를 만들 수는 없으니까요!)
추가로 저희는 DTO로 request를 받아옵니다! request에 수정할 회원 정보를 담으시면 될 것 같습니다!
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.
@JJimini MemberArgumentResolver에서 보면 request의 헤더를 통해 전달되는 토큰을 이용해 memberID 정보를 들고 올 수 있는데 혹시 memberID를 path variable에 한 번 더 쓰는 이유가 있을까요?
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.
우선 저희 두레 프로젝트는 URL에 memberId를 명시적으로 표현하여 RESTful API의 일관성을 유지하고자 했으며, 이것은 초기 컨벤션 논의 때 규칙으로 정하였고 그렇게 사용하고 있습니다!