-
Notifications
You must be signed in to change notification settings - Fork 429
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
feat: Add endpoints for feature imports #3255
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
api/features/import_export/views.py
Outdated
class FeatureImportViewSet(viewsets.GenericViewSet, mixins.ListModelMixin): | ||
serializer_class = FeatureImportSerializer | ||
permission_classes = [FeatureImportListPermissions] | ||
|
||
def get_queryset(self) -> QuerySet[FeatureImport]: | ||
environment_ids = [] | ||
user = self.request.user | ||
|
||
for environment in Environment.objects.filter( | ||
project_id=self.kwargs["project_pk"], | ||
): | ||
if user.is_environment_admin(environment): | ||
environment_ids.append(environment.id) | ||
|
||
return FeatureImport.objects.filter(environment__in=environment_ids).order_by( | ||
"-created_at" | ||
) | ||
|
||
|
||
class FeatureImportRetrieve(viewsets.GenericViewSet, mixins.RetrieveModelMixin): | ||
serializer_class = FeatureImportSerializer | ||
permission_classes = [FeatureImportRetrievePermissions] | ||
queryset = FeatureImport.objects.all() |
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.
Since we're only including one action on each of these viewsets, perhaps we should just use standard APIView
classes? ListAPIView
/ RetrieveAPIView
I mean.
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.
Thinking about it, I'm not sure why we don't just combine them? I get that in one we have the object to do the permission check, but I don't really see how it's better than just filtering out in the queryset (which would essentially have the same effect as the get_object_or_404
that I suggested above).
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.
From our discussion on slack: Switch to just using the List and used the ListAPIView
as mentioned.
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
Introduce two new endpoints to support ongoing feature imports. One is a list the other is a retrieve. These were initially going to be as part of the same viewset, but the router differences got in the way and so it was more straightforward to split the functionality apart, since the list view relies on the
project_pk
nested part of the router.How did you test this code?
Four new tests, all of which are simple.