Skip to content
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

Lruzicki/draft #7

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
*.pyc
**/venv
**/.env
**/.idea
**/__pycache__
build
dist
*.egg-info
**/.vscode
**/*.mo
locale/__init__.py
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
18 changes: 18 additions & 0 deletions govstack_test_harness_api/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,21 @@
class TestHarnessApiConfig(AppConfig):
default_auto_field = 'django.db.models.BigAutoField'
name = 'govstack_test_harness_api'

IM_CLIENT = 'eGovStack/GOV/90000009/digitalregistries'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


digital_registry = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"birth_registry": {
"1": {
'ID': 'ID'
}
},
"registryname": {
"111": {
'ID': 'chfId',
'FirstName': 'otherNames',
'LastName': 'lastName',
'BirthCertificateID': 'json_ext'
}
}
Comment on lines +16 to +23

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from ..services.record_exists_service import RecordExistsService
from ..services.services import get_query_content_values, login_with_env_variables
Comment on lines +1 to +2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change imports to absolute



def check_if_record_exists(data, jwt, context):
content_values = get_query_content_values(data['query']['content'], data['registryname'], data['versionnumber'])
service = RecordExistsService(
jwt,
context=context,
validated_data=data,
)
return service.execute()
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
def get_record_exists_query(registry_name, version_number, uuid, field):
query = f"""
{{
recordExists(registryName: "{registry_name}", versionNumber: "{version_number}", uuid: "{uuid}", field: "{field}") {{
edges {{
node {{
id
{field}
}}
}}
}}
}}
"""
return query


def get_update_registry_query(uuid="", chf_id="", update_fields="") -> str:
query = f'''
mutation {{
updateInsuree(
input: {{
clientMutationId: "552f8e55-ed5a-4e1e-a159-ea8f8cec0560"
clientMutationLabel: "Update insuree"
uuid: "{uuid}"
chfId: "{chf_id}"
{update_fields}
genderId: "F"
head: true
dob: "1974-06-11"
cardIssued:false
familyId: 1
relationshipId: 4
}}
) {{
clientMutationId
internalId
}}
}}
'''
return query


def get_insurees_query(variable_values: str = "", fetched_fields: str = "") -> str:
return f'''
query GetInsurees {{
insurees({variable_values}) {{
edges{{
node{{
{fetched_fields}
}}
}}
}}
}}
'''


def create_insurees_query(variables: dict) -> str:
return f'''
mutation {{
createInsuree(
input: {{
clientMutationLabel: "{variables['clientMutationLabel']}"
chfId: "{variables['chfId']}"
lastName: "{variables['lastName']}"
otherNames: "{variables['otherNames']}"
genderId: "{variables['genderId']}"
dob: "{variables['dob']}"
head: {str(variables['head']).lower()}
cardIssued: {str(variables['cardIssued']).lower()}
jsonExt: "{variables['jsonExt']}"
familyId: 1
}}
) {{
clientMutationId
internalId
}}
}}
'''


def delete_insuree_query(uuid):
return f'''mutation
{{
deleteInsurees(
input: {{
clientMutationId: "c164412c-45a6-4f3f-8a2b-4290739751e2"
clientMutationLabel: "Delete insuree"

uuid: "{uuid}", uuids: ["{uuid}"]
}}
) {{
clientMutationId
internalId
}}
}}
'''
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import graphene
from graphene.test import Client
# from gql import gql, Client
# from gql.transport.requests import RequestsHTTPTransport
from insuree.schema import Query, Mutation
from graphene import Schema


class GrapheneClient:
def __init__(self):
self._client = Client(Schema(query=Query, mutation=Mutation))

def execute_query(self, query, context, variables=None):
print("context: ", context)

if variables is None:
variables = {}
print("variables: ", variables)
return self._client.execute(query, context=context, variables=variables)

def execute_mutation(self, mutation, variables=None):
if variables is None:
variables = {}

return self._client.execute(mutation, variables=variables)


# class GrapheneClient:
# def __init__(self, url, headers=None):
# if headers is None:
# headers = {}
# print(url)
# self._transport = RequestsHTTPTransport(url=url, headers=headers, use_json=True)
# self._client = Client(transport=self._transport, fetch_schema_from_transport=True)
#
# def execute_query(self, query, variables=None):
# if variables is None:
# variables = {}
#
# return self._client.execute(gql(query), variable_values=variables)
#
# def execute_mutation(self, mutation, variables=None):
# if variables is None:
# variables = {}
#
# return self._client.execute(gql(mutation), variable_values=variables)
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from rest_framework import serializers


class RecordExistsSerializer(serializers.Serializer):
query = serializers.JSONField()
registryname = serializers.CharField(max_length=200)
versionnumber = serializers.IntegerField()
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# W pliku record_exists_service.py
from graphene import Schema

from ..gql.graphene_client import GrapheneClient
from ..gql.gql_queries import get_record_exists_query, get_insurees_query
from .services import get_client, get_values_for_insurees, get_search_insurees_arguments
from insuree.schema import Query, Mutation
from django.conf import global_settings


class RecordExistsService:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the name of this class it seems to be very specific for the insuree fetching query. Can we change it in order to be more generic? I'd suggest to do this through composition. You could inject logic from the execute() method using the constructor this would allow making RecordExistsService more SOLID like. You can define separate interface for this executable class using the Prototype

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()
Comment on lines +12 to +16

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 execute(self):
variables = get_values_for_insurees(
self.validated_data['query']['content'],
self.validated_data["registryname"],
self.validated_data["versionnumber"]
)
variable_values = get_search_insurees_arguments(
self.validated_data['query']['content'],
self.validated_data["registryname"],
self.validated_data["versionnumber"]
)
Comment on lines +19 to +28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two function calls appear to be quite similar, but their purpose is not immediately apparent from the code itself. It would be beneficial if the names of the functions were self-explanatory, providing clear hints about their functionality. However, if that is not feasible, I'd recommended to include a comment here to help understanding the underlying process.

query = get_insurees_query(variable_values, "lastName")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should rely on lastName in order to fetch insuree. It's also not unique across the application.

return self.client.execute_query(query, self.context, variables)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change name of this file? You already have services module and nesting services.py file inside of it doesn't look right, please make it more specific, if services added in scope of this file have different uses then you can package them to different files.

Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import os
from types import SimpleNamespace
from django.contrib.auth.models import AnonymousUser
from graphene import Schema
from graphene.test import Client
from unittest import mock
from core.schema import Query as CoreQuery, Mutation as CoreMutation
from django.apps import apps

app_config = apps.get_app_config('govstack_test_harness_api')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just import Config class from this module apps.py

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this approach is recommended when you're doing some imports from other modules, it's because if name of the Config class changes you can still use this import without changing the code. However if it's in scope of single module we should be fine.



def get_client(schema, query, mutation):
return Client(schema=schema(query=query, mutation=mutation))
Comment on lines +13 to +14

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this function? It's creating new Client, and get_ prefix would suggest fetching something that already exists (e.g. if we would follow singleton pattern for client and have only one client for whole module).



def create_base_context():
user = mock.Mock(is_anonymous=False)
user.has_perm = mock.MagicMock(return_value=False)
return SimpleNamespace(user=user)


def get_context(request):
if request.user.is_authenticated:
context = create_base_context()
context.user = request.user
else:
context = SimpleNamespace(user=request.user)
return context

Comment on lines +17 to +30

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? using request.context should be suffucient and there should be no need to use any sort of mocks in code that is part of Test classes


def get_query_content_values(query_content: dict, registry_name: str, version: str) -> dict:
content_values = {}
if not query_content:
return {}
Comment on lines +34 to +35

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why for empty query content empty string is returned? If empty string is a valid input then there's no need to return {}. If it's causing the issues with the further code execution then parthaps raising ValueError would be better handling for this case.


json_mapping = app_config.digital_registry.get(registry_name, {}).get(version, {})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you're explicitly using the digital_registry configuration. However, the name of the function, get_query_content_values, suggests a more generic purpose. It might be better to pass app_config.digital_registry as an input to the function and handle it accordingly. Alternatively, you could pass the entire mapping as an input, since the specific line app_config.digital_registry.get(registry_name, {}).get(version, {}) is only applicable to the structure of the digital registry configuration. Therefore, there's no need to define it inside this function.

Also I don't think that consecutive gets in fetching config is a good solution. I think it'd be better to fail the process if the registry_name or version is not handled rather than return empty data. It could lead developers to think that their unhandled input is valid and there are no entries which is not true. You can raise custom exception in here and in view add general exception handler and return BadRequest status.

for input_key, output_key in json_mapping.items():
content_values[output_key] = query_content.get(input_key, "")
return content_values


def get_values_for_insurees(content_values: dict, registry_name: str, version: str) -> dict:
json_mapping = app_config.digital_registry.get(registry_name, {}).get(version, {})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment applies for other uses of the nested values from digital_registry config. Just use execption. Also if you're using same rather complex get it would be good to wrap it in separate function (could be a property in mapping class). This will result in making changes only in one place instead of across the several functions (in spirit of DRY principle).

mapped_values = {json_mapping[key]: value for key, value in content_values.items() if key in json_mapping}
return {
'clientMutationLabel': f"Create insuree - {mapped_values.get('chfId', '')}",
'chfId': f"{mapped_values.get('chfId', '')}",
'lastName': f"{mapped_values.get('lastName', '')}",
'otherNames': f"{mapped_values.get('otherNames', '')}",
'genderId': mapped_values.get('Gender', 'M'),
'dob': mapped_values.get('BirthDate', '2000-06-20'),
'head': mapped_values.get('Head', True),
'cardIssued': False,
'jsonExt': '{}',
Comment on lines +47 to +55

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be entirely done by Mapper class.

}


def get_search_insurees_arguments(query_content: dict, registry_name: str, version: str) -> str:
insurees_arguments = ""
json_mapping = app_config.digital_registry.get(registry_name, {}).get(str(version), {})
for key, value in json_mapping.items():
if key in query_content:
insurees_arguments += f'{value}: "{query_content[key]}",'

if insurees_arguments.endswith(','):
return insurees_arguments[:-1]
return insurees_arguments
Comment on lines +59 to +68

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Services in this class have quite some content that is based on the dictionaries/jsons/gql templates. If user doesn't know from the begging what is the exact format used data then it's hard to follow the functionality and what given function is doing. For instance return insurees_arguments[:-1] it not obvious (why do we expect to have traling comma?). It'd be good to add docstring and inline comments for those functions.



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)
Comment on lines +71 to +78

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).



def login_with_env_variables(request):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this functionality should be under graphql client.

client = get_client(Schema, CoreQuery, CoreMutation)
username = os.getenv('login_openIMIS')
password = os.getenv('password_openIMIS')

mutation = f'''
mutation {{
tokenAuth(username: "{username}", password: "{password}") {{
token
refreshExpiresIn
}}
}}
'''
context = get_context(request)
client.execute(mutation, context=context)
return context
Loading