Skip to content
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

[AN] refactor: 모임 에러 핸들링 #601

Merged
merged 11 commits into from
Oct 1, 2024
Merged

Conversation

jinuemong
Copy link
Contributor

이슈

개발 사항

  • 모임 전체 에러 핸들링 적용하였습니다.

전달 사항 (없으면 삭제해 주세요)

  • Local은 에러 처리를 뷰에서 별도로 해서 일단 TODO

Copy link

github-actions bot commented Sep 30, 2024

Test Results

43 tests  ±0   43 ✅ ±0   1s ⏱️ ±0s
 5 suites ±0    0 💤 ±0 
 5 files   ±0    0 ❌ ±0 

Results for commit de52872. ± Comparison against base commit d01080a.

♻️ This comment has been updated with latest results.

gaeun5744
gaeun5744 previously approved these changes Sep 30, 2024
Copy link
Member

@gaeun5744 gaeun5744 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니당~

Comment on lines 52 to 59
this.error.observeEvent(owner) { clubError ->
when (clubError) {
ClubErrorEvent.FileSizeError -> sendToast(R.string.file_size_exceed_message)
ClubErrorEvent.ServerError -> sendToast(R.string.server_error_message)
ClubErrorEvent.UnKnownError -> sendToast(R.string.default_error_message)
ClubErrorEvent.InternetError -> sendSnackBar(R.string.no_internet_message)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금은 앱의 규모가 작아서 괜찮지만, 추후에는 error.toUiText()이렇게 확장 메서드를 따로 만들어서 acitivty/fragment 에서 observe하도록 수정하는것도 좋을것 같습니다!

observe의 책임은 이곳이 아니라 다른 곳이 나을듯하여 코멘트 달아요~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

observe의 책임은 이곳이 아니라 다른 곳이 나을듯하여 코멘트 달아요~동의합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaeun5744 @junjange
매번 같은 메시지를 제공해서 ui의 확장함수를 만들었어요
acitvity나 fragment에서 열도록 하는게 잘못되었을까요?
observe가 책임을 잘 수행할 수 있도록 도와줬습니다

junjange
junjange previously approved these changes Sep 30, 2024
Copy link
Member

@junjange junjange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다~~ 코멘트 관련한 부분만 같이 고민해보면 좋을 것 같아요~~

Comment on lines 52 to 59
this.error.observeEvent(owner) { clubError ->
when (clubError) {
ClubErrorEvent.FileSizeError -> sendToast(R.string.file_size_exceed_message)
ClubErrorEvent.ServerError -> sendToast(R.string.server_error_message)
ClubErrorEvent.UnKnownError -> sendToast(R.string.default_error_message)
ClubErrorEvent.InternetError -> sendSnackBar(R.string.no_internet_message)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

observe의 책임은 이곳이 아니라 다른 곳이 나을듯하여 코멘트 달아요~동의합니다!

@jinuemong jinuemong dismissed stale reviews from junjange and gaeun5744 via de52872 October 1, 2024 08:28
@jinuemong jinuemong force-pushed the refactor/club-error branch from ae64c87 to de52872 Compare October 1, 2024 08:29
@jinuemong
Copy link
Contributor Author

lifecycle에 의존하고 있는 것 같아서 수정해봤습니다.
MessageHandler를 두어서 반복되는 로직에 적용했습니다.

Copy link
Member

@gaeun5744 gaeun5744 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 반영 수고하셨습니다~

Comment on lines +11 to +18
fun ClubErrorEvent.handleError(sendMessage: (MessageHandler) -> Unit) {
when (this) {
ClubErrorEvent.FileSizeError -> sendMessage(MessageHandler.SendToast(R.string.file_size_exceed_message))
ClubErrorEvent.ServerError -> sendMessage(MessageHandler.SendToast(R.string.server_error_message))
ClubErrorEvent.UnKnownError -> sendMessage(MessageHandler.SendToast(R.string.default_error_message))
ClubErrorEvent.InternetError -> sendMessage(MessageHandler.SendSnackBar(R.string.no_internet_message))
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러 메시지를 보여주는 방법은 view/appbar/dialog 등등 다양한 방법이 있습니다!
이렇게 에러를 보여주는 방법을 activity/fragment가 아닌 다른 곳에서 정의하면, 추후에 확장하기 어려워 보이네요 🥲

저는 같은 club domain 영역이라도 뷰의 디자인에 따라 에러 메시지를 다양하게 보여줄 수 있다고 생각하기 때문에 코멘트를 남깁니다!

++ FileSizeError는 특정 뷰에서만 쓰입니다..! 나중에 시간 되시면, 이렇게 공통으로 묶는 방법이 아니라 각 뷰의 특성에 따라 에러를 나누는 방법에 대해 고민하면 좋을 것 같습니다!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전에 벼리랑 논의 했었던 내용이네여 ㅋㅋㅋㅋㅋㅋㅋㅋ 굿굿!!

Copy link
Member

@junjange junjange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다~~ 코멘트 확인 부탁드립니다~~

Comment on lines +25 to +32
when (error) {
is DataError.Network -> {
when (error) {
DataError.Network.NO_INTERNET -> ClubErrorEvent.InternetError
DataError.Network.FILE_SIZE_EXCEED -> ClubErrorEvent.FileSizeError
DataError.Network.UNKNOWN -> ClubErrorEvent.UnKnownError
else -> ClubErrorEvent.ServerError
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 분리하면 좋을 것 같아요~~
당장 반영 안해주셔도 됩니다!!

Comment on lines +11 to +18
fun ClubErrorEvent.handleError(sendMessage: (MessageHandler) -> Unit) {
when (this) {
ClubErrorEvent.FileSizeError -> sendMessage(MessageHandler.SendToast(R.string.file_size_exceed_message))
ClubErrorEvent.ServerError -> sendMessage(MessageHandler.SendToast(R.string.server_error_message))
ClubErrorEvent.UnKnownError -> sendMessage(MessageHandler.SendToast(R.string.default_error_message))
ClubErrorEvent.InternetError -> sendMessage(MessageHandler.SendSnackBar(R.string.no_internet_message))
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전에 벼리랑 논의 했었던 내용이네여 ㅋㅋㅋㅋㅋㅋㅋㅋ 굿굿!!

Comment on lines +146 to +147
showSnackbar(getString(message.messageId)) {
setAction(resources.getString(R.string.club_detail_fail_button)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context.getString(message.messageId)resources.getString(R.string.club_detail_fail_button) 차이가 무엇인가요?!!

@jinuemong jinuemong merged commit 1d8f222 into develop Oct 1, 2024
2 checks passed
takoyakimchi pushed a commit that referenced this pull request Oct 23, 2024
@jinuemong jinuemong changed the title 모임 에러 핸들링 [AN] refactor: 모임 에러 핸들링 Oct 28, 2024
@jinuemong jinuemong deleted the refactor/club-error branch November 4, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants