-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add boundary views and serializers #113
Conversation
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.
Left some comments.
My tendency has been to used function-based views in the past rather than class-based ones. ViewSets add standardization and offer good built-in support, but can make custom / non-standard work a little difficult. DRF has a brief write-up on this: https://www.django-rest-framework.org/tutorial/6-viewsets-and-routers/#trade-offs-between-views-vs-viewsets
For now, I think ViewSets are fine. If we need to refactor to function-based views down the line, we can do it then.
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.
Sorry for the piecemeal review. Other things I noticed while working on the ReferenceImage serializer.
4407843
to
41173e9
Compare
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.
I know this is still WIP, but was hoping this would still be helpful.
src/django/api/views/boundary.py
Outdated
if not request.user.has_access_to_utility( | ||
Utility.objects.get(pk=utility_id) | ||
): | ||
raise APIException('Cannot access this utility', 401) |
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.
This could merit updating the ADR, but if we just raise PermissionDenied
, DRF will convert this into a 403, which I think is more accurate than 401 given that this is an insufficient permission issue rather than incorrect creds issue.
Sample response from what I was just working on in 97ef19b (and 61615c9, updated to use the DRF import of PermissionDenied to get the accurate detail message):
GET /api/boundaries/1/reference-images/
HTTP 403 Forbidden
Allow: GET, POST, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept
{
"detail": "Validators cannot view image details."
}
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.
I'm okay with that. Looping in @rajadain
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. We can also finalize the 403 vs. 401 thing later on.
Taking a look now |
This needs the migrations to be updated:
|
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.
This looks great!
Minor comments on utility filtering, but overall this looks great. We should defer the Activity Log to a later card.
|
||
return BOUNDARY_STATUS.SUBMITTED | ||
|
||
@cached_property |
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.
Nice work caching this 👍
|
||
@property | ||
def resolved(self): | ||
return self.resolved_at is not None |
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.
Nice shortcut
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.
+1 great job! Nice, clean implementation. Sets up a solid foundation for us to build upon. This looks good to merge!
The endpoint is /boundaries/ as of #113, so the RTK query is updated accordingly.
This allows developers to create a virtual env for this project. One might want to do this so the IDE will know about required packages.
This allows for testing each submission in the same submission list.
8ce625e
to
5ccc0be
Compare
The additional boundaries will allow for testing each stage of the submission process.
5ccc0be
to
94b93c5
Compare
The endpoint is /boundaries/ as of #113, so the RTK query is updated accordingly.
The endpoint is /boundaries/ as of #113, so the RTK query is updated accordingly.
The endpoint is /boundaries/ as of #113, so the RTK query is updated accordingly.
Overview
This PR adds serializers for submissions and a few related objects and views for submissions.
Closes #107
Notes
I added a venv folder to the .gitignore so I could install requirements locally.
TODO: Create activity log
Testing Instructions
scripts/manage resetdb
in order to test the validator user, if desiredChecklist
fixup!
commits have been squashedCHANGELOG.md
updated with summary of features or fixes, following Keep a Changelog guidelinesREADME.md
updated if necessary to reflect the changes