Export current extract parquet files over FTPS

[ruaridhg]

Closes #192
Changes:

• Change SendViaStow to SendViaFTPS in orthanc-anon
• upload_parquet_files added to core module
• Tear down test data through docker otherwise later tests fail
• Tests for successful upload and exception thrown in nothing to export
• app in pixl_ehr to SendViaFTPS

[stefpiatek]

Couple of suggestions 🎉

[milanmlft]

I fixed the problem with the FTP_HOST key not being found. I think what happened is that by moving the FTP uploading functionality into export_patient_data(), the unit tests would try to run this but then fail because it can't find that environment variable. Further on that, it also needs an actual FTP server to be up and running, so added that to the docker-compose.yml as well (though this will need to be updated after merging #268).

The CI is currently still failing because of some missing files, so I think there is still a wrongly configured file path somewhere.
That being said, I don't see any actual unit tests that check wether the parquet files have been uploaded to the FTP server, like we're doing in pixl_core/tests/test_upload.py for the DICOM images. So probably still need to add that?

[stefpiatek]

@milanmlft

That being said, I don't see any actual unit tests that check wether the parquet files have been uploaded to the FTP server, like we're doing in pixl_core/tests/test_upload.py for the DICOM images. So probably still need to add that?

Are they not also in pixl_core/tests/test_upload.py test_upload_parquet?

I'm confused though, why are we testing in ehr, when all of this functionality that we care about is in core (which already covers the test)?

[stefpiatek]

I agree with keeping the FTPS-related testing in core. But then we should probably monkeypatch the ehr tests so that it doesn't try to connect to an FTPS server (and then we can remove the FTPS setup in ehr again).

👌 second option is to create a separte function export_reports_to_parquet and test that instead of testing the REST API call. That feels like less faffing around but I'm easy

[milanmlft]

Think it's looking good now! Just the osimis/orthanc docker image seems to have some problems upstream. Just re-running the CI might work 🤷‍♂️

[stefpiatek]

I think this test is still calling export_radiology_as_parquet below and not export_patient_data

[ruaridhg]

👌 second option is to create a separte function export_reports_to_parquet and test that instead of testing the REST API call. That feels like less faffing around but I'm easy

I assumed this is what you meant by having export_radiology_as_parquet and send_via_ftps as separate functions called in a single API endpoint i.e. export_patient_data but testing just export_radiology_as_parquet in test_processing since send via ftps is tested elsewhere

[milanmlft]

System test is failing now because of the call to export_patient_data() in pixl export-patient-data which in turn calls send_via_ftps() (which we want I think!). However, this branch doesn't have an FTPS server set up yet for the system tests (that's under way in #268), so it's failing because of the same reason as before.

I think we can leave this for now and wait until #268 is fixed and merged and then merge main into this branch and add an additional check in the system tests to verify the successful upload of the parquet files.

Will want final review after system tests are merged in

[stefpiatek]

Looking good! Can you add in a check for the parquet files existing in the system test?

[milanmlft]

System test currently failing because we're missing the radiology.parquet file for some reason 😕

[stefpiatek]

🎉