Fix skydDataId on TargetedMSRun (#526)

- Added a method to update skydDataId set on TargetedMSRun if the corresponding ExpData is not associated with an ExpRun.
- Updated test

Author: vagisha

panoramapublic/src/org/labkey/panoramapublic/PanoramaPublicFileImporter.java

@@ -16,6 +16,7 @@
 import org.labkey.api.pipeline.PipelineService;
 import org.labkey.api.query.BatchValidationException;
 import org.labkey.api.security.User;
+import org.labkey.api.targetedms.TargetedMSService;
 import org.labkey.api.writer.VirtualFile;
 import org.labkey.panoramapublic.pipeline.CopyExperimentPipelineJob;
 
@@ -25,6 +26,7 @@
 import java.nio.file.Paths;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 
 /**
  * This importer does a file move instead of copy to the temp directory and creates a symlink in place of the original
@@ -90,6 +92,7 @@ public void process(@Nullable PipelineJob job, FolderImportContext ctx, VirtualF
             PanoramaPublicSymlinkManager.get().moveAndSymLinkDirectory(expJob, ctx.getContainer(), sourceFiles, targetFiles, log);
 
             alignDataFileUrls(expJob.getUser(), ctx.getContainer(), log);
+            updateSkydDataIds(expJob.getUser(), ctx.getContainer(), log);
         }
     }
 
@@ -154,6 +157,78 @@ private void alignDataFileUrls(User user, Container targetContainer, Logger log)
         }
     }
 
+    /**
+     * Fixes incorrect skydDataId reference in TargetedMSRun. This happens when the relative locations of the sky.zip
+     * and .skyd file are non-standard in the folder being copied.
+     *
+     * When a sky.zip file or its exploded folder are moved, post-import, so that the relative locations of sky.zip and
+     * its corresponding .skyd file are non-standard, two ExpData rows are created for the skyd file in the Panorama Public
+     * copy pipeline job.
+     * The first ExpData (linked to the ExpRun) is created during XAR import.
+     * The second ExpData (not linked to the ExpRun) is created in the SkylineDocumentParser.parseChromatograms() method.
+     * Normally, while running the copy pipeline job,  SkylineDocumentParser.parseChromatograms() does not have to create
+     * a new ExpData, since an ExpData with the expected path already exists.
+     * Having 2 ExpDatas causes:
+     *   1. The skydDataId in TargetedMSRun references an ExpData not linked to the ExpRun. It refers to a file in the
+     *      'export' directory which gets deleted after folder import.
+     *   2. FK violations during cleanup (CopyExperimentFinalTask.cleanupExportDirectory()) prevents deletion of ExpData
+     *      corresponding to the skydDataId
+     *
+     * This method finds a match and updates skydDataId in TargetedMSRun in the case where the skyDataId is not linked
+     * to the ExpRun.
+     */
+    private void updateSkydDataIds(User user, Container targetContainer, Logger log) throws BatchValidationException, ImportException
+    {
+        log.info("Updating skydDataIds in folder: " + targetContainer.getPath());
+
+        boolean errors = false;
+        ExperimentService expService = ExperimentService.get();
+        List<? extends ExpRun> runs = expService.getExpRuns(targetContainer, null, null);
+
+        TargetedMSService tmsService = TargetedMSService.get();
+        for (ExpRun run : runs)
+        {
+            var targetedmsRun = tmsService.getRunByLsid(run.getLSID(), targetContainer);
+            if (targetedmsRun == null) continue;
+
+            var skydDataId = targetedmsRun.getSkydDataId();
+            if (skydDataId == null) continue;
+
+            var skydData = expService.getExpData(skydDataId);
+            if (skydData == null)
+            {
+                log.error("Could not find a row for skydDataId " + skydDataId + " for run " + targetedmsRun.getFileName());
+                errors = true;
+            }
+            else if (skydData.getRun() == null)
+            {
+                // skydData is not associated with an ExpRun. Find an ExpData associated with the ExpRun that matches
+                // the skydName and update the skydDataId on the run.
+                String skydName = skydData.getName();
+                Optional<? extends ExpData> matchingData = run.getAllDataUsedByRun().stream()
+                        .filter(data -> Objects.equals(skydName, data.getName()))
+                        .findFirst();
+
+                if (matchingData.isPresent())
+                {
+                    ExpData data = matchingData.get();
+                    log.debug("Updating skydDataId for run " + targetedmsRun.getFileName() + " to " + data.getRowId());
+                    tmsService.updateSkydDataId(targetedmsRun, data, user);
+                }
+                else
+                {
+                    log.error("Could not find matching skyData for run " + targetedmsRun.getFileName());
+                    errors = true;
+                }
+            }
+        }
+
+        if (errors)
+        {
+            throw new ImportException("Could not update skydDataIds");
+        }
+    }
+
     public static class Factory extends AbstractFolderImportFactory
     {
         @Override

panoramapublic/test/src/org/labkey/test/tests/panoramapublic/PanoramaPublicMoveSkyDocTest.java

@@ -58,9 +58,46 @@ public void testExperimentCopy()
         moveDocument(SKY_FILE_2, targetFolder, 2);
 
         goToProjectFolder(projectName, targetFolder);
-        log("Importing " + SKY_FILE_3 + " in folder " + skyDocSourceFolder);
+        log("Importing " + SKY_FILE_3 + " in folder " + targetFolder);
         importData(SKY_FILE_3, 3);
 
+        // Test moving the sky.zip to a subdirectory in the file root, while the .skyd remains in the original location.
+        //
+        // If the sky.zip file and the .skyd file are not in their typical relative locations, the skydDataId in
+        // TargetedMSRun has to be updated after the folder is copied to Panorama Public. Without the update the
+        // copy pipeline job fails.
+        // Example:
+        // -------------------------
+        // BEFORE MOVE:
+        // SmallMolLibA.sky.zip
+        // SmallMolLibA
+        //   - SmallMolLibA.sky
+        //   - SmallMolLibA.skyd
+        // -------------------------
+        // AFTER MOVE:
+        // - SkylineFiles
+        //   - SmallMolLibA.sky.zip (new location after move)
+        // SmallMolLibA
+        //   - SmallMolLibA.sky
+        //   - SmallMolLibA.skyd
+        // This results in:
+        // Two ExpData rows created for the .skyd file in folder copy on Panorama Public.
+        // 1. @files/export/.../Run<id>/SkylineFiles/SmallMolLibA/SmallMolLibA.skyd
+        // 2. @files/SmallMolLibA/SmallMolLibA.skyd
+        // #1 is set as the skydDataId in TargetedMSRuns, but it is not linked to the ExpRun (runId is null)
+        // #2 is linked to the ExpRun.  This is the ExpData that skydDataId in TargetedMSRun *should* refer to.
+        // This situation causes two problems
+        // 1. ExpData cleanup in CopyExperimentFinalTask fails due to FK violation - cannot delete ExpData #1 since
+        //    skydDataId in TargetedMSRun points to it.
+        // 2. Even if we were not cleaning up ExpData referring to files in the 'export' directory, chromatogram data
+        //    would become unavailable since the "export" directory gets deleted after folder import.
+        //
+        // PanoramaPublicFileImporter.updateSkydDataId() fixes the skydDataId, if required.
+        String subDir = "SkylineFiles";
+        log("Moving " + SKY_FILE_3 + " to sub directory " + subDir + " in the Files browser");
+        // Move the .sky.zip file to a subdirectory
+        moveSkyZipToSubDir(SKY_FILE_3, subDir);
+
         log("Creating and submitting an experiment");
         String experimentTitle = "Experiment to test moving Skyline documents from other folders";
         var expWebPart = createExperimentCompleteMetadata(experimentTitle);
@@ -75,6 +112,15 @@ public void testExperimentCopy()
         verifyRunFilePathRoot(SKY_FILE_1, PANORAMA_PUBLIC, panoramaCopyFolder);
         verifyRunFilePathRoot(SKY_FILE_2, PANORAMA_PUBLIC, panoramaCopyFolder);
         verifyRunFilePathRoot(SKY_FILE_3, PANORAMA_PUBLIC, panoramaCopyFolder);
+
+        // Verify that we can view chromatograms for the Skyline document that was moved to a subdirectory.
+        goToDashboard();
+        clickAndWait(Locator.linkContainingText(SKY_FILE_3));
+        clickAndWait(Locator.linkContainingText("2 replicates"));
+        clickAndWait(Locator.linkContainingText("FU2_2017_0915_RJ_05_1ab_30").index(0));
+        assertTextPresent("Sample File Summary");
+        assertTextPresent("Total Ion Chromatogram");
+        assertTextNotPresent("Unable to load chromatogram");
     }
 
     private void moveDocument(String skylineDocName, String targetFolder, int jobCount)
@@ -96,6 +142,18 @@ private void moveDocument(String skylineDocName, String targetFolder, int jobCou
         verifyRunFilePathRoot(skylineDocName, getProjectName(), targetFolder);
     }
 
+    private void moveSkyZipToSubDir(String documentName, String subDir)
+    {
+        portalHelper.goToModule("FileContent");
+        waitForText(documentName);
+        if (!_fileBrowserHelper.fileIsPresent(subDir))
+        {
+            _fileBrowserHelper.createFolder(subDir);
+        }
+        _fileBrowserHelper.moveFile(documentName, subDir);
+    }
+
+
     private void verifyRunFilePathRoot(String skylineDocName, String projectName, String targetFolder)
     {
         // Verify that exp.run filePathRoot is set to the target folder