Skip to content

Commit

Permalink
Merge pull request #3079 from alexandrevryghem/w2p-115427_fixed-delet…
Browse files Browse the repository at this point in the history
…e-item-page-freezing_contribute-main

Fixed delete item page freezing when having relationships
  • Loading branch information
tdonohue authored May 29, 2024
2 parents 3814734 + 672219b commit a5b9477
Show file tree
Hide file tree
Showing 10 changed files with 107 additions and 75 deletions.
5 changes: 5 additions & 0 deletions src/app/core/data/base/delete-data.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,11 @@ describe('DeleteDataImpl', () => {
method: RestRequestMethod.DELETE,
href: 'some-href?copyVirtualMetadata=a&copyVirtualMetadata=b&copyVirtualMetadata=c',
}));

const callback = (rdbService.buildFromRequestUUIDAndAwait as jasmine.Spy).calls.argsFor(0)[1];
callback();
expect(service.invalidateByHref).toHaveBeenCalledWith('some-href');

done();
});
});
Expand Down
5 changes: 3 additions & 2 deletions src/app/core/data/base/delete-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,16 @@ export class DeleteDataImpl<T extends CacheableObject> extends IdentifiableDataS
deleteByHref(href: string, copyVirtualMetadata?: string[]): Observable<RemoteData<NoContent>> {
const requestId = this.requestService.generateRequestId();

let deleteHref: string = href;
if (copyVirtualMetadata) {
copyVirtualMetadata.forEach((id) =>
href += (href.includes('?') ? '&' : '?')
deleteHref += (deleteHref.includes('?') ? '&' : '?')
+ 'copyVirtualMetadata='
+ id,
);
}

const request = new DeleteRequest(requestId, href);
const request = new DeleteRequest(requestId, deleteHref);
if (hasValue(this.responseMsToLive)) {
request.responseMsToLive = this.responseMsToLive;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,29 @@ <h1>{{headerMessage | translate: {id: item.handle} }}</h1>
<p>{{descriptionMessage | translate}}</p>
<ds-modify-item-overview [item]="item"></ds-modify-item-overview>

<ng-container *ngVar="(types$ | async) as types">
<ng-container *ngVar="(typeDTOs$ | async) as types">

<div *ngIf="types && types.length > 0" class="mb-4">

{{'virtual-metadata.delete-item.info' | translate}}

<div *ngFor="let type of types" class="mb-4">

<div *ngVar="(isSelected(type) | async) as selected"
<div *ngFor="let typeDto of types" class="mb-4">
<div *ngVar="(typeDto.isSelected$ | async) as selected"
class="d-flex flex-row">

<div class="m-2" (click)="setSelected(type, !selected)">
<div class="m-2" (click)="setSelected(typeDto.relationshipType, !selected)">
<label>
<input type="checkbox" [checked]="selected">
<input type="checkbox" [checked]="selected" [disabled]="isDeleting$ | async">
</label>
</div>

<div class="flex-column flex-grow-1">
<h5 (click)="setSelected(type, !selected)">
{{getRelationshipMessageKey(getLabel(type) | async) | translate}}
<h5 (click)="setSelected(typeDto.relationshipType, !selected)">
{{getRelationshipMessageKey(typeDto.label$ | async) | translate}}
</h5>
<div *ngFor="let relationship of (getRelationships(type) | async)"
<div *ngFor="let relationshipDto of (typeDto.relationshipDTOs$ | async)"
class="d-flex flex-row">
<ng-container *ngVar="(getRelatedItem(relationship) | async) as relatedItem">
<ng-container *ngVar="(relationshipDto.relatedItem$ | async) as relatedItem">

<ds-listable-object-component-loader
*ngIf="relatedItem"
Expand All @@ -46,7 +45,7 @@ <h5 (click)="setSelected(type, !selected)">
</div>

<ng-template #virtualMetadataModal>
<div>
<div class="thumb-font-1">
<div class="modal-header">
{{'virtual-metadata.delete-item.modal-head' | translate}}
<button type="button" class="close"
Expand All @@ -60,7 +59,7 @@ <h5 (click)="setSelected(type, !selected)">
[object]="relatedItem"
[viewMode]="viewMode">
</ds-listable-object-component-loader>
<div *ngFor="let metadata of (getVirtualMetadata(relationship) | async)">
<div *ngFor="let metadata of (relationshipDto.virtualMetadata$ | async)">
<div>
<div class="font-weight-bold">
{{metadata.metadataField}}
Expand All @@ -87,10 +86,11 @@ <h5 (click)="setSelected(type, !selected)">
</ng-container>

<div class="space-children-mr">
<button (click)="performAction()"
<button [disabled]="isDeleting$ | async" (click)="performAction()"
class="btn btn-outline-secondary perform-action">{{confirmMessage | translate}}
</button>
<button [routerLink]="[itemPageRoute, 'edit']" class="btn btn-outline-secondary cancel">
<button [disabled]="isDeleting$ | async" [routerLink]="[itemPageRoute, 'edit']"
class="btn btn-outline-secondary cancel">
{{cancelMessage| translate}}
</button>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// eslint-disable-next-line max-classes-per-file
import {
AsyncPipe,
NgForOf,
Expand Down Expand Up @@ -68,6 +69,34 @@ import { ModifyItemOverviewComponent } from '../modify-item-overview/modify-item
import { AbstractSimpleItemActionComponent } from '../simple-item-action/abstract-simple-item-action.component';
import { VirtualMetadata } from '../virtual-metadata/virtual-metadata.component';

/**
* Data Transfer Object used to prevent the HTML template to call function returning Observables
*/
class RelationshipTypeDTO {

relationshipType: RelationshipType;

isSelected$: Observable<boolean>;

label$: Observable<string>;

relationshipDTOs$: Observable<RelationshipDTO[]>;

}

/**
* Data Transfer Object used to prevent the HTML template to call function returning Observables
*/
class RelationshipDTO {

relationship: Relationship;

relatedItem$: Observable<Item>;

virtualMetadata$: Observable<VirtualMetadata[]>;

}

@Component({
selector: 'ds-item-delete',
templateUrl: '../item-delete/item-delete.component.html',
Expand Down Expand Up @@ -106,7 +135,7 @@ export class ItemDeleteComponent
* A list of the relationship types for which this item has relations as an observable.
* The list doesn't contain duplicates.
*/
types$: BehaviorSubject<RelationshipType[]> = new BehaviorSubject([]);
typeDTOs$: BehaviorSubject<RelationshipTypeDTO[]> = new BehaviorSubject([]);

/**
* A map which stores the relationships of this item for each type as observable lists
Expand Down Expand Up @@ -135,6 +164,8 @@ export class ItemDeleteComponent
*/
private subs: Subscription[] = [];

public isDeleting$: BehaviorSubject<boolean> = new BehaviorSubject(false);

constructor(protected route: ActivatedRoute,
protected router: Router,
protected notificationsService: NotificationsService,
Expand Down Expand Up @@ -189,13 +220,24 @@ export class ItemDeleteComponent
),
);
}),
).subscribe((types: RelationshipType[]) => this.types$.next(types)));
).subscribe((types: RelationshipType[]) => this.typeDTOs$.next(types.map((relationshipType: RelationshipType) => Object.assign(new RelationshipTypeDTO(), {
relationshipType: relationshipType,
isSelected$: this.isSelected(relationshipType),
label$: this.getLabel(relationshipType),
relationshipDTOs$: this.getRelationships(relationshipType).pipe(
map((relationships: Relationship[]) => relationships.map((relationship: Relationship) => Object.assign(new RelationshipDTO(), {
relationship: relationship,
relatedItem$: this.getRelatedItem(relationship),
virtualMetadata$: this.getVirtualMetadata(relationship),
} as RelationshipDTO))),
),
})))));
}

this.subs.push(this.types$.pipe(
this.subs.push(this.typeDTOs$.pipe(
take(1),
).subscribe((types) =>
this.objectUpdatesService.initialize(this.url, types, this.item.lastModified),
).subscribe((types: RelationshipTypeDTO[]) =>
this.objectUpdatesService.initialize(this.url, types.map((relationshipTypeDto: RelationshipTypeDTO) => relationshipTypeDto.relationshipType), this.item.lastModified),
));
}

Expand Down Expand Up @@ -368,34 +410,33 @@ export class ItemDeleteComponent
* @param selected whether the type should be selected
*/
setSelected(type: RelationshipType, selected: boolean): void {
this.objectUpdatesService.setSelectedVirtualMetadata(this.url, this.item.uuid, type.uuid, selected);
if (this.isDeleting$.value === false) {
this.objectUpdatesService.setSelectedVirtualMetadata(this.url, this.item.uuid, type.uuid, selected);
}
}

/**
* Perform the delete operation
*/
performAction() {

this.subs.push(this.types$.pipe(
switchMap((types) =>
performAction(): void {
this.isDeleting$.next(true);
this.subs.push(this.typeDTOs$.pipe(
switchMap((types: RelationshipTypeDTO[]) =>
combineLatest(
types.map((type) => this.isSelected(type)),
types.map((type: RelationshipTypeDTO) => type.isSelected$),
).pipe(
defaultIfEmpty([]),
map((selection) => types.filter(
(type, index) => selection[index],
map((selection: boolean[]) => types.filter(
(type: RelationshipTypeDTO, index: number) => selection[index],
)),
map((selectedTypes) => selectedTypes.map((type) => type.id)),
map((selectedDtoTypes: RelationshipTypeDTO[]) => selectedDtoTypes.map((typeDto: RelationshipTypeDTO) => typeDto.relationshipType.id)),
),
),
switchMap((types) =>
this.itemDataService.delete(this.item.id, types).pipe(getFirstCompletedRemoteData()),
),
).subscribe(
(rd: RemoteData<NoContent>) => {
this.notify(rd.hasSucceeded);
},
));
switchMap((types: string[]) => this.itemDataService.delete(this.item.id, types)),
getFirstCompletedRemoteData(),
).subscribe((rd: RemoteData<NoContent>) => {
this.notify(rd.hasSucceeded);
}));
}

/**
Expand All @@ -405,10 +446,10 @@ export class ItemDeleteComponent
notify(succeeded: boolean) {
if (succeeded) {
this.notificationsService.success(this.translateService.get('item.edit.' + this.messageKey + '.success'));
this.router.navigate(['']);
void this.router.navigate(['']);
} else {
this.notificationsService.error(this.translateService.get('item.edit.' + this.messageKey + '.error'));
this.router.navigate([getItemEditRoute(this.item)]);
void this.router.navigate([getItemEditRoute(this.item)]);
}
}

Expand Down
8 changes: 0 additions & 8 deletions src/app/item-page/full/full-item-page.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {
of as observableOf,
} from 'rxjs';

import { AuthService } from '../../core/auth/auth.service';
import { NotifyInfoService } from '../../core/coar-notify/notify-info/notify-info.service';
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
import { ItemDataService } from '../../core/data/item-data.service';
Expand Down Expand Up @@ -79,7 +78,6 @@ describe('FullItemPageComponent', () => {
let comp: FullItemPageComponent;
let fixture: ComponentFixture<FullItemPageComponent>;

let authService: AuthService;
let routeStub: ActivatedRouteStub;
let routeData;
let authorizationDataService: AuthorizationDataService;
Expand All @@ -102,11 +100,6 @@ describe('FullItemPageComponent', () => {
};

beforeEach(waitForAsync(() => {
authService = jasmine.createSpyObj('authService', {
isAuthenticated: observableOf(true),
setRedirectUrl: {},
});

routeData = {
dso: createSuccessfulRemoteDataObject(mockItem),
};
Expand Down Expand Up @@ -151,7 +144,6 @@ describe('FullItemPageComponent', () => {
{ provide: ActivatedRoute, useValue: routeStub },
{ provide: ItemDataService, useValue: {} },
{ provide: HeadTagService, useValue: headTagService },
{ provide: AuthService, useValue: authService },
{ provide: AuthorizationDataService, useValue: authorizationDataService },
{ provide: ServerResponseService, useValue: serverResponseService },
{ provide: SignpostingDataService, useValue: signpostingDataService },
Expand Down
4 changes: 1 addition & 3 deletions src/app/item-page/full/full-item-page.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
map,
} from 'rxjs/operators';

import { AuthService } from '../../core/auth/auth.service';
import { NotifyInfoService } from '../../core/coar-notify/notify-info/notify-info.service';
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
import { ItemDataService } from '../../core/data/item-data.service';
Expand Down Expand Up @@ -103,7 +102,6 @@ export class FullItemPageComponent extends ItemPageComponent implements OnInit,
protected route: ActivatedRoute,
protected router: Router,
protected items: ItemDataService,
protected authService: AuthService,
protected authorizationService: AuthorizationDataService,
protected _location: Location,
protected responseService: ServerResponseService,
Expand All @@ -112,7 +110,7 @@ export class FullItemPageComponent extends ItemPageComponent implements OnInit,
protected notifyInfoService: NotifyInfoService,
@Inject(PLATFORM_ID) protected platformId: string,
) {
super(route, router, items, authService, authorizationService, responseService, signpostingDataService, linkHeadService, notifyInfoService, platformId);
super(route, router, items, authorizationService, responseService, signpostingDataService, linkHeadService, notifyInfoService, platformId);
}

/*** AoT inheritance fix, will hopefully be resolved in the near future **/
Expand Down
15 changes: 11 additions & 4 deletions src/app/item-page/item-page.resolver.spec.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
import { TestBed } from '@angular/core/testing';
import { Router } from '@angular/router';
import { RouterTestingModule } from '@angular/router/testing';
import {
Router,
RouterModule,
} from '@angular/router';
import { first } from 'rxjs/operators';

import { DSpaceObject } from '../core/shared/dspace-object.model';
import { MetadataValueFilter } from '../core/shared/metadata.models';
import { createSuccessfulRemoteDataObject$ } from '../shared/remote-data.utils';
import { AuthServiceStub } from '../shared/testing/auth-service.stub';
import { itemPageResolver } from './item-page.resolver';

describe('itemPageResolver', () => {
beforeEach(() => {
TestBed.configureTestingModule({
imports: [RouterTestingModule.withRoutes([{
imports: [RouterModule.forRoot([{
path: 'entities/:entity-type/:id',
component: {} as any,
}])],
Expand All @@ -22,7 +25,8 @@ describe('itemPageResolver', () => {
let resolver: any;
let itemService: any;
let store: any;
let router: any;
let router: Router;
let authService: AuthServiceStub;

const uuid = '1234-65487-12354-1235';
let item: DSpaceObject;
Expand All @@ -42,6 +46,7 @@ describe('itemPageResolver', () => {
store = jasmine.createSpyObj('store', {
dispatch: {},
});
authService = new AuthServiceStub();
resolver = itemPageResolver;
});

Expand All @@ -54,6 +59,7 @@ describe('itemPageResolver', () => {
router,
itemService,
store,
authService,
).pipe(first())
.subscribe(
() => {
Expand All @@ -73,6 +79,7 @@ describe('itemPageResolver', () => {
router,
itemService,
store,
authService,
).pipe(first())
.subscribe(
() => {
Expand Down
Loading

0 comments on commit a5b9477

Please sign in to comment.