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 1 commit
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
27 changes: 20 additions & 7 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 @@ -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<String, String> parameters) throws BadFormDesignException {
appointmentUuidConceptMapping = parameters.get("appointmentUuidConcept");
tagControlId = parameters.get("controlId");
Expand All @@ -112,7 +116,10 @@ public ScheduleAppointmentElement(FormEntryContext context, Map<String, String>
+ 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)

@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).

Expand All @@ -127,9 +134,14 @@ 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

if (allowScheduling) {
// set up all widgets
optional = "true".equalsIgnoreCase(parameters.get("optional"));
id = parameters.get("id");
scheduleChoiceWidget = new TextFieldWidget();
Expand Down Expand Up @@ -269,7 +281,7 @@ public ScheduleAppointmentElement(FormEntryContext context, Map<String, String>

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

Expand Down Expand Up @@ -433,7 +445,7 @@ private String generateViewEditHtml() {

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

Expand Down Expand Up @@ -499,8 +511,9 @@ 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;
}

Expand Down
31 changes: 31 additions & 0 deletions ...rc/test/java/org/openmrs/module/htmlformentry/element/ScheduleAppointmentElementTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,37 @@ public void handleSubmission_withoutControlId_shouldNotStampUuidObsWithFormPath(
assertNull(obsToCreate.get(0).getFormFieldPath());
}

// --- EDIT mode: scheduling allowed only when no appointment exists yet ---

@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.

These two exercise handleSubmission, but the change also flips generateHtml and validateSubmission into scheduling mode in EDIT, and neither of those is covered. handleSubmission_editMode_existingAppointment_shouldBeNoOp also passes unchanged on master, where EDIT is already a no-op, so it reads as a guard rather than a check on the new behaviour.

ScheduleAppointmentTagTest in api-tests is where I'd put this. It already has doEditEncounter(), testEditFormHtml and setupEditRequest, and an EDIT-mode case there pins down what the PR is actually for: open an encounter with no appointment, fill the form, assert the appointment and its uuid obs land on the existing encounter. I ran exactly that against this branch and it passes, so it's a few lines of harness work rather than new debugging. It would also have caught the validation behaviour I raised on the element, which a mocked FormEntryContext can't reach because it never renders the form or runs a submission.

@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.

I'll add EDIT mode tests in ScheduleAppointmentTagTest


@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<String, String> params(String... kvs) {
Expand Down