Skip to content

BI-2100 - BrAPI Hackathon Integration of Helium with DeltaBreed#337

Merged
nickpalladino merged 17 commits intodevelopfrom
helium-integration
May 3, 2024
Merged

BI-2100 - BrAPI Hackathon Integration of Helium with DeltaBreed#337
nickpalladino merged 17 commits intodevelopfrom
helium-integration

Conversation

@nickpalladino
Copy link
Member

@nickpalladino nickpalladino commented Mar 5, 2024

Description

Story: https://breedinginsight.atlassian.net/browse/BI-2100?atlOrigin=eyJpIjoiNzcxNDBiMTNmYWM2NGNjMmJiYzlmZGU0NzI3OTI5ZGMiLCJwIjoiaiJ9

Dependencies

  • helium-integration branch of brapi server
  • helium-integration branch of brapi client (may not actually be needed since switched from using pedigree search to get)

Testing

  • See JIRA card acceptance criteria for testing procedure

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>

@nickpalladino nickpalladino changed the title Helium BrAPI integration BI-2100 - BrAPI Hackathon Integration of Helium with DeltaBreed Apr 19, 2024
@nickpalladino nickpalladino marked this pull request as ready for review April 19, 2024 21:04
@nickpalladino nickpalladino requested review from a team, davedrp and dmeidlin and removed request for a team April 19, 2024 21:06
@Controller("/${micronaut.bi.api.version}/programs/{programId}" + BrapiVersion.BRAPI_V2)
@Secured(SecurityRule.IS_AUTHENTICATED)
public class BrAPIPedigreeController {
private final String referenceSource;
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see this being used, except in the constructor). Could it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching that, removed and pushed changes.

* @return A List of BrAPIPedigreeNode objects that match the search criteria.
* @throws ApiException If an error occurs while searching for pedigree nodes.
*/
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is commenting-out everything down to line-183. Is that the intention? If so, should those lines be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I commented it out but left it here is case we want to add the search endpoints at some point we have some work done. I added a comment in the method documentation stating this and pushed changes.

* @param pedigreeNodes The list of pedigree nodes.
* @param programKey The program key to be removed.
*/
private void stripProgramKeys(List<BrAPIPedigreeNode> pedigreeNodes, String programKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Method is never used

Copy link
Member Author

Choose a reason for hiding this comment

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

Added justification for keeping it in the method documentation and pushed changes.

pedigreeNodes.forEach(node -> {
node.setGermplasmName(Utilities.removeProgramKeyAnyAccession(node.getGermplasmName(), programKey));
// TODO: pedigree stripping not working right
//node.setPedigreeString(Utilities.removeProgramKeyAnyAccession(node.getPedigreeString(), programKey));
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left that commented line in with a TODO to fix in the future as that was what was desired but it wasn't working correctly so needs to be looked into more if we decide to strip the program keys in the future.

@nickpalladino nickpalladino merged commit 2be3bf1 into develop May 3, 2024
@nickpalladino nickpalladino deleted the helium-integration branch May 3, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants