From b0aa0b85f89ae7140d8606bef1aa8a45e1ff41ab Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Mon, 13 Feb 2017 13:07:23 -0500 Subject: [PATCH 01/14] Warn if the title is a duplicate --- .../public/courier/__tests__/saved_object.js | 1 + .../saved_object/get_title_already_exists.js | 27 +++++++++ .../courier/saved_object/saved_object.js | 57 +++++++++++++++--- .../apps/dashboard/_dashboard_save.js | 55 ++++++++++++++++++ test/functional/apps/dashboard/index.js | 1 + test/support/page_objects/common.js | 21 ++++++- test/support/page_objects/dashboard_page.js | 58 +++++++++++++------ 7 files changed, 194 insertions(+), 26 deletions(-) create mode 100644 src/ui/public/courier/saved_object/get_title_already_exists.js create mode 100644 test/functional/apps/dashboard/_dashboard_save.js diff --git a/src/ui/public/courier/__tests__/saved_object.js b/src/ui/public/courier/__tests__/saved_object.js index 5120750766a45..1540c5d72f4d5 100644 --- a/src/ui/public/courier/__tests__/saved_object.js +++ b/src/ui/public/courier/__tests__/saved_object.js @@ -85,6 +85,7 @@ describe('Saved Object', function () { */ function createInitializedSavedObject(config = {}) { const savedObject = new SavedObject(config); + savedObject.title = 'my saved object'; return savedObject.init(); } diff --git a/src/ui/public/courier/saved_object/get_title_already_exists.js b/src/ui/public/courier/saved_object/get_title_already_exists.js new file mode 100644 index 0000000000000..56c4ec20281cb --- /dev/null +++ b/src/ui/public/courier/saved_object/get_title_already_exists.js @@ -0,0 +1,27 @@ +/** + * Returns true if the given saved object has a title that already exists, false otherwise. + * @param savedObject {SavedObject} The object with the title to check. + * @param esAdmin {Object} Used to query es + * @returns {Promise} + */ +export function getTitleAlreadyExists(savedObject, esAdmin) { + const { index, title, type, id } = savedObject; + if (!title) { + throw new Error('Title must be supplied'); + } + + const body = { + query: { + bool: { + must: [{ match: { title } }], + must_not: [{ match: { id } }] + } + } + }; + + const size = 0; + return esAdmin.search({ index, type, body, size }) + .then((response) => { + return response.hits.total > 0 ? true : false; + }); +} diff --git a/src/ui/public/courier/saved_object/saved_object.js b/src/ui/public/courier/saved_object/saved_object.js index 8ccf2df854365..0f9dcee785213 100644 --- a/src/ui/public/courier/saved_object/saved_object.js +++ b/src/ui/public/courier/saved_object/saved_object.js @@ -18,6 +18,7 @@ import MappingSetupProvider from 'ui/utils/mapping_setup'; import DocSourceProvider from '../data_source/admin_doc_source'; import SearchSourceProvider from '../data_source/search_source'; +import { getTitleAlreadyExists } from './get_title_already_exists'; export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, Notifier, confirmModalPromise, indexPatterns) { @@ -36,6 +37,8 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, // type name for this object, used as the ES-type const type = config.type; + this.type = type; + this.index = kbnIndex; this.getDisplayName = function () { return type; @@ -96,7 +99,9 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, * @return {Promise} */ const hydrateIndexPattern = () => { - if (!this.searchSource) { return Promise.resolve(null); } + if (!this.searchSource) { + return Promise.resolve(null); + } if (config.clearSavedIndexPattern) { this.searchSource.set('index', undefined); @@ -105,7 +110,9 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, let index = config.indexPattern || this.searchSource.getOwn('index'); - if (!index) { return Promise.resolve(null); } + if (!index) { + return Promise.resolve(null); + } // If index is not an IndexPattern object at this point, then it's a string id of an index. if (!(index instanceof indexPatterns.IndexPattern)) { @@ -263,6 +270,11 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, * @type {string} */ const OVERWRITE_REJECTED = 'Overwrite confirmation was rejected'; + /** + * An error message to be used when the user rejects a confirm save with duplicate title. + * @type {string} + */ + const SAVE_DUPLICATE_REJECTED = 'Save with duplicate title confirmation was rejected'; /** * Attempts to create the current object using the serialized source. If an object already @@ -290,6 +302,26 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, }); }; + /** + * Returns a promise that resolves to true if either the title is unique, or if the user confirmed they + * wished to save the duplicate title. Promise is rejected if the user rejects the confirmation. + */ + const warnIfDuplicateTitle = () => { + // Don't warn if the user isn't updating the title, otherwise that would become very annoying to have + // to confirm the save every time. + if (this.title === this.lastSavedTitle) { + return Promise.resolve(); + } + + return getTitleAlreadyExists(this, esAdmin) + .then((isDuplicate) => { + if (!isDuplicate) return true; + const confirmMessage = `A ${type} with the title '${this.title}' already exists. Would you like to save anyway?`; + + return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${type}` }) + .catch(() => Promise.reject(new Error(SAVE_DUPLICATE_REJECTED))); + }); + }; /** * @typedef {Object} SaveOptions @@ -325,9 +357,14 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, const source = this.serialize(); this.isSaving = true; - const doSave = saveOptions.confirmOverwrite ? createSource(source) : docSource.doIndex(source); - return doSave - .then((id) => { this.id = id; }) + + return warnIfDuplicateTitle() + .then(() => { + return saveOptions.confirmOverwrite ? createSource(source) : docSource.doIndex(source); + }) + .then((id) => { + this.id = id; + }) .then(refreshIndex) .then(() => { this.isSaving = false; @@ -337,7 +374,11 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, .catch((err) => { this.isSaving = false; this.id = originalId; - if (err && err.message === OVERWRITE_REJECTED) return; + if (err && ( + err.message === OVERWRITE_REJECTED || + err.message === SAVE_DUPLICATE_REJECTED)) { + return; + } return Promise.reject(err); }); }; @@ -360,7 +401,9 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, type: type, id: this.id }) - .then(() => { return refreshIndex(); }); + .then(() => { + return refreshIndex(); + }); }; } diff --git a/test/functional/apps/dashboard/_dashboard_save.js b/test/functional/apps/dashboard/_dashboard_save.js new file mode 100644 index 0000000000000..dae38aefb991d --- /dev/null +++ b/test/functional/apps/dashboard/_dashboard_save.js @@ -0,0 +1,55 @@ +import expect from 'expect.js'; +import { bdd } from '../../../support'; + +import PageObjects from '../../../support/page_objects'; + +bdd.describe('dashboard save', function describeIndexTests() { + const dashboardName = 'Dashboard Save Test'; + + bdd.before(async function () { + return PageObjects.dashboard.initTests(); + }); + + bdd.it('warns on duplicate name', async function() { + await PageObjects.dashboard.clickNewDashboard(); + await PageObjects.dashboard.saveDashboard(dashboardName); + + let isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(false); + + await PageObjects.dashboard.gotoDashboardLandingPage(); + await PageObjects.dashboard.clickNewDashboard(); + await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName); + + isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(true); + }); + + bdd.it('does not save on reject confirmation', async function() { + await PageObjects.common.clickCancelOnModal(); + + const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); + expect(countOfDashboards).to.equal(1); + + }); + + bdd.it('Saves on confirm duplicate title warning', async function() { + await PageObjects.dashboard.gotoDashboardLandingPage(); + await PageObjects.dashboard.clickNewDashboard(); + await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName); + + await PageObjects.common.clickConfirmOnModal(); + + const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); + expect(countOfDashboards).to.equal(2); + }); + + bdd.it('Does not warn when saving a duplicate title that remains unchanged', async function() { + await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); + await PageObjects.header.isGlobalLoadingIndicatorHidden(); + await PageObjects.dashboard.saveDashboard(dashboardName); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(false); + }); +}); diff --git a/test/functional/apps/dashboard/index.js b/test/functional/apps/dashboard/index.js index e348feb3b95da..635113f876ee7 100644 --- a/test/functional/apps/dashboard/index.js +++ b/test/functional/apps/dashboard/index.js @@ -14,5 +14,6 @@ bdd.describe('dashboard app', function () { }); require('./_dashboard'); + require('./_dashboard_save'); require('./_dashboard_time'); }); diff --git a/test/support/page_objects/common.js b/test/support/page_objects/common.js index 1a6a74f2d91b5..f528c3d7b6352 100644 --- a/test/support/page_objects/common.js +++ b/test/support/page_objects/common.js @@ -333,12 +333,29 @@ export default class Common { async getSharedItemTitleAndDescription() { const element = await this.remote - .setFindTimeout(defaultFindTimeout) - .findByCssSelector('[shared-item]'); + .setFindTimeout(defaultFindTimeout) + .findByCssSelector('[shared-item]'); return { title: await element.getAttribute('data-title'), description: await element.getAttribute('data-description') }; } + + async clickConfirmOnModal() { + this.debug('Clicking modal confirm'); + await this.findTestSubject('confirmModalConfirmButton').click(); + } + + async clickCancelOnModal() { + this.debug('Clicking modal cancel'); + await this.findTestSubject('confirmModalCancelButton').click(); + } + + async isConfirmModalOpen() { + let isOpen = true; + await this.findTestSubject('confirmModalCancelButton', 2000).catch(() => isOpen = false); + await this.remote.setFindTimeout(defaultFindTimeout); + return isOpen; + } } diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index 61a5f4f490b25..79dc4e6402705 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -41,7 +41,9 @@ export default class DashboardPage { PageObjects.common.debug('Go to dashboard landing page'); const onPage = await this.onDashboardLandingPage(); if (!onPage) { - return PageObjects.common.findByCssSelector('a[href="#/dashboard"]').click(); + return PageObjects.common.try(() => + PageObjects.common.findByCssSelector('a[href="#/dashboard"]').click() + ); } } @@ -131,13 +133,28 @@ export default class DashboardPage { } async saveDashboard(dashName, storeTimeWithDash) { + await this.enterDashboardTitleAndClickSave(dashName, storeTimeWithDash); + + await PageObjects.header.isGlobalLoadingIndicatorHidden(); + + // verify that green message at the top of the page. + // it's only there for about 5 seconds + await PageObjects.common.try(() => { + PageObjects.common.debug('verify toast-message for saved dashboard'); + return this.findTimeout + .findByCssSelector('kbn-truncated.toast-message.ng-isolate-scope') + .getVisibleText(); + }); + } + + async enterDashboardTitleAndClickSave(dashboardTitle, storeTimeWithDash) { await PageObjects.common.findTestSubject('dashboardSaveButton').click(); await PageObjects.header.waitUntilLoadingHasFinished(); await PageObjects.common.sleep(1000); PageObjects.common.debug('entering new title'); - await this.findTimeout.findById('dashboardTitle').type(dashName); + await this.findTimeout.findById('dashboardTitle').type(dashboardTitle); if (storeTimeWithDash !== undefined) { await this.storeTimeWithDashboard(storeTimeWithDash); @@ -150,17 +167,7 @@ export default class DashboardPage { PageObjects.common.debug('clicking final Save button for named dashboard'); return this.findTimeout.findByCssSelector('.btn-primary').click(); }); - await PageObjects.header.waitUntilLoadingHasFinished(); - - // verify that green message at the top of the page. - // it's only there for about 5 seconds - await PageObjects.common.try(() => { - PageObjects.common.debug('verify toast-message for saved dashboard'); - return this.findTimeout - .findByCssSelector('kbn-truncated.toast-message.ng-isolate-scope') - .getVisibleText(); - }); } clickDashboardByLinkText(dashName) { @@ -169,18 +176,35 @@ export default class DashboardPage { .click(); } - // use the search filter box to narrow the results down to a single - // entry, or at least to a single page of results - async loadSavedDashboard(dashName) { - PageObjects.common.debug(`Load Saved Dashboard ${dashName}`); - const self = this; + async searchForDashboardWithName(dashName) { + PageObjects.common.debug(`searchForDashboardWithName: ${dashName}`); + await this.gotoDashboardLandingPage(); const searchBox = await PageObjects.common.findTestSubject('searchFilter'); await searchBox.click(); + + // Note: this replacement of - to space is to preserve original logic but I'm not sure why or if it's needed. await searchBox.type(dashName.replace('-',' ')); await PageObjects.header.waitUntilLoadingHasFinished(); await PageObjects.common.sleep(1000); + return await PageObjects.header.isGlobalLoadingIndicatorHidden(); + } + + async getDashboardCountWithName(dashName) { + PageObjects.common.debug(`getDashboardCountWithName: ${dashName}`); + + await this.searchForDashboardWithName(dashName); + const links = await this.findTimeout.findAllByLinkText(dashName); + return links.length; + } + + // use the search filter box to narrow the results down to a single + // entry, or at least to a single page of results + async loadSavedDashboard(dashName) { + PageObjects.common.debug(`Load Saved Dashboard ${dashName}`); + + await this.searchForDashboardWithName(dashName); await this.clickDashboardByLinkText(dashName); return PageObjects.header.waitUntilLoadingHasFinished(); } From ee42002a16ccafdac76873057d2aff7a92692f4e Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 14 Feb 2017 09:20:37 -0500 Subject: [PATCH 02/14] warn when copyOnSave is checked. add a test --- .../courier/saved_object/saved_object.js | 4 +- .../ui/saved_object_save_as_checkbox.html | 2 +- .../apps/dashboard/_dashboard_save.js | 14 +++++-- .../apps/dashboard/_dashboard_time.js | 6 +-- test/support/page_objects/dashboard_page.js | 37 ++++++++++++++++--- 5 files changed, 48 insertions(+), 15 deletions(-) diff --git a/src/ui/public/courier/saved_object/saved_object.js b/src/ui/public/courier/saved_object/saved_object.js index 0f9dcee785213..d09db863b2390 100644 --- a/src/ui/public/courier/saved_object/saved_object.js +++ b/src/ui/public/courier/saved_object/saved_object.js @@ -308,8 +308,8 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, */ const warnIfDuplicateTitle = () => { // Don't warn if the user isn't updating the title, otherwise that would become very annoying to have - // to confirm the save every time. - if (this.title === this.lastSavedTitle) { + // to confirm the save every time, except when copyOnSave is true, then we do want to check. + if (this.title === this.lastSavedTitle && !this.copyOnSave) { return Promise.resolve(); } diff --git a/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html b/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html index 22aa9bc996dde..21a44d2e69961 100644 --- a/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html +++ b/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html @@ -3,7 +3,7 @@ In previous versions of Kibana, changing the name of a {{savedObject.getDisplayName()}} would make a copy with the new name. Use the 'Save as a new {{savedObject.getDisplayName()}}' checkbox to do this now. diff --git a/test/functional/apps/dashboard/_dashboard_save.js b/test/functional/apps/dashboard/_dashboard_save.js index dae38aefb991d..47f16a8a28bd9 100644 --- a/test/functional/apps/dashboard/_dashboard_save.js +++ b/test/functional/apps/dashboard/_dashboard_save.js @@ -10,7 +10,7 @@ bdd.describe('dashboard save', function describeIndexTests() { return PageObjects.dashboard.initTests(); }); - bdd.it('warns on duplicate name', async function() { + bdd.it('warns on duplicate name for new dashboard', async function() { await PageObjects.dashboard.clickNewDashboard(); await PageObjects.dashboard.saveDashboard(dashboardName); @@ -30,7 +30,6 @@ bdd.describe('dashboard save', function describeIndexTests() { const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); expect(countOfDashboards).to.equal(1); - }); bdd.it('Saves on confirm duplicate title warning', async function() { @@ -44,7 +43,7 @@ bdd.describe('dashboard save', function describeIndexTests() { expect(countOfDashboards).to.equal(2); }); - bdd.it('Does not warn when saving a duplicate title that remains unchanged', async function() { + bdd.it('Does not warn when saving a duplicate title that remains unchanged for an existing dashboard', async function() { await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); await PageObjects.header.isGlobalLoadingIndicatorHidden(); await PageObjects.dashboard.saveDashboard(dashboardName); @@ -52,4 +51,13 @@ bdd.describe('dashboard save', function describeIndexTests() { const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); expect(isConfirmOpen).to.equal(false); }); + + bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() { + await PageObjects.dashboard.saveDashboard(dashboardName, { saveAsNew: true }); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(true); + + await PageObjects.common.clickCancelOnModal(); + }); }); diff --git a/test/functional/apps/dashboard/_dashboard_time.js b/test/functional/apps/dashboard/_dashboard_time.js index 7113c586bbf2c..24307d4971209 100644 --- a/test/functional/apps/dashboard/_dashboard_time.js +++ b/test/functional/apps/dashboard/_dashboard_time.js @@ -17,7 +17,7 @@ bdd.describe('dashboard time', function dashboardSaveWithTime() { bdd.it('is saved', async function () { await PageObjects.dashboard.clickNewDashboard(); await PageObjects.dashboard.addVisualizations([PageObjects.dashboard.getTestVisualizationNames()[0]]); - await PageObjects.dashboard.saveDashboard(dashboardName, false); + await PageObjects.dashboard.saveDashboard(dashboardName, { storeTimeWithDashboard: false }); }); bdd.it('Does not set the time picker on open', async function () { @@ -35,7 +35,7 @@ bdd.describe('dashboard time', function dashboardSaveWithTime() { bdd.describe('dashboard with stored timed', async function () { bdd.it('is saved with quick time', async function () { await PageObjects.header.setQuickTime('Today'); - await PageObjects.dashboard.saveDashboard(dashboardName, true); + await PageObjects.dashboard.saveDashboard(dashboardName, { storeTimeWithDashboard: true }); }); bdd.it('sets quick time on open', async function () { @@ -49,7 +49,7 @@ bdd.describe('dashboard time', function dashboardSaveWithTime() { bdd.it('is saved with absolute time', async function () { await PageObjects.header.setAbsoluteRange(fromTime, toTime); - await PageObjects.dashboard.saveDashboard(dashboardName, true); + await PageObjects.dashboard.saveDashboard(dashboardName, { storeTimeWithDashboard: true }); }); bdd.it('sets absolute time on open', async function () { diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index 79dc4e6402705..b0515a40f9244 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -132,8 +132,13 @@ export default class DashboardPage { }); } - async saveDashboard(dashName, storeTimeWithDash) { - await this.enterDashboardTitleAndClickSave(dashName, storeTimeWithDash); + /** + * + * @param dashName {String} + * @param saveOptions {{storeTimeWithDashboard: boolean, saveAsNew: boolean}} + */ + async saveDashboard(dashName, saveOptions = {}) { + await this.enterDashboardTitleAndClickSave(dashName, saveOptions); await PageObjects.header.isGlobalLoadingIndicatorHidden(); @@ -147,7 +152,12 @@ export default class DashboardPage { }); } - async enterDashboardTitleAndClickSave(dashboardTitle, storeTimeWithDash) { + /** + * + * @param dashboardTitle {String} + * @param saveOptions {{storeTimeWithDashboard: boolean, saveAsNew: boolean}} + */ + async enterDashboardTitleAndClickSave(dashboardTitle, saveOptions = {}) { await PageObjects.common.findTestSubject('dashboardSaveButton').click(); await PageObjects.header.waitUntilLoadingHasFinished(); @@ -156,8 +166,12 @@ export default class DashboardPage { PageObjects.common.debug('entering new title'); await this.findTimeout.findById('dashboardTitle').type(dashboardTitle); - if (storeTimeWithDash !== undefined) { - await this.storeTimeWithDashboard(storeTimeWithDash); + if (saveOptions.storeTimeWithDashboard !== undefined) { + await this.storeTimeWithDashboard(saveOptions.storeTimeWithDashboard); + } + + if (saveOptions.saveAsNew !== undefined) { + await this.saveAsNewCheckbox(saveOptions.saveAsNew); } await PageObjects.header.waitUntilLoadingHasFinished(); @@ -304,8 +318,19 @@ export default class DashboardPage { await PageObjects.header.setAbsoluteRange(fromTime, toTime); } - async storeTimeWithDashboard(on) { + async saveAsNewCheckbox(on) { PageObjects.common.debug('Storing time with dashboard: ' + on); + const saveAsNewCheckbox = await PageObjects.common.findTestSubject('saveAsNewCheckbox'); + const checked = await saveAsNewCheckbox.getProperty('checked'); + if (checked === true && on === false || + checked === false && on === true) { + PageObjects.common.debug('Flipping save as new checkbox'); + await saveAsNewCheckbox.click(); + } + } + + async storeTimeWithDashboard(on) { + PageObjects.common.debug('saveAsNewCheckbox: ' + on); const storeTimeCheckbox = await PageObjects.common.findTestSubject('storeTimeWithDashboard'); const checked = await storeTimeCheckbox.getProperty('checked'); if (checked === true && on === false || From c90c8ae6b574f6523be4c6750135a3729dce7c01 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 14 Feb 2017 09:24:50 -0500 Subject: [PATCH 03/14] update message --- src/ui/public/courier/saved_object/saved_object.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ui/public/courier/saved_object/saved_object.js b/src/ui/public/courier/saved_object/saved_object.js index d09db863b2390..610bbf8478eb2 100644 --- a/src/ui/public/courier/saved_object/saved_object.js +++ b/src/ui/public/courier/saved_object/saved_object.js @@ -316,7 +316,8 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, return getTitleAlreadyExists(this, esAdmin) .then((isDuplicate) => { if (!isDuplicate) return true; - const confirmMessage = `A ${type} with the title '${this.title}' already exists. Would you like to save anyway?`; + const confirmMessage = + `A ${type} with the title '${this.title}' already exists. Would you like to save a duplicate with the same name?`; return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${type}` }) .catch(() => Promise.reject(new Error(SAVE_DUPLICATE_REJECTED))); From 5ec1a92ba016a46b7ef5da0300914e04bd8ce684 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 14 Feb 2017 10:04:14 -0500 Subject: [PATCH 04/14] fix tests --- src/ui/public/courier/__tests__/saved_object.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ui/public/courier/__tests__/saved_object.js b/src/ui/public/courier/__tests__/saved_object.js index 1540c5d72f4d5..01112b1b448f5 100644 --- a/src/ui/public/courier/__tests__/saved_object.js +++ b/src/ui/public/courier/__tests__/saved_object.js @@ -69,6 +69,9 @@ describe('Saved Object', function () { * @param {Object} mockDocResponse */ function stubESResponse(mockDocResponse) { + // Stub out search for duplicate title: + sinon.stub(esAdminStub, 'search').returns(BluebirdPromise.resolve({ hits: { total: 0 } })); + sinon.stub(esDataStub, 'mget').returns(BluebirdPromise.resolve({ docs: [mockDocResponse] })); sinon.stub(esDataStub, 'index').returns(BluebirdPromise.resolve(mockDocResponse)); sinon.stub(esAdminStub, 'mget').returns(BluebirdPromise.resolve({ docs: [mockDocResponse] })); @@ -279,6 +282,7 @@ describe('Saved Object', function () { }); it('on failure', function () { + stubESResponse(getMockedDocResponse('id')); return createInitializedSavedObject({ type: 'dashboard' }).then(savedObject => { sinon.stub(DocSource.prototype, 'doIndex', () => { expect(savedObject.isSaving).to.be(true); From 4335e9e404881cbd3db88367b113605740c83d3e Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 14 Feb 2017 13:54:58 -0500 Subject: [PATCH 05/14] Fix issues with substring matching add more tests. --- .../saved_object/get_title_already_exists.js | 14 ++++++++++---- test/functional/apps/dashboard/_dashboard_save.js | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/ui/public/courier/saved_object/get_title_already_exists.js b/src/ui/public/courier/saved_object/get_title_already_exists.js index 56c4ec20281cb..b122017f0d1cc 100644 --- a/src/ui/public/courier/saved_object/get_title_already_exists.js +++ b/src/ui/public/courier/saved_object/get_title_already_exists.js @@ -1,3 +1,4 @@ +import _ from 'lodash'; /** * Returns true if the given saved object has a title that already exists, false otherwise. * @param savedObject {SavedObject} The object with the title to check. @@ -13,15 +14,20 @@ export function getTitleAlreadyExists(savedObject, esAdmin) { const body = { query: { bool: { - must: [{ match: { title } }], - must_not: [{ match: { id } }] + must: { match_phrase: { title } }, + must_not: { match: { id } } } } }; - const size = 0; + // Elastic search will return the most relevant results first, which means exact matches should come + // first, and so we shouldn't need to request everything. Using 10 just to be on the safe side. + const size = 10; return esAdmin.search({ index, type, body, size }) .then((response) => { - return response.hits.total > 0 ? true : false; + const titleMatch = _.find(response.hits.hits, function currentVersion(hit) { + return hit._source.title === title; + }); + return !!titleMatch; }); } diff --git a/test/functional/apps/dashboard/_dashboard_save.js b/test/functional/apps/dashboard/_dashboard_save.js index 47f16a8a28bd9..9886adacbdbc3 100644 --- a/test/functional/apps/dashboard/_dashboard_save.js +++ b/test/functional/apps/dashboard/_dashboard_save.js @@ -60,4 +60,18 @@ bdd.describe('dashboard save', function describeIndexTests() { await PageObjects.common.clickCancelOnModal(); }); + + bdd.it('Does not warn when only the prefix matches', async function() { + await PageObjects.dashboard.saveDashboard(dashboardName.split(' ')[0]); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(false); + }); + + bdd.it('Does not warn when case is different', async function() { + await PageObjects.dashboard.saveDashboard(dashboardName.toUpperCase()); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(false); + }); }); From 8881739e9c952b1970e258651a230c7b0d08aa77 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 14 Feb 2017 14:34:04 -0500 Subject: [PATCH 06/14] make case insensitive and return matching title --- .../saved_object/get_title_already_exists.js | 12 +++++++----- src/ui/public/courier/saved_object/saved_object.js | 6 +++--- test/functional/apps/dashboard/_dashboard_save.js | 10 ++++++---- test/support/page_objects/dashboard_page.js | 14 ++++++-------- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/ui/public/courier/saved_object/get_title_already_exists.js b/src/ui/public/courier/saved_object/get_title_already_exists.js index b122017f0d1cc..b8cf6c50c5f8b 100644 --- a/src/ui/public/courier/saved_object/get_title_already_exists.js +++ b/src/ui/public/courier/saved_object/get_title_already_exists.js @@ -1,9 +1,11 @@ import _ from 'lodash'; /** - * Returns true if the given saved object has a title that already exists, false otherwise. + * Returns true if the given saved object has a title that already exists, false otherwise. Search is case + * insensitive. * @param savedObject {SavedObject} The object with the title to check. * @param esAdmin {Object} Used to query es - * @returns {Promise} + * @returns {Promise} Returns the title that matches. Because this search is not case + * sensitive, it may not exactly match the title of the object. */ export function getTitleAlreadyExists(savedObject, esAdmin) { const { index, title, type, id } = savedObject; @@ -25,9 +27,9 @@ export function getTitleAlreadyExists(savedObject, esAdmin) { const size = 10; return esAdmin.search({ index, type, body, size }) .then((response) => { - const titleMatch = _.find(response.hits.hits, function currentVersion(hit) { - return hit._source.title === title; + const match = _.find(response.hits.hits, function currentVersion(hit) { + return hit._source.title.toLowerCase() === title.toLowerCase(); }); - return !!titleMatch; + return match ? match._source.title : undefined; }); } diff --git a/src/ui/public/courier/saved_object/saved_object.js b/src/ui/public/courier/saved_object/saved_object.js index 610bbf8478eb2..28ec1f2d1f794 100644 --- a/src/ui/public/courier/saved_object/saved_object.js +++ b/src/ui/public/courier/saved_object/saved_object.js @@ -314,10 +314,10 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, } return getTitleAlreadyExists(this, esAdmin) - .then((isDuplicate) => { - if (!isDuplicate) return true; + .then((duplicateTitle) => { + if (!duplicateTitle) return true; const confirmMessage = - `A ${type} with the title '${this.title}' already exists. Would you like to save a duplicate with the same name?`; + `A ${type} with the title '${duplicateTitle}' already exists. Would you like to save anyway?`; return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${type}` }) .catch(() => Promise.reject(new Error(SAVE_DUPLICATE_REJECTED))); diff --git a/test/functional/apps/dashboard/_dashboard_save.js b/test/functional/apps/dashboard/_dashboard_save.js index 9886adacbdbc3..f4d80b11b287e 100644 --- a/test/functional/apps/dashboard/_dashboard_save.js +++ b/test/functional/apps/dashboard/_dashboard_save.js @@ -53,7 +53,7 @@ bdd.describe('dashboard save', function describeIndexTests() { }); bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() { - await PageObjects.dashboard.saveDashboard(dashboardName, { saveAsNew: true }); + await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName, { saveAsNew: true }); const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); expect(isConfirmOpen).to.equal(true); @@ -68,10 +68,12 @@ bdd.describe('dashboard save', function describeIndexTests() { expect(isConfirmOpen).to.equal(false); }); - bdd.it('Does not warn when case is different', async function() { - await PageObjects.dashboard.saveDashboard(dashboardName.toUpperCase()); + bdd.it('Warns when case is different', async function() { + await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName.toUpperCase()); const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - expect(isConfirmOpen).to.equal(false); + expect(isConfirmOpen).to.equal(true); + + await PageObjects.common.clickCancelOnModal(); }); }); diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index b0515a40f9244..773ae9c6a89dc 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -41,9 +41,11 @@ export default class DashboardPage { PageObjects.common.debug('Go to dashboard landing page'); const onPage = await this.onDashboardLandingPage(); if (!onPage) { - return PageObjects.common.try(() => - PageObjects.common.findByCssSelector('a[href="#/dashboard"]').click() + await PageObjects.common.try(async () => + await PageObjects.common.findByCssSelector('a[href="#/dashboard"]').click() ); + // Once the searchFilter can be found, we know the page finished loading. + await PageObjects.common.try(async () => await PageObjects.common.findTestSubject('searchFilter')); } } @@ -140,7 +142,7 @@ export default class DashboardPage { async saveDashboard(dashName, saveOptions = {}) { await this.enterDashboardTitleAndClickSave(dashName, saveOptions); - await PageObjects.header.isGlobalLoadingIndicatorHidden(); + await PageObjects.header.waitUntilLoadingHasFinished(); // verify that green message at the top of the page. // it's only there for about 5 seconds @@ -161,7 +163,6 @@ export default class DashboardPage { await PageObjects.common.findTestSubject('dashboardSaveButton').click(); await PageObjects.header.waitUntilLoadingHasFinished(); - await PageObjects.common.sleep(1000); PageObjects.common.debug('entering new title'); await this.findTimeout.findById('dashboardTitle').type(dashboardTitle); @@ -174,14 +175,10 @@ export default class DashboardPage { await this.saveAsNewCheckbox(saveOptions.saveAsNew); } - await PageObjects.header.waitUntilLoadingHasFinished(); - await PageObjects.common.sleep(1000); - await PageObjects.common.try(() => { PageObjects.common.debug('clicking final Save button for named dashboard'); return this.findTimeout.findByCssSelector('.btn-primary').click(); }); - await PageObjects.header.waitUntilLoadingHasFinished(); } clickDashboardByLinkText(dashName) { @@ -194,6 +191,7 @@ export default class DashboardPage { PageObjects.common.debug(`searchForDashboardWithName: ${dashName}`); await this.gotoDashboardLandingPage(); + const searchBox = await PageObjects.common.findTestSubject('searchFilter'); await searchBox.click(); From f4011ced2ed5a8c8b73230abd543c50802dd6a26 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 16 Feb 2017 11:31:13 -0500 Subject: [PATCH 07/14] Fix bug caused by subtle difference between this.type and type Add a comment and rename one of the variables to make the distinction clearer. --- .../saved_object/get_title_already_exists.js | 5 +-- .../courier/saved_object/saved_object.js | 35 +++++++++++-------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/ui/public/courier/saved_object/get_title_already_exists.js b/src/ui/public/courier/saved_object/get_title_already_exists.js index b8cf6c50c5f8b..667c2a52100c6 100644 --- a/src/ui/public/courier/saved_object/get_title_already_exists.js +++ b/src/ui/public/courier/saved_object/get_title_already_exists.js @@ -8,7 +8,8 @@ import _ from 'lodash'; * sensitive, it may not exactly match the title of the object. */ export function getTitleAlreadyExists(savedObject, esAdmin) { - const { index, title, type, id } = savedObject; + const { index, title, id } = savedObject; + const esType = savedObject.getEsType(); if (!title) { throw new Error('Title must be supplied'); } @@ -25,7 +26,7 @@ export function getTitleAlreadyExists(savedObject, esAdmin) { // Elastic search will return the most relevant results first, which means exact matches should come // first, and so we shouldn't need to request everything. Using 10 just to be on the safe side. const size = 10; - return esAdmin.search({ index, type, body, size }) + return esAdmin.search({ index, type: esType, body, size }) .then((response) => { const match = _.find(response.hits.hits, function currentVersion(hit) { return hit._source.title.toLowerCase() === title.toLowerCase(); diff --git a/src/ui/public/courier/saved_object/saved_object.js b/src/ui/public/courier/saved_object/saved_object.js index 28ec1f2d1f794..98562ca0fe80e 100644 --- a/src/ui/public/courier/saved_object/saved_object.js +++ b/src/ui/public/courier/saved_object/saved_object.js @@ -36,12 +36,17 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, const docSource = new DocSource(); // type name for this object, used as the ES-type - const type = config.type; - this.type = type; + const esType = config.type; this.index = kbnIndex; this.getDisplayName = function () { - return type; + return esType; + }; + + // NOTE: this.type (not set in this file, but somewhere else) is the sub type, e.g. 'area' or + // 'data table', while esType is the more generic type - e.g. 'visualization' or 'saved search'. + this.getEsType = function () { + return esType; }; /** @@ -54,7 +59,7 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, // Create a notifier for sending alerts const notify = new Notifier({ - location: 'Saved ' + type + location: 'Saved ' + this.getDisplayName() }); // mapping definition for the fields that this object will expose @@ -135,17 +140,17 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, * @resolved {SavedObject} */ this.init = _.once(() => { - // ensure that the type is defined - if (!type) throw new Error('You must define a type name to use SavedObject objects.'); + // ensure that the esType is defined + if (!esType) throw new Error('You must define a type name to use SavedObject objects.'); // tell the docSource where to find the doc docSource .index(kbnIndex) - .type(type) + .type(esType) .id(this.id); - // check that the mapping for this type is defined - return mappingSetup.isDefined(type) + // check that the mapping for this esType is defined + return mappingSetup.isDefined(esType) .then(function (defined) { // if it is already defined skip this step if (defined) return true; @@ -159,8 +164,8 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, } }; - // tell mappingSetup to set type - return mappingSetup.setup(type, mapping); + // tell mappingSetup to set esType + return mappingSetup.setup(esType, mapping); }) .then(() => { // If there is not id, then there is no document to fetch from elasticsearch @@ -187,7 +192,7 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, this.applyESResp = (resp) => { this._source = _.cloneDeep(resp._source); - if (resp.found != null && !resp.found) throw new errors.SavedObjectNotFound(type, this.id); + if (resp.found != null && !resp.found) throw new errors.SavedObjectNotFound(esType, this.id); const meta = resp._source.kibanaSavedObjectMeta || {}; delete resp._source.kibanaSavedObjectMeta; @@ -317,9 +322,9 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, .then((duplicateTitle) => { if (!duplicateTitle) return true; const confirmMessage = - `A ${type} with the title '${duplicateTitle}' already exists. Would you like to save anyway?`; + `A ${this.getDisplayName()} with the title '${duplicateTitle}' already exists. Would you like to save anyway?`; - return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${type}` }) + return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${this.getDisplayName()}` }) .catch(() => Promise.reject(new Error(SAVE_DUPLICATE_REJECTED))); }); }; @@ -399,7 +404,7 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, return esAdmin.delete( { index: kbnIndex, - type: type, + type: esType, id: this.id }) .then(() => { From 2d448891c71ee4157549b49b15c2b43cd8ac3282 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 16 Feb 2017 13:05:03 -0500 Subject: [PATCH 08/14] add debug messages. --- test/functional/apps/dashboard/index.js | 4 +-- test/support/page_objects/dashboard_page.js | 31 +++++++++++++-------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/test/functional/apps/dashboard/index.js b/test/functional/apps/dashboard/index.js index 635113f876ee7..48ba8a786460d 100644 --- a/test/functional/apps/dashboard/index.js +++ b/test/functional/apps/dashboard/index.js @@ -13,7 +13,7 @@ bdd.describe('dashboard app', function () { return remote.setWindowSize(1200,800); }); - require('./_dashboard'); + //require('./_dashboard'); require('./_dashboard_save'); - require('./_dashboard_time'); + // require('./_dashboard_time'); }); diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index 773ae9c6a89dc..79ce3967052c2 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -41,12 +41,19 @@ export default class DashboardPage { PageObjects.common.debug('Go to dashboard landing page'); const onPage = await this.onDashboardLandingPage(); if (!onPage) { - await PageObjects.common.try(async () => - await PageObjects.common.findByCssSelector('a[href="#/dashboard"]').click() - ); + await PageObjects.common.try(async () => { + const goToDashboardLink = await PageObjects.common.findByCssSelector('a[href="#/dashboard"]'); + PageObjects.common.debug('Clicking dashboard landing page link'); + await goToDashboardLink.click(); + await PageObjects.common.sleep(1000); + }); // Once the searchFilter can be found, we know the page finished loading. - await PageObjects.common.try(async () => await PageObjects.common.findTestSubject('searchFilter')); + await PageObjects.common.try(async () => { + const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); + PageObjects.common.debug('Search Filter object found: ' + searchFilter); + }); } + PageObjects.common.debug('Leaving go to dashboard landing page'); } clickNewDashboard() { @@ -192,15 +199,15 @@ export default class DashboardPage { await this.gotoDashboardLandingPage(); - const searchBox = await PageObjects.common.findTestSubject('searchFilter'); - await searchBox.click(); - - // Note: this replacement of - to space is to preserve original logic but I'm not sure why or if it's needed. - await searchBox.type(dashName.replace('-',' ')); + await PageObjects.common.try(async () => { + const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); + PageObjects.common.debug('searchForDashboardWithName: search Filter object found: ' + searchFilter); + await searchFilter.click(); + // Note: this replacement of - to space is to preserve original logic but I'm not sure why or if it's needed. + await searchFilter.type(dashName.replace('-',' ')); + }); - await PageObjects.header.waitUntilLoadingHasFinished(); - await PageObjects.common.sleep(1000); - return await PageObjects.header.isGlobalLoadingIndicatorHidden(); + return await PageObjects.header.waitUntilLoadingHasFinished(); } async getDashboardCountWithName(dashName) { From 3212c915aca70c2e3e7b253f81d02f4ea6f95919 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 16 Feb 2017 17:10:25 -0500 Subject: [PATCH 09/14] fix tests again --- test/support/page_objects/dashboard_page.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index 79ce3967052c2..5e1009a1e73dd 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -43,17 +43,11 @@ export default class DashboardPage { if (!onPage) { await PageObjects.common.try(async () => { const goToDashboardLink = await PageObjects.common.findByCssSelector('a[href="#/dashboard"]'); - PageObjects.common.debug('Clicking dashboard landing page link'); await goToDashboardLink.click(); - await PageObjects.common.sleep(1000); - }); - // Once the searchFilter can be found, we know the page finished loading. - await PageObjects.common.try(async () => { + // Once the searchFilter can be found, we know the page finished loading. const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); - PageObjects.common.debug('Search Filter object found: ' + searchFilter); }); } - PageObjects.common.debug('Leaving go to dashboard landing page'); } clickNewDashboard() { From 1f2cb28657deb856ee3bf70c6d315c21d1b5e7aa Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 16 Feb 2017 17:12:06 -0500 Subject: [PATCH 10/14] uncomment out tests --- test/functional/apps/dashboard/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/apps/dashboard/index.js b/test/functional/apps/dashboard/index.js index 48ba8a786460d..635113f876ee7 100644 --- a/test/functional/apps/dashboard/index.js +++ b/test/functional/apps/dashboard/index.js @@ -13,7 +13,7 @@ bdd.describe('dashboard app', function () { return remote.setWindowSize(1200,800); }); - //require('./_dashboard'); + require('./_dashboard'); require('./_dashboard_save'); - // require('./_dashboard_time'); + require('./_dashboard_time'); }); From 807b1904759f997f343e73141473af0d1b6f8c28 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 16 Feb 2017 21:09:37 -0500 Subject: [PATCH 11/14] more debug messages --- .../apps/dashboard/_dashboard_save.js | 68 +++++++++---------- test/functional/apps/dashboard/index.js | 4 +- test/support/page_objects/dashboard_page.js | 5 +- test/support/utils/try.js | 1 + 4 files changed, 41 insertions(+), 37 deletions(-) diff --git a/test/functional/apps/dashboard/_dashboard_save.js b/test/functional/apps/dashboard/_dashboard_save.js index f4d80b11b287e..9c88f5cb86ff8 100644 --- a/test/functional/apps/dashboard/_dashboard_save.js +++ b/test/functional/apps/dashboard/_dashboard_save.js @@ -42,38 +42,38 @@ bdd.describe('dashboard save', function describeIndexTests() { const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); expect(countOfDashboards).to.equal(2); }); - - bdd.it('Does not warn when saving a duplicate title that remains unchanged for an existing dashboard', async function() { - await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); - await PageObjects.header.isGlobalLoadingIndicatorHidden(); - await PageObjects.dashboard.saveDashboard(dashboardName); - - const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - expect(isConfirmOpen).to.equal(false); - }); - - bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() { - await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName, { saveAsNew: true }); - - const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - expect(isConfirmOpen).to.equal(true); - - await PageObjects.common.clickCancelOnModal(); - }); - - bdd.it('Does not warn when only the prefix matches', async function() { - await PageObjects.dashboard.saveDashboard(dashboardName.split(' ')[0]); - - const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - expect(isConfirmOpen).to.equal(false); - }); - - bdd.it('Warns when case is different', async function() { - await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName.toUpperCase()); - - const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - expect(isConfirmOpen).to.equal(true); - - await PageObjects.common.clickCancelOnModal(); - }); + // + // bdd.it('Does not warn when saving a duplicate title that remains unchanged for an existing dashboard', async function() { + // await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); + // await PageObjects.header.isGlobalLoadingIndicatorHidden(); + // await PageObjects.dashboard.saveDashboard(dashboardName); + // + // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + // expect(isConfirmOpen).to.equal(false); + // }); + // + // bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() { + // await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName, { saveAsNew: true }); + // + // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + // expect(isConfirmOpen).to.equal(true); + // + // await PageObjects.common.clickCancelOnModal(); + // }); + // + // bdd.it('Does not warn when only the prefix matches', async function() { + // await PageObjects.dashboard.saveDashboard(dashboardName.split(' ')[0]); + // + // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + // expect(isConfirmOpen).to.equal(false); + // }); + // + // bdd.it('Warns when case is different', async function() { + // await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName.toUpperCase()); + // + // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + // expect(isConfirmOpen).to.equal(true); + // + // await PageObjects.common.clickCancelOnModal(); + // }); }); diff --git a/test/functional/apps/dashboard/index.js b/test/functional/apps/dashboard/index.js index 635113f876ee7..e145fd0eb1ee4 100644 --- a/test/functional/apps/dashboard/index.js +++ b/test/functional/apps/dashboard/index.js @@ -13,7 +13,7 @@ bdd.describe('dashboard app', function () { return remote.setWindowSize(1200,800); }); - require('./_dashboard'); + // require('./_dashboard'); require('./_dashboard_save'); - require('./_dashboard_time'); + // require('./_dashboard_time'); }); diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index 5e1009a1e73dd..1b86e547039ce 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -38,14 +38,17 @@ export default class DashboardPage { } async gotoDashboardLandingPage() { - PageObjects.common.debug('Go to dashboard landing page'); + PageObjects.common.debug('gotoDashboardLandingPage'); const onPage = await this.onDashboardLandingPage(); if (!onPage) { await PageObjects.common.try(async () => { + PageObjects.common.debug('gotoDashboardLandingPage: Trying to find dashboard landing page link'); const goToDashboardLink = await PageObjects.common.findByCssSelector('a[href="#/dashboard"]'); + PageObjects.common.debug('gotoDashboardLandingPage: Click dashboard landing page link'); await goToDashboardLink.click(); // Once the searchFilter can be found, we know the page finished loading. const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); + PageObjects.common.debug('gotoDashboardLandingPage: search filter found? ' + searchFilter); }); } } diff --git a/test/support/utils/try.js b/test/support/utils/try.js index eac1084d8b1be..281baaa500bfc 100644 --- a/test/support/utils/try.js +++ b/test/support/utils/try.js @@ -18,6 +18,7 @@ class Try { let prevMessage; function attempt() { + Log.debug('--- tryForTime: Retrying...'); lastTry = Date.now(); if (lastTry - start > timeout) { From 3d025e953b5a5564ffb1b89a08145bd5e7cd7bd9 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 16 Feb 2017 21:34:29 -0500 Subject: [PATCH 12/14] save more screenshots --- test/functional/apps/dashboard/index.js | 4 ++-- test/support/page_objects/common.js | 2 +- test/support/page_objects/dashboard_page.js | 4 +++- test/support/utils/try.js | 1 - 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/test/functional/apps/dashboard/index.js b/test/functional/apps/dashboard/index.js index e145fd0eb1ee4..635113f876ee7 100644 --- a/test/functional/apps/dashboard/index.js +++ b/test/functional/apps/dashboard/index.js @@ -13,7 +13,7 @@ bdd.describe('dashboard app', function () { return remote.setWindowSize(1200,800); }); - // require('./_dashboard'); + require('./_dashboard'); require('./_dashboard_save'); - // require('./_dashboard_time'); + require('./_dashboard_time'); }); diff --git a/test/support/page_objects/common.js b/test/support/page_objects/common.js index f528c3d7b6352..49f7d1022a963 100644 --- a/test/support/page_objects/common.js +++ b/test/support/page_objects/common.js @@ -277,7 +277,7 @@ export default class Common { .findByCssSelector(selector) .then(() => true) .catch(() => false); - this.remote.setFindTimeout(defaultFindTimeout); + await this.remote.setFindTimeout(defaultFindTimeout); PageObjects.common.debug(`exists? ${exists}`); return exists; diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index 1b86e547039ce..414ec8ae407fd 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -50,6 +50,7 @@ export default class DashboardPage { const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); PageObjects.common.debug('gotoDashboardLandingPage: search filter found? ' + searchFilter); }); + await PageObjects.common.saveScreenshot('dashboard-should-be-on-landing-page'); } } @@ -200,11 +201,12 @@ export default class DashboardPage { const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); PageObjects.common.debug('searchForDashboardWithName: search Filter object found: ' + searchFilter); await searchFilter.click(); + await PageObjects.common.saveScreenshot('searchForDashboardWithName-after-click'); // Note: this replacement of - to space is to preserve original logic but I'm not sure why or if it's needed. await searchFilter.type(dashName.replace('-',' ')); }); - return await PageObjects.header.waitUntilLoadingHasFinished(); + await PageObjects.header.waitUntilLoadingHasFinished(); } async getDashboardCountWithName(dashName) { diff --git a/test/support/utils/try.js b/test/support/utils/try.js index 281baaa500bfc..eac1084d8b1be 100644 --- a/test/support/utils/try.js +++ b/test/support/utils/try.js @@ -18,7 +18,6 @@ class Try { let prevMessage; function attempt() { - Log.debug('--- tryForTime: Retrying...'); lastTry = Date.now(); if (lastTry - start > timeout) { From de00e24a850b9b847b87c804e8cc6df6ef61c531 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Fri, 17 Feb 2017 09:01:22 -0500 Subject: [PATCH 13/14] Need to wait till save finishes or the page may redirect the url after the page has navigated away --- .../apps/dashboard/_dashboard_save.js | 73 ++++++++++--------- test/support/page_objects/dashboard_page.js | 6 -- 2 files changed, 39 insertions(+), 40 deletions(-) diff --git a/test/functional/apps/dashboard/_dashboard_save.js b/test/functional/apps/dashboard/_dashboard_save.js index 9c88f5cb86ff8..8145caace9d98 100644 --- a/test/functional/apps/dashboard/_dashboard_save.js +++ b/test/functional/apps/dashboard/_dashboard_save.js @@ -39,41 +39,46 @@ bdd.describe('dashboard save', function describeIndexTests() { await PageObjects.common.clickConfirmOnModal(); + // This is important since saving a new dashboard will cause a refresh of the page. We have to + // wait till it finishes reloading or it might reload the url after simulating the + // dashboard landing page click. + await PageObjects.header.waitUntilLoadingHasFinished(); + const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); expect(countOfDashboards).to.equal(2); }); - // - // bdd.it('Does not warn when saving a duplicate title that remains unchanged for an existing dashboard', async function() { - // await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); - // await PageObjects.header.isGlobalLoadingIndicatorHidden(); - // await PageObjects.dashboard.saveDashboard(dashboardName); - // - // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - // expect(isConfirmOpen).to.equal(false); - // }); - // - // bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() { - // await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName, { saveAsNew: true }); - // - // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - // expect(isConfirmOpen).to.equal(true); - // - // await PageObjects.common.clickCancelOnModal(); - // }); - // - // bdd.it('Does not warn when only the prefix matches', async function() { - // await PageObjects.dashboard.saveDashboard(dashboardName.split(' ')[0]); - // - // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - // expect(isConfirmOpen).to.equal(false); - // }); - // - // bdd.it('Warns when case is different', async function() { - // await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName.toUpperCase()); - // - // const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - // expect(isConfirmOpen).to.equal(true); - // - // await PageObjects.common.clickCancelOnModal(); - // }); + + bdd.it('Does not warn when saving a duplicate title that remains unchanged for an existing dashboard', async function() { + await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); + await PageObjects.header.isGlobalLoadingIndicatorHidden(); + await PageObjects.dashboard.saveDashboard(dashboardName); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(false); + }); + + bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() { + await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName, { saveAsNew: true }); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(true); + + await PageObjects.common.clickCancelOnModal(); + }); + + bdd.it('Does not warn when only the prefix matches', async function() { + await PageObjects.dashboard.saveDashboard(dashboardName.split(' ')[0]); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(false); + }); + + bdd.it('Warns when case is different', async function() { + await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName.toUpperCase()); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(true); + + await PageObjects.common.clickCancelOnModal(); + }); }); diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index 414ec8ae407fd..c6f66a34b5432 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -42,15 +42,11 @@ export default class DashboardPage { const onPage = await this.onDashboardLandingPage(); if (!onPage) { await PageObjects.common.try(async () => { - PageObjects.common.debug('gotoDashboardLandingPage: Trying to find dashboard landing page link'); const goToDashboardLink = await PageObjects.common.findByCssSelector('a[href="#/dashboard"]'); - PageObjects.common.debug('gotoDashboardLandingPage: Click dashboard landing page link'); await goToDashboardLink.click(); // Once the searchFilter can be found, we know the page finished loading. const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); - PageObjects.common.debug('gotoDashboardLandingPage: search filter found? ' + searchFilter); }); - await PageObjects.common.saveScreenshot('dashboard-should-be-on-landing-page'); } } @@ -199,9 +195,7 @@ export default class DashboardPage { await PageObjects.common.try(async () => { const searchFilter = await PageObjects.common.findTestSubject('searchFilter'); - PageObjects.common.debug('searchForDashboardWithName: search Filter object found: ' + searchFilter); await searchFilter.click(); - await PageObjects.common.saveScreenshot('searchForDashboardWithName-after-click'); // Note: this replacement of - to space is to preserve original logic but I'm not sure why or if it's needed. await searchFilter.type(dashName.replace('-',' ')); }); From 7bc02684b4d5c0289e5044e71deaa5c9fe5e77f3 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Fri, 17 Feb 2017 12:28:34 -0500 Subject: [PATCH 14/14] address code review comments --- .../courier/saved_object/saved_object.js | 35 +++++++++++-------- .../ui/saved_object_save_as_checkbox.html | 7 +++- .../apps/dashboard/_dashboard_save.js | 22 ++++++------ test/functional/apps/visualize/_area_chart.js | 2 +- test/support/page_objects/dashboard_page.js | 22 ++++++------ 5 files changed, 50 insertions(+), 38 deletions(-) diff --git a/src/ui/public/courier/saved_object/saved_object.js b/src/ui/public/courier/saved_object/saved_object.js index 98562ca0fe80e..a7d35e2dae21d 100644 --- a/src/ui/public/courier/saved_object/saved_object.js +++ b/src/ui/public/courier/saved_object/saved_object.js @@ -20,6 +20,26 @@ import DocSourceProvider from '../data_source/admin_doc_source'; import SearchSourceProvider from '../data_source/search_source'; import { getTitleAlreadyExists } from './get_title_already_exists'; +/** + * An error message to be used when the user rejects a confirm overwrite. + * @type {string} + */ +const OVERWRITE_REJECTED = 'Overwrite confirmation was rejected'; +/** + * An error message to be used when the user rejects a confirm save with duplicate title. + * @type {string} + */ +const SAVE_DUPLICATE_REJECTED = 'Save with duplicate title confirmation was rejected'; + +/** + * @param error {Error} the error + * @return {boolean} + */ +function isErrorNonFatal(error) { + if (!error) return false; + return error.message === OVERWRITE_REJECTED || error.message === SAVE_DUPLICATE_REJECTED; +} + export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, Notifier, confirmModalPromise, indexPatterns) { const DocSource = Private(DocSourceProvider); @@ -270,17 +290,6 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, return esAdmin.indices.refresh({ index: kbnIndex }); } - /** - * An error message to be used when the user rejects a confirm overwrite. - * @type {string} - */ - const OVERWRITE_REJECTED = 'Overwrite confirmation was rejected'; - /** - * An error message to be used when the user rejects a confirm save with duplicate title. - * @type {string} - */ - const SAVE_DUPLICATE_REJECTED = 'Save with duplicate title confirmation was rejected'; - /** * Attempts to create the current object using the serialized source. If an object already * exists, a warning message requests an overwrite confirmation. @@ -380,9 +389,7 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private, .catch((err) => { this.isSaving = false; this.id = originalId; - if (err && ( - err.message === OVERWRITE_REJECTED || - err.message === SAVE_DUPLICATE_REJECTED)) { + if (isErrorNonFatal(err)) { return; } return Promise.reject(err); diff --git a/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html b/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html index 21a44d2e69961..63e03489ad642 100644 --- a/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html +++ b/src/ui/public/courier/saved_object/ui/saved_object_save_as_checkbox.html @@ -3,7 +3,12 @@ In previous versions of Kibana, changing the name of a {{savedObject.getDisplayName()}} would make a copy with the new name. Use the 'Save as a new {{savedObject.getDisplayName()}}' checkbox to do this now. diff --git a/test/functional/apps/dashboard/_dashboard_save.js b/test/functional/apps/dashboard/_dashboard_save.js index 8145caace9d98..0b01501498e2e 100644 --- a/test/functional/apps/dashboard/_dashboard_save.js +++ b/test/functional/apps/dashboard/_dashboard_save.js @@ -48,16 +48,18 @@ bdd.describe('dashboard save', function describeIndexTests() { expect(countOfDashboards).to.equal(2); }); - bdd.it('Does not warn when saving a duplicate title that remains unchanged for an existing dashboard', async function() { - await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); - await PageObjects.header.isGlobalLoadingIndicatorHidden(); - await PageObjects.dashboard.saveDashboard(dashboardName); - - const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); - expect(isConfirmOpen).to.equal(false); - }); - - bdd.it('Warns when saving a duplicate title that remains unchanged when Save as New Dashboard is checked', async function() { + bdd.it('Does not warn when you save an existing dashboard with the title it already has, and that title is a duplicate', + async function() { + await PageObjects.dashboard.clickDashboardByLinkText(dashboardName); + await PageObjects.header.isGlobalLoadingIndicatorHidden(); + await PageObjects.dashboard.saveDashboard(dashboardName); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.equal(false); + } + ); + + bdd.it('Warns you when you Save as New Dashboard, and the title is a duplicate', async function() { await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName, { saveAsNew: true }); const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); diff --git a/test/functional/apps/visualize/_area_chart.js b/test/functional/apps/visualize/_area_chart.js index 54cf8311a1c0a..8d1a47c229cf0 100644 --- a/test/functional/apps/visualize/_area_chart.js +++ b/test/functional/apps/visualize/_area_chart.js @@ -60,7 +60,7 @@ bdd.describe('visualize app', function describeIndexTests() { }); bdd.describe('area charts', function indexPatternCreation() { - const vizName1 = 'Visualization AreaChart'; + const vizName1 = 'Visualization AreaChart Name Test'; bdd.it('should save and load with special characters', function () { const vizNamewithSpecialChars = vizName1 + '/?&=%'; diff --git a/test/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index c6f66a34b5432..261bd00233998 100644 --- a/test/support/page_objects/dashboard_page.js +++ b/test/support/page_objects/dashboard_page.js @@ -169,11 +169,11 @@ export default class DashboardPage { await this.findTimeout.findById('dashboardTitle').type(dashboardTitle); if (saveOptions.storeTimeWithDashboard !== undefined) { - await this.storeTimeWithDashboard(saveOptions.storeTimeWithDashboard); + await this.setStoreTimeWithDashboard(saveOptions.storeTimeWithDashboard); } if (saveOptions.saveAsNew !== undefined) { - await this.saveAsNewCheckbox(saveOptions.saveAsNew); + await this.setSaveAsNewCheckBox(saveOptions.saveAsNew); } await PageObjects.common.try(() => { @@ -316,23 +316,21 @@ export default class DashboardPage { await PageObjects.header.setAbsoluteRange(fromTime, toTime); } - async saveAsNewCheckbox(on) { - PageObjects.common.debug('Storing time with dashboard: ' + on); + async setSaveAsNewCheckBox(checked) { + PageObjects.common.debug('saveAsNewCheckbox: ' + checked); const saveAsNewCheckbox = await PageObjects.common.findTestSubject('saveAsNewCheckbox'); - const checked = await saveAsNewCheckbox.getProperty('checked'); - if (checked === true && on === false || - checked === false && on === true) { + const isAlreadyChecked = await saveAsNewCheckbox.getProperty('checked'); + if (isAlreadyChecked !== checked) { PageObjects.common.debug('Flipping save as new checkbox'); await saveAsNewCheckbox.click(); } } - async storeTimeWithDashboard(on) { - PageObjects.common.debug('saveAsNewCheckbox: ' + on); + async setStoreTimeWithDashboard(checked) { + PageObjects.common.debug('Storing time with dashboard: ' + checked); const storeTimeCheckbox = await PageObjects.common.findTestSubject('storeTimeWithDashboard'); - const checked = await storeTimeCheckbox.getProperty('checked'); - if (checked === true && on === false || - checked === false && on === true) { + const isAlreadyChecked = await storeTimeCheckbox.getProperty('checked'); + if (isAlreadyChecked !== checked) { PageObjects.common.debug('Flipping store time checkbox'); await storeTimeCheckbox.click(); }