From 6055bd08569762874e7f8eb19aad676772c339b8 Mon Sep 17 00:00:00 2001 From: Phillip Chambers Date: Fri, 3 Mar 2023 10:10:16 +0000 Subject: [PATCH 01/14] feat: make use of rxjs for caching & searching users --- .../src/app/services/justice-users.service.ts | 53 +++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts b/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts index 95c489d19..412798faa 100644 --- a/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts +++ b/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts @@ -1,7 +1,7 @@ import { Injectable } from '@angular/core'; -import { Observable } from 'rxjs'; -import { map, shareReplay } from 'rxjs/operators'; -import { AddJusticeUserRequest, BHClient, JusticeUserResponse, JusticeUserRole } from './clients/api-client'; +import { BehaviorSubject, Observable } from 'rxjs'; +import { filter, map, mergeMap, share, shareReplay, switchMap, tap } from 'rxjs/operators'; +import { AddJusticeUserRequest, BHClient, EditJusticeUserRequest, JusticeUserResponse, JusticeUserRole } from './clients/api-client'; import { cleanQuery } from '../common/helpers/api-helper'; @Injectable({ @@ -9,8 +9,44 @@ import { cleanQuery } from '../common/helpers/api-helper'; }) export class JusticeUsersService { private cache$: Observable; + private refresh$: BehaviorSubject = new BehaviorSubject(null); + private searchTerm$: BehaviorSubject = new BehaviorSubject(null); + + users$ = this.refresh$.pipe( + mergeMap(() => this.requestJusticeUsers(null)), + shareReplay(1), + switchMap(users => + this.searchTerm$.pipe( + map(term => { + return this.applyFilter(term, users); + }) + ) + ), + tap(x => console.log(`Search results`, x)) + ); constructor(private apiClient: BHClient) {} + + // just for testing -- remove + // triggers an emission on users$ + refresh() { + this.refresh$.next(); + } + + // push a search term into the stream + search(searchTerm: string) { + console.log(`Searching with ${searchTerm}`); + this.searchTerm$.next(searchTerm); + } + + // more complex searching required here + applyFilter(searchTerm: string, users: JusticeUserResponse[]): JusticeUserResponse[] { + if (!searchTerm) { + return users; + } + return users.filter(user => user.first_name === searchTerm); + } + retrieveJusticeUserAccounts() { if (!this.cache$) { this.cache$ = this.requestJusticeUsers(null).pipe(shareReplay(1)); @@ -43,6 +79,15 @@ export class JusticeUsersService { telephone: telephone, role: role }); - return this.apiClient.addNewJusticeUser(request); + return this.apiClient.addNewJusticeUser(request).pipe(tap(() => this.refresh$.next())); + } + + editJusticeUser(id: string, username: string, role: JusticeUserRole) { + const request = new EditJusticeUserRequest({ + id, + username, + role + }); + return this.apiClient.editJusticeUser(request).pipe(tap(() => this.refresh$.next())); } } From d8c1556651736e1a0e5429fa90935f55681d9182 Mon Sep 17 00:00:00 2001 From: Phillip Chambers Date: Tue, 21 Mar 2023 09:30:04 +0000 Subject: [PATCH 02/14] Update components & tests to use observables --- .../services/justice-users.service.spec.ts | 144 +++++-- .../src/app/services/justice-users.service.ts | 54 +-- .../justice-users-menu.component.html | 2 +- .../justice-users-menu.component.spec.ts | 21 - .../justice-users-menu.component.ts | 21 +- .../manage-team/manage-team.component.html | 128 +++--- .../manage-team/manage-team.component.spec.ts | 407 ++++++++---------- .../manage-team/manage-team.component.ts | 135 +++--- 8 files changed, 459 insertions(+), 453 deletions(-) diff --git a/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.spec.ts b/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.spec.ts index 74dbd7e0e..f3cb2c131 100644 --- a/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.spec.ts +++ b/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.spec.ts @@ -1,63 +1,110 @@ -import { fakeAsync, TestBed, tick } from '@angular/core/testing'; -import { of } from 'rxjs'; +import { TestBed } from '@angular/core/testing'; +import { combineLatest, of } from 'rxjs'; +import { count, delay, switchMap, take } from 'rxjs/operators'; import { AddJusticeUserRequest, BHClient, EditJusticeUserRequest, JusticeUserResponse, JusticeUserRole } from './clients/api-client'; import { JusticeUsersService } from './justice-users.service'; +import { Logger } from './logger'; describe('JusticeUsersService', () => { let service: JusticeUsersService; let clientApiSpy: jasmine.SpyObj; + const loggerSpy = jasmine.createSpyObj('Logger', ['error', 'debug', 'warn']); beforeEach(() => { clientApiSpy = jasmine.createSpyObj(['getUserList', 'addNewJusticeUser', 'deleteJusticeUser', 'editJusticeUser']); - TestBed.configureTestingModule({ providers: [{ provide: BHClient, useValue: clientApiSpy }] }); + TestBed.configureTestingModule({ + providers: [ + { provide: BHClient, useValue: clientApiSpy }, + { provide: Logger, useValue: loggerSpy } + ] + }); service = TestBed.inject(JusticeUsersService); }); - describe('retrieveJusticeUserAccounts', () => { - it('should call api when retrieving justice user accounts', (done: DoneFn) => { + describe('`users$` observable', () => { + it('should emit users returned from api', (done: DoneFn) => { const users: JusticeUserResponse[] = [ new JusticeUserResponse({ id: '123', contact_email: 'user1@test.com' }), new JusticeUserResponse({ id: '456', contact_email: 'user2@test.com' }), new JusticeUserResponse({ id: '789', contact_email: 'user3@test.com' }) ]; clientApiSpy.getUserList.and.returnValue(of(users)); - service.retrieveJusticeUserAccounts().subscribe(result => { + service.users$.subscribe(result => { expect(result).toEqual(users); done(); }); }); - it('should not call api when retrieving justice user accounts and users have been already been cached', (done: DoneFn) => { - const users: JusticeUserResponse[] = [ - new JusticeUserResponse({ id: '123', contact_email: 'user1@test.com' }), - new JusticeUserResponse({ id: '456', contact_email: 'user2@test.com' }), - new JusticeUserResponse({ id: '789', contact_email: 'user3@test.com' }) - ]; - service['cache$'] = of(users); - clientApiSpy.getUserList.and.returnValue(of(users)); - service.retrieveJusticeUserAccounts().subscribe(result => { - expect(result).toEqual(users); - expect(clientApiSpy.getUserList).toHaveBeenCalledTimes(0); + it('should not make additional api requests when additional observers subscribe', (done: DoneFn) => { + clientApiSpy.getUserList.and.returnValue(of([])); + + combineLatest([service.users$, service.users$]) + .pipe( + delay(1000), + switchMap(x => service.users$) + ) + .subscribe(() => { + expect(clientApiSpy.getUserList).toHaveBeenCalledTimes(1); + done(); + }); + }); + }); + + describe('search()', () => { + it('should trigger another emission from $users observable', (done: DoneFn) => { + // arrange + clientApiSpy.getUserList.and.returnValue(of([])); + + // users$ will emit initially - after calling search() two times, we should see 3 emissions from users$ + service.filteredUsers$.pipe(take(3), count()).subscribe(c => { + // assert + expect(c).toBe(3); done(); }); + + // act + service.search(''); + service.search(''); }); - it('should call api and return user list', (done: DoneFn) => { - const users: JusticeUserResponse[] = [new JusticeUserResponse({ id: '123', contact_email: 'user1@test.com' })]; - const term = 'user1'; - clientApiSpy.getUserList.and.returnValue(of(users)); - service.retrieveJusticeUserAccountsNoCache(term).subscribe(result => { - expect(result).toEqual(users); - expect(clientApiSpy.getUserList).toHaveBeenCalledTimes(1); + it('should apply a filter to the users collection', () => { + // arrange + const users: JusticeUserResponse[] = [ + new JusticeUserResponse({ id: '123', contact_email: 'user1@test.com', first_name: 'Test' }), + new JusticeUserResponse({ id: '456', contact_email: 'user2@test.com', first_name: 'AnotherTest' }), + new JusticeUserResponse({ id: '789', contact_email: 'user3@test.com', first_name: 'LastTest' }) + ]; + + // act + const filteredUsers = service.applyFilter('Test', users); + + // assert + expect(filteredUsers[0].first_name).toBe('Test'); + }); + }); + + describe('refresh()', () => { + it('should trigger another emission from $users observable', (done: DoneFn) => { + // arrange + clientApiSpy.getUserList.and.returnValue(of([])); + + // users$ will emit initially - after calling refresh() two more times, we should see 3 emissions from users$ + service.users$.pipe(take(3), count()).subscribe(c => { + // assert + expect(c).toBe(3); done(); }); + + // act + service.refresh(); + service.refresh(); }); }); describe('addNewJusticeUser', () => { - it('should call the api to save a new user', fakeAsync(() => { + it('should call the api to save a new user & again to get the users list', (done: DoneFn) => { const username = 'john@doe.com'; const firstName = 'john'; const lastName = 'doe'; @@ -75,10 +122,8 @@ describe('JusticeUsersService', () => { }); clientApiSpy.addNewJusticeUser.and.returnValue(of(newUser)); - let result: JusticeUserResponse; + clientApiSpy.getUserList.and.returnValue(of([])); - service.addNewJusticeUser(username, firstName, lastName, telephone, role).subscribe(data => (result = data)); - tick(); const request = new AddJusticeUserRequest({ username: username, first_name: firstName, @@ -86,13 +131,20 @@ describe('JusticeUsersService', () => { telephone: telephone, role: role }); - expect(result).toEqual(newUser); - expect(clientApiSpy.addNewJusticeUser).toHaveBeenCalledWith(request); - })); + + combineLatest([service.users$, service.addNewJusticeUser(username, firstName, lastName, telephone, role)]).subscribe( + ([_, userResponse]: [JusticeUserResponse[], JusticeUserResponse]) => { + expect(clientApiSpy.getUserList).toHaveBeenCalledTimes(2); + expect(userResponse).toEqual(newUser); + expect(clientApiSpy.addNewJusticeUser).toHaveBeenCalledWith(request); + done(); + } + ); + }); }); describe('editJusticeUser', () => { - it('should call the api to edit an existing user', fakeAsync(() => { + it('should call the api to edit an existing user & again to get the users list', (done: DoneFn) => { const id = '123'; const username = 'john@doe.com'; const firstName = 'john'; @@ -110,26 +162,36 @@ describe('JusticeUsersService', () => { }); clientApiSpy.editJusticeUser.and.returnValue(of(existingUser)); - let result: JusticeUserResponse; + clientApiSpy.getUserList.and.returnValue(of([])); - service.editJusticeUser(id, username, role).subscribe(data => (result = data)); - tick(); const request = new EditJusticeUserRequest({ id, username, role }); - expect(result).toEqual(existingUser); - expect(clientApiSpy.editJusticeUser).toHaveBeenCalledWith(request); - })); + + combineLatest([service.users$, service.editJusticeUser(id, username, role)]).subscribe( + ([_, result]: [JusticeUserResponse[], JusticeUserResponse]) => { + expect(clientApiSpy.getUserList).toHaveBeenCalledTimes(2); + expect(result).toEqual(existingUser); + expect(clientApiSpy.editJusticeUser).toHaveBeenCalledWith(request); + done(); + } + ); + }); }); describe('deleteJusticeUser', () => { - it('should call the api to delete the user', () => { + it('should call the api to delete the user & again to get the users list', (done: DoneFn) => { clientApiSpy.deleteJusticeUser.and.returnValue(of('')); + clientApiSpy.getUserList.and.returnValue(of([])); + const id = '123'; - service.deleteJusticeUser(id).subscribe(); - expect(clientApiSpy.deleteJusticeUser).toHaveBeenCalledWith(id); + combineLatest([service.users$, service.deleteJusticeUser(id)]).subscribe(() => { + expect(clientApiSpy.getUserList).toHaveBeenCalledTimes(2); + expect(clientApiSpy.deleteJusticeUser).toHaveBeenCalledWith(id); + done(); + }); }); }); }); diff --git a/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts b/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts index f4e46db32..9c725b410 100644 --- a/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts +++ b/AdminWebsite/AdminWebsite/ClientApp/src/app/services/justice-users.service.ts @@ -1,41 +1,32 @@ import { Injectable } from '@angular/core'; -import { BehaviorSubject, Observable } from 'rxjs'; -import { filter, map, mergeMap, share, shareReplay, switchMap, tap } from 'rxjs/operators'; +import { BehaviorSubject, Observable, of, throwError } from 'rxjs'; +import { catchError, delay, filter, map, mergeMap, share, shareReplay, switchMap, tap } from 'rxjs/operators'; import { AddJusticeUserRequest, BHClient, EditJusticeUserRequest, JusticeUserResponse, JusticeUserRole } from './clients/api-client'; import { cleanQuery } from '../common/helpers/api-helper'; +import { Logger } from './logger'; @Injectable({ providedIn: 'root' }) export class JusticeUsersService { - private cache$: Observable; + loggerPrefix = '[JusticeUsersService] -'; private refresh$: BehaviorSubject = new BehaviorSubject(null); private searchTerm$: BehaviorSubject = new BehaviorSubject(null); users$ = this.refresh$.pipe( - mergeMap(() => this.requestJusticeUsers(null)), - shareReplay(1), - switchMap(users => - this.searchTerm$.pipe( - map(term => { - return this.applyFilter(term, users); - }) - ) - ), - tap(x => console.log(`Search results`, x)) + mergeMap(() => this.getJusticeUsers(null)), + shareReplay(1) ); - constructor(private apiClient: BHClient) {} + filteredUsers$ = this.users$.pipe(switchMap(users => this.searchTerm$.pipe(map(term => this.applyFilter(term, users))))); + + constructor(private apiClient: BHClient, private logger: Logger) {} - // just for testing -- remove - // triggers an emission on users$ refresh() { this.refresh$.next(); } - // push a search term into the stream search(searchTerm: string) { - console.log(`Searching with ${searchTerm}`); this.searchTerm$.next(searchTerm); } @@ -47,22 +38,6 @@ export class JusticeUsersService { return users.filter(user => user.first_name === searchTerm); } - retrieveJusticeUserAccounts() { - if (!this.cache$) { - this.cache$ = this.requestJusticeUsers(null).pipe(shareReplay(1)); - } - - return this.cache$; - } - - retrieveJusticeUserAccountsNoCache(term: string) { - return this.requestJusticeUsers(term).pipe(shareReplay(1)); - } - - private requestJusticeUsers(term: string) { - return this.apiClient.getUserList(cleanQuery(term)); - } - addNewJusticeUser(username: string, firstName: string, lastName: string, telephone: string, role: JusticeUserRole) { const request = new AddJusticeUserRequest({ username: username, @@ -84,6 +59,15 @@ export class JusticeUsersService { } deleteJusticeUser(id: string) { - return this.apiClient.deleteJusticeUser(id); + return this.apiClient.deleteJusticeUser(id).pipe(tap(() => this.refresh$.next())); + } + + private getJusticeUsers(term: string) { + return this.apiClient.getUserList(cleanQuery(term)).pipe( + catchError(error => { + this.logger.error(`${this.loggerPrefix} There was an unexpected error getting justice users`, new Error(error)); + return throwError(error); + }) + ); } } diff --git a/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.html b/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.html index 009b4d2b2..cd21970a6 100644 --- a/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.html +++ b/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.html @@ -4,7 +4,7 @@ id="user-list" class="custom" labelForId="users" - [items]="users" + [items]="users$ | async" bindLabel="full_name" bindValue="id" formControlName="selectedUserIds" diff --git a/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.spec.ts b/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.spec.ts index 3d91db040..273d1acfe 100644 --- a/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.spec.ts +++ b/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.spec.ts @@ -4,8 +4,6 @@ import { HttpClient, HttpHandler } from '@angular/common/http'; import { FormBuilder } from '@angular/forms'; import { MockLogger } from '../../testing/mock-logger'; import { Logger } from '../../../services/logger'; -import { of, throwError } from 'rxjs'; -import { JusticeUserResponse } from '../../../services/clients/api-client'; import { JusticeUsersService } from 'src/app/services/justice-users.service'; describe('JusticeUsersMenuComponent', () => { @@ -15,7 +13,6 @@ describe('JusticeUsersMenuComponent', () => { beforeEach(async () => { justiceUsersServiceSpy = jasmine.createSpyObj('JusticeUsersService', ['retrieveJusticeUserAccounts']); - justiceUsersServiceSpy.retrieveJusticeUserAccounts.and.returnValue(of([new JusticeUserResponse()])); await TestBed.configureTestingModule({ declarations: [JusticeUsersMenuComponent], @@ -54,22 +51,4 @@ describe('JusticeUsersMenuComponent', () => { expect(component.form.controls[component.formGroupName].enabled).toEqual(false); }); }); - - describe('loadItems', () => { - it('should call video hearing service', () => { - const expectedResponse = [new JusticeUserResponse()]; - component.loadItems(); - expect(justiceUsersServiceSpy.retrieveJusticeUserAccounts).toHaveBeenCalled(); - expect(component.users).toEqual(expectedResponse); - }); - - it('should call video hearing service, and catch thrown exception', () => { - justiceUsersServiceSpy.retrieveJusticeUserAccounts.and.returnValue(throwError({ status: 404 })); - - const handleListErrorSpy = spyOn(component, 'handleListError'); - component.loadItems(); - expect(justiceUsersServiceSpy.retrieveJusticeUserAccounts).toHaveBeenCalled(); - expect(handleListErrorSpy).toHaveBeenCalled(); - }); - }); }); diff --git a/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.ts b/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.ts index 920c96c1a..9e10bd2b2 100644 --- a/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.ts +++ b/AdminWebsite/AdminWebsite/ClientApp/src/app/shared/menus/justice-users-menu/justice-users-menu.component.ts @@ -1,20 +1,21 @@ -import { Component, EventEmitter, Input, Output } from '@angular/core'; +import { Component, EventEmitter, Input, OnInit, Output } from '@angular/core'; import { JusticeUserResponse } from '../../../services/clients/api-client'; import { FormBuilder } from '@angular/forms'; import { BookingPersistService } from '../../../services/bookings-persist.service'; import { JusticeUsersService } from '../../../services/justice-users.service'; import { Logger } from '../../../services/logger'; import { MenuBase } from '../menu-base'; +import { Observable } from 'rxjs'; @Component({ selector: 'app-justice-users-menu', templateUrl: './justice-users-menu.component.html', styleUrls: ['./justice-users-menu.component.scss'] }) -export class JusticeUsersMenuComponent extends MenuBase { +export class JusticeUsersMenuComponent extends MenuBase implements OnInit { loggerPrefix = '[MenuJusticeUser] -'; formGroupName = 'selectedUserIds'; - users: JusticeUserResponse[]; + users$: Observable; selectedItems: [] | string; formConfiguration = { selectedUserIds: [this.bookingPersistService.selectedUsers || []] @@ -32,14 +33,10 @@ export class JusticeUsersMenuComponent extends MenuBase { super(formBuilder, logger); } - loadItems(): void { - this.justiceUserService.retrieveJusticeUserAccounts().subscribe( - (data: JusticeUserResponse[]) => { - this.users = data; - this.items = data; - this.logger.debug(`${this.loggerPrefix} Updating list of users.`, { users: data.length }); - }, - error => this.handleListError(error, 'users') - ); + ngOnInit(): void { + this.users$ = this.justiceUserService.users$; + super.ngOnInit(); } + + loadItems(): void {} } diff --git a/AdminWebsite/AdminWebsite/ClientApp/src/app/work-allocation/manage-team/manage-team.component.html b/AdminWebsite/AdminWebsite/ClientApp/src/app/work-allocation/manage-team/manage-team.component.html index f2f29b6ae..b75be1eae 100644 --- a/AdminWebsite/AdminWebsite/ClientApp/src/app/work-allocation/manage-team/manage-team.component.html +++ b/AdminWebsite/AdminWebsite/ClientApp/src/app/work-allocation/manage-team/manage-team.component.html @@ -11,10 +11,15 @@
- -
+
@@ -24,65 +29,69 @@
-
+ +
- {{ message }} + {{ message | async }}
- - - - - - - - - - - - - - - - - - - - - - - -
UsernameFirst nameLast nameContact telephoneRole
- {{ user.username }} - - {{ user.first_name }} - - {{ user.lastname }} - - {{ user.telephone }} - - {{ user.is_vh_team_leader ? 'Administrator' : 'CSO' }} - - - - -
-