Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug/bi 1852 #294

Merged
merged 8 commits into from
Oct 3, 2023
Merged

Bug/bi 1852 #294

merged 8 commits into from
Oct 3, 2023

Conversation

davedrp
Copy link
Contributor

@davedrp davedrp commented Sep 26, 2023 •
edited
Loading

Description

Story: BI-1852 - Duplicate breeding method can be created

Definition: A Breeding Method is a 'duplicate' if: there already exist a breeding method with the same name or abbreviation

Dependencies

bi-web: bug/BI-1852

Testing

Set up

  1. Select a program for which the current user is a breeder
  2. Go to the Program Administration page
  3. Select the Breeding Methods tab

TEST 1 (create new breeding method)

  1. Select the Create Breeding Method button
  2. Create a new breeding method with the same name or abbreviation as an existing breeding method.

EXPECTED RESULT

  • An Error banner with the text:
    -- A breeding method with the name '<name>' is already defined in the system.
    or
    -- A breeding method with the abbreviation '<abbreviation>' is already defined in the system.
  • No new breeding method was created.
  1. Select the Create Breeding Method button
  2. Create a new breeding method with a unique name and abbreviation.

EXPECTED RESULT

  • A Success banner is displayed
  • New breeding method was created and displayed.

TEST 2 (edit existing breeding method)

  1. Edit an existing program breeding method.
  2. Modify the name or abbreviation fields to duplicate an existing name or abbreviation .
  3. Select the Save button

EXPECTED RESULT

  • An Error banner with the text:
    -- A breeding method with the name '<name>' is already defined in the system.
    or
    -- A breeding method with the abbreviation '<abbreviation>' is already defined in the system.
  • Changes were not saved.
  1. Edit an existing program breeding method.
  2. Modify the name or abbreviation fields be new unique (non-duplicate) values.
  3. Select the Save button

EXPECTED RESULT

  • A Success banner is displayed
  • Modified data is listed.

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, timparsons and mlm483 and removed request for a team September 26, 2023 19:25
@github-actions github-actions bot added the bug Something isn't working label Sep 26, 2023
@davedrp davedrp marked this pull request as ready for review September 26, 2023 20:18
timparsons
timparsons requested changes Sep 27, 2023
View reviewed changes
src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java Outdated
&& StringUtils.isNotBlank(method.getAbbreviation())
&& StringUtils.isNotBlank(method.getCategory())
&& StringUtils.isNotBlank(method.getGeneticDiversity());
boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add the private modifier to this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicitly declared the method 'protected'. Added comment 'protected' instead of 'private' so it is accessible to unit test.

src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java Outdated
|| StringUtils.isBlank(method.getCategory())
|| StringUtils.isBlank(method.getGeneticDiversity());
}
boolean isDuplicateMethodNameFoundOnList(ProgramBreedingMethodEntity method, List<ProgramBreedingMethodEntity> programBreedingMethodEntityList) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add the private modifier to this method

src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java
Comment on lines +163 to +165
else if(testMethod.getName()==null && method.getName()==null ){
isDup = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this not needed because a validation error would have already been thrown for the name not being supplied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left it as is to be extra-safe.

src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java
Comment on lines +179 to +181
else if(testMethod.getAbbreviation()==null && method.getAbbreviation()==null ){
isDup = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this not needed because a validation error would have already been thrown for the abbreviation not being supplied?

mlm483
mlm483 requested changes Sep 28, 2023
View reviewed changes
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.

Functionality is good, requested changes around specificity of error messages and some more pedantic code quality things.

src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java Outdated Show resolved Hide resolved
src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java Outdated Show resolved Hide resolved
src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java Outdated Show resolved Hide resolved
src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java Outdated Show resolved Hide resolved
src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java Outdated Show resolved Hide resolved
src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java Outdated Show resolved Hide resolved
src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java Outdated Show resolved Hide resolved
src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java Outdated Show resolved Hide resolved
src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java Outdated Show resolved Hide resolved
@davedrp davedrp closed this Oct 3, 2023
@davedrp
Copy link
Contributor Author

davedrp commented Oct 3, 2023

the branch did not get merged to develop

@davedrp davedrp reopened this Oct 3, 2023
@davedrp davedrp merged commit 7f16bf3 into develop Oct 3, 2023
2 checks passed
@davedrp davedrp deleted the bug/BI-1852 branch October 3, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlm483 mlm483 mlm483 approved these changes

@timparsons timparsons timparsons approved these changes

Assignees

@timparsons timparsons

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davedrp @timparsons @mlm483