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

Jump to bottom

[tests/functional] opt out of getting started on navigation #11881

Merged
merged 10 commits into from
May 19, 2017
Merged

[tests/functional] opt out of getting started on navigation #11881

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
263b2b6
[tests/functional] automatically opt-out of getting started page on n…
spalger May 17, 2017
1b77840
Merge branch 'master' of github.com:elastic/kibana into fix/opt-out-o…
spalger May 17, 2017
207a00c
[test/functional/commonPage] avoid circular reference
spalger May 17, 2017
9561233
[test/functional/commonPage] move check for getting started after url…
spalger May 18, 2017
a890065
[gettingStarted] prevent route resolution by returning halt promises
spalger May 18, 2017
01fef54
[test/functional/commonPage] wait for kibana to load before checking …
spalger May 18, 2017
f88c940
[uiRoutes/setupWork] provide a token that can be thrown to halt setup…
spalger May 18, 2017
b82612a
[ui/routes] rename reference to WAIT_FOR_URL_CHANGE_TOKEN
spalger May 18, 2017
abf58a7
Merge branch 'master' of github.com:elastic/kibana into fix/opt-out-o…
spalger May 18, 2017
274ba24
address review feedback
spalger May 18, 2017
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
10 changes: 4 additions & 6 deletions src/core_plugins/getting_started/public/lib/add_setup_work.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { get } from 'lodash';
import uiRoutes from 'ui/routes';
import uiRoutes, { WAIT_FOR_URL_CHANGE_TOKEN } from 'ui/routes';
import uiChrome from 'ui/chrome';
import KbnUrlProvider from 'ui/url';
import { Notifier } from 'ui/notify/notifier';
import { IndexPatternsGetIdsProvider } from 'ui/index_patterns/_get_ids';
import { hasOptedOutOfGettingStarted, optOutOfGettingStarted } from 'ui/getting_started/opt_out_helpers';
Expand All @@ -14,10 +13,9 @@ import {
uiRoutes
.addSetupWork(function gettingStartedGateCheck(Private, $injector) {
const getIds = Private(IndexPatternsGetIdsProvider);
const kbnUrl = Private(KbnUrlProvider);
const config = $injector.get('config');
const kbnUrl = $injector.get('kbnUrl');
const $route = $injector.get('$route');
const Promise = $injector.get('Promise');

const currentRoute = get($route, 'current.$$route');

Expand Down Expand Up @@ -69,14 +67,14 @@ uiRoutes
});
notify.error('Please create a new index pattern');
kbnUrl.change(CREATE_INDEX_PATTERN_ROUTE);
return Promise.halt(); //prevent route resolve
throw WAIT_FOR_URL_CHANGE_TOKEN;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ycombinator I tried a different approach, throwing this token from within a route setup work function will halt the setup work, allowing it to wait for kbnUrl to change. What do you think?

/cc @cjcenizal

Copy link
Contributor

Choose a reason for hiding this comment

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

@spalger Do you think this throw should just become part of the kbnUrl.change function? It seems to me the two would always want to travel together, no?

}

// Redirect the user to the Getting Started page (unless they are on it already)
if (!isOnGettingStartedPage) {
uiChrome.setVisible(false);
kbnUrl.change(GETTING_STARTED_ROUTE);
return Promise.halt(); //prevent route resolve
throw WAIT_FOR_URL_CHANGE_TOKEN;
}
});
});
2 changes: 1 addition & 1 deletion src/ui/public/routes/route_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _ from 'lodash';

import { wrapRouteWithPrep } from './wrap_route_with_prep';
import RouteSetupManager from './route_setup_manager';
import { RouteSetupManager } from './route_setup_manager';

function RouteManager() {
const self = this;
Expand Down
15 changes: 13 additions & 2 deletions src/ui/public/routes/route_setup_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import _ from 'lodash';

module.exports = class RouteSetupManager {
export const WAIT_FOR_URL_CHANGE_TOKEN = new Error('WAIT_FOR_URL_CHANGE_TOKEN');
Copy link
Contributor

Choose a reason for hiding this comment

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

If I were coming across this code, I'd end up here, where this token is defined, to try to understand why and how to use it. Maybe just leave a comment here, explaining its role?

// Throw this inside of an Angular route resolver after calling `kbnUrl.change` so that the $router can
// observe the $location update. Otherwise, the location won't be updated.
export const WAIT_FOR_URL_CHANGE_TOKEN = new Error('WAIT_FOR_URL_CHANGE_TOKEN');


export class RouteSetupManager {
constructor() {
this.setupWork = [];
this.onSetupComplete = [];
Expand Down Expand Up @@ -71,9 +73,18 @@ module.exports = class RouteSetupManager {

return defer.promise.then(() => Promise.all(userWork.doWork()));
})
.catch(error => {
if (error !== WAIT_FOR_URL_CHANGE_TOKEN) {
throw error;
}

// prevent moving forward, return a promise that never resolves
// so that the $router can observe the $location update
return Promise.halt();
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor nit, but this seems more like the "early exit" exception, and throwing the error seems more like the default flow, so I'd suggest swapping the two:

if (error === WAIT_FOR_URL_CHANGE_TOKEN) {
  // prevent moving forward, return a promise that never resolves
  // so that the $router can observe the $location update
  return Promise.halt();
}

throw error;

})
.then(
() => invokeEach(this.onWorkComplete),
err => callErrorHandlers(this.onWorkError, err)
);
}
};
}
2 changes: 2 additions & 0 deletions src/ui/public/routes/routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import RouteManager from './route_manager';
import 'angular-route/angular-route';
import { uiModules } from 'ui/modules';
const defaultRouteManager = new RouteManager();
import { ABORT_SETUP_WORK_TOKEN } from './route_setup_manager';
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean this to be WAIT_FOR_URL_CHANGE_TOKEN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops


module.exports = {
...defaultRouteManager,
ABORT_SETUP_WORK_TOKEN,
enable() {
uiModules
.get('kibana', ['ngRoute'])
Expand Down