-
Notifications
You must be signed in to change notification settings - Fork 2
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
129 export resource uuids #148
Conversation
1ba56be
to
610a82a
Compare
importer/main.py
Outdated
@click.option("--csv_file", required=True) | ||
@click.option("--csv_file", required=False) | ||
@click.option("--export_resources", required=False) | ||
@click.option("--parameter", required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add default values for parameter, value and batch_size so that the export works even when these are not passed
importer/main.py
Outdated
def write_csv(data, resource_type, fieldnames): | ||
logging.info("Writing to csv file") | ||
current_time = datetime.now().strftime("%Y-%m-%d-%H-%M") | ||
csv_file = f"csv/{current_time}-export_{resource_type}.csv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's maybe create a folder inside /csv called exports just so this is cleaner and easier to find
importer/main.py
Outdated
@click.option("--export_resources", required=False) | ||
@click.option("--parameter", required=False) | ||
@click.option("--value", required=False) | ||
@click.option("--batch_size", required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this batch_size needs to be changed to limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I label it as limit
and not batch_size
?
importer/main.py
Outdated
@@ -17,6 +17,7 @@ | |||
|
|||
global_access_token = "" | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this, and run black on these files
importer/main.py
Outdated
elements = ["name", "status", "id", "identifier", " organizations", "participants"] | ||
else: | ||
elements = [] | ||
for x in resources["entry"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to handle a case where the response returned has 0 resources and thus does not have "entry"
importer/main.py
Outdated
elif resource_type == "Organization": | ||
elements = ["name", "active", "id", "identifier", "alias"] | ||
elif resource_type == "CareTeam": | ||
elements = ["name", "status", "id", "identifier", " organizations", "participants"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like we are exporting the organizations and participants here, are we?
importer/main.py
Outdated
|
||
# This function exports resources from the API to a csv file | ||
def export_resources_to_csv(resource_type, parameter, value, batch_size): | ||
resource_type = get_valid_resource_type(resource_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's change this (include in the doc ) to require the resource type exactly as it would be/work in a url so we don't have to do this. Might be a it confusing
importer/main.py
Outdated
@@ -974,5 +1052,6 @@ def main( | |||
total_time = end_time - start_time | |||
logging.info("Total time: " + str(total_time.total_seconds()) + " seconds") | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I black format the files the spaces are added back
26596a8
to
74d6f73
Compare
74d6f73
to
9075ede
Compare
8a5daf1
to
63fd41a
Compare
importer/README.md
Outdated
- `export_resources` can either be True or False, checks if it is True and exports the resources | ||
- The `parameter` is used as a filter for the resources. The set default parameter is "_lastUpdated", other examples include, "name" | ||
- The `value` is where you pass the actual parameter value to filter the resources. The set default value is "gt2023-01-01", other examples include, "Good Health Clinic 1" | ||
- The `limit` is the number of resources exported at a time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add the default here as well
importer/main.py
Outdated
if response[1] == 200: | ||
resources = json.loads(response[0]) | ||
data = [] | ||
if "entry" in resources: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we check this with resources["entry"]
instead ?
importer/main.py
Outdated
rl.append(value) | ||
data.append(rl) | ||
write_csv(data, resource_type, elements) | ||
logging.info("Successfully written to csv") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing here checking that this was actually successful
I tried exporting with an invalid date time, python3 main.py --export_resources True --parameter _lastUpdated --value gt2023-02-29 --limit 20 --resource_type Location --log_level info
I got an error and no export but this still printed successful
importer/test_main.py
Outdated
self.test_fieldnames = ["name", "active", "method", "id", "identifier", "alias"] | ||
write_csv(self.test_data, self.test_resource_type, self.test_fieldnames) | ||
self.assertIsInstance(self.test_data, list) | ||
self.assertEqual(len(self.test_data), 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these assert statements are not testing the result of your function call
importer/test_main.py
Outdated
self.assertEqual(len(self.test_data), 2) | ||
current_time = datetime.now().strftime("%Y-%m-%d-%H-%M") | ||
expected_csv_file_path = ( | ||
f"csv/exports/{current_time}-export_{self.test_resource_type}.csv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible that this current_time gotten in the test is different from the current_time gotten when the function was called
IMPORTANT: Where possible all PRs must be linked to a Github issue
Fixes #129
Engineer Checklist
./gradlew spotlessApply
to check my code follows the project's style guide