Update the Location Lineage addition, Update Roles, Error handling - #343
Update the Location Lineage addition, Update Roles, Error handling#343dubdabasoduba wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates error handling, adds location lineage functionality, and improves chunking for bulk operations. The changes focus on making the importer more robust by handling API failures gracefully, processing resources in smaller chunks to avoid memory issues, and adding location hierarchy tracking.
- Enhanced error handling with null response checks and improved logging
- Added location lineage tagging to track hierarchical relationships
- Implemented chunking for bulk operations to process data in manageable batches
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| importer/main.py | Refactored resource processing to use chunking and added comprehensive error handling |
| importer/importer/users.py | Added robust error handling for user creation and group assignment operations |
| importer/importer/services/fhir_keycloak_api.py | Improved OAuth token refresh logic with retry mechanisms |
| importer/importer/request.py | Enhanced request logging for debugging |
| importer/importer/builder.py | Added location lineage functionality and improved JSON error handling |
| importer/csv/setup/roles.csv | Added new permissions for ValueSet and CodeSystem resources |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| obj = {"task": "Get existing user", "row": str(user), "error": error_msg} | ||
| return 0, obj | ||
|
|
||
| response_text, status_code = response |
There was a problem hiding this comment.
The handle_request function returns a Response object for non-GET requests, but this code expects a tuple. This will cause an AttributeError when trying to unpack the response.
| response_text, status_code = response | |
| # Support both tuple and Response object return types from handle_request | |
| if isinstance(response, tuple): | |
| response_text, status_code = response | |
| else: | |
| response_text = response.text | |
| status_code = response.status_code |
| x = org.split(":") | ||
| y["reference"] = "Organization/" + str(x[0]) | ||
| y["display"] = str(x[1]) | ||
| if len(x) > 1: |
There was a problem hiding this comment.
[nitpick] Consider extracting the pattern of splitting and conditionally adding display names into a helper function since it's repeated multiple times in this file (lines 404, 423, 440).
| elif len(value.strip()) < 1: | ||
| resource[index] = None | ||
| else: | ||
| resource[index] = value.strip() |
There was a problem hiding this comment.
Calling .strip() on a None value will raise an AttributeError. The None check should handle this case before attempting string operations.
| elif len(value.strip()) < 1: | |
| resource[index] = None | |
| else: | |
| resource[index] = value.strip() | |
| elif isinstance(value, str): | |
| if len(value.strip()) < 1: | |
| resource[index] = None | |
| else: | |
| resource[index] = value.strip() | |
| else: | |
| resource[index] = None |
| # Initialize variables | ||
| version = "1" | ||
| unique_uuid = None | ||
| identifier_uuid = None | ||
|
|
There was a problem hiding this comment.
[nitpick] These variables are initialized here but may be overwritten in the conditional blocks below. Consider moving the initialization closer to where they're used or removing redundant assignments.
| # Initialize variables | |
| version = "1" | |
| unique_uuid = None | |
| identifier_uuid = None | |
| # Set variables based on method |
IMPORTANT: Where possible all PRs must be linked to a Github issue
Fixes #[issue number] or Closes #[issue number]
Engineer Checklist
./gradlew spotlessApplyto check my code follows the project's style guide./gradlew clean assembleand run the efsity jar to verify my change fixes the issue and does not break the application