NIFI-15182: Fixed adding/removing user to multiple groups not saving…#10910
NIFI-15182: Fixed adding/removing user to multiple groups not saving…#10910Freedom9339 wants to merge 2 commits intoapache:mainfrom
Conversation
mcgilman
left a comment
There was a problem hiding this comment.
Thanks for the PR @Freedom9339! Noted a few things below.
Also, could you add some test coverage for the changed effects? The existing spec file covers loadTenants scenarios, but the effects modified in this PR (updateUserGroup$, awaitUpdateUserGroupsForCreateUser$, awaitUpdateUserGroupsForUpdateUser$, updateUserSuccess$) don't have tests yet.
In particular, a test for updateUserGroup$ verifying that multiple concurrent actions all complete their HTTP requests would help guard against regression — the original switchMap → mergeMap fix is subtle enough that it could be accidentally reverted without a test catching it.
| ofType(UserListingActions.updateUserGroup), | ||
| map((action) => action.request), | ||
| switchMap((request) => | ||
| mergeMap((request) => |
There was a problem hiding this comment.
The switchMap → mergeMap change in updateUserGroup$ correctly addresses the root cause. Thanks for catching this.
| // @ts-ignore | ||
| const expectedCount = createUserResponse.userGroupUpdate.userGroups?.length || 0; |
There was a problem hiding this comment.
You can avoid the @ts-ignore here by extending the optional chaining one level up — this lets the compiler handle the nullability without suppressing other potential errors:
const expectedCount = createUserResponse.userGroupUpdate?.userGroups?.length ?? 0;
| // @ts-ignore | ||
| const addedCount = updateUserResponse.userGroupUpdate.userGroupsAdded?.length || 0; | ||
| // @ts-ignore | ||
| const removedCount = updateUserResponse.userGroupUpdate.userGroupsRemoved?.length || 0; |
There was a problem hiding this comment.
Same pattern here:
const addedCount = updateUserResponse.userGroupUpdate?.userGroupsAdded?.length ?? 0;
const removedCount = updateUserResponse.userGroupUpdate?.userGroupsRemoved?.length ?? 0;
| return of(UserListingActions.updateUserComplete()); | ||
| } else { | ||
| return userGroupUpdates; | ||
| return from(userGroupUpdates); |
There was a problem hiding this comment.
Good change wrapping in from() here. It makes the intent clearer. Would you mind doing the same in createUserSuccess$ (line 234)? It still returns the raw array there. Both work, but it'd be nice to keep them consistent.
return userGroupUpdates;
…s$ in from(), and added unit tests.
|
Thank you for the review @mcgilman. I've made the suggested changes and added unit tests. |
… to all groups.
Summary
NIFI-15182: There was an issue where if multiple groups were selected to add a user to, the update would sometimes fail on all groups, and the operation would have to be repeated to add the user to all desired groups. This also happened when deleting a user from multiple groups. This change fixes this issue.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation