-
Notifications
You must be signed in to change notification settings - Fork 66
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
OHRI-1839 Support for rendering problem by concept class #125
Conversation
export class ClassArrayDataSource extends BaseOpenMRSDataSource { | ||
constructor() { | ||
super('/ws/rest/v1/concept?v=custom:(uuid,display,conceptClass:(uuid,display))'); | ||
} | ||
|
||
fetchData(searchTerm: string, config?: Record<string, any>): Promise<any[]> { | ||
return openmrsFetch(searchTerm ? `${this.url}&q=${searchTerm}` : this.url).then(({ data }) => { | ||
if(config?.class){ | ||
return data.results.filter(concept => concept.conceptClass && config.class.includes(concept.conceptClass.uuid)) | ||
} | ||
return data.results; | ||
}); | ||
} | ||
} |
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.
@kajambiya how is this data source different from the concept data source?
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.
So I wanted concept datasource to be used for fetching either all concepts or concepts by class if one specified the class. For example, for drug, we pass the drug class in the url when calling the api and fetch all concepts of class drug.
Class array datasource (naming could be improved) on the other hand was to fetch concepts from more than one class. Since we can't pass an array of classes to the url, the logic is to first fetch all concepts and then filter out those that belong to the specified classes. That's why I separated the 2 so that each can handle the certain type of use cases.
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.
@kajambiya if the only difference is that the concept datasource
only fetches based on one class and this one fetches based on an array of classes I believe the concept datasource
can be adjusted for that. You can make it by default accept an array and it should work if it is one or more classes. Makes sense?
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.
Had preferred to separate concerns so that there are as minimal conditions as possible but let me make the update and you take a look
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 agree with @ebambo on this. We can simply make the concept ds more configurable. It can accept an array of concept classes. If the length of the concept classes is > 1, we apply a filter to the search results. If it's simply one we apply the concept class as a query parameter.
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.
@eudson @samuelmale
I've refactored the code accordingly. Please review and revert.
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.
LGTM
Requirements
Summary
Using the ui-select-extended control to render concepts of class Diagnosis and Symptom.
Screenshots
Related Issue
Other