From ac0d87c62501daccb363079a13ea1f331e110f1d Mon Sep 17 00:00:00 2001 From: mogoodrich Date: Wed, 27 May 2026 13:33:16 -0400 Subject: [PATCH 01/14] HTML-883: add personAttribute tag for viewing/entering/editing person attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new tag that supports viewing, entering, and editing person attributes within HTML forms. Supports String, Concept, and Location PersonAttributeType formats. Features: - attributeType attribute (UUID of a PersonAttributeType) — required - VIEW mode: displays the current attribute value (concept/location rendered by name, not raw ID) - ENTER/EDIT modes: renders appropriate widget (TextFieldWidget for String, DropdownWidget for Concept/Location) - Concept type: mandatory answerConceptIds (comma-separated IDs/UUIDs) - Location type: optional tags attribute (comma-separated tag names) to filter the location dropdown - When multiple non-voided attributes of the same type exist, the most recently created one is used and a warning is logged - Blank submission voids an existing attribute New files: - PersonAttributeTagHandler.java — tag handler registered in moduleApplicationContext.xml - PersonAttributeSubmissionElement.java — widget + submission logic - PersonAttributeTagTest.java — 16 regression tests covering all cases - 5 test form XML fixtures (String, Concept, Location, Location+tag, Location-filter-only) Modified files: - moduleApplicationContext.xml — registers 'personAttribute' handler - RegressionTest-data-openmrs-2.8.xml — adds PersonAttributeType 19 (Location), a location attribute for patient 2, and two Civil Status attributes for patient 2 (for multiple-attribute warning tests) Co-Authored-By: Claude Sonnet 4.6 --- .../PersonAttributeSubmissionElement.java | 334 ++++++++ .../handler/PersonAttributeTagHandler.java | 56 ++ .../resources/moduleApplicationContext.xml | 3 + .../htmlformentry/PersonAttributeTagTest.java | 775 ++++++++++++++++++ .../data/RegressionTest-data-openmrs-2.8.xml | 17 + .../include/personAttributeConceptForm.xml | 7 + .../personAttributeLocationFilterOnlyForm.xml | 6 + .../include/personAttributeLocationForm.xml | 7 + .../personAttributeLocationWithTagForm.xml | 7 + .../include/personAttributeStringForm.xml | 7 + 10 files changed, 1219 insertions(+) create mode 100644 api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java create mode 100644 api/src/main/java/org/openmrs/module/htmlformentry/handler/PersonAttributeTagHandler.java create mode 100644 api/src/test/java/org/openmrs/module/htmlformentry/PersonAttributeTagTest.java create mode 100644 api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeConceptForm.xml create mode 100644 api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationFilterOnlyForm.xml create mode 100644 api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationForm.xml create mode 100644 api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationWithTagForm.xml create mode 100644 api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeStringForm.xml diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java new file mode 100644 index 000000000..c4e35b441 --- /dev/null +++ b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java @@ -0,0 +1,334 @@ +package org.openmrs.module.htmlformentry.element; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.Date; +import java.util.List; +import java.util.Map; + +import javax.servlet.http.HttpServletRequest; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.openmrs.Concept; +import org.openmrs.Location; +import org.openmrs.LocationTag; +import org.openmrs.Patient; +import org.openmrs.PersonAttribute; +import org.openmrs.PersonAttributeType; +import org.openmrs.api.context.Context; +import org.openmrs.module.htmlformentry.BadFormDesignException; +import org.openmrs.module.htmlformentry.FormEntryContext; +import org.openmrs.module.htmlformentry.FormEntryContext.Mode; +import org.openmrs.module.htmlformentry.FormEntrySession; +import org.openmrs.module.htmlformentry.FormSubmissionError; +import org.openmrs.module.htmlformentry.HtmlFormEntryUtil; +import org.openmrs.module.htmlformentry.action.FormSubmissionControllerAction; +import org.openmrs.module.htmlformentry.comparator.OptionComparator; +import org.openmrs.module.htmlformentry.widget.DropdownWidget; +import org.openmrs.module.htmlformentry.widget.ErrorWidget; +import org.openmrs.module.htmlformentry.widget.Option; +import org.openmrs.module.htmlformentry.widget.TextFieldWidget; +import org.openmrs.module.htmlformentry.widget.Widget; +import org.openmrs.module.htmlformentry.widget.WidgetFactory; +import org.springframework.util.StringUtils; + +/** + * Holds the widget and submission logic for the {@code } tag. + * + *

Supports three PersonAttributeType formats: + *

    + *
  • {@code java.lang.String} – plain text input
    • + *
  • {@code org.openmrs.Concept} – dropdown of concepts specified via {@code answerConceptIds}
    • + *
  • {@code org.openmrs.Location} – dropdown of locations, optionally filtered by {@code tags}
    • + *
+ * + *

When multiple non-voided attributes of the requested type exist for the patient, the most + * recently created one is used and a warning is logged. + */ +public class PersonAttributeSubmissionElement implements HtmlGeneratorElement, FormSubmissionControllerAction { + + private static final String FORMAT_STRING = "java.lang.String"; + + private static final String FORMAT_CONCEPT = "org.openmrs.Concept"; + + private static final String FORMAT_LOCATION = "org.openmrs.Location"; + + protected final Log log = LogFactory.getLog(getClass()); + + /** The resolved attribute type. */ + private final PersonAttributeType attributeType; + + /** The widget used in ENTER / EDIT modes (null in VIEW mode). */ + private Widget valueWidget; + + /** The error widget (null in VIEW mode). */ + private ErrorWidget errorWidget; + + /** The existing non-voided attribute (most recent if there are several), or null. */ + private PersonAttribute existingAttribute; + + /** Format of the attribute type (java.lang.String / org.openmrs.Concept / org.openmrs.Location). */ + private final String format; + + public PersonAttributeSubmissionElement(FormEntryContext context, Map parameters) + throws BadFormDesignException { + + // ---- 1. Resolve the PersonAttributeType ---------------------------------------- + String attributeTypeUuid = parameters.get("attributeType"); + if (!StringUtils.hasText(attributeTypeUuid)) { + throw new BadFormDesignException(" tag requires an \"attributeType\" attribute"); + } + attributeType = Context.getPersonService().getPersonAttributeTypeByUuid(attributeTypeUuid); + if (attributeType == null) { + throw new BadFormDesignException( + " tag: PersonAttributeType not found for uuid=\"" + attributeTypeUuid + "\""); + } + + format = attributeType.getFormat(); + + // ---- 2. Find existing attribute on the patient --------------------------------- + Patient patient = context.getExistingPatient(); + if (patient != null) { + List matching = new ArrayList(); + for (PersonAttribute attr : patient.getActiveAttributes()) { + if (attr.getAttributeType().equals(attributeType)) { + matching.add(attr); + } + } + if (matching.size() > 1) { + log.warn("Patient " + patient.getPatientId() + " has " + matching.size() + + " non-voided PersonAttributes of type \"" + attributeType.getName() + + "\"; using most recent."); + matching.sort(Comparator.comparing(PersonAttribute::getDateCreated).reversed()); + } + existingAttribute = matching.isEmpty() ? null : matching.get(0); + } + + // ---- 3. VIEW mode: no widget needed ------------------------------------------- + if (context.getMode() == Mode.VIEW) { + return; + } + + // ---- 4. ENTER / EDIT modes: build widget by format ---------------------------- + errorWidget = new ErrorWidget(); + + if (FORMAT_STRING.equals(format)) { + valueWidget = buildStringWidget(context); + + } else if (FORMAT_CONCEPT.equals(format)) { + valueWidget = buildConceptWidget(context, parameters); + + } else if (FORMAT_LOCATION.equals(format)) { + valueWidget = buildLocationWidget(context, parameters); + + } else { + throw new BadFormDesignException(" tag: unsupported attribute format \"" + format + + "\" for type \"" + attributeType.getName() + "\". Supported formats: " + + FORMAT_STRING + ", " + FORMAT_CONCEPT + ", " + FORMAT_LOCATION); + } + + context.registerWidget(valueWidget); + context.registerErrorWidget(valueWidget, errorWidget); + } + + // --------------------------------------------------------------------------------- + // Widget builders + // --------------------------------------------------------------------------------- + + private Widget buildStringWidget(FormEntryContext context) { + TextFieldWidget w = new TextFieldWidget(); + if (existingAttribute != null) { + w.setInitialValue(existingAttribute.getValue()); + } + return w; + } + + private Widget buildConceptWidget(FormEntryContext context, Map parameters) + throws BadFormDesignException { + + String answerConceptIds = parameters.get("answerConceptIds"); + if (!StringUtils.hasText(answerConceptIds)) { + throw new BadFormDesignException(" tag: \"answerConceptIds\" is required " + + "for PersonAttributeType \"" + attributeType.getName() + "\" (format=" + FORMAT_CONCEPT + ")"); + } + + DropdownWidget w = new DropdownWidget(); + + // blank / choose prompt + w.addOption(new Option(Context.getMessageSourceService().getMessage("htmlformentry.chooseOption"), "", false)); + + for (String idOrUuid : answerConceptIds.split(",")) { + String trimmed = idOrUuid.trim(); + if (trimmed.isEmpty()) { + continue; + } + Concept concept = HtmlFormEntryUtil.getConcept(trimmed); + if (concept == null) { + throw new BadFormDesignException( + " tag: cannot find concept for answerConceptIds value \"" + trimmed + "\""); + } + w.addOption(new Option(concept.getName().getName(), concept.getConceptId().toString(), false)); + } + + if (existingAttribute != null && StringUtils.hasText(existingAttribute.getValue())) { + w.setInitialValue(existingAttribute.getValue()); + } + + return w; + } + + private Widget buildLocationWidget(FormEntryContext context, Map parameters) + throws BadFormDesignException { + + DropdownWidget w = new DropdownWidget(); + + // blank / choose prompt + w.addOption(new Option(Context.getMessageSourceService().getMessage("htmlformentry.chooseALocation"), "", false)); + + // Build location list + List locations; + String tagsParam = parameters.get("tags"); + if (StringUtils.hasText(tagsParam)) { + List locationTags = new ArrayList(); + for (String tagName : tagsParam.split(",")) { + String trimmed = tagName.trim(); + if (trimmed.isEmpty()) { + continue; + } + LocationTag tag = HtmlFormEntryUtil.getLocationTag(trimmed); + if (tag == null) { + throw new BadFormDesignException( + " tag: cannot find location tag \"" + trimmed + "\""); + } + locationTags.add(tag); + } + locations = Context.getLocationService().getLocationsHavingAnyTag(locationTags); + } else { + locations = Context.getLocationService().getAllLocations(false); + } + + // Build and sort options + List

Supported attributes: + *

    + *
  • {@code attributeType} (required) – UUID of the {@link PersonAttributeType}
    • + *
  • {@code answerConceptIds} (required for Concept-format types) – comma-separated list of + * concept IDs or UUIDs shown in the dropdown
    • + *
  • {@code tags} (optional, Location-format types only) – comma-separated location tag names + * used to filter the location dropdown
    • + *
+ */ +public class PersonAttributeTagHandler extends AbstractTagHandler { + + @Override + protected List createAttributeDescriptors() { + List descriptors = new ArrayList(); + descriptors.add(new AttributeDescriptor("attributeType", PersonAttributeType.class)); + descriptors.add(new AttributeDescriptor("answerConceptIds", Concept.class)); + descriptors.add(new AttributeDescriptor("tags", LocationTag.class)); + return Collections.unmodifiableList(descriptors); + } + + @Override + public boolean doStartTag(FormEntrySession session, PrintWriter out, Node parent, Node node) + throws BadFormDesignException { + FormEntryContext context = session.getContext(); + PersonAttributeSubmissionElement element = new PersonAttributeSubmissionElement(context, getAttributes(node)); + session.getSubmissionController().addAction(element); + out.print(element.generateHtml(context)); + return false; // no body to process + } + + @Override + public void doEndTag(FormEntrySession session, PrintWriter out, Node parent, Node node) + throws BadFormDesignException { + // nothing needed + } +} diff --git a/api/src/main/resources/moduleApplicationContext.xml b/api/src/main/resources/moduleApplicationContext.xml index 12061c426..2b4607b8b 100644 --- a/api/src/main/resources/moduleApplicationContext.xml +++ b/api/src/main/resources/moduleApplicationContext.xml @@ -116,6 +116,9 @@ + + + diff --git a/api/src/test/java/org/openmrs/module/htmlformentry/PersonAttributeTagTest.java b/api/src/test/java/org/openmrs/module/htmlformentry/PersonAttributeTagTest.java new file mode 100644 index 000000000..85d38448f --- /dev/null +++ b/api/src/test/java/org/openmrs/module/htmlformentry/PersonAttributeTagTest.java @@ -0,0 +1,775 @@ +package org.openmrs.module.htmlformentry; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.util.Date; +import java.util.Map; + +import org.junit.Before; +import org.junit.Test; +import org.openmrs.Patient; +import org.openmrs.PersonAttribute; +import org.openmrs.PersonAttributeType; +import org.openmrs.api.context.Context; +import org.springframework.mock.web.MockHttpServletRequest; + +/** + * Regression tests for the {@code } tag. + * + *

Covers String, Concept, and Location attribute types in VIEW, ENTER, and EDIT modes, plus + * warning behaviour when multiple non-voided attributes exist, and error cases. + * + *

Standard test data note: + *

    + *
  • PersonAttributeType 2 ("Birthplace") – format {@code java.lang.String}
    • + *
  • PersonAttributeType 8 ("Civil Status") – format {@code org.openmrs.Concept}
    • + *
  • PersonAttributeType 19 ("Birthplace Location") – format {@code org.openmrs.Location} + * (added in RegressionTest-data-openmrs-2.8.xml)
    • + *
  • Patient 7 has NULL-valued Birthplace and Civil Status attributes; no Birthplace Location + * attribute. Used for ENTER/EDIT tests.
    • + *
  • Patient 2 has TWO non-voided Civil Status attributes (ids 21 and 22) pre-loaded in the + * test dataset; id=22 (value="1001"/PENICILLIN) is the most-recent one.
    • + *
+ */ +public class PersonAttributeTagTest extends BaseHtmlFormEntryTest { + + /** PersonAttributeType 2 – Birthplace – format=java.lang.String */ + private static final String STRING_ATTR_TYPE_UUID = "54fc8400-1683-4d71-a1ac-98d40836ff7c"; + + /** PersonAttributeType 8 – Civil Status – format=org.openmrs.Concept */ + private static final String CONCEPT_ATTR_TYPE_UUID = "a0f5521c-dbbd-4c10-81b2-1b7ab18330df"; + + /** PersonAttributeType 19 – Birthplace Location – format=org.openmrs.Location */ + private static final String LOCATION_ATTR_TYPE_UUID = "a6e61b61-5f1a-4a19-8a4c-test00000001"; + + /** Patient 7 has null-valued attributes and is safe to use for ENTER/EDIT tests. */ + private static final int PATIENT_ID = 7; + + @Before + public void setUp() throws Exception { + executeVersionedDataSet("org/openmrs/module/htmlformentry/data/RegressionTest-data-openmrs-2.8.xml"); + } + + // ========================================================================= + // String attribute type tests + // ========================================================================= + + @Test + public void shouldEnterStringPersonAttribute() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeStringForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Birthplace:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Birthplace:"), "Paris"); + } + + @Override + public void testResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + PersonAttributeType type = Context.getPersonService().getPersonAttributeTypeByUuid(STRING_ATTR_TYPE_UUID); + PersonAttribute attr = patient.getAttribute(type); + assertNotNull("Expected a Birthplace attribute after ENTER", attr); + assertEquals("Paris", attr.getValue()); + } + }.run(); + } + + @Test + public void shouldDisplayStringPersonAttributeInViewMode() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeStringForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Birthplace:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Birthplace:"), "Tokyo"); + } + + @Override + public boolean doViewPatient() { + return true; + } + + @Override + public void testViewingPatient(Patient patient, String html) { + assertTrue("Expected VIEW HTML to contain the entered value 'Tokyo'", html.contains("Tokyo")); + } + }.run(); + } + + @Test + public void shouldEditStringPersonAttribute() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeStringForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Birthplace:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Birthplace:"), "Paris"); + } + + @Override + public boolean doEditEncounter() { + return true; + } + + @Override + public String[] widgetLabelsForEdit() { + return widgetLabels(); + } + + @Override + public void setupEditRequest(MockHttpServletRequest request, Map widgets) { + // Use setParameter to replace the pre-populated value from the rendered EDIT form. + request.setParameter(widgets.get("Date:"), dateAsString(new Date())); + request.setParameter(widgets.get("Location:"), "2"); + request.setParameter(widgets.get("Provider:"), "502"); + request.setParameter(widgets.get("Birthplace:"), "Rome"); + } + + @Override + public void testEditedResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + PersonAttributeType type = Context.getPersonService().getPersonAttributeTypeByUuid(STRING_ATTR_TYPE_UUID); + PersonAttribute attr = patient.getAttribute(type); + assertNotNull("Expected a Birthplace attribute after EDIT", attr); + assertEquals("Rome", attr.getValue()); + } + }.run(); + } + + @Test + public void shouldVoidStringPersonAttributeWhenBlankValueSubmitted() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeStringForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Birthplace:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Birthplace:"), "Paris"); + } + + @Override + public boolean doEditEncounter() { + return true; + } + + @Override + public String[] widgetLabelsForEdit() { + return widgetLabels(); + } + + @Override + public void setupEditRequest(MockHttpServletRequest request, Map widgets) { + // Use setParameter to replace the pre-populated value from the rendered EDIT form. + request.setParameter(widgets.get("Date:"), dateAsString(new Date())); + request.setParameter(widgets.get("Location:"), "2"); + request.setParameter(widgets.get("Provider:"), "502"); + // Submit blank value → should void the existing attribute + request.setParameter(widgets.get("Birthplace:"), ""); + } + + @Override + public void testEditedResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + PersonAttributeType type = Context.getPersonService().getPersonAttributeTypeByUuid(STRING_ATTR_TYPE_UUID); + PersonAttribute attr = patient.getAttribute(type); + assertNull("Expected no active Birthplace attribute after blank submission (should be voided)", attr); + } + }.run(); + } + + // ========================================================================= + // Concept attribute type tests + // ========================================================================= + + @Test + public void shouldEnterConceptPersonAttribute() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeConceptForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Civil Status:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + // Concept 1001 = PENICILLIN (from answerConceptIds in the form) + request.addParameter(widgets.get("Civil Status:"), "1001"); + } + + @Override + public void testResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + PersonAttributeType type = Context.getPersonService().getPersonAttributeTypeByUuid(CONCEPT_ATTR_TYPE_UUID); + PersonAttribute attr = patient.getAttribute(type); + assertNotNull("Expected a Civil Status attribute after ENTER", attr); + assertEquals("Stored value should be the concept ID", "1001", attr.getValue()); + } + }.run(); + } + + @Test + public void shouldDisplayConceptPersonAttributeNameInViewMode() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeConceptForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Civil Status:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Civil Status:"), "1001"); + } + + @Override + public boolean doViewPatient() { + return true; + } + + @Override + public void testViewingPatient(Patient patient, String html) { + // Concept 1001 is named "PENICILLIN" in test data + assertTrue("Expected VIEW HTML to contain the concept name 'PENICILLIN'", html.contains("PENICILLIN")); + } + }.run(); + } + + @Test + public void shouldEditConceptPersonAttribute() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeConceptForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Civil Status:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Civil Status:"), "1001"); + } + + @Override + public boolean doEditEncounter() { + return true; + } + + @Override + public String[] widgetLabelsForEdit() { + return widgetLabels(); + } + + @Override + public void setupEditRequest(MockHttpServletRequest request, Map widgets) { + // Use setParameter to replace the pre-populated value from the rendered EDIT form. + request.setParameter(widgets.get("Date:"), dateAsString(new Date())); + request.setParameter(widgets.get("Location:"), "2"); + request.setParameter(widgets.get("Provider:"), "502"); + request.setParameter(widgets.get("Civil Status:"), "1002"); + } + + @Override + public void testEditedResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + PersonAttributeType type = Context.getPersonService().getPersonAttributeTypeByUuid(CONCEPT_ATTR_TYPE_UUID); + PersonAttribute attr = patient.getAttribute(type); + assertNotNull("Expected a Civil Status attribute after EDIT", attr); + assertEquals("1002", attr.getValue()); + } + }.run(); + } + + // ========================================================================= + // Location attribute type tests + // ========================================================================= + + @Test + public void shouldEnterLocationPersonAttribute() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeLocationForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Birthplace Location:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + // Location 1001 = Kigali + request.addParameter(widgets.get("Birthplace Location:"), "1001"); + } + + @Override + public void testResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + PersonAttributeType type = Context.getPersonService().getPersonAttributeTypeByUuid(LOCATION_ATTR_TYPE_UUID); + PersonAttribute attr = patient.getAttribute(type); + assertNotNull("Expected a Birthplace Location attribute after ENTER", attr); + assertEquals("Stored value should be the location ID", "1001", attr.getValue()); + } + }.run(); + } + + @Test + public void shouldDisplayLocationPersonAttributeNameInViewMode() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeLocationForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Birthplace Location:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Birthplace Location:"), "1001"); + } + + @Override + public boolean doViewPatient() { + return true; + } + + @Override + public void testViewingPatient(Patient patient, String html) { + // Location 1001 = Kigali + assertTrue("Expected VIEW HTML to contain the location name 'Kigali'", html.contains("Kigali")); + } + }.run(); + } + + @Test + public void shouldEditLocationPersonAttribute() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeLocationForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Birthplace Location:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Birthplace Location:"), "1001"); + } + + @Override + public boolean doEditEncounter() { + return true; + } + + @Override + public String[] widgetLabelsForEdit() { + return widgetLabels(); + } + + @Override + public void setupEditRequest(MockHttpServletRequest request, Map widgets) { + // Use setParameter to replace the pre-populated value from the rendered EDIT form. + request.setParameter(widgets.get("Date:"), dateAsString(new Date())); + request.setParameter(widgets.get("Location:"), "2"); + request.setParameter(widgets.get("Provider:"), "502"); + // Change to location 1002 = Mirebalais + request.setParameter(widgets.get("Birthplace Location:"), "1002"); + } + + @Override + public void testEditedResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + PersonAttributeType type = Context.getPersonService().getPersonAttributeTypeByUuid(LOCATION_ATTR_TYPE_UUID); + PersonAttribute attr = patient.getAttribute(type); + assertNotNull("Expected a Birthplace Location attribute after EDIT", attr); + assertEquals("1002", attr.getValue()); + } + }.run(); + } + + /** + * When the {@code tags} attribute is specified on a Location-type personAttribute, only locations + * tagged with the specified tag(s) should appear in the dropdown. This form omits + * {@code } to avoid its full location list contaminating the assertions. + */ + @Test + public void shouldFilterLocationDropdownByLocationTag() throws Exception { + new RegressionTestHelper() { + + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public String getFormName() { + return "personAttributeLocationFilterOnlyForm"; + } + + @Override + public void testBlankFormHtml(String html) { + // Locations tagged with "Some Tag": 1001 = Kigali, 1002 = Mirebalais + assertTrue("Expected Kigali (tagged 'Some Tag') in dropdown", html.contains("Kigali")); + assertTrue("Expected Mirebalais (tagged 'Some Tag') in dropdown", html.contains("Mirebalais")); + // Locations with ONLY "Another Tag": Boston (1004), Scituate (1005) must NOT appear + assertFalse("Expected Boston (not tagged 'Some Tag') to be absent", html.contains("Boston")); + assertFalse("Expected Scituate (not tagged 'Some Tag') to be absent", html.contains("Scituate")); + } + }.run(); + } + + // ========================================================================= + // Multiple non-voided attributes – warning and most-recent selection + // ========================================================================= + + /** + * When a patient has more than one non-voided attribute of the same type, the element should + * render successfully (logging a warning) and use the most recently created attribute. + * + *

Patient 2 has two Civil Status attributes pre-loaded in the test dataset: + *

    + *
  • id=21: value="6", date_created=2020-01-01 (older)
    • + *
  • id=22: value="1001" (PENICILLIN), date_created=2024-01-01 (more recent)
    • + *
+ */ + @Test + public void shouldUseMostRecentAttributeWhenMultipleNonVoidedExistInViewMode() throws Exception { + // Patient 2 already has two non-voided Civil Status attributes in the test dataset. + // The most-recent one (id=22, value="1001"/PENICILLIN) should be displayed in VIEW mode. + final Patient viewPatient = Context.getPatientService().getPatient(2); + + new RegressionTestHelper() { + + /** Use patient 7 for the required ENTER phase; patient 2 for VIEW. */ + @Override + public Patient getPatient() { + return Context.getPatientService().getPatient(PATIENT_ID); + } + + @Override + public Patient getPatientToView() { + return viewPatient; + } + + @Override + public String getFormName() { + return "personAttributeConceptForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Civil Status:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + // A valid ENTER for patient 7; the Civil Status attribute is intentionally left blank + // so patient 7's data is not affected. + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Civil Status:"), ""); + } + + @Override + public void testViewingPatient(Patient patient, String html) { + // The most-recent attribute (value "1001" / PENICILLIN) should be displayed. + // The form must render without exception even with two active attributes. + assertNotNull("Expected non-null HTML when multiple attributes exist", html); + assertTrue("Expected most-recent attribute name 'PENICILLIN' in VIEW HTML", + html.contains("PENICILLIN")); + } + }.run(); + } + + /** + * When a patient has multiple non-voided attributes of the same type and the form is edited, + * the most-recently-created attribute should be the one updated. + * + *

Patient 2 has two Civil Status attributes pre-loaded in the test dataset: + *

    + *
  • id=21: value="6", date_created=2020-01-01 (older)
    • + *
  • id=22: value="1001" (PENICILLIN), date_created=2024-01-01 (more recent)
    • + *
+ */ + @Test + public void shouldUseMostRecentAttributeWhenMultipleNonVoidedExistInEditMode() throws Exception { + // Patient 2 already has two non-voided Civil Status attributes in the test dataset. + final PersonAttributeType civilStatusType = Context.getPersonService().getPersonAttributeType(8); + final Patient patient2 = Context.getPatientService().getPatient(2); + + new RegressionTestHelper() { + + /** Use patient 2 (with multiple attributes) for the whole test. */ + @Override + public Patient getPatient() { + return patient2; + } + + @Override + public String getFormName() { + return "personAttributeConceptForm"; + } + + @Override + public String[] widgetLabels() { + return new String[] { "Date:", "Location:", "Provider:", "Civil Status:" }; + } + + @Override + public void setupRequest(MockHttpServletRequest request, Map widgets) { + // ENTER: update the most-recent attribute (value="1001") to "1002" + request.addParameter(widgets.get("Date:"), dateAsString(new Date())); + request.addParameter(widgets.get("Location:"), "2"); + request.addParameter(widgets.get("Provider:"), "502"); + request.addParameter(widgets.get("Civil Status:"), "1002"); + } + + @Override + public void testResults(SubmissionResults results) { + results.assertNoErrors(); + // Most-recent attribute should now be "1002" + Patient patient = Context.getPatientService().getPatient(2); + boolean found = patient.getActiveAttributes().stream() + .filter(a -> a.getAttributeType().equals(civilStatusType)) + .anyMatch(a -> "1002".equals(a.getValue())); + assertTrue("Expected the most-recent attribute to be updated to '1002'", found); + } + + @Override + public boolean doEditEncounter() { + return true; + } + + @Override + public String[] widgetLabelsForEdit() { + return widgetLabels(); + } + + @Override + public void setupEditRequest(MockHttpServletRequest request, Map widgets) { + // EDIT: update the most-recent attribute (now "1002") to "1003". + // Use setParameter to replace the pre-populated value from the rendered EDIT form. + request.setParameter(widgets.get("Date:"), dateAsString(new Date())); + request.setParameter(widgets.get("Location:"), "2"); + request.setParameter(widgets.get("Provider:"), "502"); + request.setParameter(widgets.get("Civil Status:"), "1003"); + } + + @Override + public void testEditedResults(SubmissionResults results) { + results.assertNoErrors(); + Patient patient = Context.getPatientService().getPatient(2); + boolean found = patient.getActiveAttributes().stream() + .filter(a -> a.getAttributeType().equals(civilStatusType)) + .anyMatch(a -> "1003".equals(a.getValue())); + assertTrue("Expected the most-recent attribute to be updated to '1003' after EDIT", found); + } + }.run(); + } + + // ========================================================================= + // Form design error cases + // ========================================================================= + + /** + * When an unknown {@code attributeType} UUID is provided, the framework renders a form-design + * error div rather than propagating the exception (this is the standard HFE behaviour for + * {@link BadFormDesignException} thrown from a tag handler). + */ + @Test + public void shouldRenderFormDesignErrorForUnknownAttributeTypeUuid() throws Exception { + String xml = "" + + ""; + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + FormEntrySession session = new FormEntrySession(patient, xml, null); + String html = session.getHtmlToDisplay(); + assertTrue("Expected form-design error to be rendered in HTML", html.contains("error")); + } + + /** + * A Concept-type PersonAttributeType without the mandatory {@code answerConceptIds} attribute + * results in a rendered form-design error. + */ + @Test + public void shouldRenderFormDesignErrorForConceptTypeWithoutAnswerConceptIds() throws Exception { + // Civil Status (type 8) has format=org.openmrs.Concept; answerConceptIds is mandatory + String xml = "" + + ""; + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + FormEntrySession session = new FormEntrySession(patient, xml, null); + String html = session.getHtmlToDisplay(); + assertTrue("Expected form-design error to be rendered in HTML", html.contains("error")); + } + + /** + * A {@code } tag without any {@code attributeType} attribute results in a + * rendered form-design error. + */ + @Test + public void shouldRenderFormDesignErrorWhenAttributeTypeAttributeIsMissing() throws Exception { + String xml = "" + + ""; + Patient patient = Context.getPatientService().getPatient(PATIENT_ID); + FormEntrySession session = new FormEntrySession(patient, xml, null); + String html = session.getHtmlToDisplay(); + assertTrue("Expected form-design error to be rendered in HTML", html.contains("error")); + } + + // ========================================================================= + // Helper + // ========================================================================= + + private static void assertFalse(String message, boolean condition) { + assertTrue(message, !condition); + } +} diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/data/RegressionTest-data-openmrs-2.8.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/data/RegressionTest-data-openmrs-2.8.xml index e8f0bc418..17b5bb795 100644 --- a/api/src/test/resources/org/openmrs/module/htmlformentry/data/RegressionTest-data-openmrs-2.8.xml +++ b/api/src/test/resources/org/openmrs/module/htmlformentry/data/RegressionTest-data-openmrs-2.8.xml @@ -299,8 +299,19 @@ + + + + + + + @@ -323,4 +334,10 @@ + + + diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeConceptForm.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeConceptForm.xml new file mode 100644 index 000000000..72f060c80 --- /dev/null +++ b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeConceptForm.xml @@ -0,0 +1,7 @@ + + Date: + Location: + Provider: + Civil Status: + + diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationFilterOnlyForm.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationFilterOnlyForm.xml new file mode 100644 index 000000000..3429651ae --- /dev/null +++ b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationFilterOnlyForm.xml @@ -0,0 +1,6 @@ + + Date: + Provider: + Birthplace Location: + + diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationForm.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationForm.xml new file mode 100644 index 000000000..8739a08fa --- /dev/null +++ b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationForm.xml @@ -0,0 +1,7 @@ + + Date: + Location: + Provider: + Birthplace Location: + + diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationWithTagForm.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationWithTagForm.xml new file mode 100644 index 000000000..3cdcf65e3 --- /dev/null +++ b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeLocationWithTagForm.xml @@ -0,0 +1,7 @@ + + Date: + Location: + Provider: + Birthplace Location: + + diff --git a/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeStringForm.xml b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeStringForm.xml new file mode 100644 index 000000000..d780c0467 --- /dev/null +++ b/api/src/test/resources/org/openmrs/module/htmlformentry/include/personAttributeStringForm.xml @@ -0,0 +1,7 @@ + + Date: + Location: + Provider: + Birthplace: + + From 09d06abb26d3adb4561fb03961fd460e821977be Mon Sep 17 00:00:00 2001 From: mogoodrich Date: Wed, 27 May 2026 14:00:18 -0400 Subject: [PATCH 02/14] HTML-883: add personAttribute tag for viewing/entering/editing person attributes --- .../PersonAttributeSubmissionElement.java | 61 +++---------------- 1 file changed, 7 insertions(+), 54 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java index c4e35b441..0e6b48cc4 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java @@ -32,7 +32,6 @@ import org.openmrs.module.htmlformentry.widget.Option; import org.openmrs.module.htmlformentry.widget.TextFieldWidget; import org.openmrs.module.htmlformentry.widget.Widget; -import org.openmrs.module.htmlformentry.widget.WidgetFactory; import org.springframework.util.StringUtils; /** @@ -61,10 +60,10 @@ public class PersonAttributeSubmissionElement implements HtmlGeneratorElement, F /** The resolved attribute type. */ private final PersonAttributeType attributeType; - /** The widget used in ENTER / EDIT modes (null in VIEW mode). */ + /** The widget used to render and submit the attribute value. */ private Widget valueWidget; - /** The error widget (null in VIEW mode). */ + /** The error widget (rendered in ENTER / EDIT modes only). */ private ErrorWidget errorWidget; /** The existing non-voided attribute (most recent if there are several), or null. */ @@ -107,12 +106,7 @@ public PersonAttributeSubmissionElement(FormEntryContext context, Map @Override public String generateHtml(FormEntryContext context) { - if (context.getMode() == Mode.VIEW) { - return generateViewHtml(); - } - // ENTER / EDIT StringBuilder sb = new StringBuilder(); sb.append(valueWidget.generateHtml(context)); - sb.append(errorWidget.generateHtml(context)); - return sb.toString(); - } - - private String generateViewHtml() { - if (existingAttribute == null) { - return WidgetFactory.displayDefaultEmptyValue(); - } - String value = existingAttribute.getValue(); - if (!StringUtils.hasText(value)) { - return WidgetFactory.displayDefaultEmptyValue(); - } - - if (FORMAT_CONCEPT.equals(format)) { - try { - Concept concept = Context.getConceptService().getConcept(Integer.parseInt(value)); - if (concept != null && concept.getName() != null) { - return WidgetFactory.displayValue(concept.getName().getName()); - } - } - catch (NumberFormatException e) { - log.warn("Invalid concept ID stored in PersonAttribute: " + value); - } - return WidgetFactory.displayValue(value); - - } else if (FORMAT_LOCATION.equals(format)) { - try { - Location location = Context.getLocationService().getLocation(Integer.parseInt(value)); - if (location != null) { - return WidgetFactory.displayValue(location.getName()); - } - } - catch (NumberFormatException e) { - log.warn("Invalid location ID stored in PersonAttribute: " + value); - } - return WidgetFactory.displayValue(value); - - } else { - // String - return WidgetFactory.displayValue(value); + if (context.getMode() != Mode.VIEW) { + sb.append(errorWidget.generateHtml(context)); } + return sb.toString(); } // --------------------------------------------------------------------------------- @@ -289,7 +242,7 @@ private String generateViewHtml() { @Override public Collection validateSubmission(FormEntryContext context, HttpServletRequest submission) { - // Person attributes are always optional; no validation required. + // No current validation return Collections.emptyList(); } From 81b75e6fbe563e082dac449f8d6d0124c75f1e27 Mon Sep 17 00:00:00 2001 From: mogoodrich Date: Wed, 27 May 2026 15:49:48 -0400 Subject: [PATCH 03/14] HTML-883: add personAttribute tag for viewing/entering/editing person attributes --- .../element/PersonAttributeSubmissionElement.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java index 0e6b48cc4..91ff0a296 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java @@ -150,9 +150,7 @@ private Widget buildConceptWidget(FormEntryContext context, Map } DropdownWidget w = new DropdownWidget(); - - // blank / choose prompt - w.addOption(new Option(Context.getMessageSourceService().getMessage("htmlformentry.chooseOption"), "", false)); + w.addOption(new Option()); for (String idOrUuid : answerConceptIds.split(",")) { String trimmed = idOrUuid.trim(); From e8dbdd21a7adca4cb996c7c158a49afee82aaa67 Mon Sep 17 00:00:00 2001 From: mogoodrich Date: Fri, 29 May 2026 10:01:34 -0400 Subject: [PATCH 04/14] HTML-883: add personAttribute tag for viewing/entering/editing person attributes (minor code review comments) --- .../PersonAttributeSubmissionElement.java | 23 +++++-------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java index 91ff0a296..d48c728d2 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java @@ -49,12 +49,6 @@ */ public class PersonAttributeSubmissionElement implements HtmlGeneratorElement, FormSubmissionControllerAction { - private static final String FORMAT_STRING = "java.lang.String"; - - private static final String FORMAT_CONCEPT = "org.openmrs.Concept"; - - private static final String FORMAT_LOCATION = "org.openmrs.Location"; - protected final Log log = LogFactory.getLog(getClass()); /** The resolved attribute type. */ @@ -69,9 +63,6 @@ public class PersonAttributeSubmissionElement implements HtmlGeneratorElement, F /** The existing non-voided attribute (most recent if there are several), or null. */ private PersonAttribute existingAttribute; - /** Format of the attribute type (java.lang.String / org.openmrs.Concept / org.openmrs.Location). */ - private final String format; - public PersonAttributeSubmissionElement(FormEntryContext context, Map parameters) throws BadFormDesignException { @@ -86,8 +77,6 @@ public PersonAttributeSubmissionElement(FormEntryContext context, Map tag: PersonAttributeType not found for uuid=\"" + attributeTypeUuid + "\""); } - format = attributeType.getFormat(); - // ---- 2. Find existing attribute on the patient --------------------------------- Patient patient = context.getExistingPatient(); if (patient != null) { @@ -109,19 +98,19 @@ public PersonAttributeSubmissionElement(FormEntryContext context, Map tag: unsupported attribute format \"" + format + throw new BadFormDesignException(" tag: unsupported attribute format \"" + attributeType.getFormat() + "\" for type \"" + attributeType.getName() + "\". Supported formats: " - + FORMAT_STRING + ", " + FORMAT_CONCEPT + ", " + FORMAT_LOCATION); + + String.class.getName() + ", " + Concept.class.getName() + ", " + Location.class.getName()); } context.registerWidget(valueWidget); @@ -146,7 +135,7 @@ private Widget buildConceptWidget(FormEntryContext context, Map String answerConceptIds = parameters.get("answerConceptIds"); if (!StringUtils.hasText(answerConceptIds)) { throw new BadFormDesignException(" tag: \"answerConceptIds\" is required " - + "for PersonAttributeType \"" + attributeType.getName() + "\" (format=" + FORMAT_CONCEPT + ")"); + + "for PersonAttributeType \"" + attributeType.getName() + "\" (format=" + Concept.class.getName() + ")"); } DropdownWidget w = new DropdownWidget(); From 0f874e7b675ea28455e658d039734db49bfa965b Mon Sep 17 00:00:00 2001 From: mogoodrich Date: Fri, 29 May 2026 10:09:42 -0400 Subject: [PATCH 05/14] HTML-883: add personAttribute tag for viewing/entering/editing person attributes (use seralization methods to set values) --- .../PersonAttributeSubmissionElement.java | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java index d48c728d2..b7c158852 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java @@ -209,6 +209,30 @@ private Widget buildLocationWidget(FormEntryContext context, Map return w; } + // --------------------------------------------------------------------------------- + // Helpers + // --------------------------------------------------------------------------------- + + /** + * Converts the raw submitted string from the widget into the canonical storage value for + * the attribute. For Concept and Location types, we look up the actual typed object and call + * its {@link org.openmrs.Attributable#serialize()} method so that core controls the format. + */ + private String toStorageValue(String rawValue) { + if (!StringUtils.hasText(rawValue)) { + return rawValue; + } + String format = attributeType.getFormat(); + if (Concept.class.getName().equals(format)) { + Concept concept = Context.getConceptService().getConcept(Integer.parseInt(rawValue)); + return concept != null ? concept.serialize() : rawValue; + } else if (Location.class.getName().equals(format)) { + Location location = Context.getLocationService().getLocation(Integer.parseInt(rawValue)); + return location != null ? location.serialize() : rawValue; + } + return rawValue; + } + // --------------------------------------------------------------------------------- // HtmlGeneratorElement // --------------------------------------------------------------------------------- @@ -247,23 +271,20 @@ public void handleSubmission(FormEntrySession session, HttpServletRequest submis if (existingAttribute != null) { if (!StringUtils.hasText(value)) { - // Blank submission → void the existing attribute existingAttribute.setVoided(true); existingAttribute.setVoidedBy(Context.getAuthenticatedUser()); existingAttribute.setVoidReason("Cleared via htmlformentry form submission"); existingAttribute.setDateVoided(new Date()); } else { - // Update existing attribute in place - existingAttribute.setValue(value); + existingAttribute.setValue(toStorageValue(value)); existingAttribute.setChangedBy(Context.getAuthenticatedUser()); existingAttribute.setDateChanged(new Date()); } } else { if (StringUtils.hasText(value)) { - // Create a new attribute PersonAttribute newAttribute = new PersonAttribute(); newAttribute.setAttributeType(attributeType); - newAttribute.setValue(value); + newAttribute.setValue(toStorageValue(value)); patient.addAttribute(newAttribute); } } From 5f28d89f7d8489c6a11973592473b8fc8d9af6a0 Mon Sep 17 00:00:00 2001 From: mogoodrich Date: Fri, 29 May 2026 10:29:03 -0400 Subject: [PATCH 06/14] docs: add design spec for personAttribute foreignKey concept widget support Co-Authored-By: Claude Sonnet 4.6 --- ...on-attribute-concept-foreign-key-design.md | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 docs/superpowers/specs/2026-05-29-person-attribute-concept-foreign-key-design.md diff --git a/docs/superpowers/specs/2026-05-29-person-attribute-concept-foreign-key-design.md b/docs/superpowers/specs/2026-05-29-person-attribute-concept-foreign-key-design.md new file mode 100644 index 000000000..133cdeb40 --- /dev/null +++ b/docs/superpowers/specs/2026-05-29-person-attribute-concept-foreign-key-design.md @@ -0,0 +1,158 @@ +# Design: PersonAttribute Concept Widget — foreignKey Support + +**Date:** 2026-05-29 +**File:** `api/src/main/java/org/openmrs/module/htmlformentry/element/PersonAttributeSubmissionElement.java` + +## Context + +`PersonAttributeSubmissionElement.buildConceptWidget()` currently requires an explicit `answerConceptIds` tag attribute listing the valid concept IDs. However, `PersonAttributeType` has a `foreignKey` (Integer) field that—for concept-typed attributes—can reference either a concept set (whose members are the valid answers) or a question concept (whose concept answers are the valid choices). Using `foreignKey` avoids duplicating answer lists between the attribute type definition and every form that uses it. + +## Behavior Change + +Priority order for populating concept dropdown options: + +1. **`answerConceptIds` tag attribute present** → use those IDs exactly (existing behavior, no change). +2. **`attributeType.getForeignKey()` not null** → look up that concept, then: + - If `concept.getSet()` is `true` → use `concept.getSetMembers(false)` (excludes retired members). + - Else → iterate `concept.getAnswers(false)` and use each `ConceptAnswer.getAnswerConcept()` (excludes retired answer concepts). +3. **Neither present** → throw `BadFormDesignException` with a message explaining that one of `answerConceptIds` or a `foreignKey` on the attribute type is required. + +The blank "choose..." `Option()` is always prepended regardless of source, consistent with the current implementation. + +`answerConceptIds` remains optional — no form that currently specifies it needs to change. + +## Implementation + +### `buildConceptWidget` in `PersonAttributeSubmissionElement` + +Replace the current guard that throws when `answerConceptIds` is absent with the three-way resolution above. Extract the concept-list-to-options loop into a private helper to avoid duplication between the `answerConceptIds` branch and the `foreignKey` branch. + +Rough structure: + +```java +private Widget buildConceptWidget(FormEntryContext context, Map parameters) + throws BadFormDesignException { + + String answerConceptIds = parameters.get("answerConceptIds"); + List concepts; + + if (StringUtils.hasText(answerConceptIds)) { + concepts = resolveConceptsFromIdList(answerConceptIds); // existing logic + } else if (attributeType.getForeignKey() != null) { + concepts = resolveConceptsFromForeignKey(attributeType.getForeignKey()); + } else { + throw new BadFormDesignException("..."); + } + + DropdownWidget w = new DropdownWidget(); + w.addOption(new Option()); + for (Concept concept : concepts) { + w.addOption(new Option(concept.getName().getName(), concept.getConceptId().toString(), false)); + } + if (existingAttribute != null && StringUtils.hasText(existingAttribute.getValue())) { + w.setInitialValue(existingAttribute.getValue()); + } + return w; +} + +private List resolveConceptsFromIdList(String answerConceptIds) throws BadFormDesignException { ... } + +private List resolveConceptsFromForeignKey(Integer foreignKey) throws BadFormDesignException { + Concept fk = Context.getConceptService().getConcept(foreignKey); + if (fk == null) { + throw new BadFormDesignException("..."); + } + if (Boolean.TRUE.equals(fk.getSet())) { + return fk.getSetMembers(false); + } else { + return fk.getAnswers(false).stream() + .map(ConceptAnswer::getAnswerConcept) + .collect(Collectors.toList()); + } +} +``` + +No changes needed outside `buildConceptWidget` — `handleSubmission`, `toStorageValue`, and the rest of the element are unaffected. + +## Test Data + +New rows added to `RegressionTest-data-openmrs-2.8.xml`: + +```xml + + + + + +``` + +Existing fixture data used by the tests: + +| Concept ID | Name | Role | +|---|---|---| +| 1004 | ANOTHER ALLERGY CONSTRUCT | concept set (foreign key for type 20) | +| 80000 | ALLERGY | set member of 1004 | +| 1119 | ALLERGY DATE | set member of 1004 | +| 1000 | ALLERGY CODED | set member of 1004 | +| 1005 | HYPER-ALLERGY CODED | set member of 1004 | +| 1000 | ALLERGY CODED | question concept (foreign key for type 21) | +| 1001 | PENICILLIN | answer concept of 1000 | +| 1002 | CATS | answer concept of 1000 | +| 1003 | OPENMRS | answer concept of 1000 | + +## New Form XML Files + +**`personAttributeConceptSetForeignKeyForm.xml`** +```xml + + Date: + Location: + Provider: + Attr: + + +``` + +**`personAttributeConceptAnswerForeignKeyForm.xml`** +```xml + + Date: + Location: + Provider: + Attr: + + +``` + +## New Tests in `PersonAttributeTagTest` + +Both tests follow the `RegressionTestHelper` pattern used by existing tests in the file. + +**`testConceptAttributeWithForeignKeyConceptSet`** +- Uses attribute type 20 (foreign_key → concept 1004, a set). +- ENTER mode: assert the rendered HTML contains the set member names as `