HTML-886: Add new tag to record appointment service as obs by chibongho · Pull Request #338 · openmrs/openmrs-module-htmlformentry

chibongho · 2026-06-10T19:08:53Z

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

Adds a <AppointmentServiceObs> tag to record appointment service as obs.

mogoodrich

Can you add some tests, to demo how this works?

What was the considering with adding this as a new tag, versus a "obs" with
"style='appointmentService'" (like we have for location with "style='location')

(If this had all been discussed and decided while I was on vacay, that's fine, but if it hasn't been discussed, worth a discussion)

mogoodrich · 2026-06-12T16:38:28Z

+			throw new IllegalArgumentException("Could not find concept: " + conceptId);
+		}
+
+		existingObs = context.removeExistingObs(concept, (Concept) null);


We should test/document what happens if you multiple of these tags on a single form with the same concept id. At minimum, I don't think it will be able to reproduce them in the same order on reload. We should likely add support for mapping via form namespace and path via the control Id as the obs tag does, see: https://openmrs.atlassian.net/browse/HTML-788 (or if we don't make a new tag but use the obs tag we should get this out of the box).

I don't think this will handle the rendering order correctly if we have multiple of these tags. That said, do we need to worry about that? An appointment can only be associated with one appointment service, and adding multiple of these tags for the same appointment should be a form design error.

But you could be scheduling multiple appointments (for different services) on this form, correct? Would that work? This also ties into my previous question about why we decided to make this a new tag (instead of a obs tag with style='appointment')

If we are going to put this tag in HFE, we should aim to support as many of the best-practice nuances that there are already established. I'm fine putting this in the ObsSubmissionElement, but I'm 100% convinced this functionality belongs in the HFE module at all (versus in pihcore, etc)

chibongho · 2026-06-15T19:25:05Z

Can you add some tests, to demo how this works?

What was the considering with adding this as a new tag, versus a "obs" with "style='appointmentService'" (like we have for location with "style='location')

(If this had all been discussed and decided while I was on vacay, that's fine, but if it hasn't been discussed, worth a discussion)

Yeah, I can add some tests.

Speaking with @mseaton, I think the biggest motivation of having a separate tag is that <obs> is a bit of a kitchen sink with too many things in it.

mogoodrich · 2026-06-15T19:27:53Z

Speaking with @mseaton, I think the biggest motivation of having a separate tag is that <obs> is a bit of a kitchen sink with too many things in it.

Ah, okay, that makes sense then--we can keep as a separate tag. I do think we may want to add support for "controlId" attribute, but why don't you start by adding some tests so I can understand it better and then we can discuss. @chibongho

mseaton

Generally looks good but see comments

mseaton · 2026-06-15T20:36:35Z

+	public AppointmentServiceObsElement(FormEntryContext context, Map<String, String> parameters) {
+		String conceptId = parameters.get("conceptId");
+		if (conceptId == null) {
+			throw new IllegalArgumentException("appointmentServiceObs requires a conceptId parameter");


All of these IllegalArgumentException should probably be BadFormDesignException

mseaton · 2026-06-15T20:39:23Z

+			throw new IllegalArgumentException("Could not find concept: " + conceptId);
+		}
+
+		existingObs = context.removeExistingObs(concept, (Concept) null);


If we are going to put this tag in HFE, we should aim to support as many of the best-practice nuances that there are already established. I'm fine putting this in the ObsSubmissionElement, but I'm 100% convinced this functionality belongs in the HFE module at all (versus in pihcore, etc)

mseaton · 2026-06-15T20:40:03Z

+		}
+
+		existingObs = context.removeExistingObs(concept, (Concept) null);
+		String existingUuid = existingObs != null ? existingObs.getValueText() : null;


So in your other active PR, you used provider id, here you are using uuid. I agree uuid is best, but why the inconsistency?

I'll change it to the "<id> - <serviceName>" format similar to Location and Person.

I'd be okay with keeping this as-is as this is a new domain we are capturing... I was more concerned about, say, capturing providers using "uuid" in some cases and "id" in others.

mseaton · 2026-06-15T20:42:05Z

+		StringBuilder sb = new StringBuilder();
+		sb.append(serviceWidget.generateHtml(context));
+		if (context.getMode() != Mode.VIEW) {
+			sb.append("<span class='required'>*</span>");


Can you explain what this does? Will this always mark this type of question as required?

It really ought to be required, but I haven't gotten a way to make it be conditionally required only if the user choose to create an appointment. What I have now is making this input technically optional in the backend, but have the subform here enforce the conditional requirement check via JS. Adding the <span> here is technically incorrect; I'll remove.

mogoodrich · 2026-06-16T21:39:15Z

+
+		serviceWidget.addOption(new Option("", "", false));
+		for (AppointmentServiceDefinition svc : services) {
+			serviceWidget.addOption(new Option(svc.getName(), svc.getId() + " - " + svc.getName(), false));


Sorry, the more I think about it, this should be uuid, regardless of what the other objects are doing... when reloading an existing obs is this doing a full match and so would fail on edit if the text name of a service as was changed?

Yeah, this should definitely be uuid. Is there no uuid on appointmentservicedefinition?

There is. I'll use uuid.

mseaton · 2026-06-17T13:02:25Z

+
+		serviceWidget.addOption(new Option("", "", false));
+		for (AppointmentServiceDefinition svc : services) {
+			serviceWidget.addOption(new Option(svc.getName(), svc.getId() + " - " + svc.getName(), false));


Yeah, this should definitely be uuid. Is there no uuid on appointmentservicedefinition?

…xt value

HTML-886: Add new tag to record appointment service as obs

39ef9b6

chibongho requested review from cioan, mogoodrich and mseaton and removed request for mseaton June 10, 2026 19:08

mogoodrich requested changes Jun 12, 2026

View reviewed changes

mseaton reviewed Jun 15, 2026

View reviewed changes

chibongho added 3 commits June 16, 2026 14:18

Add tests + controlId, chnage obs value format to <id> + <serviceName>

1f96bc9

Merge remote-tracking branch 'origin/master' into HTML-886

f3ab567

fix AppointmentServiceObs value not being displayed when in obsgroup

67383dc

chibongho requested review from mogoodrich and mseaton June 16, 2026 21:12

mogoodrich requested changes Jun 16, 2026

View reviewed changes

mseaton reviewed Jun 17, 2026

View reviewed changes

accept uuid for specialities; save appointment service uuid as obs te…

b3e9ac7

…xt value

chibongho requested review from mogoodrich and mseaton June 17, 2026 18:37

mseaton approved these changes Jun 17, 2026

View reviewed changes

chibongho merged commit 40945b0 into master Jun 17, 2026
5 checks passed

chibongho deleted the HTML-886 branch June 17, 2026 19:32

Uh oh!

Conversation

chibongho commented Jun 10, 2026

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!

chibongho commented Jun 15, 2026

Uh oh!

mogoodrich commented Jun 15, 2026

Uh oh!

mseaton 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!

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!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants