From a52eebd51c245227883849524d9809a2be0b96ad Mon Sep 17 00:00:00 2001 From: mlm483 <128052931+mlm483@users.noreply.github.com> Date: Wed, 15 May 2024 12:04:45 -0400 Subject: [PATCH 1/3] [BI-2128] implemented fix and integration tests --- .../processors/ExperimentProcessor.java | 3 +- .../importer/ExperimentFileImportTest.java | 96 +++++++++++++++++-- 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java b/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java index f27d89338..95e196c5b 100644 --- a/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java +++ b/src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java @@ -1244,7 +1244,8 @@ private void fetchOrCreateObservationPIO(Program program, } if (existingObsByObsHash.containsKey(key)) { - if (!isObservationMatched(key, value, column, rowNum)){ + // Update observation value only if it is changed and new value is not blank. + if (!isObservationMatched(key, value, column, rowNum) && StringUtils.isNotBlank(value)){ // prior observation with updated value newObservation = gson.fromJson(gson.toJson(existingObsByObsHash.get(key)), BrAPIObservation.class); diff --git a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java index 6bc203d64..09953b2f7 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java @@ -85,7 +85,6 @@ @TestInstance(TestInstance.Lifecycle.PER_CLASS) @TestMethodOrder(MethodOrderer.OrderAnnotation.class) public class ExperimentFileImportTest extends BrAPITest { - private static final String OVERWRITE = "overwrite"; private FannyPack securityFp; private String mappingId; @@ -822,6 +821,75 @@ public void importNewObservationDataByObsUnitId(boolean commit) { } } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + @SneakyThrows + public void verifyBlankObsInOverwriteIsNoOp(boolean commit) { + List traits = importTestUtils.createTraits(1); + Program program = createProgram("Overwrite Attempt With Blank Obs"+(commit ? "C" : "P"), "NOOP"+(commit ? "C" : "P"), "NOOP"+(commit ? "C" : "P"), BRAPI_REFERENCE_SOURCE, createGermplasm(1), traits); + Map newExp = new HashMap<>(); + newExp.put(Columns.GERMPLASM_GID, "1"); + newExp.put(Columns.TEST_CHECK, "T"); + newExp.put(Columns.EXP_TITLE, "Test Exp"); + newExp.put(Columns.EXP_UNIT, "Plot"); + newExp.put(Columns.EXP_TYPE, "Phenotyping"); + newExp.put(Columns.ENV, "New Env"); + newExp.put(Columns.ENV_LOCATION, "Location A"); + newExp.put(Columns.ENV_YEAR, "2023"); + newExp.put(Columns.EXP_UNIT_ID, "a-1"); + newExp.put(Columns.REP_NUM, "1"); + newExp.put(Columns.BLOCK_NUM, "1"); + newExp.put(Columns.ROW, "1"); + newExp.put(Columns.COLUMN, "1"); + newExp.put(traits.get(0).getObservationVariableName(), "1"); // Valid observation value. + + importTestUtils.uploadAndFetch(importTestUtils.writeExperimentDataToFile(List.of(newExp), traits), null, true, client, program, mappingId); + + // Fetch the ObsUnitId to use in the overwrite upload. + BrAPITrial brAPITrial = brAPITrialDAO.getTrialsByName(List.of((String)newExp.get(Columns.EXP_TITLE)), program).get(0); + Optional trialIdXref = Utilities.getExternalReference(brAPITrial.getExternalReferences(), String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.TRIALS.getName())); + assertTrue(trialIdXref.isPresent()); + BrAPIStudy brAPIStudy = brAPIStudyDAO.getStudiesByExperimentID(UUID.fromString(trialIdXref.get().getReferenceId()), program).get(0); + BrAPIObservationUnit ou = ouDAO.getObservationUnitsForStudyDbId(brAPIStudy.getStudyDbId(), program).get(0); + Optional ouIdXref = Utilities.getExternalReference(ou.getExternalReferences(), String.format("%s/%s", BRAPI_REFERENCE_SOURCE, ExternalReferenceSource.OBSERVATION_UNITS.getName())); + assertTrue(ouIdXref.isPresent()); + + assertRowSaved(newExp, program, traits); + + Map newObsVar = new HashMap<>(); + newObsVar.put(Columns.GERMPLASM_GID, "1"); + newObsVar.put(Columns.TEST_CHECK, "T"); + newObsVar.put(Columns.EXP_TITLE, "Test Exp"); + newObsVar.put(Columns.EXP_UNIT, "Plot"); + newObsVar.put(Columns.EXP_TYPE, "Phenotyping"); + newObsVar.put(Columns.ENV, "New Env"); + newObsVar.put(Columns.ENV_LOCATION, "Location A"); + newObsVar.put(Columns.ENV_YEAR, "2023"); + newObsVar.put(Columns.EXP_UNIT_ID, "a-1"); + newObsVar.put(Columns.REP_NUM, "1"); + newObsVar.put(Columns.BLOCK_NUM, "1"); + newObsVar.put(Columns.ROW, "1"); + newObsVar.put(Columns.COLUMN, "1"); + newObsVar.put(Columns.OBS_UNIT_ID, ouIdXref.get().getReferenceId()); // Indicates this is an overwrite. + newObsVar.put(traits.get(0).getObservationVariableName(), ""); // Empty string should be no op. + + Map requestBody = new HashMap<>(); + requestBody.put("overwrite", "true"); + requestBody.put("overwriteReason", "testing"); + + JsonObject result = importTestUtils.uploadAndFetch(importTestUtils.writeExperimentDataToFile(List.of(newObsVar), traits), requestBody, commit, client, program, mappingId); + JsonArray previewRows = result.get("preview").getAsJsonObject().get("rows").getAsJsonArray(); + assertEquals(1, previewRows.size()); + JsonObject row = previewRows.get(0).getAsJsonObject(); + + // Verify that the overwrite attempt with blank observation value did not overwrite the original value. + assertRowSaved(newExp, program, traits); + if(commit) { + assertRowSaved(newExp, program, traits); + } else { + assertValidPreviewRow(newExp, row, program, traits); + } + } @ParameterizedTest @ValueSource(booleans = {true, false}) @@ -1086,15 +1154,15 @@ public void importNewObsAfterFirstExpWithObs(boolean commit) { /* Scenario: - - an experiment was created with observations - - do a second upload with additional observations for the experiment. Make sure the original observations are blank values - - verify the second set of observations get uploaded successfully + - Create an experiment with valid observations. + - Upload a second file with (1) a blank observation, (2) a changed valid observation, and (3) a new observation for the experiment. + - Verify that (1) the blank observation makes no change, (2) the changed observation is overwritten, and (3) new observations are appended to the experiment. */ @ParameterizedTest @ValueSource(booleans = {true, false}) @SneakyThrows public void importNewObsAfterFirstExpWithObs_blank(boolean commit) { - List traits = importTestUtils.createTraits(2); + List traits = importTestUtils.createTraits(3); Program program = createProgram("Exp with additional Uploads (blank) "+(commit ? "C" : "P"), "EXAUB"+(commit ? "C" : "P"), "EXAUB"+(commit ? "C" : "P"), BRAPI_REFERENCE_SOURCE, createGermplasm(1), traits); Map newExp = new HashMap<>(); newExp.put(Columns.GERMPLASM_GID, "1"); @@ -1110,7 +1178,9 @@ public void importNewObsAfterFirstExpWithObs_blank(boolean commit) { newExp.put(Columns.BLOCK_NUM, "1"); newExp.put(Columns.ROW, "1"); newExp.put(Columns.COLUMN, "1"); - newExp.put(traits.get(0).getObservationVariableName(), "1"); + String originalValue = "1"; // Convenience variable, this value is reused. + newExp.put(traits.get(0).getObservationVariableName(), originalValue); + newExp.put(traits.get(1).getObservationVariableName(), "2"); importTestUtils.uploadAndFetch(importTestUtils.writeExperimentDataToFile(List.of(newExp), traits), null, true, client, program, mappingId); @@ -1140,10 +1210,15 @@ public void importNewObsAfterFirstExpWithObs_blank(boolean commit) { newObservation.put(Columns.ROW, "1"); newObservation.put(Columns.COLUMN, "1"); newObservation.put(Columns.OBS_UNIT_ID, ouIdXref.get().getReferenceId()); - newObservation.put(traits.get(0).getObservationVariableName(), ""); - newObservation.put(traits.get(1).getObservationVariableName(), "2"); + newObservation.put(traits.get(0).getObservationVariableName(), ""); // This blank value should not overwrite. + newObservation.put(traits.get(1).getObservationVariableName(), "3"); // This valid value should overwrite. + newObservation.put(traits.get(2).getObservationVariableName(), "4"); // This valid new observation should be appended. - JsonObject result = importTestUtils.uploadAndFetch(importTestUtils.writeExperimentDataToFile(List.of(newObservation), traits), null, commit, client, program, mappingId); + Map requestBody = new HashMap<>(); + requestBody.put("overwrite", "true"); + requestBody.put("overwriteReason", "testing"); + + JsonObject result = importTestUtils.uploadAndFetch(importTestUtils.writeExperimentDataToFile(List.of(newObservation), traits), requestBody, commit, client, program, mappingId); JsonArray previewRows = result.get("preview").getAsJsonObject().get("rows").getAsJsonArray(); assertEquals(1, previewRows.size()); @@ -1154,6 +1229,9 @@ public void importNewObsAfterFirstExpWithObs_blank(boolean commit) { assertEquals("EXISTING", row.getAsJsonObject("study").get("state").getAsString()); assertEquals("EXISTING", row.getAsJsonObject("observationUnit").get("state").getAsString()); + // The blank value should not have produced an overwrite. + // This change is to make the "expected" value correct. + newObservation.put(traits.get(0).getObservationVariableName(), originalValue); if(commit) { assertRowSaved(newObservation, program, traits); } else { From c3af14fe0e56d8f42d86667ec06708f254099c60 Mon Sep 17 00:00:00 2001 From: mlm483 <128052931+mlm483@users.noreply.github.com> Date: Wed, 15 May 2024 12:14:10 -0400 Subject: [PATCH 2/3] [BI-2128] added test description --- .../brapps/importer/ExperimentFileImportTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java index 09953b2f7..2d884bac8 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java @@ -821,6 +821,12 @@ public void importNewObservationDataByObsUnitId(boolean commit) { } } + /* + Scenario: + - an experiment was created with observations + - an overwrite operation is attempted with blank observation values + - verify blank obervation values do not overwrite original values + */ @ParameterizedTest @ValueSource(booleans = {true, false}) @SneakyThrows From fa8c7cb586f4bf4110a505e10e090fecee0cd714 Mon Sep 17 00:00:00 2001 From: mlm483 <128052931+mlm483@users.noreply.github.com> Date: Wed, 15 May 2024 13:11:44 -0400 Subject: [PATCH 3/3] [BI-2128] fixed typo --- .../brapps/importer/ExperimentFileImportTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java index 2d884bac8..48bffd4c2 100644 --- a/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java +++ b/src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java @@ -825,7 +825,7 @@ public void importNewObservationDataByObsUnitId(boolean commit) { Scenario: - an experiment was created with observations - an overwrite operation is attempted with blank observation values - - verify blank obervation values do not overwrite original values + - verify blank observation values do not overwrite original values */ @ParameterizedTest @ValueSource(booleans = {true, false})