Timefilter should return strings instead of moments #25625
Conversation
lukasolson
added
review
Feature:Timepicker
|
Pinging @elastic/kibana-app |
💔 Build Failed |
|
Looks like legit test failures. |
|
@lukasolson I'd like this PR to include a test which would currently fail. I think it probably needs to be a UI functional test. It might be easy at the end of one of the Discover tests to go change the dateFormat:tz from UTC to some other timezone (maybe some place that doesn't use daylight savings time like Arizona?) and then go back to discover and check the timestamp of a particular doc while using an absolute time range. With this bug it would probably fail everywhere except when run in that timezone. So maybe we need to pick a timezone where none of our team lives for the test... |
💔 Build Failed |
|
Please merge master to get CI working |
💔 Build Failed |
|
So I spent some more time digging today and it looks like this was introduced in #21827. Prior to that pull, the |
💔 Build Failed |
|
retest |
💚 Build Succeeded |
|
I am still able to reproduce the bug. Seems to involve switching between quick and absolute time ranges:
|
| @@ -103,15 +103,15 @@ module.directive('kbnTimepicker', function (refreshIntervals) { | |||
| return; | |||
| } | |||
|
|
|||
| _.set($scope, 'browserAbsolute.from', new Date(newDate.year(), newDate.month(), newDate.date())); | |||
| if (moment.isMoment(date) && $scope.mode === TIME_MODES.ABSOLUTE) { | ||
| $scope.absolute.to = date; | ||
| if (date && $scope.mode === TIME_MODES.ABSOLUTE) { | ||
| $scope.absolute.to = moment.isMoment(date) ? date.toISOString() : date; |
There was a problem hiding this comment.
If date is not a moment object, we simply return it. Could date be a native javascript date? Seems we should ensure we're returning a string no matter what.
lukasolson
force-pushed
the
fix/timepicker-absolute
branch
from
6b18c3e
to
f7bc474
Compare
lukasolson
changed the title
|
@Bargs This is ready for another look |
|
@LeeDr I've added the functional tests. They test in multiple timezones so that it shouldn't pass/fail depending on which timezone you actually live in. |
💚 Build Succeeded |
💚 Build Succeeded |
* fix timefilter to return strings instead of moment objects * add functional test * remove unused functions
* fix timefilter to return strings instead of moment objects * add functional test * remove unused functions
Fixes #25596.
Prior to this PR, if you had selected a time zone in your advanced settings other than your browser, then there was a possibility that the date histogram would show dates in your browser's time zone rather than the time zone you selected.
Part of the reason behind this is that the timefilter is inconsistent with what it returns in
getTime. Sometimes it would return an object withfromas a UTC string, and other times it would return a moment object.There is some case that we're still debugging where the moment object gets stripped of its method properties, which in turns caused errors with the date histogram which attempts to call
moment(result)with the resulting object, which in turn barfs inside the moment code because it's expecting function properties to be on the object which aren't actually there.--- Edit: See this comment which explains when that happens. --
As a follow-up PR, we should probably go through all of the places where we conditionally handle moment objects and remove it, since they are already handling the case when strings are passed.