From 8335d8f4845c8ef0b49ffaab6151be8c97f57fda Mon Sep 17 00:00:00 2001 From: Vincent Salucci <26154748+vincentsalucci@users.noreply.github.com> Date: Thu, 23 May 2024 09:05:58 -0500 Subject: [PATCH] [AC-2559] - BUG - Refactor members search observable chains (#9230) * fix: update observable chains for search, refs AC-2559 * fix: remove comment, reorganize variables, refs AC-2559 * chore: remove ngOnInit from base people component, refs AC-2559 * chore: remove async declaration from resetPaging, refs AC-2559 * chore: replace bit-search ngmodel with formControl, refs AC-2559 * chore: move destroy pattern out of base class, refs AC-2559 * fix: remove ngOnDestroy super call, refs AC-2559 * Improve performance issues --------- Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Co-authored-by: Thomas Rittson --- .../common/base.people.component.ts | 78 +++++-------------- .../members/people.component.html | 8 +- .../organizations/members/people.component.ts | 8 +- .../providers/manage/people.component.html | 12 ++- .../providers/manage/people.component.ts | 6 +- 5 files changed, 40 insertions(+), 72 deletions(-) diff --git a/apps/web/src/app/admin-console/common/base.people.component.ts b/apps/web/src/app/admin-console/common/base.people.component.ts index a33bd9298a..0dad7ab7b1 100644 --- a/apps/web/src/app/admin-console/common/base.people.component.ts +++ b/apps/web/src/app/admin-console/common/base.people.component.ts @@ -1,13 +1,6 @@ -import { Directive, OnDestroy, OnInit, ViewChild, ViewContainerRef } from "@angular/core"; -import { - BehaviorSubject, - Subject, - firstValueFrom, - from, - lastValueFrom, - switchMap, - takeUntil, -} from "rxjs"; +import { Directive, ViewChild, ViewContainerRef } from "@angular/core"; +import { FormControl } from "@angular/forms"; +import { firstValueFrom, concatMap, map, lastValueFrom, startWith, debounceTime } from "rxjs"; import { SearchPipe } from "@bitwarden/angular/pipes/search.pipe"; import { UserNamePipe } from "@bitwarden/angular/pipes/user-name.pipe"; @@ -40,10 +33,8 @@ const MaxCheckedCount = 500; @Directive() export abstract class BasePeopleComponent< - UserType extends ProviderUserUserDetailsResponse | OrganizationUserView, - > - implements OnInit, OnDestroy -{ + UserType extends ProviderUserUserDetailsResponse | OrganizationUserView, +> { @ViewChild("confirmTemplate", { read: ViewContainerRef, static: true }) confirmModalRef: ViewContainerRef; @@ -106,19 +97,22 @@ export abstract class BasePeopleComponent< protected didScroll = false; protected pageSize = 100; - protected destroy$ = new Subject(); + protected searchControl = new FormControl("", { nonNullable: true }); + protected isSearching$ = this.searchControl.valueChanges.pipe( + debounceTime(500), + concatMap((searchText) => this.searchService.isSearchable(searchText)), + startWith(false), + ); + protected isPaging$ = this.isSearching$.pipe( + map((isSearching) => { + if (isSearching && this.didScroll) { + this.resetPaging(); + } + return !isSearching && this.users && this.users.length > this.pageSize; + }), + ); private pagedUsersCount = 0; - private _searchText$ = new BehaviorSubject(""); - private isSearching: boolean = false; - - get searchText() { - return this._searchText$.value; - } - - set searchText(value: string) { - this._searchText$.next(value); - } constructor( protected apiService: ApiService, @@ -143,22 +137,6 @@ export abstract class BasePeopleComponent< abstract reinviteUser(id: string): Promise; abstract confirmUser(user: UserType, publicKey: Uint8Array): Promise; - ngOnInit(): void { - this._searchText$ - .pipe( - switchMap((searchText) => from(this.searchService.isSearchable(searchText))), - takeUntil(this.destroy$), - ) - .subscribe((isSearchable) => { - this.isSearching = isSearchable; - }); - } - - ngOnDestroy(): void { - this.destroy$.next(); - this.destroy$.complete(); - } - async load() { const response = await this.getUsers(); this.statusMap.clear(); @@ -202,8 +180,6 @@ export abstract class BasePeopleComponent< } // Reset checkbox selecton this.selectAll(false); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises this.resetPaging(); } @@ -236,7 +212,7 @@ export abstract class BasePeopleComponent< const filteredUsers = this.searchPipe.transform( this.users, - this.searchText, + this.searchControl.value, "name", "email", "id", @@ -249,7 +225,7 @@ export abstract class BasePeopleComponent< } } - async resetPaging() { + resetPaging() { this.pagedUsers = []; this.loadMore(); } @@ -418,16 +394,6 @@ export abstract class BasePeopleComponent< } } - isPaging() { - const searching = this.isSearching; - if (searching && this.didScroll) { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.resetPaging(); - } - return !searching && this.users && this.users.length > this.pageSize; - } - protected revokeWarningMessage(): string { return this.i18nService.t("revokeUserConfirmation"); } @@ -440,8 +406,6 @@ export abstract class BasePeopleComponent< let index = this.users.indexOf(user); if (index > -1) { this.users.splice(index, 1); - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises this.resetPaging(); } diff --git a/apps/web/src/app/admin-console/organizations/members/people.component.html b/apps/web/src/app/admin-console/organizations/members/people.component.html index 6e8410011b..f3eb13ed44 100644 --- a/apps/web/src/app/admin-console/organizations/members/people.component.html +++ b/apps/web/src/app/admin-console/organizations/members/people.component.html @@ -1,7 +1,7 @@ @@ -48,9 +48,9 @@

{{ "noMembersInList" | i18n }}

@@ -66,7 +66,7 @@ diff --git a/apps/web/src/app/admin-console/organizations/members/people.component.ts b/apps/web/src/app/admin-console/organizations/members/people.component.ts index 488b07c096..668526b38c 100644 --- a/apps/web/src/app/admin-console/organizations/members/people.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/people.component.ts @@ -9,6 +9,7 @@ import { map, Observable, shareReplay, + Subject, switchMap, takeUntil, } from "rxjs"; @@ -94,6 +95,8 @@ export class PeopleComponent extends BasePeopleComponent { protected canUseSecretsManager$: Observable; + private destroy$ = new Subject(); + constructor( apiService: ApiService, private route: ActivatedRoute, @@ -194,7 +197,7 @@ export class PeopleComponent extends BasePeopleComponent { await this.load(); - this.searchText = qParams.search; + this.searchControl.setValue(qParams.search); if (qParams.viewEvents != null) { const user = this.users.filter((u) => u.id === qParams.viewEvents); if (user.length > 0 && user[0].status === OrganizationUserStatusType.Confirmed) { @@ -210,7 +213,8 @@ export class PeopleComponent extends BasePeopleComponent { } ngOnDestroy(): void { - super.ngOnDestroy(); + this.destroy$.next(); + this.destroy$.complete(); } async load() { diff --git a/bitwarden_license/bit-web/src/app/admin-console/providers/manage/people.component.html b/bitwarden_license/bit-web/src/app/admin-console/providers/manage/people.component.html index 49b4ded7c6..963c70a7b1 100644 --- a/bitwarden_license/bit-web/src/app/admin-console/providers/manage/people.component.html +++ b/bitwarden_license/bit-web/src/app/admin-console/providers/manage/people.component.html @@ -1,5 +1,9 @@ - +