Skip to content

API Dicom download for phantoms: add missing candID#3787

Merged
driusan merged 2 commits intoaces:20.0-releasefrom
MounaSafiHarab:APIDICOMPhantomsBug
Jul 9, 2018
Merged

API Dicom download for phantoms: add missing candID#3787
driusan merged 2 commits intoaces:20.0-releasefrom
MounaSafiHarab:APIDICOMPhantomsBug

Conversation

@MounaSafiHarab
Copy link
Copy Markdown
Contributor

@MounaSafiHarab MounaSafiHarab commented Jul 4, 2018

The API DICOM download was missing the CandID for the phantom cases, so add it in order for the query to get all its needed parameters, and therefore for the download to work.

To test, please try to download a DICOM from a phantom scan (with an existing .tar file in your tarchive directory) before and after this PR change.

Thanks to Mark Pratti for reporting this bug while testing this feature!

@MounaSafiHarab MounaSafiHarab added Category: Bug PR or issue that aims to report or fix a bug Area: API PR or issue related to the API labels Jul 4, 2018
@MounaSafiHarab MounaSafiHarab added this to the 20.0.0 milestone Jul 4, 2018
Copy link
Copy Markdown
Contributor

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

Makes sense! Did not test.

@MounaSafiHarab MounaSafiHarab changed the title API Dicom download for phatoms: add missing candID API Dicom download for phantoms: add missing candID Jul 5, 2018
Copy link
Copy Markdown
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

While I won't block this PR for that, could you remove line #100 will we are here. I don't think $params['PPSCID'] is ever used

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Jul 5, 2018

If memory serves the MySQL PDO driver (at least used to?) throw an exception/crash if the prepared statements parameters didn't match up with the array passed (size, order, etc), so needs to be tested..

@MounaSafiHarab
Copy link
Copy Markdown
Contributor Author

While I won't block this PR for that, could you remove line #100 will we are here. I don't think $params['PPSCID'] is ever used

@xlecours
isn't line 100 being used in building the $ID on the line just above for a real candidate (i.e. non phantoms), which in turn is then used in the query on line 118?
p.s: I did not invent those lines, they are recycled from Imaging Browser (albeit for a slightly different query here).

@xlecours
Copy link
Copy Markdown
Contributor

xlecours commented Jul 9, 2018

@MounaSafiHarab You are right about line 118. Scratch my comment :)

@MounaSafiHarab
Copy link
Copy Markdown
Contributor Author

@davidblader @cmadjar
can anyone of you (or both of you) please tets this? since you both do have phantom data and have used the API before.
(@davidblader you need to copy any .tar for any phantom and test that it downloads now and it was not downloading before the change here).

@davidblader davidblader added the Passed manual tests PR has been successfully tested by at least one peer label Jul 9, 2018
@um4r12
Copy link
Copy Markdown
Contributor

um4r12 commented Jul 9, 2018

Looks good to me. Was able to retrieve the appropriate tar file after incorporating the changes.

Copy link
Copy Markdown
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

Currently on 20.0-release branch, accessing DICOMs for phantom datasets fails. This PR fixes the bug and we can get the DICOMs for phantom datasets now (as well as download them).

So basically, all good!! :)

@driusan driusan merged commit 8326797 into aces:20.0-release Jul 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: API PR or issue related to the API Category: Bug PR or issue that aims to report or fix a bug Passed manual tests PR has been successfully tested by at least one peer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants