Skip to content

BI-1881 (Remove Need to Provide Parents to processGermplasmForDisplay)#288

Closed
davedrp wants to merge 2 commits intodevelopfrom
bug/BI-1881
Closed

BI-1881 (Remove Need to Provide Parents to processGermplasmForDisplay)#288
davedrp wants to merge 2 commits intodevelopfrom
bug/BI-1881

Conversation

@davedrp
Copy link
Contributor

@davedrp davedrp commented Aug 25, 2023

Description

BI-1881 Remove Need to Provide Parents to processGermplasmForDisplay

  • Any call to processGermplasmForDisplay() now will only need to pass in a list of the modified germplasm, not the entire list of germplasm for the program
  • if a parent of a gerplasm is not referenced on the passed-in list of germplasm, then, and only then will the code fetch the entire list of germplasm for the program.

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

@davedrp davedrp requested review from a team, mlm483 and nickpalladino and removed request for a team August 25, 2023 17:37
@github-actions github-actions bot added the bug Something isn't working label Aug 25, 2023
@davedrp davedrp marked this pull request as ready for review August 25, 2023 19:54
Copy link
Contributor

@mlm483 mlm483 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the failures in the TAF run and testing locally, I'm noticing that uploading Germplasm in a program that doesn't have any Germplasm yet fails. It will hang after the user clicks "Upload" or "Confirm", I think depending on when the cache gets hit.

If you want to run the failing TAF scenarios locally, here are the tags.
--tags \"@BI-1805 or @BI-1598 or @BI-1603 or @BI-1775 or @BI-1776 or @BI-1592 or @BI-1501 or @BI-1514 or @BI-1593 or @BI-1588 or @BI-1600\"

return programGermplasmMap;
}

private boolean expand_programGermplasmByFullName(ProgramEntity program, Map<String, BrAPIGermplasm> programGermplasmByFullName) throws ApiException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless the underscore in the name is part of a convention I don't know about, I would remove it.

log.trace("processing germ for display: " + germplasmList);
Map<String, BrAPIGermplasm> programGermplasmByFullName = new HashMap<>();
for (BrAPIGermplasm germplasm: programGermplasm) {
boolean isExpanded_programGermplasmByFullName = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless the underscore in the name is part of a convention I don't know about, I would remove it.
isExpandedProgramGermplasmByFullName would be OK with me, something like isMissingParents would be OK too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

}

private boolean expand_programGermplasmByFullName(ProgramEntity program, Map<String, BrAPIGermplasm> programGermplasmByFullName) throws ApiException {
List<BrAPIGermplasm> allProgramGermplasm = getRawGermplasm(program.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getRawGermplasm hits the cache, calling it from the method processGermplasmForDisplay which is used to populate the cache may be causing the issues I'm seeing locally and in the TAF run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the original intent of this card was to grab the accession numbers from the pedigree string [programKey-accessionNumber] and use that to create the newPedigreeString rather than requiring the parent germplasm objects @timparsons? Although I'm not sure how it would create the name and UUID pedigree strings in that case.

log.trace("processing germ for display: " + germplasmList);
Map<String, BrAPIGermplasm> programGermplasmByFullName = new HashMap<>();
for (BrAPIGermplasm germplasm: programGermplasm) {
boolean isExpanded_programGermplasmByFullName = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

}

private boolean expand_programGermplasmByFullName(ProgramEntity program, Map<String, BrAPIGermplasm> programGermplasmByFullName) throws ApiException {
List<BrAPIGermplasm> allProgramGermplasm = getRawGermplasm(program.getId());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the original intent of this card was to grab the accession numbers from the pedigree string [programKey-accessionNumber] and use that to create the newPedigreeString rather than requiring the parent germplasm objects @timparsons? Although I'm not sure how it would create the name and UUID pedigree strings in that case.

@mlm483
Copy link
Contributor

mlm483 commented Sep 22, 2023

Closing in favor of #290

@mlm483 mlm483 closed this Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants