From c4b3ade27d8be5b34daf47a0d1c64fb6b87c12c6 Mon Sep 17 00:00:00 2001 From: Shaunak Kashyap Date: Tue, 23 May 2017 11:53:32 -0700 Subject: [PATCH] Prevent flicker on Getting Started page (#11826) * Consolide chrome visibility toggle code into single module * Extracting code into helper functions for better readability * Removing redundant setting * Extracting functions to higher level in scope so they are not defined multiple times * Fixing typo in comment * Starting to add unit tests * Actually adding the tests this time * Adding unit tests for handleGettingStartedOptedOutScenario function * Binding this to window to prevent Uncaught TypeError: Illegal invocation * Adding unit tests for handleExistingIndexPatternsScenario function * Add helper function to aid unit testing * Refactor unit tests * Inlining function to prevent naming awkwardness * Binding this to window to remove ambiguity * Adding missing import * Fixing import * Catching expected error --- .../getting_started/getting_started.js | 7 - .../public/lib/__tests__/add_setup_work.js | 179 ++++++++++++++++++ .../public/lib/add_setup_work.js | 158 +++++++++------- .../public/getting_started/opt_out_helpers.js | 11 +- src/ui/public/notify/notifier.js | 4 +- 5 files changed, 280 insertions(+), 79 deletions(-) create mode 100644 src/core_plugins/getting_started/public/lib/__tests__/add_setup_work.js diff --git a/src/core_plugins/getting_started/public/components/getting_started/getting_started.js b/src/core_plugins/getting_started/public/components/getting_started/getting_started.js index 1ec9c774d445fe..14de2353076aa1 100644 --- a/src/core_plugins/getting_started/public/components/getting_started/getting_started.js +++ b/src/core_plugins/getting_started/public/components/getting_started/getting_started.js @@ -1,5 +1,4 @@ import { uiModules } from 'ui/modules'; -import uiChrome from 'ui/chrome'; import 'ui/getting_started/opt_out_directive'; import { GettingStartedRegistryProvider } from 'ui/getting_started/registry'; import { GETTING_STARTED_REGISTRY_TYPES } from 'ui/getting_started/constants'; @@ -32,12 +31,6 @@ app.directive('gettingStarted', function ($injector) { controllerAs: 'gettingStarted', controller: class GettingStartedController { constructor() { - if (this.hasOptedOut()) { - uiChrome.setVisible(true); - } else { - uiChrome.setVisible(false); - } - const registeredTopMessages = registry.byType[GETTING_STARTED_REGISTRY_TYPES.TOP_MESSAGE] || []; this.topMessages = registeredTopMessages.map(item => item.template); diff --git a/src/core_plugins/getting_started/public/lib/__tests__/add_setup_work.js b/src/core_plugins/getting_started/public/lib/__tests__/add_setup_work.js new file mode 100644 index 00000000000000..bf526140db1121 --- /dev/null +++ b/src/core_plugins/getting_started/public/lib/__tests__/add_setup_work.js @@ -0,0 +1,179 @@ +import expect from 'expect.js'; +import sinon from 'sinon'; +import { set } from 'lodash'; +import uiChrome from 'ui/chrome'; +import { Notifier } from 'ui/notify/notifier'; +import { WAIT_FOR_URL_CHANGE_TOKEN } from 'ui/routes'; +import { gettingStartedGateCheck } from '../add_setup_work'; +import { + GETTING_STARTED_ROUTE, + CREATE_INDEX_PATTERN_ROUTE +} from '../constants'; +import { + hasOptedOutOfGettingStarted, + undoOptOutOfGettingStarted +} from 'ui/getting_started/opt_out_helpers'; + +describe('Getting Started page', () => { + describe('add_setup_work', () => { + describe('gettingStartedGateCheck', () => { + + let getIds; + let kbnUrl; + let config; + let $route; + + beforeEach(() => { + kbnUrl = { + change: sinon.spy() + }; + $route = {}; + set($route, 'current.$$route', {}); + }); + + describe('if index patterns exist', () => { + beforeEach(() => { + config = { + get: sinon.stub(), + set: sinon.spy() + }; + getIds = sinon.stub() + .returns(Promise.resolve([ 'logstash-*', 'cars' ])); + }); + + it('sets the chrome to visible', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(uiChrome.getVisible()).to.be(true); + }); + + it('opts the user out of the Getting Started page', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(hasOptedOutOfGettingStarted()).to.be(true); + }); + + describe('if the current route does not require a default index pattern', () => { + beforeEach(() => { + $route.current.$$route.requireDefaultIndex = false; + }); + + it('returns without performing any default index pattern checks', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(config.get.called).to.be(false); + expect(config.set.called).to.be(false); + }); + }); + + describe('if the current route requires a default index pattern', () => { + beforeEach(() => { + set($route, 'current.$$route.requireDefaultIndex', true); + }); + + describe('if a default index pattern exists', () => { + beforeEach(() => { + config.get + .withArgs('defaultIndex') + .returns('an-index-pattern'); + }); + + it('returns without setting a default index pattern', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(config.set.called).to.be(false); + }); + }); + + describe('if a default index pattern does not exist', () => { + beforeEach(() => { + config.get + .withArgs('defaultIndex') + .returns(undefined); + }); + + it('sets the first index pattern as the default index pattern', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(config.set.calledWith('defaultIndex', 'logstash-*')).to.be(true); + }); + }); + }); + }); + + describe('if no index patterns exist', () => { + beforeEach(() => { + getIds = sinon.stub() + .returns(Promise.resolve([])); + }); + + describe('if user has opted out of the Getting Started page', () => { + it('sets the chrome to visible', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(uiChrome.getVisible()).to.be(true); + }); + + describe('if the current route does not require a default index pattern', () => { + beforeEach(() => { + $route.current.$$route.requireDefaultIndex = false; + }); + + it('returns without redirecting the user', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(kbnUrl.change.called).to.be(false); + }); + }); + + describe('if the current route requires a default index pattern', () => { + beforeEach(() => { + $route.current.$$route.requireDefaultIndex = true; + }); + + afterEach(() => { + // Clear out any notifications + Notifier.prototype._notifs.length = 0; + }); + + it('redirects the user to the Create Index Pattern page', async () => { + try { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + } catch (e) { + expect(e).to.be(WAIT_FOR_URL_CHANGE_TOKEN); + } + expect(kbnUrl.change.calledWith(CREATE_INDEX_PATTERN_ROUTE)).to.be(true); + }); + }); + }); + + describe('if the user has not opted out of the Getting Started page', () => { + beforeEach(() => { + undoOptOutOfGettingStarted(); + getIds = sinon.stub() + .returns(Promise.resolve([])); + }); + + describe('if the user is not already on Getting Started page', () => { + beforeEach(() => { + $route.current.$$route.originalPath = 'discover'; + }); + + it('redirects the user to the Getting Started page', async () => { + try { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + } catch (e) { + expect(e).to.be(WAIT_FOR_URL_CHANGE_TOKEN); + } + expect(kbnUrl.change.calledWith(GETTING_STARTED_ROUTE)).to.be(true); + }); + }); + + describe('if the user is already on Getting Started page', () => { + beforeEach(() => { + $route.current.$$route.originalPath = GETTING_STARTED_ROUTE; + }); + + it('redirects the user to the Getting Started page', async () => { + await gettingStartedGateCheck(getIds, kbnUrl, config, $route); + expect(kbnUrl.change.called).to.be(false); + }); + }); + }); + }); + }); + }); +}); \ No newline at end of file diff --git a/src/core_plugins/getting_started/public/lib/add_setup_work.js b/src/core_plugins/getting_started/public/lib/add_setup_work.js index 144d37fa7c891f..3c9bf39cbcfded 100644 --- a/src/core_plugins/getting_started/public/lib/add_setup_work.js +++ b/src/core_plugins/getting_started/public/lib/add_setup_work.js @@ -3,6 +3,7 @@ import uiRoutes, { WAIT_FOR_URL_CHANGE_TOKEN } from 'ui/routes'; import uiChrome from 'ui/chrome'; import { Notifier } from 'ui/notify/notifier'; import { IndexPatternsGetIdsProvider } from 'ui/index_patterns/_get_ids'; +import KbnUrlProvider from 'ui/url'; import { hasOptedOutOfGettingStarted, optOutOfGettingStarted } from 'ui/getting_started/opt_out_helpers'; import { @@ -10,71 +11,94 @@ import { CREATE_INDEX_PATTERN_ROUTE } from './constants'; -uiRoutes - .addSetupWork(function gettingStartedGateCheck(Private, $injector) { - const getIds = Private(IndexPatternsGetIdsProvider); - const config = $injector.get('config'); - const kbnUrl = $injector.get('kbnUrl'); - const $route = $injector.get('$route'); - - const currentRoute = get($route, 'current.$$route'); - - return getIds() - .then(indexPatterns => { - const indexPatternsExist = Array.isArray(indexPatterns) && indexPatterns.length > 0; - const isOnGettingStartedPage = get(currentRoute, 'originalPath') === GETTING_STARTED_ROUTE; - - if (indexPatternsExist) { - - // The user need not see the Getting Started page, so opt them out of it - optOutOfGettingStarted(); - - // Some routes require a default index pattern to be present. If we're - // NOT on such a route, there's nothing more to do; send the user on their way - if (!currentRoute.requireDefaultIndex) { - return; - } - - // Otherwise, check if we have a default index pattern - let defaultIndexPattern = config.get('defaultIndex'); - - // If we don't have an default index pattern, make the first index pattern the - // default one - if (!Boolean(defaultIndexPattern)) { - defaultIndexPattern = indexPatterns[0]; - config.set('defaultIndex', defaultIndexPattern); - } - - // At this point, we have a default index pattern and are all set! - return; - } - - // At this point, no index patterns exist. - - // If the user has explicitly opted out of the Getting Started page - if (hasOptedOutOfGettingStarted()) { - - // Some routes require a default index pattern to be present. If we're - // NOT on such a route, there's nothing more to do; send the user on their way - if (!currentRoute.requireDefaultIndex) { - return; - } - - // Otherwise, redirect the user to the index pattern creation page with - // a notification about creating an index pattern - const notify = new Notifier({ - location: 'Index Patterns' - }); - notify.error('Please create a new index pattern'); - kbnUrl.change(CREATE_INDEX_PATTERN_ROUTE); - throw WAIT_FOR_URL_CHANGE_TOKEN; - } - - // Redirect the user to the Getting Started page (unless they are on it already) - if (!isOnGettingStartedPage) { - uiChrome.setVisible(false); - kbnUrl.change(GETTING_STARTED_ROUTE); - throw WAIT_FOR_URL_CHANGE_TOKEN; - } - }); +function handleExistingIndexPatternsScenario(indexPatterns, currentRoute, config) { + // If index patterns exist, we're not going to show the user the Getting Started page. + // So we can show the chrome again at this point. + uiChrome.setVisible(true); + + // The user need not see the Getting Started page, so opt them out of it + optOutOfGettingStarted(); + + // Some routes require a default index pattern to be present. If we're + // NOT on such a route, there's nothing more to do; send the user on their way + if (!currentRoute.requireDefaultIndex) { + return; + } + + // Otherwise, check if we have a default index pattern + let defaultIndexPattern = config.get('defaultIndex'); + + // If we don't have an default index pattern, make the first index pattern the + // default one + if (!Boolean(defaultIndexPattern)) { + defaultIndexPattern = indexPatterns[0]; + config.set('defaultIndex', defaultIndexPattern); + } + + // At this point, we have a default index pattern and are all set! + return; +} + +function handleGettingStartedOptedOutScenario(currentRoute, kbnUrl) { + // If the user has opted out of the Getting Started page, we're not going to show them that page. + // So we can show the chrome again at this point. + uiChrome.setVisible(true); + + // Some routes require a default index pattern to be present. If we're + // NOT on such a route, there's nothing more to do; send the user on their way + if (!currentRoute.requireDefaultIndex) { + return; + } + + // Otherwise, redirect the user to the index pattern creation page with + // a notification about creating an index pattern + const notify = new Notifier({ + location: 'Index Patterns' }); + notify.error('Please create a new index pattern'); + + kbnUrl.change(CREATE_INDEX_PATTERN_ROUTE); + throw WAIT_FOR_URL_CHANGE_TOKEN; +} + +function showGettingStartedPage(kbnUrl, isOnGettingStartedPage) { + // Redirect the user to the Getting Started page (unless they are on it already) + if (!isOnGettingStartedPage) { + kbnUrl.change(GETTING_STARTED_ROUTE); + throw WAIT_FOR_URL_CHANGE_TOKEN; + } +} + +/* + * This function is exported for unit testing + */ +export function gettingStartedGateCheck(getIds, kbnUrl, config, $route) { + const currentRoute = get($route, 'current.$$route'); + const isOnGettingStartedPage = get(currentRoute, 'originalPath') === GETTING_STARTED_ROUTE; + + return getIds() + .then(indexPatterns => { + const indexPatternsExist = Array.isArray(indexPatterns) && indexPatterns.length > 0; + + if (indexPatternsExist) { + return handleExistingIndexPatternsScenario(indexPatterns, currentRoute, config); + } + + if (hasOptedOutOfGettingStarted()) { + return handleGettingStartedOptedOutScenario(currentRoute, kbnUrl); + } + + return showGettingStartedPage(kbnUrl, isOnGettingStartedPage); + }); +} + +// Start out with the chrome not being shown to prevent a flicker by +// hiding it later +uiChrome.setVisible(false); +uiRoutes.addSetupWork((Private, $injector) => { + const getIds = Private(IndexPatternsGetIdsProvider); + const kbnUrl = Private(KbnUrlProvider); + const config = $injector.get('config'); + const $route = $injector.get('$route'); + return gettingStartedGateCheck(getIds, kbnUrl, config, $route); +}); \ No newline at end of file diff --git a/src/ui/public/getting_started/opt_out_helpers.js b/src/ui/public/getting_started/opt_out_helpers.js index e6cd8320f0ffe2..9e07c34a420662 100644 --- a/src/ui/public/getting_started/opt_out_helpers.js +++ b/src/ui/public/getting_started/opt_out_helpers.js @@ -1,11 +1,16 @@ -import uiChrome from 'ui/chrome'; import { GETTING_STARTED_OPT_OUT_FLAG } from './constants'; export function hasOptedOutOfGettingStarted() { - return window.localStorage.getItem(GETTING_STARTED_OPT_OUT_FLAG) || false; + return Boolean(window.localStorage.getItem(GETTING_STARTED_OPT_OUT_FLAG)); } export function optOutOfGettingStarted() { window.localStorage.setItem(GETTING_STARTED_OPT_OUT_FLAG, true); - uiChrome.setVisible(true); +} + +/** + * This function is intended for unit testing + */ +export function undoOptOutOfGettingStarted() { + window.localStorage.removeItem(GETTING_STARTED_OPT_OUT_FLAG); } \ No newline at end of file diff --git a/src/ui/public/notify/notifier.js b/src/ui/public/notify/notifier.js index 5395e440891112..0382c6cefeb2f1 100644 --- a/src/ui/public/notify/notifier.js +++ b/src/ui/public/notify/notifier.js @@ -230,8 +230,8 @@ Notifier.config = { errorLifetime: 300000, warningLifetime: 10000, infoLifetime: 5000, - setInterval: window.setInterval, - clearInterval: window.clearInterval + setInterval: window.setInterval.bind(window), + clearInterval: window.clearInterval.bind(window) }; Notifier.applyConfig = function (config) {