NIFI-3785: Added feature to move controller services between process … #8965

Freedom9339 · 2024-06-13T20:19:02Z

…groups with the new UI

Summary

NIFI-3785: Added an option on the controller services grid that moves a controller service to a specified process group. The controller service can be moved to the parent process group or a child process group. However, it can only be moved one level at a time. If there are any scope conflicts with referencing components, the move will fail displaying the reason.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 21

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

Freedom9339 · 2024-06-13T20:21:14Z

@markap14 @exceptionfactory I've resubmitted this PR with the changes done to the new UI. The backend is the same as it was in the closed PR(#7734), but the UI changes were done against the new UI.

Freedom9339 · 2024-06-14T19:02:35Z

@markobean

exceptionfactory · 2024-06-14T21:44:58Z

Thanks for putting in the work to refactor this for the new UI @Freedom9339.

Tagging @mcgilman and @rfellows for potential review.

mcgilman · 2024-06-14T21:54:45Z

Thanks for the mention @exceptionfactory! I'll have a look...

mcgilman

Thanks for the PR @Freedom9339! I've left a number of comments below that mostly appear to be from using the Enable/Disable Service dialog has a starting point. Since that dialog needs to be used for both Services in the Flow and in the Controller Settings there area a number of considerations there that are not needed here.

In addition to the items below, please ensure you run prettier and that this PR isn't introducing any new lint issues (I think there are currently 31 left that we're slowly trying to work through). From nifi/nifi-frontend/src/main/frontend please run:

$ npx nx prettier:format
$ npx nx lint

I will defer to @exceptionfactory or @markap14 for a more thorough review of the back end changes.

mcgilman · 2024-06-17T13:40:24Z

...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts

+                    const serviceId: string = request.id;
+                    request.processGroupFlow = this.flowService.getFlow(currentProcessGroupId);
+                    const moveDialogReference = this.dialog.open(MoveControllerService, {
+                        ...XL_DIALOG,


Is an XL Dialog needed here? It seems like the a smaller dialog may be more appropriate.

Changed this to a LARGE_DIALOG.

The dialog size looks great. Can we update it so the content shares the available space like other dialogs (for instance Edit Controller Service)?

I fixed this. It now extends all the way.

...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts

...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.reducer.ts

mcgilman · 2024-06-17T13:48:53Z

...i/common/controller-service/controller-service-table/controller-service-table.component.html

+                                @if (canConfigure(item)) {
+                                    <button mat-menu-item (click)="moveClicked(item)">
+                                        <i class="fa fa-arrows primary-color mr-2"></i>
+                                        Move
+                                    </button>
+                                }


This table is used for both services in the Flow as well as services in the Controller Settings. This is why the ControllerServiceTable is in the common components. Since Services in the Controller Settings do not exist within a Process Group, this action is not be available.

Made the move option not show up in management controller services.

Thanks for the update here. The new Input() supports function that returns whether the service can be moved. However, the implements are not service specific and always just return true or false. Can this Input() be simplified into a boolean?

@Input() canMove: boolean = false;

Changed this to just be a boolean.

.../ui/common/controller-service/move-controller-service/move-controller-service.component.html

...pp/ui/common/controller-service/move-controller-service/move-controller-service.component.ts

nifi-frontend/src/main/frontend/apps/nifi/src/app/state/shared/index.ts

mcgilman · 2024-06-17T14:17:53Z

...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts

+                concatLatestFrom(() => this.store.select(selectCurrentProcessGroupId)),
+                tap(([request, currentProcessGroupId]) => {
+                    const serviceId: string = request.id;
+                    request.processGroupFlow = this.flowService.getFlow(currentProcessGroupId);


The state for the flow-designer page is already loaded with the current process group. We should be able to simply use that instead of making another request here.

The current state did not store the entire flow. I did change this and I was able to use it here without an api call. However, with the implementation of disabled drop down menu process groups, I did need to 2 other api calls here.

Ah sorry. Yes you are correct. I thought we had stashed the entire processGroupFlow in the service state as well but I see that only saved the breadcrumbs and parameter context details. Now we're saving the entire processGroupFlow next to the breadcrumbs and parameter context details.

Do we only need the processGroupFlow in order to get the child Process Groups for the Move dialog? If so, what are your thoughts only saving them in the store. Often times the flow can be very large and in this page we won't need most of those details.

Let me know if I missed something and we do need additional details.

Thank you for the idea, I hadn't thought of that. I removed the storing of the whole process group flow, and just saved an array with names and IDs.

.../common/controller-service/move-controller-service/move-controller-service.component.spec.ts

Freedom9339 · 2024-07-23T13:21:48Z

@mcgilman Just checking in. Any updates on this?

mcgilman · 2024-07-23T15:08:56Z

@mcgilman Just checking in. Any updates on this?

Thanks for the ping. Sorry for the delay, I'll try to get some more eyes on this PR this week.

Freedom9339 · 2024-08-21T17:36:01Z

@mcgilman Sorry the pinging again. Just want to make sure this doesn't fall through the cracks.

mcgilman · 2024-08-23T21:43:56Z

@mcgilman Sorry the pinging again. Just want to make sure this doesn't fall through the cracks.

I just pulled down the latest. After rebasing against current main there were build failures due to changes that have landed after your PR was created. So I tried to run the PR as-is without rebasing and I'm still seeing issues. When running through the dev server it fails to serve with

Application bundle generation failed. [13.461 seconds]

✘ [ERROR] Undefined function.
╷
109 │ $surface-light-palette: mat.define-palette($surface-palette, 600, 100, 900);
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
╵
libs/shared/src/assets/themes/supplemental.scss 109:25 @import
apps/nifi/src/styles.scss 62:9 root stylesheet [plugin angular-sass]
angular:styles/global:styles:1:8:
  1 │ @import 'apps/nifi/src/styles.scss';

I then tried running with the built application but I'm seeing this error in dev tools when attempting to open the MoveControllerService Component.

TypeError: g is not iterable
    at chunk-VIZEOFFB.js:70:66450
    at Array.forEach (<anonymous>)
    at o.loadChildOptions (chunk-VIZEOFFB.js:70:66333)
    at new o (chunk-VIZEOFFB.js:70:65423)
    at o.ɵfac [as factory] (chunk-VIZEOFFB.js:70:67428)
    at hn (chunk-ZKVQE3GD.js:7:23746)
    at TC (chunk-ZKVQE3GD.js:7:63202)
    at mn.create (chunk-ZKVQE3GD.js:7:61795)
    at Vg.createComponent (chunk-ZKVQE3GD.js:7:64954)
    at t.attachComponentPortal (chunk-6XYXU2WR.js:1:110675)

Once the PR is updated I should be able to get some more eyes on it quickly.

Freedom9339 · 2024-08-26T13:58:21Z

@mcgilman I fixed the issue caused by rebasing and pushed. Thank You!

mcgilman

Thanks for the rebase! I've left some additional feedback below.

mcgilman · 2024-08-30T17:00:09Z

.../src/app/pages/flow-designer/ui/move-controller-service/move-controller-service.component.ts

+                value: child.value
+            };
+
+            const root = this.getProcessGroupById(currentProcessGroupEntity.component, child.value ?? '');


getProcessGroupById will return null for a couple different scenarios. When this happens calling processGroupContainsComponent will error with the following. The unhandled error leaves the UI in a weird state because the overlay is active but the dialog isn't shown.

TypeError: Cannot read properties of null (reading 'contents') at _MoveControllerService.processGroupContainsComponent (move-controller-service.component.ts:184:26) at move-controller-service.component.ts:166:27 at Array.forEach (<anonymous>) at _MoveControllerService.loadChildOptions (move-controller-service.component.ts:157:34) at new _MoveControllerService (move-controller-service.component.ts:99:14) at NodeInjectorFactory.MoveControllerService_Factory [as factory] (move-controller-service.component.ts:231:5) at getNodeInjectable (core.mjs:6060:44) at createRootComponent (core.mjs:17231:35) at ComponentFactory.create (core.mjs:17091:29) at ViewContainerRef2.createComponent (core.mjs:17498:47)

It appears this was happening when childProcessGroupOptions contained more than one option. The recursive logic in getProcessGroupById (and other methods with similar structure) isn't correct. When there are multiple children the following code

for (const pg of root.contents.processGroups) { return this.getProcessGroupById(pg, processGroupId); }

will return the result from the first child and not attempt to process any subsequent children.

I've fixed the recursion logic.

mcgilman · 2024-08-30T17:12:08Z

...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts

@@ -63,7 +66,8 @@ import {
 } from '../../../../state/property-verification/property-verification.selectors';
 import { VerifyPropertiesRequestContext } from '../../../../state/property-verification';
 import { BackNavigation } from '../../../../state/navigation';
-import { NiFiCommon, Storage } from '@nifi/shared';
+import { NiFiCommon, Storage, SelectOption, ComponentType, LARGE_DIALOG, SMALL_DIALOG, XL_DIALOG } from '@nifi/shared';
+import { ComponentEntity } from './../flow/index';


Suggested change

import { ComponentEntity } from './../flow/index';

import { ComponentEntity } from '../flow';

mcgilman · 2024-08-30T17:12:58Z

...d/src/main/frontend/apps/nifi/src/app/pages/flow-designer/state/controller-services/index.ts

 import { ControllerServiceEntity, ParameterContextReferenceEntity } from '../../../../state/shared';
 import { BreadcrumbEntity } from '../shared';
+import { Revision } from './../../../../state/shared/index';


Suggested change

import { Revision } from './../../../../state/shared/index';

import { Revision } from '../../../../state/shared';

mcgilman · 2024-08-30T17:26:30Z

.../src/app/pages/flow-designer/ui/move-controller-service/move-controller-service.component.ts

+        );
+
+        const processGroups: SelectOption[] = [];
+        if (request.breadcrumb != undefined) {


Under what conditions is request.breadcrumb undefined? I think the store has an initial value but even if that isn't the case we could update the concatLatestFrom with the following:

this.store.select(selectBreadcrumb).pipe(isDefinedAndNotNull())

I was trying to only use a single request model, but I've created 2 and removed the need to declare it as breadcrumb?

mcgilman · 2024-08-30T17:28:50Z

...ps/nifi/src/app/pages/flow-designer/state/controller-services/controller-services.effects.ts

+                    const clone = Object.assign({}, request.request);
+                    clone.processGroupEntity = request.processGroupEntity;
+                    clone.childProcessGroupOptions = request.childProcessGroupOptions;
+                    clone.parentControllerServices = request.controllerServices;
+                    clone.breadcrumb = request.breadcrumb;


Why is a clone being created here?

Same as above. Removed need to clone.

…groups with the new UI

… component to flow-designer. Added disabling options that have a move conflict

…upflow in the state and just load the info needed.

Freedom9339 · 2024-09-03T15:39:33Z

@mcgilman I've completed the change/fixes requested. Thank You!

Freedom9339 · 2024-09-05T18:47:20Z

@mcgilman Sorry, I just realized my latest commit didn't go through. It's there now.

ChrisSamo632 changed the title ~~NIFI-3785.1: Added feature to move controller services between process …~~ NIFI-3785: Added feature to move controller services between process … Jun 13, 2024

mcgilman self-requested a review June 14, 2024 21:54

mcgilman requested changes Jun 17, 2024

View reviewed changes

mcgilman added the new ui Pull requests for work relating to the new user interface being developed. label Jun 20, 2024

Freedom9339 force-pushed the nifi-3785.1 branch from 2de68fc to 6c4fab1 Compare June 25, 2024 18:01

Freedom9339 requested a review from mcgilman June 27, 2024 12:36

Freedom9339 force-pushed the nifi-3785.1 branch from fc7e2c1 to 9392904 Compare July 3, 2024 17:12

Freedom9339 force-pushed the nifi-3785.1 branch from 9392904 to 9009d70 Compare August 26, 2024 13:56

mcgilman reviewed Aug 30, 2024

View reviewed changes

Freedom9339 added 6 commits September 3, 2024 12:51

NIFI-3785: Added feature to move controller services between process …

a778e1c

…groups with the new UI

NIFI-3785: Fixed issue caused by rebasing

fd1a0ff

NIFI-3785: Various feedback requests completed. Moved move controller…

154c8e2

… component to flow-designer. Added disabling options that have a move conflict

NIFI-3785: removed need for second api call

89d2a15

NIFI-3785: Fixed dropdown width. Stopped storing the whole processgro…

35b5f64

…upflow in the state and just load the info needed.

NIFI-3785: Rebased.

5a86dbe

Freedom9339 force-pushed the nifi-3785.1 branch from 9009d70 to 5a86dbe Compare September 3, 2024 15:34

NIFI-3785: Fixed recursion issue. Removed need to clone.

c368f43

	import { ComponentEntity } from './../flow/index';
	import { ComponentEntity } from '../flow';

	import { Revision } from './../../../../state/shared/index';
	import { Revision } from '../../../../state/shared';

NIFI-3785: Added feature to move controller services between process … #8965

Are you sure you want to change the base?

NIFI-3785: Added feature to move controller services between process … #8965

Conversation

Freedom9339 commented Jun 13, 2024 • edited Loading

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Freedom9339 commented Jun 13, 2024

Freedom9339 commented Jun 14, 2024

exceptionfactory commented Jun 14, 2024

mcgilman commented Jun 14, 2024

mcgilman 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

Freedom9339 commented Jul 23, 2024

mcgilman commented Jul 23, 2024

Freedom9339 commented Aug 21, 2024

mcgilman commented Aug 23, 2024

Freedom9339 commented Aug 26, 2024

mcgilman 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

Freedom9339 commented Sep 3, 2024

Freedom9339 commented Sep 5, 2024

Freedom9339 commented Jun 13, 2024 •

edited

Loading