diff --git a/packages/frontend/jest.config.js b/packages/frontend/jest.config.js index c581bd2b1e..2901cd3024 100644 --- a/packages/frontend/jest.config.js +++ b/packages/frontend/jest.config.js @@ -11,6 +11,8 @@ module.exports = { '^zone.js/testing$': '/../../node_modules/zone.js/bundles/zone-testing.umd.js', // Keep other path mappings from tsconfig '^upgrade_types(.*)$': '/../../../types$1', + '^@shared-component-lib$': '/src/app/shared-standalone-component-lib/components', + '^@shared-component-lib/(.*)$': '/src/app/shared-standalone-component-lib/components/$1', }, // Updated transform configuration @@ -34,7 +36,8 @@ module.exports = { '/dist/', '/e2e/', '/src/environments/', - '/src/app/features/dashboard/', + // Dashboard specs are excluded except the routing spec, which guards subtle redirect behavior + '/src/app/features/dashboard/(?!dashboard-routing\\.spec)', '/src/app/shared/', ], diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.spec.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.spec.ts index 0d175d5319..e5c352a71c 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.spec.ts @@ -1,4 +1,5 @@ -import { HttpClient, HttpParams } from '@angular/common/http'; +import { HttpClient, HttpContext, HttpParams } from '@angular/common/http'; +import { HANDLES_404_CONTEXTUALLY } from '../http-interceptors/http-context-tokens'; import { of } from 'rxjs'; import { ASSIGNMENT_ALGORITHM, @@ -240,13 +241,26 @@ describe('ExperimentDataService', () => { }); describe('#getExperimentById', () => { - it('should get the getExperimentById http observable', () => { + it('should get the getExperimentById http observable with contextual 404 handling when requested', () => { + const experimentId = mockExperimentId; + const expectedUrl = `${API_ENDPOINTS.getExperimentById}/${experimentId}`; + + service.getExperimentById(experimentId, true); + + expect(mockHttpClient.get).toHaveBeenCalledWith(expectedUrl, { context: expect.any(HttpContext) }); + const context: HttpContext = (mockHttpClient.get as jest.Mock).mock.calls[0][1].context; + expect(context.get(HANDLES_404_CONTEXTUALLY)).toBe(true); + }); + + it('should keep the generic 404 notification by default (e.g. for preview-user callers)', () => { const experimentId = mockExperimentId; const expectedUrl = `${API_ENDPOINTS.getExperimentById}/${experimentId}`; service.getExperimentById(experimentId); - expect(mockHttpClient.get).toHaveBeenCalledWith(expectedUrl); + expect(mockHttpClient.get).toHaveBeenCalledWith(expectedUrl, { context: expect.any(HttpContext) }); + const context: HttpContext = (mockHttpClient.get as jest.Mock).mock.calls[0][1].context; + expect(context.get(HANDLES_404_CONTEXTUALLY)).toBe(false); }); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts index 52be0ad3b0..f601fb2acc 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts @@ -10,7 +10,8 @@ import { ExperimentSegmentListResponse, UpdateExperimentConditionsRequest, } from './store/experiments.model'; -import { HttpClient, HttpParams } from '@angular/common/http'; +import { HttpClient, HttpContext, HttpParams } from '@angular/common/http'; +import { HANDLES_404_CONTEXTUALLY } from '../http-interceptors/http-context-tokens'; import { API_ENDPOINTS } from '../api-endpoints.constants'; import { Observable } from 'rxjs'; import { ExperimentSegmentListRequest, SegmentFile } from '../segments/store/segments.model'; @@ -82,9 +83,11 @@ export class ExperimentDataService { return this.http.delete(url); } - getExperimentById(experimentId: string) { + getExperimentById(experimentId: string, handles404Contextually = false) { const url = `${API_ENDPOINTS.getExperimentById}/${experimentId}`; - return this.http.get(url); + // Details-page callers render their own not-found state, so they skip the generic 404 + // notification; other callers (e.g. preview-user) keep it + return this.http.get(url, { context: new HttpContext().set(HANDLES_404_CONTEXTUALLY, handles404Contextually) }); } fetchAllPartitions() { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.spec.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.spec.ts index bbb7ba8270..6d02f87b5e 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.spec.ts @@ -347,7 +347,7 @@ describe('ExperimentService', () => { }); describe('#fetchExperimentById', () => { - it('should dispatch actionGetExperimentById with the given input', () => { + it('should dispatch actionGetExperimentById without contextual 404 handling by default', () => { const experimentId = 'abc123'; service.fetchExperimentById(experimentId); @@ -355,6 +355,20 @@ describe('ExperimentService', () => { expect(mockStore.dispatch).toHaveBeenCalledWith( actionGetExperimentById({ experimentId, + handles404Contextually: false, + }) + ); + }); + + it('should dispatch actionGetExperimentById with contextual 404 handling when requested', () => { + const experimentId = 'abc123'; + + service.fetchExperimentById(experimentId, true); + + expect(mockStore.dispatch).toHaveBeenCalledWith( + actionGetExperimentById({ + experimentId, + handles404Contextually: true, }) ); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts index d1c4f559de..073a05c199 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts @@ -25,6 +25,7 @@ import { selectIsLoadingExperiment, selectIsLoadingUpsertPrivateSegmentList, selectSelectedExperiment, + selectExperimentDetailsPageError, selectExperimentOverviewDetails, selectSearchExperimentParams, selectRootTableState, @@ -74,6 +75,7 @@ export class ExperimentService { isLoadingExperiment$ = this.store$.pipe(select(selectIsLoadingExperiment)); isLoadingUpsertPrivateSegmentList$ = this.store$.pipe(select(selectIsLoadingUpsertPrivateSegmentList)); selectedExperiment$ = this.store$.pipe(select(selectSelectedExperiment)); + experimentDetailsPageError$ = this.store$.pipe(select(selectExperimentDetailsPageError)); selectedExperimentOverviewDetails$ = this.store$.pipe(select(selectExperimentOverviewDetails)); searchParams$ = this.store$.pipe(select(selectSearchExperimentParams)); selectRootTableState$ = this.store$.pipe(select(selectRootTableState)); @@ -153,14 +155,15 @@ export class ExperimentService { ); } - fetchExperimentById(experimentId: string) { - this.store$.dispatch(experimentAction.actionGetExperimentById({ experimentId })); + fetchExperimentById(experimentId: string, handles404Contextually = false) { + this.store$.dispatch(experimentAction.actionGetExperimentById({ experimentId, handles404Contextually })); } refetchCurrentSelectedExperiment() { this.selectedExperiment$.pipe(take(1)).subscribe((experiment) => { if (experiment) { - this.fetchExperimentById(experiment.id); + // selectedExperiment$ only resolves on the details page, which renders its own error state + this.fetchExperimentById(experiment.id, true); } }); } diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts index c562464d9c..ff95af4096 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts @@ -1,4 +1,5 @@ import { createAction, props } from '@ngrx/store'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; import { Experiment, UpsertExperimentType, @@ -48,7 +49,9 @@ export const actionRemoveExperimentStat = createAction( export const actionGetExperimentById = createAction( '[Experiment] Get Experiment By Id', - props<{ experimentId: string }>() + // handles404Contextually: set by details-page callers that render their own not-found state, + // so the generic 404 notification is suppressed for them only (e.g. preview-user keeps it) + props<{ experimentId: string; handles404Contextually?: boolean }>() ); export const actionGetExperimentByIdSuccess = createAction( @@ -56,7 +59,11 @@ export const actionGetExperimentByIdSuccess = createAction( props<{ experiment: Experiment }>() ); -export const actionGetExperimentByIdFailure = createAction('[Experiment] Get Experiment By Id Failure'); +export const actionGetExperimentByIdFailure = createAction( + '[Experiment] Get Experiment By Id Failure', + // Only failures of details-page fetches (handles404Contextually) may write detailsPageError + props<{ experimentId: string; errorType: PAGE_ERROR_TYPE; handles404Contextually?: boolean }>() +); export const actionUpsertExperiment = createAction( '[Experiment] Upsert Experiment', diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts index 804d9ab623..ed7ba16209 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts @@ -1,7 +1,7 @@ import { fakeAsync, tick } from '@angular/core/testing'; import { ActionsSubject } from '@ngrx/store'; -import { BehaviorSubject, of, throwError } from 'rxjs'; -import { last, pairwise, scan, take } from 'rxjs/operators'; +import { BehaviorSubject, of, throwError, timer } from 'rxjs'; +import { delay, last, mergeMap, pairwise, scan, take } from 'rxjs/operators'; import { actionDeleteExperimentSuccess, actionFetchAllExperimentNames, @@ -22,6 +22,7 @@ import { actionDeleteExperimentFailure, actionGetExperimentById, actionGetExperimentByIdSuccess, + actionGetExperimentByIdFailure, actionFetchExperimentDetailStatSuccess, actionFetchExperimentDetailStat, actionGetExperimentsSuccess, @@ -67,6 +68,7 @@ import { actionExecuteQuery, actionFetchMetrics } from '../../analysis/store/ana import { selectCurrentUser } from '../../auth/store/auth.selectors'; import { UserRole } from '../../users/store/users.model'; import { Environment } from '../../../../environments/environment-types'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; describe('ExperimentEffects', () => { let service: ExperimentEffects; @@ -596,7 +598,8 @@ describe('ExperimentEffects', () => { }); describe('#getExperimentById$', () => { - const experimentId = 'testId'; + // Must be a canonical (lowercase) UUID - the effect short-circuits non-canonical ids to a not-found failure + const experimentId = '11111111-2222-4333-8444-555555555555'; const experiment = { id: 'test1', stat: [ @@ -642,6 +645,130 @@ describe('ExperimentEffects', () => { actions$.next(actionGetExperimentById({ experimentId })); })); + + it('should dispatch a not-found failure without calling the API when the id is not a canonical UUID', fakeAsync(() => { + experimentDataService.getExperimentById = jest.fn(); + Selectors.selectExperimentStats.setResult({}); + let result: any; + service.getExperimentById$.subscribe((action: any) => (result = action)); + + actions$.next(actionGetExperimentById({ experimentId: 'not-a-uuid' })); + tick(0); + + expect(result).toEqual( + actionGetExperimentByIdFailure({ experimentId: 'not-a-uuid', errorType: PAGE_ERROR_TYPE.NOT_FOUND }) + ); + expect(experimentDataService.getExperimentById).not.toHaveBeenCalled(); + })); + + it('should dispatch a not-found failure when the fetch fails with 404', fakeAsync(() => { + experimentDataService.getExperimentById = jest.fn().mockReturnValue(throwError(() => ({ status: 404 }))); + Selectors.selectExperimentStats.setResult({}); + let result: any; + service.getExperimentById$.subscribe((action: any) => (result = action)); + + actions$.next(actionGetExperimentById({ experimentId })); + tick(0); + + expect(result).toEqual(actionGetExperimentByIdFailure({ experimentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND })); + })); + + it('should dispatch a load-failed failure when the fetch fails with an unexpected error', fakeAsync(() => { + experimentDataService.getExperimentById = jest.fn().mockReturnValue(throwError(() => ({ status: 500 }))); + Selectors.selectExperimentStats.setResult({}); + let result: any; + service.getExperimentById$.subscribe((action: any) => (result = action)); + + actions$.next(actionGetExperimentById({ experimentId })); + tick(0); + + expect(result).toEqual(actionGetExperimentByIdFailure({ experimentId, errorType: PAGE_ERROR_TYPE.LOAD_FAILED })); + })); + + it('should still dispatch success without stats when only the stats call fails', fakeAsync(() => { + experimentDataService.getExperimentById = jest.fn().mockReturnValue(of(experiment)); + experimentDataService.getAllExperimentsStats = jest.fn().mockReturnValue(throwError(() => ({ status: 500 }))); + Selectors.selectExperimentStats.setResult({}); + let result: any; + service.getExperimentById$.subscribe((action: any) => (result = action)); + + actions$.next(actionGetExperimentById({ experimentId })); + tick(0); + + expect(result).toEqual(actionGetExperimentByIdSuccess({ experiment })); + })); + + it('should cancel a stale request when a newer fetch for the same id is dispatched', fakeAsync(() => { + // The first (stale) request fails slowly; the second succeeds immediately. + // The stale failure must not surface after the newer request has succeeded. + experimentDataService.getExperimentById = jest + .fn() + .mockReturnValueOnce(timer(100).pipe(mergeMap(() => throwError(() => ({ status: 500 }))))) + .mockReturnValueOnce(of(experiment)); + experimentDataService.getAllExperimentsStats = jest.fn().mockReturnValue(of(stats)); + Selectors.selectExperimentStats.setResult({}); + const results: any[] = []; + service.getExperimentById$.subscribe((action: any) => results.push(action)); + + actions$.next(actionGetExperimentById({ experimentId })); + actions$.next(actionGetExperimentById({ experimentId })); + tick(200); + + expect(results).toContainEqual(actionGetExperimentByIdSuccess({ experiment })); + expect(results).not.toContainEqual( + actionGetExperimentByIdFailure({ experimentId, errorType: PAGE_ERROR_TYPE.LOAD_FAILED }) + ); + })); + + it('should cancel a previous details-page fetch when a newer details-page fetch for another id starts', fakeAsync(() => { + // Simulates navigating from details page A to details page B: A's slow failure must not + // surface, or it would overwrite B's detailsPageError and bring the spinner back on B. + const otherExperimentId = '22222222-3333-4333-8444-555555555555'; + const otherExperiment = { ...experiment, id: 'test2' } as any; + experimentDataService.getExperimentById = jest + .fn() + .mockReturnValueOnce(timer(100).pipe(mergeMap(() => throwError(() => ({ status: 500 }))))) + .mockReturnValueOnce(of(otherExperiment)); + experimentDataService.getAllExperimentsStats = jest.fn().mockReturnValue(of(stats)); + Selectors.selectExperimentStats.setResult({}); + const results: any[] = []; + service.getExperimentById$.subscribe((action: any) => results.push(action)); + + actions$.next(actionGetExperimentById({ experimentId, handles404Contextually: true })); + actions$.next(actionGetExperimentById({ experimentId: otherExperimentId, handles404Contextually: true })); + tick(200); + + expect(results).toContainEqual(actionGetExperimentByIdSuccess({ experiment: otherExperiment })); + expect(results).not.toContainEqual( + actionGetExperimentByIdFailure({ + experimentId, + errorType: PAGE_ERROR_TYPE.LOAD_FAILED, + handles404Contextually: true, + }) + ); + })); + + it('should keep concurrent background fetches for different experiment ids', fakeAsync(() => { + // preview-user loads several different experiments at once (without contextual 404 handling) - + // one fetch must not cancel another + const otherExperimentId = '22222222-3333-4333-8444-555555555555'; + const otherExperiment = { ...experiment, id: 'test2' } as any; + experimentDataService.getExperimentById = jest + .fn() + .mockReturnValueOnce(of(experiment).pipe(delay(50))) + .mockReturnValueOnce(of(otherExperiment)); + experimentDataService.getAllExperimentsStats = jest.fn().mockReturnValue(of(stats)); + Selectors.selectExperimentStats.setResult({}); + const results: any[] = []; + service.getExperimentById$.subscribe((action: any) => results.push(action)); + + actions$.next(actionGetExperimentById({ experimentId })); + actions$.next(actionGetExperimentById({ experimentId: otherExperimentId })); + tick(100); + + expect(results).toContainEqual(actionGetExperimentByIdSuccess({ experiment })); + expect(results).toContainEqual(actionGetExperimentByIdSuccess({ experiment: otherExperiment })); + })); }); describe('#getExperimentDetailStat', () => { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts index d2c8f23454..a65033c6ca 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts @@ -3,7 +3,7 @@ import { Actions, createEffect, ofType } from '@ngrx/effects'; import * as experimentAction from './experiments.actions'; import * as analysisActions from '../../analysis/store/analysis.actions'; import { ExperimentDataService } from '../experiments.data.service'; -import { map, filter, switchMap, catchError, tap, withLatestFrom, first, mergeMap } from 'rxjs/operators'; +import { map, filter, switchMap, catchError, tap, withLatestFrom, first, mergeMap, takeUntil } from 'rxjs/operators'; import { UpsertExperimentType, IExperimentEnrollmentStats, @@ -34,6 +34,7 @@ import JSZip from 'jszip'; import { TranslateService } from '@ngx-translate/core'; import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service'; import { CommonExportHelpersService } from '../../../shared/services/common-export-helpers.service'; +import { isCanonicalEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; @Injectable() export class ExperimentEffects { constructor( @@ -302,11 +303,20 @@ export class ExperimentEffects { getExperimentById$ = createEffect(() => this.actions$.pipe( ofType(experimentAction.actionGetExperimentById), - map((action) => action.experimentId), - filter((experimentId) => !!experimentId), + filter((action) => !!action.experimentId), withLatestFrom(this.store$.pipe(select(selectExperimentStats))), - mergeMap(([experimentId, experimentStats]) => - this.experimentDataService.getExperimentById(experimentId).pipe( + mergeMap(([action, experimentStats]) => { + const { experimentId, handles404Contextually } = action; + if (!isCanonicalEntityId(experimentId)) { + return of( + experimentAction.actionGetExperimentByIdFailure({ + experimentId, + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + handles404Contextually, + }) + ); + } + return this.experimentDataService.getExperimentById(experimentId, handles404Contextually ?? false).pipe( switchMap((data: Experiment) => this.experimentDataService.getAllExperimentsStats([data.id]).pipe( switchMap((stat: IExperimentEnrollmentStats) => { @@ -315,11 +325,37 @@ export class ExperimentEffects { experimentAction.actionGetExperimentByIdSuccess({ experiment: data }), experimentAction.actionFetchExperimentStatsSuccess({ stats }), ]; - }) + }), + // Stats are auxiliary - still show the experiment if only the stats call fails + catchError(() => [experimentAction.actionGetExperimentByIdSuccess({ experiment: data })]) + ) + ), + catchError((error) => [ + experimentAction.actionGetExperimentByIdFailure({ + experimentId, + errorType: error?.status === 404 ? PAGE_ERROR_TYPE.NOT_FOUND : PAGE_ERROR_TYPE.LOAD_FAILED, + handles404Contextually, + }), + ]), + // A newer fetch supersedes this one when it targets the same experiment, or when both are + // details-page fetches (the details page shows one experiment at a time, and a stale failure + // must not overwrite the current route's detailsPageError). Cancelling instead of switchMap + // keeps preview-user's concurrent fetches for different experiments alive. The reference + // check excludes the triggering action itself, which BehaviorSubject-backed action streams + // (e.g. ActionsSubject in tests) replay on subscription. + takeUntil( + this.actions$.pipe( + ofType(experimentAction.actionGetExperimentById), + filter( + (newerAction) => + newerAction !== action && + (newerAction.experimentId === experimentId || + !!(handles404Contextually && newerAction.handles404Contextually)) + ) ) ) - ) - ) + ); + }) ) ); diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index 212f7095d6..7337200bb3 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts @@ -1,4 +1,5 @@ import { AppState } from '../../core.module'; +import { DetailsPageError } from '@shared-component-lib/common-page-error/common-page-error.model'; import { CONSISTENCY_RULE, ASSIGNMENT_UNIT, @@ -629,6 +630,8 @@ export interface ExperimentState { isLoadingRewardsSummary: boolean; rewardsSummaries: Record; isLoadingUpsertPrivateSegmentList?: boolean; + // Details page fetch error state - drives the not-found / load-failed page + detailsPageError: DetailsPageError | null; } export interface State extends AppState { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.spec.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.spec.ts index 91e87b2c8e..cd1014dbca 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.spec.ts @@ -1,5 +1,6 @@ import { initialState, experimentsReducer } from './experiments.reducer'; import { Action } from '@ngrx/store'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; import { actionDeleteExperimentSuccess, actionFetchAllExperimentNamesSuccess, @@ -118,15 +119,64 @@ describe('ExperimentsReducer', () => { expect(newState.isLoadingExperiment).toEqual(false); }); - it('action "actionGetExperimentByIdFailure" should set loading to false', () => { + it('action "actionGetExperimentByIdFailure" should set loading to false and set the details page error for a details-page fetch', () => { const previousState = { ...initialState }; previousState.isLoadingExperiment = true; - const testAction: Action = actionGetExperimentByIdFailure(); + const testAction: Action = actionGetExperimentByIdFailure({ + experimentId: 'abc123', + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + handles404Contextually: true, + }); const newState = experimentsReducer(previousState, testAction); expect(newState).not.toBe(previousState); expect(newState.isLoadingExperiment).toEqual(false); + expect(newState.detailsPageError).toEqual({ entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }); + }); + + it('action "actionGetExperimentByIdFailure" of a background fetch should not overwrite the details page error', () => { + const detailsPageError = { entityId: 'current-details-id', errorType: PAGE_ERROR_TYPE.NOT_FOUND }; + const previousState = { ...initialState, detailsPageError }; + const testAction: Action = actionGetExperimentByIdFailure({ + experimentId: 'background-id', + errorType: PAGE_ERROR_TYPE.LOAD_FAILED, + }); + + const newState = experimentsReducer(previousState, testAction); + + expect(newState.isLoadingExperiment).toEqual(false); + expect(newState.detailsPageError).toEqual(detailsPageError); + }); + + it('action "actionGetExperimentById" should retain the details page error while a retry is in flight', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const previousState = { ...initialState, detailsPageError }; + const testAction: Action = actionGetExperimentById({ experimentId: 'abc123', handles404Contextually: true }); + + const newState = experimentsReducer(previousState, testAction); + + expect(newState.detailsPageError).toEqual(detailsPageError); + }); + + it('action "actionGetExperimentByIdSuccess" should clear the details page error of that experiment', () => { + const previousState = { ...initialState }; + previousState.detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const testAction: Action = actionGetExperimentByIdSuccess({ experiment: { id: 'abc123' } as any }); + + const newState = experimentsReducer(previousState, testAction); + + expect(newState.detailsPageError).toBeNull(); + }); + + it('action "actionGetExperimentByIdSuccess" of another experiment should not clear the details page error', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const previousState = { ...initialState, detailsPageError }; + const testAction: Action = actionGetExperimentByIdSuccess({ experiment: { id: 'other-id' } as any }); + + const newState = experimentsReducer(previousState, testAction); + + expect(newState.detailsPageError).toEqual(detailsPageError); }); it('action "actionUpsertExperimentFailure" should set loading to false', () => { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts index 2f560932f3..af7e6182d9 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts @@ -31,6 +31,7 @@ export const initialState: ExperimentState = { isLoadingRewardsSummary: false, rewardsSummaries: {}, isLoadingUpsertPrivateSegmentList: false, + detailsPageError: null, }; const reducer = createReducer( @@ -55,12 +56,21 @@ const reducer = createReducer( }), on( experimentsAction.actionGetExperimentsFailure, - experimentsAction.actionGetExperimentByIdFailure, experimentsAction.actionUpsertExperimentFailure, experimentsAction.actionUpdateExperimentFilterModeFailure, experimentsAction.actionUpdateExperimentStateFailure, (state) => ({ ...state, isLoadingExperiment: false }) ), + on( + experimentsAction.actionGetExperimentByIdFailure, + (state, { experimentId, errorType, handles404Contextually }) => ({ + ...state, + isLoadingExperiment: false, + // Only details-page fetches own the details error state - a background failure (e.g. preview-user) + // must not overwrite the error of the experiment the details page is currently showing + detailsPageError: handles404Contextually ? { entityId: experimentId, errorType } : state.detailsPageError, + }) + ), on(experimentsAction.actionUpsertExperimentSuccess, (state, { experiment }) => { // Update experiment if it exists, otherwise don't add to list (let refetch handle it) const updatedExperiments = state.experiments.map((exp) => (exp.id === experiment.id ? experiment : exp)); @@ -92,7 +102,14 @@ const reducer = createReducer( delete stats[experimentStatId]; return { ...state, stats }; }), - on(experimentsAction.actionUpsertExperiment, experimentsAction.actionGetExperimentById, (state) => ({ + on(experimentsAction.actionUpsertExperiment, (state) => ({ + ...state, + isLoadingExperiment: true, + // If the total count is unknown, assume at least one experiment is loading so the root page skips the empty state. + // Preserve 0 because it means the backend already confirmed an empty list. + totalExperiments: state.totalExperiments ?? 1, + })), + on(experimentsAction.actionGetExperimentById, (state) => ({ ...state, isLoadingExperiment: true, // If the total count is unknown, assume at least one experiment is loading so the root page skips the empty state. @@ -116,6 +133,9 @@ const reducer = createReducer( ...state, experiments: updatedExperiments, isLoadingExperiment: false, + // The error is retained while a retry is in flight (so the error page doesn't fall back to a + // stale cached entity) and only cleared once a fetch for that same experiment succeeds + detailsPageError: state.detailsPageError?.entityId === experiment.id ? null : state.detailsPageError, }; }), // Experiment Delete Actions diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selector.spec.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selector.spec.ts index 06f55bd5d7..cbe514a3a5 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selector.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selector.spec.ts @@ -34,7 +34,9 @@ import { selectTotalExperiment, selectRewardsDataForSelectedExperiment, selectIsLoadingRewardsSummary, + selectExperimentDetailsPageError, } from './experiments.selectors'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; describe('Experiments Selectors', () => { const mockState: ExperimentState = { @@ -925,4 +927,33 @@ describe('Experiments Selectors', () => { expect(result).toEqual(false); }); }); + + describe('#selectExperimentDetailsPageError', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }; + const routerStateFor = (experimentId: string) => ({ state: { params: { experimentId } } } as any); + + it('should return the error when it belongs to the experiment in the route', () => { + const previousState = { ...mockState, detailsPageError }; + + const result = selectExperimentDetailsPageError.projector(routerStateFor('abc123'), previousState); + + expect(result).toEqual(detailsPageError); + }); + + it('should return null when the error belongs to a different experiment', () => { + const previousState = { ...mockState, detailsPageError }; + + const result = selectExperimentDetailsPageError.projector(routerStateFor('other-id'), previousState); + + expect(result).toBeNull(); + }); + + it('should return null when there is no error', () => { + const previousState = { ...mockState, detailsPageError: null }; + + const result = selectExperimentDetailsPageError.projector(routerStateFor('abc123'), previousState); + + expect(result).toBeNull(); + }); + }); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts index 102e8bcf39..49ccbe1f1b 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts @@ -28,6 +28,7 @@ import { import { determineWeightingMethod, isWeightSumValid } from '../condition-helper.service'; import { formatTSConfigurablePolicyParamDetails } from '../mooclet-helper.service'; import { KeyValueFormat } from '@shared-component-lib/common-section-card-overview-details/common-section-card-overview-details.component'; +import { DetailsPageError } from '@shared-component-lib/common-page-error/common-page-error.model'; export const selectExperimentState = createFeatureSelector('experiments'); @@ -82,6 +83,18 @@ export const selectSelectedExperiment = createSelector( } ); +export const selectExperimentDetailsPageError = createSelector( + selectRouterState, + selectExperimentState, + (routerState, experimentState): DetailsPageError | null => { + const experimentId = routerState?.state?.params?.experimentId; + const detailsPageError = experimentState?.detailsPageError; + + // Only surface the error if it belongs to the experiment currently in the route + return detailsPageError && detailsPageError.entityId === experimentId ? detailsPageError : null; + } +); + export const selectConditionWeightsValid = createSelector(selectSelectedExperiment, (experiment): boolean => { return isWeightSumValid(experiment?.conditions || []); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.spec.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.spec.ts new file mode 100644 index 0000000000..6e2d751f32 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.spec.ts @@ -0,0 +1,32 @@ +import { HttpClient, HttpContext } from '@angular/common/http'; +import { of } from 'rxjs'; +import { FeatureFlagsDataService } from './feature-flags.data.service'; +import { API_ENDPOINTS } from '../api-endpoints.constants'; +import { HANDLES_404_CONTEXTUALLY } from '../http-interceptors/http-context-tokens'; + +class MockHTTPClient { + get = jest.fn().mockReturnValue(of()); +} + +describe('FeatureFlagsDataService', () => { + let mockHttpClient: any; + let service: FeatureFlagsDataService; + + beforeEach(() => { + mockHttpClient = new MockHTTPClient(); + service = new FeatureFlagsDataService(mockHttpClient as HttpClient); + }); + + describe('#fetchFeatureFlagById', () => { + it('should fetch the feature flag observable with contextual 404 handling', () => { + const flagId = 'flagId1'; + const expectedUrl = `${API_ENDPOINTS.featureFlag}/${flagId}`; + + service.fetchFeatureFlagById(flagId); + + expect(mockHttpClient.get).toHaveBeenCalledWith(expectedUrl, { context: expect.any(HttpContext) }); + const context: HttpContext = (mockHttpClient.get as jest.Mock).mock.calls[0][1].context; + expect(context.get(HANDLES_404_CONTEXTUALLY)).toBe(true); + }); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts index fc751c59dd..22211f86fb 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts @@ -1,5 +1,6 @@ import { Injectable } from '@angular/core'; -import { HttpClient, HttpParams } from '@angular/common/http'; +import { HttpClient, HttpContext, HttpParams } from '@angular/common/http'; +import { HANDLES_404_CONTEXTUALLY } from '../http-interceptors/http-context-tokens'; import { AddFeatureFlagRequest, FeatureFlag, @@ -34,7 +35,8 @@ export class FeatureFlagsDataService { fetchFeatureFlagById(id: string) { const url = `${API_ENDPOINTS.featureFlag}/${id}`; - return this.http.get(url); + // The details page renders its own not-found state, so skip the generic 404 notification + return this.http.get(url, { context: new HttpContext().set(HANDLES_404_CONTEXTUALLY, true) }); } updateFeatureFlagStatus(params: UpdateFeatureFlagStatusRequest): Observable { diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts index 64d8324d63..d8141a7987 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts @@ -13,6 +13,7 @@ import { selectIsLoadingUpsertPrivateSegmentList, selectIsLoadingUpdateFeatureFlagStatus, selectSelectedFeatureFlag, + selectFeatureFlagDetailsPageError, selectSearchFeatureFlagParams, selectRootTableState, selectFeatureFlagOverviewDetails, @@ -78,6 +79,7 @@ export class FeatureFlagsService { featureFlagTotalExposures$ = this.store$.pipe(select(selectFeatureFlagTotalExposures)); selectedFlagOverviewDetails = this.store$.pipe(select(selectFeatureFlagOverviewDetails)); selectedFeatureFlag$ = this.store$.pipe(select(selectSelectedFeatureFlag)); + featureFlagDetailsPageError$ = this.store$.pipe(select(selectFeatureFlagDetailsPageError)); searchParams$ = this.store$.pipe(select(selectSearchFeatureFlagParams)); selectRootTableState$ = this.store$.select(selectRootTableState); selectFeatureFlagInclusions$ = this.store$.pipe(select(selectFeatureFlagInclusions)); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts index 30d846ac11..f4ae3da4b2 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts @@ -1,4 +1,5 @@ import { createAction, props } from '@ngrx/store'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; import { FeatureFlag, UpdateFeatureFlagStatusRequest, @@ -34,7 +35,10 @@ export const actionFetchFeatureFlagByIdSuccess = createAction( props<{ flag: FeatureFlag }>() ); -export const actionFetchFeatureFlagByIdFailure = createAction('[Feature Flags] Fetch Feature Flags By Id Failure'); +export const actionFetchFeatureFlagByIdFailure = createAction( + '[Feature Flags] Fetch Feature Flags By Id Failure', + props<{ featureFlagId: string; errorType: PAGE_ERROR_TYPE }>() +); export const actionAddFeatureFlag = createAction( '[Feature Flags] Add Feature Flag', props<{ addFeatureFlagRequest: AddFeatureFlagRequest }>() diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.spec.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.spec.ts new file mode 100644 index 0000000000..85be9d81c8 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.spec.ts @@ -0,0 +1,124 @@ +import { fakeAsync, tick } from '@angular/core/testing'; +import { ActionsSubject } from '@ngrx/store'; +import { BehaviorSubject, of, throwError } from 'rxjs'; +import { FeatureFlagsEffects } from './feature-flags.effects'; +import * as FeatureFlagsActions from './feature-flags.actions'; +import { FeatureFlag } from './feature-flags.model'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; + +describe('FeatureFlagsEffects', () => { + let store$: any; + let actions$: ActionsSubject; + let featureFlagsDataService: any; + let router: any; + let notificationService: any; + let translate: any; + let commonExportHelpersService: any; + let commonModalEvents: any; + let service: FeatureFlagsEffects; + + beforeEach(() => { + actions$ = new ActionsSubject(); + store$ = new BehaviorSubject({}); + store$.dispatch = jest.fn(); + featureFlagsDataService = {}; + router = { + navigate: jest.fn(), + }; + notificationService = { + showSuccess: jest.fn(), + showError: jest.fn(), + }; + translate = { + instant: jest.fn((key) => key), + }; + commonExportHelpersService = {}; + commonModalEvents = { + forceCloseModal: jest.fn(), + }; + + service = new FeatureFlagsEffects( + store$, + actions$, + featureFlagsDataService, + router, + notificationService, + translate, + commonExportHelpersService, + commonModalEvents + ); + }); + + describe('#fetchFeatureFlagById$', () => { + // Must be a canonical (lowercase) UUID - the effect short-circuits non-canonical ids to a not-found failure + const featureFlagId = '11111111-2222-4333-8444-555555555555'; + const flag = { id: featureFlagId, name: 'test flag' } as FeatureFlag; + + it('should dispatch success when the flag is returned', fakeAsync(() => { + featureFlagsDataService.fetchFeatureFlagById = jest.fn().mockReturnValue(of(flag)); + let result: any; + service.fetchFeatureFlagById$.subscribe((action: any) => (result = action)); + + actions$.next(FeatureFlagsActions.actionFetchFeatureFlagById({ featureFlagId })); + tick(0); + + expect(result).toEqual(FeatureFlagsActions.actionFetchFeatureFlagByIdSuccess({ flag })); + })); + + it('should dispatch a not-found failure when the backend returns an empty body (204) for an unknown id', fakeAsync(() => { + featureFlagsDataService.fetchFeatureFlagById = jest.fn().mockReturnValue(of(null)); + let result: any; + service.fetchFeatureFlagById$.subscribe((action: any) => (result = action)); + + actions$.next(FeatureFlagsActions.actionFetchFeatureFlagById({ featureFlagId })); + tick(0); + + expect(result).toEqual( + FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ featureFlagId, errorType: PAGE_ERROR_TYPE.NOT_FOUND }) + ); + })); + + it('should dispatch a not-found failure when the fetch fails with 404', fakeAsync(() => { + featureFlagsDataService.fetchFeatureFlagById = jest.fn().mockReturnValue(throwError(() => ({ status: 404 }))); + let result: any; + service.fetchFeatureFlagById$.subscribe((action: any) => (result = action)); + + actions$.next(FeatureFlagsActions.actionFetchFeatureFlagById({ featureFlagId })); + tick(0); + + expect(result).toEqual( + FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ featureFlagId, errorType: PAGE_ERROR_TYPE.NOT_FOUND }) + ); + })); + + it('should dispatch a load-failed failure when the fetch fails with an unexpected error', fakeAsync(() => { + featureFlagsDataService.fetchFeatureFlagById = jest.fn().mockReturnValue(throwError(() => ({ status: 500 }))); + let result: any; + service.fetchFeatureFlagById$.subscribe((action: any) => (result = action)); + + actions$.next(FeatureFlagsActions.actionFetchFeatureFlagById({ featureFlagId })); + tick(0); + + expect(result).toEqual( + FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ featureFlagId, errorType: PAGE_ERROR_TYPE.LOAD_FAILED }) + ); + })); + + it('should dispatch a not-found failure without calling the API when the id is not a canonical UUID', fakeAsync(() => { + featureFlagsDataService.fetchFeatureFlagById = jest.fn(); + let result: any; + service.fetchFeatureFlagById$.subscribe((action: any) => (result = action)); + + actions$.next(FeatureFlagsActions.actionFetchFeatureFlagById({ featureFlagId: 'not-a-uuid' })); + tick(0); + + expect(result).toEqual( + FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ + featureFlagId: 'not-a-uuid', + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + }) + ); + expect(featureFlagsDataService.fetchFeatureFlagById).not.toHaveBeenCalled(); + })); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts index 41d2304efe..7acdbc6c4f 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts @@ -15,6 +15,7 @@ import { CommonExportHelpersService } from '../../../shared/services/common-expo import { of } from 'rxjs'; import { SERVER_ERROR } from 'upgrade_types'; import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service'; +import { isCanonicalEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; @Injectable() export class FeatureFlagsEffects { @@ -357,14 +358,34 @@ export class FeatureFlagsEffects { ofType(FeatureFlagsActions.actionFetchFeatureFlagById), map((action) => action.featureFlagId), filter((featureFlagId) => !!featureFlagId), - switchMap((featureFlagId) => - this.featureFlagsDataService.fetchFeatureFlagById(featureFlagId).pipe( + switchMap((featureFlagId) => { + if (!isCanonicalEntityId(featureFlagId)) { + return of( + FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ + featureFlagId, + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + }) + ); + } + return this.featureFlagsDataService.fetchFeatureFlagById(featureFlagId).pipe( map((data: FeatureFlag) => { + // The backend responds with 204 (empty body) rather than 404 when the flag doesn't exist + if (!data) { + return FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ + featureFlagId, + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + }); + } return FeatureFlagsActions.actionFetchFeatureFlagByIdSuccess({ flag: data }); }), - catchError(() => [FeatureFlagsActions.actionFetchFeatureFlagByIdFailure()]) - ) - ) + catchError((error) => [ + FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ + featureFlagId, + errorType: error?.status === 404 ? PAGE_ERROR_TYPE.NOT_FOUND : PAGE_ERROR_TYPE.LOAD_FAILED, + }), + ]) + ); + }) ) ); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts index 203b42ee7c..2be1b480d3 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts @@ -1,4 +1,5 @@ import { AppState } from '../../core.state'; +import { DetailsPageError } from '@shared-component-lib/common-page-error/common-page-error.model'; import { FEATURE_FLAG_STATUS, FILTER_MODE, FLAG_SEARCH_KEY, FLAG_SORT_KEY, SORT_AS_DIRECTION } from 'upgrade_types'; import { MemberTypes, Segment } from '../../segments/store/segments.model'; @@ -209,6 +210,8 @@ export interface FeatureFlagState { isLoadingFeatureFlagDelete: boolean; isLoadingUpsertPrivateSegmentList: boolean; duplicateKeyFound: boolean; + // Details page fetch error state - drives the not-found / load-failed page + detailsPageError: DetailsPageError | null; // Graph data graphInfo: IExposureStatByDate[] | null; diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.spec.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.spec.ts new file mode 100644 index 0000000000..ea3f5b0503 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.spec.ts @@ -0,0 +1,60 @@ +import { featureFlagsReducer, initialState } from './feature-flags.reducer'; +import * as FeatureFlagsActions from './feature-flags.actions'; +import { FeatureFlag } from './feature-flags.model'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; + +describe('FeatureFlagsReducer', () => { + describe('actionFetchFeatureFlagByIdFailure', () => { + it('should set isLoadingSelectedFeatureFlag to false and set the details page error', () => { + const previousState = { ...initialState }; + previousState.isLoadingSelectedFeatureFlag = true; + const testAction = FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ + featureFlagId: 'abc123', + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + }); + + const newState = featureFlagsReducer(previousState, testAction); + + expect(newState.isLoadingSelectedFeatureFlag).toEqual(false); + expect(newState.detailsPageError).toEqual({ entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }); + }); + }); + + describe('actionFetchFeatureFlagById', () => { + it('should retain the details page error while a retry is in flight', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const previousState = { ...initialState, detailsPageError }; + const testAction = FeatureFlagsActions.actionFetchFeatureFlagById({ featureFlagId: 'abc123' }); + + const newState = featureFlagsReducer(previousState, testAction); + + expect(newState.detailsPageError).toEqual(detailsPageError); + }); + }); + + describe('actionFetchFeatureFlagByIdSuccess', () => { + it('should clear the details page error of that feature flag', () => { + const previousState = { ...initialState }; + previousState.detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const testAction = FeatureFlagsActions.actionFetchFeatureFlagByIdSuccess({ + flag: { id: 'abc123' } as FeatureFlag, + }); + + const newState = featureFlagsReducer(previousState, testAction); + + expect(newState.detailsPageError).toBeNull(); + }); + + it('should not clear the details page error of another feature flag', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const previousState = { ...initialState, detailsPageError }; + const testAction = FeatureFlagsActions.actionFetchFeatureFlagByIdSuccess({ + flag: { id: 'other-id' } as FeatureFlag, + }); + + const newState = featureFlagsReducer(previousState, testAction); + + expect(newState.detailsPageError).toEqual(detailsPageError); + }); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts index 0794e7ff10..e6f6e1884f 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts @@ -24,6 +24,7 @@ export const initialState: FeatureFlagState = { isLoadingFeatureFlagDelete: false, isLoadingUpsertPrivateSegmentList: false, duplicateKeyFound: false, + detailsPageError: null, // Graph state graphInfo: null, @@ -78,10 +79,14 @@ const reducer = createReducer( ...state, selectedFlag: flag, isLoadingSelectedFeatureFlag: false, + // The error is retained while a retry is in flight (so the error page doesn't fall back to a + // stale cached flag) and only cleared once a fetch for that same flag succeeds + detailsPageError: state.detailsPageError?.entityId === flag.id ? null : state.detailsPageError, })), - on(FeatureFlagsActions.actionFetchFeatureFlagByIdFailure, (state) => ({ + on(FeatureFlagsActions.actionFetchFeatureFlagByIdFailure, (state, { featureFlagId, errorType }) => ({ ...state, isLoadingSelectedFeatureFlag: false, + detailsPageError: { entityId: featureFlagId, errorType }, })), // Feature Flag Upsert Actions (Add/Update both = upsert result) diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.spec.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.spec.ts new file mode 100644 index 0000000000..e6e8c6fe91 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.spec.ts @@ -0,0 +1,34 @@ +import { initialState } from './feature-flags.reducer'; +import { selectFeatureFlagDetailsPageError } from './feature-flags.selectors'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; + +describe('FeatureFlagsSelectors', () => { + describe('#selectFeatureFlagDetailsPageError', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }; + const routerStateFor = (flagId: string) => ({ state: { params: { flagId } } } as any); + + it('should return the error when it belongs to the feature flag in the route', () => { + const previousState = { ...initialState, detailsPageError }; + + const result = selectFeatureFlagDetailsPageError.projector(routerStateFor('abc123'), previousState); + + expect(result).toEqual(detailsPageError); + }); + + it('should return null when the error belongs to a different feature flag', () => { + const previousState = { ...initialState, detailsPageError }; + + const result = selectFeatureFlagDetailsPageError.projector(routerStateFor('other-id'), previousState); + + expect(result).toBeNull(); + }); + + it('should return null when there is no error', () => { + const previousState = { ...initialState, detailsPageError: null }; + + const result = selectFeatureFlagDetailsPageError.projector(routerStateFor('abc123'), previousState); + + expect(result).toBeNull(); + }); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts index b413120939..f296430bd1 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts @@ -1,4 +1,5 @@ import { createSelector, createFeatureSelector } from '@ngrx/store'; +import { DetailsPageError } from '@shared-component-lib/common-page-error/common-page-error.model'; import { FeatureFlag, FeatureFlagState, ParticipantListTableRow } from './feature-flags.model'; import { selectRouterState } from '../../core.state'; import { selectContextMetaData } from '../../experiments/store/experiments.selectors'; @@ -70,6 +71,18 @@ export const selectSelectedFeatureFlag = createSelector( } ); +export const selectFeatureFlagDetailsPageError = createSelector( + selectRouterState, + selectFeatureFlagsState, + (routerState, featureFlagState): DetailsPageError | null => { + const flagId = routerState?.state?.params?.flagId; + const detailsPageError = featureFlagState?.detailsPageError; + + // Only surface the error if it belongs to the feature flag currently in the route + return detailsPageError && detailsPageError.entityId === flagId ? detailsPageError : null; + } +); + export const selectFeatureFlagOverviewDetails = createSelector(selectSelectedFeatureFlag, (featureFlag) => ({ ['Key']: featureFlag?.key, ['Description']: featureFlag?.description, diff --git a/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-context-tokens.ts b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-context-tokens.ts new file mode 100644 index 0000000000..4ba196e66a --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-context-tokens.ts @@ -0,0 +1,7 @@ +import { HttpContextToken } from '@angular/common/http'; + +// Requests that render their own not-found state (e.g. details pages) set this token +// so a 404 doesn't also trigger the generic error notification. +// Kept in its own file so data services can import it without pulling in the +// interceptor's dependency graph (AuthService -> CoreModule), which is circular. +export const HANDLES_404_CONTEXTUALLY = new HttpContextToken(() => false); diff --git a/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.spec.ts b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.spec.ts index adbe13befe..4ab047c60d 100644 --- a/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.spec.ts @@ -1,8 +1,10 @@ +import { HttpContext } from '@angular/common/http'; import { fakeAsync, tick } from '@angular/core/testing'; import { NotificationType } from 'angular2-notifications'; import { of, throwError } from 'rxjs'; import { environment } from '../../../environments/environment'; import { HttpErrorInterceptor } from './http-error.interceptor'; +import { HANDLES_404_CONTEXTUALLY } from './http-context-tokens'; class MockAuthService {} @@ -118,6 +120,67 @@ describe('HttpErrorInterceptor', () => { expect(mockAuthService.authLogout).not.toHaveBeenCalled(); })); + it('should NOT open popup for a 404 on a request that handles 404 contextually', fakeAsync(() => { + const mockError = { status: 404, message: 'test' }; + const mockRequest: any = { context: new HttpContext().set(HANDLES_404_CONTEXTUALLY, true) }; + const mockNextHandler = { + handle: jest.fn().mockReturnValue(throwError(() => mockError)), + }; + mockAuthService.authLogout = jest.fn(); + service.openPopup = jest.fn(); + + service.intercept(mockRequest, mockNextHandler).subscribe({ + error: (error: Error) => { + expect(error).toEqual(mockError); + }, + }); + + tick(0); + + expect(service.openPopup).not.toHaveBeenCalled(); + expect(mockAuthService.authLogout).not.toHaveBeenCalled(); + })); + + it('should open popup for a 404 on a request that does NOT handle 404 contextually', fakeAsync(() => { + const mockError = { status: 404, message: 'test' }; + const mockRequest: any = { context: new HttpContext() }; + const mockNextHandler = { + handle: jest.fn().mockReturnValue(throwError(() => mockError)), + }; + mockAuthService.authLogout = jest.fn(); + service.openPopup = jest.fn(); + + service.intercept(mockRequest, mockNextHandler).subscribe({ + error: (error: Error) => { + expect(error).toEqual(mockError); + }, + }); + + tick(0); + + expect(service.openPopup).toHaveBeenCalled(); + })); + + it('should open popup for a non-404 error even when the request handles 404 contextually', fakeAsync(() => { + const mockError = { status: 500, message: 'test' }; + const mockRequest: any = { context: new HttpContext().set(HANDLES_404_CONTEXTUALLY, true) }; + const mockNextHandler = { + handle: jest.fn().mockReturnValue(throwError(() => mockError)), + }; + mockAuthService.authLogout = jest.fn(); + service.openPopup = jest.fn(); + + service.intercept(mockRequest, mockNextHandler).subscribe({ + error: (error: Error) => { + expect(error).toEqual(mockError); + }, + }); + + tick(0); + + expect(service.openPopup).toHaveBeenCalled(); + })); + it('should NOT logout or open popup if no error', fakeAsync(() => { const mockRequest: any = {}; const mockNextHandler = { diff --git a/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts index 582254113e..a7f212029f 100755 --- a/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts +++ b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts @@ -6,6 +6,7 @@ import { catchError } from 'rxjs/operators'; import { ENV, Environment } from '../../../environments/environment-types'; import { AuthService } from '../auth/auth.service'; import { SERVER_ERROR } from 'upgrade_types'; +import { HANDLES_404_CONTEXTUALLY } from './http-context-tokens'; @Injectable() export class HttpErrorInterceptor implements HttpInterceptor { @@ -33,7 +34,7 @@ export class HttpErrorInterceptor implements HttpInterceptor { if (err.status === 401) { // auto logout if 401 response returned from api this.authService.authLogout(); - } else { + } else if (!(err.status === 404 && request.context.get(HANDLES_404_CONTEXTUALLY))) { this.openPopup(err); } // re-throw to allow the error to be caught by the calling code diff --git a/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.spec.ts b/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.spec.ts index c2e4f7fd28..1f6de6ce49 100755 --- a/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.spec.ts @@ -47,6 +47,7 @@ describe('LocalStorageService', () => { rewardsSummaries: {}, isLoadingRewardsSummary: false, hasInitialExperimentsDataLoaded: false, + detailsPageError: null, }; const expectedStateWithDefaults: ExperimentState = { experiments: [], @@ -75,6 +76,7 @@ describe('LocalStorageService', () => { rewardsSummaries: {}, isLoadingRewardsSummary: false, hasInitialExperimentsDataLoaded: false, + detailsPageError: null, }; const testCases = [ diff --git a/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts b/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts index c2ea6f8ad9..69a5e3dcf4 100755 --- a/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts @@ -59,6 +59,7 @@ export class LocalStorageService { isLoadingRewardsSummary: false, rewardsSummaries: {}, hasInitialExperimentsDataLoaded: false, + detailsPageError: null, }; const featureFlagState: FeatureFlagState = { @@ -82,6 +83,7 @@ export class LocalStorageService { graphInfo: null, isGraphLoading: false, totalExposures: null, + detailsPageError: null, }; const segmentState: SegmentState = { @@ -101,6 +103,7 @@ export class LocalStorageService { sortAs: (segmentSortType as SORT_AS_DIRECTION) || SORT_AS_DIRECTION.ASCENDING, isLoadingUpsertSegment: false, listSegmentOptions: [], + detailsPageError: null, }; const state = { diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.spec.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.spec.ts index 11edf1463e..1dc01b0dd8 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.spec.ts @@ -1,6 +1,7 @@ -import { HttpClient, HttpParams } from '@angular/common/http'; +import { HttpClient, HttpContext, HttpParams } from '@angular/common/http'; import { of } from 'rxjs'; import { SegmentsDataService } from './segments.data.service'; +import { HANDLES_404_CONTEXTUALLY } from '../http-interceptors/http-context-tokens'; import { AddPrivateSegmentListRequest, EditPrivateSegmentListRequest, @@ -290,4 +291,16 @@ describe('SegmentDataService', () => { expect(mockHttpClient.delete).toHaveBeenCalledWith(expectedUrl, { body: { parentSegmentId } }); }); }); + + describe('#getSegmentById', () => { + it('should get the segment status observable with contextual 404 handling', () => { + const expectedUrl = `${API_ENDPOINTS.segments}/status/${mockSegmentId}`; + + service.getSegmentById(mockSegmentId); + + expect(mockHttpClient.get).toHaveBeenCalledWith(expectedUrl, { context: expect.any(HttpContext) }); + const context: HttpContext = (mockHttpClient.get as jest.Mock).mock.calls[0][1].context; + expect(context.get(HANDLES_404_CONTEXTUALLY)).toBe(true); + }); + }); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts index c533fb3d5f..a5ab4ccce7 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts @@ -10,7 +10,8 @@ import { SegmentsPaginationParams, } from './store/segments.model'; import { map, Observable } from 'rxjs'; -import { HttpClient, HttpParams } from '@angular/common/http'; +import { HttpClient, HttpContext, HttpParams } from '@angular/common/http'; +import { HANDLES_404_CONTEXTUALLY } from '../http-interceptors/http-context-tokens'; import { FeatureFlagSegmentListDetails } from '../feature-flags/store/feature-flags.model'; import { API_ENDPOINTS } from '../api-endpoints.constants'; @@ -45,7 +46,8 @@ export class SegmentsDataService { getSegmentById(id: string) { const url = `${API_ENDPOINTS.segments}/status/${id}`; - return this.http.get(url); + // The details page renders its own not-found state, so skip the generic 404 notification + return this.http.get(url, { context: new HttpContext().set(HANDLES_404_CONTEXTUALLY, true) }); } // Lazy-loads a list's members for editing; the /members endpoint also returns private lists. diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index 12dd838630..dd72007cd7 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -7,6 +7,7 @@ import { selectIsLoadingSegments, selectAllSegments, selectSelectedSegment, + selectSegmentDetailsPageError, selectRootTableState, selectSegmentOverviewDetails, selectExperimentSegmentsInclusion, @@ -63,6 +64,7 @@ export class SegmentsService { isLoadingGlobalSegments$ = this.store$.pipe(select(selectIsLoadingGlobalSegments)); selectAllSegments$ = this.store$.pipe(select(selectAllSegments)); selectedSegment$ = this.store$.pipe(select(selectSelectedSegment)); + segmentDetailsPageError$ = this.store$.pipe(select(selectSegmentDetailsPageError)); selectRootTableState$ = this.store$.pipe(select(selectRootTableState)); selectGlobalTableState$ = this.store$.pipe(select(selectGlobalTableState)); selectedSegmentOverviewDetails = this.store$.pipe(select(selectSegmentOverviewDetails)); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts index 039fd879a9..35c65648f4 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts @@ -1,4 +1,5 @@ import { createAction, props } from '@ngrx/store'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; import { AddPrivateSegmentListRequest, AddSegmentRequest, @@ -112,7 +113,10 @@ export const actionGetSegmentByIdSuccess = createAction( }>() ); -export const actionGetSegmentByIdFailure = createAction('[Segments] Get Segment By Id Failure'); +export const actionGetSegmentByIdFailure = createAction( + '[Segments] Get Segment By Id Failure', + props<{ segmentId: string; errorType: PAGE_ERROR_TYPE }>() +); export const actionDeleteSegment = createAction('[Segments] Delete Segment', props<{ segmentId: string }>()); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts index 16d76b10be..3724b3abdd 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts @@ -7,6 +7,7 @@ import { Segment, SegmentFile, SegmentInput, UpsertSegmentType } from './segment import { selectAllSegments } from './segments.selectors'; import * as SegmentsActions from './segments.actions'; import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; describe('SegmentsEffects', () => { let store$: any; @@ -262,4 +263,49 @@ describe('SegmentsEffects', () => { tick(0); })); }); + + describe('#getSegmentById$', () => { + // Must be a canonical (lowercase) UUID - the effect short-circuits non-canonical ids to a not-found failure + const segmentId = '11111111-2222-4333-8444-555555555555'; + + it('should dispatch a not-found failure without calling the API when the id is not a canonical UUID', fakeAsync(() => { + segmentsDataService.getSegmentById = jest.fn(); + let result: any; + service.getSegmentById$.subscribe((action: any) => (result = action)); + + actions$.next(SegmentsActions.actionGetSegmentById({ segmentId: 'not-a-uuid' })); + tick(0); + + expect(result).toEqual( + SegmentsActions.actionGetSegmentByIdFailure({ segmentId: 'not-a-uuid', errorType: PAGE_ERROR_TYPE.NOT_FOUND }) + ); + expect(segmentsDataService.getSegmentById).not.toHaveBeenCalled(); + })); + + it('should dispatch a not-found failure when the fetch fails with 404', fakeAsync(() => { + segmentsDataService.getSegmentById = jest.fn().mockReturnValue(throwError(() => ({ status: 404 }))); + let result: any; + service.getSegmentById$.subscribe((action: any) => (result = action)); + + actions$.next(SegmentsActions.actionGetSegmentById({ segmentId })); + tick(0); + + expect(result).toEqual( + SegmentsActions.actionGetSegmentByIdFailure({ segmentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND }) + ); + })); + + it('should dispatch a load-failed failure when the fetch fails with an unexpected error', fakeAsync(() => { + segmentsDataService.getSegmentById = jest.fn().mockReturnValue(throwError(() => ({ status: 500 }))); + let result: any; + service.getSegmentById$.subscribe((action: any) => (result = action)); + + actions$.next(SegmentsActions.actionGetSegmentById({ segmentId })); + tick(0); + + expect(result).toEqual( + SegmentsActions.actionGetSegmentByIdFailure({ segmentId, errorType: PAGE_ERROR_TYPE.LOAD_FAILED }) + ); + })); + }); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts index 35aff13c1d..3b2f22986c 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts @@ -16,6 +16,7 @@ import { } from './segments.selectors'; import JSZip from 'jszip'; import { of } from 'rxjs'; +import { isCanonicalEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; import { SEGMENT_STATUS, SERVER_ERROR } from 'upgrade_types'; import { SegmentsService } from '../segments.service'; import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service'; @@ -133,8 +134,11 @@ export class SegmentsEffects { ofType(SegmentsActions.actionGetSegmentById), map((action) => action.segmentId), filter((segmentId) => !!segmentId), - switchMap((segmentId) => - this.segmentsDataService.getSegmentById(segmentId).pipe( + switchMap((segmentId) => { + if (!isCanonicalEntityId(segmentId)) { + return of(SegmentsActions.actionGetSegmentByIdFailure({ segmentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND })); + } + return this.segmentsDataService.getSegmentById(segmentId).pipe( map((data: any) => { return SegmentsActions.actionGetSegmentByIdSuccess({ segment: data.segment, @@ -145,9 +149,14 @@ export class SegmentsEffects { allParentSegments: data.allParentSegments, }); }), - catchError(() => [SegmentsActions.actionGetSegmentByIdFailure()]) - ) - ) + catchError((error) => [ + SegmentsActions.actionGetSegmentByIdFailure({ + segmentId, + errorType: error?.status === 404 ? PAGE_ERROR_TYPE.NOT_FOUND : PAGE_ERROR_TYPE.LOAD_FAILED, + }), + ]) + ); + }) ) ); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts index 88c9928312..4efbecdfc7 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts @@ -1,4 +1,5 @@ import { AppState } from '../../core.state'; +import { DetailsPageError } from '@shared-component-lib/common-page-error/common-page-error.model'; import { SEGMENT_TYPE, SEGMENT_STATUS, SEGMENT_SEARCH_KEY, SORT_AS_DIRECTION, SEGMENT_SORT_KEY } from 'upgrade_types'; export { SEGMENT_STATUS }; @@ -249,6 +250,8 @@ export interface SegmentState { sortKey: SEGMENT_SORT_KEY; sortAs: SORT_AS_DIRECTION; listSegmentOptions: ListSegmentOption[]; + // Details page fetch error state - drives the not-found / load-failed page + detailsPageError: DetailsPageError | null; } export interface ListSegmentOption { diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.spec.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.spec.ts index e1a472d9e8..c4e01ae3e0 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.spec.ts @@ -2,6 +2,7 @@ import { segmentsReducer, initialState } from './segments.reducer'; import * as SegmentsActions from './segments.actions'; import { Segment } from './segments.model'; import { SEGMENT_STATUS, SEGMENT_TYPE } from 'upgrade_types'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; describe('SegmentsReducer', () => { describe('actions to kick off requests w/ isLoadingSegments ', () => { @@ -22,11 +23,68 @@ describe('SegmentsReducer', () => { } }); + describe('actionGetSegmentByIdFailure', () => { + it('should set isLoadingSegments to false and set the details page error', () => { + const previousState = { ...initialState }; + previousState.isLoadingSegments = true; + const testAction = SegmentsActions.actionGetSegmentByIdFailure({ + segmentId: 'abc123', + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + }); + + const newState = segmentsReducer(previousState, testAction); + + expect(newState.isLoadingSegments).toEqual(false); + expect(newState.detailsPageError).toEqual({ entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }); + }); + }); + + describe('actionGetSegmentById', () => { + it('should retain the details page error while a retry is in flight', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const previousState = { ...initialState, detailsPageError }; + const testAction = SegmentsActions.actionGetSegmentById({ segmentId: 'abc123' }); + + const newState = segmentsReducer(previousState, testAction); + + expect(newState.detailsPageError).toEqual(detailsPageError); + }); + }); + + describe('actionGetSegmentByIdSuccess', () => { + const successActionFor = (segmentId: string) => + SegmentsActions.actionGetSegmentByIdSuccess({ + segment: { id: segmentId } as Segment, + experimentSegmentInclusion: null, + experimentSegmentExclusion: null, + featureFlagSegmentInclusion: null, + featureFlagSegmentExclusion: null, + allParentSegments: null, + }); + + it('should clear the details page error of that segment', () => { + const previousState = { ...initialState }; + previousState.detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + + const newState = segmentsReducer(previousState, successActionFor('abc123')); + + expect(newState.detailsPageError).toBeNull(); + }); + + it('should not clear the details page error of another segment', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.LOAD_FAILED }; + const previousState = { ...initialState, detailsPageError }; + + const newState = segmentsReducer(previousState, successActionFor('other-id')); + + expect(newState.detailsPageError).toEqual(detailsPageError); + }); + }); + describe('actions to request failures to set isloadingSegments to false', () => { const testActions = { actionFetchSegmentsFailure: SegmentsActions.actionFetchSegmentsFailure, actionUpsertSegmentFailure: SegmentsActions.actionUpsertSegmentFailure, - actionGetSegmentByIdFailure: SegmentsActions.actionGetSegmentByIdFailure, }; for (const actionKey in testActions) { diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts index 8aaef2ad95..38b26a64e7 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts @@ -25,6 +25,7 @@ export const initialState: SegmentState = { sortAs: SORT_AS_DIRECTION.ASCENDING, isLoadingUpsertSegment: false, listSegmentOptions: [], + detailsPageError: null, }; const reducer = createReducer( @@ -88,11 +89,15 @@ const reducer = createReducer( on( SegmentsActions.actionFetchSegmentsFailure, SegmentsActions.actionUpsertSegmentFailure, - SegmentsActions.actionGetSegmentByIdFailure, SegmentsActions.actionUpdateSegmentSuccess, SegmentsActions.actionAddSegmentSuccess, (state) => ({ ...state, isLoadingSegments: false }) ), + on(SegmentsActions.actionGetSegmentByIdFailure, (state, { segmentId, errorType }) => ({ + ...state, + isLoadingSegments: false, + detailsPageError: { entityId: segmentId, errorType }, + })), on(SegmentsActions.actionUpsertSegmentSuccess, (state) => ({ ...state, isLoadingSegments: false, @@ -131,6 +136,9 @@ const reducer = createReducer( allFeatureFlagSegmentsExclusion: featureFlagSegmentExclusion, allParentSegments, isLoadingSegments: false, + // The error is retained while a retry is in flight (so the error page doesn't fall back to a + // stale cached segment) and only cleared once a fetch for that same segment succeeds + detailsPageError: state.detailsPageError?.entityId === segment.id ? null : state.detailsPageError, }; return newState; } diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.spec.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.spec.ts index ee390e2142..e5258f73df 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.spec.ts @@ -2,6 +2,7 @@ import { SEGMENT_STATUS, SEGMENT_TYPE } from 'upgrade_types'; import { Segment } from './segments.model'; import { initialState, initalGlobalState } from './segments.reducer'; import * as SegmentSelectors from './segments.selectors'; +import { PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; describe('SegmentSelectors', () => { const mockState = { ...initialState }; @@ -116,4 +117,36 @@ describe('SegmentSelectors', () => { expect(result).toEqual(mockSegment); }); }); + + describe('#selectSegmentDetailsPageError', () => { + const detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }; + const routerStateFor = (segmentId: string) => ({ state: { params: { segmentId } } } as any); + + it('should return the error when it belongs to the segment in the route', () => { + const previousState = { ...mockState, detailsPageError }; + + const result = SegmentSelectors.selectSegmentDetailsPageError.projector(routerStateFor('abc123'), previousState); + + expect(result).toEqual(detailsPageError); + }); + + it('should return null when the error belongs to a different segment', () => { + const previousState = { ...mockState, detailsPageError }; + + const result = SegmentSelectors.selectSegmentDetailsPageError.projector( + routerStateFor('other-id'), + previousState + ); + + expect(result).toBeNull(); + }); + + it('should return null when there is no error', () => { + const previousState = { ...mockState, detailsPageError: null }; + + const result = SegmentSelectors.selectSegmentDetailsPageError.projector(routerStateFor('abc123'), previousState); + + expect(result).toBeNull(); + }); + }); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts index c443a0f7d2..bb3ef98920 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts @@ -1,4 +1,5 @@ import { createSelector, createFeatureSelector } from '@ngrx/store'; +import { DetailsPageError } from '@shared-component-lib/common-page-error/common-page-error.model'; import { SegmentState, ParticipantListTableRow, @@ -100,6 +101,18 @@ export const selectSelectedSegment = createSelector( } ); +export const selectSegmentDetailsPageError = createSelector( + selectRouterState, + selectSegmentsState, + (routerState, segmentState): DetailsPageError | null => { + const segmentId = routerState?.state?.params?.segmentId; + const detailsPageError = segmentState?.detailsPageError; + + // Only surface the error if it belongs to the segment currently in the route + return detailsPageError && detailsPageError.entityId === segmentId ? detailsPageError : null; + } +); + export const selectSegmentOverviewDetails = createSelector(selectSelectedSegment, (segment) => ({ ['Description']: segment?.description, ['App Context']: segment?.context, diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/dashboard-routing.module.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/dashboard-routing.module.ts index ff636dfdec..2c41252fb1 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/dashboard-routing.module.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/dashboard-routing.module.ts @@ -1,9 +1,10 @@ import { NgModule } from '@angular/core'; import { Routes, RouterModule } from '@angular/router'; import { DashboardRootComponent } from './dashboard-root/dashboard-root.component'; +import { requireRouteParam } from './require-route-param.guard'; // Conditionally define segments routes based on the toggle -const segmentsRoutes = [ +const segmentsRoutes: Routes = [ { path: 'segments', loadComponent: () => @@ -12,8 +13,15 @@ const segmentsRoutes = [ title: 'app-header.title.segments', }, }, + { + // An id-less detail URL should land on the list page, not fall through the global wildcard to /home + path: 'segments/detail', + redirectTo: '/segments', + pathMatch: 'full', + }, { path: 'segments/detail/:segmentId', + canActivate: [requireRouteParam('segmentId', '/segments')], loadComponent: () => import('./segments/pages/segment-details-page/segment-details-page.component').then( (c) => c.SegmentDetailsPageComponent @@ -24,7 +32,8 @@ const segmentsRoutes = [ }, ]; -const routes: Routes = [ +// Exported for dashboard-routing.spec.ts +export const routes: Routes = [ { path: '', component: DashboardRootComponent, @@ -44,8 +53,14 @@ const routes: Routes = [ title: 'app-header.title.experiments', }, }, + { + path: 'home/detail', + redirectTo: '/home', + pathMatch: 'full', + }, { path: 'home/detail/:experimentId', + canActivate: [requireRouteParam('experimentId', '/home')], loadComponent: () => import('./experiments/pages/experiment-details-page/experiment-details-page.component').then( (c) => c.ExperimentDetailsPageComponent @@ -83,8 +98,14 @@ const routes: Routes = [ title: 'app-header.title.feature-flag', }, }, + { + path: 'featureflags/detail', + redirectTo: '/featureflags', + pathMatch: 'full', + }, { path: 'featureflags/detail/:flagId', + canActivate: [requireRouteParam('flagId', '/featureflags')], loadComponent: () => import('./feature-flags/pages/feature-flag-details-page/feature-flag-details-page.component').then( (c) => c.FeatureFlagDetailsPageComponent diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/dashboard-routing.spec.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/dashboard-routing.spec.ts new file mode 100644 index 0000000000..da8ab6dfc4 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/dashboard-routing.spec.ts @@ -0,0 +1,49 @@ +import { Component } from '@angular/core'; +import { TestBed } from '@angular/core/testing'; +import { provideRouter, Route, Router } from '@angular/router'; +import { routes } from './dashboard-routing.module'; + +@Component({ template: '' }) +class StubComponent {} + +// The routing decisions under test (redirects, pathMatch, guards) don't depend on the routed +// components, so lazy component loading is stubbed to keep the spec fast and hermetic. +const stubComponents = (route: Route): Route => ({ + ...route, + ...(route.loadComponent ? { loadComponent: () => StubComponent } : {}), + ...(route.children ? { children: route.children.map(stubComponents) } : {}), +}); + +// Regression tests for the id-less detail URL handling. The trailing-slash case is subtle: +// Angular parses '/home/detail/' as ['home', 'detail', ''], so the static 'home/detail' +// redirect (pathMatch: 'full', 2 segments) cannot match it - instead the empty segment +// matches ':experimentId' and the requireRouteParam guard performs the redirect. +describe('dashboard routing', () => { + let router: Router; + + beforeEach(() => { + TestBed.configureTestingModule({ providers: [provideRouter(routes.map(stubComponents))] }); + router = TestBed.inject(Router); + }); + + it.each([ + ['/home/detail', '/home'], + ['/home/detail/', '/home'], + ['/featureflags/detail', '/featureflags'], + ['/featureflags/detail/', '/featureflags'], + ['/segments/detail', '/segments'], + ['/segments/detail/', '/segments'], + ])('should redirect the id-less detail URL %s to %s', async (url, expected) => { + await router.navigateByUrl(url); + + expect(router.url).toBe(expected); + }); + + it('should not redirect a detail URL that has an id', async () => { + const url = '/home/detail/2382605e-1dd0-43fa-bbfd-59e3a460efa6'; + + await router.navigateByUrl(url); + + expect(router.url).toBe(url); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html index 782bc0f705..7273b79fa5 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html @@ -1,5 +1,13 @@
- @if (experiment$ | async; as experiment) { + + @if (detailsPageError$ | async; as detailsPageError) { + + } @else if (experiment$ | async; as experiment) { ; + detailsPageError$: Observable; experimentIdSub: Subscription; shouldShowRewardFeedback$: Observable; + readonly pageErrorConfig: CommonPageErrorConfig = { + notFoundTitleKey: 'experiments.details-page-error.not-found.title.text', + notFoundSubtitleKey: 'experiments.details-page-error.not-found.subtitle.text', + loadFailedTitleKey: 'experiments.details-page-error.load-failed.title.text', + loadFailedSubtitleKey: 'experiments.details-page-error.load-failed.subtitle.text', + backButtonKey: 'experiments.details-page-error.back-button.text', + backRoute: '/home', + }; + constructor( private readonly experimentsService: ExperimentService, private readonly router: Router, @@ -79,10 +94,11 @@ export class ExperimentDetailsPageContentComponent implements OnInit, OnDestroy ); this.experimentIdSub = experimentId$.subscribe((experimentId) => { - this.experimentsService.fetchExperimentById(experimentId); + this.experimentsService.fetchExperimentById(experimentId, true); }); this.experiment$ = this.experimentsService.selectedExperiment$; + this.detailsPageError$ = this.experimentsService.experimentDetailsPageError$; this.segmentService.fetchAllSegmentListOptions(); // Determine if reward feedback card should be shown @@ -107,6 +123,10 @@ export class ExperimentDetailsPageContentComponent implements OnInit, OnDestroy this.activeTabIndex = tabIndex; } + onRetry(experimentId: string): void { + this.experimentsService.fetchExperimentById(experimentId, true); + } + ngOnDestroy() { this.experimentIdSub.unsubscribe(); } diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.html b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.html index 8e84d8e043..6aad6e1be1 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.html +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.html @@ -1,6 +1,6 @@ diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.ts index 3be3cc3d93..85923eaa5b 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-header/experiment-details-page-header.component.ts @@ -2,6 +2,8 @@ import { ChangeDetectionStrategy, Component } from '@angular/core'; import { CommonDetailsPageHeaderComponent } from '@shared-component-lib'; import { ExperimentService } from '../../../../../../core/experiments/experiments.service'; import { CommonModule } from '@angular/common'; +import { combineLatest, Observable } from 'rxjs'; +import { map } from 'rxjs/operators'; @Component({ selector: 'app-experiment-details-page-header', @@ -11,7 +13,12 @@ import { CommonModule } from '@angular/common'; changeDetection: ChangeDetectionStrategy.OnPush, }) export class ExperimentDetailsPageHeaderComponent { - selectedExperiment$ = this.experimentService.selectedExperiment$; + // Suppress the cached experiment name while the details page shows its error state, + // so the breadcrumb doesn't display a stale name next to "not found" + detailsName$: Observable = combineLatest([ + this.experimentService.selectedExperiment$, + this.experimentService.experimentDetailsPageError$, + ]).pipe(map(([experiment, detailsPageError]) => (detailsPageError ? '' : experiment?.name ?? ''))); constructor(private experimentService: ExperimentService) {} } diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-details-page-content.component.html b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-details-page-content.component.html index 2a575784b1..6b0b12a5b6 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-details-page-content.component.html +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-details-page-content.component.html @@ -1,5 +1,13 @@
- @if (featureFlag$ | async; as featureFlag) { + + @if (detailsPageError$ | async; as detailsPageError) { + + } @else if (featureFlag$ | async; as featureFlag) { ; + detailsPageError$: Observable; featureFlagIdSub: Subscription; + readonly pageErrorConfig: CommonPageErrorConfig = { + notFoundTitleKey: 'feature-flags.details-page-error.not-found.title.text', + notFoundSubtitleKey: 'feature-flags.details-page-error.not-found.subtitle.text', + loadFailedTitleKey: 'feature-flags.details-page-error.load-failed.title.text', + loadFailedSubtitleKey: 'feature-flags.details-page-error.load-failed.subtitle.text', + backButtonKey: 'feature-flags.details-page-error.back-button.text', + backRoute: '/featureflags', + }; + constructor( private featureFlagsService: FeatureFlagsService, private router: Router, @@ -66,6 +81,7 @@ export class FeatureFlagDetailsPageContentComponent implements OnInit, OnDestroy }); this.featureFlag$ = this.featureFlagsService.selectedFeatureFlag$; + this.detailsPageError$ = this.featureFlagsService.featureFlagDetailsPageError$; this.segmentService.fetchAllSegmentListOptions(); } @@ -77,6 +93,10 @@ export class FeatureFlagDetailsPageContentComponent implements OnInit, OnDestroy this.activeTabIndex = tabIndex; } + onRetry(featureFlagId: string): void { + this.featureFlagsService.fetchFeatureFlagById(featureFlagId); + } + ngOnDestroy() { this.featureFlagIdSub.unsubscribe(); } diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.html b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.html index 2ddff99dfe..99c7b9aacf 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.html +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.html @@ -1,6 +1,6 @@ diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.ts index 7383d9e80b..4cee9fbcf7 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-header/feature-flag-details-page-header.component.ts @@ -2,6 +2,8 @@ import { ChangeDetectionStrategy, Component } from '@angular/core'; import { CommonDetailsPageHeaderComponent } from '@shared-component-lib'; import { FeatureFlagsService } from '../../../../../../core/feature-flags/feature-flags.service'; import { CommonModule } from '@angular/common'; +import { combineLatest, Observable } from 'rxjs'; +import { map } from 'rxjs/operators'; @Component({ selector: 'app-feature-flag-details-page-header', @@ -11,7 +13,12 @@ import { CommonModule } from '@angular/common'; changeDetection: ChangeDetectionStrategy.OnPush, }) export class FeatureFlagDetailsPageHeaderComponent { - selectedFeatureFlag$ = this.featureFlagService.selectedFeatureFlag$; + // Suppress the cached flag name while the details page shows its error state, + // so the breadcrumb doesn't display a stale name next to "not found" + detailsName$: Observable = combineLatest([ + this.featureFlagService.selectedFeatureFlag$, + this.featureFlagService.featureFlagDetailsPageError$, + ]).pipe(map(([featureFlag, detailsPageError]) => (detailsPageError ? '' : featureFlag?.name ?? ''))); constructor(private featureFlagService: FeatureFlagsService) {} } diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/require-route-param.guard.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/require-route-param.guard.ts new file mode 100644 index 0000000000..52cef3b3a4 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/require-route-param.guard.ts @@ -0,0 +1,14 @@ +import { inject } from '@angular/core'; +import { CanActivateFn, Router } from '@angular/router'; + +/** + * Redirects incomplete detail URLs to their list page when the id route param is empty. + * + * A trailing slash (e.g. `/home/detail/`) matches the `:experimentId` param with an empty + * string, which would otherwise leave the details page waiting for a fetch that never + * happens. Redirecting mirrors the wildcard behavior of the same URL without the slash. + */ +export const requireRouteParam = + (paramName: string, redirectTo: string): CanActivateFn => + (route) => + route.paramMap.get(paramName) ? true : inject(Router).parseUrl(redirectTo); diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.html b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.html index 8e23f77e9d..6af2849dc1 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.html +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.html @@ -1,5 +1,13 @@
- @if (segment$ | async; as segment) { + + @if (detailsPageError$ | async; as detailsPageError) { + + } @else if (segment$ | async; as segment) { ; + detailsPageError$: Observable; activeTabIndex = 0; // 0 for Lists, 1 for Used By + readonly pageErrorConfig: CommonPageErrorConfig = { + notFoundTitleKey: 'segments.details-page-error.not-found.title.text', + notFoundSubtitleKey: 'segments.details-page-error.not-found.subtitle.text', + loadFailedTitleKey: 'segments.details-page-error.load-failed.title.text', + loadFailedSubtitleKey: 'segments.details-page-error.load-failed.subtitle.text', + backButtonKey: 'segments.details-page-error.back-button.text', + backRoute: '/segments', + }; + constructor(private segmentsService: SegmentsService) {} ngOnInit() { this.segment$ = this.segmentsService.selectedSegment$; + this.detailsPageError$ = this.segmentsService.segmentDetailsPageError$; this.segmentsService.fetchAllSegmentListOptions(); } + onRetry(segmentId: string): void { + this.segmentsService.fetchSegmentById(segmentId); + } + onSectionCardExpandChange(expanded: boolean) { this.isSectionCardExpanded = expanded; } diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.html b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.html index 96f7674826..d6a1b7d0f4 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.html +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.html @@ -1,6 +1,6 @@ diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.ts index 7347c2fbe4..3bf7e763be 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-header/segment-details-page-header.component.ts @@ -2,6 +2,8 @@ import { ChangeDetectionStrategy, Component } from '@angular/core'; import { CommonDetailsPageHeaderComponent } from '@shared-component-lib'; import { SegmentsService } from '../../../../../../core/segments/segments.service'; import { CommonModule } from '@angular/common'; +import { combineLatest, Observable } from 'rxjs'; +import { map } from 'rxjs/operators'; @Component({ selector: 'app-segment-details-page-header', @@ -11,7 +13,12 @@ import { CommonModule } from '@angular/common'; changeDetection: ChangeDetectionStrategy.OnPush, }) export class SegmentDetailsPageHeaderComponent { - selectedSegment$ = this.segmentsService.selectedSegment$; + // Suppress the cached segment name while the details page shows its error state, + // so the breadcrumb doesn't display a stale name next to "not found" + detailsName$: Observable = combineLatest([ + this.segmentsService.selectedSegment$, + this.segmentsService.segmentDetailsPageError$, + ]).pipe(map(([segment, detailsPageError]) => (detailsPageError ? '' : segment?.name ?? ''))); constructor(private segmentsService: SegmentsService) {} } diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.html b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.html new file mode 100644 index 0000000000..94ab18cb37 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.html @@ -0,0 +1,21 @@ + diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.scss b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.scss new file mode 100644 index 0000000000..bb4840bcff --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.scss @@ -0,0 +1,40 @@ +.page-error { + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + text-align: center; + padding: 120px 32px; + + .page-error-icon { + display: flex; + align-items: center; + justify-content: center; + width: 120px; + height: 120px; + margin-bottom: 16px; + border-radius: 50%; + background-color: var(--zircon); + + .material-symbols-outlined { + font-size: 60px; + color: var(--blue); + } + } + + .page-error-title { + margin: 0 0 8px; + color: var(--light-black); + } + + .page-error-subtitle { + max-width: 560px; + margin: 0 0 32px; + color: var(--grey-message); + } + + .page-error-actions { + display: flex; + gap: 16px; + } +} diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.spec.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.spec.ts new file mode 100644 index 0000000000..d6eaa84eee --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.spec.ts @@ -0,0 +1,99 @@ +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { By } from '@angular/platform-browser'; +import { provideRouter, Router } from '@angular/router'; +import { TranslateModule } from '@ngx-translate/core'; + +import { CommonPageErrorComponent } from './common-page-error.component'; +import { CommonPageErrorConfig, PAGE_ERROR_TYPE } from './common-page-error.model'; + +describe('CommonPageErrorComponent', () => { + let component: CommonPageErrorComponent; + let fixture: ComponentFixture; + + // With no translations loaded, the translate pipe renders the keys themselves + const config: CommonPageErrorConfig = { + notFoundTitleKey: 'test.not-found.title', + notFoundSubtitleKey: 'test.not-found.subtitle', + loadFailedTitleKey: 'test.load-failed.title', + loadFailedSubtitleKey: 'test.load-failed.subtitle', + backButtonKey: 'test.back-button', + backRoute: '/home', + }; + + const buttons = () => fixture.debugElement.queryAll(By.css('.page-error-actions button')); + const text = () => (fixture.nativeElement as HTMLElement).textContent; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [CommonPageErrorComponent, TranslateModule.forRoot()], + providers: [provideRouter([])], + }).compileComponents(); + + fixture = TestBed.createComponent(CommonPageErrorComponent); + component = fixture.componentInstance; + component.config = config; + }); + + describe('NOT_FOUND variant', () => { + beforeEach(() => { + component.errorType = PAGE_ERROR_TYPE.NOT_FOUND; + fixture.detectChanges(); + }); + + it('should render the not-found title and subtitle with only a back button', () => { + expect(text()).toContain('test.not-found.title'); + expect(text()).toContain('test.not-found.subtitle'); + expect(buttons().length).toBe(1); + expect(buttons()[0].nativeElement.textContent).toContain('test.back-button'); + expect(text()).not.toContain('global.try-again.text'); + }); + + it('should navigate to the configured back route when the back button is clicked', () => { + const router = TestBed.inject(Router); + const navigateSpy = jest.spyOn(router, 'navigateByUrl').mockResolvedValue(true); + + buttons()[0].nativeElement.click(); + + expect(navigateSpy).toHaveBeenCalled(); + expect(router.serializeUrl(navigateSpy.mock.calls[0][0] as never)).toBe('/home'); + }); + + it('should announce the error state to screen readers', () => { + expect(fixture.debugElement.query(By.css('.page-error')).attributes['role']).toBe('alert'); + }); + }); + + describe('LOAD_FAILED variant', () => { + beforeEach(() => { + component.errorType = PAGE_ERROR_TYPE.LOAD_FAILED; + fixture.detectChanges(); + }); + + it('should render the load-failed title and subtitle with try-again and back buttons', () => { + expect(text()).toContain('test.load-failed.title'); + expect(text()).toContain('test.load-failed.subtitle'); + expect(buttons().length).toBe(2); + expect(buttons()[0].nativeElement.textContent).toContain('global.try-again.text'); + expect(buttons()[1].nativeElement.textContent).toContain('test.back-button'); + }); + + it('should emit retry when the try-again button is clicked', () => { + const retrySpy = jest.fn(); + component.retry.subscribe(retrySpy); + + buttons()[0].nativeElement.click(); + + expect(retrySpy).toHaveBeenCalledTimes(1); + }); + + it('should navigate to the configured back route when the back button is clicked', () => { + const router = TestBed.inject(Router); + const navigateSpy = jest.spyOn(router, 'navigateByUrl').mockResolvedValue(true); + + buttons()[1].nativeElement.click(); + + expect(navigateSpy).toHaveBeenCalled(); + expect(router.serializeUrl(navigateSpy.mock.calls[0][0] as never)).toBe('/home'); + }); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.ts new file mode 100644 index 0000000000..51575c4eb5 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.ts @@ -0,0 +1,51 @@ +import { ChangeDetectionStrategy, Component, EventEmitter, Input, Output } from '@angular/core'; +import { MatButtonModule } from '@angular/material/button'; +import { RouterModule } from '@angular/router'; +import { TranslateModule } from '@ngx-translate/core'; +import { CommonPageErrorConfig, PAGE_ERROR_TYPE } from './common-page-error.model'; + +/** + * Displays a full-page error state for pages whose data failed to load, such as a details page + * for an entity that does not exist (not found) or could not be fetched (load failed). + * + * The `NOT_FOUND` variant shows a "back to list" button. The `LOAD_FAILED` variant additionally + * shows a "Try Again" button that emits the `retry` event so the caller can re-dispatch its fetch. + * + * Example usage: + * + * ```html + * + * ``` + */ +@Component({ + selector: 'app-common-page-error', + imports: [MatButtonModule, RouterModule, TranslateModule], + templateUrl: './common-page-error.component.html', + styleUrl: './common-page-error.component.scss', + changeDetection: ChangeDetectionStrategy.OnPush, +}) +export class CommonPageErrorComponent { + @Input() errorType: PAGE_ERROR_TYPE = PAGE_ERROR_TYPE.NOT_FOUND; + @Input() config!: CommonPageErrorConfig; + @Output() retry = new EventEmitter(); + + get isNotFound(): boolean { + return this.errorType === PAGE_ERROR_TYPE.NOT_FOUND; + } + + get icon(): string { + return this.isNotFound ? 'search_off' : 'error'; + } + + get titleKey(): string { + return this.isNotFound ? this.config.notFoundTitleKey : this.config.loadFailedTitleKey; + } + + get subtitleKey(): string { + return this.isNotFound ? this.config.notFoundSubtitleKey : this.config.loadFailedSubtitleKey; + } +} diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.spec.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.spec.ts new file mode 100644 index 0000000000..4fcd8755a7 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.spec.ts @@ -0,0 +1,18 @@ +import { isCanonicalEntityId } from './common-page-error.model'; + +describe('isCanonicalEntityId', () => { + it.each<[string, string, boolean]>([ + ['a lowercase v4 UUID', '2382605e-1dd0-43fa-bbfd-59e3a460efa6', true], + ['a lowercase v1 UUID', '2382605e-1dd0-13fa-bbfd-59e3a460efa6', true], + ['the nil UUID', '00000000-0000-0000-0000-000000000000', true], + ['the max UUID', 'ffffffff-ffff-ffff-ffff-ffffffffffff', true], + ['an uppercase UUID (the app only generates lowercase URLs)', '2382605E-1DD0-43FA-BBFD-59E3A460EFA6', false], + ['a UUID with an invalid version nibble', '11111111-1111-0111-8111-111111111111', false], + ['a UUID with an invalid variant nibble', '11111111-1111-4111-0111-111111111111', false], + ['a UUID with a trailing character', '2382605e-1dd0-43fa-bbfd-59e3a460efa62', false], + ['a non-UUID string', 'not-a-uuid', false], + ['an empty string', '', false], + ])('should treat %s as canonical: %s', (label, id, expected) => { + expect(isCanonicalEntityId(id)).toBe(expected); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.ts new file mode 100644 index 0000000000..0da36f3c43 --- /dev/null +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.ts @@ -0,0 +1,31 @@ +export enum PAGE_ERROR_TYPE { + NOT_FOUND = 'notFound', + LOAD_FAILED = 'loadFailed', +} + +export interface DetailsPageError { + entityId: string; + errorType: PAGE_ERROR_TYPE; +} + +export interface CommonPageErrorConfig { + notFoundTitleKey: string; + notFoundSubtitleKey: string; + loadFailedTitleKey: string; + loadFailedSubtitleKey: string; + backButtonKey: string; + backRoute: string; +} + +// The canonical form of an entity id in this app: the lowercase subset of what the +// backend's @IsUUID() accepts (UUID versions 1-8 plus the nil/max UUIDs). Lowercase-only +// on purpose - the app always generates lowercase UUID URLs, and the selectors compare +// the route param against entity ids case-sensitively. +const CANONICAL_UUID_PATTERN = + /^(?:[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}|00000000-0000-0000-0000-000000000000|ffffffff-ffff-ffff-ffff-ffffffffffff)$/; + +// Detail routes intentionally support canonical lowercase ids only; other forms are +// treated as NOT_FOUND without a network call to keep route/store comparisons deterministic. +export function isCanonicalEntityId(id: string): boolean { + return CANONICAL_UUID_PATTERN.test(id); +} diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/index.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/index.ts index d7b00606d3..6fbda8ec64 100644 --- a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/index.ts +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/index.ts @@ -16,6 +16,7 @@ import { CommonLearnMoreLinkComponent } from './common-learn-more-link/common-le import { CommonSimpleTextValidatedConfirmationModalComponent } from './common-simple-text-validated-confirmation-modal/common-simple-text-validated-confirmation-modal.component'; import { CommonAuditLogTimelineComponent } from './common-audit-log-timeline/common-audit-log-timeline.component'; import { CommonAuditLogDiffDisplayComponent } from './common-audit-log-timeline/common-audit-log-diff-display/common-audit-log-diff-display.component'; +import { CommonPageErrorComponent } from './common-page-error/common-page-error.component'; export { CommonPageComponent, @@ -36,4 +37,5 @@ export { CommonSimpleTextValidatedConfirmationModalComponent, CommonAuditLogTimelineComponent, CommonAuditLogDiffDisplayComponent, + CommonPageErrorComponent, }; diff --git a/packages/frontend/projects/upgrade/src/assets/i18n/en.json b/packages/frontend/projects/upgrade/src/assets/i18n/en.json index 4b015c586d..c54723175d 100644 --- a/packages/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/packages/frontend/projects/upgrade/src/assets/i18n/en.json @@ -24,6 +24,7 @@ "global.delete-input-comparison.text": "delete", "global.delete.input-placeholder.text": "Type delete", "global.update.text": "UPDATE", + "global.try-again.text": "Try Again", "global.import.text": "IMPORT", "global.group-custom-type.placeHolder": "Custom Group", "global.number.text": "NO.", @@ -611,6 +612,11 @@ "experiments.edit-condition-prior-modal.required-error.text": "Enter value", "experiments.edit-condition-prior-modal.no-data.text": "No conditions available.", "experiments.edit-condition-prior-modal.hint.text": "* Prior successes/failures will be added to the reward counts for each condition to incorporate prior knowledge or beliefs about the conditions. The default of 1 is the minimum starting value. When values are all equal, there is no initial bias.", + "experiments.details-page-error.not-found.title.text": "Experiment not found", + "experiments.details-page-error.not-found.subtitle.text": "The experiment you're looking for doesn't exist. It may have been deleted, or the link may be incorrect.", + "experiments.details-page-error.load-failed.title.text": "Something went wrong", + "experiments.details-page-error.load-failed.subtitle.text": "An unexpected error occurred while loading this experiment.", + "experiments.details-page-error.back-button.text": "Back to Experiments", "feature-flags.global-name.text": "Name", "feature-flags.global-status.text": "Status", "feature-flags.global-updated-at.text": "Updated at", @@ -719,6 +725,11 @@ "feature-flags.upsert-list-modal.import-csv.error.message.text": "Import failed. Please check your file and try again.", "feature-flags.upsert-include-list-modal.name-hint.text": "The name for this include list.", "feature-flags.upsert-exclude-list-modal.name-hint.text": "The name for this exclude list.", + "feature-flags.details-page-error.not-found.title.text": "Feature flag not found", + "feature-flags.details-page-error.not-found.subtitle.text": "The feature flag you're looking for doesn't exist. It may have been deleted, or the link may be incorrect.", + "feature-flags.details-page-error.load-failed.title.text": "Something went wrong", + "feature-flags.details-page-error.load-failed.subtitle.text": "An unexpected error occurred while loading this feature flag.", + "feature-flags.details-page-error.back-button.text": "Back to Feature Flags", "segments.title.text": "Segments", "segments.subtitle.text": "Define new segments to include or exclude from any experiment", "segments.no-segments.text": "Welcome!
Let's start by creating a new segment!", @@ -832,6 +843,11 @@ "segments.upsert-segment-modal.tags-placeholder.text": "Tags separated by commas", "segments.delete-modal.warning-message.text": "Are you sure you want to delete ", "segments.errors.duplicate-name.text": "A segment with name \"{{name}}\" already exists on context \"{{context}}\".", + "segments.details-page-error.not-found.title.text": "Segment not found", + "segments.details-page-error.not-found.subtitle.text": "The segment you're looking for doesn't exist. It may have been deleted, or the link may be incorrect.", + "segments.details-page-error.load-failed.title.text": "Something went wrong", + "segments.details-page-error.load-failed.subtitle.text": "An unexpected error occurred while loading this segment.", + "segments.details-page-error.back-button.text": "Back to Segments", "stratifications.global-members-factor.text": "FACTOR", "stratifications.global-members-summary.text": "SUMMARY", "stratifications.global-members-status.text": "STATUS",