-
Notifications
You must be signed in to change notification settings - Fork 334
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
Fix/edge identity/remove deleted features #1451
Conversation
Check and Update identity document if deleted feature exists in the identity document before processing it
api/tests/integration/edge_api/identities/test_edge_identity_featurestates_viewset.py
Outdated
Show resolved
Hide resolved
api/tests/integration/edge_api/identities/test_edge_identity_featurestates_viewset.py
Show resolved
Hide resolved
api/edge_api/identities/views.py
Outdated
identity_feature_names = {fs.feature.name for fs in identity.identity_features} | ||
if not identity_feature_names.issubset(valid_feature_names): | ||
identity.prune_features(valid_feature_names) | ||
sync_identity_document.delay(args=(build_identity_dict(identity),)) |
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 we'd be better off just sending the identity id or composite key (depending on what we have available here )to the task and grabbing the identity out of the database in the task. That should avoid any issues with serialization of task arguments (and remove the need for the DjangoJSON encoder that you added).
It adds another read to the database in the task code but I think it will be cleaner overall.
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.
Yeah, that makes sense
api/task_processor/models.py
Outdated
@@ -84,7 +85,7 @@ def kwargs(self) -> typing.Dict[str, typing.Any]: | |||
@staticmethod | |||
def _serialize_data(data: typing.Any): | |||
# TODO: add datetime support if needed | |||
return json.dumps(data) | |||
return json.dumps(data, cls=DjangoJSONEncoder) |
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.
As above, I don't think we should need this?
No description provided.