BI-2641 - Remove references to static ObsUnitID column for Create Experiment Workflow #482
Conversation
davedrp
requested review from
a team,
HMS17 and
nickpalladino
and removed request for
a team
davedrp
force-pushed
the
feature/BI-2641
branch
from
cf77d04 to
ee6f2d4
Compare
|
Some unit test where failing due to a bug that I introduced. The code has now been fixed |
|
There is a problem with the Gemini CLI PR review. Please check the action logs for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This pull request aims to remove the dependency on a static ObsUnitID for the experiment creation workflow. The changes are generally well-aligned with this goal, simplifying the import process for users.
🔍 General Feedback
- The refactoring correctly removes the logic for fetching existing observation units, trials, and studies based on user-provided IDs.
- Most of the changes in the affected files are clean and contribute to the desired outcome.
I have found one critical issue that needs to be addressed before merging. Please see the inline comment for details.
| existingLocations.forEach(existingLocation -> locationByName.put(existingLocation.getName(), new PendingImportObject<>(ImportObjectState.EXISTING, existingLocation, existingLocation.getId()))); | ||
| return locationByName; | ||
| } | ||
| /** |
There was a problem hiding this comment.
🔴 critical: The trialByNameNoScope variable is initialized as a new empty HashMap within this method. This shadows the trialByNameNoScope map that is initialized and populated in the process method. As a result, getTrialPIO will always be called with an empty map, trialPIO will be Optional.empty(), and no observation variable datasets will be found.
To fix this, you should pass the trialByNameNoScope map from the process method as a parameter to this method, and remove this line.
There was a problem hiding this comment.
@gemini-cli This feedback seems incorrect. trialByNameNoScope is passed into the initializeStudyByNameNoScope method.
|
@nickpalladino I've received your request and I'm working on it now! 🤖 |
davedrp
force-pushed
the
feature/BI-2641
branch
from
b0d87ba to
974936c
Compare
davedrp
force-pushed
the
feature/BI-2641
branch
from
b395771 to
ec14ddc
Compare
…portObjectsStep.java
…portObjectsStep.java
davedrp
force-pushed
the
feature/BI-2641
branch
from
ec14ddc to
2656a29
Compare
|
The build was run manually, and was successful. |
HMS17
left a comment
HMS17
left a comment
There was a problem hiding this comment.
Successfully imported new experiment, looks good!
| Map<String, PendingImportObject<BrAPITrial>> trialByNameNoScope, | ||
| List<ExperimentObservation> experimentImportRows) { | ||
| Map<String, PendingImportObject<BrAPIListDetails>> obsVarDatasetByName = new HashMap<>(); | ||
| Map<String, PendingImportObject<BrAPITrial>> trialByNameNoScope = new HashMap<>(); |
There was a problem hiding this comment.
What was the reason for making this empty rather than using the passed in map?
Description
BI-2641 - Remove references to static ObsUnitID column for Create Experiment Workflow
Dependencies
bi-web:DEVELOP
Testing
Import a new experiment. It should still behave according to spec.
Checklist: