mcgilman commented on code in PR #10910:
URL: https://github.com/apache/nifi/pull/10910#discussion_r3196859610
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/users/state/user-listing/user-listing.effects.ts:
##########
@@ -263,17 +263,22 @@ export class UserListingEffects {
ofType(UserListingActions.createUserSuccess),
map((action) => action.response),
filter((response) => response.userGroupUpdate != null),
- mergeMap((createUserResponse) =>
- this.actions$.pipe(
+ mergeMap((createUserResponse) => {
+ const expectedCount =
createUserResponse.userGroupUpdate?.userGroups?.length ?? 0;
+ if (expectedCount === 0) {
+ return of(UserListingActions.createUserComplete({
response: createUserResponse }));
+ }
+ return this.actions$.pipe(
ofType(UserListingActions.updateUserGroupSuccess),
filter(
(updateSuccess) =>
// @ts-ignore
createUserResponse.userGroupUpdate.requestId ===
updateSuccess.response.requestId
),
+ take(expectedCount),
map(() => UserListingActions.createUserComplete({
response: createUserResponse }))
Review Comment:
The existing code here suffers from emitting `createUserComplete` for each
successful user group update. I think this can be easily improved with a
`last()` between the `take` and `map`.
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/users/state/user-listing/user-listing.effects.ts:
##########
@@ -539,17 +544,26 @@ export class UserListingEffects {
ofType(UserListingActions.updateUserSuccess),
map((action) => action.response),
filter((response) => response.userGroupUpdate != null),
- mergeMap((updateUserResponse) =>
- this.actions$.pipe(
+ mergeMap((updateUserResponse) => {
+ const addedCount =
updateUserResponse.userGroupUpdate?.userGroupsAdded?.length ?? 0;
+ const removedCount =
updateUserResponse.userGroupUpdate?.userGroupsRemoved?.length ?? 0;
+ const expectedCount = addedCount + removedCount;
+
+ if (expectedCount === 0) {
+ return of(UserListingActions.updateUserComplete());
+ }
+
+ return this.actions$.pipe(
ofType(UserListingActions.updateUserGroupSuccess),
filter(
(updateSuccess) =>
// @ts-ignore
updateUserResponse.userGroupUpdate.requestId ===
updateSuccess.response.requestId
),
+ take(expectedCount),
Review Comment:
Same comment from above about `take` not completing if any of the requests
fail.
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/users/state/user-listing/user-listing.effects.ts:
##########
@@ -539,17 +544,26 @@ export class UserListingEffects {
ofType(UserListingActions.updateUserSuccess),
map((action) => action.response),
filter((response) => response.userGroupUpdate != null),
- mergeMap((updateUserResponse) =>
- this.actions$.pipe(
+ mergeMap((updateUserResponse) => {
+ const addedCount =
updateUserResponse.userGroupUpdate?.userGroupsAdded?.length ?? 0;
+ const removedCount =
updateUserResponse.userGroupUpdate?.userGroupsRemoved?.length ?? 0;
+ const expectedCount = addedCount + removedCount;
+
+ if (expectedCount === 0) {
+ return of(UserListingActions.updateUserComplete());
+ }
+
+ return this.actions$.pipe(
ofType(UserListingActions.updateUserGroupSuccess),
filter(
(updateSuccess) =>
// @ts-ignore
updateUserResponse.userGroupUpdate.requestId ===
updateSuccess.response.requestId
),
+ take(expectedCount),
map(() => UserListingActions.updateUserComplete())
Review Comment:
Same comment from above about `last()` in between `take` and `map` to limit
`updateUserComplete` to a single emission.
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/users/state/user-listing/user-listing.effects.ts:
##########
@@ -263,17 +263,22 @@ export class UserListingEffects {
ofType(UserListingActions.createUserSuccess),
map((action) => action.response),
filter((response) => response.userGroupUpdate != null),
- mergeMap((createUserResponse) =>
- this.actions$.pipe(
+ mergeMap((createUserResponse) => {
+ const expectedCount =
createUserResponse.userGroupUpdate?.userGroups?.length ?? 0;
+ if (expectedCount === 0) {
+ return of(UserListingActions.createUserComplete({
response: createUserResponse }));
+ }
+ return this.actions$.pipe(
ofType(UserListingActions.updateUserGroupSuccess),
filter(
(updateSuccess) =>
// @ts-ignore
createUserResponse.userGroupUpdate.requestId ===
updateSuccess.response.requestId
),
+ take(expectedCount),
Review Comment:
This only considers successfully emissions. If any of the requests fail
`take` won't complete.
--
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]
