Imaging Browser: Add DICOM download: Redmine 12580 #3501

MounaSafiHarab · 2018-02-19T22:59:18Z

Projects can download the .tar that includes raw (DICOM) images, as well as .log and .meta (some meta data related to the insertion progress)

One .tar per candidate per visit:
Two .tar per candidate (example here illustrated becaus2 the candidate had to repeat a specific sequence another day, but under the same visit):

christinerogers

2 suggestions for the documentation

Also... revision to the Imaging Browser Spec? (if/when merged?)

christinerogers · 2018-02-21T19:17:23Z

modules/imaging_browser/test/imaging_browser_test_plan.md

@@ -11,7 +11,7 @@

 ### B. ViewSession / Volume List

-8. Sidebar:  all links work 
+8. Sidebar:  all links work, inlcuding the Download DICOM option


typo: inlcuding -> including

If there are multiple tars, they should be listed (and download on each should work)

@christinerogers

typo fixed! thanks

yes, this should work; I tested it as we had a case in CCNA wwhich had its initial MRi visit split into two scanning sessions and the changes here work as well. I fixed the screenshot at the beginning of the pull request to show this case.

re. your earlier comment on specs; I had already updated those in an earlier commit before you made the comment; is it not sufficient what I wrote?

PapillonMcGill · 2018-02-22T13:29:03Z

modules/imaging_browser/php/imaging_session_controlpanel.class.inc

+        foreach ($qresult as $k=>$v) {
+            $tarchiveID = $v['TarchiveID'];
+            $tarIDToTarLoc[$tarchiveID] =$v['ArchiveLocation'];
+        }


suggest using array_values() instead of foreach. Prevent unused variable $k.

removed unused $k. thanks!

davidblader · 2018-02-26T19:03:34Z

needs rebase

MounaSafiHarab · 2018-02-26T21:22:55Z

needs rebase

I just did now that PR #3500 is merged.

cmadjar

@MounaSafiHarab I tried it on a dataset with the existing archive and it worked like a charm!

I also tried it on a dataset that did not have a correct path to the archive and got this page:

@driusan, @MounaSafiHarab could it be possible to send a warning that the file cannot be found instead of the error page? It would look cleaner.

cmadjar · 2018-03-05T19:09:10Z

modules/imaging_browser/README.md

@@ -78,7 +78,10 @@ mantis_url - This setting defines a URL for LORIS to include a link to for bug r
  `$LORIS/tools/CouchDB_Import_MRI.php` alongside all other CouchDB
  import scripts, but should logically be considered part of this module.)
 - The imaging browser module includes links to BrainBrowser to visualize data.
+- The control panel on the viewsession page includes links to instruments


I would tend to right "View Session" page

done, changed in in the 5 occurrences

cmadjar · 2018-03-05T19:09:35Z

modules/imaging_browser/README.md

 - The control panel on the viewsession page includes links to the instruments
  as configured by the study.
- The control panel on the viewsession page includes links to the DICOM Archive
-  for any DICOM tars associated with the given session.
+- The control panel on the viewsession page includes links to the DICOM Archive 


Same here, "View Session" instead of viewsession

MounaSafiHarab · 2018-03-05T19:25:30Z

@driusan, @MounaSafiHarab could it be possible to send a warning that the file cannot be found instead of the error page? It would look cleaner.

perfectly valid point, but just to be clear, this is the same error you get if you try to download a MINC file that does not exist (see below).

it is in the get_file, in the section that handles the file non-existent condition:
if (!file_exists($FullPath)) { error_log("ERROR: File $File does not exist"); header("HTTP/1.1 404 Not Found"); exit(5); }

So not sure I should change it here, or this is the subject of another pull request....

cmadjar · 2018-03-05T19:46:48Z

@MounaSafiHarab I agree with you, it should probably be a separate ticket. Do you want to create a redmine ticket for it? In the meantime, I'll approve your PR :). Thanks!

cmadjar

👍 Nice work :)

MounaSafiHarab · 2018-03-06T19:15:19Z

@MounaSafiHarab I agree with you, it should probably be a separate ticket. Do you want to create a redmine ticket for it? In the meantime, I'll approve your PR :). Thanks!

Done, Redmine #14029

driusan · 2018-03-19T19:52:30Z

htdocs/mri/jiv/get_file.php

@@ -92,6 +94,12 @@
    exit(4);
 }

+// If $File contains "DCM_", prefix automatically inserted by the
+// LORIS-MRI pipeline, identify it as $FileExt: "DICOMTAR"
+if (strpos($File, "DCM_") ) {


missing === 0. The comment should also point out that it's a hack, since there's no such thing as a DICOMTAR extension.

@driusan
you can not assume the DCM is at the beginning of the filename. It used to be the case, but not anymore (now the file stored within a year directory, followed by the filename which then starts with DCM_), so in the tarchive table, a typical ArchiveLocation is:
2016/DCM_2016-09-13_ImagingUpload-9-50-abCD77.tar

but I updated the comment as you requested

In that case, it still needs !== false to differentiate from finding at the 0 index vs not finding in the string at all.

thanks, done!

ridz1208 · 2018-03-27T12:30:02Z

@kongtiaowang too many changes after you tested, please test again

cmadjar

👍 Just tested on my sandbox and it works perfectly aside from when the file does not exist in the filesystem, problem which will be resolved in a separate PR as discussed with @MounaSafiHarab.

PapillonMcGill

LGTM

stale review

driusan · 2018-04-09T15:33:57Z

modules/imaging_browser/php/imaging_session_controlpanel.class.inc

+        $tarIDToTarLoc =array();
+        foreach ($qresult as $v) {
+            $tarchiveID = $v['TarchiveID'];
+            $tarIDToTarLoc[$tarchiveID] =$v['ArchiveLocation'];


I'm surprised this passed PHPCS

I don't really know;
I added an extra space after the = sign, so it is a manual change; and this passes phpcs if this was your main concern (I still do not know how it passed before)
not sure if the other issue is also the line before where the == is not aligned with the line underneath it, but when I did force it to manually be aligned with the line below, it actually gave me phpcs error!

MounaSafiHarab · 2018-04-17T17:36:55Z

@nicolasbrossard

I addressed your comments on @davidblader PR (#2967) here for the get_file.php
which incidentally could also address one of @driusan's earlier comments on this pull request (for checking the beginning of the filename starting with DCM now that I added the basename).

@cmadjar removed your passed manual test tag because I added a couple of commits based on comments.

cmadjar

Works great!! 😀

…eToLorisDICOMDownload

MounaSafiHarab · 2018-05-10T15:14:06Z

I rebased this on minor since PR #2967 has been merged, so I do not need the get_file anymore...

is there anything else this PR is waiting on?
@davidblader @driusan @PapillonMcGill @cmadjar ?

Imaging Browser: Add DICOM download: Redmine 12580

9ce94ae

MounaSafiHarab added the [branch] minor label Feb 19, 2018

MounaSafiHarab added this to the 19.1 milestone Feb 19, 2018

MounaSafiHarab mentioned this pull request Feb 19, 2018

Imaging Browser: Make Links to instruments project customizable and add the DICOM download: Redmines 12500 and 12580 #3496

Closed

MounaSafiHarab added 2 commits February 20, 2018 10:04

update README

8f24d2e

test plan: specify the download dicom option

1a9275e

kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Feb 20, 2018

MounaSafiHarab added the Category: Feature PR or issue that aims to introduce a new feature label Feb 21, 2018

christinerogers previously approved these changes Feb 21, 2018

View reviewed changes

PapillonMcGill reviewed Feb 22, 2018

View reviewed changes

remove unused key as per Melanies feedback

94269b3

MounaSafiHarab dismissed christinerogers’s stale review via 94269b3 February 22, 2018 15:20

davidblader added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 26, 2018

rebase aces minor

b7e0974

MounaSafiHarab removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 26, 2018

driusan modified the milestones: 19.1, 19.2 Feb 28, 2018

cmadjar reviewed Mar 5, 2018

View reviewed changes

Cecile feedback on View Session

1ca82fd

cmadjar previously approved these changes Mar 5, 2018

View reviewed changes

PapillonMcGill previously approved these changes Mar 6, 2018

View reviewed changes

driusan previously requested changes Mar 19, 2018

View reviewed changes

update comment based on Daves feedback

ed1b8b4

MounaSafiHarab dismissed stale reviews from PapillonMcGill and cmadjar via ed1b8b4 March 23, 2018 15:49

MounaSafiHarab mentioned this pull request Mar 23, 2018

[DICOM Archive] Add download tar functionality #2967

Merged

Daves feedback

389d76a

ridz1208 removed the Passed manual tests PR has been successfully tested by at least one peer label Mar 27, 2018

cmadjar added the Passed manual tests PR has been successfully tested by at least one peer label Apr 9, 2018

cmadjar previously approved these changes Apr 9, 2018

View reviewed changes

PapillonMcGill previously approved these changes Apr 9, 2018

View reviewed changes

driusan reviewed Apr 9, 2018

View reviewed changes

manual PHPCS to address Daves concern and Nicks feedback for basename

821b78a

MounaSafiHarab dismissed stale reviews from PapillonMcGill and cmadjar via 821b78a April 17, 2018 17:33

MounaSafiHarab removed the Passed manual tests PR has been successfully tested by at least one peer label Apr 17, 2018

ridz1208 removed this from the 19.2 milestone Apr 18, 2018

cmadjar previously approved these changes Apr 23, 2018

View reviewed changes

PapillonMcGill previously approved these changes Apr 23, 2018

View reviewed changes

nicks comment on missing $

1cfb14b

MounaSafiHarab dismissed stale reviews from PapillonMcGill and cmadjar via 1cfb14b April 25, 2018 17:44

Merge remote-tracking branch 'aces/minor' into ImagBrowserCCNAOverrid…

11d1cdc

…eToLorisDICOMDownload

PapillonMcGill approved these changes May 11, 2018

View reviewed changes

driusan merged commit 3d7650e into aces:minor May 16, 2018

ridz1208 added this to the 19.3 milestone May 18, 2018

ridz1208 modified the milestones: 19.3, 20.0 Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imaging Browser: Add DICOM download: Redmine 12580 #3501

Imaging Browser: Add DICOM download: Redmine 12580 #3501

MounaSafiHarab commented Feb 19, 2018 •

edited

Loading

christinerogers left a comment

christinerogers Feb 21, 2018

MounaSafiHarab Feb 22, 2018 •

edited

Loading

PapillonMcGill Feb 22, 2018

MounaSafiHarab Feb 22, 2018

davidblader commented Feb 26, 2018

MounaSafiHarab commented Feb 26, 2018 •

edited

Loading

cmadjar left a comment

cmadjar Mar 5, 2018

MounaSafiHarab Mar 5, 2018 •

edited

Loading

cmadjar Mar 5, 2018

MounaSafiHarab Mar 5, 2018

MounaSafiHarab commented Mar 5, 2018

cmadjar commented Mar 5, 2018

cmadjar left a comment

MounaSafiHarab commented Mar 6, 2018

driusan Mar 19, 2018

MounaSafiHarab Mar 23, 2018 •

edited

Loading

driusan Mar 23, 2018

MounaSafiHarab Mar 23, 2018

ridz1208 commented Mar 27, 2018

cmadjar left a comment

PapillonMcGill left a comment

driusan Apr 9, 2018

MounaSafiHarab Apr 17, 2018

MounaSafiHarab commented Apr 17, 2018 •

edited

Loading

cmadjar left a comment

MounaSafiHarab commented May 10, 2018

Imaging Browser: Add DICOM download: Redmine 12580 #3501

Imaging Browser: Add DICOM download: Redmine 12580 #3501

Conversation

MounaSafiHarab commented Feb 19, 2018 • edited Loading

christinerogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounaSafiHarab Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidblader commented Feb 26, 2018

MounaSafiHarab commented Feb 26, 2018 • edited Loading

cmadjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounaSafiHarab Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounaSafiHarab commented Mar 5, 2018

cmadjar commented Mar 5, 2018

cmadjar left a comment

Choose a reason for hiding this comment

MounaSafiHarab commented Mar 6, 2018

Choose a reason for hiding this comment

MounaSafiHarab Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridz1208 commented Mar 27, 2018

cmadjar left a comment

Choose a reason for hiding this comment

PapillonMcGill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MounaSafiHarab commented Apr 17, 2018 • edited Loading

cmadjar left a comment

Choose a reason for hiding this comment

MounaSafiHarab commented May 10, 2018

MounaSafiHarab commented Feb 19, 2018 •

edited

Loading

MounaSafiHarab Feb 22, 2018 •

edited

Loading

MounaSafiHarab commented Feb 26, 2018 •

edited

Loading

MounaSafiHarab Mar 5, 2018 •

edited

Loading

MounaSafiHarab Mar 23, 2018 •

edited

Loading

MounaSafiHarab commented Apr 17, 2018 •

edited

Loading