[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add
ability to convert properties to parameters
URL: https://github.com/apache/nifi/pull/3683#discussion_r320511325
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-controller-service.js
##
@@ -1860,6 +1860,16 @@
},
goToServiceDeferred: function () {
return goToServiceFromProperty(serviceTable);
+},
+getParameterContextId: function (groupId) {
+// attempt to identify the parameter context id,
conditional based on whether
+// the user is configuring the current process group
+if (_.isNil(groupId) || groupId ===
nfCanvasUtils.getGroupId()) {
+return nfCanvasUtils.getParameterContextId();
+} else {
+var parentProcessGroup =
nfCanvasUtils.getComponentByType('ProcessGroup').get(groupId);
Review comment:
Update on the nested PG:
`nfCanvasUtils.getComponentByType('ProcessGroup').get(groupId)` only knows
about the PG's that are currently rendered on the canvas so if you are in a
nested PG that is configured to use the same parameter context of a different
nested PG you won't be able to look it up this way...
A potential fix may be that the GoTo for parameter's CS usage may need to
actually navigate to the PG before opening the PG's config shell...
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add
ability to convert properties to parameters
URL: https://github.com/apache/nifi/pull/3683#discussion_r320511325
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-controller-service.js
##
@@ -1860,6 +1860,16 @@
},
goToServiceDeferred: function () {
return goToServiceFromProperty(serviceTable);
+},
+getParameterContextId: function (groupId) {
+// attempt to identify the parameter context id,
conditional based on whether
+// the user is configuring the current process group
+if (_.isNil(groupId) || groupId ===
nfCanvasUtils.getGroupId()) {
+return nfCanvasUtils.getParameterContextId();
+} else {
+var parentProcessGroup =
nfCanvasUtils.getComponentByType('ProcessGroup').get(groupId);
Review comment:
`nfCanvasUtils.getComponentByType('ProcessGroup').get(groupId)` only knows
about the PG's that are currently rendered on the canvas so if you are in a
nested PG that is configured to use the same parameter context of a different
nested PG you won't be able to look it up this way...
A potential fix may be that the GoTo for parameter's CS usage may need to
actually navigate to the PG before opening the PG's config shell...
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters URL: https://github.com/apache/nifi/pull/3683#discussion_r320494228 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/new-parameter-context-dialog.jsp ## @@ -117,6 +117,10 @@ + + +Updating parameter context Review comment: Does this need the 'ellipsis' class? It may also need a 'min-width' 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters URL: https://github.com/apache/nifi/pull/3683#discussion_r320483995 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-parameter-contexts.js ## @@ -938,7 +957,7 @@ isEmptyStringSet: serializedParam.isEmptyStringSet, isNew: originalParameter.isNew, hasValueChanged: serializedParam.hasValueChanged, -isModified: serializedParam.value !== originalParameter.value || serializedParam.description !== originalParameter.description +isModified: serializedParam.hasValueChanged || serializedParam.description !== originalParameter.description Review comment: Then here you could just check the `serializedParam.hasValueChanged || serializedParam.hasDescriptionChanged` 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add
ability to convert properties to parameters
URL: https://github.com/apache/nifi/pull/3683#discussion_r320489059
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-parameter-contexts.js
##
@@ -1604,19 +1595,12 @@
text: '#ff'
},
disabled: function () {
-var input = $('#parameter-value-field');
-var isChecked =
$('#parameter-set-empty-string-field').hasClass('checkbox-checked');
-var serializedValue = serializeValue(input,
parameter, isChecked);
-
-var description =
$('#parameter-description-field').val();
-
-var hasChanged = serializedValue.hasChanged ||
description !== parameter.previousDescription;
-
-return !hasChanged;
+var param = serializeParameter(parameter);
+return !param.hasValueChanged &&
param.description === parameter.description;
Review comment:
```suggestion
return !param.hasValueChanged &&
param.hasDescriptionChanged;
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add
ability to convert properties to parameters
URL: https://github.com/apache/nifi/pull/3683#discussion_r320483558
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-parameter-contexts.js
##
@@ -814,12 +814,23 @@
*/
var serializeParameter = function (originalParameter) {
var name = $.trim($('#parameter-name').val());
+var value = $('#parameter-value-field').val();
var isEmptyStringSet =
$('#parameter-set-empty-string-field').hasClass('checkbox-checked');
-
-var serializedValue = serializeValue($('#parameter-value-field'),
originalParameter, isEmptyStringSet);
var description = $('#parameter-description-field').val();
var isSensitive =
$('#parameter-dialog').find('input[name="sensitive"]:checked').val() ===
'sensitive' ? true : false;
+var parameter = {
+name: name,
+value: value,
+description: description,
+sensitive: isSensitive,
+isEmptyStringSet: isEmptyStringSet,
+previousValue: null,
+isNew: true
+};
+
+var serializedValue = serializeValue($('#parameter-value-field'),
_.isNil(originalParameter) ? parameter : originalParameter, isEmptyStringSet);
+
return {
Review comment:
The serializedParam you are returning here could also have a
'hasDescriptionChanged: description !== originalParameter.description'
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add
ability to convert properties to parameters
URL: https://github.com/apache/nifi/pull/3683#discussion_r320488673
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-parameter-contexts.js
##
@@ -1042,10 +1032,11 @@
text: '#ff'
},
disabled: function () {
-if ($('#parameter-context-name').val() !== '') {
-return false;
+var param = serializeParameter();
Review comment:
I don't think you meant to do this here... This is the parameter context
dialog not the parameter dialog. The only requirement to create a parameter
context is a name... I know we have
https://issues.apache.org/jira/browse/NIFI-6602 open and I am thinking we may
need to create a `serializeParameterContext()` so we could then ask the
returned object about it's name or if there are any parameters to update... but
for this PR you probably just need to make sure there is a value in the
`$('#parameter-context-name')` field.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [nifi] scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add ability to convert properties to parameters
scottyaslan commented on a change in pull request #3683: NIFI-6506 - Add
ability to convert properties to parameters
URL: https://github.com/apache/nifi/pull/3683#discussion_r320485308
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-parameter-contexts.js
##
@@ -1856,6 +1841,58 @@
initParameterTable();
};
+/**
+ * Opens the Add Parameter Dialog.
+ * @param {Object} callbacks object with callbacks for handling the cancel
and apply button functionality:
+ *
+ * @example
+ * openAddParameterDialog({
+ * onApply: function () {
+ * // handle the apply button being clicked
+ * },
+ * onCancel: function() {
+ * // handle the cancel button being clicked
+ * }
+ * });
+ */
+var openAddParameterDialog = function (callbacks) {
+$('#parameter-dialog')
+.modal('setHeaderText', 'Add Parameter')
+.modal('setButtonModel', [{
+buttonText: 'Apply',
+color: {
+base: '#728E9B',
+hover: '#004849',
+text: '#ff'
+},
+disabled: function () {
+var param = serializeParameter();
+var isUpdatingParameterContext =
$('#parameter-context-updating-status').hasClass('show-status');
+
+if (isUpdatingParameterContext || _.isEmpty(param.name)) {
+return true;
+}
+return false;
+},
+handler: {
+click: callbacks.onApply
+}
+}, {
+buttonText: 'Cancel',
+color: {
+base: '#E3E8EB',
+hover: '#C7D2D7',
+text: '#004849'
+},
+handler: {
+click: callbacks.onCancel
+}
+}])
+.modal('show');
+
+$('#parameter-dialog').modal('show');
Review comment:
You already show the modal on line 1891...
There is also an extra `modal('show')` on line 1812...
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
