HTML-883: add personAttribute tag for viewing/entering/editing person attributes by mogoodrich · Pull Request #334 · openmrs/openmrs-module-htmlformentry

mogoodrich · 2026-05-27T18:22:48Z

See: https://openmrs.atlassian.net/browse/HTML-883

… attributes Adds a new <personAttribute> 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 <noreply@anthropic.com>

… attributes

mseaton · 2026-05-29T11:56:59Z

+
+	private static final String FORMAT_CONCEPT = "org.openmrs.Concept";
+
+	private static final String FORMAT_LOCATION = "org.openmrs.Location";


These constants seem unnecessary when you can and should just get the name off the class object itself. If you want to keep the constants, assign them values based on XXX.class.getName()

Fair. Fixed.

mseaton · 2026-05-29T11:59:17Z

+	private PersonAttribute existingAttribute;
+
+	/** Format of the attribute type (java.lang.String / org.openmrs.Concept / org.openmrs.Location). */
+	private final String format;


Do you need to keep this here as something independent from PersonAttributeType? Can't you just do attributeType.getFormat() if you need it?

Fair. Over-optimization by Claude I should have fixed.

mseaton · 2026-05-29T12:07:37Z

+	        throws BadFormDesignException {
+
+		String answerConceptIds = parameters.get("answerConceptIds");
+		if (!StringUtils.hasText(answerConceptIds)) {


I believe PersonAttributeType has a foreignKey property that may be intended to act as a pointer to the allowed value set. eg. The foreignKey could point to a concept that is a question with answers (or possibly a set with members, not sure) and this should define the set of allowed answers. We should likely support that here so that this handles the basics as expected.

Oh, fair, I had forgotten about this. I will fix. Arguably, we shouldn't even allow answerConceptIds, but I will keep that since it's been created.

mseaton · 2026-05-29T12:14:00Z

+				// Create a new attribute
+				PersonAttribute newAttribute = new PersonAttribute();
+				newAttribute.setAttributeType(attributeType);
+				newAttribute.setValue(value);


I'm not 100% sure this is the right way to set the value, can you just double check? Seems like this will set the primary key value of the concept or location if those are the types? It would be better if we could call a method to set the value to the actual Concept or Location that we parse here, and then let core store that as a string value however it sees fit, to protect against any changes in the future, but if this is the way to do it, it's fine I guess

Good catch. The "seralize" method below wasn't something I was familiar, but I double-checked and Claude is indeed correct.

Claude: The reviewer's concern is fully valid and there's a better API available.

The key findings:

PersonAttribute.setValue(String) is the only setter ? no typed API

But both Concept and Location implement Attributable, which has a serialize() method

Concept.serialize() ? "" + getConceptId(); Location.serialize() ? "" + getLocationId()

getHydratedObject() uses hydrate(getValue()) to reconstruct the typed object

The reviewer is right: we should parse the submitted value to the actual object and call serialize() on it, letting core determine the storage format

Currently we pass the raw widget string directly. The fix is to look up the typed object and call serialize() ? exactly as Attributable intends.

… attributes (minor code review comments)

… attributes (use seralization methods to set values)

…upport Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…widget support Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…oncept tests

… widget population Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…swer population When answerConceptIds is not specified on the tag, the concept dropdown now resolves its options from PersonAttributeType.foreignKey: if the referenced concept is a set, its non-retired members are used; otherwise the concept's non-retired answers are used. answerConceptIds still takes precedence when present.

… attributes (remove superpower docs)

…dropdown Aligns with ObsSubmissionElement's getName(Context.getLocale(), false) pattern rather than the no-arg getName() to ensure correct locale resolution.

… attributes

mogoodrich

Updated per code review (and one other inconsistency I found).

fyi @mseaton if you want to take another look.

I am going to test and then merge.

mogoodrich · 2026-05-29T13:50:36Z

+
+	private static final String FORMAT_CONCEPT = "org.openmrs.Concept";
+
+	private static final String FORMAT_LOCATION = "org.openmrs.Location";


Fair. Fixed.

mogoodrich · 2026-05-29T13:50:54Z

+	private PersonAttribute existingAttribute;
+
+	/** Format of the attribute type (java.lang.String / org.openmrs.Concept / org.openmrs.Location). */
+	private final String format;


Fair. Over-optimization by Claude I should have fixed.

mogoodrich · 2026-05-29T13:52:17Z

+	        throws BadFormDesignException {
+
+		String answerConceptIds = parameters.get("answerConceptIds");
+		if (!StringUtils.hasText(answerConceptIds)) {


Oh, fair, I had forgotten about this. I will fix. Arguably, we shouldn't even allow answerConceptIds, but I will keep that since it's been created.

mogoodrich · 2026-05-29T14:01:10Z

+				// Create a new attribute
+				PersonAttribute newAttribute = new PersonAttribute();
+				newAttribute.setAttributeType(attributeType);
+				newAttribute.setValue(value);


Good catch. The "seralize" method below wasn't something I was familiar, but I double-checked and Claude is indeed correct.

Claude: The reviewer's concern is fully valid and there's a better API available.

The key findings:

PersonAttribute.setValue(String) is the only setter ? no typed API

But both Concept and Location implement Attributable, which has a serialize() method

Concept.serialize() ? "" + getConceptId(); Location.serialize() ? "" + getLocationId()

getHydratedObject() uses hydrate(getValue()) to reconstruct the typed object

The reviewer is right: we should parse the submitted value to the actual object and call serialize() on it, letting core determine the storage format

Currently we pass the raw widget string directly. The fix is to look up the typed object and call serialize() ? exactly as Attributable intends.

mogoodrich and others added 2 commits May 27, 2026 14:00

HTML-883: add personAttribute tag for viewing/entering/editing person…

09d06ab

… attributes

mogoodrich marked this pull request as draft May 27, 2026 18:22

mogoodrich marked this pull request as ready for review May 27, 2026 19:47

mogoodrich requested review from chibongho, cioan and mseaton May 27, 2026 19:49

HTML-883: add personAttribute tag for viewing/entering/editing person…

81b75e6

… attributes

mseaton reviewed May 29, 2026

View reviewed changes

mogoodrich and others added 11 commits May 29, 2026 10:01

HTML-883: add personAttribute tag for viewing/entering/editing person…

e8dbdd2

… attributes (minor code review comments)

HTML-883: add personAttribute tag for viewing/entering/editing person…

0f874e7

… attributes (use seralization methods to set values)

docs: add design spec for personAttribute foreignKey concept widget s…

5f28d89

…upport Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs: add implementation plan for personAttribute foreignKey concept …

6ec5b03

…widget support Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test: add PersonAttributeType fixture rows 20 and 21 for foreignKey c…

eebe325

…oncept tests

test: add form XML files for foreignKey concept attribute tests

80d4b92

test: add failing tests for foreignKey concept set and concept answer…

41d3ac9

… widget population Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

HTML-883: add personAttribute tag for viewing/entering/editing person…

77df0d6

… attributes (remove superpower docs)

fix: use locale-aware concept name lookup in personAttribute concept …

c89aa14

…dropdown Aligns with ObsSubmissionElement's getName(Context.getLocale(), false) pattern rather than the no-arg getName() to ensure correct locale resolution.

HTML-883: add personAttribute tag for viewing/entering/editing person…

1af073a

… attributes

mogoodrich commented May 29, 2026

View reviewed changes

mogoodrich merged commit 586acdc into master May 29, 2026
5 checks passed


		private static final String FORMAT_CONCEPT = "org.openmrs.Concept";

		private static final String FORMAT_LOCATION = "org.openmrs.Location";

Uh oh!

Conversation

mogoodrich commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogoodrich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mogoodrich commented May 27, 2026 •

edited

Loading