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.

Sign up for GitHub

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

Use forceNow query parameter when parsing timefilter #16236

Merged
merged 5 commits into from
Feb 13, 2018
Merged

Use forceNow query parameter when parsing timefilter #16236

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28fe61b
Using "forceNow" parameter to adjust "now" and exposing timeRange
kobelb Jan 18, 2018
fb39267
Merge remote-tracking branch 'upstream/master' into reporting-canvas-…
kobelb Jan 23, 2018
15decc0
Moving forceNow parameter to the hash, adding tests
kobelb Feb 6, 2018
554fc7d
Renaming test
kobelb Feb 6, 2018
1ecb16d
Merge remote-tracking branch 'upstream/master' into reporting-canvas-…
kobelb Feb 7, 2018
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
16 changes: 14 additions & 2 deletions src/ui/public/timefilter/timefilter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import querystring from 'querystring';
import _ from 'lodash';
import moment from 'moment';
import dateMath from '@elastic/datemath';
Expand Down Expand Up @@ -105,10 +106,21 @@ uiModules
return this.calculateBounds(this.time);
};

Timefilter.prototype.getForceNow = function () {
const query = querystring.parse(window.location.search.replace(/^\?/, '')).forceNow;
Copy link
Contributor

Choose a reason for hiding this comment

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

Being Angular code and all, should this use $location.search() instead? And actually, isn't that required since we're using hash routing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want this to be a normal querystring parameter outside of the AngularJS based hash routing, because I assumed that Canvas itself might not be using the hash based routing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, fair enough. We are using hash routing though, and actually I think everything would need to. Though, I suppose an app could use search param routing...

So your intention is that you'd have something like http://localhost:5601/acd/app/canvas?forceNow=2001-01-01T00:00:00Z#/workpad/workpad-1ba0f600-47f0-4306-a587-17cc95a74281?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's the intent right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if an app needs the forceNow parameter to be part of the hash route?

I think that's actually going to be the case for Canvas. Our router is going to assume that the entirety of the parameters it should be looking at come after the hash. I can work around it, but I wonder if the location of this setting can be left up to the app creating the URL somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allowing the application to decide where to look for the parameter in the URL is going against the intent of this change entirely. The goal is to make the applications abide by a standard "interface" and then they'll work with Reporting, changing where the forceNow is for different applications is the exact thing that we're trying to avoid by introducing application specific logic.

With that being said, if we want to move the forceNow to the hash parameters, we can, as we can support this within Kibana. There's been discussion about moving to non hash-based routing, but we can readdress this when this happens

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. I'll add an issue in Canvas to address search params that could be in the URL already and we can push forward that way.

if (!query) {
return;
}

return new Date(Date.parse(query));
};

Timefilter.prototype.calculateBounds = function (timeRange) {
const forceNow = this.getForceNow();

return {
min: dateMath.parse(timeRange.from),
max: dateMath.parse(timeRange.to, { roundUp: true })
min: dateMath.parse(timeRange.from, { forceNow }),
max: dateMath.parse(timeRange.to, { roundUp: true, forceNow })
};
};

Expand Down
51 changes: 42 additions & 9 deletions src/ui/public/timepicker/__tests__/toggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import moment from 'moment';
import expect from 'expect.js';
import ngMock from 'ng_mock';
import $ from 'jquery';
Expand All @@ -24,28 +25,60 @@ describe('kbnGlobalTimepicker', function () {
expect($el.attr('data-test-subj')).to.be('globalTimepicker');
});

it('sets data-shared-timefilter to true when auto-refresh selector is enabled', function () {
it('sets data-shared-timefilter-* using the timefilter when auto-refresh selector is enabled', function () {
const minString = '2000-01-01T00:00:00Z';
const maxString = '2001-01-01T00:00:00Z';
const bounds = {
min: moment(minString),
max: moment(maxString),
};
scope.timefilter = {
isAutoRefreshSelectorEnabled: true,
isTimeRangeSelectorEnabled: false
isTimeRangeSelectorEnabled: false,
getBounds: () => bounds
};

const $el = compile();
expect($el.attr('data-shared-timefilter')).to.eql('true');

expect($el.attr('data-shared-timefilter-from')).to.eql(minString);
expect($el.attr('data-shared-timefilter-to')).to.eql(maxString);
});
it('sets data-shared-timefilter to true when time range selector is enabled', function () {

it('sets data-shared-timefilter-* using the timefilter when time range selector is enabled', function () {
const minString = '2000-01-01T00:00:00Z';
const maxString = '2001-01-01T00:00:00Z';
const bounds = {
min: moment(minString),
max: moment(maxString),
};
scope.timefilter = {
isAutoRefreshSelectorEnabled: false,
isTimeRangeSelectorEnabled: true
isTimeRangeSelectorEnabled: true,
getBounds: () => bounds
};

const $el = compile();
expect($el.attr('data-shared-timefilter')).to.eql('true');

expect($el.attr('data-shared-timefilter-from')).to.eql(minString);
expect($el.attr('data-shared-timefilter-to')).to.eql(maxString);
});
it(`sets data-shared-timefilter to false when auto-refresh and time range selectors are both disabled`, function () {

it(`sets data-shared-timefilter-* to null when auto-refresh and time range selectors are both disabled`, function () {
const minString = '2000-01-01T00:00:00Z';
const maxString = '2001-01-01T00:00:00Z';
const bounds = {
min: moment(minString),
max: moment(maxString),
};
scope.timefilter = {
isAutoRefreshSelectorEnabled: false,
isTimeRangeSelectorEnabled: false
isTimeRangeSelectorEnabled: false,
getBounds: () => bounds
};

const $el = compile();
expect($el.attr('data-shared-timefilter')).to.eql('false');

expect($el.attr('data-shared-timefilter-from')).to.eql('');
Copy link
Contributor

Choose a reason for hiding this comment

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

Contrary to the test description, this isn't testing for null. The code that uses this is checking for a truthy value... maybe the test should do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When there's an attribute added without a value, like this:

screen shot 2018-02-05 at 1 54 31 pm

the DOM APIs are returning a value of '', I'm open to rewording recommendations to make this more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

The value is fine, but the test says "sets data-shared-timefilter-* to null", but it's not null, and it's not testing for null. My only beef here is the description of what is being tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does doesn't set data-shared-timefilter-* using the timefilter when auto-refresh selector is enabled seem any more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. My only issue was the mention of null, since null is never the value and was not actually be tested for.

expect($el.attr('data-shared-timefilter-to')).to.eql('');
});
});
8 changes: 7 additions & 1 deletion src/ui/public/timepicker/kbn_global_timepicker.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
<div ng-show="timefilter.isAutoRefreshSelectorEnabled || timefilter.isTimeRangeSelectorEnabled" data-shared-timefilter="{{timefilter.isAutoRefreshSelectorEnabled || timefilter.isTimeRangeSelectorEnabled}}" class="kuiLocalMenu" data-test-subj="globalTimepicker">
<div
ng-show="timefilter.isAutoRefreshSelectorEnabled || timefilter.isTimeRangeSelectorEnabled"
data-shared-timefilter-from="{{timefilter.isAutoRefreshSelectorEnabled || timefilter.isTimeRangeSelectorEnabled ? timefilter.getBounds().min.utc().format() : null }}"
data-shared-timefilter-to="{{timefilter.isAutoRefreshSelectorEnabled || timefilter.isTimeRangeSelectorEnabled ? timefilter.getBounds().max.utc().format() : null }}"
class="kuiLocalMenu"
data-test-subj="globalTimepicker"
>
<button
class="kuiLocalMenuItem"
aria-label="{{ timefilter.refreshInterval.pause ? 'Resume refreshing data' : 'Pause refreshing data' }}"
Expand Down