Page MenuHomePhabricator

[betalabs-Regression] Special:Notifications page displays wrong date format for messages
Closed, ResolvedPublic

Description

When checking Special:Notifications page (at Aug 02 at 12:10 pm PDT) for messages that were received a week ago (on Jul 26), the date format differs from regular format.

Per @Catrope investigation:
moment().diff(moment("2016-07-26T11:00:00-07:00"), "weeks")
1
moment().diff(moment("2016-07-26T15:00:00-07:00"), "weeks")
0

Event Timeline

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptAug 2 2016, 7:26 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Mooeypoo claimed this task.Aug 2 2016, 11:10 PM
Mooeypoo added a subscriber: Mooeypoo.

We are calculating the "am I after 7 days" wrong.

		momentTimestamp = moment.utc( this.model.getTimestamp() );
		diff = now.diff( momentTimestamp, 'weeks' );

This is what we use to decide whether to display a day name ("Today", "Yesteday", "Monday" ... ) or a full day. My hunch is that this is calculated wrong for utc difference (which would explain how this happens (a) only in the "stitch" between day names and dates, and (b) only in the first half of the day, but not after 3pm)

We need to make sure that the difference is calculated locally and not per utc, and definitely not mixed utc and local as it seems to be above.

Change 302620 had a related patch set uploaded (by Mooeypoo):
Convert to local time when calculating weeks difference in moment

https://gerrit.wikimedia.org/r/302620

We are calculating the "am I after 7 days" wrong.

		momentTimestamp = moment.utc( this.model.getTimestamp() );
		diff = now.diff( momentTimestamp, 'weeks' );

This is what we use to decide whether to display a day name ("Today", "Yesteday", "Monday" ... ) or a full day. My hunch is that this is calculated wrong for utc difference (which would explain how this happens (a) only in the "stitch" between day names and dates, and (b) only in the first half of the day, but not after 3pm)
We need to make sure that the difference is calculated locally and not per utc, and definitely not mixed utc and local as it seems to be above.

I debugged this separately from you earlier today and came to a different conclusion: I think we're calculating it right but rounding differently.

If now is Tuesday at 11am, and ts is last Tuesday at 3pm, then ts-now is 0.976 weeks, which gets rounded down. However, last Tuesday is not considered part of lastweek by the code underlying .calendar(), and so it doesn't hit our custom locale code correctly.

It may be that you're right and I'm wrong, but let's test with this case (i.e. today a week ago's latest notification is later in the day than the present time is).

We are calculating the "am I after 7 days" wrong.

		momentTimestamp = moment.utc( this.model.getTimestamp() );
		diff = now.diff( momentTimestamp, 'weeks' );

This is what we use to decide whether to display a day name ("Today", "Yesteday", "Monday" ... ) or a full day. My hunch is that this is calculated wrong for utc difference (which would explain how this happens (a) only in the "stitch" between day names and dates, and (b) only in the first half of the day, but not after 3pm)
We need to make sure that the difference is calculated locally and not per utc, and definitely not mixed utc and local as it seems to be above.

I debugged this separately from you earlier today and came to a different conclusion: I think we're calculating it right but rounding differently.
If now is Tuesday at 11am, and ts is last Tuesday at 3pm, then ts-now is 0.976 weeks, which gets rounded down. However, last Tuesday is not considered part of lastweek by the code underlying .calendar(), and so it doesn't hit our custom locale code correctly.
It may be that you're right and I'm wrong, but let's test with this case (i.e. today a week ago's latest notification is later in the day than the present time is).

Actually, your sound a lot more plausible than mine, considering the point you and @Mattflaschen-WMF made about comparisons using timezones.

I am now wondering how to fix this, then. We're already customizing moment's locale preferences, but that's just for the translation keys... not for any roundings.

Change 302748 had a related patch set uploaded (by Mooeypoo):
Round timestamps to the day when displaying date titles in Special:Notifications

https://gerrit.wikimedia.org/r/302748

Change 302620 abandoned by Mooeypoo:
Convert to local time when calculating weeks difference in moment

Reason:
Fixed by this instead I5fcb78ac2fa168aa8bba

https://gerrit.wikimedia.org/r/302620

Change 302748 merged by jenkins-bot:
Round timestamps to the day when displaying date titles in Special:Notifications

https://gerrit.wikimedia.org/r/302748

Checked the fix in betalabs.

jmatazzoni closed this task as Resolved.Aug 19 2016, 12:05 AM