Use `forceNow` query parameter when parsing timefilter #16236

kobelb · 2018-01-23T20:09:15Z

Reporting needs a way to emulate a different Date for now when parsing relative dates. This adds a forceNow query parameter that can be used for this purpose.

Were also adding data-shared-timefilter-from and data-shared-timefilter-to to expose the effective global time filters.

…support

nreese

lgtm

w33ble · 2018-02-05T17:55:45Z

src/ui/public/timefilter/timefilter.js

@@ -105,10 +106,21 @@ uiModules
      return this.calculateBounds(this.time);
    };

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


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

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.

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?

Yup, that's the intent right now.

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.

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

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.

w33ble · 2018-02-05T18:16:12Z

src/ui/public/timepicker/__tests__/toggle.js

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


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?

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

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

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.

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

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

w33ble · 2018-02-05T18:18:49Z

@kobelb this also doesn't seem to ever test the values returned when the forceNow argument is true. Unless I'm missing something...

kobelb · 2018-02-05T18:59:32Z

@kobelb this also doesn't seem to ever test the values returned when the forceNow argument is true. Unless I'm missing something...

Would you mind elaborating, I'm not following?

w33ble · 2018-02-05T20:29:01Z

Would you mind elaborating, I'm not following?

Sorry, for some reason I was thinking forceNow was a boolean. That's not right at all.

You have 3 tests for <kbn-global-timepicker>, which all look fine. But in addition to changing the arguments for that directive in this PR, you've added checking for forceNow in timefilter.js, but I don't see any tests for the from/to dates that are provided when a forceNow value is provided.

kobelb · 2018-02-05T21:02:02Z

Yeah... at the time my reasoning was that the supplier of forceNow via Reporting's regression tests would be sufficient, but in retrospect, I was being lazy. I'll go back and add some unit tests for that new behavior.

elasticmachine · 2018-02-06T15:07:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…support

elasticmachine · 2018-02-07T20:12:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Using "forceNow" parameter to adjust "now" and exposing timeRange * Moving forceNow parameter to the hash, adding tests * Renaming test

bhavyarm · 2018-05-23T21:32:34Z

How do I test this PR @kobelb ? Please & Thank you :)

kobelb · 2018-05-24T11:41:59Z

@bhavyarm the only thing that utilizes this right now is Reporting, so I'd recommend testing that Reporting works correctly when there's a delay between the Reporting job being created and when it's actually run.

To do this, I created a Metric Visualization that displayed the max timestamp and a Metric Visualization that displayed the minimum timestamp, and I added them both to a Dashboard. I then clicked the "Generate PDF" button to create the Reporting job, and then immediately stopped the Kibana server. I wanted about 5 minutes, started the Kibana server back up and made sure that the timestamps in the PDF were relative to the time the job was created, as opposed to the current time.

bhavyarm · 2018-05-24T14:34:51Z

@kobelb thank you!!!!

kobelb added 2 commits January 18, 2018 08:16

Using "forceNow" parameter to adjust "now" and exposing timeRange

28fe61b

Merge remote-tracking branch 'upstream/master' into reporting-canvas-…

fb39267

…support

kobelb added review :Sharing v7.0.0 v6.3.0 labels Jan 23, 2018

kobelb requested review from nreese and w33ble January 23, 2018 20:09

nreese approved these changes Jan 23, 2018

View reviewed changes

w33ble suggested changes Feb 5, 2018

View reviewed changes

kobelb added 2 commits February 6, 2018 09:16

Moving forceNow parameter to the hash, adding tests

15decc0

Renaming test

554fc7d

w33ble approved these changes Feb 6, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into reporting-canvas-…

1ecb16d

…support

kobelb merged commit b677848 into elastic:master Feb 13, 2018

kobelb deleted the reporting-canvas-support branch February 13, 2018 17:09

kobelb mentioned this pull request Feb 13, 2018

[6.x] Use forceNow query parameter when parsing timefilter (#16236) #16704

Merged

kobelb added a commit that referenced this pull request Feb 13, 2018

Use forceNow query parameter when parsing timefilter (#16236) (#16704)

b1a6ef0

* Using "forceNow" parameter to adjust "now" and exposing timeRange * Moving forceNow parameter to the hash, adding tests * Renaming test

Bargs mentioned this pull request Mar 22, 2018

Avoid mutating global moment object #17341

Merged

kibanamachine mentioned this pull request Sep 14, 2018

Routing: Handle search params already in the URL #23046

Closed

This was referenced Oct 6, 2021

[Reporting] V2 PDF and PNG reports have time drift when time filter uses relative time range. #114193

Closed

[Reporting] remove forceNow hack #110568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `forceNow` query parameter when parsing timefilter #16236

Use `forceNow` query parameter when parsing timefilter #16236

kobelb commented Jan 23, 2018

nreese left a comment

w33ble Feb 5, 2018

kobelb Feb 5, 2018

w33ble Feb 5, 2018

kobelb Feb 5, 2018

w33ble Feb 5, 2018

kobelb Feb 6, 2018

w33ble Feb 6, 2018

w33ble Feb 5, 2018

kobelb Feb 5, 2018

w33ble Feb 5, 2018

kobelb Feb 5, 2018

w33ble Feb 5, 2018

w33ble commented Feb 5, 2018

kobelb commented Feb 5, 2018

w33ble commented Feb 5, 2018

kobelb commented Feb 5, 2018

elasticmachine commented Feb 6, 2018

elasticmachine commented Feb 7, 2018

bhavyarm commented May 23, 2018

kobelb commented May 24, 2018

bhavyarm commented May 24, 2018

Use forceNow query parameter when parsing timefilter #16236

Use forceNow query parameter when parsing timefilter #16236

Conversation

kobelb commented Jan 23, 2018

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble commented Feb 5, 2018

kobelb commented Feb 5, 2018

w33ble commented Feb 5, 2018

kobelb commented Feb 5, 2018

elasticmachine commented Feb 6, 2018

💚 Build Succeeded

elasticmachine commented Feb 7, 2018

💚 Build Succeeded

bhavyarm commented May 23, 2018

kobelb commented May 24, 2018

bhavyarm commented May 24, 2018

Use `forceNow` query parameter when parsing timefilter #16236

Use `forceNow` query parameter when parsing timefilter #16236