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

Add Admin Level to each locations payload #177

Merged
merged 4 commits into from
Mar 27, 2024
Merged

Conversation

TraciebelWairimu
Copy link
Contributor

IMPORTANT: Where possible all PRs must be linked to a Github issue

Fixes #172

Engineer Checklist

  • I have run ./gradlew spotlessApply to check my code follows the project's style guide
  • I have built and run the efsity jar to verify my change fixes the issue and/or does not break the application

name,status,method,id,parentName,parentID,type,typeCode,adminLevel,physicalType,physicalTypeCode,longitude,latitude
City1,active,update,ba787982-b973-4bd5-854e-eacbe161e297,test location-1,18fcbc2e-4240-4a84-a270-7a444523d7b6,site,si,0,ward,wa,36.81,-1.28
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just an example csv, but since these locations have a parent, let's not have them with an admin level that is 0. I think the actual level should be 3, but let's just check the api to confirm and update

{
"system": "https://smartregister.org/codes/administrative-level",
"code": "$adminLevelcode",
"display": "$adminLevelcode"
Copy link
Contributor

Choose a reason for hiding this comment

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

This display needs to have the string "Level" for it to show up as "display": "Level 3"

importer/main.py Outdated
@@ -302,6 +303,24 @@ def location_extras(resource, payload_string):
del obj["resource"]["type"]
payload_string = json.dumps(obj, indent=4)

try:
if len(adminLevel) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

if the csv has no value and is assigned a "0" in the try/except above, then the assignment here will always be a 0
lets have it get into the else to check the parent instead

importer/main.py Outdated
admin_level = parent_admin_level + 1
obj = json.loads(payload_string)
obj["resource"]["type"][1]["adminLevel"][0]["code"] = admin_level
obj["resource"]["type"][1]["adminLevel"][0]["display"] = admin_level
Copy link
Contributor

Choose a reason for hiding this comment

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

should have the string "Level" not just the number

importer/main.py Outdated
obj["resource"]["type"][1]["adminLevel"][0]["display"] = admin_level
payload_string = json.dumps(response_obj, indent=4)
except KeyError:
logging.info("This is a root location, admin level will be 0")
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to add an extra check here, because there are some cases where the admin levels has just not been added to the json, so it's not necessarily that this is the root

pld
pld previously approved these changes Mar 19, 2024
"code": "$adminLevelCode",
"display": "Level $adminLevelCode"
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: can we tab this bracket?

importer/main.py Outdated
def get_coding_object(array, system):
for index, value in enumerate(array):
system1 = value["coding"][0]["system"]
if system in system1:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we name these better so it is clearer?

@@ -235,7 +293,7 @@ def test_build_payload_locations(self, mock_get_resource):
"type": "object",
"properties": {
"resourceType": {"const": "Location"},
"id": {"const": "c4336f73-4450-566b-b381-d07a6e857d72"},
"id": {"const": "ba787982-b973-4bd5-854e-eacbe161e297"},
Copy link
Contributor

Choose a reason for hiding this comment

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

can we look into why this is changing?
The generated id should not get replaced by the mock data

importer/main.py Outdated
response_type = obj["type"]
current_system = "administrative-level"
index = identify_coding_object_index(response_type, current_system)
if index >= 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we handle the case where the system is not in the list and "none" is returned?
NoneTye cannot be compared or ">=" to 0

importer/main.py Outdated
payload_string = payload_string.replace("$adminLevelCode", admin_level)
else:
obj = json.loads(payload_string)
del obj["resource"]["type"]
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to add an index to this?

@Wambere Wambere merged commit 39156df into main Mar 27, 2024
4 checks passed
@Wambere Wambere deleted the 172-Add-Admin-level branch March 27, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an Administrative level for each Location
3 participants