Skip to content

HTML-882: allow creating appointment in edit mode in ScheduleAppoinme… #345

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.

Sign up for GitHub

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

Merged
chibongho merged 2 commits into from
Aug 4, 2026
Merged

HTML-882: allow creating appointment in edit mode in ScheduleAppoinme… #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d8a4e5
HTML-882: allow creating appointment in edit mode in ScheduleAppoinme…
chibongho Jul 27, 2026
906e465
address feedback
chibongho Jul 30, 2026
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
186 changes: 185 additions & 1 deletion api-tests/src/test/java/org/openmrs/module/htmlformentry/ScheduleAppointmentTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.junit.Before;
import org.junit.Test;
import org.openmrs.Encounter;
import org.openmrs.Obs;
import org.openmrs.Visit;
import org.openmrs.api.context.Context;
import org.openmrs.module.appointments.model.Appointment;
Expand Down Expand Up @@ -786,8 +787,14 @@ public void testBlankFormHtml(String html) {
}.run();
}

/**
* Once an appointment has been scheduled, the tag is read-only in both VIEW and EDIT: the details
* are shown, no scheduling widgets are rendered, and re-saving in EDIT cannot book a second one.
* The element never updates an existing appointment - that has to be done in the appointment app.
*/
@Test
public void scheduleAppointmentTag_shouldShowAppointmentDetailsInViewModeWithUuidConcept() throws Exception {
public void scheduleAppointmentTag_withUuidConcept_shouldRenderExistingAppointmentReadOnlyInViewAndEdit()
throws Exception {
new RegressionTestHelper() {

@Override
Expand Down Expand Up @@ -841,6 +848,183 @@ public void testViewingEncounter(Encounter encounter, String html) {
TestUtil.assertFuzzyContains("Kigali", html);
TestUtil.assertFuzzyContains("Consultation", html);
}

@Override
public boolean doEditEncounter() {
return true;
}

@Override
public void testEditFormHtml(String html) {
// the appointment is shown read-only...
TestUtil.assertFuzzyContains("schedule-appointment-view", html);
TestUtil.assertFuzzyContains("Consultation", html);
// ...with no way to schedule another: no yes/no choice and no scheduling fields
TestUtil.assertFuzzyDoesNotContain("schedule-appointment-choice", html);
TestUtil.assertFuzzyDoesNotContain("schedule-appointment-fields", html);
}

@Override
public void testEditedResults(SubmissionResults results) {
// the edit request is rebuilt from the rendered form, i.e. the user changed nothing
results.assertNoErrors();

AppointmentSearchRequest searchRequest = new AppointmentSearchRequest();
searchRequest.setPatientUuid(PATIENT_UUID);
searchRequest.setStartDate(new Date(0));
Assert.assertEquals("editing must not create a second appointment", 1,
Context.getService(AppointmentsService.class).search(searchRequest).size());
}
}.run();
}

// ---------------------------------------------------------------
// EDIT mode: scheduling is opt-in
// ---------------------------------------------------------------

/**
* Adding a scheduleAppointment tag to a form that already has encounters must not make those
* encounters unsaveable. Encounter 101 predates the tag, so it has no appointment-uuid obs: EDIT
* offers the yes/no choice (even though the tag is not marked optional) and defaults it to "no",
* so re-submitting the form untouched saves cleanly without booking anything.
*/
@Test
public void scheduleAppointmentTag_editMode_shouldDefaultToNotSchedulingAndSaveWithoutAppointment()
throws Exception {
new RegressionTestHelper() {

@Override
public String getFormName() {
return "scheduleAppointmentForm";
}

@Override
public String getFormXml() {
return "<htmlform>Encounter Date: <encounterDate/>"
+ "<scheduleAppointment locationTag=\"Some Tag\" appointmentUuidConcept=\"" + APPT_UUID_CONCEPT + "\"/>"
+ "<submit/></htmlform>";
}

@Override
public String[] widgetLabels() {
return new String[] {};
}

@Override
public boolean doCreateEncounter() {
return false;
}

@Override
public Encounter getEncounterToEdit() {
// pre-existing encounter for patient 2, with no appointment-uuid obs
return Context.getEncounterService().getEncounter(101);
}

@Override
public void testEditFormHtml(String html) {
// the choice is offered in EDIT regardless of the optional attribute...
TestUtil.assertFuzzyContains("value=\"no\" checked", html);
// ...and defaults to "no", with the fields hidden to match
TestUtil.assertFuzzyDoesNotContain("value=\"yes\" checked", html);
TestUtil.assertFuzzyContains("class=\"schedule-appointment-fields\" style=\"display:none\"", html);
}

@Override
public void testEditedResults(SubmissionResults results) {
// the edit request is built from the rendered form, i.e. the user changed nothing
results.assertNoErrors();

AppointmentSearchRequest searchRequest = new AppointmentSearchRequest();
searchRequest.setPatientUuid(PATIENT_UUID);
searchRequest.setStartDate(new Date(0));
Assert.assertEquals(0, Context.getService(AppointmentsService.class).search(searchRequest).size());
}
}.run();
}

/**
* An encounter that carries no appointment can have one scheduled
* during EDIT. The appointment and its uuid obs must land on the existing encounter.
*/
@Test
public void scheduleAppointmentTag_editMode_shouldCreateAppointmentWhenYesSelected() throws Exception {
new RegressionTestHelper() {

@Override
public String getFormName() {
return "scheduleAppointmentForm";
}

@Override
public String getFormXml() {
return "<htmlform>Encounter Date: <encounterDate/>"
+ "<scheduleAppointment locationTag=\"Some Tag\" appointmentUuidConcept=\"" + APPT_UUID_CONCEPT + "\"/>"
+ "<submit/></htmlform>";
}

@Override
public String[] widgetLabels() {
return new String[] {};
}

@Override
public boolean doCreateEncounter() {
return false;
}

@Override
public Encounter getEncounterToEdit() {
// pre-existing encounter for patient 2, with no appointment-uuid obs
return Context.getEncounterService().getEncounter(101);
}

@Override
public String[] widgetLabelsForEdit() {
return new String[] { "Schedule appointment", "Location", "Service", "Appointment Type",
DATE_TIME_ANCHOR, DATE_TIME_ANCHOR + "!!1", "Duration (minutes)", "Provider" };
}

@Override
public void setupEditRequest(MockHttpServletRequest request, Map<String, String> widgets) {
request.setParameter(widgets.get("Schedule appointment"), "yes");
request.setParameter(widgets.get("Location"), KIGALI_UUID);
request.setParameter(widgets.get("Service"), CONSULTATION_UUID);
request.setParameter(widgets.get("Appointment Type"), AppointmentKind.Scheduled.name());
request.setParameter(widgets.get(DATE_TIME_ANCHOR), "2025-01-15");

String timeBase = widgets.get(DATE_TIME_ANCHOR + "!!1").replace("hours", "");
request.setParameter(timeBase + "hours", "10");
request.setParameter(timeBase + "minutes", "30");
request.setParameter(timeBase + "seconds", "0");

request.setParameter(widgets.get("Duration (minutes)"), "30");
request.setParameter(widgets.get("Provider") + "_hid", PROVIDER_ID);
}

@Override
public void testEditedResults(SubmissionResults results) {
results.assertNoErrors();

AppointmentSearchRequest searchRequest = new AppointmentSearchRequest();
searchRequest.setPatientUuid(PATIENT_UUID);
searchRequest.setStartDate(new Date(0));
List<Appointment> appointments = Context.getService(AppointmentsService.class).search(searchRequest);
Assert.assertEquals(1, appointments.size());
Assert.assertEquals(CONSULTATION_UUID, appointments.get(0).getService().getUuid());

// the uuid obs must be attached to the encounter that was edited, not a new one
Encounter edited = Context.getEncounterService().getEncounter(101);
String appointmentUuid = appointments.get(0).getUuid();
boolean foundUuidObs = false;
for (Obs obs : edited.getAllObs(false)) {
if (APPT_UUID_CONCEPT.equals(obs.getConcept().getUuid())
&& appointmentUuid.equals(obs.getValueText())) {
foundUuidObs = true;
}
}
Assert.assertTrue("appointment uuid obs not found on encounter 101", foundUuidObs);
}
}.run();
}

Expand Down
63 changes: 45 additions & 18 deletions api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,14 @@ public class ScheduleAppointmentElement implements HtmlGeneratorElement, FormSub

private static final String DEFAULT_LOCATION_TAG = "Appointment Location";

private boolean optional;
// True when the yes/no "schedule an appointment?" question should be rendered: whenever the form
// author asked for it via optional="true", and always in EDIT mode.
private boolean showScheduleChoice;

// Which side of that question starts selected (and whether the fields wrapper starts visible).
// EDIT mode defaults to "no" so that editing an encounter for any other reason does not force
// the user to book an appointment; ENTER mode keeps defaulting to "yes".
private boolean defaultToScheduling;

private String id;

Expand Down Expand Up @@ -99,6 +106,10 @@ public class ScheduleAppointmentElement implements HtmlGeneratorElement, FormSub
// Appointment fetched in VIEW/EDIT mode constructor
private Appointment existingAppointment;

// True when this element should render the scheduling form and handle submission:
// always in ENTER mode, and in EDIT mode when no appointment has been scheduled yet.
private boolean allowScheduling;

public ScheduleAppointmentElement(FormEntryContext context, Map<String, String> parameters) throws BadFormDesignException {
appointmentUuidConceptMapping = parameters.get("appointmentUuidConcept");
tagControlId = parameters.get("controlId");
Expand All @@ -112,9 +123,16 @@ public ScheduleAppointmentElement(FormEntryContext context, Map<String, String>
+ appointmentUuidConceptMapping);
}

if (context.getMode() != Mode.ENTER) {
// VIEW/EDIT: claim the UUID obs so HFE doesn't warn about an unmatched obs,
// then fetch the appointment for display in generateHtml().
if (context.getMode() == Mode.ENTER) {
allowScheduling = true;
}
else {
// VIEW/EDIT: look up the UUID obs written by a previous submission, so the appointment can
// be displayed in generateHtml(). Which lookup applies depends on whether this tag stamps a
// controlId into formFieldPath, mirroring ObsSubmissionElement.setExistingObs(): the
// controlId branch matches on the stamped path, removeExistingObs() only matches obs whose
// path is null. As elsewhere in HFE, that means adding or removing a controlId on a tag that
// already has data will stop the lookup from matching.
Obs uuidObs = StringUtils.isNotBlank(tagControlId)

@dkayiwa dkayiwa Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With a controlId set, this lookup only matches obs whose formFieldPath carries that control id, so an appointment recorded while the tag had no controlId isn't found. Under the new rule, uuidObs == null now means "offer another one".

I reproduced it in api-tests: seed encounter 101 with an appointment-uuid obs whose formFieldPath is null (exactly what handleSubmission writes when no controlId is configured), then open it in EDIT with controlId="appt1" on the tag. Master renders the "No appointment scheduled" message, wrong but harmless. This branch renders the full scheduling form, and submitting it leaves the encounter with two appointment-uuid obs and the patient with two live appointments. The encounter then stops rendering at all, because two obs sharing a control id make getObsFromExistingObs throw IllegalStateException: Multiple obs are matching the control id 'appt1' in both VIEW and EDIT (that last part is framework behaviour, identical on master).

Adding a controlId to an existing tag is a routine form edit, so I'd rather this didn't hinge on the two lookups agreeing. Before concluding that nothing was scheduled, check whether the encounter has any obs for uuidConcept at all (the existingObs entry, read ahead of the remove) instead of trusting the controlId-scoped match. That also covers the mirror case, where a controlId gets dropped from a tag and the already-stamped formFieldPath makes removeExistingObs miss.

It takes a deliberate form change to trigger, so I don't think it has to hold up the merge if you'd rather split it out, but it's this PR that turns the mismatch from a cosmetic one into duplicate clinical data.

@chibongho chibongho Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, this problem happens when we first record an appointment in a form with no controlId, then change the form so that the tag has a controlId, and then if we EDIT the form, it doesn't know that the prior appointment has already been created.

Given that we expect users to version bump the form if they make edits to it, and that this is also how controlId in the <obs> tag works (if we change the controlId of <obs>, prior obs recordings will not be matched properly onto the right field), I think this is fine? Let me know if you disagree.

@mogoodrich mogoodrich Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine with what @chibongho says above (the onus is on the implementer to upgrade the form version). Alternatively, since this is a new tag, we could just mandate that a controlId is used? (This is some that should be used for all data points, but it didn't exist back in the early days).

? context.getObsFromExistingObs(uuidConcept, tagControlId)
: context.removeExistingObs(uuidConcept, (Concept) null);
Expand All @@ -127,10 +145,17 @@ public ScheduleAppointmentElement(FormEntryContext context, Map<String, String>
// appointments module unavailable
}
}
// EDIT mode with no previously-scheduled appointment: let the user schedule one now.
if (context.getMode() == Mode.EDIT && uuidObs == null) {
allowScheduling = true;
}
Comment on lines +148 to +151

@dkayiwa dkayiwa Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes an appointment mandatory in EDIT mode rather than merely possible. allowScheduling also gates validateSubmission, so all seven required fields now apply to an edit.

I ran the same api-tests probe against both branches. On master, editing an encounter that carries no appointment-uuid obs submits cleanly. On this branch the identical edit comes back with seven errors (Location, Service, Appointment type, Appointment date, Appointment time, Duration, Provider) and never reaches applyActions.

Merged as is, that means adding <scheduleAppointment> to a form that already has encounters makes every one of those encounters unsaveable in EDIT: someone correcting a diagnosis can't save until they also book a future appointment. optional="true" is milder but still caught, since generateHtml emits value="yes" checked and a browser submits that by default, so the user has to click "No" on every edit even though they already declined at entry time.

My suggestion is to make scheduling opt-in whenever the mode is EDIT: turn the yes/no choice on regardless of the optional attribute, check "no" rather than "yes" in that mode, and start the fields wrapper hidden to match. Validation and handleSubmission then only engage once the user asks for an appointment. Skipping validation in EDIT when every appointment field is blank would also work, but it gives the user no visible signal, so I'd go with the explicit choice.

@mogoodrich mogoodrich Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thoughts @chibongho , this does need to be fixed if it does work as Daniel says

@chibongho chibongho Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this should be fixed. I'll make it so in EDIT mode, if no appointment has been scheduled yet, it defaults to "Don't schedule appointment" instead, regardless of the optional attribute.

}
else {
// ENTER mode: set up all widgets
optional = "true".equalsIgnoreCase(parameters.get("optional"));

if (allowScheduling) {
// set up all widgets
boolean optional = "true".equalsIgnoreCase(parameters.get("optional"));
showScheduleChoice = optional || context.getMode() == Mode.EDIT;
defaultToScheduling = context.getMode() != Mode.EDIT;
id = parameters.get("id");
scheduleChoiceWidget = new TextFieldWidget();
context.registerWidget(scheduleChoiceWidget);
Expand Down Expand Up @@ -269,7 +294,7 @@ public ScheduleAppointmentElement(FormEntryContext context, Map<String, String>

@Override
public String generateHtml(FormEntryContext context) {
if (context.getMode() != Mode.ENTER) {
if (!allowScheduling) {
return generateViewEditHtml();
}

Expand All @@ -286,19 +311,20 @@ public String generateHtml(FormEntryContext context) {

sb.append("<div class=\"schedule-appointment\"").append(id != null ? " id=\"" + id + "\"" : "").append(">");

if (optional) {
if (showScheduleChoice) {
String choiceFieldName = context.getFieldName(scheduleChoiceWidget);
sb.append("<p class=\"schedule-appointment-choice\">");
sb.append("<label><input type=\"radio\" name=\"").append(choiceFieldName)
.append("\" value=\"yes\" checked")
.append("\" value=\"yes\"").append(defaultToScheduling ? " checked=\"true\"" : "")
.append(" onchange=\"this.closest('.schedule-appointment').querySelector('.schedule-appointment-fields').style.display='block'\"> ")
.append(msg("htmlformentry.scheduleAppointment.doSchedule")).append("</label> ");
sb.append("<label><input type=\"radio\" name=\"").append(choiceFieldName)
.append("\" value=\"no\"")
.append("\" value=\"no\"").append(defaultToScheduling ? "" : " checked=\"true\"")
.append(" onchange=\"this.closest('.schedule-appointment').querySelector('.schedule-appointment-fields').style.display='none'\"> ")
.append(msg("htmlformentry.scheduleAppointment.doNotSchedule")).append("</label>");
sb.append("</p>");
sb.append("<div class=\"schedule-appointment-fields\" style=\"display:block\">");
sb.append("<div class=\"schedule-appointment-fields\" style=\"display:")
.append(defaultToScheduling ? "block" : "none").append("\">");
}

// Location
Expand Down Expand Up @@ -355,7 +381,7 @@ public String generateHtml(FormEntryContext context) {
msg("htmlformentry.scheduleAppointment.note"),
noteWidget.generateHtml(context)));

if (optional) {
if (showScheduleChoice) {
sb.append("</div>");
}

Expand Down Expand Up @@ -433,11 +459,11 @@ private String generateViewEditHtml() {

@Override
public Collection<FormSubmissionError> validateSubmission(FormEntryContext context, HttpServletRequest submission) {
if (context.getMode() != Mode.ENTER) {
if (!allowScheduling) {
return Collections.emptyList();
}

if (optional && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) {
if (showScheduleChoice && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) {
return Collections.emptyList();
}

Expand Down Expand Up @@ -499,14 +525,15 @@ public Collection<FormSubmissionError> validateSubmission(FormEntryContext conte

@Override
public void handleSubmission(FormEntrySession session, HttpServletRequest submission) {
// EDIT mode is intentionally a no-op: the element does not update existing appointments.
if (session.getContext().getMode() != Mode.ENTER) {
// Only schedule when the form was rendered for scheduling (ENTER, or EDIT with no existing
// appointment). Otherwise this is a no-op: the element never updates an existing appointment.
if (!allowScheduling) {
return;
}

FormEntryContext context = session.getContext();

if (optional && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) {
if (showScheduleChoice && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) {
return;
}

Expand Down
Loading