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.

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

[DICOM Archive] Add download tar functionality #2967

Merged
merged 14 commits into from
Apr 30, 2018

Conversation

davidblader
Copy link
Contributor

@davidblader davidblader commented Jul 19, 2017

https://redmine.cbrain.mcgill.ca/issues/9865
image
current display of download link
will need changes made on #2649
get_file.php changes taken from #2791
adding Needs Work until these dependencies are resolved

@davidblader davidblader added the State: Needs work PR awaiting additional work by the author to proceed label Jul 19, 2017
@davidblader davidblader added this to the 17.1 milestone Jul 19, 2017
@davidblader davidblader added the Category: Feature PR or issue that aims to introduce a new feature label Jul 19, 2017
@ZainVirani ZainVirani self-assigned this Jul 19, 2017
@ZainVirani
Copy link
Contributor

unrelated to these changes but line 120 of get_file.php has incorrect strpos syntax @johnsaigle

ZainVirani
ZainVirani previously approved these changes Jul 19, 2017
Copy link
Contributor

@ZainVirani ZainVirani left a comment

Choose a reason for hiding this comment

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

👍

@johnsaigle
Copy link
Contributor

johnsaigle commented Jul 19, 2017

Thanks @ZainVirani. It's corrected here

@davidblader davidblader changed the title [DICOM Archive] Add download column [DICOM Archive] Add download tar functionality Jul 19, 2017
@davidblader davidblader modified the milestones: 17.2, 17.1 Jul 20, 2017
@MounaSafiHarab
Copy link
Contributor

Discussed at the working group today, no more pull request to 17.1 at this late stage, moved to 17.2

@davidblader davidblader added State: Needs work PR awaiting additional work by the author to proceed and removed State: Needs work PR awaiting additional work by the author to proceed labels Sep 15, 2017
@driusan driusan removed this from the 18.1 milestone Oct 2, 2017
@ridz1208 ridz1208 changed the base branch from 17.1-dev to minor October 21, 2017 17:29
@davidblader davidblader added State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) and removed State: Needs work PR awaiting additional work by the author to proceed labels Oct 24, 2017
@davidblader davidblader added State: Needs work PR awaiting additional work by the author to proceed and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) labels Oct 24, 2017
@davidblader davidblader removed the State: Needs work PR awaiting additional work by the author to proceed label Mar 12, 2018
@davidblader davidblader reopened this Mar 13, 2018
@MounaSafiHarab MounaSafiHarab added the State: Needs work PR awaiting additional work by the author to proceed label Mar 19, 2018
@MounaSafiHarab
Copy link
Contributor

MounaSafiHarab commented Mar 23, 2018

@davidblader

@driusan is reviewing my PR #3501, so might be worth it to keep an eye on his comments there as they apply here as well:)

@davidblader davidblader removed the State: Needs work PR awaiting additional work by the author to proceed label Mar 26, 2018
@davidblader davidblader closed this Apr 9, 2018
@davidblader davidblader reopened this Apr 9, 2018
@HenriRabalais
Copy link
Collaborator

HenriRabalais commented Apr 16, 2018

@davidblader Is this ready for re-review?

@davidblader
Copy link
Contributor Author

@davidblader Is this ready for re-review?

@HenriRabalais yeah should be good to go 👍

@nicolasbrossard nicolasbrossard self-assigned this Apr 16, 2018
@nicolasbrossard nicolasbrossard added the Passed manual tests PR has been successfully tested by at least one peer label Apr 16, 2018
@nicolasbrossard
Copy link
Contributor

Worked fine on my sandbox 👍

// LORIS-MRI pipeline, identify it as $FileExt: "DICOMTAR"
// Caveat: this is not a real file extension, but a LORIS-MRI
// convention to identify archived DICOMs
if (strpos($File, "DCM_") !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably nitpicking here but shouldn't the validation be performed on the file's base name? And shouldn't we also validate that 'DCM_' occurs at the very beginning of the file's base name?

Copy link
Contributor

@MounaSafiHarab MounaSafiHarab Apr 17, 2018

Choose a reason for hiding this comment

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

@nicolasbrossard
it is not at the beginning of the filename. DCM_ is usually preceded by the year directory (@driusan had the same comment on another pull request that does similar changes).
If we use the basename; then we can check that DCM_ happens at the beginning.


// Basic config validation
$imagePath = $paths['imagePath'];
$DownloadPath = $paths['DownloadPath'];
$mincPath = $paths['mincPath'];
$tarchivePath = $pipeline['tarchiveLibraryDir'];
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably check (below) that this setting is not empty.

cmadjar
cmadjar previously approved these changes Apr 23, 2018
Copy link
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.

Works great!! 😀

error_log("ERROR: Config settings are missing");
header("HTTP/1.1 500 Internal Server Error");
exit(1);
}

if ($imagePath === '/' || $DownloadPath === '/' || $mincPath === '/') {
if ($imagePath === '/' || $DownloadPath === '/'
|| $mincPath === '/' || tarchivePath === '/'
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be $tarchivePath === '/' (missing $) ?

@ridz1208
Copy link
Collaborator

@nicolasbrossard fixed, please re/review

@driusan driusan merged commit 20e326e into aces:minor Apr 30, 2018
@ridz1208 ridz1208 added this to the 19.3 milestone May 1, 2018
@ridz1208 ridz1208 modified the milestones: 19.3, 20.0 Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature 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.

10 participants