From 18c208f6a4cb7e21e23d6904b2214fbd773cee8c Mon Sep 17 00:00:00 2001 From: Kristof De Langhe Date: Wed, 31 Aug 2022 18:02:36 +0200 Subject: [PATCH] 94072: Group page list issue fixes --- .../group-form/group-form.component.ts | 4 +-- .../members-list/members-list.component.ts | 15 +++++----- .../remote-data-build.service.spec.ts | 18 +++++++++++ .../builders/remote-data-build.service.ts | 30 ++++++++++++++----- .../shared/mocks/object-cache.service.mock.ts | 3 +- 5 files changed, 52 insertions(+), 18 deletions(-) diff --git a/src/app/access-control/group-registry/group-form/group-form.component.ts b/src/app/access-control/group-registry/group-form/group-form.component.ts index 337f4470c1..b0178f1294 100644 --- a/src/app/access-control/group-registry/group-form/group-form.component.ts +++ b/src/app/access-control/group-registry/group-form/group-form.component.ts @@ -377,9 +377,7 @@ export class GroupFormComponent implements OnInit, OnDestroy { */ setActiveGroup(groupId: string) { this.groupDataService.cancelEditGroup(); - const nextGroup$ = this.groupDataService.findById(groupId); - this.subs.push(nextGroup$.subscribe()); - nextGroup$ + this.groupDataService.findById(groupId) .pipe( getFirstSucceededRemoteData(), getRemoteDataPayload()) diff --git a/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts b/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts index b092afa40e..d777c2ca52 100644 --- a/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts +++ b/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts @@ -10,7 +10,7 @@ import { combineLatest as observableCombineLatest, ObservedValueOf, } from 'rxjs'; -import { map, mergeMap, switchMap, take } from 'rxjs/operators'; +import { defaultIfEmpty, map, mergeMap, switchMap, take } from 'rxjs/operators'; import {buildPaginatedList, PaginatedList} from '../../../../core/data/paginated-list.model'; import { RemoteData } from '../../../../core/data/remote-data'; import { EPersonDataService } from '../../../../core/eperson/eperson-data.service'; @@ -25,6 +25,7 @@ import { NotificationsService } from '../../../../shared/notifications/notificat import { PaginationComponentOptions } from '../../../../shared/pagination/pagination-component-options.model'; import {EpersonDtoModel} from '../../../../core/eperson/models/eperson-dto.model'; import { PaginationService } from '../../../../core/pagination/pagination.service'; +import { isNotEmpty } from '../../../../shared/empty.util'; /** * Keys to keep track of specific subscriptions @@ -144,7 +145,7 @@ export class MembersListComponent implements OnInit, OnDestroy { } }), switchMap((epersonListRD: RemoteData>) => { - const dtos$ = observableCombineLatest(...epersonListRD.payload.page.map((member: EPerson) => { + const dtos$ = observableCombineLatest([...epersonListRD.payload.page.map((member: EPerson) => { const dto$: Observable = observableCombineLatest( this.isMemberOfGroup(member), (isMember: ObservedValueOf>) => { const epersonDtoModel: EpersonDtoModel = new EpersonDtoModel(); @@ -153,8 +154,8 @@ export class MembersListComponent implements OnInit, OnDestroy { return epersonDtoModel; }); return dto$; - })); - return dtos$.pipe(map((dtos: EpersonDtoModel[]) => { + })]); + return dtos$.pipe(defaultIfEmpty([]), map((dtos: EpersonDtoModel[]) => { return buildPaginatedList(epersonListRD.payload.pageInfo, dtos); })); })) @@ -274,7 +275,7 @@ export class MembersListComponent implements OnInit, OnDestroy { } }), switchMap((epersonListRD: RemoteData>) => { - const dtos$ = observableCombineLatest(...epersonListRD.payload.page.map((member: EPerson) => { + const dtos$ = observableCombineLatest([...epersonListRD.payload.page.map((member: EPerson) => { const dto$: Observable = observableCombineLatest( this.isMemberOfGroup(member), (isMember: ObservedValueOf>) => { const epersonDtoModel: EpersonDtoModel = new EpersonDtoModel(); @@ -283,8 +284,8 @@ export class MembersListComponent implements OnInit, OnDestroy { return epersonDtoModel; }); return dto$; - })); - return dtos$.pipe(map((dtos: EpersonDtoModel[]) => { + })]); + return dtos$.pipe(defaultIfEmpty([]), map((dtos: EpersonDtoModel[]) => { return buildPaginatedList(epersonListRD.payload.pageInfo, dtos); })); })) diff --git a/src/app/core/cache/builders/remote-data-build.service.spec.ts b/src/app/core/cache/builders/remote-data-build.service.spec.ts index 1d22da494f..ef79121753 100644 --- a/src/app/core/cache/builders/remote-data-build.service.spec.ts +++ b/src/app/core/cache/builders/remote-data-build.service.spec.ts @@ -18,6 +18,7 @@ import { take } from 'rxjs/operators'; import { HALLink } from '../../shared/hal-link.model'; import { RequestEntryState } from '../../data/request-entry-state.model'; import { RequestEntry } from '../../data/request-entry.model'; +import { cold } from 'jasmine-marbles'; describe('RemoteDataBuildService', () => { let service: RemoteDataBuildService; @@ -646,4 +647,21 @@ describe('RemoteDataBuildService', () => { }); }); }); + + describe('buildFromHref', () => { + beforeEach(() => { + (objectCache.getRequestUUIDBySelfLink as jasmine.Spy).and.returnValue(cold('a', { a: 'request/uuid' })); + }); + + describe('when both getRequestFromRequestHref and getRequestFromRequestUUID emit nothing', () => { + beforeEach(() => { + (requestService.getByHref as jasmine.Spy).and.returnValue(cold('a', { a: undefined })); + (requestService.getByUUID as jasmine.Spy).and.returnValue(cold('a', { a: undefined })); + }); + + it('should not emit anything', () => { + expect(service.buildFromHref(cold('a', { a: 'rest/api/endpoint' }))).toBeObservable(cold('')); + }); + }); + }); }); diff --git a/src/app/core/cache/builders/remote-data-build.service.ts b/src/app/core/cache/builders/remote-data-build.service.ts index 016f6b16f6..8ceab63e03 100644 --- a/src/app/core/cache/builders/remote-data-build.service.ts +++ b/src/app/core/cache/builders/remote-data-build.service.ts @@ -3,9 +3,8 @@ import { combineLatest as observableCombineLatest, Observable, of as observableOf, - race as observableRace } from 'rxjs'; -import { map, switchMap, filter, distinctUntilKeyChanged } from 'rxjs/operators'; +import { map, switchMap, filter, distinctUntilKeyChanged, startWith } from 'rxjs/operators'; import { hasValue, isEmpty, isNotEmpty, hasNoValue, isUndefined } from '../../../shared/empty.util'; import { createSuccessfulRemoteDataObject$ } from '../../../shared/remote-data.utils'; import { FollowLinkConfig, followLink } from '../../../shared/utils/follow-link-config.model'; @@ -21,7 +20,7 @@ import { HALResource } from '../../shared/hal-resource.model'; import { PAGINATED_LIST } from '../../data/paginated-list.resource-type'; import { getUrlWithoutEmbedParams } from '../../index/index.selectors'; import { getResourceTypeValueFor } from '../object-cache.reducer'; -import { hasSucceeded, RequestEntryState } from '../../data/request-entry-state.model'; +import { hasSucceeded, isStale, RequestEntryState } from '../../data/request-entry-state.model'; import { getRequestFromRequestHref, getRequestFromRequestUUID } from '../../shared/request.operators'; import { RequestEntry } from '../../data/request-entry.model'; import { ResponseState } from '../../data/response-state.model'; @@ -207,10 +206,27 @@ export class RemoteDataBuildService { this.objectCache.getRequestUUIDBySelfLink(href)), ); - const requestEntry$ = observableRace( - href$.pipe(getRequestFromRequestHref(this.requestService)), - requestUUID$.pipe(getRequestFromRequestUUID(this.requestService)), - ).pipe( + const requestEntry$ = observableCombineLatest([ + href$.pipe(getRequestFromRequestHref(this.requestService), startWith(undefined)), + requestUUID$.pipe(getRequestFromRequestUUID(this.requestService), startWith(undefined)), + ]).pipe( + filter(([r1, r2]) => hasValue(r1) || hasValue(r2)), + map(([r1, r2]) => { + // If one of the two requests has no value, return the other (both is impossible due to the filter above) + if (hasNoValue(r2)) { + return r1; + } else if (hasNoValue(r1)) { + return r2; + } + + if ((isStale(r1.state) && isStale(r2.state)) || (!isStale(r1.state) && !isStale(r2.state))) { + // Neither or both are stale, pick the most recent request + return r1.lastUpdated >= r2.lastUpdated ? r1 : r2; + } else { + // One of the two is stale, return the not stale request + return isStale(r2.state) ? r1 : r2; + } + }), distinctUntilKeyChanged('lastUpdated') ); diff --git a/src/app/shared/mocks/object-cache.service.mock.ts b/src/app/shared/mocks/object-cache.service.mock.ts index 144c82fe16..515c2abe86 100644 --- a/src/app/shared/mocks/object-cache.service.mock.ts +++ b/src/app/shared/mocks/object-cache.service.mock.ts @@ -11,7 +11,8 @@ export function getMockObjectCacheService(): ObjectCacheService { 'getRequestHrefByUUID', 'getList', 'hasByUUID', - 'hasByHref' + 'hasByHref', + 'getRequestUUIDBySelfLink', ]); }