Removal of scan_done field in session table

[CamilleBeau]

Brief summary of changes

This PR does the following:

• Removes the "Scan_done" column from the session table, which is no longer properly updated
• Replaces "Scan Done" with "Scan Uploaded" in the candidate list, timepoint list, instrument list, and DQT imaging acquisitions category. The "Scan Uploaded" is now based on whether or not there is at least one row in the "files" table associated with the session.
• Removes all references to "Scan done" column from the session table, and when appropriate replaces it with "Scan Uploaded" based on the files table.
• Removes the configuration setting / value for using "Scan done"
• Updates raisinbread dataset based on the above modifications

Modifications to LORIS-MRI code have not been made

• Have you updated related documentation?

Testing instructions (if applicable)

1. Test the candidate list "Scan Uploaded" column to make sure that it is properly populated, and correctly links to the imaging browser when a scan does exist for a candidate
2. Test the timepoint list "Scan Uploaded" column to make sure that it is properly populated, and correctly links to the imaging browser when a scan does exist for a session
3. Test the instrument list "Scan Uploaded" column in the summary at the top to make sure that it is properly populated, and correctly links to the imaging browser when a scan does exist for a session
4. Check the Imaging Acquisitons category in the DQT for the "Scan Uploaded" column. Make sure that it loads and is properly populated
5. Check the configuration module and make sure that the "Scan Done" config setting no longer exists
6. Try loading raisinbread before checking out this PR, and testing it by running the new patch. Then test again by running "make testdata" from inside this PR

Link(s) to related issue(s)

• Resolves Removal of scan_done field in session table #7564

[CamilleBeau]

@kongtiaowang Could you help me with the Test Suite errors here?

[kongtiaowang]

for unit test: --- a/test/unittests/NDB_BVL_Instrument_Test.php

+++ b/test/unittests/NDB_BVL_Instrument_Test.php
@@ -1058,12 +1058,9 @@ class NDB_BVL_Instrument_Test extends TestCase
     function testGetVisitLabel()
     {
         $this->_instrument->commentID = 'commentID1';
-        $this->_mockDB->expects($this->any(0))->method('pselectOne')
-            ->with(
-                "SELECT SessionID FROM flag WHERE CommentID = :CID",
-                ['CID' => 'commentID1']
-            )
-            ->willReturn('123');
+$this->_mockDB->expects($this->any())
+    ->method('pselectOne')
+    ->willReturn('123');

[kongtiaowang]

for integration test: it is better to remove 123458 as fake data, the sqlengine file will check the field name, but the scanUploaded from left join and returne extra fields "scanUploaded".

+++ b/modules/candidate_list/php/candidatelistrowprovisioner.class.inc
@@ -61,6 +61,7 @@ class CandidateListRowProvisioner extends \LORIS\Data\Provisioners\DBRowProvisio
                 GROUP_CONCAT(DISTINCT sp.title) AS Cohort,
                 c.Entity_type AS EntityType,
                 CASE
+                    WHEN COUNT(s.ID) = 0 THEN NULL
                     WHEN COUNT(f.FileID) > 0 THEN 'Y'
                     ELSE 'N'
                 END AS scanUploaded,
diff --git a/modules/imaging_browser/test/ImagingQueryEngineTest.php b/modules/imaging_browser/test/ImagingQueryEngineTest.php
index ee873d545..5bff18536 100644
--- a/modules/imaging_browser/test/ImagingQueryEngineTest.php
+++ b/modules/imaging_browser/test/ImagingQueryEngineTest.php
@@ -80,18 +80,6 @@ class ImagingQueryEngineTest extends TestCase
                     'EDC'                   => '1930-04-01',
                     'Entity_type'           => 'Human',
                 ],
-                [
-                    'ID'                    => 3,
-                    'CandID'                => "123458",
-                    'PSCID'                 => "test3",
-                    'RegistrationProjectID' => '1',
-                    'RegistrationCenterID'  => '3',
-                    'Active'                => 'N',
-                    'DoB'                   => '1940-01-01',
-                    'Sex'                   => 'Other',
-                    'EDC'                   => '1930-04-01',
-                    'Entity_type'           => 'Human',
-                ],
             ]
         );
 
@@ -391,7 +379,7 @@ class ImagingQueryEngineTest extends TestCase
             )
         );
 
-        $this->assertEquals(count($results), 1);
+        $this->assertEquals(count($results), 2);
         $this->assertEquals(
             $results,
             [
@@ -456,11 +444,6 @@ class ImagingQueryEngineTest extends TestCase
                         ],
                     ],
                 ],
-                "123458" => [
-                    'ScanUploaded'       => [],
-                    'ScanType1_file'     => [],
-                    'ScanType1_QCStatus' => [],
-                ],
             ]
         );
     }

[nicolasbrossard]

I think the new column Scan Uploaded should indicate whether a scan was uploaded, not whether a file was inserted in the database when the MRI pipeline processed that scan. In other words, we should check the existence of a record in table mri_upload for that candidate and not in table files to determine whether a scan was uploaded or not.

[MaximeBICMTL]

I think the new column Scan Uploaded should indicate whether a scan was uploaded, not whether a file was inserted in the database when the MRI pipeline processed that scan. In other words, we should check the existence of a record in table mri_upload for that candidate and not in table files to determine whether a scan was uploaded or not.

FYI for MPN on C-BIG, we upload the DICOMs and BIDS directly to LORIS without going through the usual LORIS imaging pipeline. As such, we have sessions that have DICOM archives and MRI files (usually both), but no upload ID. I think it would be best to display those as "scan uploaded", or I think this is going to be confusing for users (thinking about Naj specifically in my case 😅).

[nicolasbrossard]

Looking in tables mri_upload and tarchive would cover all cases then.

@cmadjar What's your take on this?

[MaximeBICMTL]

Looking in tables mri_upload and tarchive would cover all cases then.

Hhhhm, I'd rather look into files than tarchive TBH, as I expect a user to think "oh, a scan has been uploaded, that means I can go take a look at it in the imaging browser" which is not the case with tarchive. All-in-all, I like what this PR does, but if you want to add an alternative condition (or) for upload IDs it's also fine by me.

[cmadjar]

@nicolasbrossard @CamilleBeau As @MaximeBICMTL is highlighting, not all image is going through the uploader or have DICOM files. That is why we rely on the files table to know whether images are there.

There is a caveat though, what if something is uploaded in mri_upload or tarchive table but does not make it to the files table? I am wondering if we could also add a separate query to query those tables and basically, Scan done is marked as yes if there is either at least one file in the files table or one entry in tarchive or mri_upload for the session.

FYI - not sure I would rename the frontend field to Scan Uploaded instead of Scan Done. In my little brain, Scan Uploaded means there is an entry in the imaging uploader. Scan done means there have been images acquired during the session and stored somewhere in LORIS. I don't have a strong preference though.

[CamilleBeau]

@cmadjar Sounds good, I can add that to the query in the new year.

I had renamed it "Scan Uploaded" based on tasks in the issue linked, but happy to put it back to "Scan Done", I think that makes sense as well!