From 69e6cc0e8aeb0b770d43427a7fe17215acaec4fc Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 29 Aug 2025 03:58:56 +0900 Subject: [PATCH 01/15] Add upsert decision point modal component --- .../experiments/store/experiments.model.ts | 13 ++ ...upsert-decision-point-modal.component.html | 76 ++++++++ ...upsert-decision-point-modal.component.scss | 7 + .../upsert-decision-point-modal.component.ts | 183 ++++++++++++++++++ ...-decision-points-section-card.component.ts | 14 +- ...riment-details-page-content.component.html | 1 + .../shared/services/common-dialog.service.ts | 52 ++++- .../projects/upgrade/src/assets/i18n/en.json | 3 + 8 files changed, 343 insertions(+), 6 deletions(-) create mode 100644 frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.html create mode 100644 frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.scss create mode 100644 frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts 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 457c0f5d93..9c1595f268 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 @@ -339,6 +339,13 @@ export interface UpsertExperimentParams { action: UPSERT_EXPERIMENT_ACTION; } +export interface UpsertDecisionPointParams { + sourceDecisionPoint: ExperimentDecisionPoint; + context: string; + action: UPSERT_EXPERIMENT_ACTION; + experimentId: string; +} + export interface ExperimentFormData { name: string; description: string; @@ -353,6 +360,12 @@ export interface ExperimentFormData { tags: string[]; } +export interface DecisionPointFormData { + site: string; + target: string; + excludeIfReached: boolean; +} + // Base interfaces matching backend DTO structure export interface ExperimentConditionDTO { id: string; diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.html new file mode 100644 index 0000000000..7c8beb754d --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.html @@ -0,0 +1,76 @@ + +
+ + + Site + + + + {{ site }} + + + + {{ 'experiments.upsert-decision-point-modal.site-hint.text' | translate }} + + Learn more + + + + + + Target + + + + {{ target }} + + + + {{ 'experiments.upsert-decision-point-modal.target-hint.text' | translate }} + + Learn more + + + + +
+ + Exclude If Reached + + {{ 'experiments.upsert-decision-point-modal.exclude-if-reached-hint.text' | translate }} + + Learn more + + + +
+ +
+
diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.scss b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.scss new file mode 100644 index 0000000000..bee16bc5c1 --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.scss @@ -0,0 +1,7 @@ +.exclude-if-reached-container { + margin-bottom: 16px; + + .checkbox-label { + margin-left: 0; + } +} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts new file mode 100644 index 0000000000..c743b6137e --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts @@ -0,0 +1,183 @@ +import { ChangeDetectionStrategy, Component, Inject, OnDestroy, OnInit } from '@angular/core'; +import { MAT_DIALOG_DATA, MatDialogRef } from '@angular/material/dialog'; +import { FormBuilder, FormGroup, Validators, ReactiveFormsModule } from '@angular/forms'; +import { CommonModule } from '@angular/common'; +import { MatFormFieldModule } from '@angular/material/form-field'; +import { MatInputModule } from '@angular/material/input'; +import { MatCheckboxModule } from '@angular/material/checkbox'; +import { MatAutocompleteModule } from '@angular/material/autocomplete'; +import { TranslateModule } from '@ngx-translate/core'; +import { BehaviorSubject, Observable, Subscription, combineLatestWith, map, startWith } from 'rxjs'; +import isEqual from 'lodash.isequal'; + +import { CommonModalComponent } from '../../../../../shared-standalone-component-lib/components'; +import { ExperimentService } from '../../../../../core/experiments/experiments.service'; +import { CommonFormHelpersService } from '../../../../../shared/services/common-form-helpers.service'; +import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; +import { + UPSERT_EXPERIMENT_ACTION, + UpsertDecisionPointParams, + DecisionPointFormData, + IContextMetaData, + ExperimentDecisionPoint, +} from '../../../../../core/experiments/store/experiments.model'; + +@Component({ + selector: 'upsert-decision-point-modal', + imports: [ + CommonModalComponent, + MatFormFieldModule, + MatInputModule, + MatCheckboxModule, + MatAutocompleteModule, + CommonModule, + ReactiveFormsModule, + TranslateModule, + ], + templateUrl: './upsert-decision-point-modal.component.html', + styleUrl: './upsert-decision-point-modal.component.scss', + changeDetection: ChangeDetectionStrategy.OnPush, +}) +export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy { + isLoadingUpsertDecisionPoint$ = this.experimentService.isLoadingExperiment$; + contextMetaData$ = this.experimentService.contextMetaData$; + + subscriptions = new Subscription(); + isPrimaryButtonDisabled$: Observable; + isInitialFormValueChanged$: Observable; + + initialFormValues$ = new BehaviorSubject(null); + + // Filtered options for autocomplete + filteredSites$: Observable; + filteredTargets$: Observable; + + decisionPointForm: FormGroup; + currentContext: string; + + constructor( + @Inject(MAT_DIALOG_DATA) + public config: CommonModalConfig, + private formBuilder: FormBuilder, + private experimentService: ExperimentService, + public dialogRef: MatDialogRef + ) {} + + ngOnInit(): void { + this.experimentService.fetchContextMetaData(); + this.currentContext = this.config.params.context || ''; + this.createDecisionPointForm(); + this.setupAutocompleteFilters(); + + // Add listeners AFTER form is fully set up + this.listenForIsInitialFormValueChanged(); + this.listenForPrimaryButtonDisabled(); + } + + ngOnDestroy(): void { + this.subscriptions.unsubscribe(); + } + + createDecisionPointForm(): void { + const { sourceDecisionPoint, action } = this.config.params; + const initialValues = this.deriveInitialFormValues(sourceDecisionPoint, action); + + this.decisionPointForm = this.formBuilder.group({ + site: [initialValues.site, [Validators.required]], + target: [initialValues.target, [Validators.required]], + excludeIfReached: [initialValues.excludeIfReached], + }); + + this.initialFormValues$.next(this.decisionPointForm.value); + } + + deriveInitialFormValues( + sourceDecisionPoint: ExperimentDecisionPoint, + action: UPSERT_EXPERIMENT_ACTION + ): DecisionPointFormData { + const site = action === UPSERT_EXPERIMENT_ACTION.EDIT ? sourceDecisionPoint?.site || '' : ''; + const target = action === UPSERT_EXPERIMENT_ACTION.EDIT ? sourceDecisionPoint?.target || '' : ''; + const excludeIfReached = sourceDecisionPoint?.excludeIfReached || false; + + return { site, target, excludeIfReached }; + } + + setupAutocompleteFilters(): void { + this.filteredSites$ = this.contextMetaData$.pipe( + combineLatestWith(this.decisionPointForm.get('site').valueChanges.pipe(startWith(''))), + map(([metaData, value]) => this.filterSitesAndTargets(metaData, value || '', 'EXP_POINTS')) + ); + + this.filteredTargets$ = this.contextMetaData$.pipe( + combineLatestWith(this.decisionPointForm.get('target').valueChanges.pipe(startWith(''))), + map(([metaData, value]) => this.filterSitesAndTargets(metaData, value || '', 'EXP_IDS')) + ); + } + + private filterSitesAndTargets(metaData: IContextMetaData, value: string, field: 'EXP_POINTS' | 'EXP_IDS'): string[] { + const filterValue = value ? value.toLowerCase() : ''; + + if (!metaData || !this.currentContext || !metaData.contextMetadata) { + return []; + } + + const contextData = metaData.contextMetadata[this.currentContext]; + if (!contextData || !contextData[field]) { + return []; + } + + return contextData[field].filter((option) => option.toLowerCase().startsWith(filterValue)); + } + + listenForIsInitialFormValueChanged(): void { + this.isInitialFormValueChanged$ = this.decisionPointForm.valueChanges.pipe( + startWith(this.decisionPointForm.value), + map(() => !isEqual(this.decisionPointForm.value, this.initialFormValues$.value)) + ); + this.subscriptions.add(this.isInitialFormValueChanged$.subscribe()); + } + + listenForPrimaryButtonDisabled(): void { + this.isPrimaryButtonDisabled$ = this.isLoadingUpsertDecisionPoint$.pipe( + combineLatestWith(this.isInitialFormValueChanged$), + map(([isLoading, isInitialFormValueChanged]) => isLoading || !isInitialFormValueChanged) + ); + this.subscriptions.add(this.isPrimaryButtonDisabled$.subscribe()); + } + + onPrimaryActionBtnClicked(): void { + if (this.decisionPointForm.valid) { + this.sendUpsertDecisionPointRequest(); + } else { + CommonFormHelpersService.triggerTouchedToDisplayErrors(this.decisionPointForm); + } + } + + sendUpsertDecisionPointRequest(): void { + const formData = this.decisionPointForm.value; + const decisionPointRequest = { + site: formData.site?.trim() || '', + target: formData.target?.trim() || '', + excludeIfReached: formData.excludeIfReached, + experimentId: this.config.params.experimentId, + }; + + // Validate trimmed values are not empty + if (!decisionPointRequest.site || !decisionPointRequest.target) { + CommonFormHelpersService.triggerTouchedToDisplayErrors(this.decisionPointForm); + return; + } + + // TODO: Implement decision point creation/update in ExperimentService + // For now, close the modal as a placeholder + this.closeModal(); + } + + get UPSERT_EXPERIMENT_ACTION() { + return UPSERT_EXPERIMENT_ACTION; + } + + closeModal(): void { + this.dialogRef.close(); + } +} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts index 426aaf6193..757077ba22 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts @@ -19,6 +19,7 @@ import { } 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-decision-points-section-card', @@ -36,6 +37,7 @@ import { AuthService } from '../../../../../../../core/auth/auth.service'; }) export class ExperimentDecisionPointsSectionCardComponent implements OnInit { @Input() isSectionCardExpanded = true; + @Input() experimentContext = ''; permissions$: Observable; selectedExperiment$ = this.experimentService.selectedExperiment$; @@ -53,15 +55,18 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { }, ]; - constructor(public experimentService: ExperimentService, private authService: AuthService) {} + constructor( + public experimentService: ExperimentService, + private authService: AuthService, + private dialogService: DialogService + ) {} ngOnInit() { this.permissions$ = this.authService.userPermissions$; } onAddDecisionPointClick(appContext: string, experimentId: string): void { - // TODO: Implement add decision point functionality when dialog service is available - console.log('Add decision point'); + this.dialogService.openAddDecisionPointModal(experimentId, this.experimentContext); } onMenuButtonItemClick(event: string, experiment: Experiment): void { @@ -98,8 +103,7 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { } onEditDecisionPoint(decisionPoint: ExperimentDecisionPoint, experimentId: string): void { - // TODO: Implement edit functionality when dialog service is available - console.log('Edit decision point'); + this.dialogService.openEditDecisionPointModal(decisionPoint, experimentId, this.experimentContext); } onDeleteDecisionPoint(decisionPoint: ExperimentDecisionPoint): void { diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html index 0d809e03d4..32071e0dd3 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html @@ -12,6 +12,7 @@ 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 c1b6c330a1..e8f3d49c17 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 @@ -35,7 +35,8 @@ import { import { CommonImportModalComponent } from '../../shared-standalone-component-lib/components/common-import-modal/common-import-modal.component'; import { DeleteSegmentModalComponent } from '../../features/dashboard/segments/modals/delete-segment-modal/delete-segment-modal.component'; import { UpsertExperimentModalComponent } from '../../features/dashboard/experiments/modals/upsert-experiment-modal/upsert-experiment-modal.component'; -import { UPSERT_EXPERIMENT_ACTION } from '../../core/experiments/store/experiments.model'; +import { UpsertDecisionPointModalComponent } from '../../features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component'; +import { UPSERT_EXPERIMENT_ACTION, ExperimentDecisionPoint } from '../../core/experiments/store/experiments.model'; export interface ImportModalParams { importTypeAdapterToken: InjectionToken; @@ -47,6 +48,13 @@ export interface ImportModalParams { listType?: LIST_FILTER_MODE; // for feature flag list import } +export interface UpsertDecisionPointModalParams { + sourceDecisionPoint: ExperimentDecisionPoint | null; + action: UPSERT_EXPERIMENT_ACTION; + experimentId: string; + context: string; +} + @Injectable({ providedIn: 'root', }) @@ -88,6 +96,48 @@ export class DialogService { return this.dialog.open(UpsertExperimentModalComponent, config); } + openAddDecisionPointModal(experimentId: string, context: string) { + const commonModalConfig: CommonModalConfig = { + title: 'Add Decision Point', + primaryActionBtnLabel: 'Create', + primaryActionBtnColor: 'primary', + cancelBtnLabel: 'Cancel', + params: { + sourceDecisionPoint: null, + action: UPSERT_EXPERIMENT_ACTION.ADD, + experimentId, + context, + }, + }; + return this.openUpsertDecisionPointModal(commonModalConfig); + } + + openEditDecisionPointModal(sourceDecisionPoint: ExperimentDecisionPoint, experimentId: string, context: string) { + const commonModalConfig: CommonModalConfig = { + title: 'Edit Decision Point', + primaryActionBtnLabel: 'Save', + primaryActionBtnColor: 'primary', + cancelBtnLabel: 'Cancel', + params: { + sourceDecisionPoint: { ...sourceDecisionPoint }, + action: UPSERT_EXPERIMENT_ACTION.EDIT, + experimentId, + context, + }, + }; + return this.openUpsertDecisionPointModal(commonModalConfig); + } + + openUpsertDecisionPointModal(commonModalConfig: CommonModalConfig) { + const config: MatDialogConfig = { + data: commonModalConfig, + width: ModalSize.STANDARD, + autoFocus: false, + disableClose: true, + }; + return this.dialog.open(UpsertDecisionPointModalComponent, config); + } + // feature flag modal ---------------------------------------- // openAddFeatureFlagModal() { const commonModalConfig: CommonModalConfig = { diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index 70dae04cda..5fbc7f9677 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -401,6 +401,9 @@ "experiments.details.decision-points.exclude-if-reached.text": "Exclude If Reached", "experiments.details.decision-points.actions.text": "Actions", "experiments.details.decision-points.card.no-data-row.text": "No decision points defined. Experiments require at least one decision point.", + "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.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 2d3633d90ef2dcb987ddf01f45368dc9b407609e Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Wed, 3 Sep 2025 23:48:45 +0900 Subject: [PATCH 02/15] Implement sendUpsertDecisionPointRequest for Add/Edit Decision Point modal --- .../experiments/experiments.data.service.ts | 9 +++ .../core/experiments/experiments.service.ts | 7 ++ .../experiments/store/experiments.actions.ts | 15 ++++ .../experiments/store/experiments.effects.ts | 24 +++++++ .../experiments/store/experiments.model.ts | 5 ++ .../experiments/store/experiments.reducer.ts | 8 +++ .../upsert-decision-point-modal.component.ts | 68 +++++++++++++++++-- .../projects/upgrade/src/assets/i18n/en.json | 2 + 8 files changed, 133 insertions(+), 5 deletions(-) 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 a0abdff83e..e2c2b9cebf 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 @@ -4,6 +4,7 @@ import { ExperimentStateInfo, ExperimentPaginationParams, UpdateExperimentFilterModeRequest, + UpdateExperimentDecisionPointsRequest, } from './store/experiments.model'; import { HttpClient, HttpParams } from '@angular/common/http'; import { ENV, Environment } from '../../../environments/environment-types'; @@ -125,4 +126,12 @@ export class ExperimentDataService { }; return this.updateExperiment(updatedExperiment); } + + updateExperimentDecisionPoints(params: UpdateExperimentDecisionPointsRequest): Observable { + const updatedExperiment = { + ...params.experiment, + partitions: params.decisionPoints, + }; + 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 2e544b81c6..7b4bc5e891 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts @@ -13,6 +13,7 @@ import { EXPERIMENT_STATE, AddExperimentRequest, UpdateExperimentFilterModeRequest, + UpdateExperimentDecisionPointsRequest, } from './store/experiments.model'; import { Store, select } from '@ngrx/store'; import { @@ -190,6 +191,12 @@ export class ExperimentService { this.store$.dispatch(experimentAction.actionUpdateExperimentFilterMode({ updateExperimentFilterModeRequest })); } + updateExperimentDecisionPoints(updateExperimentDecisionPointsRequest: UpdateExperimentDecisionPointsRequest) { + this.store$.dispatch( + experimentAction.actionUpdateExperimentDecisionPoints({ updateExperimentDecisionPointsRequest }) + ); + } + fetchContextMetaData() { this.store$.dispatch(experimentAction.actionFetchContextMetaData({ isLoadingContextMetaData: true })); } 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 a98a8356b2..6d4b3bd2e5 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 @@ -12,6 +12,7 @@ import { IEnrollmentStatByDate, IContextMetaData, UpdateExperimentFilterModeRequest, + UpdateExperimentDecisionPointsRequest, } from './experiments.model'; export const actionGetExperiments = createAction('[Experiment] Get Experiments', props<{ fromStarting?: boolean }>()); @@ -99,6 +100,20 @@ export const actionUpdateExperimentFilterModeFailure = createAction( '[Experiment] Update Experiment Filter Mode Failure' ); +export const actionUpdateExperimentDecisionPoints = createAction( + '[Experiment] Update Experiment Decision Points', + props<{ updateExperimentDecisionPointsRequest: UpdateExperimentDecisionPointsRequest }>() +); + +export const actionUpdateExperimentDecisionPointsSuccess = createAction( + '[Experiment] Update Experiment Decision Points Success', + props<{ experiment: Experiment }>() +); + +export const actionUpdateExperimentDecisionPointsFailure = createAction( + '[Experiment] Update Experiment Decision Points Failure' +); + export const actionFetchAllDecisionPoints = createAction('[Experiment] Fetch All Decision Points'); export const actionFetchAllDecisionPointsSuccess = createAction( 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 89a4e2b90e..31c7611fc5 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 @@ -216,6 +216,30 @@ export class ExperimentEffects { ) ); + updateExperimentDecisionPoints$ = createEffect(() => + this.actions$.pipe( + ofType(experimentAction.actionUpdateExperimentDecisionPoints), + switchMap((action) => { + return this.experimentDataService + .updateExperimentDecisionPoints(action.updateExperimentDecisionPointsRequest) + .pipe( + map((experiment) => { + this.notificationService.showSuccess( + this.translate.instant('experiments.decision-points.update-success.text') + ); + return experimentAction.actionUpdateExperimentDecisionPointsSuccess({ experiment }); + }), + catchError(() => { + this.notificationService.showError( + this.translate.instant('experiments.decision-points.update-error.text') + ); + return [experimentAction.actionUpdateExperimentDecisionPointsFailure()]; + }) + ); + }) + ) + ); + 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 9c1595f268..ae1ec1cd1a 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 @@ -500,6 +500,11 @@ export interface UpdateExperimentFilterModeRequest { filterMode: FILTER_MODE; } +export interface UpdateExperimentDecisionPointsRequest { + experiment: Experiment; + decisionPoints: ExperimentDecisionPoint[]; +} + export const EXPERIMENT_ROOT_COLUMN_NAMES = { NAME: 'name', STATUS: 'state', 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 3e82fbc821..5845043d28 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 @@ -98,6 +98,14 @@ const reducer = createReducer( on(experimentsAction.actionUpdateExperimentFilterModeSuccess, (state, { experiment }) => adapter.upsertOne(experiment, { ...state, isLoadingExperiment: false }) ), + on(experimentsAction.actionUpdateExperimentDecisionPoints, (state) => ({ ...state, isLoadingExperiment: true })), + on(experimentsAction.actionUpdateExperimentDecisionPointsSuccess, (state, { experiment }) => + adapter.upsertOne(experiment, { ...state, isLoadingExperiment: false }) + ), + on(experimentsAction.actionUpdateExperimentDecisionPointsFailure, (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-decision-point-modal/upsert-decision-point-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts index c743b6137e..fe0192f876 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts @@ -7,8 +7,9 @@ import { MatInputModule } from '@angular/material/input'; import { MatCheckboxModule } from '@angular/material/checkbox'; import { MatAutocompleteModule } from '@angular/material/autocomplete'; import { TranslateModule } from '@ngx-translate/core'; -import { BehaviorSubject, Observable, Subscription, combineLatestWith, map, startWith } from 'rxjs'; +import { BehaviorSubject, Observable, Subscription, combineLatestWith, map, startWith, take } from 'rxjs'; import isEqual from 'lodash.isequal'; +import { v4 as uuidv4 } from 'uuid'; import { CommonModalComponent } from '../../../../../shared-standalone-component-lib/components'; import { ExperimentService } from '../../../../../core/experiments/experiments.service'; @@ -20,6 +21,8 @@ import { DecisionPointFormData, IContextMetaData, ExperimentDecisionPoint, + UpdateExperimentDecisionPointsRequest, + Experiment, } from '../../../../../core/experiments/store/experiments.model'; @Component({ @@ -140,7 +143,12 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy { listenForPrimaryButtonDisabled(): void { this.isPrimaryButtonDisabled$ = this.isLoadingUpsertDecisionPoint$.pipe( combineLatestWith(this.isInitialFormValueChanged$), - map(([isLoading, isInitialFormValueChanged]) => isLoading || !isInitialFormValueChanged) + map( + ([isLoading, isInitialFormValueChanged]) => + isLoading || + this.decisionPointForm.invalid || + (!isInitialFormValueChanged && this.config.params.action !== UPSERT_EXPERIMENT_ACTION.ADD) + ) ); this.subscriptions.add(this.isPrimaryButtonDisabled$.subscribe()); } @@ -168,9 +176,59 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy { return; } - // TODO: Implement decision point creation/update in ExperimentService - // For now, close the modal as a placeholder - this.closeModal(); + // Get the current experiment to update its decision points + this.experimentService.selectedExperiment$.pipe(take(1)).subscribe((experiment: Experiment) => { + if (!experiment) { + console.error('No experiment selected'); + return; + } + + const currentDecisionPoints = [...(experiment.partitions || [])]; + let updatedDecisionPoints: ExperimentDecisionPoint[]; + + if (this.config.params.action === UPSERT_EXPERIMENT_ACTION.ADD) { + // Add new decision point + const newDecisionPoint = { + id: uuidv4(), + site: decisionPointRequest.site, + target: decisionPointRequest.target, + description: '', + order: currentDecisionPoints.length + 1, + excludeIfReached: decisionPointRequest.excludeIfReached, + }; + updatedDecisionPoints = [...currentDecisionPoints, newDecisionPoint] as ExperimentDecisionPoint[]; + } else { + // Edit existing decision point + const sourceDecisionPoint = this.config.params.sourceDecisionPoint; + if (!sourceDecisionPoint) { + console.error('No source decision point for edit action'); + return; + } + + updatedDecisionPoints = currentDecisionPoints.map((dp) => + dp.id === sourceDecisionPoint.id + ? { + ...dp, + site: decisionPointRequest.site, + target: decisionPointRequest.target, + excludeIfReached: decisionPointRequest.excludeIfReached, + } + : dp + ); + } + + // Create the update request + const updateRequest: UpdateExperimentDecisionPointsRequest = { + experiment, + decisionPoints: updatedDecisionPoints, + }; + + // Dispatch the update action + this.experimentService.updateExperimentDecisionPoints(updateRequest); + + // Close the modal + this.closeModal(); + }); } get UPSERT_EXPERIMENT_ACTION() { diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index 5fbc7f9677..810bb7a3f8 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -404,6 +404,8 @@ "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 043dfb514d2e18c12631170cadb6ccd36f96b6f4 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 4 Sep 2025 00:59:51 +0900 Subject: [PATCH 03/15] Create decision-point-helper.service.ts and implement the Delete Decision Point modal --- .../decision-point-helper.service.ts | 90 +++++++++++++++++++ .../upsert-decision-point-modal.component.ts | 47 ++-------- ...-decision-points-section-card.component.ts | 26 ++++-- .../shared/services/common-dialog.service.ts | 14 +++ 4 files changed, 131 insertions(+), 46 deletions(-) create mode 100644 frontend/projects/upgrade/src/app/core/experiments/decision-point-helper.service.ts diff --git a/frontend/projects/upgrade/src/app/core/experiments/decision-point-helper.service.ts b/frontend/projects/upgrade/src/app/core/experiments/decision-point-helper.service.ts new file mode 100644 index 0000000000..e70b415516 --- /dev/null +++ b/frontend/projects/upgrade/src/app/core/experiments/decision-point-helper.service.ts @@ -0,0 +1,90 @@ +import { Injectable } from '@angular/core'; +import { v4 as uuidv4 } from 'uuid'; + +import { ExperimentService } from './experiments.service'; +import { + Experiment, + ExperimentDecisionPoint, + UpdateExperimentDecisionPointsRequest, + DecisionPointFormData, +} from './store/experiments.model'; + +@Injectable({ + providedIn: 'root', +}) +export class DecisionPointHelperService { + constructor(private experimentService: ExperimentService) {} + + /** + * Add a new decision point to an experiment + */ + addDecisionPoint(experiment: Experiment, decisionPointData: DecisionPointFormData): void { + const currentDecisionPoints = [...(experiment.partitions || [])]; + const newDecisionPoint = { + id: uuidv4(), + site: decisionPointData.site, + target: decisionPointData.target, + description: '', + order: currentDecisionPoints.length + 1, + excludeIfReached: decisionPointData.excludeIfReached, + }; + + const updatedDecisionPoints = [...currentDecisionPoints, newDecisionPoint] as ExperimentDecisionPoint[]; + + this.updateExperimentDecisionPoints(experiment, updatedDecisionPoints); + } + + /** + * Update an existing decision point in an experiment + */ + updateDecisionPoint( + experiment: Experiment, + sourceDecisionPoint: ExperimentDecisionPoint, + decisionPointData: DecisionPointFormData + ): void { + const currentDecisionPoints = [...(experiment.partitions || [])]; + const updatedDecisionPoints = currentDecisionPoints.map((dp) => + dp.id === sourceDecisionPoint.id + ? { + ...dp, + site: decisionPointData.site, + target: decisionPointData.target, + excludeIfReached: decisionPointData.excludeIfReached, + } + : dp + ); + + this.updateExperimentDecisionPoints(experiment, updatedDecisionPoints); + } + + /** + * Delete a decision point from an experiment + */ + deleteDecisionPoint(experiment: Experiment, decisionPointToDelete: ExperimentDecisionPoint): void { + const currentDecisionPoints = [...(experiment.partitions || [])]; + const updatedDecisionPoints = currentDecisionPoints.filter((dp) => dp.id !== decisionPointToDelete.id); + + // Reorder the remaining decision points + const reorderedDecisionPoints = updatedDecisionPoints.map((dp, index) => ({ + ...dp, + order: index + 1, + })); + + this.updateExperimentDecisionPoints(experiment, reorderedDecisionPoints); + } + + /** + * Common method to update experiment decision points + */ + private updateExperimentDecisionPoints( + experiment: Experiment, + updatedDecisionPoints: ExperimentDecisionPoint[] + ): void { + const updateRequest: UpdateExperimentDecisionPointsRequest = { + experiment, + decisionPoints: updatedDecisionPoints, + }; + + this.experimentService.updateExperimentDecisionPoints(updateRequest); + } +} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts index fe0192f876..3283af5ab9 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts @@ -9,10 +9,10 @@ import { MatAutocompleteModule } from '@angular/material/autocomplete'; import { TranslateModule } from '@ngx-translate/core'; import { BehaviorSubject, Observable, Subscription, combineLatestWith, map, startWith, take } from 'rxjs'; import isEqual from 'lodash.isequal'; -import { v4 as uuidv4 } from 'uuid'; import { CommonModalComponent } from '../../../../../shared-standalone-component-lib/components'; import { ExperimentService } from '../../../../../core/experiments/experiments.service'; +import { DecisionPointHelperService } from '../../../../../core/experiments/decision-point-helper.service'; import { CommonFormHelpersService } from '../../../../../shared/services/common-form-helpers.service'; import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; import { @@ -21,7 +21,6 @@ import { DecisionPointFormData, IContextMetaData, ExperimentDecisionPoint, - UpdateExperimentDecisionPointsRequest, Experiment, } from '../../../../../core/experiments/store/experiments.model'; @@ -63,6 +62,7 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy { public config: CommonModalConfig, private formBuilder: FormBuilder, private experimentService: ExperimentService, + private decisionPointHelperService: DecisionPointHelperService, public dialogRef: MatDialogRef ) {} @@ -163,70 +163,37 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy { sendUpsertDecisionPointRequest(): void { const formData = this.decisionPointForm.value; - const decisionPointRequest = { + const decisionPointData: DecisionPointFormData = { site: formData.site?.trim() || '', target: formData.target?.trim() || '', excludeIfReached: formData.excludeIfReached, - experimentId: this.config.params.experimentId, }; // Validate trimmed values are not empty - if (!decisionPointRequest.site || !decisionPointRequest.target) { + if (!decisionPointData.site || !decisionPointData.target) { CommonFormHelpersService.triggerTouchedToDisplayErrors(this.decisionPointForm); return; } - // Get the current experiment to update its decision points + // 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 currentDecisionPoints = [...(experiment.partitions || [])]; - let updatedDecisionPoints: ExperimentDecisionPoint[]; - if (this.config.params.action === UPSERT_EXPERIMENT_ACTION.ADD) { - // Add new decision point - const newDecisionPoint = { - id: uuidv4(), - site: decisionPointRequest.site, - target: decisionPointRequest.target, - description: '', - order: currentDecisionPoints.length + 1, - excludeIfReached: decisionPointRequest.excludeIfReached, - }; - updatedDecisionPoints = [...currentDecisionPoints, newDecisionPoint] as ExperimentDecisionPoint[]; + this.decisionPointHelperService.addDecisionPoint(experiment, decisionPointData); } else { - // Edit existing decision point const sourceDecisionPoint = this.config.params.sourceDecisionPoint; if (!sourceDecisionPoint) { console.error('No source decision point for edit action'); return; } - updatedDecisionPoints = currentDecisionPoints.map((dp) => - dp.id === sourceDecisionPoint.id - ? { - ...dp, - site: decisionPointRequest.site, - target: decisionPointRequest.target, - excludeIfReached: decisionPointRequest.excludeIfReached, - } - : dp - ); + this.decisionPointHelperService.updateDecisionPoint(experiment, sourceDecisionPoint, decisionPointData); } - // Create the update request - const updateRequest: UpdateExperimentDecisionPointsRequest = { - experiment, - decisionPoints: updatedDecisionPoints, - }; - - // Dispatch the update action - this.experimentService.updateExperimentDecisionPoints(updateRequest); - - // Close the modal this.closeModal(); }); } diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts index 757077ba22..b9d7e63b33 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts @@ -9,13 +9,14 @@ import { TranslateModule } from '@ngx-translate/core'; import { IMenuButtonItem } from 'upgrade_types'; import { ExperimentDecisionPointsTableComponent } from './experiment-decision-points-table/experiment-decision-points-table.component'; import { ExperimentService } from '../../../../../../../core/experiments/experiments.service'; -import { Observable } from 'rxjs'; +import { DecisionPointHelperService } from '../../../../../../../core/experiments/decision-point-helper.service'; +import { Observable, take } from 'rxjs'; import { - Experiment, EXPERIMENT_BUTTON_ACTION, EXPERIMENT_ROW_ACTION, ExperimentDecisionPoint, ExperimentDecisionPointRowActionEvent, + Experiment, } from '../../../../../../../core/experiments/store/experiments.model'; import { UserPermission } from '../../../../../../../core/auth/store/auth.models'; import { AuthService } from '../../../../../../../core/auth/auth.service'; @@ -58,7 +59,8 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { constructor( public experimentService: ExperimentService, private authService: AuthService, - private dialogService: DialogService + private dialogService: DialogService, + private decisionPointHelperService: DecisionPointHelperService ) {} ngOnInit() { @@ -69,7 +71,7 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { this.dialogService.openAddDecisionPointModal(experimentId, this.experimentContext); } - onMenuButtonItemClick(event: string, experiment: Experiment): void { + onMenuButtonItemClick(event: string): void { switch (event) { case EXPERIMENT_BUTTON_ACTION.IMPORT_DECISION_POINT: // TODO: Implement import functionality when dialog service is available @@ -107,7 +109,19 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { } onDeleteDecisionPoint(decisionPoint: ExperimentDecisionPoint): void { - // TODO: Implement delete functionality when dialog service is available - console.log('Delete decision point'); + const decisionPointDisplayName = `${decisionPoint.site}; ${decisionPoint.target}`; + + this.dialogService + .openDeleteDecisionPointModal(decisionPointDisplayName) + .afterClosed() + .subscribe((confirmClicked) => { + if (confirmClicked) { + this.selectedExperiment$.pipe(take(1)).subscribe((experiment: Experiment) => { + if (experiment) { + this.decisionPointHelperService.deleteDecisionPoint(experiment, decisionPoint); + } + }); + } + }); } } 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 e8f3d49c17..da6bfd1180 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 @@ -477,6 +477,20 @@ export class DialogService { return this.dialog.open(DeleteFeatureFlagModalComponent, config); } + openDeleteDecisionPointModal(decisionPointName: string) { + const deleteDecisionPointModalConfig: CommonModalConfig = { + title: 'Delete Decision Point', + primaryActionBtnLabel: 'Delete', + primaryActionBtnColor: 'warn', + cancelBtnLabel: 'Cancel', + params: { + message: `Are you sure you want to delete decision point "${decisionPointName}"?`, + }, + }; + + return this.openSimpleCommonConfirmationModal(deleteDecisionPointModalConfig, ModalSize.SMALL); + } + openDeleteSegmentModal() { const commonModalConfig: CommonModalConfig = { title: 'Delete Segment', From 7824c2e2cb0025ad44c442bb7635ebbb77fe688a Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 4 Sep 2025 01:17:56 +0900 Subject: [PATCH 04/15] Remove unneeded experimentContext input --- ...ment-decision-points-section-card.component.html | 4 ++-- ...riment-decision-points-section-card.component.ts | 13 ++++++------- .../experiment-details-page-content.component.html | 1 - 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html index 638937acbb..1e346befc4 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html @@ -15,7 +15,7 @@ [showMenuButton]="(permissions$ | async)?.experiments.update" [menuButtonItems]="menuButtonItems" [isSectionCardExpanded]="isSectionCardExpanded" - (primaryButtonClick)="onAddDecisionPointClick(experiment.context[0], experiment.id)" + (primaryButtonClick)="onAddDecisionPointClick(experiment.id, experiment.context[0])" (menuButtonItemClick)="onMenuButtonItemClick($event, experiment)" (sectionCardExpandChange)="onSectionCardExpandChange($event)" > @@ -28,6 +28,6 @@ [decisionPoints]="experiment.partitions" [isLoading$]="experimentService.isLoadingSelectedExperiment$" [actionsDisabled]="!(permissions$ | async)?.experiments.update" - (rowAction)="onRowAction($event, experiment.id)" + (rowAction)="onRowAction($event, experiment.id, experiment.context[0] || '')" > diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts index b9d7e63b33..8419806cbb 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.ts @@ -38,7 +38,6 @@ import { DialogService } from '../../../../../../../shared/services/common-dialo }) export class ExperimentDecisionPointsSectionCardComponent implements OnInit { @Input() isSectionCardExpanded = true; - @Input() experimentContext = ''; permissions$: Observable; selectedExperiment$ = this.experimentService.selectedExperiment$; @@ -67,8 +66,8 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { this.permissions$ = this.authService.userPermissions$; } - onAddDecisionPointClick(appContext: string, experimentId: string): void { - this.dialogService.openAddDecisionPointModal(experimentId, this.experimentContext); + onAddDecisionPointClick(experimentId: string, appContext: string): void { + this.dialogService.openAddDecisionPointModal(experimentId, appContext); } onMenuButtonItemClick(event: string): void { @@ -91,10 +90,10 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { } // Decision point row action events - onRowAction(event: ExperimentDecisionPointRowActionEvent, experimentId: string): void { + onRowAction(event: ExperimentDecisionPointRowActionEvent, experimentId: string, context: string): void { switch (event.action) { case EXPERIMENT_ROW_ACTION.EDIT: - this.onEditDecisionPoint(event.decisionPoint, experimentId); + this.onEditDecisionPoint(event.decisionPoint, experimentId, context); break; case EXPERIMENT_ROW_ACTION.DELETE: this.onDeleteDecisionPoint(event.decisionPoint); @@ -104,8 +103,8 @@ export class ExperimentDecisionPointsSectionCardComponent implements OnInit { } } - onEditDecisionPoint(decisionPoint: ExperimentDecisionPoint, experimentId: string): void { - this.dialogService.openEditDecisionPointModal(decisionPoint, experimentId, this.experimentContext); + onEditDecisionPoint(decisionPoint: ExperimentDecisionPoint, experimentId: string, context: string): void { + this.dialogService.openEditDecisionPointModal(decisionPoint, experimentId, context); } onDeleteDecisionPoint(decisionPoint: ExperimentDecisionPoint): void { diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html index 32071e0dd3..0d809e03d4 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-details-page-content.component.html @@ -12,7 +12,6 @@ From 2908884b40334170a857595d5362f9039a6a7c26 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 4 Sep 2025 02:48:38 +0900 Subject: [PATCH 05/15] Remove redundant fallback --- .../experiment-decision-points-section-card.component.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html index 1e346befc4..2c0d3f4f45 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-details-page/experiment-details-page-content/experiment-decision-points-section-card/experiment-decision-points-section-card.component.html @@ -28,6 +28,6 @@ [decisionPoints]="experiment.partitions" [isLoading$]="experimentService.isLoadingSelectedExperiment$" [actionsDisabled]="!(permissions$ | async)?.experiments.update" - (rowAction)="onRowAction($event, experiment.id, experiment.context[0] || '')" + (rowAction)="onRowAction($event, experiment.id, experiment.context[0])" > From d0258556afe7227b5b7ae049e466c0aa9165bd5e Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 12 Sep 2025 20:17:06 +0900 Subject: [PATCH 06/15] Add upsert metric modal component --- .../experiments/store/experiments.model.ts | 27 + .../upsert-metric-modal.component.html | 214 +++++++ .../upsert-metric-modal.component.scss | 40 ++ .../upsert-metric-modal.component.ts | 561 ++++++++++++++++++ ...periment-metrics-section-card.component.ts | 45 +- .../shared/services/common-dialog.service.ts | 53 +- 6 files changed, 930 insertions(+), 10 deletions(-) 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/store/experiments.model.ts b/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index bc1e767f6b..2d77a7013c 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 @@ -366,6 +366,20 @@ export interface DecisionPointFormData { excludeIfReached: boolean; } +export interface MetricFormData { + metricType: 'global' | 'repeatable'; + 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; @@ -607,6 +621,11 @@ export interface ExperimentConditionRowActionEvent { condition: ExperimentCondition; } +export interface ExperimentQueryRowActionEvent { + action: EXPERIMENT_ROW_ACTION; + query: ExperimentQueryDTO; +} + export enum EXPERIMENT_PAYLOAD_DISPLAY_TYPE { UNIVERSAL = 'universal', SPECIFIC = 'specific', @@ -628,3 +647,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/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..736d2fd3d6 --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.html @@ -0,0 +1,214 @@ + +
+ + +
+ + + + + Global Metric + Used for globally accumulated measures (e.g., total time spent using the app). + + + + + Repeatable Metric + Used for repeatable measures (e.g., score on a quiz that can be taken multiple times). + + + +
+ + + + + Metric Class + + + + {{ metricClass.key }} + + + + The metric class categorizes what type of app component is being measured (e.g., workspace). + + + + + + + Metric Key + + + + {{ metricKey.key }} + + + + The metric key specifies the specific instance of the metric class being measured (e.g., problem ID). + + + + + + + Metric ID + + + + {{ metric.key }} + + + + The Metric ID specifies the data type (continuous or categorical) and what data to measure. + + + + + + + Individual Statistic + + + {{ option.label }} + + + + The Individual Statistic determines which value to use for each student. + + Learn more + + + + + + + + + Aggregate Statistic + + + {{ option.label }} + + + + The Aggregate Statistic determines how to combine values across all students. + + Learn more + + + + + + + +
+ + + + {{ option.label }} + + +
+
+ + + + + Value + + + {{ value }} + + + + The categorical metric data type requires you to specify which allowed value to measure. + + + + + + + Display Name + + + The display name is used to refer to this metric in the experiment UI. + + + + + + Description (optional) + + + Optional description for this metric. + + + +
+
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..9223b0b1f6 --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.scss @@ -0,0 +1,40 @@ +.comparison-container { + .section-label { + display: block; + margin-bottom: 8px; + } + + .radio-group-horizontal { + display: flex; + gap: 16px; + + mat-radio-button { + margin-right: 0; + } + } +} + +.metric-type-container { + .radio-group-vertical { + display: flex; + flex-direction: column; + gap: 12px; + + mat-radio-button { + margin-right: 0; + + .radio-label { + display: flex; + flex-direction: column; + gap: 4px; + + .radio-description { + font-weight: 400; + color: rgba(0, 0, 0, 0.6); + font-size: 12px; + line-height: 1.4; + } + } + } + } +} 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..2ae559cc65 --- /dev/null +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts @@ -0,0 +1,561 @@ +import { ChangeDetectionStrategy, 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 { map, startWith } from 'rxjs/operators'; +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 { + MetricFormData, + UPSERT_EXPERIMENT_ACTION, + UpsertMetricParams, +} from '../../../../../core/experiments/store/experiments.model'; +import { ExperimentService } from '../../../../../core/experiments/experiments.service'; +import { AnalysisService } from '../../../../../core/analysis/analysis.service'; +import { IMetricMetaData, 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; + + // Dropdown options + aggregateStatisticOptions: StatisticOption[] = []; + individualStatisticOptions: StatisticOption[] = []; + + // Autocomplete - Professional solution with BehaviorSubjects for source data + allMetrics$ = this.analysisService.allMetrics$; + allMetrics: any[] = []; + + // BehaviorSubjects for source data - this is the professional way + private metricClassOptions$ = new BehaviorSubject([]); + private metricKeyOptions$ = new BehaviorSubject([]); + private metricIdOptions$ = new BehaviorSubject([]); + + // Filtered observables that combine user input with reactive source data + filteredMetricClasses$: Observable; + filteredMetricKeys$: Observable; + filteredMetricIds$: Observable; + + // Current selections + currentSelectedClass: any = null; + currentSelectedKey: any = null; + + // For categorical metrics comparison + allowableDataKeys: string[] = []; + comparisonOptions = [ + { value: '=', label: 'Equal' }, + { value: '<>', label: 'Not equal' }, + ]; + + // Continuous statistic options + 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: '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' }, + ]; + + // Categorical statistic options + 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 analysisService: AnalysisService, + public dialogRef: MatDialogRef + ) {} + + ngOnInit(): void { + this.createMetricForm(); + this.setupFormChangeListeners(); + this.setupAutocomplete(); + + // 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 + this.updateFormVisibility(); + this.detectMetricDataType(initialValues.metricId); + } + + deriveInitialFormValues(sourceQuery: any, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { + if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { + // Extract values from existing query for edit mode + return { + metricType: sourceQuery.repeatedMeasure ? 'repeatable' : 'global', + metricId: sourceQuery.metric?.key || '', + displayName: sourceQuery.name || '', + description: '', // Not available in current structure + metricClass: '', // Not available in current structure + metricKey: '', // Not available in current structure + aggregateStatistic: sourceQuery.query?.operationType || '', + individualStatistic: sourceQuery.repeatedMeasure || '', + comparison: sourceQuery.query?.compareFn || '=', + compareValue: sourceQuery.query?.compareValue || '', + allowableDataKeys: [], + }; + } + + // Default values for add mode + return { + metricType: 'global', + metricId: '', + displayName: '', + description: '', + metricClass: '', + metricKey: '', + aggregateStatistic: '', + individualStatistic: '', + comparison: '=', + compareValue: '', + allowableDataKeys: [], + }; + } + + setupFormChangeListeners(): void { + // Listen for metric type changes + this.subscriptions.add( + this.metricForm.get('metricType')?.valueChanges.subscribe(() => { + this.onMetricTypeChange(); + }) + ); + + // Listen for metric class changes + this.subscriptions.add( + this.metricForm.get('metricClass')?.valueChanges.subscribe((selectedClass) => { + this.onMetricClassChange(selectedClass); + }) + ); + + // Listen for metric key changes + this.subscriptions.add( + this.metricForm.get('metricKey')?.valueChanges.subscribe((selectedKey) => { + this.onMetricKeyChange(selectedKey); + }) + ); + + // Listen for metric ID changes + this.subscriptions.add( + this.metricForm.get('metricId')?.valueChanges.subscribe((metricId) => { + this.onMetricIdChange(metricId); + }) + ); + } + + setupAutocomplete(): void { + // Load all metrics data - EXACTLY like legacy component + this.subscriptions.add( + this.allMetrics$.subscribe((metrics) => { + this.allMetrics = metrics || []; + this.populateOptions(); + this.createFilteredObservables(); + }) + ); + } + + populateOptions(): void { + const metricType = this.metricForm.get('metricType')?.value; + + if (metricType === 'global') { + // Global metrics: only show metrics without children + this.metricClassOptions$.next([]); + this.metricKeyOptions$.next([]); + this.metricIdOptions$.next( + this.allMetrics.filter((metric) => !metric.children || metric.children.length === 0) + ); + } else { + // Repeatable metrics: show hierarchical structure + this.metricClassOptions$.next( + this.allMetrics.filter((metric) => metric.children && metric.children.length > 0) + ); + this.metricKeyOptions$.next([]); + this.metricIdOptions$.next([]); + } + } + + createFilteredObservables(): void { + // Professional solution: combine user input with reactive source data + 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 | undefined { + if (option && option.key) { + return option.key; + } else if (typeof option === 'string') { + return option; + } + return option ? option : undefined; + } + + private _filter(value: any, options: any[]): any[] { + let filterValue: string; + if (typeof value === 'string') { + filterValue = value.toLowerCase(); + } else if (value && value.key) { + filterValue = value.key.toLowerCase(); + } else { + filterValue = ''; + } + return options.filter((option) => option.key?.toLowerCase().includes(filterValue)); + } + + onMetricTypeChange(): void { + // Clear everything and repopulate + this.currentSelectedClass = null; + this.currentSelectedKey = null; + + // Clear form fields + this.metricForm.get('metricClass')?.setValue(''); + this.metricForm.get('metricKey')?.setValue(''); + this.metricForm.get('metricId')?.setValue(''); + + // Repopulate options based on new metric type + // BehaviorSubjects will automatically trigger observable re-emission + this.populateOptions(); + + // Update form state + this.updateFormVisibility(); + this.resetStatisticFields(); + this.updateFormValidators(); + } + + onMetricIdChange(metricId: any): void { + if (metricId) { + this.detectMetricDataType(metricId); + this.updateStatisticOptions(); + this.updateFormVisibility(); + this.updateFormValidators(); + } + } + + detectMetricDataType(metricId: any): void { + let selectedMetric: any = null; + + // Find the selected metric in current metricIdOptions + if (typeof metricId === 'object' && metricId.metadata) { + selectedMetric = metricId; + } else if (typeof metricId === 'string') { + // Find by string key - get current value from BehaviorSubject + const currentOptions = this.metricIdOptions$.getValue(); + selectedMetric = currentOptions.find((metric) => metric.key === metricId); + } + + if (selectedMetric && selectedMetric.metadata && selectedMetric.metadata.type) { + this.metricDataType = selectedMetric.metadata.type; + + // For categorical metrics, populate allowable data from the metric and set default comparison + if (this.metricDataType === IMetricMetaData.CATEGORICAL) { + if (selectedMetric.allowedData) { + this.allowableDataKeys = [...selectedMetric.allowedData]; + } + // Set default comparison to "=" if not already set + if (!this.metricForm.get('comparison')?.value) { + this.metricForm.get('comparison')?.setValue('='); + } + } else { + this.allowableDataKeys = []; + } + } else { + // Fallback to heuristic if metadata is not available + const metricKey = typeof metricId === 'string' ? metricId : metricId?.key || ''; + const continuousKeywords = ['time', 'count', 'score', 'number', 'seconds', 'minutes', 'duration']; + const categoricalKeywords = ['status', 'type', 'category', 'level', 'completion']; + + const lowerMetricKey = metricKey.toLowerCase(); + + if (continuousKeywords.some((keyword) => lowerMetricKey.includes(keyword))) { + this.metricDataType = IMetricMetaData.CONTINUOUS; + } else if (categoricalKeywords.some((keyword) => lowerMetricKey.includes(keyword))) { + this.metricDataType = IMetricMetaData.CATEGORICAL; + } else { + // Default to continuous for unknown types + this.metricDataType = IMetricMetaData.CONTINUOUS; + } + } + } + + updateFormVisibility(): void { + const metricType = this.metricForm.get('metricType')?.value; + this.showMetricClass = metricType === 'repeatable'; + this.showMetricKey = metricType === 'repeatable'; + this.showIndividualStatistic = metricType === 'repeatable'; + } + + updateStatisticOptions(): void { + if (this.metricDataType === IMetricMetaData.CONTINUOUS) { + this.aggregateStatisticOptions = this.continuousAggregateOptions; + this.individualStatisticOptions = this.continuousIndividualOptions; + this.showComparison = false; + } else { + this.aggregateStatisticOptions = this.categoricalAggregateOptions; + this.individualStatisticOptions = this.categoricalIndividualOptions; + this.showComparison = true; + } + } + + showStatisticDropdowns(): void { + this.showAggregateStatistic = true; + } + + hideStatisticDropdowns(): void { + this.showAggregateStatistic = false; + this.showIndividualStatistic = false; + this.showComparison = false; + } + + resetStatisticFields(): void { + this.metricForm.patchValue({ + aggregateStatistic: '', + individualStatistic: '', + }); + this.allowableDataKeys = []; + } + + updateFormValidators(): void { + const metricType = this.metricForm.get('metricType')?.value; + + // Update validators based on metric type + if (metricType === '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$ = combineLatest([ + this.metricForm.valueChanges.pipe(startWith(this.metricForm.value)), + this.initialFormValues$, + ]).pipe( + map(([currentFormValue, initialFormValue]) => { + if (!initialFormValue) return false; + + const currentWithKeys = { + ...currentFormValue, + allowableDataKeys: this.allowableDataKeys, + }; + + return JSON.stringify(currentWithKeys) !== JSON.stringify(initialFormValue); + }) + ); + } + + listenForPrimaryButtonDisabled(): void { + this.isPrimaryButtonDisabled$ = combineLatest([ + this.metricForm.statusChanges.pipe(startWith(this.metricForm.status)), + this.isLoadingUpsertMetric$, + this.isInitialFormValueChanged$, + ]).pipe( + map(([formStatus, isLoading, isFormChanged]) => { + const isFormInvalid = formStatus !== 'VALID'; + const isEditModeWithoutChanges = this.config.params.action === UPSERT_EXPERIMENT_ACTION.EDIT && !isFormChanged; + + return isFormInvalid || isLoading || isEditModeWithoutChanges; + }) + ); + } + + onPrimaryActionBtnClicked(): void { + if (this.metricForm.valid) { + this.sendUpsertMetricRequest(); + } + } + + sendUpsertMetricRequest(): void { + const formValue = this.metricForm.value; + const metricData: MetricFormData = { + ...formValue, + allowableDataKeys: this.allowableDataKeys, + }; + + // TODO: Implement the actual metric creation/update logic + // This would typically call a service method to create or update the metric + console.log('Metric data to be processed:', metricData); + console.log('Experiment ID:', this.config.params.experimentId); + console.log('Action:', this.config.params.action); + + // For now, just close the modal + this.closeModal(); + } + + get UPSERT_EXPERIMENT_ACTION() { + return UPSERT_EXPERIMENT_ACTION; + } + + closeModal(): void { + this.dialogRef.close(); + } +} 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..020d2c9bda 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 @@ -13,13 +13,17 @@ import { } from './experiment-metrics-table/experiment-metrics-table.component'; import { ExperimentService } from '../../../../../../../core/experiments/experiments.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'; +import { ModalSize } from '../../../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; @Component({ selector: 'app-experiment-metrics-section-card', @@ -63,15 +67,23 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { }, ]; - constructor(private experimentService: ExperimentService, private authService: AuthService) {} + constructor( + private experimentService: ExperimentService, + 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 { @@ -106,13 +118,28 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { } } - 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, experimentId: string): void { + const metricDisplayName = query.name || `${query.metric?.key}`; + + const confirmationParams = { + title: 'Delete Metric', + description: `Are you sure you want to delete the metric "${metricDisplayName}"?`, + primaryActionBtnText: 'Delete', + cancelBtnText: 'Cancel', + }; + + this.dialogService + .openSimpleCommonConfirmationModal(confirmationParams, ModalSize.MEDIUM) + .afterClosed() + .subscribe((confirmClicked) => { + if (confirmClicked) { + // TODO: Implement actual metric deletion logic + console.log('Delete metric confirmed for query:', query.id, 'in experiment:', experimentId); + } + }); } } 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 361b7d89a4..af89e067c7 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 @@ -36,7 +36,12 @@ import { CommonImportModalComponent } from '../../shared-standalone-component-li import { DeleteSegmentModalComponent } from '../../features/dashboard/segments/modals/delete-segment-modal/delete-segment-modal.component'; 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 { UPSERT_EXPERIMENT_ACTION, ExperimentDecisionPoint } from '../../core/experiments/store/experiments.model'; +import { UpsertMetricModalComponent } from '../../features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component'; +import { + UPSERT_EXPERIMENT_ACTION, + ExperimentDecisionPoint, + ExperimentQueryDTO, +} from '../../core/experiments/store/experiments.model'; export interface ImportModalParams { importTypeAdapterToken: InjectionToken; @@ -55,6 +60,12 @@ export interface UpsertDecisionPointModalParams { context: string; } +export interface UpsertMetricModalParams { + sourceQuery: ExperimentQueryDTO | null; + action: UPSERT_EXPERIMENT_ACTION; + experimentId: string; +} + @Injectable({ providedIn: 'root', }) @@ -138,6 +149,46 @@ export class DialogService { return this.dialog.open(UpsertDecisionPointModalComponent, 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.LARGE, + autoFocus: false, + disableClose: true, + }; + return this.dialog.open(UpsertMetricModalComponent, config); + } + // feature flag modal ---------------------------------------- // openAddFeatureFlagModal() { const commonModalConfig: CommonModalConfig = { From 2e86e845cde3bd2a223c3b75c7e723c116bcf1b2 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 15 Sep 2025 18:16:10 +0900 Subject: [PATCH 07/15] Fix linting errors --- .../upsert-metric-modal/upsert-metric-modal.component.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 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 2ae559cc65..9efdcc943c 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 @@ -253,14 +253,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Global metrics: only show metrics without children this.metricClassOptions$.next([]); this.metricKeyOptions$.next([]); - this.metricIdOptions$.next( - this.allMetrics.filter((metric) => !metric.children || metric.children.length === 0) - ); + this.metricIdOptions$.next(this.allMetrics.filter((metric) => !metric.children || metric.children.length === 0)); } else { // Repeatable metrics: show hierarchical structure - this.metricClassOptions$.next( - this.allMetrics.filter((metric) => metric.children && metric.children.length > 0) - ); + this.metricClassOptions$.next(this.allMetrics.filter((metric) => metric.children && metric.children.length > 0)); this.metricKeyOptions$.next([]); this.metricIdOptions$.next([]); } From 42d54349d91971c9c3a6a1bdff524330335fd66a Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 19 Sep 2025 21:46:00 +0900 Subject: [PATCH 08/15] refactor: add context validation and improve ternary readability --- .../upsert-decision-point-modal.component.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts index 3283af5ab9..af414b8aec 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts @@ -68,7 +68,12 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy { ngOnInit(): void { this.experimentService.fetchContextMetaData(); - this.currentContext = this.config.params.context || ''; + + if (!this.config.params.context) { + throw new Error('Context parameter is required for decision point modal'); + } + + this.currentContext = this.config.params.context; this.createDecisionPointForm(); this.setupAutocompleteFilters(); @@ -98,8 +103,9 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy { sourceDecisionPoint: ExperimentDecisionPoint, action: UPSERT_EXPERIMENT_ACTION ): DecisionPointFormData { - const site = action === UPSERT_EXPERIMENT_ACTION.EDIT ? sourceDecisionPoint?.site || '' : ''; - const target = action === UPSERT_EXPERIMENT_ACTION.EDIT ? sourceDecisionPoint?.target || '' : ''; + const site = action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceDecisionPoint?.site ? sourceDecisionPoint.site : ''; + const target = + action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceDecisionPoint?.target ? sourceDecisionPoint.target : ''; const excludeIfReached = sourceDecisionPoint?.excludeIfReached || false; return { site, target, excludeIfReached }; From 29b38db50b84b7195c1175a9d74119a87a4a84ca Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Mon, 22 Sep 2025 21:16:18 +0900 Subject: [PATCH 09/15] Make statistics dropdown appear on Metric ID selection --- .../upsert-metric-modal.component.html | 2 +- .../upsert-metric-modal.component.ts | 65 ++++++++++++------- 2 files changed, 44 insertions(+), 23 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 736d2fd3d6..3b09925276 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 @@ -103,7 +103,7 @@ - + Individual Statistic 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 9efdcc943c..b0e4a35e56 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,4 +1,4 @@ -import { ChangeDetectionStrategy, Component, Inject, OnDestroy, OnInit } from '@angular/core'; +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'; @@ -103,7 +103,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { { value: OPERATION_TYPES.AVERAGE, label: 'Mean' }, { value: OPERATION_TYPES.MODE, label: 'Mode' }, { value: OPERATION_TYPES.MEDIAN, label: 'Median' }, - { value: 'STDEV', label: 'Standard Deviation' }, + { value: OPERATION_TYPES.STDEV, label: 'Standard Deviation' }, ]; continuousIndividualOptions: StatisticOption[] = [ @@ -129,6 +129,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { private formBuilder: FormBuilder, private experimentService: ExperimentService, private analysisService: AnalysisService, + private cdr: ChangeDetectorRef, public dialogRef: MatDialogRef ) {} @@ -166,9 +167,12 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.allowableDataKeys = initialValues.allowableDataKeys || []; this.initialFormValues$.next(initialValues); - // Set initial form visibility states + // 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.detectMetricDataType(initialValues.metricId); } deriveInitialFormValues(sourceQuery: any, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { @@ -347,19 +351,21 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // 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.updateFormVisibility(); this.resetStatisticFields(); + this.updateFormVisibility(); this.updateFormValidators(); } @@ -369,6 +375,12 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.updateStatisticOptions(); this.updateFormVisibility(); this.updateFormValidators(); + } else { + // Clear everything when metric ID is cleared + this.metricDataType = null; + this.hideStatisticDropdowns(); + this.resetStatisticFields(); + this.updateFormValidators(); } } @@ -420,25 +432,36 @@ 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 === 'repeatable'; this.showMetricKey = metricType === 'repeatable'; - this.showIndividualStatistic = metricType === 'repeatable'; + + // Statistics only show when metric ID is selected + if (hasMetricId && this.metricDataType) { + this.showAggregateStatistic = true; + this.showIndividualStatistic = metricType === '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(); } updateStatisticOptions(): void { if (this.metricDataType === IMetricMetaData.CONTINUOUS) { this.aggregateStatisticOptions = this.continuousAggregateOptions; this.individualStatisticOptions = this.continuousIndividualOptions; - this.showComparison = false; - } else { + } else if (this.metricDataType === IMetricMetaData.CATEGORICAL) { this.aggregateStatisticOptions = this.categoricalAggregateOptions; this.individualStatisticOptions = this.categoricalIndividualOptions; - this.showComparison = true; } - } - - showStatisticDropdowns(): void { - this.showAggregateStatistic = true; + // Note: showComparison is handled in updateFormVisibility() } hideStatisticDropdowns(): void { @@ -451,6 +474,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.metricForm.patchValue({ aggregateStatistic: '', individualStatistic: '', + comparison: '=', + compareValue: '', }); this.allowableDataKeys = []; } @@ -531,17 +556,13 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } sendUpsertMetricRequest(): void { - const formValue = this.metricForm.value; - const metricData: MetricFormData = { - ...formValue, - allowableDataKeys: this.allowableDataKeys, - }; - // TODO: Implement the actual metric creation/update logic // This would typically call a service method to create or update the metric - console.log('Metric data to be processed:', metricData); - console.log('Experiment ID:', this.config.params.experimentId); - console.log('Action:', this.config.params.action); + // const formValue = this.metricForm.value; + // const metricData: MetricFormData = { + // ...formValue, + // allowableDataKeys: this.allowableDataKeys, + // }; // For now, just close the modal this.closeModal(); From 4e421bdaf3f46980f19595bb224f511c5ed315d5 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Tue, 23 Sep 2025 01:31:54 +0900 Subject: [PATCH 10/15] Disable global metric type when assignment unit is within-subject --- .../upsert-metric-modal.component.html | 2 +- .../upsert-metric-modal.component.ts | 139 ++++++++++++++++-- 2 files changed, 131 insertions(+), 10 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 3b09925276..58cbd9e658 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 @@ -12,7 +12,7 @@
- + Global Metric Used for globally accumulated measures (e.g., total time spent using the app). 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 b0e4a35e56..d25fad0e46 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 @@ -22,7 +22,7 @@ import { } from '../../../../../core/experiments/store/experiments.model'; import { ExperimentService } from '../../../../../core/experiments/experiments.service'; import { AnalysisService } from '../../../../../core/analysis/analysis.service'; -import { IMetricMetaData, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; +import { ASSIGNMENT_UNIT, IMetricMetaData, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; interface StatisticOption { value: string; @@ -64,6 +64,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { showIndividualStatistic = false; showComparison = false; metricDataType: IMetricMetaData | null = null; + isGlobalMetricDisabled = false; // Dropdown options aggregateStatisticOptions: StatisticOption[] = []; @@ -87,6 +88,10 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { currentSelectedClass: any = null; currentSelectedKey: any = null; + // Assignment unit and context for filtering (same as legacy component) + currentAssignmentUnit: ASSIGNMENT_UNIT | null = null; + currentContext: string[] | null = null; + // For categorical metrics comparison allowableDataKeys: string[] = []; comparisonOptions = [ @@ -137,6 +142,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.createMetricForm(); this.setupFormChangeListeners(); this.setupAutocomplete(); + this.setupExperimentContext(); // Add listeners AFTER form is fully set up this.listenForIsInitialFormValueChanged(); @@ -173,6 +179,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.updateStatisticOptions(); } this.updateFormVisibility(); + this.updateMetricTypeAvailability(); } deriveInitialFormValues(sourceQuery: any, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { @@ -250,17 +257,76 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { ); } + setupExperimentContext(): void { + // Get experiment context and assignment unit for filtering (same as legacy component) + this.subscriptions.add( + this.experimentService.selectedExperiment$.subscribe((experiment) => { + if (experiment) { + this.currentAssignmentUnit = experiment.assignmentUnit; + this.currentContext = experiment.context; + // Update metric type availability based on assignment unit + this.updateMetricTypeAvailability(); + // Repopulate options when experiment context changes + this.populateOptions(); + } + }) + ); + + // Also try to get experiment from the experimentId parameter if no selected experiment + 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; + // Update metric type availability based on assignment unit + this.updateMetricTypeAvailability(); + this.populateOptions(); + } + }) + ); + } + } + populateOptions(): void { const metricType = this.metricForm.get('metricType')?.value; + // Start with all metrics - this ensures dropdowns always have options + let filteredMetrics = this.allMetrics || []; + + // Apply assignment unit filtering ONLY if we have a valid experiment context + // This prevents breaking the dropdowns when no experiment is selected + if (this.currentAssignmentUnit && filteredMetrics.length > 0) { + if (this.currentAssignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS) { + // Within-subjects: only show metrics with children (repeatable metrics) + const withinSubjectsMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); + // Only apply filter if we found matching metrics, otherwise keep all metrics + if (withinSubjectsMetrics.length > 0) { + filteredMetrics = withinSubjectsMetrics; + } + } else if (this.currentContext && this.currentContext.length > 0) { + // Between-subjects or other: filter by context + const contextFilteredMetrics = filteredMetrics.filter( + (metric) => metric.context && this.currentContext?.some((ctx) => metric.context.includes(ctx)) + ); + // Only apply filter if we found matching metrics, otherwise keep all metrics + if (contextFilteredMetrics.length > 0) { + filteredMetrics = contextFilteredMetrics; + } + } + } + if (metricType === 'global') { - // Global metrics: only show metrics without children + // Global metrics: only show metrics without children from filtered set this.metricClassOptions$.next([]); this.metricKeyOptions$.next([]); - this.metricIdOptions$.next(this.allMetrics.filter((metric) => !metric.children || metric.children.length === 0)); + const globalMetrics = filteredMetrics.filter((metric) => !metric.children || metric.children.length === 0); + this.metricIdOptions$.next(globalMetrics); } else { - // Repeatable metrics: show hierarchical structure - this.metricClassOptions$.next(this.allMetrics.filter((metric) => metric.children && metric.children.length > 0)); + // Repeatable metrics: show hierarchical structure from filtered set + const repeatableMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); + this.metricClassOptions$.next(repeatableMetrics); this.metricKeyOptions$.next([]); this.metricIdOptions$.next([]); } @@ -453,6 +519,18 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { this.cdr.markForCheck(); } + updateMetricTypeAvailability(): void { + // Disable global metrics for within-subjects experiments (same as legacy component) + 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 === 'global') { + this.metricForm.get('metricType')?.setValue('repeatable'); + } + + this.cdr.markForCheck(); + } + updateStatisticOptions(): void { if (this.metricDataType === IMetricMetaData.CONTINUOUS) { this.aggregateStatisticOptions = this.continuousAggregateOptions; @@ -559,15 +637,58 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // TODO: Implement the actual metric creation/update logic // This would typically call a service method to create or update the metric // const formValue = this.metricForm.value; - // const metricData: MetricFormData = { - // ...formValue, - // allowableDataKeys: this.allowableDataKeys, - // }; + // const metricData = this.prepareMetricDataForBackend(formValue); + // Example: this.experimentService.createMetric(this.config.params.experimentId, metricData) // For now, just close the modal this.closeModal(); } + private prepareMetricDataForBackend(formValue: any): any { + const metricType = formValue.metricType; + const METRICS_JOIN_TEXT = '@__@'; // Same as backend constant + + // Prepare metric key based on type + let metricKey: string; + if (metricType === 'global') { + // Global metrics use just the metric ID + metricKey = typeof formValue.metricId === 'object' ? formValue.metricId.key : formValue.metricId; + } else { + // Repeatable metrics use class@__@key@__@id format + const metricClass = typeof formValue.metricClass === 'object' ? formValue.metricClass.key : formValue.metricClass; + const metricKeyValue = typeof formValue.metricKey === 'object' ? formValue.metricKey.key : formValue.metricKey; + const metricId = typeof formValue.metricId === 'object' ? formValue.metricId.key : formValue.metricId; + metricKey = `${metricClass}${METRICS_JOIN_TEXT}${metricKeyValue}${METRICS_JOIN_TEXT}${metricId}`; + } + + // Prepare query object in the same format as legacy component + const queryObj: any = { + name: formValue.displayName, + query: { + operationType: formValue.aggregateStatistic, + }, + metric: { + key: metricKey, + }, + }; + + // Add repeatedMeasure for repeatable metrics + if (metricType === 'repeatable') { + queryObj.repeatedMeasure = formValue.individualStatistic; + } + + // Add comparison function and value for categorical metrics + if (this.metricDataType === IMetricMetaData.CATEGORICAL && formValue.comparison && formValue.compareValue) { + queryObj.query = { + ...queryObj.query, + compareFn: formValue.comparison, + compareValue: formValue.compareValue, + }; + } + + return queryObj; + } + get UPSERT_EXPERIMENT_ACTION() { return UPSERT_EXPERIMENT_ACTION; } From 660449e4d4ada6dfea816fd73c80aa05f02cce9c Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 25 Sep 2025 20:00:45 +0900 Subject: [PATCH 11/15] Update template and styles to be consistent with other modals --- .../upsert-metric-modal.component.html | 64 ++++++++++--------- .../upsert-metric-modal.component.scss | 42 ++---------- .../shared/services/common-dialog.service.ts | 2 +- 3 files changed, 39 insertions(+), 69 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 58cbd9e658..503056b0bc 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 @@ -9,20 +9,30 @@
-
- - - - - Global Metric - Used for globally accumulated measures (e.g., total time spent using the app). - +
+ + + +
+ + Global Metric + + + Used for globally accumulated measures (e.g., total time spent using the app). + +
- - - Repeatable Metric - Used for repeatable measures (e.g., score on a quiz that can be taken multiple times). - + +
+ Repeatable Metric + Used for repeatable measures (e.g., score on a quiz that can be taken multiple times). +
@@ -48,7 +58,7 @@ - The metric class categorizes what type of app component is being measured (e.g., workspace). + Categorizes what type of app component is being measured (e.g., workspace). @@ -73,7 +83,7 @@ - The metric key specifies the specific instance of the metric class being measured (e.g., problem ID). + Specifies the specific instance of the metric class being measured (e.g., problem ID). @@ -98,7 +108,7 @@ - The Metric ID specifies the data type (continuous or categorical) and what data to measure. + Specifies the data type (continuous or categorical) and what data to measure. @@ -148,15 +158,16 @@ -
- - +
+ + - {{ option.label }} +
+ {{ option.label }} +
@@ -198,16 +209,7 @@ Description (optional) - - - Optional description for this metric. - + 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 index 9223b0b1f6..8511a0cc14 100644 --- 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 @@ -1,40 +1,8 @@ -.comparison-container { - .section-label { - display: block; - margin-bottom: 8px; - } - - .radio-group-horizontal { - display: flex; - gap: 16px; - - mat-radio-button { - margin-right: 0; - } - } +.disabled-text { + color: rgba(0, 0, 0, 0.38) !important; } -.metric-type-container { - .radio-group-vertical { - display: flex; - flex-direction: column; - gap: 12px; - - mat-radio-button { - margin-right: 0; - - .radio-label { - display: flex; - flex-direction: column; - gap: 4px; - - .radio-description { - font-weight: 400; - color: rgba(0, 0, 0, 0.6); - font-size: 12px; - line-height: 1.4; - } - } - } - } +.metric-type-section, +.comparison-section { + padding: 4px 0; } 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 f73c4a5b17..61231ff39a 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 @@ -188,7 +188,7 @@ export class DialogService { openUpsertMetricModal(commonModalConfig: CommonModalConfig) { const config: MatDialogConfig = { data: commonModalConfig, - width: ModalSize.LARGE, + width: ModalSize.STANDARD, autoFocus: false, disableClose: true, }; From a09795e8ba31a724db110db713d9c6c7f266a545 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 25 Sep 2025 21:55:37 +0900 Subject: [PATCH 12/15] Implement the backend requests for add/edit/delete --- .../experiments/experiments.data.service.ts | 9 + .../core/experiments/experiments.service.ts | 5 + .../core/experiments/metric-helper.service.ts | 66 ++++++++ .../experiments/store/experiments.actions.ts | 13 ++ .../experiments/store/experiments.effects.ts | 18 ++ .../experiments/store/experiments.model.ts | 5 + .../experiments/store/experiments.reducer.ts | 8 + .../upsert-metric-modal.component.ts | 155 +++++++++++++++--- ...periment-metrics-section-card.component.ts | 23 ++- .../shared/services/common-dialog.service.ts | 14 ++ 10 files changed, 281 insertions(+), 35 deletions(-) create mode 100644 frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.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 592a5771c9..c848e6f6cb 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, } from './store/experiments.model'; import { HttpClient, HttpParams } from '@angular/common/http'; @@ -165,4 +166,12 @@ export class ExperimentDataService { }; return this.updateExperiment(updatedExperiment); } + + 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 e3da2c5808..dca913ee5c 100644 --- a/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts +++ b/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts @@ -14,6 +14,7 @@ import { AddExperimentRequest, UpdateExperimentFilterModeRequest, UpdateExperimentDecisionPointsRequest, + UpdateExperimentMetricsRequest, } from './store/experiments.model'; import { Store, select } from '@ngrx/store'; import { @@ -199,6 +200,10 @@ export class ExperimentService { ); } + 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 b/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.ts new file mode 100644 index 0000000000..b08afdbec2 --- /dev/null +++ b/frontend/projects/upgrade/src/app/core/experiments/metric-helper.service.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); + } +} 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 f094bc0a8b..2d2b8523fc 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, } from './experiments.model'; import { ExperimentSegmentListRequest, ExperimentSegmentListDetails } from '../../segments/store/segments.model'; @@ -116,6 +117,18 @@ export const actionUpdateExperimentDecisionPointsFailure = createAction( '[Experiment] Update Experiment Decision Points 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'); + export const actionFetchAllDecisionPoints = createAction('[Experiment] Fetch All Decision Points'); export const actionFetchAllDecisionPointsSuccess = createAction( 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 eaa28786de..b353516e5e 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 @@ -240,6 +240,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 c00d9bdf88..e1e9617e5d 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 @@ -514,6 +514,11 @@ export interface UpdateExperimentDecisionPointsRequest { decisionPoints: ExperimentDecisionPoint[]; } +export interface UpdateExperimentMetricsRequest { + experiment: Experiment; + metrics: ExperimentQueryDTO[]; +} + export const EXPERIMENT_ROOT_COLUMN_NAMES = { NAME: 'name', STATUS: 'state', 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 48b45921ae..3d1a779f7f 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 @@ -106,6 +106,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.ts b/frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/upsert-metric-modal/upsert-metric-modal.component.ts index d25fad0e46..a2129dab5c 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,8 @@ 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 { map, startWith } from 'rxjs/operators'; +import { 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'; @@ -15,13 +16,18 @@ 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, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; interface StatisticOption { @@ -133,6 +139,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { public config: CommonModalConfig, private formBuilder: FormBuilder, private experimentService: ExperimentService, + private metricHelperService: MetricHelperService, private analysisService: AnalysisService, private cdr: ChangeDetectorRef, public dialogRef: MatDialogRef @@ -180,18 +187,48 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } this.updateFormVisibility(); this.updateMetricTypeAvailability(); + + // For edit mode, populate form after allMetrics are loaded + if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { + this.populateFormForEditMode(sourceQuery, initialValues); + } } deriveInitialFormValues(sourceQuery: any, action: UPSERT_EXPERIMENT_ACTION): MetricFormData { if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { - // Extract values from existing query for edit mode + 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: sourceQuery.repeatedMeasure ? 'repeatable' : 'global', - metricId: sourceQuery.metric?.key || '', + metricType: (isRepeatable ? 'repeatable' : 'global') as 'repeatable' | 'global', + metricId, displayName: sourceQuery.name || '', description: '', // Not available in current structure - metricClass: '', // Not available in current structure - metricKey: '', // Not available in current structure + metricClass, + metricKey: metricKeyValue, aggregateStatistic: sourceQuery.query?.operationType || '', individualStatistic: sourceQuery.repeatedMeasure || '', comparison: sourceQuery.query?.compareFn || '=', @@ -216,6 +253,67 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { }; } + populateFormForEditMode(sourceQuery: any, 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 = initialValues.metricType; + let classObject = null; + let keyObject = null; + let idObject = null; + + if (metricType === 'repeatable') { + // Find the class object + classObject = metrics.find((m) => m.key === initialValues.metricClass); + if (classObject && classObject.children) { + // Find the key object within the class children + keyObject = classObject.children.find((k) => k.key === initialValues.metricKey); + if (keyObject && keyObject.children) { + // Find the ID object within the key children + idObject = keyObject.children.find((id) => id.key === initialValues.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 === initialValues.metricId); + } + + // Update form with found objects (or keep strings if objects not found) + const formUpdates = { + metricType: initialValues.metricType, // Ensure metric type is properly set + metricClass: classObject || initialValues.metricClass, + metricKey: keyObject || initialValues.metricKey, + metricId: idObject || initialValues.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 { // Listen for metric type changes this.subscriptions.add( @@ -630,23 +728,40 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { onPrimaryActionBtnClicked(): void { if (this.metricForm.valid) { this.sendUpsertMetricRequest(); + } else { + CommonFormHelpersService.triggerTouchedToDisplayErrors(this.metricForm); } } sendUpsertMetricRequest(): void { - // TODO: Implement the actual metric creation/update logic - // This would typically call a service method to create or update the metric - // const formValue = this.metricForm.value; - // const metricData = this.prepareMetricDataForBackend(formValue); - // Example: this.experimentService.createMetric(this.config.params.experimentId, metricData) - - // For now, just close the modal - this.closeModal(); + 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): any { + private prepareMetricDataForBackend(formValue: any): ExperimentQueryDTO { const metricType = formValue.metricType; - const METRICS_JOIN_TEXT = '@__@'; // Same as backend constant // Prepare metric key based on type let metricKey: string; @@ -662,7 +777,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } // Prepare query object in the same format as legacy component - const queryObj: any = { + const queryObj: ExperimentQueryDTO = { name: formValue.displayName, query: { operationType: formValue.aggregateStatistic, @@ -670,13 +785,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metric: { key: metricKey, }, + repeatedMeasure: metricType === 'repeatable' ? formValue.individualStatistic : REPEATED_MEASURE.mostRecent, }; - // Add repeatedMeasure for repeatable metrics - if (metricType === 'repeatable') { - queryObj.repeatedMeasure = formValue.individualStatistic; - } - // Add comparison function and value for categorical metrics if (this.metricDataType === IMetricMetaData.CATEGORICAL && formValue.comparison && formValue.compareValue) { queryObj.query = { 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 020d2c9bda..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,6 +12,7 @@ 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 { @@ -23,7 +24,6 @@ import { import { UserPermission } from '../../../../../../../core/auth/store/auth.models'; import { AuthService } from '../../../../../../../core/auth/auth.service'; import { DialogService } from '../../../../../../../shared/services/common-dialog.service'; -import { ModalSize } from '../../../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; @Component({ selector: 'app-experiment-metrics-section-card', @@ -69,6 +69,7 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { constructor( private experimentService: ExperimentService, + private metricHelperService: MetricHelperService, private authService: AuthService, private dialogService: DialogService ) {} @@ -111,7 +112,7 @@ 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); @@ -122,23 +123,19 @@ export class ExperimentMetricsSectionCardComponent implements OnInit { this.dialogService.openEditMetricModal(query, experimentId); } - private onDeleteMetric(query: ExperimentQueryDTO, experimentId: string): void { + private onDeleteMetric(query: ExperimentQueryDTO): void { const metricDisplayName = query.name || `${query.metric?.key}`; - const confirmationParams = { - title: 'Delete Metric', - description: `Are you sure you want to delete the metric "${metricDisplayName}"?`, - primaryActionBtnText: 'Delete', - cancelBtnText: 'Cancel', - }; - this.dialogService - .openSimpleCommonConfirmationModal(confirmationParams, ModalSize.MEDIUM) + .openDeleteMetricModal(metricDisplayName) .afterClosed() .subscribe((confirmClicked) => { if (confirmClicked) { - // TODO: Implement actual metric deletion logic - console.log('Delete metric confirmed for query:', query.id, 'in experiment:', experimentId); + 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 61231ff39a..6e8c109fb5 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 @@ -650,6 +650,20 @@ export class DialogService { return this.openSimpleCommonConfirmationModal(deleteDecisionPointModalConfig, 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', From b70dddcad82b5cab3e557863bd24a85aeb206d02 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Thu, 25 Sep 2025 23:03:30 +0900 Subject: [PATCH 13/15] Move the descriptions and hints to en.json --- .../upsert-metric-modal.component.html | 20 ++++++++++--------- .../projects/upgrade/src/assets/i18n/en.json | 11 +++++++++- 2 files changed, 21 insertions(+), 10 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 503056b0bc..3cd20385fa 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 @@ -24,14 +24,16 @@ class="radio-description ft-12-400" [class.disabled-text]="isGlobalMetricDisabled" > - Used for globally accumulated measures (e.g., total time spent using the app). + {{ 'experiments.upsert-metric-modal.metric-type-global-metric-description.text' | translate }}
Repeatable Metric - Used for repeatable measures (e.g., score on a quiz that can be taken multiple times). + + {{ 'experiments.upsert-metric-modal.metric-type-repeatable-metric-description.text' | translate }} +
@@ -58,7 +60,7 @@ - Categorizes what type of app component is being measured (e.g., workspace). + {{ 'experiments.upsert-metric-modal.metric-class-hint.text' | translate }} @@ -83,7 +85,7 @@ - Specifies the specific instance of the metric class being measured (e.g., problem ID). + {{ 'experiments.upsert-metric-modal.metric-key-hint.text' | translate }} @@ -108,7 +110,7 @@ - Specifies the data type (continuous or categorical) and what data to measure. + {{ 'experiments.upsert-metric-modal.metric-id-hint.text' | translate }} @@ -126,7 +128,7 @@ - The Individual Statistic determines which value to use for each student. + {{ 'experiments.upsert-metric-modal.individual-statistic-hint.text' | translate }} Learn more @@ -148,7 +150,7 @@ - The Aggregate Statistic determines how to combine values across all students. + {{ 'experiments.upsert-metric-modal.aggregate-statistic-hint.text' | translate }} Learn more @@ -187,7 +189,7 @@ - The categorical metric data type requires you to specify which allowed value to measure. + {{ 'experiments.upsert-metric-modal.value-hint.text' | translate }} @@ -202,7 +204,7 @@ class="ft-14-400" /> - The display name is used to refer to this metric in the experiment UI. + {{ 'experiments.upsert-metric-modal.display-name-hint.text' | translate }} diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index f7c594ce42..54314495a5 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -440,6 +440,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", @@ -702,4 +711,4 @@ "monitor.metric-repeated-measure.text": "Repeated measure treatment", "monitor.monitored-metrics.text": "Monitored Metrics", "common-import-modal.incompatible-import-warning.text": "*Incompatible files will not be imported" -} \ No newline at end of file +} From 42b9cf64a431e83f93469052676066a0bb38cb4c Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 26 Sep 2025 18:32:04 +0900 Subject: [PATCH 14/15] refactor: clean up and optimize UpsertMetricModalComponent --- .../upsert-metric-modal.component.ts | 254 ++++++++---------- 1 file changed, 108 insertions(+), 146 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 a2129dab5c..3846639fca 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 { map, startWith, take } from 'rxjs/operators'; +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'; @@ -76,36 +76,34 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { aggregateStatisticOptions: StatisticOption[] = []; individualStatisticOptions: StatisticOption[] = []; - // Autocomplete - Professional solution with BehaviorSubjects for source data + // Autocomplete allMetrics$ = this.analysisService.allMetrics$; allMetrics: any[] = []; - // BehaviorSubjects for source data - this is the professional way - private metricClassOptions$ = new BehaviorSubject([]); - private metricKeyOptions$ = new BehaviorSubject([]); - private metricIdOptions$ = new BehaviorSubject([]); - - // Filtered observables that combine user input with reactive source data + // 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 - currentSelectedClass: any = null; - currentSelectedKey: any = null; + private currentSelectedClass: any = null; + private currentSelectedKey: any = null; - // Assignment unit and context for filtering (same as legacy component) - currentAssignmentUnit: ASSIGNMENT_UNIT | null = null; - currentContext: string[] | null = null; + // Assignment unit and context for filtering + private currentAssignmentUnit: ASSIGNMENT_UNIT | null = null; + private currentContext: string[] | null = null; - // For categorical metrics comparison allowableDataKeys: string[] = []; comparisonOptions = [ { value: '=', label: 'Equal' }, { value: '<>', label: 'Not equal' }, ]; - // Continuous statistic options continuousAggregateOptions: StatisticOption[] = [ { value: OPERATION_TYPES.SUM, label: 'Sum' }, { value: OPERATION_TYPES.MIN, label: 'Min' }, @@ -123,7 +121,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { { value: REPEATED_MEASURE.mostRecent, label: 'Most Recent' }, ]; - // Categorical statistic options categoricalAggregateOptions: StatisticOption[] = [ { value: OPERATION_TYPES.COUNT, label: 'Count' }, { value: OPERATION_TYPES.PERCENTAGE, label: 'Percentage' }, @@ -190,7 +187,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // For edit mode, populate form after allMetrics are loaded if (action === UPSERT_EXPERIMENT_ACTION.EDIT && sourceQuery) { - this.populateFormForEditMode(sourceQuery, initialValues); + this.populateFormForEditMode(initialValues); } } @@ -253,26 +250,28 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { }; } - populateFormForEditMode(sourceQuery: any, initialValues: MetricFormData): void { + 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 = initialValues.metricType; + const { metricType, metricClass, metricKey, metricId } = initialValues; let classObject = null; let keyObject = null; let idObject = null; if (metricType === 'repeatable') { // Find the class object - classObject = metrics.find((m) => m.key === initialValues.metricClass); - if (classObject && classObject.children) { + 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 === initialValues.metricKey); - if (keyObject && keyObject.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 === initialValues.metricId); + idObject = keyObject.children.find((id) => id.key === metricId); } else if (keyObject) { // If no children in keyObject, keyObject itself might be the ID idObject = keyObject; @@ -280,15 +279,14 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } else { // Global metric: find the metric directly - idObject = metrics.find((m) => m.key === initialValues.metricId); + idObject = metrics.find((m) => m.key === metricId); } // Update form with found objects (or keep strings if objects not found) const formUpdates = { - metricType: initialValues.metricType, // Ensure metric type is properly set - metricClass: classObject || initialValues.metricClass, - metricKey: keyObject || initialValues.metricKey, - metricId: idObject || initialValues.metricId, + metricClass: classObject || metricClass, + metricKey: keyObject || metricKey, + metricId: idObject || metricId, }; this.metricForm.patchValue(formUpdates); @@ -315,28 +313,24 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } setupFormChangeListeners(): void { - // Listen for metric type changes this.subscriptions.add( this.metricForm.get('metricType')?.valueChanges.subscribe(() => { this.onMetricTypeChange(); }) ); - // Listen for metric class changes this.subscriptions.add( this.metricForm.get('metricClass')?.valueChanges.subscribe((selectedClass) => { this.onMetricClassChange(selectedClass); }) ); - // Listen for metric key changes this.subscriptions.add( this.metricForm.get('metricKey')?.valueChanges.subscribe((selectedKey) => { this.onMetricKeyChange(selectedKey); }) ); - // Listen for metric ID changes this.subscriptions.add( this.metricForm.get('metricId')?.valueChanges.subscribe((metricId) => { this.onMetricIdChange(metricId); @@ -345,7 +339,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } setupAutocomplete(): void { - // Load all metrics data - EXACTLY like legacy component this.subscriptions.add( this.allMetrics$.subscribe((metrics) => { this.allMetrics = metrics || []; @@ -356,21 +349,17 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } setupExperimentContext(): void { - // Get experiment context and assignment unit for filtering (same as legacy component) this.subscriptions.add( this.experimentService.selectedExperiment$.subscribe((experiment) => { if (experiment) { this.currentAssignmentUnit = experiment.assignmentUnit; this.currentContext = experiment.context; - // Update metric type availability based on assignment unit this.updateMetricTypeAvailability(); - // Repopulate options when experiment context changes this.populateOptions(); } }) ); - // Also try to get experiment from the experimentId parameter if no selected experiment if (this.config.params.experimentId && !this.currentAssignmentUnit) { this.subscriptions.add( this.experimentService.experiments$.subscribe((experiments) => { @@ -378,7 +367,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { if (experiment && !this.currentAssignmentUnit) { this.currentAssignmentUnit = experiment.assignmentUnit; this.currentContext = experiment.context; - // Update metric type availability based on assignment unit this.updateMetricTypeAvailability(); this.populateOptions(); } @@ -389,26 +377,18 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { populateOptions(): void { const metricType = this.metricForm.get('metricType')?.value; - - // Start with all metrics - this ensures dropdowns always have options let filteredMetrics = this.allMetrics || []; - // Apply assignment unit filtering ONLY if we have a valid experiment context - // This prevents breaking the dropdowns when no experiment is selected if (this.currentAssignmentUnit && filteredMetrics.length > 0) { if (this.currentAssignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS) { - // Within-subjects: only show metrics with children (repeatable metrics) const withinSubjectsMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); - // Only apply filter if we found matching metrics, otherwise keep all metrics if (withinSubjectsMetrics.length > 0) { filteredMetrics = withinSubjectsMetrics; } } else if (this.currentContext && this.currentContext.length > 0) { - // Between-subjects or other: filter by context const contextFilteredMetrics = filteredMetrics.filter( (metric) => metric.context && this.currentContext?.some((ctx) => metric.context.includes(ctx)) ); - // Only apply filter if we found matching metrics, otherwise keep all metrics if (contextFilteredMetrics.length > 0) { filteredMetrics = contextFilteredMetrics; } @@ -416,13 +396,11 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } if (metricType === 'global') { - // Global metrics: only show metrics without children from filtered set this.metricClassOptions$.next([]); this.metricKeyOptions$.next([]); const globalMetrics = filteredMetrics.filter((metric) => !metric.children || metric.children.length === 0); this.metricIdOptions$.next(globalMetrics); } else { - // Repeatable metrics: show hierarchical structure from filtered set const repeatableMetrics = filteredMetrics.filter((metric) => metric.children && metric.children.length > 0); this.metricClassOptions$.next(repeatableMetrics); this.metricKeyOptions$.next([]); @@ -431,7 +409,6 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } createFilteredObservables(): void { - // Professional solution: combine user input with reactive source data this.filteredMetricClasses$ = combineLatest([ this.metricForm.get('metricClass')?.valueChanges.pipe(startWith('')) || new BehaviorSubject(''), this.metricClassOptions$, @@ -490,24 +467,16 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Don't do anything for string values - user is typing } - displayFn(option?: any): string | undefined { - if (option && option.key) { - return option.key; - } else if (typeof option === 'string') { - return option; - } - return option ? option : undefined; + 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[] { - let filterValue: string; - if (typeof value === 'string') { - filterValue = value.toLowerCase(); - } else if (value && value.key) { - filterValue = value.key.toLowerCase(); - } else { - filterValue = ''; - } + const filterValue = this.extractKey(value).toLowerCase(); return options.filter((option) => option.key?.toLowerCase().includes(filterValue)); } @@ -549,48 +518,60 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } detectMetricDataType(metricId: any): void { - let selectedMetric: any = null; + const selectedMetric = this.findSelectedMetric(metricId); - // Find the selected metric in current metricIdOptions + // 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) { - selectedMetric = metricId; - } else if (typeof metricId === 'string') { - // Find by string key - get current value from BehaviorSubject + return metricId; + } + + if (typeof metricId === 'string') { const currentOptions = this.metricIdOptions$.getValue(); - selectedMetric = currentOptions.find((metric) => metric.key === metricId); + return currentOptions.find((metric) => metric.key === metricId); } - if (selectedMetric && selectedMetric.metadata && selectedMetric.metadata.type) { - this.metricDataType = selectedMetric.metadata.type; + return null; + } - // For categorical metrics, populate allowable data from the metric and set default comparison - if (this.metricDataType === IMetricMetaData.CATEGORICAL) { - if (selectedMetric.allowedData) { - this.allowableDataKeys = [...selectedMetric.allowedData]; - } - // Set default comparison to "=" if not already set - if (!this.metricForm.get('comparison')?.value) { - this.metricForm.get('comparison')?.setValue('='); - } - } else { - this.allowableDataKeys = []; + 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 { - // Fallback to heuristic if metadata is not available - const metricKey = typeof metricId === 'string' ? metricId : metricId?.key || ''; - const continuousKeywords = ['time', 'count', 'score', 'number', 'seconds', 'minutes', 'duration']; - const categoricalKeywords = ['status', 'type', 'category', 'level', 'completion']; + this.allowableDataKeys = []; + } + } - const lowerMetricKey = metricKey.toLowerCase(); + private detectMetricTypeByHeuristic(metricId: any): void { + const metricKey = this.extractKey(metricId); + const lowerMetricKey = metricKey.toLowerCase(); - if (continuousKeywords.some((keyword) => lowerMetricKey.includes(keyword))) { - this.metricDataType = IMetricMetaData.CONTINUOUS; - } else if (categoricalKeywords.some((keyword) => lowerMetricKey.includes(keyword))) { - this.metricDataType = IMetricMetaData.CATEGORICAL; - } else { - // Default to continuous for unknown types - this.metricDataType = IMetricMetaData.CONTINUOUS; - } + 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); } } @@ -618,7 +599,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } updateMetricTypeAvailability(): void { - // Disable global metrics for within-subjects experiments (same as legacy component) + // 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 @@ -693,35 +674,27 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } listenForIsInitialFormValueChanged(): void { - this.isInitialFormValueChanged$ = combineLatest([ - this.metricForm.valueChanges.pipe(startWith(this.metricForm.value)), - this.initialFormValues$, - ]).pipe( - map(([currentFormValue, initialFormValue]) => { - if (!initialFormValue) return false; - + this.isInitialFormValueChanged$ = this.metricForm.valueChanges.pipe( + startWith(this.metricForm.value), + map(() => { const currentWithKeys = { - ...currentFormValue, + ...this.metricForm.value, allowableDataKeys: this.allowableDataKeys, }; - - return JSON.stringify(currentWithKeys) !== JSON.stringify(initialFormValue); + return !isEqual(currentWithKeys, this.initialFormValues$.value); }) ); } listenForPrimaryButtonDisabled(): void { - this.isPrimaryButtonDisabled$ = combineLatest([ - this.metricForm.statusChanges.pipe(startWith(this.metricForm.status)), - this.isLoadingUpsertMetric$, - this.isInitialFormValueChanged$, - ]).pipe( - map(([formStatus, isLoading, isFormChanged]) => { - const isFormInvalid = formStatus !== 'VALID'; - const isEditModeWithoutChanges = this.config.params.action === UPSERT_EXPERIMENT_ACTION.EDIT && !isFormChanged; - - return isFormInvalid || isLoading || isEditModeWithoutChanges; - }) + this.isPrimaryButtonDisabled$ = this.isLoadingUpsertMetric$.pipe( + combineLatestWith(this.isInitialFormValueChanged$), + map( + ([isLoading, isInitialFormValueChanged]) => + isLoading || + this.metricForm.invalid || + (!isInitialFormValueChanged && this.config.params.action === UPSERT_EXPERIMENT_ACTION.EDIT) + ) ); } @@ -761,26 +734,28 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } private prepareMetricDataForBackend(formValue: any): ExperimentQueryDTO { - const metricType = formValue.metricType; + const { metricType, metricClass, metricKey: metricKeyValue, metricId } = formValue; // Prepare metric key based on type - let metricKey: string; - if (metricType === 'global') { - // Global metrics use just the metric ID - metricKey = typeof formValue.metricId === 'object' ? formValue.metricId.key : formValue.metricId; - } else { - // Repeatable metrics use class@__@key@__@id format - const metricClass = typeof formValue.metricClass === 'object' ? formValue.metricClass.key : formValue.metricClass; - const metricKeyValue = typeof formValue.metricKey === 'object' ? formValue.metricKey.key : formValue.metricKey; - const metricId = typeof formValue.metricId === 'object' ? formValue.metricId.key : formValue.metricId; - metricKey = `${metricClass}${METRICS_JOIN_TEXT}${metricKeyValue}${METRICS_JOIN_TEXT}${metricId}`; - } - - // Prepare query object in the same format as legacy component + const metricKey = + metricType === '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, @@ -788,22 +763,9 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { repeatedMeasure: metricType === 'repeatable' ? formValue.individualStatistic : REPEATED_MEASURE.mostRecent, }; - // Add comparison function and value for categorical metrics - if (this.metricDataType === IMetricMetaData.CATEGORICAL && formValue.comparison && formValue.compareValue) { - queryObj.query = { - ...queryObj.query, - compareFn: formValue.comparison, - compareValue: formValue.compareValue, - }; - } - return queryObj; } - get UPSERT_EXPERIMENT_ACTION() { - return UPSERT_EXPERIMENT_ACTION; - } - closeModal(): void { this.dialogRef.close(); } From ff25580a38a468b3d18439ddf141797d1da113f7 Mon Sep 17 00:00:00 2001 From: Zack Lee Date: Fri, 26 Sep 2025 19:49:45 +0900 Subject: [PATCH 15/15] Add METRIC_TYPE enum to replace string literals in metric modal --- .../experiments/store/experiments.model.ts | 4 ++- .../upsert-metric-modal.component.ts | 27 ++++++++++--------- types/src/Experiment/enums.ts | 5 ++++ types/src/index.ts | 1 + 4 files changed, 23 insertions(+), 14 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 e1e9617e5d..0e6d8cbbcc 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 { @@ -362,7 +364,7 @@ export interface DecisionPointFormData { } export interface MetricFormData { - metricType: 'global' | 'repeatable'; + metricType: METRIC_TYPE; metricId: string; displayName: string; description?: 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 3846639fca..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 @@ -28,7 +28,7 @@ import { ExperimentService } from '../../../../../core/experiments/experiments.s 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, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; +import { ASSIGNMENT_UNIT, IMetricMetaData, METRIC_TYPE, OPERATION_TYPES, REPEATED_MEASURE } from 'upgrade_types'; interface StatisticOption { value: string; @@ -220,7 +220,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } return { - metricType: (isRepeatable ? 'repeatable' : 'global') as 'repeatable' | 'global', + metricType: isRepeatable ? METRIC_TYPE.REPEATABLE : METRIC_TYPE.GLOBAL, metricId, displayName: sourceQuery.name || '', description: '', // Not available in current structure @@ -236,7 +236,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Default values for add mode return { - metricType: 'global', + metricType: METRIC_TYPE.GLOBAL, metricId: '', displayName: '', description: '', @@ -261,7 +261,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { let keyObject = null; let idObject = null; - if (metricType === 'repeatable') { + if (metricType === METRIC_TYPE.REPEATABLE) { // Find the class object classObject = metrics.find((m) => m.key === metricClass); @@ -395,7 +395,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { } } - if (metricType === 'global') { + if (metricType === METRIC_TYPE.GLOBAL) { this.metricClassOptions$.next([]); this.metricKeyOptions$.next([]); const globalMetrics = filteredMetrics.filter((metric) => !metric.children || metric.children.length === 0); @@ -580,13 +580,13 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { const hasMetricId = !!this.metricForm.get('metricId')?.value; // Base visibility for metric type - this.showMetricClass = metricType === 'repeatable'; - this.showMetricKey = metricType === 'repeatable'; + 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 === 'repeatable'; + this.showIndividualStatistic = metricType === METRIC_TYPE.REPEATABLE; this.showComparison = this.metricDataType === IMetricMetaData.CATEGORICAL; } else { this.showAggregateStatistic = false; @@ -603,8 +603,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 === 'global') { - this.metricForm.get('metricType')?.setValue('repeatable'); + if (this.isGlobalMetricDisabled && this.metricForm.get('metricType')?.value === METRIC_TYPE.GLOBAL) { + this.metricForm.get('metricType')?.setValue(METRIC_TYPE.REPEATABLE); } this.cdr.markForCheck(); @@ -641,7 +641,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { const metricType = this.metricForm.get('metricType')?.value; // Update validators based on metric type - if (metricType === 'repeatable') { + 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]); @@ -738,7 +738,7 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { // Prepare metric key based on type const metricKey = - metricType === 'global' + metricType === METRIC_TYPE.GLOBAL ? this.extractKey(metricId) : `${this.extractKey(metricClass)}${METRICS_JOIN_TEXT}${this.extractKey( metricKeyValue @@ -760,7 +760,8 @@ export class UpsertMetricModalComponent implements OnInit, OnDestroy { metric: { key: metricKey, }, - repeatedMeasure: metricType === 'repeatable' ? formValue.individualStatistic : REPEATED_MEASURE.mostRecent, + repeatedMeasure: + metricType === METRIC_TYPE.REPEATABLE ? formValue.individualStatistic : REPEATED_MEASURE.mostRecent, }; return queryObj; diff --git a/types/src/Experiment/enums.ts b/types/src/Experiment/enums.ts index acefd15539..7531053ac9 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,