address code review comments

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);

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.
  </div>
  <label>
- <input type="checkbox" data-test-subj="saveAsNewCheckbox" ng-model="savedObject.copyOnSave" ng-checked="savedObject.copyOnSave">
+ <input
+ type="checkbox"
+ data-test-subj="saveAsNewCheckbox"
+ ng-model="savedObject.copyOnSave"
+ ng-checked="savedObject.copyOnSave"
+ >
  Save as a new {{savedObject.getDisplayName()}}
  </label>
 </div>

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();

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();
  }