fix: get direct url pixel data should be optional for video #4152
Conversation
get direct url pixel data should be optional for video /rendered urls
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Passing run #3978 ↗︎
Details:
Review all test suite changes for PR #4152 ↗︎ |
|||||||||||||||
| @@ -29,31 +29,31 @@ const getDirectURL = (config, params) => { | |||
| } | |||
|
|
|||
| const value = instance[tag]; | |||
| if (!value) { | |||
There was a problem hiding this comment.
I don't understand what value there is in increasing the indent level. The recommended approach for handling things is to return quickly at the top of the function, rather than nesting a function inside, as was done originally.
There was a problem hiding this comment.
Ok, I see what you are doing, github made it non obvious that there was more code - cut things off below. What I think should be done is to separate this out into two functions, one of which constructs a URL, the other of which uses the value - and export both of them so that they can be used independently, or combined. Then getDirectURL becomes:
const value = instance[tag];
return getBulkdataValue(value) || createRenderedRetrieve(instance)
Then, you can leave the return immediate as is inside getBulkdataValue.
Also, you might add a note that it is incorrect to not have a value here, or rather it means that the value itself is supposed to be empty, but that some DICOMweb systems incorrectly return empty tag values.
Then, we should handle both:
TAG: null
and
TAG: {}
I'd like to see a unit test to check a couple of the various cases, since I think this logic is getting complex enough to warrant that.
Sorry for the extra work, but by the time things get this complex, splitting it up makes it easier to figure out what one should expect to receive back.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4152 +/- ##
=======================================
Coverage 45.76% 45.76%
=======================================
Files 77 77
Lines 1274 1274
Branches 315 315
=======================================
Hits 583 583
Misses 550 550
Partials 141 141 ☔ View full report in Codecov by Sentry. |
|
Can you update to head and check that the tests pass before committing please? Otherwise I like how you've added the tests and separated out the functionality to make it clearly understandable. |
sedghi
changed the title
Refactor getDirectURL into three documented and tested functions. Also, this change allows building /rendered URLs for video instances that does not have pixel data present.
Context
The get direct URL was a multi-purpose util function. This PR splits it into three independent functions that get called within to get the direct url.
Changes & Results
Now getDirectURL calls getBulkDataValue and createRenderedRetrieve as fallbacks.
Testing
(!singlepart || (singlepart !== true && singlepart.indexOf(fetchPart) === -1)
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
N/A
Tested Environment