Improve logic for dashboard duplication title computation #185056

eokoneyo · 2024-06-08T00:53:51Z

Summary

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

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2024-06-08T00:53:54Z

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

ThomThomson

This is looking great so far! Especially nice to see the functional and jest test additions.

Left a few code quality nits and some other comments.

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

...public/services/dashboard_content_management/lib/check_for_duplicate_dashboard_title.test.ts

...oard/public/services/dashboard_content_management/lib/check_for_duplicate_dashboard_title.ts

ThomThomson · 2024-06-11T19:14:47Z

...oard/public/services/dashboard_content_management/lib/check_for_duplicate_dashboard_title.ts

+
+  if (isMatchedResultInvalid) {
+    ({ hits: searchMatchPageHits, pagination: searchMatchPaginationObject } =
+      await contentManagement.client.search<


Why exactly do we need to run the search again? Or use a cursor? It seems to me this could work quite well with one search of ~20 or so Dashboards with no cursor where we simply loop through the results to find the highest number and increment that.

If we miss and accidentally return a number that isn't unique, nothing really breaks, the user just has to increment it themselves.

Before switching the previous implementation, I'd used a cursor so we are able to fetch the last page of the search so we can pick the last created dashboard without having to loop multiple times. I opted for this approach because if we have a situation where the duplicated dashboard count exceeds search limit (right now it's 10) the returned page would not give us a way to determine the highest number without performing multiple loops.

I have switched the implementation though now so we are able to sort and simply pick the the last incremented dashboard.

I think that's okay. If the user has more than 10 Dashboards with the same basic title structure, it's completely okay if this doesn't recommend a unique title in ~5% or less of cases. Remember that the behaviour we're replacing simply told the user their title wasn't unique and let them figure out how to make it unique.

...oard/public/services/dashboard_content_management/lib/check_for_duplicate_dashboard_title.ts

eokoneyo · 2024-06-13T08:58:02Z

src/plugins/dashboard/common/content_management/v2/cm_services.ts

+export const dashboardSearchOptionsSchema = dashboardSearchOptionsSchemaV1.extends(
+  {
+    sort: schema.maybe(
+      schema.object({
+        sortField: searchOptionsSchemas.sortField,
+        sortOrder: searchOptionsSchemas.sortOrder,
+      })
+    ),
+  },
+  { unknowns: 'ignore' }
+);
+


I've added this search option in V2, as it's a backward compatible change and also opt-in I'm not sure if this warrants a V3

eokoneyo · 2024-06-13T09:02:15Z

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

+          const [baseTitle] = extractTitleAndCount(newTitle);
+
+          const { hits: recentlyCreatedDashboards } = await findDashboards.search({
+            size: 1,
+            search: baseTitle,
+            options: {
+              onlyTitle: true,
+              sort: {
+                sortField: 'created_at',
+                sortOrder: 'desc',
+              },
+            },
+          });
+
+          const [, mostRecentDuplicationId] = extractTitleAndCount(
+            recentlyCreatedDashboards[0].attributes.title
+          );
+
+          let copyCount = mostRecentDuplicationId + 1;
+
          newTitle = `${baseTitle} (${copyCount})`;


Request the last created dashboard, and use it's duplication Id to create the proposed duplicate title.

…itle

ThomThomson

Looking at this PR again, it seems like we're adding a lot of difficult to maintain code for very dubious user benefit.

All we're doing here is recommending a new title that might be unique. The consequences of accidentally returning a title that isn't unique are very low stakes. In that case the user just needs to write a unique title themselves. That tells me that this code does not need to work perfectly and a "good enough" approach is warranted.

I'd recommend removing almost all of the complexity here - especially the changes to the schema and content management, and building this functionality to cover 90% of the ways it will be used (duplicating a Dashboard once or twice).

… check

…mputation

cqliu1 · 2024-06-17T18:19:53Z

@ThomThomson I'm taking over this PR while Eyo's out. I've removed all of the content management changes and simplified the logic for the duplicate title check. Only one dashboard search request is fired when the save modal renders to generate a best attempt at a unique title. It reliably finds a unique title until you have 20+ copies of the same dashboard with the same base title, and only struggles to find a unique title suggestion if you're duplicating one of the older duplicates with a lower count. It should still consistently suggest a unique title when duplicating one of the newer duplicate dashboards with a higher count. This should sufficiently cover most of the scenarios a user will run into.

ThomThomson

Changes LGTM! Tested locally in chrome, and it's working great. Left a few nits

ThomThomson · 2024-06-17T19:33:29Z

...oard/public/services/dashboard_content_management/lib/check_for_duplicate_dashboard_title.ts

@@ -30,6 +35,7 @@ export async function checkForDuplicateDashboardTitle(
    lastSavedTitle,
    onTitleDuplicate,
    isTitleDuplicateConfirmed,
+    searchLimit = 20,


Why does this need to be an argument? It seems like it can just be hardcoded in the function more simply.

ThomThomson · 2024-06-17T19:34:13Z

...oard/public/services/dashboard_content_management/lib/check_for_duplicate_dashboard_title.ts

  const { hits } = await contentManagement.client.search<
    DashboardCrudTypes['SearchIn'],
    DashboardCrudTypes['SearchOut']
  >({
    contentTypeId: DASHBOARD_CONTENT_ID,
    query: {
-      text: title ? `${title}*` : undefined,
-      limit: 10,
+      text: title ? `${baseDashboardName}*` : undefined,


Instead of falling back to undefined here, can we just cancel the whole operation if the title is undefined?

It shouldn't be undefined, but it's possible to receive an empty string. We can certainly return early and skip the whole check.

ThomThomson · 2024-06-17T19:34:46Z

...oard/public/services/dashboard_content_management/lib/check_for_duplicate_dashboard_title.ts

-      limit: 10,
+      text: title ? `${baseDashboardName}*` : undefined,
+      limit: searchLimit,
+      cursor: String(duplicationId ? Math.ceil(duplicationId / searchLimit) : 1),


Do we need a cursor still?

Not necessarily. Setting the cursor is only useful if there are more than the 20 duplicate dashboards, but even without this, we'll still come up with a good enough suggestion.

ThomThomson · 2024-06-17T19:48:53Z

test/functional/apps/dashboard/group4/dashboard_clone.ts

@@ -49,5 +49,20 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
      await PageObjects.dashboard.gotoDashboardLandingPage();
      await listingTable.searchAndExpectItemsCount('dashboard', `${dashboardName} (2)`, 1);
    });
+
+    it('Clone should always increment from the last duplicated dashboard with a unique title', async function () {


Thanks for adding a functional test!

kibana-ci · 2024-06-18T16:30:40Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: b2962d2

Failed CI Steps

FTR Configs #6

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`	489.3KB	489.6KB	+310.0B

History

💔 Build #216277 failed ad498fd
💚 Build #216225 succeeded 16a01b1
💔 Build #216206 failed 9f077ab
💔 Build #215707 failed c5ba26c
💔 Build #215563 failed 6b7bb62

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

cc @eokoneyo

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

eokoneyo added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes Project:Dashboard Usability Related to the Dashboard Usability initiative labels Jun 8, 2024

eokoneyo self-assigned this Jun 8, 2024

eokoneyo requested a review from a team as a code owner June 8, 2024 00:53

eokoneyo force-pushed the chore/improve-dashboard-duplication-title-computation branch 4 times, most recently from e7270a6 to fe490a0 Compare June 11, 2024 08:05

ThomThomson reviewed Jun 11, 2024

View reviewed changes

eokoneyo force-pushed the chore/improve-dashboard-duplication-title-computation branch from 75676e8 to 6b7bb62 Compare June 12, 2024 18:12

eokoneyo commented Jun 13, 2024

View reviewed changes

eokoneyo force-pushed the chore/improve-dashboard-duplication-title-computation branch 2 times, most recently from a7444cd to e5b8ce2 Compare June 13, 2024 09:49

eokoneyo added 6 commits June 13, 2024 11:54

improve computation of dashboard duplicate title

fc31efc

add ftr test

219485a

resolve PR feedback

83a2306

include support to sort dashboard items by field for search

99d2738

leverage sort options to suggest collision free title

30db174

provide and leverage speculative dashboard collision free duplicate t…

c5ba26c

…itle

eokoneyo force-pushed the chore/improve-dashboard-duplication-title-computation branch from e5b8ce2 to c5ba26c Compare June 13, 2024 09:55

ThomThomson reviewed Jun 13, 2024

View reviewed changes

cqliu1 added 3 commits June 17, 2024 10:26

Revert content management schema changes and simplify duplicate title…

9f077ab

… check

Start with better base suggestion

576309f

Merge branch 'main' into chore/improve-dashboard-duplication-title-co…

16a01b1

…mputation

ThomThomson approved these changes Jun 17, 2024

View reviewed changes

Apply feedback

ad498fd

cqliu1 enabled auto-merge (squash) June 18, 2024 01:16

Fix check for duplicate dashboard title tests

b2962d2

cqliu1 merged commit 641a71b into elastic:main Jun 18, 2024
26 checks passed

kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels Jun 18, 2024

eokoneyo deleted the chore/improve-dashboard-duplication-title-computation branch July 1, 2024 11:27

rshen91 mentioned this pull request Aug 20, 2024

[Dashboard] Interactive Managed Dashboard Popover vs badge #189404

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve logic for dashboard duplication title computation #185056

Improve logic for dashboard duplication title computation #185056

eokoneyo commented Jun 8, 2024 •

edited

Loading

elasticmachine commented Jun 8, 2024

ThomThomson left a comment

ThomThomson Jun 11, 2024

eokoneyo Jun 12, 2024

ThomThomson Jun 13, 2024

eokoneyo Jun 13, 2024

eokoneyo Jun 13, 2024

ThomThomson left a comment •

edited

Loading

cqliu1 commented Jun 17, 2024 •

edited

Loading

ThomThomson left a comment

ThomThomson Jun 17, 2024

ThomThomson Jun 17, 2024

cqliu1 Jun 18, 2024

ThomThomson Jun 17, 2024

cqliu1 Jun 18, 2024

ThomThomson Jun 17, 2024

kibana-ci commented Jun 18, 2024

Improve logic for dashboard duplication title computation #185056

Improve logic for dashboard duplication title computation #185056

Conversation

eokoneyo commented Jun 8, 2024 • edited Loading

Summary

How to test

Checklist

elasticmachine commented Jun 8, 2024

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomThomson left a comment • edited Loading

Choose a reason for hiding this comment

cqliu1 commented Jun 17, 2024 • edited Loading

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jun 18, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

History

eokoneyo commented Jun 8, 2024 •

edited

Loading

ThomThomson left a comment •

edited

Loading

cqliu1 commented Jun 17, 2024 •

edited

Loading