Skip to content

Commit

Permalink
[Dashboard] Fix "Unlink from library" panel title bug (#156589)
Browse files Browse the repository at this point in the history
Closes #156539
Closes #156544

## Summary


As part of investigating the attached flaky test suite, I realized that
the flakiness was because we weren't waiting long enough for a panel to
be added and/or removed from the library - so, I added a check to ensure
the library notification appears in `saveToLibrary`, and similarly I
added a check to ensure that the library notification **disappears** in
`unlinkFromLibary`.

However, after adding these extra checks, the following test started to
fail:


https://github.com/elastic/kibana/blob/23a45bde21b36be49a1174b9bde1fc340de67534/x-pack/test/functional/apps/dashboard/group2/panel_titles.ts#L148-L155

Way, way, way back in `8.1`, one of my [first
PRs](#120815) was meant to fix
some problems with dashboard panel titles - as part of this, I was
**supposed** to make sure that, if a by-reference panel is given a
custom panel title, the title should remain the same after unlinking
(i.e. it should remain as the custom title rather than resetting to the
by-reference title). So, I added the above test to verify this
behaviour.

Turns out, though, that this test had a flaw - because we weren't
waiting long enough for the panel to actually be disconnected from the
library, this test was only passing because it was grabbing and
comparing titles **before** the unlink was complete - so, even though
the title actually **was** being reset back to the original library
title, this test did not catch this bug. This has been the case since
`8.1` when this test was introduced - not sure how it was missed, but we
never actually fixed the bug where dashboard panel titles are getting
reset when unlinking from the library.

So, this PR actually accomplishes two things:
1) It fixes the flakiness of the attached tests by adding the extra
library notification checks:
<a
href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2217"><img
src="https://user-images.githubusercontent.com/8698078/236044233-8a07aadc-8a55-40f2-9ed2-798a91501f68.png"/></a>
2) It ensures that, if a by-reference panel has a custom title,
unlinking it from the library will not impact that title.

### Before


https://user-images.githubusercontent.com/8698078/236062484-b1cedc47-cb19-4273-aec7-17d24b55953b.mov


### After


https://user-images.githubusercontent.com/8698078/236062862-aace4f09-d007-401a-ba57-b9b74fda0b33.mov



### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
Heenawter authored May 5, 2023
1 parent 01fd184 commit ce7ef40
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,12 @@ export class AttributeService<
return input as ValType;
}
const { attributes } = await this.unwrapAttributes(input);
const libraryTitle = attributes.title;
const { savedObjectId, ...originalInputToPropagate } = input;

return {
...originalInputToPropagate,
// by value visualizations should not have default titles and/or descriptions
...{ attributes: omit(attributes, ['title', 'description']) },
title: libraryTitle,
} as unknown as ValType;
};

Expand Down
9 changes: 9 additions & 0 deletions test/functional/services/dashboard/panel_actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const DASHBOARD_TOP_OFFSET = 96 + 105; // 96 for Kibana navigation bar + 105 for

export class DashboardPanelActionsService extends FtrService {
private readonly log = this.ctx.getService('log');
private readonly retry = this.ctx.getService('retry');
private readonly browser = this.ctx.getService('browser');
private readonly inspector = this.ctx.getService('inspector');
private readonly testSubjects = this.ctx.getService('testSubjects');
Expand Down Expand Up @@ -221,6 +222,9 @@ export class DashboardPanelActionsService extends FtrService {
await this.clickContextMenuMoreItem();
}
await this.testSubjects.click(UNLINK_FROM_LIBRARY_TEST_SUBJ);
await this.testSubjects.waitForDeleted(
'embeddablePanelNotification-ACTION_LIBRARY_NOTIFICATION'
);
}

async saveToLibrary(newTitle: string, parent?: WebElementWrapper) {
Expand All @@ -235,6 +239,11 @@ export class DashboardPanelActionsService extends FtrService {
clearWithKeyboard: true,
});
await this.testSubjects.click('confirmSaveSavedObjectButton');
await this.retry.try(async () => {
await this.testSubjects.existOrFail(
'embeddablePanelNotification-ACTION_LIBRARY_NOTIFICATION'
);
});
}

async expectExistsPanelAction(testSubject: string, title?: string) {
Expand Down
4 changes: 1 addition & 3 deletions x-pack/test/functional/apps/dashboard/group2/panel_titles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
});
});

// FLAKY: https://github.com/elastic/kibana/issues/156539
// FLAKY: https://github.com/elastic/kibana/issues/156544
describe.skip('by reference', () => {
describe('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.customizePanel();
await dashboardCustomizePanel.setCustomPanelTitle(CUSTOM_TITLE);
Expand Down
2 changes: 0 additions & 2 deletions x-pack/test/functional/apps/lens/group4/dashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
it('save lens panel to embeddable library', async () => {
const originalPanel = await testSubjects.find('embeddablePanelHeading-lnsPieVis');
await panelActions.saveToLibrary('lnsPieVis - copy', originalPanel);
await testSubjects.click('confirmSaveSavedObjectButton');
await testSubjects.existOrFail('addPanelToLibrarySuccess');

const updatedPanel = await testSubjects.find('embeddablePanelHeading-lnsPieVis-copy');
const libraryActionExists = await testSubjects.descendantExists(
Expand Down

0 comments on commit ce7ef40

Please sign in to comment.