Skip to content

Commit

Permalink
Backport #10321 (#10568)
Browse files Browse the repository at this point in the history
  • Loading branch information
stacey-gammon authored Feb 25, 2017
1 parent 5e3af54 commit 1c414d0
Show file tree
Hide file tree
Showing 10 changed files with 316 additions and 66 deletions.
5 changes: 5 additions & 0 deletions src/ui/public/courier/__tests__/saved_object.js
Original file line number Diff line number Diff line change
Expand Up @@ -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] }));
Expand All @@ -85,6 +88,7 @@ describe('Saved Object', function () {
*/
function createInitializedSavedObject(config = {}) {
const savedObject = new SavedObject(config);
savedObject.title = 'my saved object';
return savedObject.init();
}

Expand Down Expand Up @@ -278,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);
Expand Down
36 changes: 36 additions & 0 deletions src/ui/public/courier/saved_object/get_title_already_exists.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import _ from 'lodash';
/**
* 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<string|undefined>} 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, id } = savedObject;
const esType = savedObject.getEsType();
if (!title) {
throw new Error('Title must be supplied');
}

const body = {
query: {
bool: {
must: { match_phrase: { title } },
must_not: { match: { id } }
}
}
};

// 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: esType, body, size })
.then((response) => {
const match = _.find(response.hits.hits, function currentVersion(hit) {
return hit._source.title.toLowerCase() === title.toLowerCase();
});
return match ? match._source.title : undefined;
});
}
107 changes: 81 additions & 26 deletions src/ui/public/courier/saved_object/saved_object.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,27 @@ 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';

/**
* 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) {

Expand All @@ -35,10 +56,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;
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;
};

/**
Expand All @@ -51,7 +79,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
Expand Down Expand Up @@ -96,7 +124,9 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
* @return {Promise<IndexPattern | null>}
*/
const hydrateIndexPattern = () => {
if (!this.searchSource) { return Promise.resolve(null); }
if (!this.searchSource) {
return Promise.resolve(null);
}

if (config.clearSavedIndexPattern) {
this.searchSource.set('index', undefined);
Expand All @@ -105,7 +135,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)) {
Expand All @@ -128,17 +160,16 @@ 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((defined) => {
// if it is already defined skip this step
if (defined) return true;
Expand All @@ -152,8 +183,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
Expand All @@ -180,7 +211,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;
Expand Down Expand Up @@ -258,12 +289,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';

/**
* Attempts to create the current object using the serialized source. If an object already
* exists, a warning message requests an overwrite confirmation.
Expand All @@ -290,6 +315,27 @@ 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, except when copyOnSave is true, then we do want to check.
if (this.title === this.lastSavedTitle && !this.copyOnSave) {
return Promise.resolve();
}

return getTitleAlreadyExists(this, esAdmin)
.then((duplicateTitle) => {
if (!duplicateTitle) return true;
const confirmMessage =
`A ${this.getDisplayName()} with the title '${duplicateTitle}' already exists. Would you like to save anyway?`;

return confirmModalPromise(confirmMessage, { confirmButtonText: `Save ${this.getDisplayName()}` })
.catch(() => Promise.reject(new Error(SAVE_DUPLICATE_REJECTED)));
});
};

/**
* @typedef {Object} SaveOptions
Expand Down Expand Up @@ -325,9 +371,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;
Expand All @@ -337,7 +388,9 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
.catch((err) => {
this.isSaving = false;
this.id = originalId;
if (err && err.message === OVERWRITE_REJECTED) return;
if (isErrorNonFatal(err)) {
return;
}
return Promise.reject(err);
});
};
Expand All @@ -357,10 +410,12 @@ export default function SavedObjectFactory(esAdmin, kbnIndex, Promise, Private,
return esAdmin.delete(
{
index: kbnIndex,
type: type,
type: esType,
id: this.id
})
.then(() => { return refreshIndex(); });
.then(() => {
return refreshIndex();
});
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" 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>
86 changes: 86 additions & 0 deletions test/functional/apps/dashboard/_dashboard_save.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
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 for new dashboard', 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();

// 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 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();
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();
});
});
6 changes: 3 additions & 3 deletions test/functional/apps/dashboard/_dashboard_time.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand All @@ -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 () {
Expand All @@ -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 () {
Expand Down
Loading

0 comments on commit 1c414d0

Please sign in to comment.