[Fix/#871] 마이페이지 오류 수정 #907

leeeyubin · 2024-10-13T07:30:30Z

closed [Fix] 마이페이지 오류 수정 #871

What is this issue?

로그아웃 오류 수정
한마디 편집 기능 수정 (이전 한마디 보이도록 + 토스트 띄우기)
스탬프 초기화 수정 (성공 시 로직 작성)
비회원 시, 둘러보기 기능 에러 수정
비회원 시, 로그인 기능 수정
닉네임 설정 파일 삭제

Screen_Recording_20241013_084004_SOPT.DEBUG.mp4

Reference

코드를 보시다보면 저걸 왜 저렇게 고쳤을까하는 의문이 드는 곳이 많을 거라 생각하는데요.. 질문 마구마구 해주십쇼..!
다이얼로그를 side effect로 처리해보려다가 아직 감이 안 잡혀서.. 일단은 state로 리팩해두었어요!

height · 2024-10-13T07:30:32Z

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

s9hn

개인적으로 저는 코드 일관성정도를 중요하게 여기는데요. 현재 코드는 비슷한 책임에 여러다른 방식들이 혼용되어있는 것 같아요!
uistate에 action을 분리해보는 것도 좋을 것 같아요 :)

s9hn · 2024-10-13T07:36:11Z

core/authimpl/src/main/java/org/sopt/official/auth/impl/api/AuthService.kt

@@ -41,5 +40,5 @@ interface AuthService {
    suspend fun withdraw()

    @HTTP(method = "DELETE", path = "user/logout", hasBody = true)


이거 그냥 궁금한건데 Delete 어노테이션과 차이가 있나요?

Delete에 body 값이 안들어가는게 원칙인데 굳이 넣어줘야할 때에는 이렇게 사용하는걸로 알고 있음

저거 무시하고 그냥 body 패러미터 넣으면 에러 일으킴ㅋㅋㅋ

그래서 원칙적으로는

서버한테 바디 패러미터 빼달라고 하거나

저렇게 사용하거나

인데 난 갠적으로 1로 쇼부치는게 더 깔끔하다고 생각하긴함. (정 안되면 2번)

오.. 신기하네 하나 알아갑니다. 저도 같은이유라면 서버에게 요청하고 안되면 뭐 2번해야쥬

s9hn · 2024-10-13T07:38:16Z

core/authimpl/src/main/java/org/sopt/official/auth/impl/remote/DefaultRemoteAuthDataSource.kt

@@ -46,7 +45,7 @@ class DefaultRemoteAuthDataSource @Inject constructor(
        authService.withdraw()
    }

-    override suspend fun logout(request: LogOutRequest): LogOutResponse {
-        return authService.logOut(request)
+    override suspend fun logout(request: LogOutRequest) {


개인적으로 data 레이어에서 로그인 혹은 로그아웃이라는 함수 네이밍을 지양하는 편입니다! 저장소 역할을 담당하는 객체가 로그아웃이라는 책임이 있다고 생각하지 않아요!

@s9hn 그러면 함수 네이밍 추천좀...

ㅋ한번쯤은 고민해보셔야지~

저장소는 CRUD 관련된 prefix만을 사용하려고 합니다.
개인적으로 PRND 컨벤션인 save, fetch도 나쁘지않다고 생각해요

앱을 사용하는 입장에서 보면 유저 상태를 날리는 함수인데 save, fetch라....논리적으로 맞지 않는다고 생각하는데 어떻게 생각하시나요

앱을 사용하는 입장에서 보는게 잘못된 입장이라고 생각해요!
RemoteDataSource 입장에서 보았을 때, 들어온 요청이 '원격 저장소에 있는 유저 상태를 없애줘'니까
deleteUserInfo, removeUserInfo 라는 네이밍이 적절할 것 같아요 혹은 saveDeleted(removed)userInfo으로도 쓸수는 있을것같아요

s9hn · 2024-10-13T07:40:09Z

core/authimpl/src/main/java/org/sopt/official/auth/impl/repository/AuthRepositoryImpl.kt

@@ -54,7 +54,7 @@ class AuthRepositoryImpl @Inject constructor(
        remoteAuthDataSource.withdraw()
    }

-    override suspend fun logout(pushToken: String): Result<Unit> = runCatching {
+    override suspend fun logout(pushToken: String) = runCatching {


토큰은 로컬 저장소에 저장 및 관리되고 있고 로그아웃은 무조건 본인의 토큰만을 보낼 것 같은데 파라미터로 받는 이유가 있으신가요?

pushToken을 파라미터로 받을 이유가 없다는 말씀이신가요..?!
로그아웃 로직을 살펴보니 기존 코드는 파이어베이스에서 pushToken을 가져와 파라미터로 넣어주고 있는 것 같습니다.
파이어베이스에 저장되어 있는 토큰과 로컬 저장소에 저장되어 있는 토큰이 차이점이 있을까요.........?

아하 저도 실제 사용처 찾아보니 파이어베이스 토큰을 호출해서 넣어주고 있었네요.
아마 유빈님이 작성하신 코드는 아닌 것으로 보이는데 뷰모델에서 런캐칭으로 Firebase의 토큰을 호출해주고 다음 logout함수에 전달해주고 있는 것 같아요.
저라면 Firebase에서 토큰을 가져오는 행위를 뷰모델에서 하지 않았을 것 같아요.
해당 코드는 레거시 코드이니 이번 PR에서 리팩터링 하실지는 선택에 맡기겠습니다 :)

s9hn · 2024-10-13T07:49:35Z

feature/mypage/src/main/java/org/sopt/official/feature/mypage/mypage/MyPageActivity.kt

+                                when ((state as MyPageUiState.Authenticated).action) {
+                                    MyPageAction.CLEAR_SOPTAMP -> {
+                                        MyPageDialog(
+                                            onDismissRequest = viewModel::onDismiss,


dismiss시 수행해야할 동작이 omDismiss인것은 어떤 동작인지 파악하기 힘들 것 같아요. 아래 resetSoptamp처럼 함수가 해야할 일을 직관적으로 적어도 좋을 것 같아요

함수명에 조금 더 신경 써야겠네요..!!

s9hn · 2024-10-13T07:54:22Z

feature/mypage/src/main/java/org/sopt/official/feature/mypage/mypage/MyPageActivity.kt

+                                        )
+                                    }
+
+                                    else -> {}


가급적이면 sealed나 enum으로 else 분기 없애주는게 좋긴한데 정 안되면 Unit이나 정말 else로 갈 수가 없는 경우이면 익셉션 혹은 에러뷰를 나타내도 괜찮겠어요

s9hn · 2024-10-13T08:01:48Z

feature/mypage/src/main/java/org/sopt/official/feature/mypage/mypage/MyPageViewModel.kt

-    fun setUserActiveState(new: MyPageUiState) {
-        _userActiveState.value = new
+    fun setUserActiveState(activeState: UserActiveState) {
+        if (activeState == UserActiveState.UNAUTHENTICATED) _state.value = MyPageUiState.UnAuthenticated


아래에선 update를 사용하는데 여기선 세터로 다르게 사용하는 이유가 있나용

예리하시군요.. 다른 이유가 있었던 건 아니었습니다..! 코드 통일시켜두도록 하겠습니다!

s9hn · 2024-10-13T08:04:08Z

...ge/src/main/java/org/sopt/official/feature/mypage/soptamp/sentence/AdjustSentenceActivity.kt

-                            onBackPressedDispatcher.onBackPressed()
+                val keyboardController = LocalSoftwareKeyboardController.current
+
+                LaunchedEffect(viewModel.sideEffect, lifecycleOwner) {


저도 궁금한건데 다음 전체 분기를 launchedEffect로 감싼 이유가 뭔가용..?

한마디 편집 화면에서 마이페이지로 돌아가기 위해서는 토스트를 띄우고, 뒤로가기 기능을 동시에 해줘야 하는데 이는 부수효과라고 생각해서 SharedFlow를 통해 LaunchedEffect로 한번에 처리하고자 했습니다!

혹시 keyboardController가 왜 사용됐는지를 여쭤보신걸까요..?!

음 collect는 일종의 구독 행위인데, 구독하는 행위가 왜 LaunchedEffect 내부에 있는지 궁금하신 것 같아요. 저도 궁금합니다 !

아하 음,, collect를 통해서 구독은 하고 있지만, 제가 알기론 flow가 수명주기까지는 인식을 못한다고 알고 있습니다.
그래서 lifecycleOwner를 통해 수명주기가 살아있을 때만 구독을 할 수 있도록 이를 LaunchedEffect 내부에 존재하도록 하였습니다!

질문해주신 의도에 대한 답이 되었을까요..........?

collect가 suspend function이니까...? 정도로 대답할 수 있지 않을까유

아하 sharedFlow였군요. 왜 제공되는 collect 확장함수를 사용하지 않았나해서 궁금했습니다.
그렇다면 또 궁금한것은 Event 혹은 SideEffect들을 다음과 같은 stateless Flow로 방출할 때 유실되는 이벤트는 어떻게 관리해야 하나요?

s9hn · 2024-10-13T08:04:49Z

.../src/main/java/org/sopt/official/feature/mypage/soptamp/sentence/AdjustSentenceSideEffect.kt

-    @SerialName("message")
-    val message: String
-)
+sealed class AdjustSentenceSideEffect {


별다른 생성자가 필요해보이진 않는데 sealed interface는 어떤가요?

s9hn · 2024-10-13T08:06:19Z

...e/src/main/java/org/sopt/official/feature/mypage/soptamp/sentence/AdjustSentenceViewModel.kt

+
+    private var initSentence: String = ""
+
+    val isConfirmed = sentence.map { sentence ->


자료구조임을 나타낼수 있는 네이밍이 좋을 것 같아요!

initSentence 말씀하신거 맞죠?? 저도 동의합니다 !

isConfirmed를 얘기한거긴..합니다 ㅋㅋ isXXX는 Boolean타입을 떠올리니까요!

giovannijunseokim

고생하셨습니다~! 세훈님이 너무 잘 달아주셔서 추가로 남길만한 코멘트가 없네요!
이건 별개의 이야긴데 실력이 많이 늘으셨군요 화이팅입니다 !!

다이얼로그를 side effect로 처리해보려다가 아직 감이 안 잡혀서.. 일단은 state로 리팩해두었어요!

Compose에서 SideEffect는 화면을 그리는 기능 외의 작업을 의미한다고 생각합니다. 화면의 상태를 의미하는 state에서 다이얼로그가 보이고 있는지, 아닌지를 체크하는 것이 좋다고 생각해요!

l2hyunwoo · 2024-10-15T02:09:31Z

feature/mypage/src/main/java/org/sopt/official/feature/mypage/mypage/MyPageActivity.kt

+                                when ((state as MyPageUiState.Authenticated).action) {
+                                    MyPageAction.CLEAR_SOPTAMP -> {
+                                        MyPageDialog(
+                                            onDismissRequest = viewModel::onDismiss,


이 MyPageDialog가 섹션들 사이에 있는 지 이유가 따로 있을까요?

처음에 코드를 작성할 때는 uiState가 Authenticated인 상황에서 다이얼로그를 띄우는 거라 섹션들 사이에 같이 넣어뒀었습니다.
그런데 uiState와 action은 분리하는 게 좋을 것 같다고 하셔서 현재는 바꿔주었어요!

l2hyunwoo · 2024-10-15T23:57:30Z

Compose에서 SideEffect는 화면을 그리는 기능 외의 작업을 의미한다고 생각합니다. 화면의 상태를 의미하는 state에서 다이얼로그가 보이고 있는지, 아닌지를 체크하는 것이 좋다고 생각해요!

다이얼로그나 스낵바등 알림을 띄우는 행위의 UI는 일회성 이벤트로 간주해서 effect로 관리하는게 훨씬 편합니다 (스낵바나 다이얼로그는 한번 띄워놓고 저희가 굳이 닫혀있는지 열려있는지 관리할 필요가 없자나요? 그리고 스낵바도 누르면 몇개씩 띄워지는 형태의 UI도 있구요) @giovannijunseokim

l2hyunwoo

고생하셨습니다

leeeyubin added 20 commits September 26, 2024 13:00

[fix/#871] remove logout responses

b91142f

[fix/#871] delete LogOutResponse.kt

661e1b5

[feature/#871] pull from develop

44b6727

[feature/#871] get Profile Message

ec9c02d

[feature/#871] modify button enable condition

749226f

[feature/#871] modify button enable condition

f6a3495

[feature/#871] git pull from develop

9d9b32c

[feature/#871] git pull from develop

6270326

[feature/#871] make adjust sentence toast

9eec6da

[feature/#871] delete mypage nickname view

70f077e

[feature/#871] make resetSoptamp success

eb25798

[feature/#871] add refreshToken empty function

7194eff

[feature/#871] modify login and logout logic

2afdbfc

[feature/#871] change mypage state

a994e4b

[feature/#871] delete delay function

880fb19

[feature/#871] git pull from develop

eea9cc4

[feature/#871] code refactoring

adf5d5b

[feature/#871] code refactoring

fdfaf0b

[feature/#871] git pull from develop

b12835f

[feature/#871] MIT License

9b6b873

leeeyubin self-assigned this Oct 13, 2024

leeeyubin requested a review from a team as a code owner October 13, 2024 07:30

pull-request-size bot added the size/XXL label Oct 13, 2024

s9hn requested changes Oct 13, 2024

View reviewed changes

leeeyubin added 5 commits October 14, 2024 09:06

[feature/#871] change ui state

c487625

[feature/#871] rename onDismiss function

7746918

[feature/#871] update 함수 사용

2f50399

[feature/#871] myPageState 변수명 수정

8dc7cac

[feature/#871] sealed class -> sealed interface

8bac4ac

giovannijunseokim reviewed Oct 14, 2024

View reviewed changes

leeeyubin changed the title ~~[Feature/#871] 마이페이지 오류 수정~~ [Fix/#871] 마이페이지 오류 수정 Oct 14, 2024

[feature/#871] 초기 한마디 변수명 수정

abd79f9

l2hyunwoo reviewed Oct 15, 2024

View reviewed changes

s9hn approved these changes Oct 15, 2024

View reviewed changes

[feature/#871] git pull from develop

f8d30f3

l2hyunwoo approved these changes Oct 16, 2024

View reviewed changes

[feature/#871] git pull from develop

482e355

pull-request-size bot added size/XL and removed size/XXL labels Oct 17, 2024

leeeyubin added 3 commits October 17, 2024 13:00

[feature/#871] logout -> deleteUserInfo

8fd8527

[feature/#871] 타입 명시

6083cb9

[feature/#871] License

e22c296

pull-request-size bot added size/XXL and removed size/XL labels Oct 17, 2024

leeeyubin merged commit 8abbfad into develop Oct 17, 2024
1 check passed

leeeyubin deleted the feature/#871 branch October 17, 2024 05:14

		@@ -41,5 +40,5 @@ interface AuthService {
		suspend fun withdraw()

		@HTTP(method = "DELETE", path = "user/logout", hasBody = true)


		private var initSentence: String = ""

		val isConfirmed = sentence.map { sentence ->

[Fix/#871] 마이페이지 오류 수정 #907

[Fix/#871] 마이페이지 오류 수정 #907

Conversation

leeeyubin commented Oct 13, 2024 • edited Loading

What is this issue?

Reference

height bot commented Oct 13, 2024

s9hn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l2hyunwoo Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

s9hn Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s9hn Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeeyubin Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannijunseokim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l2hyunwoo commented Oct 15, 2024

l2hyunwoo left a comment

Choose a reason for hiding this comment

leeeyubin commented Oct 13, 2024 •

edited

Loading

l2hyunwoo Oct 13, 2024 •

edited

Loading

s9hn Oct 13, 2024 •

edited

Loading

s9hn Oct 16, 2024 •

edited

Loading

leeeyubin Oct 14, 2024 •

edited

Loading

giovannijunseokim left a comment •

edited

Loading