Skip to content

Commit

Permalink
112198: Ensure relationship requests are sent one by one
Browse files Browse the repository at this point in the history
- remove (de)select all buttons in DynamicLookupRelationSearchTab
- add requestQueue to RelationshipEffects
  • Loading branch information
nona-luypaert committed Feb 19, 2024
1 parent 404ccd9 commit 325728d
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,8 @@ describe('RelationshipEffects', () => {
let action;
describe('When the last value in the debounceMap is also an ADD_RELATIONSHIP action', () => {
beforeEach(() => {
jasmine.getEnv().allowRespy(true);
spyOn((relationEffects as any), 'addRelationship').and.returnValue(createSuccessfulRemoteDataObject$(relationship));
(relationEffects as any).initialActionMap[identifier] = RelationshipActionTypes.ADD_RELATIONSHIP;
((relationEffects as any).debounceTime as jasmine.Spy).and.returnValue((v) => v);
});
Expand Down Expand Up @@ -247,6 +249,8 @@ describe('RelationshipEffects', () => {
let action;
describe('When the last value in the debounceMap is also an REMOVE_RELATIONSHIP action', () => {
beforeEach(() => {
jasmine.getEnv().allowRespy(true);
spyOn((relationEffects as any), 'removeRelationship').and.returnValue(createSuccessfulRemoteDataObject$(undefined));
((relationEffects as any).debounceTime as jasmine.Spy).and.returnValue((v) => v);
(relationEffects as any).initialActionMap[identifier] = RelationshipActionTypes.REMOVE_RELATIONSHIP;
});
Expand All @@ -256,7 +260,7 @@ describe('RelationshipEffects', () => {
actions = hot('--a-|', { a: action });
const expected = cold('--b-|', { b: undefined });
expect(relationEffects.mapLastActions$).toBeObservable(expected);
expect((relationEffects as any).removeRelationship).toHaveBeenCalledWith(leftItem, rightItem, relationshipType.leftwardType, '1234',);
expect((relationEffects as any).removeRelationship).toHaveBeenCalledWith(leftItem, rightItem, relationshipType.leftwardType);
});
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Inject, Injectable } from '@angular/core';
import { Actions, createEffect, ofType } from '@ngrx/effects';
import { filter, map, mergeMap, switchMap, take } from 'rxjs/operators';
import { BehaviorSubject, Observable } from 'rxjs';
import { filter, map, mergeMap, switchMap, take, tap, concatMap } from 'rxjs/operators';
import { BehaviorSubject, Observable, Subject } from 'rxjs';
import { RelationshipDataService } from '../../../../../core/data/relationship-data.service';
import {
getRemoteDataPayload,
Expand Down Expand Up @@ -36,11 +36,31 @@ import { TranslateService } from '@ngx-translate/core';

const DEBOUNCE_TIME = 500;

enum RelationOperationType {
Add,
Remove,
}

interface RelationOperation {
type: RelationOperationType
item1: Item
item2: Item
relationshipType: string
submissionId: string
nameVariant?: string
}

/**
* NGRX effects for RelationshipEffects
*/
@Injectable()
export class RelationshipEffects {

/**
* Queue to hold all requests, so we can ensure they get sent one at a time
*/
private requestQueue: Subject<RelationOperation> = new Subject();

/**
* Map that keeps track of the latest RelationshipEffects for each relationship's composed identifier
*/
Expand Down Expand Up @@ -82,9 +102,22 @@ export class RelationshipEffects {
nameVariant = this.nameVariantUpdates[identifier];
delete this.nameVariantUpdates[identifier];
}
this.addRelationship(item1, item2, relationshipType, submissionId, nameVariant);
this.requestQueue.next({
type: RelationOperationType.Add,
item1,
item2,
relationshipType,
submissionId,
nameVariant
});
} else {
this.removeRelationship(item1, item2, relationshipType, submissionId);
this.requestQueue.next({
type: RelationOperationType.Remove,
item1,
item2,
relationshipType,
submissionId,
});
}
}
delete this.debounceMap[identifier];
Expand Down Expand Up @@ -161,8 +194,41 @@ export class RelationshipEffects {
private selectableListService: SelectableListService,
@Inject(DEBOUNCE_TIME_OPERATOR) private debounceTime: <T>(dueTime: number) => (source: Observable<T>) => Observable<T>,
) {
this.executeRequestsInQueue();
}

/**
* Subscribe to the request queue, execute the requests inside. Wait for each request to complete
* before sending the next one
* @private
*/
private executeRequestsInQueue() {
this.requestQueue.pipe(
// concatMap ensures the next request in the queue will only start after the previous one has emitted
concatMap((next: RelationOperation) => {
switch (next.type) {
case RelationOperationType.Add:
return this.addRelationship(next.item1, next.item2, next.relationshipType, next.submissionId, next.nameVariant).pipe(
map(() => next)
);
case RelationOperationType.Remove:
return this.removeRelationship(next.item1, next.item2, next.relationshipType).pipe(
map(() => next)
);
default:
return [next];
}
}),
// refresh the workspaceitem after each request. It would be great if we could find a way to
// optimize this so it only happens when the queue is empty.
switchMap((next: RelationOperation) => this.refreshWorkspaceItemInCache(next.submissionId))
// update the form after the workspaceitem is refreshed
).subscribe((next: SubmissionObject) => {
this.store.dispatch(new SaveSubmissionSectionFormSuccessAction(next.id, [next], false));
});
}


private createIdentifier(item1: Item, item2: Item, relationshipType: string): string {
return `${item1.uuid}-${item2.uuid}-${relationshipType}`;
}
Expand All @@ -185,7 +251,7 @@ export class RelationshipEffects {
}
}),
take(1),
switchMap((rd: RemoteData<Relationship>) => {
tap((rd: RemoteData<Relationship>) => {
if (hasNoValue(rd) || rd.hasFailed) {
// An error occurred, deselect the object from the selectable list and display an error notification
const listId = `list-${submissionId}-${relationshipType}`;
Expand All @@ -203,19 +269,15 @@ export class RelationshipEffects {
}
this.notificationsService.error(this.translateService.instant('relationships.add.error.title'), errorContent);
}
return this.refreshWorkspaceItemInCache(submissionId);
}),
).subscribe((submissionObject: SubmissionObject) => this.store.dispatch(new SaveSubmissionSectionFormSuccessAction(submissionId, [submissionObject], false)));
})
);
}

private removeRelationship(item1: Item, item2: Item, relationshipType: string, submissionId: string) {
this.relationshipService.getRelationshipByItemsAndLabel(item1, item2, relationshipType).pipe(
private removeRelationship(item1: Item, item2: Item, relationshipType: string) {
return this.relationshipService.getRelationshipByItemsAndLabel(item1, item2, relationshipType).pipe(
mergeMap((relationship: Relationship) => this.relationshipService.deleteRelationship(relationship.id, 'none')),
take(1),
switchMap(() => this.refreshWorkspaceItemInCache(submissionId)),
).subscribe((submissionObject: SubmissionObject) => {
this.store.dispatch(new SaveSubmissionSectionFormSuccessAction(submissionId, [submissionObject], false));
});
);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,27 +15,7 @@
(selectObject)="selectObject.emit($event)">
<div additionalSearchOptions *ngIf="repeatable" class="position-absolute">
<div class="input-group mb-3">
<div class="input-group-prepend">
<div class="input-group-text">
<!-- In theory we don't need separate checkboxes for this,
but I wasn't able to get this to work correctly without them.
Checkboxes that are in the indeterminate state always switch to checked when clicked
This seemed like the cleanest and clearest solution to solve this issue for now. -->

<input *ngIf="!allSelected && !(someSelected$ | async)"
type="checkbox"
[indeterminate]="false"
(change)="selectAll()">
<input *ngIf="!allSelected && (someSelected$ | async)"
type="checkbox"
[indeterminate]="true"
(change)="deselectAll()">
<input *ngIf="allSelected" type="checkbox"
[checked]="true"
(change)="deselectAll()">
</div>
</div>
<div ngbDropdown class="input-group-append">
<div ngbDropdown class="input-group dropdown-button">
<button *ngIf="selectAllLoading" type="button"
class="btn btn-outline-secondary rounded-right">
<span class="spinner-border spinner-border-sm" role="status"
Expand All @@ -55,8 +35,6 @@
(click)="selectPage(resultsRD?.page)">{{ ('submission.sections.describe.relationship-lookup.search-tab.select-page' | translate) }}</button>
<button class="dropdown-item"
(click)="deselectPage(resultsRD?.page)">{{ ('submission.sections.describe.relationship-lookup.search-tab.deselect-page' | translate) }}</button>
<button class="dropdown-item" (click)="selectAll()">{{ ('submission.sections.describe.relationship-lookup.search-tab.select-all' | translate) }}</button>
<button class="dropdown-item" (click)="deselectAll()">{{ ('submission.sections.describe.relationship-lookup.search-tab.deselect-all' | translate) }}</button>
</div>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
.position-absolute {
right: var(--bs-spacer);
}

.dropdown-button {
z-index: 3;
}

0 comments on commit 325728d

Please sign in to comment.