-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[OHI-1357] feat(static-wado): add support for case-insensitive searching #4603
[OHI-1357] feat(static-wado): add support for case-insensitive searching #4603
Conversation
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ohif-dev canceled.
|
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.
we should make it per qido call and not global
Viewers Run #4780
Run Properties:
|
Project |
Viewers
|
Branch Review |
feat/case-insensitive-searching-for-static-wado
|
Run status |
Passed #4780
|
Run duration | 02m 10s |
Commit |
31c006b091: fixes
|
Committer | Ibrahim |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
2
|
Skipped |
0
|
Passing |
44
|
View all changes introduced in this branch ↗︎ |
Can you try it in Orthanc too please? |
I did, it works fine |
@wayfarer3130 can you take a look at this please |
extensions/default/src/DicomWebDataSource/utils/StaticWadoClient.ts
Outdated
Show resolved
Hide resolved
extensions/default/src/DicomWebDataSource/utils/StaticWadoClient.ts
Outdated
Show resolved
Hide resolved
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.
Just a couple of small changes.
Also, please change the default.js and e2e.js configurations to set this on by default - you may need to modify an integration test to support that. |
thanks Bill, I'll do them as soon as I can |
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.
wait a minute does this only work on static wado backends? what happened to orthanc testing? did you do it?
orthanc case sensitivity is server side, OHIF doesn’t control it. I tested what orthanc does server side and that this doesn’t break or interfere with it |
I thought the filtering happens client side when we looked at the code |
It only happens if it's a static wado datasource, otherwise it calls the super method, we don't control or filter anything there, so it's all up to backend (orthanc in this case) what to return See here Viewers/extensions/default/src/DicomWebDataSource/utils/StaticWadoClient.ts Lines 101 to 123 in a863808
async searchForStudies(options) {
if (!this.staticWado) {
return super.searchForStudies(options);
}
const searchResult = await super.searchForStudies(options);
const { queryParams } = options;
if (!queryParams) {
return searchResult;
}
const lowerParams = this.toLowerParams(queryParams);
const filtered = searchResult.filter(study => {
for (const key of Object.keys(StaticWadoClient.studyFilterKeys)) {
if (!this.filterItem(key, lowerParams, study, StaticWadoClient.studyFilterKeys)) {
return false;
}
}
return true;
});
return filtered;
} |
I'm hesitant to merge this PR if it's only for static WADO. Sorry if that wasn't clear from the start. Based on my research, the standard doesn't specify anything about this. As you mentioned, Orthanc has a setting called Then we can decide how to proceed. |
I think the way forward is to add Right now it is set in the config file as From the link |
I implemented it according to the definition from GCP. please take another look now, thanks |
extensions/default/src/DicomWebDataSource/utils/StaticWadoClient.ts
Fixed
Show resolved
Hide resolved
extensions/default/src/DicomWebDataSource/utils/StaticWadoClient.ts
Outdated
Show resolved
Hide resolved
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.
See Bill's comment and it is good to go thanks
extensions/default/src/DicomWebDataSource/utils/StaticWadoClient.ts
Outdated
Show resolved
Hide resolved
|
||
if (fuzzyMatching && typeof actual === 'string' && typeof desired === 'string') { | ||
const normalizeValue = str => { | ||
if (str.startsWith('*')) { |
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.
If you supported wildcard matching here too, with prefix/suffix, then you can support fuzzy matching for names, and wildcard start/ends with for other fields. I would suggest changing the support of supportsWildcard to suffix
, to automatically have the wildcard added only at the end, although doing a full contains works as well for client side matching.
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 think this implementation is sufficient for now. We can dedicate more time to implementing proper server-side filtering logic for static wado using lamdas or something similar then we can avoid all of this in the frontend.
Context
This adds the option to filter static-wado results without being case-sensitive. You need to set the
caseSensitive
property on your static wadodataSource
tofalse
, the default istrue
.Orthanc and other sources support case insensitive searching already.
Fixes OHI-1357
Close #4522