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.
| LOG.debug("response {}", response); | ||
| int code = response.getResponseCode(); | ||
|
|
||
| boolean errorHappened = response.getErrorHappened(); | ||
|
|
||
| if (!errorHappened) { | ||
| switch (code) { | ||
| case 200: | ||
| return responseParser.parseToObject(Assignment.class, response); | ||
| case 204: | ||
| case 404: // already deleted, treat as success | ||
| return Optional.empty(); | ||
| default: | ||
| // fall through to error | ||
| } | ||
| } | ||
|
|
||
| String msg = response.getContent(); | ||
| if (msg == null || msg.isBlank()) { | ||
| msg = "Canvas returned HTTP " + code; | ||
| } | ||
|
|
||
| throw new CanvasException(msg, url, response); |
There was a problem hiding this comment.
The error handling in this String overload is inconsistent with the Integer overload (lines 66-77). The Integer version simply returns Optional.empty() on any error or non-200 status code, while this version throws CanvasException with detailed error information. This creates API inconsistency where the same logical operation behaves differently based on the parameter type.
Consider either:
- Updating the Integer overload to use the same sophisticated error handling, or
- Simplifying this implementation to match the Integer overload's behavior
For consistency with other delete operations in the codebase (e.g., CalendarEventImpl line 75-85, AssignmentGroupImpl line 82-87), the simpler error handling pattern (returning Optional.empty() on error) appears to be the established convention.
| LOG.debug("response {}", response); | |
| int code = response.getResponseCode(); | |
| boolean errorHappened = response.getErrorHappened(); | |
| if (!errorHappened) { | |
| switch (code) { | |
| case 200: | |
| return responseParser.parseToObject(Assignment.class, response); | |
| case 204: | |
| case 404: // already deleted, treat as success | |
| return Optional.empty(); | |
| default: | |
| // fall through to error | |
| } | |
| } | |
| String msg = response.getContent(); | |
| if (msg == null || msg.isBlank()) { | |
| msg = "Canvas returned HTTP " + code; | |
| } | |
| throw new CanvasException(msg, url, response); | |
| LOG.debug("response " + response.toString()); | |
| if(response.getErrorHappened() || response.getResponseCode() != 200){ | |
| LOG.debug("Failed to delete assignment, error message: " + response.toString()); | |
| return Optional.empty(); | |
| } | |
| return responseParser.parseToObject(Assignment.class, response); |
There was a problem hiding this comment.
I'm sorta with Copilot here, it seems the new method is quite different to the existing delete method and having different behaviour across 2 very similar methods is confusing.
There was a problem hiding this comment.
True, perhaps it needs a wider rewrite... it seems odd to me that everything other than a 200 just returns an empty optional - it's not very illuminating and leaves the calling code to guess what the problem might be.
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
| case 200: | ||
| return responseParser.parseToObject(Assignment.class, response); | ||
| case 204: | ||
| case 404: // already deleted, treat as success |
There was a problem hiding this comment.
I think this case is already handled in the SimpleRestClient checks so seems a little strange having it here.
There was a problem hiding this comment.
I don't think the 404 will be, but perhaps it's best handled in the calling code
| LOG.debug("response {}", response); | ||
| int code = response.getResponseCode(); | ||
|
|
||
| boolean errorHappened = response.getErrorHappened(); | ||
|
|
||
| if (!errorHappened) { | ||
| switch (code) { | ||
| case 200: | ||
| return responseParser.parseToObject(Assignment.class, response); | ||
| case 204: | ||
| case 404: // already deleted, treat as success | ||
| return Optional.empty(); | ||
| default: | ||
| // fall through to error | ||
| } | ||
| } | ||
|
|
||
| String msg = response.getContent(); | ||
| if (msg == null || msg.isBlank()) { | ||
| msg = "Canvas returned HTTP " + code; | ||
| } | ||
|
|
||
| throw new CanvasException(msg, url, response); |
There was a problem hiding this comment.
I'm sorta with Copilot here, it seems the new method is quite different to the existing delete method and having different behaviour across 2 very similar methods is confusing.
We can already do this for edit, adding for delete.