feat: Merge person API call #332
Conversation
utils/api.py
Outdated
|
|
||
| @wraps(f) | ||
| def wrapped(request, *args, **kwargs): | ||
| if not isinstance(request, Request): |
There was a problem hiding this comment.
I had to make this change to make because wrapper gets a MergePersonView object for the request. May be a short coming of the wrapper because of the way MergePersonView is implemented?
There was a problem hiding this comment.
It's because the decorator is designed for functions, not instance methods, and the self argument is getting in the way. We should probably rewrite the decorator along the lines of:
def requires_api_token(...):
def _get_request_from_args(args, kwargs):
if "request" in kwargs:
return kwargs["request"]
elif isinstance(args[0], Request):
return args[0]
elif isinstance(args[1], Request):
return args[1]
raise ValueError # or something
def decorate(f):
# ...
def wrapped(*args, **kwargs):
request = _get_request_from_args(args, kwargs)
# ...That should work as long as we're reasonably careful about where we use the decorator. There may be cleaner ways to do it. It also might be preferable to define a separate decorator for instance methods to avoid the complexity of trying to figure it out dynamically.
There was a problem hiding this comment.
As we mean for this to work with function-based views (request is first arg) or these class-based views (request is second arg and a View subclass is the first arg as self), it might also be good to verify that args[0] is a View subclass when accepting args[1]. Just a thought.
There was a problem hiding this comment.
Yes, but see minor suggestions there.
| if serializer.is_valid(): | ||
| old_person_id = serializer.validated_data["old_person_id"] | ||
| new_person_id = serializer.validated_data["new_person_id"] | ||
| DatatrackerPerson.objects.filter(datatracker_id=old_person_id).update( |
There was a problem hiding this comment.
I think this step should update any references to DatatrackerPerson with old_person_id to DatatrackerPerson with new_person_id.
May be delete the old record afterwards?
(@jennifer-richards)
There was a problem hiding this comment.
As we discussed separately, I am hoping we can avoid having to rewrite a lot of records. If we can let things work with multiple DatatrackerPerson instances holding the same datatracker_id (i.e., removing the current unique constraint plus some additional work), a merge may not need to be much more complicated than what you suggest here.
| @wraps(f) | ||
| def wrapped(request, *args, **kwargs): | ||
| if not isinstance(request, Request): | ||
| if not isinstance(request, Request) and isinstance(request, APIView): |
There was a problem hiding this comment.
Good idea, though I think isinstance(request, django.views.generic.View) is the right class to use. (APIView is a subclass and gets its request-handling properties through it.)
Maybe you could go full pythonic and just do ... and hasattr(request, "request")? 🦆
# Conflicts: # rpc/api.py
|
When you have a chance, it'd be good to update this now that #357 has been merged. |
2 participants
No description provided.