fix group tests and fix issues with the process detail page

2025-10-07 10:04:11 +00:00 · 2021-01-25 15:39:56 +01:00
parent 634836158b
commit 3b67efe09d
8 changed files with 97 additions and 56 deletions
--- 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) => {
--- 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<Group> {
        return observableOf(this.activeGroup);
      },
@@ -51,7 +63,11 @@ describe('SubgroupsListComponent', () => {
        return this.activeGroup;
      },
      findAllByHref(href: string): Observable<RemoteData<PaginatedList<Group>>> {
-        return createSuccessfulRemoteDataObject$(buildPaginatedList<Group>(new PageInfo(), this.subgroups));
+        return this.subgroups$.pipe(
+          map((currentGroups: Group[]) => {
+            return createSuccessfulRemoteDataObject(buildPaginatedList<Group>(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<RestResponse> {
-        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<RestResponse> {
-        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) => {
--- 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<RemoteData<Group>>, nameObject: string, activeGroup: Group) {
+  showNotifications(messageSuffix: string, response: Observable<RemoteData<Group|NoContent>>, nameObject: string, activeGroup: Group) {
    response.pipe(getFirstCompletedRemoteData()).subscribe((rd: RemoteData<Group>) => {
      if (rd.hasSucceeded) {
        this.notificationsService.success(this.translateService.get(this.messagePrefix + '.notification.success.' + messageSuffix, { name: nameObject }));
--- a/src/app/core/data/data.service.ts
+++ b/src/app/core/data/data.service.ts
@@ -339,7 +339,7 @@ export abstract class DataService<T extends CacheableObject> implements UpdateDa

    this.createAndSendGetRequest(requestHref$, useCachedVersionIfAvailable);

-    return this.rdbService.buildSingle<T>(href$, ...linksToFollow).pipe(
+    return this.rdbService.buildSingle<T>(requestHref$, ...linksToFollow).pipe(
      reRequestStaleRemoteData(reRequestOnStale, () =>
        this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow))
    );
--- 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);
      });
--- 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;
      }
    }
  }
--- 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<Group> {
   * @param activeGroup   Group we want to delete subgroup from
   * @param subgroup      Subgroup we want to delete from activeGroup
   */
-  deleteSubGroupFromGroup(activeGroup: Group, subgroup: Group): Observable<RemoteData<Group>> {
+  deleteSubGroupFromGroup(activeGroup: Group, subgroup: Group): Observable<RemoteData<NoContent>> {
    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<Group> {
   * @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<RemoteData<Group>> {
+  deleteMemberFromGroup(activeGroup: Group, ePerson: EPerson): Observable<RemoteData<NoContent>> {
    const requestId = this.requestService.generateRequestId();
    const deleteRequest = new DeleteRequest(requestId, activeGroup.self + '/' + this.ePersonsEndpoint + '/' + ePerson.id);
    this.requestService.send(deleteRequest);
--- 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<string>;
+  outputLogs$: BehaviorSubject<string> = 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<RemoteData<Bitstream>> = this.processRD$.pipe(
@@ -114,15 +119,15 @@ export class ProcessDetailComponent implements OnInit {
        })
      );
      this.outputLogFileUrl$ = processOutputRD$.pipe(
+        getFirstSucceededRemoteData(),
        tap((processOutputFileRD: RemoteData<Bitstream>) => {
          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<Bitstream>) => {
+          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<string> {