From 66eebc2b594b26518d41641ae7dedb89275170ff Mon Sep 17 00:00:00 2001 From: Arya Khochare Date: Tue, 7 Jul 2026 01:23:51 +0530 Subject: [PATCH 1/2] Add ESLint rule to prevent Observable-returning method --- .eslintrc.json | 1 + docs/lint/html/index.md | 1 + .../rules/no-method-call-with-async-pipe.md | 161 ++++++++++++ lint/src/rules/html/index.ts | 2 + .../html/no-method-call-with-async-pipe.ts | 233 ++++++++++++++++++ 5 files changed, 398 insertions(+) create mode 100644 docs/lint/html/rules/no-method-call-with-async-pipe.md create mode 100644 lint/src/rules/html/no-method-call-with-async-pipe.ts diff --git a/.eslintrc.json b/.eslintrc.json index 9102c4719b7..bda42cef8e2 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -341,6 +341,7 @@ // Custom DSpace Angular rules "dspace-angular-html/themed-component-usages": "error", "dspace-angular-html/no-disabled-attribute-on-button": "error", + "dspace-angular-html/no-method-call-with-async-pipe": "error", "@angular-eslint/template/prefer-control-flow": "error" } }, diff --git a/docs/lint/html/index.md b/docs/lint/html/index.md index e134e1070f4..b915e4029b3 100644 --- a/docs/lint/html/index.md +++ b/docs/lint/html/index.md @@ -3,3 +3,4 @@ _______ - [`dspace-angular-html/themed-component-usages`](./rules/themed-component-usages.md): Themeable components should be used via the selector of their `ThemedComponent` wrapper class - [`dspace-angular-html/no-disabled-attribute-on-button`](./rules/no-disabled-attribute-on-button.md): Buttons should use the `dsBtnDisabled` directive instead of the HTML `disabled` attribute. +- [`dspace-angular-html/no-method-call-with-async-pipe`](./rules/no-method-call-with-async-pipe.md): Don't call a method directly as the input of the `async` pipe (e.g. `getFoo$() | async`). diff --git a/docs/lint/html/rules/no-method-call-with-async-pipe.md b/docs/lint/html/rules/no-method-call-with-async-pipe.md new file mode 100644 index 00000000000..008bca34ae3 --- /dev/null +++ b/docs/lint/html/rules/no-method-call-with-async-pipe.md @@ -0,0 +1,161 @@ +[DSpace ESLint plugins](../../../../lint/README.md) > [HTML rules](../index.md) > `dspace-angular-html/no-method-call-with-async-pipe` +_______ + +Don't call a method directly as the input of the `async` pipe (e.g. `getFoo$() | async`). + +This re-executes the method - and the RxJS pipeline it returns - on every change detection cycle, which often leads to performance issues. + +Instead: +- Subscribe to the Observable in `ngOnInit()` +- Push emitted values into a `BehaviorSubject` +- Bind `| async` to that `BehaviorSubject` in the template +- Unsubscribe in `ngOnDestroy()` + + +_______ + +[Source code](../../../../lint/src/rules/html/no-method-call-with-async-pipe.ts) + + + +### Examples + + +#### Valid code + +##### binding the async pipe to a property is still valid + +```html +{{ foo$ | async }} +``` + + +##### calling a method without piping the result through async is still valid + +```html +{{ getFoo() }} +``` + + +##### piping a property through an unrelated pipe is still valid + +```html +{{ foo$ | someOtherPipe }} +``` + + +##### binding the async pipe to a property accessed through a member expression is still valid + +```html +{{ (foo$ | async)?.length }} +``` + + + + + +#### Invalid code + +##### should not call a method as the direct input of the async pipe + +```html +{{ getFoo$() | async }} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method with arguments as the direct input of the async pipe + +```html +{{ getFoo$(id) | async }} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not use the safe-call variant as the direct input of the async pipe + +```html +{{ getFoo$?.() | async }} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe in the *ngIf microsyntax + +```html +
{{ foo }}
+ + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe in an @if condition, even when wrapped in a member access + +```html +@if ((getFoo$() | async)?.length > 0) { + hi +} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe in an @for expression + +```html +@for (item of getFoo$() | async; track item) { + {{ item }} +} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe inside an object literal binding + +```html +
+ + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + + diff --git a/lint/src/rules/html/index.ts b/lint/src/rules/html/index.ts index 7c1da7fef28..8fdaeb7c622 100644 --- a/lint/src/rules/html/index.ts +++ b/lint/src/rules/html/index.ts @@ -11,11 +11,13 @@ import { RuleExports, } from '../../util/structure'; import * as noDisabledAttributeOnButton from './no-disabled-attribute-on-button'; +import * as noMethodCallWithAsyncPipe from './no-method-call-with-async-pipe'; import * as themedComponentUsages from './themed-component-usages'; const index = [ themedComponentUsages, noDisabledAttributeOnButton, + noMethodCallWithAsyncPipe, ] as unknown as RuleExports[]; diff --git a/lint/src/rules/html/no-method-call-with-async-pipe.ts b/lint/src/rules/html/no-method-call-with-async-pipe.ts new file mode 100644 index 00000000000..842ed872e28 --- /dev/null +++ b/lint/src/rules/html/no-method-call-with-async-pipe.ts @@ -0,0 +1,233 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +import { + AST, + BindingPipe, + Call, + PropertyRead, + RecursiveAstVisitor, + SafeCall, + TmplAstBoundAttribute, + TmplAstBoundDeferredTrigger, + TmplAstBoundEvent, + TmplAstBoundText, + TmplAstForLoopBlock, + TmplAstIfBlockBranch, + TmplAstLetDeclaration, + TmplAstSwitchBlock, + TmplAstSwitchBlockCase, +} from '@angular-eslint/bundled-angular-compiler'; +import { ESLintUtils } from '@typescript-eslint/utils'; +import { + RuleContext, + RuleListener, +} from '@typescript-eslint/utils/ts-eslint'; + +import { + DSpaceESLintRuleInfo, + NamedTests, +} from '../../util/structure'; +import { getSourceCode } from '../../util/typescript'; + +export enum Message { + NO_METHOD_CALL_WITH_ASYNC_PIPE = 'noMethodCallWithAsyncPipe', +} + +export const info = { + name: 'no-method-call-with-async-pipe', + meta: { + docs: { + description: `Don't call a method directly as the input of the \`async\` pipe (e.g. \`getFoo$() | async\`). + +This re-executes the method - and the RxJS pipeline it returns - on every change detection cycle, which often leads to performance issues. + +Instead: +- Subscribe to the Observable in \`ngOnInit()\` +- Push emitted values into a \`BehaviorSubject\` +- Bind \`| async\` to that \`BehaviorSubject\` in the template +- Unsubscribe in \`ngOnDestroy()\` + `, + }, + type: 'problem', + schema: [], + messages: { + [Message.NO_METHOD_CALL_WITH_ASYNC_PIPE]: 'Don\'t call \'{{ methodName }}\' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead.', + }, + }, + optionDocs: [], + defaultOptions: [], +} as DSpaceESLintRuleInfo; + +/** + * @angular-eslint/template-parser's own visitor keys (used to drive ESLint's node traversal) + * don't cover every expression AST node type (e.g. SafePropertyRead, LiteralMap, KeyedRead, ...), + * so selectors like `BindingPipe[name="async"] > Call` silently miss anything wrapped in one of + * those unsupported node types (e.g. `(getFoo$() | async)?.length`, or `[ngClass]="{x: getFoo$() | async}"`). + * We work around this by walking each expression ourselves with the Angular compiler's own + * RecursiveAstVisitor, which correctly covers the full expression AST regardless of what + * @angular-eslint/template-parser exposes to ESLint's selector engine. + */ +class AsyncPipedCallVisitor extends RecursiveAstVisitor { + constructor(private readonly onFound: (call: Call | SafeCall) => void) { + super(); + } + + override visitPipe(ast: BindingPipe, context: unknown) { + if (ast.name === 'async' && (ast.exp instanceof Call || ast.exp instanceof SafeCall)) { + this.onFound(ast.exp); + } + super.visitPipe(ast, context); + } +} + +export const rule = ESLintUtils.RuleCreator.withoutDocs({ + meta: info.meta, + defaultOptions: info.defaultOptions, + create(context: RuleContext): RuleListener { + const sourceCode = getSourceCode(context); + + const visitor = new AsyncPipedCallVisitor((node) => { + const methodName = node.receiver instanceof PropertyRead ? node.receiver.name : 'this method'; + + context.report({ + messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE, + loc: { + start: sourceCode.getLocFromIndex(node.sourceSpan.start), + end: sourceCode.getLocFromIndex(node.sourceSpan.end), + }, + data: { + methodName, + }, + }); + }); + + function checkExpression(expression: AST | null | undefined) { + if (expression != null) { + visitor.visit(expression); + } + } + + return { + BoundAttribute(node: TmplAstBoundAttribute) { + checkExpression(node.value); + }, + BoundEvent(node: TmplAstBoundEvent) { + checkExpression(node.handler); + }, + BoundText(node: TmplAstBoundText) { + checkExpression(node.value); + }, + IfBlockBranch(node: TmplAstIfBlockBranch) { + checkExpression(node.expression); + }, + ForLoopBlock(node: TmplAstForLoopBlock) { + checkExpression(node.expression); + checkExpression(node.trackBy); + }, + SwitchBlock(node: TmplAstSwitchBlock) { + checkExpression(node.expression); + }, + SwitchBlockCase(node: TmplAstSwitchBlockCase) { + checkExpression(node.expression); + }, + LetDeclaration(node: TmplAstLetDeclaration) { + checkExpression(node.value); + }, + BoundDeferredTrigger(node: TmplAstBoundDeferredTrigger) { + checkExpression(node.value); + }, + }; + }, +}); + +export const tests = { + plugin: info.name, + valid: [ + { + name: 'binding the async pipe to a property is still valid', + code: ` +{{ foo$ | async }} + `, + }, + { + name: 'calling a method without piping the result through async is still valid', + code: ` +{{ getFoo() }} + `, + }, + { + name: 'piping a property through an unrelated pipe is still valid', + code: ` +{{ foo$ | someOtherPipe }} + `, + }, + { + name: 'binding the async pipe to a property accessed through a member expression is still valid', + code: ` +{{ (foo$ | async)?.length }} + `, + }, + ], + invalid: [ + { + name: 'should not call a method as the direct input of the async pipe', + code: ` +{{ getFoo$() | async }} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method with arguments as the direct input of the async pipe', + code: ` +{{ getFoo$(id) | async }} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not use the safe-call variant as the direct input of the async pipe', + code: ` +{{ getFoo$?.() | async }} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe in the *ngIf microsyntax', + code: ` +
{{ foo }}
+ `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe in an @if condition, even when wrapped in a member access', + code: ` +@if ((getFoo$() | async)?.length > 0) { + hi +} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe in an @for expression', + code: ` +@for (item of getFoo$() | async; track item) { + {{ item }} +} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe inside an object literal binding', + code: ` +
+ `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + ], +} as NamedTests; + +export default rule; From 84a7606fbedb5b62911d61501fff5064825731af Mon Sep 17 00:00:00 2001 From: Arya Khochare Date: Thu, 9 Jul 2026 02:41:32 +0530 Subject: [PATCH 2/2] esLint rule migration --- .../audit-table/audit-table.component.html | 7 +- .../audit-table/audit-table.component.spec.ts | 8 +- .../audit-table/audit-table.component.ts | 54 +++++++++-- .../community-list.component.html | 4 +- .../item-edit-bitstream-bundle.component.html | 2 +- ...em-edit-bitstream-bundle.component.spec.ts | 11 ++- .../item-edit-bitstream-bundle.component.ts | 16 +++- .../metadata-values.component.html | 6 +- .../metadata-values.component.spec.ts | 7 +- .../metadata-values.component.ts | 25 +++++- .../extended-file-section.component.html | 2 +- .../extended-file-section.component.spec.ts | 21 ++--- .../extended-file-section.component.ts | 2 - .../ePerson-data/ePerson-data.component.html | 2 +- .../ePerson-data.component.spec.ts | 5 +- .../ePerson-data/ePerson-data.component.ts | 38 +++++--- .../quality-assurance-events.component.html | 2 +- .../quality-assurance-events.component.ts | 8 ++ .../quality-assurance-source.component.html | 2 +- ...quality-assurance-source.component.spec.ts | 10 ++- .../quality-assurance-source.component.ts | 30 +++---- .../quality-assurance-topics.component.html | 10 +-- ...quality-assurance-topics.component.spec.ts | 10 ++- .../quality-assurance-topics.component.ts | 37 ++++---- .../publication-claim.component.html | 8 +- .../publication-claim.component.ts | 30 +++---- src/app/root/root.component.html | 2 +- .../onebox/dynamic-onebox.component.html | 4 +- .../models/onebox/dynamic-onebox.component.ts | 9 +- src/app/shared/form/form.component.html | 2 +- src/app/shared/form/form.component.spec.ts | 2 +- src/app/shared/form/form.component.ts | 13 ++- ...electable-list-item-control.component.html | 2 +- ...-search-result-list-element.component.html | 2 +- ...arch-result-list-element.component.spec.ts | 4 +- .../search-result-list-element.component.ts | 13 ++- .../form/resource-policy-form.component.html | 2 +- .../resource-policy-form.component.spec.ts | 5 +- .../form/resource-policy-form.component.ts | 19 ++-- .../resource-policies.component.html | 14 +-- .../resource-policies.component.spec.ts | 6 +- .../resource-policies.component.ts | 60 ++++++------- .../section-container.component.html | 6 +- .../section-container.component.spec.ts | 16 ++-- .../section-duplicates.component.html | 2 +- .../section-duplicates.component.ts | 6 ++ .../section-coar-notify.component.html | 18 ++-- .../section-coar-notify.component.ts | 90 +++++++++++-------- .../submission/sections/sections.directive.ts | 24 +---- 49 files changed, 378 insertions(+), 300 deletions(-) diff --git a/src/app/audit-page/audit-table/audit-table.component.html b/src/app/audit-page/audit-table/audit-table.component.html index d44a5131ad3..0f4714a8903 100644 --- a/src/app/audit-page/audit-table/audit-table.component.html +++ b/src/app/audit-page/audit-table/audit-table.component.html @@ -24,7 +24,8 @@ - @for (audit of audits?.page; track audit) { + @for (row of auditRows; track row.audit) { + @let audit = row.audit; @if (audit.hasDetails) { @@ -51,7 +52,7 @@ @if (isOverviewPage) { @if (audit.objectUUID) { - + @if (objectRoute !== ('/' + auditPath)) { {{audit.objectUUID}} } @else { @@ -63,7 +64,7 @@ {{ audit.objectType }} @if (audit.subjectUUID) { - + @if (subjectRoute !== ('/' + auditPath)) { {{audit.subjectUUID}} } @else { diff --git a/src/app/audit-page/audit-table/audit-table.component.spec.ts b/src/app/audit-page/audit-table/audit-table.component.spec.ts index 04515769ec8..65fe6534cf1 100644 --- a/src/app/audit-page/audit-table/audit-table.component.spec.ts +++ b/src/app/audit-page/audit-table/audit-table.component.spec.ts @@ -1,4 +1,7 @@ -import { NO_ERRORS_SCHEMA } from '@angular/core'; +import { + NO_ERRORS_SCHEMA, + SimpleChange, +} from '@angular/core'; import { ComponentFixture, TestBed, @@ -53,6 +56,9 @@ describe('AuditTableComponent', () => { component = fixture.componentInstance; component.audits = audits; component.isOverviewPage = true; + component.ngOnChanges({ + audits: new SimpleChange(null, audits, true), + }); fixture.detectChanges(); }); diff --git a/src/app/audit-page/audit-table/audit-table.component.ts b/src/app/audit-page/audit-table/audit-table.component.ts index 4b548b73f5c..c499d44b0e5 100644 --- a/src/app/audit-page/audit-table/audit-table.component.ts +++ b/src/app/audit-page/audit-table/audit-table.component.ts @@ -8,6 +8,8 @@ import { ChangeDetectorRef, Component, Input, + OnChanges, + SimpleChanges, } from '@angular/core'; import { RouterLink } from '@angular/router'; import { DSpaceObjectDataService } from '@dspace/core/data/dspace-object-data.service'; @@ -18,7 +20,10 @@ import { URLCombiner } from '@dspace/core/url-combiner/url-combiner'; import { NgbCollapseModule } from '@ng-bootstrap/ng-bootstrap'; import { TranslateModule } from '@ngx-translate/core'; import { Observable } from 'rxjs'; -import { map } from 'rxjs/operators'; +import { + map, + shareReplay, +} from 'rxjs/operators'; import { Audit } from '../../core/audit/model/audit.model'; import { DSONameService } from '../../core/breadcrumbs/dso-name.service'; @@ -28,6 +33,15 @@ import { PaginationComponent } from '../../shared/pagination/pagination.componen import { StringReplacePipe } from '../../shared/utils/string-replace.pipe'; import { VarDirective } from '../../shared/utils/var.directive'; +/** + * An audit record paired with its (memoized) resolved object/subject routes. + */ +export interface AuditRow { + audit: Audit; + objectRoute$: Observable; + subjectRoute$: Observable; +} + /** * Renders a paginated table of audit records, either in overview mode for all the environemnt or tied to a specific DSpaceObject. * Supports row expansion to show details. @@ -49,12 +63,17 @@ import { VarDirective } from '../../shared/utils/var.directive'; VarDirective, ], }) -export class AuditTableComponent { +export class AuditTableComponent implements OnChanges { /** * The Audit items to be shown */ @Input() audits: PaginatedList; + /** + * The current page's audit items, paired with their (memoized) resolved object/subject routes. + */ + auditRows: AuditRow[] = []; + /** * Config for pagination */ @@ -81,23 +100,44 @@ export class AuditTableComponent { */ protected readonly dateFormat = 'yyyy-MM-dd HH:mm:ss'; + /** + * Memoized object/subject route observables, keyed by DSpaceObject id. + * Ensures repeated ids (across rows, or across change detection cycles) resolve to the exact + * same Observable instance, rather than triggering a new lookup every time. + */ + private readonly objectRoutesById = new Map>(); + constructor( private dsoNameService: DSONameService, private changeDetectorRef: ChangeDetectorRef, private dsoDataService: DSpaceObjectDataService, ) {} + ngOnChanges(changes: SimpleChanges): void { + if (changes.audits) { + this.auditRows = (this.audits?.page ?? []).map((audit: Audit) => ({ + audit, + objectRoute$: audit.objectUUID ? this.getObjectRoute$(audit.objectUUID) : undefined, + subjectRoute$: audit.subjectUUID ? this.getObjectRoute$(audit.subjectUUID) : undefined, + })); + } + } + toggleCollapse(audit: Audit) { audit.isCollapsed = !audit.isCollapsed; this.changeDetectorRef.detectChanges(); } - getObjectRoute$(id: string): Observable { - return this.dsoDataService.findById(id).pipe( - getFirstSucceededRemoteDataPayload(), - map(resolvedDso => new URLCombiner(getDSORoute(resolvedDso), this.auditPath).toString()), - ); + private getObjectRoute$(id: string): Observable { + if (!this.objectRoutesById.has(id)) { + this.objectRoutesById.set(id, this.dsoDataService.findById(id).pipe( + getFirstSucceededRemoteDataPayload(), + map(resolvedDso => new URLCombiner(getDSORoute(resolvedDso), this.auditPath).toString()), + shareReplay({ bufferSize: 1, refCount: false }), + )); + } + return this.objectRoutesById.get(id); } getDsoName(dso: DSpaceObject): string { diff --git a/src/app/community-list-page/community-list/community-list.component.html b/src/app/community-list-page/community-list/community-list.component.html index 0b6ba6e37e4..6ff93a49a6d 100644 --- a/src/app/community-list-page/community-list/community-list.component.html +++ b/src/app/community-list-page/community-list/community-list.component.html @@ -30,7 +30,7 @@
- @if (hasChild(null, node) | async) { + @if (node.isExpandable$ | async) { - @if ((canReplaceBitstream(entry.bitstream.self) | async)) { + @if ((entry.canReplace$ | async)) {
- {{ (bitstreamFormatDataService.findByBitstream(file) | async).payload?.shortDescription }} + {{ (file.format | async)?.payload?.shortDescription }}
{{ (file?.sizeBytes) | dsFileSize }} diff --git a/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.spec.ts b/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.spec.ts index f809f3713a0..e8d8bb6998d 100644 --- a/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.spec.ts +++ b/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.spec.ts @@ -9,7 +9,6 @@ import { BrowserAnimationsModule } from '@angular/platform-browser/animations'; import { ActivatedRoute } from '@angular/router'; import { APP_CONFIG } from '@dspace/config/app-config.interface'; import { BitstreamDataService } from '@dspace/core/data/bitstream-data.service'; -import { BitstreamFormatDataService } from '@dspace/core/data/bitstream-format-data.service'; import { LocaleService } from '@dspace/core/locale/locale.service'; import { NotificationsService } from '@dspace/core/notification-system/notifications.service'; import { PaginationService } from '@dspace/core/pagination/pagination.service'; @@ -60,6 +59,13 @@ describe('ExtendedFileSectionComponent', () => { }, }); + const mockBitstreamFormat = Object.assign(new BitstreamFormat(), { + resourceType: 'testResourceType', + shortDescription: 'testShortDescription', + description: 'testDescription', + mimetype: 'test/mimeType', + }); + const mockBitstream = Object.assign(new Bitstream(), { id: 'test-bitstream-id', uuid: 'test-bitstream-id', @@ -68,13 +74,7 @@ describe('ExtendedFileSectionComponent', () => { _links: { self: { href: 'test-bitstream-selflink' }, }, - }); - - const mockBitstreamFormat = Object.assign(new BitstreamFormat(), { - resourceType: 'testResourceType', - shortDescription: 'testShortDescription', - description: 'testDescription', - mimetype: 'test/mimeType', + format: createSuccessfulRemoteDataObject$(mockBitstreamFormat), }); const paginatedList = createPaginatedList([mockBitstream]); @@ -85,10 +85,6 @@ describe('ExtendedFileSectionComponent', () => { findAllByItemAndBundleName: createSuccessfulRemoteDataObject$(paginatedList), }); - const bitstreamFormatDataService = jasmine.createSpyObj('bitstreamFormatDataService', { - findByBitstream: createSuccessfulRemoteDataObject$(mockBitstreamFormat), - }); - beforeEach(waitForAsync(() => { TestBed.configureTestingModule({ @@ -109,7 +105,6 @@ describe('ExtendedFileSectionComponent', () => { { provide: XSRFService, useValue: {} }, { provide: BitstreamDataService, useValue: bitstreamDataService }, { provide: NotificationsService, useValue: new NotificationsServiceStub() }, - { provide: BitstreamFormatDataService, useValue: bitstreamFormatDataService }, { provide: ThemeService, useValue: getMockThemeService() }, { provide: SearchConfigurationService, useValue: jasmine.createSpyObj(['getCurrentConfiguration']) }, { provide: PaginationService, useValue: paginationServiceStub }, diff --git a/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.ts b/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.ts index 174f2ee1cf7..3592cf0a7e8 100644 --- a/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.ts +++ b/src/app/item-page/simple/field-components/extended-file-section/extended-file-section.component.ts @@ -11,7 +11,6 @@ import { } from '@dspace/config/app-config.interface'; import { DSONameService } from '@dspace/core/breadcrumbs/dso-name.service'; import { BitstreamDataService } from '@dspace/core/data/bitstream-data.service'; -import { BitstreamFormatDataService } from '@dspace/core/data/bitstream-format-data.service'; import { PaginatedList } from '@dspace/core/data/paginated-list.model'; import { RemoteData } from '@dspace/core/data/remote-data'; import { PaginationService } from '@dspace/core/pagination/pagination.service'; @@ -69,7 +68,6 @@ export class ExtendedFileSectionComponent implements OnInit { constructor( protected bitstreamDataService: BitstreamDataService, - protected bitstreamFormatDataService: BitstreamFormatDataService, public dsoNameService: DSONameService, @Inject(APP_CONFIG) protected appConfig: AppConfig, private paginationService: PaginationService, diff --git a/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.html b/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.html index 2955a44998b..fd866147214 100644 --- a/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.html +++ b/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.html @@ -1,5 +1,5 @@ @if (ePersonId) { - @if (getEPersonData$() | async; as ePersonData) { + @if (ePersonData$ | async; as ePersonData) { @for (property of properties; track property) { @if (ePersonData[property]) { diff --git a/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.spec.ts b/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.spec.ts index 15e2080e7b7..a1a1c23f7ab 100644 --- a/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.spec.ts +++ b/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.spec.ts @@ -1,4 +1,5 @@ /* tslint:disable:no-unused-variable */ +import { SimpleChange } from '@angular/core'; import { ComponentFixture, TestBed, @@ -55,7 +56,9 @@ describe('EPersonDataComponent', () => { const ePersonDataRD$ = createSuccessfulRemoteDataObject$(ePersonData); ePersonDataService.findById.and.returnValue(ePersonDataRD$); component.ePersonId = ePersonId; - component.getEPersonData$(); + component.ngOnChanges({ + ePersonId: new SimpleChange(null, ePersonId, true), + }); fixture.detectChanges(); expect(ePersonDataService.findById).toHaveBeenCalledWith(ePersonId, true); }); diff --git a/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.ts b/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.ts index f1d619fb24e..2b9605597d8 100644 --- a/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.ts +++ b/src/app/notifications/qa/events/ePerson-data/ePerson-data.component.ts @@ -2,6 +2,9 @@ import { AsyncPipe } from '@angular/common'; import { Component, Input, + OnChanges, + OnDestroy, + SimpleChanges, } from '@angular/core'; import { EPersonDataService } from '@dspace/core/eperson/eperson-data.service'; import { EPerson } from '@dspace/core/eperson/models/eperson.model'; @@ -9,7 +12,11 @@ import { getFirstCompletedRemoteData, getRemoteDataPayload, } from '@dspace/core/shared/operators'; -import { Observable } from 'rxjs'; +import { hasValue } from '@dspace/shared/utils/empty.util'; +import { + BehaviorSubject, + Subscription, +} from 'rxjs'; @Component({ selector: 'ds-eperson-data', @@ -22,7 +29,7 @@ import { Observable } from 'rxjs'; /** * Represents the component for displaying ePerson data. */ -export class EPersonDataComponent { +export class EPersonDataComponent implements OnChanges, OnDestroy { /** * The ID of the ePerson. @@ -34,22 +41,31 @@ export class EPersonDataComponent { */ @Input() properties: string[]; + /** + * The EPerson data based on the provided ePersonId. + */ + ePersonData$: BehaviorSubject = new BehaviorSubject(null); + + private subs: Subscription[] = []; + /** * Creates an instance of the EPersonDataComponent. * @param ePersonDataService The service for retrieving ePerson data. */ constructor(private ePersonDataService: EPersonDataService) { } - /** - * Retrieves the EPerson data based on the provided ePersonId. - * @returns An Observable that emits the EPerson data. - */ - getEPersonData$(): Observable { - if (this.ePersonId) { - return this.ePersonDataService.findById(this.ePersonId, true).pipe( - getFirstCompletedRemoteData(), - getRemoteDataPayload(), + ngOnChanges(changes: SimpleChanges): void { + if (changes.ePersonId && hasValue(this.ePersonId)) { + this.subs.push( + this.ePersonDataService.findById(this.ePersonId, true).pipe( + getFirstCompletedRemoteData(), + getRemoteDataPayload(), + ).subscribe((ePerson: EPerson) => this.ePersonData$.next(ePerson)), ); } } + + ngOnDestroy(): void { + this.subs.filter((sub) => hasValue(sub)).forEach((sub) => sub.unsubscribe()); + } } diff --git a/src/app/notifications/qa/events/quality-assurance-events.component.html b/src/app/notifications/qa/events/quality-assurance-events.component.html index 7faca0876c7..6ba154c9731 100644 --- a/src/app/notifications/qa/events/quality-assurance-events.component.html +++ b/src/app/notifications/qa/events/quality-assurance-events.component.html @@ -14,7 +14,7 @@

@if (targetId) { - {{(getTargetItemTitle() | async)}} + {{(targetItemTitle$ | async)}} }

diff --git a/src/app/notifications/qa/events/quality-assurance-events.component.ts b/src/app/notifications/qa/events/quality-assurance-events.component.ts index 65d24ab1264..4cf4f58849c 100644 --- a/src/app/notifications/qa/events/quality-assurance-events.component.ts +++ b/src/app/notifications/qa/events/quality-assurance-events.component.ts @@ -189,6 +189,11 @@ export class QualityAssuranceEventsComponent implements OnInit, OnDestroy { */ isAdmin$: Observable; + /** + * The title of the target item, when a targetId is present. + */ + public targetItemTitle$: Observable; + /** * Initialize the component variables. * @param {ActivatedRoute} activatedRoute @@ -233,6 +238,9 @@ export class QualityAssuranceEventsComponent implements OnInit, OnDestroy { this.targetId = splitList.length > 2 ? splitList.pop() : null; this.selectedTopicName = splitList[1]; this.sourceId = splitList[0]; + if (this.targetId) { + this.targetItemTitle$ = this.getTargetItemTitle(); + } return this.getQualityAssuranceEvents(); }), ).subscribe( diff --git a/src/app/notifications/qa/source/quality-assurance-source.component.html b/src/app/notifications/qa/source/quality-assurance-source.component.html index 54906a0adbe..e5b3bec33f8 100644 --- a/src/app/notifications/qa/source/quality-assurance-source.component.html +++ b/src/app/notifications/qa/source/quality-assurance-source.component.html @@ -5,7 +5,7 @@

{{'quality-assurance.title'| translate}}

- { expect(compAsAny.getQualityAssuranceSource).toHaveBeenCalled(); }); - it(('isSourceLoading should return FALSE'), () => { - expect(comp.isSourceLoading()).toBeObservable(cold('(a|)', { + it(('isSourceLoading$ should return FALSE'), () => { + comp.ngOnInit(); + expect(comp.isSourceLoading$).toBeObservable(cold('(a|)', { a: false, })); }); - it(('isSourceProcessing should return FALSE'), () => { - expect(comp.isSourceProcessing()).toBeObservable(cold('(a|)', { + it(('isSourceProcessing$ should return FALSE'), () => { + comp.ngOnInit(); + expect(comp.isSourceProcessing$).toBeObservable(cold('(a|)', { a: false, })); }); diff --git a/src/app/notifications/qa/source/quality-assurance-source.component.ts b/src/app/notifications/qa/source/quality-assurance-source.component.ts index cc92b7e75f1..4b03c0a2a0f 100644 --- a/src/app/notifications/qa/source/quality-assurance-source.component.ts +++ b/src/app/notifications/qa/source/quality-assurance-source.component.ts @@ -71,6 +71,14 @@ export class QualityAssuranceSourceComponent implements OnDestroy, OnInit, After * The total number of Quality Assurance sources. */ public totalElements$: Observable; + /** + * The loading status of the Quality Assurance source (if it's running or not). + */ + public isSourceLoading$: Observable; + /** + * The processing status of the Quality Assurance source (if it's running or not). + */ + public isSourceProcessing$: Observable; /** * Array to track all the component subscriptions. Useful to unsubscribe them with 'onDestroy'. * @type {Array} @@ -106,6 +114,8 @@ export class QualityAssuranceSourceComponent implements OnDestroy, OnInit, After }), ); this.totalElements$ = this.notificationsStateService.getQualityAssuranceSourceTotals(); + this.isSourceLoading$ = this.notificationsStateService.isQualityAssuranceSourceLoading(); + this.isSourceProcessing$ = this.notificationsStateService.isQualityAssuranceSourceProcessing(); } /** @@ -129,26 +139,6 @@ export class QualityAssuranceSourceComponent implements OnDestroy, OnInit, After this.router.navigate([sourceId], { relativeTo: this.route }); } - /** - * Returns the information about the loading status of the Quality Assurance source (if it's running or not). - * - * @return Observable - * 'true' if the source are loading, 'false' otherwise. - */ - public isSourceLoading(): Observable { - return this.notificationsStateService.isQualityAssuranceSourceLoading(); - } - - /** - * Returns the information about the processing status of the Quality Assurance source (if it's running or not). - * - * @return Observable - * 'true' if there are operations running on the source (ex.: a REST call), 'false' otherwise. - */ - public isSourceProcessing(): Observable { - return this.notificationsStateService.isQualityAssuranceSourceProcessing(); - } - /** * Dispatch the Quality Assurance source retrieval. */ diff --git a/src/app/notifications/qa/topics/quality-assurance-topics.component.html b/src/app/notifications/qa/topics/quality-assurance-topics.component.html index 84c91d21501..ac4592ccfe2 100644 --- a/src/app/notifications/qa/topics/quality-assurance-topics.component.html +++ b/src/app/notifications/qa/topics/quality-assurance-topics.component.html @@ -8,7 +8,7 @@

{{'quality-assurance.title'| translate}}

@if (targetId) { {{'quality-assurance.topics.description-with-target'| translate:{source: sourceId} }} - {{(getTargetItemTitle() | async)}} + {{(targetItemTitle$ | async)}} } @@ -17,20 +17,20 @@

{{'quality-assurance.title'| translate}}

{{'quality-assurance.topics'| translate}}

- @if ((isTopicsLoading() | async)) { + @if ((isTopicsLoading$ | async)) { } - @if ((isTopicsLoading() | async) !== true) { + @if ((isTopicsLoading$ | async) !== true) { - @if ((isTopicsProcessing() | async)) { + @if ((isTopicsProcessing$ | async)) { } - @if ((isTopicsProcessing() | async) !== true) { + @if ((isTopicsProcessing$ | async) !== true) { @if ((topics$ | async)?.length === 0) {