-
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: Clone identity flag states #3773
Changes from 16 commits
0f95266
0327df9
d0aa54d
2c6fa65
80edfbc
1d440eb
24522c4
4c4f4fe
fd504fb
3ba5df8
2a70bbe
1032376
476005a
d59c38c
81e6e55
40abe1b
938ade1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
from environments.identities.models import Identity | ||
from environments.identities.serializers import ( | ||
IdentityAllFeatureStatesSerializer, | ||
IdentitySourceIdentityRequestSerializer, | ||
) | ||
from environments.models import Environment | ||
from environments.permissions.permissions import ( | ||
|
@@ -636,6 +637,34 @@ def all(self, request, *args, **kwargs): | |
|
||
return Response(serializer.data) | ||
|
||
@swagger_auto_schema( | ||
request_body=IdentitySourceIdentityRequestSerializer(), | ||
responses={200: IdentityAllFeatureStatesSerializer(many=True)}, | ||
) | ||
@action(methods=["POST"], detail=False, url_path="clone-from-given-identity") | ||
def clone_from_given_identity(self, request, *args, **kwargs) -> Response: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: missing typing in the args There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't seen our selves using typing in rest_framework handlers, in general I haven't seen that Python HTTP event handlers, even my IDE doesn't complaint about it, or including the unsed params args and kwargs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a ton of code that sets |
||
""" | ||
Clone feature states from a given source identity. | ||
""" | ||
serializer = IdentitySourceIdentityRequestSerializer( | ||
data=request.data, context={"request": request} | ||
) | ||
serializer.is_valid(raise_exception=True) | ||
# Get and validate source and target identities | ||
target_identity = get_object_or_404( | ||
queryset=Identity, pk=self.kwargs["identity_pk"] | ||
) | ||
source_identity = get_object_or_404( | ||
queryset=Identity, pk=request.data.get("source_identity_id") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use the serializer which has already been validated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My IDE compliants about using the serialized.validated_data since it doesn't understand that, as you mentioned, they were alreay validated, but precily because of that we can use any of both ways right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the right way of doing it. When people refactor code they may easily miss accessing data directly but with the moved code it could be done now before validation. To keep things safe, it's best to use the validated data directly. |
||
) | ||
|
||
# Clone feature states | ||
FeatureState.copy_identity_feature_states( | ||
target_identity=target_identity, source_identity=source_identity | ||
) | ||
|
||
return self.all(request, *args, **kwargs) | ||
|
||
|
||
@method_decorator( | ||
name="list", | ||
|
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.
Related thread: #3773 (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.
Right, we don't need to load the whole related feature but just the FK which is already in the feature_sate data.
Done