diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/FormSubmissionActions.java b/api/src/main/java/org/openmrs/module/htmlformentry/FormSubmissionActions.java index b2fcd3f3a..d74cdcf2c 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/FormSubmissionActions.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/FormSubmissionActions.java @@ -381,7 +381,7 @@ public Obs createObs(Concept concept, Object value, Date datetime, String access * @param comment comment for the obs */ public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date newDatetime, String accessionNumber, - String comment) { + String comment, String controlFormPath) { if (newValue == null || "".equals(newValue)) { // we want to delete the existing obs if (log.isDebugEnabled()) @@ -397,6 +397,7 @@ public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date ne return; } Obs newObs = HtmlFormEntryUtil.createObs(concept, newValue, newDatetime, accessionNumber); + // random note... it is possible that the existing obs has changed even though the value as string is the same (probably not?) String oldString = existingObs.getValueAsString(Context.getLocale()); String newString = newObs.getValueAsString(Context.getLocale()); if (log.isDebugEnabled() && concept != null) { @@ -418,7 +419,7 @@ public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date ne // TODO: really the voided obs should link to the new one, but this is a pain to implement due to the dreaded error: org.hibernate.NonUniqueObjectException: a different object with the same identifier value was already associated with the session obsToVoid.add(existingObs); - newObs = createObs(concept, newValue, newDatetime, accessionNumber, comment); + newObs = createObs(concept, newValue, newDatetime, accessionNumber, comment, controlFormPath); newObs.setPreviousVersion(existingObs); } else { if (existingObs != null && StringUtils.isNotBlank(comment)) @@ -430,79 +431,13 @@ public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date ne } } - /** - * Legacy modifyObs methods without the comment argument - */ - public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date newDatetime, String accessionNumber) { - modifyObs(existingObs, concept, newValue, newDatetime, accessionNumber, null); + public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date newDatetime, String accessionNumber, + String comment) { + modifyObs(existingObs, concept, newValue, newDatetime, accessionNumber, comment, null); } - /** - * Modifies an existing Obs. - *

- * This method works by adding the current Obs to a list of Obs to void, and then adding the new Obs - * to a list of Obs to create. Note that this method does not commit the changes to the - * database--the changes are applied elsewhere in the framework. - * - * @param existingObs the Obs to modify - * @param concept concept associated with the Obs - * @param newValue the new value of the Obs - * @param newDatetime the new date information for the Obs - * @param accessionNumber new accession number for the Obs - * @param comment comment for the obs - */ - public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date newDatetime, String accessionNumber, - String comment, String controlFormPath) { - if (newValue == null || "".equals(newValue)) { - // we want to delete the existing obs - if (log.isDebugEnabled()) { - log.debug("VOID: " + printObsHelper(existingObs)); - } - obsToVoid.add(existingObs); - return; - } - if (concept == null) { - // we want to delete the existing obs - if (log.isDebugEnabled()) - log.debug("VOID: " + printObsHelper(existingObs)); - obsToVoid.add(existingObs); - return; - } - Obs newObs = HtmlFormEntryUtil.createObs(concept, newValue, newDatetime, accessionNumber); - if (controlFormPath != null) { - newObs.setFormField(FORM_NAMESPACE, controlFormPath); - } - String oldString = existingObs.getValueAsString(Context.getLocale()); - String newString = newObs.getValueAsString(Context.getLocale()); - if (log.isDebugEnabled() && concept != null) { - log.debug("For concept " + concept.getName(Context.getLocale()) + ": " + oldString + " -> " + newString); - } - boolean valueChanged = !newString.equals(oldString); - // TODO: handle dates that may equal encounter date - boolean dateChanged = dateChangedHelper(existingObs.getObsDatetime(), newObs.getObsDatetime()); - boolean accessionNumberChanged = accessionNumberChangedHelper(existingObs.getAccessionNumber(), - newObs.getAccessionNumber()); - boolean conceptsHaveChanged = false; - if (!existingObs.getConcept().getConceptId().equals(concept.getConceptId())) { - conceptsHaveChanged = true; - } - if (valueChanged || dateChanged || accessionNumberChanged || conceptsHaveChanged) { - if (log.isDebugEnabled()) { - log.debug("CHANGED: " + printObsHelper(existingObs)); - } - // TODO: really the voided obs should link to the new one, but this is a pain to implement due to the dreaded error: org.hibernate.NonUniqueObjectException: a different object with the same identifier value was already associated with the session - obsToVoid.add(existingObs); - - newObs = createObs(concept, newValue, newDatetime, accessionNumber, comment); - newObs.setPreviousVersion(existingObs); - } else { - if (existingObs != null && StringUtils.isNotBlank(comment)) - existingObs.setComment(comment); - - if (log.isDebugEnabled()) { - log.debug("SAME: " + printObsHelper(existingObs)); - } - } + public void modifyObs(Obs existingObs, Concept concept, Object newValue, Date newDatetime, String accessionNumber) { + modifyObs(existingObs, concept, newValue, newDatetime, accessionNumber, null, null); } /** diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/ObsGroupComponent.java b/api/src/main/java/org/openmrs/module/htmlformentry/ObsGroupComponent.java index 944cb412a..2c7acfaa3 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/ObsGroupComponent.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/ObsGroupComponent.java @@ -30,6 +30,8 @@ public class ObsGroupComponent { private Drug answerDrug; + private String controlId = null; + private Boolean partOfSet = false; private Boolean lastInSet = false; @@ -40,21 +42,25 @@ public class ObsGroupComponent { public ObsGroupComponent() { } - public ObsGroupComponent(Concept question, Concept answer) { + public ObsGroupComponent(Concept question, Concept answer, String controlId) { this.question = question; this.answer = answer; + this.controlId = controlId; } - public ObsGroupComponent(Concept question, Concept answer, Drug answerDrug) { + public ObsGroupComponent(Concept question, Concept answer, Drug answerDrug, String controlId) { this.question = question; this.answer = answer; this.answerDrug = answerDrug; + this.controlId = controlId; } - public ObsGroupComponent(Concept question, Concept answer, Drug answerDrug, Boolean partOfSet, Boolean lastInSet) { + public ObsGroupComponent(Concept question, Concept answer, Drug answerDrug, String controlId, Boolean partOfSet, + Boolean lastInSet) { this.question = question; this.answer = answer; this.answerDrug = answerDrug; + this.controlId = controlId; this.partOfSet = partOfSet; this.lastInSet = lastInSet; } @@ -86,32 +92,7 @@ public Drug getAnswerDrug() { public void setAnswerDrug(Drug answerDrug) { this.answerDrug = answerDrug; } - - @Deprecated - public static boolean supports(List questionsAndAnswers, Obs parentObs, Set group) { - for (Obs obs : group) { - boolean match = false; - for (ObsGroupComponent test : questionsAndAnswers) { - boolean questionMatches = test.getQuestion().getConceptId().equals(obs.getConcept().getConceptId()); - boolean answerMatches = test.getAnswer() == null || (obs.getValueCoded() != null - && test.getAnswer().getConceptId().equals(obs.getValueCoded().getConceptId())); - - if (questionMatches && !answerMatches) { - match = false; - break; - } - - if (questionMatches && answerMatches) { - match = true; - } - } - if (!match) { - return false; - } - } - return true; - } - + public static int supportingRank(List obsGroupComponents, Set obsSet) { int rank = 0; @@ -121,6 +102,13 @@ public static int supportingRank(List obsGroupComponents, Set // iterate though all form obs elements for obs group we are testing for a match against for (ObsGroupComponent obsGroupComponent : obsGroupComponents) { + + // if any one of the obs group components have a control id, and it matches the obs control id, just return 1000 to force a match + if (obsGroupComponent.getControlId() != null + && obsGroupComponent.getControlId().equals(HtmlFormEntryUtil.getControlId(obs))) { + return 1000; + } + Concept groupComponentQuestion = obsGroupComponent.getQuestion(); if (groupComponentQuestion == null) { // The correct error should be thrown with useful contextual information from ObsSubmissionElement:174 @@ -180,7 +168,7 @@ public static List findQuestionsAndAnswersForGroup(String par Pair hiddenObs, Node node) { List ret = new ArrayList(); if (hiddenObs != null) { // consider the hidden obs when making a match - ret.add(new ObsGroupComponent(hiddenObs.getKey(), hiddenObs.getValue(), null, false, false)); + ret.add(new ObsGroupComponent(hiddenObs.getKey(), hiddenObs.getValue(), null, null, false, false)); } findQuestionsAndAnswersForGroupHelper(parentGroupingConceptId, node, ret); return ret; @@ -195,6 +183,7 @@ private static void findQuestionsAndAnswersForGroupHelper(String parentGroupingC Concept answer = null; Drug answerDrug = null; List answersList = null; + String controlId = null; NamedNodeMap attrs = node.getAttributes(); try { String questionsStr = attrs.getNamedItem("conceptIds").getNodeValue(); @@ -250,6 +239,12 @@ private static void findQuestionsAndAnswersForGroupHelper(String parentGroupingC } } } + try { + controlId = attrs.getNamedItem("controlId").getNodeValue(); + } + catch (Exception ex) { + // this is fine + } //determine whether or not the obs group parent of this obs is the obsGroup obs that we're looking at. boolean thisObsInThisGroup = false; @@ -278,17 +273,17 @@ private static void findQuestionsAndAnswersForGroupHelper(String parentGroupingC while (i.hasNext()) { Concept c = i.next(); // add all the answers as separate obs group components, flagging them all as part of a set, and flagging the last one as the last one in the set - obsGroupComponents.add(new ObsGroupComponent(question, c, null, true, !i.hasNext())); + obsGroupComponents.add(new ObsGroupComponent(question, c, null, controlId, true, !i.hasNext())); } } else if (questions != null && questions.size() > 0) { Iterator i = questions.iterator(); while (i.hasNext()) { Concept c = i.next(); // add all the questions as separate obs group components, flagging them all as part of a set, and flagging the last one as the last one in the set - obsGroupComponents.add(new ObsGroupComponent(c, answer, null, true, !i.hasNext())); + obsGroupComponents.add(new ObsGroupComponent(c, answer, null, controlId, true, !i.hasNext())); } } else { - addToObsGroupComponentList(obsGroupComponents, question, answer, answerDrug); + addToObsGroupComponentList(obsGroupComponents, question, answer, answerDrug, controlId); } } } else if ("obsgroup".equals(node.getNodeName())) { @@ -296,7 +291,7 @@ private static void findQuestionsAndAnswersForGroupHelper(String parentGroupingC NamedNodeMap attrs = node.getAttributes(); attrs.getNamedItem("groupingConceptId").getNodeValue(); obsGroupComponents.add(new ObsGroupComponent( - HtmlFormEntryUtil.getConcept(attrs.getNamedItem("groupingConceptId").getNodeValue()), null)); + HtmlFormEntryUtil.getConcept(attrs.getNamedItem("groupingConceptId").getNodeValue()), null, null)); } catch (Exception ex) { throw new RuntimeException("Unable to get groupingConcept out of obsgroup tag."); @@ -310,7 +305,7 @@ private static void findQuestionsAndAnswersForGroupHelper(String parentGroupingC // see: https://issues.openmrs.org/browse/HTML-806 private static void addToObsGroupComponentList(List list, Concept question, Concept answer, - Drug answerDrug) { + Drug answerDrug, String controlId) { boolean isSet = false; // if there are any existing components with the same question, make sure they are flagged as part of set, but *not* the last one in the set for (ObsGroupComponent component : list) { @@ -321,7 +316,7 @@ private static void addToObsGroupComponentList(List list, Con } } // if existing components with the same question were found, flag this new component as part of a set, *and* the last element in the set - list.add(new ObsGroupComponent(question, answer, answerDrug, isSet, isSet)); + list.add(new ObsGroupComponent(question, answer, answerDrug, controlId, isSet, isSet)); } /** @@ -378,4 +373,8 @@ public Boolean isLastInSet() { public void setLastInSet(Boolean lastInSet) { this.lastInSet = lastInSet; } + + public String getControlId() { + return controlId; + } } diff --git a/api/src/test/java/org/openmrs/module/htmlformentry/FormNamespaceAndPathRegressionTest.java b/api/src/test/java/org/openmrs/module/htmlformentry/FormNamespaceAndPathRegressionTest.java index 4b69bf00d..38bcfc081 100644 --- a/api/src/test/java/org/openmrs/module/htmlformentry/FormNamespaceAndPathRegressionTest.java +++ b/api/src/test/java/org/openmrs/module/htmlformentry/FormNamespaceAndPathRegressionTest.java @@ -2,16 +2,21 @@ import org.junit.Assert; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import org.openmrs.Encounter; import org.openmrs.Obs; +import org.openmrs.api.APIException; +import org.openmrs.api.context.Context; import org.springframework.mock.web.MockHttpServletRequest; import java.awt.image.BufferedImage; import java.awt.image.WritableRaster; import java.util.Date; import java.util.Iterator; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; public class FormNamespaceAndPathRegressionTest extends BaseHtmlFormEntryTest { @@ -132,6 +137,185 @@ public void testResults(SubmissionResults results) { }.run(); } + @Test + public void testDoubleObsGroupFormWithControlIdSavesNamespaceAndPath() throws Exception { + final Date date = new Date(); + new RegressionTestHelper() { + + @Override + public String getFormName() { + return "doubleObsGroupFormWithControlId"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Allergy 1:", "Allergy 1 Date:", "Allergy 2:", + "Allergy 2 Date:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(date)); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Allergy 1:"), "Bee stings"); + request.addParameter(widgets.get("Allergy 1 Date:"), dateAsString(date)); + request.addParameter(widgets.get("Allergy 2:"), "Lactose"); + request.addParameter(widgets.get("Allergy 2 Date:"), dateAsString(date)); + } + + @Override + public void testResults(SubmissionResults results) { + results.assertNoErrors(); + Encounter encounter = results.getEncounterCreated(); + Assert.assertEquals(4, encounter.getObs().size()); + + List formFieldPaths = encounter.getObs().stream().map(Obs::getFormFieldPath) + .collect(Collectors.toList()); + + // note that "FakeForm and 2.0" are just hardcoded into the Regression Test Helper context setup + Assert.assertTrue(formFieldPaths.contains("FakeForm.2.0/allergy-1-0")); + Assert.assertTrue(formFieldPaths.contains("FakeForm.2.0/allergy-2-0")); + Assert.assertTrue(formFieldPaths.contains("FakeForm.2.0/allergy-date-1-0")); + Assert.assertTrue(formFieldPaths.contains("FakeForm.2.0/allergy-date-2-0")); + + } + }.run(); + } + + @Test + public void testDoubleObsGroupFormWithControlIdCorrectlyReloadsObs() throws Exception { + final Date date = new Date(); + new RegressionTestHelper() { + + @Override + public String getFormName() { + return "doubleObsGroupFormWithControlId"; + } + + @Override + public Encounter getEncounterToView() throws Exception { + Encounter e = new Encounter(); + e.setPatient(getPatient()); + Date date = Context.getDateFormat().parse("01/02/2003"); + e.setDateCreated(new Date()); + e.setEncounterType(Context.getEncounterService().getEncounterType(1)); + e.setEncounterDatetime(date); + e.setLocation(Context.getLocationService().getLocation(2)); + e.addProvider(Context.getEncounterService().getEncounterRole(1), + Context.getProviderService().getProvider(1)); + + // first create the second allergies obs group + Obs allergy2Parent = TestUtil.createObs(e, 70000, null, date); + e.addObs(allergy2Parent); + Obs allergy2 = TestUtil.createObs(e, 80000, "Lactose", date); + allergy2.setFormField(null, "FakeForm.2.0/allergy-2-0"); + allergy2Parent.addGroupMember(allergy2); + + // then create the first allergies obs group + Obs allergy1Parent = TestUtil.createObs(e, 70000, null, date); + e.addObs(allergy1Parent); + Obs allergy1 = TestUtil.createObs(e, 80000, "Bee stings", date); + allergy1.setFormField(null, "FakeForm.2.0/allergy-1-0"); + allergy1Parent.addGroupMember(allergy1); + + e = Context.getEncounterService().saveEncounter(e); + return e; + } + + @Override + public void testViewingEncounter(Encounter encounter, String html) { + TestUtil.assertFuzzyContains("Allergy 1:Bee stings", html); + TestUtil.assertFuzzyContains("Allergy 2:Lactose", html); + } + }.run(); + } + + @Test + public void testDoubleObsGroupFormWithControlIdCorrectlyEditsObs() throws Exception { + final Date date = new Date(); + new RegressionTestHelper() { + + @Override + public String getFormName() { + return "doubleObsGroupFormWithControlId"; + } + + @Override + public Encounter getEncounterToEdit() { + Encounter e = new Encounter(); + e.setPatient(getPatient()); + Date date; + try { + date = Context.getDateFormat().parse("01/02/2003"); + } + catch (Exception ex) { + throw new APIException(); + } + + e.setDateCreated(new Date()); + e.setEncounterType(Context.getEncounterService().getEncounterType(1)); + e.setEncounterDatetime(date); + e.setLocation(Context.getLocationService().getLocation(2)); + e.addProvider(Context.getEncounterService().getEncounterRole(1), + Context.getProviderService().getProvider(1)); + + // first create the second allergies obs group + Obs allergy2Parent = TestUtil.createObs(e, 70000, null, date); + e.addObs(allergy2Parent); + Obs allergy2 = TestUtil.createObs(e, 80000, "Lactose", date); + allergy2.setFormField(null, "FakeForm.2.0/allergy-2-0"); + allergy2Parent.addGroupMember(allergy2); + + // then create the first allergies obs group + Obs allergy1Parent = TestUtil.createObs(e, 70000, null, date); + e.addObs(allergy1Parent); + Obs allergy1 = TestUtil.createObs(e, 80000, "Bee stings", date); + allergy1.setFormField(null, "FakeForm.2.0/allergy-1-0"); + allergy1Parent.addGroupMember(allergy1); + + e = Context.getEncounterService().saveEncounter(e); + return e; + } + + @Override + public boolean doEditEncounter() { + return true; + } + + @Override + public String[] widgetLabelsForEdit() { + return new String[] { "Allergy 1:", "Allergy 2:" }; + } + + @Override + public void setupEditRequest(MockHttpServletRequest request, Map widgets) { + request.setParameter(widgets.get("Allergy 1:"), "Wasp stings"); + request.setParameter(widgets.get("Allergy 2:"), "Milk"); + } + + @Override + public void testEditedResults(SubmissionResults results) { + results.assertNoErrors(); + Encounter encounter = results.getEncounterCreated(); + Assert.assertEquals(2, encounter.getObs().size()); + + List obsValues = encounter.getObs().stream().map(Obs::getValueText).collect(Collectors.toList()); + Assert.assertTrue(obsValues.contains("Wasp stings")); + Assert.assertTrue(obsValues.contains("Milk")); + + List formFieldPaths = encounter.getObs().stream().map(Obs::getFormFieldPath) + .collect(Collectors.toList()); + + // testing that the form field paths were preserved, see https://openmrs.atlassian.net/browse/HTML-841 + // note that "FakeForm and 2.0" are just hardcoded into the Regression Test Helper context setup + Assert.assertTrue(formFieldPaths.contains("FakeForm.2.0/allergy-1-0")); + Assert.assertTrue(formFieldPaths.contains("FakeForm.2.0/allergy-2-0")); + } + + }.run(); + } + private BufferedImage createImage() { int width = 10; int height = 10; diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/include/doubleObsGroupFormWithControlId.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/include/doubleObsGroupFormWithControlId.xml new file mode 100644 index 000000000..ff39e095b --- /dev/null +++ b/api/src/test/resources/org/openmrs/module/htmlformentry/include/doubleObsGroupFormWithControlId.xml @@ -0,0 +1,14 @@ + + Date: + Location: + Provider: + + Allergy 1: + Allergy 1 Date: + + + Allergy 2: + Allergy 2 Date: + + + diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/include/singleObsGroupFormWithControlId.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/include/singleObsGroupFormWithControlId.xml deleted file mode 100644 index ca45d9dc5..000000000 --- a/api/src/test/resources/org/openmrs/module/htmlformentry/include/singleObsGroupFormWithControlId.xml +++ /dev/null @@ -1,10 +0,0 @@ - - Date: - Location: - Provider: - - Allergy: - Allergy Date: - - -