Feature/bi 1189 - 4.0 Exp Preview & Commit: Create Exp Independent Variables by davedrp · Pull Request #187 · Breeding-Insight/bi-api

davedrp · 2022-06-01T19:11:48Z

Description

Story: BI1189

4.0 Exp Preview & Commit: Create Exp Independent Variables

Dependencies

bi-web branch feature/BI-1146

Testing

Using the Experiment Import Template, create an import file with
-- One experiment (every row has the same Exp Title and Exp Description)
-- several Obsevation Units
import and commit
verify that the correct data is saved

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: this TAF run has the same errors as future/1.0 run

…t it is a BOOLEAN

...eedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java

src/main/resources/db/migration/V0.5.37__create_obs_unit_sequence.sql

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

...eedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java

ctucker3

Overall looks good! Some change requests but nothing major.

Is there a plan to add tests for this experiment import? If not for this card, we should make it a requirement for the next card.

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

...eedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java

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

ctucker3 · 2022-06-09T14:39:16Z

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

+        ImportObjectState expState = this.trialByNameNoScope.get(experimentTitle).getState();
+        boolean isExperimentNew = (expState == ImportObjectState.NEW);
+
+        if (isExperimentNew) {


What about the case that the trial exists and the study exists and you are just trying to add obs units to a study. Seems like you would need two checks.

If trial name is new:

Trial data is required

Study data is required

Obs Unit data is required

If Env is new:

Env data is required

ObsUnitData is required

As the story is written, this is out of scope. I will check Tim to see if it should be added.

@shawnyarnes please correct what I may get wrong.

The experiment upload functionality should not allow you to append observation units to an existing environment (aka study). Observation Units can only be added to an environment when the environment is first created.

ctucker3

Looks like some of my change requests from before were missed and I added a couple of comments on some discussions.

...eedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java

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

timparsons · 2022-07-11T15:21:09Z

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

+                .map(ExperimentObservation::getEnv)
+                .distinct()
+                .filter(Objects::nonNull)
+                .collect(Collectors.toList());


I think this comment still needs to be addressed

…or existing studies

…reference sourse names

timparsons

A couple of very small things

timparsons · 2022-07-26T18:44:50Z

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

+            PendingImportObject<BrAPITrial> trialPIO = createTrialPIO(program, commit, importRow, expSeqValue);
            this.trialByNameNoScope.put(importRow.getExpTitle(), trialPIO);

+            //TODO move this above the creation of trialPIO (see BI-1189 tech spec 4.b)?


Does this TODO still apply?

timparsons · 2022-07-26T18:51:59Z

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

+        return seasonDbId;
+    }
+
+    private boolean isRefSource(BrAPIExternalReference brAPIExternalReference) {


I'd recommend either renaming this method to isTrialRefSource or add an additional parameter to specify what ExternalReferenceSource to append.

Feature/bi 1189 - 4.0 Exp Preview & Commit: Create Exp Independent Variables

davedrp and others added 16 commits May 2, 2022 12:34

[BI-1146] WIP

e77ac44

[BI-1146] experiment import: fix import template migration

8245f0b

[BI-1146] support for summary stats

f345aab

[BI-1146] refactor. added code for coditionally required cells

ef1d80d

[BI-1146] refactor. added getNewBrapiData()

4b9e373

[BI-1146] remove unneeded Classes, clean up code

565be9f

[BI-1146] added validation for unique observation Unit Id

15c37be

[BI-1146] Fixed bug. If spreadsheet cell is 'T', tableSaw assumes tha…

3facc3d

…t it is a BOOLEAN

[BI-1189] WIP

0f21719

[BI-1189] added studySequence to Program model object

2520ecc

BI-1189 WIP

d2193e8

[BI-1189] added external references

84fb5a1

[BI-1189] fixed bugs, convert year to season DbId

21c775d

[BI-1189] Add experiment sequence number to study name

ab9cad4

[BI-1189] remove references to study sequence

4bf78f1

[BI-1189] caching for year-to-seasonDbId

b87056a

davedrp requested review from a team, HMS17 and ctucker3 and removed request for a team June 1, 2022 19:11

github-actions bot added the feature label Jun 1, 2022

HMS17 requested changes Jun 1, 2022

View reviewed changes

davedrp assigned ctucker3 Jun 2, 2022

[BI-1189]Addrssed the PR comments by HMS17

c56f5a7

HMS17 requested changes Jun 2, 2022

View reviewed changes

...eedinginsight/brapps/importer/model/imports/experimentObservation/ExperimentObservation.java Outdated Show resolved Hide resolved

davedrp requested a review from HMS17 June 6, 2022 17:00

[BI-1189]Addrssed more PR comments by HMS17

4d6ac34

HMS17 approved these changes Jun 7, 2022

View reviewed changes

davedrp requested a review from HMS17 June 7, 2022 15:02

HMS17 approved these changes Jun 7, 2022

View reviewed changes

ctucker3 suggested changes Jun 9, 2022

View reviewed changes

rob-ouser-bi force-pushed the future/1.0 branch from f6bc537 to d4cf3aa Compare June 15, 2022 15:00

[BI-1189] responses of PR comments from Chris T

14e5f7b

davedrp requested a review from ctucker3 July 6, 2022 18:20

ctucker3 suggested changes Jul 7, 2022

View reviewed changes

timparsons force-pushed the future/1.0 branch from d4cf3aa to 067b493 Compare July 7, 2022 16:23

[BI-1189]Addrssed more PR comments by Chris T

4ca4dfe

timparsons requested changes Jul 11, 2022

View reviewed changes

timparsons force-pushed the future/1.0 branch from d73d097 to 0d26432 Compare July 19, 2022 19:50

timparsons and others added 4 commits July 20, 2022 10:38

Merge branch 'future/1.0' into feature/BI-1189

ae77f3b

[BI-1189] appending experiment number to study names when searching f…

71b5351

…or existing studies

[BI-1189] added experiment sequence number to study name

2bfdf31

[BI-1189] fixed bug. Added ExternalReferenceSource.java to emumerate …

5d3ac00

…reference sourse names

davedrp requested a review from timparsons July 26, 2022 14:38

timparsons requested changes Jul 26, 2022

View reviewed changes

[BI-1189] two small changes suggested by Tim in PR

6bfe649

davedrp requested a review from timparsons July 29, 2022 15:25

timparsons approved these changes Jul 29, 2022

View reviewed changes

Merge branch 'future/1.0' into feature/BI-1189

be90382

davedrp merged commit ed63210 into future/1.0 Aug 1, 2022

davedrp deleted the feature/BI-1189 branch August 1, 2022 12:23

timparsons pushed a commit that referenced this pull request Aug 2, 2022

Merge pull request #187 from Breeding-Insight/feature/BI-1189

9eeae4d

Feature/bi 1189 - 4.0 Exp Preview & Commit: Create Exp Independent Variables

Conversation

davedrp commented Jun 1, 2022

Description

Dependencies

Testing

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctucker3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctucker3 Jun 9, 2022

Choose a reason for hiding this comment

Uh oh!

davedrp Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

timparsons Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

ctucker3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timparsons Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

timparsons left a comment

Choose a reason for hiding this comment

Uh oh!

timparsons Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

timparsons Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants