Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dashboard] Fix blank panel save and display issue. #120815

Merged
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 @@ -56,12 +57,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 @@ -82,16 +83,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 @@ -169,8 +169,9 @@ export class AttributeService<
// 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.
resolve({ ...newInput, ...wrappedInput });
// Combine input and wrapped input to preserve any passed in explicit Input while ensuring that the
// library title ovewrites the original title
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

resolve({ ...newInput, ...wrappedInput, title: newAttributes.title });
Copy link
Contributor

@jloleysens jloleysens Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; This code overall is a little tricksy to follow 😅 it seems a little clearer to me if we did:

          const wrappedInput = (await this.wrapAttributes(newAttributes, true, {
            title: props.newTitle,
          })) as RefType;

perhaps @Dosant has some input (pun intended) too.

return { id: wrappedInput.savedObjectId };
} catch (error) {
reject(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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have the data-test-subj above on line 156, do you need to wrap all of this with the same data-test-subj?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, yup! The data-test-subj from line 156 is for when the panel title is explicitly hidden using the toggle - the one on line 215, on the other hand, is for when the title is anything else (including when it is set to the empty string). This is the only way to be able to capture all titles regardless of whether or not they are explicitly hidden.

<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>
);
}
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
3 changes: 2 additions & 1 deletion test/functional/services/common/test_subjects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,8 @@ export class TestSubjects extends FtrService {
// call clearValue() and type() on the element that is focused after
// clicking on the testSubject
const input = await this.findService.activeElement();
if (clearWithKeyboard === true) {
// if `text` is explicitely set to the empty string, then call clearValueWithKeyboard() rather than clearValue()
if (clearWithKeyboard === true || text === '') {
await input.clearValueWithKeyboard();
} else {
await input.clearValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will share more context here: both clearValueWithKeyboard and clearValue clean the input field, but do it differently. We are expecting clearValue to be a default solution as it kinda native to user actions. But for some input fields/text area across Kibana plugins this default way is not working. And in this case, you can explicitly state to use clearValueWithKeyboard which is using keyboard to clean input.
We want to know all these cases, and that is why you explicitly need to provide an argument in function call.

It would be better not to change default behaviour to keep consistency across all the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I wonder if setCustomPanelTitle did not work properly with title change before?

await this.testSubjects.setValue('customEmbeddablePanelTitleInput', customTitle);

call means it was using the default way to clean the field

Copy link
Contributor Author

@Heenawter Heenawter Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes perfect sense! Thanks for the feedback :) These changes to test_subjects.ts were undone in commit 4ee8a49.

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
162 changes: 162 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,162 @@
/*
* 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very well put together test suite 🔥🔥

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

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 EMPTY_TITLE = '[No Title]';
const NEW_CUSTOM_TITLE = 'Test Custom Title';
const LIBRARY_TITLE_FOR_EMPTY_TESTS = 'Library Title for Empty String';

describe('panel titles', () => {
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');
});
};

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

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(NEW_CUSTOM_TITLE);
const panelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(panelTitle).to.equal(NEW_CUSTOM_TITLE);
await clearUnsavedChanges();
});

it('resetting title on a by reference panel sets it to the library title', async () => {
const BY_REFERENCE_TITLE = 'Reset Title - By Reference';
await dashboardPanelActions.saveToLibrary(BY_REFERENCE_TITLE);
await dashboardPanelActions.setCustomPanelTitle('This should go away');

await dashboardPanelActions.resetCustomPanelTitle();
const resetPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(resetPanelTitle).to.equal(BY_REFERENCE_TITLE);

// unlink so that panel goes back to by value for next tests
await dashboardPanelActions.unlinkFromLibary();
Copy link
Contributor

@jloleysens jloleysens Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests should be fully isolated from each other so that if ordering ever changes everything should still work. This, and other post-test teardown logic should live inside of an after that ensures each test starts from the same point.

Perhaps we can navigate to dashboard home if we need to refresh the page and create helpers for setting up a new panel in before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great feedback, thanks! :)

});

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(NEW_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 prepare for next tests
await PageObjects.dashboard.switchToEditMode();
await dashboardPanelActions.toggleHidePanelTitle();
await PageObjects.dashboard.clickQuickSave();
});

it('linking a by value panel with a custom title to the library will overwrite the custom title with the library title', async () => {
// note that the panel already has a custom title from the previous tests
const BY_REFERENCE_TITLE = 'Test Custom Title on Link';

await PageObjects.dashboard.switchToEditMode();
await dashboardPanelActions.saveToLibrary(BY_REFERENCE_TITLE);
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(BY_REFERENCE_TITLE);
});
});

it('unlinking a by reference panel with a custom title will keep the current title', async () => {
await dashboardPanelActions.setCustomPanelTitle(NEW_CUSTOM_TITLE);
await dashboardPanelActions.unlinkFromLibary();
const newPanelTitle = (await PageObjects.dashboard.getPanelTitles())[0];
expect(newPanelTitle).to.equal(NEW_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);
});
});
}