-
Notifications
You must be signed in to change notification settings - Fork 108
[MBL-19586][Student][Teacher] Add Portfolio to the Navigation menu #3432
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-19586][Student][Teacher] Add Portfolio to the Navigation menu #3432
Conversation
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 adds Portfolio navigation drawer integration to both Student and Teacher apps, following the same pattern as existing LTI tools (Studio, Gauge, Mastery, Arc).
Positive Observations
✅ Consistent implementation - The changes follow the exact same pattern as existing LTI tools (Studio, Gauge, Mastery, Arc)
✅ Both apps updated - Student and Teacher apps are updated in parallel with matching functionality
✅ Complete integration - All necessary touchpoints are covered: click handlers, offline state handling, visibility management
✅ Proper UI structure - Layout XML follows existing navigation drawer item patterns
✅ Icon resource added - Vector drawable for the portfolio icon is properly included
Issues Found
- Region-specific domain (
LaunchDefinition.kt:40) - The Portfolio domain usesiad.portfolio.instructure.comwhich appears region-specific (IAD = US-East), unlike other services. Verify if this should be configurable or if a generic domain should be used instead. - Mastery offline handling added (
NavigationActivity.kt:779) - This PR incidentally fixes missing offline alpha handling for the Mastery item (in addition to adding Portfolio). This is correct but represents an additional fix beyond the PR's primary goal.
Code Quality
The implementation is clean and follows established patterns. No performance, security, or architectural concerns identified.
Test Coverage
- UI tests for the Portfolio navigation item visibility based on launch definitions
- Tests for offline state handling of the Portfolio item
- Tests for the LTI launch flow with Portfolio
Recommendation
The implementation looks solid overall. The main concern is the hardcoded region-specific domain which may cause issues for users in other regions. Please verify this is the intended behavior before merging.
apps/student/src/main/java/com/instructure/student/activity/NavigationActivity.kt
Show resolved
Hide resolved
libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/models/LaunchDefinition.kt
Outdated
Show resolved
Hide resolved
🧪 Unit Test Results✅ 📱 Student App
✅ 📱 Teacher App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Fri, 05 Dec 2025 15:12:36 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
|
hermannakos
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
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
Added Portfolio launch definition support to the navigation drawer for Student and Teacher apps. Portfolio appears when the backend provides the Portfolio launch definition with domain "iad.portfolio.instructure.com".
Changes
Test plan:
refs: MBL-19586
affects: Student, Teacher
release note: Added Portfolio to the navigation menu for Student and Teacher apps
Checklist
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]