-
Notifications
You must be signed in to change notification settings - Fork 3
Program Letter View #605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Program Letter View #605
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #605 +/- ##
==========================================
+ Coverage 76.50% 76.52% +0.02%
==========================================
Files 242 244 +2
Lines 10141 10296 +155
Branches 1721 1738 +17
==========================================
+ Hits 7758 7879 +121
- Misses 2217 2248 +31
- Partials 166 169 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall works great! Just noticed a couple things:
- Some docstrings are wrong or missing.
- If I enter the wrong uuid in the letter url, I would expect to get a 404 page but instead I get an error message if it is a valid but wrong uuid, or a mostly blank letter on the frontend and a backend 'not a valid UUID' ValidationError if it isn't a valid uuid:
|
@mbertrand Re
You're actually seeing a 404 page covered up by a dev-mode error overlay. In production, you'd only get the 404 page. The error overlay has a little X in upper right corner that you can click to get rid of it. I find the error overlay annoying / confusing for 404 and 403 errors, too. IIRC the error overlay is pretty easy to disable entirely, but I don't think I found a way to disable it just for the 404/403/401 errors that we handle with NotFound and Forbidden pages. Although, the error overlay only shows for synchronous errors—which doesn't include the 500 API response. (It does include the 401, 403, 404 b/c we intentionally monitor for those asynchronous errors and re-throw them so a handler can show the forbidden or not-found pages). And in my experience synchronous errors don't happen too often, so maybe disabling it would be best.
I see that issue, too. |
|
@mbertrand I resolved the blank page issue by checking for valid uuids and 404'ing server side if invalid which will trigger the 404 on the frontend. |
mbertrand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 👍


What are the relevant tickets?
Closes #555
Description (What does it do?)
This PR implements a shareable program letter view for users who have earned a program certificate in micromasters.
The the flow for this is:
Main Changes
The changes in this PR can be grouped into 3 main parts:
1 - An api route added to create a program letter object and fetch template data from micromasters
2 - Autogenerated api schema for above route
3 - fronted react view to render the program letter
How can this be tested?
docker-compose exec web python manage.py shell