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

HTML-841: Errors with Form Namespace and Path #301

Merged
merged 3 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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) {
Expand All @@ -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))
Expand All @@ -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.
* <p/>
* 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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public class ObsGroupComponent {

private Drug answerDrug;

private String controlId = null;

private Boolean partOfSet = false;

private Boolean lastInSet = false;
Expand All @@ -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;
}
Expand Down Expand Up @@ -86,32 +92,7 @@ public Drug getAnswerDrug() {
public void setAnswerDrug(Drug answerDrug) {
this.answerDrug = answerDrug;
}

@Deprecated
Copy link
Member Author

Choose a reason for hiding this comment

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

Just removed this as it was deprecated and not used anywhere as far as I could tell

public static boolean supports(List<ObsGroupComponent> questionsAndAnswers, Obs parentObs, Set<Obs> 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<ObsGroupComponent> obsGroupComponents, Set<Obs> obsSet) {
int rank = 0;

Expand All @@ -121,6 +102,13 @@ public static int supportingRank(List<ObsGroupComponent> 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))) {
Copy link
Member

Choose a reason for hiding this comment

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

Defer to you as to whether this 1000 value is sufficiently safe, vs. (eg) max value integer

Copy link
Member Author

Choose a reason for hiding this comment

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

We used -1000 for the max negative, so just using this for consistency. I'm pretty sure it sufficiently safe.

return 1000;
}

Concept groupComponentQuestion = obsGroupComponent.getQuestion();
if (groupComponentQuestion == null) {
// The correct error should be thrown with useful contextual information from ObsSubmissionElement:174
Expand Down Expand Up @@ -180,7 +168,7 @@ public static List<ObsGroupComponent> findQuestionsAndAnswersForGroup(String par
Pair<Concept, Concept> hiddenObs, Node node) {
List<ObsGroupComponent> ret = new ArrayList<ObsGroupComponent>();
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;
Expand All @@ -195,6 +183,7 @@ private static void findQuestionsAndAnswersForGroupHelper(String parentGroupingC
Concept answer = null;
Drug answerDrug = null;
List<Concept> answersList = null;
String controlId = null;
NamedNodeMap attrs = node.getAttributes();
try {
String questionsStr = attrs.getNamedItem("conceptIds").getNodeValue();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -278,25 +273,25 @@ 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<Concept> 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())) {
try {
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.");
Expand All @@ -310,7 +305,7 @@ private static void findQuestionsAndAnswersForGroupHelper(String parentGroupingC

// see: https://issues.openmrs.org/browse/HTML-806
private static void addToObsGroupComponentList(List<ObsGroupComponent> 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) {
Expand All @@ -321,7 +316,7 @@ private static void addToObsGroupComponentList(List<ObsGroupComponent> 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));
}

/**
Expand Down Expand Up @@ -378,4 +373,8 @@ public Boolean isLastInSet() {
public void setLastInSet(Boolean lastInSet) {
this.lastInSet = lastInSet;
}

public String getControlId() {
return controlId;
}
}
Loading
Loading