BI-1855 - Create germplasm list not working by nickpalladino · Pull Request #281 · Breeding-Insight/bi-api

nickpalladino · 2023-08-08T15:24:55Z

Description

Story: BI-1855

Germplasm update logic changed:
Allow empty pedigree values to enable creating a new germplasm list without requiring the original pedigree values
Only require parent gids to be entered when updating a germplasm pedigree rather than gid and entry number
Various bug fixes found when testing against Breedbase

Dependencies

release/0.8 bi-web

Testing

Test different combinations of updating pedigrees, synonyms, and not allowed pedigree changes
Test files used in development, number indicates order ascending: 1855-tep46-np398.zip

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>

mlm483

Getting a NullPointerException. To reproduce, try to upload the attached germplasm file. It works on develop but on bug/BI-1855-tep46 I get a NullPointerException during the "Upload and Validate" process.

100 Germplasm.xlsx

davedrp · 2023-08-15T17:18:48Z

Passed Developer testing!

senario 1: has existing pedigree and file pedigree is different and not empty.
result: showed error.
PASSED

senario 2: no existing pedigree and file different pedigree.
result: no error.
PASSED

senario 3: existing pedigree and file pedigree samee.
result: no error.
PASSED

senario 4: existing pedigree and file pedigree empty.
result: no error.
PASSED

davedrp

The code and logic seem great to me. I left some comments on minor cosmetic changes.

src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java

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

nickpalladino · 2023-08-15T20:58:11Z

Getting a NullPointerException. To reproduce, try to upload the attached germplasm file. It works on develop but on bug/BI-1855-tep46 I get a NullPointerException during the "Upload and Validate" process.

100 Germplasm.xlsx

Pushed a fix for this

mlm483

Thanks for the test files, super helpful!

The file 3_new list-wrongped - fail.xls is expected to produce an error, but I noticed it says "See details below" when there are no details to be seen below. Possibly out of scope, but I recommend either (1) providing details in this error case or (2) if it's truly exceptional, removing "See details below". Could make a new story.

FWIW I'm seeing "The following parental GIDs were not found in the database: 5, 6, 7, 8." being sent from the server, so it might be as simple as showing that message on the frontend.

Here's the story I created to refactor processGermplasmForDisplay, BI-1881.

Assuming the missingParentalDbIds and missingDbIds business is sorted out and you feel good about it, I'm OK to merge.

mlm483 · 2023-08-16T14:31:03Z

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

+    // TODO: hack for now, probably should update breedbase
+    // Breedbase will return NA/NA for no pedigree or NA/father, mother/NA
+    // strip NAs before saving RAW_PEDIGREE, if there was a germplasm with name NA it would be in format NA [program key]
+    // so that case should be ok if we just strip NA/NA, NA/, or /NA<\0>
+    private String processBreedbasePedigree(String pedigree) {


Is there a story for the change to BreedBase? I think that story should have removing this code as part of it's acceptance criteria. You could also potentially link to that story in the TODO comment.

I created a story BI-1883 and updated the comment.

mlm483 · 2023-08-16T15:25:18Z

src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIGermplasmDAO.java

+                String pedigree = processBreedbasePedigree(germplasm.getPedigree());
+                additionalInfo.addProperty(BrAPIAdditionalInfoFields.GERMPLASM_RAW_PEDIGREE, pedigree);


Will we want to clean this up eventually once BreedBase is handling pedigree correctly?
Just want to make sure we're documenting future work with Jira stories and TODOs.

Added a comment

mlm483 · 2023-08-16T16:41:15Z

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

+        List<String> missingParentalDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList());
+        List<String> missingDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList());


Given that missingParentalDbIds and missingDbIds contain the same values, the way they are used doesn't make sense. See my comment below.

I looked into this more and did some testing. I ended up changing List<String> missingDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList()); to List<String> missingDbIds = germplasmDBIDs.entrySet().stream().filter(entry -> !entry.getValue()).map(Map.Entry::getKey).collect(Collectors.toList());. The map is setup with values of true for parent gids and false for update gids. So now these lists are filtered appropriately from the map and will have different values.

mlm483 · 2023-08-16T16:44:00Z

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

+        //GID existence check
+        if (missingDbIds.size() > 0) {
+            throw new HttpStatusException(HttpStatus.UNPROCESSABLE_ENTITY,
+                                          String.format(missingDbIdsMsg,


This code will never be reached because if missingDbIds.size() > 0, missingParentalDbIds.size() > 0 as well, because they have the same values and are mutated in the same ways, so the exception will always be thrown above for missingParentalDbIds.

With the change that has been made this has been resolved.

nickpalladino · 2023-08-17T14:50:03Z

Thanks for the test files, super helpful!

The file 3_new list-wrongped - fail.xls is expected to produce an error, but I noticed it says "See details below" when there are no details to be seen below. Possibly out of scope, but I recommend either (1) providing details in this error case or (2) if it's truly exceptional, removing "See details below". Could make a new story.

FWIW I'm seeing "The following parental GIDs were not found in the database: 5, 6, 7, 8." being sent from the server, so it might be as simple as showing that message on the frontend.

Here's the story I created to refactor processGermplasmForDisplay, BI-1881.

Assuming the missingParentalDbIds and missingDbIds business is sorted out and you feel good about it, I'm OK to merge.

The notification message displays on release/0.8 as it is sent from bi-api so I think we can hold off on this until things are merged into develop and have a new card to address it at that point. Created a card: BI-1882

nickpalladino and others added 5 commits August 2, 2023 09:54

Updated logic to allow empty pedigrees and only require gids for updates

fc23e97

Logic updates

153d204

refactoring some of the logic in GermplasmProcessor

bc885a6

Fix missing gid message formatting

ce4bee7

Updated hasPedigree to check for unknown being true, not just keys

f22046a

github-actions bot added the bug Something isn't working label Aug 8, 2023

Update to make sure existing brapi object is populated in all cases

4f43283

nickpalladino changed the title ~~BI-1855 2~~ BI-1855 - Create germplasm list not working Aug 8, 2023

nickpalladino added 4 commits August 9, 2023 11:04

Workaround for breedbase pedigree strings

4c74c6d

Update breedbase string logic

1f4291f

Fixes for issues found testing with Breedbase

09ccfd5

Cleanup some comments

2329ca9

nickpalladino requested review from davedrp and mlm483 August 10, 2023 17:28

mlm483 requested changes Aug 14, 2023

View reviewed changes

davedrp requested changes Aug 15, 2023

View reviewed changes

src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java Outdated Show resolved Hide resolved

src/main/java/org/breedinginsight/brapps/importer/services/processors/GermplasmProcessor.java Outdated Show resolved Hide resolved

Null fix

4bd3006

nickpalladino added 2 commits August 16, 2023 09:41

Removed unused import

22bc163

Removed unused method

21eba3d

mlm483 self-requested a review August 16, 2023 14:02

nickpalladino requested a review from davedrp August 16, 2023 14:06

mlm483 approved these changes Aug 16, 2023

View reviewed changes

nickpalladino added 2 commits August 17, 2023 16:55

Added comments

33a38c9

Update logic for filtering missing gids

f0f7ec4

nickpalladino merged commit 93cfd44 into release/0.8 Aug 18, 2023

nickpalladino deleted the bug/BI-1855-tep46 branch August 18, 2023 13:53

		String pedigree = processBreedbasePedigree(germplasm.getPedigree());
		additionalInfo.addProperty(BrAPIAdditionalInfoFields.GERMPLASM_RAW_PEDIGREE, pedigree);

		List<String> missingParentalDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList());
		List<String> missingDbIds = germplasmDBIDs.entrySet().stream().filter(Map.Entry::getValue).map(Map.Entry::getKey).collect(Collectors.toList());

Conversation

nickpalladino commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Testing

Checklist:

Uh oh!

mlm483 left a comment

Choose a reason for hiding this comment

Uh oh!

davedrp commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davedrp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickpalladino commented Aug 15, 2023

Uh oh!

mlm483 left a comment

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!

nickpalladino commented Aug 17, 2023 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nickpalladino commented Aug 8, 2023 •

edited

Loading

davedrp commented Aug 15, 2023 •

edited

Loading

nickpalladino commented Aug 17, 2023 •

edited by atlassian bot

Loading