allow deletetion by lti context id#76
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 adds support for deleting assignments using non-numeric identifiers (such as lti_context_id:ab84f579-4442-4d4a-acd8-85c5ec6fd2b6), mirroring existing functionality that already exists for editing assignments.
Changes:
- Added a String overload for
deleteAssignment()to accept non-numeric assignment IDs alongside the existing Integer version - Includes enhanced error handling with CanvasException for detailed error reporting
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/edu/ksu/canvas/interfaces/AssignmentWriter.java | Adds interface method signature for deleteAssignment with String assignmentId parameter |
| src/main/java/edu/ksu/canvas/impl/AssignmentImpl.java | Implements the new deleteAssignment overload with comprehensive error handling including 200, 204, and 404 status codes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| LOG.debug("response {}", response); | ||
| int code = response.getResponseCode(); | ||
|
|
||
| boolean errorHappened = response.getErrorHappened(); |
There was a problem hiding this comment.
I'm not sure this is actually used in the production code.
There was a problem hiding this comment.
Don't follow sorry?
There was a problem hiding this comment.
Ah because it's getting intercepted by simple rest client
There was a problem hiding this comment.
The setErrorHappened() doesn't look to ever be called in the production code and it's a class that's defined by the library. It's called by tests, but that's it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Optional<Assignment> deleteAssignment(String courseId, String assignmentId) throws IOException { | ||
| String url = buildCanvasUrl("courses/" + courseId + "/assignments/" + assignmentId, Collections.emptyMap()); | ||
| return deleteAssignment(url); | ||
| } |
There was a problem hiding this comment.
The new String-based deleteAssignment overload lacks test coverage. While the existing Integer-based deleteAssignment method also doesn't have explicit tests, this pattern follows the editAssignment methods which similarly lack explicit tests. However, given that this codebase has comprehensive test coverage for other writer operations (as seen in CalendarWriterUTest, CourseManagerUTest, etc.), consider adding test coverage for both delete methods to maintain consistency with other tested writer methods like createAssignment.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@sebastianchristopher I've opened a new pull request, #77, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: sebastianchristopher <40264653+sebastianchristopher@users.noreply.github.com>
Add test coverage for deleteAssignment methods
We can already do this for edit, adding for delete.