Conversation
dborowiecki
left a comment
dborowiecki
left a comment
There was a problem hiding this comment.
I left some general comments but I think that we need to put more focus in designing general use mapper as well as it's specific variation for the digital registries bb. it should make our work easier in future as well as avoid code repetition in the future.
| default_auto_field = 'django.db.models.BigAutoField' | ||
| name = 'govstack_test_harness_api' | ||
|
|
||
| IM_CLIENT = 'eGovStack/GOV/90000009/digitalregistries' |
There was a problem hiding this comment.
Ideally this shouldn't be hardcoded in apps.py, but rather pulled from env. Leaviing this piece of code seems to be a security risk.
|
|
||
| IM_CLIENT = 'eGovStack/GOV/90000009/digitalregistries' | ||
|
|
||
| digital_registry = { |
There was a problem hiding this comment.
Yo can consider adding some schema for the BB definition (e.g. as Pydantic dataclass). Different BBs could have different requirements. For the digital registries it's valid to have registryname and versionnumber but for other it could be different.
| "registryname": { | ||
| "111": { | ||
| 'ID': 'chfId', | ||
| 'FirstName': 'otherNames', | ||
| 'LastName': 'lastName', | ||
| 'BirthCertificateID': 'json_ext' | ||
| } | ||
| } |
There was a problem hiding this comment.
As for the mapping - have you considered classes that would be loaded instead of JSON definition? Classes could be determined through config (e.g. mapper class "govvstack_test_harness_api.mappers.DigitalRegistry") could be added as a config and then this actual piece of code would be loaded. It does make sense as plenty of entries will not be simple key-value and should be determined throguh some logic (as in the Post Partum we have mother-child-father relation, adressed in IMIS through the family relations with the head of family).
| from ..services.record_exists_service import RecordExistsService | ||
| from ..services.services import get_query_content_values, login_with_env_variables |
| def __init__(self, jwt_token, context, validated_data): | ||
| self.context = context | ||
| self.validated_data = validated_data | ||
| # self.headers = {"Authorization": "Bearer " + jwt_token} | ||
| self.client = GrapheneClient() |
There was a problem hiding this comment.
I don't see jwt_token argument to be used anywhere. Also jwt_token is a specific attribute that wouldn't be used inside of application oftern (it is generated/stored/used mostly during the in the django auth, and shouldn't be mixed in business logic.
| def get_update_fields(write_values) -> str: | ||
| field_mapping = { | ||
| "LastName": "lastName", | ||
| "FirstName": "otherNames" | ||
| } | ||
| return "".join(f'{field_mapping[key]}: "{value}" ' | ||
| for key, value in write_values.items() | ||
| if value and key in field_mapping) |
There was a problem hiding this comment.
This function also seems to be quite specific for the insuree and I don't fully understand what's it's purpose. Can you add some inline explanation and also wrap everything related to the digital-registry fetching under single class (or module if class would be hundred lines long).
| if value and key in field_mapping) | ||
|
|
||
|
|
||
| def login_with_env_variables(request): |
There was a problem hiding this comment.
I think that this functionality should be under graphql client.
| from ..serializers.record_exists_serializer import RecordExistsSerializer | ||
| from drf_yasg.utils import swagger_auto_schema | ||
| from .swaggers_schema import exists_request_body, exists_response_body | ||
| from ..swaggers_schema import info_mediator_client | ||
| from ..services.services import login_with_env_variables | ||
| from ..controllers.record_exists_controller import check_if_record_exists |
There was a problem hiding this comment.
Please replace relative imports with absolute ones.
If you're using PyCharm IDE it can be set as an default in settings.
| ) | ||
| return JsonResponse(response_data) | ||
| else: | ||
| return JsonResponse(serializer.errors, status=400) |
There was a problem hiding this comment.
You can consider raising request execptions in the validator. It will give more flexibility 400 is quite general and perhaps we could be more specific with 401, 403, 404 and so on.
|
|
||
|
|
||
| urlpatterns = [ | ||
| path('data/<str:registryname>/<str:versionnumber>/exists', check_if_record_exists_in_registry) |
There was a problem hiding this comment.
can you also add name to the url? (django doc)
|
SonarCloud Quality Gate failed.
|
2 participants
This is draft branch that will be deleted. It was created to show flow for implementing one endpoint.
Description: