FHIR terminology import #2610
FHIR terminology import #2610
Conversation
|
Just following up on this, has anybody had a chance to take a look? |
|
@anthonysena, @alex-odysseus, @chrisknoll - Who is responsible for reviewing this PR? |
|
@ablack3 thanks for tagging me here. @johngrimes very sorry for the long delay here. I will take on the code review. @johngrimes - let me make sure I understand the use case you are trying to address. You have a value set in FHIR and you'd like to import that as a concept set in ATLAS. Then you mention in the limitations:
I don't have access to the chat link you referenced and given the age of this PR, I'm unsure of the status of the mapping of FHIR to OMOP vocabularies. What happens if/when a value set is imported and no source codes are found in the vocabulary? There was some work done in #2653 that may be of interest to allow for importing of codes and then selecting of the vocabulary. After quickly reviewing the code I had a few thoughts:
|
| @@ -3,7 +3,7 @@ define(function(require, exports) { | |||
| const config = require('appConfig'); | |||
| const sharedState = require('atlas-state'); | |||
| const { Api:OHDSIApi, STATUS } = require('ohdsi-api'); | |||
| const JSON_RESPONSE_TYPE = 'application/json'; | |||
There was a problem hiding this comment.
This looks like it replaced JSON_RESPONSE_TYPE with FHIR_JSON_RESPONSE_TYPE, and I don't think you should remove JSON_RESPONSE_TYPE...can you confirm?
There was a problem hiding this comment.
I was also curious about this change and it looks like JSON_RESPONSE_TYPE is already defined in the base class here: https://github.com/OHDSI/UiToolbox/blob/master/src/api/index.js#L15. So I think it is safe to remove.
| @@ -100,6 +100,18 @@ define(function(require, exports) { | |||
| } | |||
| } | |||
|
|
|||
| class FhirApi extends Api { | |||
There was a problem hiding this comment.
I don't think FhirApi should be a nested class in http.js. The http.js is a module which facilitates http communication. I think the FHIR class handles communicating to a FHIR service. We have a separate folder of services which you can implement your class there, and then you can import the class in the javascript compoennt you want to use (via the define())
There was a problem hiding this comment.
FYI: within this file we do define 2 classes: Api and PlainTextApi. I agree with moving this to another file but noting that we may want to do that for the PlainTextApi as well.
There was a problem hiding this comment.
Apologies, my understanding about the http.js module was that it just wrapped some lower-level HTTP calls to handle passing specific headers, but not something that made some kind of 'client api' to a specific type of service (like the FHIR one or the PlainText one)...I'm not sure what the design decision was about introducing the PlainText api into http.js but it seems that whoever put that forward thought that you would put these type of API clients directly into the http.js module...so, I can't argue with that, so my only comment then is how the JSON_RESPONSE_TYPE was removed....although not sure where that is being used (but possibly it's a constant that is refernced elsewhere in the app).
|
@anthonysena Thanks very much for your time and detailed response.
The change looks great and really improves the import source codes feature. Once we have a solid vocabulary identifier mapping, I can imagine that you could enhance this implementation to remove the intermediate step in the user interface for FHIR source codes, as they are self-describing. Unfortunately I'm not sure that the work on the vocabulary mapping is complete, or I don't have visibility of it.
It would be nice if there was a functional default for this, maybe "https://tx.fhir.org/r4" as the official reference server for the current version of the spec?
Is that a different component, or another instantiation of the same component? If there is a different one, could you point me towards where the source for that lives?
I think that this change adds significant benefit even without this feature, as it allows users to interact with well-known terminologies that are available on public reference servers (e.g. tx.fhir.org, tx.ontoserver.csiro.au). I'd be keen to get your thoughts on what you think would be involved in adding authentication capability. I am not as familliar with the code base and any existing infrastructure or features of the framework that could be used for this purpose. |
I'm pretty sure there's one component being reused, similar how you defined your |
|
@johngrimes - just reviewing the upcoming v2.13 release and we are going to move this item to v2.14 since we were unsure if you still wanted to contribute this to the codebase? |
|
I definitely still want to get it in, I just haven't had a chance to address the comments yet 😔 |
|
There's probably not too much work remaining. |
|
No worries @johngrimes - we'll put it into v2.14 and we can check in with you next year. Thanks! |
This change adds a tab to the Import section of the Concept Sets area.
A user can enter:
A request will be made to the ValueSet expand operation on that server, and the resulting codes will be imported.
Limitations