[ACS-12288] Fetch all aspects for aspect list component (#12105)

* [ACS-12288] Fetch all aspects for aspect list component

* [ACS-12288] CR fixes

* [ACS-12288] Unit test fix

lib/content-services/src/lib/aspect-list/aspect-list-dialog.component.spec.ts

@@ -20,7 +20,7 @@ import { ComponentFixture, TestBed } from '@angular/core/testing';
 import { AspectListDialogComponent } from './aspect-list-dialog.component';
 import { of, Subject } from 'rxjs';
 import { AspectListDialogComponentData } from './aspect-list-dialog-data.interface';
-import { AspectListService } from './services/aspect-list.service';
+import { AspectListService, StandardAspectsWhere } from './services/aspect-list.service';
 import { delay } from 'rxjs/operators';
 import { AspectEntry, Node } from '@alfresco/js-api';
 import { NodesApiService } from '../common/services/nodes-api.service';
@@ -144,8 +144,8 @@ describe('AspectListDialogComponent', () => {
     describe('Without passing node id', () => {
         beforeEach(() => {
             aspectListService = TestBed.inject(AspectListService);
-            spyOn(aspectListService, 'getAllAspects').and.returnValue(
-                of({ standardAspectPaging: { list: { entries: aspectListMock } }, customAspectPaging: { list: { entries: customAspectListMock } } })
+            spyOn(aspectListService, 'getAspects').and.callFake((_visibleAspects, opts) =>
+                of({ list: { entries: opts.where === StandardAspectsWhere ? aspectListMock : customAspectListMock } })
             );
             fixture.detectChanges();
         });
@@ -251,11 +251,10 @@ describe('AspectListDialogComponent', () => {
             data.nodeId = 'fake-node-id';
             aspectListService = TestBed.inject(AspectListService);
             nodeService = TestBed.inject(NodesApiService);
-            spyOn(aspectListService, 'getAllAspects').and.returnValue(
-                of({ standardAspectPaging: { list: { entries: aspectListMock } }, customAspectPaging: { list: { entries: customAspectListMock } } })
-            );
             spyOn(aspectListService, 'getVisibleAspects').and.returnValue(['frs:AspectOne']);
-            spyOn(aspectListService, 'getAspects').and.returnValue(of({ list: { entries: customAspectListMock } }));
+            spyOn(aspectListService, 'getAspects').and.callFake((_visibleAspects, opts) =>
+                of({ list: { entries: opts.where === StandardAspectsWhere ? aspectListMock : customAspectListMock } })
+            );
             spyOn(nodeService, 'getNode').and.returnValue(
                 of(new Node({ id: 'fake-node-id', aspectNames: ['frs:AspectOne', 'cst:customAspect'] })).pipe(delay(0))
             );

lib/content-services/src/lib/aspect-list/aspect-list.component.spec.ts

@@ -19,15 +19,14 @@ import { ComponentFixture, TestBed } from '@angular/core/testing';
 import { NodesApiService } from '../common/services/nodes-api.service';
 import { AspectListComponent } from './aspect-list.component';
 import { AspectListService, CustomAspectsWhere, StandardAspectsWhere } from './services/aspect-list.service';
-import { EMPTY, of } from 'rxjs';
-import { AspectEntry, Pagination } from '@alfresco/js-api';
+import { NEVER, of } from 'rxjs';
+import { AspectEntry } from '@alfresco/js-api';
 import { HarnessLoader } from '@angular/cdk/testing';
 import { TestbedHarnessEnvironment } from '@angular/cdk/testing/testbed';
 import { MatExpansionPanelHarness } from '@angular/material/expansion/testing';
 import { MatTableHarness } from '@angular/material/table/testing';
 import { MatCheckboxHarness } from '@angular/material/checkbox/testing';
 import { MatProgressSpinnerHarness } from '@angular/material/progress-spinner/testing';
-import { CustomAspectPaging } from './interfaces/custom-aspect-paging.interface';
 import { provideApiTesting } from '../testing/providers';
 
 const aspectListMock: AspectEntry[] = [
@@ -111,11 +110,6 @@ const customAspectListMock: AspectEntry[] = [
     }
 ];
 
-const allAspectsMock: CustomAspectPaging = {
-    standardAspectPaging: { list: { entries: aspectListMock } },
-    customAspectPaging: { list: { entries: customAspectListMock } }
-};
-
 describe('AspectListComponent', () => {
     let loader: HarnessLoader;
     let component: AspectListComponent;
@@ -138,8 +132,8 @@ describe('AspectListComponent', () => {
 
     describe('Loading', () => {
         it('should show the loading spinner when result is loading', async () => {
-            spyOn(nodeService, 'getNode').and.returnValue(EMPTY);
-            spyOn(aspectListService, 'getAspects').and.returnValue(EMPTY);
+            spyOn(nodeService, 'getNode').and.returnValue(NEVER);
+            spyOn(aspectListService, 'getAspects').and.returnValue(NEVER);
             fixture.detectChanges();
 
             expect(await loader.hasHarness(MatProgressSpinnerHarness)).toBe(true);
@@ -148,8 +142,9 @@ describe('AspectListComponent', () => {
 
     describe('When passing a node id', () => {
         beforeEach(() => {
-            spyOn(aspectListService, 'getAllAspects').and.returnValue(of(allAspectsMock));
-            spyOn(aspectListService, 'getAspects').and.returnValue(of({ list: { entries: customAspectListMock } }));
+            spyOn(aspectListService, 'getAspects').and.callFake((_visibleAspects, opts) =>
+                of({ list: { entries: opts.where === StandardAspectsWhere ? aspectListMock : customAspectListMock } })
+            );
             spyOn(aspectListService, 'getVisibleAspects').and.returnValue(['frs:AspectOne']);
             spyOn(nodeService, 'getNode').and.returnValue(of({ id: 'fake-node-id', aspectNames: ['frs:AspectOne', 'stored:aspect'] } as any));
             component.nodeId = 'fake-node-id';
@@ -252,10 +247,18 @@ describe('AspectListComponent', () => {
             });
 
             it('should load aspects with 0 skip count as pagination by default', () => {
-                expect(aspectListService.getAllAspects).toHaveBeenCalledWith(
-                    { where: StandardAspectsWhere, include: ['properties'], skipCount: 0, maxItems: 100 },
-                    { where: CustomAspectsWhere, include: ['properties'], skipCount: 0, maxItems: 100 }
-                );
+                expect(aspectListService.getAspects).toHaveBeenCalledWith(jasmine.anything(), {
+                    where: StandardAspectsWhere,
+                    include: ['properties'],
+                    skipCount: 0,
+                    maxItems: 100
+                });
+                expect(aspectListService.getAspects).toHaveBeenCalledWith(jasmine.anything(), {
+                    where: CustomAspectsWhere,
+                    include: ['properties'],
+                    skipCount: 0,
+                    maxItems: 100
+                });
             });
         });
 
@@ -271,7 +274,9 @@ describe('AspectListComponent', () => {
 
     describe('When no node id is passed', () => {
         beforeEach(() => {
-            spyOn(aspectListService, 'getAllAspects').and.returnValue(of(allAspectsMock));
+            spyOn(aspectListService, 'getAspects').and.callFake((_visibleAspects, opts) =>
+                of({ list: { entries: opts.where === StandardAspectsWhere ? aspectListMock : customAspectListMock } })
+            );
         });
 
         afterEach(() => {
@@ -294,35 +299,73 @@ describe('AspectListComponent', () => {
 
         it('should load aspects with 0 skip count as pagination by default', () => {
             fixture.detectChanges();
-            expect(aspectListService.getAllAspects).toHaveBeenCalledWith(
-                { where: StandardAspectsWhere, include: ['properties'], skipCount: 0, maxItems: 100 },
-                { where: CustomAspectsWhere, include: ['properties'], skipCount: 0, maxItems: 100 }
-            );
+            expect(aspectListService.getAspects).toHaveBeenCalledWith(jasmine.anything(), {
+                where: StandardAspectsWhere,
+                include: ['properties'],
+                skipCount: 0,
+                maxItems: 100
+            });
+            expect(aspectListService.getAspects).toHaveBeenCalledWith(jasmine.anything(), {
+                where: CustomAspectsWhere,
+                include: ['properties'],
+                skipCount: 0,
+                maxItems: 100
+            });
         });
     });
 
-    it('should load next batch of aspects if not all items were returned by first call', (done) => {
-        fixture.detectChanges();
-        const moreItemsPagination: Pagination = { count: 2, hasMoreItems: true };
-        const allAspectsWithMoreItems: CustomAspectPaging = {
-            standardAspectPaging: { list: { entries: aspectListMock, pagination: moreItemsPagination } },
-            customAspectPaging: { list: { entries: customAspectListMock, pagination: moreItemsPagination } }
-        };
-        const getAspectsSpy = spyOn(aspectListService, 'getAllAspects').and.returnValues(of(allAspectsWithMoreItems), of(allAspectsMock));
-        spyOn(aspectListService, 'getAspects').and.returnValues(
-            of({ list: { entries: aspectListMock } }),
-            of({ list: { entries: customAspectListMock } })
-        );
+    it('should keep requesting a type until its hasMoreItems is false, without re-querying an exhausted type', (done) => {
+        const getAspectsSpy = spyOn(aspectListService, 'getAspects').and.callFake((_visibleAspects, opts) => {
+            if (opts.where === StandardAspectsWhere) {
+                return opts.skipCount === 0
+                    ? of({ list: { entries: aspectListMock, pagination: { count: 2, hasMoreItems: true } } })
+                    : of({ list: { entries: aspectListMock, pagination: { count: 2, hasMoreItems: false } } });
+            }
+            return of({ list: { entries: customAspectListMock, pagination: { count: 2, hasMoreItems: false } } });
+        });
+        component.ngOnInit();
 
         component.aspects$.subscribe(() => {
-            expect(getAspectsSpy.calls.argsFor(0)[0]).toEqual({ where: StandardAspectsWhere, include: ['properties'], skipCount: 0, maxItems: 100 });
-            expect(getAspectsSpy.calls.argsFor(0)[1]).toEqual({ where: CustomAspectsWhere, include: ['properties'], skipCount: 0, maxItems: 100 });
-            expect(getAspectsSpy.calls.argsFor(1)[0]).toEqual({ where: StandardAspectsWhere, include: ['properties'], skipCount: 2, maxItems: 100 });
-            expect(getAspectsSpy.calls.argsFor(1)[1]).toEqual({ where: CustomAspectsWhere, include: ['properties'], skipCount: 2, maxItems: 100 });
+            const standardCalls = getAspectsSpy.calls.allArgs().filter(([, opts]) => opts.where === StandardAspectsWhere);
+            const customCalls = getAspectsSpy.calls.allArgs().filter(([, opts]) => opts.where === CustomAspectsWhere);
+            expect(standardCalls.map(([, opts]) => opts.skipCount)).toEqual([0, 2]);
+            expect(customCalls.map(([, opts]) => opts.skipCount)).toEqual([0]);
             done();
         });
+    });
 
+    it('should page through all aspects (standard and custom) when categorising node aspects', (done) => {
+        const standardPage1: AspectEntry[] = [{ entry: { id: 'std:first', title: 'First', properties: [] } }];
+        const standardPage2: AspectEntry[] = [{ entry: { id: 'std:second', title: 'Second', properties: [] } }];
+        spyOn(aspectListService, 'getVisibleAspects').and.returnValue([]);
+        spyOn(nodeService, 'getNode').and.returnValue(of({ id: 'node-id', aspectNames: ['std:second'] } as any));
+        const getAspectsSpy = spyOn(aspectListService, 'getAspects').and.callFake((_visibleAspects, opts) => {
+            if (opts.where === StandardAspectsWhere) {
+                return opts.skipCount === 0
+                    ? of({ list: { entries: standardPage1, pagination: { count: 1, hasMoreItems: true } } })
+                    : of({ list: { entries: standardPage2, pagination: { count: 1, hasMoreItems: false } } });
+            }
+            return of({ list: { entries: [], pagination: { count: 0, hasMoreItems: false } } });
+        });
+        component.nodeId = 'node-id';
         component.ngOnInit();
-        fixture.detectChanges();
+
+        component.aspects$.subscribe(() => {
+            expect(getAspectsSpy).toHaveBeenCalledWith(jasmine.anything(), {
+                where: StandardAspectsWhere,
+                include: ['properties'],
+                skipCount: 0,
+                maxItems: 100
+            });
+            expect(getAspectsSpy).toHaveBeenCalledWith(jasmine.anything(), {
+                where: StandardAspectsWhere,
+                include: ['properties'],
+                skipCount: 1,
+                maxItems: 100
+            });
+            expect(component.nodeAspects).toContain('std:second');
+            expect(component.notDisplayedAspects).not.toContain('std:second');
+            done();
+        });
     });
 });

lib/content-services/src/lib/aspect-list/aspect-list.component.ts

@@ -17,11 +17,11 @@
 
 import { Component, DestroyRef, EventEmitter, inject, Input, OnInit, Output, ViewEncapsulation } from '@angular/core';
 import { NodesApiService } from '../common/services/nodes-api.service';
-import { EMPTY, Observable, zip } from 'rxjs';
-import { concatMap, expand, map, reduce, take, tap } from 'rxjs/operators';
+import { EMPTY, forkJoin, Observable } from 'rxjs';
+import { expand, map, reduce } from 'rxjs/operators';
 import { AspectListService, CustomAspectsWhere, StandardAspectsWhere } from './services/aspect-list.service';
 import { MatCheckboxChange, MatCheckboxModule } from '@angular/material/checkbox';
-import { AspectEntry, ContentPagingQuery, ListAspectsOpts } from '@alfresco/js-api';
+import { AspectEntry, ListAspectsOpts, Node } from '@alfresco/js-api';
 import { CommonModule } from '@angular/common';
 import { MatExpansionModule } from '@angular/material/expansion';
 import { MatTableModule } from '@angular/material/table';
@@ -66,46 +66,19 @@ export class AspectListComponent implements OnInit {
 
     private readonly destroyRef = inject(DestroyRef);
 
-    private customAspectsLoaded = 0;
-    private standardAspectsLoaded = 0;
-    private hasMoreAspects = false;
-
     ngOnInit(): void {
-        let aspects$: Observable<AspectEntry[]>;
-        if (this.nodeId) {
-            const node$ = this.nodeApiService.getNode(this.nodeId);
-            const customAspect$ = this.aspectListService
-                .getAspects(this.aspectListService.getVisibleAspects(), {
-                    where: CustomAspectsWhere,
-                    include: ['properties'],
-                    skipCount: 0,
-                    maxItems: 100
-                })
-                .pipe(map((customAspects) => customAspects?.list?.entries.flatMap((customAspect) => customAspect.entry.id)));
-            aspects$ = zip(node$, customAspect$).pipe(
-                tap(([node, customAspects]) => {
-                    this.nodeAspects = node.aspectNames.filter(
-                        (aspect) => this.aspectListService.getVisibleAspects().includes(aspect) || customAspects.includes(aspect)
-                    );
-                    this.nodeAspectStatus = [...this.nodeAspects];
-                    this.notDisplayedAspects = node.aspectNames.filter(
-                        (aspect) => !this.aspectListService.getVisibleAspects().includes(aspect) && !customAspects.includes(aspect)
-                    );
-                    this.valueChanged.emit([...this.nodeAspects, ...this.notDisplayedAspects]);
-                    this.updateCounter.emit(this.nodeAspects.length);
-                }),
-                concatMap(() => this.loadAspects({ skipCount: this.standardAspectsLoaded }, { skipCount: this.customAspectsLoaded })),
-                takeUntilDestroyed(this.destroyRef)
-            );
-        } else {
-            aspects$ = this.loadAspects({ skipCount: this.standardAspectsLoaded }, { skipCount: this.customAspectsLoaded });
-        }
-        this.aspects$ = aspects$.pipe(
-            expand(() =>
-                this.hasMoreAspects ? this.loadAspects({ skipCount: this.standardAspectsLoaded }, { skipCount: this.customAspectsLoaded }) : EMPTY
-            ),
-            map((aspects) => aspects.filter((aspect) => !this.excludedAspects.includes(aspect.entry.id))),
-            reduce((acc, aspects) => [...acc, ...aspects])
+        const allAspects$ = this.loadAllAspects();
+        const displayAspects$ = this.nodeId
+            ? forkJoin([this.nodeApiService.getNode(this.nodeId), allAspects$]).pipe(
+                  map(([node, allAspects]) => {
+                      this.categoriseNodeAspects(node, allAspects);
+                      return allAspects;
+                  })
+              )
+            : allAspects$;
+        this.aspects$ = displayAspects$.pipe(
+            map((aspects) => aspects.filter((aspect) => !(this.excludedAspects ?? []).includes(aspect.entry.id))),
+            takeUntilDestroyed(this.destroyRef)
         );
     }
 
@@ -158,32 +131,38 @@ export class AspectListComponent implements OnInit {
         }
     }
 
-    private loadAspects(standardAspectsPagination?: ContentPagingQuery, customAspectsPagination?: ContentPagingQuery): Observable<AspectEntry[]> {
-        const standardAspectOpts: ListAspectsOpts = {
-            where: StandardAspectsWhere,
-            include: ['properties'],
-            skipCount: standardAspectsPagination?.skipCount ?? 0,
-            maxItems: 100
-        };
-        const customAspectOpts: ListAspectsOpts = {
-            where: CustomAspectsWhere,
-            include: ['properties'],
-            skipCount: customAspectsPagination?.skipCount ?? 0,
-            maxItems: 100
-        };
-        return this.aspectListService.getAllAspects(standardAspectOpts, customAspectOpts).pipe(
-            take(1),
-            tap((aspectsPaging) => {
-                this.customAspectsLoaded += aspectsPaging.customAspectPaging?.list?.pagination?.count ?? 0;
-                this.standardAspectsLoaded += aspectsPaging.standardAspectPaging?.list?.pagination?.count ?? 0;
-                this.hasMoreAspects =
-                    aspectsPaging.customAspectPaging?.list?.pagination?.hasMoreItems ||
-                    aspectsPaging.standardAspectPaging?.list?.pagination?.hasMoreItems;
-            }),
-            map((aspectsPaging) => [
-                ...(aspectsPaging.standardAspectPaging?.list?.entries ?? []),
-                ...(aspectsPaging.customAspectPaging?.list?.entries ?? [])
-            ])
+    private loadAllAspects(): Observable<AspectEntry[]> {
+        return forkJoin([this.loadAllAspectsOfType(StandardAspectsWhere), this.loadAllAspectsOfType(CustomAspectsWhere)]).pipe(
+            map(([standardAspects, customAspects]) => [...standardAspects, ...customAspects])
         );
     }
+
+    private loadAllAspectsOfType(where: string): Observable<AspectEntry[]> {
+        const visibleAspects = this.aspectListService.getVisibleAspects();
+        const fetchPage = (skipCount: number): Observable<{ entries: AspectEntry[]; skipCount: number; hasMoreItems: boolean }> => {
+            const opts: ListAspectsOpts = { where, include: ['properties'], skipCount, maxItems: 100 };
+            return this.aspectListService.getAspects(visibleAspects, opts).pipe(
+                map((aspectPaging) => ({
+                    entries: aspectPaging?.list?.entries ?? [],
+                    skipCount: skipCount + (aspectPaging?.list?.pagination?.count ?? 0),
+                    hasMoreItems: aspectPaging?.list?.pagination?.hasMoreItems ?? false
+                }))
+            );
+        };
+        return fetchPage(0).pipe(
+            expand((page) => (page.hasMoreItems ? fetchPage(page.skipCount) : EMPTY)),
+            reduce((allEntries, page) => [...allEntries, ...page.entries], [] as AspectEntry[])
+        );
+    }
+
+    private categoriseNodeAspects(node: Node, allAspects: AspectEntry[]): void {
+        const allAspectIds = allAspects.map((aspect) => aspect.entry.id);
+        const visibleAspects = this.aspectListService.getVisibleAspects();
+        const aspectNames = node.aspectNames ?? [];
+        this.nodeAspects = aspectNames.filter((aspect) => visibleAspects.includes(aspect) || allAspectIds.includes(aspect));
+        this.nodeAspectStatus = [...this.nodeAspects];
+        this.notDisplayedAspects = aspectNames.filter((aspect) => !visibleAspects.includes(aspect) && !allAspectIds.includes(aspect));
+        this.valueChanged.emit([...this.nodeAspects, ...this.notDisplayedAspects]);
+        this.updateCounter.emit(this.nodeAspects.length);
+    }
 }