-
Notifications
You must be signed in to change notification settings - Fork 1
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/force update #545
Feature/force update #545
Conversation
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 com.teampophory.pophory.designsystem.R | ||
import com.teampophory.pophory.designsystem.databinding.DialogCommonOneButtonBinding | ||
|
||
class ForceUpdateDialog : DialogFragment() { |
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.
class ForceUpdateDialog : DialogFragment() { | |
class ForceUpdateDialogFragment : DialogFragment() { |
val title = arguments?.getString("title", "") | ||
val description = arguments?.getString("description", "") | ||
val imageResId = arguments?.getInt("imageResId") | ||
val buttonText = arguments?.getString("buttonText", "") |
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.
이거 stringArgs 같은 확장함수 있지 않았나?
): ForceUpdateDialog { | ||
ForceUpdateDialog().apply { | ||
arguments = Bundle().apply { | ||
putString("title", title) | ||
putString("description", description) | ||
putInt("imageResId", imageResId) | ||
putString("buttonText", buttonText) | ||
} | ||
}.also { | ||
return 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.
그냥
return ForceUpdateDialog().apply {
arguments = Bundle().apply {
putString("title", title)
putString("description", description)
putInt("imageResId", imageResId)
putString("buttonText", buttonText)
}
}
이게 가독성 더 좋지 않나요..?
): ForceUpdateDialog { | ||
ForceUpdateDialog().apply { | ||
arguments = Bundle().apply { | ||
putString("title", title) | ||
putString("description", description) | ||
putInt("imageResId", imageResId) | ||
putString("buttonText", buttonText) | ||
} | ||
}.also { | ||
return 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.
그냥
return ForceUpdateDialog().apply {
arguments = Bundle().apply {
putString("title", title)
putString("description", description)
putInt("imageResId", imageResId)
putString("buttonText", buttonText)
}
}
이게 가독성 더 좋지 않나요..?
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.
재사용하지 않고 ForceUpdateDialog에 상수값을 넣어두는 것으로 변경하였습니다!
remoteConfig.setDefaultsAsync( | ||
mapOf( | ||
"minimum_required_version" to "1.4.0" | ||
) | ||
) |
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를 set하면 최소 버전 업할때마다 여기 코드를 손수 바꿔주고 배포를 해줘야 하겠네요? 강업은 서버에서 언제든지 변수를 조절해서 바꾸는데 의미가 있는건데 이런 default 설정은 불필요한 것 같아요.
물론 파베에서 minimum_required_version 값을 설정해줘서 이 디폴트는 그냥 순수 디폴트다 라고 말은 할 수 있겠지만, 로컬 디폴트도 결국에는 우리가 원하는 수준까지는 올려줘야 하는걸 신경써야 하는거니까 유지보수 코스트는 올라가는거라고 볼 수 있죠
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.
로직을 생각해서 반영했는데 체크해주시면 감사드리겠습니다.
fun getString(key: String): String { | ||
return remoteConfig.getString(key) | ||
} |
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.
나라면 operator fun get 함수 오버로딩 할 것 같음. 의미론상 그게 좀 더 적절할 것 같고, getString은 너무 포괄적인 함수이름이야... getStringKey일 수 있고, getStringValue일 수 있자늠?
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.
넵 이거 제거 해버렸습니다!
suspendCancellableCoroutine { continuation -> | ||
remoteConfig.fetchAndActivate().addOnCompleteListener { task -> | ||
if (task.isSuccessful) { | ||
continuation.resume(remoteConfig[key]) | ||
} else { | ||
continuation.resume(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.
kotlinx-coroutines-play-services
이거 참고해보실라우?
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.
제거완료
) : RemoteConfigRepository { | ||
|
||
override suspend fun getMinRequiredVersion(): String { | ||
val minVersion = remoteConfigDataSource.getString(KEY_MIN_VERSION, "1.4.0") |
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.
디폴트 여긴 1.4.0인데 내부에선 왜 1.4.1 쓰고 있죠?
private fun isUpdateRequired(currentVersion: String, minRequiredVersion: String): Boolean { | ||
val currentVersionParts = currentVersion.split(".").map { it.toIntOrNull() ?: 0 } | ||
val minVersionParts = minRequiredVersion.split(".").map { it.toIntOrNull() ?: 0 } | ||
|
||
for (i in currentVersionParts.indices) { | ||
val currentPart = currentVersionParts.getOrElse(i) { 0 } | ||
val minPart = minVersionParts.getOrElse(i) { 0 } | ||
if (currentPart < minPart) { | ||
return true | ||
} else if (currentPart > minPart) { | ||
return false | ||
} | ||
} | ||
return 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.
이거 사용해보세요
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.
생각해보니까 firebase 안쓰네? 라이브러리 없애주세요!
gradle/libs.versions.toml
Outdated
@@ -198,6 +200,7 @@ sentry = { module = "io.sentry:sentry", version.ref = "sentry-android" } | |||
|
|||
zxing-core = { module = "com.google.zxing:core", version = "zxing-core" } | |||
zxing-android-embedded = { module = "com.journeyapps:zxing-android-embedded", version.ref = "zxing-android-embedded" } | |||
firebase-config-ktx = { group = "com.google.firebase", name = "firebase-config-ktx", version.ref = "firebaseConfigKtx" } |
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.
firebase-config-ktx = { group = "com.google.firebase", name = "firebase-config-ktx", version.ref = "firebaseConfigKtx" } | |
firebase-config-ktx = { group = "com.google.firebase", name = "firebase-config", version.ref = "firebaseConfigKtx" } |
이거 ktx는 예전 라이브러리일거입니다. 지금은 firebase-config
로 다 대체되었어.
참고 - https://firebase.google.com/support/release-notes/android
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.
맞습니다 제거할게요!
gradle/libs.versions.toml
Outdated
appVersion = "1.4.1" | ||
versionCode = "10401" |
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.
여기서 1.4.1 버전을 올리나요?
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.
여기서 올리는 것보단 릴리즈 브랜치에서 1.4.1로 올리는게 나을 것 같습니다. 원복해주시는게 좋을 것 같아요!
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.
넹!
private fun getCurrentVersionName(): String { | ||
return try { | ||
val packageInfo = context.packageManager.getPackageInfo(context.packageName, 0) | ||
packageInfo.versionName | ||
} catch (e: Exception) { | ||
"1.4.1" | ||
} | ||
} |
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/config/proguard-rules.pro
Outdated
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.
삭제 해도 될듯
fun provideMinimumVersionService(@Secured retrofit: Retrofit): MinimumVersionService = | ||
retrofit.create(MinimumVersionService::class.java) |
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.
fun provideMinimumVersionService(@Secured retrofit: Retrofit): MinimumVersionService = | |
retrofit.create(MinimumVersionService::class.java) | |
fun provideMinimumVersionService(@Secured retrofit: Retrofit): MinimumVersionService = retrofit.create() |
이렇게 해도 됩니당
data/config/proguard-rules.pro
Outdated
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.
지워도 될듯?
2f86aee
to
064534c
Compare
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.
고생하셨습니다.
gradle/libs.versions.toml
Outdated
@@ -79,6 +79,7 @@ kakao = "2.20.4" | |||
splash-screen = "1.0.1" | |||
javax-inject = "1" | |||
spotless = "6.25.0" | |||
firebaseConfigKtx = "22.0.0" |
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.
이거만 삭제합시다.
이슈 코드
📸 스크린샷
Screen_recording_20240731_175855.mp4
Screen_recording_20240731_180309.mp4
🍀 관련 이슈