From bc0c7398077ed1ff68ebad52e27edad3f5f12d2f Mon Sep 17 00:00:00 2001 From: pratik Date: Sat, 29 Mar 2025 23:42:03 +0530 Subject: [PATCH 01/10] add-edit-duplicate segment modal functionality --- .../src/api/services/SegmentService.ts | 4 + .../store/feature-flags.effects.ts | 3 +- .../store/feature-flags.model.ts | 2 +- .../local-storage/local-storage.service.ts | 1 + .../core/segments/segments.data.service.ts | 12 +- .../src/app/core/segments/segments.service.ts | 31 ++- .../core/segments/store/segments.actions.ts | 23 ++ .../core/segments/store/segments.effects.ts | 38 ++- .../app/core/segments/store/segments.model.ts | 46 ++++ .../core/segments/store/segments.reducer.ts | 1 + .../core/segments/store/segments.selectors.ts | 22 ++ .../upsert-feature-flag-modal.component.ts | 2 - .../upsert-segment-modal.component.html | 55 ++++- .../upsert-segment-modal.component.ts | 219 +++++++++++++++++- ...overview-details-section-card.component.ts | 4 +- .../segment-root-section-card.component.ts | 2 +- .../shared/services/common-dialog.service.ts | 65 +++++- .../projects/upgrade/src/assets/i18n/en.json | 5 + types/src/Experiment/enums.ts | 1 + 19 files changed, 520 insertions(+), 16 deletions(-) diff --git a/backend/packages/Upgrade/src/api/services/SegmentService.ts b/backend/packages/Upgrade/src/api/services/SegmentService.ts index 67b6328ba7..53da2f871f 100644 --- a/backend/packages/Upgrade/src/api/services/SegmentService.ts +++ b/backend/packages/Upgrade/src/api/services/SegmentService.ts @@ -242,6 +242,9 @@ export class SegmentService { case SEGMENT_SEARCH_KEY.NAME: searchString.push("coalesce(segment.name::TEXT,'')"); break; + case SEGMENT_SEARCH_KEY.STATUS: + searchString.push("coalesce(segment.status::TEXT,'')"); + break; case SEGMENT_SEARCH_KEY.CONTEXT: searchString.push("coalesce(segment.context::TEXT,'')"); break; @@ -250,6 +253,7 @@ export class SegmentService { break; default: searchString.push("coalesce(segment.name::TEXT,'')"); + searchString.push("coalesce(feature_flag.status::TEXT,'')"); searchString.push("coalesce(segment.context::TEXT,'')"); searchString.push("coalesce(segment.tags::TEXT,'')"); break; diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts index 43aaf8b03c..a3112f2c32 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts @@ -92,7 +92,7 @@ export class FeatureFlagsEffects { ) ); - // actionCreateFeatureFlag dispatch POST feature flag + // actionAddFeatureFlag dispatch POST feature flag addFeatureFlag$ = createEffect(() => this.actions$.pipe( ofType(FeatureFlagsActions.actionAddFeatureFlag), @@ -313,6 +313,7 @@ export class FeatureFlagsEffects { ) ) ); + exportFeatureFlagsDesign$ = createEffect(() => this.actions$.pipe( ofType(FeatureFlagsActions.actionExportFeatureFlagDesign), diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts index c04f82e479..47c5345cb0 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts @@ -23,7 +23,7 @@ export interface CoreFeatureFlagDetails { key: string; description?: string; context: string[]; - tags: string[]; + tags?: string[]; status: FEATURE_FLAG_STATUS; filterMode: FILTER_MODE; } diff --git a/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts b/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts index 0f83554b0d..9e99588f43 100755 --- a/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts +++ b/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts @@ -91,6 +91,7 @@ export class LocalStorageService { searchString: segmentSearchString || null, sortKey: (segmentSortKey as SEGMENT_SORT_KEY) || SEGMENT_SORT_KEY.NAME, sortAs: (segmentSortType as SORT_AS_DIRECTION) || SORT_AS_DIRECTION.ASCENDING, + isLoadingUpsertSegment: false, }; const state = { diff --git a/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts b/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts index 0406df73b2..df5db40d5c 100644 --- a/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts +++ b/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts @@ -1,5 +1,5 @@ import { Inject, Injectable } from '@angular/core'; -import { SegmentFile, SegmentInput } from './store/segments.model'; +import { AddSegmentRequest, SegmentFile, SegmentInput } from './store/segments.model'; import { HttpClient, HttpParams } from '@angular/common/http'; import { ENV, Environment } from '../../../environments/environment-types'; @@ -17,6 +17,11 @@ export class SegmentsDataService { return this.http.post(url, segment); } + addSegment(segment: AddSegmentRequest) { + const url = this.environment.api.segments; + return this.http.post(url, segment); + } + getSegmentById(id: string) { const url = `${this.environment.api.segments}/status/${id}`; return this.http.get(url); @@ -32,6 +37,11 @@ export class SegmentsDataService { return this.http.post(url, segment); } + modifySegment(segment: AddSegmentRequest) { + const url = this.environment.api.segments; + return this.http.post(url, segment); + } + exportSegments(segmentIds: string[]) { let ids = new HttpParams(); segmentIds.forEach((id) => { diff --git a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index 458f6f4d61..e1d6d96c21 100644 --- a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -17,15 +17,19 @@ import { selectSortKey, selectSortAs, selectSegmentLists, + selectAppContexts, + selectIsLoadingUpsertSegment, } from './store/segments.selectors'; import { + AddSegmentRequest, LIST_OPTION_TYPE, Segment, SegmentInput, SegmentLocalStorageKeys, + UpdateSegmentRequest, UpsertSegmentType, } from './store/segments.model'; -import { filter, map, tap, withLatestFrom } from 'rxjs/operators'; +import { filter, map, pairwise, tap, withLatestFrom } from 'rxjs/operators'; import { Observable, combineLatest } from 'rxjs'; import { SegmentsDataService } from './segments.data.service'; import { SEGMENT_SEARCH_KEY, SORT_AS_DIRECTION, SEGMENT_SORT_KEY } from 'upgrade_types'; @@ -34,6 +38,8 @@ import { selectShouldUseLegacyUI } from './store/segments.selectors'; import { selectContextMetaData } from '../experiments/store/experiments.selectors'; import { selectSelectedFeatureFlag } from '../feature-flags/store/feature-flags.selectors'; import { CommonTextHelpersService } from '../../shared/services/common-text-helpers.service'; +import { actionFetchContextMetaData } from '../experiments/store/experiments.actions'; +import isEqual from 'lodash.isequal'; @Injectable({ providedIn: 'root' }) export class SegmentsService { @@ -44,6 +50,15 @@ export class SegmentsService { ) {} isLoadingSegments$ = this.store$.pipe(select(selectIsLoadingSegments)); + isLoadingUpsertSegment$ = this.store$.pipe(select(selectIsLoadingUpsertSegment)); + isSelectedSegmentUpdated$ = this.store$.pipe( + select(selectSelectedSegment), + pairwise(), + filter(([prev, curr]) => { + return prev && curr && !isEqual(prev, curr); + }), + map(([, curr]) => curr) + ); selectAllSegments$ = this.store$.pipe(select(selectAllSegments)); selectedSegment$ = this.store$.pipe(select(selectSelectedSegment)); shouldUseLegacyView$ = this.store$.pipe(select(selectShouldUseLegacyUI)); @@ -57,6 +72,7 @@ export class SegmentsService { select(selectSegmentLists), map((lists) => lists.length) ); + appContexts$ = this.store$.pipe(select(selectAppContexts)); allExperimentSegmentsInclusion$ = this.store$.pipe(select(selectExperimentSegmentsInclusion)); allExperimentSegmentsExclusion$ = this.store$.pipe(select(selectExperimentSegmentsExclusion)); allFeatureFlagSegmentsExclusion$ = this.store$.pipe(select(selectFeatureFlagSegmentsExclusion)); @@ -132,6 +148,10 @@ export class SegmentsService { this.store$.dispatch(SegmentsActions.actionGetSegmentById({ segmentId })); } + fetchContextMetaData() { + this.store$.dispatch(actionFetchContextMetaData({ isLoadingContextMetaData: true })); + } + isInitialSegmentsLoading() { return combineLatest(this.store$.pipe(select(selectIsLoadingSegments)), this.allSegments$).pipe( map(([isLoading, segments]) => !isLoading || !!segments.length) @@ -178,6 +198,15 @@ export class SegmentsService { ); } + addSegment(addSegmentRequest: AddSegmentRequest) { + this.store$.dispatch(SegmentsActions.actionAddSegment({ addSegmentRequest })); + } + + + modifySegment(updateSegmentRequest: UpdateSegmentRequest) { + this.store$.dispatch(SegmentsActions.actionUpdateSegment({ updateSegmentRequest })); + } + exportSegments(segmentIds: string[]) { this.store$.dispatch(SegmentsActions.actionExportSegments({ segmentIds })); } diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts index c7234c3dd5..f4f4f4abf9 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts @@ -1,7 +1,9 @@ import { createAction, props } from '@ngrx/store'; import { + AddSegmentRequest, Segment, SegmentInput, + UpdateSegmentRequest, UpsertSegmentType, experimentSegmentInclusionExclusionData, featureFlagSegmentInclusionExclusionData, @@ -39,6 +41,27 @@ export const actionUpsertSegmentSuccess = createAction( export const actionUpsertSegmentFailure = createAction('[Segments] Upsert Segment Failure'); +export const actionAddSegment = createAction( + '[Segments] Add Segment', + props<{ addSegmentRequest: AddSegmentRequest }>() +); + +export const actionAddSegmentSuccess = createAction('[Segments] Add Segment Success', props<{ response: Segment }>()); + +export const actionAddSegmentFailure = createAction('[Segments] Add Segment Failure'); + +export const actionUpdateSegment = createAction( + '[Segments] Update Segment', + props<{ updateSegmentRequest: UpdateSegmentRequest }>() +); + +export const actionUpdateSegmentSuccess = createAction( + '[Segments] Update Segment Success', + props<{ response: Segment }>() +); + +export const actionUpdateSegmentFailure = createAction('[Segments] Update Segment Failure'); + export const actionGetSegmentById = createAction('[Segments] Get Segment By Id', props<{ segmentId: string }>()); export const actionGetSegmentByIdSuccess = createAction( diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts index 226dfcf724..bad7292247 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts @@ -2,13 +2,14 @@ import { Injectable } from '@angular/core'; import { Router } from '@angular/router'; import { Actions, createEffect, ofType } from '@ngrx/effects'; import { select, Store } from '@ngrx/store'; -import { catchError, filter, map, switchMap, withLatestFrom } from 'rxjs/operators'; +import { catchError, filter, map, switchMap, tap, withLatestFrom } from 'rxjs/operators'; import { AppState } from '../../core.module'; import { SegmentsDataService } from '../segments.data.service'; import * as SegmentsActions from './segments.actions'; import { Segment, UpsertSegmentType } from './segments.model'; import { selectAllSegments } from './segments.selectors'; import JSZip from 'jszip'; +import { SERVER_ERROR } from 'upgrade_types'; @Injectable() export class SegmentsEffects { @@ -81,6 +82,41 @@ export class SegmentsEffects { ) ); + // actionAddSegment dispatch POST segment + addSegment$ = createEffect(() => + this.actions$.pipe( + ofType(SegmentsActions.actionAddSegment), + switchMap((action) => { + return this.segmentsDataService.addSegment(action.addSegmentRequest).pipe( + map((response: Segment) => SegmentsActions.actionAddSegmentSuccess({ response })), + tap(({ response }) => { + this.router.navigate(['/segments', 'detail', response.id]); + }), + catchError(() => { + return [SegmentsActions.actionAddSegmentFailure()]; + }) + ); + }) + ) + ); + + updateSegment$ = createEffect(() => + this.actions$.pipe( + ofType(SegmentsActions.actionUpdateSegment), + switchMap((action) => { + return this.segmentsDataService.modifySegment(action.updateSegmentRequest).pipe( + map((response: Segment) => SegmentsActions.actionUpdateSegmentSuccess({ response })), + tap(({ response }) => { + this.router.navigate(['/segments', 'detail', response.id]); + }), + catchError(() => { + return [SegmentsActions.actionUpdateSegmentFailure()]; + }) + ); + }) + ) + ); + deleteSegment$ = createEffect(() => this.actions$.pipe( ofType(SegmentsActions.actionDeleteSegment), diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts index a6ef4a5106..dd6ddbd14a 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts @@ -6,6 +6,7 @@ import { SEGMENT_SEARCH_KEY, SORT_AS_DIRECTION, SEGMENT_SORT_KEY, + FILTER_MODE, FEATURE_FLAG_LIST_FILTER_MODE, } from 'upgrade_types'; export { SEGMENT_STATUS }; @@ -111,6 +112,27 @@ export interface Segment { status: SEGMENT_STATUS; } +export interface CoreSegmentDetails { + id?: string; + name: string; + context: string; + description?: string; + tags?: string[]; + userIds: string[]; + groups: Group[]; + subSegmentIds: string[]; + status?: SEGMENT_STATUS; + type: SEGMENT_TYPE; +} + +// Currently there is no difference between these types, but they semantically different and could diverge later +export type AddSegmentRequest = CoreSegmentDetails; + +// so that we can throw an error if we try to update the id +export interface UpdateSegmentRequest extends AddSegmentRequest { + readonly id: string; +} + export interface SegmentInput { createdAt: string; updatedAt: string; @@ -140,6 +162,7 @@ export enum SEGMENT_DETAILS_PAGE_ACTIONS { export interface SegmentState extends EntityState { isLoadingSegments: boolean; + isLoadingUpsertSegment: boolean; // TODO: remove any allExperimentSegmentsInclusion: any; allExperimentSegmentsExclusion: any; @@ -249,3 +272,26 @@ export interface AddPrivateSegmentListRequest extends PrivateSegmentListRequest export interface EditPrivateSegmentListRequest extends PrivateSegmentListRequest { segment: EditPrivateSegmentListDetails; } + +export enum CommonTagInputType { + TAGS = 'tags', + VALUES = 'values', +} + +export interface SegmentFormData { + name: string; + description: string; + appContext: string; + tags: string[]; +} + +export enum UPSERT_SEGMENT_ACTION { + ADD = 'add', + EDIT = 'edit', + DUPLICATE = 'duplicate', +} + +export interface UpsertSegmentParams { + sourceSegment: Segment; + action: UPSERT_SEGMENT_ACTION; +} diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts index 6090241d3d..0809ff158e 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts @@ -22,6 +22,7 @@ export const initialState: SegmentState = adapter.getInitialState({ searchString: null, sortKey: SEGMENT_SORT_KEY.NAME, sortAs: SORT_AS_DIRECTION.ASCENDING, + isLoadingUpsertSegment: false, }); const reducer = createReducer( diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts index a0c5764b72..f2e6ee7edd 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts @@ -18,6 +18,10 @@ export const selectSegmentById = createSelector( (state, { segmentId }) => state.entities[segmentId] ); +export const selectAppContexts = createSelector(selectContextMetaData, (contextMetaData) => + Object.keys(contextMetaData?.contextMetadata ?? []) +); + export const selectExperimentSegmentsInclusion = createSelector( selectSegmentsState, (state) => state.allExperimentSegmentsInclusion @@ -38,6 +42,24 @@ export const selectFeatureFlagSegmentsExclusion = createSelector( (state) => state.allFeatureFlagSegmentsExclusion ); +export const selectIsLoadingUpsertSegment = createSelector( + selectSegmentsState, + (state) => state.isLoadingUpsertSegment +); + +// export const selectSelectedSegment = createSelector( +// selectRouterState, +// selectSegmentsState, +// (routerState, segmentState) => { +// // be very defensive here to make sure routerState is correct +// const segmentId = routerState?.state?.params?.segmentId; +// if (segmentId) { +// return segmentState.entities[segmentId]; +// } +// return undefined; +// } +// ); + export const selectSelectedSegment = createSelector( selectRouterState, selectSegmentsState, diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 616f77ee5c..309c133197 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -72,7 +72,6 @@ export class UpsertFeatureFlagModalComponent { @Inject(MAT_DIALOG_DATA) public config: CommonModalConfig, public dialog: MatDialog, - private router: Router, private formBuilder: FormBuilder, private featureFlagsService: FeatureFlagsService, private experimentService: ExperimentService, @@ -211,7 +210,6 @@ export class UpsertFeatureFlagModalComponent { sendRequest(action: UPSERT_FEATURE_FLAG_ACTION, sourceFlag?: FeatureFlag): void { const formData: FeatureFlagFormData = this.featureFlagForm.value; - if (action === UPSERT_FEATURE_FLAG_ACTION.ADD || action === UPSERT_FEATURE_FLAG_ACTION.DUPLICATE) { this.createAddRequest(formData); } else if (action === UPSERT_FEATURE_FLAG_ACTION.EDIT && sourceFlag) { diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html index cc87ad298d..ab2d1a2c69 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html @@ -1 +1,54 @@ -

upsert-segment-modal works!

+ +
+ + Name + + + {{ 'segments.upsert-segment-modal.name-hint.text' | translate }} + + + + + Description (optional) + + + + + App Context + + {{ + context + }} + + + {{ 'segments.upsert-segment-modal.app-context-hint.text' | translate }} + + Learn more + + + + {{ 'segments.upsert-segment-modal.app-context-warning.text' | translate }} + + + + +
+ diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts index da6180dbf8..17852cd5f9 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts @@ -1,10 +1,221 @@ -import { ChangeDetectionStrategy, Component } from '@angular/core'; +import { ChangeDetectionStrategy, Component, Inject } from '@angular/core'; +import { + CommonModalComponent, + CommonTagsInputComponent, +} from '../../../../../shared-standalone-component-lib/components'; +import { MAT_DIALOG_DATA, MatDialogRef } from '@angular/material/dialog'; +import { CommonModule } from '@angular/common'; +import { FormBuilder, FormGroup, ReactiveFormsModule, Validators } from '@angular/forms'; +import { MatButtonModule } from '@angular/material/button'; +import { MatCardModule } from '@angular/material/card'; +import { MatFormFieldModule } from '@angular/material/form-field'; +import { MatInputModule } from '@angular/material/input'; +import { MatSelectModule } from '@angular/material/select'; +import { CommonFormHelpersService } from '../../../../../shared/services/common-form-helpers.service'; +import { + CommonTagInputType, + Segment, + SEGMENT_STATUS, + SegmentFormData, + UPSERT_SEGMENT_ACTION, + UpsertSegmentParams, + AddSegmentRequest, + UpdateSegmentRequest, +} from '../../../../../core/segments/store/segments.model'; +import { SegmentsService } from '../../../../../core/segments/segments.service'; +import { BehaviorSubject, Observable, map, startWith, Subscription, combineLatestWith } from 'rxjs'; +import { TranslateModule } from '@ngx-translate/core'; +import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; +import { ExperimentService } from '../../../../../core/experiments/experiments.service'; +import { SEGMENT_TYPE } from '../../../../../../../../../../types/src'; +import isEqual from 'lodash.isequal'; @Component({ - selector: 'app-upsert-segment-modal', - imports: [], + selector: 'upsert-add-segment-modal', + imports: [ + CommonModalComponent, + MatCardModule, + MatFormFieldModule, + MatInputModule, + MatButtonModule, + MatSelectModule, + CommonModule, + ReactiveFormsModule, + TranslateModule, + CommonTagsInputComponent, + ], templateUrl: './upsert-segment-modal.component.html', styleUrl: './upsert-segment-modal.component.scss', changeDetection: ChangeDetectionStrategy.OnPush, }) -export class UpsertSegmentModalComponent {} +export class UpsertSegmentModalComponent { + segmentForm: FormGroup; + isLoadingUpsertSegment$ = this.segmentService.isLoadingUpsertSegment$; + isPrimaryButtonDisabled$: Observable; + appContexts$ = this.segmentService.appContexts$; + CommonTagInputType = CommonTagInputType; + isSelectedSegmentUpdated$ = this.segmentService.isSelectedSegmentUpdated$; + initialFormValues$ = new BehaviorSubject(null); + subscriptions = new Subscription(); + isContextChanged = false; + initialContext = ''; + isInitialFormValueChanged$: Observable; + + constructor( + @Inject(MAT_DIALOG_DATA) + public config: CommonModalConfig, + private formBuilder: FormBuilder, + private segmentService: SegmentsService, + private experimentService: ExperimentService, + public dialogRef: MatDialogRef + ) {} + + ngOnInit(): void { + this.experimentService.fetchContextMetaData(); + this.createSegmentForm(); + this.listenForSegmentGetUpdated(); + this.listenForIsInitialFormValueChanged(); + this.listenForPrimaryButtonDisabled(); + this.listenOnContext(); + + if (this.isDisabled()) { + this.disableRestrictedFields(); + } + } + + isDisabled() { + return ( + this.config.params.action === UPSERT_SEGMENT_ACTION.EDIT && + this.config.params.sourceSegment?.status === SEGMENT_STATUS.USED + ); + } + + disableRestrictedFields(): void { + this.segmentForm.get('name')?.disable(); + this.segmentForm.get('appContext')?.disable(); + } + + createSegmentForm(): void { + const { sourceSegment, action } = this.config.params; + const initialValues = this.deriveInitialFormValues(sourceSegment, action); + this.initialContext = initialValues.appContext; + + this.segmentForm = this.formBuilder.group({ + name: [initialValues.name, Validators.required], + description: [initialValues.description], + appContext: [initialValues.appContext, Validators.required], + tags: [initialValues.tags], + }); + + this.initialFormValues$.next(this.segmentForm.value); + } + + deriveInitialFormValues(sourceSegment: any, action: string): SegmentFormData { + const name = action === UPSERT_SEGMENT_ACTION.EDIT ? sourceSegment?.name : ''; + const description = sourceSegment?.description || ''; + const appContext = sourceSegment?.context || ''; + const tags = sourceSegment?.tags || []; + return { name, description, appContext, tags }; + } + + listenForPrimaryButtonDisabled() { + this.isPrimaryButtonDisabled$ = this.isLoadingUpsertSegment$.pipe( + combineLatestWith(this.isInitialFormValueChanged$), + map(([isLoading, isInitialFormValueChanged]) => isLoading || !isInitialFormValueChanged) + ); + this.subscriptions.add(this.isPrimaryButtonDisabled$.subscribe()); + } + + // Close the modal once the feature flag list length changes, as that indicates actual success + listenForSegmentGetUpdated(): void { + this.subscriptions.add( + this.isSelectedSegmentUpdated$.subscribe(() => { + this.closeModal(); + }) + ); + } + + listenOnContext(): void { + this.subscriptions.add( + this.segmentForm.get('appContext')?.valueChanges.subscribe((context) => { + this.isContextChanged = context !== this.initialContext; + }) + ); + } + + listenForIsInitialFormValueChanged() { + this.isInitialFormValueChanged$ = this.segmentForm.valueChanges.pipe( + startWith(this.segmentForm.value), + map(() => !isEqual(this.segmentForm.value, this.initialFormValues$.value)) + ); + this.subscriptions.add(this.isInitialFormValueChanged$.subscribe()); + } + + onPrimaryActionBtnClicked() { + if (this.segmentForm.valid) { + this.sendRequest(this.config.params.action, this.config.params.sourceSegment); + } else { + CommonFormHelpersService.triggerTouchedToDisplayErrors(this.segmentForm); + } + } + + sendRequest(action: UPSERT_SEGMENT_ACTION, sourceSegment?: Segment): void { + const formData: SegmentFormData = this.segmentForm.value; + if (action === UPSERT_SEGMENT_ACTION.ADD || action === UPSERT_SEGMENT_ACTION.DUPLICATE) { + this.createAddRequest(formData); + } else if (action === UPSERT_SEGMENT_ACTION.EDIT && sourceSegment) { + this.createEditRequest(formData, sourceSegment); + } else { + console.error('UpsertSegmentModalComponent: sendRequest: Invalid action or missing sourceSegment'); + } + } + + createAddRequest({ name, description, appContext, tags }: SegmentFormData): void { + const segmentRequest: AddSegmentRequest = { + name, + description, + context: appContext, + userIds: [], + groups: [], + subSegmentIds: [], + status: SEGMENT_STATUS.UNUSED, + type: SEGMENT_TYPE.PUBLIC, + tags, + }; + this.segmentService.addSegment(segmentRequest); + } + + createEditRequest({ name, description, appContext, tags }: SegmentFormData, sourceSegment: Segment): void { + const { id, status } = sourceSegment; + // Not allow editing segment name and context if segment is in used status: + if (sourceSegment.status === SEGMENT_STATUS.USED) { + name = sourceSegment.name; + appContext = sourceSegment.context; + } + const segmentRequest: UpdateSegmentRequest = { + id, + name, + description, + context: appContext, + userIds: [], + groups: [], + subSegmentIds: [], + status, + type: SEGMENT_TYPE.PUBLIC, + tags, + }; + this.segmentService.modifySegment(segmentRequest); + } + + get UPSERT_SEGMENT_ACTION() { + return UPSERT_SEGMENT_ACTION; + } + + closeModal() { + this.dialogRef.close(); + } + + ngOnDestroy() { + this.subscriptions.unsubscribe(); + } +} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts index e822f9773f..aea72f9716 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts @@ -72,10 +72,10 @@ export class SegmentOverviewDetailsSectionCardComponent implements OnInit, OnDes onMenuButtonItemClick(action: SEGMENT_DETAILS_PAGE_ACTIONS, segment: Segment) { switch (action) { case SEGMENT_DETAILS_PAGE_ACTIONS.EDIT: - // this.dialogService.openEditSegmentModal(segment); + this.dialogService.openEditSegmentModal(segment); break; case SEGMENT_DETAILS_PAGE_ACTIONS.DUPLICATE: - // this.dialogService.openDuplicateSegmentModal(segment); + this.dialogService.openDuplicateSegmentModal(segment); break; case SEGMENT_DETAILS_PAGE_ACTIONS.DELETE: // this.dialogService.openDeleteSegmentModal(segment); diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card.component.ts index 05ab7029b6..94c21eb023 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card.component.ts @@ -149,7 +149,7 @@ export class SegmentRootSectionCardComponent { } onAddSegmentButtonClick() { - // this.dialogService.openNewSegmentModal(); + this.dialogService.openAddSegmentModal(); } onMenuButtonItemClick(menuButtonItemName: string) { 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 ef2fbfb47c..4148ba0749 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 @@ -6,8 +6,11 @@ import { ImportFeatureFlagModalComponent } from '../../features/dashboard/featur import { UpsertFeatureFlagModalComponent } from '../../features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component'; import { UpsertPrivateSegmentListModalComponent } from '../../features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component'; import { + Segment, UPSERT_PRIVATE_SEGMENT_LIST_ACTION, + UPSERT_SEGMENT_ACTION, UpsertPrivateSegmentListParams, + UpsertSegmentParams, } from '../../core/segments/store/segments.model'; import { FEATURE_FLAG_DETAILS_PAGE_ACTIONS, @@ -24,6 +27,7 @@ import { SimpleConfirmationModalParams, } from '../../shared-standalone-component-lib/components/common-modal/common-modal.types'; import { FEATURE_FLAG_LIST_FILTER_MODE } from 'upgrade_types'; +import { UpsertSegmentModalComponent } from '../../features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component'; @Injectable({ providedIn: 'root', @@ -39,7 +43,7 @@ export class DialogService { }); } - // Common modal flags ---------------------------------------- // + // feature flag modal ---------------------------------------- // openAddFeatureFlagModal() { const commonModalConfig: CommonModalConfig = { title: 'Add Feature Flag', @@ -352,6 +356,65 @@ export class DialogService { return this.openSimpleCommonConfirmationModal(commonModalConfig, ModalSize.MEDIUM); } + // segment modal ---------------------------------------- // + openUpsertSegmentModal(commonModalConfig: CommonModalConfig) { + const config: MatDialogConfig = { + data: commonModalConfig, + width: ModalSize.STANDARD, + autoFocus: 'input', + disableClose: true, + }; + return this.dialog.open(UpsertSegmentModalComponent, config); + } + + openAddSegmentModal() { + const commonModalConfig: CommonModalConfig = { + title: 'Add Segment', + tagsLabel: 'segments.upsert-segment-modal.tags-label.text', + tagsPlaceholder: 'segments.upsert-segment-modal.tags-placeholder.text', + primaryActionBtnLabel: 'Create', + primaryActionBtnColor: 'primary', + cancelBtnLabel: 'Cancel', + params: { + sourceSegment: null, + action: UPSERT_SEGMENT_ACTION.ADD, + }, + }; + return this.openUpsertSegmentModal(commonModalConfig); + } + + openEditSegmentModal(sourceSegment: Segment) { + const commonModalConfig: CommonModalConfig = { + title: 'Edit Segment', + tagsLabel: 'segments.upsert-segment-modal.tags-label.text', + tagsPlaceholder: 'segments.upsert-segment-modal.tags-placeholder.text', + primaryActionBtnLabel: 'Save', + primaryActionBtnColor: 'primary', + cancelBtnLabel: 'Cancel', + params: { + sourceSegment: { ...sourceSegment }, + action: UPSERT_SEGMENT_ACTION.EDIT, + }, + }; + return this.openUpsertSegmentModal(commonModalConfig); + } + + openDuplicateSegmentModal(sourceSegment: Segment) { + const commonModalConfig: CommonModalConfig = { + title: 'Duplicate Segment', + tagsLabel: 'segments.upsert-segment-modal.tags-label.text', + tagsPlaceholder: 'segments.upsert-segment-modal.tags-placeholder.text', + primaryActionBtnLabel: 'Add', + primaryActionBtnColor: 'primary', + cancelBtnLabel: 'Cancel', + params: { + sourceSegment: { ...sourceSegment }, + action: UPSERT_SEGMENT_ACTION.DUPLICATE, + }, + }; + return this.openUpsertSegmentModal(commonModalConfig); + } + openImportFeatureFlagModal() { return this.openImportModal('Import Feature Flag', null, null); } diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index 64ec9dee00..3e05980386 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -486,6 +486,11 @@ "segments.segment-experiment-list-type.text": "Type", "segments.segment-experiment-list-usedList.text": "Used List", "segments.segment-experiment-list-zeroState.text": "No Data", + "segments.upsert-segment-modal.name-hint.text": "The name for this segment.", + "segments.upsert-segment-modal.app-context-hint.text": "The App Context indicates the segment belongs to which context.", + "segments.upsert-segment-modal.app-context-warning.text": "Updating the App Context will clear the existing include and exclude lists.", + "segments.upsert-segment-modal.tags-label.text": "Tags (optional)", + "segments.upsert-segment-modal.tags-placeholder.text": "Tags separated by commas", "stratifications.global-members-factor.text": "FACTOR", "stratifications.global-members-summary.text": "SUMMARY", "stratifications.global-members-status.text": "STATUS", diff --git a/types/src/Experiment/enums.ts b/types/src/Experiment/enums.ts index 639a8f6057..3bcf92ecae 100644 --- a/types/src/Experiment/enums.ts +++ b/types/src/Experiment/enums.ts @@ -213,6 +213,7 @@ export enum SEGMENT_SEARCH_KEY { ALL = 'all', NAME = 'name', TAG = 'tag', + STATUS = 'status', CONTEXT = 'context', } From 792a80760c555bb685bd7b53f53265fc3fa31d81 Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Tue, 1 Apr 2025 17:51:49 -0400 Subject: [PATCH 02/10] modify actions strategy a bit to get this working --- .../src/app/core/segments/segments.service.ts | 10 +++++++++ .../core/segments/store/segments.actions.ts | 4 ++-- .../core/segments/store/segments.effects.ts | 21 +++++++++--------- .../core/segments/store/segments.reducer.ts | 2 ++ .../core/segments/store/segments.selectors.ts | 22 ++++--------------- ...overview-details-section-card.component.ts | 2 +- .../common-modal/common-modal-config.ts | 1 + 7 files changed, 31 insertions(+), 31 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index a57243fb35..ac3775002e 100644 --- a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -51,6 +51,7 @@ export class SegmentsService { ) {} isLoadingSegments$ = this.store$.pipe(select(selectIsLoadingSegments)); + isLoadingUpsertSegment$ = this.store$.pipe(select(selectIsLoadingUpsertSegment)); setIsLoadingImportSegment$ = this.store$.pipe(select(selectIsLoadingSegments)); selectAllSegments$ = this.store$.pipe(select(selectAllSegments)); selectedSegment$ = this.store$.pipe(select(selectSelectedSegment)); @@ -103,6 +104,15 @@ export class SegmentsService { ) ); + isSelectedSegmentUpdated$ = this.store$.pipe( + select(selectSelectedSegment), + pairwise(), + filter(([prev, curr]) => { + return prev && curr && !isEqual(prev, curr); + }), + map(([, curr]) => curr) + ); + selectPrivateSegmentListTypeOptions$ = this.store$.pipe( select(selectContextMetaData), withLatestFrom(this.store$.pipe(select(selectSelectedFeatureFlag))), diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts index 495116c9a8..8a7a9db806 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.actions.ts @@ -63,7 +63,7 @@ export const actionAddSegment = createAction( props<{ addSegmentRequest: AddSegmentRequest }>() ); -export const actionAddSegmentSuccess = createAction('[Segments] Add Segment Success', props<{ response: Segment }>()); +export const actionAddSegmentSuccess = createAction('[Segments] Add Segment Success', props<{ segment: Segment }>()); export const actionAddSegmentFailure = createAction('[Segments] Add Segment Failure'); @@ -74,7 +74,7 @@ export const actionUpdateSegment = createAction( export const actionUpdateSegmentSuccess = createAction( '[Segments] Update Segment Success', - props<{ response: Segment }>() + props<{ segment: Segment }>() ); export const actionUpdateSegmentFailure = createAction('[Segments] Update Segment Failure'); diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts index d1dfeff76e..28a229b259 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts @@ -2,7 +2,7 @@ import { Injectable } from '@angular/core'; import { Router } from '@angular/router'; import { Actions, createEffect, ofType } from '@ngrx/effects'; import { select, Store } from '@ngrx/store'; -import { catchError, filter, first, map, switchMap, tap, withLatestFrom } from 'rxjs/operators'; +import { catchError, concatMap, filter, first, map, switchMap, tap, withLatestFrom } from 'rxjs/operators'; import { AppState } from '../../core.module'; import { SegmentsDataService } from '../segments.data.service'; import * as SegmentsActions from './segments.actions'; @@ -18,7 +18,7 @@ import { selectTotalSegments, } from './segments.selectors'; import JSZip from 'jszip'; -import { SERVER_ERROR } from 'upgrade_types'; +import { of } from 'rxjs'; @Injectable() export class SegmentsEffects { @@ -159,15 +159,14 @@ export class SegmentsEffects { ) ); - // actionAddSegment dispatch POST segment addSegment$ = createEffect(() => this.actions$.pipe( ofType(SegmentsActions.actionAddSegment), switchMap((action) => { return this.segmentsDataService.addSegment(action.addSegmentRequest).pipe( - map((response: Segment) => SegmentsActions.actionAddSegmentSuccess({ response })), - tap(({ response }) => { - this.router.navigate(['/segments', 'detail', response.id]); + map((response: Segment) => SegmentsActions.actionAddSegmentSuccess({ segment: response })), + tap(({ segment }) => { + this.router.navigate(['/segments', 'detail', segment.id]); }), catchError(() => { return [SegmentsActions.actionAddSegmentFailure()]; @@ -182,12 +181,14 @@ export class SegmentsEffects { ofType(SegmentsActions.actionUpdateSegment), switchMap((action) => { return this.segmentsDataService.modifySegment(action.updateSegmentRequest).pipe( - map((response: Segment) => SegmentsActions.actionUpdateSegmentSuccess({ response })), - tap(({ response }) => { - this.router.navigate(['/segments', 'detail', response.id]); + concatMap((response: Segment) => { + return [ + SegmentsActions.actionUpdateSegmentSuccess({ segment: response }), + SegmentsActions.actionGetSegmentById({ segmentId: response.id }), + ]; }), catchError(() => { - return [SegmentsActions.actionUpdateSegmentFailure()]; + return of(SegmentsActions.actionUpdateSegmentFailure()); }) ); }) diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts index 82111cee1c..87b68a3b3d 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts @@ -91,6 +91,8 @@ const reducer = createReducer( SegmentsActions.actionFetchSegmentsFailure, SegmentsActions.actionUpsertSegmentFailure, SegmentsActions.actionGetSegmentByIdFailure, + SegmentsActions.actionUpdateSegmentSuccess, + SegmentsActions.actionAddSegmentSuccess, (state) => ({ ...state, isLoadingSegments: false }) ), on(SegmentsActions.actionUpsertSegmentSuccess, (state, { segment }) => diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts index 30a7494dfd..b0cc4a4484 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts @@ -47,29 +47,15 @@ export const selectIsLoadingUpsertSegment = createSelector( (state) => state.isLoadingUpsertSegment ); -// export const selectSelectedSegment = createSelector( -// selectRouterState, -// selectSegmentsState, -// (routerState, segmentState) => { -// // be very defensive here to make sure routerState is correct -// const segmentId = routerState?.state?.params?.segmentId; -// if (segmentId) { -// return segmentState.entities[segmentId]; -// } -// return undefined; -// } -// ); - export const selectSelectedSegment = createSelector( selectRouterState, selectSegmentsState, (routerState, segmentState) => { - if (routerState?.state && segmentState?.entities) { - const { - state: { params }, - } = routerState; - return segmentState.entities[params.segmentId] ? segmentState.entities[params.segmentId] : undefined; + const segmentId = routerState?.state?.params?.segmentId; + if (segmentId) { + return segmentState.entities[segmentId]; } + return undefined; } ); diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts index 33ba4fe330..3738d32170 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts @@ -65,7 +65,7 @@ export class SegmentOverviewDetailsSectionCardComponent implements OnInit, OnDes { label: 'segments.details.menu-button.duplicate-segment.text', action: SEGMENT_DETAILS_PAGE_ACTIONS.DUPLICATE, - disabled: !permissions?.segments?.create, + disabled: true || !permissions?.segments?.create, // TODO: This modal is implemented, but not sure we have refined how it should work for segments }, { label: 'segments.details.menu-button.export-segment-design.text', diff --git a/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-modal/common-modal-config.ts b/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-modal/common-modal-config.ts index d48cb898ae..48f58c8ba8 100644 --- a/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-modal/common-modal-config.ts +++ b/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-modal/common-modal-config.ts @@ -4,4 +4,5 @@ import { environment } from '../../../../environments/environment'; export const ENDPOINTS_TO_INTERCEPT_FOR_MODAL_CLOSE = [ environment.api.addFlagInclusionList, environment.api.addFlagExclusionList, + environment.api.segments, ]; From 805cc172e63f742ce4f55e8a0140f92413724935 Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Tue, 1 Apr 2025 20:56:08 -0400 Subject: [PATCH 03/10] prevent GET segments from triggering close, it breaks the add-list modals --- .../src/app/core/http-interceptors/close-modal.interceptor.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts b/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts index acfb0e8ed8..18cd0df926 100644 --- a/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts +++ b/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts @@ -12,7 +12,9 @@ export class CloseModalInterceptor implements HttpInterceptor { intercept(req: HttpRequest, next: HttpHandler): Observable> { return next.handle(req).pipe( tap((event) => { - if (event instanceof HttpResponse) { + const isPostOrPut = req.method === 'POST' || req.method === 'PUT'; + + if (event instanceof HttpResponse && isPostOrPut) { const shouldCloseModal = ENDPOINTS_TO_INTERCEPT_FOR_MODAL_CLOSE.some((endpoint) => event.url.includes(endpoint) ); From d92d3dfe26f754b1bb339972449dc4a69ca56dcd Mon Sep 17 00:00:00 2001 From: pratik Date: Wed, 2 Apr 2025 11:47:23 +0530 Subject: [PATCH 04/10] updated at uniformity for new segments UI --- .../segment-overview-details-section-card.component.html | 1 + .../segment-root-section-card-table.component.html | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.html b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.html index e6d2552478..5c5e75c550 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.html @@ -5,6 +5,7 @@ [title]="segment.name" [createdAt]="segment.createdAt" [updatedAt]="segment.updatedAt" + [versionNumber]="segment.versionNumber" [chipClass]="segment.status.toLowerCase()" > diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card-table/segment-root-section-card-table.component.html b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card-table/segment-root-section-card-table.component.html index d0abae44e3..ff75c094c2 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card-table/segment-root-section-card-table.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card-table/segment-root-section-card-table.component.html @@ -53,8 +53,13 @@ {{ SEGMENT_TRANSLATION_KEYS.UPDATED_AT | translate }} - {{ segment.updatedAt | date : 'MMM d, y h:mm a' }} - + + {{ '-' }} + + + {{ segment.updatedAt | date : 'MMM d, y h:mm a' }} + + From e7d03285ab5a956489d4950e8da13c52e5bc9c0e Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Thu, 3 Apr 2025 20:35:58 -0400 Subject: [PATCH 05/10] add duplicate name segment validation in backend and mat-error hookup in modal form --- .../src/api/services/SegmentService.ts | 35 +++++++++++- .../src/app/core/segments/segments.service.ts | 13 +++-- .../segments/store/segments.effects.spec.ts | 3 +- .../core/segments/store/segments.effects.ts | 8 ++- .../core/segments/store/segments.selectors.ts | 4 +- .../upsert-segment-modal.component.html | 6 +- .../upsert-segment-modal.component.ts | 55 +++++++++++++++++-- types/src/Experiment/enums.ts | 1 + types/src/Experiment/interfaces.ts | 9 +++ types/src/index.ts | 1 + 10 files changed, 121 insertions(+), 14 deletions(-) diff --git a/backend/packages/Upgrade/src/api/services/SegmentService.ts b/backend/packages/Upgrade/src/api/services/SegmentService.ts index 7a24870d20..65ea30adb2 100644 --- a/backend/packages/Upgrade/src/api/services/SegmentService.ts +++ b/backend/packages/Upgrade/src/api/services/SegmentService.ts @@ -16,6 +16,7 @@ import { IMPORT_COMPATIBILITY_TYPE, ValidatedImportResponse, SEGMENT_SEARCH_KEY, + DuplicateSegmentNameError, } from 'upgrade_types'; import { In } from 'typeorm'; import { EntityManager, DataSource } from 'typeorm'; @@ -377,7 +378,9 @@ export class SegmentService { return queryBuilder; } - public upsertSegment(segment: SegmentInputValidator, logger: UpgradeLogger): Promise { + public async upsertSegment(segment: SegmentInputValidator, logger: UpgradeLogger): Promise { + await this.checkIsDuplicateSegmentName(segment.name, segment.context, logger); + logger.info({ message: `Upsert segment => ${JSON.stringify(segment, undefined, 2)}` }); return this.addSegmentDataInDB(segment, logger); } @@ -959,4 +962,34 @@ export class SegmentService { } } } + + async checkIsDuplicateSegmentName(name: string, context: string, logger: UpgradeLogger): Promise { + logger.info({ message: `Check for duplicate segment name ${name} in context ${context}` }); + const sameNameSegment = await this.segmentRepository.find({ where: { name, context } }); + + if (sameNameSegment.length) { + logger.error({ + message: `Segment name ${name} already exists in context ${context}`, + }); + + // const error = new ErrorWithType(); + // error.type = SERVER_ERROR.SEGMENT_DUPLICATE_NAME; + // error.details = `Segment name ${name} already exists in context ${context}`; + // (error as any).duplicateName = name; + // (error as any).context = context; + // (error as any).httpCode = 400; + + const error: DuplicateSegmentNameError = { + type: SERVER_ERROR.SEGMENT_DUPLICATE_NAME, + message: `Segment name ${name} already exists in context ${context}`, + duplicateName: name, + context, + httpCode: 400, + }; + + throw error; + } else { + return false; + } + } } diff --git a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index 49bd96b028..d35fb7d461 100644 --- a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -19,12 +19,12 @@ import { selectSortAs, selectSegmentLists, selectAppContexts, - selectIsLoadingUpsertSegment, selectSegmentUsageData, selectIsLoadingGlobalSegments, selectGlobalTableState, selectGlobalSortKey, selectGlobalSortAs, + isLoadingUpsertSegment, } from './store/segments.selectors'; import { AddSegmentRequest, @@ -36,9 +36,9 @@ import { UpsertSegmentType, } from './store/segments.model'; import { filter, map, pairwise, tap, withLatestFrom } from 'rxjs/operators'; -import { Observable, combineLatest } from 'rxjs'; +import { BehaviorSubject, Observable, combineLatest } from 'rxjs'; import { SegmentsDataService } from './segments.data.service'; -import { SEGMENT_SEARCH_KEY, SORT_AS_DIRECTION, SEGMENT_SORT_KEY } from 'upgrade_types'; +import { SEGMENT_SEARCH_KEY, SORT_AS_DIRECTION, SEGMENT_SORT_KEY, DuplicateSegmentNameError } from 'upgrade_types'; import { LocalStorageService } from '../local-storage/local-storage.service'; import { selectShouldUseLegacyUI } from './store/segments.selectors'; import { selectContextMetaData } from '../experiments/store/experiments.selectors'; @@ -56,7 +56,7 @@ export class SegmentsService { ) {} isLoadingSegments$ = this.store$.pipe(select(selectIsLoadingSegments)); - isLoadingUpsertSegment$ = this.store$.pipe(select(selectIsLoadingUpsertSegment)); + isLoadingUpsertSegment$ = this.store$.pipe(select(isLoadingUpsertSegment)); setIsLoadingImportSegment$ = this.store$.pipe(select(selectIsLoadingSegments)); isLoadingGlobalSegments$ = this.store$.pipe(select(selectIsLoadingGlobalSegments)); selectAllSegments$ = this.store$.pipe(select(selectAllSegments)); @@ -82,6 +82,7 @@ export class SegmentsService { allFeatureFlagSegmentsExclusion$ = this.store$.pipe(select(selectFeatureFlagSegmentsExclusion)); allFeatureFlagSegmentsInclusion$ = this.store$.pipe(select(selectFeatureFlagSegmentsInclusion)); segmentUsageData$ = this.store$.pipe(select(selectSegmentUsageData)); + duplicateSegmentNameError$ = new BehaviorSubject(null); selectSearchSegmentParams(): Observable> { return combineLatest([this.selectSearchKey$, this.selectSearchString$]).pipe( @@ -249,4 +250,8 @@ export class SegmentsService { exportSegmentCSV(segmentIds: string[]): Observable { return this.segmentsDataService.exportSegmentCSV(segmentIds); } + + setDuplicateSegmentNameError(error: DuplicateSegmentNameError) { + this.duplicateSegmentNameError$.next(error); + } } diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts index 83945a6b3f..8b9b5299e4 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts @@ -11,6 +11,7 @@ describe('SegmentsEffects', () => { let store$: any; let actions$: ActionsSubject; let segmentsDataService: any; + let segmentService: any; let router: any; let service: SegmentsEffects; const mockSegment: Segment = { @@ -52,7 +53,7 @@ describe('SegmentsEffects', () => { navigate: jest.fn(), }; - service = new SegmentsEffects(store$, actions$, segmentsDataService, router); + service = new SegmentsEffects(store$, actions$, segmentsDataService, segmentService, router); }); describe('fetchAllSegments$', () => { diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts index e7534c8346..5cc1676045 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts @@ -20,6 +20,8 @@ import { } from './segments.selectors'; import JSZip from 'jszip'; import { of } from 'rxjs'; +import { SERVER_ERROR } from 'upgrade_types'; +import { SegmentsService } from '../segments.service'; @Injectable() export class SegmentsEffects { @@ -27,6 +29,7 @@ export class SegmentsEffects { private store$: Store, private actions$: Actions, private segmentsDataService: SegmentsDataService, + private segmentsService: SegmentsService, private router: Router ) {} @@ -186,7 +189,10 @@ export class SegmentsEffects { tap(({ segment }) => { this.router.navigate(['/segments', 'detail', segment.id]); }), - catchError(() => { + catchError((error) => { + if (error?.error?.type === SERVER_ERROR.SEGMENT_DUPLICATE_NAME) { + this.segmentsService.setDuplicateSegmentNameError(error.error); + } return [SegmentsActions.actionAddSegmentFailure()]; }) ); diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts index 83fed7e26b..731455cc1c 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts @@ -1,4 +1,4 @@ -import { createSelector, createFeatureSelector } from '@ngrx/store'; +import { createSelector, createFeatureSelector, select } from '@ngrx/store'; import { LIST_OPTION_TYPE, SegmentState, @@ -27,6 +27,8 @@ export const selectGlobalSegments = createSelector(selectGlobalSegmentsState, se export const selectIsLoadingSegments = createSelector(selectSegmentsState, (state) => state.isLoadingSegments); +export const isLoadingUpsertSegment = createSelector(selectSegmentsState, (state) => state.isLoadingUpsertSegment); + export const selectIsLoadingGlobalSegments = createSelector( selectGlobalSegmentsState, (state) => state.isLoadingGlobalSegments diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html index ab2d1a2c69..9a25afcc79 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html @@ -10,7 +10,11 @@ Name - + + A segment with name "{{ error.duplicateName }}" already exists on context "{{ error.context }}". + + + {{ 'segments.upsert-segment-modal.name-hint.text' | translate }} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts index 17852cd5f9..d3f75ada0c 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts @@ -23,11 +23,11 @@ import { UpdateSegmentRequest, } from '../../../../../core/segments/store/segments.model'; import { SegmentsService } from '../../../../../core/segments/segments.service'; -import { BehaviorSubject, Observable, map, startWith, Subscription, combineLatestWith } from 'rxjs'; +import { BehaviorSubject, Observable, map, startWith, Subscription, combineLatestWith, withLatestFrom } from 'rxjs'; import { TranslateModule } from '@ngx-translate/core'; import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; import { ExperimentService } from '../../../../../core/experiments/experiments.service'; -import { SEGMENT_TYPE } from '../../../../../../../../../../types/src'; +import { DuplicateSegmentNameError, SEGMENT_TYPE } from 'upgrade_types'; import isEqual from 'lodash.isequal'; @Component({ @@ -60,6 +60,7 @@ export class UpsertSegmentModalComponent { isContextChanged = false; initialContext = ''; isInitialFormValueChanged$: Observable; + duplicateSegmentNameError$ = this.segmentService.duplicateSegmentNameError$; constructor( @Inject(MAT_DIALOG_DATA) @@ -91,8 +92,8 @@ export class UpsertSegmentModalComponent { } disableRestrictedFields(): void { - this.segmentForm.get('name')?.disable(); - this.segmentForm.get('appContext')?.disable(); + this.nameControl?.disable(); + this.appContextControl?.disable(); } createSegmentForm(): void { @@ -108,6 +109,34 @@ export class UpsertSegmentModalComponent { }); this.initialFormValues$.next(this.segmentForm.value); + + this.listenForDuplicateNameError(); + this.listenForNameChanges(); + } + + setDuplicateSegmentNameErrorOnNameControl(error: DuplicateSegmentNameError) { + if (error) { + this.nameControl.setErrors({ duplicateSegmentName: error }); + } else { + this.nameControl.setErrors(null); + } + } + + handleCheckingDuplicateName(error: DuplicateSegmentNameError, nameInput: string) { + const contextInput = this.appContextControl.value; + if (nameInput === error?.duplicateName && contextInput === error?.context) { + this.nameControl.setErrors({ duplicateSegmentName: error }); + } else { + this.nameControl.setErrors(null); + } + } + + get nameControl() { + return this.segmentForm.get('name'); + } + + get appContextControl() { + return this.segmentForm.get('appContext'); } deriveInitialFormValues(sourceSegment: any, action: string): SegmentFormData { @@ -135,10 +164,25 @@ export class UpsertSegmentModalComponent { ); } + listenForNameChanges(): void { + this.subscriptions.add( + this.nameControl.valueChanges + .pipe(withLatestFrom(this.duplicateSegmentNameError$)) + .subscribe(([name, error]) => this.handleCheckingDuplicateName(error, name)) + ); + } + + listenForDuplicateNameError(): void { + this.subscriptions.add( + this.duplicateSegmentNameError$.subscribe((error) => this.setDuplicateSegmentNameErrorOnNameControl(error)) + ); + } + listenOnContext(): void { this.subscriptions.add( - this.segmentForm.get('appContext')?.valueChanges.subscribe((context) => { + this.appContextControl?.valueChanges.subscribe((context) => { this.isContextChanged = context !== this.initialContext; + this.handleCheckingDuplicateName(this.duplicateSegmentNameError$.value, this.nameControl.value); }) ); } @@ -153,6 +197,7 @@ export class UpsertSegmentModalComponent { onPrimaryActionBtnClicked() { if (this.segmentForm.valid) { + this.segmentService.setDuplicateSegmentNameError(null); this.sendRequest(this.config.params.action, this.config.params.sourceSegment); } else { CommonFormHelpersService.triggerTouchedToDisplayErrors(this.segmentForm); diff --git a/types/src/Experiment/enums.ts b/types/src/Experiment/enums.ts index 3bcf92ecae..3f7d2faa4e 100644 --- a/types/src/Experiment/enums.ts +++ b/types/src/Experiment/enums.ts @@ -74,6 +74,7 @@ export enum SERVER_ERROR { UNSUPPORTED_CALIPER = 'Caliper profile or event not supported', DUPLICATE_KEY = 'Feature Flag with same key already exists for this app-context', MISSING_HEADER_USER_ID = 'Missing `User-Id` header', + SEGMENT_DUPLICATE_NAME = 'Segment with same name already exists for this app-context, please edit name to be unique.', } export enum MARKED_DECISION_POINT_STATUS { diff --git a/types/src/Experiment/interfaces.ts b/types/src/Experiment/interfaces.ts index 9e292e39b6..f3f9774311 100644 --- a/types/src/Experiment/interfaces.ts +++ b/types/src/Experiment/interfaces.ts @@ -13,6 +13,7 @@ import { FILTER_MODE, FEATURE_FLAG_LIST_FILTER_MODE, IMPORT_COMPATIBILITY_TYPE, + SERVER_ERROR, } from './enums'; export interface IEnrollmentCompleteCondition { userCount: number; @@ -294,3 +295,11 @@ export interface ValidatedImportResponse { compatibilityType: IMPORT_COMPATIBILITY_TYPE; error?: string; } + +export interface DuplicateSegmentNameError { + type: SERVER_ERROR.SEGMENT_DUPLICATE_NAME; + message: string; + duplicateName: string; + context: string; + httpCode: 400; +} diff --git a/types/src/index.ts b/types/src/index.ts index 6f380d9f31..ed3b3e82d9 100644 --- a/types/src/index.ts +++ b/types/src/index.ts @@ -78,6 +78,7 @@ export { FeatureFlagStateChangedData, FeatureFlagDeletedData, ValidatedImportResponse, + DuplicateSegmentNameError, } from './Experiment/interfaces'; export { MoocletPolicyParametersDTO, From 48ead210e5e1a05ca74017ddd090cdffa98b4c62 Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Fri, 4 Apr 2025 14:39:19 -0400 Subject: [PATCH 06/10] fix duplicate modal issues, fetch-by-id timing --- .../src/app/core/segments/segments.service.ts | 15 +------ .../core/segments/store/segments.effects.ts | 14 ++++-- .../core/segments/store/segments.selectors.ts | 20 +++++++++ .../upsert-segment-modal.component.html | 14 ++++-- .../upsert-segment-modal.component.ts | 44 +++++++++++++------ .../segment-details-page-content.component.ts | 20 ++------- ...overview-details-section-card.component.ts | 2 +- .../segment-details-page.component.ts | 30 ++++++++----- .../shared/services/common-dialog.service.ts | 2 +- .../projects/upgrade/src/assets/i18n/en.json | 1 + 10 files changed, 101 insertions(+), 61 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index d35fb7d461..57bb62d19e 100644 --- a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -12,7 +12,6 @@ import { selectExperimentSegmentsExclusion, selectFeatureFlagSegmentsInclusion, selectFeatureFlagSegmentsExclusion, - selectSegmentById, selectSearchString, selectSearchKey, selectSortKey, @@ -25,6 +24,7 @@ import { selectGlobalSortKey, selectGlobalSortAs, isLoadingUpsertSegment, + selectSegmentIdAfterNavigation, } from './store/segments.selectors'; import { AddSegmentRequest, @@ -83,6 +83,7 @@ export class SegmentsService { allFeatureFlagSegmentsInclusion$ = this.store$.pipe(select(selectFeatureFlagSegmentsInclusion)); segmentUsageData$ = this.store$.pipe(select(selectSegmentUsageData)); duplicateSegmentNameError$ = new BehaviorSubject(null); + selectSegmentIdAfterNavigation$ = this.store$.pipe(select(selectSegmentIdAfterNavigation)); selectSearchSegmentParams(): Observable> { return combineLatest([this.selectSearchKey$, this.selectSearchString$]).pipe( @@ -91,18 +92,6 @@ export class SegmentsService { ); } - selectSegmentById(segmentId: string) { - return this.store$.pipe( - select(selectSegmentById, { segmentId }), - tap((segment) => { - if (!segment) { - this.fetchSegmentById(segmentId); - } - }), - map((segment) => ({ ...segment })) - ); - } - allSegments$ = this.store$.pipe( select(selectAllSegments), filter((allSegments) => !!allSegments), diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts index 5cc1676045..4f4f8194b0 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts @@ -186,9 +186,6 @@ export class SegmentsEffects { switchMap((action) => { return this.segmentsDataService.addSegment(action.addSegmentRequest).pipe( map((response: Segment) => SegmentsActions.actionAddSegmentSuccess({ segment: response })), - tap(({ segment }) => { - this.router.navigate(['/segments', 'detail', segment.id]); - }), catchError((error) => { if (error?.error?.type === SERVER_ERROR.SEGMENT_DUPLICATE_NAME) { this.segmentsService.setDuplicateSegmentNameError(error.error); @@ -200,6 +197,17 @@ export class SegmentsEffects { ) ); + navigateToSegmentDetail$ = createEffect( + () => + this.actions$.pipe( + ofType(SegmentsActions.actionAddSegmentSuccess), + tap(({ segment }) => { + this.router.navigate(['/segments', 'detail', segment.id]); + }) + ), + { dispatch: false } + ); + updateSegment$ = createEffect(() => this.actions$.pipe( ofType(SegmentsActions.actionUpdateSegment), diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts index 731455cc1c..42602f3f68 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts @@ -72,6 +72,26 @@ export const selectAllSegmentEntities = createSelector( }) ); +export const selectSegmentIdFromRoute = createSelector(selectRouterState, (routerState) => { + if (routerState?.state?.params?.segmentId) { + return routerState.state.params.segmentId; + } + return null; +}); + +// Create a selector that only emits after navigation is complete +export const selectSegmentIdAfterNavigation = createSelector( + selectSegmentIdFromRoute, + selectRouterState, + (segmentId, routerState) => { + // Only return the segmentId if we have a completed navigation + if (segmentId && routerState?.state?.url) { + return segmentId; + } + return null; + } +); + export const selectSelectedSegment = createSelector( selectRouterState, selectAllSegmentEntities, diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html index 9a25afcc79..c0ec136643 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.html @@ -10,11 +10,19 @@ Name - - A segment with name "{{ error.duplicateName }}" already exists on context "{{ error.context }}". + + Name is required + + + + {{ + 'segments.errors.duplicate-name.text' + | translate : { name: errorData.duplicateName, context: errorData.context } + }} - + + {{ 'segments.upsert-segment-modal.name-hint.text' | translate }} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts index d3f75ada0c..e343b48c8c 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts @@ -114,21 +114,25 @@ export class UpsertSegmentModalComponent { this.listenForNameChanges(); } - setDuplicateSegmentNameErrorOnNameControl(error: DuplicateSegmentNameError) { - if (error) { - this.nameControl.setErrors({ duplicateSegmentName: error }); - } else { - this.nameControl.setErrors(null); + private updateNameControlErrors(shouldSetDuplicateError: boolean, error?: DuplicateSegmentNameError) { + const currentErrors = this.nameControl.errors || {}; + const { duplicateSegmentName, ...otherErrors } = currentErrors; + + if (shouldSetDuplicateError && error) { + this.nameControl.setErrors({ ...otherErrors, duplicateSegmentName: error }); + } else if (currentErrors.duplicateSegmentName) { + this.nameControl.setErrors(Object.keys(otherErrors).length ? otherErrors : null); } } + setDuplicateSegmentNameErrorOnNameControl(error: DuplicateSegmentNameError) { + this.updateNameControlErrors(!!error, error); + } + handleCheckingDuplicateName(error: DuplicateSegmentNameError, nameInput: string) { const contextInput = this.appContextControl.value; - if (nameInput === error?.duplicateName && contextInput === error?.context) { - this.nameControl.setErrors({ duplicateSegmentName: error }); - } else { - this.nameControl.setErrors(null); - } + const isDuplicate = nameInput === error?.duplicateName && contextInput === error?.context; + this.updateNameControlErrors(isDuplicate, error); } get nameControl() { @@ -139,8 +143,16 @@ export class UpsertSegmentModalComponent { return this.segmentForm.get('appContext'); } - deriveInitialFormValues(sourceSegment: any, action: string): SegmentFormData { - const name = action === UPSERT_SEGMENT_ACTION.EDIT ? sourceSegment?.name : ''; + deriveInitialFormValues(sourceSegment: Segment, action: UPSERT_SEGMENT_ACTION): SegmentFormData { + let name = ''; + if (action === UPSERT_SEGMENT_ACTION.DUPLICATE) { + name = `${sourceSegment?.name} (COPY)`; + } + + if (action === UPSERT_SEGMENT_ACTION.EDIT) { + name = sourceSegment?.name; + } + const description = sourceSegment?.description || ''; const appContext = sourceSegment?.context || ''; const tags = sourceSegment?.tags || []; @@ -150,12 +162,16 @@ export class UpsertSegmentModalComponent { listenForPrimaryButtonDisabled() { this.isPrimaryButtonDisabled$ = this.isLoadingUpsertSegment$.pipe( combineLatestWith(this.isInitialFormValueChanged$), - map(([isLoading, isInitialFormValueChanged]) => isLoading || !isInitialFormValueChanged) + map( + ([isLoading, isInitialFormValueChanged]) => + isLoading || + this.segmentForm.invalid || + (!isInitialFormValueChanged && this.config.params.action !== UPSERT_SEGMENT_ACTION.DUPLICATE) + ) ); this.subscriptions.add(this.isPrimaryButtonDisabled$.subscribe()); } - // Close the modal once the feature flag list length changes, as that indicates actual success listenForSegmentGetUpdated(): void { this.subscriptions.add( this.isSelectedSegmentUpdated$.subscribe(() => { diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.ts index acdfe83fa5..956ad2ea5a 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-details-page-content.component.ts @@ -1,13 +1,12 @@ -import { ChangeDetectionStrategy, Component, OnDestroy, OnInit } from '@angular/core'; +import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core'; import { CommonSectionCardListComponent } from '../../../../../../shared-standalone-component-lib/components'; import { CommonModule } from '@angular/common'; import { SegmentOverviewDetailsSectionCardComponent } from './segment-overview-details-section-card/segment-overview-details-section-card.component'; import { SegmentListsSectionCardComponent } from './segment-lists-section-card/segment-lists-section-card.component'; import { SegmentUsedBySectionCardComponent } from './segment-used-by-section-card/segment-used-by-section-card.component'; import { SegmentsService } from '../../../../../../core/segments/segments.service'; -import { Observable, Subscription } from 'rxjs'; +import { Observable } from 'rxjs'; import { Segment } from '../../../../../../core/segments/store/segments.model'; -import { ActivatedRoute } from '@angular/router'; import { SharedModule } from '../../../../../../shared/shared.module'; @Component({ @@ -24,21 +23,14 @@ import { SharedModule } from '../../../../../../shared/shared.module'; styleUrl: './segment-details-page-content.component.scss', changeDetection: ChangeDetectionStrategy.OnPush, }) -export class SegmentDetailsPageContentComponent implements OnInit, OnDestroy { +export class SegmentDetailsPageContentComponent implements OnInit { isSectionCardExpanded = true; segment$: Observable; activeTabIndex = 0; // 0 for Lists, 1 for Used By - segmentIdSub: Subscription; - - constructor(private segmentsService: SegmentsService, private _Activatedroute: ActivatedRoute) {} + constructor(private segmentsService: SegmentsService) {} ngOnInit() { - this.segmentIdSub = this._Activatedroute.paramMap.subscribe((params) => { - const segmentIdFromParams = params.get('segmentId'); - this.segmentsService.fetchSegmentById(segmentIdFromParams); - }); - this.segment$ = this.segmentsService.selectedSegment$; } @@ -49,8 +41,4 @@ export class SegmentDetailsPageContentComponent implements OnInit, OnDestroy { onTabChange(tabIndex: number) { this.activeTabIndex = tabIndex; } - - ngOnDestroy() { - this.segmentIdSub.unsubscribe(); - } } diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts index 3738d32170..33ba4fe330 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page-content/segment-overview-details-section-card/segment-overview-details-section-card.component.ts @@ -65,7 +65,7 @@ export class SegmentOverviewDetailsSectionCardComponent implements OnInit, OnDes { label: 'segments.details.menu-button.duplicate-segment.text', action: SEGMENT_DETAILS_PAGE_ACTIONS.DUPLICATE, - disabled: true || !permissions?.segments?.create, // TODO: This modal is implemented, but not sure we have refined how it should work for segments + disabled: !permissions?.segments?.create, }, { label: 'segments.details.menu-button.export-segment-design.text', diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page.component.ts index 5b3310355a..9b51dc9eb0 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-details-page/segment-details-page.component.ts @@ -5,11 +5,10 @@ import { SegmentDetailsPageHeaderComponent } from './segment-details-page-header import { SegmentDetailsPageContentComponent } from './segment-details-page-content/segment-details-page-content.component'; import { SegmentsModule } from '../../../segments-legacy/segments.module'; import { SegmentsService } from '../../../../../core/segments/segments.service'; -import { ActivatedRoute } from '@angular/router'; +import { ActivatedRoute, NavigationEnd, Router } from '@angular/router'; import { Segment } from '../../../../../core/segments/store/segments.model'; -import { SEGMENT_TYPE } from 'upgrade_types'; -import { Observable, Subscription } from 'rxjs'; -import { filter, map } from 'rxjs/operators'; +import { combineLatest, Observable, Subscription } from 'rxjs'; +import { filter, map, startWith } from 'rxjs/operators'; import { CommonModule } from '@angular/common'; @Component({ @@ -29,14 +28,25 @@ export class SegmentDetailsPageComponent implements OnInit, OnDestroy { segmentIdSub: Subscription; shouldUseLegacyView$: Observable; - constructor(private segmentsService: SegmentsService, private route: ActivatedRoute) {} + constructor(private segmentsService: SegmentsService, private router: Router, private route: ActivatedRoute) {} ngOnInit() { - this.segmentIdSub = this.route.paramMap.subscribe((params) => { - const segmentId = params.get('segmentId'); - if (segmentId) { - this.segmentsService.fetchSegmentById(segmentId); - } + const segmentIdFromRoute$ = this.route.paramMap.pipe( + map((params) => params.get('segmentId')), + filter((segmentId) => !!segmentId) + ); + + const navigationComplete$ = this.router.events.pipe( + filter((event) => event instanceof NavigationEnd), + startWith(null) + ); + + // Combine both observables to ensure we only fetch after navigation completes + // This will ensure no weird router behavior when navigating back and forth + const segmentId$ = combineLatest([segmentIdFromRoute$, navigationComplete$]).pipe(map(([segmentId]) => segmentId)); + + this.segmentIdSub = segmentId$.subscribe((segmentId) => { + this.segmentsService.fetchSegmentById(segmentId); }); this.segment$ = this.segmentsService.selectedSegment$; 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 d3f0b0bf09..b8d8b3a788 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 @@ -419,7 +419,7 @@ export class DialogService { title: 'Duplicate Segment', tagsLabel: 'segments.upsert-segment-modal.tags-label.text', tagsPlaceholder: 'segments.upsert-segment-modal.tags-placeholder.text', - primaryActionBtnLabel: 'Add', + primaryActionBtnLabel: 'Create', primaryActionBtnColor: 'primary', cancelBtnLabel: 'Cancel', params: { diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index e0762112d3..cdaddde67e 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -527,6 +527,7 @@ "segments.upsert-segment-modal.app-context-warning.text": "Updating the App Context will clear the existing include and exclude lists.", "segments.upsert-segment-modal.tags-label.text": "Tags (optional)", "segments.upsert-segment-modal.tags-placeholder.text": "Tags separated by commas", + "segments.errors.duplicate-name.text": "A segment with name \"{{name}}\" already exists on context \"{{context}}\".", "stratifications.global-members-factor.text": "FACTOR", "stratifications.global-members-summary.text": "SUMMARY", "stratifications.global-members-status.text": "STATUS", From e84c10e5900118d53a6f85d0127e67e773614b8b Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Fri, 4 Apr 2025 15:44:32 -0400 Subject: [PATCH 07/10] hook up full list details on duplicate create --- .../close-modal.interceptor.ts | 7 +++--- .../upsert-segment-modal.component.ts | 23 ++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts b/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts index 18cd0df926..5f0dfdc48e 100644 --- a/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts +++ b/frontend/projects/upgrade/src/app/core/http-interceptors/close-modal.interceptor.ts @@ -15,9 +15,10 @@ export class CloseModalInterceptor implements HttpInterceptor { const isPostOrPut = req.method === 'POST' || req.method === 'PUT'; if (event instanceof HttpResponse && isPostOrPut) { - const shouldCloseModal = ENDPOINTS_TO_INTERCEPT_FOR_MODAL_CLOSE.some((endpoint) => - event.url.includes(endpoint) - ); + const shouldCloseModal = ENDPOINTS_TO_INTERCEPT_FOR_MODAL_CLOSE.some((endpoint) => { + const url = event.url.match(/\/api(.+)/)[1]; + return url === endpoint; + }); if (shouldCloseModal && event.status === 200) { this.dialog.closeAll(); } diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts index e343b48c8c..23255cfdf8 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts @@ -222,10 +222,12 @@ export class UpsertSegmentModalComponent { sendRequest(action: UPSERT_SEGMENT_ACTION, sourceSegment?: Segment): void { const formData: SegmentFormData = this.segmentForm.value; - if (action === UPSERT_SEGMENT_ACTION.ADD || action === UPSERT_SEGMENT_ACTION.DUPLICATE) { + if (action === UPSERT_SEGMENT_ACTION.ADD) { this.createAddRequest(formData); } else if (action === UPSERT_SEGMENT_ACTION.EDIT && sourceSegment) { this.createEditRequest(formData, sourceSegment); + } else if (action === UPSERT_SEGMENT_ACTION.DUPLICATE && sourceSegment) { + this.createDuplicateRequest(formData, sourceSegment); } else { console.error('UpsertSegmentModalComponent: sendRequest: Invalid action or missing sourceSegment'); } @@ -268,6 +270,25 @@ export class UpsertSegmentModalComponent { this.segmentService.modifySegment(segmentRequest); } + createDuplicateRequest({ name, description, tags }: SegmentFormData, sourceSegment: Segment): void { + const userIds = sourceSegment.individualForSegment.map((user) => user.userId); + const groups = sourceSegment.groupForSegment; + const subSegmentIds = sourceSegment.subSegments.map((subSegment) => subSegment.id); + + const segmentRequest: AddSegmentRequest = { + name, + description, + context: sourceSegment.context, + userIds, + groups, + subSegmentIds, + status: SEGMENT_STATUS.UNUSED, + type: SEGMENT_TYPE.PUBLIC, + tags, + }; + this.segmentService.addSegment(segmentRequest); + } + get UPSERT_SEGMENT_ACTION() { return UPSERT_SEGMENT_ACTION; } From 3b08580c8a7d2693439ee582ec1b4ebad7a17360 Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Mon, 7 Apr 2025 18:14:44 -0400 Subject: [PATCH 08/10] disable context on dupe, remove comments --- .../src/api/services/SegmentService.ts | 7 ----- .../upsert-segment-modal.component.ts | 31 +++++++++---------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/backend/packages/Upgrade/src/api/services/SegmentService.ts b/backend/packages/Upgrade/src/api/services/SegmentService.ts index 65ea30adb2..8c9983f57c 100644 --- a/backend/packages/Upgrade/src/api/services/SegmentService.ts +++ b/backend/packages/Upgrade/src/api/services/SegmentService.ts @@ -972,13 +972,6 @@ export class SegmentService { message: `Segment name ${name} already exists in context ${context}`, }); - // const error = new ErrorWithType(); - // error.type = SERVER_ERROR.SEGMENT_DUPLICATE_NAME; - // error.details = `Segment name ${name} already exists in context ${context}`; - // (error as any).duplicateName = name; - // (error as any).context = context; - // (error as any).httpCode = 400; - const error: DuplicateSegmentNameError = { type: SERVER_ERROR.SEGMENT_DUPLICATE_NAME, message: `Segment name ${name} already exists in context ${context}`, diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts index 23255cfdf8..4bd6659023 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts @@ -78,22 +78,6 @@ export class UpsertSegmentModalComponent { this.listenForIsInitialFormValueChanged(); this.listenForPrimaryButtonDisabled(); this.listenOnContext(); - - if (this.isDisabled()) { - this.disableRestrictedFields(); - } - } - - isDisabled() { - return ( - this.config.params.action === UPSERT_SEGMENT_ACTION.EDIT && - this.config.params.sourceSegment?.status === SEGMENT_STATUS.USED - ); - } - - disableRestrictedFields(): void { - this.nameControl?.disable(); - this.appContextControl?.disable(); } createSegmentForm(): void { @@ -107,6 +91,7 @@ export class UpsertSegmentModalComponent { appContext: [initialValues.appContext, Validators.required], tags: [initialValues.tags], }); + this.disableRestrictedFields(); this.initialFormValues$.next(this.segmentForm.value); @@ -114,6 +99,20 @@ export class UpsertSegmentModalComponent { this.listenForNameChanges(); } + disableRestrictedFields(): void { + if ( + this.config.params.action === UPSERT_SEGMENT_ACTION.EDIT && + this.config.params.sourceSegment.status === SEGMENT_STATUS.USED + ) { + this.appContextControl.disable(); + this.nameControl.disable(); + } + + if (this.config.params.action === UPSERT_SEGMENT_ACTION.DUPLICATE) { + this.appContextControl.enable(); + } + } + private updateNameControlErrors(shouldSetDuplicateError: boolean, error?: DuplicateSegmentNameError) { const currentErrors = this.nameControl.errors || {}; const { duplicateSegmentName, ...otherErrors } = currentErrors; From d418305f38e13f89d252c85af072d2e288ac0302 Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Tue, 8 Apr 2025 15:58:54 -0400 Subject: [PATCH 09/10] fix edit dupe name check, use modal event service to close modals --- .../src/api/services/SegmentService.ts | 5 ++- .../test/unit/services/SegmentService.test.ts | 44 ++++++++++++++++++- .../src/app/core/segments/segments.service.ts | 12 +---- .../segments/store/segments.effects.spec.ts | 14 +++++- .../core/segments/store/segments.effects.ts | 32 ++++++++------ .../upsert-segment-modal.component.ts | 10 ----- 6 files changed, 79 insertions(+), 38 deletions(-) diff --git a/backend/packages/Upgrade/src/api/services/SegmentService.ts b/backend/packages/Upgrade/src/api/services/SegmentService.ts index 64f12d48fd..6573b3dea7 100644 --- a/backend/packages/Upgrade/src/api/services/SegmentService.ts +++ b/backend/packages/Upgrade/src/api/services/SegmentService.ts @@ -386,7 +386,10 @@ export class SegmentService { } public async upsertSegment(segment: SegmentInputValidator, logger: UpgradeLogger): Promise { - await this.checkIsDuplicateSegmentName(segment.name, segment.context, logger); + // skip dupe check if segment has id, which means it's an update not an add + if (!segment.id) { + await this.checkIsDuplicateSegmentName(segment.name, segment.context, logger); + } logger.info({ message: `Upsert segment => ${JSON.stringify(segment, undefined, 2)}` }); return this.addSegmentDataInDB(segment, logger); diff --git a/backend/packages/Upgrade/test/unit/services/SegmentService.test.ts b/backend/packages/Upgrade/test/unit/services/SegmentService.test.ts index e0986d2c12..c892e017ec 100644 --- a/backend/packages/Upgrade/test/unit/services/SegmentService.test.ts +++ b/backend/packages/Upgrade/test/unit/services/SegmentService.test.ts @@ -421,12 +421,14 @@ describe('Segment Service Testing', () => { expect(segments).toEqual(ff_include); }); - it('should upsert a segment', async () => { + it('should upsert a segment with id (edit)', async () => { + service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); const segments = await service.upsertSegment(segVal, logger); expect(segments).toEqual(seg1); }); - it('should upsert a segment with no id', async () => { + it('should upsert a segment with no id (add)', async () => { + service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); const err = new Error('error'); const segment = new SegmentInputValidator(); segment.subSegmentIds = ['seg1']; @@ -437,24 +439,62 @@ describe('Segment Service Testing', () => { indivRepo.insertIndividualForSegment = jest.fn().mockImplementation(() => { throw err; }); + repo.find = jest.fn().mockResolvedValue([]); expect(async () => { await service.upsertSegment(segment, logger); }).rejects.toThrow(new Error('Error in creating individualDocs, groupDocs in "addSegmentInDB"')); }); + it('should throw an error if the segment has a duplicate name', async () => { + const dupeError = new Error( + JSON.stringify({ + type: SERVER_ERROR.SEGMENT_DUPLICATE_NAME, + message: `Segment name ${segVal.name} already exists in context ${segVal.context}`, + duplicateName: segVal.name, + context: segVal.context, // Fix: Make sure this is a string, not the segment object + httpCode: 400, + }) + ); + + const addSegment = { ...segVal, id: undefined }; + + // Make sure this mock is on the service instance being tested + service.checkIsDuplicateSegmentName = jest.fn().mockImplementation(() => { + throw dupeError; + }); + + // Also verify that addSegmentDataInDB isn't being called + service.addSegmentDataInDB = jest.fn(); + + expect(async () => { + await service.upsertSegment(addSegment, logger); + }).rejects.toThrow(dupeError); + + // Verify checkIsDuplicateSegmentName was called with correct parameters + expect(service.checkIsDuplicateSegmentName).toHaveBeenCalledWith(segVal.name, segVal.context, logger); + + // Verify addSegmentDataInDB was never called + expect(service.addSegmentDataInDB).not.toHaveBeenCalled(); + }); + it('should throw an error when unable to delete segment', async () => { + service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); const err = new Error('error'); const indivRepo = module.get(getRepositoryToken(IndividualForSegmentRepository)); indivRepo.insertIndividualForSegment = jest.fn().mockImplementation(() => { throw err; }); + repo.find = jest.fn().mockResolvedValue([]); + expect(async () => { await service.upsertSegment(segVal, logger); }).rejects.toThrow(err); }); it('should throw an error when unable to save segment', async () => { + service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); const err = new Error('error'); + repo.find = jest.fn().mockResolvedValue([]); repo.save = jest.fn().mockImplementation(() => { throw err; }); diff --git a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index 57bb62d19e..4a966e63d5 100644 --- a/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -35,7 +35,7 @@ import { UpdateSegmentRequest, UpsertSegmentType, } from './store/segments.model'; -import { filter, map, pairwise, tap, withLatestFrom } from 'rxjs/operators'; +import { filter, map, withLatestFrom } from 'rxjs/operators'; import { BehaviorSubject, Observable, combineLatest } from 'rxjs'; import { SegmentsDataService } from './segments.data.service'; import { SEGMENT_SEARCH_KEY, SORT_AS_DIRECTION, SEGMENT_SORT_KEY, DuplicateSegmentNameError } from 'upgrade_types'; @@ -45,7 +45,6 @@ import { selectContextMetaData } from '../experiments/store/experiments.selector import { selectSelectedFeatureFlag } from '../feature-flags/store/feature-flags.selectors'; import { CommonTextHelpersService } from '../../shared/services/common-text-helpers.service'; import { actionFetchContextMetaData } from '../experiments/store/experiments.actions'; -import isEqual from 'lodash.isequal'; @Injectable({ providedIn: 'root' }) export class SegmentsService { @@ -104,15 +103,6 @@ export class SegmentsService { ) ); - isSelectedSegmentUpdated$ = this.store$.pipe( - select(selectSelectedSegment), - pairwise(), - filter(([prev, curr]) => { - return prev && curr && !isEqual(prev, curr); - }), - map(([, curr]) => curr) - ); - selectPrivateSegmentListTypeOptions$ = this.store$.pipe( select(selectContextMetaData), withLatestFrom(this.store$.pipe(select(selectSelectedFeatureFlag))), diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts index 8b9b5299e4..f1c2d6f192 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.spec.ts @@ -6,6 +6,7 @@ import { SegmentsEffects } from './segments.effects'; import { Segment, SegmentFile, SegmentInput, UpsertSegmentType } from './segments.model'; import { selectAllSegments } from './segments.selectors'; import * as SegmentsActions from './segments.actions'; +import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service'; describe('SegmentsEffects', () => { let store$: any; @@ -13,6 +14,7 @@ describe('SegmentsEffects', () => { let segmentsDataService: any; let segmentService: any; let router: any; + let commonModalEventService: any; let service: SegmentsEffects; const mockSegment: Segment = { createdAt: 'test', @@ -49,11 +51,21 @@ describe('SegmentsEffects', () => { store$ = new BehaviorSubject({}); store$.dispatch = jest.fn(); segmentsDataService = {}; + commonModalEventService = { + forceCloseModal: jest.fn(), + }; router = { navigate: jest.fn(), }; - service = new SegmentsEffects(store$, actions$, segmentsDataService, segmentService, router); + service = new SegmentsEffects( + store$, + actions$, + segmentsDataService, + segmentService, + router, + commonModalEventService + ); }); describe('fetchAllSegments$', () => { diff --git a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts index 4f4f8194b0..06c7c9dbd7 100644 --- a/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts +++ b/frontend/projects/upgrade/src/app/core/segments/store/segments.effects.ts @@ -22,6 +22,7 @@ import JSZip from 'jszip'; import { of } from 'rxjs'; import { SERVER_ERROR } from 'upgrade_types'; import { SegmentsService } from '../segments.service'; +import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service'; @Injectable() export class SegmentsEffects { @@ -30,7 +31,8 @@ export class SegmentsEffects { private actions$: Actions, private segmentsDataService: SegmentsDataService, private segmentsService: SegmentsService, - private router: Router + private router: Router, + private commonModalEventService: CommonModalEventsService ) {} fetchSegmentsPaginated$ = createEffect(() => @@ -185,7 +187,10 @@ export class SegmentsEffects { ofType(SegmentsActions.actionAddSegment), switchMap((action) => { return this.segmentsDataService.addSegment(action.addSegmentRequest).pipe( - map((response: Segment) => SegmentsActions.actionAddSegmentSuccess({ segment: response })), + map((response: Segment) => { + this.commonModalEventService.forceCloseModal(); + return SegmentsActions.actionAddSegmentSuccess({ segment: response }); + }), catchError((error) => { if (error?.error?.type === SERVER_ERROR.SEGMENT_DUPLICATE_NAME) { this.segmentsService.setDuplicateSegmentNameError(error.error); @@ -197,23 +202,13 @@ export class SegmentsEffects { ) ); - navigateToSegmentDetail$ = createEffect( - () => - this.actions$.pipe( - ofType(SegmentsActions.actionAddSegmentSuccess), - tap(({ segment }) => { - this.router.navigate(['/segments', 'detail', segment.id]); - }) - ), - { dispatch: false } - ); - updateSegment$ = createEffect(() => this.actions$.pipe( ofType(SegmentsActions.actionUpdateSegment), switchMap((action) => { return this.segmentsDataService.modifySegment(action.updateSegmentRequest).pipe( concatMap((response: Segment) => { + this.commonModalEventService.forceCloseModal(); return [ SegmentsActions.actionUpdateSegmentSuccess({ segment: response }), SegmentsActions.actionGetSegmentById({ segmentId: response.id }), @@ -244,6 +239,17 @@ export class SegmentsEffects { ) ); + navigateToSegmentDetail$ = createEffect( + () => + this.actions$.pipe( + ofType(SegmentsActions.actionAddSegmentSuccess), + tap(({ segment }) => { + this.router.navigate(['/segments', 'detail', segment.id]); + }) + ), + { dispatch: false } + ); + exportSegments$ = createEffect(() => this.actions$.pipe( ofType(SegmentsActions.actionExportSegments), diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts index 4bd6659023..4b0979a5ee 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts @@ -54,7 +54,6 @@ export class UpsertSegmentModalComponent { isPrimaryButtonDisabled$: Observable; appContexts$ = this.segmentService.appContexts$; CommonTagInputType = CommonTagInputType; - isSelectedSegmentUpdated$ = this.segmentService.isSelectedSegmentUpdated$; initialFormValues$ = new BehaviorSubject(null); subscriptions = new Subscription(); isContextChanged = false; @@ -74,7 +73,6 @@ export class UpsertSegmentModalComponent { ngOnInit(): void { this.experimentService.fetchContextMetaData(); this.createSegmentForm(); - this.listenForSegmentGetUpdated(); this.listenForIsInitialFormValueChanged(); this.listenForPrimaryButtonDisabled(); this.listenOnContext(); @@ -171,14 +169,6 @@ export class UpsertSegmentModalComponent { this.subscriptions.add(this.isPrimaryButtonDisabled$.subscribe()); } - listenForSegmentGetUpdated(): void { - this.subscriptions.add( - this.isSelectedSegmentUpdated$.subscribe(() => { - this.closeModal(); - }) - ); - } - listenForNameChanges(): void { this.subscriptions.add( this.nameControl.valueChanges From 2526135d8cc11bea45c92d5b7695a0236fcd3cca Mon Sep 17 00:00:00 2001 From: danoswaltCL Date: Tue, 8 Apr 2025 17:01:10 -0400 Subject: [PATCH 10/10] make sure subsegments copied over for upsert --- .../upsert-segment-modal.component.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts index 4b0979a5ee..b3575dfcc1 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-segment-modal/upsert-segment-modal.component.ts @@ -239,6 +239,8 @@ export class UpsertSegmentModalComponent { createEditRequest({ name, description, appContext, tags }: SegmentFormData, sourceSegment: Segment): void { const { id, status } = sourceSegment; + const subSegmentIds = sourceSegment.subSegments.map((subSegment) => subSegment.id); + // Not allow editing segment name and context if segment is in used status: if (sourceSegment.status === SEGMENT_STATUS.USED) { name = sourceSegment.name; @@ -251,7 +253,7 @@ export class UpsertSegmentModalComponent { context: appContext, userIds: [], groups: [], - subSegmentIds: [], + subSegmentIds, status, type: SEGMENT_TYPE.PUBLIC, tags, @@ -260,16 +262,14 @@ export class UpsertSegmentModalComponent { } createDuplicateRequest({ name, description, tags }: SegmentFormData, sourceSegment: Segment): void { - const userIds = sourceSegment.individualForSegment.map((user) => user.userId); - const groups = sourceSegment.groupForSegment; const subSegmentIds = sourceSegment.subSegments.map((subSegment) => subSegment.id); const segmentRequest: AddSegmentRequest = { name, description, context: sourceSegment.context, - userIds, - groups, + userIds: [], + groups: [], subSegmentIds, status: SEGMENT_STATUS.UNUSED, type: SEGMENT_TYPE.PUBLIC,