Skip to content

[NIFI-13981] inline parameter context creation#9613

Merged
rfellows merged 5 commits intoapache:mainfrom
scottyaslan:NIFI-13981
Jan 24, 2025
Merged

[NIFI-13981] inline parameter context creation#9613
rfellows merged 5 commits intoapache:mainfrom
scottyaslan:NIFI-13981

Conversation

@scottyaslan
Copy link
Contributor

Inline parameter context creation from both the create PG and edit PG dialogs. All listings of parameter contexts are also now sorted.

@scottyaslan scottyaslan requested a review from mcgilman January 8, 2025 20:25
@scottyaslan scottyaslan force-pushed the NIFI-13981 branch 2 times, most recently from fc7eb37 to 195072f Compare January 10, 2025 16:42
@mcgilman mcgilman added the ui Pull requests for work relating to the user interface label Jan 11, 2025
Copy link
Contributor

@rfellows rfellows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things...

Comment on lines +66 to +72
<button
mat-icon-button
class="primary-icon-button mt-1 ml-1"
(click)="openNewParameterContextDialog()"
title="Create parameter context">
<i class="fa fa-plus"></i>
</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be available if the current user has canWrite for parameterContextPermissions. If the user can't modify parameter contexts, creating one will fail. We should prevent the user from getting into a situation where we know the desired outcome will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

Comment on lines +73 to +79
<button
mat-icon-button
class="primary-icon-button mt-1 ml-1"
(click)="openNewParameterContextDialog()"
title="Create parameter context">
<i class="fa fa-plus"></i>
</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be available if the current user has canWrite for parameterContextPermissions. If the user can't modify parameter contexts, creating one will fail. We should prevent the user from getting into a situation where we know the desired outcome will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

Comment on lines +59 to +64
it('should not modify the original array', () => {
const items: SortableBy[] = [{ name: 'Banana' }, { name: 'Apple' }];
const itemsCopy = [...items];
pipe.transform(items);
expect(items).not.toEqual(itemsCopy);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by this test. It suggests that the original array should not get modified however, the underlying transform uses array.sort which sorts array elements in place. In many paces we depend on it modifying the array that is passed in as we don't bother assigning a variable to the result (which is just a reference to the same array).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This test is pretty pointless. I will remove it.

@rfellows
Copy link
Contributor

@scottyaslan, One more thing that was noticed: when adding a new process group, if you hit enter after typing in the name of the pg, the new parameter context dialog opens rather than creating the pg. i think we should fix that too.

@scottyaslan
Copy link
Contributor Author

@scottyaslan, One more thing that was noticed: when adding a new process group, if you hit enter after typing in the name of the pg, the new parameter context dialog opens rather than creating the pg. i think we should fix that too.

It is done now. Thanks!

Copy link
Contributor

@rfellows rfellows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for the changes @scottyaslan 👍

@rfellows rfellows merged commit af5c88b into apache:main Jan 24, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui Pull requests for work relating to the user interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants