diff --git a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java index 0d5f0aeeb24..d230f3ecf73 100644 --- a/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java +++ b/api/src/org/labkey/api/assay/actions/AssayRunUploadForm.java @@ -70,7 +70,6 @@ import org.springframework.web.multipart.MultipartFile; import org.springframework.web.multipart.MultipartHttpServletRequest; -import jakarta.servlet.http.HttpServletRequest; import java.io.File; import java.io.IOException; import java.util.ArrayList; @@ -85,14 +84,9 @@ import static java.util.Collections.emptyMap; -/** - * User: brittp -* Date: Jul 11, 2007 -* Time: 2:52:54 PM -*/ public class AssayRunUploadForm extends ProtocolIdForm implements AssayRunUploadContext { - protected Map _uploadSetProperties = null; + protected Map _batchProperties = null; protected Map _runProperties = null; private String _comments; private String _name; @@ -127,54 +121,63 @@ public List getRunDataProperties() public Map getRunProperties() throws ExperimentException { if (_runProperties == null) - { - Domain domain = getProvider().getRunDomain(getProtocol()); - List properties = domain.getProperties(); - _runProperties = getPropertyMapFromRequest(properties); - } + _runProperties = getPropertyMapFromRequest(getProvider().getRunDomain(getProtocol())); + return Collections.unmodifiableMap(_runProperties); } - /** @return property descriptor to value */ @Override public Map getBatchProperties() throws ExperimentException { - if (_uploadSetProperties == null) - { - Domain domain = getProvider().getBatchDomain(getProtocol()); - _uploadSetProperties = getPropertyMapFromRequest(domain.getProperties()); - } - return Collections.unmodifiableMap(_uploadSetProperties); + if (_batchProperties == null) + _batchProperties = getPropertyMapFromRequest(getProvider().getBatchDomain(getProtocol())); + + return Collections.unmodifiableMap(_batchProperties); } - protected Map getPropertyMapFromRequest(List columns) throws ExperimentException + protected Map getPropertyMapFromRequest(Domain domain) throws ExperimentException { + List columns = domain.getProperties(); Map properties = new LinkedHashMap<>(); Map additionalFiles = getAdditionalPostedFiles(columns); + Map defaults = getDefaultValues(domain, false); + for (DomainProperty pd : columns) { String propName = UploadWizardAction.getInputName(pd); - String value = getRequest().getParameter(propName); - if (pd.getPropertyDescriptor().getPropertyType() == PropertyType.BOOLEAN && - (value == null || value.isEmpty())) + String value = StringUtils.trimToNull(getRequest().getParameter(propName)); + if (pd.getPropertyDescriptor().getPropertyType() == PropertyType.BOOLEAN && (value == null || value.isEmpty())) value = Boolean.FALSE.toString(); - value = StringUtils.trimToNull(value); - if (pd.getName().equals(AbstractAssayProvider.PARTICIPANT_VISIT_RESOLVER_PROPERTY_NAME) && value != null) - { + if (AbstractAssayProvider.PARTICIPANT_VISIT_RESOLVER_PROPERTY_NAME.equalsIgnoreCase(pd.getName()) && value != null) value = ParticipantVisitResolverType.Serializer.encode(value, getRequest()); - } if (additionalFiles.containsKey(pd)) { - if (additionalFiles.get(pd).equals(BLANK_FILE)) // file has been removed + if (BLANK_FILE.equals(additionalFiles.get(pd))) // file has been removed properties.put(pd, null); else properties.put(pd, additionalFiles.get(pd).toNioPathForRead().toString()); } else properties.put(pd, value); + + // Issue 54112: Retain previous file values when reimporting a run + if (PropertyType.FILE_LINK == pd.getPropertyDescriptor().getPropertyType()) + { + boolean postedNewFile = additionalFiles.containsKey(pd); + String current = properties.get(pd); + boolean hasValue = current != null && !current.isEmpty(); + + if (!postedNewFile && !hasValue) + { + Object prior = defaults.get(pd); + if (prior != null) + properties.put(pd, prior.toString()); + } + } } + return properties; } @@ -316,79 +319,75 @@ public Map getAdditionalFiles() public static File getAssayDirectory(Container c, File root) { - if(null != root) + if (null != root) return new File(root.getAbsolutePath(), AssayFileWriter.DIR_NAME); else return new File(PipelineService.get().findPipelineRoot(c).getRootPath().getAbsolutePath(), AssayFileWriter.DIR_NAME); - } public Map getAdditionalPostedFiles(List pds) throws ExperimentException { - if (_additionalFiles == null) - { - Map fileParameters = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); - List filePdNames = new ArrayList<>(); + if (_additionalFiles != null) + return _additionalFiles; + + Map fileParameters = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + List filePdNames = new ArrayList<>(); - for (DomainProperty pd : pds) + for (DomainProperty pd : pds) + { + if (pd.getPropertyDescriptor().getPropertyType() == PropertyType.FILE_LINK) { - if (pd.getPropertyDescriptor().getPropertyType() == PropertyType.FILE_LINK) - { - fileParameters.put(UploadWizardAction.getInputName(pd), pd); - filePdNames.add(pd.getName()); - } + fileParameters.put(UploadWizardAction.getInputName(pd), pd); + filePdNames.add(pd.getName()); } + } - if (!fileParameters.isEmpty()) - { - AssayFileWriter> writer = new AssayFileWriter<>(); - try - { - // Initialize member variable so we know that we've already tried to save the posted files in case of error - _additionalFiles = new HashMap<>(); - Map postedFiles = writer.savePostedFiles(this, fileParameters.keySet(), false, false); - for (Map.Entry entry : postedFiles.entrySet()) - _additionalFiles.put(fileParameters.get(entry.getKey()), entry.getValue()); + if (fileParameters.isEmpty()) + return _additionalFiles = emptyMap(); - File previousFile; - HttpServletRequest request = getViewContext().getRequest(); + AssayFileWriter> writer = new AssayFileWriter<>(); + try + { + // Initialize member variable so we know that we've already tried to save the posted files in case of error + _additionalFiles = new HashMap<>(); + Map postedFiles = writer.savePostedFiles(this, fileParameters.keySet(), false, false); + for (Map.Entry entry : postedFiles.entrySet()) + _additionalFiles.put(fileParameters.get(entry.getKey()), entry.getValue()); - // Hidden values in form containing previously uploaded files if previous upload resulted in error - for (String fileParam : filePdNames) - { - if (request instanceof MultipartHttpServletRequest mhsr && null != request.getParameter(fileParam)) - { - String previousFileName = request.getParameter(fileParam); - if (null != previousFileName) - { - previousFile = FileUtil.appendName(getAssayDirectory(getContainer(), null), previousFileName); - - MultipartFile multiFile = mhsr.getFileMap().get(UploadWizardAction.getInputName(fileParameters.get(fileParam))); - - // If file is removed from form after error, override hidden file name with empty file - if (null != multiFile && multiFile.getOriginalFilename().isEmpty()) - _additionalFiles.put(fileParameters.get(fileParam), BLANK_FILE); - - // Only add hidden file parameter if it is a valid file in the pipeline root directory and - // a new file hasn't been uploaded for that parameter - if (previousFile.isFile() - && FileUtils.directoryContains(getAssayDirectory(getContainer(), null), previousFile) - && !_additionalFiles.containsKey(fileParameters.get(fileParam))) - { - _additionalFiles.put(fileParameters.get(fileParam), FileSystemLike.wrapFile(previousFile)); - } - } - } - } - } - catch (IOException e) + if (!(getViewContext().getRequest() instanceof MultipartHttpServletRequest request)) + return _additionalFiles; + + File assayDirectory = getAssayDirectory(getContainer(), null); + + // Hidden values in form containing previously uploaded files if the previous upload resulted in error + for (String fileParam : filePdNames) + { + DomainProperty domainProperty = fileParameters.get(fileParam); + MultipartFile multiFile = request.getFileMap().get(fileParam); + + // If the file is removed from form after error, override hidden file name with an empty file + if (null != multiFile && multiFile.getOriginalFilename().isEmpty()) { - throw new RuntimeException(e); + _additionalFiles.put(domainProperty, BLANK_FILE); + continue; } + + String previousFileName = request.getParameter(fileParam); + if (previousFileName == null) + continue; + + // Only add a hidden file parameter if it is a valid file in the pipeline root directory and + // a new file hasn't been uploaded for that parameter + File previousFile = FileUtil.appendName(assayDirectory, previousFileName); + if (previousFile.isFile() && FileUtils.directoryContains(assayDirectory, previousFile) && !_additionalFiles.containsKey(domainProperty)) + _additionalFiles.put(domainProperty, FileSystemLike.wrapFile(previousFile)); } - else - _additionalFiles = emptyMap(); } + catch (IOException e) + { + throw new RuntimeException(e); + } + return _additionalFiles; } @@ -530,6 +529,7 @@ else if (AbstractAssayProvider.PARTICIPANT_VISIT_RESOLVER_PROPERTY_NAME.equals(k } } } + return value; } @@ -580,7 +580,6 @@ public void setBatchId(Long batchId) _batchId = batchId; } - public void clearDefaultValues(Domain domain) { DefaultValueService.get().clearDefaultValues(getContainer(), domain, getUser()); @@ -629,6 +628,11 @@ public Map getDefaultValues(Domain domain, String scope) } public Map getDefaultValues(Domain domain) throws ExperimentException + { + return getDefaultValues(domain, true); + } + + private Map getDefaultValues(Domain domain, boolean includeNameAndComment) throws ExperimentException { ExpRun reRun = getReRun(); if (reRun != null) @@ -661,15 +665,18 @@ else if (runDomainURI.equals(domain.getTypeURI())) // repopulated just like the rest of the domain properties, but they aren't actually part of the // domain- they're hard columns on the ExperimentRun table. Since the DomainProperty objects are // just used to calculate the input form element names, this hack works to pre-populate the values. - DomainProperty nameProperty = domain.addProperty(); - nameProperty.setName("Name"); - ret.put(nameProperty, reRun.getName()); - nameProperty.delete(); - - DomainProperty commentsProperty = domain.addProperty(); - commentsProperty.setName("Comments"); - ret.put(commentsProperty, reRun.getComments()); - commentsProperty.delete(); + if (includeNameAndComment) + { + DomainProperty nameProperty = domain.addProperty(); + nameProperty.setName("Name"); + ret.put(nameProperty, reRun.getName()); + nameProperty.delete(); + + DomainProperty commentsProperty = domain.addProperty(); + commentsProperty.setName("Comments"); + ret.put(commentsProperty, reRun.getComments()); + commentsProperty.delete(); + } } return ret; } diff --git a/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java b/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java index 605c08d3799..9d30bd91a60 100644 --- a/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java +++ b/api/src/org/labkey/api/data/AbstractFileDisplayColumn.java @@ -286,7 +286,7 @@ public void renderInputHtml(RenderContext ctx, HtmlWriter out, Object value) if (null != filename) { // Existing value, so tell the user the file name, allow the file to be removed, and a new file uploaded - renderThumbnailAndRemoveLink(out, ctx, filename, input); + renderThumbnailAndRemoveLink(out, ctx, formFieldName, filename, input); } else { @@ -307,13 +307,15 @@ protected String getRemovalWarningText(String filename) return "Previous file " + filename + " will be removed."; } - private void renderThumbnailAndRemoveLink(HtmlWriter out, RenderContext ctx, String filename, InputBuilder filePicker) + private void renderThumbnailAndRemoveLink(HtmlWriter out, RenderContext ctx, String fieldName, String filename, InputBuilder filePicker) { String divId = GUID.makeGUID(); String linkId = "remove" + divId; DIV( - id(divId), + id(divId) + .data("fieldName", fieldName) + .cl("lk-remove-file"), (Renderable) ret -> { renderIconAndFilename(ctx, out, filename, false, false); out.write(HtmlString.NBSP); diff --git a/api/src/org/labkey/api/data/DataColumn.java b/api/src/org/labkey/api/data/DataColumn.java index 0dd5961e11b..d2c2f593322 100644 --- a/api/src/org/labkey/api/data/DataColumn.java +++ b/api/src/org/labkey/api/data/DataColumn.java @@ -58,7 +58,6 @@ import org.labkey.api.writer.HtmlWriter; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Date; @@ -153,7 +152,6 @@ public DataColumn(ColumnInfo col, boolean withLookups) _textAlign = _displayColumn.getTextAlign(); } - boolean analyticsProviderInitialized = false; @Override @@ -177,7 +175,6 @@ public DataColumn(ColumnInfo col, boolean withLookups) return super.getAnalyticsProviders(); } - @Override public @NotNull Set getClientDependencies() { @@ -186,7 +183,6 @@ public DataColumn(ColumnInfo col, boolean withLookups) return super.getClientDependencies(); } - protected ColumnInfo getDisplayField(@NotNull ColumnInfo col, boolean withLookups) { if (!withLookups) @@ -195,7 +191,6 @@ protected ColumnInfo getDisplayField(@NotNull ColumnInfo col, boolean withLookup return null==display ? col : display; } - @Override public String toString() { @@ -738,7 +733,6 @@ else if (_inputType.equalsIgnoreCase("checkbox")) return ctx.getForm() == null || col == null ? HtmlString.EMPTY_STRING : ctx.getErrors(col); } - private void renderSelectFormInput(HtmlWriter out, String formFieldName, Object value, List strValues, boolean disabledInput, NamedObjectList entryList) { SelectBuilder select = new SelectBuilder() diff --git a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java index 8c5db2ba1a2..95d511e261a 100644 --- a/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java +++ b/assay/test/src/org/labkey/test/tests/assay/AssayReimportIndexTest.java @@ -1,41 +1,42 @@ package org.labkey.test.tests.assay; +import org.apache.commons.lang3.StringUtils; import org.assertj.core.api.Assertions; import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.labkey.remoteapi.domain.PropertyDescriptor; import org.labkey.test.BaseWebDriverTest; import org.labkey.test.Locator; +import org.labkey.test.TestFileUtils; import org.labkey.test.categories.Assays; import org.labkey.test.categories.Daily; -import org.labkey.test.components.assay.AssayConstants; import org.labkey.test.pages.assay.AssayImportPage; import org.labkey.test.pages.assay.AssayRunsPage; -import org.labkey.test.params.FieldDefinition; +import org.labkey.test.params.FieldDefinition.ColumnType; +import org.labkey.test.params.FieldInfo; import org.labkey.test.params.assay.GeneralAssayDesign; +import org.labkey.test.util.DomainUtils.DomainKind; +import org.labkey.test.util.TestDataGenerator; import org.labkey.test.util.search.SearchAdminAPIHelper; +import java.io.File; import java.time.Duration; -import java.util.Arrays; +import java.util.Collections; import java.util.List; @Category({Daily.class, Assays.class}) public class AssayReimportIndexTest extends BaseWebDriverTest { - public static String ASSAY_NAME = "test_assay"; - public static String STRING_FIELD_NAME = "string_field" + TRICKY_CHARACTERS_NO_QUOTES; - - @Override - protected void doCleanup(boolean afterTest) - { - _containerHelper.deleteProject(getProjectName(), afterTest); - } + private static final String ASSAY_NAME = DomainKind.Assay.randomName("test+assay"); + // TODO: Replace each of the following with DomainKind.Assay.randomField(, ColumnType.File) once Issue 54218 is fixed. + private static final FieldInfo BATCH_FILE_FIELD = new FieldInfo("batchFile", ColumnType.File); + private static final FieldInfo RUN_FILE_FIELD = new FieldInfo("runFile", ColumnType.File); @BeforeClass public static void setupProject() throws Exception { AssayReimportIndexTest init = getCurrentTest(); - init.doSetup(); } @@ -44,9 +45,19 @@ private void doSetup() throws Exception _containerHelper.createProject(getProjectName(), "Assay"); goToProjectHome(); + List batchFields = List.of( + DomainKind.Assay.randomField("batchData", ColumnType.String).getFieldDefinition(), + BATCH_FILE_FIELD.getFieldDefinition() + ); + + List runFields = List.of( + DomainKind.Assay.randomField("runString", ColumnType.String).getFieldDefinition(), + RUN_FILE_FIELD.getFieldDefinition() + ); + new GeneralAssayDesign(ASSAY_NAME) - .setBatchFields(List.of(new FieldDefinition("batchData", FieldDefinition.ColumnType.String)), true) - .setRunFields(List.of(new FieldDefinition(STRING_FIELD_NAME, FieldDefinition.ColumnType.String)), true) + .setBatchFields(batchFields, true) + .setRunFields(runFields, true) .createAssay(getProjectName(), createDefaultConnection()); } @@ -67,15 +78,12 @@ public void testIndexLatestAssayRun() 2 2 1 11/12/2024 whee """; - // import the first run - goToProjectHome(); - clickAndWait(Locator.linkWithText(ASSAY_NAME)); - clickButton("Import Data"); - clickButton("Next"); - AssayImportPage importPage = new AssayImportPage(getDriver()); - importPage.setNamedInputText("Name", firstRun); - importPage.setNamedTextAreaValue(AssayConstants.TEXT_AREA_DATA_COLLECTOR_TEXT_AREA_NAME, firstRunData); - importPage.clickSaveAndFinish(); + // import a run + importNewRun() + .clickNext() + .setNamedInputText("Name", firstRun) + .setDataText(firstRunData) + .clickSaveAndFinish(); SearchAdminAPIHelper.waitForIndexer(); // verify it can be searched @@ -85,16 +93,12 @@ public void testIndexLatestAssayRun() .as("expect to find assay run") .isTrue()); - // re-import with secondRunData, call the run version 2 - goToProjectHome(); - clickAndWait(Locator.linkWithText(ASSAY_NAME)); - clickAndWait(Locator.linkWithText(firstRun)); - clickButton("Re-import run"); - clickButton("Next"); - AssayImportPage importPage2 = new AssayImportPage(getDriver()); - importPage2.setNamedInputText("Name", secondRun); - importPage2.setNamedTextAreaValue(AssayConstants.TEXT_AREA_DATA_COLLECTOR_TEXT_AREA_NAME, secondRunData); - importPage2.clickSaveAndFinish(); + // re-import the run with a different name and data + reimportRun(firstRun) + .clickNext() + .setNamedInputText("Name", secondRun) + .setDataText(secondRunData) + .clickSaveAndFinish(); SearchAdminAPIHelper.waitForIndexer(); // verify it can be searched @@ -103,7 +107,7 @@ public void testIndexLatestAssayRun() ()-> Assertions.assertThat(searchResultPage2.hasResultLocatedBy(Locator.linkWithText("Assay Run - " + secondRun))) .as("expect to find second assay run after re-import") .isTrue()); - // verify first run cannot be searched + // verify the first run cannot be searched searchResultPage2.searchForm().searchFor(firstRun); checker().withScreenshot("first_run_unexpectedly_found_after_reimport").awaiting(Duration.ofSeconds(2), ()-> Assertions.assertThat(searchResultPage2.hasResultLocatedBy(Locator.linkWithText("Assay Run - " + firstRun))) @@ -118,18 +122,77 @@ public void testIndexLatestAssayRun() runsPage.getTable().deleteSelectedRows(); SearchAdminAPIHelper.waitForIndexer(); - // verify second run cannot be searched + // verify the second run cannot be searched var searchResultPage3 = navBar().search(firstRun); checker().withScreenshot("first_run_not_found_after_de-indexing_second_run") .verifyTrue("expect to find first assay run", searchResultPage3.hasResultLocatedBy(Locator.linkWithText("Assay Run - " + firstRun))); - // verify first run cannot be searched post-delete + // verify the first run cannot be searched post-delete searchResultPage3.searchForm().searchFor(secondRun); checker().withScreenshot("second_run_found_after_delete") .verifyFalse("expect not to find second assay run after deletion", searchResultPage3.hasResultLocatedBy(Locator.linkWithText("Assay Run - " + secondRun))); } + @Test // Issue 54112 + public void testFileFieldValuesRetainedRunReimport() + { + String runName = TestDataGenerator.randomString(TestDataGenerator.randomInt(10, 50)); + File batchFile = TestFileUtils.getSampleData("dataLoading/excel/fruits.tsv"); + File runFile = TestFileUtils.getSampleData("dataLoading/excel/ClientAPITestList.xls"); + File dataFile = TestFileUtils.getSampleData("assay/GPAT_Run1.tsv"); + + // import the initial run + importNewRun() + .setFileField(BATCH_FILE_FIELD.getName(), batchFile) + .clickNext() + .setNamedInputText("Name", runName) + .setFileField(RUN_FILE_FIELD.getName(), runFile) + .setDataFile(dataFile) + .clickSaveAndFinish(); + + // Reimport the run and verify expected file field values + var importPage = reimportRun(runName); + var reimportBatchFileValue = importPage.getFileFieldValue(BATCH_FILE_FIELD.getName()); + checker().withScreenshot("reimport-run-batch-field-value") + .verifyEquals("Expected batch file name", batchFile.getName(), reimportBatchFileValue); + importPage = importPage.clickNext(); + var reimportRunFileValue = importPage.getFileFieldValue(RUN_FILE_FIELD.getName()); + checker().withScreenshot("reimport-run-run-field-value") + .verifyEquals("Expected run file name", runFile.getName(), reimportRunFileValue); + importPage.selectUploadFileRadioButton() + .clickSaveAndFinish(); + + // Verify that the reimport retained the file values + var row = new AssayRunsPage(getDriver()) + .getTable() + .getRowDataAsMap("Name", runName); + reimportBatchFileValue = StringUtils.trimToNull(row.get("Batch/" + BATCH_FILE_FIELD.getName())); + reimportRunFileValue = StringUtils.trimToNull(row.get(RUN_FILE_FIELD.getName())); + + checker().verifyEquals("Expected batch file to appear in data", batchFile.getName(), reimportBatchFileValue); + checker().verifyEquals("Expected run file to appear in data", runFile.getName(), reimportRunFileValue); + } + + private AssayImportPage importNewRun() + { + goToProjectHome(); + clickAndWait(Locator.linkWithText(ASSAY_NAME)); + clickButton("Import Data"); + + return new AssayImportPage(getDriver()); + } + + private AssayImportPage reimportRun(String runName) + { + goToProjectHome(); + clickAndWait(Locator.linkWithText(ASSAY_NAME)); + clickAndWait(Locator.linkWithText(runName)); + clickButton("Re-import run"); + + return new AssayImportPage(getDriver()); + } + @Override protected String getProjectName() { @@ -139,6 +202,6 @@ protected String getProjectName() @Override public List getAssociatedModules() { - return Arrays.asList(); + return Collections.emptyList(); } }