From c970c824438634888211e8aaeefd4d8648db1136 Mon Sep 17 00:00:00 2001 From: Ryan McCauley <32387857+k4pran@users.noreply.github.com> Date: Wed, 1 May 2024 17:58:36 +0100 Subject: [PATCH] TRUNK-6218: Concept Validations should be more lenient with retired concepts (#4555) --- .../java/org/openmrs/api/ConceptService.java | 17 +++++- .../java/org/openmrs/api/db/ConceptDAO.java | 7 ++- .../api/db/hibernate/HibernateConceptDAO.java | 59 +++++++++++++------ .../openmrs/api/impl/ConceptServiceImpl.java | 8 ++- .../ConceptReferenceTermValidator.java | 3 +- .../openmrs/validator/ConceptValidator.java | 12 +++- .../org/openmrs/api/ConceptServiceTest.java | 56 ++++++++++++++++-- .../ConceptReferenceTermValidatorTest.java | 20 +++++++ .../validator/ConceptValidatorTest.java | 26 ++++++++ .../openmrs/include/standardTestDataset.xml | 2 + 10 files changed, 180 insertions(+), 30 deletions(-) diff --git a/api/src/main/java/org/openmrs/api/ConceptService.java b/api/src/main/java/org/openmrs/api/ConceptService.java index cb13b04e5eb7..29405d851bad 100644 --- a/api/src/main/java/org/openmrs/api/ConceptService.java +++ b/api/src/main/java/org/openmrs/api/ConceptService.java @@ -1596,7 +1596,22 @@ public List getDrugs(String drugName, Concept concept, boolean searchKeywo */ @Authorized(PrivilegeConstants.GET_CONCEPT_REFERENCE_TERMS) public ConceptReferenceTerm getConceptReferenceTermByCode(String code, ConceptSource conceptSource) throws APIException; - + + /** + * Gets a list of concept reference terms with the specified code from the specified concept source + * + * @param code the code to match against + * @param conceptSource the concept source to match against + * @param includeRetired specifies if retired concept reference terms should be included + * @return concept reference term object + * @since 2.7 + * @throws APIException + * Should return a list of concept reference terms that matches the given code from the given source + */ + @Authorized(PrivilegeConstants.GET_CONCEPT_REFERENCE_TERMS) + public List getConceptReferenceTermByCode(String code, ConceptSource conceptSource, boolean includeRetired) throws APIException; + + /** * Stores the specified concept reference term to the database * diff --git a/api/src/main/java/org/openmrs/api/db/ConceptDAO.java b/api/src/main/java/org/openmrs/api/db/ConceptDAO.java index 7ca7ac3549db..ae365425d4bb 100644 --- a/api/src/main/java/org/openmrs/api/db/ConceptDAO.java +++ b/api/src/main/java/org/openmrs/api/db/ConceptDAO.java @@ -520,7 +520,12 @@ public List getDrugs(String drugName, Concept concept, boolean searchOnPhr * @see ConceptService#getConceptReferenceTermByCode(String, ConceptSource) */ public ConceptReferenceTerm getConceptReferenceTermByCode(String code, ConceptSource conceptSource) throws DAOException; - + + /** + * @see ConceptService#getConceptReferenceTermByCode(String, ConceptSource, boolean) + */ + public List getConceptReferenceTermByCode(String code, ConceptSource conceptSource, boolean includeRetired) throws DAOException; + /** * @see ConceptService#saveConceptReferenceTerm(ConceptReferenceTerm) */ diff --git a/api/src/main/java/org/openmrs/api/db/hibernate/HibernateConceptDAO.java b/api/src/main/java/org/openmrs/api/db/hibernate/HibernateConceptDAO.java index ca5dd5ea2ceb..9800387ed89b 100644 --- a/api/src/main/java/org/openmrs/api/db/hibernate/HibernateConceptDAO.java +++ b/api/src/main/java/org/openmrs/api/db/hibernate/HibernateConceptDAO.java @@ -9,6 +9,8 @@ */ package org.openmrs.api.db.hibernate; +import static java.util.stream.Collectors.toList; + import javax.persistence.Query; import javax.persistence.TypedQuery; import javax.persistence.criteria.CriteriaBuilder; @@ -1117,7 +1119,7 @@ public List getConceptsByMapping(String code, String sourceName, boolea } return session.createQuery(cq).getResultList() - .stream().distinct().collect(Collectors.toList()); + .stream().distinct().collect(toList()); } /** @@ -1142,7 +1144,7 @@ public List getConceptIdsByMapping(String code, String sourceName, bool } return session.createQuery(cq).getResultList() - .stream().distinct().collect(Collectors.toList()); + .stream().distinct().collect(toList()); } /** @@ -1741,32 +1743,55 @@ public ConceptReferenceTerm getConceptReferenceTermByName(String name, ConceptSo } return terms.get(0); } - + /** * @see org.openmrs.api.db.ConceptDAO#getConceptReferenceTermByCode(java.lang.String, * org.openmrs.ConceptSource) */ @Override public ConceptReferenceTerm getConceptReferenceTermByCode(String code, ConceptSource conceptSource) throws DAOException { + List conceptReferenceTerms = getConceptReferenceTermByCode(code, conceptSource, true); + + if (conceptReferenceTerms.isEmpty()) { + return null; + } else if (conceptReferenceTerms.size() > 1) { + List unretiredConceptReferenceTerms = conceptReferenceTerms.stream() + .filter(term -> !term.getRetired()) + .collect(toList()); + if (unretiredConceptReferenceTerms.size() == 1) { + return unretiredConceptReferenceTerms.get(0); + } + + // either more than one unretired concept term or more than one retired concept term + throw new APIException("ConceptReferenceTerm.foundMultipleTermsWithCodeInSource", + new Object[] { code, conceptSource.getName() }); + } + + return conceptReferenceTerms.get(0); + } + + /** + * @see org.openmrs.api.db.ConceptDAO#getConceptReferenceTermByCode(java.lang.String, + * org.openmrs.ConceptSource, boolean) + */ + @Override + public List getConceptReferenceTermByCode(String code, ConceptSource conceptSource, + boolean includeRetired) throws DAOException { Session session = sessionFactory.getCurrentSession(); CriteriaBuilder cb = session.getCriteriaBuilder(); CriteriaQuery cq = cb.createQuery(ConceptReferenceTerm.class); Root root = cq.from(ConceptReferenceTerm.class); - Predicate codePredicate = cb.equal(root.get("code"), code); - Predicate sourcePredicate = cb.equal(root.get("conceptSource"), conceptSource); - - cq.where(cb.and(codePredicate, sourcePredicate)); - - List terms = session.createQuery(cq).getResultList(); - - if (terms.isEmpty()) { - return null; - } else if (terms.size() > 1) { - throw new APIException("ConceptReferenceTerm.foundMultipleTermsWithCodeInSource", - new Object[] { code, conceptSource.getName() }); + List predicates = new ArrayList<>(); + predicates.add(cb.equal(root.get("code"), code)); + predicates.add(cb.equal(root.get("conceptSource"), conceptSource)); + + if (!includeRetired) { + predicates.add(cb.isFalse(root.get("retired"))); } - return terms.get(0); + cq.where(predicates.toArray(new Predicate[]{})); + + return session.createQuery(cq).getResultList(); } /** @@ -2121,7 +2146,7 @@ public List getDrugsByMapping(String code, ConceptSource conceptSource, cq.where(basePredicates.toArray(new Predicate[]{})); - return session.createQuery(cq).getResultList().stream().distinct().collect(Collectors.toList()); + return session.createQuery(cq).getResultList().stream().distinct().collect(toList()); } /** diff --git a/api/src/main/java/org/openmrs/api/impl/ConceptServiceImpl.java b/api/src/main/java/org/openmrs/api/impl/ConceptServiceImpl.java index 7f73acf1b122..acfb8ee2d34a 100644 --- a/api/src/main/java/org/openmrs/api/impl/ConceptServiceImpl.java +++ b/api/src/main/java/org/openmrs/api/impl/ConceptServiceImpl.java @@ -1785,7 +1785,13 @@ public ConceptReferenceTerm getConceptReferenceTermByName(String name, ConceptSo public ConceptReferenceTerm getConceptReferenceTermByCode(String code, ConceptSource conceptSource) throws APIException { return dao.getConceptReferenceTermByCode(code, conceptSource); } - + + @Override + @Transactional(readOnly = true) + public List getConceptReferenceTermByCode(String code, ConceptSource conceptSource, boolean includeRetired) throws APIException { + return dao.getConceptReferenceTermByCode(code, conceptSource, includeRetired); + } + /** * @see org.openmrs.api.ConceptService#saveConceptReferenceTerm(org.openmrs.ConceptReferenceTerm) */ diff --git a/api/src/main/java/org/openmrs/validator/ConceptReferenceTermValidator.java b/api/src/main/java/org/openmrs/validator/ConceptReferenceTermValidator.java index d039d3de72e3..33d15315f3e7 100644 --- a/api/src/main/java/org/openmrs/validator/ConceptReferenceTermValidator.java +++ b/api/src/main/java/org/openmrs/validator/ConceptReferenceTermValidator.java @@ -62,6 +62,7 @@ public boolean supports(Class c) { * Should fail if a term is mapped multiple times to the same term * Should pass validation if field lengths are correct * Should fail validation if field lengths are not correct + * Should pass validation if the duplicate concept reference term is retired */ @Override public void validate(Object obj, Errors errors) throws APIException { @@ -93,7 +94,7 @@ public void validate(Object obj, Errors errors) throws APIException { //Ensure that there are no terms with the same code in the same source ConceptReferenceTerm termWithDuplicateCode = Context.getConceptService().getConceptReferenceTermByCode(code, conceptReferenceTerm.getConceptSource()); - if (termWithDuplicateCode != null + if (termWithDuplicateCode != null && !termWithDuplicateCode.getRetired() && !OpenmrsUtil.nullSafeEquals(termWithDuplicateCode.getUuid(), conceptReferenceTerm.getUuid())) { errors.rejectValue("code", "ConceptReferenceTerm.duplicate.code", "Duplicate concept reference term code in its concept source: " + code); diff --git a/api/src/main/java/org/openmrs/validator/ConceptValidator.java b/api/src/main/java/org/openmrs/validator/ConceptValidator.java index be6a40901da2..857ee0273468 100644 --- a/api/src/main/java/org/openmrs/validator/ConceptValidator.java +++ b/api/src/main/java/org/openmrs/validator/ConceptValidator.java @@ -87,6 +87,8 @@ public boolean supports(Class c) { * Should pass if different concepts have the same short name * Should fail if the concept datatype is null * Should fail if the concept class is null + * Should pass if the concept is retired and the only validation failures would be in ConceptName + * or ConceptMap, as a retired Concept bypasses ConceptName and ConceptMap validation. */ @Override public void validate(Object obj, Errors errors) throws APIException, DuplicateConceptNameException { @@ -104,9 +106,13 @@ public void validate(Object obj, Errors errors) throws APIException, DuplicateCo ValidationUtils.rejectIfEmpty(errors, "datatype", "Concept.datatype.empty"); ValidationUtils.rejectIfEmpty(errors, "conceptClass", "Concept.conceptClass.empty"); - + boolean hasFullySpecifiedName = false; for (Locale conceptNameLocale : conceptToValidate.getAllConceptNameLocales()) { + if (conceptToValidate.getRetired()) { + continue; + } + boolean fullySpecifiedNameForLocaleFound = false; boolean preferredNameForLocaleFound = false; boolean shortNameForLocaleFound = false; @@ -199,12 +205,12 @@ else if (nameInLocale.getLocalePreferred() && preferredNameForLocaleFound) { } //Ensure that each concept has at least a fully specified name - if (!hasFullySpecifiedName) { + if (!hasFullySpecifiedName && !conceptToValidate.getRetired()) { log.debug("Concept has no fully specified name"); errors.reject("Concept.error.no.FullySpecifiedName"); } - if (CollectionUtils.isNotEmpty(conceptToValidate.getConceptMappings())) { + if (CollectionUtils.isNotEmpty(conceptToValidate.getConceptMappings()) && !conceptToValidate.getRetired()) { //validate all the concept maps int index = 0; Set mappedTermIds = null; diff --git a/api/src/test/java/org/openmrs/api/ConceptServiceTest.java b/api/src/test/java/org/openmrs/api/ConceptServiceTest.java index 7731a9269887..a14cebdaf6d9 100644 --- a/api/src/test/java/org/openmrs/api/ConceptServiceTest.java +++ b/api/src/test/java/org/openmrs/api/ConceptServiceTest.java @@ -42,6 +42,7 @@ import java.util.List; import java.util.Locale; import java.util.Set; +import java.util.stream.Collectors; import net.sf.ehcache.Ehcache; import org.apache.commons.collections.CollectionUtils; @@ -2173,6 +2174,49 @@ public void getConceptReferenceTermByCode_shouldReturnAConceptReferenceTermThatM assertNotNull(term); assertEquals("2332523", term.getCode()); } + + /** + * @see ConceptService#getConceptReferenceTermByCode(String,ConceptSource) + */ + @Test + public void getConceptReferenceTermByCode_shouldReturnUnretiredTermIfRetiredAlsoExists() + { + ConceptReferenceTerm term = Context.getConceptService().getConceptReferenceTermByCode("898989", + new ConceptSource(1)); + assertNotNull(term); + assertEquals("898989", term.getCode()); + assertFalse(term.getRetired()); + } + + /** + * @see ConceptService#getConceptReferenceTermByCode(String,ConceptSource, boolean) + */ + @Test + public void getConceptReferenceTermByCode_shouldReturnBothRetiredAndUnretiredTerms() { + List terms = Context.getConceptService().getConceptReferenceTermByCode( + "898989", new ConceptSource(1), true); + + assertEquals(2, terms.size()); + + List termStates = terms.stream().map(ConceptReferenceTerm::getRetired) + .sorted().collect(Collectors.toList()); + + List expectedStates = Arrays.asList(false, true); + + assertEquals(expectedStates, termStates); + } + + /** + * @see ConceptService#getConceptReferenceTermByCode(String,ConceptSource, boolean) + */ + @Test + public void getConceptReferenceTermByCode_shouldExcludeRetiredConcepts() { + List terms = Context.getConceptService().getConceptReferenceTermByCode( + "898989", new ConceptSource(1), false); + + assertEquals(1, terms.size()); + assertFalse(terms.get(0).getRetired()); + } /** * @see ConceptService#getConceptMapTypes(null,null) @@ -2332,7 +2376,7 @@ public void retireConceptMapType_shouldShouldSetTheDefaultRetireReasonIfNoneIsGi @Test public void getConceptReferenceTerms_shouldReturnAllTheConceptReferenceTermsIfIncludeRetiredIsSetToTrue() { - assertEquals(11, Context.getConceptService().getConceptReferenceTerms(true).size()); + assertEquals(13, Context.getConceptService().getConceptReferenceTerms(true).size()); } /** @@ -2341,7 +2385,7 @@ public void getConceptReferenceTerms_shouldReturnAllTheConceptReferenceTermsIfIn @Test public void getConceptReferenceTerms_shouldReturnOnlyUnRetiredConceptReferenceTermsIfIncludeRetiredIsSetToFalse() { - assertEquals(10, Context.getConceptService().getConceptReferenceTerms(false).size()); + assertEquals(11, Context.getConceptService().getConceptReferenceTerms(false).size()); } /** @@ -2359,7 +2403,7 @@ public void getConceptReferenceTermByUuid_shouldReturnTheConceptReferenceTermTha @Test public void getConceptReferenceTerms_shouldReturnOnlyTheConceptReferenceTermsFromTheGivenConceptSource() { - assertEquals(9, conceptService.getConceptReferenceTerms(null, conceptService.getConceptSource(1), 0, null, + assertEquals(11, conceptService.getConceptReferenceTerms(null, conceptService.getConceptSource(1), 0, null, true).size()); } @@ -2475,7 +2519,7 @@ public void unretireConceptReferenceTerm_shouldUnretireTheSpecifiedConceptRefere */ @Test public void getAllConceptReferenceTerms_shouldReturnAllConceptReferenceTermsInTheDatabase() { - assertEquals(11, Context.getConceptService().getAllConceptReferenceTerms().size()); + assertEquals(13, Context.getConceptService().getAllConceptReferenceTerms().size()); } /** @@ -2591,7 +2635,7 @@ public void getConceptsByClass_shouldReturnAnEmptyListIfNoneWasFound() { */ @Test public void getCountOfConceptReferenceTerms_shouldIncludeRetiredTermsIfIncludeRetiredIsSetToTrue() { - assertEquals(11, conceptService.getCountOfConceptReferenceTerms("", null, true).intValue()); + assertEquals(13, conceptService.getCountOfConceptReferenceTerms("", null, true).intValue()); } /** @@ -2599,7 +2643,7 @@ public void getCountOfConceptReferenceTerms_shouldIncludeRetiredTermsIfIncludeRe */ @Test public void getCountOfConceptReferenceTerms_shouldNotIncludeRetiredTermsIfIncludeRetiredIsSetToFalse() { - assertEquals(10, conceptService.getCountOfConceptReferenceTerms("", null, false).intValue()); + assertEquals(11, conceptService.getCountOfConceptReferenceTerms("", null, false).intValue()); } /** diff --git a/api/src/test/java/org/openmrs/validator/ConceptReferenceTermValidatorTest.java b/api/src/test/java/org/openmrs/validator/ConceptReferenceTermValidatorTest.java index a8b0b27104fa..796664704005 100644 --- a/api/src/test/java/org/openmrs/validator/ConceptReferenceTermValidatorTest.java +++ b/api/src/test/java/org/openmrs/validator/ConceptReferenceTermValidatorTest.java @@ -310,4 +310,24 @@ public void validate_shouldFailValidationIfFieldLengthsAreNotCorrect() { assertTrue(errors.hasFieldErrors("description")); assertTrue(errors.hasFieldErrors("retireReason")); } + + @Test + public void validate_shouldPassIfTheConceptReferenceTermCodeIsDuplicateButRetired() { + ConceptReferenceTerm term = new ConceptReferenceTerm(); + term.setName("name"); + term.setCode("WGT234"); + term.setConceptSource(Context.getConceptService().getConceptSource(1)); + Errors errors = new BindException(term, "term"); + + ConceptReferenceTerm termWithDuplicateCode = Context.getConceptService() + .getConceptReferenceTermByCode(term.getCode(), term.getConceptSource()); + + if (termWithDuplicateCode != null) { + termWithDuplicateCode.setRetired(true); + } + + new ConceptReferenceTermValidator().validate(term, errors); + + assertFalse(errors.hasFieldErrors("code")); + } } diff --git a/api/src/test/java/org/openmrs/validator/ConceptValidatorTest.java b/api/src/test/java/org/openmrs/validator/ConceptValidatorTest.java index 0975014c4440..16012f50e139 100644 --- a/api/src/test/java/org/openmrs/validator/ConceptValidatorTest.java +++ b/api/src/test/java/org/openmrs/validator/ConceptValidatorTest.java @@ -32,6 +32,7 @@ import org.openmrs.ConceptDatatype; import org.openmrs.ConceptDescription; import org.openmrs.ConceptMap; +import org.openmrs.ConceptMapType; import org.openmrs.ConceptName; import org.openmrs.ConceptReferenceTerm; import org.openmrs.api.ConceptNameType; @@ -534,4 +535,29 @@ public void validate_shouldNotFailIfBlankConceptDescriptionIsPassed() { assertThat(errors, not(hasFieldErrors("description"))); } + + @Test + public void validate_shouldSkipConceptNameValidationForRetiredConcepts() { + concept.setRetired(true); + concept.addName(new ConceptName("", Context.getLocale())); + concept.setDatatype(new ConceptDatatype()); + concept.setConceptClass(new ConceptClass()); + + validator.validate(concept, errors); + + assertFalse(errors.hasErrors(), "Expected no validation errors for a retired concept"); + } + + @Test + public void validate_shouldSkipConceptMapValidationForRetiredConcepts() { + concept.setRetired(true); + concept.addName(new ConceptName("some name", Context.getLocale())); + concept.setConceptClass(new ConceptClass()); + concept.setDatatype(new ConceptDatatype()); + concept.addConceptMapping(new ConceptMap(null, new ConceptMapType())); + + validator.validate(concept, errors); + + assertFalse(errors.hasErrors(), "Expected no validation errors for a retired concept"); + } } diff --git a/api/src/test/resources/org/openmrs/include/standardTestDataset.xml b/api/src/test/resources/org/openmrs/include/standardTestDataset.xml index aec298d8af7a..6af982e5c0a9 100644 --- a/api/src/test/resources/org/openmrs/include/standardTestDataset.xml +++ b/api/src/test/resources/org/openmrs/include/standardTestDataset.xml @@ -142,6 +142,8 @@ + +