-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-13125 - Flow Version Diff View #10473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
010fc08 to
d1ff571
Compare
scottyaslan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution. I have left some comments. I am happy to review and help iterate here and continue to improve the UX for this feature.
| private store = inject<Store<CanvasState>>(Store); | ||
|
|
||
| displayedColumns: string[] = ['current', 'version', 'created', 'comments']; | ||
| displayedColumns: string[] = ['actions', 'current', 'version', 'created', 'comments']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actions column should be displayed as the last column in the table listing.
| (matSortChange)="sortData($event)" | ||
| [matSortActive]="sort.active" | ||
| [matSortDirection]="sort.direction"> | ||
| <!-- Actions Column --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actions column should be displayed as the last column in the table listing.
| .mat-column-actions { | ||
| width: 54px; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actions column is already set for all listings so we don't need this here
| .mat-column-actions { | |
| width: 54px; | |
| } |
| <div class="flow-diff-loading" *ngIf="isLoading"> | ||
| <mat-progress-spinner diameter="36" mode="indeterminate"></mat-progress-spinner> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The listings in nifi do not show mat-progress-spinner or any other loading UX.
| <div class="flow-diff-loading" *ngIf="isLoading"> | |
| <mat-progress-spinner diameter="36" mode="indeterminate"></mat-progress-spinner> | |
| </div> |
| <div *ngIf="errorMessage" class="flow-diff-error" data-qa="flow-diff-error"> | ||
| {{ errorMessage }} | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The listings in nifi do not show errors like this. If there is an error when requesting the diff it should be displayed in a context error banner at the top of the dialog like we do in other cases.
| <div *ngIf="errorMessage" class="flow-diff-error" data-qa="flow-diff-error"> | |
| {{ errorMessage }} | |
| </div> |
| } | ||
|
|
||
| ngAfterViewInit(): void { | ||
| // Sorting handled automatically when MatSort is available via the ViewChild setter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really dug in here yet but it seems sorting is handled differently for this listing then other listings throughout nifi. I am not saying this is incorrect but it is a different pattern so at the very least it would need some testing and verification. It would be nice it we could just update this to follow the pattern used in other listings. We should not need to use a ViewChild to sort an angular material table.
|
|
||
| <table | ||
| mat-table | ||
| *ngIf="dataSource.data.length > 0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| *ngIf="dataSource.data.length > 0" |
| data-qa="flow-diff-message" | ||
| *ngIf="comparisonSummary.length > 0"> | ||
| <div class="flow-diff-message-title">Comparing versions:</div> | ||
| <ul class="flow-diff-message-list"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unordered list is not the correct way to display this information. Please follow the label/value UX used throughout the application to display values. I believe we have an example in the change version dialog as well.
| private store = inject<Store<CanvasState>>(Store); | ||
| private timeOffset = this.store.selectSignal(selectTimeOffset); | ||
|
|
||
| @ViewChild(MatSort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can follow how the change version dialog implemented sorting instead of using a ViewChild here.
| private formatTimestamp(metadata: VersionedFlowSnapshotMetadata): string | undefined { | ||
| if (!metadata.timestamp) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const now = new Date(); | ||
| const userOffset = now.getTimezoneOffset() * 60 * 1000; | ||
| const date = new Date(metadata.timestamp + userOffset + this.getTimezoneOffset()); | ||
| return this.nifiCommon.formatDateTime(date); | ||
| } | ||
|
|
||
| private getTimezoneOffset(): number { | ||
| return this.timeOffset() || 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we typically format timestamps like this. Is there a reason to here? I now see that we do have two implementations of:
formatTimestamp(flowVersion: VersionedFlowSnapshotMetadata) {
// get the current user time to properly convert the server time
const now: Date = new Date();
// convert the user offset to millis
const userTimeOffset: number = now.getTimezoneOffset() * 60 * 1000;
// create the proper date by adjusting by the offsets
const date: Date = new Date(flowVersion.timestamp + userTimeOffset + this.timeOffset);
return this.nifiCommon.formatDateTime(date);
}
One in the select-flow-version.componet.ts and one in the import-from-registry.component.ts. If we really have a need for this formatting we should have a single implementation in a shared area. A new formatTimestamp Angular pipe in https://github.com/apache/nifi/tree/main/nifi-frontend/src/main/frontend/libs/shared/src/pipes would work well for this use case but I think you will find that formatting timestamps has lots or nuanced considerations. we used to have a library for this but it wasn't well maintained and was deprecated if that is any clue as to LOE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we can just leave the timestamp unformatted.
|
Thanks for all the feedback @scottyaslan - highly appreciated! I have (hopefully) made the changes you had in mind. The only thing that is not ok is the timestamp in the dropdown lists when selecting versions. Will figure it out tomorrow. It looks so much better now, thanks for all of the review comments!
|




Summary
NIFI-13125 - Flow Version Diff View
I used #9109 as a starting point and coded something similar for NiFi 2.x. Definitely not a UI expert so my approach may not be the best one. Happy to collaborate with someone more experienced on the UI side.
This feature is particularly useful when a user sees that a versioned process group can be updated to a new version and the user would like to see what are the differences with the current version before upgrading the process group.
Access to the view is made available when listing versions for a versioned process group:
Clicking on the diff icon for the desired version will show the diff between the current version and the selected version
Versions can be selected via the dropdown lists:
Filtering is also possible to filter the content of the table, and the columns can also be sorted alphabetically.
The current feature does not allow comparison across branches but that could be considered as a follow-up improvement if the community sees value into it (I'm not sure I picture a use case where it would be very useful).
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation