mcgilman commented on code in PR #7353:
URL: https://github.com/apache/nifi/pull/7353#discussion_r1223267396
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/new-process-group-dialog.jsp:
##########
@@ -28,6 +28,12 @@
</div>
<input id="new-process-group-name" type="text"
placeholder="Enter a name or select a file to upload"/>
</div>
+ </div>
+ <div class="setting">
+ <div class="setting-name">Parameter Context</div>
Review Comment:
The changes to the new Process Group Dialog should warrant increasing the
vertical height.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/process-group-configuration.jsp:
##########
@@ -109,6 +100,34 @@
<div id="process-group-controller-services-tab-content"
class="configuration-tab">
<div id="process-group-controller-services-table"
class="settings-table"></div>
</div>
+ <div id="process-group-parameter-contexts-tab-content"
class="configuration-tab">
Review Comment:
What was the motivation for moving this configuration into a new tab?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -127,6 +130,67 @@
return filepath.replace(/^.*[\\\/]/, '').replace(/\..*/, '');
};
+ /**
+ * parameter context
+ */
+ var parameterContextOptions = [];
+ var noParameterContext = {
+ text: 'No parameter context',
+ value: null
+ };
+ var selectedOption = noParameterContext;
+ var parentParameterContextUrl = '../nifi-api/flow/process-groups/' +
encodeURIComponent(nfCanvasUtils.getGroupId());
Review Comment:
I think there is a bug here. I think this code is evaluated once when this
loads. And current Process Group Id will be different based on when and where a
new Process Group is being added. We should determine this URL when the Process
Group is being added. I think we should limit variables here to things that are
constants and do not change based on the current context.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if
($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId =
$('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version,
processGroup.id, parameterContextId);
Review Comment:
What happens if one of these calls fails? Is there a reason why the
`recursive` flag isn't passed into the call update the current Process Group?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -349,11 +416,15 @@
}
});
+ getParameterContextOptions();
Review Comment:
This should return a `deferred` with the options so we don't execute the
following code until it completes.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/header/components/nf-ng-group-component.js:
##########
@@ -127,6 +130,67 @@
return filepath.replace(/^.*[\\\/]/, '').replace(/\..*/, '');
};
+ /**
+ * parameter context
+ */
+ var parameterContextOptions = [];
Review Comment:
I would also suggest we move this variable out of this scope and instead
return it from `getParameterContextOptions`. The function could return a
`deferred` which provides the populated array.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if
($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId =
$('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version,
processGroup.id, parameterContextId);
Review Comment:
Applying the Parameter Context change recursively does not work when the
user lacks permission to any Parameter Contexts referenced by any descendent
Process Groups. Currently, the UI reports that everything was saved
successfully. But you can see there are requests failing with 403.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -513,6 +552,20 @@
// initialize the parameter context combo
$('#process-group-parameter-context-combo').combo('destroy').combo(comboOptions);
+
+ // initialize parameter context recursive checkbox
+ getAllDescendantProcessGroup(groupId).then(function (response) {
Review Comment:
Making this call when the user does not have permissions to the current
Process Group, results in an error in the UI. Currently, we should be handling
this without a user-facing error because the user may lack permissions for the
current PG but have explicit permissions for a Controller Service in the
listing.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -85,6 +85,58 @@
return $('#process-group-controller-services-table');
};
+ /**
+ *
+ * Gets all process groups of a process group
+ * @param groupId
+ * @returns process groups array
+ **/
+ var allDescendantProcessGroups;
Review Comment:
It would be preferable if this was retrieved when needed or passed in as an
argument rather then storing it here.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-process-group-configuration.js:
##########
@@ -123,6 +175,14 @@
dataType: 'json',
contentType: 'application/json'
}).done(function (response) {
+ // check parameter context recursive update requirement
+ if
($('#parameter-contexts-recursive').hasClass('checkbox-checked')) {
+ var parameterContextId =
$('#process-group-parameter-context-combo').combo('getSelectedOption').value;
+ allDescendantProcessGroups.forEach(function (processGroup) {
+ updateParameterContext(processGroup.revision.version,
processGroup.id, parameterContextId);
Review Comment:
Applying the Parameter Context change recursively does not work when the
user lacks permissions to any descendent Process Group. The UI here also
reported success but there was a failed request with 500. The corresponding
stack trace was:
```
15 java.lang.NullPointerException: null
16 at
org.apache.nifi.web.api.ProcessGroupResource.lambda$updateProcessGroup$4(ProcessGroupResource.java:561)
17 at
org.apache.nifi.web.StandardNiFiServiceFacade.authorizeAccess(StandardNiFiServiceFacade.java:468)
18 at
org.apache.nifi.web.StandardNiFiServiceFacade$$FastClassBySpringCGLIB$$358780e0.invoke(<generated>)
19 at
org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
