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

알림함에서 강의상세 진입 시 강의평 조회실패 수정 #390

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

plgafhd
Copy link
Collaborator

@plgafhd plgafhd commented Jan 27, 2025

@plgafhd plgafhd requested a review from a team as a code owner January 27, 2025 16:27
@JuTaK97
Copy link
Collaborator

JuTaK97 commented Jan 30, 2025

겁나게 복잡허네잉

@eastshine2741
Copy link
Collaborator

굿 이해했어
딥링크로 timetable_id, lecture_id 보니 원본 강의말고 시간표의 강의를 보여주는 게 의도된 거 같고
실제로 시간표의 강의를 갖고오는 로직도 DeeplinkExecutor에 있고

그렇다면 LectureDetailPage의 mode도 Normal(시간표의 강의)인 게 맞는듯??

기획상으로 맞는 건지 더블체크까지 되면
플로팅버튼은 새로 논의해서 풀어보자

Comment on lines 116 to 120
if (navController.currentDestination?.route == NavigationDestination.LectureDetail) {
navController.popBackStack()
} else if (navController.currentDestination?.route?.contains(NavigationDestination.TimetableLecture) == true) {
onCloseViewMode(scope) // 알림함에서 진입하는 경우 다시 알림함으로 돌아가야 함
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

navController.currentDestination?.route == NavigationDestination.LectureDetail 분기의 정체)
뒤로가기 연타하면 흰 화면 보이는? 버그를 막기 위한 코드
요 분기때문에 이 작업에서 TimetableLecture 분기도 추가되어 코드를 이해하기 어려워짐

-> 지금은 발생하지 않는 버그거나 발생하더라도 사소한 수준이라면
요 분기 없애고
Normal, Viewing 통일하고
onCloseViewMode를 항상 넣어줄 수 있도록 코드상에서 강제해본다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이거 분기 없애면 강의 상세에서 좌상단의 뒤로가기 버튼이랑 그냥 뒤로가기 동시에 눌렀을 때
하얀 화면 되는 버그 100% 재현 가능한데다 한번 그렇게 되면 앱 나갔다가 들어와도 하얀 화면에 갇혀서,
분기 없애기는 힘든 것 같아
#184 (comment)

@plgafhd
Copy link
Collaborator Author

plgafhd commented Feb 26, 2025

이거 알림함에서 여는건 편집 불가능한게 맞대.. ㅋㅋㅋㅋ
그리고 강의 편집할 때 현재 자신의 tableId가 아니라 그 강의가 있는 tableId도 보내야 하는데 그거 처리도 꽤 까다롭고...

Comment on lines 77 to 80
}.onFailure { e ->
apiOnError(e)
return
}.getOrElse { return }
Copy link
Collaborator

Choose a reason for hiding this comment

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

getOrElse의 block이랑 onFailure의 block 실행 조건이 같아서
onFailure 지우고 그 내용을 getOrElse에만 넣어도 될듯??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 그러네..

@@ -53,7 +53,7 @@ class LectureDetailViewModel @Inject constructor(
* 로컬 저장소에는 리뷰 정보를 저장하지 않으므로, 시간표탭에서 강의상세로 진입하면 editingLectureDetail.value.review가 null이다
* 따라서 getLectureReview()로 리뷰 정보만을 따로 불러온다
*/
lecture.review?.rating?.let { lecture.review }
lecture.review?.reviewCount?.let { lecture.review }
Copy link
Collaborator

Choose a reason for hiding this comment

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

요거 rating에서 reviewCount로 바뀐 이유가 있닝

Copy link
Collaborator Author

@plgafhd plgafhd Mar 8, 2025

Choose a reason for hiding this comment

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

이거는 그냥 reivew 정보가 없다 == reviewCount가 null이다 != rating이 null이다 인것 같았어
리뷰 정보가 있더라도 reviewCount가 0이면 rating이 null로 내려와서

Comment on lines 75 to 80
val lectureReview = runCatching {
homePageLectureDetailViewModel.getLectureReview(lectureToShow.lecture_id)
}.onFailure { e ->
apiOnError(e)
return
}.getOrElse { return }
Copy link
Collaborator

Choose a reason for hiding this comment

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

lectureReview가 없으면 ev에서 새로 받아오는 로직이 LectureDetailViewModel에 있는데
그것 때문에 에러가 나니까 DeeplinkExecutor에서 lectureReview를 미리 채워두는거지?
조금 암시적인 것 같아서,, 일단 주석만 달아두는 것 어떨까

Copy link
Collaborator Author

@plgafhd plgafhd Mar 8, 2025

Choose a reason for hiding this comment

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

'그것 때문에 에러가 나니까'도 맞고,
기존 코드에서 LectureDetailViewModel 내부에서는 ev에 lecture_id를 보낼지, id를 보낼지 판단 근거가 ModeType밖에 없는데,
알림함에서 진입할 때는 Viewing임에도 lecture_id를 보내야 하기 때문에, LectureDetailViewModel 내부의 코드랑 어긋나서, 미리 채우는 방법으로 하긴 했어

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

주석 달아놓을게

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants