-
Notifications
You must be signed in to change notification settings - Fork 108
[MBL-19457][Student] Institutional Announcements Widget #3401
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
[MBL-19457][Student] Institutional Announcements Widget #3401
Conversation
Adds institutional announcements widget to Student dashboard that displays up to 5 recent institutional announcements in a carousel layout. - Create AccountNotificationRepository with implementation - Add InstitutionalAnnouncement domain model - Implement LoadInstitutionalAnnouncementsUseCase (sorts by date, limits to 5) - Create InstitutionalAnnouncementsWidget with carousel and pagination - Integrate widget into dashboard with DashboardRouter navigation - Add comprehensive unit tests for use case and ViewModel (14 tests) - Widget hides when loading, empty, or error state - Supports responsive grid layout (1-3 columns) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…go and comprehensive tests - Add UserRepository interface and implementation for account data fetching - Update LoadInstitutionalAnnouncementsUseCase to fetch institution name and logo URL - Add logoUrl field to InstitutionalAnnouncement domain model - Update widget UI to display square logo with rounded corners - Add notification icon badge on top-start corner with half overlap - Use brand color for institution name and fallback logo background - Add solid icon variants (warning, calendar, question, info) - Create comprehensive repository tests (AccountNotificationRepositoryTest, UserRepositoryTest) - Update use case and view model tests with new dependencies - All 31 tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
… and add screen tests Updates the Institutional Announcements Widget UI design and adds comprehensive instrumentation tests. ## Changes ### UI Design Updates - Changed logo from circular to square with 8dp rounded corners - Repositioned icon badge to top-start corner with 50% overlap (8dp offset) - Updated widget title to "Announcements (x)" format with dynamic count - Changed icon resources to solid variants for better visibility - Updated WARNING icon tint from textDanger to textWarning ### Icon Resources - Removed padding from ic_calendar_solid.xml while preserving structure - Removed padding from ic_question_solid.xml while preserving structure - Changed viewport from 16x16 to 13.333x13.333 and adjusted all coordinates ### Testing - Added InstitutionalAnnouncementsWidgetTest.kt with 9 screen tests - Tests cover loading/error/empty states, single/multiple announcements - Tests verify paging, click callbacks, multi-column layout, and pager indicators - Updated EnsureDefaultWidgetsUseCaseTest for new widget positioning ### Test Results - All 9 instrumentation tests passing on Pixel 2 API 29 emulator - All 4 EnsureDefaultWidgetsUseCaseTest unit tests passing - All existing unit tests passing (31 total) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
📊 Code Coverage Report
|
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.
PR Review: Institutional Announcements Widget
Summary
This PR adds a new Institutional Announcements widget to the dashboard. The implementation includes proper repository layers, use cases, ViewModels, and comprehensive test coverage. Overall, the code follows good architectural patterns and Android best practices.
Positive Highlights
✅ Excellent test coverage - Unit tests for repository, use case, ViewModel, and UI components
✅ Clean architecture - Proper separation with repository, use case, and ViewModel layers
✅ Compose best practices - Good use of state management and composables
✅ Accessibility considerations - Includes proper content descriptions for icons
✅ Proper dependency injection - Uses Hilt throughout (with one exception noted below)
✅ Error handling - Handles loading, error, and empty states appropriately
Issues to Address
Code Quality
- Missing @Inject annotation in
UserRepositoryImpl.kt:24- Constructor needs@Injectfor Hilt dependency injection - Lazy initialization over injection in
DashboardFragment.kt:35- Consider using Hilt@Injectfor the router instead of lazy initialization for better testability - Generic exception catching in
InstitutionalAnnouncementsViewModel.kt:59- Should log exceptions for debugging purposes
Maintainability
- Magic number in
LoadInstitutionalAnnouncementsUseCase.kt:47- The hardcoded limit of 5 announcements should be extracted as a named constant - Unused imports in
InstitutionalAnnouncementsWidget.kt:67- Consider organizing imports to remove unused ones
Accessibility & Localization
- Color contrast concern in
InstitutionalAnnouncementsWidget.kt:201- UsingThemePrefs.brandColordirectly without considering contrast ratios could lead to accessibility issues - Locale handling in
InstitutionalAnnouncementsWidget.kt:307- Date formatting uses default locale which may not match Canvas user preferences
Security & Performance
No security vulnerabilities or significant performance concerns identified. The implementation:
- Uses proper Kotlin coroutines for async operations
- Implements efficient pagination with HorizontalPager
- Properly limits data to 5 announcements
- Uses appropriate caching strategies
Test Coverage
Comprehensive test coverage includes:
- Repository layer tests (AccountNotificationRepositoryTest, UserRepositoryTest)
- Use case tests (LoadInstitutionalAnnouncementsUseCaseTest)
- ViewModel tests (InstitutionalAnnouncementsViewModelTest)
- UI component tests (InstitutionalAnnouncementsWidgetTest)
Recommendation
The implementation is solid with only minor issues that should be addressed. The architectural approach is sound and the code is well-tested. Once the issues above are resolved, this will be ready to merge.
|
Addressing the comments: Already Fixed✅ Router injection - Not Needed✅ @Inject annotation in UserRepositoryImpl - This repository is manually provided in ✅ Magic number (5 announcements limit) - This is the business requirement. Having a named constant for this single use case would be over-engineering. ✅ Exception logging - The ViewModel catch block properly updates the UI state. Additional logging would be redundant with existing error tracking. ✅ Brand color contrast - ✅ Date formatting locale - All tests passing (31 unit tests + 9 instrumentation tests). |
Changes DashboardFragment to use proper Hilt dependency injection for the router instead of lazy initialization for better testability. Also updates DashboardScreenTest to provide mock router parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
adamNagy56
left a comment
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.
QA +1
Implements a new Institutional Announcements Widget for the Student dashboard that displays account notifications with institutional branding.
Implementation Details
New Features
Architecture
UserRepositoryfor account data,AccountNotificationRepositoryfor notificationsLoadInstitutionalAnnouncementsUseCasecombines data from both repositories, sorts by date, limits to 5InstitutionalAnnouncementsViewModelwith StateFlow-based UI state managementDesign
Testing
Test plan:
refs: MBL-19457
affects: Student
release note: Students can now view institutional announcements on their dashboard
Checklist