Fix/refactor: remove doc observables, fix username async (#8908)

src-ui/src/app/pipes/correspondent-name.pipe.spec.ts

@@ -0,0 +1,28 @@
+import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'
+import { provideHttpClientTesting } from '@angular/common/http/testing'
+import { TestBed } from '@angular/core/testing'
+import { PermissionsService } from '../services/permissions.service'
+import { CorrespondentService } from '../services/rest/correspondent.service'
+import { CorrespondentNamePipe } from './correspondent-name.pipe'
+
+describe('CorrespondentNamePipe', () => {
+  let pipe: CorrespondentNamePipe
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({
+      providers: [
+        provideHttpClient(withInterceptorsFromDi()),
+        provideHttpClientTesting(),
+      ],
+    })
+  })
+
+  // The pipe is a simple wrapper around ObjectNamePipe, see ObjectNamePipe for the actual tests.
+  it('should be created', () => {
+    pipe = new CorrespondentNamePipe(
+      TestBed.inject(PermissionsService),
+      TestBed.inject(CorrespondentService)
+    )
+    expect(pipe).toBeTruthy()
+  })
+})

src-ui/src/app/pipes/correspondent-name.pipe.ts

@@ -0,0 +1,22 @@
+import { Pipe, PipeTransform } from '@angular/core'
+import {
+  PermissionsService,
+  PermissionType,
+} from '../services/permissions.service'
+import { CorrespondentService } from '../services/rest/correspondent.service'
+import { ObjectNamePipe } from './object-name.pipe'
+
+@Pipe({
+  name: 'correspondentName',
+})
+export class CorrespondentNamePipe
+  extends ObjectNamePipe
+  implements PipeTransform
+{
+  constructor(
+    permissionsService: PermissionsService,
+    objectService: CorrespondentService
+  ) {
+    super(permissionsService, PermissionType.Correspondent, objectService)
+  }
+}

src-ui/src/app/pipes/document-type-name.pipe.spec.ts

@@ -0,0 +1,28 @@
+import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'
+import { provideHttpClientTesting } from '@angular/common/http/testing'
+import { TestBed } from '@angular/core/testing'
+import { PermissionsService } from '../services/permissions.service'
+import { DocumentTypeService } from '../services/rest/document-type.service'
+import { DocumentTypeNamePipe } from './document-type-name.pipe'
+
+describe('DocumentTypeNamePipe', () => {
+  let pipe: DocumentTypeNamePipe
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({
+      providers: [
+        provideHttpClient(withInterceptorsFromDi()),
+        provideHttpClientTesting(),
+      ],
+    })
+  })
+
+  // The pipe is a simple wrapper around ObjectNamePipe, see ObjectNamePipe for the actual tests.
+  it('should be created', () => {
+    pipe = new DocumentTypeNamePipe(
+      TestBed.inject(PermissionsService),
+      TestBed.inject(DocumentTypeService)
+    )
+    expect(pipe).toBeTruthy()
+  })
+})

src-ui/src/app/pipes/document-type-name.pipe.ts

@@ -0,0 +1,22 @@
+import { Pipe, PipeTransform } from '@angular/core'
+import {
+  PermissionsService,
+  PermissionType,
+} from '../services/permissions.service'
+import { DocumentTypeService } from '../services/rest/document-type.service'
+import { ObjectNamePipe } from './object-name.pipe'
+
+@Pipe({
+  name: 'documentTypeName',
+})
+export class DocumentTypeNamePipe
+  extends ObjectNamePipe
+  implements PipeTransform
+{
+  constructor(
+    permissionsService: PermissionsService,
+    objectService: DocumentTypeService
+  ) {
+    super(permissionsService, PermissionType.DocumentType, objectService)
+  }
+}

src-ui/src/app/pipes/object-name.pipe.spec.ts

@@ -0,0 +1,88 @@
+import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'
+import { provideHttpClientTesting } from '@angular/common/http/testing'
+import { TestBed } from '@angular/core/testing'
+import { of, throwError } from 'rxjs'
+import { MatchingModel } from '../data/matching-model'
+import { PermissionsService } from '../services/permissions.service'
+import { AbstractNameFilterService } from '../services/rest/abstract-name-filter-service'
+import { CorrespondentService } from '../services/rest/correspondent.service'
+import { CorrespondentNamePipe } from './correspondent-name.pipe'
+import { ObjectNamePipe } from './object-name.pipe'
+
+describe('ObjectNamePipe', () => {
+  /*
+    ObjectNamePipe is an abstract class to prevent instantiation,
+    so we test the concrete implementation CorrespondentNamePipe instead.
+  */
+  let pipe: CorrespondentNamePipe
+  let permissionsService: PermissionsService
+  let objectService: AbstractNameFilterService<MatchingModel>
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({
+      providers: [
+        ObjectNamePipe,
+        provideHttpClient(withInterceptorsFromDi()),
+        provideHttpClientTesting(),
+      ],
+    })
+
+    permissionsService = TestBed.inject(PermissionsService)
+    objectService = TestBed.inject(CorrespondentService)
+    pipe = new CorrespondentNamePipe(permissionsService, objectService)
+  })
+
+  it('should return object name if user has permission', (done) => {
+    const mockObjects = {
+      results: [
+        { id: 1, name: 'Object 1' },
+        { id: 2, name: 'Object 2' },
+      ],
+      count: 2,
+      all: [1, 2],
+    }
+    jest.spyOn(permissionsService, 'currentUserCan').mockReturnValue(true)
+    jest.spyOn(objectService, 'listAll').mockReturnValue(of(mockObjects))
+
+    pipe.transform(1).subscribe((result) => {
+      expect(result).toBe('Object 1')
+      done()
+    })
+  })
+
+  it('should return empty string if object not found', (done) => {
+    const mockObjects = {
+      results: [{ id: 2, name: 'Object 2' }],
+      count: 1,
+      all: [2],
+    }
+    jest.spyOn(permissionsService, 'currentUserCan').mockReturnValue(true)
+    jest.spyOn(objectService, 'listAll').mockReturnValue(of(mockObjects))
+
+    pipe.transform(1).subscribe((result) => {
+      expect(result).toBe('')
+      done()
+    })
+  })
+
+  it('should return "Private" if user does not have permission', (done) => {
+    jest.spyOn(permissionsService, 'currentUserCan').mockReturnValue(false)
+
+    pipe.transform(1).subscribe((result) => {
+      expect(result).toBe('Private')
+      done()
+    })
+  })
+
+  it('should handle error and return empty string', (done) => {
+    jest.spyOn(permissionsService, 'currentUserCan').mockReturnValue(true)
+    jest
+      .spyOn(objectService, 'listAll')
+      .mockReturnValueOnce(throwError(() => new Error('Error getting objects')))
+
+    pipe.transform(1).subscribe((result) => {
+      expect(result).toBe('')
+      done()
+    })
+  })
+})

src-ui/src/app/pipes/object-name.pipe.ts

@@ -0,0 +1,46 @@
+import { Pipe, PipeTransform } from '@angular/core'
+import { catchError, map, Observable, of } from 'rxjs'
+import { MatchingModel } from '../data/matching-model'
+import {
+  PermissionAction,
+  PermissionsService,
+  PermissionType,
+} from '../services/permissions.service'
+import { AbstractNameFilterService } from '../services/rest/abstract-name-filter-service'
+
+@Pipe({
+  name: 'objectName',
+})
+export abstract class ObjectNamePipe implements PipeTransform {
+  /*
+    ObjectNamePipe is an abstract class to prevent instantiation,
+    object-specific pipes extend this class and provide the
+    correct permission type, and object service.
+  */
+  protected objects: MatchingModel[]
+
+  constructor(
+    protected permissionsService: PermissionsService,
+    protected permissionType: PermissionType,
+    protected objectService: AbstractNameFilterService<MatchingModel>
+  ) {}
+
+  transform(obejctId: number): Observable<string> {
+    if (
+      this.permissionsService.currentUserCan(
+        PermissionAction.View,
+        this.permissionType
+      )
+    ) {
+      return this.objectService.listAll().pipe(
+        map((objects) => {
+          this.objects = objects.results
+          return this.objects.find((o) => o.id === obejctId)?.name || ''
+        }),
+        catchError(() => of(''))
+      )
+    } else {
+      return of($localize`Private`)
+    }
+  }
+}

src-ui/src/app/pipes/storage-path-name.pipe.spec.ts

@@ -0,0 +1,28 @@
+import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http'
+import { provideHttpClientTesting } from '@angular/common/http/testing'
+import { TestBed } from '@angular/core/testing'
+import { PermissionsService } from '../services/permissions.service'
+import { StoragePathService } from '../services/rest/storage-path.service'
+import { StoragePathNamePipe } from './storage-path-name.pipe'
+
+describe('StoragePathNamePipe', () => {
+  let pipe: StoragePathNamePipe
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({
+      providers: [
+        provideHttpClient(withInterceptorsFromDi()),
+        provideHttpClientTesting(),
+      ],
+    })
+  })
+
+  // The pipe is a simple wrapper around ObjectNamePipe, see ObjectNamePipe for the actual tests.
+  it('should be created', () => {
+    pipe = new StoragePathNamePipe(
+      TestBed.inject(PermissionsService),
+      TestBed.inject(StoragePathService)
+    )
+    expect(pipe).toBeTruthy()
+  })
+})

src-ui/src/app/pipes/storage-path-name.pipe.ts

@@ -0,0 +1,22 @@
+import { Pipe, PipeTransform } from '@angular/core'
+import {
+  PermissionsService,
+  PermissionType,
+} from '../services/permissions.service'
+import { StoragePathService } from '../services/rest/storage-path.service'
+import { ObjectNamePipe } from './object-name.pipe'
+
+@Pipe({
+  name: 'storagePathName',
+})
+export class StoragePathNamePipe
+  extends ObjectNamePipe
+  implements PipeTransform
+{
+  constructor(
+    permissionsService: PermissionsService,
+    objectService: StoragePathService
+  ) {
+    super(permissionsService, PermissionType.StoragePath, objectService)
+  }
+}

src-ui/src/app/pipes/username.pipe.spec.ts

@@ -37,7 +37,11 @@ describe('UsernamePipe', () => {
     httpTestingController.verify()
   })
 
-  it('should transform user id to username', () => {
+  it('should transform user id to username', (done) => {
+    pipe.transform(2).subscribe((username) => {
+      expect(username).toEqual('username2')
+    })
+
     const req = httpTestingController.expectOne(
       `${environment.apiBaseUrl}users/?page=1&page_size=100000`
     )
@@ -55,24 +59,39 @@ describe('UsernamePipe', () => {
         },
       ],
     })
+    pipe.transform(3).subscribe((username) => {
+      expect(username).toEqual('User Name3')
+    })
 
-    let username = pipe.transform(2)
-    expect(username).toEqual('username2')
-
-    username = pipe.transform(3)
-    expect(username).toEqual('User Name3')
-
-    username = pipe.transform(4)
-    expect(username).toEqual('')
+    pipe.transform(4).subscribe((username) => {
+      expect(username).toEqual('')
+      done()
+    })
   })
 
-  it('should show generic label when no users retrieved', () => {
+  it('should show generic label when insufficient permissions', (done) => {
+    jest
+      .spyOn(permissionsService, 'currentUserCan')
+      .mockImplementation((action, type) => {
+        return false
+      })
+    pipe.transform(4).subscribe((username) => {
+      expect(username).toEqual('Shared')
+      done()
+    })
+    httpTestingController.expectNone(
+      `${environment.apiBaseUrl}users/?page=1&page_size=100000`
+    )
+  })
+
+  it('should show empty string when no users retrieved due to error', (done) => {
+    pipe.transform(4).subscribe((username) => {
+      expect(username).toEqual('')
+      done()
+    })
     const req = httpTestingController.expectOne(
       `${environment.apiBaseUrl}users/?page=1&page_size=100000`
     )
-    req.flush(null)
-
-    let username = pipe.transform(4)
-    expect(username).toEqual('Shared')
+    req.error(new ProgressEvent('error'))
   })
 })

src-ui/src/app/pipes/username.pipe.ts

@@ -1,9 +1,10 @@
 import { Pipe, PipeTransform } from '@angular/core'
+import { catchError, map, Observable, of } from 'rxjs'
 import { User } from '../data/user'
 import {
   PermissionAction,
-  PermissionType,
   PermissionsService,
+  PermissionType,
 } from '../services/permissions.service'
 import { UserService } from '../services/rest/user.service'
 
@@ -14,25 +15,29 @@ export class UsernamePipe implements PipeTransform {
   users: User[]
 
   constructor(
-    permissionsService: PermissionsService,
-    userService: UserService
-  ) {
+    private permissionsService: PermissionsService,
+    private userService: UserService
+  ) {}
+
+  transform(userID: number): Observable<string> {
     if (
-      permissionsService.currentUserCan(
+      this.permissionsService.currentUserCan(
         PermissionAction.View,
         PermissionType.User
       )
     ) {
-      userService.listAll().subscribe((r) => (this.users = r.results))
+      return this.userService.listAll().pipe(
+        map((users) => {
+          this.users = users.results
+          return this.getName(this.users.find((u) => u.id === userID))
+        }),
+        catchError(() => of(''))
+      )
+    } else {
+      return of($localize`Shared`)
     }
   }
 
-  transform(userID: number): string {
-    return this.users
-      ? (this.getName(this.users.find((u) => u.id === userID)) ?? '')
-      : $localize`Shared`
-  }
-
   getName(user: User): string {
     if (!user) return ''
     const name = [user.first_name, user.last_name].join(' ')