AB#113906 allow get assignment options to have string assignmentId#78
Conversation
sebastianchristopher
requested review from
buckett,
Copilot,
lardo-de-arnaud and
nicholaswilson100
There was a problem hiding this comment.
Pull request overview
This pull request modifies GetSingleAssignmentOptions to support non-numeric assignment identifiers, specifically allowing LTI context IDs in the format "lti_context_id:ab84f579-4442-4d4a-acd8-85c5ec6fd2b6". The change aligns with Canvas API's support for qualified identifiers (similar to "sis_course_id:abc123" for courses) and maintains backward compatibility through constructor overloading.
Changes:
- Changed
assignmentIdfield type fromIntegertoStringto support both numeric IDs and qualified identifiers - Added constructor overload accepting
Integerto maintain backward compatibility with existing code
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| public GetSingleAssignmentOptions(String courseId, Integer assignmentId) { | ||
| public GetSingleAssignmentOptions(String courseId, String assignmentId) { |
There was a problem hiding this comment.
The new functionality allowing String assignment IDs (e.g., "lti_context_id:ab84f579-4442-4d4a-acd8-85c5ec6fd2b6") lacks test coverage. While existing tests will continue to work through the backward-compatible Integer constructor, there should be a test that verifies the String constructor works correctly with non-numeric IDs, ensuring the URL is properly constructed and the API call succeeds.
| public GetSingleAssignmentOptions(String courseId, String assignmentId) { | ||
| if(courseId == null || assignmentId == null) { | ||
| throw new IllegalArgumentException("Course and assignment IDs are required"); | ||
| } | ||
| this.courseId = courseId; | ||
| this.assignmentId = assignmentId; | ||
| } |
There was a problem hiding this comment.
The constructor Javadoc (inherited from line 17-21 in AssignmentReader) states it retrieves "a specific assignment from Canvas by its Canvas ID number", but this is now outdated since the constructor accepts String to support non-numeric identifiers like "lti_context_id:...". Consider updating the documentation to clarify that it accepts either a numeric Canvas ID or a qualified identifier (e.g., "lti_context_id:abc123"), similar to how GetSingleCourseOptions documents "May be of the form sis_course_id:abc123".
…tOptions.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* refactor GetSingleAssignmentOptions constructor to use Objects.toString for null-safe assignmentId handling
|
@sebastianchristopher Looks like build is failing. |
Allow us to pass non-numeric ids like ~"lti_context_id:ab84f579-4442-4d4a-acd8-85c5ec6fd2b6"