Skip to content
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

Jump to bottom

fix: Display the correct remaining dates in given tz #19389

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: Display the correct remaining dates in given tz #19389

wants to merge 4 commits into from
+90 −28

Conversation

emrysal
Copy link
Contributor

@emrysal emrysal commented Feb 19, 2025

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

See tests.

@linear Linear
Copy link

linear bot commented Feb 19, 2025

CAL-4055 Not able to book a time on a date that is earlier than today(but actually is today in the browsing timezone)

@graphite-app graphite-app bot requested a review from a team February 19, 2025 17:09
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 19, 2025 •
edited
Loading

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Display the correct remaining dates in given tz". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@github-actions github-actions bot added bookings area: bookings, availability, timezones, double booking community Created by Linear-GitHub Sync High priority Created by Linear-GitHub Sync 🐛 bug Something isn't working labels Feb 19, 2025
@graphite-app Graphite App
Copy link

graphite-app bot commented Feb 19, 2025

Graphite Automations

"Add foundation team as reviewer" took an action on this PR • (02/19/25)

1 reviewer was added to this PR based on Keith Williams's automation.

@keithwillcode keithwillcode added core area: core, team members only foundation labels Feb 19, 2025
@dosubot dosubot bot added the calendar-apps area: calendar, google calendar, outlook, lark, microsoft 365, apple calendar label Feb 19, 2025
@vercel Vercel
Copy link

vercel bot commented Feb 19, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cal-com-ui-playground ❌ Failed (Inspect) Feb 19, 2025 5:28pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Feb 19, 2025 5:28pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Feb 19, 2025 5:28pm

emrysal
emrysal commented Feb 19, 2025
View reviewed changes
packages/features/calendars/DatePicker.tsx
@@ -160,7 +160,7 @@ const Days = ({
const weekdayOfFirst = browsingDate.date(1).day();

const includedDates = getAvailableDatesInMonth({
browsingDate: browsingDate.toDate(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was losing the information given in the browsingDate, which includes custom tz's selected through the timezone selector. getAvailableDatesInMonth needs this.

packages/features/calendars/lib/getAvailableDatesInMonth.timezone.test.ts
expect(result).toHaveLength(daysInMonth(currentDate) - currentDate.getDate() + 1);
}
{
const currentDate = dayjs().startOf("month");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests the remaining available dates are cut off according to the current time, even if the "currentDate" is in the past.

packages/features/calendars/lib/getAvailableDatesInMonth.timezone.test.ts
@@ -82,4 +98,47 @@ describe("Test Suite: Date Picker", () => {
vi.useRealTimers();
});
});

test("it returns the current date in selected timezone", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are (a slightly simplified) copy of #15840 - thanks @vijayraghav-io 🙏

packages/features/calendars/lib/getAvailableDatesInMonth.ts
import dayjs from "@calcom/dayjs";
import { daysInMonth, yyyymmdd } from "@calcom/lib/date-fns";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rely fully on Dayjs for now (until we have a TimezonedDate object to efficiently pass the wrapped Date object complimented by booker tz.

packages/features/calendars/lib/getAvailableDatesInMonth.ts
minDate?: Date;
includedDates?: string[];
}) {
// get minDate but with the same UTC offset as the browsingDate.
const minDayjs = dayjs(minDate).utcOffset(browsingDate.utcOffset());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible without hacks to get the original timezone from the passed date, but we only need UTC offset granularity to shift the min start time for dateString determination.

packages/features/calendars/lib/getAvailableDatesInMonth.ts
// or is the same day, in the same month, in the same year.
date < lastDateOfMonth || dayjs(date).isSame(lastDateOfMonth, "day");
date = new Date(date.getFullYear(), date.getMonth(), date.getDate() + 1)
date.valueOf() <= lastDateOfMonth.valueOf();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

more efficient isSameOrBefore check - needed as isSame on a tz'ed Date is not efficient.

@emrysal emrysal mentioned this pull request Feb 19, 2025
Draft
3 tasks
@emrysal emrysal changed the title Display the correct remaining dates in given tz fix: Display the correct remaining dates in given tz Feb 19, 2025
@CarinaWolli
Copy link
Member

This doesn't fix the issue for me. See the loom what I tested: https://www.loom.com/share/cb914fe7f1f34274bacb2ccb27e6a4c3

@vijayraghav-io
Copy link
Contributor

Just in case, if decision is made to go back with this PR - #15840
Video showing fix is attached in it's description, 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working calendar-apps area: calendar, google calendar, outlook, lark, microsoft 365, apple calendar community Created by Linear-GitHub Sync core area: core, team members only foundation High priority Created by Linear-GitHub Sync
Projects
None yet
Milestone
No milestone
4 participants
@emrysal @CarinaWolli @vijayraghav-io @keithwillcode