BI-1832 - Treatment factors not being stored in BreedBase#272
BI-1832 - Treatment factors not being stored in BreedBase#272nickpalladino merged 5 commits intorelease/0.8from
Conversation
timparsons
left a comment
There was a problem hiding this comment.
Haven't tested it yet, but one small code change
|
|
||
| // Store treatments in additionalinfo as well for temporary BreedBase workaround | ||
| observationUnit.putAdditionalInfoItem(BrAPIAdditionalInfoFields.TREATMENTS, List.of(treatment)); |
There was a problem hiding this comment.
This may be better to do in the BrAPIObservationUnitDAO to encapsulate that logic only there.
There was a problem hiding this comment.
Moved and pushed changes
|
If a treatment factor is empty when the experiment is imported, "No ManagementFactor" is still being reported in the download file (it should be blank). |
src/main/java/org/breedinginsight/brapps/importer/daos/BrAPIObservationUnitDAO.java
Outdated
Show resolved
Hide resolved
src/main/java/org/breedinginsight/brapps/importer/daos/BrAPIObservationUnitDAO.java
Outdated
Show resolved
Hide resolved
|
|
||
| private void processObservationUnits(List<BrAPIObservationUnit> brapiObservationUnits) { | ||
|
|
||
| // if has treatments in additionalInfo but not treatments property, copy to treatments property |
There was a problem hiding this comment.
The code doesn't match this comment fully. Looks like the code checks if there's the treatments property in the additionalInfo, and if so, replace whatever could be in the Treatments field of the OU. Doesn't look like it's checking to see if there is any value in the Treatments field in the OU before calling setTreatments. Is this the desired logic?
There was a problem hiding this comment.
Updated comment to match code, intent is to just overwrite the treatments property if one exists in the additionalInfo
| // treatments field | ||
| BrAPIObservationUnit ou1 = new BrAPIObservationUnit(); | ||
| ou1.setObservationUnitName("test1"); | ||
| ou1.putAdditionalInfoItem(BrAPIAdditionalInfoFields.TREATMENTS, List.of(testTreatment)); |
There was a problem hiding this comment.
Should this test be updated to set the treatments in the Treatments field rather than in the additionalInfo?
There was a problem hiding this comment.
In this test I wanted to simulate what breedbase would do which would be to only have the treatments in the additionalinfo and not the treatments field.
This is due to no additionalInfo property entry being saved when a treatment factor is blank in the import file. BreedBase always returns "No ManagementFactor" in the treatments property regardless. I will update the bi-api processing logic to clear the treatments property if there is no treatments additionalInfo property. This won't affect existing BreedBase data because nothing was saved anyways but it would affect data saved to the BrAPI test server as any existing treatments would be cleared out since additionalInfo is missing. Would require a migration of data to work in that case if required. @timparsons Has any treatment data been saved that must be kept or can we just assume having the additionalinfo in there going forward is fine? Ended up modifying Breedbase to return no treatments rather than "No ManagementFactor" to resolve this |
Description
Story: BI-1832
Dependencies
Testing
Checklist: