[Dashboard] Check suggested dashboard title for duplicates on save as #184777

cqliu1 · 2024-06-04T19:31:03Z

Summary

Follow up to #180938.

On Save as, the suggested title should always be a unique title. I've added a check for duplicate titles when generating a suggested new dashboard title. If the dashboards Dashboard A, Dashboard A (1), and Dashboard A (2) already exist, the next suggested dashboard title will be Dashboard A (3). This way the user doesn't end up hitting the duplicate title warning if they want to quickly clone a dashboard without updating the title.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-06-04T19:31:08Z

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

kibanamachine · 2024-06-04T21:56:52Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6214

[❌] test/functional/apps/dashboard/group4/config.ts: 0/100 tests passed.

see run history

fix failing test and improve implementation so fix is clear in diff

kibanamachine · 2024-06-05T17:00:43Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6218

[✅] test/functional/apps/dashboard/group4/config.ts: 100/100 tests passed.

see run history

eokoneyo · 2024-06-07T09:57:47Z

@elasticmachine merge upstream

kibana-ci · 2024-06-07T11:48:16Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: fe1bbad

Failed CI Steps

FTR Configs #11

Test Failures

[job] [logs] FTR Configs #11 / Index Management APIs Inference endpoints "before all" hook in "Inference endpoints"

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`	496.3KB	496.5KB	+157.0B

History

💚 Build #214181 succeeded 469ea33
💔 Build #214002 failed 95b3c47

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

cc @cqliu1 @eokoneyo

ThomThomson

Tested this locally and everything works correctly. Thanks for the quick fix @cqliu1!

ThomThomson · 2024-06-07T15:03:30Z

src/plugins/dashboard/public/dashboard_container/embeddable/api/run_save_functions.tsx

+
+          // increment count until we find a unique title
+          while (
+            !(await checkForDuplicateDashboardTitle({


A possible efficiency improvement for this would be to search for all SO titles like DashboardTitle*, then increment locally with the results. In some cases this could result in many network requests. That said, this seems fairly unlikely, so it isn't a blocker.

merging... this in, as I've opened another PR that addresses this

…elastic#184777) ## Summary Follow up to elastic#180938. On `Save as`, the suggested title should always be a unique title. I've added a check for duplicate titles when generating a suggested new dashboard title. If the dashboards `Dashboard A`, `Dashboard A (1)`, and `Dashboard A (2)` already exist, the next suggested dashboard title will be `Dashboard A (3)`. This way the user doesn't end up hitting the duplicate title warning if they want to quickly clone a dashboard without updating the title. ![Jun-04-2024 12-27-59](https://github.com/elastic/kibana/assets/1697105/b1f1ae1a-87d3-4dcd-80cd-395bd6ee9800) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### 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) --------- Co-authored-by: Eyo Okon Eyo <eyo.eyo@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary follow up to #184777 (comment) Previously, if a user has cloned a dashboard N times, in the worst case that the next duplication attempt is initiated from the initial dashboard N network calls would be required to find a unique title for the next uniquely titled clone. This update short circuits that process by querying for the last created dashboard, getting it's duplicationId, increment it and proposing it as the next dashboard duplicate title, with this approach there's still a chance we might suggest a dashboard title that's been claimed if the user modifies the dashboard title to a higher duplication Id after creation, especially that we are querying for the last created and this is situation we can't correct for, the resolution for the appropriate title in that case would be on the user. ## How to test - Create couple of duplicated dashboards, the titles should follow the normal increment progression ie. n+1 - Create some that skip the expected pattern by any increment of choice, on attempting to duplicate this dashboard the suggested title should be previous + 1 - Choose to duplicate any previously created dashboard the suggested title should be the increment of the last duplicated dashboard ### 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  --------- Co-authored-by: Catherine Liu <catherine.liu@elastic.co>

…5056) ## Summary follow up to elastic#184777 (comment) Previously, if a user has cloned a dashboard N times, in the worst case that the next duplication attempt is initiated from the initial dashboard N network calls would be required to find a unique title for the next uniquely titled clone. This update short circuits that process by querying for the last created dashboard, getting it's duplicationId, increment it and proposing it as the next dashboard duplicate title, with this approach there's still a chance we might suggest a dashboard title that's been claimed if the user modifies the dashboard title to a higher duplication Id after creation, especially that we are querying for the last created and this is situation we can't correct for, the resolution for the appropriate title in that case would be on the user. ## How to test - Create couple of duplicated dashboards, the titles should follow the normal increment progression ie. n+1 - Create some that skip the expected pattern by any increment of choice, on attempting to duplicate this dashboard the suggested title should be previous + 1 - Choose to duplicate any previously created dashboard the suggested title should be the increment of the last duplicated dashboard ### 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  --------- Co-authored-by: Catherine Liu <catherine.liu@elastic.co>

Check suggested new dashboard title for duplicates on save as

0ae0605

cqliu1 requested a review from a team as a code owner June 4, 2024 19:31

kibanamachine added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Jun 4, 2024

Add clone test for unique dashboard title suggestion

95b3c47

cqliu1 added the release_note:skip Skip the PR/issue when compiling release notes label Jun 4, 2024

cqliu1 self-assigned this Jun 4, 2024

eokoneyo and others added 2 commits June 5, 2024 13:10

fix failing test and improve implementation so fix is clear in diff

ce1d603

Merge pull request #22 from eokoneyo/suggest-unique-title-on-save-as

469ea33

fix failing test and improve implementation so fix is clear in diff

eokoneyo self-assigned this Jun 6, 2024

Merge branch 'main' into dashboard/suggest-unique-title-on-save-as

fe1bbad

ThomThomson approved these changes Jun 7, 2024

View reviewed changes

eokoneyo mentioned this pull request Jun 8, 2024

Improve logic for dashboard duplication title computation #185056

Merged

1 task

eokoneyo merged commit 229b3e8 into elastic:main Jun 8, 2024
18 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jun 8, 2024

cqliu1 deleted the dashboard/suggest-unique-title-on-save-as branch June 12, 2024 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Check suggested dashboard title for duplicates on save as #184777

[Dashboard] Check suggested dashboard title for duplicates on save as #184777

cqliu1 commented Jun 4, 2024 •

edited

Loading

elasticmachine commented Jun 4, 2024

kibanamachine commented Jun 4, 2024

kibanamachine commented Jun 5, 2024

eokoneyo commented Jun 7, 2024

kibana-ci commented Jun 7, 2024

ThomThomson left a comment

ThomThomson Jun 7, 2024

eokoneyo Jun 8, 2024

[Dashboard] Check suggested dashboard title for duplicates on save as #184777

[Dashboard] Check suggested dashboard title for duplicates on save as #184777

Conversation

cqliu1 commented Jun 4, 2024 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Jun 4, 2024

kibanamachine commented Jun 4, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6214

kibanamachine commented Jun 5, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6218

eokoneyo commented Jun 7, 2024

kibana-ci commented Jun 7, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

ThomThomson left a comment

Choose a reason for hiding this comment

ThomThomson Jun 7, 2024

Choose a reason for hiding this comment

eokoneyo Jun 8, 2024

Choose a reason for hiding this comment

cqliu1 commented Jun 4, 2024 •

edited

Loading