-
Notifications
You must be signed in to change notification settings - Fork 121
[MBL-19532][S] Learner Dashboard - Course Invitations #3873
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
Conversation
…nsions of stored properties.
refs: MBL-19528 builds: Student affects: Student release note: none test plan:
…ions # Conflicts: # Canvas.xcworkspace/xcshareddata/swiftpm/Package.resolved
Co-authored-by: Attila Varga <[email protected]>
- Fix carousel layout issues. - Centralize course logic to shared interactor. - Remove no longer needed business logic entities.
…ature/MBL-19532-Course-Invitations # Conflicts: # Student/Student/LearnerDashboard/Widgets/Common/View/DashboardWidgetLayout.swift # Student/Student/LearnerDashboard/Widgets/Common/View/LearnerDashboardWidgetLayoutHelpers.swift # Student/Student/LearnerDashboard/Widgets/DashboardWidgetIdentifier.swift # Student/Student/LearnerDashboard/Widgets/FullWidthWidget/ViewModel/FullWidthWidgetViewModel.swift # Student/Student/LearnerDashboard/Widgets/LearnerDashboardWidgetAssembly.swift
- Fix default widgets not getting ordered properly. - Extracted default widget animation to standalone file. - Removed unnecessary nav title. - Remove unnecessary selfs.
…ature/MBL-19532-Course-Invitations # Conflicts: # Student/Student/LearnerDashboard/Widgets/FullWidthWidget/ViewModel/FullWidthWidgetViewModel.swift
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.
Code Review Summary
This PR implements a comprehensive Course Invitations Widget for the Learner Dashboard. The implementation is well-structured with good separation of concerns, comprehensive test coverage, and follows MVVM architecture. However, there are several critical issues that should be addressed before merging.
🎯 What This PR Does
- Adds Core layer APIs for accepting/declining course invitations
- Implements a new dashboard widget with horizontal carousel UI
- Adds request coalescing for optimized network performance
- Includes database schema changes to track enrollment creation dates
- Provides comprehensive unit test coverage
✅ Strengths
- Clean Architecture: Proper MVVM separation with clear boundaries
- Comprehensive Testing: Good test coverage for ViewModels and Interactors
- Performance Optimization: Request coalescing prevents duplicate network calls
- Reusable Components:
HorizontalCarouselViewcan be reused for other widgets - Error Handling: Most failure scenarios are handled with user feedback
⚠️ Issues Found
I've identified several issues that need attention. Please review the inline comments for specific code locations and recommendations.
Critical Issues (Must Fix)
- Memory leak risk in
AcceptCourseInvitation.swift- subscriptions never cleared (line 103) - Silent error swallowing in
AcceptCourseInvitation.swift- failed fetches are not logged (lines 52-75) - Potential data loss in
GetAllUserCourses.swift- deletes courses on partial API failures (lines 88-92) - Race condition in
Course.swift- enrollment delete/fetch not synchronized (lines 137-150) - Thread safety concern in
CoursesInteractor.swift- subscriptions storage not protected (lines 77-84)
High Priority Issues (Should Fix)
- Force unwrapping in
Enrollment.swift- could crash if fetch fails (lines 208-209) - Edge case in
CourseInvitationCardView.swift- empty section names display as ", " (line 149) - Concurrency edge case in
CourseInvitationCardViewModel.swift- rapid accept/decline taps (line 62)
Additional Recommendations
- Consider adding UI/integration tests for the complete invitation flow
- Add accessibility tests for dynamic content announcements
- Add timeout configuration for API requests to prevent indefinite loading states
- Consider extracting hardcoded spacing values to design system constants
- Add tests for CoreData migration with the new
createdAtfield
🔍 Testing Notes
The unit tests are comprehensive and cover most success/failure scenarios. Consider adding:
- UI tests for the actual widget interaction flow
- Performance tests for request coalescing under high load
- Edge case tests for horizontal carousel pagination
- Accessibility tests for VoiceOver announcements
📝 Architecture Notes
The implementation follows the project's MVVM pattern well. Minor observation: some business logic in CourseInvitationsWidgetViewModel.refresh() could potentially be extracted to the Interactor layer for better separation, but this is not a blocking issue.
Overall, this is a well-crafted feature implementation. Once the critical issues are addressed, particularly around thread safety and error handling, this will be ready to merge.
Student/Student/LearnerDashboard/Container/Model/GetAllUserCourses.swift
Show resolved
Hide resolved
...Student/LearnerDashboard/Widgets/CourseInvitationsWidget/View/CourseInvitationCardView.swift
Show resolved
Hide resolved
...arnerDashboard/Widgets/CourseInvitationsWidget/ViewModel/CourseInvitationCardViewModel.swift
Show resolved
Hide resolved
Student/Student/LearnerDashboard/Container/Model/CoursesInteractor.swift
Show resolved
Hide resolved
BuildsCommit: Remove extra new line. (09bb788) |
# Conflicts: # Student/Student/LearnerDashboard/Widgets/Common/View/DashboardWidgetLayout.swift
|
Code +1 |
suhaibabsi-inst
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
Tested on iPhone 16, iOS 26.2.
| import CoreData | ||
| import Foundation | ||
|
|
||
| public class DeclineCourseInvitation: UseCase { |
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.
Interesting way of using UseCase!
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.
Just realized I could have used DeleteUseCase to make this simpler. Next time.
# Conflicts: # Student/Student/LearnerDashboard/Widgets/LearnerDashboardWidgetAssembly.swift # Student/Student/Localizable.xcstrings # Student/StudentUnitTests/LearnerDashboard/Container/Model/LearnerDashboardInteractorTests.swift
petkybenedek
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.
Student/Student/LearnerDashboard/Container/Model/CoursesInteractor.swift
Outdated
Show resolved
Hide resolved
...erDashboard/Widgets/CourseInvitationsWidget/ViewModel/CourseInvitationsWidgetViewModel.swift
Outdated
Show resolved
Hide resolved
Student/StudentUnitTests/LearnerDashboard/Container/Model/CoursesLiveInteractorTests.swift
Outdated
Show resolved
Hide resolved
...nt/StudentUnitTests/LearnerDashboard/Container/Model/CoursesInteractorInvitationsTests.swift
Outdated
Show resolved
Hide resolved
...erDashboard/Widgets/CourseInvitationsWidget/ViewModel/CourseInvitationsWidgetViewModel.swift
Outdated
Show resolved
Hide resolved
| @State private var totalPages: Int = 1 | ||
|
|
||
| var body: some View { | ||
| ZStack { |
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.
Do we need the ZStack?
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.
Without this, the preview doesn't appear. I tried moving the ZStack to the preview but didn't fix it. If you feel you can play with it, I'm also curious what happens there.
...arnerDashboard/Widgets/CourseInvitationsWidget/ViewModel/CourseInvitationCardViewModel.swift
Outdated
Show resolved
Hide resolved
...Student/LearnerDashboard/Widgets/CourseInvitationsWidget/View/CourseInvitationCardView.swift
Show resolved
Hide resolved
Student/Student/LearnerDashboard/Widgets/Common/View/HorizontalCarouselView.swift
Outdated
Show resolved
Hide resolved
rh12
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 (tested on iPhone @ 26, iPad @ 18)
Thanks for all the changes!

What's new?
Known Issues
refs: MBL-19532
builds: Student
affects: Student, Teacher, Parent
release note: none
test plan:
Screenshots
Checklist