-
Notifications
You must be signed in to change notification settings - Fork 5
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] - 자체 회원 가입과 로그인 구현 #234
Conversation
Test Results 25 files 25 suites 5s ⏱️ Results for commit 729795a. ♻️ This comment has been updated with latest results. |
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.
변경됐으면 하는 지점 몇개 써놓고 APPROVE 드립니다!
천천히 리뷰해주세요 👍 👍
) | ||
}) | ||
@PostMapping | ||
public ResponseEntity<LoginResponse> defaultLogin(@Valid @RequestBody LoginRequest 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.
그냥 제안) 위를 kakaoLogin()
으로 하고 일반 로그인을 login()
으로 하면 어떨까요? 뭔가 디폴트가 뭐지? 싶을 수도 있을 것 같은
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.
저도 같은 생각..!
@@ -37,8 +37,10 @@ public class JwtAuthFilter extends OncePerRequestFilter { | |||
new HttpRequestInfo(HttpMethod.GET, "/swagger-resources/**"), | |||
new HttpRequestInfo(HttpMethod.GET, "/v3/api-docs/**"), | |||
new HttpRequestInfo(HttpMethod.GET, "/api/v1/travelogues/**"), | |||
new HttpRequestInfo(HttpMethod.GET, "/api/v1/login/**"), |
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.
바로 제거하겠습니다!
@@ -95,7 +95,7 @@ public String copyImageToPermanentStorage(String imageUrl) { | |||
|
|||
private void validateS3Path(String imageKey) { | |||
if (!imageKey.startsWith(imageBaseUri + temporaryStoragePath)) { | |||
throw new BadRequestException("이미지 url 형식이 잘못되었습니다."); | |||
throw new BadRequestException("S3 이미지 url 형식이 잘못되었습니다."); |
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.
굳굳
@@ -49,7 +49,7 @@ void setUp() { | |||
RestAssured.port = port; | |||
databaseCleaner.executeTruncate(); | |||
|
|||
member = travelogueTestHelper.initMemberTestData(); | |||
member = testHelper.initKakaoMemberTestData(); |
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.
여기 travelPlan이랑 Travelogue 테스트 헬퍼 각각 쓰여서 명시해주는게 좋을듯함다
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.
리건 수고하셨습니다 👍
질문과 얘기해볼만한 부분 코멘트 남겼어유
) | ||
}) | ||
@PostMapping | ||
public ResponseEntity<LoginResponse> defaultLogin(@Valid @RequestBody LoginRequest 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.
저도 같은 생각..!
.body("message", is("이메일은 비어있을 수 없습니다.")); | ||
} |
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.
이거 예전에 제가 ObjectMapper로 바디 전체를 검증해야 한다고 말씀드렸었는데, 이곳 저곳에서 컨벤션이 지켜지지 않고 있는 듯 해요..!
한번 컨벤션 맞추는 과정 필요할지도 ..
지금에서는 저도 생각이 바뀌어서 핵심을 담은 json 필드만 검증하는 것도 괜찮다고 생각합니다 👍
package kr.touroot.member.fixture; | ||
|
||
import kr.touroot.member.dto.request.MemberRequest; | ||
|
||
public class MemberRequestFixture { | ||
|
||
private static final String EMAIL = "user@email.com"; | ||
private static final String PASSWORD = "@testpassword"; | ||
private static final String NICKNAME = "뚜리"; | ||
private static final String PROFILE_IMAGE_URL = "https://dev.touroot.kr/images/f8c26e9f-5a9f-42ae-84f1-d507fd9020c1.png"; | ||
|
||
private MemberRequestFixture() { | ||
} | ||
|
||
public static MemberRequest getMemberRequest() { | ||
return new MemberRequest(EMAIL, PASSWORD, NICKNAME, PROFILE_IMAGE_URL); | ||
} | ||
|
||
public static MemberRequest getMemberRequestWithEmptyEmail() { | ||
return new MemberRequest("", PASSWORD, NICKNAME, PROFILE_IMAGE_URL); | ||
} | ||
|
||
public static MemberRequest getMemberRequestWithEmptyPassword() { | ||
return new MemberRequest(EMAIL, "", NICKNAME, PROFILE_IMAGE_URL); | ||
} | ||
|
||
public static MemberRequest getMemberRequestWithEmptyNickname() { | ||
return new MemberRequest(EMAIL, PASSWORD, "", PROFILE_IMAGE_URL); | ||
} | ||
|
||
public static MemberRequest getMemberRequestWithEmptyProfileImageUrl() { | ||
return new MemberRequest(EMAIL, PASSWORD, NICKNAME, ""); | ||
} | ||
|
||
public static MemberRequest getMemberRequestDuplicateEmail() { | ||
return new MemberRequest(EMAIL, PASSWORD, "뚜리뚜리", PROFILE_IMAGE_URL); | ||
} | ||
|
||
public static MemberRequest getMemberRequestDuplicateNickname() { | ||
return new MemberRequest("useruser@email.com", PASSWORD, NICKNAME, PROFILE_IMAGE_URL); | ||
} | ||
} |
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.
이 부분이 enumFixture의 진가가 발휘될 수 있는 부분이라고 생각하는데요..!
enumFixture 혹은 생성된 Request를 상수로써 가지고 있는 fixutre의 형태는 어떠신간요?
fixture 관련 구현도 도메인별로 상이해지고 있는 것 같아서 모두의 의견을 모으고 컨벤션을 맞추어야 할 것 같습니다.
관련한 리건의 의도가 있었다면 적극적으로 공유해주시면 너무 고마울 것 같습니다 🙇🏻♂️
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.
이미 enum
fixture를 만들어 봤으면서 여기선 활용하지 않았네요!
저도 리비의 생각에 동의합니다. enum fixture로 바꿔보겠습니다!
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.
수고하셨습니다 리건!
남긴 리뷰는 다 질문 or 상의해볼 만한 내용이라 approve 드리겠습니닷!
@@ -95,7 +95,7 @@ public String copyImageToPermanentStorage(String imageUrl) { | |||
|
|||
private void validateS3Path(String imageKey) { | |||
if (!imageKey.startsWith(imageBaseUri + temporaryStoragePath)) { | |||
throw new BadRequestException("이미지 url 형식이 잘못되었습니다."); | |||
throw new BadRequestException("S3 이미지 url 형식이 잘못되었습니다."); |
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.
훨씬 좋아졌네요! 감사합니다!!
return ResponseEntity.created(URI.create("/api/v1/members/" + id)) | ||
.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.
그냥 질문) 반환한 members uri는 어떻게 쓰이나용
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.
프론트엔드 분들이 끝의 id를 활용하거나 단순히 몇 번째 멤버가 잘 생성되었는가(어디에 자원이 위치하는가)에 대한 메타데이터 용도이기도 합니다!
@Schema(description = "사용자 프로필 사진 URL", example = "S3 이미지 URL") | ||
@NotBlank(message = "프로필 사진 URL은 비어있을 수 없습니다.") | ||
String profileImageUrl |
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.
프사를 설정하지 않는 상남자일 경우 default url을 넣는건 백에서 따로 처리 안해도 되는 건가욤
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.
요것도 고민해볼 부분인데, 저희 default 이미지 url이 있다면 그걸로 지정해줘도 좋을 것 같아요.
아직 저희 기본 프로필 사진이 없어서 지금은 비워뒀습니다!
* feat: 투룻 서비스 자체 로그인 API 작성 * feat: 회원 가입 및 로그인 기능 구현 * feat: 회원의 로그인 타입에 따른 구분을 위한 열거형 추가 * feat: 회원 가입 기능 구현 * feat: 회원 가입 및 로그인 기능 토큰 필터에서 제외 * feat: 이미지 URL 검증 추가에 따른 Swagger example 수정 * refactor: URL 검증 예외 메시지 수정 * feat: 로그인 요청 DTO 구현 * refactor: 회원 가입 및 로그인 기능 구현에 따른 테스트 코드 수정 * chore: 반환 URI 수정 * refactor: 이메일과 비밀번호 null 검증 추가 * refactor: 사용자 도메인 테스트 추가 작성 * refactor: 사용자 test fixture 추가 * refactor: Test fixture 이름 변경에 따른 리팩토링 * test: 사용자 회원 가입 로직 컨트롤러 계층과 서비스 계층 통합 테스트 작성 * fix: Conflict 해결 과정에서 누락된 수정 사항 반영 * fix: Conflict 해결 과정에서 누락된 수정 사항 반영 * refactor: 불필요한 요청 화이트리스트 제거 * refactor: test fixture lombok을 활용한 enum fixture로 개선 * fix: 변경된 메소드 이름이 반영되지 않은 코드 수정
✅ 작업 내용
🙈 참고 사항