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.

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

Fix leap year issue #2453

Merged
merged 4 commits into from
May 14, 2024
Merged

Fix leap year issue #2453

merged 4 commits into from
May 14, 2024

Conversation

SleeplessOne1917
Copy link
Member

Closes #2441

Besides fixing the bug listed in the issue, if a user creates their account on February 29th of a leap year, their cake day is March 1st for non leap years.

import { parseISO, getYear, getDayOfYear } from "date-fns";
import { parseISO, getYear, getDayOfYear, isLeapYear } from "date-fns";

const leapDay = getDayOfYear(new Date(2024, 1, 29));

export default function isCakeDay(published: string): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

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

I made sure to test that this behaves as described in the PR description. If we weren't pivoting to a rewritten UI, this would be a prime candidate for unit testing.

@matc-pub
Copy link
Collaborator

Date constructor docs (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date):

If any parameter overflows its defined bounds, it "carries over". For example, if a monthIndex greater than 11 is passed in, those months will cause the year to increment; if a minutes greater than 59 is passed in, hours will increment accordingly, etc. Therefore, new Date(1990, 12, 1) will return January 1st, 1991; new Date(2020, 5, 19, 25, 65) will return 2:05 A.M. June 20th, 2020.

setMonth and setFullYear have similar descriptions in their docs. This also considers leap days in non leap years as first of march:

  return (
    isSameDay(currentDate, setYear(createDate, getYear(currentDate))) &&
    !isSameYear(currentDate, createDate)
  );

There is also an issue with how Profile displays the date. The same profile can show different dates to users in different timezones. This assertion will fail unless the user is in UTC+0:

console.assert(
  format(parseISO("2023-05-11T00:00:00.000Z"), "PPP") ===
    format(parseISO("2023-05-11T23:59:00.000Z"), "PPP"),
);

<span className="ms-2">
{I18NextService.i18n.t("cake_day_title")}{" "}
{format(parseISO(pv.person.published), "PPP")}
</span>

@SleeplessOne1917
Copy link
Member Author

Is the first codeblock you shared supposed to be a simpler implementation and/or fix the timezone bug you mentioned? If so, I can change the PR to use that instead.

@matc-pub
Copy link
Collaborator

Just simpler.

I think this should fix the timezone issue, as long as published always starts with "yyyy-MM-dd" (the optional current date is just for testing):

import {
  format,
  getYear,
  isSameDay,
  isSameYear,
  parse,
  setYear,
} from "date-fns";

// Returns a date in local time with the same year, month and day. Ignores the
// source timezone.
export function cakeDate(published: string): Date {
  return parse(published.substring(0, 10), "yyyy-MM-dd", new Date(0));
}

export default function isCakeDay(published: string, current?: Date): boolean {
  const createDate = cakeDate(published);
  const currentDate = current ?? new Date();

  return (
    isSameDay(currentDate, setYear(createDate, getYear(currentDate))) &&
    !isSameYear(currentDate, createDate)
  );
}

console.assert(
  isCakeDay("2023-05-11T00:00:00.000Z", new Date(2024, 4, 11)),
  "basic, early",
);
console.assert(
  isCakeDay("2023-05-11T23:59:00.000Z", new Date(2024, 4, 11)),
  "basic, late",
);

console.assert(
  !isCakeDay("2024-05-11T00:00:00.000Z", new Date(2024, 4, 11)),
  "not today, early",
);
console.assert(
  !isCakeDay("2024-05-11T23:59:00.000Z", new Date(2024, 4, 11)),
  "not today, late",
);

console.assert(
  isCakeDay("2020-02-29T00:00:00.000Z", new Date(2024, 1, 29)),
  "leap day, leap year",
);
console.assert(
  isCakeDay("2020-02-29T00:00:00.000Z", new Date(2025, 2, 1)),
  "leap day, non leap year",
);

console.assert(
  isCakeDay("2023-03-01T00:00:00.000Z", new Date(2024, 2, 1)),
  "first of march, leap year",
);
console.assert(
  isCakeDay("2023-03-01T00:00:00.000Z", new Date(2024, 2, 1)),
  "first of march, non leap year",
);

console.assert(
  isCakeDay("2020-03-01T00:00:00.000Z", new Date(2024, 2, 1)),
  "first of march leap year, leap year",
);
console.assert(
  isCakeDay("2020-03-01T00:00:00.000Z", new Date(2024, 2, 1)),
  "first of march leap year, non leap year",
);

console.assert(
  format(cakeDate("2023-05-11T00:00:00.000Z"), "PPP") ===
    format(cakeDate("2023-05-11T23:59:00.000Z"), "PPP"),
);
console.assert(
  format(cakeDate("2023-05-11T23:59:00.000Z"), "PPP") === "May 11th, 2023",
  format(cakeDate("2023-05-11T23:59:00.000Z"), "PPP"),
);

@dessalines
Copy link
Member

@matc-pub could you open that one up either as a PR to this branch, or to main?

matc-pub and others added 2 commits May 12, 2024 11:53
* Show same cake day date independent of timezone

* Remove commented out assertions
@SleeplessOne1917 SleeplessOne1917 enabled auto-merge (squash) May 12, 2024 15:57
@SleeplessOne1917 SleeplessOne1917 merged commit d705f36 into main May 14, 2024
2 checks passed
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.

Cake days are 1 day off on leap years
3 participants