Skip to content

Advanced date override handling and timezone compatibility improvements#5

Open
zaibkhan wants to merge 1 commit into
date-algorithm-basefrom
date-algorithm-enhanced
Open

Advanced date override handling and timezone compatibility improvements#5
zaibkhan wants to merge 1 commit into
date-algorithm-basefrom
date-algorithm-enhanced

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

* fix date override for fixed round robin + time zone in date override

* check if slot is within working hours of fixed hosts

* add test for date override in different time zone

* fix date overrides for not fixed hosts (round robin)

* code clean up

* fix added test

* use the correct timezone of user for date overrides

---------

Co-authored-by: CarinaWolli <wollencarina@gmail.com>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Fix override availability logic, honor busy and hours
What’s good: Solid step toward timezone-aware date overrides; tests add coverage for cross-timezone overrides and UTC invariance.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Date override short-circuits busy checks; working-hours e... …/viewer/slots.ts
Early return on dateOverrideExist bypasses busy-time checks and can expose bookable slots overlapping confirmed calendar events or bookings. Example: override 14:00–16:00 with a busy event 15:00–15:30; current code returns true here and never checks busy, incorrectly exposing 15:00. Also within this function: (1) working-hours 'end' is computed using slotStartTime rather than slotEndTime, so slots that cross the end boundary aren't excluded; (2) comparing two dayjs instances with === always yields false—use .isSame() to compare values; (3) consider aligning working-hours day/minute computations to organizerTimeZone rather than UTC to avoid off-by-one day around midnight and DST.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: The new availability check short-circuits when a date override exists and accidentally skips busy-time conflicts and miscomputes slot end minutes against working hours.
  • Testing: Add tests where a date override exists but the invitee/user has a busy event during the override window to ensure conflicts are still blocked. Also include a case where a slot spans the end of working hours (e.g., starts within hours but ends after) to catch the 'end' minute computation bug. Consider DST and day-boundary tests for working-hours checks with organizerTimeZone.
  • Documentation: Inline comments around date override semantics would help clarify intent (whether overrides bypass working hours only vs. all checks). Briefly document the timezone basis for working hours vs. slot times to avoid future regressions.
  • Compatibility: Adding timeZone to TimeRange is backward-compatible due to optional typing; ensure any downstream consumers that serialize/deserialize overrides tolerate the new field.
  • Performance: No material perf regressions spotted. Minor: repeated dayjs(...) computations inside loops could be hoisted, but correctness should be addressed first.
  • Security: No new security surface identified in this change set.
  • Open questions: Should date overrides supersede only working hours (i.e., still honor busy events) or intentionally bypass all other constraints? If the former, we should avoid early return and continue busy checks.

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

) {
// slot is not within the date override
return false;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ High: Early return on dateOverrideExist bypasses busy-time checks and can expose bookable slots overlapping confirmed calendar events or bookings. Example: override 14:00–16:00 with a busy event 15:00–15:30; current code returns true here and never checks busy, incorrectly exposing 15:00. Also within this function: (1) working-hours 'end' is computed using slotStartTime rather than slotEndTime, so slots that cross the end boundary aren't excluded; (2) comparing two dayjs instances with === always yields false—use .isSame() to compare values; (3) consider aligning working-hours day/minute computations to organizerTimeZone rather than UTC to avoid off-by-one day around midnight and DST.

Suggested change
}
```suggestion
// Do not early-return on date overrides; still enforce busy checks.
// Also guard the working-hours check so overrides can supersede working hours.
if (!dateOverrideExist && workingHours.find((workingHour) => {
if (workingHour.days.includes(slotStartTime.day())) {
const start = slotStartTime.hour() * 60 + slotStartTime.minute();
const end = slotEndTime.hour() * 60 + slotEndTime.minute();
if (start < workingHour.startTime || end > workingHour.endTime) {
return true;
}
}
})) {
return false;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants