Skip to content

Fix time validation, standardize request modals #609

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

Closed
mjaydenkim wants to merge 4 commits into from
Closed

Fix time validation, standardize request modals #609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4988761
Fix time validation, standardize request modals
mjaydenkim Nov 2, 2025
0921772
Update MainCard.tsx
mjaydenkim Nov 2, 2025
3003214
Merge branch 'master' into mk2672/standardize-request-ride-modals
mjaydenkim Dec 5, 2025
ad77f3b
Merge branch 'master' into mk2672/standardize-request-ride-modals
mjaydenkim Dec 22, 2025
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
49 changes: 49 additions & 0 deletions frontend/src/components/RideDetails/RideActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import AdminPanelSettingsIcon from '@mui/icons-material/AdminPanelSettings';
import CloseIcon from '@mui/icons-material/Close';
import { RideType, Status, SchedulingState } from '../../types';
import { useRideEdit } from './RideEditContext';
import { validateRideTimes, TimeValidationError } from './TimeValidation';
import {
canUpdateStatus,
canCancelRide,
Expand Down Expand Up @@ -92,6 +93,8 @@ const RideActions: React.FC<RideActionsProps> = ({
const [selectedStatus, setSelectedStatus] = useState<Status | null>(null);
const [updating, setUpdating] = useState(false);
const [saving, setSaving] = useState(false);
const [timeErrorOpen, setTimeErrorOpen] = useState(false);
const [timeErrors, setTimeErrors] = useState<TimeValidationError[]>([]);

@benkoppe benkoppe Nov 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do timeErrorOpen and timeErrors need to be independent pieces of state? Can one be derived with a useMemo?


const ride = editedRide!; // We know this exists from the context
const rideCompleted = ride.status === Status.COMPLETED;
Expand Down Expand Up @@ -159,6 +162,24 @@ const RideActions: React.FC<RideActionsProps> = ({
};

const handleSave = async () => {
// Validate times before saving; show modal with errors if invalid
const allowPastTimes = false;
const timeValidation = validateRideTimes(
ride.startTime,
ride.endTime,
{
allowPastTimes,
maxDurationHours: 24,
minDurationMinutes: 5,

@benkoppe benkoppe Nov 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How was 5 minutes chosen as the minimum? We might want to just make this 0 or 1 so we never get in the way on minimums.

}
);

if (!timeValidation.isValid) {
setTimeErrors(timeValidation.errors);
setTimeErrorOpen(true);
return;
}

setSaving(true);
try {
const success = await saveChanges();
Expand Down Expand Up @@ -527,6 +548,34 @@ const RideActions: React.FC<RideActionsProps> = ({
</DialogActions>
</Dialog>

{/* Time Validation Errors Modal */}

@benkoppe benkoppe Nov 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I might have missed when this was already discussed, but what was the reasoning behind a modal specifically for these validation errors? Wouldn't something inline on the layout be a little more clear, to avoid stacking modals on top of modals, or are we switching due to layout shift and consistency issues? Either way it might be useful if the PR summary had justification behind this consideration.

<Dialog
open={timeErrorOpen}
onClose={() => setTimeErrorOpen(false)}
fullWidth
maxWidth="sm"
>
<DialogTitle>Cannot Save – Please Fix Time Settings</DialogTitle>
<DialogContent>
<Typography variant="body2" color="textSecondary" gutterBottom>
The following issues were found:
</Typography>
<List>
{timeErrors.map((err, idx) => (
<ListItem key={idx}>
<ListItemIcon>
<ReportIcon color="error" />
</ListItemIcon>
<ListItemText primary={err.message} />
</ListItem>
))}
</List>
</DialogContent>
<DialogActions>
<Button onClick={() => setTimeErrorOpen(false)}>Close</Button>
</DialogActions>
</Dialog>

{/* Contact Admin Modal */}
<Dialog
open={contactAdminOpen}
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/RideDetails/RideEditContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export const RideEditProvider: React.FC<RideEditProviderProps> = ({
editedRide.startTime,
editedRide.endTime,
{
allowPastTimes: !isNewRide(editedRide),
allowPastTimes: false,

@benkoppe benkoppe Nov 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Am I misreading something, or does this change mean past rides can never be edited? Does this effectively freeze past rides in place?

maxDurationHours: 24,
minDurationMinutes: 5,
}
Expand Down
245 changes: 78 additions & 167 deletions frontend/src/components/RideDetails/RideOverview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import { useRideEdit } from './RideEditContext';
import RecurrenceDisplay from './RecurrenceDisplay';
import RiderList from './RiderList';
import { isNewRide } from '../../util/modelFixtures';
import { validateRideTimes } from './TimeValidation';
import styles from './RideOverview.module.css';

interface RideOverviewProps {
Expand Down Expand Up @@ -208,6 +207,9 @@ const RideOverview: React.FC<RideOverviewProps> = ({ userRole }) => {

const formatDateTime = (dateTimeString: string) => {
const date = new Date(dateTimeString);
if (isNaN(date.getTime())) {
return { date: '', time: '' };
}
return {
date: date.toLocaleDateString('en-US', {
weekday: 'long',
Expand All @@ -228,34 +230,25 @@ const RideOverview: React.FC<RideOverviewProps> = ({ userRole }) => {

// Helper functions for date/time editing
const handleStartDateChange = (newDate: Dayjs | null) => {
if (!newDate) return;
if (!newDate || !newDate.isValid()) return;

const currentStartTime = dayjs(ride.startTime);
const startHour = currentStartTime.isValid() ? currentStartTime.hour() : 0;
const startMinute = currentStartTime.isValid() ? currentStartTime.minute() : 0;
const updatedStartTime = newDate
.hour(currentStartTime.hour())
.minute(currentStartTime.minute())
.hour(startHour)
.minute(startMinute)
.second(0)
.millisecond(0);

const currentEndTime = dayjs(ride.endTime);

// Ensure end time is after start time
if (
updatedStartTime.isAfter(currentEndTime) ||
updatedStartTime.isSame(currentEndTime)
) {
const newEndTime = updatedStartTime.add(30, 'minute');
updateRideField('endTime', newEndTime.toISOString());
}

updateRideField('startTime', updatedStartTime.toISOString());
};

const handleStartTimeChange = (newTime: Dayjs | null) => {
if (!newTime) return;
if (!newTime || !newTime.isValid()) return;

const currentStartDate = dayjs(ride.startTime);
const updatedStartTime = currentStartDate
const base = currentStartDate.isValid() ? currentStartDate : dayjs();
const updatedStartTime = base
.hour(newTime.hour())
.minute(newTime.minute())
.second(0)
Expand All @@ -276,23 +269,26 @@ const RideOverview: React.FC<RideOverviewProps> = ({ userRole }) => {
};

const handleEndDateChange = (newDate: Dayjs | null) => {
if (!newDate) return;
if (!newDate || !newDate.isValid()) return;

const currentEndTime = dayjs(ride.endTime);
const endHour = currentEndTime.isValid() ? currentEndTime.hour() : 0;
const endMinute = currentEndTime.isValid() ? currentEndTime.minute() : 0;
const updatedEndTime = newDate
.hour(currentEndTime.hour())
.minute(currentEndTime.minute())
.hour(endHour)
.minute(endMinute)
.second(0)
.millisecond(0);

updateRideField('endTime', updatedEndTime.toISOString());
};

const handleEndTimeChange = (newTime: Dayjs | null) => {
if (!newTime) return;
if (!newTime || !newTime.isValid()) return;

const currentEndDate = dayjs(ride.endTime);
const updatedEndTime = currentEndDate
const base = currentEndDate.isValid() ? currentEndDate : dayjs();
const updatedEndTime = base
.hour(newTime.hour())
.minute(newTime.minute())
.second(0)
Expand Down Expand Up @@ -384,150 +380,65 @@ const RideOverview: React.FC<RideOverviewProps> = ({ userRole }) => {
{isEditing ? (
<LocalizationProvider dateAdapter={AdapterDayjs}>
<div className={styles.editFormFields}>
{(() => {
// For editing existing rides, allow past times; for new rides, don't
const allowPastTimes = !isNewRide(ride);

const validation = validateRideTimes(
ride.startTime,
ride.endTime,
{
allowPastTimes,
maxDurationHours: 24,
minDurationMinutes: 5,
}
);

// Check for specific error types
const startTimePastError = validation.errors.find(
(e) => e.type === 'start_time_past'
);
const endTimeBeforeStartError = validation.errors.find(
(e) =>
e.type === 'end_time_before_start' ||
e.type === 'same_time'
);
const durationError = validation.errors.find(
(e) => e.type === 'too_long_duration'
);

const hasStartTimeError =
startTimePastError !== undefined;
const hasEndTimeError =
endTimeBeforeStartError !== undefined ||
durationError !== undefined;

return (
<>
<div className={styles.dateTimeRow}>
<div style={{ flex: 1 }}>
<DatePicker
label="Start Date"
value={dayjs(ride.startTime)}
onChange={handleStartDateChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
error: hasStartTimeError,
helperText: hasStartTimeError
? ' '
: undefined,
},
}}
/>
</div>
<div style={{ flex: 1 }}>
<TimePicker
label="Start Time"
value={dayjs(ride.startTime)}
onChange={handleStartTimeChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
error: hasStartTimeError,
helperText: hasStartTimeError
? ' '
: undefined,
},
}}
/>
{/* Start time specific error - directly below start time field */}
{startTimePastError && (
<Typography
variant="caption"
color="error"
sx={{
display: 'block',
fontSize: '0.75rem',
marginTop: 0.5,
marginLeft: 1,
}}
>
{startTimePastError.message}
</Typography>
)}
</div>
</div>

<div className={styles.dateTimeRow}>
<div style={{ flex: 1 }}>
<DatePicker
label="End Date"
value={dayjs(ride.endTime)}
onChange={handleEndDateChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
error: hasEndTimeError,
helperText: hasEndTimeError
? ' '
: undefined,
},
}}
/>
</div>
<div style={{ flex: 1 }}>
<TimePicker
label="End Time"
value={dayjs(ride.endTime)}
onChange={handleEndTimeChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
error: hasEndTimeError,
helperText: hasEndTimeError
? ' '
: undefined,
},
}}
/>
{/* End time specific errors - directly below end time field */}
{(endTimeBeforeStartError || durationError) && (
<Typography
variant="caption"
color="error"
sx={{
display: 'block',
fontSize: '0.75rem',
marginTop: 0.5,
marginLeft: 1,
}}
>
{
(endTimeBeforeStartError || durationError)
?.message
}
</Typography>
)}
</div>
</div>
</>
);
})()}
<>
<div className={styles.dateTimeRow}>
<div style={{ flex: 1 }}>
<DatePicker
label="Start Date"
value={dayjs(ride.startTime)}
onChange={handleStartDateChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
},
}}
/>
</div>
<div style={{ flex: 1 }}>
<TimePicker
label="Start Time"
value={dayjs(ride.startTime)}
onChange={handleStartTimeChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
},
}}
/>
</div>
</div>

<div className={styles.dateTimeRow}>
<div style={{ flex: 1 }}>
<DatePicker
label="End Date"
value={dayjs(ride.endTime)}
onChange={handleEndDateChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
},
}}
/>
</div>
<div style={{ flex: 1 }}>
<TimePicker
label="End Time"
value={dayjs(ride.endTime)}
onChange={handleEndTimeChange}
slotProps={{
textField: {
size: 'small',
fullWidth: true,
},
}}
/>
</div>
</div>
</>
</div>
</LocalizationProvider>
) : (
Expand Down
Loading
Loading