Skip to content

allow target to be empty; update displays #3111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bcb37 wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

allow target to be empty; update displays #3111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d39ab1
allow target to be empty; update displays
bcb37 May 11, 2026
72e0ee5
review fixes
bcb37 May 11, 2026
8380ab8
Potential fix for pull request finding
bcb37 May 11, 2026
42892b6
use new format for exports
bcb37 May 12, 2026
5debb5a
review suggestions
bcb37 May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/backend/src/api/DTO/ExperimentDTO.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,8 @@ export class PartitionValidator {
@IsString()
public site: string;

@IsOptional()
@IsString()
public target?: string;
public target = '';

@IsOptional()
@IsString()
Expand Down
8 changes: 4 additions & 4 deletions packages/backend/src/api/services/AnalyticsService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,8 @@ export class AnalyticsService {
const excludeIfReached = row.excludeIfReached ? 'TRUE' : 'FALSE';
const stratification =
row.stratification && row.stratificationValue ? `${row.stratification}: ${row.stratificationValue}` : 'NA';
const target = row.target ? ` (${row.target})` : '';
const decisionPoint = row.site + target;

return {
ExperimentId: row.experimentId,
Expand All @@ -356,8 +358,7 @@ export class AnalyticsService {
DesignType: row.designType,
AlgorithmType: row.algorithmType,
Stratification: stratification,
Site: row.site,
Target: row.target,
DecisionPoint: decisionPoint,
ExcludeifReached: excludeIfReached,
ConditionName: row.conditionName,
Payload: row.payload ? row.payload : row.conditionName,
Expand Down Expand Up @@ -406,8 +407,7 @@ export class AnalyticsService {
DesignType: '',
AlgorithmType: '',
Stratification: '',
Site: '',
Target: '',
DecisionPoint: '',
ExcludeifReached: '',
ConditionName: '',
Payload: '',
Expand Down
44 changes: 33 additions & 11 deletions packages/backend/src/api/services/ExperimentService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,13 @@ export class ExperimentService {
const [experimentData, count] = await Promise.all([queryBuilderToReturn.getMany(), countQuery.getCount()]);
return [
experimentData.map((experiment) => {
const experimentWithStringTargets = this.coerceExperimentDecisionPointTargets(experiment);
return {
...experiment,
state: EXPERIMENT_STATE_DISPLAY_NAME_OVERRIDES[experiment.state] || experiment.state,
stateTimeLogs: this.transformStateTimeLogs(experiment.stateTimeLogs),
...experimentWithStringTargets,
state:
EXPERIMENT_STATE_DISPLAY_NAME_OVERRIDES[experimentWithStringTargets.state] ||
experimentWithStringTargets.state,
stateTimeLogs: this.transformStateTimeLogs(experimentWithStringTargets.stateTimeLogs),
};
}),
count || 0,
Expand Down Expand Up @@ -430,14 +433,15 @@ export class ExperimentService {
public async getExperimentPartitions(experimentId: string, logger: UpgradeLogger): Promise<DecisionPoint[]> {
logger.info({ message: `getExperimentPartitions experiment => ${experimentId}` });
const experiment: Experiment = await this.findOne(experimentId, logger);
return experiment?.partitions;
return this.coerceDecisionPointTargets(experiment?.partitions || []);
}

public async getAllExperimentPartitions(
logger: UpgradeLogger
): Promise<Array<Pick<DecisionPoint, 'site' | 'target'>>> {
logger.info({ message: 'getAllExperimentPartitions experiment' });
return this.decisionPointRepository.partitionPointAndName();
const decisionPoints = await this.decisionPointRepository.partitionPointAndName();
return this.coerceDecisionPointTargets(decisionPoints as DecisionPoint[]);
}

public async getAllUniqueIdentifiers(logger: UpgradeLogger): Promise<string[]> {
Expand Down Expand Up @@ -1868,20 +1872,22 @@ export class ExperimentService {
}

public formattingConditionPayload(experiment: Experiment): Experiment {
if (experiment.type === EXPERIMENT_TYPE.FACTORIAL) {
const experimentWithStringTargets = this.coerceExperimentDecisionPointTargets(experiment);

if (experimentWithStringTargets.type === EXPERIMENT_TYPE.FACTORIAL) {
const conditionPayload: ConditionPayload[] = [];
experiment.conditions.forEach((condition) => {
experimentWithStringTargets.conditions.forEach((condition) => {
const conditionPayloads = condition.conditionPayloads.map((conditionPayload) => {
return { ...conditionPayload, parentCondition: condition };
});
conditionPayload.push(...conditionPayloads);
delete condition.conditionPayloads;
});

return { ...experiment, conditionPayloads: conditionPayload };
return { ...experimentWithStringTargets, conditionPayloads: conditionPayload };
}

const { conditions, partitions } = experiment;
const { conditions, partitions } = experimentWithStringTargets;

const conditionPayload: ConditionPayload[] = [];
partitions.forEach((partition) => {
Expand All @@ -1895,7 +1901,7 @@ export class ExperimentService {
}
});
});
return { ...experiment, conditionPayloads: conditionPayload };
return { ...experimentWithStringTargets, conditionPayloads: conditionPayload };
}

public reducedConditionPayload(experiment: Experiment): any {
Expand All @@ -1910,6 +1916,8 @@ export class ExperimentService {
}

public formattingPayload(experiment: Experiment): any {
const experimentWithStringTargets = this.coerceExperimentDecisionPointTargets(experiment);

const updatedConditionPayloads = experiment.conditionPayloads.map((conditionPayload) => {
const { payloadType, payloadValue, ...rest } = conditionPayload;
return {
Expand All @@ -1927,7 +1935,21 @@ export class ExperimentService {
return { ...rest, levels: updatedLevels };
});

return { ...experiment, factors: updatedFactors, conditionPayloads: updatedConditionPayloads };
return { ...experimentWithStringTargets, factors: updatedFactors, conditionPayloads: updatedConditionPayloads };
}

private coerceDecisionPointTargets(decisionPoints: DecisionPoint[]): DecisionPoint[] {
return decisionPoints.map((decisionPoint) => ({
...decisionPoint,
target: decisionPoint.target ?? '',
}));
}

private coerceExperimentDecisionPointTargets(experiment: Experiment): Experiment {
return {
...experiment,
partitions: this.coerceDecisionPointTargets(experiment.partitions || []),
};
}

private mapStatusStrings(statusString: string): string {
Expand Down
1 change: 1 addition & 0 deletions packages/backend/test/integration/mockData/experiment/raw.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ export const experiment = {
{
id: 'd2702d3c-5e04-41a7-8766-1da8a95b72ae',
site: 'CurriculumSequence',
target: '',
description: 'No Decision Point',
twoCharacterId: 'NP',
excludeIfReached: false,
Expand Down
2 changes: 2 additions & 0 deletions packages/backend/test/unit/services/ExperimentAssignmentService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ describe('Experiment Assignment Service Test', () => {
const moocletExperimentServiceMock = sinon.createStubInstance(MoocletExperimentService);
experimentServiceMock.formattingConditionPayload.restore();
experimentServiceMock.formattingPayload.restore();
experimentServiceMock.coerceExperimentDecisionPointTargets.restore();
experimentServiceMock.coerceDecisionPointTargets.restore();

beforeAll(() => {
configureLogger();
Expand Down
6 changes: 3 additions & 3 deletions packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ export interface ExperimentConditionFilterOptions {

export interface ExperimentPartitionFilterOptions {
id: string;
point: string;
twoCharacterId: string;
site: string;
target: string;
}

export interface ExperimentDateFilterOptions {
Expand Down Expand Up @@ -432,7 +432,7 @@ export interface ExperimentConditionDTO {
export interface ExperimentPartitionDTO {
id: string;
site: string;
target?: string;
target: string;
description?: string;
order: number;
excludeIfReached: boolean;
Expand Down
3 changes: 3 additions & 0 deletions ...rojects/upgrade/src/app/features/dashboard/experiments/experiment-decision-point.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function formatDecisionPointDisplay(decisionPoint: { site: string; target: string }): string {
return decisionPoint.target ? `${decisionPoint.site} (${decisionPoint.target})` : decisionPoint.site;
}
2 changes: 1 addition & 1 deletion ...experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
</mat-form-field>

<mat-form-field appearance="outline">
<mat-label class="ft-14-400">Target</mat-label>
<mat-label class="ft-14-400">Target (optional)</mat-label>
<input
matInput
formControlName="target"
Expand Down
18 changes: 9 additions & 9 deletions ...d/experiments/modals/upsert-decision-point-modal/upsert-decision-point-modal.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy {
this.decisionPointForm = this.formBuilder.group(
{
site: [initialValues.site, [Validators.required]],
target: [initialValues.target, [Validators.required]],
target: [initialValues.target],
excludeIfReached: [initialValues.excludeIfReached],
},
options
Expand Down Expand Up @@ -168,8 +168,8 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy {
const site = siteControl.value?.trim() || '';
const target = targetControl.value?.trim() || '';

// Don't validate if either field is empty (required validator will handle that)
if (!site || !target) {
// Don't validate if site field is empty (required validator will handle that)
if (!site) {
// Clear any existing duplicate errors when fields are empty
this.clearDuplicateError(siteControl);
this.clearDuplicateError(targetControl);
Comment thread
bcb37 marked this conversation as resolved.
Expand All @@ -189,16 +189,16 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy {

// Check if this decision point already exists
const isDuplicate = currentExperiment.partitions.some((decisionPoint) => {
const isSameSite = decisionPoint.site?.trim() === site;
const isSameTarget = decisionPoint.target?.trim() === target;
const isSameSite = decisionPoint.site.trim() === site;
const isSameTarget = decisionPoint.target.trim() === target;

// For edit action, exclude the current decision point being edited
if (this.config.params.action === UPSERT_EXPERIMENT_ACTION.EDIT) {
const sourceDecisionPoint = this.config.params.sourceDecisionPoint;
if (sourceDecisionPoint) {
const isCurrentDecisionPoint =
decisionPoint.site?.trim() === sourceDecisionPoint.site?.trim() &&
decisionPoint.target?.trim() === sourceDecisionPoint.target?.trim();
decisionPoint.site.trim() === sourceDecisionPoint.site.trim() &&
decisionPoint.target.trim() === sourceDecisionPoint.target.trim();

// Skip validation if it's the same decision point being edited
if (isCurrentDecisionPoint) {
Expand Down Expand Up @@ -276,8 +276,8 @@ export class UpsertDecisionPointModalComponent implements OnInit, OnDestroy {
excludeIfReached: formData.excludeIfReached,
};

// Validate trimmed values are not empty
if (!decisionPointData.site || !decisionPointData.target) {
// Validate trimmed site value is not empty
if (!decisionPointData.site) {
CommonFormHelpersService.triggerTouchedToDisplayErrors(this.decisionPointForm);
return;
}
Expand Down
30 changes: 11 additions & 19 deletions ...ion-card/experiment-decision-points-table/experiment-decision-points-table.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,18 @@
[ngClass]="{ 'no-data': !decisionPoints?.length }"
class="decision-points-table"
>
<!-- Site Column -->
<ng-container matColumnDef="site">
<th mat-header-cell *matHeaderCellDef class="site-column ft-14-600">
{{ DECISION_POINT_TRANSLATION_KEYS.SITE | translate }}
<!-- Decision Point Column -->
<ng-container matColumnDef="decisionPoint">
<th mat-header-cell *matHeaderCellDef class="decision-point-column ft-14-600">
{{ DECISION_POINT_TRANSLATION_KEYS.DECISION_POINT | translate }}
</th>
<td mat-cell *matCellDef="let decisionPoint" class="site-column ft-14-400">
<span [appTextOverflowTooltip]="decisionPoint.site" matTooltipPosition="above" class="line-clamp-1">
{{ decisionPoint.site }}
</span>
</td>
</ng-container>

<!-- Target Column -->
<ng-container matColumnDef="target">
<th mat-header-cell *matHeaderCellDef class="target-column ft-14-600">
{{ DECISION_POINT_TRANSLATION_KEYS.TARGET | translate }}
</th>
<td mat-cell *matCellDef="let decisionPoint" class="target-column ft-14-400">
<span [appTextOverflowTooltip]="decisionPoint.target" matTooltipPosition="above" class="line-clamp-1">
{{ decisionPoint.target }}
<td mat-cell *matCellDef="let decisionPoint" class="decision-point-column ft-14-400">
<span
[appTextOverflowTooltip]="getDecisionPoint(decisionPoint)"
matTooltipPosition="above"
class="line-clamp-1"
>
{{ getDecisionPoint(decisionPoint) }}
</span>
</td>
</ng-container>
Expand Down
8 changes: 2 additions & 6 deletions ...ion-card/experiment-decision-points-table/experiment-decision-points-table.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,11 @@
}
}

.site-column {
width: 36%;
.decision-point-column {
width: 70%;
padding-left: 32px;
}

.target-column {
width: 34%;
}

.exclude-if-reached-column {
width: 20%;
text-align: center;
Expand Down
10 changes: 7 additions & 3 deletions ...ction-card/experiment-decision-points-table/experiment-decision-points-table.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
EXPERIMENT_ROW_ACTION,
} from '../../../../../../../../core/experiments/store/experiments.model';
import { SharedModule } from '../../../../../../../../shared/shared.module';
import { formatDecisionPointDisplay } from '../../../../../experiment-decision-point.utils';

@Component({
selector: 'app-experiment-decision-points-table',
Expand All @@ -40,15 +41,18 @@ export class ExperimentDecisionPointsTableComponent {
@Input() actionsTooltip?: string = '';
@Output() rowAction = new EventEmitter<ExperimentDecisionPointRowActionEvent>();

displayedColumns: string[] = ['site', 'target', 'excludeIfReached', 'actions'];
displayedColumns: string[] = ['decisionPoint', 'excludeIfReached', 'actions'];

DECISION_POINT_TRANSLATION_KEYS = {
SITE: 'experiments.details.decision-points.site.text',
TARGET: 'experiments.details.decision-points.target.text',
DECISION_POINT: 'experiments.details.decision-points.decision-point.text',
EXCLUDE_IF_REACHED: 'experiments.details.decision-points.exclude-if-reached.text',
ACTIONS: 'experiments.details.decision-points.actions.text',
};

getDecisionPoint(decisionPoint: ExperimentDecisionPoint): string {
return formatDecisionPointDisplay(decisionPoint);
}

onEditButtonClick(decisionPoint: ExperimentDecisionPoint): void {
this.rowAction.emit({ action: EXPERIMENT_ROW_ACTION.EDIT, decisionPoint });
}
Expand Down
9 changes: 6 additions & 3 deletions ...le/enrollment-condition-expandable-row/enrollment-condition-expandable-row.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ div {
}

mat-cell:nth-child(2),
mat-cell:nth-child(3),
mat-header-cell:nth-child(2),
mat-header-cell:nth-child(3) {
mat-header-cell:nth-child(2) {
flex: 1;
}

mat-cell:nth-child(n + 3),
mat-header-cell:nth-child(n + 3) {
flex: 0 0 25%;
width: 25%;
}
Expand Down
2 changes: 0 additions & 2 deletions ...able/enrollment-condition-expandable-row/enrollment-condition-expandable-row.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export class EnrollmentConditionExpandableRowComponent implements OnDestroy {
'home.view-experiment.global.users-enrolled.text',
'home.view-experiment.global.group-enrolled.text',
'home.view-experiment-global.experiment-site.text',
'home.view-experiment-global.experiment-target.text',
])
.subscribe((arrayValues) => {
this.columnHeaders = {
Expand All @@ -49,7 +48,6 @@ export class EnrollmentConditionExpandableRowComponent implements OnDestroy {
userEnrolled: arrayValues['home.view-experiment.global.users-enrolled.text'],
groupEnrolled: arrayValues['home.view-experiment.global.group-enrolled.text'],
experimentPoint: arrayValues['home.view-experiment-global.experiment-site.text'],
experimentId: arrayValues['home.view-experiment-global.experiment-target.text'],
};
});
}
Expand Down
12 changes: 5 additions & 7 deletions ...ment-data-section-card/enrollment-condition-table/enrollment-condition-table.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { TranslateModule } from '@ngx-translate/core';

import { EnrollmentConditionExpandableRowComponent } from './enrollment-condition-expandable-row/enrollment-condition-expandable-row.component';
import { MatProgressSpinnerModule } from '@angular/material/progress-spinner';
import { formatDecisionPointDisplay } from '../../../../../experiment-decision-point.utils';

@Component({
selector: 'app-enrollment-condition-table',
Expand Down Expand Up @@ -54,8 +55,7 @@ export class EnrollmentConditionTableComponent implements OnChanges, OnInit, OnD
const partitions = [];
condition.partitions.forEach((partition) => {
const partitionObj: EnrollmentByConditionOrPartitionData = {
experimentPoint: this.getPartitionData(partition.id, 'site'),
experimentId: this.getPartitionData(partition.id, 'target') || '',
experimentPoint: this.getDecisionPointData(partition.id),
userEnrolled: partition.users,
groupEnrolled: partition.groups,
};
Expand All @@ -73,11 +73,9 @@ export class EnrollmentConditionTableComponent implements OnChanges, OnInit, OnD
});
}

getPartitionData(partitionId: string, key: string) {
return this.experiment.partitions.reduce(
(acc, partition) => (partition.id === partitionId ? (acc = partition[key]) : acc),
null
);
getDecisionPointData(partitionId: string) {
const decisionPoint = this.experiment.partitions.find((partition) => partition.id === partitionId);
return decisionPoint ? formatDecisionPointDisplay(decisionPoint) : '';
}

getConditionData(conditionId: string, key: string) {
Expand Down
Loading