Skip to content

[BI-1420] Add synonyms to germplasm import and export#180

Merged
ctucker3 merged 4 commits intodevelopfrom
feature/BI-1420
Jun 9, 2022
Merged

[BI-1420] Add synonyms to germplasm import and export#180
ctucker3 merged 4 commits intodevelopfrom
feature/BI-1420

Conversation

@ctucker3
Copy link
Contributor

@ctucker3 ctucker3 commented Apr 19, 2022 •
edited
Loading

Description

Story: https://breedinginsight.atlassian.net/jira/software/c/projects/BI/boards/1?modal=detail&selectedIssue=BI-1420

  • Added synonyms to germplasm import
  • Added synonyms to germplasm export

Dependencies

Testing

  • Download the new template
  • Try importing germplasm with synonyms
  • Try importing germplasm without synonyms
  • Make sure synonyms show up on the germplasm details page and exported excel file.

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: https://github.com/Breeding-Insight/taf/actions/runs/2202653395

@ctucker3 ctucker3 marked this pull request as ready for review April 21, 2022 15:04
@ctucker3 ctucker3 requested review from a team, HMS17 and nickpalladino and removed request for a team April 22, 2022 13:06
nickpalladino
nickpalladino reviewed May 19, 2022
View reviewed changes
src/main/java/org/breedinginsight/brapps/importer/model/base/Germplasm.java
if (synonyms != null) {
List<String> synonyms = Arrays.asList(blobSynonyms.split(";")).stream()
.map(synonym -> synonym.strip())
.distinct()
Copy link
Member

@nickpalladino nickpalladino May 19, 2022

Choose a reason for hiding this comment

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

Is silently dropping duplicates what is desired? Also synonyms are currently only removed as duplicate if case is the same. Is that what is wanted? Can also have the same synonyms for different germplasm (not including the key), not sure if that matters.

Copy link
Member

@timparsons timparsons May 23, 2022

Choose a reason for hiding this comment

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

Relaying these questions to @shawnyarnes

Copy link
Contributor Author

@ctucker3 ctucker3 May 31, 2022

Choose a reason for hiding this comment

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

Shawn added comments to the JIRA card:

https://breedinginsight.atlassian.net/browse/BI-1420

Looks like duplicate synonyms for a single germplasm are supposed to be dropped. But, Shawn is requesting a warning message. Let's talk about this because it will be a decent to large amount of extra work to add on the warning messages because we don't have a great way to handle this kind of flow currently.

@timparsons timparsons removed their request for review June 3, 2022 15:53
@ctucker3 ctucker3 merged commit c3547cb into develop Jun 9, 2022
@ctucker3 ctucker3 deleted the feature/BI-1420 branch June 9, 2022 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timparsons timparsons timparsons left review comments

@nickpalladino nickpalladino nickpalladino approved these changes

@HMS17 HMS17 HMS17 approved these changes

Assignees

@nickpalladino nickpalladino

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ctucker3 @timparsons @nickpalladino @HMS17