-
Notifications
You must be signed in to change notification settings - Fork 0
Add import feature #57
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
base: main
Are you sure you want to change the base?
Conversation
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.
issue (non-blocking): updating dependencies can introduce bugs into the code base, especially when incrementing major versions (looks like we had to make some changes in our pagination). If this does end up introducing a bug into the codebase, there is no way to roll back without also undoing your import/export changes.
suggestion: update dependencies in a separate PR
| // Import button | ||
| FilledButton.icon( | ||
| onPressed: | ||
| _urlController.text.trim().isNotEmpty ? handleImport : null, |
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.
issue (blocking): Import button doesn't work properly
The urlController doesn't trigger a rebuild of the widget when it is updated, so this button doesn't get 'enabled' correctly.
suggestion: use the textfield onChanged to set a local stateful variable, or add a listener to the urlController to trigger a rebuild.
adding an empty setState listener to the urlController (as below) would trigger the rebuild, and it's less invasive than refactoring the whole widget to get rid of the urlController and replace it with a stateful variable. One could argue that this is a bit hacky, as we are then causing the entire page to rebuild instead of just that button - but for a page like this I don't see that being problematic.
@override
void initState() {
super.initState();
_urlController.addListener(_onUrlChanged);
}
void _onUrlChanged() {
setState(() {});
}
@override
void dispose() {
_urlController.removeListener(_onUrlChanged);
_urlController.dispose();
_urlFocusNode.dispose();
super.dispose();
}|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return Scaffold( |
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.
nit: scaffold isn't needed here afaik
| "type": "text", | ||
| "placeholders": {} | ||
| }, | ||
| "ndjsonFileToImport": "NDJSON file to import", |
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.
suggestion (non-blocking): update this text and description to reference URL rather than file
This makes it clearer that we are performing an import from another hosted fhir server, rather than a NDJSON file that the user might have locally saved.
| } | ||
|
|
||
| // Make the import request using the underlying Dio client | ||
| final response = await fhirClient.dio.post( |
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.
question: it looks like we are only authenticating against the server we are importing into, but not against the server that we are importing from. How involved would it be to add some type of auth here?
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.
Since in this case we submit a URL to the server, the server needs to be able to access the file stored at the specified URL. Microsoft FHIR requires a public URL for this (no joke).
I'm using signed URLs that time out after 1h or so, that way the exposure is very limited. But it's definitely not fun if you have a large patient database that you need to import.
| ); | ||
|
|
||
| if (fhirClient == null) { | ||
| // TODO: Show error message |
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.
todo: did you mean to leave this todo here?
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 mean to implement a dialog here, ended up deciding against it because it's a highly unlikely internal state problem and then forgot to remove the comment. Will fix.
|
|
||
| class _ResourcePaginatedListState extends State<ResourcePaginatedList> { | ||
| final pagingController = PagingController<int, Resource>(firstPageKey: 0); | ||
| // final pagingController = PagingController<int, Resource>(firstPageKey: 0); |
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.
todo: remove commented out code
This feature adds bulk import of resources to Fire Scribe. So far this is poorly tested but want to merge the feature and fix in subsequent PRs in case any bugs exist.
Note that even though the screen is named "ImportExportPage", I removed the export feature because it is very complicated and proprietary with Microsoft. I plan to re-add it later on, so for now I just changed the strings to avoid showing "export" anywhere.
This PR includes basic chores to update packages, so has unrelated code changes because of breaking changes in packages.
Most notable change is go_router which requires mixins to be generated, requiring relocation of the route declaration into the central
routes.dartfile.