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

Fixing incorrect message timing display when changing timezones #27838

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 49 additions & 6 deletions src/libs/DateUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,10 @@ import {es, enGB} from 'date-fns/locale';
import {
formatDistanceToNow,
subMinutes,
addDays,
subDays,
isBefore,
subMilliseconds,
isToday,
isTomorrow,
isYesterday,
startOfWeek,
endOfWeek,
format,
Expand Down Expand Up @@ -85,6 +84,47 @@ function getLocalDateFromDatetime(locale, datetime, currentSelectedTimezone = ti
return utcToZonedTime(parsedDatetime, currentSelectedTimezone);
}

/**
* Checks if a given date is today in the specified time zone.
*
* @param {Date} date - The date to compare.
* @param {String} timeZone - The time zone to consider.
* @returns {Boolean} True if the date is today; otherwise, false.
*/
function isToday(date, timeZone) {
const currentDate = new Date();
const currentDateInTimeZone = utcToZonedTime(currentDate, timeZone);
return isSameDay(date, currentDateInTimeZone);
}

/**
* Checks if a given date is tomorrow in the specified time zone.
*
* @param {Date} date - The date to compare.
* @param {String} timeZone - The time zone to consider.
* @returns {Boolean} True if the date is tomorrow; otherwise, false.
*/
function isTomorrow(date, timeZone) {
const currentDate = new Date();
const tomorrow = addDays(currentDate, 1); // Get the date for tomorrow in the current time zone
const tomorrowInTimeZone = utcToZonedTime(tomorrow, timeZone);
return isSameDay(date, tomorrowInTimeZone);
}

/**
* Checks if a given date is yesterday in the specified time zone.
*
* @param {Date} date - The date to compare.
* @param {String} timeZone - The time zone to consider.
* @returns {Boolean} True if the date is yesterday; otherwise, false.
*/
function isYesterday(date, timeZone) {
const currentDate = new Date();
const yesterday = subDays(currentDate, 1); // Get the date for yesterday in the current time zone
const yesterdayInTimeZone = utcToZonedTime(yesterday, timeZone);
return isSameDay(date, yesterdayInTimeZone);
}

/**
* Formats an ISO-formatted datetime string to local date and time string
*
Expand Down Expand Up @@ -117,13 +157,13 @@ function datetimeToCalendarTime(locale, datetime, includeTimeZone = false, curre
yesterdayAt = yesterdayAt.toLowerCase();
}

if (isToday(date)) {
if (isToday(date, currentSelectedTimezone)) {
return `${todayAt} ${format(date, CONST.DATE.LOCAL_TIME_FORMAT)}${tz}`;
}
if (isTomorrow(date)) {
if (isTomorrow(date, currentSelectedTimezone)) {
return `${tomorrowAt} ${format(date, CONST.DATE.LOCAL_TIME_FORMAT)}${tz}`;
}
if (isYesterday(date)) {
if (isYesterday(date, currentSelectedTimezone)) {
return `${yesterdayAt} ${format(date, CONST.DATE.LOCAL_TIME_FORMAT)}${tz}`;
}
if (date >= startOfCurrentWeek && date <= endOfCurrentWeek) {
Expand Down Expand Up @@ -344,6 +384,9 @@ const DateUtils = {
subtractMillisecondsFromDateTime,
getDateStringFromISOTimestamp,
getStatusUntilDate,
isToday,
isTomorrow,
isYesterday,
};

export default DateUtils;
30 changes: 29 additions & 1 deletion tests/unit/DateUtilsTest.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Onyx from 'react-native-onyx';
import {format as tzFormat} from 'date-fns-tz';
import {format as tzFormat, utcToZonedTime} from 'date-fns-tz';
import {addMinutes, subHours, subMinutes, subSeconds, format, setMinutes, setHours, subDays, addDays} from 'date-fns';
import CONST from '../../src/CONST';
import DateUtils from '../../src/libs/DateUtils';
Expand Down Expand Up @@ -130,6 +130,34 @@ describe('DateUtils', () => {
expect(result).toBe(expectedDateTime);
});

describe('Date Comparison Functions', () => {
const today = new Date();
const tomorrow = addDays(today, 1);
const yesterday = subDays(today, 1);

const todayInTimezone = utcToZonedTime(today, timezone);
const tomorrowInTimezone = utcToZonedTime(tomorrow, timezone);
const yesterdayInTimezone = utcToZonedTime(yesterday, timezone);

it('isToday should correctly identify today', () => {
expect(DateUtils.isToday(todayInTimezone, timezone)).toBe(true);
expect(DateUtils.isToday(tomorrowInTimezone, timezone)).toBe(false);
expect(DateUtils.isToday(yesterdayInTimezone, timezone)).toBe(false);
Copy link
Contributor

@Beamanator Beamanator Nov 6, 2023

Choose a reason for hiding this comment

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

FYI it looks like this is failing in early timezones like mine - see https://expensify.slack.com/archives/C01GTK53T8Q/p1699255028952729?thread_ts=1699253289.471629&cid=C01GTK53T8Q

This isn't a HUGE problem since most of our devs are in later timezones, but it's a bit annoying for me :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if these changes make sense to you @abzokhattab - they are currently passing for me: #30900

I'll run these tests again in 20 minutes, when Los Angeles becomes the same day as me :D

Copy link
Contributor Author

@abzokhattab abzokhattab Nov 6, 2023

Choose a reason for hiding this comment

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

Interesting!

i still don't get the difference between the two approaches, aren't they returning the same result? no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to look into it a bit more early my timezone - I agree they look like the same approach, but somehow this let my updated test pass 😅

});

it('isTomorrow should correctly identify tomorrow', () => {
expect(DateUtils.isTomorrow(tomorrowInTimezone, timezone)).toBe(true);
expect(DateUtils.isTomorrow(todayInTimezone, timezone)).toBe(false);
expect(DateUtils.isTomorrow(yesterdayInTimezone, timezone)).toBe(false);
});

it('isYesterday should correctly identify yesterday', () => {
expect(DateUtils.isYesterday(yesterdayInTimezone, timezone)).toBe(true);
expect(DateUtils.isYesterday(todayInTimezone, timezone)).toBe(false);
expect(DateUtils.isYesterday(tomorrowInTimezone, timezone)).toBe(false);
});
});

describe('getDBTime', () => {
it('should return the date in the format expected by the database', () => {
const getDBTime = DateUtils.getDBTime();
Expand Down
Loading