BI-2132 - Create new experiment workflow by nickpalladino · Pull Request #366 · Breeding-Insight/bi-api

nickpalladino · 2024-06-21T19:38:55Z

Description

Story: BI-2132

Workflow selection code (BI-2122) will be shown as changes in here until it is merged into develop
- Review should focus on stuff from CreateNewExperimentWorkflow process down
- However, there were a few changes from BI-2122 to pass exceptions up
Updated experiment import integration tests
- Only updated tests that were related to creating experiments or environments
- Tests that were updating based on obsunitid were not changed
Checked for ExperimentProcessor work done after refactor branched off develop
- BI-2128, added change
- BI-2065, BI-1993, BI-2104 were already in develop prior to refactor being branched off
Workflow organization
- Code common to the create and append/update workflows that depended on DAOs was broken out into Experiment services
- A number of steps were defined in the create workflow package that correspond to the main steps in the experiment processor code. The main goal was just to remove use of member variables and explicitly pass data between methods for everything other than referenced services, daos, or properties.

Dependencies

bi-web feature/BI-2121

Testing

Given I have navigated to the Experiment Import page
And I have selected the Create New Experiment option from the Workflows select box
And I have an experiment import file with values in the ObsUnitID column
When I click Upload
Then I should see an error notification Error detected in file: <filename>. ObsUnitIDs are detected. Import cannot proceed

Regression testing ideas:

Verify that appending a new environment to an experiment using the create new experiment workflow works

Verify that status updates on the jobs page work

Test different import file error conditions and verify they still occur as before

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: <please include a link to TAF run>

…flow

nickpalladino · 2024-06-21T21:12:49Z

TraitControllerIntegrationTest tests are failing with null program cache, seems like something is messed up with DI. Application stuff related to traits seems to work fine...

I'm seeing the same errors locally on develop but not in github actions. There was a failure in github actions but for something different:

Error: Failures:
Error: SampleSubmissionFileImportTest.importConflictingWellsFailure:389->uploadAndVerifyFailure:517 expected: <1> but was: <2>

mlm483

Some minor changes suggested, otherwise I approve.

src/main/java/org/breedinginsight/brapps/importer/model/imports/DomainImportService.java

src/main/java/org/breedinginsight/brapps/importer/controllers/UploadController.java

src/main/java/org/breedinginsight/brapps/importer/model/imports/DomainImportService.java

mlm483 · 2024-06-27T17:51:56Z

src/main/java/org/breedinginsight/brapps/importer/model/imports/ImportServiceContext.java

+@AllArgsConstructor
+@NoArgsConstructor
+public class ImportServiceContext {
+    private String workflow;


Maybe it would be more clear if this were renamed to workflowName? That way it would be clear that it's not the UUID converted to a string or something else.

What about workflowId? It's called id in the Workflow enum in ExperimentWorkflowNavigator. May make sense to change that in the previous workflowName suggestion as well if you're good with it.

@nickpalladino I think workflowId makes sense for the reason you said. WorkflowName is used for the display name. Also, see my comment in the UploadController about using workflowId there for consistency.

Pushed changes

mlm483 · 2024-06-27T17:54:59Z

...ight/brapps/importer/services/processors/experiment/create/model/ProcessedPhenotypeData.java

+import java.util.List;
+import java.util.Map;
+
+// TODO: move to common higher level location


What does this TODO mean?

Clarified comment, let me know if that makes more sense

Pushed changes

Yeah, makes sense.

mlm483 · 2024-06-27T17:58:21Z

...ght/brapps/importer/services/processors/experiment/create/model/PendingImportObjectData.java

+@ToString
+@AllArgsConstructor
+@NoArgsConstructor
+public class PendingImportObjectData {


Will anything analogous be needed for Germplasm?

I could see doing something similar for other processors in the future to try and clean things up a bit but it's not required.

Ok, just wondering if the class name should be specific to experiment to prepare for that possibility.

It could be, it's already under the experiment package namespace so I guess it's a matter of preference. Could we leave it as-is until the point we may do some Germplasm or other processor work?

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

...pps/importer/services/processors/experiment/create/workflow/CreateNewExperimentWorkflow.java

mlm483 · 2024-06-27T20:26:28Z

...ervices/processors/experiment/create/workflow/steps/PopulateNewPendingImportObjectsStep.java

+    public ProcessedData process(ProcessContext context, ProcessedPhenotypeData phenotypeData)
+            throws MissingRequiredInfoException, UnprocessableEntityException, ApiException {
+
+        Table data = context.getImportContext().getData();
+        ImportUpload upload = context.getImportContext().getUpload();
+        ImportContext importContext = context.getImportContext();
+
+        populatePendingImportObjects(context, phenotypeData);
+
+
+        // TODO: implement
+        return new ProcessedData();
+    }
+


Is the TODO to return data instead of mutating in-place? Or something else?

Ya, I'll clean up the interface and add some comments so it's clear what's actually happening

Pushed changes

Co-authored-by: mlm483 <128052931+mlm483@users.noreply.github.com>

dmeidlin · 2024-07-01T15:18:38Z

src/main/java/org/breedinginsight/brapps/importer/controllers/UploadController.java

+    @ProgramSecured(roles = {ProgramSecuredRole.BREEDER, ProgramSecuredRole.SYSTEM_ADMIN})
+    public HttpResponse<Response<ImportResponse>> commitData(@PathVariable UUID programId, @PathVariable UUID mappingId,
+                                                             @PathVariable String workflow, @PathVariable UUID uploadId,
+                                                             @Body @Nullable Map<String, Object> userInput) {
+        try {


Changing workflow to workflowId would be more consistent with the use here and with the enum.

Pushed changes

dmeidlin · 2024-07-01T15:27:59Z

...dinginsight/brapps/importer/model/imports/experimentObservation/ExperimentImportService.java

+    // TODO: delete processor fields once WorkflowNavigator is used
    @Inject
-    public ExperimentImportService(Provider<ExperimentProcessor> experimentProcessorProvider, Provider<ProcessorManager> processorManagerProvider)
+    public ExperimentImportService(Provider<ExperimentProcessor> experimentProcessorProvider,
+                                   Provider<ProcessorManager> processorManagerProvider,
+                                   ExperimentWorkflowNavigator workflowNavigator)
    {
-        this.experimentProcessorProvider = experimentProcessorProvider;
-        this.processorManagerProvider = processorManagerProvider;
+        super(experimentProcessorProvider, processorManagerProvider, workflowNavigator);
    }


I merged into BI-2134 the changes from BI-2132 for ExperimentFileImportTest and was able to implement this TODO. So, there isn't any additional work required, you can just accept the changes during the merge.

dmeidlin · 2024-07-01T15:43:01Z

...edinginsight/brapps/importer/services/processors/experiment/ExperimentWorkflowNavigator.java

+
+    @Override
+    public List<ImportWorkflow> getWorkflows() throws Exception {
+        // Each workflow returns in the field workflow the metadata about the workflow that processed the import context.
+        // Loop over all workflows, processing a null context, to collect just the metadata


The throws Exception doesn't apply here since there isn't a scenario where getWorkflows() would ever pass up an exception from a lower level; it's only here because throws Exception was added to the Workflow#process signature. See my comments in BI-2134 on using the caughtException field instead.

The throws Exception was added there because ExperimentWorkflowNavigator::getWorkflows calls process on the ExperimentWorkflow and the process method in CreateNewExperimentWorkflow can throw exceptions. The particular code path for returning workflows wouldn't but the method in general can. I'll look at the BI-2134 stuff.

dmeidlin · 2024-07-01T16:21:01Z

.../org/breedinginsight/brapps/importer/services/processors/experiment/ExperimentUtilities.java

+    public static void validateObservationValue(Trait variable, String value,
+                                          String columnHeader, ValidationErrors validationErrors, int row) {
+        if (StringUtils.isBlank(value)) {
+            log.debug(String.format("skipping validation of observation because there is no value.\n\tvariable: %s\n\trow: %d", variable.getObservationVariableName(), row));
+            return;
+        }
+
+        if (isNAObservation(value)) {
+            log.debug(String.format("skipping validation of observation because it is NA.\n\tvariable: %s\n\trow: %d", variable.getObservationVariableName(), row));


In BI-2134, I put these validations into instances of the ObservationValidator functional interface with FieldValidator acting as the primary. The package is at the service level, so it can be used for both workflows.

I created a card BI-2230 to do cleanup and reduce code duplication between workflows to limit the scope of changes in this PR.

dmeidlin · 2024-07-01T19:51:36Z

...pps/importer/services/processors/experiment/create/workflow/CreateNewExperimentWorkflow.java

+        // TODO: unify usage of single import context type throughout
+        ImportContext importContext = ImportContext.from(context);


Agree. I suggest for ImportServiceContext renaming workflow to workflowId, brapiImports to importRows, and adding the Map<Integer, PendingImport> mappedBrAPIImport field. Then eliminating the ImportContext class.

I created a card BI-2230 to do cleanup and reduce code duplication between workflows to limit the scope of changes in this PR.

dmeidlin · 2024-07-01T20:13:10Z

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

+        newExperimentWorkflowId = getNewExperimentWorkflowId();
+    }
+
+    /**
+     * TODO: assumes new workflow is first in list, doesn't look at position property, would be more robust to
+     * look at that instead of assuming order
+     * @return
+     */
+    public String getNewExperimentWorkflowId() {


I've merged this into BI-2134 and renamed to ExperimentFileImportTest#getExperimentWorkflowId with an integer argument for selecting the workflow id from the list fetched by getWorkflows().

dmeidlin · 2024-07-02T18:21:18Z

...pps/importer/services/processors/experiment/create/workflow/CreateNewExperimentWorkflow.java

+        // Start processing the import...
+        ImportPreviewResponse response = runWorkflow(importContext);
+
+        result = Optional.of(ImportWorkflowResult.builder()
+                .workflow(workflow)
+                .importPreviewResponse(Optional.of(response))
+                .build());


When merging with BI-2134, just wrap the runWorkflow() call in a try-catch as below, and any exceptions will be passed and handled by the higher levels as usual. No other changes are needed.

Suggested change

// Start processing the import...

ImportPreviewResponse response = runWorkflow(importContext);

result = Optional.of(ImportWorkflowResult.builder()

.workflow(workflow)

.importPreviewResponse(Optional.of(response))

.build());

// Start processing the import...

try {

ImportPreviewResponse successResponse;

successResponse = runWorkflow(importContext);

result.ifPresent(importWorkflowResult -> importWorkflowResult.setImportPreviewResponse(Optional.of(successResponse)));

} catch (Exception e) {

result.ifPresent(importWorkflowResult -> importWorkflowResult.setCaughtException(Optional.of(e)));

}

return result;

Thanks, just about have it done. Need to remove Exception stuff on method signatures.

Pushed changes

nickpalladino and others added 30 commits May 6, 2024 17:07

Added db migration to create workflows

314d0b1

Added bean mapping for factory producing appropriate workflow

8fc2f31

Added endpoint for retrieving workflows for given mapping id

4b61708

Simplify record mapping

c1fe8fa

Pass workflow id through to import services

3ca1a30

Add workflow factory to create workflows from ids

c8e0bfe

Add missing file header

4391479

Add bean to import mapping workflow model

87ffdb5

Added position column to workflows for explicit ordering

1c1df39

Added additional workflow skeletons

36944a5

Clean up factory code

7a9af10

Merge branch 'develop' into feature/BI-2122

09b9133

Moved create workflow work to own branch and rearranged things a little

0585568

Added file headers and dynamic column parser

777b7e2

create micronaut composite pattern classes for experiment import work…

5564ca1

…flow

create FileImportService method for GET workflows

f726611

delete unused classes

f6952dd

clean up and renaming

5e31aa6

add overrides to import services

708793d

create ExperimentWorkflow interface that extends Workflow interface

68bdacb

create interfaces for germplasm and sample workflows

0e022f1

create interfaces

f2a1034

create DomainImportService abstract class

402eb5d

Shared services work in progress

c5dd139

Renamed common services

8825462

Simplified workflow step calling

2b0256d

change Workflow field names

bce2c7f

use processorManager until Workflow business logic in place

6060fcc

fix workflow navigator injection

90f478b

set the order field for workflows

62cf9ac

nickpalladino added 2 commits June 21, 2024 16:42

Removed navigator stuff handled in domain import service

d11ef3c

Removed some development comments

eebb493

nickpalladino requested review from a team, davedrp, dmeidlin and mlm483 and removed request for a team and davedrp June 21, 2024 20:50

mlm483 approved these changes Jun 27, 2024

View reviewed changes

nickpalladino and others added 6 commits July 1, 2024 09:49

Reference enum for new-experiment

b5dc3e8

Change variable name to reflect string type

2bbf7f3

Co-authored-by: mlm483 <128052931+mlm483@users.noreply.github.com>

Clarified comment

ccc1414

Clarified comment

7a35165

Removed old comment

aa06c6e

Updated doc comment

dd6f348

dmeidlin reviewed Jul 1, 2024

View reviewed changes

Cleaned up ProcessedData code

a6c3590

dmeidlin reviewed Jul 1, 2024

View reviewed changes

Changed from workflow to workflowId

1a6de32

dmeidlin reviewed Jul 2, 2024

View reviewed changes

nickpalladino added 2 commits July 2, 2024 15:20

Change exception handling

a488612

Merge branch 'develop' into feature/BI-2132-A

9b04d44

nickpalladino requested a review from dmeidlin July 2, 2024 20:34

dmeidlin approved these changes Jul 2, 2024

View reviewed changes

nickpalladino merged commit c9ed7d0 into develop Jul 24, 2024

		// TODO: unify usage of single import context type throughout
		ImportContext importContext = ImportContext.from(context);

Conversation

nickpalladino commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Testing

Checklist:

Uh oh!

nickpalladino commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlm483 left a comment

Choose a reason for hiding this comment

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!

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!

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

dmeidlin Jul 1, 2024 • 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!

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!

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!

Reviewers

Assignees

nickpalladino commented Jun 21, 2024 •

edited

Loading

nickpalladino commented Jun 21, 2024 •

edited

Loading

dmeidlin Jul 1, 2024 •

edited

Loading