Add Canvas error details and standardize API error body (#205)

tl-its-umich-edu / canvas-app-explorer

A Web application that presents a list of Canvas external (LTI) tools with details. When integrated within Canvas, the user can search for specific LTI tool(s), and add or remove those tools from Canvas courses.

Apache License 2.0

4 stars 6 forks source link

Add Canvas error details and standardize API error body (#205) #214

Closed ssciolla closed 2 years ago

ssciolla commented 2 years ago

The PR aims to resolve #205. This is still a draft. Feedback is welcome.

With these changes, I believe many errors originating from our application and all from Canvas should have an error body like the following:

{
  status_code: 404
  message: "An error occurred while communicating with Canvas: "Not Found"
}

TEST PLAN:

Setup proper values for TEST_API_KEY, TEST_API_URL and TEST_COURSE_ID fields in .env
Start CAE tool
In command line, run the test cases docker exec -it canvas_app_explorer /bin/bash -c "python manage.py test"
Observe the success test results. The test_unauthorized_error and test_not_found_error test cases actually tested for the error.status_code and error.message changes.

ssciolla commented 2 years ago

Sorry, @zqian and @jonespm, I added you as assignees accidentally, instead of reviewers. Let me know what you think of this when you have a moment. Honestly, Canvas error handling gave me huge headaches with CCM, this is similar to what we did there but may or may not be an improvement. It does seem to fix the issue raised by @zqian, but it may too complicated. It would be nice if canvasapi provided the status code in their exception subclasses.

zqian commented 2 years ago

Since this is a "wrapper" for canvasapi library, maybe we can provide this code and file an issue to UCF's canvasapi library?

ssciolla commented 2 years ago

Since this is a "wrapper" for canvasapi library, maybe we can provide this code and file an issue to UCF's canvasapi library?

Which part particularly? I think it would be nice if the library's exceptions included the status codes, is that what you mean?

zqian commented 2 years ago

I think it would be nice if the library's exceptions included the status codes, is that what you mean? Yes, that's what I meant.

jonespm commented 2 years ago

Yeah, I was kind of thinking the same thing as @zqian when I was looking at this, but that may not be the quickest solution. Still probably worth filing an issue though they probably wouldn't fix it unless we did a PR.