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.

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

Add new title query param to dashboard listing page #14760

Merged
merged 3 commits into from
Nov 9, 2017

Conversation

stacey-gammon
Copy link
Contributor

If a single dashboard with that exact title is found, it's automatically loaded up. If not, it'll load the listing table up with the search box prefilled.

autoloaddashbytitle

There is a little blip when loading the dashboard that first shows the listing page. I thought putting the logic in the resolve portion of the route would prevent the listing page from being loaded until that logic is complete. @spalger, do you know if there is an angular way to prevent that blip that I'm doing wrong?

@stacey-gammon
Copy link
Contributor Author

cc @epixa

@ctindel - I think this should solve your use cases? It also adds the filter parameter to populate the search query box, if you don't want to use the title option.

@ctindel
Copy link

ctindel commented Nov 3, 2017

@stacey-gammon I think this definitely does solve it! Awesome! Did you try search by title with spaces and weird characters and things that might be in a title in your tests?

Does it kill two birds with one stone and allow a query that pre-populates the dashboard filter as well?

@stacey-gammon
Copy link
Contributor Author

@stacey-gammon I think this definitely does solve it! Awesome! Did you try search by title with spaces and weird characters and things that might be in a title in your tests?

So far seems to handle everything I can throw at it:
my-dash
my%23dash
(hi)
hi!
hi %$ I have 67^&)(!@# special Chars!';

(though I use the encoded version of that title)

Does it kill two birds with one stone and allow a query that pre-populates the dashboard filter as well?

yep, with the new filter param. It's separate from the title param in case the user doesn't want the automatic redirection. Also the title param will add quotes around the title since it's looking for an exact match.

@stacey-gammon
Copy link
Contributor Author

hmmm failure looks unrelated:

17:21:43          │ debg  filter visualization (Visualization PieChart)
17:21:43          │ debg  findByCssSelector input[placeholder="Visualizations Filter..."]
17:21:43          │ debg  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/screenshots/failure/dashboard app dashboard state Directly modifying url updates dashboard state for panel size parameters.png"
17:21:43        └- ✖ fail: "dashboard app dashboard state Directly modifying url updates dashboard state for panel size parameters"
17:21:43        │        [POST http://localhost:9515/session/e8f3cfb3e1f3cbfb52cc0e89d0303bdd/element/0.6442045786004662-100/click] unknown error: Element <input class="kuiSearchInput__input ng-pristine ng-untouched ng-valid ng-empty" input-focus="" disable-input-focus="disableAutoFocus" ng-model="filter" ng-attr-placeholder="{{ finder.properties.nouns | label }} Filter..." ng-keydown="finder.filterKeyDown($event)" name="filter" type="text" autocomplete="off" data-test-subj="savedObjectFinderSearchInput" placeholder="Visualizations Filter..."> is not clickable at point (544, 194). Other element would receive the click: <button class="paginate-heading list-group-item list-sort-button" ng-click="finder.sortHits(finder.hits)" aria-live="assertive">...</button>
17:21:43        │         (Session info: chrome=60.0.3112.101)
17:21:43        │         (Driver info: chromedriver=2.32.498513 (2c63aa53b2c658de596ed550eb5267ec5967b351),platform=Linux 3.13.0-125-generic x86_64)
17:21:43        │         at Server._post (test/functional/services/remote/verbose_remote_logging.js:15:21)
17:21:43        │         at runRequest (node_modules/leadfoot/Session.js:92:40)
17:21:43        │         at getFinalValue (node_modules/dojo/Promise.js:258:35)
17:21:43        │         at node_modules/dojo/Promise.js:272:24
17:21:43        │         at node_modules/dojo/Promise.js:156:41
17:21:43        │         at runCallbacks (node_modules/dojo/Promise.js:19:22)
17:21:43        │         at node_modules/dojo/Promise.js:103:21
17:21:43        │         at run (node_modules/dojo/Promise.js:51:33)
17:21:43        │         at node_modules/dojo/nextTick.js:35:17
17:21:43        │         at _combinedTickCallback (internal/process/next_tick.js:73:7)
17:21:43        │         at process._tickDomainCallback (internal/process/next_tick.js:128:9)
17:21:43        │       
17:21:43        │       
17:21:43      └-> "after all" hook
17:21:43        │ debg  gotoDashboardLandingPage
17:21:43        │ debg  onDashboardLandingPage
17:21:43        │ debg  TestSubjects.exists(dashboardLandingPage

screenshot:
dashboard app dashboard state directly modifying url updates dashboard state for panel size parameters 2

jenkins, test this

kbnUrl.redirect(createDashboardEditUrl(matchingDashboards[0].id));
} else {
kbnUrl.redirect(`${DashboardConstants.LANDING_PAGE_PATH}?filter="${title}"`);
}
Copy link
Contributor

@spalger spalger Nov 7, 2017

Choose a reason for hiding this comment

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

If you throw uiRoutes.WAIT_FOR_URL_CHANGE_TOKEN after calling kbnUrl.redirect() then the router will wait for the url change to take effect (might take a tick or two) rather than rendering the route.

type: 'dashboard',
}).then(results => {
// The search isn't an exact match, lets see if we can find a single exact match to use
const matchingDashboards = results.savedObjects.filter(dashboard => dashboard.attributes.title === title);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be case-sensitive? We're prompting the user for duplicate names on save even if they have different cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my gut feel is that it shouldn't be case sensitive, because I think it'd be a little weird to require them to write in the correct case in order to load the dashboard. That's just me though, I'd probably guess not case sensitive if I was wondering how it worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

being case-insensitive feels right to me as well

@kobelb
Copy link
Contributor

kobelb commented Nov 7, 2017

This is great, just one question but besides that everything appears to be working here! Also, should we add functional tests for this?

@alexfrancoeur
Copy link

This should resolve #11338 and #12404

@stacey-gammon stacey-gammon force-pushed the feature/search-by-title branch from d1439a5 to c33e31e Compare November 9, 2017 12:54
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm

@stacey-gammon stacey-gammon merged commit 6d49728 into elastic:master Nov 9, 2017
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Nov 9, 2017
* introduce a mechanism to load a dashboard by title is a single one is found, if not, to refill the search box on the listing page

* Make case insensitive and prevent listing page from "blipping" up on the screen before the redirect

* Add tests
stacey-gammon added a commit that referenced this pull request Nov 9, 2017
* introduce a mechanism to load a dashboard by title is a single one is found, if not, to refill the search box on the listing page

* Make case insensitive and prevent listing page from "blipping" up on the screen before the redirect

* Add tests
@stacey-gammon stacey-gammon deleted the feature/search-by-title branch November 9, 2017 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants