From 9d8a4e58e14a92e3753181304aedf7347c293461 Mon Sep 17 00:00:00 2001 From: Chi Bong Ho Date: Mon, 27 Jul 2026 13:23:44 -0400 Subject: [PATCH 1/2] HTML-882: allow creating appointment in edit mode in ScheduleAppoinment tag --- .../element/ScheduleAppointmentElement.java | 27 +++++++++++----- .../ScheduleAppointmentElementTest.java | 31 +++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java b/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java index f270be1f7..448f96daa 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java @@ -99,6 +99,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 parameters) throws BadFormDesignException { appointmentUuidConceptMapping = parameters.get("appointmentUuidConcept"); tagControlId = parameters.get("controlId"); @@ -112,7 +116,10 @@ public ScheduleAppointmentElement(FormEntryContext context, Map + appointmentUuidConceptMapping); } - if (context.getMode() != Mode.ENTER) { + if (context.getMode() == Mode.ENTER) { + allowScheduling = true; + } + else { // VIEW/EDIT: claim the UUID obs so HFE doesn't warn about an unmatched obs, // then fetch the appointment for display in generateHtml(). Obs uuidObs = StringUtils.isNotBlank(tagControlId) @@ -127,9 +134,14 @@ public ScheduleAppointmentElement(FormEntryContext context, Map // 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; + } } - else { - // ENTER mode: set up all widgets + + if (allowScheduling) { + // set up all widgets optional = "true".equalsIgnoreCase(parameters.get("optional")); id = parameters.get("id"); scheduleChoiceWidget = new TextFieldWidget(); @@ -269,7 +281,7 @@ public ScheduleAppointmentElement(FormEntryContext context, Map @Override public String generateHtml(FormEntryContext context) { - if (context.getMode() != Mode.ENTER) { + if (!allowScheduling) { return generateViewEditHtml(); } @@ -433,7 +445,7 @@ private String generateViewEditHtml() { @Override public Collection validateSubmission(FormEntryContext context, HttpServletRequest submission) { - if (context.getMode() != Mode.ENTER) { + if (!allowScheduling) { return Collections.emptyList(); } @@ -499,8 +511,9 @@ public Collection 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; } diff --git a/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java b/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java index ce8375a2d..dc99a73bf 100644 --- a/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java +++ b/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java @@ -233,6 +233,37 @@ public void handleSubmission_withoutControlId_shouldNotStampUuidObsWithFormPath( assertNull(obsToCreate.get(0).getFormFieldPath()); } + // --- EDIT mode: scheduling allowed only when no appointment exists yet --- + + @Test + public void handleSubmission_editMode_noExistingAppointment_shouldScheduleAppointment() throws Exception { + when(context.getMode()).thenReturn(Mode.EDIT); + when(context.removeExistingObs(concept, (Concept) null)).thenReturn(null); + + ScheduleAppointmentElement element = new ScheduleAppointmentElement(context, enterParams()); + + element.handleSubmission(session, buildRequest()); + + assertEquals(1, actions.getObsToCreate().size()); + assertEquals(1, actions.getAppointmentsToCreate().size()); + } + + @Test + public void handleSubmission_editMode_existingAppointment_shouldBeNoOp() throws Exception { + Obs obs = new Obs(); + obs.setValueText(APPT_UUID); + when(context.getMode()).thenReturn(Mode.EDIT); + when(context.removeExistingObs(concept, (Concept) null)).thenReturn(obs); + when(appointmentsService.getAppointmentByUuid(APPT_UUID)).thenReturn(new Appointment()); + + ScheduleAppointmentElement element = new ScheduleAppointmentElement(context, enterParams()); + + element.handleSubmission(session, new MockHttpServletRequest()); + + assertEquals(0, actions.getObsToCreate().size()); + assertEquals(0, actions.getAppointmentsToCreate().size()); + } + // --- Helpers --- private Map params(String... kvs) { From 906e465606dae1ebea6eeb611ee7955a992a9e38 Mon Sep 17 00:00:00 2001 From: Chi Bong Ho Date: Thu, 30 Jul 2026 10:32:20 -0400 Subject: [PATCH 2/2] address feedback --- .../ScheduleAppointmentTagTest.java | 186 +++++++++++++++++- .../element/ScheduleAppointmentElement.java | 36 ++-- .../ScheduleAppointmentElementTest.java | 93 ++++++++- 3 files changed, 295 insertions(+), 20 deletions(-) diff --git a/api-tests/src/test/java/org/openmrs/module/htmlformentry/ScheduleAppointmentTagTest.java b/api-tests/src/test/java/org/openmrs/module/htmlformentry/ScheduleAppointmentTagTest.java index 2ab42cd1a..b5803e340 100644 --- a/api-tests/src/test/java/org/openmrs/module/htmlformentry/ScheduleAppointmentTagTest.java +++ b/api-tests/src/test/java/org/openmrs/module/htmlformentry/ScheduleAppointmentTagTest.java @@ -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; @@ -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 @@ -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 "Encounter Date: " + + "" + + ""; + } + + @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 "Encounter Date: " + + "" + + ""; + } + + @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 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 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(); } diff --git a/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java b/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java index 448f96daa..52789139a 100644 --- a/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java +++ b/api/src/main/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElement.java @@ -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; @@ -120,8 +127,12 @@ public ScheduleAppointmentElement(FormEntryContext context, Map allowScheduling = true; } else { - // VIEW/EDIT: claim the UUID obs so HFE doesn't warn about an unmatched obs, - // then fetch the appointment for display in generateHtml(). + // 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) ? context.getObsFromExistingObs(uuidConcept, tagControlId) : context.removeExistingObs(uuidConcept, (Concept) null); @@ -142,7 +153,9 @@ public ScheduleAppointmentElement(FormEntryContext context, Map if (allowScheduling) { // set up all widgets - optional = "true".equalsIgnoreCase(parameters.get("optional")); + 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); @@ -298,19 +311,20 @@ public String generateHtml(FormEntryContext context) { sb.append("
"); - if (optional) { + if (showScheduleChoice) { String choiceFieldName = context.getFieldName(scheduleChoiceWidget); sb.append("

"); sb.append(" "); sb.append(""); sb.append("

"); - sb.append("
"); + sb.append("
"); } // Location @@ -367,7 +381,7 @@ public String generateHtml(FormEntryContext context) { msg("htmlformentry.scheduleAppointment.note"), noteWidget.generateHtml(context))); - if (optional) { + if (showScheduleChoice) { sb.append("
"); } @@ -449,7 +463,7 @@ public Collection validateSubmission(FormEntryContext conte return Collections.emptyList(); } - if (optional && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) { + if (showScheduleChoice && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) { return Collections.emptyList(); } @@ -519,7 +533,7 @@ public void handleSubmission(FormEntrySession session, HttpServletRequest submis FormEntryContext context = session.getContext(); - if (optional && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) { + if (showScheduleChoice && !"yes".equals(scheduleChoiceWidget.getValue(context, submission))) { return; } diff --git a/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java b/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java index dc99a73bf..528b08ba4 100644 --- a/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java +++ b/api/src/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java @@ -2,6 +2,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; @@ -50,6 +51,8 @@ import org.openmrs.module.htmlformentry.HtmlFormEntryUtil; import org.openmrs.module.htmlformentry.widget.DateWidget; import org.openmrs.module.htmlformentry.widget.DropdownWidget; +import org.openmrs.module.htmlformentry.widget.ErrorWidget; +import org.openmrs.module.htmlformentry.widget.NumberFieldWidget; import org.openmrs.module.htmlformentry.widget.ProviderAjaxAutoCompleteWidget; import org.openmrs.module.htmlformentry.widget.TextFieldWidget; import org.openmrs.module.htmlformentry.widget.TimeWidget; @@ -242,12 +245,67 @@ public void handleSubmission_editMode_noExistingAppointment_shouldScheduleAppoin ScheduleAppointmentElement element = new ScheduleAppointmentElement(context, enterParams()); - element.handleSubmission(session, buildRequest()); + MockHttpServletRequest req = buildRequest(); + chooseToSchedule(req); + element.handleSubmission(session, req); assertEquals(1, actions.getObsToCreate().size()); assertEquals(1, actions.getAppointmentsToCreate().size()); } + // --- EDIT mode: scheduling is opt-in, so an edit that leaves the choice alone must not schedule --- + + @Test + public void handleSubmission_editMode_choiceNo_shouldBeNoOp() throws Exception { + when(context.getMode()).thenReturn(Mode.EDIT); + when(context.removeExistingObs(concept, (Concept) null)).thenReturn(null); + + ScheduleAppointmentElement element = new ScheduleAppointmentElement(context, enterParams()); + + // appointment fields are all filled in, but the user declined to schedule + MockHttpServletRequest req = buildRequest(); + declineScheduling(req); + element.handleSubmission(session, req); + + assertEquals(0, actions.getObsToCreate().size()); + assertEquals(0, actions.getAppointmentsToCreate().size()); + } + + @Test + public void validateSubmission_editMode_choiceNo_shouldReturnNoErrors() throws Exception { + when(context.getMode()).thenReturn(Mode.EDIT); + when(context.removeExistingObs(concept, (Concept) null)).thenReturn(null); + + ScheduleAppointmentElement element = new ScheduleAppointmentElement(context, enterParams()); + + // an edit made for some other reason: choice left at its "no" default, every field blank + MockHttpServletRequest req = emptyRequest(); + declineScheduling(req); + assertTrue(element.validateSubmission(context, req).isEmpty()); + } + + @Test + public void validateSubmission_editMode_choiceYes_shouldRequireAppointmentFields() throws Exception { + when(context.getMode()).thenReturn(Mode.EDIT); + when(context.removeExistingObs(concept, (Concept) null)).thenReturn(null); + + ScheduleAppointmentElement element = new ScheduleAppointmentElement(context, enterParams()); + + MockHttpServletRequest req = emptyRequest(); + chooseToSchedule(req); + + // location, service, date, time, duration, provider (type is a single fixed value here) + assertEquals(6, element.validateSubmission(context, req).size()); + } + + @Test + public void validateSubmission_enterMode_notOptional_shouldRequireAppointmentFields() throws Exception { + ScheduleAppointmentElement element = new ScheduleAppointmentElement(context, enterParams()); + + // ENTER mode without optional="true" has no choice to make, so validation still always applies + assertEquals(6, element.validateSubmission(context, emptyRequest()).size()); + } + @Test public void handleSubmission_editMode_existingAppointment_shouldBeNoOp() throws Exception { Obs obs = new Obs(); @@ -283,16 +341,35 @@ private Map enterParams() { } private MockHttpServletRequest buildRequest() { - when(context.getFieldName(any(DropdownWidget.class))).thenReturn("dd"); - when(context.getFieldName(any(TextFieldWidget.class))).thenReturn("tf"); - when(context.getFieldName(any(DateWidget.class))).thenReturn("dt"); - lenient().when(context.getFieldName(any(TimeWidget.class))).thenReturn("time"); - when(context.getFieldName(any(ProviderAjaxAutoCompleteWidget.class))).thenReturn("prov"); - - MockHttpServletRequest req = new MockHttpServletRequest(); + MockHttpServletRequest req = emptyRequest(); req.addParameter("dd", AppointmentKind.Scheduled.name()); // all DropdownWidgets share "dd"; value must be a valid AppointmentKind for the type widget req.addParameter("dt", "2025-01-15"); // date (parsed as yyyy-MM-dd by HtmlFormEntryUtil) // prov_hid not set → provider = null (appointment is created without provider) return req; } + + /** Registers the widget field names but leaves every appointment field blank. */ + private MockHttpServletRequest emptyRequest() { + lenient().when(context.getFieldName(any(DropdownWidget.class))).thenReturn("dd"); + lenient().when(context.getFieldName(any(TextFieldWidget.class))).thenReturn("tf"); + lenient().when(context.getFieldName(any(DateWidget.class))).thenReturn("dt"); + lenient().when(context.getFieldName(any(TimeWidget.class))).thenReturn("time"); + lenient().when(context.getFieldName(any(NumberFieldWidget.class))).thenReturn("dur"); + lenient().when(context.getFieldName(any(ProviderAjaxAutoCompleteWidget.class))).thenReturn("prov"); + lenient().when(context.getFieldName(any(ErrorWidget.class))).thenReturn("err"); + return new MockHttpServletRequest(); + } + + /** + * Answers "yes" to the schedule-an-appointment question. The choice widget shares the "tf" field + * name with the note widget, so this also lands "yes" in the note — harmless for these assertions. + */ + private void chooseToSchedule(MockHttpServletRequest req) { + req.setParameter("tf", "yes"); + } + + /** Answers "no" — what an untouched EDIT form submits, since that radio starts checked. */ + private void declineScheduling(MockHttpServletRequest req) { + req.setParameter("tf", "no"); + } }