From a0c88968a65a46d8c4a38981547e56477140712e Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Fri, 17 Feb 2017 12:28:34 -0500 Subject: [PATCH] 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/support/page_objects/dashboard_page.js | 22 ++++++------ 4 files changed, 49 insertions(+), 37 deletions(-) diff --git a/src/ui/public/courier/saved_object/saved_object.js b/src/ui/public/courier/saved_object/saved_object.js index 98562ca0fe80eb..83e4a803914dbf 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 21a44d2e699616..63e03489ad642d 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 8145caace9d988..0b01501498e2e0 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/support/page_objects/dashboard_page.js b/test/support/page_objects/dashboard_page.js index c6f66a34b54324..261bd00233998b 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(); }