Skip to content

HTML-882: drop support for all-day appointments in ScheduleAppointment tag #344

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 3 commits into from
Jul 23, 2026
Merged

HTML-882: drop support for all-day appointments in ScheduleAppointment tag #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d1c5ae
drop support for all-day appointments in ScheduleAppointment tag
chibongho Jul 21, 2026
3964a92
address feedback
chibongho Jul 23, 2026
8c0162a
Merge remote-tracking branch 'origin/master' into HTML-882
chibongho Jul 23, 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
167 changes: 77 additions & 90 deletions api-tests/src/test/java/org/openmrs/module/htmlformentry/ScheduleAppointmentTagTest.java
View file
Open in desktop

Large diffs are not rendered by default.

124 changes: 37 additions & 87 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 @@ -52,7 +52,6 @@
import org.openmrs.module.htmlformentry.widget.NumberFieldWidget;
import org.openmrs.module.htmlformentry.widget.TextFieldWidget;
import org.openmrs.module.htmlformentry.widget.TimeWidget;
import org.openmrs.util.OpenmrsUtil;

public class ScheduleAppointmentElement implements HtmlGeneratorElement, FormSubmissionControllerAction {

Expand All @@ -76,9 +75,6 @@ public class ScheduleAppointmentElement implements HtmlGeneratorElement, FormSub

private TextFieldWidget noteWidget;

// Hidden field tracking "allday" vs "specific" radio state
private TextFieldWidget allDayWidget;

private NumberFieldWidget durationWidget;

private ErrorWidget locationErrorWidget;
Expand Down Expand Up @@ -252,10 +248,6 @@ public ScheduleAppointmentElement(FormEntryContext context, Map<String, String>
context.registerErrorWidget(startDateTimeWidget, startDateTimeErrorWidget);
context.registerWidget(timeErrorWidget);

// --- All-day toggle (hidden field; value set by JS radio buttons) ---
allDayWidget = new TextFieldWidget();
context.registerWidget(allDayWidget);

// --- Duration ---
durationWidget = new NumberFieldWidget(1.0, null, false);
durationErrorWidget = new ErrorWidget();
Expand Down Expand Up @@ -288,8 +280,6 @@ public String generateHtml(FormEntryContext context) {
sb.append(".schedule-appointment p>label{display:block;margin-bottom:0.2em;}");
sb.append(".schedule-appointment .field-widget{display:flex;align-items:center;gap:0.25em;flex-wrap:wrap;}");
sb.append(".schedule-appointment-choice label{display:inline-flex;align-items:center;gap:0.3em;cursor:pointer;margin-right:1em;}");
sb.append(".schedule-appointment-timing-options{display:flex;gap:1.5em;align-items:center;}");
sb.append(".schedule-appointment-timing-option{display:inline-flex;align-items:center;gap:0.3em;cursor:pointer;}");
sb.append(".schedule-appointment-date-time-widget{display:inline-flex;align-items:center;gap:0.5em;flex-wrap:wrap;}");
sb.append(".schedule-appt-time{display:inline-flex;align-items:center;gap:0.25em;}");
sb.append("</style>");
Expand Down Expand Up @@ -332,28 +322,7 @@ public String generateHtml(FormEntryContext context) {
.append("\" value=\"").append(typeWidget.getOptions().get(0).getValue()).append("\"/>");
}

// All-day / Specific-time toggle — shown BEFORE the date/time row
String allDayFieldName = context.getFieldName(allDayWidget);
sb.append("<p class=\"schedule-appointment-timing\">");
sb.append("<label>").append(msg("htmlformentry.scheduleAppointment.timing"))

@dkayiwa dkayiwa Jul 22, 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 the toggle gone, the htmlformentry.scheduleAppointment.timing and htmlformentry.scheduleAppointment.specificTime keys aren't referenced from anywhere anymore, but they're still in messages.properties and the es/fr/ht bundles. Worth dropping them in this PR so the dead strings don't stick around in the translation files. (allDay is still used by the view-mode rendering, so that one stays.)

.append(" <span class=\"required\">*</span></label>");
sb.append("<span class=\"field-widget schedule-appointment-timing-options\">");
sb.append("<label class=\"schedule-appointment-timing-option\"><input type=\"radio\" name=\"").append(allDayFieldName).append("-radio\" value=\"allday\"")
.append(" onchange=\"this.closest('.schedule-appointment').querySelector('.schedule-appt-allday').value='allday';")
.append("this.closest('.schedule-appointment').querySelector('.schedule-appt-time').style.display='none';")
.append("this.closest('.schedule-appointment').querySelector('.schedule-appt-duration').style.display='none';\"> ")
.append(msg("htmlformentry.scheduleAppointment.allDay")).append("</label>");
sb.append("<label class=\"schedule-appointment-timing-option\"><input type=\"radio\" name=\"").append(allDayFieldName).append("-radio\" value=\"specific\" checked")
.append(" onchange=\"this.closest('.schedule-appointment').querySelector('.schedule-appt-allday').value='specific';")
.append("this.closest('.schedule-appointment').querySelector('.schedule-appt-time').style.display='inline-flex';")
.append("this.closest('.schedule-appointment').querySelector('.schedule-appt-duration').style.display='block';\"> ")
.append(msg("htmlformentry.scheduleAppointment.specificTime")).append("</label>");
sb.append("</span>");
sb.append("<input type=\"hidden\" name=\"").append(allDayFieldName)
.append("\" class=\"schedule-appt-allday\" value=\"specific\"/>");
sb.append("</p>");

// Date + Time on one line; time portion shown/hidden by the toggle above
// Date + Time
sb.append("<p class=\"schedule-appointment-date-time\">");
sb.append("<label>").append(msg("htmlformentry.scheduleAppointment.date"))
.append(" <span class=\"required\">*</span></label>");
Expand All @@ -370,13 +339,11 @@ public String generateHtml(FormEntryContext context) {
.append(context.getFieldName(startDateTimeWidget.getDateWidget())).append("', '")
.append(context.getFieldName(startDateTimeWidget.getTimeWidget())).append("')</script>");

// Duration (shown only for specific-time)
sb.append("<div class=\"schedule-appt-duration\">");
// Duration
sb.append(fieldRow("schedule-appointment-duration",
msg("htmlformentry.scheduleAppointment.duration"),
durationWidget.generateHtml(context) + durationErrorWidget.generateHtml(context),
true));
sb.append("</div>");

// Provider
sb.append(fieldRow("schedule-appointment-provider",
Expand Down Expand Up @@ -503,27 +470,22 @@ public Collection<FormSubmissionError> validateSubmission(FormEntryContext conte
msg("htmlformentry.scheduleAppointment.error.dateRequired")));
}

// Time and duration required only for specific-time appointments
String allDayValue = (String) allDayWidget.getValue(context, submission);
boolean isAllDay = "allday".equals(allDayValue);
if (!isAllDay) {
Date time = (Date) startDateTimeWidget.getTimeWidget().getValue(context, submission);
if (time == null) {
errors.add(new FormSubmissionError(context.getFieldName(timeErrorWidget),
msg("htmlformentry.scheduleAppointment.error.timeRequired")));
}
Date time = (Date) startDateTimeWidget.getTimeWidget().getValue(context, submission);
if (time == null) {

@dkayiwa dkayiwa Jul 22, 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.

Could we add a validation test that omits the time selects (and one omitting duration), along the lines of shouldRequireServiceInlineNotAsGlobalError? Making time and duration unconditionally required is the one behavior this PR changes, and every submission test in the suite supplies both, so nothing would fail if the requirement quietly regressed. Not a blocker.

errors.add(new FormSubmissionError(context.getFieldName(timeErrorWidget),
msg("htmlformentry.scheduleAppointment.error.timeRequired")));
}

Number durNum = null;
try {
durNum = (Number) durationWidget.getValue(context, submission);
}
catch (IllegalArgumentException e) {
// non-numeric input — falls through to error
}
if (durNum == null || durNum.intValue() < 1) {
errors.add(new FormSubmissionError(context.getFieldName(durationErrorWidget),
msg("htmlformentry.scheduleAppointment.error.durationRequired")));
}
Number durNum = null;
try {
durNum = (Number) durationWidget.getValue(context, submission);
}
catch (IllegalArgumentException e) {
// non-numeric input — falls through to error
}
if (durNum == null || durNum.intValue() < 1) {
errors.add(new FormSubmissionError(context.getFieldName(durationErrorWidget),
msg("htmlformentry.scheduleAppointment.error.durationRequired")));
}

Provider provider = (Provider) providerWidget.getValue(context, submission);
Expand Down Expand Up @@ -552,8 +514,6 @@ public void handleSubmission(FormEntrySession session, HttpServletRequest submis
String serviceUuid = (String) serviceWidget.getValue(context, submission);
String typeValue = (String) typeWidget.getValue(context, submission);
Date date = (Date) startDateTimeWidget.getDateWidget().getValue(context, submission);
String allDayValue = (String) allDayWidget.getValue(context, submission);
boolean isAllDay = "allday".equals(allDayValue);
Provider selectedProvider = (Provider) providerWidget.getValue(context, submission);
String note = (String) noteWidget.getValue(context, submission);

Expand All @@ -563,44 +523,34 @@ public void handleSubmission(FormEntrySession session, HttpServletRequest submis
// Build start datetime
Calendar cal = Calendar.getInstance();
cal.setTime(date);
if (!isAllDay) {
Date time = (Date) startDateTimeWidget.getTimeWidget().getValue(context, submission);
if (time != null) {
Calendar timeCal = Calendar.getInstance();
timeCal.setTime(time);
cal.set(Calendar.HOUR_OF_DAY, timeCal.get(Calendar.HOUR_OF_DAY));
cal.set(Calendar.MINUTE, timeCal.get(Calendar.MINUTE));
}
} else {
cal.set(Calendar.HOUR_OF_DAY, 0);
cal.set(Calendar.MINUTE, 0);
Date time = (Date) startDateTimeWidget.getTimeWidget().getValue(context, submission);
if (time != null) {
Calendar timeCal = Calendar.getInstance();
timeCal.setTime(time);
cal.set(Calendar.HOUR_OF_DAY, timeCal.get(Calendar.HOUR_OF_DAY));
cal.set(Calendar.MINUTE, timeCal.get(Calendar.MINUTE));
}
cal.set(Calendar.SECOND, 0);
cal.set(Calendar.MILLISECOND, 0);
Date startDateTime = cal.getTime();

// Build end datetime
Date endDateTime;
if (isAllDay) {
endDateTime = OpenmrsUtil.getLastMomentOfDay(date);
} else {
Integer durationMins = null;
try {
Number durNum = (Number) durationWidget.getValue(context, submission);
if (durNum != null) {
durationMins = durNum.intValue();
}
}
catch (IllegalArgumentException e) {
// non-numeric input — validation should have caught this
}
if (durationMins == null && service != null) {
durationMins = service.getDurationMins();
Integer durationMins = null;
try {
Number durNum = (Number) durationWidget.getValue(context, submission);
if (durNum != null) {
durationMins = durNum.intValue();
}
endDateTime = durationMins != null
? new Date(startDateTime.getTime() + (long) durationMins * 60 * 1000)
: null;
}
catch (IllegalArgumentException e) {
// non-numeric input — validation should have caught this
}
if (durationMins == null && service != null) {
durationMins = service.getDurationMins();
}
Date endDateTime = durationMins != null
? new Date(startDateTime.getTime() + (long) durationMins * 60 * 1000)
: null;

Appointment appointment = new Appointment();
appointment.setPatient(session.getPatient());
Expand Down
2 changes: 0 additions & 2 deletions api/src/main/resources/messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,7 @@ htmlformentry.scheduleAppointment.service=Service
htmlformentry.scheduleAppointment.appointmentType=Appointment Type
htmlformentry.scheduleAppointment.date=Date
htmlformentry.scheduleAppointment.appointmentTime=Time
htmlformentry.scheduleAppointment.timing=Timing
htmlformentry.scheduleAppointment.allDay=All day
htmlformentry.scheduleAppointment.specificTime=Specific time
htmlformentry.scheduleAppointment.duration=Duration (minutes)
htmlformentry.scheduleAppointment.provider=Provider
htmlformentry.scheduleAppointment.note=Note
Expand Down
2 changes: 0 additions & 2 deletions api/src/main/resources/messages_es.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,7 @@ htmlformentry.scheduleAppointment.service=Servicio
htmlformentry.scheduleAppointment.appointmentType=Tipo de cita
htmlformentry.scheduleAppointment.date=Fecha
htmlformentry.scheduleAppointment.appointmentTime=Hora
htmlformentry.scheduleAppointment.timing=Horario
htmlformentry.scheduleAppointment.allDay=Todo el día
htmlformentry.scheduleAppointment.specificTime=Hora específica
htmlformentry.scheduleAppointment.duration=Duración (minutos)
htmlformentry.scheduleAppointment.provider=Proveedor
htmlformentry.scheduleAppointment.note=Nota
Expand Down
2 changes: 0 additions & 2 deletions api/src/main/resources/messages_fr.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,7 @@ htmlformentry.scheduleAppointment.service=Service
htmlformentry.scheduleAppointment.appointmentType=Type de rendez-vous
htmlformentry.scheduleAppointment.date=Date
htmlformentry.scheduleAppointment.appointmentTime=Heure
htmlformentry.scheduleAppointment.timing=Horaire
htmlformentry.scheduleAppointment.allDay=Toute la journée
htmlformentry.scheduleAppointment.specificTime=Heure précise
htmlformentry.scheduleAppointment.duration=Durée (minutes)
htmlformentry.scheduleAppointment.provider=Prestataire de soins
htmlformentry.scheduleAppointment.note=Note
Expand Down
2 changes: 0 additions & 2 deletions api/src/main/resources/messages_ht.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,7 @@ htmlformentry.scheduleAppointment.service=Sèvis
htmlformentry.scheduleAppointment.appointmentType=Kalite randevou
htmlformentry.scheduleAppointment.date=Dat
htmlformentry.scheduleAppointment.appointmentTime=Lè
htmlformentry.scheduleAppointment.timing=Orè
htmlformentry.scheduleAppointment.allDay=Tout jounen an
htmlformentry.scheduleAppointment.specificTime=Lè presi
htmlformentry.scheduleAppointment.duration=Kantite tan (minit)
htmlformentry.scheduleAppointment.provider=Moun ki bay swen
htmlformentry.scheduleAppointment.note=Nòt
Expand Down
23 changes: 21 additions & 2 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 @@ -14,7 +14,9 @@
import static org.mockito.Mockito.when;

import javax.servlet.http.HttpServletRequest;
import java.text.SimpleDateFormat;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -104,10 +106,28 @@ public void setup() throws Exception {
mockedHtmlFormEntryUtil.when(() -> HtmlFormEntryUtil.getConcept(CONCEPT_ID)).thenReturn(concept);
mockedHtmlFormEntryUtil.when(() -> HtmlFormEntryUtil.createObs(any(Concept.class), any(), any(), any()))
.thenCallRealMethod();
// real conversion logic per requested type, since different widgets request different types
// (DateWidget wants a Date, TimeWidget wants Integers) and a blanket canned value breaks whichever
// widget didn't ask for that type
mockedHtmlFormEntryUtil
.when(() -> HtmlFormEntryUtil.getParameterAsType(any(HttpServletRequest.class), anyString(),
any(Class.class)))
.thenReturn(new java.util.Date());
.thenAnswer(invocation -> {
HttpServletRequest req = invocation.getArgument(0);
String name = invocation.getArgument(1);
Class<?> clazz = invocation.getArgument(2);
String val = req.getParameter(name);
if (val == null) {
return null;
}
if (Date.class.isAssignableFrom(clazz)) {
return new SimpleDateFormat("yyyy-MM-dd").parse(val);
}
if (Integer.class.isAssignableFrom(clazz)) {
return Integer.valueOf(val);
}
return val;
});

mockedContext.when(Context::getAdministrationService).thenReturn(administrationService);
mockedContext.when(Context::getMessageSourceService).thenReturn(messageSourceService);
Expand Down Expand Up @@ -240,7 +260,6 @@ private MockHttpServletRequest buildRequest() {

MockHttpServletRequest req = new MockHttpServletRequest();
req.addParameter("dd", AppointmentKind.Scheduled.name()); // all DropdownWidgets share "dd"; value must be a valid AppointmentKind for the type widget
req.addParameter("tf", "allday"); // allDayWidget → all-day, no time widget needed
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;
Expand Down