981559 - Autophone - handle daylight saving time transition

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: dst-transition.patch

bug 949017 comment 10 was right and I should have followed through with bug 949017 comment 11.

I've already applied this to phonedash.mozilla.org, but not phonedash-dev.allimoz.org. You can see the problem on phonedash-dev and that it works on phonedash.

Comment 1

[Mark Côté [:mcote]]

Comment on attachment 8388446 [details] [diff] [review]
dst-transition.patch

Review of attachment 8388446 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still not sure I totally understand this, but if it works...

::: html/scripts/app.js
@@ +189,5 @@
>    $.makeArray($('#controls select').each(function(i, e) { params[e.name] = e.value; }));
> +  // Create a MountainView date for the startdate and enddate to get their
> +  // timezone offsets in minutes. Create the timezone string for the
> +  // MountainView date to force input to be parsed as MountainView.
> +  startdatestr = $('#startdate').attr('value')

Missing a semicolon.

@@ +193,5 @@
> +  startdatestr = $('#startdate').attr('value')
> +  tzoffset = makeMountainViewDate(startdatestr).getTimezoneOffset();
> +  tzhours = Math.floor(Math.abs(tzoffset)/60);
> +  tzminutes = Math.abs(tzoffset) - 60*tzhours;
> +  tzstring = 'T00:00:00' + (tzoffset < 0 ? '+' : '-') + pad(tzhours) + ':' + pad(tzminutes);

Just saw this: you're using a + if tzoffset is negative...?  Maybe I'm still not understanding exactly what this code is doing...

@@ +196,5 @@
> +  tzminutes = Math.abs(tzoffset) - 60*tzhours;
> +  tzstring = 'T00:00:00' + (tzoffset < 0 ? '+' : '-') + pad(tzhours) + ':' + pad(tzminutes);
> +  startdatestr = ISODateString(new Date(startdatestr + tzstring));
> +
> +  enddatestr = $('#enddate').attr('value')

Missing semicolon.  Yay cutting & pasting. ;)

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: dst-transition-v2.patch

The whole testing of positive/negative is unnecessary. Is this clearer?

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 8388879 [details] [diff] [review]
dst-transition-v2.patch

Review of attachment 8388879 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, although I would have created a function as I was hinting in my cutting & pasting remark... ;)

I think the comment could be clarified too.  You describe what you're doing but not really why--*why* do we force it to be parsed as Mountain View time, and for that matter, what does that mean exactly?  That we get a UTC-looking time that is actually in local MV time?

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: dst-transition.patch v3

The entered dates are already in MVT so there is not need to do anything.

Comment 5

[Bob Clary [:bc] (inactive)]

https://github.com/markrcote/phonedash/commit/1741bee46e1e09d5af5faaa830746c119cb2c545