[chibongho]

…nt tag

This PR modifies that existing <scheduleAppointment>, which only allows for scheduling a new appointment in ENTER mode. With this change, scheduling a new appointment in EDIT mode is allowed, provided that no appointment has been scheduled in prior ENTER / EDIT mode submissions.

We retrain the existing behavior that once an appointment has been created by the form, the appointment cannot be modified in subsequent EDIT mode. (The user will need to go to the appointment app to do so.)

[mogoodrich]

LGTM, thanks @chibongho !

[dkayiwa]

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]

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

[chibongho]

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.

[dkayiwa]

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]

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]

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

[dkayiwa]

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]

I'll add EDIT mode tests in ScheduleAppointmentTagTest