From 9c50466d7dc642bfa5486b2274fb104698564be1 Mon Sep 17 00:00:00 2001 From: nsemets Date: Tue, 27 Oct 2026 13:25:02 +0200 Subject: [PATCH 1/5] fix(error-handling): updated error handling --- src/app/app.routes.server.ts | 4 + src/app/app.routes.ts | 11 +- .../page-not-found.component.html | 11 +- .../page-not-found.component.ts | 7 + .../request-access.component.ts | 35 ++-- src/app/core/constants/error-messages.ts | 16 +- .../constants/http-error-details.const.ts | 2 + .../constants/page-not-found.constants.ts | 11 ++ .../interceptors/error-interceptor.tokens.ts | 6 +- .../core/interceptors/error.interceptor.ts | 156 ++++++++++-------- .../core/services/maintenance-mode.service.ts | 19 ++- .../collections/collections.routes.ts | 3 +- .../preprint-details.component.ts | 17 +- .../services/project-overview.service.ts | 24 +-- .../settings/services/settings.service.ts | 4 +- .../registry-resources.component.ts | 2 - .../services/account-settings.service.ts | 7 +- .../constants/json-api-headers.const.ts | 3 + .../shared/services/addons/addons.service.ts | 38 ++--- .../shared/services/contributors.service.ts | 13 +- .../services/datacite/datacite.service.ts | 25 +-- src/app/shared/services/json-api.service.ts | 12 +- src/assets/i18n/en.json | 12 +- 23 files changed, 238 insertions(+), 200 deletions(-) create mode 100644 src/app/core/constants/http-error-details.const.ts create mode 100644 src/app/core/constants/page-not-found.constants.ts create mode 100644 src/app/shared/constants/json-api-headers.const.ts diff --git a/src/app/app.routes.server.ts b/src/app/app.routes.server.ts index 6e31c3e26..9ffcdb6b4 100644 --- a/src/app/app.routes.server.ts +++ b/src/app/app.routes.server.ts @@ -25,6 +25,10 @@ export const serverRoutes: ServerRoute[] = [ path: 'not-found', renderMode: RenderMode.Prerender, }, + { + path: 'content-deleted', + renderMode: RenderMode.Prerender, + }, { path: 'forgotpassword', renderMode: RenderMode.Prerender, diff --git a/src/app/app.routes.ts b/src/app/app.routes.ts index 36b92bc58..b61606a89 100644 --- a/src/app/app.routes.ts +++ b/src/app/app.routes.ts @@ -7,6 +7,7 @@ import { isFileGuard } from '@core/guards/is-file.guard'; import { isProjectGuard } from '@core/guards/is-project.guard'; import { isRegistryGuard } from '@core/guards/is-registry.guard'; import { redirectIfLoggedInGuard } from '@core/guards/redirect-if-logged-in.guard'; +import { CONTENT_DELETED_ROUTE_DATA, PAGE_NOT_FOUND_ROUTE_DATA } from '@osf/core/constants/page-not-found.constants'; import { MyPreprintsState } from './features/preprints/store/my-preprints'; import { ProfileState } from './features/profile/store'; @@ -195,7 +196,13 @@ export const routes: Routes = [ path: 'not-found', loadComponent: () => import('./core/components/page-not-found/page-not-found.component').then((mod) => mod.PageNotFoundComponent), - data: { skipBreadcrumbs: true }, + data: PAGE_NOT_FOUND_ROUTE_DATA, + }, + { + path: 'content-deleted', + loadComponent: () => + import('./core/components/page-not-found/page-not-found.component').then((mod) => mod.PageNotFoundComponent), + data: CONTENT_DELETED_ROUTE_DATA, }, { path: ':id/files/:fileGuid/preview', @@ -244,7 +251,7 @@ export const routes: Routes = [ path: '**', loadComponent: () => import('./core/components/page-not-found/page-not-found.component').then((mod) => mod.PageNotFoundComponent), - data: { skipBreadcrumbs: true }, + data: PAGE_NOT_FOUND_ROUTE_DATA, }, ], }, diff --git a/src/app/core/components/page-not-found/page-not-found.component.html b/src/app/core/components/page-not-found/page-not-found.component.html index 47db29983..bc1d5613f 100644 --- a/src/app/core/components/page-not-found/page-not-found.component.html +++ b/src/app/core/components/page-not-found/page-not-found.component.html @@ -1,8 +1,7 @@
-

{{ 'pageNotFound.title' | translate }}

-

- {{ 'pageNotFound.message' | translate }} - {{ supportEmail }}. -

+

{{ titleKey | translate }}

+

{{ messageKey | translate }}

+ + {{ 'pageNotFound.contactSupport' | translate }} +
diff --git a/src/app/core/components/page-not-found/page-not-found.component.ts b/src/app/core/components/page-not-found/page-not-found.component.ts index b8e4aadf5..d0f941de1 100644 --- a/src/app/core/components/page-not-found/page-not-found.component.ts +++ b/src/app/core/components/page-not-found/page-not-found.component.ts @@ -1,9 +1,12 @@ import { TranslatePipe } from '@ngx-translate/core'; import { ChangeDetectionStrategy, Component, inject } from '@angular/core'; +import { ActivatedRoute } from '@angular/router'; import { ENVIRONMENT } from '@core/provider/environment.provider'; +import { PAGE_NOT_FOUND_ROUTE_DATA } from '../../constants/page-not-found.constants'; + @Component({ selector: 'osf-page-not-found', imports: [TranslatePipe], @@ -13,5 +16,9 @@ import { ENVIRONMENT } from '@core/provider/environment.provider'; }) export class PageNotFoundComponent { private readonly environment = inject(ENVIRONMENT); + private readonly route = inject(ActivatedRoute); + readonly supportEmail = this.environment.supportEmail; + readonly titleKey = this.route.snapshot.data['titleKey'] ?? PAGE_NOT_FOUND_ROUTE_DATA.titleKey; + readonly messageKey = this.route.snapshot.data['messageKey'] ?? PAGE_NOT_FOUND_ROUTE_DATA.messageKey; } diff --git a/src/app/core/components/request-access/request-access.component.ts b/src/app/core/components/request-access/request-access.component.ts index fe8002095..ee817064f 100644 --- a/src/app/core/components/request-access/request-access.component.ts +++ b/src/app/core/components/request-access/request-access.component.ts @@ -5,7 +5,7 @@ import { TranslatePipe } from '@ngx-translate/core'; import { Button } from 'primeng/button'; import { Textarea } from 'primeng/textarea'; -import { map, of } from 'rxjs'; +import { finalize, map } from 'rxjs'; import { HttpErrorResponse } from '@angular/common/http'; import { ChangeDetectionStrategy, Component, computed, inject, model } from '@angular/core'; @@ -36,7 +36,7 @@ export class RequestAccessComponent { readonly commentLimit = InputLimits.requestAccessComment.maxLength; private readonly route = inject(ActivatedRoute); - private readonly id = toSignal(this.route?.params.pipe(map((params) => params['id'])) ?? of(undefined)); + private readonly id = toSignal(this.route.params.pipe(map((params) => params['id']))); private readonly router = inject(Router); private readonly requestAccessService = inject(RequestAccessService); @@ -55,19 +55,26 @@ export class RequestAccessComponent { ); requestAccess() { + const id = this.id(); + if (!id) { + return; + } + this.loaderService.show(); - this.requestAccessService.requestAccessToProject(this.id(), this.comment()).subscribe({ - next: () => { - this.loaderService.hide(); - this.router.navigate(['/']); - this.toastService.showSuccess('requestAccess.requestedSuccessMessage'); - }, - error: (error: HttpErrorResponse) => { - if (error.status === 409) { - this.toastService.showError('requestAccess.alreadyRequestedMessage'); - } - }, - }); + this.requestAccessService + .requestAccessToProject(id, this.comment()) + .pipe(finalize(() => this.loaderService.hide())) + .subscribe({ + next: () => { + this.toastService.showSuccess('requestAccess.requestedSuccessMessage'); + this.router.navigate(['/']); + }, + error: (error: HttpErrorResponse) => { + if (error.status === 409) { + this.toastService.showError('requestAccess.alreadyRequestedMessage'); + } + }, + }); } switchAccount() { diff --git a/src/app/core/constants/error-messages.ts b/src/app/core/constants/error-messages.ts index c3284b020..b7948442d 100644 --- a/src/app/core/constants/error-messages.ts +++ b/src/app/core/constants/error-messages.ts @@ -1,9 +1,9 @@ export const ERROR_MESSAGES = { - 400: 'Invalid request. Please check your input.', - 401: 'You are not authorized to perform this action.', - 403: `You don't have permission to access this resource.`, - 404: 'The requested resource was not found.', - 500: 'An unexpected error occurred. Please try again later.', - 0: 'An unexpected error occurred. Please try again later.', - default: 'An unknown error occurred. Please try again.', -}; + 0: 'common.errorMessages.serverError', + 400: 'common.errorMessages.badRequest', + 401: 'common.errorMessages.unauthorized', + 403: 'common.errorMessages.forbidden', + 404: 'common.errorMessages.notFound', + 500: 'common.errorMessages.serverError', + default: 'common.errorMessages.unknown', +} as const; diff --git a/src/app/core/constants/http-error-details.const.ts b/src/app/core/constants/http-error-details.const.ts new file mode 100644 index 000000000..afed22ea3 --- /dev/null +++ b/src/app/core/constants/http-error-details.const.ts @@ -0,0 +1,2 @@ +export const PREPRINT_PENDING_MODERATION_DETAIL = + 'This preprint is pending moderation and is not yet publicly available.'; diff --git a/src/app/core/constants/page-not-found.constants.ts b/src/app/core/constants/page-not-found.constants.ts new file mode 100644 index 000000000..c2bd8b2e1 --- /dev/null +++ b/src/app/core/constants/page-not-found.constants.ts @@ -0,0 +1,11 @@ +export const PAGE_NOT_FOUND_ROUTE_DATA = { + skipBreadcrumbs: true, + titleKey: 'pageNotFound.title', + messageKey: 'pageNotFound.message', +} as const; + +export const CONTENT_DELETED_ROUTE_DATA = { + skipBreadcrumbs: true, + titleKey: 'contentDeleted.title', + messageKey: 'contentDeleted.message', +} as const; diff --git a/src/app/core/interceptors/error-interceptor.tokens.ts b/src/app/core/interceptors/error-interceptor.tokens.ts index 83b2c9e1b..395d3381d 100644 --- a/src/app/core/interceptors/error-interceptor.tokens.ts +++ b/src/app/core/interceptors/error-interceptor.tokens.ts @@ -1,3 +1,7 @@ -import { HttpContextToken } from '@angular/common/http'; +import { HttpContext, HttpContextToken } from '@angular/common/http'; export const BYPASS_ERROR_INTERCEPTOR = new HttpContextToken(() => false); + +export function bypassErrorInterceptorContext(): HttpContext { + return new HttpContext().set(BYPASS_ERROR_INTERCEPTOR, true); +} diff --git a/src/app/core/interceptors/error.interceptor.ts b/src/app/core/interceptors/error.interceptor.ts index 127f18219..c4ac11379 100644 --- a/src/app/core/interceptors/error.interceptor.ts +++ b/src/app/core/interceptors/error.interceptor.ts @@ -1,15 +1,14 @@ import { Store } from '@ngxs/store'; -import { throwError } from 'rxjs'; -import { catchError } from 'rxjs/operators'; +import { catchError, throwError } from 'rxjs'; import { isPlatformBrowser } from '@angular/common'; -import { HttpErrorResponse, HttpInterceptorFn } from '@angular/common/http'; +import { HttpErrorResponse, HttpInterceptorFn, HttpRequest } from '@angular/common/http'; import { inject, PLATFORM_ID } from '@angular/core'; import { Router } from '@angular/router'; import { ERROR_MESSAGES } from '@core/constants/error-messages'; -import { MaintenanceResponse } from '@core/models/maintenance-response.model'; +import { PREPRINT_PENDING_MODERATION_DETAIL } from '@core/constants/http-error-details.const'; import { SENTRY_TOKEN } from '@core/provider/sentry.provider'; import { AuthService } from '@core/services/auth.service'; import { MaintenanceModeService } from '@core/services/maintenance-mode.service'; @@ -20,6 +19,35 @@ import { ViewOnlyLinkHelperService } from '@osf/shared/services/view-only-link-h import { BYPASS_ERROR_INTERCEPTOR } from './error-interceptor.tokens'; +const NO_AUTH_REDIRECT_HEADER = 'X-No-Auth-Redirect'; +const REQUEST_ACCESS_URL_REGEX = /\/v2\/nodes\/[^/]+\/requests\/?$/i; +const NODE_ID_URL_REGEX = /\/v2\/nodes\/([^/]+)/; +const DRAFT_REGISTRY_ROUTE_REGEX = /\/registries\/drafts\/.+/i; + +const isServerError = (status: number): boolean => status >= 500 && status < 600; + +const isSilentForbidden = (req: HttpRequest, error: HttpErrorResponse): boolean => + req.headers.has(NO_AUTH_REDIRECT_HEADER) || (!!error.url && REQUEST_ACCESS_URL_REGEX.test(error.url)); + +const isPendingModerationForbidden = (error: HttpErrorResponse): boolean => + error.error?.errors?.[0]?.detail === PREPRINT_PENDING_MODERATION_DETAIL; + +const getErrorMessage = (error: HttpErrorResponse): string => { + if (error.error instanceof ErrorEvent) { + return error.error.message; + } + + if (error.error?.errors?.[0]?.detail) { + return error.error.errors[0].detail; + } + + if (isServerError(error.status)) { + return error.error?.message || ERROR_MESSAGES[500]; + } + + return ERROR_MESSAGES[error.status as keyof typeof ERROR_MESSAGES] || ERROR_MESSAGES.default; +}; + export const errorInterceptor: HttpInterceptorFn = (req, next) => { const toastService = inject(ToastService); const loaderService = inject(LoaderService); @@ -27,89 +55,75 @@ export const errorInterceptor: HttpInterceptorFn = (req, next) => { const authService = inject(AuthService); const maintenanceModeService = inject(MaintenanceModeService); const sentry = inject(SENTRY_TOKEN); - const platformId = inject(PLATFORM_ID); const viewOnlyHelper = inject(ViewOnlyLinkHelperService); const store = inject(Store); + const isBrowser = isPlatformBrowser(inject(PLATFORM_ID)); + + const handleUnauthorized = () => { + if (viewOnlyHelper.hasViewOnlyParam(router)) { + return; + } + + if (store.selectSnapshot(UserSelectors.isAuthenticated)) { + authService.logout(window.location.href); + } else { + authService.navigateToSignIn(); + } + }; + + const handleForbidden = (url: string | null) => { + const nodeId = url?.match(NODE_ID_URL_REGEX)?.[1]; + + if (!nodeId) { + router.navigate(['/forbidden']); + } else if (!DRAFT_REGISTRY_ROUTE_REGEX.test(router.url)) { + router.navigate(['/request-access', nodeId]); + } + }; return next(req).pipe( catchError((error: HttpErrorResponse) => { - let errorMessage: string; - if (req.context.get(BYPASS_ERROR_INTERCEPTOR)) { - sentry.captureException(error); - return throwError(() => error); - } - - if (isPlatformBrowser(platformId) && error.error instanceof ErrorEvent) { - errorMessage = error.error.message; - } else { - if (error.error?.errors?.[0]?.detail) { - errorMessage = error.error.errors[0].detail; - } else { - errorMessage = ERROR_MESSAGES[error.status as keyof typeof ERROR_MESSAGES] || ERROR_MESSAGES.default; - } - } + const rethrow = () => throwError(() => error); - const serverErrorRegex = /5\d{2}/; - const maintenanceResponse = error.error as MaintenanceResponse | null; - - const maintenanceMode = error.status === 503 && maintenanceResponse?.meta?.maintenance_mode === true; - - if (maintenanceMode) { - loaderService.hide(); - if (isPlatformBrowser(platformId)) { - maintenanceModeService.activate(); - } - return throwError(() => error); + if (!isBrowser) { + return rethrow(); } - if (serverErrorRegex.test(error.status.toString())) { - errorMessage = error.error.message || 'common.errorMessages.serverError'; - } - - if (error.status === 409) { - return throwError(() => error); + if (req.context.get(BYPASS_ERROR_INTERCEPTOR)) { + sentry.captureException(error); + return rethrow(); } - if (error.status === 401) { - if (!viewOnlyHelper.hasViewOnlyParam(router)) { - if (isPlatformBrowser(platformId)) { - if (store.selectSnapshot(UserSelectors.isAuthenticated)) { - authService.logout(window.location.href); - } else { - authService.navigateToSignIn(); - } - } - } - return throwError(() => error); + if (maintenanceModeService.isMaintenanceError(error)) { + loaderService.hide(); + maintenanceModeService.activate(); + return rethrow(); } - if (error.status === 403) { - const requestAccessRegex = /\/v2\/nodes\/[^/]+\/requests\/?$/i; - - if (error.url && (requestAccessRegex.test(error.url) || req.headers.has('X-No-Auth-Redirect'))) { - loaderService.hide(); - return throwError(() => error); - } - - if (error.url?.includes('v2/nodes/')) { - const match = error.url.match(/\/nodes\/([^/]+)/); - const id = match ? match[1] : null; - const draftRegistryUrlRegex = /\/registries\/drafts\/.+/i; - if (!draftRegistryUrlRegex.test(router.url)) { - router.navigate([`/request-access/${id}`]); + switch (error.status) { + case 401: + handleUnauthorized(); + break; + case 403: + if (!isSilentForbidden(req, error) && !isPendingModerationForbidden(error)) { + handleForbidden(error.url); } - } else { - router.navigate(['/forbidden']); - } + break; + case 404: + router.navigate(['/not-found']); + break; + case 409: + return rethrow(); + case 410: + router.navigate(['/content-deleted']); + break; + default: + toastService.showError(getErrorMessage(error)); + break; } loaderService.hide(); - - if (isPlatformBrowser(platformId)) { - toastService.showError(errorMessage); - } - - return throwError(() => error); + return rethrow(); }) ); }; diff --git a/src/app/core/services/maintenance-mode.service.ts b/src/app/core/services/maintenance-mode.service.ts index bb92be21c..f5a501d76 100644 --- a/src/app/core/services/maintenance-mode.service.ts +++ b/src/app/core/services/maintenance-mode.service.ts @@ -1,13 +1,13 @@ import { catchError, map, Observable, of, Subscription, switchMap, timer } from 'rxjs'; -import { HttpClient, HttpContext, HttpErrorResponse } from '@angular/common/http'; +import { HttpClient, HttpErrorResponse } from '@angular/common/http'; import { inject, Injectable, OnDestroy, signal } from '@angular/core'; import { MaintenanceStatus } from '@core/enums/maintenance-status.enum'; import { MaintenanceResponse } from '@core/models/maintenance-response.model'; import { ENVIRONMENT } from '@core/provider/environment.provider'; -import { BYPASS_ERROR_INTERCEPTOR } from '../interceptors/error-interceptor.tokens'; +import { bypassErrorInterceptorContext } from '../interceptors/error-interceptor.tokens'; @Injectable({ providedIn: 'root', @@ -18,7 +18,6 @@ export class MaintenanceModeService implements OnDestroy { private readonly POLL_INTERVAL_MS = 5 * 60 * 1_000; private readonly _isActive = signal(false); - private readonly bypassContext = new HttpContext().set(BYPASS_ERROR_INTERCEPTOR, true); private pollingSubscription: Subscription | null = null; @@ -45,6 +44,11 @@ export class MaintenanceModeService implements OnDestroy { }); } + isMaintenanceError(error: HttpErrorResponse): boolean { + const response = error.error as MaintenanceResponse | null; + return error.status === 503 && response?.meta?.maintenance_mode === true; + } + activate(): void { this._isActive.set(true); if (this.isForcedByConfig || this.pollingSubscription) { @@ -83,7 +87,9 @@ export class MaintenanceModeService implements OnDestroy { private checkMaintenanceStatus(): Observable { return this.http - .get(`${this.environment.apiDomainUrl}/v2/`, { context: this.bypassContext }) + .get(`${this.environment.apiDomainUrl}/v2/`, { + context: bypassErrorInterceptorContext(), + }) .pipe( map((response) => response.meta?.maintenance_mode === true ? MaintenanceStatus.Active : MaintenanceStatus.Inactive @@ -93,9 +99,6 @@ export class MaintenanceModeService implements OnDestroy { } private statusFromError(error: HttpErrorResponse): MaintenanceStatus { - const response = error.error as MaintenanceResponse | null; - return error.status === 503 && response?.meta?.maintenance_mode === true - ? MaintenanceStatus.Active - : MaintenanceStatus.Unknown; + return this.isMaintenanceError(error) ? MaintenanceStatus.Active : MaintenanceStatus.Unknown; } } diff --git a/src/app/features/collections/collections.routes.ts b/src/app/features/collections/collections.routes.ts index ee9fe1831..e7827146e 100644 --- a/src/app/features/collections/collections.routes.ts +++ b/src/app/features/collections/collections.routes.ts @@ -3,6 +3,7 @@ import { provideStates } from '@ngxs/store'; import { Routes } from '@angular/router'; import { authGuard } from '@core/guards/auth.guard'; +import { PAGE_NOT_FOUND_ROUTE_DATA } from '@osf/core/constants/page-not-found.constants'; import { ConfirmLeavingGuard } from '@osf/shared/guards'; import { ActivityLogsState } from '@osf/shared/stores/activity-logs'; import { BookmarksState } from '@osf/shared/stores/bookmarks'; @@ -27,7 +28,7 @@ export const collectionsRoutes: Routes = [ pathMatch: 'full', loadComponent: () => import('@core/components/page-not-found/page-not-found.component').then((m) => m.PageNotFoundComponent), - data: { skipBreadcrumbs: true }, + data: PAGE_NOT_FOUND_ROUTE_DATA, }, { path: ':providerId', diff --git a/src/app/features/preprints/pages/preprint-details/preprint-details.component.ts b/src/app/features/preprints/pages/preprint-details/preprint-details.component.ts index a3083d392..d454e1bb2 100644 --- a/src/app/features/preprints/pages/preprint-details/preprint-details.component.ts +++ b/src/app/features/preprints/pages/preprint-details/preprint-details.component.ts @@ -24,11 +24,11 @@ import { import { takeUntilDestroyed, toObservable, toSignal } from '@angular/core/rxjs-interop'; import { ActivatedRoute, Router, RouterLink } from '@angular/router'; +import { PREPRINT_PENDING_MODERATION_DETAIL } from '@core/constants/http-error-details.const'; import { ENVIRONMENT } from '@core/provider/environment.provider'; import { HelpScoutService } from '@core/services/help-scout.service'; import { PrerenderReadyService } from '@core/services/prerender-ready.service'; import { ClearCurrentProvider } from '@core/store/provider'; -import { UserSelectors } from '@core/store/user'; import { ReviewPermissions } from '@osf/shared/enums/review-permissions.enum'; import { ReviewActionTrigger } from '@osf/shared/enums/trigger-action.enum'; import { CustomDialogService } from '@osf/shared/services/custom-dialog.service'; @@ -120,7 +120,6 @@ export class PreprintDetailsComponent implements OnInit, OnDestroy { clearCurrentProvider: ClearCurrentProvider, }); - currentUser = select(UserSelectors.getCurrentUser); preprintProvider = select(PreprintProvidersSelectors.getPreprintProviderDetails(this.providerId())); isPreprintProviderLoading = select(PreprintProvidersSelectors.isPreprintProviderDetailsLoading); preprint = select(PreprintSelectors.getPreprint); @@ -362,11 +361,11 @@ export class PreprintDetailsComponent implements OnInit, OnDestroy { this.actions .createNewVersion(preprintId) .pipe( - catchError((e) => { + catchError((error: HttpErrorResponse) => { this.loaderService.hide(); - if (e instanceof HttpErrorResponse && e.status === 409) { - this.toastService.showError(e.error.errors[0].detail); + if (error.status === 409) { + this.toastService.showError(error?.error?.errors[0]?.detail); } return EMPTY; @@ -411,12 +410,8 @@ export class PreprintDetailsComponent implements OnInit, OnDestroy { } } }, - error: (error) => { - if ( - error instanceof HttpErrorResponse && - error.status === 403 && - error?.error?.errors[0]?.detail === 'This preprint is pending moderation and is not yet publicly available.' - ) { + error: (error: HttpErrorResponse) => { + if (error?.status === 403 && error.error?.errors?.[0]?.detail === PREPRINT_PENDING_MODERATION_DETAIL) { this.router.navigate(['/preprints', this.providerId(), preprintId, 'pending-moderation']); } }, diff --git a/src/app/features/project/overview/services/project-overview.service.ts b/src/app/features/project/overview/services/project-overview.service.ts index ae2e609c7..9b93a910c 100644 --- a/src/app/features/project/overview/services/project-overview.service.ts +++ b/src/app/features/project/overview/services/project-overview.service.ts @@ -1,11 +1,10 @@ -import { Observable, of } from 'rxjs'; -import { map, switchMap } from 'rxjs/operators'; +import { map, Observable, of, switchMap } from 'rxjs'; -import { HttpContext } from '@angular/common/http'; import { inject, Injectable } from '@angular/core'; -import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; +import { bypassErrorInterceptorContext } from '@core/interceptors/error-interceptor.tokens'; import { ENVIRONMENT } from '@core/provider/environment.provider'; +import { JSON_API_BULK_HEADERS } from '@osf/shared/constants/json-api-headers.const'; import { IdentifiersMapper } from '@osf/shared/mappers/identifiers.mapper'; import { InstitutionsMapper } from '@osf/shared/mappers/institutions'; import { LicensesMapper } from '@osf/shared/mappers/licenses.mapper'; @@ -92,12 +91,8 @@ export class ProjectOverviewService { data: data.map((item) => ({ id: item.id, type: 'nodes', attributes: { public: item.public } })), }; - const headers = { - 'Content-Type': 'application/vnd.api+json; ext=bulk', - }; - return this.jsonApiService - .patch(`${this.apiUrl}/nodes/`, payload, undefined, headers) + .patch(`${this.apiUrl}/nodes/`, payload, undefined, JSON_API_BULK_HEADERS) .pipe(map((res) => BaseNodeMapper.getNodesData(res))); } @@ -189,11 +184,8 @@ export class ProjectOverviewService { getParentProject(projectId: string): Observable { const params: Record = { 'embed[]': ['bibliographic_contributors'] }; - const context = new HttpContext(); - context.set(BYPASS_ERROR_INTERCEPTOR, true); - return this.jsonApiService - .get(`${this.apiUrl}/nodes/${projectId}/`, params, context) + .get(`${this.apiUrl}/nodes/${projectId}/`, params, bypassErrorInterceptorContext()) .pipe(map((response) => BaseNodeMapper.getNodeWithEmbedContributors(response.data))); } @@ -208,15 +200,11 @@ export class ProjectOverviewService { })), }; - const headers = { - 'Content-Type': 'application/vnd.api+json; ext=bulk', - }; - return this.jsonApiService.patch( `${this.apiUrl}/nodes/${projectId}/reorder_components/`, payload, undefined, - headers + JSON_API_BULK_HEADERS ); } } diff --git a/src/app/features/project/settings/services/settings.service.ts b/src/app/features/project/settings/services/settings.service.ts index 6337092e2..3c965041d 100644 --- a/src/app/features/project/settings/services/settings.service.ts +++ b/src/app/features/project/settings/services/settings.service.ts @@ -3,6 +3,7 @@ import { map, Observable } from 'rxjs'; import { inject, Injectable } from '@angular/core'; import { ENVIRONMENT } from '@core/provider/environment.provider'; +import { JSON_API_BULK_HEADERS } from '@osf/shared/constants/json-api-headers.const'; import { CurrentResourceType } from '@osf/shared/enums/resource-type.enum'; import { SubscriptionFrequency } from '@osf/shared/enums/subscriptions/subscription-frequency.enum'; import { NotificationSubscriptionMapper } from '@osf/shared/mappers/notification-subscription.mapper'; @@ -85,9 +86,8 @@ export class SettingsService { deleteProject(projects: NodeShortInfoModel[]): Observable { const payload = { data: projects.map((project) => ({ id: project.id, type: CurrentResourceType.Projects })) }; - const headers = { 'Content-Type': 'application/vnd.api+json; ext=bulk' }; - return this.jsonApiService.delete(`${this.apiUrl}/nodes/`, payload, headers); + return this.jsonApiService.delete(`${this.apiUrl}/nodes/`, payload, JSON_API_BULK_HEADERS); } deleteInstitution(institutionId: string, projectId: string): Observable { diff --git a/src/app/features/registry/pages/registry-resources/registry-resources.component.ts b/src/app/features/registry/pages/registry-resources/registry-resources.component.ts index 00f559dbd..623572577 100644 --- a/src/app/features/registry/pages/registry-resources/registry-resources.component.ts +++ b/src/app/features/registry/pages/registry-resources/registry-resources.component.ts @@ -126,7 +126,6 @@ export class RegistryResourcesComponent { this.resetPagination(); this.toastService.showSuccess('resources.toastMessages.addResourceSuccess'); }, - error: () => this.toastService.showError('resources.toastMessages.addResourceError'), }); } @@ -146,7 +145,6 @@ export class RegistryResourcesComponent { ) .subscribe({ next: () => this.toastService.showSuccess('resources.toastMessages.updatedResourceSuccess'), - error: () => this.toastService.showError('resources.toastMessages.updateResourceError'), }); } diff --git a/src/app/features/settings/account-settings/services/account-settings.service.ts b/src/app/features/settings/account-settings/services/account-settings.service.ts index e58fc1696..1ec363a9c 100644 --- a/src/app/features/settings/account-settings/services/account-settings.service.ts +++ b/src/app/features/settings/account-settings/services/account-settings.service.ts @@ -1,9 +1,8 @@ import { map, Observable } from 'rxjs'; -import { HttpContext } from '@angular/common/http'; import { inject, Injectable } from '@angular/core'; -import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; +import { bypassErrorInterceptorContext } from '@core/interceptors/error-interceptor.tokens'; import { ENVIRONMENT } from '@core/provider/environment.provider'; import { UserMapper } from '@osf/shared/mappers/user'; import { UserModel } from '@osf/shared/models/user/user.model'; @@ -115,13 +114,11 @@ export class AccountSettingsService { } deleteExternalIdentity(id: string): Observable { - const context = new HttpContext().set(BYPASS_ERROR_INTERCEPTOR, true); - return this.jsonApiService.delete( `${this.apiUrl}/users/me/settings/identities/${id}/`, undefined, undefined, - context + bypassErrorInterceptorContext() ); } } diff --git a/src/app/shared/constants/json-api-headers.const.ts b/src/app/shared/constants/json-api-headers.const.ts new file mode 100644 index 000000000..12d65cffa --- /dev/null +++ b/src/app/shared/constants/json-api-headers.const.ts @@ -0,0 +1,3 @@ +export const JSON_API_BULK_HEADERS: Record = { + 'Content-Type': 'application/vnd.api+json; ext=bulk', +}; diff --git a/src/app/shared/services/addons/addons.service.ts b/src/app/shared/services/addons/addons.service.ts index cf12e1983..91701cf5d 100644 --- a/src/app/shared/services/addons/addons.service.ts +++ b/src/app/shared/services/addons/addons.service.ts @@ -1,11 +1,10 @@ import { select } from '@ngxs/store'; -import { map, Observable } from 'rxjs'; +import { map, Observable, throwError } from 'rxjs'; -import { HttpContext } from '@angular/common/http'; import { inject, Injectable } from '@angular/core'; -import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; +import { bypassErrorInterceptorContext } from '@core/interceptors/error-interceptor.tokens'; import { ENVIRONMENT } from '@core/provider/environment.provider'; import { UserSelectors } from '@core/store/user'; import { AddonMapper } from '@osf/shared/mappers/addon.mapper'; @@ -64,7 +63,9 @@ export class AddonsService { getAddonsUserReference(): Observable { const currentUser = this.currentUser(); - if (!currentUser) throw new Error('Current user not found'); + if (!currentUser) { + return throwError(() => new Error('Current user not found')); + } const userUri = `${this.webUrl}/${currentUser.id}`; const params = { 'filter[user_uri]': userUri }; @@ -98,24 +99,23 @@ export class AddonsService { } getAuthorizedStorageOauthToken(accountId: string, addonType: string): Observable { - const context = new HttpContext(); - context.set(BYPASS_ERROR_INTERCEPTOR, true); + const payload = { + data: { + id: accountId, + type: `authorized-${addonType}-accounts`, + attributes: { serialize_oauth_token: 'true' }, + }, + }; return this.jsonApiService .patch( `${this.apiUrl}/authorized-${addonType}-accounts/${accountId}`, - { - data: { - id: accountId, - type: `authorized-${addonType}-accounts`, - attributes: { serialize_oauth_token: 'true' }, - }, - }, - {}, - {}, - context + payload, + undefined, + undefined, + bypassErrorInterceptorContext() ) - .pipe(map((response) => AddonMapper.fromAuthorizedAddonResponse(response as AuthorizedAddonDataJsonApi))); + .pipe(map((response) => AddonMapper.fromAuthorizedAddonResponse(response))); } getConfiguredAddons(addonType: string, referenceId: string): Observable { @@ -149,8 +149,8 @@ export class AddonsService { addonType: string, addonId: string ): Observable { - return this.jsonApiService.http - .patch( + return this.jsonApiService + .patchResponse( `${this.apiUrl}/authorized-${addonType}-accounts/${addonId}/?include=external-${addonType}-service`, addonRequestPayload ) diff --git a/src/app/shared/services/contributors.service.ts b/src/app/shared/services/contributors.service.ts index 094c3beee..4360da2b7 100644 --- a/src/app/shared/services/contributors.service.ts +++ b/src/app/shared/services/contributors.service.ts @@ -4,6 +4,7 @@ import { inject, Injectable } from '@angular/core'; import { ENVIRONMENT } from '@core/provider/environment.provider'; import { DEFAULT_TABLE_PARAMS } from '@osf/shared/constants/default-table-params.constants'; +import { JSON_API_BULK_HEADERS } from '@osf/shared/constants/json-api-headers.const'; import { AddContributorType } from '../enums/contributors/add-contributor-type.enum'; import { ContributorPermission } from '../enums/contributors/contributor-permission.enum'; @@ -201,12 +202,8 @@ export class ContributorsService { data: contributors.map((contributor) => ContributorsMapper.toContributorUpdateRequest(contributor)), }; - const headers = { - 'Content-Type': 'application/vnd.api+json; ext=bulk', - }; - return this.jsonApiService - .patch(baseUrl, contributorData, undefined, headers) + .patch(baseUrl, contributorData, undefined, JSON_API_BULK_HEADERS) .pipe(map((response) => ContributorsMapper.getContributors(response.data))); } @@ -229,12 +226,8 @@ export class ContributorsService { }), }; - const headers = { - 'Content-Type': 'application/vnd.api+json; ext=bulk', - }; - return this.jsonApiService - .post(baseUrl, contributorData, undefined, headers) + .post(baseUrl, contributorData, undefined, JSON_API_BULK_HEADERS) .pipe(map((response) => ContributorsMapper.getContributors(response.data))); } diff --git a/src/app/shared/services/datacite/datacite.service.ts b/src/app/shared/services/datacite/datacite.service.ts index 38ed9f53d..3efff79b6 100644 --- a/src/app/shared/services/datacite/datacite.service.ts +++ b/src/app/shared/services/datacite/datacite.service.ts @@ -1,10 +1,10 @@ import { EMPTY, filter, map, Observable, of, switchMap, take } from 'rxjs'; import { isPlatformBrowser } from '@angular/common'; -import { HttpClient, HttpContext } from '@angular/common/http'; +import { HttpClient } from '@angular/common/http'; import { inject, Injectable, PLATFORM_ID } from '@angular/core'; -import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; +import { bypassErrorInterceptorContext } from '@core/interceptors/error-interceptor.tokens'; import { ENVIRONMENT } from '@core/provider/environment.provider'; import { DataciteEvent } from '@osf/shared/enums/datacite/datacite-event.enum'; import { IdentifierModel } from '@osf/shared/models/identifiers/identifier.model'; @@ -61,7 +61,7 @@ export class DataciteService { private logFile(targetId: string, targetType: string, event: DataciteEvent): Observable { const url = `${this.apiDomainUrl}/v2/${targetType}/${targetId}/identifiers`; - return this.http.get(url).pipe( + return this.http.get(url, { context: bypassErrorInterceptorContext() }).pipe( map((item) => ({ identifiers: item.data.map((identifierData) => ({ id: identifierData.id, @@ -74,21 +74,8 @@ export class DataciteService { ); } - /** - * Internal helper to log a specific Datacite event for a given DOI. - * Only tracks in browser environment using sendBeacon with HTTP POST fallback. - * - * @param event - The Datacite event type (VIEW or DOWNLOAD). - * @param doi - The DOI (Digital Object Identifier) of the resource. - * @returns An Observable that completes when the tracking request is sent, - * or EMPTY if DOI, repo ID is missing, or not in browser. - */ private logActivity(event: DataciteEvent, doi: string): Observable { - if (!doi || !this.dataciteTrackerRepoId) { - return EMPTY; - } - - if (!isPlatformBrowser(this.platformId)) { + if (!this.dataciteTrackerRepoId || !isPlatformBrowser(this.platformId)) { return EMPTY; } @@ -105,13 +92,11 @@ export class DataciteService { return of(void 0); } else { const headers = { 'Content-Type': 'application/json' }; - const context = new HttpContext(); - context.set(BYPASS_ERROR_INTERCEPTOR, true); return this.http .post(this.dataciteTrackerAddress, payload, { headers, - context, + context: bypassErrorInterceptorContext(), }) .pipe(map(() => undefined)); } diff --git a/src/app/shared/services/json-api.service.ts b/src/app/shared/services/json-api.service.ts index 9997fb335..52ebfcf39 100644 --- a/src/app/shared/services/json-api.service.ts +++ b/src/app/shared/services/json-api.service.ts @@ -9,7 +9,7 @@ import { JsonApiResponse } from '../models/common/json-api/responses.model'; providedIn: 'root', }) export class JsonApiService { - http: HttpClient = inject(HttpClient); + private readonly http = inject(HttpClient); get( url: string, @@ -61,6 +61,16 @@ export class JsonApiService { .pipe(map((response) => response?.data)); } + patchResponse( + url: string, + body: unknown, + params?: Record, + headers?: Record, + context?: HttpContext + ): Observable { + return this.http.patch(url, body, { params: this.buildHttpParams(params), headers, context }); + } + put(url: string, body: unknown, params?: Record): Observable { return this.http .put>(url, body, { params: this.buildHttpParams(params) }) diff --git a/src/assets/i18n/en.json b/src/assets/i18n/en.json index dde38924e..7304da4ff 100644 --- a/src/assets/i18n/en.json +++ b/src/assets/i18n/en.json @@ -480,7 +480,12 @@ "message": "Are you sure you want to discard your unsaved changes?" }, "errorMessages": { + "badRequest": "Invalid request. Please check your input.", + "unauthorized": "You are not authorized to perform this action.", + "forbidden": "You don't have permission to access this resource.", + "notFound": "The requested resource was not found.", "serverError": "An unexpected error occurred. Please try again later.", + "unknown": "An unknown error occurred. Please try again.", "actionUnavailable": "This action is no longer available. Contact support if you have any questions." }, "hint": { @@ -1281,8 +1286,13 @@ "software": "Software", "other": "Other" }, + "contentDeleted": { + "message": "This page has been removed and is no longer available. Contact support if you have any questions.", + "title": "This content has been deleted" + }, "pageNotFound": { - "message": "The page you were looking for is either doesn't exist, was deleted, or the URL may be incorrect. If this should not have occurred and the issue persists, please report it to", + "contactSupport": "Contact Support", + "message": "The page you're looking for doesn't exist or may have been moved. Contact support if you have any questions.", "title": "Page not found" }, "preprints": { From 7b1003e8a7410037d5c9943fc4764af895a1b358 Mon Sep 17 00:00:00 2001 From: nsemets Date: Tue, 27 Oct 2026 13:57:55 +0200 Subject: [PATCH 2/5] test(error-handling): updated unit tests --- .../page-not-found.component.spec.ts | 38 ++- .../request-access.component.spec.ts | 15 +- .../interceptors/error.interceptor.spec.ts | 132 ++++++++++- .../services/maintenance-mode.service.spec.ts | 31 +++ .../preprint-details.component.spec.ts | 3 +- .../services/settings.service.spec.ts | 3 +- .../registry-resources.component.spec.ts | 32 +-- .../registry-resources.component.ts | 2 - .../services/account-settings.service.spec.ts | 190 +++++++++++++-- .../services/addons/addons.service.spec.ts | 216 ++++++++++++------ .../maintenance-mode.service.mock.ts | 5 + 11 files changed, 531 insertions(+), 136 deletions(-) diff --git a/src/app/core/components/page-not-found/page-not-found.component.spec.ts b/src/app/core/components/page-not-found/page-not-found.component.spec.ts index 9cf89eddf..757476f29 100644 --- a/src/app/core/components/page-not-found/page-not-found.component.spec.ts +++ b/src/app/core/components/page-not-found/page-not-found.component.spec.ts @@ -1,6 +1,9 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { CONTENT_DELETED_ROUTE_DATA, PAGE_NOT_FOUND_ROUTE_DATA } from '@core/constants/page-not-found.constants'; + import { provideOSFCore } from '@testing/osf.testing.provider'; +import { ActivatedRouteMockBuilder, provideActivatedRouteMock } from '@testing/providers/route-provider.mock'; import { PageNotFoundComponent } from './page-not-found.component'; @@ -8,22 +11,51 @@ describe('PageNotFoundComponent', () => { let component: PageNotFoundComponent; let fixture: ComponentFixture; - beforeEach(() => { + function setup(data: Record = {}) { TestBed.configureTestingModule({ imports: [PageNotFoundComponent], - providers: [provideOSFCore()], + providers: [ + provideOSFCore(), + provideActivatedRouteMock(ActivatedRouteMockBuilder.create().withData(data).build()), + ], }); fixture = TestBed.createComponent(PageNotFoundComponent); component = fixture.componentInstance; fixture.detectChanges(); - }); + } it('should create', () => { + setup(); expect(component).toBeTruthy(); }); it('should expose support email from environment', () => { + setup(); expect(component.supportEmail).toBe('support@test.com'); }); + + it('should fall back to page not found keys when route data is empty', () => { + setup(); + expect(component.titleKey).toBe(PAGE_NOT_FOUND_ROUTE_DATA.titleKey); + expect(component.messageKey).toBe(PAGE_NOT_FOUND_ROUTE_DATA.messageKey); + }); + + it('should use page not found route data when provided', () => { + setup(PAGE_NOT_FOUND_ROUTE_DATA); + expect(component.titleKey).toBe(PAGE_NOT_FOUND_ROUTE_DATA.titleKey); + expect(component.messageKey).toBe(PAGE_NOT_FOUND_ROUTE_DATA.messageKey); + }); + + it('should use content deleted route data when provided', () => { + setup(CONTENT_DELETED_ROUTE_DATA); + expect(component.titleKey).toBe(CONTENT_DELETED_ROUTE_DATA.titleKey); + expect(component.messageKey).toBe(CONTENT_DELETED_ROUTE_DATA.messageKey); + }); + + it('should render a contact support mailto link', () => { + setup(); + const link = fixture.nativeElement.querySelector('a') as HTMLAnchorElement; + expect(link.getAttribute('href')).toBe('mailto:support@test.com'); + }); }); diff --git a/src/app/core/components/request-access/request-access.component.spec.ts b/src/app/core/components/request-access/request-access.component.spec.ts index 48a7984a7..61b30d41c 100644 --- a/src/app/core/components/request-access/request-access.component.spec.ts +++ b/src/app/core/components/request-access/request-access.component.spec.ts @@ -118,8 +118,19 @@ describe('RequestAccessComponent', () => { expect(loaderServiceMock.show).toHaveBeenCalled(); expect(requestAccessServiceMock.requestAccessToProject).toHaveBeenCalledWith('project-123', 'please grant access'); expect(loaderServiceMock.hide).toHaveBeenCalled(); - expect(routerMock.navigate).toHaveBeenCalledWith(['/']); expect(toastServiceMock.showSuccess).toHaveBeenCalledWith('requestAccess.requestedSuccessMessage'); + expect(routerMock.navigate).toHaveBeenCalledWith(['/']); + expect(toastServiceMock.showSuccess.mock.invocationCallOrder[0]).toBeLessThan( + (routerMock.navigate as Mock).mock.invocationCallOrder[0] + ); + }); + + it('should not request access when route id is missing', () => { + setup({ routeId: '' }); + component.requestAccess(); + + expect(loaderServiceMock.show).not.toHaveBeenCalled(); + expect(requestAccessServiceMock.requestAccessToProject).not.toHaveBeenCalled(); }); it('should show already requested error on 409', () => { @@ -129,6 +140,7 @@ describe('RequestAccessComponent', () => { component.requestAccess(); expect(loaderServiceMock.show).toHaveBeenCalled(); + expect(loaderServiceMock.hide).toHaveBeenCalled(); expect(toastServiceMock.showError).toHaveBeenCalledWith('requestAccess.alreadyRequestedMessage'); expect(routerMock.navigate).not.toHaveBeenCalled(); }); @@ -139,6 +151,7 @@ describe('RequestAccessComponent', () => { }); component.requestAccess(); + expect(loaderServiceMock.hide).toHaveBeenCalled(); expect(toastServiceMock.showError).not.toHaveBeenCalled(); expect(routerMock.navigate).not.toHaveBeenCalled(); }); diff --git a/src/app/core/interceptors/error.interceptor.spec.ts b/src/app/core/interceptors/error.interceptor.spec.ts index 74a78140d..423e1417a 100644 --- a/src/app/core/interceptors/error.interceptor.spec.ts +++ b/src/app/core/interceptors/error.interceptor.spec.ts @@ -7,6 +7,8 @@ import { PLATFORM_ID } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { Router } from '@angular/router'; +import { ERROR_MESSAGES } from '@core/constants/error-messages'; +import { PREPRINT_PENDING_MODERATION_DETAIL } from '@core/constants/http-error-details.const'; import { SENTRY_TOKEN } from '@core/provider/sentry.provider'; import { AuthService } from '@core/services/auth.service'; import { MaintenanceModeService } from '@core/services/maintenance-mode.service'; @@ -28,7 +30,7 @@ import { ToastServiceMock, ToastServiceMockType } from '@testing/providers/toast import { ViewOnlyLinkHelperMock, ViewOnlyLinkHelperMockType } from '@testing/providers/view-only-link-helper.mock'; import { errorInterceptor } from './error.interceptor'; -import { BYPASS_ERROR_INTERCEPTOR } from './error-interceptor.tokens'; +import { bypassErrorInterceptorContext } from './error-interceptor.tokens'; describe('errorInterceptor', () => { let router: RouterMockType; @@ -72,7 +74,7 @@ describe('errorInterceptor', () => { } function createRequest(url = '/api/v2/nodes/abc', bypass = false, noAuthRedirect = false) { - const context = bypass ? new HttpContext().set(BYPASS_ERROR_INTERCEPTOR, true) : new HttpContext(); + const context = bypass ? bypassErrorInterceptorContext() : new HttpContext(); const headers = noAuthRedirect ? new HttpHeaders({ 'X-No-Auth-Redirect': 'true' }) : new HttpHeaders(); return new HttpRequest('GET', url, { context, headers } as unknown); } @@ -113,7 +115,7 @@ describe('errorInterceptor', () => { expect(toastServiceMock.showError).toHaveBeenCalledWith('server failed'); }); - it('should hide loader without toast on server platform', async () => { + it('should rethrow without handling on server platform', async () => { setup('server'); const request = createRequest('/api/v2/nodes/abc'); const error = new HttpErrorResponse({ status: 500, error: { message: 'server failed' }, url: request.url }); @@ -121,8 +123,9 @@ describe('errorInterceptor', () => { const caught = await runInterceptor(request, error); expect(caught?.status).toBe(500); - expect(loaderServiceMock.hide).toHaveBeenCalled(); + expect(loaderServiceMock.hide).not.toHaveBeenCalled(); expect(toastServiceMock.showError).not.toHaveBeenCalled(); + expect(sentryMock.captureException).not.toHaveBeenCalled(); }); it('should rethrow 409 without toast or loader handling', async () => { @@ -147,7 +150,7 @@ describe('errorInterceptor', () => { expect(caught?.status).toBe(401); expect(authServiceMock.navigateToSignIn).toHaveBeenCalled(); expect(authServiceMock.logout).not.toHaveBeenCalled(); - expect(loaderServiceMock.hide).not.toHaveBeenCalled(); + expect(loaderServiceMock.hide).toHaveBeenCalled(); expect(toastServiceMock.showError).not.toHaveBeenCalled(); }); @@ -161,7 +164,7 @@ describe('errorInterceptor', () => { expect(caught?.status).toBe(401); expect(authServiceMock.logout).toHaveBeenCalledWith(window.location.href); expect(authServiceMock.navigateToSignIn).not.toHaveBeenCalled(); - expect(loaderServiceMock.hide).not.toHaveBeenCalled(); + expect(loaderServiceMock.hide).toHaveBeenCalled(); expect(toastServiceMock.showError).not.toHaveBeenCalled(); }); @@ -174,9 +177,10 @@ describe('errorInterceptor', () => { expect(caught?.status).toBe(401); expect(authServiceMock.logout).not.toHaveBeenCalled(); + expect(loaderServiceMock.hide).toHaveBeenCalled(); }); - it('should navigate to request-access for 403 node URL', async () => { + it('should navigate to request-access for 403 node URL without toast', async () => { setup('browser', false, '/projects/abc'); const request = createRequest('/api/v2/nodes/abc'); const error = new HttpErrorResponse({ status: 403, error: {}, url: '/api/v2/nodes/abc' }); @@ -184,9 +188,35 @@ describe('errorInterceptor', () => { const caught = await runInterceptor(request, error); expect(caught?.status).toBe(403); - expect(router.navigate).toHaveBeenCalledWith(['/request-access/abc']); + expect(router.navigate).toHaveBeenCalledWith(['/request-access', 'abc']); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + expect(toastServiceMock.showError).not.toHaveBeenCalled(); + }); + + it('should navigate to forbidden for 403 without a node URL', async () => { + setup(); + const request = createRequest('/api/v2/users/me/'); + const error = new HttpErrorResponse({ status: 403, error: {}, url: '/api/v2/users/me/' }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(403); + expect(router.navigate).toHaveBeenCalledWith(['/forbidden']); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + expect(toastServiceMock.showError).not.toHaveBeenCalled(); + }); + + it('should not redirect on 403 when current route is a draft registry', async () => { + setup('browser', false, '/registries/drafts/abc'); + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ status: 403, error: {}, url: '/api/v2/nodes/abc' }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(403); + expect(router.navigate).not.toHaveBeenCalled(); expect(loaderServiceMock.hide).toHaveBeenCalled(); - expect(toastServiceMock.showError).toHaveBeenCalled(); + expect(toastServiceMock.showError).not.toHaveBeenCalled(); }); it('should not redirect on 403 when no-auth-redirect header is present', async () => { @@ -202,6 +232,36 @@ describe('errorInterceptor', () => { expect(toastServiceMock.showError).not.toHaveBeenCalled(); }); + it('should not redirect on 403 for node request-access URL', async () => { + setup(); + const request = createRequest('/api/v2/nodes/abc/requests/'); + const error = new HttpErrorResponse({ status: 403, error: {}, url: '/api/v2/nodes/abc/requests/' }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(403); + expect(router.navigate).not.toHaveBeenCalled(); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + expect(toastServiceMock.showError).not.toHaveBeenCalled(); + }); + + it('should not redirect on 403 when preprint is pending moderation', async () => { + setup(); + const request = createRequest('/api/v2/preprints/abc'); + const error = new HttpErrorResponse({ + status: 403, + error: { errors: [{ detail: PREPRINT_PENDING_MODERATION_DETAIL }] }, + url: request.url, + }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(403); + expect(router.navigate).not.toHaveBeenCalled(); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + expect(toastServiceMock.showError).not.toHaveBeenCalled(); + }); + it('should activate maintenance mode on 503 maintenance response', async () => { setup('browser', false); const request = createRequest('/api/v2/'); @@ -218,4 +278,58 @@ describe('errorInterceptor', () => { expect(loaderServiceMock.hide).toHaveBeenCalled(); expect(toastServiceMock.showError).not.toHaveBeenCalled(); }); + + it('should navigate to not-found for 404 without toast', async () => { + setup(); + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ status: 404, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(404); + expect(router.navigate).toHaveBeenCalledWith(['/not-found']); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + expect(toastServiceMock.showError).not.toHaveBeenCalled(); + }); + + it('should navigate to content-deleted for 410 without toast', async () => { + setup(); + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ status: 410, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(410); + expect(router.navigate).toHaveBeenCalledWith(['/content-deleted']); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + expect(toastServiceMock.showError).not.toHaveBeenCalled(); + }); + + it('should toast API detail for default status codes', async () => { + setup(); + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ + status: 400, + error: { errors: [{ detail: 'Invalid title' }] }, + url: request.url, + }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(400); + expect(toastServiceMock.showError).toHaveBeenCalledWith('Invalid title'); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + }); + + it('should toast mapped i18n key when no API detail is present', async () => { + setup(); + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ status: 400, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(400); + expect(toastServiceMock.showError).toHaveBeenCalledWith(ERROR_MESSAGES[400]); + expect(loaderServiceMock.hide).toHaveBeenCalled(); + }); }); diff --git a/src/app/core/services/maintenance-mode.service.spec.ts b/src/app/core/services/maintenance-mode.service.spec.ts index 7c82286ec..3209df559 100644 --- a/src/app/core/services/maintenance-mode.service.spec.ts +++ b/src/app/core/services/maintenance-mode.service.spec.ts @@ -1,3 +1,4 @@ +import { HttpErrorResponse } from '@angular/common/http'; import { HttpTestingController, TestRequest } from '@angular/common/http/testing'; import { TestBed } from '@angular/core/testing'; @@ -85,6 +86,36 @@ describe('MaintenanceModeService', () => { expect(service.isActive()).toBe(false); }); + it('should detect maintenance from a 503 maintenance error response', () => { + const error = new HttpErrorResponse({ + status: 503, + error: maintenanceOn, + statusText: 'Service Unavailable', + }); + + expect(service.isMaintenanceError(error)).toBe(true); + }); + + it('should not detect maintenance from a 503 without maintenance_mode', () => { + const error = new HttpErrorResponse({ + status: 503, + error: maintenanceOff, + statusText: 'Service Unavailable', + }); + + expect(service.isMaintenanceError(error)).toBe(false); + }); + + it('should not detect maintenance from a non-503 error', () => { + const error = new HttpErrorResponse({ + status: 500, + error: maintenanceOn, + statusText: 'Server Error', + }); + + expect(service.isMaintenanceError(error)).toBe(false); + }); + it('should deactivate when a poll receives a successful non-maintenance response', () => { vi.useFakeTimers(); service.activate(); diff --git a/src/app/features/preprints/pages/preprint-details/preprint-details.component.spec.ts b/src/app/features/preprints/pages/preprint-details/preprint-details.component.spec.ts index d32a9a371..e09726031 100644 --- a/src/app/features/preprints/pages/preprint-details/preprint-details.component.spec.ts +++ b/src/app/features/preprints/pages/preprint-details/preprint-details.component.spec.ts @@ -11,6 +11,7 @@ import { PLATFORM_ID } from '@angular/core'; import { ComponentFixture, TestBed } from '@angular/core/testing'; import { ActivatedRoute, Router } from '@angular/router'; +import { PREPRINT_PENDING_MODERATION_DETAIL } from '@core/constants/http-error-details.const'; import { HelpScoutService } from '@core/services/help-scout.service'; import { PrerenderReadyService } from '@core/services/prerender-ready.service'; import { ClearCurrentProvider } from '@core/store/provider'; @@ -308,7 +309,7 @@ describe('PreprintDetailsComponent', () => { const errorResponse = new HttpErrorResponse({ status: 403, error: { - errors: [{ detail: 'This preprint is pending moderation and is not yet publicly available.' }], + errors: [{ detail: PREPRINT_PENDING_MODERATION_DETAIL }], }, }); diff --git a/src/app/features/project/settings/services/settings.service.spec.ts b/src/app/features/project/settings/services/settings.service.spec.ts index 6ce7330a4..6abc8230a 100644 --- a/src/app/features/project/settings/services/settings.service.spec.ts +++ b/src/app/features/project/settings/services/settings.service.spec.ts @@ -4,6 +4,7 @@ import { firstValueFrom, of } from 'rxjs'; import { TestBed } from '@angular/core/testing'; +import { JSON_API_BULK_HEADERS } from '@osf/shared/constants/json-api-headers.const'; import { CurrentResourceType } from '@osf/shared/enums/resource-type.enum'; import { SubscriptionFrequency } from '@osf/shared/enums/subscriptions/subscription-frequency.enum'; import { NotificationSubscriptionMapper } from '@osf/shared/mappers/notification-subscription.mapper'; @@ -218,7 +219,7 @@ describe('SettingsService', () => { { id: 'node-2', type: CurrentResourceType.Projects }, ], }, - { 'Content-Type': 'application/vnd.api+json; ext=bulk' } + JSON_API_BULK_HEADERS ); }); diff --git a/src/app/features/registry/pages/registry-resources/registry-resources.component.spec.ts b/src/app/features/registry/pages/registry-resources/registry-resources.component.spec.ts index a251a0542..42a4706cb 100644 --- a/src/app/features/registry/pages/registry-resources/registry-resources.component.spec.ts +++ b/src/app/features/registry/pages/registry-resources/registry-resources.component.spec.ts @@ -2,10 +2,7 @@ import { Store } from '@ngxs/store'; import { MockComponents, MockProvider } from 'ng-mocks'; -import { Button } from 'primeng/button'; -import { DynamicDialogRef } from 'primeng/dynamicdialog'; - -import { Subject, throwError } from 'rxjs'; +import { Subject } from 'rxjs'; import { Mock } from 'vitest'; @@ -66,8 +63,6 @@ function setup(overrides: BaseSetupOverrides = {}) { { selector: RegistryResourcesSelectors.getResources, value: [] }, { selector: RegistryResourcesSelectors.getResourcesTotalCount, value: 0 }, { selector: RegistryResourcesSelectors.isResourcesLoading, value: false }, - { selector: RegistryResourcesSelectors.getCurrentResource, value: null }, - { selector: RegistrySelectors.getRegistry, value: null }, { selector: RegistrySelectors.getIdentifiers, value: [MOCK_PROJECT_IDENTIFIERS] }, { selector: RegistrySelectors.hasWriteAccess, value: true }, ]; @@ -77,7 +72,7 @@ function setup(overrides: BaseSetupOverrides = {}) { TestBed.configureTestingModule({ imports: [ RegistryResourcesComponent, - ...MockComponents(Button, LoadingSpinnerComponent, SubHeaderComponent, IconComponent, CustomPaginatorComponent), + ...MockComponents(LoadingSpinnerComponent, SubHeaderComponent, IconComponent, CustomPaginatorComponent), ], providers: [ provideOSFCore(), @@ -170,15 +165,6 @@ describe('RegistryResourcesComponent', () => { expect(component.isAddingResource()).toBe(false); }); - it('should show an error toast when addResource fails', () => { - const { component, store, mockToastService } = setup(); - - vi.spyOn(store, 'dispatch').mockReturnValue(throwError(() => new Error('fail'))); - component.addResource(); - - expect(mockToastService.showError).toHaveBeenCalledWith('resources.toastMessages.addResourceError'); - }); - it('should update a resource and show a success toast', () => { const { component, dialogClose$, mockDialogService, mockToastService } = setup(); @@ -196,20 +182,6 @@ describe('RegistryResourcesComponent', () => { expect(mockToastService.showSuccess).toHaveBeenCalledWith('resources.toastMessages.updatedResourceSuccess'); }); - it('should show an error toast when updateResource fails', () => { - const errorSubject = new Subject(); - const { component, mockDialogService, mockToastService } = setup(); - - mockDialogService.open.mockReturnValue({ - onClose: errorSubject.pipe(), - close: vi.fn(), - } as unknown as DynamicDialogRef); - component.updateResource(MOCK_RESOURCE); - errorSubject.error(new Error('fail')); - - expect(mockToastService.showError).toHaveBeenCalledWith('resources.toastMessages.updateResourceError'); - }); - it('should delete a resource, reset pagination, and show a success toast', () => { const { component, mockConfirmationService, mockToastService, store } = setup(); diff --git a/src/app/features/registry/pages/registry-resources/registry-resources.component.ts b/src/app/features/registry/pages/registry-resources/registry-resources.component.ts index 623572577..ffa227a29 100644 --- a/src/app/features/registry/pages/registry-resources/registry-resources.component.ts +++ b/src/app/features/registry/pages/registry-resources/registry-resources.component.ts @@ -68,8 +68,6 @@ export class RegistryResourcesComponent { readonly resources = select(RegistryResourcesSelectors.getResources); readonly resourcesTotalCount = select(RegistryResourcesSelectors.getResourcesTotalCount); readonly isResourcesLoading = select(RegistryResourcesSelectors.isResourcesLoading); - readonly currentResource = select(RegistryResourcesSelectors.getCurrentResource); - readonly registry = select(RegistrySelectors.getRegistry); readonly identifiers = select(RegistrySelectors.getIdentifiers); readonly canEdit = select(RegistrySelectors.hasWriteAccess); diff --git a/src/app/features/settings/account-settings/services/account-settings.service.spec.ts b/src/app/features/settings/account-settings/services/account-settings.service.spec.ts index 7b732a216..4c711e41e 100644 --- a/src/app/features/settings/account-settings/services/account-settings.service.spec.ts +++ b/src/app/features/settings/account-settings/services/account-settings.service.spec.ts @@ -1,37 +1,193 @@ +import { firstValueFrom } from 'rxjs'; + +import { HttpTestingController } from '@angular/common/http/testing'; import { TestBed } from '@angular/core/testing'; import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; -import { JsonApiService } from '@osf/shared/services/json-api.service'; +import { UserMapper } from '@osf/shared/mappers/user'; + +import { getUserDataJsonApi } from '@testing/data/user/user.data'; +import { provideOSFCore, provideOSFHttp } from '@testing/osf.testing.provider'; -import { MOCK_API_URL, provideMockEnvironment } from '@testing/providers/environment.provider.mock'; -import { JsonApiServiceMock, JsonApiServiceMockType } from '@testing/providers/json-api.service.mock'; +import { AccountSettingsMapper, MapExternalIdentities } from '../mappers'; +import { AccountSettingsDataJsonApi, AccountSettingsResponseJsonApi, ListIdentitiesResponseJsonApi } from '../models'; import { AccountSettingsService } from './account-settings.service'; describe('AccountSettingsService', () => { let service: AccountSettingsService; - let jsonApiService: JsonApiServiceMockType; + let httpMock: HttpTestingController; - beforeEach(() => { - jsonApiService = JsonApiServiceMock.simple(); + const apiUrl = 'http://localhost:8000/v2'; + const userId = 'user-1'; + + const accountSettingsData: AccountSettingsDataJsonApi = { + id: userId, + type: 'user-settings', + attributes: { + two_factor_enabled: true, + two_factor_confirmed: false, + subscribe_osf_general_email: true, + subscribe_osf_help_email: false, + deactivation_requested: false, + contacted_deactivation: false, + secret: 'secret-value', + }, + }; + + const identitiesResponse: ListIdentitiesResponseJsonApi = { + data: [ + { + id: 'orcid', + type: 'external-identities', + attributes: { + external_id: '0000-0001-2345-6789', + status: 'verified', + }, + }, + ], + meta: { + total: 1, + per_page: 10, + version: '2.0', + }, + }; + beforeEach(() => { TestBed.configureTestingModule({ - providers: [ - AccountSettingsService, - provideMockEnvironment(), - { provide: JsonApiService, useValue: jsonApiService }, - ], + providers: [provideOSFCore(), provideOSFHttp(), AccountSettingsService], }); service = TestBed.inject(AccountSettingsService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + }); + + it('should expose the api url from the environment', () => { + expect(service.apiUrl).toBe(apiUrl); }); - it('should delete an external identity with the error interceptor bypassed', () => { - service.deleteExternalIdentity('orcid').subscribe(); + it('should get account settings', async () => { + const response: AccountSettingsResponseJsonApi = { data: accountSettingsData }; + const resultPromise = firstValueFrom(service.getSettings()); + + const req = httpMock.expectOne(`${apiUrl}/users/me/settings/`); + expect(req.request.method).toBe('GET'); + req.flush(response); + + await expect(resultPromise).resolves.toEqual(AccountSettingsMapper.getAccountSettings(accountSettingsData)); + }); + + it('should update account settings', async () => { + const settings = { subscribe_osf_general_email: 'true' }; + const resultPromise = firstValueFrom(service.updateSettings(userId, settings)); + + const req = httpMock.expectOne(`${apiUrl}/users/${userId}/settings/`); + expect(req.request.method).toBe('PATCH'); + expect(req.request.body).toEqual({ + data: { + id: userId, + attributes: settings, + type: 'user_settings', + }, + }); + req.flush({ data: accountSettingsData }); + + await expect(resultPromise).resolves.toEqual(AccountSettingsMapper.getAccountSettings(accountSettingsData)); + }); + + it('should update location', async () => { + const userData = getUserDataJsonApi(); + const resultPromise = firstValueFrom(service.updateLocation(userId, 'us')); + + const req = httpMock.expectOne(`${apiUrl}/users/${userId}/`); + expect(req.request.method).toBe('PATCH'); + expect(req.request.body).toEqual({ + data: { + id: userId, + attributes: {}, + relationships: { + default_region: { + data: { + type: 'regions', + id: 'us', + }, + }, + }, + type: 'users', + }, + }); + req.flush({ data: userData }); + + await expect(resultPromise).resolves.toEqual(UserMapper.fromUserGetResponse(userData)); + }); + + it('should update indexing', async () => { + const userData = getUserDataJsonApi(); + const resultPromise = firstValueFrom(service.updateIndexing(userId, false)); + + const req = httpMock.expectOne(`${apiUrl}/users/${userId}/`); + expect(req.request.method).toBe('PATCH'); + expect(req.request.body).toEqual({ + data: { + id: userId, + attributes: { + allow_indexing: false, + }, + relationships: {}, + type: 'users', + }, + }); + req.flush({ data: userData }); + + await expect(resultPromise).resolves.toEqual(UserMapper.fromUserGetResponse(userData)); + }); + + it('should update password', async () => { + const resultPromise = firstValueFrom(service.updatePassword('old-pass', 'new-pass')); + + const req = httpMock.expectOne(`${apiUrl}/users/me/settings/password/`); + expect(req.request.method).toBe('POST'); + expect(req.request.body).toEqual({ + data: { + type: 'user_passwords', + attributes: { + existing_password: 'old-pass', + new_password: 'new-pass', + }, + }, + }); + req.flush(null); + + await expect(resultPromise).resolves.toBeNull(); + }); + + it('should get external identities', async () => { + const resultPromise = firstValueFrom(service.getExternalIdentities()); + + const req = httpMock.expectOne( + (request) => + request.url === `${apiUrl}/users/me/settings/identities/` && + request.params.get('page') === '1' && + request.params.get('page[size]') === '10' + ); + expect(req.request.method).toBe('GET'); + req.flush(identitiesResponse); + + await expect(resultPromise).resolves.toEqual(MapExternalIdentities(identitiesResponse.data)); + }); + + it('should delete an external identity with the error interceptor bypassed', async () => { + const resultPromise = firstValueFrom(service.deleteExternalIdentity('orcid')); + + const req = httpMock.expectOne(`${apiUrl}/users/me/settings/identities/orcid/`); + expect(req.request.method).toBe('DELETE'); + expect(req.request.context.get(BYPASS_ERROR_INTERCEPTOR)).toBe(true); + req.flush(null); - expect(jsonApiService.delete).toHaveBeenCalledTimes(1); - const [url, , , context] = jsonApiService.delete.mock.calls[0]; - expect(url).toBe(`${MOCK_API_URL}/users/me/settings/identities/orcid/`); - expect(context?.get(BYPASS_ERROR_INTERCEPTOR)).toBe(true); + await expect(resultPromise).resolves.toBeNull(); }); }); diff --git a/src/app/shared/services/addons/addons.service.spec.ts b/src/app/shared/services/addons/addons.service.spec.ts index 729e5ccfe..b789650fe 100644 --- a/src/app/shared/services/addons/addons.service.spec.ts +++ b/src/app/shared/services/addons/addons.service.spec.ts @@ -1,9 +1,17 @@ +import { firstValueFrom } from 'rxjs'; + import { HttpTestingController } from '@angular/common/http/testing'; -import { inject, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; + +import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; +import { UserSelectors } from '@core/store/user'; +import { AuthorizedAddonRequestJsonApi } from '@osf/shared/models/addons/authorized-addon-json-api.model'; +import { UserModel } from '@osf/shared/models/user/user.model'; import { getAddonsAuthorizedStorageData } from '@testing/data/addons/addons.authorized-storage.data'; import { getConfiguredAddonsData } from '@testing/data/addons/addons.configured.data'; import { getAddonsExternalStorageData } from '@testing/data/addons/addons.external-storage.data'; +import { MOCK_USER } from '@testing/mocks/data.mock'; import { provideOSFCore, provideOSFHttp } from '@testing/osf.testing.provider'; import { provideMockStore } from '@testing/providers/store-provider.mock'; @@ -11,25 +19,37 @@ import { AddonsService } from './addons.service'; describe('Service: Addons', () => { let service: AddonsService; + let httpMock: HttpTestingController; - beforeEach(() => { + function setup(currentUser: UserModel | null = null) { TestBed.configureTestingModule({ - providers: [provideOSFCore(), provideOSFHttp(), provideMockStore(), AddonsService], + providers: [ + provideOSFCore(), + provideOSFHttp(), + provideMockStore({ + signals: [{ selector: UserSelectors.getCurrentUser, value: currentUser }], + }), + AddonsService, + ], }); service = TestBed.inject(AddonsService); + httpMock = TestBed.inject(HttpTestingController); + } + + afterEach(() => { + httpMock.verify(); }); - it('should test getAddons', inject([HttpTestingController], (httpMock: HttpTestingController) => { - let results: any[] = []; - service.getAddons('storage').subscribe((result) => { - results = result; - }); + it('should test getAddons', async () => { + setup(); + const resultsPromise = firstValueFrom(service.getAddons('storage')); const request = httpMock.expectOne('http://addons.localhost:8000/external-storage-services'); expect(request.request.method).toBe('GET'); request.flush(getAddonsExternalStorageData()); + const results = await resultsPromise; expect(results[0]).toEqual( Object({ authUrl: 'https://figshare.com/account/applications/authorize', @@ -45,22 +65,19 @@ describe('Service: Addons', () => { configurableApiRoot: false, }) ); + }); - expect(httpMock.verify).toBeTruthy(); - })); - - it('should test getConfigureAddons', inject([HttpTestingController], (httpMock: HttpTestingController) => { - let results: any[] = []; - service.getConfiguredAddons('storage', 'reference-id').subscribe((result) => { - results = result; - }); + it('should test getConfigureAddons', async () => { + setup(); + const resultsPromise = firstValueFrom(service.getConfiguredAddons('storage', 'reference-id')); const request = httpMock.expectOne( 'http://addons.localhost:8000/resource-references/reference-id/configured_storage_addons/?include=external-storage-service&fields%5Bexternal-storage-services%5D=external_service_name,credentials_format,icon_url' ); expect(request.request.method).toBe('GET'); request.flush(getConfiguredAddonsData()); + const results = await resultsPromise; expect(results[0]).toEqual( Object({ baseAccountId: '62ed6dd7-f7b7-4003-b7b4-855789c1f991', @@ -80,22 +97,19 @@ describe('Service: Addons', () => { iconUrl: 'https://osf.io/assets/images/logo.svg', }) ); + }); - expect(httpMock.verify).toBeTruthy(); - })); - - it('should test getAuthorizedAddons', inject([HttpTestingController], (httpMock: HttpTestingController) => { - let results: any[] = []; - service.getAuthorizedAddons('storage', 'reference-id').subscribe((result) => { - results = result; - }); + it('should test getAuthorizedAddons', async () => { + setup(); + const resultsPromise = firstValueFrom(service.getAuthorizedAddons('storage', 'reference-id')); const request = httpMock.expectOne( 'http://addons.localhost:8000/user-references/reference-id/authorized_storage_accounts/?include=external-storage-service&fields%5Bexternal-storage-services%5D=external_service_name,credentials_format,icon_url' ); expect(request.request.method).toBe('GET'); request.flush(getAddonsAuthorizedStorageData()); + const results = await resultsPromise; expect(results[0]).toEqual( Object({ accountOwnerId: '0b441148-83e5-4f7f-b302-b07b528b160b', @@ -117,56 +131,114 @@ describe('Service: Addons', () => { type: 'authorized-storage-accounts', }) ); + }); + + it('should error when getAddonsUserReference has no current user', async () => { + setup(null); + + await expect(firstValueFrom(service.getAddonsUserReference())).rejects.toThrow('Current user not found'); + }); + + it('should fetch getAddonsUserReference for the current user', async () => { + setup(MOCK_USER); + + const resultsPromise = firstValueFrom(service.getAddonsUserReference()); + const request = httpMock.expectOne( + (req) => + req.url === 'http://addons.localhost:8000/user-references/' && + req.params.get('filter[user_uri]') === `http://localhost:4200/${MOCK_USER.id}` + ); + expect(request.request.method).toBe('GET'); + request.flush({ + data: [ + { + id: 'user-ref-1', + type: 'user-references', + attributes: { user_uri: `http://localhost:4200/${MOCK_USER.id}` }, + }, + ], + }); - expect(httpMock.verify).toBeTruthy(); - })); - - it('should test getAuthorizedStorageOauthToken', inject( - [HttpTestingController], - (httpMock: HttpTestingController) => { - let results; - service.getAuthorizedStorageOauthToken('account-id', 'storage').subscribe((result) => { - results = result; - }); - - const request = httpMock.expectOne('http://addons.localhost:8000/authorized-storage-accounts/account-id'); - expect(request.request.method).toBe('PATCH'); - expect(request.request.body).toEqual( - Object({ - data: Object({ - attributes: Object({ - serialize_oauth_token: 'true', - }), - id: 'account-id', - type: 'authorized-storage-accounts', + await expect(resultsPromise).resolves.toEqual([ + { + id: 'user-ref-1', + userUri: `http://localhost:4200/${MOCK_USER.id}`, + }, + ]); + }); + + it('should test getAuthorizedStorageOauthToken', async () => { + setup(); + + const resultsPromise = firstValueFrom(service.getAuthorizedStorageOauthToken('account-id', 'storage')); + const request = httpMock.expectOne('http://addons.localhost:8000/authorized-storage-accounts/account-id'); + expect(request.request.method).toBe('PATCH'); + expect(request.request.context.get(BYPASS_ERROR_INTERCEPTOR)).toBe(true); + expect(request.request.body).toEqual( + Object({ + data: Object({ + attributes: Object({ + serialize_oauth_token: 'true', }), - }) - ); - request.flush(getAddonsAuthorizedStorageData(0)); - - expect(results).toEqual( - Object({ - accountOwnerId: '0b441148-83e5-4f7f-b302-b07b528b160b', - apiBaseUrl: 'https://www.googleapis.com', - authUrl: null, - authorizedCapabilities: ['ACCESS', 'UPDATE'], - authorizedOperationNames: ['list_root_items', 'get_item_info', 'list_child_items'], - credentialsAvailable: true, - credentialsFormat: '', - iconUrl: '', - defaultRootFolder: '', - displayName: 'Google Drive', - externalServiceName: '', - oauthToken: 'ya29.A0AS3H6NzDCKgrUx', - externalStorageServiceId: '8aeb85e9-3a73-426f-a89b-5624b4b9d418', - id: '331b7333-a13a-4d3b-add0-5af0fd1d4ac4', - providerName: '', - supportedFeatures: [], + id: 'account-id', type: 'authorized-storage-accounts', - }) - ); + }), + }) + ); + request.flush(getAddonsAuthorizedStorageData(0)); - expect(httpMock.verify).toBeTruthy(); - } - )); + await expect(resultsPromise).resolves.toEqual( + Object({ + accountOwnerId: '0b441148-83e5-4f7f-b302-b07b528b160b', + apiBaseUrl: 'https://www.googleapis.com', + authUrl: null, + authorizedCapabilities: ['ACCESS', 'UPDATE'], + authorizedOperationNames: ['list_root_items', 'get_item_info', 'list_child_items'], + credentialsAvailable: true, + credentialsFormat: '', + iconUrl: '', + defaultRootFolder: '', + displayName: 'Google Drive', + externalServiceName: '', + oauthToken: 'ya29.A0AS3H6NzDCKgrUx', + externalStorageServiceId: '8aeb85e9-3a73-426f-a89b-5624b4b9d418', + id: '331b7333-a13a-4d3b-add0-5af0fd1d4ac4', + providerName: '', + supportedFeatures: [], + type: 'authorized-storage-accounts', + }) + ); + }); + + it('should update authorized addon with the full response document', async () => { + setup(); + + const payload = { + data: { + type: 'authorized-storage-accounts', + attributes: { + api_base_url: 'https://www.googleapis.com', + auth_url: null, + authorized_capabilities: ['ACCESS'], + credentials: {}, + credentials_available: true, + display_name: 'Google Drive', + initiate_oauth: false, + }, + relationships: {}, + }, + } as AuthorizedAddonRequestJsonApi; + + const resultsPromise = firstValueFrom(service.updateAuthorizedAddon(payload, 'storage', 'account-id')); + const request = httpMock.expectOne( + 'http://addons.localhost:8000/authorized-storage-accounts/account-id/?include=external-storage-service' + ); + expect(request.request.method).toBe('PATCH'); + expect(request.request.body).toEqual(payload); + request.flush(getAddonsAuthorizedStorageData(0)); + + const result = await resultsPromise; + expect(result.id).toBe('331b7333-a13a-4d3b-add0-5af0fd1d4ac4'); + expect(result.displayName).toBe('Google Drive'); + }); }); diff --git a/src/testing/providers/maintenance-mode.service.mock.ts b/src/testing/providers/maintenance-mode.service.mock.ts index fcea0a289..95e03ad5c 100644 --- a/src/testing/providers/maintenance-mode.service.mock.ts +++ b/src/testing/providers/maintenance-mode.service.mock.ts @@ -1,5 +1,6 @@ import { Mock } from 'vitest'; +import { HttpErrorResponse } from '@angular/common/http'; import { Signal, signal } from '@angular/core'; import { MaintenanceModeService } from '@core/services/maintenance-mode.service'; @@ -8,6 +9,7 @@ export type MaintenanceModeServiceMockType = Partial & { activate: Mock<() => void>; deactivate: Mock<() => void>; checkOnce: Mock<() => void>; + isMaintenanceError: Mock<(error: HttpErrorResponse) => boolean>; isActive: Signal; }; @@ -17,6 +19,9 @@ export const MaintenanceModeServiceMock = { activate: vi.fn(), deactivate: vi.fn(), checkOnce: vi.fn(), + isMaintenanceError: vi.fn( + (error: HttpErrorResponse) => error.status === 503 && error.error?.meta?.maintenance_mode === true + ), isActive: signal(isActive).asReadonly(), } as MaintenanceModeServiceMockType; }, From 79df9d46baa27ab83c65eaccab9687d23483e2b3 Mon Sep 17 00:00:00 2001 From: nsemets Date: Tue, 27 Oct 2026 14:05:58 +0200 Subject: [PATCH 3/5] docs(error-handling): added docs --- docs/error-handling.md | 162 +++++++++++++++++++++++++++++++++++++++++ docs/ngxs.md | 3 +- 2 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 docs/error-handling.md diff --git a/docs/error-handling.md b/docs/error-handling.md new file mode 100644 index 000000000..e96b16939 --- /dev/null +++ b/docs/error-handling.md @@ -0,0 +1,162 @@ +# Error handling + +Two complementary layers handle failures from API-backed work: + +| Layer | Responsibility | Entry point | +| ----- | -------------- | ----------- | +| **HTTP interceptor** | Global UX: toast, navigation, auth, maintenance, loader | `error.interceptor.ts` | +| **NGXS state** | Section bookkeeping: clear loading flags, store `error`, Sentry tag | `handleSectionError` | + +Both **rethrow**. For a store action that calls HTTP: interceptor runs first → rethrow → state’s `catchError` → `handleSectionError`. + +## Index + +- [HTTP interceptor](#http-interceptor) + - [Flow](#flow) + - [Status handling](#status-handling) + - [Bypass](#bypass) + - [Silent 403](#silent-403) + - [Messages](#messages) + - [Caller guidelines](#caller-guidelines) +- [NGXS state errors](#ngxs-state-errors) +- [Related files](#related-files) + +## HTTP interceptor + +Central HTTP UX lives in `src/app/core/interceptors/error.interceptor.ts`. Prefer the interceptor for shared UX; handle locally only when the UI needs a special message or flow. + +### Flow + +Order before the status `switch` matters: + +1. **SSR (`!isBrowser`)** — no toast, navigation, loader, auth, or Sentry; rethrow only. +2. **Bypass** — Sentry capture + rethrow; skip all UX. +3. **Maintenance** — `503` with `meta.maintenance_mode: true` → hide loader, activate maintenance mode, rethrow. +4. **Status `switch`** — `401` / `403` / `404` / `409` / `410` / default. + +After the `switch` (except early `409` return), the interceptor hides the loader and rethrows. `409` does **not** hide the loader; the caller owns that. + +```mermaid +flowchart TD + err[HttpErrorResponse] --> ssr{Browser?} + ssr -->|no| rethrow[Rethrow] + ssr -->|yes| bypass{BYPASS?} + bypass -->|yes| sentry[Sentry + rethrow] + bypass -->|no| maint{Maintenance 503?} + maint -->|yes| activate[Hide loader + activate maintenance + rethrow] + maint -->|no| status{Status} + status -->|409| conflict[Rethrow only] + status -->|other| rethrow2[Hide loader + rethrow] + rethrow2 --> state[NGXS catchError / handleSectionError] + conflict --> state + rethrow --> state + sentry --> state + activate --> state +``` + +### Status handling + +| Status | Interceptor behavior | +| ------ | -------------------- | +| **401** | Skip auth redirect on view-only (`?view_only=`). Else logout if authenticated, or navigate to sign-in. Hide loader. | +| **403** | Navigate unless silent / pending-moderation (see below). Node URL → `/request-access/:id` (skipped on draft registry routes). Other → `/forbidden`. Hide loader. No toast. | +| **404** | Navigate to `/not-found`. Hide loader. No toast. | +| **409** | Rethrow only (no toast, no loader hide). Caller handles conflict UX. | +| **410** | Navigate to `/content-deleted`. Hide loader. No toast. | +| **Other** | Toast via `getErrorMessage`, hide loader. | + +`/not-found` and `/content-deleted` share `PageNotFoundComponent` with different route data (`PAGE_NOT_FOUND_ROUTE_DATA` / `CONTENT_DELETED_ROUTE_DATA`). + +### Bypass + +Use when the **caller** owns all error UX (or the failure must not navigate/toast). + +```ts +import { bypassErrorInterceptorContext } from '@core/interceptors/error-interceptor.tokens'; + +this.http.get(url, { context: bypassErrorInterceptorContext() }); +``` + +`bypassErrorInterceptorContext()` returns a **new** `HttpContext` each call (do not reuse one mutable instance). + +Bypass still reports to Sentry and rethrows. It skips toast, navigation, loader, auth, and maintenance activation. + +Current examples: maintenance status polling, parent project fetch, Datacite tracker, addon OAuth token refresh, external identity delete. + +### Silent 403 + +These skip `/forbidden` / request-access navigation (loader still hides; error still rethrows): + +| Case | How | +| ---- | --- | +| Metadata record fetch | `X-No-Auth-Redirect` on `MetadataRecordsService.getMetadataRecord` (optional schema.org; prefer migrating to `HttpContext` later) | +| Node access-request API | URL matches `/v2/nodes/:id/requests` | +| Preprint pending moderation | Detail equals `PREPRINT_PENDING_MODERATION_DETAIL` — component routes to pending-moderation | + +### Messages + +`getErrorMessage` resolution: + +1. Client `ErrorEvent.message` +2. JSON:API `error.errors[0].detail` +3. For 5xx: `error.message` or `common.errorMessages.serverError` +4. Else `ERROR_MESSAGES[status]` or `common.errorMessages.unknown` + +`ERROR_MESSAGES` maps statuses to i18n keys under `common.errorMessages.*`. Toasts run through the translate pipe. + +### Caller guidelines + +- **Default** — do not toast in the subscriber; the interceptor already does for unhandled statuses. +- **Custom toast / flow** — either `bypassErrorInterceptorContext()` **or** remove the local toast so users do not see two messages. +- **409** — handle in the caller (interceptor is silent). +- **Expected 404/410 on secondary requests** — use bypass; otherwise the interceptor navigates away from the current page. +- **Pending-moderation preprint** — leave interceptor special-case + component navigation; do not send that request with full bypass unless you also handle 404/410/5xx locally. +- **Loader** — interceptor hides for most statuses; for `409` (and bypass) hide in the caller (`finalize` is a good pattern). + +## NGXS state errors + +Store actions that hit HTTP should end failed sections with `handleSectionError` from `src/app/shared/helpers/state-error.handler.ts`. + +It does **not** replace the interceptor. It: + +1. Reports to Sentry with `state.section` / feature tags (a second report if the interceptor already captured on bypass) +2. Patches the named section: `isLoading: false`, `isSubmitting: false`, `error: error.message` +3. Rethrows so components / `dispatch` subscribers can still react + +```ts +.catchError((error) => handleSectionError(ctx, 'registry', error)) +``` + +`section` must be a key on the state model whose value follows `AsyncStateModel` (see [NGXS docs](./ngxs.md#state-models)): + +```ts +export interface AsyncStateModel { + data: T; + isLoading: boolean; + isSubmitting?: boolean; + error: string | null; +} +``` + +Guidelines: + +- Use `handleSectionError` in action `catchError` for async sections — do not only clear loading in an ad-hoc patch unless the section is not `AsyncStateModel`-shaped. +- Do not toast inside the state handler; leave shared toasts to the interceptor (or the component when bypassed / `409`). +- UI that needs an inline error can read `section.error` from the store; many screens rely on interceptor toasts instead. +- Components that `dispatch(...).subscribe({ error })` still receive the error after both layers. + +## Related files + +| File | Role | +| ---- | ---- | +| `src/app/core/interceptors/error.interceptor.ts` | HTTP UX handler | +| `src/app/core/interceptors/error-interceptor.tokens.ts` | Bypass token + factory | +| `src/app/shared/helpers/state-error.handler.ts` | NGXS section error helper | +| `src/app/shared/models/store/async-state.model.ts` | Async section shape (`error` field) | +| `src/app/core/constants/error-messages.ts` | Status → i18n keys | +| `src/app/core/constants/http-error-details.const.ts` | Shared API detail strings | +| `src/app/core/constants/page-not-found.constants.ts` | Route data for 404 / 410 pages | +| `src/app/core/services/maintenance-mode.service.ts` | Maintenance detection + activation | +| `src/app/core/components/page-not-found/` | Shared not-found / content-deleted UI | +| `docs/ngxs.md` | Store / `AsyncStateModel` conventions | +| `docs/sentry.md` | Which HTTP errors are filtered from Sentry | diff --git a/docs/ngxs.md b/docs/ngxs.md index cb3df1551..a57c9c5c9 100644 --- a/docs/ngxs.md +++ b/docs/ngxs.md @@ -87,7 +87,7 @@ export interface FilesStateModel { 1. `data` holds strongly typed domain data (not raw JSON:API payloads when a domain model exists). 2. `isLoading` indicates a read/fetch is in progress. 3. `isSubmitting` indicates a write (create/update/delete) is in progress. -4. `error` stores a failed request message for UI or logging. +4. `error` stores a failed request message for UI or logging. Set it via `handleSectionError` in action `catchError` — see [Error handling](./error-handling.md#ngxs-state-errors). Each domain state should be minimal and scoped to its feature. @@ -112,4 +112,5 @@ Each domain state should be minimal and scoped to its feature. - [Models Conventions](./models.md) - [Folder Structure](./arch.md) +- [Error handling](./error-handling.md) (HTTP interceptor + `handleSectionError`) - Official NGXS docs: [https://www.ngxs.io/docs](https://www.ngxs.io/docs) From e5e4d495c29f78fb03695e375975207bf84c211f Mon Sep 17 00:00:00 2001 From: nsemets Date: Tue, 27 Oct 2026 15:40:06 +0200 Subject: [PATCH 4/5] fix(tests): updated tests and toast --- .../services/project-overview.service.spec.ts | 334 ++++++++++++++++++ .../components/toast/toast.component.html | 2 +- .../shared/services/json-api.service.spec.ts | 133 +++++++ .../providers/json-api.service.mock.ts | 13 +- 4 files changed, 480 insertions(+), 2 deletions(-) create mode 100644 src/app/features/project/overview/services/project-overview.service.spec.ts create mode 100644 src/app/shared/services/json-api.service.spec.ts diff --git a/src/app/features/project/overview/services/project-overview.service.spec.ts b/src/app/features/project/overview/services/project-overview.service.spec.ts new file mode 100644 index 000000000..314248cf7 --- /dev/null +++ b/src/app/features/project/overview/services/project-overview.service.spec.ts @@ -0,0 +1,334 @@ +import { MockProvider } from 'ng-mocks'; + +import { firstValueFrom, of } from 'rxjs'; + +import { HttpContext } from '@angular/common/http'; +import { TestBed } from '@angular/core/testing'; + +import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; +import { JSON_API_BULK_HEADERS } from '@osf/shared/constants/json-api-headers.const'; +import { IdentifiersMapper } from '@osf/shared/mappers/identifiers.mapper'; +import { InstitutionsMapper } from '@osf/shared/mappers/institutions'; +import { LicensesMapper } from '@osf/shared/mappers/licenses.mapper'; +import { BaseNodeMapper } from '@osf/shared/mappers/nodes'; +import { NodePreprintMapper } from '@osf/shared/mappers/nodes/node-preprint.mapper'; +import { NodeStorageMapper } from '@osf/shared/mappers/nodes/node-storage.mapper'; +import { IdentifiersResponseJsonApi } from '@osf/shared/models/identifiers/identifier-json-api.model'; +import { InstitutionsJsonApiResponse } from '@osf/shared/models/institutions/institution-json-api.model'; +import { Institution } from '@osf/shared/models/institutions/institutions.model'; +import { LicenseResponseJsonApi } from '@osf/shared/models/license/licenses-json-api.model'; +import { BaseNodeDataJsonApi } from '@osf/shared/models/nodes/base-node-data-json-api.model'; +import { NodePreprintsResponseJsonApi } from '@osf/shared/models/nodes/node-preprint-json-api.model'; +import { NodeStorageModel } from '@osf/shared/models/nodes/node-storage.model'; +import { NodeResponseJsonApi, NodesResponseJsonApi } from '@osf/shared/models/nodes/nodes-json-api.model'; +import { PaginatedData } from '@osf/shared/models/paginated-data.model'; +import { JsonApiService } from '@osf/shared/services/json-api.service'; +import { LicenseModel } from '@shared/models/license/license.model'; + +import { MOCK_NODE_WITH_ADMIN } from '@testing/mocks/node.mock'; +import { MOCK_NODE_PREPRINTS } from '@testing/mocks/node-preprint.mock'; +import { + MOCK_PROJECT_AFFILIATED_INSTITUTIONS, + MOCK_PROJECT_IDENTIFIERS, + MOCK_PROJECT_OVERVIEW, +} from '@testing/mocks/project-overview.mock'; +import { MOCK_API_URL, provideMockEnvironment } from '@testing/providers/environment.provider.mock'; +import { JsonApiServiceMock, JsonApiServiceMockType } from '@testing/providers/json-api.service.mock'; + +import { ProjectOverviewMapper } from '../mappers'; +import { PrivacyStatusModel } from '../models'; + +import { ProjectOverviewService } from './project-overview.service'; + +describe('ProjectOverviewService', () => { + let service: ProjectOverviewService; + let jsonApiService: JsonApiServiceMockType; + + const projectId = 'project-1'; + const nodeData = { id: projectId, type: 'nodes' } as BaseNodeDataJsonApi; + const nodeResponse = { data: nodeData, meta: { anonymous: false } } as NodeResponseJsonApi; + + function setup() { + jsonApiService = JsonApiServiceMock.simple(); + + TestBed.configureTestingModule({ + providers: [ProjectOverviewService, MockProvider(JsonApiService, jsonApiService), provideMockEnvironment()], + }); + + service = TestBed.inject(ProjectOverviewService); + } + + it('should expose apiUrl from environment', () => { + setup(); + expect(service.apiUrl).toBe(MOCK_API_URL); + }); + + it('getProjectById should request node with related counts and map response', async () => { + setup(); + const mapperSpy = vi.spyOn(ProjectOverviewMapper, 'getProjectOverview').mockReturnValue(MOCK_PROJECT_OVERVIEW); + jsonApiService.get.mockReturnValue(of(nodeResponse)); + + const response = await firstValueFrom(service.getProjectById(projectId)); + + expect(jsonApiService.get).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/${projectId}/`, { + related_counts: 'forks,view_only_links', + }); + expect(mapperSpy).toHaveBeenCalledWith(nodeData); + expect(response).toEqual({ + data: MOCK_PROJECT_OVERVIEW, + totalCount: 1, + pageSize: 1, + isAnonymous: false, + }); + }); + + it('getProjectById should default isAnonymous to false when meta is missing', async () => { + setup(); + vi.spyOn(ProjectOverviewMapper, 'getProjectOverview').mockReturnValue(MOCK_PROJECT_OVERVIEW); + jsonApiService.get.mockReturnValue(of({ data: nodeData } as NodeResponseJsonApi)); + + const response = await firstValueFrom(service.getProjectById(projectId)); + + expect(response.isAnonymous).toBe(false); + }); + + it('getProjectInstitutions should request institutions and map response', async () => { + setup(); + const apiResponse = { data: [] } as unknown as InstitutionsJsonApiResponse; + const mappedInstitutions = MOCK_PROJECT_AFFILIATED_INSTITUTIONS as unknown as Institution[]; + const mapperSpy = vi.spyOn(InstitutionsMapper, 'fromInstitutionsResponse').mockReturnValue(mappedInstitutions); + jsonApiService.get.mockReturnValue(of(apiResponse)); + + const response = await firstValueFrom(service.getProjectInstitutions(projectId)); + + expect(jsonApiService.get).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/${projectId}/institutions/`, { + 'page[size]': 100, + }); + expect(mapperSpy).toHaveBeenCalledWith(apiResponse); + expect(response).toEqual(mappedInstitutions); + }); + + it('getProjectIdentifiers should request identifiers and map response', async () => { + setup(); + const apiResponse = { data: [] } as unknown as IdentifiersResponseJsonApi; + const mapperSpy = vi.spyOn(IdentifiersMapper, 'fromJsonApi').mockReturnValue([MOCK_PROJECT_IDENTIFIERS]); + jsonApiService.get.mockReturnValue(of(apiResponse)); + + const response = await firstValueFrom(service.getProjectIdentifiers(projectId)); + + expect(jsonApiService.get).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/${projectId}/identifiers/`); + expect(mapperSpy).toHaveBeenCalledWith(apiResponse); + expect(response).toEqual([MOCK_PROJECT_IDENTIFIERS]); + }); + + it('getProjectLicense should request license and map response', async () => { + setup(); + const licenseId = 'license-1'; + const mappedLicense = { id: licenseId, name: 'MIT' } as LicenseModel; + const apiResponse = { data: { id: licenseId } } as LicenseResponseJsonApi; + const mapperSpy = vi.spyOn(LicensesMapper, 'fromLicenseDataJsonApi').mockReturnValue(mappedLicense); + jsonApiService.get.mockReturnValue(of(apiResponse)); + + const response = await firstValueFrom(service.getProjectLicense(licenseId)); + + expect(jsonApiService.get).toHaveBeenCalledWith(`${MOCK_API_URL}/licenses/${licenseId}/`); + expect(mapperSpy).toHaveBeenCalledWith(apiResponse.data); + expect(response).toEqual(mappedLicense); + }); + + it('getProjectStorage should request storage and map response', async () => { + setup(); + const mappedStorage = { storageLimitStatus: 'DEFAULT', storageUsage: '1' } as NodeStorageModel; + const apiResponse = { data: { id: projectId } }; + const mapperSpy = vi.spyOn(NodeStorageMapper, 'getNodeStorage').mockReturnValue(mappedStorage); + jsonApiService.get.mockReturnValue(of(apiResponse)); + + const response = await firstValueFrom(service.getProjectStorage(projectId)); + + expect(jsonApiService.get).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/${projectId}/storage/`); + expect(mapperSpy).toHaveBeenCalledWith(apiResponse.data); + expect(response).toEqual(mappedStorage); + }); + + it('getProjectPreprints should request preprints and map response', async () => { + setup(); + const apiResponse = { data: [] } as unknown as NodePreprintsResponseJsonApi; + const mapperSpy = vi.spyOn(NodePreprintMapper, 'getNodePreprints').mockReturnValue(MOCK_NODE_PREPRINTS); + jsonApiService.get.mockReturnValue(of(apiResponse)); + + const response = await firstValueFrom(service.getProjectPreprints(projectId)); + + expect(jsonApiService.get).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/${projectId}/preprints/`); + expect(mapperSpy).toHaveBeenCalledWith(apiResponse.data); + expect(response).toEqual(MOCK_NODE_PREPRINTS); + }); + + it('updateProjectPublicStatus should bulk patch nodes and map response', async () => { + setup(); + const payload: PrivacyStatusModel[] = [ + { id: 'node-1', public: true }, + { id: 'node-2', public: false }, + ]; + const mapped = [MOCK_NODE_WITH_ADMIN]; + const mapperSpy = vi.spyOn(BaseNodeMapper, 'getNodesData').mockReturnValue(mapped); + jsonApiService.patch.mockReturnValue(of([nodeData])); + + const response = await firstValueFrom(service.updateProjectPublicStatus(payload)); + + expect(jsonApiService.patch).toHaveBeenCalledWith( + `${MOCK_API_URL}/nodes/`, + { + data: [ + { id: 'node-1', type: 'nodes', attributes: { public: true } }, + { id: 'node-2', type: 'nodes', attributes: { public: false } }, + ], + }, + undefined, + JSON_API_BULK_HEADERS + ); + expect(mapperSpy).toHaveBeenCalledWith([nodeData]); + expect(response).toEqual(mapped); + }); + + it('duplicateProject should post templated node and map response', async () => { + setup(); + const mapperSpy = vi.spyOn(BaseNodeMapper, 'getNodeData').mockReturnValue(MOCK_NODE_WITH_ADMIN); + jsonApiService.post.mockReturnValue(of(nodeResponse)); + + const response = await firstValueFrom(service.duplicateProject(projectId, 'My Project')); + + expect(jsonApiService.post).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/`, { + data: { + type: 'nodes', + attributes: { + template_from: projectId, + category: 'project', + title: 'Templated from My Project', + }, + }, + }); + expect(mapperSpy).toHaveBeenCalledWith(nodeData); + expect(response).toEqual(MOCK_NODE_WITH_ADMIN); + }); + + it('createComponent should create child without affiliations when none provided', async () => { + setup(); + jsonApiService.post.mockReturnValue(of({ data: { id: 'child-1' } } as NodeResponseJsonApi)); + + const response = await firstValueFrom(service.createComponent(projectId, 'Child', null, ['tag'], null, [], true)); + + expect(jsonApiService.post).toHaveBeenCalledWith( + `${MOCK_API_URL}/nodes/${projectId}/children/`, + { + data: { + type: 'nodes', + attributes: { + title: 'Child', + category: 'project', + description: '', + tags: ['tag'], + }, + }, + }, + { inherit_contributors: true } + ); + expect(jsonApiService.patch).not.toHaveBeenCalled(); + expect(response).toBeUndefined(); + }); + + it('createComponent should include region and patch affiliations when provided', async () => { + setup(); + jsonApiService.post.mockReturnValue(of({ data: { id: 'child-1' } } as NodeResponseJsonApi)); + jsonApiService.patch.mockReturnValue(of(undefined)); + + await firstValueFrom( + service.createComponent(projectId, 'Child', 'Desc', [], 'us-east', ['inst-1', 'inst-2'], false) + ); + + expect(jsonApiService.post).toHaveBeenCalledWith( + `${MOCK_API_URL}/nodes/${projectId}/children/`, + { + data: { + type: 'nodes', + attributes: { + title: 'Child', + category: 'project', + description: 'Desc', + tags: [], + }, + }, + }, + { inherit_contributors: false, region: 'us-east' } + ); + expect(jsonApiService.patch).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/child-1/relationships/institutions/`, { + data: [ + { type: 'institutions', id: 'inst-1' }, + { type: 'institutions', id: 'inst-2' }, + ], + }); + }); + + it('getComponents should request children with embeds and map response', async () => { + setup(); + const mapped: PaginatedData<(typeof MOCK_NODE_WITH_ADMIN)[]> = { + data: [MOCK_NODE_WITH_ADMIN], + totalCount: 1, + pageSize: 10, + }; + const apiResponse = { + data: [], + meta: { total: 1, per_page: 10, version: '2.0' }, + } as NodesResponseJsonApi; + const mapperSpy = vi.spyOn(BaseNodeMapper, 'getNodesWithEmbedsAndTotalData').mockReturnValue(mapped); + jsonApiService.get.mockReturnValue(of(apiResponse)); + + const response = await firstValueFrom(service.getComponents(projectId, 2, 25)); + + expect(jsonApiService.get).toHaveBeenCalledWith(`${MOCK_API_URL}/nodes/${projectId}/children/`, { + embed: 'bibliographic_contributors', + 'fields[users]': 'family_name,full_name,given_name,middle_name', + page: 2, + 'page[size]': 25, + sort: '_order', + }); + expect(mapperSpy).toHaveBeenCalledWith(apiResponse); + expect(response).toEqual(mapped); + }); + + it('getParentProject should request node with bypass context and map response', async () => { + setup(); + const mapperSpy = vi.spyOn(BaseNodeMapper, 'getNodeWithEmbedContributors').mockReturnValue(MOCK_NODE_WITH_ADMIN); + jsonApiService.get.mockReturnValue(of(nodeResponse)); + + const response = await firstValueFrom(service.getParentProject(projectId)); + + expect(jsonApiService.get).toHaveBeenCalledWith( + `${MOCK_API_URL}/nodes/${projectId}/`, + { 'embed[]': ['bibliographic_contributors'] }, + expect.any(HttpContext) + ); + const context = jsonApiService.get.mock.calls[0][2] as HttpContext; + expect(context.get(BYPASS_ERROR_INTERCEPTOR)).toBe(true); + expect(mapperSpy).toHaveBeenCalledWith(nodeData); + expect(response).toEqual(MOCK_NODE_WITH_ADMIN); + }); + + it('reorderComponents should bulk patch component order', async () => { + setup(); + jsonApiService.patch.mockReturnValue(of(undefined)); + + await firstValueFrom(service.reorderComponents(projectId, ['c-2', 'c-1'])); + + expect(jsonApiService.patch).toHaveBeenCalledWith( + `${MOCK_API_URL}/nodes/${projectId}/reorder_components/`, + { + data: [ + { type: 'nodes', id: 'c-2', attributes: { _order: 0 } }, + { type: 'nodes', id: 'c-1', attributes: { _order: 1 } }, + ], + }, + undefined, + JSON_API_BULK_HEADERS + ); + }); +}); diff --git a/src/app/shared/components/toast/toast.component.html b/src/app/shared/components/toast/toast.component.html index ca02a6bba..cc9007bfc 100644 --- a/src/app/shared/components/toast/toast.component.html +++ b/src/app/shared/components/toast/toast.component.html @@ -1,6 +1,6 @@ { + let service: JsonApiService; + let httpMock: HttpTestingController; + + const resource = { + id: '1', + type: 'nodes', + attributes: { title: 'Test' }, + }; + + const document = { + data: resource, + included: [{ id: '2', type: 'users' }], + }; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [provideOSFCore(), provideOSFHttp(), JsonApiService], + }); + + service = TestBed.inject(JsonApiService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + }); + + it('should get the full response document', async () => { + const resultPromise = firstValueFrom(service.get('/v2/nodes/1/')); + + const req = httpMock.expectOne('/v2/nodes/1/'); + expect(req.request.method).toBe('GET'); + req.flush(document); + + await expect(resultPromise).resolves.toEqual(document); + }); + + it('should append array query params', async () => { + const resultPromise = firstValueFrom( + service.get('/v2/nodes/', { 'embed[]': ['bibliographic_contributors', 'parent'] }) + ); + + const req = httpMock.expectOne( + (request) => + request.url === '/v2/nodes/' && + request.params.getAll('embed[]')?.join(',') === 'bibliographic_contributors,parent' + ); + expect(req.request.method).toBe('GET'); + req.flush(document); + + await expect(resultPromise).resolves.toEqual(document); + }); + + it('should post the full response document', async () => { + const body = { data: { type: 'nodes', attributes: { title: 'New' } } }; + const resultPromise = firstValueFrom(service.post('/v2/nodes/', body)); + + const req = httpMock.expectOne('/v2/nodes/'); + expect(req.request.method).toBe('POST'); + expect(req.request.body).toEqual(body); + req.flush(document); + + await expect(resultPromise).resolves.toEqual(document); + }); + + it('should unwrap data from patch responses', async () => { + const body = { data: resource }; + const resultPromise = firstValueFrom(service.patch('/v2/nodes/1/', body)); + + const req = httpMock.expectOne('/v2/nodes/1/'); + expect(req.request.method).toBe('PATCH'); + expect(req.request.body).toEqual(body); + req.flush(document); + + await expect(resultPromise).resolves.toEqual(resource); + }); + + it('should return the full document from patchResponse', async () => { + const body = { data: resource }; + const context = bypassErrorInterceptorContext(); + const resultPromise = firstValueFrom( + service.patchResponse('/v2/nodes/1/?include=users', body, undefined, undefined, context) + ); + + const req = httpMock.expectOne('/v2/nodes/1/?include=users'); + expect(req.request.method).toBe('PATCH'); + expect(req.request.body).toEqual(body); + expect(req.request.context.get(BYPASS_ERROR_INTERCEPTOR)).toBe(true); + req.flush(document); + + await expect(resultPromise).resolves.toEqual(document); + }); + + it('should unwrap data from put responses', async () => { + const body = { data: resource }; + const resultPromise = firstValueFrom(service.put('/v2/nodes/1/', body)); + + const req = httpMock.expectOne('/v2/nodes/1/'); + expect(req.request.method).toBe('PUT'); + req.flush(document); + + await expect(resultPromise).resolves.toEqual(resource); + }); + + it('should delete with optional body, headers, and context', async () => { + const body = { data: [{ id: '1', type: 'nodes' }] }; + const headers = { 'Content-Type': 'application/vnd.api+json; ext=bulk' }; + const context = new HttpContext(); + const resultPromise = firstValueFrom(service.delete('/v2/nodes/', body, headers, context)); + + const req = httpMock.expectOne('/v2/nodes/'); + expect(req.request.method).toBe('DELETE'); + expect(req.request.body).toEqual(body); + expect(req.request.headers.get('Content-Type')).toBe(headers['Content-Type']); + expect(req.request.context).toBe(context); + req.flush(null); + + await expect(resultPromise).resolves.toBeNull(); + }); +}); diff --git a/src/testing/providers/json-api.service.mock.ts b/src/testing/providers/json-api.service.mock.ts index c4c39f312..7134d401f 100644 --- a/src/testing/providers/json-api.service.mock.ts +++ b/src/testing/providers/json-api.service.mock.ts @@ -34,10 +34,14 @@ type DeleteFn = ( context?: HttpContext ) => Observable; -export type JsonApiServiceMockType = Pick & { +export type JsonApiServiceMockType = Pick< + JsonApiService, + 'get' | 'post' | 'patch' | 'patchResponse' | 'put' | 'putFile' | 'delete' +> & { get: Mock; post: Mock; patch: Mock; + patchResponse: Mock; put: Mock; putFile: Mock; delete: Mock; @@ -47,6 +51,7 @@ export class JsonApiServiceMockBuilder { private getMock: Mock = vi.fn().mockReturnValue(of({})); private postMock: Mock = vi.fn().mockReturnValue(of({})); private patchMock: Mock = vi.fn().mockReturnValue(of({})); + private patchResponseMock: Mock = vi.fn().mockReturnValue(of({})); private putMock: Mock = vi.fn().mockReturnValue(of({})); private putFileMock: Mock = vi.fn().mockReturnValue(of({} as HttpEvent)); private deleteMock: Mock = vi.fn().mockReturnValue(of(void 0)); @@ -70,6 +75,11 @@ export class JsonApiServiceMockBuilder { return this; } + withPatchResponse(mockImpl: Mock): JsonApiServiceMockBuilder { + this.patchResponseMock = mockImpl; + return this; + } + withPut(mockImpl: Mock): JsonApiServiceMockBuilder { this.putMock = mockImpl; return this; @@ -90,6 +100,7 @@ export class JsonApiServiceMockBuilder { get: this.getMock, post: this.postMock, patch: this.patchMock, + patchResponse: this.patchResponseMock, put: this.putMock, putFile: this.putFileMock, delete: this.deleteMock, From 0f847322d98f1fe5df182caf59f8d9034d37d1ed Mon Sep 17 00:00:00 2001 From: nsemets Date: Tue, 27 Oct 2026 15:44:59 +0200 Subject: [PATCH 5/5] fix(docs): fixed linting --- docs/error-handling.md | 60 +++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/docs/error-handling.md b/docs/error-handling.md index e96b16939..2545145b3 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -2,10 +2,10 @@ Two complementary layers handle failures from API-backed work: -| Layer | Responsibility | Entry point | -| ----- | -------------- | ----------- | -| **HTTP interceptor** | Global UX: toast, navigation, auth, maintenance, loader | `error.interceptor.ts` | -| **NGXS state** | Section bookkeeping: clear loading flags, store `error`, Sentry tag | `handleSectionError` | +| Layer | Responsibility | Entry point | +| -------------------- | ------------------------------------------------------------------- | ---------------------- | +| **HTTP interceptor** | Global UX: toast, navigation, auth, maintenance, loader | `error.interceptor.ts` | +| **NGXS state** | Section bookkeeping: clear loading flags, store `error`, Sentry tag | `handleSectionError` | Both **rethrow**. For a store action that calls HTTP: interceptor runs first → rethrow → state’s `catchError` → `handleSectionError`. @@ -56,14 +56,14 @@ flowchart TD ### Status handling -| Status | Interceptor behavior | -| ------ | -------------------- | -| **401** | Skip auth redirect on view-only (`?view_only=`). Else logout if authenticated, or navigate to sign-in. Hide loader. | -| **403** | Navigate unless silent / pending-moderation (see below). Node URL → `/request-access/:id` (skipped on draft registry routes). Other → `/forbidden`. Hide loader. No toast. | -| **404** | Navigate to `/not-found`. Hide loader. No toast. | -| **409** | Rethrow only (no toast, no loader hide). Caller handles conflict UX. | -| **410** | Navigate to `/content-deleted`. Hide loader. No toast. | -| **Other** | Toast via `getErrorMessage`, hide loader. | +| Status | Interceptor behavior | +| --------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **401** | Skip auth redirect on view-only (`?view_only=`). Else logout if authenticated, or navigate to sign-in. Hide loader. | +| **403** | Navigate unless silent / pending-moderation (see below). Node URL → `/request-access/:id` (skipped on draft registry routes). Other → `/forbidden`. Hide loader. No toast. | +| **404** | Navigate to `/not-found`. Hide loader. No toast. | +| **409** | Rethrow only (no toast, no loader hide). Caller handles conflict UX. | +| **410** | Navigate to `/content-deleted`. Hide loader. No toast. | +| **Other** | Toast via `getErrorMessage`, hide loader. | `/not-found` and `/content-deleted` share `PageNotFoundComponent` with different route data (`PAGE_NOT_FOUND_ROUTE_DATA` / `CONTENT_DELETED_ROUTE_DATA`). @@ -87,11 +87,11 @@ Current examples: maintenance status polling, parent project fetch, Datacite tra These skip `/forbidden` / request-access navigation (loader still hides; error still rethrows): -| Case | How | -| ---- | --- | -| Metadata record fetch | `X-No-Auth-Redirect` on `MetadataRecordsService.getMetadataRecord` (optional schema.org; prefer migrating to `HttpContext` later) | -| Node access-request API | URL matches `/v2/nodes/:id/requests` | -| Preprint pending moderation | Detail equals `PREPRINT_PENDING_MODERATION_DETAIL` — component routes to pending-moderation | +| Case | How | +| --------------------------- | --------------------------------------------------------------------------------------------------------------------------------- | +| Metadata record fetch | `X-No-Auth-Redirect` on `MetadataRecordsService.getMetadataRecord` (optional schema.org; prefer migrating to `HttpContext` later) | +| Node access-request API | URL matches `/v2/nodes/:id/requests` | +| Preprint pending moderation | Detail equals `PREPRINT_PENDING_MODERATION_DETAIL` — component routes to pending-moderation | ### Messages @@ -147,16 +147,16 @@ Guidelines: ## Related files -| File | Role | -| ---- | ---- | -| `src/app/core/interceptors/error.interceptor.ts` | HTTP UX handler | -| `src/app/core/interceptors/error-interceptor.tokens.ts` | Bypass token + factory | -| `src/app/shared/helpers/state-error.handler.ts` | NGXS section error helper | -| `src/app/shared/models/store/async-state.model.ts` | Async section shape (`error` field) | -| `src/app/core/constants/error-messages.ts` | Status → i18n keys | -| `src/app/core/constants/http-error-details.const.ts` | Shared API detail strings | -| `src/app/core/constants/page-not-found.constants.ts` | Route data for 404 / 410 pages | -| `src/app/core/services/maintenance-mode.service.ts` | Maintenance detection + activation | -| `src/app/core/components/page-not-found/` | Shared not-found / content-deleted UI | -| `docs/ngxs.md` | Store / `AsyncStateModel` conventions | -| `docs/sentry.md` | Which HTTP errors are filtered from Sentry | +| File | Role | +| ------------------------------------------------------- | ------------------------------------------ | +| `src/app/core/interceptors/error.interceptor.ts` | HTTP UX handler | +| `src/app/core/interceptors/error-interceptor.tokens.ts` | Bypass token + factory | +| `src/app/shared/helpers/state-error.handler.ts` | NGXS section error helper | +| `src/app/shared/models/store/async-state.model.ts` | Async section shape (`error` field) | +| `src/app/core/constants/error-messages.ts` | Status → i18n keys | +| `src/app/core/constants/http-error-details.const.ts` | Shared API detail strings | +| `src/app/core/constants/page-not-found.constants.ts` | Route data for 404 / 410 pages | +| `src/app/core/services/maintenance-mode.service.ts` | Maintenance detection + activation | +| `src/app/core/components/page-not-found/` | Shared not-found / content-deleted UI | +| `docs/ngxs.md` | Store / `AsyncStateModel` conventions | +| `docs/sentry.md` | Which HTTP errors are filtered from Sentry |