From 7a28a6084584840bd919d75806a7ce2ad320904e Mon Sep 17 00:00:00 2001 From: David Randolph Phillips Date: Mon, 25 Sep 2023 16:51:31 -0400 Subject: [PATCH 1/7] [BI-1852] check for duplicate breeding methods --- .../controller/BreedingMethodController.java | 4 +- .../impl/BreedingMethodServiceImpl.java | 78 +++++++++++++++---- .../validators/TraitValidatorUnitTest.java | 2 +- 3 files changed, 68 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java index 6e89bd7cd..c9399dfef 100644 --- a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java +++ b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java @@ -64,7 +64,7 @@ public HttpResponse>> getSyst @Post("programs/{programId}/breeding-methods") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse> createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { + public HttpResponse createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { log.debug("Saving new program breeding method"); try { @@ -79,6 +79,8 @@ public HttpResponse> createProgramBreeding response.metadata = metadata; return HttpResponse.ok(response); + } catch (BadRequestException ex) { + return HttpResponse.badRequest(ex.getMessage()); } catch (Exception e) { log.error("Error creating program breeding method", e); throw e; diff --git a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java index 9b5c9fab6..83958ed71 100644 --- a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java +++ b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java @@ -1,6 +1,5 @@ package org.breedinginsight.services.impl; -import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; import org.brapi.client.v2.model.exceptions.ApiException; import org.breedinginsight.brapi.v2.services.BrAPIGermplasmService; @@ -14,8 +13,9 @@ import java.util.*; import java.util.stream.Collectors; +import static java.lang.String.format; + @Singleton -@Slf4j public class BreedingMethodServiceImpl implements BreedingMethodService { private final BreedingMethodDAO breedingMethodDAO; @@ -60,19 +60,21 @@ public List fetchBreedingMethodsInUse(UUID programI } @Override - public ProgramBreedingMethodEntity createBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException, ApiException { - if(!validateBreedingMethod(breedingMethod)) { - throw new BadRequestException("Missing required data"); - } + public ProgramBreedingMethodEntity createBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException { + validate(breedingMethod, programId); return dsl.transactionResult(() -> breedingMethodDAO.createProgramMethod(breedingMethod, programId, userId)); } + private boolean methodAlreadyExist(ProgramBreedingMethodEntity breedingMethod, UUID programId) { + List programMethods = getBreedingMethods(programId); + List systemMethods = getSystemBreedingMethods(); + return isDuplicateMethodFoundAnywhere(breedingMethod, systemMethods, programMethods); + } + @Override public ProgramBreedingMethodEntity updateBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException, ApiException { - if(!validateBreedingMethod(breedingMethod)) { - throw new BadRequestException("Missing required data"); - } + validate(breedingMethod, programId); List inUseMethods = fetchBreedingMethodsInUse(programId); if(inUseMethods.stream().anyMatch(method -> method.getId().equals(breedingMethod.getId()))) { @@ -82,6 +84,16 @@ public ProgramBreedingMethodEntity updateBreedingMethod(ProgramBreedingMethodEnt return dsl.transactionResult(() -> breedingMethodDAO.updateProgramMethod(breedingMethod, programId, userId)); } + private void validate(ProgramBreedingMethodEntity breedingMethod, UUID programId) throws BadRequestException { + if (isMissingRequiredFields(breedingMethod)) { + throw new BadRequestException("Missing required data"); + } + if (methodAlreadyExist(breedingMethod, programId)) { + throw new BadRequestException(format("A method with name:'%s' or abbreviation:'%s already exist", breedingMethod.getName(), breedingMethod.getAbbreviation())); + } + } + + @Override public void enableSystemMethods(List systemBreedingMethods, UUID programId, UUID userId) throws ApiException, BadRequestException { List inUseMethods = fetchBreedingMethodsInUse(programId); @@ -111,10 +123,48 @@ public void deleteBreedingMethod(UUID programId, UUID breedingMethodId) throws A dsl.transaction(() -> breedingMethodDAO.deleteProgramMethod(programId, breedingMethodId)); } - private boolean validateBreedingMethod(ProgramBreedingMethodEntity method) { - return StringUtils.isNotBlank(method.getName()) - && StringUtils.isNotBlank(method.getAbbreviation()) - && StringUtils.isNotBlank(method.getCategory()) - && StringUtils.isNotBlank(method.getGeneticDiversity()); + public boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) { + return StringUtils.isBlank(method.getName()) + || StringUtils.isBlank(method.getAbbreviation()) + || StringUtils.isBlank(method.getCategory()) + || StringUtils.isBlank(method.getGeneticDiversity()); } + + public boolean isDuplicateMethodFoundAnywhere(ProgramBreedingMethodEntity testMethod, List systemBreedingMethodEntityList, List programBreedingMethodEntityList) { + boolean foundDup = isDuplicateMethodFoundOnList(testMethod, systemBreedingMethodEntityList); + if (!foundDup && programBreedingMethodEntityList!=null){ + foundDup = isDuplicateMethodFoundOnList(testMethod, programBreedingMethodEntityList); + } + return foundDup; + } + + private boolean isDuplicateMethodFoundOnList(ProgramBreedingMethodEntity testMethod, List programBreedingMethodEntityList) { + boolean foundDup = false; + for (ProgramBreedingMethodEntity method: programBreedingMethodEntityList) { + if(areMethodsDuplicate(testMethod, method)){ + foundDup = true; + break; + } + } + return foundDup; + } + + + public boolean areMethodsDuplicate(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { + boolean isDup = false; + + if(testMethod.getName()!= null && testMethod.getName().equals(method.getName())){ + isDup = true; + } + else if(testMethod.getAbbreviation()!= null && testMethod.getAbbreviation().equals(method.getAbbreviation())){ + isDup = true; + } + else if(testMethod.getName()==null && method.getName()==null || + testMethod.getAbbreviation()==null && method.getAbbreviation()==null){ + isDup = true; + } + + return isDup; + } + } diff --git a/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java b/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java index f40151f19..cc563772c 100644 --- a/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java +++ b/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java @@ -40,6 +40,7 @@ import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; @TestInstance(TestInstance.Lifecycle.PER_CLASS) public class TraitValidatorUnitTest { @@ -86,7 +87,6 @@ public void missingMethod() { ValidationErrors validationErrors = traitValidatorService.checkRequiredTraitFields(List.of(trait), new TraitValidatorError()); - assertEquals(1, validationErrors.getRowErrors().size(), "Wrong number of row errors returned"); RowValidationErrors rowValidationErrors = validationErrors.getRowErrors().get(0); assertEquals(1, rowValidationErrors.getErrors().size(), "Wrong number of errors for row"); From 24e99f042041c1a2d56c45b04df27ee3cef716ac Mon Sep 17 00:00:00 2001 From: David Randolph Phillips Date: Tue, 26 Sep 2023 12:32:27 -0400 Subject: [PATCH 2/7] [BI-1852] respond with 400 errer with exception's text Add unit test --- .../controller/BreedingMethodController.java | 4 +- .../impl/BreedingMethodServiceImpl.java | 31 ++-- .../impl/BreedingMethodServiceImplTest.java | 135 ++++++++++++++++++ 3 files changed, 151 insertions(+), 19 deletions(-) create mode 100644 src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java diff --git a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java index c9399dfef..eca384476 100644 --- a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java +++ b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java @@ -112,7 +112,7 @@ public HttpResponse>> getProg @Put("programs/{programId}/breeding-methods/{breedingMethodId}") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse> updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { + public HttpResponse updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { log.debug("Saving new program breeding method"); try { @@ -130,6 +130,8 @@ public HttpResponse> updateProgramBreeding response.metadata = metadata; return HttpResponse.ok(response); + } catch (BadRequestException ex) { + return HttpResponse.badRequest(ex.getMessage()); } catch (Exception e) { log.error("Error updating program breeding method", e); throw e; diff --git a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java index 83958ed71..3c523682e 100644 --- a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java +++ b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java @@ -67,19 +67,17 @@ public ProgramBreedingMethodEntity createBreedingMethod(ProgramBreedingMethodEnt } private boolean methodAlreadyExist(ProgramBreedingMethodEntity breedingMethod, UUID programId) { - List programMethods = getBreedingMethods(programId); - List systemMethods = getSystemBreedingMethods(); - return isDuplicateMethodFoundAnywhere(breedingMethod, systemMethods, programMethods); + List programAndSystemMethods = getBreedingMethods(programId); + return isDuplicateMethodFoundOnList(breedingMethod, programAndSystemMethods); } @Override public ProgramBreedingMethodEntity updateBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException, ApiException { - validate(breedingMethod, programId); - List inUseMethods = fetchBreedingMethodsInUse(programId); if(inUseMethods.stream().anyMatch(method -> method.getId().equals(breedingMethod.getId()))) { throw new BadRequestException("Breeding method is not allowed to be edited"); } + validate(breedingMethod, programId); return dsl.transactionResult(() -> breedingMethodDAO.updateProgramMethod(breedingMethod, programId, userId)); } @@ -123,24 +121,16 @@ public void deleteBreedingMethod(UUID programId, UUID breedingMethodId) throws A dsl.transaction(() -> breedingMethodDAO.deleteProgramMethod(programId, breedingMethodId)); } - public boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) { + boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) { return StringUtils.isBlank(method.getName()) || StringUtils.isBlank(method.getAbbreviation()) || StringUtils.isBlank(method.getCategory()) || StringUtils.isBlank(method.getGeneticDiversity()); } - public boolean isDuplicateMethodFoundAnywhere(ProgramBreedingMethodEntity testMethod, List systemBreedingMethodEntityList, List programBreedingMethodEntityList) { - boolean foundDup = isDuplicateMethodFoundOnList(testMethod, systemBreedingMethodEntityList); - if (!foundDup && programBreedingMethodEntityList!=null){ - foundDup = isDuplicateMethodFoundOnList(testMethod, programBreedingMethodEntityList); - } - return foundDup; - } - - private boolean isDuplicateMethodFoundOnList(ProgramBreedingMethodEntity testMethod, List programBreedingMethodEntityList) { + boolean isDuplicateMethodFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { boolean foundDup = false; - for (ProgramBreedingMethodEntity method: programBreedingMethodEntityList) { + for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) { if(areMethodsDuplicate(testMethod, method)){ foundDup = true; break; @@ -150,9 +140,14 @@ private boolean isDuplicateMethodFoundOnList(ProgramBreedingMethodEntity testMet } - public boolean areMethodsDuplicate(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { - boolean isDup = false; + boolean areMethodsDuplicate(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { + // SPECIAL CASE: If the two methods are the same method, then they are not duplicates + if( (testMethod.getId()!=null) && testMethod.getId().equals(method.getId()) ){ + return false; + } + + boolean isDup = false; if(testMethod.getName()!= null && testMethod.getName().equals(method.getName())){ isDup = true; } diff --git a/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java new file mode 100644 index 000000000..9e7ec3c87 --- /dev/null +++ b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java @@ -0,0 +1,135 @@ +package org.breedinginsight.services.impl; + +import lombok.SneakyThrows; +import org.breedinginsight.brapi.v2.services.BrAPIGermplasmService; +import org.breedinginsight.dao.db.tables.pojos.ProgramBreedingMethodEntity; +import org.breedinginsight.daos.BreedingMethodDAO; +import org.jooq.DSLContext; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; + +import java.util.ArrayList; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; + +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class BreedingMethodServiceImplTest { + BreedingMethodServiceImpl breedingMethodService; + BreedingMethodDAO breedingMethodDAO = null; + BrAPIGermplasmService germplasmService = null; + DSLContext dsl = null; + + @BeforeAll + public void setup() { + breedingMethodService = new BreedingMethodServiceImpl(breedingMethodDAO, germplasmService, dsl); + } + + @Test + public void hasRequiredFields(){ + ProgramBreedingMethodEntity method= new ProgramBreedingMethodEntity(); + assertTrue(breedingMethodService.isMissingRequiredFields(method),"the method has blanks"); + + method.setName("Dave"); + method.setAbbreviation("DRP"); + method.setCategory("human"); + method.setGeneticDiversity("none"); + assertFalse(breedingMethodService.isMissingRequiredFields(method),"the method has all required fields"); + } + + @Test + @SneakyThrows + public void isDupOfProgramMethod() { + List programBreedingMethodEntityList = new ArrayList<>(); + programBreedingMethodEntityList.add(makeMethod("1")); + programBreedingMethodEntityList.add(makeMethod("2")); + + boolean isDup = breedingMethodService.isDuplicateMethodFoundOnList(makeMethod("1"), programBreedingMethodEntityList); + assertTrue(isDup, "Duplicate found in list."); + } + + @Test + @SneakyThrows + public void isNotADupMethod() { + List programBreedingMethodEntityList = new ArrayList<>(); + programBreedingMethodEntityList.add(makeMethod("1")); + programBreedingMethodEntityList.add(makeMethod("2")); + + + ProgramBreedingMethodEntity methodUnique = makeMethod("unique");; + boolean isDup = breedingMethodService.isDuplicateMethodFoundOnList(methodUnique, programBreedingMethodEntityList); + assertFalse(isDup, "No duplicates method found."); + + } + @Test + @SneakyThrows + public void methodNameEqual() { + ProgramBreedingMethodEntity method = makeMethod("ABC"); + ProgramBreedingMethodEntity testMethod = makeMethod("ABC"); + testMethod.setAbbreviation("misatch Abbreviation"); + assertTrue( + breedingMethodService.areMethodsDuplicate(testMethod, method), + "Method Names are Equal" + ); + assertTrue( + breedingMethodService.areMethodsDuplicate(method, testMethod), + "Method Names are Equal (switched order)" + ); + + testMethod.setName("misatch Name"); + assertFalse( + breedingMethodService.areMethodsDuplicate(testMethod, method), + "Method Names are not Equal" + ); + testMethod.setName(null); + assertFalse( + breedingMethodService.areMethodsDuplicate(testMethod, method), + "Method Names are not Equal (one is null)" + ); + + testMethod.setName("name"); + method.setName(null); + assertFalse( + breedingMethodService.areMethodsDuplicate(testMethod, method), + "Method Names are not Equal (the other is null)" + ); + + } + + @Test + @SneakyThrows + public void methodAbbreviationEqual() { + ProgramBreedingMethodEntity method = makeMethod("ABC"); + ProgramBreedingMethodEntity testMethod = makeMethod("ABC"); + testMethod.setName("misatch Name"); + assertTrue( + breedingMethodService.areMethodsDuplicate(testMethod, method), + "Method Abbreviations are Equal" + ); + assertTrue( + breedingMethodService.areMethodsDuplicate(method, testMethod), + "Method Abbreviations are Equal (switched order)" + ); + + testMethod.setAbbreviation("misatch Abbreviation"); + assertFalse( + breedingMethodService.areMethodsDuplicate(method, testMethod), + "Method Abbreviations are not equal." + ); + testMethod.setAbbreviation(null); + assertFalse( + breedingMethodService.areMethodsDuplicate(method, testMethod), + "Method Abbreviations are not equal (one is null)." + ); + } + + // Helper Methods // + private ProgramBreedingMethodEntity makeMethod(String suffix){ + ProgramBreedingMethodEntity method = new ProgramBreedingMethodEntity(); + method.setName("Name" + suffix); + method.setAbbreviation("Abbreviation" + suffix); + return method; + } + +} From 3212e8af783a6c7f0dffb47faec510f7ee40b2aa Mon Sep 17 00:00:00 2001 From: David Randolph Phillips Date: Tue, 26 Sep 2023 12:45:10 -0400 Subject: [PATCH 3/7] [BI-1852]fixed some existing 'Problems' in BreedingMethodController.java --- .../api/v1/controller/BreedingMethodController.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java index eca384476..7b3fc2bb5 100644 --- a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java +++ b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java @@ -7,7 +7,6 @@ import io.micronaut.security.annotation.Secured; import io.micronaut.security.rules.SecurityRule; import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.StringUtils; import org.brapi.client.v2.model.exceptions.ApiException; import org.breedinginsight.api.auth.*; import org.breedinginsight.api.model.v1.response.DataResponse; @@ -17,18 +16,14 @@ import org.breedinginsight.api.model.v1.response.metadata.Status; import org.breedinginsight.api.model.v1.response.metadata.StatusCode; import org.breedinginsight.api.v1.controller.metadata.AddMetadata; -import org.breedinginsight.dao.db.tables.pojos.BreedingMethodEntity; import org.breedinginsight.dao.db.tables.pojos.ProgramBreedingMethodEntity; -import org.breedinginsight.daos.BreedingMethodDAO; import org.breedinginsight.services.BreedingMethodService; import org.breedinginsight.services.exceptions.BadRequestException; import javax.inject.Inject; -import javax.validation.Valid; import java.util.ArrayList; import java.util.List; import java.util.UUID; -import java.util.stream.Collectors; @Slf4j @Controller("/${micronaut.bi.api.version}") @@ -64,7 +59,7 @@ public HttpResponse>> getSyst @Post("programs/{programId}/breeding-methods") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { + public HttpResponse createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException { log.debug("Saving new program breeding method"); try { @@ -112,7 +107,7 @@ public HttpResponse>> getProg @Put("programs/{programId}/breeding-methods/{breedingMethodId}") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws BadRequestException, ApiException { + public HttpResponse updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException { log.debug("Saving new program breeding method"); try { From 177eeb30c098b05a266d2a2f842384debf83e99e Mon Sep 17 00:00:00 2001 From: David Randolph Phillips Date: Tue, 26 Sep 2023 15:29:12 -0400 Subject: [PATCH 4/7] [BI-1852] fixed error message --- .../services/impl/BreedingMethodServiceImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java index 3c523682e..38c63b1ae 100644 --- a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java +++ b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java @@ -87,7 +87,7 @@ private void validate(ProgramBreedingMethodEntity breedingMethod, UUID programId throw new BadRequestException("Missing required data"); } if (methodAlreadyExist(breedingMethod, programId)) { - throw new BadRequestException(format("A method with name:'%s' or abbreviation:'%s already exist", breedingMethod.getName(), breedingMethod.getAbbreviation())); + throw new BadRequestException(format("A method with name:'%s' or abbreviation:'%s' already exist.", breedingMethod.getName(), breedingMethod.getAbbreviation())); } } From cc4ddf67a3e406e492ce46b202305da76d87c79c Mon Sep 17 00:00:00 2001 From: David Randolph Phillips Date: Wed, 27 Sep 2023 12:14:42 -0400 Subject: [PATCH 5/7] [BI-1852] Improved error messages. fixed bug. --- .../impl/BreedingMethodServiceImpl.java | 52 +++++++---- .../impl/BreedingMethodServiceImplTest.java | 91 ++++++++++++------- 2 files changed, 92 insertions(+), 51 deletions(-) diff --git a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java index 38c63b1ae..239a3115d 100644 --- a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java +++ b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java @@ -66,10 +66,6 @@ public ProgramBreedingMethodEntity createBreedingMethod(ProgramBreedingMethodEnt return dsl.transactionResult(() -> breedingMethodDAO.createProgramMethod(breedingMethod, programId, userId)); } - private boolean methodAlreadyExist(ProgramBreedingMethodEntity breedingMethod, UUID programId) { - List programAndSystemMethods = getBreedingMethods(programId); - return isDuplicateMethodFoundOnList(breedingMethod, programAndSystemMethods); - } @Override public ProgramBreedingMethodEntity updateBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException, ApiException { @@ -86,8 +82,13 @@ private void validate(ProgramBreedingMethodEntity breedingMethod, UUID programId if (isMissingRequiredFields(breedingMethod)) { throw new BadRequestException("Missing required data"); } - if (methodAlreadyExist(breedingMethod, programId)) { - throw new BadRequestException(format("A method with name:'%s' or abbreviation:'%s' already exist.", breedingMethod.getName(), breedingMethod.getAbbreviation())); + + List programAndSystemMethods = getBreedingMethods(programId); + if( isDuplicateMethodNameFoundOnList(breedingMethod, programAndSystemMethods)){ + throw new BadRequestException(format("'%s' is already defined in the system.", breedingMethod.getName())); + } + if( isDuplicateMethodAbbreviationFoundOnList(breedingMethod, programAndSystemMethods)){ + throw new BadRequestException(format("'%s' is already defined in the system.", breedingMethod.getAbbreviation())); } } @@ -127,11 +128,10 @@ boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) { || StringUtils.isBlank(method.getCategory()) || StringUtils.isBlank(method.getGeneticDiversity()); } - - boolean isDuplicateMethodFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { + boolean isDuplicateMethodNameFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { boolean foundDup = false; for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) { - if(areMethodsDuplicate(testMethod, method)){ + if(isDuplicateName(testMethod, method)){ foundDup = true; break; } @@ -139,26 +139,46 @@ boolean isDuplicateMethodFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { + boolean foundDup = false; + for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) { + if(isDuplicateAbbreviation(testMethod, method)){ + foundDup = true; + break; + } + } + return foundDup; + } - boolean areMethodsDuplicate(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { - + boolean isDuplicateName(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { // SPECIAL CASE: If the two methods are the same method, then they are not duplicates if( (testMethod.getId()!=null) && testMethod.getId().equals(method.getId()) ){ return false; } boolean isDup = false; - if(testMethod.getName()!= null && testMethod.getName().equals(method.getName())){ + if(testMethod.getName()!= null && testMethod.getName().equalsIgnoreCase(method.getName())){ isDup = true; } - else if(testMethod.getAbbreviation()!= null && testMethod.getAbbreviation().equals(method.getAbbreviation())){ + else if(testMethod.getName()==null && method.getName()==null ){ isDup = true; } - else if(testMethod.getName()==null && method.getName()==null || - testMethod.getAbbreviation()==null && method.getAbbreviation()==null){ - isDup = true; + return isDup; + } + + boolean isDuplicateAbbreviation(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { + // SPECIAL CASE: If the two methods are the same method, then they are not duplicates + if( (testMethod.getId()!=null) && testMethod.getId().equals(method.getId()) ){ + return false; } + boolean isDup = false; + if(testMethod.getAbbreviation()!= null && testMethod.getAbbreviation().equalsIgnoreCase(method.getAbbreviation())){ + isDup = true; + } + else if(testMethod.getAbbreviation()==null && method.getAbbreviation()==null ){ + isDup = true; + } return isDup; } diff --git a/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java index 9e7ec3c87..428c8fe88 100644 --- a/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java +++ b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java @@ -11,6 +11,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.UUID; import static org.junit.jupiter.api.Assertions.*; @@ -40,95 +41,115 @@ public void hasRequiredFields(){ @Test @SneakyThrows - public void isDupOfProgramMethod() { + public void isDuplicateMethodNameFoundOnList() { List programBreedingMethodEntityList = new ArrayList<>(); - programBreedingMethodEntityList.add(makeMethod("1")); - programBreedingMethodEntityList.add(makeMethod("2")); + programBreedingMethodEntityList.add(makeMethod("1", null)); + programBreedingMethodEntityList.add(makeMethod("2", null)); - boolean isDup = breedingMethodService.isDuplicateMethodFoundOnList(makeMethod("1"), programBreedingMethodEntityList); - assertTrue(isDup, "Duplicate found in list."); + boolean isDup = breedingMethodService.isDuplicateMethodNameFoundOnList(makeMethod("1", null), programBreedingMethodEntityList); + assertTrue(isDup, "Duplicate name found in list."); } @Test @SneakyThrows - public void isNotADupMethod() { + public void isDuplicateMethodAbbreviationFoundOnList() { List programBreedingMethodEntityList = new ArrayList<>(); - programBreedingMethodEntityList.add(makeMethod("1")); - programBreedingMethodEntityList.add(makeMethod("2")); + programBreedingMethodEntityList.add(makeMethod(null, "1")); + programBreedingMethodEntityList.add(makeMethod(null, "2")); + + boolean isDup = breedingMethodService.isDuplicateMethodAbbreviationFoundOnList(makeMethod(null, "1"), programBreedingMethodEntityList); + assertTrue(isDup, "Duplicate abbreviation found in list."); + isDup = breedingMethodService.isDuplicateMethodAbbreviationFoundOnList(makeMethod(null, "unique"), programBreedingMethodEntityList); + assertFalse(isDup, "Duplicate abbreviation NOT found in list."); + } - ProgramBreedingMethodEntity methodUnique = makeMethod("unique");; - boolean isDup = breedingMethodService.isDuplicateMethodFoundOnList(methodUnique, programBreedingMethodEntityList); - assertFalse(isDup, "No duplicates method found."); - } @Test @SneakyThrows - public void methodNameEqual() { - ProgramBreedingMethodEntity method = makeMethod("ABC"); - ProgramBreedingMethodEntity testMethod = makeMethod("ABC"); - testMethod.setAbbreviation("misatch Abbreviation"); + public void isDuplicateName() { + ProgramBreedingMethodEntity method = makeMethod("ABC", null); + ProgramBreedingMethodEntity testMethod = makeMethod("ABC", null); + ProgramBreedingMethodEntity testMethodLowerCase = makeMethod("abc", null); + + + assertTrue( - breedingMethodService.areMethodsDuplicate(testMethod, method), + breedingMethodService.isDuplicateName(testMethod, method), "Method Names are Equal" ); assertTrue( - breedingMethodService.areMethodsDuplicate(method, testMethod), + breedingMethodService.isDuplicateName(method, testMethod), "Method Names are Equal (switched order)" ); + assertTrue( + breedingMethodService.isDuplicateName(method, testMethodLowerCase), + "Method Names are Equal (one is LowerCase)" + ); + testMethod.setId(method.getId()); + assertFalse( + breedingMethodService.isDuplicateName(testMethod, method), + "The methods are the same method (not duplicate data)" + ); + testMethod.setName("misatch Name"); + testMethod.setId(UUID.randomUUID()); assertFalse( - breedingMethodService.areMethodsDuplicate(testMethod, method), + breedingMethodService.isDuplicateName(testMethod, method), "Method Names are not Equal" ); testMethod.setName(null); assertFalse( - breedingMethodService.areMethodsDuplicate(testMethod, method), + breedingMethodService.isDuplicateName(testMethod, method), "Method Names are not Equal (one is null)" ); testMethod.setName("name"); method.setName(null); assertFalse( - breedingMethodService.areMethodsDuplicate(testMethod, method), + breedingMethodService.isDuplicateName(testMethod, method), "Method Names are not Equal (the other is null)" ); - } @Test @SneakyThrows - public void methodAbbreviationEqual() { - ProgramBreedingMethodEntity method = makeMethod("ABC"); - ProgramBreedingMethodEntity testMethod = makeMethod("ABC"); - testMethod.setName("misatch Name"); + public void isDuplicateAbbreviation() { + ProgramBreedingMethodEntity method = makeMethod("ABC", "ABC"); + ProgramBreedingMethodEntity testMethod = makeMethod("mismatch_name", "ABC"); + ProgramBreedingMethodEntity testMethodLowerCase = makeMethod("mismatch_name", "abc"); assertTrue( - breedingMethodService.areMethodsDuplicate(testMethod, method), + breedingMethodService.isDuplicateAbbreviation(testMethod, method), "Method Abbreviations are Equal" ); assertTrue( - breedingMethodService.areMethodsDuplicate(method, testMethod), + breedingMethodService.isDuplicateAbbreviation(method, testMethod), "Method Abbreviations are Equal (switched order)" ); - - testMethod.setAbbreviation("misatch Abbreviation"); + assertTrue( + breedingMethodService.isDuplicateAbbreviation(testMethodLowerCase, method), + "Method Abbreviations are Equal (one is lower case)" + ); + testMethod.setAbbreviation("misatch_abbreviation"); assertFalse( - breedingMethodService.areMethodsDuplicate(method, testMethod), + breedingMethodService.isDuplicateAbbreviation(method, testMethod), "Method Abbreviations are not equal." ); testMethod.setAbbreviation(null); assertFalse( - breedingMethodService.areMethodsDuplicate(method, testMethod), + breedingMethodService.isDuplicateAbbreviation(method, testMethod), "Method Abbreviations are not equal (one is null)." ); } // Helper Methods // - private ProgramBreedingMethodEntity makeMethod(String suffix){ + private ProgramBreedingMethodEntity makeMethod(String nameSuffix, String abbrevSuffix){ ProgramBreedingMethodEntity method = new ProgramBreedingMethodEntity(); - method.setName("Name" + suffix); - method.setAbbreviation("Abbreviation" + suffix); + + method.setName("Name" + nameSuffix!=null? nameSuffix : "junk"); + method.setAbbreviation("Abbreviation" + abbrevSuffix!=null? abbrevSuffix : "junk"); + method.setId(UUID.randomUUID()); return method; } From 45d54d76021280d8e380c002968921507091b92c Mon Sep 17 00:00:00 2001 From: David Randolph Phillips Date: Fri, 29 Sep 2023 13:53:22 -0400 Subject: [PATCH 6/7] [BI-1852]changed returned type 'HttpResponse' to 'HttpResponse' to the linter happy --- .../api/v1/controller/BreedingMethodController.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java index 7b3fc2bb5..9d80f9115 100644 --- a/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java +++ b/src/main/java/org/breedinginsight/api/v1/controller/BreedingMethodController.java @@ -59,7 +59,7 @@ public HttpResponse>> getSyst @Post("programs/{programId}/breeding-methods") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException { + public HttpResponse createProgramBreedingMethod(@PathVariable UUID programId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException{ log.debug("Saving new program breeding method"); try { @@ -107,7 +107,7 @@ public HttpResponse>> getProg @Put("programs/{programId}/breeding-methods/{breedingMethodId}") @Produces(MediaType.APPLICATION_JSON) @ProgramSecured(roles = {ProgramSecuredRole.BREEDER}) - public HttpResponse updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException { + public HttpResponse updateProgramBreedingMethod(@PathVariable UUID programId, @PathVariable UUID breedingMethodId, @Body ProgramBreedingMethodEntity breedingMethod) throws ApiException { log.debug("Saving new program breeding method"); try { From 070e1822a6e33bc44559e6e7714410bd5162ffd0 Mon Sep 17 00:00:00 2001 From: David Randolph Phillips Date: Fri, 29 Sep 2023 14:59:24 -0400 Subject: [PATCH 7/7] [BI-1852] explisitly declaired 'protected' methods fixed type-o in test titles Improved error messages --- .../impl/BreedingMethodServiceImpl.java | 20 ++++++++++++------- .../impl/BreedingMethodServiceImplTest.java | 10 +++++----- .../validators/TraitValidatorUnitTest.java | 1 - 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java index 239a3115d..19e880309 100644 --- a/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java +++ b/src/main/java/org/breedinginsight/services/impl/BreedingMethodServiceImpl.java @@ -85,10 +85,10 @@ private void validate(ProgramBreedingMethodEntity breedingMethod, UUID programId List programAndSystemMethods = getBreedingMethods(programId); if( isDuplicateMethodNameFoundOnList(breedingMethod, programAndSystemMethods)){ - throw new BadRequestException(format("'%s' is already defined in the system.", breedingMethod.getName())); + throw new BadRequestException(format("A breeding method with the name '%s' is already defined in the system.", breedingMethod.getName())); } if( isDuplicateMethodAbbreviationFoundOnList(breedingMethod, programAndSystemMethods)){ - throw new BadRequestException(format("'%s' is already defined in the system.", breedingMethod.getAbbreviation())); + throw new BadRequestException(format("A breeding method with the abbreviation '%s' is already defined in the system.", breedingMethod.getAbbreviation())); } } @@ -122,13 +122,16 @@ public void deleteBreedingMethod(UUID programId, UUID breedingMethodId) throws A dsl.transaction(() -> breedingMethodDAO.deleteProgramMethod(programId, breedingMethodId)); } - boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) { + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isMissingRequiredFields(ProgramBreedingMethodEntity method) { return StringUtils.isBlank(method.getName()) || StringUtils.isBlank(method.getAbbreviation()) || StringUtils.isBlank(method.getCategory()) || StringUtils.isBlank(method.getGeneticDiversity()); } - boolean isDuplicateMethodNameFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { + + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateMethodNameFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { boolean foundDup = false; for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) { if(isDuplicateName(testMethod, method)){ @@ -139,7 +142,8 @@ boolean isDuplicateMethodNameFoundOnList(ProgramBreedingMethodEntity method, Lis return foundDup; } - boolean isDuplicateMethodAbbreviationFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateMethodAbbreviationFoundOnList(ProgramBreedingMethodEntity method, List programBreedingMethodEntityList) { boolean foundDup = false; for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) { if(isDuplicateAbbreviation(testMethod, method)){ @@ -150,7 +154,8 @@ boolean isDuplicateMethodAbbreviationFoundOnList(ProgramBreedingMethodEntity met return foundDup; } - boolean isDuplicateName(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateName(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { // SPECIAL CASE: If the two methods are the same method, then they are not duplicates if( (testMethod.getId()!=null) && testMethod.getId().equals(method.getId()) ){ return false; @@ -166,7 +171,8 @@ else if(testMethod.getName()==null && method.getName()==null ){ return isDup; } - boolean isDuplicateAbbreviation(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { + //'protected' instead of 'private' so it is accessible to unit test. + protected boolean isDuplicateAbbreviation(ProgramBreedingMethodEntity testMethod, ProgramBreedingMethodEntity method) { // SPECIAL CASE: If the two methods are the same method, then they are not duplicates if( (testMethod.getId()!=null) && testMethod.getId().equals(method.getId()) ){ return false; diff --git a/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java index 428c8fe88..cc59fd6ae 100644 --- a/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java +++ b/src/test/java/org/breedinginsight/services/impl/BreedingMethodServiceImplTest.java @@ -47,7 +47,7 @@ public void isDuplicateMethodNameFoundOnList() { programBreedingMethodEntityList.add(makeMethod("2", null)); boolean isDup = breedingMethodService.isDuplicateMethodNameFoundOnList(makeMethod("1", null), programBreedingMethodEntityList); - assertTrue(isDup, "Duplicate name found in list."); + assertTrue(isDup, "Duplicate name found in list."); } @Test @@ -58,9 +58,9 @@ public void isDuplicateMethodAbbreviationFoundOnList() { programBreedingMethodEntityList.add(makeMethod(null, "2")); boolean isDup = breedingMethodService.isDuplicateMethodAbbreviationFoundOnList(makeMethod(null, "1"), programBreedingMethodEntityList); - assertTrue(isDup, "Duplicate abbreviation found in list."); + assertTrue(isDup, "Duplicate abbreviation found in list."); isDup = breedingMethodService.isDuplicateMethodAbbreviationFoundOnList(makeMethod(null, "unique"), programBreedingMethodEntityList); - assertFalse(isDup, "Duplicate abbreviation NOT found in list."); + assertFalse(isDup, "Duplicate abbreviation NOT found in list."); } @@ -147,8 +147,8 @@ public void isDuplicateAbbreviation() { private ProgramBreedingMethodEntity makeMethod(String nameSuffix, String abbrevSuffix){ ProgramBreedingMethodEntity method = new ProgramBreedingMethodEntity(); - method.setName("Name" + nameSuffix!=null? nameSuffix : "junk"); - method.setAbbreviation("Abbreviation" + abbrevSuffix!=null? abbrevSuffix : "junk"); + method.setName("Name" + ( nameSuffix != null? nameSuffix : "junk" )); + method.setAbbreviation("Abbreviation" + ( abbrevSuffix != null? abbrevSuffix : "junk" )); method.setId(UUID.randomUUID()); return method; } diff --git a/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java b/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java index cc563772c..a7fa4507b 100644 --- a/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java +++ b/src/test/java/org/breedinginsight/services/validators/TraitValidatorUnitTest.java @@ -40,7 +40,6 @@ import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; @TestInstance(TestInstance.Lifecycle.PER_CLASS) public class TraitValidatorUnitTest {