Skip to content

Commit

Permalink
GIX-1872 Make secondsToDuration consistent with daysToDuration (#3310)
Browse files Browse the repository at this point in the history
# Motivation

We use both `secondsToDuration` and `daysToDuration` to render the same
time period so they should be consistent but currently they are not.

This PR fixes https://dfinity.atlassian.net/browse/GIX-1872.
It does not aim to fix https://dfinity.atlassian.net/browse/GIX-1770
which is still being discussed.
I think it's worthwhile to fix GIX-1872 and put a test in place that the
rendering is consistent before we have a decision on GIX-1770.

# Changes

1. Reimplement `secondsToDuration`.
2. Update tests for `secondsToDuration` for the new implementation.
3. Test that both functions return the same string for periods that are
a whole number of days between 1 and 3000.

# Tests

Extensive unit test are updated.

# Todos

- [x] Add entry to changelog (if necessary).
  • Loading branch information
dskloetd authored Sep 13, 2023
1 parent b106bb4 commit 6025503
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 32 deletions.
1 change: 1 addition & 0 deletions CHANGELOG-Nns-Dapp.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ The NNS Dapp is released through proposals in the Network Nervous System. Theref
* Fixed issues with SetDissolveDelay component.
* Fix sent transaction icon background color dark theme.
* Improve text color of total value locked's label.
* Make duration rendering consistent.

#### Security

Expand Down
8 changes: 6 additions & 2 deletions frontend/src/lib/constants/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@ export const INFINITE_SCROLL_OFFSET = 0.2;
export const DEFAULT_TOAST_DURATION_MILLIS = 4000;

export const SECONDS_IN_MINUTE = 60;
export const SECONDS_IN_HOUR = SECONDS_IN_MINUTE * 60;
export const SECONDS_IN_DAY = SECONDS_IN_HOUR * 24;
export const MINUTES_IN_HOUR = 60;
export const HOURS_IN_DAY = 24;
export const SECONDS_IN_HOUR = SECONDS_IN_MINUTE * MINUTES_IN_HOUR;
export const SECONDS_IN_DAY = SECONDS_IN_HOUR * HOURS_IN_DAY;
// Taking into account 1/4 of leap year
export const SECONDS_IN_YEAR = ((4 * 365 + 1) * SECONDS_IN_DAY) / 4;
export const SECONDS_IN_HALF_YEAR = SECONDS_IN_YEAR / 2;
export const SECONDS_IN_MONTH = SECONDS_IN_YEAR / 12;
export const SECONDS_IN_FOUR_YEARS = SECONDS_IN_YEAR * 4;
export const SECONDS_IN_EIGHT_YEARS = SECONDS_IN_YEAR * 8;

export const DAYS_IN_NON_LEAP_YEAR = 365;

export const NANO_SECONDS_IN_MILLISECOND = 1_000_000;
40 changes: 34 additions & 6 deletions frontend/src/lib/utils/date.utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {
DAYS_IN_NON_LEAP_YEAR,
HOURS_IN_DAY,
MINUTES_IN_HOUR,
SECONDS_IN_DAY,
SECONDS_IN_HOUR,
SECONDS_IN_MINUTE,
SECONDS_IN_MONTH,
SECONDS_IN_YEAR,
Expand All @@ -18,13 +20,39 @@ const createLabel = (labelKey: LabelKey, amount: bigint): LabelInfo => ({
amount: Number(amount),
});

// Returns how many days there are in the given number of years, adding a leap
// day for every 4 years.
const daysInYears = (years: bigint): bigint => {
// Use integer division.
const leapDays = years / BigInt(4);
return years * BigInt(DAYS_IN_NON_LEAP_YEAR) + leapDays;
};

// Returns how many full years, requiring a leap day for every 4 full years,
// there are in the given number of days.
const fullYearsInDays = (days: bigint): bigint => {
// Use integer division.
let years = days / BigInt(DAYS_IN_NON_LEAP_YEAR);
while (daysInYears(years) > days) {
years--;
}
return years;
};

export const secondsToDuration = (seconds: bigint): string => {
const i18nObj = get(i18n);
const years = seconds / BigInt(SECONDS_IN_YEAR);
const days = (seconds % BigInt(SECONDS_IN_YEAR)) / BigInt(SECONDS_IN_DAY);
const hours = (seconds % BigInt(SECONDS_IN_DAY)) / BigInt(SECONDS_IN_HOUR);
const minutes =
(seconds % BigInt(SECONDS_IN_HOUR)) / BigInt(SECONDS_IN_MINUTE);

let minutes = seconds / BigInt(SECONDS_IN_MINUTE);

let hours = minutes / BigInt(MINUTES_IN_HOUR);
minutes -= hours * BigInt(MINUTES_IN_HOUR);

let days = hours / BigInt(HOURS_IN_DAY);
hours -= days * BigInt(HOURS_IN_DAY);

const years = fullYearsInDays(days);
days -= daysInYears(years);

const periods = [
createLabel("year", years),
createLabel("day", days),
Expand Down
47 changes: 23 additions & 24 deletions frontend/src/tests/lib/utils/date.utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,38 +34,36 @@ describe("secondsToDuration", () => {
return secondsToDuration(BigInt(seconds));
};

// Most of these are wrong.
// TODO: Fix.
it("should give year details", () => {
expect(renderSeconds({ nonLeapYears: 1 })).toBe("365 days");
expect(renderSeconds({ nonLeapYears: 1, seconds: 59 })).toBe("365 days");
expect(renderSeconds({ nonLeapYears: 1 })).toBe("1 year");
expect(renderSeconds({ nonLeapYears: 1, seconds: 59 })).toBe("1 year");
expect(renderSeconds({ nonLeapYears: 1, minutes: 59 })).toBe(
"365 days, 59 minutes"
"1 year, 59 minutes"
);
expect(renderSeconds({ nonLeapYears: 1, hours: 23 })).toBe(
"1 year, 23 hours"
);
expect(renderSeconds({ nonLeapYears: 1, days: 1, seconds: -1 })).toBe(
"1 year, 23 hours"
);
expect(renderSeconds({ nonLeapYears: 1, days: 1 })).toBe("1 year");
expect(renderSeconds({ nonLeapYears: 1, days: 2 })).toBe("1 year, 1 day");
expect(renderSeconds({ nonLeapYears: 1, days: 1 })).toBe("1 year, 1 day");
expect(renderSeconds({ nonLeapYears: 1, days: 2 })).toBe("1 year, 2 days");
expect(renderSeconds({ nonLeapYears: 2, seconds: -1 })).toBe(
"1 year, 364 days"
);
expect(renderSeconds({ nonLeapYears: 2 })).toBe("1 year, 364 days");
expect(renderSeconds({ nonLeapYears: 2 })).toBe("2 years");
expect(renderSeconds({ nonLeapYears: 2, minutes: 59 })).toBe(
"1 year, 364 days"
"2 years, 59 minutes"
);
expect(renderSeconds({ nonLeapYears: 2, hours: 23 })).toBe(
"2 years, 23 hours"
);
expect(renderSeconds({ nonLeapYears: 2, days: 1 })).toBe("2 years");
expect(renderSeconds({ nonLeapYears: 2, days: 2 })).toBe("2 years, 1 day");
expect(renderSeconds({ nonLeapYears: 2, days: 1 })).toBe("2 years, 1 day");
expect(renderSeconds({ nonLeapYears: 2, days: 2 })).toBe("2 years, 2 days");
expect(renderSeconds({ nonLeapYears: 3, seconds: -1 })).toBe(
"2 years, 364 days"
);
expect(renderSeconds({ nonLeapYears: 3 })).toBe("2 years, 364 days");
expect(renderSeconds({ nonLeapYears: 3 })).toBe("3 years");
// 4 actual years have a leap day so we add 1 day to 4 nonLeap years.
expect(renderSeconds({ nonLeapYears: 4, days: 1, seconds: -1 })).toBe(
"3 years, 365 days"
Expand All @@ -74,21 +72,15 @@ describe("secondsToDuration", () => {
expect(renderSeconds({ nonLeapYears: 5, days: 1, seconds: -1 })).toBe(
"4 years, 364 days"
);
expect(renderSeconds({ nonLeapYears: 5, days: 1 })).toBe(
"4 years, 365 days"
);
expect(renderSeconds({ nonLeapYears: 5, days: 1 })).toBe("5 years");
expect(renderSeconds({ nonLeapYears: 6, days: 1, seconds: -1 })).toBe(
"5 years, 364 days"
);
expect(renderSeconds({ nonLeapYears: 6, days: 1 })).toBe(
"5 years, 364 days"
);
expect(renderSeconds({ nonLeapYears: 6, days: 1 })).toBe("6 years");
expect(renderSeconds({ nonLeapYears: 7, days: 1, seconds: -1 })).toBe(
"6 years, 364 days"
);
expect(renderSeconds({ nonLeapYears: 7, days: 1 })).toBe(
"6 years, 364 days"
);
expect(renderSeconds({ nonLeapYears: 7, days: 1 })).toBe("7 years");
// 4 actual years have 2 leap days so we add 2 days to 8 nonLeap years.
expect(renderSeconds({ nonLeapYears: 8, days: 2, seconds: -1 })).toBe(
"7 years, 365 days"
Expand All @@ -97,9 +89,7 @@ describe("secondsToDuration", () => {
expect(renderSeconds({ nonLeapYears: 9, days: 2, seconds: -1 })).toBe(
"8 years, 364 days"
);
expect(renderSeconds({ nonLeapYears: 9, days: 2 })).toBe(
"8 years, 365 days"
);
expect(renderSeconds({ nonLeapYears: 9, days: 2 })).toBe("9 years");
});

it("should give day details", () => {
Expand Down Expand Up @@ -191,6 +181,15 @@ describe("daysToDuration", () => {
expect(daysToDuration(8 * 365 + 2)).toBe("8 years");
expect(daysToDuration(8 * 365 + 3)).toBe("8 years, 1 day");
});

it("should be consistent with secondsToDuration", () => {
for (let days = 1; days < 3000; days++) {
expect({ days, duration: daysToDuration(days) }).toEqual({
days,
duration: secondsToDuration(BigInt(days * SECONDS_IN_DAY)),
});
}
});
});

describe("secondsToDissolveDelayDuration", () => {
Expand Down

0 comments on commit 6025503

Please sign in to comment.