Reorganizing spaces client-side plugin #53644

legrego · 2019-12-19T21:31:17Z

Summary

The previous spaces client-side plugin had a rather messy folder structure. This attempts to re-organize the code to make porting to the NP a bit easier.

A bulk of the changes are simple moves (with changes to import paths). The rest of the changes are introducing the lightweight services mentioned below, and adding tests for them.

Specifically:

Introduced Advanced Settings Service

Location: plugins/spaces/public/advanced_settings
This is responsible for registering the React components which decorate the Advanced Settings screen, informing users that the settings are space-aware, unless otherwise noted.

Introduced "Copy Saved Objects to Space" Service

Location: plugins/spaces/public/copy_saved_objects_to_space
Rather wordy, but this service is responsible for the entire "copy saved objects to space" functionality that exists on the client-side.

Introduced Management Service

Location: plugins/spaces/public/management
This is responsible for registering the Spaces management link under the Kibana section, which contains the screens allowing for spaces management. As part of this service, we migrated to the NP management registration service.

Removed unnecessary `lib` folders

Consolidated logic under the new services where appropriate

A note on the `SpacesManager`

Once we are fully migrated, I want to re-evaluate the SpacesManager (out of scope for this PR). I want to deconstruct this into a few focused entities:

A simple activeSpace$ observable to get the most up-to-date representation of the active space.
A Spaces API for the CRUD operations required by the plugin. This will need a way to update the activeSpace$ when appropriate.
A "Copy Saved Objects to Spaces" API

elasticmachine · 2019-12-20T15:38:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 69e4931

History

💔 Build #16792 failed 92d25d1

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

elasticmachine · 2019-12-20T15:39:44Z

Pinging @elastic/kibana-security (Team:Security)

azasypkin · 2019-12-23T17:50:53Z

ACK: reviewing...

azasypkin

Looks great, just few very minor questions/nits!

Rather wordy, but this service is responsible for the entire "copy saved objects to space" functionality that exists on the client-side.

That looks good to me, if/once we have more Space related saved objects actions (and screens) we can make it a sub-folder of saved_objects_actions or something, but for now it feels reasonable to have a very specific name.

Removed unnecessary lib folders

💯 Navigation through folder structure is much more intuitive now!

azasypkin · 2019-12-24T12:04:51Z

x-pack/legacy/plugins/spaces/public/plugin.tsx


+  public setup(core: CoreSetup, plugins: PluginsSetup) {
+    const serverBasePath = core.injectedMetadata.getInjectedVar('serverBasePath') as string;


note: hmm, I'm surprised that we don't have serverBasePath as a property of core basePath service on the client side like we have on the server side. Is it intentional @elastic/kibana-platform or we just haven't had a chance to implement that yet?

There is a naming inconsistency. It's available under http.basePath.get().

azasypkin · 2019-12-24T12:25:21Z

x-pack/legacy/plugins/spaces/public/management/_index.scss

@@ -0,0 +1,3 @@
+@import './components/confirm_delete_modal/confirm_delete_modal';
+@import './edit_space/enabled_features/index';
+


question: I see we don't import ./edit_space/section_panel/_section_panel.scss here (or anywhere else). Do we still need it or there is some magic that imports it automatically? I also noticed similar case with _collapsible_panel.scss in security plugin.

Great catch -- it turns out that the Spaces SectionPanel does not rely on the styles in that stylesheet, because Spaces does not make use of the optional iconType prop, whereas the Security panel does. I'll see if we need it for security since you mentioned we're not importing it there either.
I'll update to import this .scss file, as that's the least invasive change to this PR.

We should consider removing the optional iconType prop from the spaces version since it's not used at all, but I'll leave that for another PR.

azasypkin · 2019-12-24T12:43:11Z

...y/plugins/spaces/public/copy_saved_objects_to_space/components/copy_to_space_flyout.test.tsx

@@ -403,7 +403,7 @@ describe('CopyToSpaceFlyout', () => {
            type: 'visualization',
            id: 'my-viz',
            error: {
-              type: 'missing_references',
+              type: 'missing_references' as any,


nit: maybe just add blocking: [], error property to make TS happy and not resort to any?

azasypkin · 2019-12-24T12:47:34Z

...acy/plugins/spaces/public/copy_saved_objects_to_space/copy_saved_objects_to_space_service.ts

+export class CopySavedObjectsToSpaceService {
+  public setup({ spacesManager, managementSetup }: SetupDeps) {
+    const action = new CopyToSpaceSavedObjectsManagementAction(spacesManager);
+    if (!managementSetup.savedObjects.registry.has(action.id)) {


question: out of curiosity, what is the use case when this will evaluate to false?

hmm, the original legacy location for this code block would execute whenever the management route was resolved, so we needed this check in place to prevent duplicate registration. I expect we don't need this anymore. I'll remove and verify before merging.

azasypkin · 2019-12-24T13:10:28Z

x-pack/legacy/plugins/spaces/public/management/management_service.test.ts

+      const service = new ManagementService();
+      service.start(deps);
+
+      expect(deps.managementStart.legacy.getSection).toHaveBeenCalledTimes(1);


optional nit: this expect doesn't seem to be very useful - we already checked that it's called with the right section name in the previous test and here we can either omit it entirely (+1) or we should check that it was called with the right section name.

azasypkin · 2019-12-24T13:11:22Z

x-pack/legacy/plugins/spaces/public/management/management_service.test.ts

+      const service = new ManagementService();
+      service.start(deps);
+
+      expect(deps.managementStart.legacy.getSection).toHaveBeenCalledTimes(1);


question: shouldn't we check here that register wasn't called instead?

good catch!

Actually, since the Kibana section isn't available on this scenario, there isn't a register function to assert against.

azasypkin · 2019-12-24T13:32:32Z

x-pack/legacy/plugins/spaces/public/nav_control/nav_control_popover.test.tsx

@@ -42,6 +42,7 @@ describe('NavControlPopover', () => {
        disabledFeatures: [],
      },
    ]);
+    // @ts-ignore readonly check


note: looks like we need a separate ISpaceManager that all consumers can rely on and then we won't have to cast to jest.Mocked<SpaceManager> in spacesManagerMock.create() and hence can change properties here without ts-ignore. Obviously it's out of scope of this PR, just wanted to note.

azasypkin · 2019-12-24T13:37:49Z

x-pack/legacy/plugins/spaces/public/space_selector/index.tsx


 const module = uiModules.get('spaces_selector', []);
 module.controller('spacesSelectorController', ($scope: any) => {
  $scope.$$postDigest(async () => {
    const domNode = document.getElementById('spaceSelectorRoot');

-    const { spacesManager } = await spacesNPStart;
+    const { spacesManager } = spacesNPStart;


optional nit: looks like we can remove async from the postDigest callback function now.

…-reorg

ryankeairns

The SCSS organization looks good, thanks for cleaning this up.

One nitpicky consideration, the ...copy_saved_objects_to_space/components/_index.scss file has styles directly in it as opposed to importing another file (e.g. we could add a _copy_to_space.scss). For comparison, the ...nav_control/components/_index.scss file imports other scss files with names aligned to the corresponding component.

That said, no other components in the ...copy_saved_objects_to_space/components/ directory have styles, so maybe this is a pattern we follow when there is only one component with styles.

Thoughts?

legrego · 2020-01-06T15:43:07Z

@ryankeairns based on #53880 (comment), I think we should move all the styles out of the _index.scss files

x-pack/legacy/plugins/spaces/public/copy_saved_objects_to_space/_index.scss

kibanamachine · 2020-01-06T17:43:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4581d58

History

💚 Build #18080 succeeded 1d83015
💚 Build #16910 succeeded 69e4931
💔 Build #16792 failed 92d25d1

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

ryankeairns

👍 LGTM, thanks for the changes!

* reorganizing spaces client-side plugin * additional testing and cleanup * address PR feedback * additional cleanup * rename scss file * one more index Co-authored-by: Elastic Machine <[email protected]>

legrego added 2 commits December 19, 2019 16:29

reorganizing spaces client-side plugin

92d25d1

additional testing and cleanup

69e4931

legrego added chore release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.6.0 v8.0.0 labels Dec 20, 2019

legrego marked this pull request as ready for review December 20, 2019 15:39

legrego requested review from a team as code owners December 20, 2019 15:39

azasypkin self-requested a review December 20, 2019 15:46

azasypkin approved these changes Dec 24, 2019

View reviewed changes

legrego added 3 commits January 6, 2020 09:24

address PR feedback

9efcf68

Merge branch 'master' of github.com:elastic/kibana into spaces/client…

307822c

…-reorg

additional cleanup

1d83015

ryankeairns reviewed Jan 6, 2020

View reviewed changes

rename scss file

eb26a77

ryankeairns reviewed Jan 6, 2020

View reviewed changes

x-pack/legacy/plugins/spaces/public/copy_saved_objects_to_space/_index.scss Outdated Show resolved Hide resolved

one more index

4581d58

ryankeairns approved these changes Jan 6, 2020

View reviewed changes

legrego merged commit 194c97f into elastic:master Jan 6, 2020

legrego deleted the spaces/client-reorg branch January 6, 2020 19:12

legrego mentioned this pull request Jan 6, 2020

[7.x] Reorganizing spaces client-side plugin (#53644) #54044

Merged

legrego mentioned this pull request Jan 8, 2020

[New platform] Spaces plugin integration #39011

Closed

25 tasks

azasypkin mentioned this pull request Feb 6, 2020

Spaces -> Client to NP #54298

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganizing spaces client-side plugin #53644

Reorganizing spaces client-side plugin #53644

legrego commented Dec 19, 2019 •

edited

Loading

elasticmachine commented Dec 20, 2019

elasticmachine commented Dec 20, 2019

azasypkin commented Dec 23, 2019

azasypkin left a comment

azasypkin Dec 24, 2019

mshustov Jan 10, 2020

azasypkin Dec 24, 2019

legrego Jan 6, 2020

azasypkin Dec 24, 2019

azasypkin Dec 24, 2019

legrego Jan 6, 2020

azasypkin Dec 24, 2019

azasypkin Dec 24, 2019

legrego Jan 6, 2020

legrego Jan 6, 2020

azasypkin Dec 24, 2019

azasypkin Dec 24, 2019

ryankeairns left a comment

legrego commented Jan 6, 2020

kibanamachine commented Jan 6, 2020

ryankeairns left a comment


		public setup(core: CoreSetup, plugins: PluginsSetup) {
		const serverBasePath = core.injectedMetadata.getInjectedVar('serverBasePath') as string;

		@@ -0,0 +1,3 @@
		@import './components/confirm_delete_modal/confirm_delete_modal';
		@import './edit_space/enabled_features/index';

Reorganizing spaces client-side plugin #53644

Reorganizing spaces client-side plugin #53644

Conversation

legrego commented Dec 19, 2019 • edited Loading

Summary

Introduced Advanced Settings Service

Introduced "Copy Saved Objects to Space" Service

Introduced Management Service

Removed unnecessary lib folders

A note on the SpacesManager

elasticmachine commented Dec 20, 2019

💚 Build Succeeded

History

elasticmachine commented Dec 20, 2019

azasypkin commented Dec 23, 2019

azasypkin 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

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

ryankeairns left a comment

Choose a reason for hiding this comment

legrego commented Jan 6, 2020

kibanamachine commented Jan 6, 2020

💚 Build Succeeded

History

ryankeairns left a comment

Choose a reason for hiding this comment

legrego commented Dec 19, 2019 •

edited

Loading

Removed unnecessary `lib` folders

A note on the `SpacesManager`