Spaces - Copy Saved Objects to Spaces UI by legrego · Pull Request #39002 · elastic/kibana

legrego · 2019-06-14T17:33:52Z

Summary

This introduces a new "Copy to space" action within the Saved Objects Management UI. When Spaces is enabled, a new action appears in the table next to each saved object. From there, users are given the option to copy that saved object (with or without references) to one or more other spaces.

Tasks

create workflow
create integration points in saved objects management ui
integrate with actual APIs from Copy Saved Objects to Spaces API #38014
testing
i18n verification
a11y verification
copy review

Menu option

Select spaces to copy into

Processing copy

Copy response, showing sample error

Copy response, showing conflict resolution

Resolves #37286

"Release notes: Saved objects can be easily copied from one space to another from the Saved Objects Management screen"

legrego · 2019-06-14T17:35:20Z

@cchaos no rush, as the implementation is blocked on several other PRs, but if you get some free time, I'd love a preliminary design review!

elasticmachine · 2019-06-14T17:35:42Z

Pinging @elastic/kibana-security

legrego · 2019-06-14T17:39:39Z

src/legacy/ui/public/management/saved_objects_management/saved_objects_management_action.ts

+  };
+}
+
+export abstract class SavedObjectsManagementAction<T = unknown> {


This is tightly coupled to both EUI and React. My ultimate goal is to replace this entirely with the Embeddable/Actions APIs once they're ready, so I don't want to spend a ton of time crafting the perfect abstraction here. Instead, this provides enough functionality to enable the copy-to-space feature, without coupling it to Spaces itself.

elasticmachine · 2019-06-14T18:31:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

^ Unrelated CI failure

src/legacy/ui/public/management/saved_objects_management/saved_objects_management_action.ts

elasticmachine · 2019-06-25T20:11:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-03T13:23:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-08T19:58:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

Conky5 · 2019-07-08T20:34:41Z

retest

elasticmachine · 2019-07-08T21:36:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos · 2019-07-15T18:01:05Z

@legrego Sorry it's taken so long for the design team to take a look at this. @ryankeairns will actually be able to help you out on it. You'll probably want to sync with him to go over the purpose and intricacies of all the Spaces stuffs.

legrego · 2019-08-01T13:45:37Z

@ryankeairns I updated the screenshots in this PR summary to reflect my changes based on your mockup. It's not production ready, but it hopefully gives you an idea of the direction I took.

I deviated from your mockup in a couple of places. I'm more than willing to make any adjustments you see fit:

Added "skip" option to undo the "overwrite" button click. Based on the mockup, there was no way to indicate that you no longer wanted to overwrite the saved object. This adds a "skip" option if you want to cancel this operation.
Added selected options to the top of the processing screen, which just reflects the chosen options (include related, automatically overwrite)
Added operation summary to the footer, showing number of copied, pending, skipped (due to conflict), and errors.

Since there is no way to automatically expand accordions (except on initial render), I wanted a way to surface the fact that something might need your attention, so I put that summary near the "Finish" button to hopefully make that a little clearer.

This also includes a couple of sass entries, which are probably worth looking at to make sure I'm not doing anything crazy.

kobelb

Nice work, everything seems to be working well. My primary concern is with regard to the missing references functionality, and not being able to test it.

It's super pedantic, but I'm not the biggest fan of using the CTS acronym. It's used rather inconsistently... For the classes/interfaces which aren't exported outside of the x-pack/legacy/plugins/spaces/public/views/management/components/copy_saved_objects_to_space folder, perhaps we could just use "short names" and drop the "Copy to Space" part?

We also are re-using types and processImportResponse from the saved objects import, which is somewhat awkward. It isn't obvious that internally copy to space is relying upon import/export.

Also, when choosing to "overwrite" a saved object, should we change the test of the "Finish" button? It wasn't immediately obvious to me that clicking "overwrite" wasn't enough to actually overwrite it and that we were just marking it to be retried once we hit "Finish".

kobelb · 2019-08-22T16:22:21Z

src/legacy/ui/public/management/saved_objects_management/index.ts

+export {
+  processImportResponse,
+  ProcessedImportResponse,
+} from '../../../../core_plugins/kibana/public/management/sections/objects/lib/process_import_response';


We shouldn't be reaching into the kibana plugin from here, should we move process_import_response to src/legacy/ui/public/management/saved_objects_management/?

Ah, I thought this was acceptable because we do the same thing from src/legacy/ui/public/management/index.js today to share functionality between OSS and X-Pack

Interesting... well, let the "bad practices" roll :) Feel free to ignore this.

kobelb · 2019-08-22T17:35:51Z

x-pack/legacy/plugins/spaces/public/lib/hooks/use_spaces.ts

+
+  useEffect(() => {
+    setIsLoading(true);
+    kfetch({ pathname: '/api/spaces/space', query: { purpose } }).then((response: Space[]) => {


We're using kfetch directly, should we be using the SpacesManager?

Yeah ideally. Passing the SpacesManager into the hook would be problematic I think...it's awkward to pass it as a dependency of the hook, because then the hook really isn't doing anything important...it's just calling the SpacesManager for you.

I think I'll just remove this hook and replace it with a direct use of useEffect within the flyout to take advantage of the SpacesManager.

kobelb · 2019-08-22T18:15:42Z

src/legacy/ui/public/management/saved_objects_management/saved_objects_management_action.ts

+  id: string;
+  name: string;
+}
+export interface SavedObjectRecord {


Suggested change

export interface SavedObjectRecord {

export interface SavedObjectsManagementRecord {

kobelb · 2019-08-22T18:16:06Z

src/legacy/ui/public/management/saved_objects_management/saved_objects_management_action.ts

+
+import { ReactNode } from '@elastic/eui/node_modules/@types/react';
+
+export interface SavedObjectReference {


Suggested change

export interface SavedObjectReference {

export interface SavedObjectsManagementRecordReference {

kobelb · 2019-08-22T19:30:56Z

...public/views/management/components/copy_saved_objects_to_space/selectable_spaces_control.tsx

+export const SelectableSpacesControl = (props: Props) => {
+  const [options, setOptions] = useState<SpaceOption[]>([]);
+
+  useEffect(() => {


Are we using this to compute the initial options for performance reasons? If doesn't seem like we're performing a true side effect here.

Good call, this was a holdover from an earlier experiment

kobelb · 2019-08-22T21:43:10Z

.../public/views/management/components/copy_saved_objects_to_space/processing_copy_to_space.tsx

+          props.savedObject,
+          spaceCopyResult,
+          props.copyOptions.includeRelated
+        ) as SummarizedCopyToSpaceResult;


Suggested change

) as SummarizedCopyToSpaceResult;

);

kobelb · 2019-08-22T21:45:17Z

.../public/views/management/components/copy_saved_objects_to_space/processing_copy_to_space.tsx

+import { FormattedMessage } from '@kbn/i18n/react';
+import {
+  summarizeCopyResult,
+  SummarizedCopyToSpaceResult,


Suggested change

SummarizedCopyToSpaceResult,

kobelb · 2019-08-22T22:04:33Z

x-pack/legacy/plugins/spaces/public/lib/copy_saved_objects_to_space/summarize_copy_result.ts

+    conflicts: conflicts.filter(
+      c => c.obj.type === savedObject.type && c.obj.id === savedObject.id
+    ),
+    missingReferences,


Currently, if you try to use export or copy to space where a reference is missing, we get a 400 Bad Request is thrown. So, it's hard to tell how the missing references will actually be returned once this is fixed.

Do we really want all missing reference conflicts to show up here? Wouldn't this be all missing references for the entire "object graph" including transitive references?

For the direct references and transitive references, we end up listing all missing reference errors that are for the object itself. If that's correct, shouldn't the "missing reference" for the saved object we're trying to copy always be null/undefined?

Hmm, I think we're talking about different scenarios here. Any copy to space code that mentions "missing references" is talking about conditions which produce this import error:

kibana/src/core/server/saved_objects/import/types.ts

Lines 68 to 78 in 4996c5d

export interface SavedObjectsImportMissingReferencesError {

type: 'missing_references';

references: Array<{

type: string;

id: string;

}>;

blocking: Array<{

type: string;

id: string;

}>;

}

Specifically, there are two conditions which cause this (that I'm aware of), both of which are produced by validate_references:

An object being imported directly depends on an index pattern which isn't provided in the upload/copy request, nor in the target space (e.g., unchecking "include references" when copying a visualization which references an index pattern into a fresh space)

An object being imported directly depends on a saved search which isn't provided in the upload/copy request, nor in the target space (e.g., unchecking "include references" when copying an index pattern which references a saved search into a fresh space)

The normal import/export flow shows a screen which allows users to choose a new index pattern or saved search to associate, but this is one of the features that differs between import/export and copy to space.

I have this "missing references" code in place for the sole purpose of showing a more informative tooltip when reviewing the "copy to space" results.

Wouldn't this be all missing references for the entire "object graph" including transitive references?

No, currently, the import/export service only deals with direct references to index patterns and saved searches. No other reference types are checked for this condition on import, and we don't fail if this happens on a transitive reference either. This feels inconsistent to me, but also outside the scope of the "Copy to Space" feature IMO.

kobelb · 2019-08-22T22:41:21Z

...ces/public/views/management/components/copy_saved_objects_to_space/copy_status_indicator.tsx

+    );
+    return <EuiIconTip type={'check'} color={successColor} content={message} />;
+  }
+  if (hasMissingRefs) {


Were you able to test this functionality somehow? I get a 400 whenever trying...

See comment above, I think we're talking about two different scenarios

kobelb · 2019-08-22T22:43:50Z

x-pack/legacy/plugins/spaces/public/lib/copy_saved_objects_to_space/summarize_copy_result.ts

+  | UnsuccessfulResponse
+  | ProcessingResponse;
+
+export function summarizeCopyResult(


If we only use this from within x-pack/legacy/plugins/spaces/public/views/management/components/copy_saved_objects_to_space should we move it there?

We can...I chose to put it here so that I could test it independently from the React components, and it felt wrong to have this non-component file within the **/components/** directory structure

legrego · 2019-08-23T02:14:42Z

We also are re-using types and processImportResponse from the saved objects import, which is somewhat awkward. It isn't obvious that internally copy to space is relying upon import/export.

I didn't see it that way - I liked that we weren't duplicating functionality or type definitions for what's effectively a thin wrapper around the import/export feature. It felt to me like we'd be able to see how changes to import/export will impact Copy to Space in the future. Do you have suggestions on how we can improve this? We can divorce the two from a types perspective, but then I feel like it's even less obvious that copy to space relies upon import/export.

Also, when choosing to "overwrite" a saved object, should we change the test of the "Finish" button? It wasn't immediately obvious to me that clicking "overwrite" wasn't enough to actually overwrite it and that we were just marking it to be retried once we hit "Finish".

Sure, that's a good idea.

elasticmachine · 2019-08-23T12:38:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2019-08-23T16:08:33Z

Following a discussion with @kobelb, we decided on the following:

We will no longer explain when a copy failed due to "missing references"
We will no longer allow a saved object to be copied without it's references (related objects)

These decisions were made after discovering shortcomings in the import/export functionality which would have led to a confusing user experience for the Copy to Space feature.

legrego · 2019-08-23T16:18:20Z

Also discovered during testing: #43876

kobelb

LGTM on green CI

snide · 2019-08-23T17:02:06Z

@legrego I'll have a review / PR for you in a couple hours from the design side.

elasticmachine · 2019-08-23T17:21:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

snide

I loaded this locally and looked through the code. There are some changes I'd like to make on the design / sass side that I can do on my own. There's nothing in here that looks scary that I would want to hold this PR up for.

Since we're got the green, I'm ok with a merge, and I'll do my design pass as a separate concern before freeze.

elasticmachine · 2019-08-23T20:57:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sample UI for copying a single object to multiple spaces

e0aca37

legrego added Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// Feature:Security/Spaces Platform Security - Spaces feature WIP Work in progress labels Jun 14, 2019

legrego commented Jun 14, 2019

View reviewed changes

legrego commented Jun 17, 2019

View reviewed changes

src/legacy/ui/public/management/saved_objects_management/saved_objects_management_action.ts Outdated Show resolved Hide resolved

legrego added 3 commits June 25, 2019 13:53

Simplify action

933817e

Merge branch 'master' of github.com:elastic/kibana into copy-to-space-ui

8401fee

fix merge from master

46a68df

legrego added 4 commits June 27, 2019 11:25

WIP status indicator

d33e73d

POC conflict resolution UI

c07f905

prefix exported types with SavedObjects

e27e111

Merge branch 'master' of github.com:elastic/kibana into copy-to-space-ui

29f2bdc

show finish button

4b18c51

legrego added 6 commits July 30, 2019 16:44

adopting design's mockup

61491ff

Merge branch 'master' of github.com:elastic/kibana into copy-to-space-ui

a1b662e

enabling conflict resolution, and other improvements

8e00c79

move inline styles to sass

8c4425b

ux adjustments

bea29e3

add loading indicator

92edadc

kobelb reviewed Aug 22, 2019

View reviewed changes

legrego added 4 commits August 22, 2019 21:26

renaming interfaces

aa14457

reorganize types; type SpaecsManager CTS functions

0d62721

transient => transitive

9beea0c

removing incorrect union type (null)

56f949d

legrego added 6 commits August 23, 2019 06:46

using spaces manager to retrieve the list of available spaces

fcf1870

removing unnecessary useEffect

189f872

removing cts_ prefix

e615838

improved button text when resolving conflicts

917fc63

test updates

2fe0e21

Merge branch 'master' of github.com:elastic/kibana into copy-to-space-ui

09ba89e

legrego added 2 commits August 23, 2019 11:37

remove 'missing references' functionality

5f1cde4

require related objects to be included

65e3594

kobelb approved these changes Aug 23, 2019

View reviewed changes

snide self-requested a review August 23, 2019 17:02

snide approved these changes Aug 23, 2019

View reviewed changes

legrego merged commit 6a9844c into elastic:master Aug 23, 2019

legrego deleted the copy-to-space-ui branch August 23, 2019 18:24

legrego mentioned this pull request Aug 23, 2019

[7.x] Spaces - Copy Saved Objects to Spaces UI (#39002) #43888

Merged

legrego added a commit to legrego/kibana that referenced this pull request Aug 23, 2019

Spaces - Copy Saved Objects to Spaces UI (elastic#39002)

efcdb63

legrego added a commit that referenced this pull request Aug 23, 2019

Spaces - Copy Saved Objects to Spaces UI (#39002) (#43888)

85522c6

legrego mentioned this pull request Aug 26, 2019

Spaces - copy saved objects should allow copying without references #43985

Closed

peterschretlen mentioned this pull request Sep 25, 2019

Stack Monitoring + Kibana Alerting #45571

Open

3 tasks

	export interface SavedObjectRecord {
	export interface SavedObjectsManagementRecord {


		import { ReactNode } from '@elastic/eui/node_modules/@types/react';

		export interface SavedObjectReference {

	export interface SavedObjectsImportMissingReferencesError {
	type: 'missing_references';
	references: Array<{
	type: string;
	id: string;
	}>;
	blocking: Array<{
	type: string;
	id: string;
	}>;
	}

Conversation

legrego commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tasks

Menu option

Select spaces to copy into

Processing copy

Copy response, showing sample error

Copy response, showing conflict resolution

Uh oh!

legrego commented Jun 14, 2019

Uh oh!

elasticmachine commented Jun 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 14, 2019 • edited by legrego Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💔 Build Failed

Uh oh!

Uh oh!

elasticmachine commented Jun 25, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 3, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 8, 2019

💔 Build Failed

Uh oh!

Conky5 commented Jul 8, 2019

Uh oh!

elasticmachine commented Jul 8, 2019

💔 Build Failed

Uh oh!

cchaos commented Jul 15, 2019

Uh oh!

legrego commented Aug 1, 2019

Uh oh!

kobelb 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legrego commented Aug 23, 2019

Uh oh!

legrego commented Jun 14, 2019 •

edited

Loading

elasticmachine commented Jun 14, 2019 •

edited by legrego

Loading

kobelb left a comment •

edited

Loading