Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix the DICOMweb uploader and its tests #381

Merged
merged 58 commits into from
May 1, 2024
Merged

Fix the DICOMweb uploader and its tests #381

merged 58 commits into from
May 1, 2024

Conversation

milanmlft
Copy link
Member

@milanmlft milanmlft commented Apr 29, 2024 •
edited
Loading

Alright, this should hopefully rectify the mess I made in #379...

  • DicomWebUploader member variables fixed with better names, correct credentials and hostnames
  • Tests now actually check if the studies reached the DICOMweb server
  • Additional tests to check validity of credentials

To do

stefpiatek and others added 14 commits April 29, 2024 11:51
@stefpiatek @milanmlft
Rework dicomweb
4d01044
@milanmlft
Update dicomweb tests after rework
ce6d3cf
@milanmlft
Docker compose version is obsolete
d224ea5
@milanmlft
Fix dicomweb-server healthcheck; use correct credentials
19ec2c1
@milanmlft
Fix the DicomWebUploader tests
bdbb1df
@milanmlft
Simplify services setup for core tests
2a91054
@milanmlft
Fix volume path
70182af 
Make the path relative to where the Docker compose file lives, not to
the working directory. So we can run the tests from any aribtrary
directory.
@milanmlft
Clean up orphan containers at startup
2f6ff49 
Gets rid of some warnings
@milanmlft
Fix typo
262ffee
@milanmlft
Add negative checks in case of wrong DICOMweb credentials
be15576
@milanmlft
Remove obsolete function
0e1aa3a
@milanmlft
Fix orthanc credentials
a0c7f50 
These need to use the mapped environment variables inside the Orthanc
container
@milanmlft
Use different credentials for DICOMweb test server and fix volume paths
baf54e9
@milanmlft
Fix system test: actually check for presence of studies on DICOMweb
53fb47f
jeremyestein
jeremyestein reviewed Apr 29, 2024
View reviewed changes
pixl_core/src/core/uploader/_dicomweb.py Outdated Show resolved Hide resolved
pixl_core/src/core/uploader/_dicomweb.py Outdated Show resolved Hide resolved
test/system_test.py Outdated Show resolved Hide resolved
pixl_core/tests/uploader/test_dicomweb.py Outdated Show resolved Hide resolved
pixl_core/tests/uploader/test_dicomweb.py Outdated Show resolved Hide resolved
pixl_core/tests/docker-compose.yml Outdated Show resolved Hide resolved
pixl_core/tests/docker-compose.yml Show resolved Hide resolved
pixl_core/tests/uploader/test_dicomweb.py Outdated Show resolved Hide resolved
pixl_core/tests/uploader/test_dicomweb.py Show resolved Hide resolved
pixl_core/tests/uploader/test_dicomweb.py Outdated Show resolved Hide resolved
stefpiatek
stefpiatek approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for getting into this. some questions here and there and maybe some tidying up of comments but I could also be understanding things incorrectly.

pixl_core/src/core/uploader/_dicomweb.py Outdated Show resolved Hide resolved
pixl_core/tests/conftest.py Outdated Show resolved Hide resolved
pixl_core/tests/uploader/test_dicomweb.py Outdated Show resolved Hide resolved
test/docker-compose.yml Show resolved Hide resolved
test/system_test.py Outdated Show resolved Hide resolved
jeremyestein
jeremyestein approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@jeremyestein jeremyestein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding one more comment. Should have approved before. Fine once issues are addressed.

pixl_core/src/core/uploader/_dicomweb.py Outdated Show resolved Hide resolved
milanmlft added 23 commits April 30, 2024 15:55
@milanmlft
Add test for DICOMweb uploader to check failure if image already expo…
afbb78f 
…rted
@milanmlft
Implement checking for image already uploaded in dicomweb uploader
db9233d
@milanmlft
Merge branch 'main' into milanmlft/fix-dicomweb-uploader
82596f5
@milanmlft
Move Orthanc helpers to core.uploader
f886059
@milanmlft
Make Uploader.upload_dicom_image methods work with Orthanc study ID…
9ba5775 
… as only input arg
@milanmlft
Move FTPS-specific fixtures to its own test file
5ede1a6 
Avoid some premature checking of envvars that haven't been set yet
@milanmlft
Fix orthanc-anon related envvar references
abf25ff
@milanmlft
ORTHANC_URL -> ORTHANC_ANON_URL, which is more accurate
c177902
@milanmlft
Fix TEST_DIR path
d469acf
@milanmlft
Fix return type for fixture
1ece0ee
@milanmlft
Refactor FTPSUploader.upload_dicom_image(); extract FTPS specific f…
6a401d8 
…unctionality for testing
@milanmlft
Fix return types for fixtures
34cc6ef
@milanmlft
Rename fixture so it doesn't get detected as a test
45c056a
@milanmlft
Move 'already exported' check to send_via_ftps()
0b27ae6 
For easier testing
@milanmlft
Refactor: let get_uploader() work with just the project slug
16f57bc
@milanmlft
Add missing envvars for tests
b500442
@milanmlft
Fix import
a10fa3d
@milanmlft
Fix docker compose, messed up during merge
f438a1a
@milanmlft
Implement DicomWebUploader.upload_dicom_image()
00ed2da
@milanmlft
Set ORTHANC_ANON_* envvars in the orthanc-anon container
4d6eeda 
It's ugly but it works
@milanmlft
Avoid loading envvars when they're not needed
5cf8d39
@milanmlft
And another missing envvar
a8d8d17
@milanmlft
Why did I even do this
5ba4f59
stefpiatek
stefpiatek approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for getting this in. Will edit my suggestion in

pixl_core/src/core/uploader/_dicomweb.py Outdated Show resolved Hide resolved
@stefpiatek stefpiatek enabled auto-merge (squash) May 1, 2024 13:37
@stefpiatek stefpiatek merged commit c39da2f into main May 1, 2024
8 checks passed
@stefpiatek stefpiatek deleted the milanmlft/fix-dicomweb-uploader branch May 1, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefpiatek stefpiatek stefpiatek approved these changes

@jeremyestein jeremyestein Awaiting requested review from jeremyestein

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@milanmlft @jeremyestein @stefpiatek