[MBL-19515][Student] Fix cross-shard user submissions #3412
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixed issue where cross-shard users would get page errors when viewing their submission details and comments. Changes: - Added shard ID parsing from course tab URLs in EnabledTabs - Created APIHelper methods for shard ID extraction and global user ID conversion - Updated SubmissionDetailsEffectHandler to use global user IDs for cross-shard courses - Updated DiscussionSubmissionViewFragment to handle override domains - Injected ApiPrefs into SubmissionDetailsEffectHandler for better testability - Added comprehensive unit tests for shard ID handling Technical details: - Parse shard IDs from tab.externalUrl (format: "https://domain.com/courses/7053~2848/...") - Store shard IDs per course in ApiPrefs.shardIds map - Extract shard ID from access token if present (format: "shardId~token") - Convert user IDs to global format (shardId * 10^13 + userId) for cross-shard API calls - Handle routing and authentication for override domains in discussion submissions Test plan: - Cross-shard user can view submission details without page error - Same-shard user continues to work as before - Discussion submissions route correctly for both main and override domains - Unit tests verify shard ID parsing and conversion logic Affects: Student 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
kristofnemere
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR implements cross-shard course support for Canvas Android by handling cases where a user's courses exist on different shards than their account. The implementation adds shard ID tracking, global user ID conversion, and domain override handling for multi-shard scenarios.
Strengths
- Comprehensive test coverage: Two new unit tests verify both cross-shard and same-shard scenarios in
SubmissionDetailsEffectHandlerTest - Solid utility functions:
APIHelperadditions are well-structured with clear responsibilities - Good separation of concerns: Effect handler properly injects
ApiPrefsdependency rather than using singleton directly - Consistent pattern: The domain override logic in
DiscussionSubmissionViewFragmentfollows good encapsulation practices
Issues Found
- Null safety concern in
SubmissionDetailsEffectHandler.kt:75- Using!!operator without null check (see inline comment) - Error handling needed in
APIHelper.createGlobalUserId()-toLong()conversion can throw (see inline comment) - Empty string validation in
DiscussionSubmissionViewFragment.kt:58- Should checkisNullOrEmpty()instead of just null (see inline comment) - Defensive programming in
EnabledTabs.kt:79- Consider usinggetOrNull()for safer array access (see inline comment)
Performance Considerations
- The shard ID parsing from URL happens on each tab initialization, but this should be acceptable given it's a one-time setup operation
- The
getUserIdForCourse()function is efficient with simple map lookups and string comparisons
Architecture Notes
- Follows MVVM pattern correctly with dependency injection via Hilt
- Properly extends the existing
ApiPrefsobject with newshardIdsmap - Maintains backward compatibility by falling back to original behavior when shard IDs aren't present
Test Coverage
The new tests cover the critical scenarios:
- ✅ Cross-shard course access with global user ID conversion
- ✅ Same-shard course access without conversion
- ✅ Shard ID extraction from tokens
- ✅ Global user ID creation logic
Overall, this is a well-designed implementation with good test coverage. The inline comments highlight some defensive programming improvements that would make the code more robust against edge cases.
...ructure/student/mobius/assignmentDetails/submissionDetails/SubmissionDetailsEffectHandler.kt
Show resolved
Hide resolved
apps/student/src/main/java/com/instructure/student/router/EnabledTabs.kt
Show resolved
Hide resolved
libs/canvas-api-2/src/main/java/com/instructure/canvasapi2/utils/APIHelper.kt
Show resolved
Hide resolved
...udent/mobius/assignmentDetails/submissionDetails/content/DiscussionSubmissionViewFragment.kt
Outdated
Show resolved
Hide resolved
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
Changes based on PR review comments: - EnabledTabs.kt: Use getOrNull() for safer array access when parsing shard IDs - APIHelper.kt: Add error handling for toLong() conversion in createGlobalUserId() - DiscussionSubmissionViewFragment.kt: Check isNullOrEmpty() instead of just null for override domains These changes improve null safety and error handling throughout the cross-shard submission code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Student Install Page |
hermannakos
approved these changes
Nov 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixed issue where cross-shard user submissions were throwing page errors. When a course is on a different shard than the user's home shard, the user ID now gets converted to a global user ID for API calls.
Changes:
Test plan
refs: MBL-19515
affects: Student
release note: Fixed errors when viewing submissions in courses across different Canvas shards