Add option to save time with dashboard #3164
Conversation
|
In the settings screen, the |
|
|
||
| // I don't like this, but not sure how else to tell if this is a fresh load of the | ||
| // saved dashboard object, so looking for _a in the URL. Ideas? | ||
| if (dash.timeRestore && dash.timeTo && dash.timeFrom && !$routeParams._a) { |
There was a problem hiding this comment.
As you pointed out, checking for _a is a little dangerous - it's makes assumptions about how AppState works. It's unlikely to cause problems as it's unlikely to change, but it's still not a good idea.
To me, it seems like you want to offload this to the AppState service somehow though. My immediate thought is keeping a reference to what the AppState in the URL was before defaults were injected, and making that "original" state available from the service, but I'm not convinced that's the answer.
|
Fixed the _a thing by adding a method called As for timeRestore not showing up for existing dashboards in the saved object editor. Its probably not a huge deal. Makes me wonder if we should offer either a pure JSON editor for saved objects, or a way to add new fields to the root of a saved object. |
| var currentAppState; | ||
|
|
||
| function get() { | ||
| return currentAppState; | ||
| } | ||
|
|
||
| // Checks to see if the appState might already exist, even if it hasn't been newed up | ||
| get.mightExist = function () { |
There was a problem hiding this comment.
Can you rename this to exists, or isLoaded or something? The might seems ambiguous, and I kind of expect it to take some action instead of just returning a bool.
There was a problem hiding this comment.
couldExist? I'm trying to convey that a non-default app state could or will exist if you chose to instantiate one.
|
There's still a strange disconnect in the saved object editor, and I think a bug in that interface as well when dealing with dashboards that don't have a saved time. Hopefully I can explain this well enough... Old DashboardHere's a dashboard I created before checking this PR (this is every dashboard that exists in the wild already). I have no way of enabling saved time - maybe this is intention? If not, it's a bug.
New Dashboard, Time Not SavedI created a new dashboard with the PR, but did not save the time. I then went ahead and checked the save time box, but it doesn't work, since I can't enter the desired time settings. This seems like a bug.
New Dashboard, Time SavedThis one I saved with the time, and now I get the fields I expected, and that I need to make changes.
And when I uncheck the box, I still have the fields I need to re-enable it.
Even when I uncheck the box and clear the fields, I still have the means to restore this. This is all expected and I think how the "Time Not Saved" example should have worked.
|
…d object instead of client. Add defaults to savedObject
|
|
||
| // I don't like this, but not sure how else to tell if this is a fresh load of the | ||
| // saved dashboard object, so looking for _a in the URL. Ideas? | ||
| if (dash.timeRestore && dash.timeTo && dash.timeFrom && !getAppState.mightExist()) { |
There was a problem hiding this comment.
Should be getAppState.wouldExist - or whatever it ends up getting named
| version: 1 | ||
| version: 1, | ||
| timeRestore: false, | ||
| timeTo: moment().toISOString(), |
There was a problem hiding this comment.
I'm not sure we want the defaults to be real timestamps...
|
As for the timestamp default stuff, this is what the fields look like on a dashboard that was saved without the time.
This will probably never be what the user wants to see. More likely, it'll be As-is, this is how the dashboard loads when you just check that box:
|
|
What do you think about renaming |
|
Ok, so I undefined timeTo and timeFrom, for exactly the reason you point out. I originally put them there to give people the hint that they should be ISO8601. Then I remembered they can also be datemath. Long term it would be nice to have a date input here, but we should remember we consider this to be an "advanced" screen and thats way outside the scope of this ticket :-D |
| @@ -57,6 +57,14 @@ define(function (require) { | |||
| }); | |||
|
|
|||
| var dash = $scope.dash = $route.current.locals.dash; | |||
|
|
|||
| // I don't like this, but not sure how else to tell if this is a fresh load of the | |||
| // saved dashboard object, so looking for _a in the URL. Ideas? | |||
There was a problem hiding this comment.
Now that this is officially a method, the comment should probably be removed
|
One last, small thing. Once you remove that comment, go ahead and merge, this LGTM! 🚜 |
Add option to save time with dashboard
Closes #2473
This pull implements the ability to link a time filter to a saved dashboard, and have it restored when the dashboard is loaded.
It will only restore the time if on of the following is true:
The time filter will be changed upon the loading of the dashboard, it will not be somehow changed back if the user leaves the dashboard. Not sure if this is the behavior we want, but since time is global, its the behavior that exists currently.
Also, as I was typing this up I wondered if this should be an option for all saved objects. However, things get a lot more complicated if we do that. If we enabled saving time with say, a visualization, it would be expected that the time would stick with that visualization even if was added to a dashboard, which would break the whole global time filter thing.
Side effects of implementation