-
Notifications
You must be signed in to change notification settings - Fork 4
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
User regions #446
User regions #446
Conversation
New Request: |
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.
Some comments on the python code from an initial look over, I'll test this locally and do a deeper review that includes the frontend changes next.
rdwatch/core/views/region.py
Outdated
@router.get('/details', response=list[dict]) | ||
@paginate(PageNumberPagination, page_size=1000) | ||
def list_region_details(request: HttpRequest): | ||
regions = Region.objects.values_list( | ||
'name', 'owner__username', 'public', 'pk', 'geom' | ||
) | ||
return [ | ||
{ | ||
'name': region[0], | ||
'owner': region[1] if region[1] is not None else 'None', | ||
'value': region[0] if region[1] is None else f'{region[0]}_{region[1]}', | ||
'public': region[2], | ||
'id': region[3], | ||
'deleteBlock': get_delete_block(region[3], request.user), | ||
'hasGeom': True if region[4] else False, | ||
} | ||
for region in regions | ||
] |
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 think this should use a ninja Schema
instead of an untyped dict. As it is, it's difficult to parse what's going on with the indexes IMO, and we're also iterating over a queryset when we don't need to. i.e. changing to something like
@router.get('/details', response=list[dict]) | |
@paginate(PageNumberPagination, page_size=1000) | |
def list_region_details(request: HttpRequest): | |
regions = Region.objects.values_list( | |
'name', 'owner__username', 'public', 'pk', 'geom' | |
) | |
return [ | |
{ | |
'name': region[0], | |
'owner': region[1] if region[1] is not None else 'None', | |
'value': region[0] if region[1] is None else f'{region[0]}_{region[1]}', | |
'public': region[2], | |
'id': region[3], | |
'deleteBlock': get_delete_block(region[3], request.user), | |
'hasGeom': True if region[4] else False, | |
} | |
for region in regions | |
] | |
@router.get('/details', response=list[RegionDetailResponseSchema]) | |
@paginate(PageNumberPagination, page_size=1000) | |
def list_region_details(request: HttpRequest): | |
regions = Region.objects.values_list( | |
'name', 'owner__username', 'public', 'pk', 'geom' | |
) | |
return regions |
Where RegionDetailResponseSchema
is defined as needed.
rdwatch/core/views/region.py
Outdated
return Response( | ||
{'detail': 'Region model created successfully', 'region_id': str(region.name)}, | ||
status=201, | ||
) |
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.
It should be possible to simplify this to
return Response( | |
{'detail': 'Region model created successfully', 'region_id': str(region.name)}, | |
status=201, | |
) | |
return 201, {'detail': 'Region model created successfully', 'region_id': str(region.name)} |
But, if we want to return structured output that has a detail
and region_id
field, I think this should be defined as a Schema
instead of using a raw Reponse
object.
rdwatch/core/views/region.py
Outdated
) | ||
# return a successful response for deletion of the region | ||
selected_region.delete() | ||
return JsonResponse({'success': 'Region deleted successfully.'}, status=200) |
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.
See previous comment about simplifying the response.
rdwatch/core/views/region.py
Outdated
selected_region = get_object_or_404(Region, pk=region_id) | ||
if selected_region.owner != owner: # 403 response, only owner can delete a region | ||
return JsonResponse( | ||
{'error': 'You do not have permission to delete this region.'}, status=403 |
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.
See previous comment about simplifying the response.
Co-authored-by: Mike VanDenburgh <[email protected]>
Co-authored-by: Mike VanDenburgh <[email protected]>
Co-authored-by: Mike VanDenburgh <[email protected]>
Co-authored-by: Mike VanDenburgh <[email protected]>
Co-authored-by: Mike VanDenburgh <[email protected]>
resolves #445
owner, public
fields to aregion
andmodel_run
. Defaults public toTrue
and owner toNone
in both instancesPOST
endpoint for Region creation to create a region not associated with model run and being bound to a user with the option to make it PublicGET /region/details
endpoint to get information about owners and public to the list. This also includes some information about why a user can't delete a region. They are only able to delete regions they own that have all model runs removed from them./regions/{region_id}/vector-tile/z/x/y.pbf
endpoint. This is to support visualizing regions when no model-runs are associated with them. This is required to be able to see a region and eventually be able to delete it.regionMap
to the store to align a region.