-
Notifications
You must be signed in to change notification settings - Fork 139
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
FHIR terminology import #2610
base: master
Are you sure you want to change the base?
FHIR terminology import #2610
Changes from all commits
b99b3b5
9fe7b3c
4b5fe4c
1b07a3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<div> | ||
<div class="heading"> | ||
<span data-bind="text: ko.i18n('components.conceptSet.fhirHeading', 'Import a value set from a FHIR terminology server')"></span> | ||
</div> | ||
<label> | ||
<span data-bind="text: ko.i18n('components.conceptSet.fhirServer', 'FHIR server URL')"></span> | ||
<input class="form-control" type="text" name="fhirServer" data-bind="textInput: fhirServer"/> | ||
</label> | ||
<label> | ||
<span data-bind="text: ko.i18n('components.conceptSet.valueSet', 'Value set URI')"></span> | ||
<input class="form-control" type="text" name="valueSet" data-bind="textInput: valueSet"/> | ||
</label> | ||
<concept-add-box params="{ | ||
isActive: canAddConcepts, | ||
onSubmit: doImport, | ||
}"></concept-add-box> | ||
<div class="clear"></div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
define([ | ||
'knockout', | ||
'text!./fhir.html', | ||
'components/Component', | ||
'./ImportComponent', | ||
'utils/AutoBind', | ||
'utils/Clipboard', | ||
'utils/CommonUtils', | ||
'services/VocabularyProvider', | ||
'services/http', | ||
'appConfig', | ||
], function( | ||
ko, | ||
view, | ||
Component, | ||
ImportComponent, | ||
AutoBind, | ||
Clipboard, | ||
commonUtils, | ||
vocabularyApi, | ||
httpService, | ||
config | ||
){ | ||
|
||
class FhirImport extends AutoBind(ImportComponent(Component)) { | ||
constructor(params) { | ||
super(params); | ||
this.fhirServer = ko.observable(config.fhirTerminologyUrl); | ||
this.valueSet = ko.observable(""); | ||
this.appendConcepts = params.appendConcepts; | ||
this.canAddConcepts = ko.pureComputed(() => | ||
this.fhirServer().length > 0 && | ||
this.valueSet().length > 0 && | ||
params.canEdit() | ||
); | ||
this.doImport = this.doImport.bind(this); | ||
} | ||
|
||
async runImport(options) { | ||
const expandUrl = `${this.fhirServer()}/ValueSet/$expand`, | ||
expandParams = { | ||
url: this.valueSet() | ||
}, result = await httpService.fhirService.doGet(expandUrl, expandParams), | ||
codes = result.data.expansion.contains.map(c => c.code), | ||
{data} = await vocabularyApi.getConceptsByCode(codes); | ||
this.appendConcepts(data, options); | ||
} | ||
} | ||
|
||
return commonUtils.build('conceptset-list-import-fhir', FhirImport, view); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
const FHIR_JSON_RESPONSE_TYPE = 'application/fhir+json'; | ||
const TEXT_RESPONSE_TYPE = 'text/plain'; | ||
const EventBus = require('services/EventBus'); | ||
|
||
|
@@ -100,6 +100,18 @@ define(function(require, exports) { | |
} | ||
} | ||
|
||
class FhirApi extends Api { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
getHeaders(requestUrl) { | ||
return { | ||
'Accept': FHIR_JSON_RESPONSE_TYPE, | ||
}; | ||
} | ||
|
||
parseResponse(json) { | ||
return JSON.parse(json); | ||
} | ||
} | ||
|
||
const singletonApi = new Api(); | ||
singletonApi.setAuthTokenHeader('Authorization'); | ||
|
||
|
@@ -108,7 +120,10 @@ define(function(require, exports) { | |
plainTextService.setUnauthorizedHandler(() => singletonApi.handleUnauthorized()); | ||
plainTextService.setUserTokenGetter(() => singletonApi.getUserToken()); | ||
|
||
const fhirService = new FhirApi(); | ||
|
||
singletonApi.plainTextService = plainTextService; | ||
singletonApi.fhirService = fhirService; | ||
|
||
return singletonApi; | ||
}); |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.