Skip to content

Commit

Permalink
[Dashboard] Fix blank panel save and display issue. (elastic#120815)
Browse files Browse the repository at this point in the history
* Added special case to panel comparison.

* Fixed panel title behaviour when linking/unlinking to/from the library.

* Added panel title test suite.

* Made the injection of the 'savedSearchId' key conditional on if it is defined when unlinking.

* Adjusted use of 'setValue()' based on feedback from @dmlemeshko

* Refractor test suite.

* Improved code for removal of title on library link.
  • Loading branch information
Heenawter authored Dec 15, 2021
1 parent 2c6ef20 commit b6013d1
Show file tree
Hide file tree
Showing 9 changed files with 247 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,24 @@ export class UnlinkFromLibraryAction implements Action<UnlinkFromLibraryActionCo

const dashboard = embeddable.getRoot() as DashboardContainer;
const panelToReplace = dashboard.getInput().panels[embeddable.id] as DashboardPanelState;

if (!panelToReplace) {
throw new PanelNotFoundError();
}

const newPanel: PanelState<EmbeddableInput> = {
type: embeddable.type,
explicitInput: { ...newInput },
explicitInput: { ...newInput, title: embeddable.getTitle() },
};
// since by value visualizations should not have default titles, unlinking a visualization should remove
// the library title from the attributes.
_.unset(newPanel, 'explicitInput.attributes.title');
dashboard.replacePanel(panelToReplace, newPanel, true);

const title = dashboardUnlinkFromLibraryAction.getSuccessMessage(
embeddable.getTitle() ? `'${embeddable.getTitle()}'` : ''
);

this.deps.toasts.addSuccess({
title,
'data-test-subj': 'unlinkPanelSuccess',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import _ from 'lodash';
import { DashboardPanelState } from '..';
import { esFilters, Filter } from '../../services/data';
import { EmbeddableInput } from '../../services/embeddable';
import {
DashboardContainerInput,
DashboardOptions,
Expand Down Expand Up @@ -64,12 +65,12 @@ export const diffDashboardState = (
};

const optionsAreEqual = (optionsA: DashboardOptions, optionsB: DashboardOptions): boolean => {
const optionKeys = [...Object.keys(optionsA), ...Object.keys(optionsB)];
const optionKeys = [
...(Object.keys(optionsA) as Array<keyof DashboardOptions>),
...(Object.keys(optionsB) as Array<keyof DashboardOptions>),
];
for (const key of optionKeys) {
if (
Boolean((optionsA as unknown as { [key: string]: boolean })[key]) !==
Boolean((optionsB as unknown as { [key: string]: boolean })[key])
) {
if (Boolean(optionsA[key]) !== Boolean(optionsB[key])) {
return false;
}
}
Expand All @@ -90,16 +91,40 @@ const panelsAreEqual = (panelsA: DashboardPanelMap, panelsB: DashboardPanelMap):
const panelCommonDiff = commonDiff<DashboardPanelState>(
panelsA[id] as unknown as DashboardDiffCommon,
panelsB[id] as unknown as DashboardDiffCommon,
['panelRefName']
['panelRefName', 'explicitInput']
);
if (Object.keys(panelCommonDiff).length > 0) {
if (
Object.keys(panelCommonDiff).length > 0 ||
!explicitInputIsEqual(panelsA[id].explicitInput, panelsB[id].explicitInput)
) {
return false;
}
}

return true;
};

/**
* Need to compare properties of explicitInput *directly* in order to handle special comparisons for 'title'
* and 'hidePanelTitles.' For example, if some object 'obj1' has 'obj1[title] = undefined' and some other
* object `obj2' simply does not have the key `title,' we want obj1 to still equal obj2 - in normal comparisons
* without this special case, `obj1 != obj2.'
* @param originalInput
* @param newInput
*/
const explicitInputIsEqual = (
originalInput: EmbeddableInput,
newInput: EmbeddableInput
): boolean => {
const diffs = commonDiff<DashboardPanelState>(originalInput, newInput, [
'hidePanelTitles',
'title',
]);
const hidePanelsAreEqual =
Boolean(originalInput.hidePanelTitles) === Boolean(newInput.hidePanelTitles);
const titlesAreEqual = originalInput.title === newInput.title;
return Object.keys(diffs).length === 0 && hidePanelsAreEqual && titlesAreEqual;
};

const commonDiffFilters = <T extends { filters: Filter[] }>(
originalObj: DashboardDiffCommonFilters,
newObj: DashboardDiffCommonFilters,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,11 @@ export class AttributeService<
newAttributes,
true
)) as unknown as RefType;
// Remove unneeded attributes from the original input. Note that the original panel title
// is removed in favour of the new attributes title
const newInput = omit(input, [ATTRIBUTE_SERVICE_KEY, 'title']);

// Remove unneeded attributes from the original input.
const newInput = omit(input, ATTRIBUTE_SERVICE_KEY);

// Combine input and wrapped input to preserve any passed in explicit Input.
// Combine input and wrapped input to preserve any passed in explicit Input
resolve({ ...newInput, ...wrappedInput });
return { id: wrappedInput.savedObjectId };
} catch (error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export function PanelHeader({

if (!showPanelBar) {
return (
<div className={classes}>
<div data-test-subj="dashboardPanelTitle__wrapper" className={classes}>
<PanelOptionsMenu
getActionContextMenuPanel={getActionContextMenuPanel}
isViewMode={isViewMode}
Expand Down Expand Up @@ -212,22 +212,24 @@ export function PanelHeader({
};

return (
<figcaption
className={classes}
data-test-subj={`embeddablePanelHeading-${(title || '').replace(/\s/g, '')}`}
>
<h2 data-test-subj="dashboardPanelTitle" className="embPanel__title embPanel__dragger">
<EuiScreenReaderOnly>{getAriaLabel()}</EuiScreenReaderOnly>
{renderTitle()}
{renderBadges(badges, embeddable)}
</h2>
{renderNotifications(notifications, embeddable)}
<PanelOptionsMenu
isViewMode={isViewMode}
getActionContextMenuPanel={getActionContextMenuPanel}
closeContextMenu={closeContextMenu}
title={title}
/>
</figcaption>
<span data-test-subj="dashboardPanelTitle__wrapper">
<figcaption
className={classes}
data-test-subj={`embeddablePanelHeading-${(title || '').replace(/\s/g, '')}`}
>
<h2 data-test-subj="dashboardPanelTitle" className="embPanel__title embPanel__dragger">
<EuiScreenReaderOnly>{getAriaLabel()}</EuiScreenReaderOnly>
{renderTitle()}
{renderBadges(badges, embeddable)}
</h2>
{renderNotifications(notifications, embeddable)}
<PanelOptionsMenu
isViewMode={isViewMode}
getActionContextMenuPanel={getActionContextMenuPanel}
closeContextMenu={closeContextMenu}
title={title}
/>
</figcaption>
</span>
);
}
2 changes: 1 addition & 1 deletion src/plugins/visualizations/public/vis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ export class Vis<TVisParams = VisParams> {
data: {
aggs: aggs as any,
searchSource: this.data.searchSource ? this.data.searchSource.getSerializedFields() : {},
savedSearchId: this.data.savedSearchId,
...(this.data.savedSearchId ? { savedSearchId: this.data.savedSearchId } : {}),
},
};
}
Expand Down
21 changes: 21 additions & 0 deletions test/functional/page_objects/dashboard_page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,27 @@ export class DashboardPageObject extends FtrService {
return await Promise.all(titleObjects.map(async (title) => await title.getVisibleText()));
}

// returns an array of Boolean values - true if the panel title is visible in view mode, false if it is not
public async getVisibilityOfPanelTitles() {
this.log.debug('in getVisibilityOfPanels');
// only works if the dashboard is in view mode
const inViewMode = await this.getIsInViewMode();
if (!inViewMode) {
await this.clickCancelOutOfEditMode();
}
const visibilities: boolean[] = [];
const titleObjects = await this.testSubjects.findAll('dashboardPanelTitle__wrapper');
for (const titleObject of titleObjects) {
const exists = !(await titleObject.elementHasClass('embPanel__header--floater'));
visibilities.push(exists);
}
// return to edit mode if a switch to view mode above was necessary
if (!inViewMode) {
await this.switchToEditMode();
}
return visibilities;
}

public async getPanelDimensions() {
const panels = await this.find.allByCssSelector('.react-grid-item'); // These are gridster-defined elements and classes
return await Promise.all(
Expand Down
4 changes: 3 additions & 1 deletion test/functional/services/dashboard/panel_actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,9 @@ export class DashboardPanelActionsService extends FtrService {
} else {
await this.customizePanel();
}
await this.testSubjects.setValue('customEmbeddablePanelTitleInput', customTitle);
await this.testSubjects.setValue('customEmbeddablePanelTitleInput', customTitle, {
clearWithKeyboard: customTitle === '', // if clearing the title using the empty string as the new value, 'clearWithKeyboard' must be true; otherwise, false
});
await this.testSubjects.click('saveNewTitleButton');
}

Expand Down
1 change: 1 addition & 0 deletions x-pack/test/functional/apps/dashboard/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export default function ({ loadTestFile }: FtrProviderContext) {
loadTestFile(require.resolve('./dashboard_tagging'));
loadTestFile(require.resolve('./dashboard_lens_by_value'));
loadTestFile(require.resolve('./dashboard_maps_by_value'));
loadTestFile(require.resolve('./panel_titles'));

loadTestFile(require.resolve('./migration_smoke_tests/lens_migration_smoke_test'));
loadTestFile(require.resolve('./migration_smoke_tests/controls_migration_smoke_test'));
Expand Down
158 changes: 158 additions & 0 deletions x-pack/test/functional/apps/dashboard/panel_titles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import expect from '@kbn/expect';
import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ getService, getPageObjects }: FtrProviderContext) {
const esArchiver = getService('esArchiver');
const kibanaServer = getService('kibanaServer');
const testSubjects = getService('testSubjects');
const retry = getService('retry');
const dashboardPanelActions = getService('dashboardPanelActions');
const PageObjects = getPageObjects([
'common',
'dashboard',
'visualize',
'visEditor',
'timePicker',
'lens',
]);

const DASHBOARD_NAME = 'Panel Title Test';
const CUSTOM_TITLE = 'Test Custom Title';
const EMPTY_TITLE = '[No Title]';
const LIBRARY_TITLE_FOR_CUSTOM_TESTS = 'Library Title for Custom Title Tests';
const LIBRARY_TITLE_FOR_EMPTY_TESTS = 'Library Title for Empty Title Tests';

describe('panel titles', () => {
before(async () => {
await esArchiver.load('test/functional/fixtures/es_archiver/dashboard/current/kibana');
await esArchiver.loadIfNeeded('x-pack/test/functional/es_archives/logstash_functional');
await kibanaServer.importExport.load(
'x-pack/test/functional/fixtures/kbn_archiver/lens/lens_basic.json'
);
await PageObjects.common.navigateToApp('dashboard');
await PageObjects.dashboard.preserveCrossAppState();
await PageObjects.dashboard.clickNewDashboard();
await PageObjects.dashboard.saveDashboard(DASHBOARD_NAME);
});

describe('panel titles - by value', () => {
const clearUnsavedChanges = async () => {
await retry.try(async () => {
// avoid flaky test by surrounding in retry
await testSubjects.existOrFail('dashboardUnsavedChangesBadge');
await PageObjects.dashboard.clickQuickSave();
await testSubjects.missingOrFail('dashboardUnsavedChangesBadge');
});
};

it('new panel by value has empty title', async () => {
await PageObjects.lens.createAndAddLensFromDashboard({});
const newPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(newPanelTitle).to.equal(EMPTY_TITLE);
});

it('saving new panel with blank title clears "unsaved changes" badge', async () => {
await dashboardPanelActions.setCustomPanelTitle('');
await clearUnsavedChanges();
});

it('custom title causes unsaved changes and saving clears it', async () => {
await dashboardPanelActions.setCustomPanelTitle(CUSTOM_TITLE);
const panelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(panelTitle).to.equal(CUSTOM_TITLE);
await clearUnsavedChanges();
});

it('resetting title on a by value panel sets it to the empty string', async () => {
const BY_VALUE_TITLE = 'Reset Title - By Value';
await dashboardPanelActions.setCustomPanelTitle(BY_VALUE_TITLE);

await dashboardPanelActions.resetCustomPanelTitle();
const panelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(panelTitle).to.equal(EMPTY_TITLE);
await clearUnsavedChanges();
});

it('blank titles are hidden in view mode', async () => {
await PageObjects.dashboard.clickCancelOutOfEditMode();

const titleVisibility = (await PageObjects.dashboard.getVisibilityOfPanelTitles())[0];
expect(titleVisibility).to.be(false);
});

it('custom titles are visible in view mode', async () => {
await PageObjects.dashboard.switchToEditMode();
await dashboardPanelActions.setCustomPanelTitle(CUSTOM_TITLE);
await PageObjects.dashboard.clickQuickSave();
await PageObjects.dashboard.clickCancelOutOfEditMode();

const titleVisibility = (await PageObjects.dashboard.getVisibilityOfPanelTitles())[0];
expect(titleVisibility).to.be(true);
});

it('hiding an individual panel title hides it in view mode', async () => {
await PageObjects.dashboard.switchToEditMode();
await dashboardPanelActions.toggleHidePanelTitle();
await PageObjects.dashboard.clickQuickSave();
await PageObjects.dashboard.clickCancelOutOfEditMode();

const titleVisibility = (await PageObjects.dashboard.getVisibilityOfPanelTitles())[0];
expect(titleVisibility).to.be(false);

// undo the previous hide panel toggle (i.e. make the panel visible) to keep state consistent
await PageObjects.dashboard.switchToEditMode();
await dashboardPanelActions.toggleHidePanelTitle();
await PageObjects.dashboard.clickQuickSave();
});
});

describe('panel titles - by reference', () => {
it('linking a by value panel with a custom title to the library will overwrite the custom title with the library title', async () => {
await dashboardPanelActions.setCustomPanelTitle(CUSTOM_TITLE);
await dashboardPanelActions.saveToLibrary(LIBRARY_TITLE_FOR_CUSTOM_TESTS);
await retry.try(async () => {
// need to surround in 'retry' due to delays in HTML updates causing the title read to be behind
const newPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(newPanelTitle).to.equal(LIBRARY_TITLE_FOR_CUSTOM_TESTS);
});
});

it('resetting title on a by reference panel sets it to the library title', async () => {
await dashboardPanelActions.setCustomPanelTitle('This should go away');
await dashboardPanelActions.resetCustomPanelTitle();
const resetPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(resetPanelTitle).to.equal(LIBRARY_TITLE_FOR_CUSTOM_TESTS);
});

it('unlinking a by reference panel with a custom title will keep the current title', async () => {
await dashboardPanelActions.setCustomPanelTitle(CUSTOM_TITLE);
await dashboardPanelActions.unlinkFromLibary();
const newPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(newPanelTitle).to.equal(CUSTOM_TITLE);
});

it("linking a by value panel with a blank title to the library will set the panel's title to the library title", async () => {
await dashboardPanelActions.setCustomPanelTitle('');
await dashboardPanelActions.saveToLibrary(LIBRARY_TITLE_FOR_EMPTY_TESTS);
await retry.try(async () => {
// need to surround in 'retry' due to delays in HTML updates causing the title read to be behind
const newPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(newPanelTitle).to.equal(LIBRARY_TITLE_FOR_EMPTY_TESTS);
});
});

it('unlinking a by reference panel without a custom title will keep the library title', async () => {
await dashboardPanelActions.unlinkFromLibary();
const newPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(newPanelTitle).to.equal(LIBRARY_TITLE_FOR_EMPTY_TESTS);
});
});
});
}

0 comments on commit b6013d1

Please sign in to comment.