[BI-1818] View exp: Obs dataset tab, details, and summary stats by dmeidlin · Pull Request #268 · Breeding-Insight/bi-api

dmeidlin · 2023-06-22T18:45:53Z

Description

Story: BI-1818

Created a new controller named ListController within the org.breedinginsight.brapi.v2 package

Moved the getLists method from BrAPIV2Controller to this class
Updated ListQuery to include externalReferenceId and externalReferenceSource
Updated getLists to handle a listType of observationVariables and to pass-through externalReferenceId and externalReferenceSource, stripping the program key from the listName for each list that is returned

Created a Dataset model within the org.breedinginsight.model package, with fields:

experimentId - String
additionalInfo - JsonObject
data - List
observationUnits - List
observationVariables - List

In ExperimentController created a method to fetch a dataset’s data for an experiment
method: GET
url: /${micronaut.bi.api.version}/programs/{programId}/experiments/{experimentId}/datasets/{datasetId}?stats=[true|false]
the result field of the response is of typeDataset
In BrAPITrialService created a method to fetch a dataset’s data for an experiment parameters

programId - UUID
experimentId - UUID
datasetId - UUID
includeStats - boolean
returns → Dataset

Dependencies

these backend changes are a dependency for BI-1645

Testing

Verify that the germplam lists table still display as expected and clicking on details displays the list as expected.
Import an experiment with observation variables included as columns in the import. Add observation data for some of the rows of the import, but leave some rows empty, without data.
Using a client like curl, GET http:///v1/programs/7c0d1523-0cc7-4a7f-b262-3e81746df5e7/brapi/v2/lists?externalReferenceSource=dataset&externalReferenceId=82d55336-0856-4fe2-a33d-a711a5af6bfc&listType=observationVariables, and verify the list in the response contains the observation variables used in the experiment import.
Using curl, GET http:///v1/programs/7c0d1523-0cc7-4a7f-b262-3e81746df5e7/experiments/bdb3bd47-020a-459c-a254-5a0e062a33c2/dataset/82d55336-0856-4fe2-a33d-a711a5af6bfc?stats=true, and verify the response contains an entity of Dataset with the imported observations.
Compare the statistics in the additionalInfo field of the response dataset with the experiment import and confirm the counts are accurate.

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>

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

timparsons · 2023-06-26T15:17:39Z

src/main/java/org/breedinginsight/brapi/v2/services/BrAPIListService.java

+            throw new DoesNotExistException("list not returned from BrAPI service");
+        }
+
+        for (BrAPIListSummary list: lists) {


In this for loop, I think you should also ensure that the program ID matches the program.id value that was passed in to the method.

ok, that check was added

timparsons · 2023-06-26T15:20:13Z

src/main/java/org/breedinginsight/brapi/v2/model/request/query/ListQuery.java

    private String name;
    private String description;
    private String size;
+    private ExternalReferenceSource externalReferenceSource;


I don't think this should be the enum that we have internally, but should be a String. This would make the query params BrAPI compliant

ok, made the change

timparsons · 2023-06-26T15:26:02Z

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

+            if (type == null) {
+                type = BrAPIListTypes.GERMPLASM;
+            }
+            ExternalReferenceSource source = queryParams.getExternalReferenceSource() == null ?


This should be a String instead of the BI enum

timparsons · 2023-06-26T15:32:40Z

src/main/java/org/breedinginsight/brapi/v2/services/BrAPIListService.java

+            ExternalReferenceSource xrefSource,
+            UUID xrefId,


I think both of these fields should be of type String

ok, made the change

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

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

timparsons · 2023-07-11T17:26:20Z

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

+                type = BrAPIListTypes.GERMPLASM;
+            }
+            String source = queryParams.getExternalReferenceSource() == null ?
+                    ExternalReferenceSource.PROGRAMS.getName() : queryParams.getExternalReferenceSource();


if the externalReferenceSource is null, then the source value will be "programs". I think the default code should be referenceSource + ExternalReferenceSource.PROGRAMS.getName()

timparsons · 2023-07-11T17:28:14Z

src/main/java/org/breedinginsight/brapi/v2/services/BrAPIListService.java

+        List<BrAPIListSummary> lists = listDAO.getListByTypeAndExternalRef(
+                type,
+                program.getId(),
+                String.format("%s/%s", referenceSource, xrefSource),


I think this will cause problems. If the xrefSource coming in is "breedinginsight.net/program", then this line will result in a string of "breedinginsight.net/breedinginsight.net/program"

timparsons · 2023-07-12T21:13:22Z

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

+            String source = queryParams.getExternalReferenceSource() == null ?
+                    String.format("%s/%s", referenceSource, ExternalReferenceSource.PROGRAMS.getName()) :
+                    queryParams.getExternalReferenceSource();
+            UUID id = queryParams.getExternalReferenceId() == null || queryParams.getExternalReferenceId().isBlank() ?


I tried testing with only setting the externalReferenceSource query param and got no results back. This line appears to be the reason why. I don't think a default should be set for this (maybe not even for the externalReferenceSource either). Changing this to allow nulls may require changes to downstream code in this call stack

I removed the default settings for the xref params in ListController, added checks in BrAPIListService so that the a search request could be formed from the params, and added BrAPIListDAO::getListsBySearch. The xrefSource and XrefId params can now be set independently and return any lists. The service and DAO are set up to treat listType as an independent search param, too; however, the default value of germplasm is still being set in the controller unitl the existing germplasm code is updated. So, just setting xrefSource=breeding-insight.net/dataset will return no data because a listType=germplasm is also being used in the search.

github-actions bot added the feature label Jun 22, 2023

dmeidlin marked this pull request as ready for review June 23, 2023 18:44

dmeidlin requested review from a team, davedrp and timparsons and removed request for a team June 23, 2023 18:44

dmeidlin assigned timparsons Jun 23, 2023

timparsons requested changes Jun 26, 2023

View reviewed changes

davedrp requested changes Jul 5, 2023

View reviewed changes

src/main/java/org/breedinginsight/brapi/v2/ListController.java Show resolved Hide resolved

src/main/java/org/breedinginsight/brapi/v2/BrAPIV2Controller.java Show resolved Hide resolved

dmeidlin force-pushed the feature/BI-1818 branch from 2af9eee to b3ede66 Compare July 6, 2023 15:28

dmeidlin requested review from davedrp and timparsons July 10, 2023 19:25

davedrp approved these changes Jul 11, 2023

View reviewed changes

timparsons requested changes Jul 11, 2023

View reviewed changes

timparsons requested changes Jul 12, 2023

View reviewed changes

dmeidlin added 13 commits July 18, 2023 13:10

create ListController

efb95c5

Update ListQuery to include xrefsource and xrefid

db7f402

move GET /lists from BrAPIV2Controller to ListsController

ec1c8ab

create Dataset class

9d7dfbd

add GET /dataset/{datasetId}{?stats} to ExperimentController

3cb7a95

add method TrialService::getDatasetData

0d9a340

update stats calculation

91aedbf

fix querymapper

f031a95

cleanup code

f74dcdd

respond to comments

271a302

fix program filter in list fetch

3a44782

respond to comment

38bf53e

remove default settings for xref params

84e14c7

dmeidlin force-pushed the feature/BI-1818 branch from 75ff82b to 84e14c7 Compare July 18, 2023 17:10

dmeidlin requested a review from timparsons July 18, 2023 17:26

timparsons approved these changes Jul 18, 2023

View reviewed changes

dmeidlin merged commit 4afac5d into develop Jul 18, 2023

dmeidlin deleted the feature/BI-1818 branch July 18, 2023 18:36

Conversation

dmeidlin commented Jun 22, 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!

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!

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

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmeidlin commented Jun 22, 2023 •

edited

Loading