From 3b67efe09d91fa9cbb053dba5aee0d9ee85816a3 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Mon, 25 Jan 2021 15:39:56 +0100 Subject: [PATCH] fix group tests and fix issues with the process detail page --- .../members-list.component.spec.ts | 2 +- .../subgroups-list.component.spec.ts | 48 ++++++++++++------- .../subgroup-list/subgroups-list.component.ts | 4 +- src/app/core/data/data.service.ts | 2 +- src/app/core/data/request.service.spec.ts | 32 ++++++------- src/app/core/data/request.service.ts | 35 ++++++++++---- src/app/core/eperson/group-data.service.ts | 4 +- .../detail/process-detail.component.ts | 26 ++++++---- 8 files changed, 97 insertions(+), 56 deletions(-) diff --git a/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.spec.ts b/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.spec.ts index 4735748909..10735cbde5 100644 --- a/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.spec.ts +++ b/src/app/+admin/admin-access-control/group-registry/group-form/members-list/members-list.component.spec.ts @@ -150,7 +150,7 @@ describe('MembersListComponent', () => { })); it('should show list of eperson members of current active group', () => { - const epersonIdsFound = fixture.debugElement.queryAll(By.css('#members$ tr td:first-child')); + const epersonIdsFound = fixture.debugElement.queryAll(By.css('#ePeopleMembersOfGroup tr td:first-child')); expect(epersonIdsFound.length).toEqual(1); epersonMembers.map((eperson: EPerson) => { expect(epersonIdsFound.find((foundEl) => { 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 7149d12330..9841d2b02e 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 @@ -1,12 +1,20 @@ import { CommonModule } from '@angular/common'; import { NO_ERRORS_SCHEMA } from '@angular/core'; -import { ComponentFixture, fakeAsync, flush, inject, TestBed, tick, waitForAsync } from '@angular/core/testing'; +import { + ComponentFixture, + fakeAsync, + flush, + inject, + TestBed, + tick, + waitForAsync +} from '@angular/core/testing'; import { FormsModule, ReactiveFormsModule } from '@angular/forms'; import { BrowserModule, By } from '@angular/platform-browser'; import { Router } from '@angular/router'; import { NgbModule } from '@ng-bootstrap/ng-bootstrap'; import { TranslateLoader, TranslateModule, TranslateService } from '@ngx-translate/core'; -import { Observable, of as observableOf } from 'rxjs'; +import { Observable, of as observableOf, BehaviorSubject } from 'rxjs'; import { RestResponse } from '../../../../../core/cache/response.models'; import { buildPaginatedList, PaginatedList } from '../../../../../core/data/paginated-list.model'; import { RemoteData } from '../../../../../core/data/remote-data'; @@ -17,12 +25,16 @@ import { FormBuilderService } from '../../../../../shared/form/builder/form-buil import { NotificationsService } from '../../../../../shared/notifications/notifications.service'; import { GroupMock, GroupMock2 } from '../../../../../shared/testing/group-mock'; import { SubgroupsListComponent } from './subgroups-list.component'; -import { createSuccessfulRemoteDataObject$ } from '../../../../../shared/remote-data.utils'; +import { + createSuccessfulRemoteDataObject$, + createSuccessfulRemoteDataObject +} from '../../../../../shared/remote-data.utils'; import { RouterMock } from '../../../../../shared/mocks/router.mock'; import { getMockFormBuilderService } from '../../../../../shared/mocks/form-builder-service.mock'; import { getMockTranslateService } from '../../../../../shared/mocks/translate.service.mock'; import { TranslateLoaderMock } from '../../../../../shared/testing/translate-loader.mock'; import { NotificationsServiceStub } from '../../../../../shared/testing/notifications-service.stub'; +import { map } from 'rxjs/operators'; describe('SubgroupsListComponent', () => { let component: SubgroupsListComponent; @@ -43,7 +55,7 @@ describe('SubgroupsListComponent', () => { ePersonDataServiceStub = {}; groupsDataServiceStub = { activeGroup: activeGroup, - subgroups: subgroups, + subgroups$: new BehaviorSubject(subgroups), getActiveGroup(): Observable { return observableOf(this.activeGroup); }, @@ -51,7 +63,11 @@ describe('SubgroupsListComponent', () => { return this.activeGroup; }, findAllByHref(href: string): Observable>> { - return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), this.subgroups)); + return this.subgroups$.pipe( + map((currentGroups: Group[]) => { + return createSuccessfulRemoteDataObject(buildPaginatedList(new PageInfo(), currentGroups)); + }) + ); }, getGroupEditPageRouterLink(group: Group): string { return '/admin/access-control/groups/' + group.id; @@ -63,7 +79,7 @@ describe('SubgroupsListComponent', () => { return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), [])); }, addSubGroupToGroup(parentGroup, subgroup: Group): Observable { - this.subgroups = [...this.subgroups, subgroup]; + this.subgroups$.next([...this.subgroups$.getValue(), subgroup]); return observableOf(new RestResponse(true, 200, 'Success')); }, clearGroupsRequests() { @@ -73,11 +89,11 @@ describe('SubgroupsListComponent', () => { // empty }, deleteSubGroupFromGroup(parentGroup, subgroup: Group): Observable { - this.subgroups = this.subgroups.find((group: Group) => { + this.subgroups$.next(this.subgroups$.getValue().filter((group: Group) => { if (group.id !== subgroup.id) { return group; } - }); + })); return observableOf(new RestResponse(true, 200, 'Success')); } }; @@ -120,7 +136,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 +148,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 +157,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 +167,15 @@ describe('SubgroupsListComponent', () => { let groupsFound; beforeEach(fakeAsync(() => { component.search({ query: '' }); - groupsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr')); + groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr')); })); it('should display all groups', () => { fixture.detectChanges(); - groupsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr')); + groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr')); expect(groupsFound.length).toEqual(2); - groupsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr')); - const groupIdsFound = fixture.debugElement.queryAll(By.css('#searchResults$ tbody tr td:first-child')); + groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr')); + const groupIdsFound = fixture.debugElement.queryAll(By.css('#groupsSearch tbody tr td:first-child')); allGroups.map((group: Group) => { expect(groupIdsFound.find((foundEl) => { return (foundEl.nativeElement.textContent.trim() === group.uuid); @@ -170,7 +186,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('#searchResults$ tbody tr')); + groupsFound = fixture.debugElement.queryAll(By.css('#groupsSearch 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 62b4c34bb3..fa1174792a 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 @@ -15,7 +15,7 @@ import { } from '../../../../../core/shared/operators'; import { NotificationsService } from '../../../../../shared/notifications/notifications.service'; import { PaginationComponentOptions } from '../../../../../shared/pagination/pagination-component-options.model'; -import { EPerson } from '../../../../../core/eperson/models/eperson.model'; +import { NoContent } from '../../../../../core/shared/NoContent.model'; /** * Keys to keep track of specific subscriptions @@ -264,7 +264,7 @@ export class SubgroupsListComponent implements OnInit, OnDestroy { * @param nameObject Object request was about * @param activeGroup Group currently being edited */ - showNotifications(messageSuffix: string, response: Observable>, nameObject: string, activeGroup: Group) { + showNotifications(messageSuffix: string, response: Observable>, nameObject: string, activeGroup: Group) { response.pipe(getFirstCompletedRemoteData()).subscribe((rd: RemoteData) => { if (rd.hasSucceeded) { this.notificationsService.success(this.translateService.get(this.messagePrefix + '.notification.success.' + messageSuffix, { name: nameObject })); diff --git a/src/app/core/data/data.service.ts b/src/app/core/data/data.service.ts index 28625ee27b..e98de0f77d 100644 --- a/src/app/core/data/data.service.ts +++ b/src/app/core/data/data.service.ts @@ -339,7 +339,7 @@ export abstract class DataService implements UpdateDa this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable); - return this.rdbService.buildSingle(href$, ...linksToFollow).pipe( + return this.rdbService.buildSingle(requestHref$, ...linksToFollow).pipe( reRequestStaleRemoteData(reRequestOnStale, () => this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)) ); diff --git a/src/app/core/data/request.service.spec.ts b/src/app/core/data/request.service.spec.ts index 78324ec070..cf137c80cf 100644 --- a/src/app/core/data/request.service.spec.ts +++ b/src/app/core/data/request.service.spec.ts @@ -284,13 +284,13 @@ describe('RequestService', () => { it('should track it on it\'s way to the store', () => { spyOn(serviceAsAny, 'trackRequestsOnTheirWayToTheStore'); - spyOn(serviceAsAny, 'isCachedOrPending').and.returnValue(false); + spyOn(serviceAsAny, 'shouldDispatchRequest').and.returnValue(true); service.send(request); expect(serviceAsAny.trackRequestsOnTheirWayToTheStore).toHaveBeenCalledWith(request); }); describe('and it isn\'t cached or pending', () => { beforeEach(() => { - spyOn(serviceAsAny, 'isCachedOrPending').and.returnValue(false); + spyOn(serviceAsAny, 'shouldDispatchRequest').and.returnValue(true); }); it('should dispatch the request', () => { @@ -301,7 +301,7 @@ describe('RequestService', () => { }); describe('and it is already cached or pending', () => { beforeEach(() => { - spyOn(serviceAsAny, 'isCachedOrPending').and.returnValue(true); + spyOn(serviceAsAny, 'shouldDispatchRequest').and.returnValue(false); }); it('shouldn\'t dispatch the request', () => { @@ -335,7 +335,7 @@ describe('RequestService', () => { }); - describe('isCachedOrPending', () => { + describe('shouldDispatchRequest', () => { describe('when the request is cached', () => { describe('in the ObjectCache', () => { beforeEach(() => { @@ -344,9 +344,9 @@ describe('RequestService', () => { spyOn(serviceAsAny, 'hasByUUID').and.returnValue(true); }); - it('should return true for GetRequest', () => { - const result = serviceAsAny.isCachedOrPending(testGetRequest); - const expected = true; + it('should return false for GetRequest', () => { + const result = serviceAsAny.shouldDispatchRequest(testGetRequest, true); + const expected = false; expect(result).toEqual(expected); }); @@ -357,9 +357,9 @@ describe('RequestService', () => { spyOn(serviceAsAny, 'hasByHref').and.returnValues(true); spyOn(serviceAsAny, 'hasByUUID').and.returnValue(false); }); - it('should return true', () => { - const result = serviceAsAny.isCachedOrPending(testGetRequest); - const expected = true; + it('should return false', () => { + const result = serviceAsAny.shouldDispatchRequest(testGetRequest, true); + const expected = false; expect(result).toEqual(expected); }); @@ -371,9 +371,9 @@ describe('RequestService', () => { spyOn(service, 'isPending').and.returnValue(true); }); - it('should return true', () => { - const result = serviceAsAny.isCachedOrPending(testGetRequest); - const expected = true; + it('should return false', () => { + const result = serviceAsAny.shouldDispatchRequest(testGetRequest, true); + const expected = false; expect(result).toEqual(expected); }); @@ -386,9 +386,9 @@ describe('RequestService', () => { spyOn(serviceAsAny, 'hasByUUID').and.returnValue(false); }); - it('should return false', () => { - const result = serviceAsAny.isCachedOrPending(testGetRequest); - const expected = false; + it('should return true', () => { + const result = serviceAsAny.shouldDispatchRequest(testGetRequest, true); + const expected = true; expect(result).toEqual(expected); }); diff --git a/src/app/core/data/request.service.ts b/src/app/core/data/request.service.ts index 1a6ad1d396..4a85df0d34 100644 --- a/src/app/core/data/request.service.ts +++ b/src/app/core/data/request.service.ts @@ -142,10 +142,16 @@ export class RequestService { } /** - * Check if a request is currently pending + * Check if a GET request is currently pending */ isPending(request: GetRequest): boolean { - // first check requests that haven't made it to the store yet + // If the request is not a GET request, it will never be considered pending, because you may + // want to execute the exact same e.g. POST multiple times + if (request.method !== RestRequestMethod.GET) { + return false; + } + + // check requests that haven't made it to the store yet if (this.requestsOnTheirWayToTheStore.includes(request.href)) { return true; } @@ -232,7 +238,7 @@ export class RequestService { console.warn(`${JSON.stringify(request, null, 2)} is not a GET request. In general only GET requests should reuse cached data.`); } - if (!useCachedVersionIfAvailable || !this.isCachedOrPending(request)) { + if (this.shouldDispatchRequest(request, useCachedVersionIfAvailable)) { this.dispatchRequest(request); if (request.method === RestRequestMethod.GET) { this.trackRequestsOnTheirWayToTheStore(request); @@ -301,25 +307,38 @@ export class RequestService { } /** - * Check if a request is in the cache or if it's still pending + * Check if a GET request is in the cache or if it's still pending * @param {GetRequest} request The request to check + * @param {boolean} useCachedVersionIfAvailable Whether or not to allow the use of a cached version * @returns {boolean} True if the request is cached or still pending */ - public isCachedOrPending(request: GetRequest): boolean { - if (this.isPending(request)) { + public shouldDispatchRequest(request: GetRequest, useCachedVersionIfAvailable: boolean): boolean { + // if it's not a GET request + if (request.method !== RestRequestMethod.GET) { + return true; + // if it is a GET request, check it isn't pending + } else if (this.isPending(request)) { + return false; + // if it is pending, check if we're allowed to use a cached version + } else if (!useCachedVersionIfAvailable) { return true; } else { + // if we are, check the request cache const urlWithoutEmbedParams = getUrlWithoutEmbedParams(request.href); if (this.hasByHref(urlWithoutEmbedParams) === true) { - return true; + return false; } else { + // if it isn't in the request cache, check the object cache let inObjCache = false; this.objectCache.getByHref(urlWithoutEmbedParams) .subscribe((entry: ObjectCacheEntry) => { + // if the object cache has a match, check if the request that the object came with is + // still valid inObjCache = this.hasByUUID(entry.requestUUID); }).unsubscribe(); - return inObjCache; + // we should send the request if it isn't cached + return !inObjCache; } } } diff --git a/src/app/core/eperson/group-data.service.ts b/src/app/core/eperson/group-data.service.ts index 3df7b63281..dc5fd97d9a 100644 --- a/src/app/core/eperson/group-data.service.ts +++ b/src/app/core/eperson/group-data.service.ts @@ -132,7 +132,7 @@ export class GroupDataService extends DataService { * @param activeGroup Group we want to delete subgroup from * @param subgroup Subgroup we want to delete from activeGroup */ - deleteSubGroupFromGroup(activeGroup: Group, subgroup: Group): Observable> { + deleteSubGroupFromGroup(activeGroup: Group, subgroup: Group): Observable> { const requestId = this.requestService.generateRequestId(); const deleteRequest = new DeleteRequest(requestId, activeGroup.self + '/' + this.subgroupsEndpoint + '/' + subgroup.id); this.requestService.send(deleteRequest); @@ -162,7 +162,7 @@ export class GroupDataService extends DataService { * @param activeGroup Group we want to delete member from * @param ePerson EPerson we want to delete from members of given activeGroup */ - deleteMemberFromGroup(activeGroup: Group, ePerson: EPerson): Observable> { + deleteMemberFromGroup(activeGroup: Group, ePerson: EPerson): Observable> { const requestId = this.requestService.generateRequestId(); const deleteRequest = new DeleteRequest(requestId, activeGroup.self + '/' + this.ePersonsEndpoint + '/' + ePerson.id); this.requestService.send(deleteRequest); diff --git a/src/app/process-page/detail/process-detail.component.ts b/src/app/process-page/detail/process-detail.component.ts index 8dfb2327fe..b90f040a09 100644 --- a/src/app/process-page/detail/process-detail.component.ts +++ b/src/app/process-page/detail/process-detail.component.ts @@ -11,7 +11,11 @@ import { ProcessDataService } from '../../core/data/processes/process-data.servi import { RemoteData } from '../../core/data/remote-data'; import { Bitstream } from '../../core/shared/bitstream.model'; import { DSpaceObject } from '../../core/shared/dspace-object.model'; -import { getFirstSucceededRemoteDataPayload, redirectOn4xx } from '../../core/shared/operators'; +import { + getFirstSucceededRemoteDataPayload, + redirectOn4xx, + getFirstSucceededRemoteData +} from '../../core/shared/operators'; import { URLCombiner } from '../../core/url-combiner/url-combiner'; import { AlertType } from '../../shared/alert/aletr-type'; import { hasValue } from '../../shared/empty.util'; @@ -51,7 +55,7 @@ export class ProcessDetailComponent implements OnInit { /** * The Process's Output logs */ - outputLogs$: Observable; + outputLogs$: BehaviorSubject = new BehaviorSubject(undefined); /** * Boolean on whether or not to show the output logs @@ -105,6 +109,7 @@ export class ProcessDetailComponent implements OnInit { * Sets the outputLogs when retrieved and sets the showOutputLogs boolean to show them and hide the button. */ showProcessOutputLogs() { + console.log('showProcessOutputLogs'); this.retrievingOutputLogs$.next(true); this.zone.runOutsideAngular(() => { const processOutputRD$: Observable> = this.processRD$.pipe( @@ -114,15 +119,15 @@ export class ProcessDetailComponent implements OnInit { }) ); this.outputLogFileUrl$ = processOutputRD$.pipe( + getFirstSucceededRemoteData(), tap((processOutputFileRD: RemoteData) => { if (processOutputFileRD.statusCode === 204) { this.zone.run(() => this.retrievingOutputLogs$.next(false)); this.showOutputLogs = true; } }), - getFirstSucceededRemoteDataPayload(), - mergeMap((processOutput: Bitstream) => { - const url = processOutput._links.content.href; + switchMap((processOutput: RemoteData) => { + const url = processOutput.payload._links.content.href; return this.authService.getShortlivedToken().pipe(take(1), map((token: string) => { return hasValue(token) ? new URLCombiner(url, `?authentication-token=${token}`).toString() : url; @@ -130,13 +135,14 @@ export class ProcessDetailComponent implements OnInit { }) ); }); - this.outputLogs$ = this.outputLogFileUrl$.pipe(take(1), - mergeMap((url: string) => { + this.outputLogFileUrl$.pipe(take(1), + switchMap((url: string) => { return this.getTextFile(url); }), - finalize(() => this.zone.run(() => this.retrievingOutputLogs$.next(false))), - ); - this.outputLogs$.pipe(take(1)).subscribe(); + finalize(() => this.zone.run(() => this.retrievingOutputLogs$.next(false))) + ).subscribe((logs: string) => { + this.outputLogs$.next(logs); + }); } getTextFile(filename: string): Observable {