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

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Dec 8, 2021

Closes #118624
Relies on #119913

Summary

Originally, blank panel titles resulted in issues when panel states were compared in diff_dashboard_state.ts because each of the following panel object states was considered to be different:

  • panel object has title = ''
  • panel object has title = undefined
  • panel object does not contain the title key

This caused unexpected behavior when dealing with empty titles. To fix this, I started by creating a special case inside of diff_dashboard_state.ts for comparing panels so that each one of the above scenarios is seen as equal.

However, as part of this bug fix, it was noticed that the behaviour when a panel is linked/unlinked to/from the library was inconsistent - to fix this, the panel's title always gets overwritten when it is linked to the library and the attribute title is dropped when a panel is unlinked (since by value panels do not have attribute titles).

To hopefully prevent these types of issues in the future, a full test suite related to panel titles was added.

Video of Fix in Action

Dashboard-FixedBlankPanelSave.mp4

Flaky Test
To ensure that the new test suite was not flaky, I ran it 100 times and it passed every single time.

TitleTestSuite_FlakyTest

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Heenawter Heenawter changed the title Fix blank title save bug 2021 11 18 v2 [Dashboard] Fix blank panel save and display issue. Dec 8, 2021
@Heenawter Heenawter self-assigned this Dec 8, 2021
@Heenawter Heenawter added Feature:Dashboard Dashboard related features impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Dec 8, 2021
@Heenawter Heenawter force-pushed the fix-blank-title-save-bug_2021-11-18_v2 branch from 1f4c2fd to 2a475ff Compare December 8, 2021 19:45
@Heenawter Heenawter force-pushed the fix-blank-title-save-bug_2021-11-18_v2 branch from e142cd7 to 2a475ff Compare December 9, 2021 15:59
@Heenawter Heenawter marked this pull request as ready for review December 9, 2021 16:21
@Heenawter Heenawter requested review from a team as code owners December 9, 2021 16:21
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Functional tests

Great to see the flaky test runner results! I have no input at all about the functional test suite. It looks good, runs well, is well commented, and doesn't repeat itself!

I tested this locally by:

  • Setting the title to a blank string and saving - no unsaved changes ✔️
  • Giving a custom title to a panel and saving - no unsaved changes ✔️
  • Adding a by value panel to the library - custom title is default in modal - custom title is replaced with library title ✔️
  • Unlinking a panel from the library - library title is moved to panel title, resetting defaults to blank title ✔️
  • Editing a panel title causes unsaved changes - resetting that title causes unsaved changes to disappear ✔️
  • I also imported a dashboard from 7.9 with a few titles hidden. I can confirm that titles that were hidden in 7.9 are still hidden after being imported in this PR. ✔️✔️✔️

I ran through all of the above tests with Lens, Maps & Visualize panels, and everything looks good except for one problem with visualize.

Visualize unlink issue

Because visualize doesn't store its by value information under the attributes key, unsetting explicitInput.attributes.title doesn't do anything. Therefore unlinking a visualize by reference panel, then saving the dashboard will not clear the unsaved changes badge. This should be a pretty quick fix!

Overall, everything is looking awesome, and we should be able to merge this soon. I also left one tiny nit.

Thanks for tackling this as your first major PR!

// 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.

👌

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.

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.

💪

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Dec 9, 2021
@Heenawter Heenawter requested a review from a team as a code owner December 9, 2021 20:47
@@ -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 } : {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When adding a by reference Visualize panel, the savedSearchId key does not exist in this.data. However, when unlinking, this key was injected in to this.data via this original line - i.e. this.data.savedSearchId = undefined | string. Unfortunately, our current deep comparison of panels when saving treats a key not existing and key = undefined as unequal - this prevented the unsaved changes badge from being cleared.

To prevent this, the savedSearchID key only gets added if it has a value.

@Heenawter
Copy link
Contributor Author

Visualize unlink issue

Because visualize doesn't store its by value information under the attributes key, unsetting explicitInput.attributes.title doesn't do anything. Therefore unlinking a visualize by reference panel, then saving the dashboard will not clear the unsaved changes badge. This should be a pretty quick fix!

This is fixed via commit 84afa07 ☺️

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

With the slight changes to the visualize serialization method, there are no longer any problems with comparisons on unlink / add to library.

Everything LGTM!

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Tested locally and behaviour LGTM. I left a couple of comments on how I think code could be made clearer and restructuring of functional tests - I won't block on those though 👍🏻 great work @Heenawter

Comment on lines 80 to 81
// 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! :)

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
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.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

VisEditors changes LGTM

@LeeDr
Copy link
Contributor

LeeDr commented Dec 13, 2021

For version labels, v7.16.0 has already shipped and 7.16.1 is probably going to be released today.
It looks like the next release will be 7.17.0 (but always a possibility of a 7.16.2).

@Heenawter Heenawter force-pushed the fix-blank-title-save-bug_2021-11-18_v2 branch from f99cdf1 to 77c622f Compare December 14, 2021 15:36
@Heenawter Heenawter enabled auto-merge (squash) December 14, 2021 23:23
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 275.1KB 275.4KB +233.0B
visualizations 59.4KB 59.4KB +32.0B
total +265.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dashboard 65.3KB 65.4KB +65.0B
embeddable 63.8KB 63.9KB +130.0B
total +195.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Heenawter

@Heenawter Heenawter merged commit b6013d1 into elastic:main Dec 15, 2021
ecezalp pushed a commit to ecezalp/kibana that referenced this pull request Dec 15, 2021
* 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.
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 17, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 120815 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 120815 or prevent reminders by adding the backport:skip label.

@Heenawter Heenawter added auto-backport Deprecated - use backport:version if exact versions are needed and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Dec 20, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
8.0 Commit could not be cherrypicked due to conflicts
7.17 Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 120815

Heenawter added a commit to Heenawter/kibana that referenced this pull request Dec 20, 2021
* 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.
# Conflicts:
#	src/plugins/embeddable/public/lib/attribute_service/attribute_service.tsx
Heenawter added a commit to Heenawter/kibana that referenced this pull request Dec 20, 2021
* 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.
# Conflicts:
#	src/plugins/embeddable/public/lib/attribute_service/attribute_service.tsx
Heenawter added a commit that referenced this pull request Dec 20, 2021
* 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.
# Conflicts:
#	src/plugins/embeddable/public/lib/attribute_service/attribute_service.tsx
@Heenawter Heenawter deleted the fix-blank-title-save-bug_2021-11-18_v2 branch December 20, 2021 17:54
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
* 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.
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
* 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.
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Feb 1, 2022
* 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.

(cherry picked from commit b6013d1)

# Conflicts:
#	src/plugins/embeddable/public/lib/attribute_service/attribute_service.tsx
ThomThomson added a commit that referenced this pull request Feb 2, 2022
…124295)

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

* 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.

(cherry picked from commit b6013d1)

# Conflicts:
#	src/plugins/embeddable/public/lib/attribute_service/attribute_service.tsx

* replace lens basic with esarchiver

* remove describe.only

Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
Heenawter added a commit that referenced this pull request May 5, 2023
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)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 5, 2023
Closes elastic#156539
Closes elastic#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](elastic#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)

(cherry picked from commit ce7ef40)
kibanamachine added a commit that referenced this pull request May 5, 2023
…#156871)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[Dashboard] Fix "Unlink from library" panel title bug
(#156589)](#156589)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Hannah
Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-05-05T14:54:39Z","message":"[Dashboard]
Fix \"Unlink from library\" panel title bug (#156589)\n\nCloses
https://github.com/elastic/kibana/issues/156539\r\nCloses
https://github.com/elastic/kibana/issues/156544\r\n\r\n##
Summary\r\n\r\n\r\nAs part of investigating the attached flaky test
suite, I realized that\r\nthe flakiness was because we weren't waiting
long enough for a panel to\r\nbe added and/or removed from the library -
so, I added a check to ensure\r\nthe library notification appears in
`saveToLibrary`, and similarly I\r\nadded a check to ensure that the
library notification **disappears**
in\r\n`unlinkFromLibary`.\r\n\r\nHowever, after adding these extra
checks, the following test started
to\r\nfail:\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/23a45bde21b36be49a1174b9bde1fc340de67534/x-pack/test/functional/apps/dashboard/group2/panel_titles.ts#L148-L155\r\n\r\nWay,
way, way back in `8.1`, one of my
[first\r\nPRs](#120815) was meant
to fix\r\nsome problems with dashboard panel titles - as part of this, I
was\r\n**supposed** to make sure that, if a by-reference panel is given
a\r\ncustom panel title, the title should remain the same after
unlinking\r\n(i.e. it should remain as the custom title rather than
resetting to the\r\nby-reference title). So, I added the above test to
verify this\r\nbehaviour.\r\n\r\nTurns out, though, that this test had a
flaw - because we weren't\r\nwaiting long enough for the panel to
actually be disconnected from the\r\nlibrary, this test was only passing
because it was grabbing and\r\ncomparing titles **before** the unlink
was complete - so, even though\r\nthe title actually **was** being reset
back to the original library\r\ntitle, this test did not catch this bug.
This has been the case since\r\n`8.1` when this test was introduced -
not sure how it was missed, but we\r\nnever actually fixed the bug where
dashboard panel titles are getting\r\nreset when unlinking from the
library.\r\n\r\nSo, this PR actually accomplishes two things:\r\n1) It
fixes the flakiness of the attached tests by adding the extra\r\nlibrary
notification
checks:\r\n<a\r\nhref=\"https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2217\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/236044233-8a07aadc-8a55-40f2-9ed2-798a91501f68.png\"/></a>\r\n2)
It ensures that, if a by-reference panel has a custom
title,\r\nunlinking it from the library will not impact that
title.\r\n\r\n###
Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/236062484-b1cedc47-cb19-4273-aec7-17d24b55953b.mov\r\n\r\n\r\n###
After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/236062862-aace4f09-d007-401a-ba57-b9b74fda0b33.mov\r\n\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] This was
checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n###
For maintainers\r\n\r\n- [ ] This was checked for breaking API changes
and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ce7ef40d9a79313be64057af6fb7f143a44a434a","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Embedding","release_note:fix","Team:Presentation","loe:days","impact:high","backport:prev-minor","v8.9.0"],"number":156589,"url":"https://github.com/elastic/kibana/pull/156589","mergeCommit":{"message":"[Dashboard]
Fix \"Unlink from library\" panel title bug (#156589)\n\nCloses
https://github.com/elastic/kibana/issues/156539\r\nCloses
https://github.com/elastic/kibana/issues/156544\r\n\r\n##
Summary\r\n\r\n\r\nAs part of investigating the attached flaky test
suite, I realized that\r\nthe flakiness was because we weren't waiting
long enough for a panel to\r\nbe added and/or removed from the library -
so, I added a check to ensure\r\nthe library notification appears in
`saveToLibrary`, and similarly I\r\nadded a check to ensure that the
library notification **disappears**
in\r\n`unlinkFromLibary`.\r\n\r\nHowever, after adding these extra
checks, the following test started
to\r\nfail:\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/23a45bde21b36be49a1174b9bde1fc340de67534/x-pack/test/functional/apps/dashboard/group2/panel_titles.ts#L148-L155\r\n\r\nWay,
way, way back in `8.1`, one of my
[first\r\nPRs](#120815) was meant
to fix\r\nsome problems with dashboard panel titles - as part of this, I
was\r\n**supposed** to make sure that, if a by-reference panel is given
a\r\ncustom panel title, the title should remain the same after
unlinking\r\n(i.e. it should remain as the custom title rather than
resetting to the\r\nby-reference title). So, I added the above test to
verify this\r\nbehaviour.\r\n\r\nTurns out, though, that this test had a
flaw - because we weren't\r\nwaiting long enough for the panel to
actually be disconnected from the\r\nlibrary, this test was only passing
because it was grabbing and\r\ncomparing titles **before** the unlink
was complete - so, even though\r\nthe title actually **was** being reset
back to the original library\r\ntitle, this test did not catch this bug.
This has been the case since\r\n`8.1` when this test was introduced -
not sure how it was missed, but we\r\nnever actually fixed the bug where
dashboard panel titles are getting\r\nreset when unlinking from the
library.\r\n\r\nSo, this PR actually accomplishes two things:\r\n1) It
fixes the flakiness of the attached tests by adding the extra\r\nlibrary
notification
checks:\r\n<a\r\nhref=\"https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2217\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/236044233-8a07aadc-8a55-40f2-9ed2-798a91501f68.png\"/></a>\r\n2)
It ensures that, if a by-reference panel has a custom
title,\r\nunlinking it from the library will not impact that
title.\r\n\r\n###
Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/236062484-b1cedc47-cb19-4273-aec7-17d24b55953b.mov\r\n\r\n\r\n###
After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/236062862-aace4f09-d007-401a-ba57-b9b74fda0b33.mov\r\n\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] This was
checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n###
For maintainers\r\n\r\n- [ ] This was checked for breaking API changes
and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ce7ef40d9a79313be64057af6fb7f143a44a434a"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156589","number":156589,"mergeCommit":{"message":"[Dashboard]
Fix \"Unlink from library\" panel title bug (#156589)\n\nCloses
https://github.com/elastic/kibana/issues/156539\r\nCloses
https://github.com/elastic/kibana/issues/156544\r\n\r\n##
Summary\r\n\r\n\r\nAs part of investigating the attached flaky test
suite, I realized that\r\nthe flakiness was because we weren't waiting
long enough for a panel to\r\nbe added and/or removed from the library -
so, I added a check to ensure\r\nthe library notification appears in
`saveToLibrary`, and similarly I\r\nadded a check to ensure that the
library notification **disappears**
in\r\n`unlinkFromLibary`.\r\n\r\nHowever, after adding these extra
checks, the following test started
to\r\nfail:\r\n\r\n\r\nhttps://github.com/elastic/kibana/blob/23a45bde21b36be49a1174b9bde1fc340de67534/x-pack/test/functional/apps/dashboard/group2/panel_titles.ts#L148-L155\r\n\r\nWay,
way, way back in `8.1`, one of my
[first\r\nPRs](#120815) was meant
to fix\r\nsome problems with dashboard panel titles - as part of this, I
was\r\n**supposed** to make sure that, if a by-reference panel is given
a\r\ncustom panel title, the title should remain the same after
unlinking\r\n(i.e. it should remain as the custom title rather than
resetting to the\r\nby-reference title). So, I added the above test to
verify this\r\nbehaviour.\r\n\r\nTurns out, though, that this test had a
flaw - because we weren't\r\nwaiting long enough for the panel to
actually be disconnected from the\r\nlibrary, this test was only passing
because it was grabbing and\r\ncomparing titles **before** the unlink
was complete - so, even though\r\nthe title actually **was** being reset
back to the original library\r\ntitle, this test did not catch this bug.
This has been the case since\r\n`8.1` when this test was introduced -
not sure how it was missed, but we\r\nnever actually fixed the bug where
dashboard panel titles are getting\r\nreset when unlinking from the
library.\r\n\r\nSo, this PR actually accomplishes two things:\r\n1) It
fixes the flakiness of the attached tests by adding the extra\r\nlibrary
notification
checks:\r\n<a\r\nhref=\"https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2217\"><img\r\nsrc=\"https://user-images.githubusercontent.com/8698078/236044233-8a07aadc-8a55-40f2-9ed2-798a91501f68.png\"/></a>\r\n2)
It ensures that, if a by-reference panel has a custom
title,\r\nunlinking it from the library will not impact that
title.\r\n\r\n###
Before\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/236062484-b1cedc47-cb19-4273-aec7-17d24b55953b.mov\r\n\r\n\r\n###
After\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/236062862-aace4f09-d007-401a-ba57-b9b74fda0b33.mov\r\n\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] This was
checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n###
For maintainers\r\n\r\n- [ ] This was checked for breaking API changes
and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"ce7ef40d9a79313be64057af6fb7f143a44a434a"}}]}]
BACKPORT-->

Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
smith pushed a commit to smith/kibana that referenced this pull request May 5, 2023
Closes elastic#156539
Closes elastic#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](elastic#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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Blank Panel Title Save and Display Issue
9 participants