[Feature/#869] 오늘의 솝마디 대시보드 기능 구현 #872

s9hn · 2024-09-26T07:25:47Z

What is this issue?

closed [Feat] 오늘의 솝마디 기능 구현 #869

Reference

KakaoTalk_Video_2024-09-26-16-34-36.mp4

To Reviewer

동민이랑 딥톡했는데, 이번기수 앱팀 피쳐들은 ideal~하게 코딩할 예정입니다!
컴포즈 환경 멀티 모듈이 처음이라 코딩보다 세팅?같은게 어렵고 오래걸렸네요. 실수나 rc있으면 꼭 리뷰주십쇼!
Compose Text로 xml의 breakStrategy가 가능한가요? 아시는분?

- di 모듈 추가 - network api 구현

- mapper 구현 - 레포지토리 인터페이스 생성 및 적용 - 힐트모듈 수정

…into feature/869

height · 2024-09-26T07:25:50Z

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.

l2hyunwoo

고생하셨습니다. 리뷰 한번 봐주세요

l2hyunwoo · 2024-09-26T07:32:24Z

domain/fortune/src/main/java/org/sopt/official/domain/fortune/usecase/GetTodayDateUseCase.kt

+    private fun LocalDate.toFormattedDate(): String {
+        val month = monthValue.toString().padStart(2, '0')
+        val day = dayOfMonth.toString().padStart(2, '0')
+
+        return "$year-$month-$day"
+    }


SImpleDataFormatter 같은거 사용해보시는건 어떠신가요?

domain/fortune/src/main/java/org/sopt/official/domain/fortune/usecase/GetTodayDateUseCase.kt

l2hyunwoo · 2024-09-26T07:34:28Z

.../androidTest/java/org/sopt/official/feature/fortune/fortuneDetail/FortuneDetailScreenTest.kt

JUnit5 Compose 테스트 지원안하나?

하긴 할 것 같은디?

개인적으로 생각하는건, 우리의 코드베이스가 JUnit5로 맞춰져있으면 코드 통일성도 올라가고 Test 데이터 제공할때도 CsvSource나 ParamterProvider였나? 암튼 함수로 데이터 제공할 수 있어서 좀 더 편하게 코드를 생산할 수 있을거라 기대해서 테스트 상관없이 JUnit5로 스펙이 맞춰지면 좋을 것 같다고 생각하걸랑 @sopt-makers/android 어떻게 생각하시는지요들?

일단 그전에 테스트를 다들 작성할건지부터 ㅋ.ㅋ

ㅋㅋㅋ 그런데 네비게이션 테스트해서 전반적인 검수 양 줄이는 건 도입하면 좋을 것 같아 👍🏻

l2hyunwoo · 2024-09-26T07:35:02Z

...src/main/java/org/sopt/official/feature/fortune/feature/fortuneDetail/FortuneDetailScreen.kt

-        Spacer(modifier = Modifier.weight(1f))
+            is Error -> {
+                // 오류 처리
+            }

-        Button(
-            onClick = navigateToFortuneAmulet
-        ) {
-            Text(text = "Go to Fortune Amulet")
+            is Loading -> {
+                // 로딩 뷰
+            }


이건 차후에...?

아 이것도 물어보려했는데 저희 에러나 로딩일 때 띄워주는 뷰?같은거 뭐 있나요

이거 건의하겠습니다. 디폴트 로딩뷰/에러뷰 아직 없습니다.

l2hyunwoo · 2024-09-26T07:35:32Z

.../main/java/org/sopt/official/feature/fortune/feature/fortuneDetail/FortuneDetailViewModel.kt

+            }.onFailure { error ->
+                _uiState.update {
+                    Error(error)
+                }
+            }


Crashlytics에서 잡히게 Timber.e도 넣어주세요

l2hyunwoo · 2024-09-26T07:35:45Z

...n/java/org/sopt/official/feature/fortune/feature/fortuneDetail/model/FortuneDetailUiState.kt

+    @Immutable
+    data class TodaySentence(
+        val userName: String,
+        val content: String,
+    ) : FortuneDetailUiState {
+        val message: String
+            get() = "${userName}님,\n${content}"
+    }


얘는 Immutable인가요?

읽기 전용 프로퍼티를 갖고 있지만 정확한 구현체를 모르는 인터페이스나 absract한테 stable주고
그 외에 데이터클래스는 이무배 쓰는 편입니다

l2hyunwoo · 2024-09-26T07:35:50Z

...n/java/org/sopt/official/feature/fortune/feature/fortuneDetail/model/FortuneDetailUiState.kt

+        val message: String
+            get() = "${userName}님,\n${content}"


chattymin · 2024-09-26T07:37:07Z

data/fortune/build.gradle.kts

+ */
+
+plugins {
+    sopt("feature")


data 레이어인데 sopt("feature")를 사용하는게 안맞는거 같네요.
다른 data 모듈들도 보니 다 feature용을 쓰고있네요. feature에는 최근에 추가한 immutableList처럼 정말 feature에서만 쓰이는 것들이 있는데 data와 공유하기 보다는 data에 해당하는 친구를 새로 만드는게 좋을 것 같아요. data에 해당하는 custom plugin 조만간 만들어서 PR올리겠슴다.

chattymin · 2024-09-26T07:38:42Z

data/fortune/src/main/java/org/sopt/official/data/fortune/mapper/FortuneMapper.kt

+internal fun TodayFortuneCardResponse.toDomain(): TodayFortuneCard = TodayFortuneCard(
+    description = description,
+    imageColorCode = imageColorCode,
+    imageUrl = imageUrl,
+    name = name,
+)
+
+internal fun TodayFortuneWordResponse.toDomain(): TodayFortuneWord = TodayFortuneWord(
+    userName = userName,
+    title = title,
+)


저는 mapper를 클래스의 하위 함수로 만들어서 같이 두는 편인데 따로 분리하신 이유가 있나요??
mapper의 경우 해당 클래스에서만 사용되는 것이기 때문에 굳이 분리할 필요가 없고 오히려 패키지가 더 늘어난다는 생각에 합쳐두는 편입니다.

스타일 차이라고 생각하는데요 data class의 주 역할은 model로써 데이터를 wrapping하는 역할이지 mapping까지의 책임은 없다고 생각해요. 해당 model을 wrapping 하고 싶은 주체가 매핑을 하는게 맞을 것 같습니다.
매퍼를 하나의 코틀린 파일로 모아서 관리하는게 좀 더 직관적이였던 것 같고,
최상위함수로써 어떠한 인스턴스도 잡아먹지 않고 함수 2개라 큰 리소스를 차지하지 않습니다.
패키지 분리가 불편했다면,, 멀티모듈 구조를 비판해야합니다

저도 처음에는 굳이 분리할 필요 없다고 생각했는데, 추후에 복잡한 로직이 들어갈 수도 있고 명시적으로 분리해서 표시하는 게 직관적일 것 같아서 최근에는 분리해두려고 합니다. 스타일 차이인 것 같긴 하네요!

chattymin · 2024-09-26T07:47:06Z

settings.gradle.kts

+include(":data:fortune")
+include(":domain:fortune")


위에처럼 합쳐주실? ㅎ.ㅎ

chattymin · 2024-09-26T07:48:05Z

...n/java/org/sopt/official/feature/fortune/feature/fortuneDetail/component/FortuneDetailBox.kt

+@Composable
+internal fun TodayFortuneBox(
+    content: @Composable () -> Unit,
+    modifier: Modifier = Modifier,
+) {
+    Box(
+        contentAlignment = Alignment.Center,
+        modifier = modifier
+            .fillMaxWidth()
+            .padding(horizontal = 20.dp)
+            .background(
+                color = Gray700,
+                shape = RoundedCornerShape(12.dp),
+            ),
+    ) {
+        content()
+    }
+}


이부분은 분리한 이유가 있나요??
배경 색상과 모양을 나타내고 있기 때문에 따로 분리 후 Slot을 뚫어서 써준.. 그런 느낌인걸까요?

카드 형태의 뷰면 차라리

Suggested change

@Composable

internal fun TodayFortuneBox(

content: @Composable () -> Unit,

modifier: Modifier = Modifier,

) {

Box(

contentAlignment = Alignment.Center,

modifier = modifier

.fillMaxWidth()

.padding(horizontal = 20.dp)

.background(

color = Gray700,

shape = RoundedCornerShape(12.dp),

),

) {

content()

}

}

@Composable

internal fun TodayFortuneCard(

content: @Composable () -> Unit,

modifier: Modifier = Modifier,

) {

Box(

contentAlignment = Alignment.Center,

modifier = modifier

.fillMaxWidth()

.padding(horizontal = 20.dp)

.background(

color = Gray700,

shape = RoundedCornerShape(12.dp),

),

) {

content()

}

}

로 하는게 좀 더 직관적을 것 같다는 생각이 들어요

fortuneDetail 패키지 내부에서 사용할 공용 컴포넌트로 솝마디 대시보드와 콕 찌르기 대시보드의 배경 Box 컴포넌트입니다.
개인적으로 패키지 내부에서 분리하는 기준은 1. 테스트 2. 프리뷰 3. screen 내에서의 재사용성입니다.
Card보단 Box가 어울리지 않나용?

ㅇㅎ 하긴 border도 없고 그냥 RoundedCornerShape만 있네요. 이대로 가도 좋을 것 같습니다

chattymin · 2024-09-26T07:50:40Z

.../main/java/org/sopt/official/feature/fortune/feature/fortuneDetail/FortuneDetailViewModel.kt

+    init {
+        viewModelScope.launch {
+            runCatching {
+                getTodayFortuneUseCase()
+            }.onSuccess { result ->
+                _uiState.update {
+                    TodaySentence(
+                        userName = result.userName,
+                        content = result.title,
+                    )
+                }
+            }.onFailure { error ->
+                _uiState.update {
+                    Error(error)
+                }
+            }
+        }
+    }


저는 api통신을 할 때 리턴값으로 runCatching과정을 거친 Result 객체를 리턴해주는 방식을 주로 사용했습니다. 이렇게 뷰모델에서 runCatching을 처리하는 것과, �domain에서 처리 후 result를 보내주는 것. 두개의 어떤 차이가 있어서 이런 방식을 사용하시나요??
순수 궁금증

피차일반이긴 합니다 (개인적으로는)

현우형이랑은 얘기했는데, 서버통신의 runcatching, 검증과정을 누구의 역할로 볼 것인지 시각의 차이일 것 같아요.
저는 호출부의 책임이라고 생각해서 뷰모델에서 런캐칭을 열어주는 편입니다.
추가로, 지금 fortuneDetail 뷰는 오늘의한마디 API + 콕찌르기추천인 API 2개가 합쳐져서 뷰에 init되어야 하기때문에
async를 사용합니다.
result 객체를 받게되면 onSuccess, onFailure에 대한 분기 2가지를 모두 대응해줘야해서 async, awaitall을 적용하기 복잡한 구조가 됩니다.

chattymin

운세 카드 api 대신 만들어주셔서 감사합니다~
pull받고 연결 하겟슴다!

그리고 breakStrategy은 여기 참고!

leeeyubin

코드 보면서 감탄하고 갑니다.. 수고하셨어요~!!

leeeyubin · 2024-09-27T03:50:45Z

...a/org/sopt/official/feature/fortune/feature/fortuneDetail/component/TodayFortuneDashboard.kt

+                Text(
+                    text = todaySentence,
+                    style = SoptTheme.typography.title24SB,
+                    color = Gray30,
+                    textAlign = TextAlign.Center,
+                    modifier = Modifier
+                        .fillMaxWidth()
+                        .padding(horizontal = 68.dp)
+                        .semantics { contentDescription = "todaySentence" },


단순 궁금증인데 semantics는 사용자 편의를 위해 추가해주신 건가여,,,?

Compose UI Test는 컴포즈 트리구조를 가져오고 특정 노드에 접근해, 의도한 동작을 수행하는 지 검증할 수 있습니다.
특정 노드에 접근하는 방법이 여러가지가 있습니다.

실제 디스플레이에 노출되어지는 문자열을 통해 접근하기도 하고,

컴포넌트에 testTag를 달 수도,

위처럼 contentDescription을 부여하기도 합니다.

NIA 에서는 완전한 블랙박스로 특정 리소스에 의존하기 보다는 contentDescription을 부여하여 특정 "개념"이 보일 것이다. 라는 방법을 사용하고 있습니다.
https://github.com/android/nowinandroid/blob/main/feature/interests/src/androidTes[…]ogle/samples/apps/nowinandroid/interests/InterestsScreenTest.kt
실제 디스플레이에 노출되는 문자열을 통해 접근하는 경우는 가변적인 인풋에 대한 테스트가 힘들어집니다.
테스트만을 위한 testTag가 실제 컴포넌트 코드에 달려있는 것도 조금.. 불편하죠.
그나마 제 개인 스타일로는 테스트만을 위한 수단이 아닌 문자 그대로 컴포넌트의 설명 및 주석을 뜻하는 description을 부여하는게
위의 두 방법보단 좀 더 납득되�었던 것 같아요!

leeeyubin · 2024-09-27T04:06:12Z

...n/java/org/sopt/official/feature/fortune/feature/fortuneDetail/model/FortuneDetailUiState.kt

+
+@Stable
+internal sealed interface FortuneDetailUiState {
+
+    @Immutable
+    data class TodaySentence(
+        val userName: String,
+        val content: String,
+    ) : FortuneDetailUiState {
+        val message: String
+            get() = "${userName}님,\n${content}"


state 관리 좋네요 참고해가겠습니다!!

leeeyubin · 2024-09-27T04:20:43Z

data/fortune/src/main/java/org/sopt/official/data/fortune/mapper/FortuneMapper.kt

+internal fun TodayFortuneCardResponse.toDomain(): TodayFortuneCard = TodayFortuneCard(
+    description = description,
+    imageColorCode = imageColorCode,
+    imageUrl = imageUrl,
+    name = name,
+)
+
+internal fun TodayFortuneWordResponse.toDomain(): TodayFortuneWord = TodayFortuneWord(
+    userName = userName,
+    title = title,
+)


저도 처음에는 굳이 분리할 필요 없다고 생각했는데, 추후에 복잡한 로직이 들어갈 수도 있고 명시적으로 분리해서 표시하는 게 직관적일 것 같아서 최근에는 분리해두려고 합니다. 스타일 차이인 것 같긴 하네요!

l2hyunwoo · 2024-09-27T22:19:38Z

domain/fortune/src/main/java/org/sopt/official/domain/fortune/usecase/GetTodayDateUseCase.kt

+import javax.inject.Inject
+
+class GetTodayDateUseCase @Inject constructor() {
+


Suggested change

그리고 이정도 로직은 UseCase 굳이 만들어야하나 하는 생각이 있긴 합니다.

@l2hyunwoo 삽인정입니다. 근데 뭐 UI에서 사용할 로직도 아니긴해서 ideal하겐 도메인이 적합할 것 같아서 분리했습니다.
개행은,, 저는 프로퍼티는 개행없이 놓고 함수는 한줄 띄우는 편인데 이렇게하면 뭐 걸리거나 잡히나요?

l2hyunwoo · 2024-09-27T22:22:40Z

.../androidTest/java/org/sopt/official/feature/fortune/fortuneDetail/FortuneDetailScreenTest.kt

@s9hn @chattymin UI 테스트 좋은데 개발자가 손수 적는 것보다 스크린샷 테스팅해서 PR에 올라오는게 생산성 좋을듯 (동민아 해줘)

근데 또 개인적으론, 스크린샷 테스팅도 배웠는데 아직 많이 지원이 .....미숙합니다.
리소스는 들지만 직접 작성하는 테스트에 비해 테스트의 이점들을 완전히 커버하기도 힘들어보여서 저는 갠적으론,, 절레절레

l2hyunwoo

가자 🚀

…into feature/869

s9hn added 17 commits September 25, 2024 16:23

feat: TodayFortuneBox 구현

1aa4880

feat: 오늘의 운세(TodayFortuneDashboard) UI 구현

427fcc1

feat: FortuneDetailRoute, FortuneDetailScreen 파일 분리

1a75a66

refactor: 패키지 네이밍 수정

e265c4e

feat: fortune 데이터레이어 구현

3b483ac

- di 모듈 추가 - network api 구현

feat: fortune 도메인레이어 구현, internal 키워드 일괄 적용

4241e3f

- mapper 구현 - 레포지토리 인터페이스 생성 및 적용 - 힐트모듈 수정

refactor: internal 키워드 삭제

533a29f

refactor: 함수 네이밍 수정

f655b50

feat: 레이어 의존성 추가

e26989c

refactor: 네트워크 path 수정

5950cec

feat: 네트워크 통신 구현 및 state 적용

71001df

feat: 오늘의 솝마디 관련 유즈케이스 생성

1e42c44

refactor: TodayFortuneBox 패딩값 추가

625d11e

refactor: 타입 및 네이밍 수정

430b830

refactor: internal 추가

83c0641

test: FortuneDetailScreenTest 구현

8d86bcb

Merge branch 'develop' of https://github.com/sopt-makers/sopt-android …

340c78e

…into feature/869

s9hn requested a review from a team as a code owner September 26, 2024 07:25

auto-assign bot assigned s9hn Sep 26, 2024

pull-request-size bot added the size/XL label Sep 26, 2024

s9hn changed the title ~~신병받아라~~~ [Feature/#869] 오늘의 솝마디 대시보드 기능 구현 Sep 26, 2024

l2hyunwoo requested changes Sep 26, 2024

View reviewed changes

chattymin reviewed Sep 26, 2024

View reviewed changes

chattymin approved these changes Sep 26, 2024

View reviewed changes

refactor: SimpleDataFormatter로 리팩터링

2f9ffff

s9hn added 3 commits September 27, 2024 01:37

refactor: Timber 적용

69a0acc

build: 의존성 수정

8981910

refactor: break strategy 적용

a7c5b50

leeeyubin approved these changes Sep 27, 2024

View reviewed changes

l2hyunwoo reviewed Sep 27, 2024

View reviewed changes

l2hyunwoo approved these changes Sep 27, 2024

View reviewed changes

Merge branch 'develop' of https://github.com/sopt-makers/sopt-android …

9384ee5

…into feature/869

s9hn merged commit 23fbdb9 into develop Sep 30, 2024
1 check passed

s9hn deleted the feature/869 branch September 30, 2024 01:36

s9hn restored the feature/869 branch September 30, 2024 01:43

		import javax.inject.Inject

		class GetTodayDateUseCase @Inject constructor() {

		include(":data:fortune")
		include(":domain:fortune")

[Feature/#869] 오늘의 솝마디 대시보드 기능 구현 #872

[Feature/#869] 오늘의 솝마디 대시보드 기능 구현 #872

Conversation

s9hn commented Sep 26, 2024 • edited Loading

What is this issue?

Reference

To Reviewer

height bot commented Sep 26, 2024

l2hyunwoo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s9hn Sep 26, 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

chattymin left a comment

Choose a reason for hiding this comment

leeeyubin left a 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

Choose a reason for hiding this comment

l2hyunwoo left a comment

Choose a reason for hiding this comment

s9hn commented Sep 26, 2024 •

edited

Loading

s9hn Sep 26, 2024 •

edited

Loading