Conversation
|
the original PR had some bugs that need to be addressed:
|
There was a problem hiding this comment.
Pull request overview
This PR reverts a previous revert (PR #1231), effectively re-introducing a comprehensive form refactoring that modernizes the booking form's sponsor and secondary contact handling. The changes improve data collection by splitting names into first/last components, standardize email input formats, and add automatic security requirements for large events.
Changes:
- Refactored sponsor email field to accept Net ID format only (e.g., "abc123") instead of full email addresses
- Split secondary contact name into separate first/last name fields and added secondary contact email field with full NYU email validation
- Added automatic security requirement for events with 75+ attendees with manual override tracking
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| booking-app/tests/unit/form-context-field-visibility.unit.test.tsx | Updated test expectations for new sponsor email label format |
| booking-app/tests/unit/calendar-description.unit.test.ts | Added "en-US" locale to date/time formatting for consistency |
| booking-app/tests/unit/MoreInfoModal.basic.unit.test.tsx | Updated expected count of "none" values to account for new secondary email field |
| booking-app/components/src/utils/validationHelpers.ts | New utility with centralized Net ID and NYU email regex patterns and validation functions |
| booking-app/components/src/utils/formatters.ts | Added getSecondaryContactName helper for backwards-compatible name handling |
| booking-app/components/src/types.ts | Updated Inputs type to split secondaryName into firstName/lastName/email fields |
| booking-app/components/src/server/db.ts | Standardized date/time formatting with explicit "en-US" locale |
| booking-app/components/src/server/calendars.ts | Updated calendar description to include secondary contact email and use new helper |
| booking-app/components/src/server/admin.ts | Added secondary contact email sending in approval flow with legacy format handling |
| booking-app/components/src/client/routes/components/bookingTable/MoreInfoModal.tsx | Updated modal to display split secondary contact fields and new email field |
| booking-app/components/src/client/routes/booking/components/FormInput.tsx | Major refactor: sponsor email to Net ID, secondary contact split, auto-security logic, ChartField validation |
| booking-app/components/src/client/routes/booking/components/BookingFormInputs.tsx | Enhanced validation logic for optional fields and added disabled prop support |
| booking-app/app/templates/booking_detail.html | Updated email template to use secondaryContactName helper and display secondary email |
| booking-app/app/api/bookings/route.ts | Standardized date/time formatting and added secondaryContactName to email context |
| booking-app/app/api/bookings/edit/route.ts | Added secondaryContactName to edit notification email context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
booking-app/components/src/types.ts
Outdated
| secondaryFirstName: string; | ||
| secondaryLastName: string; | ||
| secondaryEmail: string; |
There was a problem hiding this comment.
The secondaryName field is marked as optional (secondaryName?: string) for backwards compatibility, but the new fields secondaryFirstName, secondaryLastName, and secondaryEmail are required (no ? operator). This creates an inconsistency where old bookings have secondaryName and new bookings have the split fields. Consider making the new fields optional as well to better represent the data model, or add a comment explaining that these fields are mutually exclusive.
| secondaryFirstName: string; | |
| secondaryLastName: string; | |
| secondaryEmail: string; | |
| // Secondary attendee: legacy data may use `secondaryName` only; newer flows use the split fields. | |
| secondaryFirstName?: string; | |
| secondaryLastName?: string; | |
| secondaryEmail?: string; |
| <BookingFormTextField | ||
| id="sponsorEmail" | ||
| label="Sponsor Email" | ||
| description="Must be an nyu.edu email address." | ||
| label="Sponsor Email (NYU Net ID)" | ||
| description="Enter the sponsor's NYU Net ID (e.g., abc123)" | ||
| required={watch("role") === Role.STUDENT} | ||
| pattern={{ | ||
| value: /^[A-Z0-9._%+-]+@nyu.edu$/i, | ||
| message: "Invalid email address", | ||
| value: NET_ID_REGEX, | ||
| message: "Invalid Net ID", | ||
| }} | ||
| validate={validateSponsorEmailSimple} | ||
| validate={validateSponsorNetIdSimple} | ||
| {...{ control, errors, trigger }} |
There was a problem hiding this comment.
The sponsor email field now accepts Net ID format only (e.g., "abc123"), but the email is sent to the stored value without transformation to a full email address in the serverApproveEvent function. Unlike secondaryEmail which has legacy format handling at lines 677-679 in admin.ts, sponsorEmail is used directly at line 663. This means sponsor emails will be sent to invalid addresses like "abc123" instead of "abc123@nyu.edu". The sponsor email needs transformation logic similar to secondaryEmail when approval emails are sent.
| // for sponsor, if we have one | ||
| const contents = await serverBookingContents(id, tenant); | ||
| if (contents.role === "Student" && contents.sponsorEmail?.length > 0) { | ||
| serverSendBookingDetailEmail({ | ||
| calendarEventId: id, | ||
| targetEmail: contents.sponsorEmail, | ||
| headerMessage: | ||
| "A reservation that you are the Sponsor of has been approved.<br /><br />" + | ||
| emailConfig.emailMessages.approvalNotice, | ||
| status: BookingStatusLabel.APPROVED, | ||
| replyTo: guestEmail, | ||
| tenant, | ||
| }); |
There was a problem hiding this comment.
The sponsor email is being used directly without transformation from Net ID format to full email format. Since the form now accepts Net IDs only (not full emails), this code will attempt to send emails to invalid addresses like "abc123" instead of "abc123@nyu.edu". Add transformation logic to construct the full email address before sending, similar to how secondaryEmail is handled at lines 677-679.
| const expectedAttendanceValue = watch("expectedAttendance"); | ||
| const isLargeEvent = parseInt(expectedAttendanceValue || "0") >= 75; | ||
|
|
||
| const hireSecurityValue = watch("hireSecurity"); | ||
| // Track if hireSecurity was auto-set by attendance logic | ||
| const hireSecurityWasAutoSet = useRef(false); | ||
| // Track the last value we set automatically (e.g. "yes" or "") | ||
| const autoHireSecurityValueRef = useRef<string | undefined>(undefined); | ||
| // Track whether the user has manually overridden hireSecurity | ||
| const hireSecurityManuallySet = useRef(false); | ||
|
|
||
| // Detect manual changes to hireSecurity by comparing against the last auto-set value | ||
| useEffect(() => { | ||
| // If we have an auto baseline, and the current value differs, treat as manual override | ||
| if ( | ||
| autoHireSecurityValueRef.current !== undefined && | ||
| hireSecurityValue !== autoHireSecurityValueRef.current | ||
| ) { | ||
| hireSecurityManuallySet.current = true; | ||
| hireSecurityWasAutoSet.current = false; | ||
| autoHireSecurityValueRef.current = undefined; | ||
| } | ||
| }, [hireSecurityValue]); | ||
|
|
||
| useEffect(() => { | ||
| // Do not auto-manage hireSecurity if the user has manually overridden it | ||
| if (hireSecurityManuallySet.current) { | ||
| return; | ||
| } | ||
| if (isLargeEvent) { | ||
| if (hireSecurityValue !== "yes") { | ||
| setValue("hireSecurity", "yes", { shouldValidate: true }); | ||
| hireSecurityWasAutoSet.current = true; | ||
| autoHireSecurityValueRef.current = "yes"; | ||
| } | ||
| } else { | ||
| // Only reset if it was auto-set previously | ||
| if (hireSecurityWasAutoSet.current && hireSecurityValue !== "") { | ||
| setValue("hireSecurity", "", { shouldValidate: true }); | ||
| hireSecurityWasAutoSet.current = false; | ||
| autoHireSecurityValueRef.current = ""; | ||
| } | ||
| } | ||
| }, [isLargeEvent, hireSecurityValue, setValue]); |
There was a problem hiding this comment.
The auto-setting logic for hireSecurity could cause issues if expectedAttendance is changed from above 75 to below 75 and then back above 75. Once the user manually changes hireSecurity (line 244), the hireSecurityManuallySet flag is set to true and never reset. This means if attendance goes from 80 -> 70 -> 85, hireSecurity won't auto-set to "yes" on the second threshold crossing since it's still marked as manually set. Consider resetting hireSecurityManuallySet when expectedAttendance crosses the threshold in the auto-enabling direction.
| validate: (value) => { | ||
| if (!required) return true; | ||
| const isNotEmpty = value?.trim().length > 0; | ||
| const isValid = validate(value); | ||
| return (isNotEmpty && isValid) || `${label} is required`; | ||
| // If field is not required and empty, allow it | ||
| if (!required && (!value || value.trim().length === 0)) return true; | ||
|
|
||
| // Check for whitespace-only values | ||
| if (value && typeof value === "string" && value.trim().length === 0) { | ||
| return `${label} cannot be empty whitespace`; | ||
| } | ||
|
|
||
| // For required fields, check if value is valid | ||
| if (required) { | ||
| const isNotEmpty = value?.trim().length > 0; | ||
| const isValid = validate(value); | ||
| return (isNotEmpty && isValid) || `${label} is required`; | ||
| } | ||
|
|
||
| // For optional fields with a value, run custom validation | ||
| return validate(value); | ||
| }, |
There was a problem hiding this comment.
The validate function will be called with the value even for optional fields with values (line 173), but the validate parameter defaults to returning true (line 147). This means if a custom validation function expects a non-empty value but receives an empty string for an optional field, it should handle that case. Consider adding a comment to clarify that custom validate functions for optional fields should handle empty values, or ensure empty values aren't passed to custom validators for optional fields.
| await fetch(`${process.env.NEXT_PUBLIC_BASE_URL}/api/inviteUser`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify(secondaryFormData), | ||
| }); |
There was a problem hiding this comment.
The fetch to /api/inviteUser for the secondary contact is awaited but doesn't have error handling. If the invite fails, the error will propagate and potentially stop the approval process. While it's acceptable to fail the approval if email delivery fails, the error message won't be informative. Consider adding try-catch with a specific error message, or at minimum add a comment explaining the intentional error propagation behavior.
| await fetch(`${process.env.NEXT_PUBLIC_BASE_URL}/api/inviteUser`, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify(secondaryFormData), | |
| }); | |
| try { | |
| const inviteResponse = await fetch( | |
| `${process.env.NEXT_PUBLIC_BASE_URL}/api/inviteUser`, | |
| { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify(secondaryFormData), | |
| } | |
| ); | |
| if (!inviteResponse.ok) { | |
| throw new Error( | |
| `Failed to invite secondary contact (${secondaryEmailAddress}); received status ${inviteResponse.status}.` | |
| ); | |
| } | |
| } catch (error) { | |
| // Intentionally propagate a descriptive error so the approval failure is informative | |
| const message = | |
| error instanceof Error ? error.message : String(error); | |
| throw new Error( | |
| `Failed to invite secondary contact (${secondaryEmailAddress}): ${message}` | |
| ); | |
| } |
| label="ChartField for Room Setup" | ||
| required={false} | ||
| pattern={{ | ||
| value: /^.{5}-.{2}-.{5}-.{5}$/, |
There was a problem hiding this comment.
The ChartField validation pattern uses /^.{5}-.{2}-.{5}-.{5}$/ which accepts ANY 5 characters (including spaces or special characters) in each segment. This is overly permissive. Consider using a more specific pattern like /^[A-Z0-9]{5}-[A-Z0-9]{2}-[A-Z0-9]{5}-[A-Z0-9]{5}$/ if ChartFields should only contain alphanumeric characters, or document the expected format more clearly if the current pattern is intentional.
| value: /^.{5}-.{2}-.{5}-.{5}$/, | |
| value: /^[A-Z0-9]{5}-[A-Z0-9]{2}-[A-Z0-9]{5}-[A-Z0-9]{5}$/, |
| @@ -319,12 +319,12 @@ describe("Calendar Description Functions", () => { | |||
| ...data, | |||
| roomId: selectedRoomIds, | |||
| startDate: startDateObj.toLocaleDateString(), | |||
There was a problem hiding this comment.
The test function uses toLocaleDateString() without specifying a locale at line 321, but the actual production code in booking-app/app/api/bookings/route.ts:195 uses toLocaleDateString("en-US"). For consistency and to ensure the test accurately reflects production behavior, add the "en-US" locale parameter here as well.
…e secondary contact fields for compatibility
…package-lock.json conflict
…otifications to attendees - Added sendUpdates: 'all' to insertEvent() when creating calendar events - Added sendUpdates: 'all' to patchCalendarEvent() when updating events (e.g., adding secondary contacts) - Added sendUpdates: 'all' to deleteEvent() when deleting events This ensures that secondary contacts and other attendees receive Google Calendar notifications when they are added to bookings, when events are updated, or when bookings are cancelled.
- Updated 3 tests in server-calendars.unit.test.ts to expect sendUpdates: 'all' parameter - Added new test 'deleteEvent sends cancellation notifications to all attendees' - Fixed date locale issue in calendar-description.unit.test.ts (toLocaleDateString without locale)
ScreenshotsFix to "the secondary contact email field is validating netIDs (i.e., abc123) but it needs to validate nyu netID emails (i.e., abc123@nyu.edu)."
Fix to "the secondary contact is being invited to the approved calendar event however they are not receiving an approved email notification."Notes: Add sendUpdates parameter to Google Calendar API calls to send notifications to attendees
|
|
@n3xta I see in your screenshots the Google Calendar invite email but did you confirm the Secondary Contact is receiving an APPROVED email notification from the booking app? |
| // Handle both legacy full email format and new Net ID format | ||
| const sponsorEmailAddress = contents.sponsorEmail.includes("@") | ||
| ? contents.sponsorEmail | ||
| : `${contents.sponsorEmail}@nyu.edu`; |
There was a problem hiding this comment.
@n3xta
sponsorEmail field stores an email address, so why did you add this line?
| * @param value - The string to validate | ||
| * @returns true if the value matches NYU email format, false otherwise | ||
| */ | ||
| export const isValidNyuEmailFormat = (value: string): boolean => { |
There was a problem hiding this comment.
Where do you use isValidNyuEmailFormat ?
| const expectedAttendanceValue = watch("expectedAttendance"); | ||
| const isLargeEvent = parseInt(expectedAttendanceValue || "0") >= 75; | ||
|
|
||
| const hireSecurityValue = watch("hireSecurity"); | ||
| // Track if hireSecurity was auto-set by attendance logic | ||
| const hireSecurityWasAutoSet = useRef(false); | ||
| // Track the last value we set automatically (e.g. "yes" or "") | ||
| const autoHireSecurityValueRef = useRef<string | undefined>(undefined); | ||
| // Track whether the user has manually overridden hireSecurity | ||
| const hireSecurityManuallySet = useRef(false); | ||
|
|
||
| // Detect manual changes to hireSecurity by comparing against the last auto-set value | ||
| useEffect(() => { | ||
| // If we have an auto baseline, and the current value differs, treat as manual override | ||
| if ( | ||
| autoHireSecurityValueRef.current !== undefined && | ||
| hireSecurityValue !== autoHireSecurityValueRef.current | ||
| ) { | ||
| hireSecurityManuallySet.current = true; | ||
| hireSecurityWasAutoSet.current = false; | ||
| autoHireSecurityValueRef.current = undefined; | ||
| } | ||
| }, [hireSecurityValue]); | ||
|
|
||
| useEffect(() => { | ||
| // Do not auto-manage hireSecurity if the user has manually overridden it | ||
| if (hireSecurityManuallySet.current) { | ||
| return; | ||
| } | ||
| if (isLargeEvent) { | ||
| if (hireSecurityValue !== "yes") { | ||
| setValue("hireSecurity", "yes", { shouldValidate: true }); | ||
| hireSecurityWasAutoSet.current = true; | ||
| autoHireSecurityValueRef.current = "yes"; | ||
| } | ||
| } else { | ||
| // Only reset if it was auto-set previously | ||
| if (hireSecurityWasAutoSet.current && hireSecurityValue !== "") { | ||
| setValue("hireSecurity", "", { shouldValidate: true }); | ||
| hireSecurityWasAutoSet.current = false; | ||
| autoHireSecurityValueRef.current = ""; | ||
| } | ||
| } | ||
| }, [isLargeEvent, hireSecurityValue, setValue]); |
| @@ -693,14 +785,14 @@ export default function FormInput({ | |||
| /> | |||
| <BookingFormTextField | |||
| id="sponsorEmail" | |||
There was a problem hiding this comment.
Please don’t use the same column to store different types of data (like an email address and a NetID).
If you want to keep using this column as is, please convert it to an email format before saving it.
3 participants
Reverts #1231