Dashboard: Load a default rather than starting new - Against master - Closing #5820 #7626

LuudJanssen · 2016-07-05T14:14:36Z

This is a pull request closing #5820 against master. I wasn't able to run the tests locally. Could someone test it for me? It implements the default dashboard setup in the config options in the dashboard:

It does not use the suggestion from @dotfold to use a 'star' icon in the toolbar to set the dashboard as default, but this can be easily implemented since the logic is already there.

I'm doing this pull request against 4.5 since that is the version we're running at our company, but I might do a pull request against master as well (or someone else can rewrite / merge it, if they have the time).

Conflicts: src/plugins/kibana/public/dashboard/index.js src/ui/public/config/defaults.js

…encies Conflicts: src/plugins/kibana/public/dashboard/index.js

elasticsearch-release · 2016-07-05T14:14:38Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

elasticmachine · 2016-07-05T14:14:38Z

Can one of the admins verify this patch?

LuudJanssen · 2016-07-05T14:15:23Z

@epixa This is the same pull request as #7625, but against master.

epixa · 2016-07-05T19:08:03Z

Sorry @LuudJanssen, we just had a change that got merged to master that fixed our CI builds. Can you rebase?

Conflicts: src/core_plugins/kibana/public/dashboard/index.js

LuudJanssen · 2016-07-06T11:12:08Z

@epixa I rebased again and fixed some minor bugs! :)

epixa · 2016-07-06T23:28:04Z

jenkins, test this

epixa · 2016-08-19T14:50:33Z

src/core_plugins/kibana/public/dashboard/index.js

+    },
+    resolve: {
+      dash: function (savedDashboards, config, kbnUrl, Notifier, $location, $route) {
+        let defaultDashboard = config.get('dashboard:defaultDashboard', '');


This should be const since the reference is never mutated.

Why bother with the empty string fallback?

epixa · 2016-08-19T15:03:23Z

@LuudJanssen I did a preliminary code review and added some inline comments.

In general, I don't really like how this muddies the water between new and existing dashboards, so whenever you want to make a decision about templates and such, you need to also do a check on the new property.

What do you think about this instead:

/dashboard - sole responsibility is checking whether we have a default dashboard and sending the user to where they need to go as a result (using .replace() as to ensure this doesn't remain in browser history)
/dashboard/new - always used for creating a new dashboard
/dashboard/:id - always used for loading an existing dashboard

elasticmachine · 2016-08-19T15:03:25Z

Can one of the admins verify this patch?

LuudJanssen · 2016-08-23T22:23:28Z

Hey @epixa,

Thanks for your comments. I'll take a look at the inline comments and fix them. For your comment on the routes, I initially had this approach in #5851 but @spalger commented saying:

I would prefer that we didn't have a route for /dashboard/new. I think we should simply be redirecting requests from /dashboard to /dashboard/:id if a default dashboard is defined.

I think we should instead use a query string to tell the route handler for /dashboard that we are not interested in redirecting but definitely want a blank dashboard, something like ?create=true or ?default=false.

Either way is fine by me, but since you're on the core Kibana team you'll have to decide, haha! :)

spalger · 2016-08-23T22:50:18Z

I just don't like the ambiguity of /dashboard/new and /dashboard/:id

LuudJanssen · 2016-08-24T09:51:43Z

Spencer, thanks for your quick response.

The ideal situation as I see it is:

/dashboard links to:

/dashboard/:id when a saved dashboard is available. If not it'll link to the default dashboard.
/dashboard/new when neither a saved dashboard nor a default dashboard is available.

This means that the user will always be redirected when going to /dashboard.

@epixa & @spalger Is this something you both agree on?

I remember having trouble doing redirecting, without triggering any errors. What is the best way to implement redirecting?

spalger · 2016-08-24T10:24:26Z

I suppose that works for me! We just need to make sure people can't save dashboards with the name "new" :) What should we do if someone already has a dashboard with the name "new"?

Re redirecting: I suggest doing it in a route resolve function and using the kbnUrl service (which will take care of preventing the route from continuing to load before the redirect processes)

LuudJanssen · 2016-08-24T10:41:03Z

@spalger That's a good point! The solution I came up with is using a / in the 'new url'. So, for example, we use /dashboard/new/empty as the url for creating a new dashboard. This also useful for when you want to build in support for creating a new dashboard based on a template. Its url could be /dasbhoard/new/template/:id for example.

It's not the prettiest solution, but it is a solution. Any other ideas are welcome.

EDIT: FYI, when creating a dashboard called new/empty the ID becomes new-slash-empty.

spalger · 2016-08-24T17:49:24Z

That's a great idea, I support doing /dashboard/:id and /dashboard/new/:templateid (where we just have a single template to start, named blank or empty or something)

epixa · 2016-10-08T15:21:03Z

@LuudJanssen Do you still plan to work on this, or should I throw an "adoptme" label on this in case someone else has time to pull it over the line?

epixa · 2016-12-21T19:25:10Z

I'm going to close this out due to inactivity. Please feel free to resubmit if you pick this back up again!

`v93.5.1` ⏩ `v93.5.2` --- ## [`v93.5.2`](https://github.com/elastic/eui/releases/v93.5.2) **Dependency updates** - Updated `react-virtualized-auto-sizer` to 1.0.24 ([#7598](elastic/eui#7598)) - Updated `react-window` to 1.8.10 ([#7600](elastic/eui#7600)) **CSS-in-JS conversions** - Updated EUI's internal style memoization/performance utility to have configurable error/warning levels via `setEuiDevProviderWarning` ([#7626](elastic/eui#7626))

Luud Janssen added 5 commits July 5, 2016 15:36

Add default dashboard option

be1e99a

Conflicts: src/plugins/kibana/public/dashboard/index.js src/ui/public/config/defaults.js

Add default dashboard option

1befdb7

Cherry pick 'default dashboard' commits

2233cba

Cherry pick 'default dashboard' commits and remove jQuery from depend…

a394714

…encies Conflicts: src/plugins/kibana/public/dashboard/index.js

Rebase 'default dashboard' against master

cd2851f

epixa mentioned this pull request Jul 5, 2016

Dashboard: Load a default rather than starting new - Closing #5820 #7625

Closed

Luud Janssen added 2 commits July 6, 2016 12:21

Rebase default dashboards against master

4409590

Conflicts: src/core_plugins/kibana/public/dashboard/index.js

Rebase 'default dashboard' against master

79e92ab

epixa added the review label Jul 6, 2016

epixa removed the review label Jul 6, 2016

epixa added the review label Jul 22, 2016

cjcenizal assigned epixa Aug 5, 2016

epixa reviewed Aug 19, 2016
View reviewed changes

epixa added updates_needed and removed review labels Aug 19, 2016

epixa added the help wanted adoptme label Oct 29, 2016

epixa removed their assignment Oct 29, 2016

tbragin added Feature:Dashboard Dashboard related features :Sharing labels Nov 12, 2016

epixa closed this Dec 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard: Load a default rather than starting new - Against master - Closing #5820 #7626

Dashboard: Load a default rather than starting new - Against master - Closing #5820 #7626

LuudJanssen commented Jul 5, 2016 •

edited

Loading

elasticsearch-release commented Jul 5, 2016

elasticmachine commented Jul 5, 2016

LuudJanssen commented Jul 5, 2016

epixa commented Jul 5, 2016

LuudJanssen commented Jul 6, 2016

epixa commented Jul 6, 2016

epixa Aug 19, 2016

epixa Aug 19, 2016

epixa commented Aug 19, 2016

elasticmachine commented Aug 19, 2016

LuudJanssen commented Aug 23, 2016

spalger commented Aug 23, 2016

LuudJanssen commented Aug 24, 2016

spalger commented Aug 24, 2016

LuudJanssen commented Aug 24, 2016 •

edited

Loading

spalger commented Aug 24, 2016

epixa commented Oct 8, 2016

epixa commented Dec 21, 2016

Dashboard: Load a default rather than starting new - Against master - Closing #5820 #7626

Dashboard: Load a default rather than starting new - Against master - Closing #5820 #7626

Conversation

LuudJanssen commented Jul 5, 2016 • edited Loading

elasticsearch-release commented Jul 5, 2016

elasticmachine commented Jul 5, 2016

LuudJanssen commented Jul 5, 2016

epixa commented Jul 5, 2016

LuudJanssen commented Jul 6, 2016

epixa commented Jul 6, 2016

epixa Aug 19, 2016

Choose a reason for hiding this comment

epixa Aug 19, 2016

Choose a reason for hiding this comment

epixa commented Aug 19, 2016

elasticmachine commented Aug 19, 2016

LuudJanssen commented Aug 23, 2016

spalger commented Aug 23, 2016

LuudJanssen commented Aug 24, 2016

spalger commented Aug 24, 2016

LuudJanssen commented Aug 24, 2016 • edited Loading

spalger commented Aug 24, 2016

epixa commented Oct 8, 2016

epixa commented Dec 21, 2016

LuudJanssen commented Jul 5, 2016 •

edited

Loading

LuudJanssen commented Aug 24, 2016 •

edited

Loading