[CLXR-57][Horizon] Learn program screen#3502
[CLXR-57][Horizon] Learn program screen#3502domonkosadam wants to merge 12 commits intoCLX-3743-learn-course-screenfrom
Conversation
Implemented unit and UI tests for Learn Program features following the Learn Course test patterns: Unit Tests: - LearnProgramListRepositoryTest (5 tests) - LearnProgramListViewModelTest (24 tests) - ProgramDetailsRepositoryTest (6 tests) - ProgramDetailsViewModelTest (28 tests) UI Tests: - LearnProgramListUiTest (6 tests) - ProgramDetailsUiTest (8 tests) Tests use real data objects instead of mocks for data classes, following best practices. All tests cover filtering, search, pagination, progress calculation, course enrollment, and UI rendering. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Thu, 05 Feb 2026 15:40:06 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR implements a comprehensive Learn Programs feature for the Horizon module, including program listing and detailed views with excellent test coverage. The implementation follows the project's MVVM architecture and Compose patterns well.
Positive Highlights ✅
- Excellent test coverage: Both unit tests and UI tests are comprehensive, covering edge cases and various states
- Clean architecture: Proper separation of concerns with Repository, ViewModel, and UI layers
- Good use of Compose: Leverages modern Compose features like HorizontalPager, LaunchedEffect, and state management
- Reusable components: Well-structured components like
ProgramCourseCard,ProgramProgress, and progress indicators - Consistent patterns: Follows existing project conventions for feature organization and naming
Issues to Address 🔧
- libs/horizon/.../ProgramCourseCard.kt:76 - Code style: Remove space before colon in
enabled : Boolean - libs/horizon/.../ProgramCourseCard.kt:108 - Architecture: Consider passing a lambda instead of
NavHostControllerfor better testability - libs/pandautils/.../DateExtensions.kt:189 - Edge case:
toFormattedString()returns empty string for zero duration - libs/horizon/.../LearnScreen.kt:59 - Potential infinite loop: Add guard condition in
LaunchedEffect(state.selectedTab) - libs/horizon/.../LearnProgramListRepository.kt:36 - Error handling:
getCoursesByIdlacks exception handling for failed async calls
Test Coverage ✅
The PR includes comprehensive testing:
- UI Tests:
LearnProgramListUiTestandProgramDetailsUiTestcover loading states, content display, filtering, and edge cases - Unit Tests:
LearnProgramListViewModelTestandProgramDetailsViewModelTestthoroughly test business logic with 20+ test cases each - Repository Tests: Both list and details repositories have proper test coverage
All tests follow the project's testing patterns using Mockk and Compose testing utilities.
Security & Performance
No security concerns identified. The code properly handles network calls asynchronously and uses appropriate loading states. Consider the error handling improvement suggested for parallel course fetching to prevent failures from affecting the entire list.
Overall, this is a well-implemented feature that maintains high code quality standards. The inline comments address minor improvements that would enhance robustness and maintainability.
🤖 Generated with Claude Code
| @OptIn(ExperimentalLayoutApi::class) | ||
| @Composable | ||
| fun ProgramCourseCard(state: ProgramCourseCardState, modifier: Modifier = Modifier) { | ||
| fun ProgramCourseCard( |
There was a problem hiding this comment.
Consider extracting the navigation logic from the composable. Rather than passing NavHostController directly, pass a lambda like onCourseClicked: (Long) -> Unit to maintain better separation of concerns and make the component more testable and reusable.
This would change the signature to:
fun ProgramCourseCard(
state: ProgramCourseCardState,
onCourseClicked: (Long) -> Unit,
modifier: Modifier = Modifier
)Then in the clickable:
.conditional(state.enabled) {
clickable { onCourseClicked(state.id) }
}This approach keeps the composable independent of navigation framework details.
| val courseClicked: (() -> Unit)? = null, | ||
| val onEnrollClicked: (() -> Unit)? = null, | ||
| val enrollLoading: Boolean = false, | ||
| val enabled : Boolean = false, |
There was a problem hiding this comment.
Spacing issue: Remove the space before the colon. Kotlin style guide recommends no space before colons in property declarations.
val enabled: Boolean = false,| null | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The toFormattedString function could return an empty string if both hours and minutes are 0. Consider handling this edge case explicitly to provide a meaningful return value (e.g., "0 mins") or document this behavior.
fun Duration.toFormattedString(
resources: Resources,
@PluralsRes hoursLabelRes: Int = R.plurals.durationHours,
@PluralsRes minutesLabelRes: Int = R.plurals.durationMins,
separator: String = " "
): String {
val hours = inWholeHours.toInt()
val minutes = (inWholeMinutes % 60).toInt()
val formattedStringParts = mutableListOf<String>()
if (hours > 0) {
formattedStringParts.add(resources.getQuantityString(hoursLabelRes, hours, hours))
}
if (minutes > 0) {
formattedStringParts.add(resources.getQuantityString(minutesLabelRes, minutes, minutes))
}
// Handle zero duration case
if (formattedStringParts.isEmpty()) {
return resources.getQuantityString(minutesLabelRes, 0, 0)
}
return formattedStringParts.joinToString(separator)
}| } | ||
| } | ||
|
|
||
| LaunchedEffect(state.selectedTab) { |
There was a problem hiding this comment.
The LaunchedEffect with state.selectedTab could cause an infinite loop if pagerState.animateScrollToPage triggers a state change that updates selectedTab. Consider adding a check to prevent unnecessary animations:
LaunchedEffect(state.selectedTab) {
val pageIndex = LearnTab.entries.indexOf(state.selectedTab)
if (pagerState.currentPage != pageIndex) {
pagerState.animateScrollToPage(pageIndex)
}
}| return getProgramsManager.getPrograms(forceRefresh) | ||
| } | ||
|
|
||
| suspend fun getCoursesById(courseIds: List<Long>, forceNetwork: Boolean = false): List<CourseWithModuleItemDurations> = coroutineScope { |
There was a problem hiding this comment.
The getCoursesById function could throw an exception if any of the async calls fail. Consider adding error handling to make this more robust:
suspend fun getCoursesById(courseIds: List<Long>, forceNetwork: Boolean = false): List<CourseWithModuleItemDurations> = coroutineScope {
courseIds.mapNotNull { id ->
async {
try {
getCoursesManager.getProgramCourses(id, forceNetwork).dataOrThrow
} catch (e: Exception) {
// Log error or handle appropriately
null
}
}
}.awaitAll().filterNotNull()
}Or document that callers should handle exceptions from this method.
|
Student Install Page |
1 participant
refs: CLXR-57
affects: Student
release note: none