From 634836158bce6a1e30910bc8da831f5bfc3529c1 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Thu, 21 Jan 2021 14:30:59 +0100 Subject: [PATCH] fix issues with the group edit page --- .../members-list/members-list.component.html | 20 ++-- .../members-list.component.spec.ts | 2 +- .../members-list/members-list.component.ts | 93 ++++++++++++------- .../subgroups-list.component.html | 20 ++-- .../subgroups-list.component.spec.ts | 16 ++-- .../subgroup-list/subgroups-list.component.ts | 84 +++++++++++------ .../groups-registry.component.ts | 92 ++++++++++-------- src/app/core/eperson/eperson-data.service.ts | 2 +- src/app/core/eperson/group-data.service.ts | 2 +- 9 files changed, 202 insertions(+), 129 deletions(-) diff --git a/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.html b/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.html index d24bf35e2a..0ac67aff75 100644 --- a/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.html +++ b/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.html @@ -24,10 +24,10 @@ - @@ -42,7 +42,7 @@ - + {{ePerson.id}} {{ePerson.name}} @@ -70,7 +70,7 @@ - - @@ -36,7 +36,7 @@ - + {{group.id}} {{group.name}} @@ -65,17 +65,17 @@ -

{{messagePrefix + '.headSubgroups' | translate}}

- @@ -90,7 +90,7 @@ - + {{group.id}} {{group.name}} @@ -109,7 +109,7 @@ - diff --git a/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.spec.ts b/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.spec.ts index 2122a96cf3..7149d12330 100644 --- a/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.spec.ts +++ b/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.spec.ts @@ -120,7 +120,7 @@ describe('SubgroupsListComponent', () => { })); it('should show list of subgroups of current active group', () => { - const groupIdsFound = fixture.debugElement.queryAll(By.css('#subgroupsOfGroup tr td:first-child')); + const groupIdsFound = fixture.debugElement.queryAll(By.css('#subgroupsOfGroup$ tr td:first-child')); expect(groupIdsFound.length).toEqual(1); activeGroup.subgroups.map((group: Group) => { expect(groupIdsFound.find((foundEl) => { @@ -132,7 +132,7 @@ describe('SubgroupsListComponent', () => { describe('if first group delete button is pressed', () => { let groupsFound; beforeEach(fakeAsync(() => { - const addButton = fixture.debugElement.query(By.css('#subgroupsOfGroup tbody .deleteButton')); + const addButton = fixture.debugElement.query(By.css('#subgroupsOfGroup$ tbody .deleteButton')); addButton.triggerEventHandler('click', { preventDefault: () => {/**/ } @@ -141,7 +141,7 @@ describe('SubgroupsListComponent', () => { fixture.detectChanges(); })); it('one less subgroup in list from 1 to 0 (of 2 total groups)', () => { - groupsFound = fixture.debugElement.queryAll(By.css('#subgroupsOfGroup tbody tr')); + groupsFound = fixture.debugElement.queryAll(By.css('#subgroupsOfGroup$ tbody tr')); expect(groupsFound.length).toEqual(0); }); }); @@ -151,15 +151,15 @@ describe('SubgroupsListComponent', () => { let groupsFound; beforeEach(fakeAsync(() => { component.search({ query: '' }); - groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr')); + groupsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr')); })); it('should display all groups', () => { fixture.detectChanges(); - groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr')); + groupsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr')); expect(groupsFound.length).toEqual(2); - groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr')); - const groupIdsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr td:first-child')); + groupsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr')); + const groupIdsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr td:first-child')); allGroups.map((group: Group) => { expect(groupIdsFound.find((foundEl) => { return (foundEl.nativeElement.textContent.trim() === group.uuid); @@ -170,7 +170,7 @@ describe('SubgroupsListComponent', () => { describe('if group is already a subgroup', () => { it('should have delete button, else it should have add button', () => { fixture.detectChanges(); - groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr')); + groupsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr')); const getSubgroups = groupsDataServiceStub.getSubgroups().subgroups; if (getSubgroups !== undefined && getSubgroups.length > 0) { groupsFound.map((foundGroupRowElement) => { diff --git a/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.ts b/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.ts index 4c45827861..62b4c34bb3 100644 --- a/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.ts +++ b/src/app/+admin/admin-access-control/group-registry/group-form/subgroup-list/subgroups-list.component.ts @@ -2,7 +2,7 @@ import { Component, Input, OnDestroy, OnInit } from '@angular/core'; import { FormBuilder } from '@angular/forms'; import { Router } from '@angular/router'; import { TranslateService } from '@ngx-translate/core'; -import { Observable, of as observableOf, Subscription } from 'rxjs'; +import { Observable, of as observableOf, Subscription, BehaviorSubject } from 'rxjs'; import { map, mergeMap, take } from 'rxjs/operators'; import { PaginatedList } from '../../../../../core/data/paginated-list.model'; import { RemoteData } from '../../../../../core/data/remote-data'; @@ -13,9 +13,18 @@ import { getFirstSucceededRemoteData, getFirstCompletedRemoteData } from '../../../../../core/shared/operators'; -import { hasValue } from '../../../../../shared/empty.util'; import { NotificationsService } from '../../../../../shared/notifications/notifications.service'; import { PaginationComponentOptions } from '../../../../../shared/pagination/pagination-component-options.model'; +import { EPerson } from '../../../../../core/eperson/models/eperson.model'; + +/** + * Keys to keep track of specific subscriptions + */ +enum SubKey { + Members, + ActiveGroup, + SearchResults, +} @Component({ selector: 'ds-subgroups-list', @@ -32,16 +41,16 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { /** * Result of search groups, initially all groups */ - groupsSearch: Observable>>; + searchResults$: BehaviorSubject>> = new BehaviorSubject(undefined); /** * List of all subgroups of group being edited */ - subgroupsOfGroup: Observable>>; + subGroups$: BehaviorSubject>> = new BehaviorSubject(undefined); /** - * List of subscriptions + * Map of active subscriptions */ - subs: Subscription[] = []; + subs: Map = new Map(); /** * Pagination config used to display the list of groups that are result of groups search @@ -84,10 +93,10 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { this.searchForm = this.formBuilder.group(({ query: '', })); - this.subs.push(this.groupDataService.getActiveGroup().subscribe((activeGroup: Group) => { + this.subs.set(SubKey.ActiveGroup, this.groupDataService.getActiveGroup().subscribe((activeGroup: Group) => { if (activeGroup != null) { this.groupBeingEdited = activeGroup; - this.forceUpdateGroups(activeGroup); + this.retrieveSubGroups(this.config.currentPage); } })); } @@ -106,10 +115,26 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { * @param event */ onPageChange(event) { - this.subgroupsOfGroup = this.groupDataService.findAllByHref(this.groupBeingEdited._links.subgroups.href, { - currentPage: event, - elementsPerPage: this.config.pageSize - }); + this.retrieveSubGroups(event); + } + + /** + * Retrieve the Subgroups that are members of the group + * + * @param page the number of the page to retrieve + * @private + */ + private retrieveSubGroups(page: number) { + this.unsubFrom(SubKey.Members); + this.subs.set( + SubKey.Members, + this.groupDataService.findAllByHref(this.groupBeingEdited._links.subgroups.href, { + currentPage: page, + elementsPerPage: this.config.pageSize + } + ).subscribe((rd: RemoteData>) => { + this.subGroups$.next(rd); + })); } /** @@ -124,7 +149,7 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { return observableOf(false); } else { return this.groupDataService.findAllByHref(activeGroup._links.subgroups.href, { - currentPage: 0, + currentPage: 1, elementsPerPage: 9999 }) .pipe( @@ -162,7 +187,6 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { if (activeGroup != null) { const response = this.groupDataService.deleteSubGroupFromGroup(activeGroup, subgroup); this.showNotifications('deleteSubgroup', response, subgroup.name, activeGroup); - this.forceUpdateGroups(activeGroup); } else { this.notificationsService.error(this.translateService.get(this.messagePrefix + '.notification.failure.noActiveGroup')); } @@ -186,7 +210,6 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { this.notificationsService.error(this.translateService.get(this.messagePrefix + '.notification.failure.noActiveGroup')); } }); - this.forceUpdateGroups(this.groupBeingEdited); } /** @@ -201,29 +224,37 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { this.configSearch.currentPage = 1; } this.searchDone = true; - this.groupsSearch = this.groupDataService.searchGroups(this.currentSearchQuery, { + + this.unsubFrom(SubKey.SearchResults); + this.subs.set(SubKey.SearchResults, this.groupDataService.searchGroups(this.currentSearchQuery, { currentPage: this.configSearch.currentPage, elementsPerPage: this.configSearch.pageSize - }); + }).subscribe((rd: RemoteData>) => { + this.searchResults$.next(rd); + })); } /** - * Force-update the list of groups by first clearing the cache of results of this active groups' subgroups, then performing a new REST call - * @param activeGroup Group currently being edited + * Unsubscribe from a subscription if it's still subscribed, and remove it from the map of + * active subscriptions + * + * @param key The key of the subscription to unsubscribe from + * @private */ - public forceUpdateGroups(activeGroup: Group) { - this.router.navigateByUrl(this.groupDataService.getGroupEditPageRouterLink(activeGroup)); - this.subgroupsOfGroup = this.groupDataService.findAllByHref(activeGroup._links.subgroups.href, { - currentPage: this.config.currentPage, - elementsPerPage: this.config.pageSize - }, false); + private unsubFrom(key: SubKey) { + if (this.subs.has(key)) { + this.subs.get(key).unsubscribe(); + this.subs.delete(key); + } } /** * unsub all subscriptions */ ngOnDestroy(): void { - this.subs.filter((sub) => hasValue(sub)).forEach((sub) => sub.unsubscribe()); + for (const key of this.subs.keys()) { + this.unsubFrom(key); + } } /** @@ -237,6 +268,7 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { response.pipe(getFirstCompletedRemoteData()).subscribe((rd: RemoteData) => { if (rd.hasSucceeded) { this.notificationsService.success(this.translateService.get(this.messagePrefix + '.notification.success.' + messageSuffix, { name: nameObject })); + this.groupDataService.clearGroupLinkRequests(activeGroup._links.subgroups.href); } else { this.notificationsService.error(this.translateService.get(this.messagePrefix + '.notification.failure.' + messageSuffix, { name: nameObject })); } diff --git a/src/app/+admin/admin-access-control/group-registry/groups-registry.component.ts b/src/app/+admin/admin-access-control/group-registry/groups-registry.component.ts index 3202ee56c3..ffa90b3805 100644 --- a/src/app/+admin/admin-access-control/group-registry/groups-registry.component.ts +++ b/src/app/+admin/admin-access-control/group-registry/groups-registry.component.ts @@ -2,9 +2,14 @@ import { Component, OnDestroy, OnInit } from '@angular/core'; import { FormBuilder } from '@angular/forms'; import { Router } from '@angular/router'; import { TranslateService } from '@ngx-translate/core'; -import { BehaviorSubject, combineLatest as observableCombineLatest, Subscription, Observable, ObservedValueOf, of as observableOf } from 'rxjs'; -import { filter } from 'rxjs/operators'; -import { catchError, map, switchMap, take } from 'rxjs/operators'; +import { + BehaviorSubject, + combineLatest as observableCombineLatest, + Subscription, + Observable, + of as observableOf +} from 'rxjs'; +import { filter, catchError, map, switchMap, take } from 'rxjs/operators'; import { DSpaceObjectDataService } from '../../../core/data/dspace-object-data.service'; import { AuthorizationDataService } from '../../../core/data/feature-authorization/authorization-data.service'; import { FeatureID } from '../../../core/data/feature-authorization/feature-id'; @@ -20,7 +25,7 @@ import { RouteService } from '../../../core/services/route.service'; import { DSpaceObject } from '../../../core/shared/dspace-object.model'; import { getAllSucceededRemoteDataPayload, - getFirstCompletedRemoteData + getFirstCompletedRemoteData, getAllSucceededRemoteData } from '../../../core/shared/operators'; import { PageInfo } from '../../../core/shared/page-info.model'; import { hasValue } from '../../../shared/empty.util'; @@ -70,6 +75,11 @@ export class GroupsRegistryComponent implements OnInit, OnDestroy { // Current search in groups registry currentSearchQuery: string; + /** + * The subscription for the search method + */ + searchSub: Subscription; + /** * List of subscriptions */ @@ -93,6 +103,30 @@ export class GroupsRegistryComponent implements OnInit, OnDestroy { ngOnInit() { this.search({ query: this.currentSearchQuery }); + + this.subs.push(this.groups$.pipe( + getAllSucceededRemoteDataPayload(), + switchMap((groups: PaginatedList) => { + return observableCombineLatest(groups.page.map((group: Group) => { + return observableCombineLatest([ + this.authorizationService.isAuthorized(FeatureID.CanDelete, hasValue(group) ? group.self : undefined), + this.hasLinkedDSO(group) + ]).pipe( + map(([isAuthorized, hasLinkedDSO]: boolean[]) => { + const groupDtoModel: GroupDtoModel = new GroupDtoModel(); + groupDtoModel.ableToDelete = isAuthorized && !hasLinkedDSO; + groupDtoModel.group = group; + return groupDtoModel; + } + ) + ); + })).pipe(map((dtos: GroupDtoModel[]) => { + return buildPaginatedList(groups.pageInfo, dtos); + })); + })).subscribe((value: PaginatedList) => { + this.groupsDto$.next(value); + this.pageInfoState$.next(value.pageInfo); + })); } /** @@ -115,37 +149,20 @@ export class GroupsRegistryComponent implements OnInit, OnDestroy { this.currentSearchQuery = query; this.config.currentPage = 1; } - this.subs.push(this.groupService.searchGroups(this.currentSearchQuery.trim(), { + if (hasValue(this.searchSub)) { + this.searchSub.unsubscribe(); + this.subs = this.subs.filter((sub: Subscription) => sub !== this.searchSub); + } + this.searchSub = this.groupService.searchGroups(this.currentSearchQuery.trim(), { currentPage: this.config.currentPage, elementsPerPage: this.config.pageSize - }).pipe(getFirstCompletedRemoteData()) - .subscribe((groupsRD: RemoteData>) => { - this.groups$.next(groupsRD); - this.pageInfoState$.next(groupsRD.payload.pageInfo); - } - )); - - this.subs.push(this.groups$.pipe( - getAllSucceededRemoteDataPayload(), - switchMap((groups: PaginatedList) => { - return observableCombineLatest(...groups.page.map((group: Group) => { - return observableCombineLatest( - this.authorizationService.isAuthorized(FeatureID.CanDelete, hasValue(group) ? group.self : undefined), - this.hasLinkedDSO(group), - (isAuthorized: ObservedValueOf>, hasLinkedDSO: ObservedValueOf>) => { - const groupDtoModel: GroupDtoModel = new GroupDtoModel(); - groupDtoModel.ableToDelete = isAuthorized && !hasLinkedDSO; - groupDtoModel.group = group; - return groupDtoModel; - } - ); - })).pipe(map((dtos: GroupDtoModel[]) => { - return buildPaginatedList(groups.pageInfo, dtos); - })); - })).subscribe((value: PaginatedList) => { - this.groupsDto$.next(value); - this.pageInfoState$.next(value.pageInfo); - })); + }).pipe( + getAllSucceededRemoteData() + ).subscribe((groupsRD: RemoteData>) => { + this.groups$.next(groupsRD); + this.pageInfoState$.next(groupsRD.payload.pageInfo); + }); + this.subs.push(this.searchSub); } /** @@ -168,16 +185,13 @@ export class GroupsRegistryComponent implements OnInit, OnDestroy { } /** - * This method will ensure that the page gets reset and that the cache is cleared + * This method will set everything to stale, which will cause the lists on this page to update. */ reset() { this.groupService.getBrowseEndpoint().pipe( - switchMap((href) => this.requestService.removeByHrefSubstring(href)), - filter((isCached) => isCached), take(1) - ).subscribe(() => { - this.cleanupSubscribes(); - this.search({ query: this.currentSearchQuery }); + ).subscribe((href: string) => { + this.requestService.setStaleByHrefSubstring(href); }); } diff --git a/src/app/core/eperson/eperson-data.service.ts b/src/app/core/eperson/eperson-data.service.ts index 3a5c4e4b9c..9d1be80366 100644 --- a/src/app/core/eperson/eperson-data.service.ts +++ b/src/app/core/eperson/eperson-data.service.ts @@ -224,7 +224,7 @@ export class EPersonDataService extends DataService { * Method that clears a link's requests in cache */ public clearLinkRequests(href: string): void { - this.requestService.removeByHrefSubstring(href); + this.requestService.setStaleByHrefSubstring(href); } /** diff --git a/src/app/core/eperson/group-data.service.ts b/src/app/core/eperson/group-data.service.ts index 276ae114c6..3df7b63281 100644 --- a/src/app/core/eperson/group-data.service.ts +++ b/src/app/core/eperson/group-data.service.ts @@ -205,7 +205,7 @@ export class GroupDataService extends DataService { * Method that clears a cached get subgroups of certain group request */ public clearGroupLinkRequests(href: string): void { - this.requestService.removeByHrefSubstring(href); + this.requestService.setStaleByHrefSubstring(href); } public getGroupRegistryRouterLink(): string {