BI-1195 - 4.3 Exp Preview: Append Experiment with Observations by timparsons · Pull Request #241 · Breeding-Insight/bi-api

timparsons · 2023-02-09T01:17:38Z

Description

Story: https://breedinginsight.atlassian.net/browse/BI-1195

Added support to recognize the ObsUnitId column in an experiment import file
- will fetch existing observationunits by ObsUnitId
- will fetch trials, studies, and locations if there is an ObsUnitId
Added validation to check for existing observations for a given OUxTrait combo
- if there is, throw an error
Refactored the ExperimentProcessor to be easier to read
Updated the creation of new locations via the experiment import to create them in the BI database before creating them in the BrAPI server
Updated locations being created in a BrAPI server to have
- the program key in the name
- an external reference to the program
Created a Java-based migration to update existing locations with an external reference to the program
Created unit test for testing experiment file import

Dependencies

bi-web: Breeding-Insight/bi-web#302
sgn: Breeding-Insight/sgn#109

Testing

This code isn't testable via the UI with BreedBase until the UI to download an experiment with ObsUnitIds is implemented, however, you can hit the BrAPI endpoints to fetch the ObservationUnit external references that were generated by DeltaBreed to then put into an import file.

Review the tests in ExperimentFileImportTest for coverage
Run the ExperimentFileImportTest and verify all tests pass

Checklist:

I have performed a self-review of my own code
I have tested my code and ensured it meets the acceptance criteria of the story
I have tested that my code works with both the brapi-java-server and BreedBase
I have create/modified unit tests to cover this change
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to documentation
I have run TAF: https://github.com/Breeding-Insight/taf/actions/runs/4129982227

src/main/java/org/breedinginsight/brapi/v2/GermplasmController.java

src/main/java/org/breedinginsight/brapps/importer/daos/BrAPITrialDAO.java

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

src/main/java/org/breedinginsight/daos/ProgramLocationDAO.java

src/main/java/org/breedinginsight/brapps/importer/daos/BrAPIObservationUnitDAO.java

davedrp · 2023-02-23T17:14:06Z

src/main/java/org/breedinginsight/brapps/importer/daos/BrAPIStudyDAO.java

+        );
+    }
+
+    public Optional<BrAPIStudy> getStudyByDbId(String studyDbId, Program program) throws ApiException {


I don't think getStudyByDbId() is ever called

Yep, you're right. Thoughts on leaving it as I can see it being used at some point?

src/main/java/org/breedinginsight/brapps/importer/daos/BrAPITrialDAO.java

davedrp · 2023-02-23T17:34:21Z

src/main/java/org/breedinginsight/brapps/importer/model/base/Location.java

@@ -24,6 +24,7 @@
 import org.breedinginsight.brapps.importer.model.config.ImportFieldType;


the statement import org.brapi.v2.model.core.BrAPILocation; is no longer needed.

src/main/java/org/breedinginsight/brapps/importer/services/FileImportService.java

davedrp · 2023-02-23T21:49:13Z

src/main/java/org/breedinginsight/brapps/importer/services/processors/LocationProcessor.java

 import org.breedinginsight.model.User;
+import org.breedinginsight.services.ProgramLocationService;
 import org.breedinginsight.services.exceptions.ValidatorException;
+import org.breedinginsight.utilities.Utilities;


Unused import statement

davedrp · 2023-02-23T21:54:49Z

src/main/java/org/breedinginsight/daos/impl/TraitDAOImpl.java

 import org.brapi.client.v2.BrAPIClient;
 import org.brapi.client.v2.model.exceptions.ApiException;
 import org.brapi.client.v2.model.queryParams.phenotype.VariableQueryParams;
+import org.brapi.client.v2.modules.core.LocationsApi;


unused import statement

davedrp · 2023-02-23T22:01:17Z

src/main/java/org/breedinginsight/services/ProgramLocationService.java

+        ProgramLocation location = dsl.transactionResult(configuration -> {
            programLocationDao.insert(placeEntity);
-            ProgramLocation progLocation = programLocationDao.getById(programId, placeEntity.getId()).get();
+            ProgramLocation progLocation = programLocationDao.getById(programId, placeEntity.getId(), false).get();


'Optional.get()' without 'isPresent()' check

davedrp · 2023-02-23T22:01:30Z

src/main/java/org/breedinginsight/services/ProgramLocationService.java

        ProgramLocation location = dsl.transactionResult(configuration -> {
            programLocationDao.update(placeEntity);
-            ProgramLocation progLocation = programLocationDao.getById(programId, placeEntity.getId()).get();
+            ProgramLocation progLocation = programLocationDao.getById(programId, placeEntity.getId(), false).get();


'Optional.get()' without 'isPresent()' check

davedrp · 2023-02-23T22:16:56Z

src/test/java/org/breedinginsight/brapps/importer/ExperimentFileImportTest.java

+        return ret;
+    }
+
+    private Map<String, Object> assertValidPreviewRow(Map<String, Object> expected, JsonObject actual, Program program, List<Trait> traits) {


is this private method ever called

Not at the moment. I was thinking that there should probably be some tests added for preview of import (current tests are only focused on the full save), so it may be worth keeping this here. Thoughts?

Makes sense

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java

davedrp · 2023-02-24T21:35:51Z

Test failed - see the comment

davedrp · 2023-03-01T22:19:37Z

Unit Test passed

… experiment file import

… observations to existing OUs Also refactored the ExperimentProcessor to improve readability/maintainability

More refactored to the ExperimentProcessor to improve readability/maintainability

Fixing bugs and adding methods as needed to get the tests to pass

Added a migration for updating existing locations with a program xref

…I server

…a BrAPI search request Returning an empty list if list parameters are empty

timparsons requested review from davedrp and dmeidlin February 9, 2023 01:17

timparsons assigned davedrp Feb 9, 2023

github-actions bot added the feature label Feb 9, 2023

timparsons force-pushed the feature/BI-1195 branch from ddf15ec to e3e713a Compare February 9, 2023 17:55

dmeidlin reviewed Feb 15, 2023

View reviewed changes

timparsons requested a review from dmeidlin February 16, 2023 19:51

timparsons force-pushed the feature/BI-1195 branch from 4b896f0 to e52d6ae Compare February 16, 2023 19:53

dmeidlin approved these changes Feb 16, 2023

View reviewed changes

davedrp requested changes Feb 23, 2023

View reviewed changes

davedrp reviewed Feb 24, 2023

View reviewed changes

src/main/java/org/breedinginsight/brapps/importer/services/processors/ExperimentProcessor.java Show resolved Hide resolved

timparsons force-pushed the feature/BI-1195 branch from e52d6ae to bda0c86 Compare February 28, 2023 16:33

timparsons requested a review from davedrp February 28, 2023 16:33

timparsons added 11 commits March 1, 2023 22:17

[BI-1195] Adding functionality to fetch existing observation units on…

6804c88

… experiment file import

[BI-1195] Completed initial implementation of supporting appending of…

ca0ed49

… observations to existing OUs Also refactored the ExperimentProcessor to improve readability/maintainability

[BI-1195] Updating experiment import validation logic

6293ac2

More refactored to the ExperimentProcessor to improve readability/maintainability

[BI-1195] Implementing unit test for experiment import.

98b1fe9

Fixing bugs and adding methods as needed to get the tests to pass

[BI-1195] Finishing implementation of tests for experiment import

8b5bf8c

Added a migration for updating existing locations with a program xref

[BI-1195] Appending program key to location names when saving to BrAP…

7ec6a5f

…I server

[BI-1195] Fixing failed test

5ee8daa

[BI-1195] Fixing failed test

c6fe388

[BI-1195] Fixing failed test

f23c95c

[BI-1195] Updates from PR feedback

cc9214a

[BI-1195] Fixing error when uploading multiple new environments

ace31c9

timparsons force-pushed the feature/BI-1195 branch from bda0c86 to ace31c9 Compare March 2, 2023 04:17

timparsons added 3 commits March 1, 2023 23:52

[BI-1195] Renaming migration due to conflict

79bab6b

[BI-1195] updates from PR comments

8e830f5

[BI-1195] Checking that list parameters have values before executing …

f33d54e

…a BrAPI search request Returning an empty list if list parameters are empty

[BI-1195] Fixing bug when saving observations

004e9d0

davedrp approved these changes Mar 8, 2023

View reviewed changes

timparsons merged commit a13fe6e into develop Mar 9, 2023

timparsons deleted the feature/BI-1195 branch March 9, 2023 00:06

		@@ -24,6 +24,7 @@
		import org.breedinginsight.brapps.importer.model.config.ImportFieldType;

Conversation

timparsons commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Testing

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timparsons Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davedrp commented Feb 24, 2023

Uh oh!

davedrp commented Mar 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timparsons commented Feb 9, 2023 •

edited

Loading

timparsons Mar 2, 2023 •

edited

Loading