From 51c32b3affdf7a98c9f18a4961320069148a20a1 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 28 Aug 2026 17:13:20 -0400 Subject: [PATCH 01/10] Add proper not-found and error screens to experiment, feature flag, and segment detail pages --- packages/frontend/jest.config.js | 2 + .../experiments.data.service.spec.ts | 9 ++-- .../experiments/experiments.data.service.ts | 6 ++- .../core/experiments/experiments.service.ts | 2 + .../experiments/store/experiments.actions.ts | 6 ++- .../store/experiments.effects.spec.ts | 3 +- .../experiments/store/experiments.effects.ts | 26 +++++++--- .../experiments/store/experiments.model.ts | 3 ++ .../store/experiments.reducer.spec.ts | 20 +++++++- .../experiments/store/experiments.reducer.ts | 8 ++- .../store/experiments.selectors.ts | 13 +++++ .../feature-flags.data.service.ts | 6 ++- .../feature-flags/feature-flags.service.ts | 2 + .../store/feature-flags.actions.ts | 6 ++- .../store/feature-flags.effects.ts | 31 +++++++++-- .../store/feature-flags.model.ts | 3 ++ .../store/feature-flags.reducer.ts | 5 +- .../store/feature-flags.selectors.ts | 13 +++++ .../http-interceptors/http-context-tokens.ts | 7 +++ .../http-error.interceptor.ts | 3 +- .../local-storage.service.spec.ts | 2 + .../local-storage/local-storage.service.ts | 3 ++ .../core/segments/segments.data.service.ts | 6 ++- .../src/app/core/segments/segments.service.ts | 2 + .../core/segments/store/segments.actions.ts | 6 ++- .../core/segments/store/segments.effects.ts | 19 +++++-- .../app/core/segments/store/segments.model.ts | 3 ++ .../segments/store/segments.reducer.spec.ts | 30 ++++++++++- .../core/segments/store/segments.reducer.ts | 8 ++- .../core/segments/store/segments.selectors.ts | 13 +++++ .../dashboard/dashboard-routing.module.ts | 4 ++ ...riment-details-page-content.component.html | 10 +++- ...periment-details-page-content.component.ts | 22 +++++++- ...e-flag-details-page-content.component.html | 10 +++- ...ure-flag-details-page-content.component.ts | 22 +++++++- .../dashboard/require-route-param.guard.ts | 14 +++++ ...egment-details-page-content.component.html | 10 +++- .../segment-details-page-content.component.ts | 22 +++++++- .../common-page-error.component.html | 21 ++++++++ .../common-page-error.component.scss | 40 +++++++++++++++ .../common-page-error.component.ts | 51 +++++++++++++++++++ .../common-page-error.model.ts | 28 ++++++++++ .../components/index.ts | 2 + .../projects/upgrade/src/assets/i18n/en.json | 16 ++++++ 44 files changed, 496 insertions(+), 42 deletions(-) create mode 100644 packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-context-tokens.ts create mode 100644 packages/frontend/projects/upgrade/src/app/features/dashboard/require-route-param.guard.ts create mode 100644 packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.html create mode 100644 packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.scss create mode 100644 packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.component.ts create mode 100644 packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.ts diff --git a/packages/frontend/jest.config.js b/packages/frontend/jest.config.js index c581bd2b1e..76c1490213 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 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..333fbb833a 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,15 @@ describe('ExperimentDataService', () => { }); describe('#getExperimentById', () => { - it('should get the getExperimentById http observable', () => { + it('should get the getExperimentById http observable with contextual 404 handling', () => { 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(true); }); }); 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..ac613a1e43 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'; @@ -84,7 +85,8 @@ export class ExperimentDataService { getExperimentById(experimentId: string) { const url = `${API_ENDPOINTS.getExperimentById}/${experimentId}`; - 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) }); } fetchAllPartitions() { 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..2e6eea70f2 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)); 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..c6713c9cc6 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, @@ -56,7 +57,10 @@ 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', + props<{ experimentId: string; errorType: PAGE_ERROR_TYPE }>() +); 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..0248631b57 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 @@ -596,7 +596,8 @@ describe('ExperimentEffects', () => { }); describe('#getExperimentById$', () => { - const experimentId = 'testId'; + // Must be a valid UUID - the effect short-circuits malformed ids to a not-found failure + const experimentId = '11111111-2222-4333-8444-555555555555'; const experiment = { id: 'test1', stat: [ 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..ec0cf43a8d 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 @@ -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 { isValidEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; @Injectable() export class ExperimentEffects { constructor( @@ -305,8 +306,13 @@ export class ExperimentEffects { map((action) => action.experimentId), filter((experimentId) => !!experimentId), withLatestFrom(this.store$.pipe(select(selectExperimentStats))), - mergeMap(([experimentId, experimentStats]) => - this.experimentDataService.getExperimentById(experimentId).pipe( + mergeMap(([experimentId, experimentStats]) => { + if (!isValidEntityId(experimentId)) { + return of( + experimentAction.actionGetExperimentByIdFailure({ experimentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND }) + ); + } + return this.experimentDataService.getExperimentById(experimentId).pipe( switchMap((data: Experiment) => this.experimentDataService.getAllExperimentsStats([data.id]).pipe( switchMap((stat: IExperimentEnrollmentStats) => { @@ -315,11 +321,19 @@ 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, + }), + ]) + ); + }) ) ); 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..b3a767a2a9 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,30 @@ 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', () => { const previousState = { ...initialState }; previousState.isLoadingExperiment = true; - const testAction: Action = actionGetExperimentByIdFailure(); + const testAction: Action = actionGetExperimentByIdFailure({ + experimentId: 'abc123', + errorType: PAGE_ERROR_TYPE.NOT_FOUND, + }); 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 "actionGetExperimentById" should clear the details page error', () => { + const previousState = { ...initialState }; + previousState.detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }; + const testAction: Action = actionGetExperimentById({ experimentId: 'abc123' }); + + const newState = experimentsReducer(previousState, testAction); + + expect(newState).not.toBe(previousState); + expect(newState.detailsPageError).toBeNull(); }); 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..e371a465bb 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,16 @@ const reducer = createReducer( }), on( experimentsAction.actionGetExperimentsFailure, - experimentsAction.actionGetExperimentByIdFailure, experimentsAction.actionUpsertExperimentFailure, experimentsAction.actionUpdateExperimentFilterModeFailure, experimentsAction.actionUpdateExperimentStateFailure, (state) => ({ ...state, isLoadingExperiment: false }) ), + on(experimentsAction.actionGetExperimentByIdFailure, (state, { experimentId, errorType }) => ({ + ...state, + isLoadingExperiment: false, + detailsPageError: { entityId: experimentId, errorType }, + })), 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)); @@ -95,6 +100,7 @@ const reducer = createReducer( on(experimentsAction.actionUpsertExperiment, experimentsAction.actionGetExperimentById, (state) => ({ ...state, isLoadingExperiment: true, + detailsPageError: null, // 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, 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.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.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts index 41d2304efe..2e0f1af171 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 { isValidEntityId, 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 (!isValidEntityId(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.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts index 0794e7ff10..25e6d9a0c0 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, @@ -73,15 +74,17 @@ const reducer = createReducer( on(FeatureFlagsActions.actionFetchFeatureFlagById, (state) => ({ ...state, isLoadingSelectedFeatureFlag: true, + detailsPageError: null, })), on(FeatureFlagsActions.actionFetchFeatureFlagByIdSuccess, (state, { flag }) => ({ ...state, selectedFlag: flag, isLoadingSelectedFeatureFlag: false, })), - 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.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.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.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.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts index 35aff13c1d..03787c50df 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 { isValidEntityId, 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 (!isValidEntityId(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..726f8e17cf 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,38 @@ 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 clear the details page error', () => { + const previousState = { ...initialState }; + previousState.detailsPageError = { entityId: 'abc123', errorType: PAGE_ERROR_TYPE.NOT_FOUND }; + const testAction = SegmentsActions.actionGetSegmentById({ segmentId: 'abc123' }); + + const newState = segmentsReducer(previousState, testAction); + + expect(newState.detailsPageError).toBeNull(); + }); + }); + 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..72e7091068 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( @@ -32,6 +33,7 @@ const reducer = createReducer( on(SegmentsActions.actionUpsertSegment, SegmentsActions.actionGetSegmentById, (state) => ({ ...state, isLoadingSegments: true, + detailsPageError: null, })), on( SegmentsActions.actionFetchSegmentsSuccess, @@ -88,11 +90,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, 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..1fcdbe1229 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,6 +1,7 @@ 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 = [ @@ -14,6 +15,7 @@ const segmentsRoutes = [ }, { path: 'segments/detail/:segmentId', + canActivate: [requireRouteParam('segmentId', '/segments')], loadComponent: () => import('./segments/pages/segment-details-page/segment-details-page.component').then( (c) => c.SegmentDetailsPageComponent @@ -46,6 +48,7 @@ const routes: Routes = [ }, { path: 'home/detail/:experimentId', + canActivate: [requireRouteParam('experimentId', '/home')], loadComponent: () => import('./experiments/pages/experiment-details-page/experiment-details-page.component').then( (c) => c.ExperimentDetailsPageComponent @@ -85,6 +88,7 @@ const routes: Routes = [ }, { 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/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, @@ -83,6 +98,7 @@ export class ExperimentDetailsPageContentComponent implements OnInit, OnDestroy }); 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); + } + ngOnDestroy() { this.experimentIdSub.unsubscribe(); } 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/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/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..f2817abc7a --- /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 @@ +
+ +

{{ titleKey | translate }}

+

{{ subtitleKey | translate }}

+
+ @if (isNotFound) { + + } @else { + + + } +
+
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.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.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..d10cda3ba1 --- /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,28 @@ +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; +} + +// Lowercase-only on purpose: the app always generates lowercase UUIDs, and the selectors +// compare the route param against entity ids case-sensitively. +const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/; + +// A malformed id in the URL can never identify an entity, so details pages treat it as +// NOT_FOUND without a network call (the backend would reject it with 400, not 404). +export function isValidEntityId(id: string): boolean { + return 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", From 8b88a4522cb9e3a61caca729c5cee27d1cb2a799 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 28 Aug 2026 20:01:33 -0400 Subject: [PATCH 02/10] Align the entity id check with the backend's UUID validation and add tests for the details-page error branches --- .../store/experiments.effects.spec.ts | 56 +++++++- .../experiments/store/experiments.effects.ts | 4 +- .../store/experiments.selector.spec.ts | 31 +++++ .../store/feature-flags.effects.spec.ts | 124 ++++++++++++++++++ .../store/feature-flags.effects.ts | 4 +- .../store/feature-flags.selectors.spec.ts | 34 +++++ .../http-error.interceptor.spec.ts | 63 +++++++++ .../segments/store/segments.effects.spec.ts | 46 +++++++ .../core/segments/store/segments.effects.ts | 4 +- .../segments/store/segments.selectors.spec.ts | 33 +++++ .../common-page-error.model.spec.ts | 18 +++ .../common-page-error.model.ts | 17 ++- 12 files changed, 420 insertions(+), 14 deletions(-) create mode 100644 packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.spec.ts create mode 100644 packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.spec.ts create mode 100644 packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-page-error/common-page-error.model.spec.ts 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 0248631b57..ce5284b59e 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 @@ -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,7 @@ describe('ExperimentEffects', () => { }); describe('#getExperimentById$', () => { - // Must be a valid UUID - the effect short-circuits malformed ids to a not-found failure + // 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', @@ -643,6 +645,58 @@ 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 })); + })); }); 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 ec0cf43a8d..34cdda3668 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 @@ -34,7 +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 { isValidEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; +import { isCanonicalEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; @Injectable() export class ExperimentEffects { constructor( @@ -307,7 +307,7 @@ export class ExperimentEffects { filter((experimentId) => !!experimentId), withLatestFrom(this.store$.pipe(select(selectExperimentStats))), mergeMap(([experimentId, experimentStats]) => { - if (!isValidEntityId(experimentId)) { + if (!isCanonicalEntityId(experimentId)) { return of( experimentAction.actionGetExperimentByIdFailure({ experimentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND }) ); 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/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 2e0f1af171..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,7 +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 { isValidEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; +import { isCanonicalEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; @Injectable() export class FeatureFlagsEffects { @@ -359,7 +359,7 @@ export class FeatureFlagsEffects { map((action) => action.featureFlagId), filter((featureFlagId) => !!featureFlagId), switchMap((featureFlagId) => { - if (!isValidEntityId(featureFlagId)) { + if (!isCanonicalEntityId(featureFlagId)) { return of( FeatureFlagsActions.actionFetchFeatureFlagByIdFailure({ featureFlagId, 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/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/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 03787c50df..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,7 +16,7 @@ import { } from './segments.selectors'; import JSZip from 'jszip'; import { of } from 'rxjs'; -import { isValidEntityId, PAGE_ERROR_TYPE } from '@shared-component-lib/common-page-error/common-page-error.model'; +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'; @@ -135,7 +135,7 @@ export class SegmentsEffects { map((action) => action.segmentId), filter((segmentId) => !!segmentId), switchMap((segmentId) => { - if (!isValidEntityId(segmentId)) { + if (!isCanonicalEntityId(segmentId)) { return of(SegmentsActions.actionGetSegmentByIdFailure({ segmentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND })); } return this.segmentsDataService.getSegmentById(segmentId).pipe( 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/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 index d10cda3ba1..0da36f3c43 100644 --- 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 @@ -17,12 +17,15 @@ export interface CommonPageErrorConfig { backRoute: string; } -// Lowercase-only on purpose: the app always generates lowercase UUIDs, and the selectors -// compare the route param against entity ids case-sensitively. -const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/; +// 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)$/; -// A malformed id in the URL can never identify an entity, so details pages treat it as -// NOT_FOUND without a network call (the backend would reject it with 400, not 404). -export function isValidEntityId(id: string): boolean { - return UUID_PATTERN.test(id); +// 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); } From ccf8be465e5ad486dd0e55c3c6dc8f58aadbf2d2 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 28 Aug 2026 21:40:25 -0400 Subject: [PATCH 03/10] Cancel superseded same-id experiment fetches so a stale failure can't replace a newer success --- .../store/experiments.effects.spec.ts | 47 ++++++++++++++++++- .../experiments/store/experiments.effects.ts | 21 +++++++-- 2 files changed, 61 insertions(+), 7 deletions(-) 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 ce5284b59e..8453720b68 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, @@ -697,6 +697,49 @@ describe('ExperimentEffects', () => { 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 keep concurrent fetches for different experiment ids', fakeAsync(() => { + // preview-user loads several different experiments at once - 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 34cdda3668..c9f5887f3f 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, @@ -303,10 +303,10 @@ 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]) => { + mergeMap(([action, experimentStats]) => { + const { experimentId } = action; if (!isCanonicalEntityId(experimentId)) { return of( experimentAction.actionGetExperimentByIdFailure({ experimentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND }) @@ -331,7 +331,18 @@ export class ExperimentEffects { experimentId, errorType: error?.status === 404 ? PAGE_ERROR_TYPE.NOT_FOUND : PAGE_ERROR_TYPE.LOAD_FAILED, }), - ]) + ]), + // A newer fetch for the same experiment supersedes this one - cancel it so a late + // failure can't set detailsPageError after the newer request has succeeded. + // mergeMap is kept because preview-user loads several different experiments concurrently, + // and 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) + ) + ) ); }) ) From 80a6e405dab6450a20a5e44f606bf312607c2afc Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Sat, 29 Aug 2026 13:13:16 -0400 Subject: [PATCH 04/10] Redirect id-less detail URLs to their list pages, keep the 404 toast for non-details fetches, and announce the error state to screen readers --- .../experiments.data.service.spec.ts | 15 +++++++++++++-- .../experiments/experiments.data.service.ts | 7 ++++--- .../experiments/experiments.service.spec.ts | 16 +++++++++++++++- .../core/experiments/experiments.service.ts | 7 ++++--- .../experiments/store/experiments.actions.ts | 4 +++- .../experiments/store/experiments.effects.ts | 2 +- .../dashboard/dashboard-routing.module.ts | 18 +++++++++++++++++- ...xperiment-details-page-content.component.ts | 4 ++-- .../common-page-error.component.html | 2 +- 9 files changed, 60 insertions(+), 15 deletions(-) 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 333fbb833a..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 @@ -241,16 +241,27 @@ describe('ExperimentDataService', () => { }); describe('#getExperimentById', () => { - it('should get the getExperimentById http observable with contextual 404 handling', () => { + 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); + 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, { 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); + }); }); describe('#fetchAllPartitions', () => { 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 ac613a1e43..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 @@ -83,10 +83,11 @@ export class ExperimentDataService { return this.http.delete(url); } - getExperimentById(experimentId: string) { + getExperimentById(experimentId: string, handles404Contextually = false) { const url = `${API_ENDPOINTS.getExperimentById}/${experimentId}`; - // 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) }); + // 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 2e6eea70f2..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 @@ -155,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 c6713c9cc6..5a586e9a5c 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 @@ -49,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( 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 c9f5887f3f..2376f95531 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 @@ -312,7 +312,7 @@ export class ExperimentEffects { experimentAction.actionGetExperimentByIdFailure({ experimentId, errorType: PAGE_ERROR_TYPE.NOT_FOUND }) ); } - return this.experimentDataService.getExperimentById(experimentId).pipe( + return this.experimentDataService.getExperimentById(experimentId, action.handles404Contextually ?? false).pipe( switchMap((data: Experiment) => this.experimentDataService.getAllExperimentsStats([data.id]).pipe( switchMap((stat: IExperimentEnrollmentStats) => { 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 1fcdbe1229..8b3cf331b2 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 @@ -4,7 +4,7 @@ import { DashboardRootComponent } from './dashboard-root/dashboard-root.componen import { requireRouteParam } from './require-route-param.guard'; // Conditionally define segments routes based on the toggle -const segmentsRoutes = [ +const segmentsRoutes: Routes = [ { path: 'segments', loadComponent: () => @@ -13,6 +13,12 @@ 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')], @@ -46,6 +52,11 @@ const routes: Routes = [ title: 'app-header.title.experiments', }, }, + { + path: 'home/detail', + redirectTo: '/home', + pathMatch: 'full', + }, { path: 'home/detail/:experimentId', canActivate: [requireRouteParam('experimentId', '/home')], @@ -86,6 +97,11 @@ const routes: Routes = [ title: 'app-header.title.feature-flag', }, }, + { + path: 'featureflags/detail', + redirectTo: '/featureflags', + pathMatch: 'full', + }, { path: 'featureflags/detail/:flagId', canActivate: [requireRouteParam('flagId', '/featureflags')], 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.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.ts index ca49d91669..b15991aeeb 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.ts @@ -94,7 +94,7 @@ 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$; @@ -124,7 +124,7 @@ export class ExperimentDetailsPageContentComponent implements OnInit, OnDestroy } onRetry(experimentId: string): void { - this.experimentsService.fetchExperimentById(experimentId); + this.experimentsService.fetchExperimentById(experimentId, true); } ngOnDestroy() { 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 index f2817abc7a..94ab18cb37 100644 --- 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 @@ -1,4 +1,4 @@ -
+