-
Notifications
You must be signed in to change notification settings - Fork 107
[MBL-19559][Student] Fix New Quizzes error handling for expired assignments #3414
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
base: master
Are you sure you want to change the base?
Conversation
Fixed issue where New Quizzes with past until dates would fail to load properly. Changed getExternalToolLaunchUrl to return null on failure instead of throwing exception, and added proper error handling in the ViewModel to show a user-friendly error message. Changes: - AssignmentDetailsNetworkDataSource: Return null instead of throwing exception when getExternalToolLaunchUrl API call fails - AssignmentDetailsViewModel: Check for null externalLTITool and show error toast instead of attempting navigation - Added comprehensive test coverage for both success and failure cases Test plan: - Create a New Quiz assignment with an until date in the past - As a student, try to view the assignment submission - Verify error toast is displayed instead of app crash 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
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.
Review Summary
This PR improves error handling for external tool launch URLs by changing from exception-based error handling to nullable return values. This is a good architectural improvement that makes error handling more explicit and prevents crashes.
Key Changes
- Changed
getExternalToolLaunchUrlreturn type fromLTITooltoLTITool?in the repository interface and network data source - Updated error handling from
dataOrThrowtodataOrNullinAssignmentDetailsNetworkDataSource - Added null check in
AssignmentDetailsViewModelto gracefully handle failures with a user-friendly error message - Updated tests to reflect the new behavior (returns null instead of throwing exception)
- Added new test case for the null LTI tool scenario in the ViewModel
Positive Aspects
✅ Consistent with existing code: The LocalDataSource already returns null for this method, so this change makes the network implementation consistent
✅ Better user experience: Users now see a friendly error message instead of a crash
✅ Proper test coverage: Both the data source test and ViewModel test were updated to cover the new null-handling behavior
✅ Follows Kotlin idioms: Using nullable types is more idiomatic than throwing exceptions for expected failure cases
✅ Clean implementation: The null check is straightforward and handles the error case appropriately
Suggestions for Improvement
- Consider adding error logging: Track when this failure occurs to understand its frequency in production (see inline comment on
AssignmentDetailsViewModel.kt:691) - Consider more specific error message: The generic "unexpected error" message could be replaced with something more specific like "Unable to load external tool" (see inline comment on
AssignmentDetailsViewModel.kt:691)
Code Quality Assessment
- Architecture: ✅ Good - follows repository pattern correctly
- Error Handling: ✅ Good - graceful degradation instead of crashes
- Test Coverage: ✅ Good - both unit tests updated appropriately
- Code Style: ✅ Good - follows Kotlin conventions and project patterns
- Performance: ✅ No concerns
- Security: ✅ No concerns
Overall, this is a solid improvement to error handling. The suggestions above are minor enhancements and not blocking issues.
...n/java/com/instructure/pandautils/features/assignments/details/AssignmentDetailsViewModel.kt
Show resolved
Hide resolved
...n/java/com/instructure/pandautils/features/assignments/details/AssignmentDetailsViewModel.kt
Show resolved
Hide resolved
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
Test plan:
refs: MBL-19559
affects: Student
release note: Fixed issue where students couldn't view New Quizzes submissions after the assignment's until date had passed
Checklist