From 3e3eb37300ec48a3c6b0d5294c580490577397f5 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 21 Nov 2025 14:22:38 -0500 Subject: [PATCH 01/30] Implement Upsert Metric Modal for Experiments v2 --- .../experiments/experiments.data.service.ts | 9 + .../core/experiments/experiments.service.ts | 5 + .../experiments/metric-helper.service.ts.ts | 66 ++ .../experiments/store/experiments.actions.ts | 15 + .../experiments/store/experiments.effects.ts | 18 + .../experiments/store/experiments.model.ts | 34 + .../experiments/store/experiments.reducer.ts | 8 + .../upsert-metric-modal.component.html | 218 +++++ .../upsert-metric-modal.component.scss | 8 + .../upsert-metric-modal.component.ts | 773 ++++++++++++++++++ ...periment-metrics-section-card.component.ts | 44 +- .../shared/services/common-dialog.service.ts | 64 +- .../projects/upgrade/src/assets/i18n/en.json | 9 + types/src/Experiment/enums.ts | 5 + types/src/index.ts | 1 + 15 files changed, 1265 insertions(+), 12 deletions(-) create mode 100644 frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts.ts create mode 100644 frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html create mode 100644 frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.scss create mode 100644 frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts diff --git a/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts b/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts index e02dad4819..f72bc6b57c 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/experiments.data.service.ts @@ -5,6 +5,7 @@ import { ExperimentPaginationParams, UpdateExperimentFilterModeRequest, UpdateExperimentDecisionPointsRequest, + UpdateExperimentMetricsRequest, ExperimentSegmentListResponse, UpdateExperimentConditionsRequest, } from './store/experiments.model'; @@ -196,4 +197,12 @@ export class ExperimentDataService { const url = `${this.environment.api.exportAllExperimentIncludeLists}/${id}`; return this.http.get(url); } + + updateExperimentMetrics(params: UpdateExperimentMetricsRequest): Observable { + const updatedExperiment = { + ...params.experiment, + queries: params.metrics, + }; + return this.updateExperiment(updatedExperiment); + } } diff --git a/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts b/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts index daeed07ba9..a9d9d62222 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts @@ -15,6 +15,7 @@ import { UpdateExperimentFilterModeRequest, UpdateExperimentDecisionPointsRequest, UpdateExperimentConditionsRequest, + UpdateExperimentMetricsRequest, } from './store/experiments.model'; import { Store, select } from '@ngrx/store'; import { @@ -208,6 +209,10 @@ export class ExperimentService { this.store$.dispatch(experimentAction.actionUpdateExperimentConditions({ updateExperimentConditionsRequest })); } + updateExperimentMetrics(updateExperimentMetricsRequest: UpdateExperimentMetricsRequest) { + this.store$.dispatch(experimentAction.actionUpdateExperimentMetrics({ updateExperimentMetricsRequest })); + } + fetchContextMetaData() { this.store$.dispatch(experimentAction.actionFetchContextMetaData({ isLoadingContextMetaData: true })); } diff --git a/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts.ts b/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts.ts new file mode 100644 index 0000000000..fe812f7d29 --- /dev/null +++ b/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts.ts @@ -0,0 +1,66 @@ +import { Injectable } from '@angular/core'; +import { v4 as uuidv4 } from 'uuid'; + +import { ExperimentService } from './experiments.service'; +import { Experiment, ExperimentQueryDTO, UpdateExperimentMetricsRequest } from './store/experiments.model'; + +@Injectable({ + providedIn: 'root', +}) +export class MetricHelperService { + constructor(private experimentService: ExperimentService) {} + + /** + * Add a new metric to an experiment + */ + addMetric(experiment: Experiment, metricData: ExperimentQueryDTO): void { + const currentMetrics = [...(experiment.queries || [])]; + const newMetric = { + ...metricData, + id: uuidv4(), + }; + + const updatedMetrics = [...currentMetrics, newMetric] as ExperimentQueryDTO[]; + + this.updateExperimentMetrics(experiment, updatedMetrics); + } + + /** + * Update an existing metric in an experiment + */ + updateMetric(experiment: Experiment, sourceMetric: ExperimentQueryDTO, metricData: ExperimentQueryDTO): void { + const currentMetrics = [...(experiment.queries || [])]; + const updatedMetrics = currentMetrics.map((metric) => + metric.id === sourceMetric.id + ? { + ...metric, + ...metricData, + } + : metric + ); + + this.updateExperimentMetrics(experiment, updatedMetrics); + } + + /** + * Delete a metric from an experiment + */ + deleteMetric(experiment: Experiment, metricToDelete: ExperimentQueryDTO): void { + const currentMetrics = [...(experiment.queries || [])]; + const updatedMetrics = currentMetrics.filter((metric) => metric.id !== metricToDelete.id); + + this.updateExperimentMetrics(experiment, updatedMetrics); + } + + /** + * Common method to update experiment metrics + */ + private updateExperimentMetrics(experiment: Experiment, updatedMetrics: ExperimentQueryDTO[]): void { + const updateRequest: UpdateExperimentMetricsRequest = { + experiment, + metrics: updatedMetrics, + }; + + this.experimentService.updateExperimentMetrics(updateRequest); + } +} \ No newline at end of file diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts index 3ccc4b69f6..f2bb00d4b2 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts @@ -13,6 +13,7 @@ import { IContextMetaData, UpdateExperimentFilterModeRequest, UpdateExperimentDecisionPointsRequest, + UpdateExperimentMetricsRequest, ExperimentSegmentListResponse, UpdateExperimentConditionsRequest, } from './experiments.model'; @@ -407,3 +408,17 @@ export const actionExportAllIncludeListsDesignSuccess = createAction( export const actionExportAllIncludeListsDesignFailure = createAction( '[Experiment] Export All Include Lists Design Failure' ); + +export const actionUpdateExperimentMetrics = createAction( + '[Experiment] Update Experiment Metrics', + props<{ updateExperimentMetricsRequest: UpdateExperimentMetricsRequest }>() +); + +export const actionUpdateExperimentMetricsSuccess = createAction( + '[Experiment] Update Experiment Metrics Success', + props<{ experiment: Experiment }>() +); + +export const actionUpdateExperimentMetricsFailure = createAction( + '[Experiment] Update Experiment Metrics Failure' +); diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts index ba4e65c74b..c3033a2a31 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts @@ -273,6 +273,24 @@ export class ExperimentEffects { ) ); + updateExperimentMetrics$ = createEffect(() => + this.actions$.pipe( + ofType(experimentAction.actionUpdateExperimentMetrics), + switchMap((action) => { + return this.experimentDataService.updateExperimentMetrics(action.updateExperimentMetricsRequest).pipe( + map((experiment) => { + this.notificationService.showSuccess(this.translate.instant('experiments.metrics.update-success.text')); + return experimentAction.actionUpdateExperimentMetricsSuccess({ experiment }); + }), + catchError(() => { + this.notificationService.showError(this.translate.instant('experiments.metrics.update-error.text')); + return [experimentAction.actionUpdateExperimentMetricsFailure()]; + }) + ); + }) + ) + ); + deleteExperiment$ = createEffect(() => this.actions$.pipe( ofType(experimentAction.actionDeleteExperiment), diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index c427368a15..dcb316ee17 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts @@ -24,6 +24,7 @@ import { REPEATED_MEASURE, SEGMENT_TYPE, IEnrollmentCompleteCondition, + METRIC_TYPE, } from 'upgrade_types'; import { Segment } from '../../segments/store/segments.model'; @@ -41,6 +42,7 @@ export { IExperimentSortParams, IExperimentEnrollmentDetailStats, DATE_RANGE, + METRIC_TYPE, }; export interface ExperimentConditionFilterOptions { @@ -367,6 +369,20 @@ export interface ConditionFormData { description: string; } +export interface MetricFormData { + metricType: METRIC_TYPE; + metricId: string; + displayName: string; + description?: string; + metricClass?: string; // For repeatable metrics only + metricKey?: string; // For repeatable metrics only + aggregateStatistic?: string; + individualStatistic?: string; // For repeatable metrics only + comparison?: string; + compareValue?: string; + allowableDataKeys?: string[]; // For categorical metrics only +} + // Base interfaces matching backend DTO structure export interface ExperimentConditionDTO { id: string; @@ -511,6 +527,11 @@ export interface UpdateExperimentConditionsRequest { conditions: ExperimentCondition[]; } +export interface UpdateExperimentMetricsRequest { + experiment: Experiment; + metrics: ExperimentQueryDTO[]; +} + export const EXPERIMENT_ROOT_COLUMN_NAMES = { NAME: 'name', STATUS: 'state', @@ -619,6 +640,11 @@ export interface ExperimentPayloadRowActionEvent { payload: ExperimentConditionPayload; } +export interface ExperimentQueryRowActionEvent { + action: EXPERIMENT_ROW_ACTION; + query: ExperimentQueryDTO; +} + export enum EXPERIMENT_PAYLOAD_DISPLAY_TYPE { UNIVERSAL = 'universal', SPECIFIC = 'specific', @@ -640,3 +666,11 @@ export interface RewardMetricData { export interface ExperimentSegmentListResponse extends SegmentNew { experiment: Experiment; } + +export interface UpsertMetricParams { + sourceQuery: ExperimentQueryDTO | null; + action: UPSERT_EXPERIMENT_ACTION; + experimentId: string; + currentContext?: string; + experimentInfo?: ExperimentVM; +} diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts index a5f436cea8..245e3fb4e4 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts @@ -127,6 +127,14 @@ const reducer = createReducer( ...state, isLoadingExperiment: false, })), + on(experimentsAction.actionUpdateExperimentMetrics, (state) => ({ ...state, isLoadingExperiment: true })), + on(experimentsAction.actionUpdateExperimentMetricsSuccess, (state, { experiment }) => + adapter.upsertOne(experiment, { ...state, isLoadingExperiment: false }) + ), + on(experimentsAction.actionUpdateExperimentMetricsFailure, (state) => ({ + ...state, + isLoadingExperiment: false, + })), on(experimentsAction.actionFetchAllDecisionPointsSuccess, (state, { decisionPoints }) => ({ ...state, allDecisionPoints: decisionPoints, diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html new file mode 100644 index 0000000000..3855025e25 --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -0,0 +1,218 @@ + +
+ + +
+ + + +
+ + Global Metric + + + {{ 'experiments.upsert-metric-modal.metric-type-global-metric-description.text' | translate }} + +
+
+ +
+ Repeatable Metric + + {{ 'experiments.upsert-metric-modal.metric-type-repeatable-metric-description.text' | translate }} + +
+
+
+
+ + + + + Metric Class + + + + {{ metricClass.key }} + + + + {{ 'experiments.upsert-metric-modal.metric-class-hint.text' | translate }} + + + + + + + Metric Key + + + + {{ metricKey.key }} + + + + {{ 'experiments.upsert-metric-modal.metric-key-hint.text' | translate }} + + + + + + + Metric ID + + + + {{ metric.key }} + + + + {{ 'experiments.upsert-metric-modal.metric-id-hint.text' | translate }} + + + + + + + Individual Statistic + + + {{ option.label }} + + + + {{ 'experiments.upsert-metric-modal.individual-statistic-hint.text' | translate }} + + Learn more + + + + + + + + + Aggregate Statistic + + + {{ option.label }} + + + + {{ 'experiments.upsert-metric-modal.aggregate-statistic-hint.text' | translate }} + + Learn more + + + + + + + +
+ + + +
+ {{ option.label }} +
+
+
+
+
+ + + + + Value + + + {{ value }} + + + + {{ 'experiments.upsert-metric-modal.value-hint.text' | translate }} + + + + + + + Display Name + + + {{ 'experiments.upsert-metric-modal.display-name-hint.text' | translate }} + + + + + + Description (optional) + + + +
+
\ No newline at end of file diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.scss b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.scss new file mode 100644 index 0000000000..fa641f89cf --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.scss @@ -0,0 +1,8 @@ +.disabled-text { + color: rgba(0, 0, 0, 0.38) !important; +} + +.metric-type-section, +.comparison-section { + padding: 4px 0; +} \ No newline at end of file diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts new file mode 100644 index 0000000000..4da649a894 --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -0,0 +1,773 @@ +import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, OnDestroy, OnInit } from '@angular/core'; +import { FormBuilder, FormGroup, ReactiveFormsModule, Validators } from '@angular/forms'; +import { MAT_DIALOG_DATA, MatDialogRef } from '@angular/material/dialog'; +import { BehaviorSubject, Observable, Subscription, combineLatest } from 'rxjs'; +import { combineLatestWith, map, startWith, take } from 'rxjs/operators'; +import isEqual from 'lodash.isequal'; +import { CommonModule } from '@angular/common'; +import { MatFormFieldModule } from '@angular/material/form-field'; +import { MatInputModule } from '@angular/material/input'; +import { MatRadioModule } from '@angular/material/radio'; +import { MatSelectModule } from '@angular/material/select'; +import { MatChipsModule } from '@angular/material/chips'; +import { MatIconModule } from '@angular/material/icon'; +import { MatAutocompleteModule } from '@angular/material/autocomplete'; +import { TranslateModule } from '@ngx-translate/core'; + +import { CommonModalComponent } from '../../../../../shared-standalone-component-lib/components'; +import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; +import { CommonFormHelpersService } from '../../../../../shared/services/common-form-helpers.service'; +import { + MetricFormData, + UPSERT_EXPERIMENT_ACTION, + UpsertMetricParams, + Experiment, + ExperimentQueryDTO, +} from '../../../../../core/experiments/store/experiments.model'; +import { ExperimentService } from '../../../../../core/experiments/experiments.service'; +import { MetricHelperService } from '../../../../../core/experiments/metric-helper.service'; +import { AnalysisService } from '../../../../../core/analysis/analysis.service'; +import { METRICS_JOIN_TEXT } from '../../../../../core/analysis/store/analysis.models'; +import { ASSIGNMENT_UNIT, IMetricMetaData, METRIC_TYPE, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; + +interface StatisticOption { + value: string; + label: string; +} + +@Component({ + selector: 'upsert-metric-modal', + imports: [ + CommonModalComponent, + MatFormFieldModule, + MatInputModule, + MatRadioModule, + MatSelectModule, + MatChipsModule, + MatIconModule, + MatAutocompleteModule, + CommonModule, + ReactiveFormsModule, + TranslateModule, + ], + templateUrl: './upsert-metric-modal.component.html', + styleUrl: './upsert-metric-modal.component.scss', + changeDetection: ChangeDetectionStrategy.OnPush, +}) +export class UpsertMetricModalComponent implements OnInit, OnDestroy { + isLoadingUpsertMetric$ = this.experimentService.isLoadingExperiment$; + + subscriptions = new Subscription(); + isPrimaryButtonDisabled$: Observable; + isInitialFormValueChanged$: Observable; + + initialFormValues$ = new BehaviorSubject(null); + + metricForm: FormGroup; + showMetricClass = false; + showMetricKey = false; + showAggregateStatistic = false; + showIndividualStatistic = false; + showComparison = false; + metricDataType: IMetricMetaData | null = null; + isGlobalMetricDisabled = false; + + // Dropdown options + aggregateStatisticOptions: StatisticOption[] = []; + individualStatisticOptions: StatisticOption[] = []; + + // Autocomplete + allMetrics$ = this.analysisService.allMetrics$; + allMetrics: any[] = []; + + // Filtered autocomplete observables + filteredMetricClasses$: Observable; + filteredMetricKeys$: Observable; + filteredMetricIds$: Observable; + + // BehaviorSubjects for source data + private metricClassOptions$ = new BehaviorSubject([]); + private metricKeyOptions$ = new BehaviorSubject([]); + private metricIdOptions$ = new BehaviorSubject([]); + + // Current selections + private currentSelectedClass: any = null; + private currentSelectedKey: any = null; + + // Assignment unit and context for filtering + private currentAssignmentUnit: ASSIGNMENT_UNIT | null = null; + private currentContext: string[] | null = null; + + allowableDataKeys: string[] = []; + comparisonOptions = [ + { value: '=', label: 'Equal' }, + { value: '<>', label: 'Not equal' }, + ]; + + continuousAggregateOptions: StatisticOption[] = [ + { value: OPERATION_TYPES.SUM, label: 'Sum' }, + { value: OPERATION_TYPES.MIN, label: 'Min' }, + { value: OPERATION_TYPES.MAX, label: 'Max' }, + { value: OPERATION_TYPES.COUNT, label: 'Count' }, + { value: OPERATION_TYPES.AVERAGE, label: 'Mean' }, + { value: OPERATION_TYPES.MODE, label: 'Mode' }, + { value: OPERATION_TYPES.MEDIAN, label: 'Median' }, + { value: OPERATION_TYPES.STDEV, label: 'Standard Deviation' }, + ]; + + continuousIndividualOptions: StatisticOption[] = [ + { value: REPEATED_MEASURE.mean, label: 'Mean' }, + { value: REPEATED_MEASURE.earliest, label: 'Earliest' }, + { value: REPEATED_MEASURE.mostRecent, label: 'Most Recent' }, + ]; + + categoricalAggregateOptions: StatisticOption[] = [ + { value: OPERATION_TYPES.COUNT, label: 'Count' }, + { value: OPERATION_TYPES.PERCENTAGE, label: 'Percentage' }, + ]; + + categoricalIndividualOptions: StatisticOption[] = [ + { value: REPEATED_MEASURE.earliest, label: 'Earliest' }, + { value: REPEATED_MEASURE.mostRecent, label: 'Most Recent' }, + ]; + + constructor( + @Inject(MAT_DIALOG_DATA) + public config: CommonModalConfig, + private formBuilder: FormBuilder, + private experimentService: ExperimentService, + private metricHelperService: MetricHelperService, + private analysisService: AnalysisService, + private cdr: ChangeDetectorRef, + public dialogRef: MatDialogRef + ) {} + + ngOnInit(): void { + this.createMetricForm(); + this.setupFormChangeListeners(); + this.setupAutocomplete(); + this.setupExperimentContext(); + + // Add listeners AFTER form is fully set up + this.listenForIsInitialFormValueChanged(); + this.listenForPrimaryButtonDisabled(); + } + + ngOnDestroy(): void { + this.subscriptions.unsubscribe(); + } + + createMetricForm(): void { + const { sourceQuery, action } = this.config.params; + const initialValues = this.deriveInitialFormValues(sourceQuery, action); + + this.metricForm = this.formBuilder.group({ + metricType: [initialValues.metricType, Validators.required], + metricId: [initialValues.metricId, Validators.required], + displayName: [initialValues.displayName, Validators.required], + description: [initialValues.description], + metricClass: [initialValues.metricClass], + metricKey: [initialValues.metricKey], + aggregateStatistic: [initialValues.aggregateStatistic], + individualStatistic: [initialValues.individualStatistic], + comparison: [initialValues.comparison || '='], + compareValue: [initialValues.compareValue], + }); + + this.allowableDataKeys = initialValues.allowableDataKeys || []; + this.initialFormValues$.next(initialValues); + + // Set initial form visibility states - detect metric data type first if we have a metric ID + if (initialValues.metricId) { + this.detectMetricDataType(initialValues.metricId); + this.updateStatisticOptions(); + } + this.updateFormVisibility(); + this.updateMetricTypeAvailability(); + + // For edit mode, populate form after allMetrics are loaded + if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { + this.populateFormForEditMode(initialValues); + } + } + + deriveInitialFormValues(sourceQuery: any, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { + if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { + const metricKey = sourceQuery.metric?.key || ''; + + // The correct way to determine if it's repeatable is by checking if the metric key contains METRICS_JOIN_TEXT + // NOT by checking if repeatedMeasure exists (global metrics can also have individual statistics) + const isRepeatable = metricKey.includes(METRICS_JOIN_TEXT); + + let metricClass = ''; + let metricKeyValue = ''; + let metricId = ''; + + if (isRepeatable) { + // Parse combined key for repeatable metrics: "class@__@key@__@id" + const keyParts = metricKey.split(METRICS_JOIN_TEXT); + if (keyParts.length === 3) { + metricClass = keyParts[0]; + metricKeyValue = keyParts[1]; + metricId = keyParts[2]; + } else { + // Fallback if format is unexpected + metricId = metricKey; + } + } else { + // Global metric: use the key as-is for metricId + metricId = metricKey; + } + + return { + metricType: isRepeatable ? METRIC_TYPE.REPEATABLE : METRIC_TYPE.GLOBAL, + metricId, + displayName: sourceQuery.name || '', + description: '', // Not available in current structure + metricClass, + metricKey: metricKeyValue, + aggregateStatistic: sourceQuery.query?.operationType || '', + individualStatistic: sourceQuery.repeatedMeasure || '', + comparison: sourceQuery.query?.compareFn || '=', + compareValue: sourceQuery.query?.compareValue || '', + allowableDataKeys: [], + }; + } + + // Default values for add mode + return { + metricType: METRIC_TYPE.GLOBAL, + metricId: '', + displayName: '', + description: '', + metricClass: '', + metricKey: '', + aggregateStatistic: '', + individualStatistic: '', + comparison: '=', + compareValue: '', + allowableDataKeys: [], + }; + } + + populateFormForEditMode(initialValues: MetricFormData): void { + // Wait for allMetrics to be loaded, then populate form with proper objects + this.subscriptions.add( + this.allMetrics$.pipe(take(1)).subscribe((metrics) => { + if (!metrics || metrics.length === 0) return; + + const { metricType, metricClass, metricKey, metricId } = initialValues; + let classObject = null; + let keyObject = null; + let idObject = null; + + if (metricType === METRIC_TYPE.REPEATABLE) { + // Find the class object + classObject = metrics.find((m) => m.key === metricClass); + + if (classObject?.children) { + // Find the key object within the class children + keyObject = classObject.children.find((k) => k.key === metricKey); + + if (keyObject?.children) { + // Find the ID object within the key children + idObject = keyObject.children.find((id) => id.key === metricId); + } else if (keyObject) { + // If no children in keyObject, keyObject itself might be the ID + idObject = keyObject; + } + } + } else { + // Global metric: find the metric directly + idObject = metrics.find((m) => m.key === metricId); + } + + // Update form with found objects (or keep strings if objects not found) + const formUpdates = { + metricClass: classObject || metricClass, + metricKey: keyObject || metricKey, + metricId: idObject || metricId, + }; + + this.metricForm.patchValue(formUpdates); + + // Update the initial form values to reflect the object values + const currentInitialValues = this.initialFormValues$.value; + const newInitialValues = { ...currentInitialValues, ...formUpdates }; + if (!isEqual(currentInitialValues, newInitialValues)) { + this.initialFormValues$.next(newInitialValues); + } + + // Update the options and form state + this.populateOptions(); + if (idObject) { + this.detectMetricDataType(idObject); + this.updateStatisticOptions(); + this.updateFormVisibility(); + } + + // Trigger change detection to ensure UI updates + this.cdr.markForCheck(); + }) + ); + } + + setupFormChangeListeners(): void { + this.subscriptions.add( + this.metricForm.get('metricType')?.valueChanges.subscribe(() => { + this.onMetricTypeChange(); + }) + ); + + this.subscriptions.add( + this.metricForm.get('metricClass')?.valueChanges.subscribe((selectedClass) => { + this.onMetricClassChange(selectedClass); + }) + ); + + this.subscriptions.add( + this.metricForm.get('metricKey')?.valueChanges.subscribe((selectedKey) => { + this.onMetricKeyChange(selectedKey); + }) + ); + + this.subscriptions.add( + this.metricForm.get('metricId')?.valueChanges.subscribe((metricId) => { + this.onMetricIdChange(metricId); + }) + ); + } + + setupAutocomplete(): void { + this.subscriptions.add( + this.allMetrics$.subscribe((metrics) => { + this.allMetrics = metrics || []; + this.populateOptions(); + this.createFilteredObservables(); + }) + ); + } + + setupExperimentContext(): void { + this.subscriptions.add( + this.experimentService.selectedExperiment$.subscribe((experiment) => { + if (experiment) { + this.currentAssignmentUnit = experiment.assignmentUnit; + this.currentContext = experiment.context; + this.updateMetricTypeAvailability(); + this.populateOptions(); + } + }) + ); + + if (this.config.params.experimentId && !this.currentAssignmentUnit) { + this.subscriptions.add( + this.experimentService.experiments$.subscribe((experiments) => { + const experiment = experiments.find((exp) => exp.id === this.config.params.experimentId); + if (experiment && !this.currentAssignmentUnit) { + this.currentAssignmentUnit = experiment.assignmentUnit; + this.currentContext = experiment.context; + this.updateMetricTypeAvailability(); + this.populateOptions(); + } + }) + ); + } + } + + populateOptions(): void { + const metricType = this.metricForm.get('metricType')?.value; + let filteredMetrics = this.allMetrics || []; + + if (this.currentAssignmentUnit && filteredMetrics.length > 0) { + if (this.currentAssignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS) { + const withinSubjectsMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); + if (withinSubjectsMetrics.length > 0) { + filteredMetrics = withinSubjectsMetrics; + } + } else if (this.currentContext && this.currentContext.length > 0) { + const contextFilteredMetrics = filteredMetrics.filter( + (metric) => metric.context && this.currentContext?.some((ctx) => metric.context.includes(ctx)) + ); + if (contextFilteredMetrics.length > 0) { + filteredMetrics = contextFilteredMetrics; + } + } + } + + if (metricType === METRIC_TYPE.GLOBAL) { + this.metricClassOptions$.next([]); + this.metricKeyOptions$.next([]); + const globalMetrics = filteredMetrics.filter((metric) => !metric.children || metric.children.length === 0); + this.metricIdOptions$.next(globalMetrics); + } else { + const repeatableMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); + this.metricClassOptions$.next(repeatableMetrics); + this.metricKeyOptions$.next([]); + this.metricIdOptions$.next([]); + } + } + + createFilteredObservables(): void { + this.filteredMetricClasses$ = combineLatest([ + this.metricForm.get('metricClass')?.valueChanges.pipe(startWith('')) || new BehaviorSubject(''), + this.metricClassOptions$, + ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); + + this.filteredMetricKeys$ = combineLatest([ + this.metricForm.get('metricKey')?.valueChanges.pipe(startWith('')) || new BehaviorSubject(''), + this.metricKeyOptions$, + ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); + + this.filteredMetricIds$ = combineLatest([ + this.metricForm.get('metricId')?.valueChanges.pipe(startWith('')) || new BehaviorSubject(''), + this.metricIdOptions$, + ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); + } + + onMetricClassChange(selectedClass: any): void { + if (selectedClass && typeof selectedClass === 'object' && selectedClass.children) { + this.currentSelectedClass = selectedClass; + this.metricKeyOptions$.next(selectedClass.children); + + // Reset dependent fields - no forced refresh + this.metricForm.get('metricKey')?.setValue(''); + this.metricForm.get('metricId')?.setValue(''); + this.currentSelectedKey = null; + this.metricIdOptions$.next([]); + } else if (selectedClass === '' || selectedClass === null) { + // Clear everything if class is cleared + this.currentSelectedClass = null; + this.metricKeyOptions$.next([]); + this.metricIdOptions$.next([]); + this.metricForm.get('metricKey')?.setValue(''); + this.metricForm.get('metricId')?.setValue(''); + } + // Don't do anything for string values - user is typing + } + + onMetricKeyChange(selectedKey: any): void { + if (selectedKey && typeof selectedKey === 'object') { + this.currentSelectedKey = selectedKey; + + // Set metric IDs based on selected key's children + if (selectedKey.children && selectedKey.children.length > 0) { + this.metricIdOptions$.next(selectedKey.children); + } else { + this.metricIdOptions$.next([selectedKey]); + } + + // Reset ID field - no forced refresh + this.metricForm.get('metricId')?.setValue(''); + } else if (selectedKey === '' || selectedKey === null) { + // Clear IDs if key is cleared + this.metricIdOptions$.next([]); + this.metricForm.get('metricId')?.setValue(''); + } + // Don't do anything for string values - user is typing + } + + displayFn = (option?: any): string => { + return option?.key || option || ''; + }; + + private extractKey(value: any): string { + return typeof value === 'object' ? value?.key || '' : value || ''; + } + + private _filter(value: any, options: any[]): any[] { + const filterValue = this.extractKey(value).toLowerCase(); + return options.filter((option) => option.key?.toLowerCase().includes(filterValue)); + } + + onMetricTypeChange(): void { + // Clear everything and repopulate + this.currentSelectedClass = null; + this.currentSelectedKey = null; + this.metricDataType = null; + + // Clear form fields + this.metricForm.get('metricClass')?.setValue(''); + this.metricForm.get('metricKey')?.setValue(''); + this.metricForm.get('metricId')?.setValue(''); + this.metricForm.get('displayName')?.setValue(''); // Clear display name when metric type changes + + // Repopulate options based on new metric type + // BehaviorSubjects will automatically trigger observable re-emission + this.populateOptions(); + + // Update form state + this.resetStatisticFields(); + this.updateFormVisibility(); + this.updateFormValidators(); + } + + onMetricIdChange(metricId: any): void { + if (metricId) { + this.detectMetricDataType(metricId); + this.updateStatisticOptions(); + this.updateFormVisibility(); + this.updateFormValidators(); + } else { + // Clear everything when metric ID is cleared + this.metricDataType = null; + this.hideStatisticDropdowns(); + this.resetStatisticFields(); + this.updateFormValidators(); + } + } + + detectMetricDataType(metricId: any): void { + const selectedMetric = this.findSelectedMetric(metricId); + + // Use metadata if available + if (selectedMetric?.metadata?.type) { + this.setMetricDataType(selectedMetric.metadata.type, selectedMetric); + return; + } + + // Fallback to heuristic detection + this.detectMetricTypeByHeuristic(metricId); + } + + private findSelectedMetric(metricId: any): any { + if (typeof metricId === 'object' && metricId.metadata) { + return metricId; + } + + if (typeof metricId === 'string') { + const currentOptions = this.metricIdOptions$.getValue(); + return currentOptions.find((metric) => metric.key === metricId); + } + + return null; + } + + private setMetricDataType(dataType: IMetricMetaData, selectedMetric?: any): void { + this.metricDataType = dataType; + + if (dataType === IMetricMetaData.CATEGORICAL) { + this.allowableDataKeys = selectedMetric?.allowedData ? [...selectedMetric.allowedData] : []; + + // Set default comparison if not already set + if (!this.metricForm.get('comparison')?.value) { + this.metricForm.get('comparison')?.setValue('='); + } + } else { + this.allowableDataKeys = []; + } + } + + private detectMetricTypeByHeuristic(metricId: any): void { + const metricKey = this.extractKey(metricId); + const lowerMetricKey = metricKey.toLowerCase(); + + const continuousKeywords = ['time', 'count', 'score', 'number', 'seconds', 'minutes', 'duration']; + const categoricalKeywords = ['status', 'type', 'category', 'level', 'completion']; + + if (continuousKeywords.some((keyword) => lowerMetricKey.includes(keyword))) { + this.setMetricDataType(IMetricMetaData.CONTINUOUS); + } else if (categoricalKeywords.some((keyword) => lowerMetricKey.includes(keyword))) { + this.setMetricDataType(IMetricMetaData.CATEGORICAL); + } else { + // Default to continuous for unknown types + this.setMetricDataType(IMetricMetaData.CONTINUOUS); + } + } + + updateFormVisibility(): void { + const metricType = this.metricForm.get('metricType')?.value; + const hasMetricId = !!this.metricForm.get('metricId')?.value; + + // Base visibility for metric type + this.showMetricClass = metricType === METRIC_TYPE.REPEATABLE; + this.showMetricKey = metricType === METRIC_TYPE.REPEATABLE; + + // Statistics only show when metric ID is selected + if (hasMetricId && this.metricDataType) { + this.showAggregateStatistic = true; + this.showIndividualStatistic = metricType === METRIC_TYPE.REPEATABLE; + this.showComparison = this.metricDataType === IMetricMetaData.CATEGORICAL; + } else { + this.showAggregateStatistic = false; + this.showIndividualStatistic = false; + this.showComparison = false; + } + + // Trigger change detection with OnPush strategy + this.cdr.markForCheck(); + } + + updateMetricTypeAvailability(): void { + // Disable global metrics for within-subjects experiments + this.isGlobalMetricDisabled = this.currentAssignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS; + + // If global metrics are disabled and global is currently selected, switch to repeatable + if (this.isGlobalMetricDisabled && this.metricForm.get('metricType')?.value === METRIC_TYPE.GLOBAL) { + this.metricForm.get('metricType')?.setValue(METRIC_TYPE.REPEATABLE); + } + + this.cdr.markForCheck(); + } + + updateStatisticOptions(): void { + if (this.metricDataType === IMetricMetaData.CONTINUOUS) { + this.aggregateStatisticOptions = this.continuousAggregateOptions; + this.individualStatisticOptions = this.continuousIndividualOptions; + } else if (this.metricDataType === IMetricMetaData.CATEGORICAL) { + this.aggregateStatisticOptions = this.categoricalAggregateOptions; + this.individualStatisticOptions = this.categoricalIndividualOptions; + } + // Note: showComparison is handled in updateFormVisibility() + } + + hideStatisticDropdowns(): void { + this.showAggregateStatistic = false; + this.showIndividualStatistic = false; + this.showComparison = false; + } + + resetStatisticFields(): void { + this.metricForm.patchValue({ + aggregateStatistic: '', + individualStatistic: '', + comparison: '=', + compareValue: '', + }); + this.allowableDataKeys = []; + } + + updateFormValidators(): void { + const metricType = this.metricForm.get('metricType')?.value; + + // Update validators based on metric type + if (metricType === METRIC_TYPE.REPEATABLE) { + this.metricForm.get('metricClass')?.setValidators([Validators.required]); + this.metricForm.get('metricKey')?.setValidators([Validators.required]); + this.metricForm.get('individualStatistic')?.setValidators([Validators.required]); + } else { + this.metricForm.get('metricClass')?.clearValidators(); + this.metricForm.get('metricKey')?.clearValidators(); + this.metricForm.get('individualStatistic')?.clearValidators(); + } + + // Update aggregate statistic validator + if (this.showAggregateStatistic) { + this.metricForm.get('aggregateStatistic')?.setValidators([Validators.required]); + } else { + this.metricForm.get('aggregateStatistic')?.clearValidators(); + } + + // Update comparison validators for categorical metrics + if (this.showComparison) { + this.metricForm.get('comparison')?.setValidators([Validators.required]); + this.metricForm.get('compareValue')?.setValidators([Validators.required]); + } else { + this.metricForm.get('comparison')?.clearValidators(); + this.metricForm.get('compareValue')?.clearValidators(); + } + + // Update all validators without emitting events to prevent recursion + Object.keys(this.metricForm.controls).forEach((key) => { + this.metricForm.get(key)?.updateValueAndValidity({ emitEvent: false }); + }); + } + + listenForIsInitialFormValueChanged(): void { + this.isInitialFormValueChanged$ = this.metricForm.valueChanges.pipe( + startWith(this.metricForm.value), + map(() => { + const currentWithKeys = { + ...this.metricForm.value, + allowableDataKeys: this.allowableDataKeys, + }; + return !isEqual(currentWithKeys, this.initialFormValues$.value); + }) + ); + } + + listenForPrimaryButtonDisabled(): void { + this.isPrimaryButtonDisabled$ = this.isLoadingUpsertMetric$.pipe( + combineLatestWith(this.isInitialFormValueChanged$), + map( + ([isLoading, isInitialFormValueChanged]) => + isLoading || + this.metricForm.invalid || + (!isInitialFormValueChanged && this.config.params.action === UPSERT_EXPERIMENT_ACTION.EDIT) + ) + ); + } + + onPrimaryActionBtnClicked(): void { + if (this.metricForm.valid) { + this.sendUpsertMetricRequest(); + } else { + CommonFormHelpersService.triggerTouchedToDisplayErrors(this.metricForm); + } + } + + sendUpsertMetricRequest(): void { + const formValue = this.metricForm.value; + const metricData = this.prepareMetricDataForBackend(formValue); + + // Get current experiment and call helper service + this.experimentService.selectedExperiment$.pipe(take(1)).subscribe((experiment: Experiment) => { + if (!experiment) { + console.error('No experiment selected'); + return; + } + + if (this.config.params.action === UPSERT_EXPERIMENT_ACTION.ADD) { + this.metricHelperService.addMetric(experiment, metricData); + } else { + const sourceQuery = this.config.params.sourceQuery; + if (!sourceQuery) { + console.error('No source query for edit action'); + return; + } + + this.metricHelperService.updateMetric(experiment, sourceQuery, metricData); + } + + this.closeModal(); + }); + } + + private prepareMetricDataForBackend(formValue: any): ExperimentQueryDTO { + const { metricType, metricClass, metricKey: metricKeyValue, metricId } = formValue; + + // Prepare metric key based on type + const metricKey = + metricType === METRIC_TYPE.GLOBAL + ? this.extractKey(metricId) + : `${this.extractKey(metricClass)}${METRICS_JOIN_TEXT}${this.extractKey( + metricKeyValue + )}${METRICS_JOIN_TEXT}${this.extractKey(metricId)}`; + + // Prepare query object + const queryObj: ExperimentQueryDTO = { + name: formValue.displayName, + query: { + operationType: formValue.aggregateStatistic, + // Add comparison for categorical metrics + ...(this.metricDataType === IMetricMetaData.CATEGORICAL && + formValue.comparison && + formValue.compareValue && { + compareFn: formValue.comparison, + compareValue: formValue.compareValue, + }), + }, + metric: { + key: metricKey, + }, + repeatedMeasure: + metricType === METRIC_TYPE.REPEATABLE ? formValue.individualStatistic : REPEATED_MEASURE.mostRecent, + }; + + return queryObj; + } + + closeModal(): void { + this.dialogRef.close(); + } +} \ No newline at end of file diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts index 1cd573efc1..1d96def72d 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts @@ -12,14 +12,18 @@ import { ExperimentQueryRowActionEvent, } from './experiment-metrics-table/experiment-metrics-table.component'; import { ExperimentService } from '../../../../../../../core/experiments/experiments.service'; +import { MetricHelperService } from '../../../../../../../core/experiments/metric-helper.service'; import { Observable, map } from 'rxjs'; +import { take } from 'rxjs/operators'; import { Experiment, EXPERIMENT_BUTTON_ACTION, EXPERIMENT_ROW_ACTION, + ExperimentQueryDTO, } from '../../../../../../../core/experiments/store/experiments.model'; import { UserPermission } from '../../../../../../../core/auth/store/auth.models'; import { AuthService } from '../../../../../../../core/auth/auth.service'; +import { DialogService } from '../../../../../../../shared/services/common-dialog.service'; @Component({ selector: 'app-experiment-metrics-section-card', @@ -63,15 +67,24 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { }, ]; - constructor(private experimentService: ExperimentService, private authService: AuthService) {} + constructor( + private experimentService: ExperimentService, + private metricHelperService: MetricHelperService, + private authService: AuthService, + private dialogService: DialogService + ) {} ngOnInit() { this.permissions$ = this.authService.userPermissions$; } onAddMetricClick(): void { - // TODO: Implement add metric functionality when dialog service is available - console.log('Add metric clicked'); + // Get experiment ID from selected experiment + this.selectedExperiment$.pipe(take(1)).subscribe((experiment: Experiment) => { + if (experiment?.id) { + this.dialogService.openAddMetricModal(experiment.id); + } + }); } onMenuButtonItemClick(event: string, experiment: Experiment): void { @@ -99,20 +112,31 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { this.onEditMetric(event.query, experimentId); break; case EXPERIMENT_ROW_ACTION.DELETE: - this.onDeleteMetric(event.query, experimentId); + this.onDeleteMetric(event.query); break; default: console.log('Unknown action:', event.action); } } - private onEditMetric(query: any, experimentId: string): void { - // TODO: Implement edit metric functionality when dialog service is available - console.log('Edit metric clicked for query:', query.id, 'in experiment:', experimentId); + private onEditMetric(query: ExperimentQueryDTO, experimentId: string): void { + this.dialogService.openEditMetricModal(query, experimentId); } - private onDeleteMetric(query: any, experimentId: string): void { - // TODO: Implement delete metric functionality when dialog service is available - console.log('Delete metric clicked for query:', query.id, 'in experiment:', experimentId); + private onDeleteMetric(query: ExperimentQueryDTO): void { + const metricDisplayName = query.name || `${query.metric?.key}`; + + this.dialogService + .openDeleteMetricModal(metricDisplayName) + .afterClosed() + .subscribe((confirmClicked) => { + if (confirmClicked) { + this.selectedExperiment$.pipe(take(1)).subscribe((experiment: Experiment) => { + if (experiment) { + this.metricHelperService.deleteMetric(experiment, query); + } + }); + } + }); } } diff --git a/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts b/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts index 12d4ec7194..6141207316 100644 --- a/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts +++ b/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts @@ -38,13 +38,13 @@ import { DeleteSegmentModalComponent } from '../../features/dashboard/segments/m import { UpsertExperimentModalComponent } from '../../features/dashboard/experiments/modals/upsert-experiment-modal/upsert-experiment-modal.component'; import { UpsertDecisionPointModalComponent } from '../../features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component'; import { UpsertConditionModalComponent } from '../../features/dashboard/experiments/modals/upsert-condition-modal/upsert-condition-modal.component'; +import { UpsertMetricModalComponent } from '../../features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component'; import { UPSERT_EXPERIMENT_ACTION, ExperimentDecisionPoint, ExperimentCondition, ExperimentConditionPayload, - Experiment, - ExperimentVM, + ExperimentQueryDTO, } from '../../core/experiments/store/experiments.model'; import { ConditionWeightUpdate, @@ -82,6 +82,12 @@ export interface UpsertConditionModalParams { context: string; } +export interface UpsertMetricModalParams { + sourceQuery: ExperimentQueryDTO | null; + action: UPSERT_EXPERIMENT_ACTION; + experimentId: string; +} + @Injectable({ providedIn: 'root', }) @@ -228,6 +234,46 @@ export class DialogService { return this.dialog.open(EditPayloadModalComponent, config); } + openAddMetricModal(experimentId: string) { + const commonModalConfig: CommonModalConfig = { + title: 'Add Metric', + primaryActionBtnLabel: 'Create', + primaryActionBtnColor: 'primary', + cancelBtnLabel: 'Cancel', + params: { + sourceQuery: null, + action: UPSERT_EXPERIMENT_ACTION.ADD, + experimentId, + }, + }; + return this.openUpsertMetricModal(commonModalConfig); + } + + openEditMetricModal(sourceQuery: ExperimentQueryDTO, experimentId: string) { + const commonModalConfig: CommonModalConfig = { + title: 'Edit Metric', + primaryActionBtnLabel: 'Save', + primaryActionBtnColor: 'primary', + cancelBtnLabel: 'Cancel', + params: { + sourceQuery: { ...sourceQuery }, + action: UPSERT_EXPERIMENT_ACTION.EDIT, + experimentId, + }, + }; + return this.openUpsertMetricModal(commonModalConfig); + } + + openUpsertMetricModal(commonModalConfig: CommonModalConfig) { + const config: MatDialogConfig = { + data: commonModalConfig, + width: ModalSize.STANDARD, + autoFocus: false, + disableClose: true, + }; + return this.dialog.open(UpsertMetricModalComponent, config); + } + // feature flag modal ---------------------------------------- // openAddFeatureFlagModal() { const commonModalConfig: CommonModalConfig = { @@ -713,6 +759,20 @@ export class DialogService { return this.openSimpleCommonConfirmationModal(deleteConditionModalConfig, ModalSize.SMALL); } + openDeleteMetricModal(metricName: string) { + const deleteMetricModalConfig: CommonModalConfig = { + title: 'Delete Metric', + primaryActionBtnLabel: 'Delete', + primaryActionBtnColor: 'warn', + cancelBtnLabel: 'Cancel', + params: { + message: `Are you sure you want to delete the metric "${metricName}"?`, + }, + }; + + return this.openSimpleCommonConfirmationModal(deleteMetricModalConfig, ModalSize.SMALL); + } + openDeleteSegmentModal() { const commonModalConfig: CommonModalConfig = { title: 'Delete Segment', diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index d13aabcad2..21127fba73 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -449,6 +449,15 @@ "experiments.details.metrics.display-name.text": "Display Name", "experiments.details.metrics.actions.text": "Actions", "experiments.details.metrics.card.no-data-row.text": "No metrics defined. No metrics will be monitored in the experiment.", + "experiments.upsert-metric-modal.metric-type-global-metric-description.text": "Used for globally accumulated measures (e.g., total time spent using the app).", + "experiments.upsert-metric-modal.metric-type-repeatable-metric-description.text": "Used for repeatable measures (e.g., score on a quiz that can be taken multiple times).", + "experiments.upsert-metric-modal.metric-class-hint.text": "Categorizes what type of app component is being measured (e.g., workspace).", + "experiments.upsert-metric-modal.metric-key-hint.text": "Specifies the specific instance of the metric class being measured (e.g., problem ID).", + "experiments.upsert-metric-modal.metric-id-hint.text": "Specifies the data type (continuous or categorical) and what data to measure.", + "experiments.upsert-metric-modal.individual-statistic-hint.text": "The individual statistic determines which value to use for each student.", + "experiments.upsert-metric-modal.aggregate-statistic-hint.text": "The aggregate statistic determines how to combine values across all students.", + "experiments.upsert-metric-modal.value-hint.text": "The categorical metric data type requires you to specify which allowed value to measure.", + "experiments.upsert-metric-modal.display-name-hint.text": "The display name is used to refer to this metric in the experiment UI.", "experiments.details.enrollment-data.card.title.text": "Enrollment Data", "experiments.details.enrollment-data.card.subtitle.text": "Enrollments reflect participants who have started the experiment.", "experiments.details.export-enrollment-data.menu-item.text": "Export Enrollment Data", diff --git a/types/src/Experiment/enums.ts b/types/src/Experiment/enums.ts index 2358acf49b..bc40584b52 100644 --- a/types/src/Experiment/enums.ts +++ b/types/src/Experiment/enums.ts @@ -185,6 +185,11 @@ export enum UserRole { READER = 'reader', } +export enum METRIC_TYPE { + GLOBAL = 'global', + REPEATABLE = 'repeatable', +} + export enum OPERATION_TYPES { SUM = 'sum', COUNT = 'count', diff --git a/types/src/index.ts b/types/src/index.ts index c87e363bad..59fec415a6 100644 --- a/types/src/index.ts +++ b/types/src/index.ts @@ -14,6 +14,7 @@ export { EXPERIMENT_LIST_OPERATION, SORT_AS_DIRECTION, UserRole, + METRIC_TYPE, OPERATION_TYPES, IMetricMetaData, DATE_RANGE, From d89609336ea53e3f405e2aa93e59ecd0a83647ec Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 21 Nov 2025 15:17:16 -0500 Subject: [PATCH 02/30] Correct the metric-helper.service.ts filename --- .../{metric-helper.service.ts.ts => metric-helper.service.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename frontend/projects/upgrade/src/app/core/experiments/{metric-helper.service.ts.ts => metric-helper.service.ts} (100%) diff --git a/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts.ts b/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts similarity index 100% rename from frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts.ts rename to frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts From 2691597128ad692b8c3d51e47a40a1fcbb0d64f1 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 21 Nov 2025 16:32:17 -0500 Subject: [PATCH 03/30] Fix lint errors --- .../upgrade/src/app/core/experiments/metric-helper.service.ts | 2 +- .../src/app/core/experiments/store/experiments.actions.ts | 4 +--- .../upsert-metric-modal/upsert-metric-modal.component.ts | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts b/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts index fe812f7d29..b08afdbec2 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts @@ -63,4 +63,4 @@ export class MetricHelperService { this.experimentService.updateExperimentMetrics(updateRequest); } -} \ No newline at end of file +} diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts index f2bb00d4b2..39185fe898 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.actions.ts @@ -419,6 +419,4 @@ export const actionUpdateExperimentMetricsSuccess = createAction( props<{ experiment: Experiment }>() ); -export const actionUpdateExperimentMetricsFailure = createAction( - '[Experiment] Update Experiment Metrics Failure' -); +export const actionUpdateExperimentMetricsFailure = createAction('[Experiment] Update Experiment Metrics Failure'); diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 4da649a894..be69d35adc 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -770,4 +770,4 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { closeModal(): void { this.dialogRef.close(); } -} \ No newline at end of file +} From 14177184b76c033d44a95cc86edf97215d548a93 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 24 Nov 2025 00:04:09 -0500 Subject: [PATCH 04/30] Filter out all reward metric options in the Metric ID field --- .../upsert-metric-modal/upsert-metric-modal.component.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index be69d35adc..7d35a39273 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -395,6 +395,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } + // Filter out all reward metric keys (ending with _REWARD) + // These are automatically added by the app and should not be manually selectable + filteredMetrics = filteredMetrics.filter((metric) => !metric.key?.endsWith('_REWARD')); + if (metricType === METRIC_TYPE.GLOBAL) { this.metricClassOptions$.next([]); this.metricKeyOptions$.next([]); From 828b6a2ed6673c6bef1f08e8567799fd582e22a9 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 24 Nov 2025 00:50:16 -0500 Subject: [PATCH 05/30] Disallow reward metric editing and deleting --- .../experiment-metrics-section-card.component.html | 1 + .../experiment-metrics-section-card.component.ts | 3 +++ .../experiment-metrics-table.component.html | 4 ++-- .../experiment-metrics-table.component.ts | 12 ++++++++++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html index fc35bca7cd..6ee2359dc6 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html @@ -28,6 +28,7 @@ [queries]="experiment.queries || []" [isLoading$]="isLoadingExperiment$" [actionsDisabled]="!(permissions$ | async)?.experiments.update" + [isRewardMetricActionsDisabled]="isRewardMetricActionsDisabled$ | async" (rowAction)="onRowAction($event, experiment.id)" > diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts index 1d96def72d..58a1b57a22 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts @@ -47,6 +47,9 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { isLoadingExperiment$ = this.experimentService.isLoadingExperiment$; tableRowCount$ = this.selectedExperiment$.pipe(map((experiment) => experiment?.queries?.length || 0)); + isRewardMetricActionsDisabled$ = this.selectedExperiment$.pipe( + map((experiment) => experiment?.assignmentAlgorithm === 'ts_configurable') + ); get tableRowCount(): number { let count = 0; diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html index b488664431..53e27a655f 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html @@ -62,7 +62,7 @@ @@ -71,7 +71,7 @@ diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts index 62c158f0fa..ed058c138c 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts @@ -40,6 +40,7 @@ export class ExperimentMetricsTableComponent { @Input() queries: ExperimentQueryDTO[] = []; @Input() isLoading$: Observable; @Input() actionsDisabled?: boolean = false; + @Input() isRewardMetricActionsDisabled?: boolean = false; @Output() rowAction = new EventEmitter(); displayedColumns: string[] = ['metric', 'statistic', 'displayName', 'actions']; @@ -117,6 +118,17 @@ export class ExperimentMetricsTableComponent { return repeatedMeasure.replace(/_/g, ' ').toLowerCase(); } + isRewardMetric(query: ExperimentQueryDTO): boolean { + return query.metric?.key?.endsWith('_REWARD') || false; + } + + isRowActionsDisabled(query: ExperimentQueryDTO): boolean { + // Disable actions if: + // 1. General actions are disabled (no update permission), OR + // 2. This is a reward metric AND the experiment uses TS Configurable algorithm + return this.actionsDisabled || (this.isRewardMetricActionsDisabled && this.isRewardMetric(query)); + } + onEditButtonClick(query: ExperimentQueryDTO): void { this.rowAction.emit({ action: EXPERIMENT_ROW_ACTION.EDIT, query }); } From a7f9a3b2620f56db69c1ba7a4c3e8e2fb3fbe6fc Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 24 Nov 2025 03:11:20 -0500 Subject: [PATCH 06/30] fix: require metric ID selection from autocomplete options and prevent premature statistics display --- .../upsert-metric-modal.component.html | 14 +++- .../upsert-metric-modal.component.ts | 71 +++++++++++++++---- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html index 3855025e25..f802ed21d2 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -100,7 +100,12 @@ class="ft-14-400" [matAutocomplete]="metricIdAutocomplete" /> - + - + + + + Please select a Metric ID from the available options. + + {{ 'experiments.upsert-metric-modal.metric-id-hint.text' | translate }} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 7d35a39273..6b03400dbc 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -94,6 +94,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { private currentSelectedClass: any = null; private currentSelectedKey: any = null; + // Track if user has selected a valid option from autocomplete (vs just typing) + private hasValidMetricIdSelection$ = new BehaviorSubject(false); + // Assignment unit and context for filtering private currentAssignmentUnit: ASSIGNMENT_UNIT | null = null; private currentContext: string[] | null = null; @@ -157,13 +160,27 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.subscriptions.unsubscribe(); } + // Custom validator to ensure metricId is selected from options (object) not typed (string) + private metricIdSelectedValidator(control: any): { [key: string]: any } | null { + const value = control.value; + if (!value) { + return null; // Let required validator handle empty values + } + // Valid if it's an object (selected from autocomplete) + if (typeof value === 'object' && value !== null) { + return null; + } + // Invalid if it's a string (typed, not selected) + return { mustSelectFromOptions: true }; + } + createMetricForm(): void { const { sourceQuery, action } = this.config.params; const initialValues = this.deriveInitialFormValues(sourceQuery, action); this.metricForm = this.formBuilder.group({ metricType: [initialValues.metricType, Validators.required], - metricId: [initialValues.metricId, Validators.required], + metricId: [initialValues.metricId, [Validators.required, this.metricIdSelectedValidator.bind(this)]], displayName: [initialValues.displayName, Validators.required], description: [initialValues.description], metricClass: [initialValues.metricClass], @@ -301,6 +318,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Update the options and form state this.populateOptions(); if (idObject) { + this.hasValidMetricIdSelection$.next(true); // Mark as valid selection in edit mode this.detectMetricDataType(idObject); this.updateStatisticOptions(); this.updateFormVisibility(); @@ -333,7 +351,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.subscriptions.add( this.metricForm.get('metricId')?.valueChanges.subscribe((metricId) => { - this.onMetricIdChange(metricId); + this.onMetricIdValueChange(metricId); }) ); } @@ -439,6 +457,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.metricForm.get('metricId')?.setValue(''); this.currentSelectedKey = null; this.metricIdOptions$.next([]); + this.hasValidMetricIdSelection$.next(false); } else if (selectedClass === '' || selectedClass === null) { // Clear everything if class is cleared this.currentSelectedClass = null; @@ -446,6 +465,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.metricIdOptions$.next([]); this.metricForm.get('metricKey')?.setValue(''); this.metricForm.get('metricId')?.setValue(''); + this.hasValidMetricIdSelection$.next(false); } // Don't do anything for string values - user is typing } @@ -463,10 +483,12 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Reset ID field - no forced refresh this.metricForm.get('metricId')?.setValue(''); + this.hasValidMetricIdSelection$.next(false); } else if (selectedKey === '' || selectedKey === null) { // Clear IDs if key is cleared this.metricIdOptions$.next([]); this.metricForm.get('metricId')?.setValue(''); + this.hasValidMetricIdSelection$.next(false); } // Don't do anything for string values - user is typing } @@ -489,6 +511,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.currentSelectedClass = null; this.currentSelectedKey = null; this.metricDataType = null; + this.hasValidMetricIdSelection$.next(false); // Clear form fields this.metricForm.get('metricClass')?.setValue(''); @@ -506,14 +529,19 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.updateFormValidators(); } - onMetricIdChange(metricId: any): void { - if (metricId) { - this.detectMetricDataType(metricId); - this.updateStatisticOptions(); - this.updateFormVisibility(); + onMetricIdValueChange(metricId: any): void { + // If user is typing (string value) after selecting an option, invalidate the selection + if (typeof metricId === 'string' && this.hasValidMetricIdSelection$.getValue()) { + this.hasValidMetricIdSelection$.next(false); + this.metricDataType = null; + this.hideStatisticDropdowns(); + this.resetStatisticFields(); this.updateFormValidators(); - } else { - // Clear everything when metric ID is cleared + } + + // If field is cleared completely + if (!metricId) { + this.hasValidMetricIdSelection$.next(false); this.metricDataType = null; this.hideStatisticDropdowns(); this.resetStatisticFields(); @@ -521,6 +549,17 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } + onMetricIdOptionSelected(metricId: any): void { + // This is called when user actually selects an option from autocomplete + if (metricId && typeof metricId === 'object') { + this.hasValidMetricIdSelection$.next(true); + this.detectMetricDataType(metricId); + this.updateStatisticOptions(); + this.updateFormVisibility(); + this.updateFormValidators(); + } + } + detectMetricDataType(metricId: any): void { const selectedMetric = this.findSelectedMetric(metricId); @@ -581,14 +620,13 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { updateFormVisibility(): void { const metricType = this.metricForm.get('metricType')?.value; - const hasMetricId = !!this.metricForm.get('metricId')?.value; // Base visibility for metric type this.showMetricClass = metricType === METRIC_TYPE.REPEATABLE; this.showMetricKey = metricType === METRIC_TYPE.REPEATABLE; - // Statistics only show when metric ID is selected - if (hasMetricId && this.metricDataType) { + // Statistics only show when a valid metric ID option has been selected + if (this.hasValidMetricIdSelection$.getValue() && this.metricDataType) { this.showAggregateStatistic = true; this.showIndividualStatistic = metricType === METRIC_TYPE.REPEATABLE; this.showComparison = this.metricDataType === IMetricMetaData.CATEGORICAL; @@ -675,6 +713,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { Object.keys(this.metricForm.controls).forEach((key) => { this.metricForm.get(key)?.updateValueAndValidity({ emitEvent: false }); }); + + // Update the form's overall validity status WITH event emission + // This ensures the observable streams are updated for button state + this.metricForm.updateValueAndValidity({ emitEvent: true }); } listenForIsInitialFormValueChanged(): void { @@ -692,11 +734,12 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { listenForPrimaryButtonDisabled(): void { this.isPrimaryButtonDisabled$ = this.isLoadingUpsertMetric$.pipe( - combineLatestWith(this.isInitialFormValueChanged$), + combineLatestWith(this.isInitialFormValueChanged$, this.hasValidMetricIdSelection$), map( - ([isLoading, isInitialFormValueChanged]) => + ([isLoading, isInitialFormValueChanged, hasValidMetricIdSelection]) => isLoading || this.metricForm.invalid || + !hasValidMetricIdSelection || (!isInitialFormValueChanged && this.config.params.action === UPSERT_EXPERIMENT_ACTION.EDIT) ) ); From 6d020077464c5c133328ea05884b6f0fe3bf0058 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 24 Nov 2025 03:45:24 -0500 Subject: [PATCH 07/30] Add validation requiring metric class, key, and ID selection from autocomplete options with cascading field clearing --- .../upsert-metric-modal.component.html | 28 ++++- .../upsert-metric-modal.component.ts | 119 ++++++++++++++---- 2 files changed, 119 insertions(+), 28 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html index f802ed21d2..d49c21dba8 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -50,7 +50,12 @@ class="ft-14-400" [matAutocomplete]="metricClassAutocomplete" /> - + - + + + + Please select a Metric Class from the available options. + + {{ 'experiments.upsert-metric-modal.metric-class-hint.text' | translate }} @@ -75,7 +85,12 @@ class="ft-14-400" [matAutocomplete]="metricKeyAutocomplete" /> - + - + + + + Please select a Metric Key from the available options. + + {{ 'experiments.upsert-metric-modal.metric-key-hint.text' | translate }} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 6b03400dbc..872f38eba2 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -95,6 +95,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { private currentSelectedKey: any = null; // Track if user has selected a valid option from autocomplete (vs just typing) + private hasValidMetricClassSelection$ = new BehaviorSubject(false); + private hasValidMetricKeySelection$ = new BehaviorSubject(false); private hasValidMetricIdSelection$ = new BehaviorSubject(false); // Assignment unit and context for filtering @@ -160,7 +162,33 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.subscriptions.unsubscribe(); } - // Custom validator to ensure metricId is selected from options (object) not typed (string) + // Custom validators to ensure fields are selected from options (object) not typed (string) + private metricClassSelectedValidator(control: any): { [key: string]: any } | null { + const value = control.value; + if (!value) { + return null; // Let required validator handle empty values + } + // Valid if it's an object (selected from autocomplete) + if (typeof value === 'object' && value !== null) { + return null; + } + // Invalid if it's a string (typed, not selected) + return { mustSelectFromOptions: true }; + } + + private metricKeySelectedValidator(control: any): { [key: string]: any } | null { + const value = control.value; + if (!value) { + return null; // Let required validator handle empty values + } + // Valid if it's an object (selected from autocomplete) + if (typeof value === 'object' && value !== null) { + return null; + } + // Invalid if it's a string (typed, not selected) + return { mustSelectFromOptions: true }; + } + private metricIdSelectedValidator(control: any): { [key: string]: any } | null { const value = control.value; if (!value) { @@ -183,8 +211,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metricId: [initialValues.metricId, [Validators.required, this.metricIdSelectedValidator.bind(this)]], displayName: [initialValues.displayName, Validators.required], description: [initialValues.description], - metricClass: [initialValues.metricClass], - metricKey: [initialValues.metricKey], + metricClass: [initialValues.metricClass, this.metricClassSelectedValidator.bind(this)], + metricKey: [initialValues.metricKey, this.metricKeySelectedValidator.bind(this)], aggregateStatistic: [initialValues.aggregateStatistic], individualStatistic: [initialValues.individualStatistic], comparison: [initialValues.comparison || '='], @@ -318,7 +346,11 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Update the options and form state this.populateOptions(); if (idObject) { - this.hasValidMetricIdSelection$.next(true); // Mark as valid selection in edit mode + // Mark as valid selections in edit mode + if (classObject) this.hasValidMetricClassSelection$.next(true); + if (keyObject) this.hasValidMetricKeySelection$.next(true); + this.hasValidMetricIdSelection$.next(true); + this.detectMetricDataType(idObject); this.updateStatisticOptions(); this.updateFormVisibility(); @@ -339,13 +371,13 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.subscriptions.add( this.metricForm.get('metricClass')?.valueChanges.subscribe((selectedClass) => { - this.onMetricClassChange(selectedClass); + this.onMetricClassValueChange(selectedClass); }) ); this.subscriptions.add( this.metricForm.get('metricKey')?.valueChanges.subscribe((selectedKey) => { - this.onMetricKeyChange(selectedKey); + this.onMetricKeyValueChange(selectedKey); }) ); @@ -447,31 +479,70 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); } - onMetricClassChange(selectedClass: any): void { + onMetricClassValueChange(selectedClass: any): void { + // If user is typing (string value) after selecting an option, invalidate the selection + if (typeof selectedClass === 'string' && this.hasValidMetricClassSelection$.getValue()) { + this.hasValidMetricClassSelection$.next(false); + // Clear dependent fields when parent becomes invalid + this.metricKeyOptions$.next([]); + this.metricIdOptions$.next([]); + this.metricForm.get('metricKey')?.setValue(''); + this.metricForm.get('metricId')?.setValue(''); + this.hasValidMetricKeySelection$.next(false); + this.hasValidMetricIdSelection$.next(false); + } + + // If field is cleared completely + if (!selectedClass) { + this.hasValidMetricClassSelection$.next(false); + this.currentSelectedClass = null; + this.metricKeyOptions$.next([]); + this.metricIdOptions$.next([]); + this.metricForm.get('metricKey')?.setValue(''); + this.metricForm.get('metricId')?.setValue(''); + this.hasValidMetricKeySelection$.next(false); + this.hasValidMetricIdSelection$.next(false); + } + } + + onMetricClassOptionSelected(selectedClass: any): void { if (selectedClass && typeof selectedClass === 'object' && selectedClass.children) { + this.hasValidMetricClassSelection$.next(true); this.currentSelectedClass = selectedClass; this.metricKeyOptions$.next(selectedClass.children); - // Reset dependent fields - no forced refresh + // Reset dependent fields this.metricForm.get('metricKey')?.setValue(''); this.metricForm.get('metricId')?.setValue(''); this.currentSelectedKey = null; this.metricIdOptions$.next([]); + this.hasValidMetricKeySelection$.next(false); this.hasValidMetricIdSelection$.next(false); - } else if (selectedClass === '' || selectedClass === null) { - // Clear everything if class is cleared - this.currentSelectedClass = null; - this.metricKeyOptions$.next([]); + } + } + + onMetricKeyValueChange(selectedKey: any): void { + // If user is typing (string value) after selecting an option, invalidate the selection + if (typeof selectedKey === 'string' && this.hasValidMetricKeySelection$.getValue()) { + this.hasValidMetricKeySelection$.next(false); + // Clear dependent fields when parent becomes invalid + this.metricIdOptions$.next([]); + this.metricForm.get('metricId')?.setValue(''); + this.hasValidMetricIdSelection$.next(false); + } + + // If field is cleared completely + if (!selectedKey) { + this.hasValidMetricKeySelection$.next(false); this.metricIdOptions$.next([]); - this.metricForm.get('metricKey')?.setValue(''); this.metricForm.get('metricId')?.setValue(''); this.hasValidMetricIdSelection$.next(false); } - // Don't do anything for string values - user is typing } - onMetricKeyChange(selectedKey: any): void { + onMetricKeyOptionSelected(selectedKey: any): void { if (selectedKey && typeof selectedKey === 'object') { + this.hasValidMetricKeySelection$.next(true); this.currentSelectedKey = selectedKey; // Set metric IDs based on selected key's children @@ -481,16 +552,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.metricIdOptions$.next([selectedKey]); } - // Reset ID field - no forced refresh - this.metricForm.get('metricId')?.setValue(''); - this.hasValidMetricIdSelection$.next(false); - } else if (selectedKey === '' || selectedKey === null) { - // Clear IDs if key is cleared - this.metricIdOptions$.next([]); + // Reset ID field this.metricForm.get('metricId')?.setValue(''); this.hasValidMetricIdSelection$.next(false); } - // Don't do anything for string values - user is typing } displayFn = (option?: any): string => { @@ -511,6 +576,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.currentSelectedClass = null; this.currentSelectedKey = null; this.metricDataType = null; + this.hasValidMetricClassSelection$.next(false); + this.hasValidMetricKeySelection$.next(false); this.hasValidMetricIdSelection$.next(false); // Clear form fields @@ -684,8 +751,12 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Update validators based on metric type if (metricType === METRIC_TYPE.REPEATABLE) { - this.metricForm.get('metricClass')?.setValidators([Validators.required]); - this.metricForm.get('metricKey')?.setValidators([Validators.required]); + this.metricForm + .get('metricClass') + ?.setValidators([Validators.required, this.metricClassSelectedValidator.bind(this)]); + this.metricForm + .get('metricKey') + ?.setValidators([Validators.required, this.metricKeySelectedValidator.bind(this)]); this.metricForm.get('individualStatistic')?.setValidators([Validators.required]); } else { this.metricForm.get('metricClass')?.clearValidators(); From f6d68fdbf002ca72c9e9801e1ea04c616fcc572f Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 24 Nov 2025 04:09:19 -0500 Subject: [PATCH 08/30] Prevent Save button from being incorrectly enabled in edit mode for categorical metrics --- .../upsert-metric-modal.component.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 872f38eba2..4823fd4372 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -354,6 +354,14 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.detectMetricDataType(idObject); this.updateStatisticOptions(); this.updateFormVisibility(); + + // Update initial form values with allowableDataKeys to prevent false "changed" detection + // This is crucial for categorical metrics where allowableDataKeys gets populated + const updatedInitialValues = { + ...this.initialFormValues$.value, + allowableDataKeys: this.allowableDataKeys, + }; + this.initialFormValues$.next(updatedInitialValues); } // Trigger change detection to ensure UI updates @@ -658,11 +666,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { if (dataType === IMetricMetaData.CATEGORICAL) { this.allowableDataKeys = selectedMetric?.allowedData ? [...selectedMetric.allowedData] : []; - - // Set default comparison if not already set - if (!this.metricForm.get('comparison')?.value) { - this.metricForm.get('comparison')?.setValue('='); - } } else { this.allowableDataKeys = []; } From e32acb66614ad2be96423a8a87d523b65071e417 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 24 Nov 2025 10:05:48 -0500 Subject: [PATCH 09/30] Apply appTrimInput directive to all the input fields in the UpsertMetricModalComponent --- .../upsert-metric-modal/upsert-metric-modal.component.html | 6 +++++- .../upsert-metric-modal/upsert-metric-modal.component.ts | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html index d49c21dba8..57f431a1ce 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -49,6 +49,7 @@ placeholder="e.g., workspace" class="ft-14-400" [matAutocomplete]="metricClassAutocomplete" + appTrimInput /> {{ 'experiments.upsert-metric-modal.display-name-hint.text' | translate }} @@ -241,7 +245,7 @@ Description (optional) - + diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 4823fd4372..e7ce93cb79 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -17,6 +17,7 @@ import { TranslateModule } from '@ngx-translate/core'; import { CommonModalComponent } from '../../../../../shared-standalone-component-lib/components'; import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; import { CommonFormHelpersService } from '../../../../../shared/services/common-form-helpers.service'; +import { SharedModule } from '../../../../../shared/shared.module'; import { MetricFormData, UPSERT_EXPERIMENT_ACTION, @@ -49,6 +50,7 @@ interface StatisticOption { CommonModule, ReactiveFormsModule, TranslateModule, + SharedModule, ], templateUrl: './upsert-metric-modal.component.html', styleUrl: './upsert-metric-modal.component.scss', From 54fdf42ea8e2c628a029473fa77341d82acc5bab Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 2 Dec 2025 13:46:14 -0500 Subject: [PATCH 10/30] Resolve sonarqube warnings --- .../upsert-metric-modal.component.ts | 78 +++++-------------- 1 file changed, 21 insertions(+), 57 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index e7ce93cb79..496425d8bb 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -88,18 +88,14 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { filteredMetricIds$: Observable; // BehaviorSubjects for source data - private metricClassOptions$ = new BehaviorSubject([]); - private metricKeyOptions$ = new BehaviorSubject([]); - private metricIdOptions$ = new BehaviorSubject([]); - - // Current selections - private currentSelectedClass: any = null; - private currentSelectedKey: any = null; + private readonly metricClassOptions$ = new BehaviorSubject([]); + private readonly metricKeyOptions$ = new BehaviorSubject([]); + private readonly metricIdOptions$ = new BehaviorSubject([]); // Track if user has selected a valid option from autocomplete (vs just typing) - private hasValidMetricClassSelection$ = new BehaviorSubject(false); - private hasValidMetricKeySelection$ = new BehaviorSubject(false); - private hasValidMetricIdSelection$ = new BehaviorSubject(false); + private readonly hasValidMetricClassSelection$ = new BehaviorSubject(false); + private readonly hasValidMetricKeySelection$ = new BehaviorSubject(false); + private readonly hasValidMetricIdSelection$ = new BehaviorSubject(false); // Assignment unit and context for filtering private currentAssignmentUnit: ASSIGNMENT_UNIT | null = null; @@ -141,12 +137,12 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { constructor( @Inject(MAT_DIALOG_DATA) public config: CommonModalConfig, - private formBuilder: FormBuilder, - private experimentService: ExperimentService, - private metricHelperService: MetricHelperService, - private analysisService: AnalysisService, - private cdr: ChangeDetectorRef, - public dialogRef: MatDialogRef + private readonly formBuilder: FormBuilder, + private readonly experimentService: ExperimentService, + private readonly metricHelperService: MetricHelperService, + private readonly analysisService: AnalysisService, + private readonly cdr: ChangeDetectorRef, + public readonly dialogRef: MatDialogRef ) {} ngOnInit(): void { @@ -164,34 +160,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.subscriptions.unsubscribe(); } - // Custom validators to ensure fields are selected from options (object) not typed (string) - private metricClassSelectedValidator(control: any): { [key: string]: any } | null { - const value = control.value; - if (!value) { - return null; // Let required validator handle empty values - } - // Valid if it's an object (selected from autocomplete) - if (typeof value === 'object' && value !== null) { - return null; - } - // Invalid if it's a string (typed, not selected) - return { mustSelectFromOptions: true }; - } - - private metricKeySelectedValidator(control: any): { [key: string]: any } | null { - const value = control.value; - if (!value) { - return null; // Let required validator handle empty values - } - // Valid if it's an object (selected from autocomplete) - if (typeof value === 'object' && value !== null) { - return null; - } - // Invalid if it's a string (typed, not selected) - return { mustSelectFromOptions: true }; - } - - private metricIdSelectedValidator(control: any): { [key: string]: any } | null { + // Custom validator to ensure fields are selected from options (object) not typed (string) + private autocompleteSelectionValidator(control: any): { [key: string]: any } | null { const value = control.value; if (!value) { return null; // Let required validator handle empty values @@ -210,11 +180,11 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.metricForm = this.formBuilder.group({ metricType: [initialValues.metricType, Validators.required], - metricId: [initialValues.metricId, [Validators.required, this.metricIdSelectedValidator.bind(this)]], + metricId: [initialValues.metricId, [Validators.required, this.autocompleteSelectionValidator.bind(this)]], displayName: [initialValues.displayName, Validators.required], description: [initialValues.description], - metricClass: [initialValues.metricClass, this.metricClassSelectedValidator.bind(this)], - metricKey: [initialValues.metricKey, this.metricKeySelectedValidator.bind(this)], + metricClass: [initialValues.metricClass, this.autocompleteSelectionValidator.bind(this)], + metricKey: [initialValues.metricKey, this.autocompleteSelectionValidator.bind(this)], aggregateStatistic: [initialValues.aggregateStatistic], individualStatistic: [initialValues.individualStatistic], comparison: [initialValues.comparison || '='], @@ -505,7 +475,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // If field is cleared completely if (!selectedClass) { this.hasValidMetricClassSelection$.next(false); - this.currentSelectedClass = null; this.metricKeyOptions$.next([]); this.metricIdOptions$.next([]); this.metricForm.get('metricKey')?.setValue(''); @@ -518,13 +487,11 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onMetricClassOptionSelected(selectedClass: any): void { if (selectedClass && typeof selectedClass === 'object' && selectedClass.children) { this.hasValidMetricClassSelection$.next(true); - this.currentSelectedClass = selectedClass; this.metricKeyOptions$.next(selectedClass.children); // Reset dependent fields this.metricForm.get('metricKey')?.setValue(''); this.metricForm.get('metricId')?.setValue(''); - this.currentSelectedKey = null; this.metricIdOptions$.next([]); this.hasValidMetricKeySelection$.next(false); this.hasValidMetricIdSelection$.next(false); @@ -553,7 +520,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onMetricKeyOptionSelected(selectedKey: any): void { if (selectedKey && typeof selectedKey === 'object') { this.hasValidMetricKeySelection$.next(true); - this.currentSelectedKey = selectedKey; // Set metric IDs based on selected key's children if (selectedKey.children && selectedKey.children.length > 0) { @@ -583,8 +549,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onMetricTypeChange(): void { // Clear everything and repopulate - this.currentSelectedClass = null; - this.currentSelectedKey = null; this.metricDataType = null; this.hasValidMetricClassSelection$.next(false); this.hasValidMetricKeySelection$.next(false); @@ -758,10 +722,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { if (metricType === METRIC_TYPE.REPEATABLE) { this.metricForm .get('metricClass') - ?.setValidators([Validators.required, this.metricClassSelectedValidator.bind(this)]); + ?.setValidators([Validators.required, this.autocompleteSelectionValidator.bind(this)]); this.metricForm .get('metricKey') - ?.setValidators([Validators.required, this.metricKeySelectedValidator.bind(this)]); + ?.setValidators([Validators.required, this.autocompleteSelectionValidator.bind(this)]); this.metricForm.get('individualStatistic')?.setValidators([Validators.required]); } else { this.metricForm.get('metricClass')?.clearValidators(); @@ -786,9 +750,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } // Update all validators without emitting events to prevent recursion - Object.keys(this.metricForm.controls).forEach((key) => { + for (const key of Object.keys(this.metricForm.controls)) { this.metricForm.get(key)?.updateValueAndValidity({ emitEvent: false }); - }); + } // Update the form's overall validity status WITH event emission // This ensures the observable streams are updated for button state From d7ace57d02264786639c7928c17d24d26657fe3b Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 2 Dec 2025 14:25:20 -0500 Subject: [PATCH 11/30] Extract populateFormForEditMode into focused helper methods --- .../upsert-metric-modal.component.ts | 184 +++++++++++------- 1 file changed, 118 insertions(+), 66 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 496425d8bb..c9ebddaea3 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -269,77 +269,129 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { populateFormForEditMode(initialValues: MetricFormData): void { // Wait for allMetrics to be loaded, then populate form with proper objects - this.subscriptions.add( - this.allMetrics$.pipe(take(1)).subscribe((metrics) => { - if (!metrics || metrics.length === 0) return; - - const { metricType, metricClass, metricKey, metricId } = initialValues; - let classObject = null; - let keyObject = null; - let idObject = null; - - if (metricType === METRIC_TYPE.REPEATABLE) { - // Find the class object - classObject = metrics.find((m) => m.key === metricClass); - - if (classObject?.children) { - // Find the key object within the class children - keyObject = classObject.children.find((k) => k.key === metricKey); - - if (keyObject?.children) { - // Find the ID object within the key children - idObject = keyObject.children.find((id) => id.key === metricId); - } else if (keyObject) { - // If no children in keyObject, keyObject itself might be the ID - idObject = keyObject; - } - } - } else { - // Global metric: find the metric directly - idObject = metrics.find((m) => m.key === metricId); - } + // Using take(1) ensures subscription auto-completes, no manual cleanup needed + this.allMetrics$.pipe(take(1)).subscribe((metrics) => { + if (!metrics?.length) { + return; + } - // Update form with found objects (or keep strings if objects not found) - const formUpdates = { - metricClass: classObject || metricClass, - metricKey: keyObject || metricKey, - metricId: idObject || metricId, - }; + const metricObjects = this.findMetricObjects(metrics, initialValues); + this.updateFormWithMetricObjects(initialValues, metricObjects); - this.metricForm.patchValue(formUpdates); + // Only update validation state and initialize statistics if idObject exists + if (metricObjects.idObject) { + this.updateValidationState(metricObjects); + this.initializeMetricTypeAndStatistics(metricObjects.idObject); + } - // Update the initial form values to reflect the object values - const currentInitialValues = this.initialFormValues$.value; - const newInitialValues = { ...currentInitialValues, ...formUpdates }; - if (!isEqual(currentInitialValues, newInitialValues)) { - this.initialFormValues$.next(newInitialValues); - } + this.cdr.markForCheck(); + }); + } - // Update the options and form state - this.populateOptions(); - if (idObject) { - // Mark as valid selections in edit mode - if (classObject) this.hasValidMetricClassSelection$.next(true); - if (keyObject) this.hasValidMetricKeySelection$.next(true); - this.hasValidMetricIdSelection$.next(true); - - this.detectMetricDataType(idObject); - this.updateStatisticOptions(); - this.updateFormVisibility(); - - // Update initial form values with allowableDataKeys to prevent false "changed" detection - // This is crucial for categorical metrics where allowableDataKeys gets populated - const updatedInitialValues = { - ...this.initialFormValues$.value, - allowableDataKeys: this.allowableDataKeys, - }; - this.initialFormValues$.next(updatedInitialValues); - } + private findMetricObjects( + metrics: any[], + initialValues: MetricFormData + ): { classObject: any; keyObject: any; idObject: any } { + const { metricType, metricClass, metricKey, metricId } = initialValues; - // Trigger change detection to ensure UI updates - this.cdr.markForCheck(); - }) - ); + if (metricType === METRIC_TYPE.REPEATABLE) { + return this.findRepeatableMetricObjects(metrics, metricClass, metricKey, metricId); + } + + // Global metric: find the metric directly + return { + classObject: null, + keyObject: null, + idObject: metrics.find((m) => m.key === metricId) ?? null, + }; + } + + private findRepeatableMetricObjects( + metrics: any[], + metricClass: string, + metricKey: string, + metricId: string + ): { classObject: any; keyObject: any; idObject: any } { + const classObject = metrics.find((m) => m.key === metricClass) ?? null; + + if (!classObject?.children) { + return { classObject, keyObject: null, idObject: null }; + } + + const keyObject = classObject.children.find((k: any) => k.key === metricKey) ?? null; + + if (!keyObject) { + return { classObject, keyObject: null, idObject: null }; + } + + // Find ID object in key's children, or use key itself if no children + const idObject = keyObject.children?.length + ? keyObject.children.find((id: any) => id.key === metricId) ?? null + : keyObject; + + return { classObject, keyObject, idObject }; + } + + private updateFormWithMetricObjects( + initialValues: MetricFormData, + metricObjects: { classObject: any; keyObject: any; idObject: any } + ): void { + const { classObject, keyObject, idObject } = metricObjects; + + // Update form with found objects (fallback to string values if not found) + const formUpdates = { + metricClass: classObject ?? initialValues.metricClass, + metricKey: keyObject ?? initialValues.metricKey, + metricId: idObject ?? initialValues.metricId, + }; + + this.metricForm.patchValue(formUpdates); + this.populateOptions(); + + // Update initial form values once with all necessary data + const updatedInitialValues: MetricFormData = { + ...this.initialFormValues$.value, + ...formUpdates, + // Include allowableDataKeys if available (for categorical metrics) + allowableDataKeys: this.allowableDataKeys.length > 0 ? this.allowableDataKeys : initialValues.allowableDataKeys, + }; + + if (!isEqual(this.initialFormValues$.value, updatedInitialValues)) { + this.initialFormValues$.next(updatedInitialValues); + } + } + + private updateValidationState(metricObjects: { classObject: any; keyObject: any; idObject: any }): void { + const { classObject, keyObject, idObject } = metricObjects; + + // Mark selections as valid based on found objects + if (classObject) { + this.hasValidMetricClassSelection$.next(true); + } + if (keyObject) { + this.hasValidMetricKeySelection$.next(true); + } + if (idObject) { + this.hasValidMetricIdSelection$.next(true); + } + } + + private initializeMetricTypeAndStatistics(idObject: any): void { + this.detectMetricDataType(idObject); + this.updateStatisticOptions(); + this.updateFormVisibility(); + + // Re-sync allowableDataKeys to initial values after detection + // This prevents false "changed" detection for categorical metrics + if (this.allowableDataKeys.length > 0) { + const syncedInitialValues: MetricFormData = { + ...this.initialFormValues$.value, + allowableDataKeys: this.allowableDataKeys, + }; + if (!isEqual(this.initialFormValues$.value, syncedInitialValues)) { + this.initialFormValues$.next(syncedInitialValues); + } + } } setupFormChangeListeners(): void { From 8c44ad0d1c66d2e084be7add80a1d1073e89d962 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 2 Dec 2025 14:45:53 -0500 Subject: [PATCH 12/30] Make Metric Class and ID options to appear on Edit Metric modal, and add placeholders for statistic drop-downs --- .../upsert-metric-modal.component.html | 12 +- .../upsert-metric-modal.component.ts | 133 +++++++++++++----- 2 files changed, 108 insertions(+), 37 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html index 57f431a1ce..06f1e284bf 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -151,7 +151,11 @@ Individual Statistic - + Aggregate Statistic - + 0) { - if (this.currentAssignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS) { - const withinSubjectsMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); - if (withinSubjectsMetrics.length > 0) { - filteredMetrics = withinSubjectsMetrics; - } - } else if (this.currentContext && this.currentContext.length > 0) { - const contextFilteredMetrics = filteredMetrics.filter( - (metric) => metric.context && this.currentContext?.some((ctx) => metric.context.includes(ctx)) - ); - if (contextFilteredMetrics.length > 0) { - filteredMetrics = contextFilteredMetrics; - } - } + if (!this.metricForm) { + return; } - // Filter out all reward metric keys (ending with _REWARD) - // These are automatically added by the app and should not be manually selectable - filteredMetrics = filteredMetrics.filter((metric) => !metric.key?.endsWith('_REWARD')); + const metricType = this.metricForm.get('metricType')?.value; + const filteredMetrics = this.sanitizeMetricOptions(this.filterMetricsByAssignmentContext(this.allMetrics || [])); if (metricType === METRIC_TYPE.GLOBAL) { - this.metricClassOptions$.next([]); - this.metricKeyOptions$.next([]); - const globalMetrics = filteredMetrics.filter((metric) => !metric.children || metric.children.length === 0); - this.metricIdOptions$.next(globalMetrics); - } else { - const repeatableMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); - this.metricClassOptions$.next(repeatableMetrics); - this.metricKeyOptions$.next([]); - this.metricIdOptions$.next([]); + this.populateGlobalMetricOptions(filteredMetrics); + return; } + + this.populateRepeatableMetricOptions(filteredMetrics); } createFilteredObservables(): void { @@ -539,7 +518,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onMetricClassOptionSelected(selectedClass: any): void { if (selectedClass && typeof selectedClass === 'object' && selectedClass.children) { this.hasValidMetricClassSelection$.next(true); - this.metricKeyOptions$.next(selectedClass.children); + this.metricKeyOptions$.next(this.sanitizeMetricOptions(selectedClass.children)); // Reset dependent fields this.metricForm.get('metricKey')?.setValue(''); @@ -574,11 +553,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.hasValidMetricKeySelection$.next(true); // Set metric IDs based on selected key's children - if (selectedKey.children && selectedKey.children.length > 0) { - this.metricIdOptions$.next(selectedKey.children); - } else { - this.metricIdOptions$.next([selectedKey]); - } + this.metricIdOptions$.next(this.getMetricIdOptionsFromKey(selectedKey)); // Reset ID field this.metricForm.get('metricId')?.setValue(''); @@ -599,6 +574,94 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return options.filter((option) => option.key?.toLowerCase().includes(filterValue)); } + private sanitizeMetricOptions(options?: any[]): any[] { + if (!options?.length) { + return []; + } + return options.filter((option) => !option?.key?.endsWith('_REWARD')); + } + + private findOptionByKey(options: any[], key: string): any { + if (!options.length || !key) { + return undefined; + } + return options.find((option) => option?.key === key); + } + + private resolveSelectedOption(controlValue: any, options: any[] = []): any { + if (!controlValue) { + return undefined; + } + if (typeof controlValue === 'object') { + return controlValue; + } + return this.findOptionByKey(options, this.extractKey(controlValue)); + } + + private getMetricIdOptionsFromKey(selectedKey: any): any[] { + if (!selectedKey || typeof selectedKey !== 'object') { + return []; + } + + if (selectedKey.children && selectedKey.children.length > 0) { + return this.sanitizeMetricOptions(selectedKey.children); + } + + return this.sanitizeMetricOptions([selectedKey]); + } + + private filterMetricsByAssignmentContext(metrics: any[]): any[] { + if (!metrics?.length) { + return []; + } + + if (this.currentAssignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS) { + const withinSubjectsMetrics = metrics.filter((metric) => metric.children && metric.children.length > 0); + return withinSubjectsMetrics.length > 0 ? withinSubjectsMetrics : metrics; + } + + if (this.currentAssignmentUnit && this.currentContext?.length) { + const contextFilteredMetrics = metrics.filter( + (metric) => metric.context && this.currentContext?.some((ctx) => metric.context.includes(ctx)) + ); + if (contextFilteredMetrics.length > 0) { + return contextFilteredMetrics; + } + } + + return metrics; + } + + private populateGlobalMetricOptions(metrics: any[]): void { + this.metricClassOptions$.next([]); + this.metricKeyOptions$.next([]); + const globalMetrics = metrics.filter((metric) => !metric.children || metric.children.length === 0); + this.metricIdOptions$.next(this.sanitizeMetricOptions(globalMetrics)); + } + + private populateRepeatableMetricOptions(metrics: any[]): void { + const repeatableMetrics = metrics.filter((metric) => metric.children && metric.children.length > 0); + this.metricClassOptions$.next(repeatableMetrics); + + const selectedClass = this.resolveSelectedOption(this.metricForm.get('metricClass')?.value, repeatableMetrics); + if (!selectedClass?.children?.length) { + this.metricKeyOptions$.next([]); + this.metricIdOptions$.next([]); + return; + } + + const sanitizedClassChildren = this.sanitizeMetricOptions(selectedClass.children); + this.metricKeyOptions$.next(sanitizedClassChildren); + + const selectedKey = this.resolveSelectedOption(this.metricForm.get('metricKey')?.value, sanitizedClassChildren); + if (!selectedKey) { + this.metricIdOptions$.next([]); + return; + } + + this.metricIdOptions$.next(this.getMetricIdOptionsFromKey(selectedKey)); + } + onMetricTypeChange(): void { // Clear everything and repopulate this.metricDataType = null; From 11c1fa2b2c44aa5429ea27dbd1705cc3ac8434bb Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 2 Dec 2025 14:54:57 -0500 Subject: [PATCH 13/30] Remove the Description field from the Add/Edit Metrics modal --- .../src/app/core/experiments/store/experiments.model.ts | 1 - .../upsert-metric-modal/upsert-metric-modal.component.html | 7 ------- .../upsert-metric-modal/upsert-metric-modal.component.ts | 3 --- 3 files changed, 11 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index dcb316ee17..2721ed01a4 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts @@ -373,7 +373,6 @@ export interface MetricFormData { metricType: METRIC_TYPE; metricId: string; displayName: string; - description?: string; metricClass?: string; // For repeatable metrics only metricKey?: string; // For repeatable metrics only aggregateStatistic?: string; diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html index 06f1e284bf..ca0ecff6f6 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -249,12 +249,5 @@ {{ 'experiments.upsert-metric-modal.display-name-hint.text' | translate }} - - - - Description (optional) - - - \ No newline at end of file diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index adaf2f1e84..f0d2f9caad 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -182,7 +182,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metricType: [initialValues.metricType, Validators.required], metricId: [initialValues.metricId, [Validators.required, this.autocompleteSelectionValidator.bind(this)]], displayName: [initialValues.displayName, Validators.required], - description: [initialValues.description], metricClass: [initialValues.metricClass, this.autocompleteSelectionValidator.bind(this)], metricKey: [initialValues.metricKey, this.autocompleteSelectionValidator.bind(this)], aggregateStatistic: [initialValues.aggregateStatistic], @@ -240,7 +239,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metricType: isRepeatable ? METRIC_TYPE.REPEATABLE : METRIC_TYPE.GLOBAL, metricId, displayName: sourceQuery.name || '', - description: '', // Not available in current structure metricClass, metricKey: metricKeyValue, aggregateStatistic: sourceQuery.query?.operationType || '', @@ -256,7 +254,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metricType: METRIC_TYPE.GLOBAL, metricId: '', displayName: '', - description: '', metricClass: '', metricKey: '', aggregateStatistic: '', From 687599f4fa0155afb796bfd42a60cdd3f91ae94b Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 2 Dec 2025 16:44:49 -0500 Subject: [PATCH 14/30] Wait for metrics data before hydrating edit metric form --- .../upsert-metric-modal.component.ts | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index f0d2f9caad..44b0bf131b 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -2,7 +2,7 @@ import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, OnDestro import { FormBuilder, FormGroup, ReactiveFormsModule, Validators } from '@angular/forms'; import { MAT_DIALOG_DATA, MatDialogRef } from '@angular/material/dialog'; import { BehaviorSubject, Observable, Subscription, combineLatest } from 'rxjs'; -import { combineLatestWith, map, startWith, take } from 'rxjs/operators'; +import { combineLatestWith, filter, map, startWith, take } from 'rxjs/operators'; import isEqual from 'lodash.isequal'; import { CommonModule } from '@angular/common'; import { MatFormFieldModule } from '@angular/material/form-field'; @@ -267,22 +267,23 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { populateFormForEditMode(initialValues: MetricFormData): void { // Wait for allMetrics to be loaded, then populate form with proper objects // Using take(1) ensures subscription auto-completes, no manual cleanup needed - this.allMetrics$.pipe(take(1)).subscribe((metrics) => { - if (!metrics?.length) { - return; - } - - const metricObjects = this.findMetricObjects(metrics, initialValues); - this.updateFormWithMetricObjects(initialValues, metricObjects); - - // Only update validation state and initialize statistics if idObject exists - if (metricObjects.idObject) { - this.updateValidationState(metricObjects); - this.initializeMetricTypeAndStatistics(metricObjects.idObject); - } + this.allMetrics$ + .pipe( + filter((metrics) => Array.isArray(metrics) && metrics.length > 0), + take(1) + ) + .subscribe((metrics) => { + const metricObjects = this.findMetricObjects(metrics, initialValues); + this.updateFormWithMetricObjects(initialValues, metricObjects); + + // Only update validation state and initialize statistics if idObject exists + if (metricObjects.idObject) { + this.updateValidationState(metricObjects); + this.initializeMetricTypeAndStatistics(metricObjects.idObject); + } - this.cdr.markForCheck(); - }); + this.cdr.markForCheck(); + }); } private findMetricObjects( From b0ad916a7fdab0eaeb0f372567d9ed52fb095089 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 2 Dec 2025 16:52:18 -0500 Subject: [PATCH 15/30] Resolve potential memory leak --- .../upsert-metric-modal.component.ts | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 44b0bf131b..a6f8288c24 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -267,23 +267,25 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { populateFormForEditMode(initialValues: MetricFormData): void { // Wait for allMetrics to be loaded, then populate form with proper objects // Using take(1) ensures subscription auto-completes, no manual cleanup needed - this.allMetrics$ - .pipe( - filter((metrics) => Array.isArray(metrics) && metrics.length > 0), - take(1) - ) - .subscribe((metrics) => { - const metricObjects = this.findMetricObjects(metrics, initialValues); - this.updateFormWithMetricObjects(initialValues, metricObjects); - - // Only update validation state and initialize statistics if idObject exists - if (metricObjects.idObject) { - this.updateValidationState(metricObjects); - this.initializeMetricTypeAndStatistics(metricObjects.idObject); - } + this.subscriptions.add( + this.allMetrics$ + .pipe( + filter((metrics) => Array.isArray(metrics) && metrics.length > 0), + take(1) + ) + .subscribe((metrics) => { + const metricObjects = this.findMetricObjects(metrics, initialValues); + this.updateFormWithMetricObjects(initialValues, metricObjects); + + // Only update validation state and initialize statistics if idObject exists + if (metricObjects.idObject) { + this.updateValidationState(metricObjects); + this.initializeMetricTypeAndStatistics(metricObjects.idObject); + } - this.cdr.markForCheck(); - }); + this.cdr.markForCheck(); + }) + ); } private findMetricObjects( From cfe405900a353a60143a5c5fc6b8f50e09874280 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 2 Dec 2025 23:19:08 -0500 Subject: [PATCH 16/30] Disable Save button on Edit Metric modal when Metric ID type (continuous, categorical) change clears aggregate statistic --- .../upsert-metric-modal.component.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index a6f8288c24..39eceb1e94 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -379,6 +379,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { private initializeMetricTypeAndStatistics(idObject: any): void { this.detectMetricDataType(idObject); this.updateStatisticOptions(); + this.clearInvalidStatisticSelections(); this.updateFormVisibility(); // Re-sync allowableDataKeys to initial values after detection @@ -711,6 +712,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.hasValidMetricIdSelection$.next(true); this.detectMetricDataType(metricId); this.updateStatisticOptions(); + this.clearInvalidStatisticSelections(); this.updateFormVisibility(); this.updateFormValidators(); } @@ -814,6 +816,29 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Note: showComparison is handled in updateFormVisibility() } + private clearInvalidStatisticSelections(): void { + if (!this.metricForm || !this.hasValidMetricIdSelection$.getValue()) { + return; + } + + const aggregateControl = this.metricForm.get('aggregateStatistic'); + const individualControl = this.metricForm.get('individualStatistic'); + + const validAggregateValues = this.aggregateStatisticOptions.map((option) => option.value); + const currentAggregateValue = aggregateControl?.value; + + if (currentAggregateValue && !validAggregateValues.includes(currentAggregateValue)) { + aggregateControl?.setValue('', { emitEvent: false }); + } + + const validIndividualValues = this.individualStatisticOptions.map((option) => option.value); + const currentIndividualValue = individualControl?.value; + + if (currentIndividualValue && !validIndividualValues.includes(currentIndividualValue)) { + individualControl?.setValue('', { emitEvent: false }); + } + } + hideStatisticDropdowns(): void { this.showAggregateStatistic = false; this.showIndividualStatistic = false; From f7bc0fa3b1c49830878187f29e9bdf035d965982 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Wed, 3 Dec 2025 00:11:02 -0500 Subject: [PATCH 17/30] Fix some fields not being required on the Edit Metric modal --- .../modals/upsert-metric-modal/upsert-metric-modal.component.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 39eceb1e94..5d7b0d5bac 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -381,6 +381,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.updateStatisticOptions(); this.clearInvalidStatisticSelections(); this.updateFormVisibility(); + this.updateFormValidators(); // Re-sync allowableDataKeys to initial values after detection // This prevents false "changed" detection for categorical metrics From 41594cf97a1d00b6f7221dd1482a2bcd68acae7f Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Wed, 3 Dec 2025 00:14:40 -0500 Subject: [PATCH 18/30] Add placeholder for the Value field --- .../upsert-metric-modal/upsert-metric-modal.component.html | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html index ca0ecff6f6..bc0d91d7d5 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -220,7 +220,11 @@ Value - + Date: Wed, 3 Dec 2025 00:51:07 -0500 Subject: [PATCH 19/30] Remove misleading comment --- .../modals/upsert-metric-modal/upsert-metric-modal.component.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 5d7b0d5bac..9aa31876d6 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -266,7 +266,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { populateFormForEditMode(initialValues: MetricFormData): void { // Wait for allMetrics to be loaded, then populate form with proper objects - // Using take(1) ensures subscription auto-completes, no manual cleanup needed this.subscriptions.add( this.allMetrics$ .pipe( From 6936d9d6ddf54965153f7dfceb79d9a29a0b1bf1 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Wed, 3 Dec 2025 10:28:53 -0500 Subject: [PATCH 20/30] Remove logic to disable actions for reward metrics in table, and remove logic to filter out reward metrics in modal --- .../upsert-metric-modal.component.ts | 24 +++++++------------ ...riment-metrics-section-card.component.html | 1 - ...periment-metrics-section-card.component.ts | 3 --- .../experiment-metrics-table.component.html | 4 ++-- .../experiment-metrics-table.component.ts | 12 ---------- 5 files changed, 11 insertions(+), 33 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 9aa31876d6..13b238d2ca 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -464,7 +464,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } const metricType = this.metricForm.get('metricType')?.value; - const filteredMetrics = this.sanitizeMetricOptions(this.filterMetricsByAssignmentContext(this.allMetrics || [])); + const filteredMetrics = this.filterMetricsByAssignmentContext(this.allMetrics || []); if (metricType === METRIC_TYPE.GLOBAL) { this.populateGlobalMetricOptions(filteredMetrics); @@ -519,7 +519,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onMetricClassOptionSelected(selectedClass: any): void { if (selectedClass && typeof selectedClass === 'object' && selectedClass.children) { this.hasValidMetricClassSelection$.next(true); - this.metricKeyOptions$.next(this.sanitizeMetricOptions(selectedClass.children)); + const classChildren = Array.isArray(selectedClass.children) ? selectedClass.children : []; + this.metricKeyOptions$.next(classChildren); // Reset dependent fields this.metricForm.get('metricKey')?.setValue(''); @@ -575,13 +576,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return options.filter((option) => option.key?.toLowerCase().includes(filterValue)); } - private sanitizeMetricOptions(options?: any[]): any[] { - if (!options?.length) { - return []; - } - return options.filter((option) => !option?.key?.endsWith('_REWARD')); - } - private findOptionByKey(options: any[], key: string): any { if (!options.length || !key) { return undefined; @@ -605,10 +599,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } if (selectedKey.children && selectedKey.children.length > 0) { - return this.sanitizeMetricOptions(selectedKey.children); + return selectedKey.children; } - return this.sanitizeMetricOptions([selectedKey]); + return [selectedKey]; } private filterMetricsByAssignmentContext(metrics: any[]): any[] { @@ -637,7 +631,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.metricClassOptions$.next([]); this.metricKeyOptions$.next([]); const globalMetrics = metrics.filter((metric) => !metric.children || metric.children.length === 0); - this.metricIdOptions$.next(this.sanitizeMetricOptions(globalMetrics)); + this.metricIdOptions$.next(globalMetrics); } private populateRepeatableMetricOptions(metrics: any[]): void { @@ -651,10 +645,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return; } - const sanitizedClassChildren = this.sanitizeMetricOptions(selectedClass.children); - this.metricKeyOptions$.next(sanitizedClassChildren); + const classChildren = Array.isArray(selectedClass.children) ? selectedClass.children : []; + this.metricKeyOptions$.next(classChildren); - const selectedKey = this.resolveSelectedOption(this.metricForm.get('metricKey')?.value, sanitizedClassChildren); + const selectedKey = this.resolveSelectedOption(this.metricForm.get('metricKey')?.value, classChildren); if (!selectedKey) { this.metricIdOptions$.next([]); return; diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html index 6ee2359dc6..fc35bca7cd 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html @@ -28,7 +28,6 @@ [queries]="experiment.queries || []" [isLoading$]="isLoadingExperiment$" [actionsDisabled]="!(permissions$ | async)?.experiments.update" - [isRewardMetricActionsDisabled]="isRewardMetricActionsDisabled$ | async" (rowAction)="onRowAction($event, experiment.id)" > diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts index 58a1b57a22..1d96def72d 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts @@ -47,9 +47,6 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { isLoadingExperiment$ = this.experimentService.isLoadingExperiment$; tableRowCount$ = this.selectedExperiment$.pipe(map((experiment) => experiment?.queries?.length || 0)); - isRewardMetricActionsDisabled$ = this.selectedExperiment$.pipe( - map((experiment) => experiment?.assignmentAlgorithm === 'ts_configurable') - ); get tableRowCount(): number { let count = 0; diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html index 53e27a655f..b488664431 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.html @@ -62,7 +62,7 @@ @@ -71,7 +71,7 @@ diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts index ed058c138c..62c158f0fa 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-table/experiment-metrics-table.component.ts @@ -40,7 +40,6 @@ export class ExperimentMetricsTableComponent { @Input() queries: ExperimentQueryDTO[] = []; @Input() isLoading$: Observable; @Input() actionsDisabled?: boolean = false; - @Input() isRewardMetricActionsDisabled?: boolean = false; @Output() rowAction = new EventEmitter(); displayedColumns: string[] = ['metric', 'statistic', 'displayName', 'actions']; @@ -118,17 +117,6 @@ export class ExperimentMetricsTableComponent { return repeatedMeasure.replace(/_/g, ' ').toLowerCase(); } - isRewardMetric(query: ExperimentQueryDTO): boolean { - return query.metric?.key?.endsWith('_REWARD') || false; - } - - isRowActionsDisabled(query: ExperimentQueryDTO): boolean { - // Disable actions if: - // 1. General actions are disabled (no update permission), OR - // 2. This is a reward metric AND the experiment uses TS Configurable algorithm - return this.actionsDisabled || (this.isRewardMetricActionsDisabled && this.isRewardMetric(query)); - } - onEditButtonClick(query: ExperimentQueryDTO): void { this.rowAction.emit({ action: EXPERIMENT_ROW_ACTION.EDIT, query }); } From 5d3a62aae4582d60524b695048b2b38263691ef5 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Wed, 3 Dec 2025 11:43:07 -0500 Subject: [PATCH 21/30] Remove menu button from Metrics section card --- .../experiments/store/experiments.model.ts | 2 - ...riment-metrics-section-card.component.html | 3 -- ...periment-metrics-section-card.component.ts | 38 ++----------------- 3 files changed, 4 insertions(+), 39 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index 2721ed01a4..640a18137e 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts @@ -321,8 +321,6 @@ export enum EXPERIMENT_BUTTON_ACTION { IMPORT_EXCLUDE_LIST = 'import exclude list', EXPORT_ALL_INCLUDE_LISTS = 'export all include lists', EXPORT_ALL_EXCLUDE_LISTS = 'export all exclude lists', - IMPORT_METRIC = 'import metric', - EXPORT_ALL_METRICS = 'export all metrics', } export interface UpsertExperimentParams { diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html index fc35bca7cd..b4b5cc9aa9 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.html @@ -12,11 +12,8 @@ header-right [showPrimaryButton]="(permissions$ | async)?.experiments.update" [primaryButtonText]="'experiments.details.add-metric.button.text' | translate" - [showMenuButton]="(permissions$ | async)?.experiments.update" - [menuButtonItems]="menuButtonItems" [isSectionCardExpanded]="isSectionCardExpanded" (primaryButtonClick)="onAddMetricClick()" - (menuButtonItemClick)="onMenuButtonItemClick($event, experiment)" (sectionCardExpandChange)="onSectionCardExpandChange($event)" > diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts index 1d96def72d..fd4ac63281 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-metrics-section-card/experiment-metrics-section-card.component.ts @@ -6,7 +6,6 @@ import { } from '../../../../../../../shared-standalone-component-lib/components'; import { CommonModule } from '@angular/common'; import { TranslateModule } from '@ngx-translate/core'; -import { IMenuButtonItem } from 'upgrade_types'; import { ExperimentMetricsTableComponent, ExperimentQueryRowActionEvent, @@ -17,7 +16,6 @@ import { Observable, map } from 'rxjs'; import { take } from 'rxjs/operators'; import { Experiment, - EXPERIMENT_BUTTON_ACTION, EXPERIMENT_ROW_ACTION, ExperimentQueryDTO, } from '../../../../../../../core/experiments/store/experiments.model'; @@ -54,24 +52,11 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { return count; } - menuButtonItems: IMenuButtonItem[] = [ - { - label: 'experiments.details.import-metric.menu-item.text', - action: EXPERIMENT_BUTTON_ACTION.IMPORT_METRIC, - disabled: false, - }, - { - label: 'experiments.details.export-all-metrics.menu-item.text', - action: EXPERIMENT_BUTTON_ACTION.EXPORT_ALL_METRICS, - disabled: false, - }, - ]; - constructor( - private experimentService: ExperimentService, - private metricHelperService: MetricHelperService, - private authService: AuthService, - private dialogService: DialogService + private readonly experimentService: ExperimentService, + private readonly metricHelperService: MetricHelperService, + private readonly authService: AuthService, + private readonly dialogService: DialogService ) {} ngOnInit() { @@ -87,21 +72,6 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { }); } - onMenuButtonItemClick(event: string, experiment: Experiment): void { - switch (event) { - case EXPERIMENT_BUTTON_ACTION.IMPORT_METRIC: - // TODO: Implement import functionality when dialog service is available - console.log('Import metric clicked for experiment:', experiment.id); - break; - case EXPERIMENT_BUTTON_ACTION.EXPORT_ALL_METRICS: - // TODO: Implement export functionality when experiment service methods are available - console.log('Export all metrics clicked for experiment:', experiment.id); - break; - default: - console.log('Unknown action:', event); - } - } - onSectionCardExpandChange(isSectionCardExpanded: boolean): void { this.isSectionCardExpanded = isSectionCardExpanded; } From a072f6cf3cde3f6f8c8b28bdf7b09211470d6a4d Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 4 Dec 2025 08:57:47 -0500 Subject: [PATCH 22/30] Extract warning messages to translation keys --- .../upsert-metric-modal/upsert-metric-modal.component.html | 6 +++--- frontend/projects/upgrade/src/assets/i18n/en.json | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html index bc0d91d7d5..e37987dbe4 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -68,7 +68,7 @@ - Please select a Metric Class from the available options. + {{ 'experiments.upsert-metric-modal.metric-class-warning.text' | translate }} {{ 'experiments.upsert-metric-modal.metric-class-hint.text' | translate }} @@ -104,7 +104,7 @@ - Please select a Metric Key from the available options. + {{ 'experiments.upsert-metric-modal.metric-key-warning.text' | translate }} {{ 'experiments.upsert-metric-modal.metric-key-hint.text' | translate }} @@ -140,7 +140,7 @@ - Please select a Metric ID from the available options. + {{ 'experiments.upsert-metric-modal.metric-id-warning.text' | translate }} {{ 'experiments.upsert-metric-modal.metric-id-hint.text' | translate }} diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index 21127fba73..3b25f20492 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -452,8 +452,11 @@ "experiments.upsert-metric-modal.metric-type-global-metric-description.text": "Used for globally accumulated measures (e.g., total time spent using the app).", "experiments.upsert-metric-modal.metric-type-repeatable-metric-description.text": "Used for repeatable measures (e.g., score on a quiz that can be taken multiple times).", "experiments.upsert-metric-modal.metric-class-hint.text": "Categorizes what type of app component is being measured (e.g., workspace).", + "experiments.upsert-metric-modal.metric-class-warning.text": "Please select a Metric Class from the available options.", "experiments.upsert-metric-modal.metric-key-hint.text": "Specifies the specific instance of the metric class being measured (e.g., problem ID).", + "experiments.upsert-metric-modal.metric-key-warning.text": "Please select a Metric Key from the available options.", "experiments.upsert-metric-modal.metric-id-hint.text": "Specifies the data type (continuous or categorical) and what data to measure.", + "experiments.upsert-metric-modal.metric-id-warning.text": "Please select a Metric ID from the available options.", "experiments.upsert-metric-modal.individual-statistic-hint.text": "The individual statistic determines which value to use for each student.", "experiments.upsert-metric-modal.aggregate-statistic-hint.text": "The aggregate statistic determines how to combine values across all students.", "experiments.upsert-metric-modal.value-hint.text": "The categorical metric data type requires you to specify which allowed value to measure.", From 6a2d386ce5ea2e3cc27e54ca1d7bde0fcedaa833 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 4 Dec 2025 17:21:26 -0500 Subject: [PATCH 23/30] Add missing translation keys for metrics and conditions update notification --- frontend/projects/upgrade/src/assets/i18n/en.json | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index 3b25f20492..fe3ee68943 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -384,6 +384,12 @@ "experiments.upsert-experiment-modal.stratification-factor-hint.text": "Select a stratification factor for balanced random assignment.", "experiments.upsert-experiment-modal.tags-label.text": "Tags (optional)", "experiments.upsert-experiment-modal.tags-placeholder.text": "Tags separated by commas", + "experiments.decision-points.update-success.text": "Decision points updated successfully.", + "experiments.decision-points.update-error.text": "Failed to update decision points. Please try again.", + "experiments.conditions.update-success.text": "Conditions updated successfully.", + "experiments.conditions.update-error.text": "Failed to update conditions. Please try again.", + "experiments.metrics.update-success.text": "Metrics updated successfully.", + "experiments.metrics.update-error.text": "Failed to update metrics. Please try again.", "experiments.delete-modal.warning-message.text": "Are you sure you want to delete ", "experiments.details.edit-experiment.menu-item.text": "Edit Experiment", "experiments.details.duplicate-experiment.menu-item.text": "Duplicate Experiment", @@ -402,8 +408,6 @@ "experiments.upsert-decision-point-modal.site-hint.text": "The site indicates a place in the code where the condition will be assigned.", "experiments.upsert-decision-point-modal.target-hint.text": "The target indicates the app component that will undergo the experiment.", "experiments.upsert-decision-point-modal.exclude-if-reached-hint.text": "Exclude students who visited the decision point while the experiment is inactive.", - "experiments.decision-points.update-success.text": "Decision point updated successfully.", - "experiments.decision-points.update-error.text": "Failed to update decision point. Please try again.", "experiments.details.conditions.card.title.text": "Conditions", "experiments.details.conditions.card.subtitle.text": "Define conditions for this experiment.", "experiments.details.add-condition.button.text": "Add Condition", From 7ecbc7d30d48e0f49d17a0cf9e07357e00af2d56 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 4 Dec 2025 19:08:07 -0500 Subject: [PATCH 24/30] Show snackbar when upsert metric lacks experiment or source query --- .../upsert-metric-modal.component.ts | 50 +++++++++++++------ 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 13b238d2ca..c86fa3be44 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -28,6 +28,7 @@ import { import { ExperimentService } from '../../../../../core/experiments/experiments.service'; import { MetricHelperService } from '../../../../../core/experiments/metric-helper.service'; import { AnalysisService } from '../../../../../core/analysis/analysis.service'; +import { NotificationService } from '../../../../../core/notifications/notification.service'; import { METRICS_JOIN_TEXT } from '../../../../../core/analysis/store/analysis.models'; import { ASSIGNMENT_UNIT, IMetricMetaData, METRIC_TYPE, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; @@ -100,6 +101,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Assignment unit and context for filtering private currentAssignmentUnit: ASSIGNMENT_UNIT | null = null; private currentContext: string[] | null = null; + private currentExperiment: Experiment | null = null; allowableDataKeys: string[] = []; comparisonOptions = [ @@ -141,6 +143,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { private readonly experimentService: ExperimentService, private readonly metricHelperService: MetricHelperService, private readonly analysisService: AnalysisService, + private readonly notificationService: NotificationService, private readonly cdr: ChangeDetectorRef, public readonly dialogRef: MatDialogRef ) {} @@ -435,6 +438,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.subscriptions.add( this.experimentService.selectedExperiment$.subscribe((experiment) => { if (experiment) { + this.currentExperiment = experiment; this.currentAssignmentUnit = experiment.assignmentUnit; this.currentContext = experiment.context; this.updateMetricTypeAvailability(); @@ -448,6 +452,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.experimentService.experiments$.subscribe((experiments) => { const experiment = experiments.find((exp) => exp.id === this.config.params.experimentId); if (experiment && !this.currentAssignmentUnit) { + this.currentExperiment = experiment; this.currentAssignmentUnit = experiment.assignmentUnit; this.currentContext = experiment.context; this.updateMetricTypeAvailability(); @@ -931,29 +936,42 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { const formValue = this.metricForm.value; const metricData = this.prepareMetricDataForBackend(formValue); - // Get current experiment and call helper service - this.experimentService.selectedExperiment$.pipe(take(1)).subscribe((experiment: Experiment) => { - if (!experiment) { - console.error('No experiment selected'); - return; - } + const experiment = this.currentExperiment; - if (this.config.params.action === UPSERT_EXPERIMENT_ACTION.ADD) { - this.metricHelperService.addMetric(experiment, metricData); - } else { - const sourceQuery = this.config.params.sourceQuery; - if (!sourceQuery) { - console.error('No source query for edit action'); - return; - } + if (experiment) { + this.executeMetricUpsert(experiment, metricData); + return; + } - this.metricHelperService.updateMetric(experiment, sourceQuery, metricData); + this.experimentService.selectedExperiment$.pipe(take(1)).subscribe((selectedExperiment) => { + if (!selectedExperiment) { + this.notificationService.showError('Unable to load the selected experiment. Please refresh and try again.'); + return; } - this.closeModal(); + const resolvedExperiment = selectedExperiment as Experiment; + this.currentExperiment = resolvedExperiment; + this.executeMetricUpsert(resolvedExperiment, metricData); }); } + private executeMetricUpsert(experiment: Experiment, metricData: ExperimentQueryDTO): void { + if (this.config.params.action === UPSERT_EXPERIMENT_ACTION.ADD) { + this.metricHelperService.addMetric(experiment, metricData); + this.closeModal(); + return; + } + + const sourceQuery = this.config.params.sourceQuery; + if (!sourceQuery) { + this.notificationService.showError('Unable to update the metric because required query details are missing.'); + return; + } + + this.metricHelperService.updateMetric(experiment, sourceQuery, metricData); + this.closeModal(); + } + private prepareMetricDataForBackend(formValue: any): ExperimentQueryDTO { const { metricType, metricClass, metricKey: metricKeyValue, metricId } = formValue; From 4baa8560405b0aad93c73775c47b53d61313f6ab Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 4 Dec 2025 19:16:11 -0500 Subject: [PATCH 25/30] Add missing import WeightingMethod --- .../upgrade/src/app/shared/services/common-dialog.service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts b/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts index 804fa6c05f..096e97c515 100644 --- a/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts +++ b/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts @@ -45,6 +45,7 @@ import { ExperimentCondition, ExperimentConditionPayload, ExperimentQueryDTO, + WeightingMethod, } from '../../core/experiments/store/experiments.model'; import { ConditionWeightUpdate, From 01fe8ec3c80f5f0faeee2af803892eecb53ef1eb Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 5 Dec 2025 07:40:46 -0500 Subject: [PATCH 26/30] Tighten the metric autocomplete typing --- .../upsert-metric-modal.component.ts | 191 +++++++++++------- 1 file changed, 121 insertions(+), 70 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index c86fa3be44..a3b5a5a13a 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -30,13 +30,30 @@ import { MetricHelperService } from '../../../../../core/experiments/metric-help import { AnalysisService } from '../../../../../core/analysis/analysis.service'; import { NotificationService } from '../../../../../core/notifications/notification.service'; import { METRICS_JOIN_TEXT } from '../../../../../core/analysis/store/analysis.models'; -import { ASSIGNMENT_UNIT, IMetricMetaData, METRIC_TYPE, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; +import { + ASSIGNMENT_UNIT, + IMetricMetaData, + IMetricUnit, + METRIC_TYPE, + OPERATION_TYPES, + REPEATED_MEASURE, +} from 'upgrade_types'; interface StatisticOption { value: string; label: string; } +type MetricNode = IMetricUnit; + +type MetricControlValue = MetricNode | string | string[] | null; + +interface MetricSelection { + classNode: MetricNode | null; + keyNode: MetricNode | null; + idNode: MetricNode | null; +} + @Component({ selector: 'upsert-metric-modal', imports: [ @@ -81,17 +98,17 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Autocomplete allMetrics$ = this.analysisService.allMetrics$; - allMetrics: any[] = []; + allMetrics: MetricNode[] = []; // Filtered autocomplete observables - filteredMetricClasses$: Observable; - filteredMetricKeys$: Observable; - filteredMetricIds$: Observable; + filteredMetricClasses$: Observable; + filteredMetricKeys$: Observable; + filteredMetricIds$: Observable; // BehaviorSubjects for source data - private readonly metricClassOptions$ = new BehaviorSubject([]); - private readonly metricKeyOptions$ = new BehaviorSubject([]); - private readonly metricIdOptions$ = new BehaviorSubject([]); + private readonly metricClassOptions$ = new BehaviorSubject([]); + private readonly metricKeyOptions$ = new BehaviorSubject([]); + private readonly metricIdOptions$ = new BehaviorSubject([]); // Track if user has selected a valid option from autocomplete (vs just typing) private readonly hasValidMetricClassSelection$ = new BehaviorSubject(false); @@ -280,9 +297,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.updateFormWithMetricObjects(initialValues, metricObjects); // Only update validation state and initialize statistics if idObject exists - if (metricObjects.idObject) { + if (metricObjects.idNode) { this.updateValidationState(metricObjects); - this.initializeMetricTypeAndStatistics(metricObjects.idObject); + this.initializeMetricTypeAndStatistics(metricObjects.idNode); } this.cdr.markForCheck(); @@ -290,10 +307,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { ); } - private findMetricObjects( - metrics: any[], - initialValues: MetricFormData - ): { classObject: any; keyObject: any; idObject: any } { + private findMetricObjects(metrics: MetricNode[], initialValues: MetricFormData): MetricSelection { const { metricType, metricClass, metricKey, metricId } = initialValues; if (metricType === METRIC_TYPE.REPEATABLE) { @@ -302,58 +316,57 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Global metric: find the metric directly return { - classObject: null, - keyObject: null, - idObject: metrics.find((m) => m.key === metricId) ?? null, + classNode: null, + keyNode: null, + idNode: metrics.find((m) => this.extractKey(m) === metricId) ?? null, }; } private findRepeatableMetricObjects( - metrics: any[], + metrics: MetricNode[], metricClass: string, metricKey: string, metricId: string - ): { classObject: any; keyObject: any; idObject: any } { - const classObject = metrics.find((m) => m.key === metricClass) ?? null; + ): MetricSelection { + const classNode = metrics.find((m) => this.extractKey(m) === metricClass) ?? null; - if (!classObject?.children) { - return { classObject, keyObject: null, idObject: null }; + if (!classNode?.children) { + return { classNode, keyNode: null, idNode: null }; } - const keyObject = classObject.children.find((k: any) => k.key === metricKey) ?? null; + const keyNode = classNode.children?.find((k) => this.extractKey(k) === metricKey) ?? null; - if (!keyObject) { - return { classObject, keyObject: null, idObject: null }; + if (!keyNode) { + return { classNode, keyNode: null, idNode: null }; } // Find ID object in key's children, or use key itself if no children - const idObject = keyObject.children?.length - ? keyObject.children.find((id: any) => id.key === metricId) ?? null - : keyObject; + const idNode = keyNode.children?.length + ? keyNode.children.find((id) => this.extractKey(id) === metricId) ?? null + : keyNode; - return { classObject, keyObject, idObject }; + return { classNode, keyNode, idNode }; } - private updateFormWithMetricObjects( - initialValues: MetricFormData, - metricObjects: { classObject: any; keyObject: any; idObject: any } - ): void { - const { classObject, keyObject, idObject } = metricObjects; + private updateFormWithMetricObjects(initialValues: MetricFormData, metricObjects: MetricSelection): void { + const { classNode, keyNode, idNode } = metricObjects; // Update form with found objects (fallback to string values if not found) - const formUpdates = { - metricClass: classObject ?? initialValues.metricClass, - metricKey: keyObject ?? initialValues.metricKey, - metricId: idObject ?? initialValues.metricId, + const formControlUpdates = { + metricClass: classNode ?? initialValues.metricClass, + metricKey: keyNode ?? initialValues.metricKey, + metricId: idNode ?? initialValues.metricId, }; - this.metricForm.patchValue(formUpdates); + this.metricForm.patchValue(formControlUpdates); this.populateOptions(); // Update initial form values once with all necessary data const updatedInitialValues: MetricFormData = { ...this.initialFormValues$.value, - ...formUpdates, + metricClass: classNode ? this.extractKey(classNode) : initialValues.metricClass, + metricKey: keyNode ? this.extractKey(keyNode) : initialValues.metricKey, + metricId: idNode ? this.extractKey(idNode) : initialValues.metricId, // Include allowableDataKeys if available (for categorical metrics) allowableDataKeys: this.allowableDataKeys.length > 0 ? this.allowableDataKeys : initialValues.allowableDataKeys, }; @@ -363,23 +376,23 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - private updateValidationState(metricObjects: { classObject: any; keyObject: any; idObject: any }): void { - const { classObject, keyObject, idObject } = metricObjects; + private updateValidationState(metricObjects: MetricSelection): void { + const { classNode, keyNode, idNode } = metricObjects; // Mark selections as valid based on found objects - if (classObject) { + if (classNode) { this.hasValidMetricClassSelection$.next(true); } - if (keyObject) { + if (keyNode) { this.hasValidMetricKeySelection$.next(true); } - if (idObject) { + if (idNode) { this.hasValidMetricIdSelection$.next(true); } } - private initializeMetricTypeAndStatistics(idObject: any): void { - this.detectMetricDataType(idObject); + private initializeMetricTypeAndStatistics(idNode: MetricControlValue): void { + this.detectMetricDataType(idNode); this.updateStatisticOptions(); this.clearInvalidStatisticSelections(); this.updateFormVisibility(); @@ -496,7 +509,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); } - onMetricClassValueChange(selectedClass: any): void { + onMetricClassValueChange(selectedClass: MetricControlValue): void { // If user is typing (string value) after selecting an option, invalidate the selection if (typeof selectedClass === 'string' && this.hasValidMetricClassSelection$.getValue()) { this.hasValidMetricClassSelection$.next(false); @@ -521,7 +534,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - onMetricClassOptionSelected(selectedClass: any): void { + onMetricClassOptionSelected(selectedClass: MetricNode): void { if (selectedClass && typeof selectedClass === 'object' && selectedClass.children) { this.hasValidMetricClassSelection$.next(true); const classChildren = Array.isArray(selectedClass.children) ? selectedClass.children : []; @@ -536,7 +549,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - onMetricKeyValueChange(selectedKey: any): void { + onMetricKeyValueChange(selectedKey: MetricControlValue): void { // If user is typing (string value) after selecting an option, invalidate the selection if (typeof selectedKey === 'string' && this.hasValidMetricKeySelection$.getValue()) { this.hasValidMetricKeySelection$.next(false); @@ -555,7 +568,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - onMetricKeyOptionSelected(selectedKey: any): void { + onMetricKeyOptionSelected(selectedKey: MetricNode): void { if (selectedKey && typeof selectedKey === 'object') { this.hasValidMetricKeySelection$.next(true); @@ -568,38 +581,71 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - displayFn = (option?: any): string => { - return option?.key || option || ''; + displayFn = (option?: MetricControlValue): string => { + if (!option) { + return ''; + } + + if (typeof option === 'string') { + return option; + } + + if (Array.isArray(option)) { + return option.join(', '); + } + + const key = Array.isArray(option.key) ? option.key.join(METRICS_JOIN_TEXT) : option.key; + return key || ''; }; - private extractKey(value: any): string { - return typeof value === 'object' ? value?.key || '' : value || ''; + private extractKey(value: MetricControlValue): string { + if (!value) { + return ''; + } + + if (typeof value === 'string') { + return value; + } + + if (Array.isArray(value)) { + return value.join(METRICS_JOIN_TEXT); + } + + const key = Array.isArray(value.key) ? value.key.join(METRICS_JOIN_TEXT) : value.key; + return key || ''; } - private _filter(value: any, options: any[]): any[] { + private _filter(value: MetricControlValue, options: MetricNode[]): MetricNode[] { const filterValue = this.extractKey(value).toLowerCase(); - return options.filter((option) => option.key?.toLowerCase().includes(filterValue)); + return options.filter((option) => { + const optionKey = Array.isArray(option.key) ? option.key.join(METRICS_JOIN_TEXT) : option.key; + return optionKey?.toLowerCase().includes(filterValue) ?? false; + }); } - private findOptionByKey(options: any[], key: string): any { + private findOptionByKey(options: MetricNode[], key: string): MetricNode | undefined { if (!options.length || !key) { return undefined; } return options.find((option) => option?.key === key); } - private resolveSelectedOption(controlValue: any, options: any[] = []): any { + private resolveSelectedOption(controlValue: MetricControlValue, options: MetricNode[] = []): MetricNode | undefined { if (!controlValue) { return undefined; } + if (Array.isArray(controlValue)) { + return this.findOptionByKey(options, controlValue.join(METRICS_JOIN_TEXT)); + } + if (typeof controlValue === 'object') { return controlValue; } return this.findOptionByKey(options, this.extractKey(controlValue)); } - private getMetricIdOptionsFromKey(selectedKey: any): any[] { - if (!selectedKey || typeof selectedKey !== 'object') { + private getMetricIdOptionsFromKey(selectedKey: MetricControlValue): MetricNode[] { + if (!selectedKey || typeof selectedKey !== 'object' || Array.isArray(selectedKey)) { return []; } @@ -610,7 +656,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return [selectedKey]; } - private filterMetricsByAssignmentContext(metrics: any[]): any[] { + private filterMetricsByAssignmentContext(metrics: MetricNode[]): MetricNode[] { if (!metrics?.length) { return []; } @@ -632,14 +678,14 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return metrics; } - private populateGlobalMetricOptions(metrics: any[]): void { + private populateGlobalMetricOptions(metrics: MetricNode[]): void { this.metricClassOptions$.next([]); this.metricKeyOptions$.next([]); const globalMetrics = metrics.filter((metric) => !metric.children || metric.children.length === 0); this.metricIdOptions$.next(globalMetrics); } - private populateRepeatableMetricOptions(metrics: any[]): void { + private populateRepeatableMetricOptions(metrics: MetricNode[]): void { const repeatableMetrics = metrics.filter((metric) => metric.children && metric.children.length > 0); this.metricClassOptions$.next(repeatableMetrics); @@ -685,7 +731,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.updateFormValidators(); } - onMetricIdValueChange(metricId: any): void { + onMetricIdValueChange(metricId: MetricControlValue): void { // If user is typing (string value) after selecting an option, invalidate the selection if (typeof metricId === 'string' && this.hasValidMetricIdSelection$.getValue()) { this.hasValidMetricIdSelection$.next(false); @@ -705,7 +751,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - onMetricIdOptionSelected(metricId: any): void { + onMetricIdOptionSelected(metricId: MetricNode): void { // This is called when user actually selects an option from autocomplete if (metricId && typeof metricId === 'object') { this.hasValidMetricIdSelection$.next(true); @@ -730,8 +776,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.detectMetricTypeByHeuristic(metricId); } - private findSelectedMetric(metricId: any): any { - if (typeof metricId === 'object' && metricId.metadata) { + private findSelectedMetric(metricId: MetricControlValue): MetricNode | null { + if (metricId && typeof metricId === 'object' && !Array.isArray(metricId) && metricId.metadata) { return metricId; } @@ -740,10 +786,15 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return currentOptions.find((metric) => metric.key === metricId); } + if (Array.isArray(metricId)) { + const joinedKey = metricId.join(METRICS_JOIN_TEXT); + return this.metricIdOptions$.getValue().find((metric) => this.extractKey(metric) === joinedKey) ?? null; + } + return null; } - private setMetricDataType(dataType: IMetricMetaData, selectedMetric?: any): void { + private setMetricDataType(dataType: IMetricMetaData, selectedMetric?: MetricNode): void { this.metricDataType = dataType; if (dataType === IMetricMetaData.CATEGORICAL) { @@ -753,7 +804,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - private detectMetricTypeByHeuristic(metricId: any): void { + private detectMetricTypeByHeuristic(metricId: MetricControlValue): void { const metricKey = this.extractKey(metricId); const lowerMetricKey = metricKey.toLowerCase(); From 1b2eb293ae217564be86b1a4701fe02d0057dbda Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 5 Dec 2025 08:04:57 -0500 Subject: [PATCH 27/30] Typed the form pipeline so the modal now works with explicit metric shapes instead of any --- .../upsert-metric-modal.component.ts | 234 ++++++++++++------ 1 file changed, 158 insertions(+), 76 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index a3b5a5a13a..6cf5a412cb 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -1,5 +1,12 @@ import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, OnDestroy, OnInit } from '@angular/core'; -import { FormBuilder, FormGroup, ReactiveFormsModule, Validators } from '@angular/forms'; +import { + AbstractControl, + FormBuilder, + FormGroup, + ReactiveFormsModule, + ValidationErrors, + Validators, +} from '@angular/forms'; import { MAT_DIALOG_DATA, MatDialogRef } from '@angular/material/dialog'; import { BehaviorSubject, Observable, Subscription, combineLatest } from 'rxjs'; import { combineLatestWith, filter, map, startWith, take } from 'rxjs/operators'; @@ -44,6 +51,12 @@ interface StatisticOption { label: string; } +interface ExperimentQueryDetails { + operationType?: string; + compareFn?: string; + compareValue?: string; +} + type MetricNode = IMetricUnit; type MetricControlValue = MetricNode | string | string[] | null; @@ -54,6 +67,28 @@ interface MetricSelection { idNode: MetricNode | null; } +interface MetricFormValueBase { + metricType: METRIC_TYPE; + metricId: MetricControlValue; + displayName: string; + metricClass: MetricControlValue; + metricKey: MetricControlValue; + aggregateStatistic: string; + individualStatistic: REPEATED_MEASURE | ''; + comparison: string; + compareValue: string; +} + +interface GlobalMetricFormValue extends MetricFormValueBase { + metricType: METRIC_TYPE.GLOBAL; +} + +interface RepeatableMetricFormValue extends MetricFormValueBase { + metricType: METRIC_TYPE.REPEATABLE; +} + +type MetricFormValue = GlobalMetricFormValue | RepeatableMetricFormValue; + @Component({ selector: 'upsert-metric-modal', imports: [ @@ -165,6 +200,40 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { public readonly dialogRef: MatDialogRef ) {} + private getCurrentFormValue(): MetricFormValue { + return this.metricForm.getRawValue() as MetricFormValue; + } + + private isRepeatableFormValue(value: MetricFormValue): value is RepeatableMetricFormValue { + return value.metricType === METRIC_TYPE.REPEATABLE; + } + + private getNodeKey(node: MetricNode | null | undefined): string { + if (!node) { + return ''; + } + + return Array.isArray(node.key) ? node.key.join(METRICS_JOIN_TEXT) : node.key ?? ''; + } + + private toMetricFormData( + formValue: MetricFormValue, + allowableDataKeys: string[] = this.allowableDataKeys + ): MetricFormData { + return { + metricType: formValue.metricType, + metricId: this.extractKey(formValue.metricId), + displayName: formValue.displayName, + metricClass: this.extractKey(formValue.metricClass), + metricKey: this.extractKey(formValue.metricKey), + aggregateStatistic: formValue.aggregateStatistic, + individualStatistic: formValue.individualStatistic, + comparison: formValue.comparison, + compareValue: formValue.compareValue, + allowableDataKeys, + }; + } + ngOnInit(): void { this.createMetricForm(); this.setupFormChangeListeners(); @@ -181,7 +250,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } // Custom validator to ensure fields are selected from options (object) not typed (string) - private autocompleteSelectionValidator(control: any): { [key: string]: any } | null { + private autocompleteSelectionValidator(control: AbstractControl): ValidationErrors | null { const value = control.value; if (!value) { return null; // Let required validator handle empty values @@ -227,9 +296,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - deriveInitialFormValues(sourceQuery: any, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { + deriveInitialFormValues(sourceQuery: ExperimentQueryDTO | null, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { const metricKey = sourceQuery.metric?.key || ''; + const queryDetails = (sourceQuery.query as ExperimentQueryDetails) || {}; // The correct way to determine if it's repeatable is by checking if the metric key contains METRICS_JOIN_TEXT // NOT by checking if repeatedMeasure exists (global metrics can also have individual statistics) @@ -261,10 +331,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { displayName: sourceQuery.name || '', metricClass, metricKey: metricKeyValue, - aggregateStatistic: sourceQuery.query?.operationType || '', + aggregateStatistic: queryDetails.operationType || '', individualStatistic: sourceQuery.repeatedMeasure || '', - comparison: sourceQuery.query?.compareFn || '=', - compareValue: sourceQuery.query?.compareValue || '', + comparison: queryDetails.compareFn || '=', + compareValue: queryDetails.compareValue || '', allowableDataKeys: [], }; } @@ -362,17 +432,13 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.populateOptions(); // Update initial form values once with all necessary data - const updatedInitialValues: MetricFormData = { - ...this.initialFormValues$.value, - metricClass: classNode ? this.extractKey(classNode) : initialValues.metricClass, - metricKey: keyNode ? this.extractKey(keyNode) : initialValues.metricKey, - metricId: idNode ? this.extractKey(idNode) : initialValues.metricId, - // Include allowableDataKeys if available (for categorical metrics) - allowableDataKeys: this.allowableDataKeys.length > 0 ? this.allowableDataKeys : initialValues.allowableDataKeys, - }; + const normalizedInitialValues = this.toMetricFormData( + this.getCurrentFormValue(), + this.allowableDataKeys.length > 0 ? this.allowableDataKeys : initialValues.allowableDataKeys ?? [] + ); - if (!isEqual(this.initialFormValues$.value, updatedInitialValues)) { - this.initialFormValues$.next(updatedInitialValues); + if (!isEqual(this.initialFormValues$.value, normalizedInitialValues)) { + this.initialFormValues$.next(normalizedInitialValues); } } @@ -420,19 +486,19 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.subscriptions.add( this.metricForm.get('metricClass')?.valueChanges.subscribe((selectedClass) => { - this.onMetricClassValueChange(selectedClass); + this.onMetricClassValueChange(selectedClass as MetricControlValue); }) ); this.subscriptions.add( this.metricForm.get('metricKey')?.valueChanges.subscribe((selectedKey) => { - this.onMetricKeyValueChange(selectedKey); + this.onMetricKeyValueChange(selectedKey as MetricControlValue); }) ); this.subscriptions.add( this.metricForm.get('metricId')?.valueChanges.subscribe((metricId) => { - this.onMetricIdValueChange(metricId); + this.onMetricIdValueChange(metricId as MetricControlValue); }) ); } @@ -440,7 +506,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { setupAutocomplete(): void { this.subscriptions.add( this.allMetrics$.subscribe((metrics) => { - this.allMetrics = metrics || []; + this.allMetrics = Array.isArray(metrics) ? (metrics as MetricNode[]) : []; this.populateOptions(); this.createFilteredObservables(); }) @@ -481,7 +547,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return; } - const metricType = this.metricForm.get('metricType')?.value; + const { metricType } = this.getCurrentFormValue(); const filteredMetrics = this.filterMetricsByAssignmentContext(this.allMetrics || []); if (metricType === METRIC_TYPE.GLOBAL) { @@ -493,25 +559,41 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } createFilteredObservables(): void { - this.filteredMetricClasses$ = combineLatest([ - this.metricForm.get('metricClass')?.valueChanges.pipe(startWith('')) || new BehaviorSubject(''), - this.metricClassOptions$, - ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); - - this.filteredMetricKeys$ = combineLatest([ - this.metricForm.get('metricKey')?.valueChanges.pipe(startWith('')) || new BehaviorSubject(''), - this.metricKeyOptions$, - ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); - - this.filteredMetricIds$ = combineLatest([ - this.metricForm.get('metricId')?.valueChanges.pipe(startWith('')) || new BehaviorSubject(''), - this.metricIdOptions$, - ]).pipe(map(([searchValue, options]) => this._filter(searchValue || '', options))); + const metricClassControl = this.metricForm.get('metricClass'); + const metricKeyControl = this.metricForm.get('metricKey'); + const metricIdControl = this.metricForm.get('metricId'); + + const metricClassChanges$ = metricClassControl + ? metricClassControl.valueChanges.pipe(startWith((metricClassControl.value ?? null) as MetricControlValue)) + : new BehaviorSubject(null); + + const metricKeyChanges$ = metricKeyControl + ? metricKeyControl.valueChanges.pipe(startWith((metricKeyControl.value ?? null) as MetricControlValue)) + : new BehaviorSubject(null); + + const metricIdChanges$ = metricIdControl + ? metricIdControl.valueChanges.pipe(startWith((metricIdControl.value ?? null) as MetricControlValue)) + : new BehaviorSubject(null); + + this.filteredMetricClasses$ = combineLatest([metricClassChanges$, this.metricClassOptions$]).pipe( + map(([searchValue, options]) => this._filter(searchValue as MetricControlValue, options)) + ); + + this.filteredMetricKeys$ = combineLatest([metricKeyChanges$, this.metricKeyOptions$]).pipe( + map(([searchValue, options]) => this._filter(searchValue as MetricControlValue, options)) + ); + + this.filteredMetricIds$ = combineLatest([metricIdChanges$, this.metricIdOptions$]).pipe( + map(([searchValue, options]) => this._filter(searchValue as MetricControlValue, options)) + ); } onMetricClassValueChange(selectedClass: MetricControlValue): void { // If user is typing (string value) after selecting an option, invalidate the selection - if (typeof selectedClass === 'string' && this.hasValidMetricClassSelection$.getValue()) { + if ( + (typeof selectedClass === 'string' || Array.isArray(selectedClass)) && + this.hasValidMetricClassSelection$.getValue() + ) { this.hasValidMetricClassSelection$.next(false); // Clear dependent fields when parent becomes invalid this.metricKeyOptions$.next([]); @@ -537,7 +619,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onMetricClassOptionSelected(selectedClass: MetricNode): void { if (selectedClass && typeof selectedClass === 'object' && selectedClass.children) { this.hasValidMetricClassSelection$.next(true); - const classChildren = Array.isArray(selectedClass.children) ? selectedClass.children : []; + const classChildren: MetricNode[] = Array.isArray(selectedClass.children) ? selectedClass.children : []; this.metricKeyOptions$.next(classChildren); // Reset dependent fields @@ -551,7 +633,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onMetricKeyValueChange(selectedKey: MetricControlValue): void { // If user is typing (string value) after selecting an option, invalidate the selection - if (typeof selectedKey === 'string' && this.hasValidMetricKeySelection$.getValue()) { + if ( + (typeof selectedKey === 'string' || Array.isArray(selectedKey)) && + this.hasValidMetricKeySelection$.getValue() + ) { this.hasValidMetricKeySelection$.next(false); // Clear dependent fields when parent becomes invalid this.metricIdOptions$.next([]); @@ -594,8 +679,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return option.join(', '); } - const key = Array.isArray(option.key) ? option.key.join(METRICS_JOIN_TEXT) : option.key; - return key || ''; + const key = this.getNodeKey(option); + return key.includes(METRICS_JOIN_TEXT) ? key.split(METRICS_JOIN_TEXT).join(' / ') : key; }; private extractKey(value: MetricControlValue): string { @@ -611,14 +696,13 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return value.join(METRICS_JOIN_TEXT); } - const key = Array.isArray(value.key) ? value.key.join(METRICS_JOIN_TEXT) : value.key; - return key || ''; + return this.getNodeKey(value); } private _filter(value: MetricControlValue, options: MetricNode[]): MetricNode[] { const filterValue = this.extractKey(value).toLowerCase(); return options.filter((option) => { - const optionKey = Array.isArray(option.key) ? option.key.join(METRICS_JOIN_TEXT) : option.key; + const optionKey = this.getNodeKey(option); return optionKey?.toLowerCase().includes(filterValue) ?? false; }); } @@ -627,7 +711,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { if (!options.length || !key) { return undefined; } - return options.find((option) => option?.key === key); + return options.find((option) => this.getNodeKey(option) === key); } private resolveSelectedOption(controlValue: MetricControlValue, options: MetricNode[] = []): MetricNode | undefined { @@ -650,7 +734,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } if (selectedKey.children && selectedKey.children.length > 0) { - return selectedKey.children; + return Array.isArray(selectedKey.children) ? selectedKey.children : []; } return [selectedKey]; @@ -689,17 +773,19 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { const repeatableMetrics = metrics.filter((metric) => metric.children && metric.children.length > 0); this.metricClassOptions$.next(repeatableMetrics); - const selectedClass = this.resolveSelectedOption(this.metricForm.get('metricClass')?.value, repeatableMetrics); + const selectedClassValue = this.metricForm.get('metricClass')?.value as MetricControlValue; + const selectedClass = this.resolveSelectedOption(selectedClassValue, repeatableMetrics); if (!selectedClass?.children?.length) { this.metricKeyOptions$.next([]); this.metricIdOptions$.next([]); return; } - const classChildren = Array.isArray(selectedClass.children) ? selectedClass.children : []; + const classChildren: MetricNode[] = Array.isArray(selectedClass.children) ? selectedClass.children : []; this.metricKeyOptions$.next(classChildren); - const selectedKey = this.resolveSelectedOption(this.metricForm.get('metricKey')?.value, classChildren); + const selectedKeyValue = this.metricForm.get('metricKey')?.value as MetricControlValue; + const selectedKey = this.resolveSelectedOption(selectedKeyValue, classChildren); if (!selectedKey) { this.metricIdOptions$.next([]); return; @@ -732,8 +818,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } onMetricIdValueChange(metricId: MetricControlValue): void { - // If user is typing (string value) after selecting an option, invalidate the selection - if (typeof metricId === 'string' && this.hasValidMetricIdSelection$.getValue()) { + // If user is typing (string or string[] value) after selecting an option, invalidate the selection + if ((typeof metricId === 'string' || Array.isArray(metricId)) && this.hasValidMetricIdSelection$.getValue()) { this.hasValidMetricIdSelection$.next(false); this.metricDataType = null; this.hideStatisticDropdowns(); @@ -763,7 +849,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - detectMetricDataType(metricId: any): void { + detectMetricDataType(metricId: MetricControlValue): void { const selectedMetric = this.findSelectedMetric(metricId); // Use metadata if available @@ -783,12 +869,12 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { if (typeof metricId === 'string') { const currentOptions = this.metricIdOptions$.getValue(); - return currentOptions.find((metric) => metric.key === metricId); + return currentOptions.find((metric) => this.getNodeKey(metric) === metricId) ?? null; } if (Array.isArray(metricId)) { const joinedKey = metricId.join(METRICS_JOIN_TEXT); - return this.metricIdOptions$.getValue().find((metric) => this.extractKey(metric) === joinedKey) ?? null; + return this.metricIdOptions$.getValue().find((metric) => this.getNodeKey(metric) === joinedKey) ?? null; } return null; @@ -822,7 +908,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } updateFormVisibility(): void { - const metricType = this.metricForm.get('metricType')?.value; + const { metricType } = this.getCurrentFormValue(); // Base visibility for metric type this.showMetricClass = metricType === METRIC_TYPE.REPEATABLE; @@ -848,7 +934,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.isGlobalMetricDisabled = this.currentAssignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS; // If global metrics are disabled and global is currently selected, switch to repeatable - if (this.isGlobalMetricDisabled && this.metricForm.get('metricType')?.value === METRIC_TYPE.GLOBAL) { + const { metricType } = this.getCurrentFormValue(); + if (this.isGlobalMetricDisabled && metricType === METRIC_TYPE.GLOBAL) { this.metricForm.get('metricType')?.setValue(METRIC_TYPE.REPEATABLE); } @@ -906,7 +993,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } updateFormValidators(): void { - const metricType = this.metricForm.get('metricType')?.value; + const { metricType } = this.getCurrentFormValue(); // Update validators based on metric type if (metricType === METRIC_TYPE.REPEATABLE) { @@ -951,14 +1038,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { listenForIsInitialFormValueChanged(): void { this.isInitialFormValueChanged$ = this.metricForm.valueChanges.pipe( - startWith(this.metricForm.value), - map(() => { - const currentWithKeys = { - ...this.metricForm.value, - allowableDataKeys: this.allowableDataKeys, - }; - return !isEqual(currentWithKeys, this.initialFormValues$.value); - }) + startWith(this.getCurrentFormValue()), + map((value) => value as MetricFormValue), + map((formValue) => this.toMetricFormData(formValue)), + map((normalizedValue) => !isEqual(normalizedValue, this.initialFormValues$.value)) ); } @@ -984,7 +1067,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } sendUpsertMetricRequest(): void { - const formValue = this.metricForm.value; + const formValue = this.getCurrentFormValue(); const metricData = this.prepareMetricDataForBackend(formValue); const experiment = this.currentExperiment; @@ -1023,16 +1106,16 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.closeModal(); } - private prepareMetricDataForBackend(formValue: any): ExperimentQueryDTO { - const { metricType, metricClass, metricKey: metricKeyValue, metricId } = formValue; + private prepareMetricDataForBackend(formValue: MetricFormValue): ExperimentQueryDTO { + const metricKey = this.isRepeatableFormValue(formValue) + ? `${this.extractKey(formValue.metricClass)}${METRICS_JOIN_TEXT}${this.extractKey( + formValue.metricKey + )}${METRICS_JOIN_TEXT}${this.extractKey(formValue.metricId)}` + : this.extractKey(formValue.metricId); - // Prepare metric key based on type - const metricKey = - metricType === METRIC_TYPE.GLOBAL - ? this.extractKey(metricId) - : `${this.extractKey(metricClass)}${METRICS_JOIN_TEXT}${this.extractKey( - metricKeyValue - )}${METRICS_JOIN_TEXT}${this.extractKey(metricId)}`; + const repeatedMeasure = this.isRepeatableFormValue(formValue) + ? formValue.individualStatistic || REPEATED_MEASURE.mostRecent + : REPEATED_MEASURE.mostRecent; // Prepare query object const queryObj: ExperimentQueryDTO = { @@ -1050,8 +1133,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metric: { key: metricKey, }, - repeatedMeasure: - metricType === METRIC_TYPE.REPEATABLE ? formValue.individualStatistic : REPEATED_MEASURE.mostRecent, + repeatedMeasure, }; return queryObj; From a6895c265ab5a044b6bf47098cd5508b69c4a637 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 5 Dec 2025 08:31:56 -0500 Subject: [PATCH 28/30] Type the backend and frontend around experiment queries by sharing a single ExperimentQueryPayload contract and wiring the modal to the new comparator typing --- .../packages/Upgrade/src/api/models/Query.ts | 4 +- .../core/analysis/store/analysis.models.ts | 7 +-- .../experiments/store/experiments.model.ts | 9 ++-- .../upsert-metric-modal.component.ts | 49 ++++++++++--------- types/src/Experiment/interfaces.ts | 9 ++++ types/src/index.ts | 2 + 6 files changed, 47 insertions(+), 33 deletions(-) diff --git a/backend/packages/Upgrade/src/api/models/Query.ts b/backend/packages/Upgrade/src/api/models/Query.ts index ef639b1975..da84367b51 100644 --- a/backend/packages/Upgrade/src/api/models/Query.ts +++ b/backend/packages/Upgrade/src/api/models/Query.ts @@ -3,7 +3,7 @@ import { BaseModel } from './base/BaseModel'; import { Metric } from './Metric'; import { Experiment } from './Experiment'; import { IsDefined } from 'class-validator'; -import { REPEATED_MEASURE } from 'upgrade_types'; +import { ExperimentQueryPayload, REPEATED_MEASURE } from 'upgrade_types'; import { ArchivedStats } from './ArchivedStats'; import { Type } from 'class-transformer'; @@ -17,7 +17,7 @@ export class Query extends BaseModel { public name: string; @Column('jsonb') - public query: any; + public query: ExperimentQueryPayload; @ManyToOne(() => Metric, (metric) => metric.key, { onDelete: 'CASCADE' }) public metric: Metric; diff --git a/frontend/projects/upgrade/src/app/core/analysis/store/analysis.models.ts b/frontend/projects/upgrade/src/app/core/analysis/store/analysis.models.ts index ec878e2d43..04d77430b1 100644 --- a/frontend/projects/upgrade/src/app/core/analysis/store/analysis.models.ts +++ b/frontend/projects/upgrade/src/app/core/analysis/store/analysis.models.ts @@ -1,7 +1,8 @@ import { AppState } from '../../core.module'; -import { OPERATION_TYPES, IMetricMetaData, IMetricUnit, REPEATED_MEASURE } from 'upgrade_types'; +import { IMetricUnit, ExperimentQueryPayload } from 'upgrade_types'; +import type { REPEATED_MEASURE } from 'upgrade_types'; -export { OPERATION_TYPES, IMetricMetaData, REPEATED_MEASURE }; +export { OPERATION_TYPES, IMetricMetaData, REPEATED_MEASURE } from 'upgrade_types'; export const METRICS_JOIN_TEXT = '@__@'; @@ -17,7 +18,7 @@ export interface UpsertMetrics { export interface Query { name: string; - query: any; + query: ExperimentQueryPayload; metric: { key: string; }; diff --git a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index d4fca350ac..6be2e77214 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts @@ -8,7 +8,6 @@ import { EXPERIMENT_SORT_KEY, SORT_AS_DIRECTION, EXPERIMENT_STATE, - IExperimentEnrollmentStats, IExperimentSearchParams, IExperimentSortParams, IExperimentEnrollmentDetailStats, @@ -25,6 +24,8 @@ import { SEGMENT_TYPE, IEnrollmentCompleteCondition, METRIC_TYPE, + ExperimentQueryPayload, + ExperimentQueryComparator, } from 'upgrade_types'; import { Segment } from '../../segments/store/segments.model'; @@ -43,7 +44,7 @@ export { IExperimentEnrollmentDetailStats, DATE_RANGE, METRIC_TYPE, -}; +} from 'upgrade_types'; export interface ExperimentConditionFilterOptions { code: string; @@ -383,7 +384,7 @@ export interface MetricFormData { metricKey?: string; // For repeatable metrics only aggregateStatistic?: string; individualStatistic?: string; // For repeatable metrics only - comparison?: string; + comparison?: ExperimentQueryComparator; compareValue?: string; allowableDataKeys?: string[]; // For categorical metrics only } @@ -431,7 +432,7 @@ export interface ExperimentConditionPayloadDTO { export interface ExperimentQueryDTO { id?: string; name: string; - query: object; + query: ExperimentQueryPayload; metric: { key: string; }; diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index 6cf5a412cb..d74ad2d30f 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -44,6 +44,8 @@ import { METRIC_TYPE, OPERATION_TYPES, REPEATED_MEASURE, + ExperimentQueryPayload, + ExperimentQueryComparator, } from 'upgrade_types'; interface StatisticOption { @@ -51,12 +53,6 @@ interface StatisticOption { label: string; } -interface ExperimentQueryDetails { - operationType?: string; - compareFn?: string; - compareValue?: string; -} - type MetricNode = IMetricUnit; type MetricControlValue = MetricNode | string | string[] | null; @@ -73,9 +69,9 @@ interface MetricFormValueBase { displayName: string; metricClass: MetricControlValue; metricKey: MetricControlValue; - aggregateStatistic: string; + aggregateStatistic: OPERATION_TYPES | ''; individualStatistic: REPEATED_MEASURE | ''; - comparison: string; + comparison: ExperimentQueryComparator | ''; compareValue: string; } @@ -156,7 +152,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { private currentExperiment: Experiment | null = null; allowableDataKeys: string[] = []; - comparisonOptions = [ + comparisonOptions: Array<{ value: ExperimentQueryComparator; label: string }> = [ { value: '=', label: 'Equal' }, { value: '<>', label: 'Not equal' }, ]; @@ -228,7 +224,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metricKey: this.extractKey(formValue.metricKey), aggregateStatistic: formValue.aggregateStatistic, individualStatistic: formValue.individualStatistic, - comparison: formValue.comparison, + comparison: formValue.comparison || undefined, compareValue: formValue.compareValue, allowableDataKeys, }; @@ -299,7 +295,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { deriveInitialFormValues(sourceQuery: ExperimentQueryDTO | null, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { const metricKey = sourceQuery.metric?.key || ''; - const queryDetails = (sourceQuery.query as ExperimentQueryDetails) || {}; + const aggregateStatistic = sourceQuery.query?.operationType ?? ''; + const comparison = sourceQuery.query?.compareFn ?? '='; + const compareValue = sourceQuery.query?.compareValue ?? ''; // The correct way to determine if it's repeatable is by checking if the metric key contains METRICS_JOIN_TEXT // NOT by checking if repeatedMeasure exists (global metrics can also have individual statistics) @@ -331,10 +329,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { displayName: sourceQuery.name || '', metricClass, metricKey: metricKeyValue, - aggregateStatistic: queryDetails.operationType || '', + aggregateStatistic, individualStatistic: sourceQuery.repeatedMeasure || '', - comparison: queryDetails.compareFn || '=', - compareValue: queryDetails.compareValue || '', + comparison, + compareValue, allowableDataKeys: [], }; } @@ -1117,19 +1115,22 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { ? formValue.individualStatistic || REPEATED_MEASURE.mostRecent : REPEATED_MEASURE.mostRecent; + const operationType = formValue.aggregateStatistic as OPERATION_TYPES; + + const queryPayload: ExperimentQueryPayload = { + operationType, + ...(this.metricDataType === IMetricMetaData.CATEGORICAL && + formValue.comparison && + formValue.compareValue && { + compareFn: formValue.comparison, + compareValue: formValue.compareValue, + }), + }; + // Prepare query object const queryObj: ExperimentQueryDTO = { name: formValue.displayName, - query: { - operationType: formValue.aggregateStatistic, - // Add comparison for categorical metrics - ...(this.metricDataType === IMetricMetaData.CATEGORICAL && - formValue.comparison && - formValue.compareValue && { - compareFn: formValue.comparison, - compareValue: formValue.compareValue, - }), - }, + query: queryPayload, metric: { key: metricKey, }, diff --git a/types/src/Experiment/interfaces.ts b/types/src/Experiment/interfaces.ts index b33d272cf1..d4e0a864cf 100644 --- a/types/src/Experiment/interfaces.ts +++ b/types/src/Experiment/interfaces.ts @@ -15,6 +15,7 @@ import { IMPORT_COMPATIBILITY_TYPE, SERVER_ERROR, EXPERIMENT_LIST_OPERATION, + OPERATION_TYPES, } from './enums'; export interface IEnrollmentCompleteCondition { userCount: number; @@ -148,6 +149,14 @@ export interface IMetricUnit { allowedData?: string[]; } +export type ExperimentQueryComparator = '=' | '<>' | '!='; + +export interface ExperimentQueryPayload { + operationType: OPERATION_TYPES; + compareFn?: ExperimentQueryComparator; + compareValue?: string; +} + export interface IFlagVariation { id: string; value: string; diff --git a/types/src/index.ts b/types/src/index.ts index 59fec415a6..326f9b50f8 100644 --- a/types/src/index.ts +++ b/types/src/index.ts @@ -84,6 +84,8 @@ export { FeatureFlagDeletedData, ValidatedImportResponse, DuplicateSegmentNameError, + ExperimentQueryPayload, + ExperimentQueryComparator, } from './Experiment/interfaces'; export { MoocletPolicyParametersDTO, From 47c343e1cb978625c37ffcd92f8489c780ed71d0 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 5 Dec 2025 09:18:24 -0500 Subject: [PATCH 29/30] Add unit tests for UpsertMetricModalComponent --- .../upsert-metric-modal.component.spec.ts | 209 ++++++++++++++++++ 1 file changed, 209 insertions(+) create mode 100644 frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.spec.ts diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.spec.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.spec.ts new file mode 100644 index 0000000000..92c8304234 --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.spec.ts @@ -0,0 +1,209 @@ +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { BehaviorSubject, of } from 'rxjs'; +import { ReactiveFormsModule, FormBuilder } from '@angular/forms'; +import { MAT_DIALOG_DATA, MatDialogRef } from '@angular/material/dialog'; + +import { UpsertMetricModalComponent } from './upsert-metric-modal.component'; +import { + Experiment, + ExperimentQueryDTO, + MetricFormData, + UpsertMetricParams, + UPSERT_EXPERIMENT_ACTION, +} from '../../../../../core/experiments/store/experiments.model'; +import { METRICS_JOIN_TEXT } from '../../../../../core/analysis/store/analysis.models'; +import { + ExperimentQueryComparator, + IMetricMetaData, + IMetricUnit, + METRIC_TYPE, + OPERATION_TYPES, + REPEATED_MEASURE, +} from 'upgrade_types'; +import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; +import { AnalysisService } from '../../../../../core/analysis/analysis.service'; +import { ExperimentService } from '../../../../../core/experiments/experiments.service'; +import { MetricHelperService } from '../../../../../core/experiments/metric-helper.service'; +import { NotificationService } from '../../../../../core/notifications/notification.service'; + +describe('UpsertMetricModalComponent', () => { + let fixture: ComponentFixture; + let component: UpsertMetricModalComponent; + let analysisMetrics$: BehaviorSubject; + let experimentServiceStub: Partial; + let metricHelperServiceStub: MetricHelperService; + let notificationServiceStub: NotificationService; + + const defaultConfig: CommonModalConfig = { + title: 'Upsert Metric', + params: { + action: UPSERT_EXPERIMENT_ACTION.ADD, + experimentId: 'experiment-001', + sourceQuery: null, + }, + }; + + const dialogRefStub = { + close: jest.fn(), + } as unknown as MatDialogRef; + + const createComponent = (configOverride?: Partial>) => { + const mergedParams = configOverride?.params + ? { ...defaultConfig.params, ...configOverride.params } + : { ...defaultConfig.params }; + + const mergedConfig: CommonModalConfig = { + ...defaultConfig, + ...configOverride, + params: mergedParams, + }; + + TestBed.overrideProvider(MAT_DIALOG_DATA, { useValue: mergedConfig }); + + fixture = TestBed.createComponent(UpsertMetricModalComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + + return component; + }; + + beforeEach(async () => { + analysisMetrics$ = new BehaviorSubject([]); + + experimentServiceStub = { + isLoadingExperiment$: of(false), + selectedExperiment$: new BehaviorSubject(null), + experiments$: new BehaviorSubject([]), + updateExperimentMetrics: jest.fn(), + updateExperiment: jest.fn(), + }; + + metricHelperServiceStub = { + addMetric: jest.fn(), + updateMetric: jest.fn(), + } as unknown as MetricHelperService; + + notificationServiceStub = { + showError: jest.fn(), + } as unknown as NotificationService; + + await TestBed.configureTestingModule({ + imports: [UpsertMetricModalComponent, ReactiveFormsModule], + providers: [ + FormBuilder, + { provide: MAT_DIALOG_DATA, useValue: defaultConfig }, + { provide: MatDialogRef, useValue: dialogRefStub }, + { provide: AnalysisService, useValue: { allMetrics$: analysisMetrics$ } }, + { provide: ExperimentService, useValue: experimentServiceStub }, + { provide: MetricHelperService, useValue: metricHelperServiceStub }, + { provide: NotificationService, useValue: notificationServiceStub }, + ], + }) + .overrideComponent(UpsertMetricModalComponent, { + set: { template: '' }, + }) + .compileComponents(); + }); + + afterEach(() => { + TestBed.resetTestingModule(); + }); + + it('should initialize global metric defaults when adding a new metric', () => { + createComponent(); + + expect(component.metricForm.get('metricType')?.value).toBe(METRIC_TYPE.GLOBAL); + expect(component.metricForm.get('displayName')?.value).toBe(''); + expect(component.metricForm.get('comparison')?.value).toBe('='); + expect(component.metricForm.get('compareValue')?.value).toBe(''); + expect(component.showAggregateStatistic).toBe(false); + expect(component.showComparison).toBe(false); + }); + + it('should populate repeatable metric data when editing an existing query', () => { + const repeatableMetricTree: IMetricUnit = { + key: 'orders', + children: [ + { + key: 'status', + children: [ + { + key: 'completed', + metadata: { type: IMetricMetaData.CATEGORICAL }, + allowedData: ['GRADUATED', 'DROPPED'], + }, + ], + metadata: { type: IMetricMetaData.CATEGORICAL }, + }, + ], + metadata: { type: IMetricMetaData.CATEGORICAL }, + }; + + analysisMetrics$.next([repeatableMetricTree]); + + const sourceQuery: ExperimentQueryDTO = { + id: 'query-123', + name: 'Completion Rate', + metric: { + key: `orders${METRICS_JOIN_TEXT}status${METRICS_JOIN_TEXT}completed`, + }, + query: { + operationType: OPERATION_TYPES.COUNT, + compareFn: '=', + compareValue: 'GRADUATED', + }, + repeatedMeasure: REPEATED_MEASURE.mostRecent, + }; + + createComponent({ + params: { + action: UPSERT_EXPERIMENT_ACTION.EDIT, + experimentId: 'experiment-001', + sourceQuery, + currentContext: 'mathia', + }, + }); + + expect(component.metricForm.get('metricType')?.value).toBe(METRIC_TYPE.REPEATABLE); + + const metricClassControl = component.metricForm.get('metricClass')?.value as IMetricUnit; + expect(metricClassControl?.key).toBe('orders'); + + expect(component.metricForm.get('aggregateStatistic')?.value).toBe(OPERATION_TYPES.COUNT); + expect(component.metricForm.get('comparison')?.value as ExperimentQueryComparator).toBe('='); + expect(component.metricForm.get('compareValue')?.value).toBe('GRADUATED'); + expect(component.showAggregateStatistic).toBe(true); + expect(component.showComparison).toBe(true); + }); + + it('should build a categorical repeatable metric payload for the backend', () => { + createComponent(); + + component.metricDataType = IMetricMetaData.CATEGORICAL; + + const repeatableFormValue = { + metricType: METRIC_TYPE.REPEATABLE, + metricId: 'completed', + displayName: 'Completion Rate', + metricClass: 'orders', + metricKey: 'status', + aggregateStatistic: OPERATION_TYPES.COUNT, + individualStatistic: REPEATED_MEASURE.mean, + comparison: '=', + compareValue: 'GRADUATED', + } as MetricFormData & { + metricType: METRIC_TYPE.REPEATABLE; + aggregateStatistic: OPERATION_TYPES; + comparison: ExperimentQueryComparator; + }; + + const dto = (component as any).prepareMetricDataForBackend(repeatableFormValue); + + expect(dto.name).toBe('Completion Rate'); + expect(dto.metric.key).toBe(`orders${METRICS_JOIN_TEXT}status${METRICS_JOIN_TEXT}completed`); + expect(dto.query.operationType).toBe(OPERATION_TYPES.COUNT); + expect(dto.query.compareFn).toBe('='); + expect(dto.query.compareValue).toBe('GRADUATED'); + expect(dto.repeatedMeasure).toBe(REPEATED_MEASURE.mean); + }); +}); From 18d0dec2847ddb05b785e7be1d555fdbcb0ca998 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 5 Dec 2025 09:33:19 -0500 Subject: [PATCH 30/30] Add targeted TSDoc blocks in UpsertMetricModalComponent --- .../upsert-metric-modal.component.ts | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index d74ad2d30f..2f597a6779 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -85,6 +85,11 @@ interface RepeatableMetricFormValue extends MetricFormValueBase { type MetricFormValue = GlobalMetricFormValue | RepeatableMetricFormValue; +/** + * Presents the add/edit metric modal and normalizes form values for global and repeatable metrics. + * Centralizes the multi-step UX (autocomplete selections, validation, payload build) so callers only + * inject services and consume the resulting DTO sent back through the modal close. + */ @Component({ selector: 'upsert-metric-modal', imports: [ @@ -230,6 +235,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { }; } + /** + * Bootstraps the reactive form, wiring together data sources, listeners, and initial state so the + * UI reflects the selected experiment context and metric being edited (if any). + */ ngOnInit(): void { this.createMetricForm(); this.setupFormChangeListeners(); @@ -259,6 +268,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { return { mustSelectFromOptions: true }; } + /** + * Builds the reactive form with validators that reflect the current modal mode (add vs edit) and + * metric type (global vs repeatable). When editing, seeds controls with parsed source query values. + */ createMetricForm(): void { const { sourceQuery, action } = this.config.params; const initialValues = this.deriveInitialFormValues(sourceQuery, action); @@ -292,6 +305,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } + /** + * Translates the incoming query into form-ready values, handling both global and repeatable key formats. + */ deriveInitialFormValues(sourceQuery: ExperimentQueryDTO | null, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { const metricKey = sourceQuery.metric?.key || ''; @@ -352,6 +368,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { }; } + /** + * When editing, waits for available metrics so the form can rehydrate selections with real node objects + * instead of raw strings, restoring validator state and statistic dropdowns accurately. + */ populateFormForEditMode(initialValues: MetricFormData): void { // Wait for allMetrics to be loaded, then populate form with proper objects this.subscriptions.add( @@ -1034,6 +1054,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.metricForm.updateValueAndValidity({ emitEvent: true }); } + /** + * Emits whether the user has diverged from the original payload so the modal can toggle its action button. + */ listenForIsInitialFormValueChanged(): void { this.isInitialFormValueChanged$ = this.metricForm.valueChanges.pipe( startWith(this.getCurrentFormValue()), @@ -1043,6 +1066,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { ); } + /** + * Combines form validity, loading status, and selection completeness to drive the primary button state. + */ listenForPrimaryButtonDisabled(): void { this.isPrimaryButtonDisabled$ = this.isLoadingUpsertMetric$.pipe( combineLatestWith(this.isInitialFormValueChanged$, this.hasValidMetricIdSelection$), @@ -1064,6 +1090,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } + /** + * Resolves the target experiment and dispatches the normalized metric payload through the helper service. + */ sendUpsertMetricRequest(): void { const formValue = this.getCurrentFormValue(); const metricData = this.prepareMetricDataForBackend(formValue); @@ -1104,6 +1133,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.closeModal(); } + /** + * Normalizes the form value into the backend DTO, joining repeatable keys and attaching categorical filters + * only when required by the selected metric metadata. + */ private prepareMetricDataForBackend(formValue: MetricFormValue): ExperimentQueryDTO { const metricKey = this.isRepeatableFormValue(formValue) ? `${this.extractKey(formValue.metricClass)}${METRICS_JOIN_TEXT}${this.extractKey(