Conversation
There was a problem hiding this comment.
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.
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.
nit: scaffold isn't needed here afaik
| "type": "text", | ||
| "placeholders": {} | ||
| }, | ||
| "ndjsonFileToImport": "NDJSON file to import", |
There was a problem hiding this comment.
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.
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.
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.
todo: did you mean to leave this todo here?
There was a problem hiding this comment.
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.
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.