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.

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

Bug/bi 1852 #294

Merged
merged 8 commits into from
Oct 3, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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}")
Expand Down Expand Up @@ -64,7 +59,7 @@ public HttpResponse<Response<DataResponse<ProgramBreedingMethodEntity>>> getSyst
@Post("programs/{programId}/breeding-methods")
@Produces(MediaType.APPLICATION_JSON)
@ProgramSecured(roles = {ProgramSecuredRole.BREEDER})
public HttpResponse<Response<ProgramBreedingMethodEntity>> 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 {
Expand All @@ -79,6 +74,8 @@ public HttpResponse<Response<ProgramBreedingMethodEntity>> 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;
Expand Down Expand Up @@ -110,7 +107,7 @@ public HttpResponse<Response<DataResponse<ProgramBreedingMethodEntity>>> getProg
@Put("programs/{programId}/breeding-methods/{breedingMethodId}")
@Produces(MediaType.APPLICATION_JSON)
@ProgramSecured(roles = {ProgramSecuredRole.BREEDER})
public HttpResponse<Response<ProgramBreedingMethodEntity>> 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 {
Expand All @@ -128,6 +125,8 @@ public HttpResponse<Response<ProgramBreedingMethodEntity>> 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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -60,28 +60,39 @@ public List<ProgramBreedingMethodEntity> 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));
}


@Override
public ProgramBreedingMethodEntity updateBreedingMethod(ProgramBreedingMethodEntity breedingMethod, UUID programId, UUID userId) throws BadRequestException, ApiException {
if(!validateBreedingMethod(breedingMethod)) {
throw new BadRequestException("Missing required data");
}

List<ProgramBreedingMethodEntity> 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));
}

private void validate(ProgramBreedingMethodEntity breedingMethod, UUID programId) throws BadRequestException {
if (isMissingRequiredFields(breedingMethod)) {
throw new BadRequestException("Missing required data");
}

List<ProgramBreedingMethodEntity> programAndSystemMethods = getBreedingMethods(programId);
if( isDuplicateMethodNameFoundOnList(breedingMethod, programAndSystemMethods)){
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("A breeding method with the abbreviation '%s' is already defined in the system.", breedingMethod.getAbbreviation()));
}
}


@Override
public void enableSystemMethods(List<UUID> systemBreedingMethods, UUID programId, UUID userId) throws ApiException, BadRequestException {
List<ProgramBreedingMethodEntity> inUseMethods = fetchBreedingMethodsInUse(programId);
Expand Down Expand Up @@ -111,10 +122,70 @@ 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());
//'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());
}

//'protected' instead of 'private' so it is accessible to unit test.
protected boolean isDuplicateMethodNameFoundOnList(ProgramBreedingMethodEntity method, List<ProgramBreedingMethodEntity> programBreedingMethodEntityList) {
boolean foundDup = false;
for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) {
if(isDuplicateName(testMethod, method)){
foundDup = true;
break;
}
}
return foundDup;
}

//'protected' instead of 'private' so it is accessible to unit test.
protected boolean isDuplicateMethodAbbreviationFoundOnList(ProgramBreedingMethodEntity method, List<ProgramBreedingMethodEntity> programBreedingMethodEntityList) {
boolean foundDup = false;
for (ProgramBreedingMethodEntity testMethod: programBreedingMethodEntityList) {
if(isDuplicateAbbreviation(testMethod, method)){
foundDup = true;
break;
}
}
return foundDup;
}

//'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;
}

boolean isDup = false;
if(testMethod.getName()!= null && testMethod.getName().equalsIgnoreCase(method.getName())){
isDup = true;
}
else if(testMethod.getName()==null && method.getName()==null ){
isDup = true;
}
Comment on lines +168 to +170
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.

return isDup;
}

//'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;
}

boolean isDup = false;
if(testMethod.getAbbreviation()!= null && testMethod.getAbbreviation().equalsIgnoreCase(method.getAbbreviation())){
isDup = true;
}
else if(testMethod.getAbbreviation()==null && method.getAbbreviation()==null ){
isDup = true;
}
Comment on lines +185 to +187
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?

return isDup;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
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 java.util.UUID;

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 isDuplicateMethodNameFoundOnList() {
List<ProgramBreedingMethodEntity> programBreedingMethodEntityList = new ArrayList<>();
programBreedingMethodEntityList.add(makeMethod("1", null));
programBreedingMethodEntityList.add(makeMethod("2", null));

boolean isDup = breedingMethodService.isDuplicateMethodNameFoundOnList(makeMethod("1", null), programBreedingMethodEntityList);
assertTrue(isDup, "Duplicate name found in list.");
}

@Test
@SneakyThrows
public void isDuplicateMethodAbbreviationFoundOnList() {
List<ProgramBreedingMethodEntity> programBreedingMethodEntityList = new ArrayList<>();
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.");
}



@Test
@SneakyThrows
public void isDuplicateName() {
ProgramBreedingMethodEntity method = makeMethod("ABC", null);
ProgramBreedingMethodEntity testMethod = makeMethod("ABC", null);
ProgramBreedingMethodEntity testMethodLowerCase = makeMethod("abc", null);



assertTrue(
breedingMethodService.isDuplicateName(testMethod, method),
"Method Names are Equal"
);
assertTrue(
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.isDuplicateName(testMethod, method),
"Method Names are not Equal"
);
testMethod.setName(null);
assertFalse(
breedingMethodService.isDuplicateName(testMethod, method),
"Method Names are not Equal (one is null)"
);

testMethod.setName("name");
method.setName(null);
assertFalse(
breedingMethodService.isDuplicateName(testMethod, method),
"Method Names are not Equal (the other is null)"
);
}

@Test
@SneakyThrows
public void isDuplicateAbbreviation() {
ProgramBreedingMethodEntity method = makeMethod("ABC", "ABC");
ProgramBreedingMethodEntity testMethod = makeMethod("mismatch_name", "ABC");
ProgramBreedingMethodEntity testMethodLowerCase = makeMethod("mismatch_name", "abc");
assertTrue(
breedingMethodService.isDuplicateAbbreviation(testMethod, method),
"Method Abbreviations are Equal"
);
assertTrue(
breedingMethodService.isDuplicateAbbreviation(method, testMethod),
"Method Abbreviations are Equal (switched order)"
);
assertTrue(
breedingMethodService.isDuplicateAbbreviation(testMethodLowerCase, method),
"Method Abbreviations are Equal (one is lower case)"
);
testMethod.setAbbreviation("misatch_abbreviation");
assertFalse(
breedingMethodService.isDuplicateAbbreviation(method, testMethod),
"Method Abbreviations are not equal."
);
testMethod.setAbbreviation(null);
assertFalse(
breedingMethodService.isDuplicateAbbreviation(method, testMethod),
"Method Abbreviations are not equal (one is null)."
);
}

// Helper Methods //
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.setId(UUID.randomUUID());
return method;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,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");
Expand Down