-
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
[TNT-114] 회원가입 데이터 취합 및 API 연동 #54
Conversation
- authId, authType, email 전달
- 몸무게 Float에서 Double로 수정 - FileUtils 이용해 이미지 파일 변환 - SignUpRequestMapper를 통해 trainer, trainee 데이터 다르게 전달
- 생일 String으로 변경 - 주의사항 ""이면 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.
큰 작업이었는데 고생많으셨습니다 ㅎㅎ
계층별로 어떤 부분까지 알고 있는게 좋을지,
그리고 클래스, 컴포저블 함수마다 어떤 정보까지만 알고 있으면 될지 조금 더 생각해보면 좋을 것 같아요!
domain/build.gradle.kts
Outdated
@@ -5,5 +5,6 @@ plugins { | |||
|
|||
dependencies { | |||
implementation(libs.inject) | |||
implementation(libs.okhttp.logging) |
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.
클린 아키텍처 원칙에 따라 도메인 계층은 네트워크와 무관해야 한다는건 알고 있지만 Multipart 때문에 넣었습니다..😿
지금 생각해보니 아래 리뷰처럼 File로 넘기고 SignUpRepositoryImpl
에서 MultipartBody.Part로 변환해도 될 것 같네요!
심약 이슈였습니다.. 고쳐올게요!
|
||
interface SignUpRepository { | ||
suspend fun signUp( | ||
profileImage: MultipartBody.Part?, |
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.
File
형태로 알고 있어도 되지 않을까요?
selectedDate = birthday, | ||
onDateSelected = { birthday = it }, | ||
selectedDate = state.traineeState.birthday, | ||
onDateSelected = { onBirthdayChange(it) }, |
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.
onDateSelected = { onBirthdayChange(it) }, | |
onDateSelected = onBirthdayChange, |
warningMessage = stringResource(R.string.entered_wrong_number), | ||
isRequired = true, | ||
keyboardType = KeyboardType.Number, | ||
trailingComponent = { | ||
UnitLabel(uiResource.string.height_unit) | ||
}, | ||
onValueChange = { height = it }, | ||
onValueChange = { onHeightChange(it) }, |
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.
onValueChange = { onHeightChange(it) }, | |
onValueChange = onHeightChange, |
warningMessage = stringResource(R.string.entered_wrong_number), | ||
isRequired = true, | ||
keyboardType = KeyboardType.Number, | ||
trailingComponent = { | ||
UnitLabel(uiResource.string.weight_unit) | ||
}, | ||
onValueChange = { weight = it }, | ||
onValueChange = { onWeightChange(it) }, |
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.
onValueChange = { onWeightChange(it) }, | |
onValueChange = onWeightChange, |
@Preview(showBackground = true) | ||
@Composable | ||
private fun TraineeBasicInfoPagePreview() { | ||
TnTTheme { | ||
TraineeBasicInfoPage( | ||
onBackClick = {}, | ||
onNextClick = {}, | ||
state = TODO(), |
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.
노트북 성능 이슈인 줄 알았는데 그냥 안 보이는 거였군요..! 고쳐보겠습니다
import co.kr.tnt.trainee.signup.component.ProgressSteps | ||
import co.kr.tnt.core.ui.R as uiResource | ||
|
||
private const val MAX_LENGTH = 100 | ||
|
||
@Composable | ||
fun TraineeNoteForTrainerPage( | ||
internal fun TraineeNoteForTrainerPage( | ||
state: TraineeSignUpUiState, |
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.
이 화면에서 state
라는 방대한 지식을 알고 있을 필요가 있을까요?!
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.
주의사항도 state로 관리하고 있기 때문에 넣어뒀습니다! 흠.. 그냥 다음 버튼을 눌렀을 때 TraineeSignUpUiState
에 반영되도록 수정해야 할까요?
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.
State
에는 주의사항 외에 height
, weight
등 이 페이지에서 전혀 사용하지 않는 상태까지 포함하고 있는 상황입니다!
이 페이지에선 어떤 상태가 필요할까요?
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.
state.traineeState.caution
으로 주의사항을 보여주고 있었습니다!
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.
그렇다면 caution: String?
형태로 넘겨줄 수 있지 않을까요?
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.
caution 넘겨받도록 수정했습니다..ㅎㅎ
// TODO 이름, 프로필 사진 불러오기 | ||
val name = "김헬짱" | ||
val profileImage = "https://buly.kr/7FQeS5M" | ||
Log.d("test", "${state.traineeState}") |
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.
로그 제거해주세요~
val height: String = "", | ||
val weight: String = "", |
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.
TraineeState
에도 해당 정보가 포함되어 있는데, 이 State
에서 따로 다루는 이유가 있을까요?
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.
height
와 weight
가 필수 값이다 보니 UserType.Trainee
에서 기본값을 각각 0
과 0.0
으로 설정했습니다.
이 정보를 그대로 state에서 사용하다 보니, 기본 정보 입력 화면
으로 이동하면 기본값인 0과 0.0이 잘못된 입력 값으로 간주되어 에러 문구가 바로 표시되는 상황이 발생했습니다
nullable로 변경하는 것은 정책상 맞지 않고 마땅한 방안이 떠오르지 않는 상황에서 더 이상 시간을 지체하지 않기 위해 기본값을 별도로 처리하는 방식으로 구현했습니다..!
val height: String = "", | ||
val weight: String = "", | ||
val isHeightValid: Boolean = false, | ||
val isWeightValid: Boolean = false, |
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.
이름 입력 화면에서는 valid
여부가 화면에 포함되어 있는데 (isWarning
), height
, weight
의 유효성 여부는 요기에 있네요!
모두 State
에 넣어 통일성을 맞추는게 좋을듯 합니다!
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.
넵 이름의 valid 여부도 State에서 관리하도록 수정하겠습니다!
- 도메인이 네트워크 무관해야 하기 때문 - UserType을 UI 데이터 저장 용도로 쓰지 않게 수정
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.
고생하셨습니다!
동작되는대로 우선 바로 머지해주세요!~~
val cautionNote: String? = "", | ||
) | ||
|
||
object SignUpRequestMapper { |
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.
LoginResponse
, SignupResponse
에서 매핑할 때와 같이 통일성 맞춰주는게 좋을 것 같습니다..!
@POST("/members/sign-up") | ||
suspend fun postSignUp( | ||
@Part profileImage: MultipartBody.Part?, | ||
@Part("request") request: RequestBody, |
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
로는 불가능한가요?
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.
multipart/form-data
요청에서는 @Body
대신 @Part
또는@PartMap
을 사용해야 한다고 해서 @Part
로 구현했습니다!
@Body
로 수정해서 호출해보면 아래와 같은 오류가 뜹니다
java.lang.IllegalArgumentException: @Body parameters cannot be used with form or multi-part encoding. (parameter #2)
response.sessionId.let { sessionId -> | ||
sessionLocalDataSource.updateSessionId(sessionId) | ||
} |
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.
response.sessionId.let { sessionId -> | |
sessionLocalDataSource.updateSessionId(sessionId) | |
} | |
sessionLocalDataSource.updateSessionId(response.sessionId) |
} | ||
|
||
private fun prepareJsonRequestBody(signUpRequest: SignUpRequest): RequestBody { | ||
val jsonString = Json.encodeToString(signUpRequest) |
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.
DI Container 에 Json
인스턴스가 등록되어 있어서, 새롭게 초기화하지 않아도 됩니다!
collectionAgreement = true, | ||
advertisementAgreement = true, | ||
) | ||
fun UserType.toSignUpRequest( |
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.
요 클래스에서 UserType
에 대한 확장 함수를 가지고 있는건 너무 어색해보입니다..!
SignUpRequest.fromDomain
이나, SignUpRequest.fromUserType
과 같은 형태가 맞는 것 같아요!
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.
지금 제 코드는 UserType
을 기반으로 새로운 SignUpRequest
객체를 생성하는 방식이라 SignUpRequest에 대한 확장 함수로 구현하기는 어려울 것 같습니다..!
companion object를 이용해 fromUserType
을 구현하는건 안 될까요?😨
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.
얽 이거 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.
그럼 지금 코드대로 머지하겠습니다!!
📝 작업 내용
Closes [TNT-114] 회원가입 기능 구현 #17
회원가입 플로우 데이터를 뷰모델에서 관리하도록 수정했습니다
회원가입 API 연동 완료했습니다
SignUpRequestMapper
는 로그인 시 전달받은 정보와 UserType 데이터를 조합하여 회원가입에 필요한 SignUpRequest 객체로 변환합니다.사진은
FileUtils
에서 Uri를 MultipartBody.Part로 변환하여 전송합니다필수 입력 항목이 아닌 경우 null로 전송합니다
아래 화면에서
시작하기
버튼을 누르면 회원가입 API가 호출되고, API 요청이 성공하면 연결하기 화면으로 이동합니다📸 실행 화면
회원가입 (트레이니)
trainee.signup.mp4
🙆🏻 리뷰 요청 사항
없습니다!
👀 레퍼런스