[Portable Dashboards] Remove Saved Object Class by ThomThomson · Pull Request #138774 · elastic/kibana

ThomThomson · 2022-08-12T22:33:25Z

Summary

This PR executes a number of large changes to the way that Dashboards are loaded from Saved Objects. This is required as preparation for the first phase of the Portable Dashboards project. Merging this PR will complete all remaining prep work items and allow us to move on to implementation.

What does this PR do?

Removes the Dashboard Saved Object Class as well as the Saved Dashboard Loader, as well the implementation of the generic Saved Object Loader which was moved into the Dashboard plugin in Move saved objects loader to dashboard plugin #124708 because we were the only consumer. @elastic/ml-ui the codeowners review requirement is only due to a type change.
Introduces a new Dashboard Saved Object Service using the services abstraction to load, find, and save Dashboard saved objects. Finishes up some small todos left from [Dashboard] Services abstraction #139145 by removing all instances of useKibana, and dashboardAppState.
Removes clientside migrations Fixes [Portable Dashboards] Remove Legacy Clientside Migrations #137200 by removing all of the client side migration code that runs on every dashboard startup. Instead, any dashboard panels from before 7.3 will not be loaded into the dashboard state, and a warning toast will show. The functional tests have also been updated to expect this toast.
Reorganizes legacy saved object migrations Places all of the types and migrations related to Dashboard saved object migrations from before 7.3 into their own folder, simplifying the readability of the dashboard migration code. The migrations have no changes.
Standardizes Dashboard Attributes Typing by adding a new type DashboardAttributes which should be always be the latest / current state of the Dashboard Saved Object Attributes. Rather than keeping version information in this type, we should create new types to store the previous versions, maintaining DashboardAttributes as the source of truth for the latest typings.
Deletes dead code including an entire persistable state system implementation and
the unused url library function.
Imports all actions async in order to reduce bundle size, and allow the whole services implementation to be loaded as an async chunk, all dashboard actions are now registered and imported asynchronously.
Adds a few functional test lib functions because dashboard security and spaces tests were using the common object for navigation which made the tests liable to break with the code changes. I introduced functions on the dashboard page object to handle these navigations.

Test Guide

Saving and loading

Make sure to manually save in as many ways as possible:

Quick save
Save as copy
Save as, with rename
Save as, change tags
Save as change description
Clone dashboard

Everything should work correctly

Migrations

~~Export a dashboard from a version before 7.3 and import it into main. All of the saved object migrations should run correctly.~~
Edit: It looks like versioning must go through 7.17.x before being upgraded to 8.x, for both upgrades and saved object imports. So we should be okay just ensuring that imports from 7.17.x work correctly.

Saved Object Resolve

To test the conflict, exactMatch, and aliasMatch outcomes of the savedObjectsClient.resolve function, the results of which are used slightly differently in this PR you should:

Run this PR locally and log in as the system_indices_superuser
Install sample flights data
Load the resolve-staging-data.txt via dev tools
Make the flights data view available in all spaces through the Saved Objects management screen
Navigate to {your Kibana URL}/s/test1/app/dashboards#/view/b348e380-39d1-11ed-88ea-15ffd58ee0d7-test1 and ensure that the dashboard loads properly.
Navigate to {your Kibana URL}/s/test2/app/dashboards#/view/b348e380-39d1-11ed-88ea-15ffd58ee0d7-test2 and you should be redirected to another dashboard with the id b348e380-39d1-11ed-88ea-15ffd58ee0d7-newId
Navigate to {your Kibana URL}/s/test3/app/dashboards#/view/b348e380-39d1-11ed-88ea-15ffd58ee0d7-test3 and the conflict warning should show.

…eDashboard/removeSavedObjectClass

…-fix'

…eDashboard/removeSavedObjectClass

rudolf · 2022-09-05T08:52:06Z

src/plugins/dashboard/common/types.ts

+/**
+ * A saved dashboard panel parsed directly from the Dashboard Attributes panels JSON
+ */
+export type SavedDashboardPanel = SerializableRecord & {


While on the surface having a type that guarantees that data is serialisable is a good thing, we should be very careful with the SerializableRecord or Serializable types. These are really anti-types in that they reduce type safety making code more prone to bugs.

As an example mySavedDashboardPanel.recordDoesNotExist.fieldDoesNotExist = 'some string' would happily pass the type checker. So it really has the same problems as using any.

I know this PR didn't introduce this type to the dashboard code, but if we could limit/reduce it's usage that would improve the quality of the code.

I realised this problem is much bigger and we also have the SavedObjectAttributes type causing similar problems 🙈 https://github.com/elastic/kibana/pull/140099/files

Hey @rudolf, this is a really good point! Let me look into removing any instances of SerializableRecord from Dashboard. I wonder if there are any Typescript extension types that we can use to enforce serializability without introducing that nasty key signature.

I've removed a couple references to SerializableRecord here, but removing all of the SavedObjectAttributes imports from Dashboard is proving to be a big enough project to warrant its own follow-up PR. I have created this issue to track it

…thub.com/ThomThomson/kibana into portableDashboard/removeSavedObjectClass

…eDashboard/removeSavedObjectClass

…ersion lower than 7.3

…eDashboard/removeSavedObjectClass

…-fix'

…thub.com/ThomThomson/kibana into portableDashboard/removeSavedObjectClass

…eDashboard/removeSavedObjectClass

…-fix'

…thub.com/ThomThomson/kibana into portableDashboard/removeSavedObjectClass

…eDashboard/removeSavedObjectClass

elasticmachine · 2022-09-23T17:25:32Z

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

Heenawter · 2022-09-23T19:12:03Z

src/plugins/dashboard/public/application/dashboard_app.tsx

+    screenshotMode: { isScreenshotMode },
    coreContext: { executionContext },
-    data: { search },
    embeddable: { getStateTransfer },
    notifications: { toasts },
-    screenshotMode: { isScreenshotMode },
    settings: { uiSettings },
-    spaces: { getLegacyUrlConflict },
+    data: { search },


Very minor nit: I was trying to keep the services alphabetical when I did the abstraction, but I notice here you're ordering by the number of characters in the line - not sure if we want to be consistent about this? 🤷

I always organize imports and de-structuring by number of characters to make the file look pretty ✨. The way I do it is definitely not something we should standardize in my opinion hahah. So that leaves two options here:

Option 1: We make alphabetical imports / de-structuring our team's standard, and we look into making a prettier / eslint rule to automatically do that for us.

Option 2: We leave it totally up to the engineer who is editing the file.

I'm totally okay with either option! @cqliu1, this is something the 3 of us should probably think about.

I'm also good with either! Definitely something worth discussing so we can all get on the same page, since I know it's come up a few times at this point 👍

src/plugins/dashboard/public/application/dashboard_router.tsx

src/plugins/dashboard/public/application/embeddable/dashboard_container_factory.tsx

src/plugins/dashboard/public/services/embeddable/embeddable.stub.ts

src/plugins/dashboard/public/services/plugin_services.stub.ts

src/plugins/dashboard/public/dashboard_strings.ts

src/plugins/dashboard/public/application/top_nav/show_share_modal.test.tsx

src/plugins/dashboard/public/application/lib/sync_dashboard_container_input.ts

src/plugins/dashboard/public/plugin.tsx

Heenawter

Code review only - looks like this realllly helps clean things up! Left a few nits and questions, but nothing code-specific stood out to me 👍

Will follow up with approval once I test this locally

src/plugins/dashboard/public/application/top_nav/show_share_modal.tsx

src/plugins/dashboard/public/application/top_nav/dashboard_top_nav.tsx

Heenawter

Working on testing this PR as thoroughly as possible, and was able to get stuck in an unsaved changes state via the following steps:

Create a new dashboard and save it with Store time with dashboard off
Click Save as, untick Save as new, set Store time with dashboard to true
Change the dashboard time range - notice that this does not trigger unsaved changes, even though it should 🔥
Go back to Save as, untick Save as new but keep Store time with dashboard as true
Notice you get stuck in unsaved changes and can't get out even if you discard changes 🔥

Screen.Recording.2022-09-23.at.2.54.43.PM.mov

If in step one you save the brand new dashboard with Store time with dashboard on in the first place, the unsaved changes badge seems to respond to time changes as expected - it's only when you go from Store time with dashboard off to on where things get buggy,

Heenawter

Tested this and everything worked great minus the bug discussed in #138774 (review). Will approve once that is resolved so I can do another round of testing.

Saving and loading

For an empty dashboard and for dashboards with content:

Quick save:
- Added and modified controls, renamed panels, moved them around, etc. etc. Everything worked as expected
Save as:
- Saved dashboard with title, description, tags, and time store
- Made sure I could make changes to title, description, tags, and time store by unchecking Save as new dashboard when saving
- Get a warning when trying to Save as with duplicated title - can overwrite it
Clone dashboard:
- Cloned the dashboard and ensured everything got copied as expected, including by reference and by value visualizations (if the dashboard wasn't empty), tags, description, and time store
- Get a warning when trying to clone with duplicated title - can overwrite it

Migrations

Took a dashboard with content from 7.17.3 and imported it
Took an empty dashboard from 7.17.3 and imported it

Saved Object Resolve

Followed the steps outlined and everything worked as expected 👍

darnautov

ML changes LGTM

…eDashboard/removeSavedObjectClass

ThomThomson · 2022-09-27T18:27:57Z

@Heenawter - I was able to fix the bug you described in #138774 (review). Nice catch! It was basically a matter of correctly applying all of the state from the save modal. I also cleaned up the state comparison of times a little bit to make it match with the rest of the state comparison.

Heenawter

Reviewed code changes + re-tested the timeRestore bug and everything works great 🎉 Also tried the following and everything worked beautifully:

Save a new dashboard with timeRestore === true
Change the time and verify that unsaved changes badge shows up
Click Save as, untick both Store time with dashboard and Save as new dashboard, then confirm
The unsaved changes badge disappears as expected 👍

src/plugins/dashboard/public/application/lib/diff_dashboard_state.ts

ThomThomson · 2022-09-27T20:01:06Z

@elasticmachine merge upstream

jbudz · 2022-09-27T20:16:23Z

buildkite test this

ThomThomson · 2022-09-28T18:13:26Z

@elasticmachine merge upstream

kibana-ci · 2022-09-28T19:14:49Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`dashboard`	395	385	-10

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`dashboard`	139	113	-26

Async chunks

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

id	before	after	diff
`dashboard`	388.1KB	418.2KB	+30.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`dashboard`	10	3	-7

Page load bundle

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

id	before	after	diff
`dashboard`	72.2KB	41.0KB	-31.2KB

Unknown metric groups

API count

id	before	after	diff
`dashboard`	144	120	-24

async chunk count

id	before	after	diff
`dashboard`	5	8	+3

ESLint disabled line counts

id	before	after	diff
`dashboard`	8	7	-1

References to deprecated APIs

id	before	after	diff
`dashboard`	60	56	-4

Total ESLint disabled count

id	before	after	diff
`dashboard`	8	7	-1

Unreferenced deprecated APIs

id	before	after	diff
`savedObjects`	1	2	+1

History

💔 Build #76534 failed 1fc1f55
💔 Build #76275 failed 1d93a4d
💔 Build #76267 failed 61452cec22558e5b5e4f04a61d0bbf53a8678619
💔 Build #76217 failed 05fb7ee
💚 Build #75331 succeeded 26103a4

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

ThomThomson and others added 5 commits August 10, 2022 10:54

Replaced dashboard loading, removed savedDashboard from app state

bbfc992

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

94dedf1

…eDashboard/removeSavedObjectClass

Added dashboard save util

989e377

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

b9b2f98

…-fix'

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

f2576c1

…eDashboard/removeSavedObjectClass

rudolf reviewed Sep 5, 2022

View reviewed changes

Heenawter mentioned this pull request Sep 8, 2022

[Dashboard] Services abstraction #139145

Merged

3 tasks

ThomThomson and others added 23 commits September 8, 2022 20:25

Removed dashboard saved object class

08e72f3

Merge branch 'portableDashboard/removeSavedObjectClass' of https://gi…

7963ffb

…thub.com/ThomThomson/kibana into portableDashboard/removeSavedObjectClass

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

f90e5ce

…eDashboard/removeSavedObjectClass

fix types, jest tests, and skip loading panels when any panel has a v…

3abdb38

…ersion lower than 7.3

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

b9da296

…eDashboard/removeSavedObjectClass

jest test import fixes

1ccc78a

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

42e3991

…-fix'

More type fixes and jest test fixes

a06e8b8

some functional test fixes

f8c9390

Merge branch 'portableDashboard/removeSavedObjectClass' of https://gi…

2b52d04

…thub.com/ThomThomson/kibana into portableDashboard/removeSavedObjectClass

more type fixes - fixed clone modal not applying title

1453d75

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

b56ecdf

…eDashboard/removeSavedObjectClass

dashboard import change for ML

e8bb483

Fix locator getFilterFields to async import

9801890

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

0063716

…eDashboard/removeSavedObjectClass

Create and implement dashboard saved objects service

5989797

remove unused lib functions, rules of hooks

7caa447

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

0db6bae

…-fix'

type fixes

7f12bbc

Merge branch 'portableDashboard/removeSavedObjectClass' of https://gi…

eef0a73

…thub.com/ThomThomson/kibana into portableDashboard/removeSavedObjectClass

remove duplicate import from merge

48a6b25

shrink bundle size, fix jest tests

43eb8e2

restore export

0cd0bf1

ThomThomson added 4 commits September 22, 2022 17:36

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

a625b3d

…eDashboard/removeSavedObjectClass

resolve merge issue

8c136f2

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

4448541

…eDashboard/removeSavedObjectClass

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

26103a4

…eDashboard/removeSavedObjectClass

ThomThomson marked this pull request as ready for review September 23, 2022 17:25

ThomThomson requested review from a team as code owners September 23, 2022 17:25

Heenawter self-requested a review September 23, 2022 17:26

Heenawter reviewed Sep 23, 2022

View reviewed changes

src/plugins/dashboard/public/application/top_nav/show_share_modal.tsx Outdated Show resolved Hide resolved

src/plugins/dashboard/public/application/top_nav/dashboard_top_nav.tsx Outdated Show resolved Hide resolved

Heenawter requested changes Sep 23, 2022

View reviewed changes

Heenawter reviewed Sep 23, 2022

View reviewed changes

darnautov approved these changes Sep 26, 2022

View reviewed changes

ThomThomson added 2 commits September 26, 2022 09:38

Merge branch 'main' of https://github.com/elastic/kibana into portabl…

488024a

…eDashboard/removeSavedObjectClass

Address review comments

ad7e29c

Heenawter approved these changes Sep 27, 2022

View reviewed changes

src/plugins/dashboard/public/application/lib/diff_dashboard_state.ts Show resolved Hide resolved

Merge branch 'main' into portableDashboard/removeSavedObjectClass

05fb7ee

Undo change which backed up time range into session storage

1d93a4d

ThomThomson force-pushed the portableDashboard/removeSavedObjectClass branch from 61452ce to 1d93a4d Compare September 27, 2022 23:42

remove extra conditional from duplicate title check

1fc1f55

Merge branch 'main' into portableDashboard/removeSavedObjectClass

474b6ed

ThomThomson merged commit f5b6a2c into elastic:main Sep 28, 2022

kibanamachine added the backport:skip This PR does not require backporting label Sep 28, 2022

Heenawter mentioned this pull request Sep 28, 2022

[Dashboard] Stuck in unsaved changes state when adding combination of pinned + unpinned filters #142161

Closed

Conversation

ThomThomson commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What does this PR do?

Test Guide

Saving and loading

Migrations

Saved Object Resolve

Uh oh!

rudolf Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudolf Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

ThomThomson Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

ThomThomson Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 23, 2022

Uh oh!

Heenawter Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

ThomThomson Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

Heenawter Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Heenawter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Heenawter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Heenawter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Saving and loading

Migrations

Saved Object Resolve

Uh oh!

darnautov left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson commented Sep 27, 2022

Uh oh!

Heenawter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThomThomson commented Sep 27, 2022

Uh oh!

jbudz commented Sep 27, 2022

Uh oh!

ThomThomson commented Sep 28, 2022

Uh oh!

kibana-ci commented Sep 28, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

ThomThomson commented Aug 12, 2022 •

edited

Loading

rudolf Sep 5, 2022 •

edited

Loading

ThomThomson Sep 19, 2022 •

edited

Loading

Heenawter left a comment •

edited

Loading

Heenawter left a comment •

edited

Loading