From ec93d7fb55a1335b757c6e7be94b9aa4f5f6187f Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Fri, 5 Jun 2026 11:02:22 -0400 Subject: [PATCH 01/10] phases 1-4 of refactor plan --- docs/mark-context-refactor-plan.md | 170 +++++++++++++++++ .../ExperimentClientController.v5.ts | 1 + .../ExperimentClientController.v6.ts | 1 + .../validators/MarkExperimentValidator.v5.ts | 4 + .../validators/MarkExperimentValidator.v6.ts | 4 + .../services/ExperimentAssignmentService.ts | 179 +++++++++++------- .../src/api/services/ExperimentService.ts | 38 +--- .../test/integration/UserNotDefined/index.ts | 1 + 8 files changed, 299 insertions(+), 99 deletions(-) create mode 100644 docs/mark-context-refactor-plan.md diff --git a/docs/mark-context-refactor-plan.md b/docs/mark-context-refactor-plan.md new file mode 100644 index 0000000000..f6293ad474 --- /dev/null +++ b/docs/mark-context-refactor-plan.md @@ -0,0 +1,170 @@ +# Implementation Plan: Mark Path Correctness Refactor — Pooling Logic Consolidation & Context-Aware Caching + +## Rationale + +This refactor addresses three related correctness problems in the mark path. + +### 1. Pooling logic divergence between assign and mark + +`getAllExperimentConditions` (the assign path) applies a full pipeline: global exclusion → group-experiment filtering → experiment-level inclusion/exclusion → `processExperimentPools`. Before this refactor, `markExperimentPoint` did none of that — it fetched decision points from a separate cache and performed ad-hoc filtering. This meant assign and mark could disagree on which experiment a user was enrolled in, leading to incorrect or mismatched mark records. + +Consolidating the selection logic into `resolveExperimentForMarkPoint` — a shared private method that mirrors the assign pipeline — guarantees the two paths always agree. + +### 2. Missing `context` on mark requests + +`context` is a required, client-supplied identifier that scopes all experiment activity. The assign path has always required it. The mark path never received it, which caused two downstream problems: + +- **Cross-context cache contamination** — the mark cache key `MARK_KEY_PREFIX-{site}-{target}` has no context dimension, so experiments from different contexts sharing a site/target can collide in the cache. +- **Derived context is fragile** — the service was inferring context from the first cached experiment (`allExperimentsAtDP[0]?.context?.[0]`), which is undefined when the cache is empty and wrong if experiments from multiple contexts are present. + +Requiring `context` on mark requests makes the contract explicit, eliminates the derivation hack, and allows mark to use the same `EXPERIMENT_KEY_PREFIX-{context}` cache as assign. + +### 3. No state filter on the mark cache query + +`DecisionPointRepository.find()` returns experiments in any state (e.g., `CANCELLED`, `PREVIEW`). Switching to `getCachedValidExperiments(context)` — which uses `getValidExperimentsForContextAndDecisionPoint` under the hood — ensures only `ENROLLING` and `ENROLLMENT_COMPLETE` experiments are considered, matching the assign path's behavior. + +### Cache strategy + +Replacing the mark-specific cache with the shared `EXPERIMENT_KEY_PREFIX-{context}` cache eliminates a redundant DB query pattern and removes a stale cache key. Mark does a cheap in-memory filter over the cached experiments to find those relevant to the given `site`/`target`. The 60-second production TTL is not a concern in practice because there is no mark-only traffic — assign reliably warms the cache before mark is called in the normal usage pattern. A cold mark fetch (e.g., after TTL expiry or a server restart) is heavier than before since it loads all experiments for the context rather than only the DPs for a single site/target, but at ~74 experiments this is not a meaningful concern. + +--- + +## Phase 0 — Backend: Pooling Logic Consolidation ✅ COMPLETED + +**File:** `ExperimentAssignmentService.ts` + +Prior to this plan, `markExperimentPoint` used a simple cache lookup and did not apply the same experiment-selection logic as `getAllExperimentConditions`. This meant the two methods could disagree on which experiment a user was enrolled in. + +The following work has already been completed: + +1. Extracted `resolveExperimentForMarkPoint(site, target, experimentId, userDoc, previewUser, logger)` — a private method that contains the full filtering and pooling pipeline mirroring `getAllExperimentConditions`: + - Global user/group exclusion check + - When `experimentId` is provided: validates it exists at the decision point, then runs experiment-level inclusion/exclusion + - When `experimentId` is absent: runs group-experiment filtering → experiment-level inclusion/exclusion → `processExperimentPools`, returning the single selected experiment +2. `markExperimentPoint` now delegates to `resolveExperimentForMarkPoint` instead of doing ad-hoc filtering inline +3. The indirect group exclusion save and all downstream enrollment/monitoring logic now operates on the correctly resolved experiment + +This guarantees that mark and assign always agree on the selected experiment for a given user and decision point. + +--- + +## Phase 1 — Backend: Validators ✅ COMPLETED + +**Files:** `MarkExperimentValidator.v5.ts`, `MarkExperimentValidator.v6.ts` + +- Add `@IsString() @IsNotEmpty() context: string` to `MarkExperimentValidatorv6` (required — v6 is the current client-facing version) +- Add `@IsString() @IsOptional() context?: string` to `MarkExperimentValidatorv5` (optional — for backward compatibility with older clients) + +--- + +## Phase 2 — Backend: Service ✅ COMPLETED + +**File:** `ExperimentAssignmentService.ts` + +1. Add `context: string` parameter to `markExperimentPoint` +2. Thread `context` down into `resolveExperimentForMarkPoint` +3. In `resolveExperimentForMarkPoint`: + - Replace `getCachedExperiments(site, target)` with `experimentService.getCachedValidExperiments(context)` (same call used by the assign path) + - Add in-memory filter: `allExperimentsForContext.filter(exp => exp.partitions.some(p => p.site === site && p.target === target))` + - Remove the `context` derivation hack (`allExperimentsAtDP[0]?.context?.[0]`) — it will now be a first-class parameter +4. Update `resolveExperimentForMarkPoint` return type — remove `dpExperiments: DecisionPoint[]` from the return shape +5. In `markExperimentPoint`, replace `dpExperiments.find(dp => dp.experiment.id === experimentId)` with `experiments[0]?.partitions.find(p => p.site === site && p.target === target)` for `selectedExperimentDP` +6. Delete the `getCachedExperiments` private method entirely +7. Remove `MARK_KEY_PREFIX` import/usage + +--- + +## Phase 3 — Backend: Controllers ✅ COMPLETED + +**Files:** `ExperimentClientController.v6.ts`, `ExperimentClientController.v5.ts` + +- Thread `experiment.context` from the validated body into the `markExperimentPoint` service call in both controllers +- For v5, pass a fallback (e.g., `experiment.context ?? ''`) since the field is optional in that version + +--- + +## Phase 4 — Backend: Cache Cleanup ✅ COMPLETED + +**File:** `ExperimentService.ts` — `clearExperimentCacheDetail` private method + +- Remove the `delCache(MARK_KEY_PREFIX + '-' + partition.site + '-' + partition.target)` loop — that key no longer exists +- The method body simplifies to just `delCache(EXPERIMENT_KEY_PREFIX + context)` + +--- + +## Phase 5 — JS Client Library + +**Files:** `UpGradeClient.types.ts`, `types/requests.ts`, `ApiService.ts`, `UpgradeClient.ts`, `Assignment.ts` + +1. `IMarkDecisionPointParams` — add `context: string` +2. `IMarkDecisionPointRequestBody` — add `context: string` at the top level of the request body +3. `ApiService.markDecisionPoint` — include `context` in the built `requestBody` +4. `UpgradeClient.markDecisionPoint` — pass the stored context (already available via the `ApiService` constructor config) into the params object +5. `Assignment.markDecisionPoint` — `Assignment` currently has no direct reference to `context`; the `ApiService` already holds it, so `ApiService.markDecisionPoint` can read it internally from `this.context` without changing the `Assignment` public API + +--- + +## Phase 6 — Java Client Library + +**Files:** `MarkExperimentRequest.java`, `ExperimentClient.java` + +1. `MarkExperimentRequest.java` — add `context` field with getter/setter and update all constructors to include it +2. `ExperimentClient.java` — all six `markDecisionPoint` overloads delegate to the private `markDecisionPoint(status, data, clientError, uniquifier, callbacks)` method; add `this.context` to the `MarkExperimentRequest` construction in that private method. No public API changes needed. + +--- + +## Phase 7 — Integration Tests + +**Files:** `packages/backend/test/integration/Experiment/markExperimentPoint/**` + +- Update all `markExperimentPoint` test utility calls to include `context` +- Verify the no-experiment and with-experiment paths still pass + +### New tests to add + +#### 7a. State filter — testable against the unfixed code (demonstrates bug, then verifies fix) + +This is the cleanest test to write before Phases 1–4 are complete, since it doesn't require the `context` wire changes to observe the behavioral difference. + +**Setup:** + +1. Create an experiment at `site`/`target`, put it into `ENROLLING` +2. Transition the experiment to `CANCELLED` +3. Call `markExperimentPoint` at the same `site`/`target` + +**Before fix:** The cancelled experiment appears in the mark result (no state filter on `DecisionPointRepository.find()`). +**After fix:** The experiment is excluded — `getCachedValidExperiments` filters by state, so no experiment is found and the mark records no enrollment. + +--- + +#### 7b. Context contamination — testable once Phase 1 adds `context` to the wire format + +**Setup:** + +1. Create experiment A in `context="math"`, `site="homepage"`, `target="button"` +2. Create experiment B in `context="science"`, `site="homepage"`, `target="button"` +3. Mark user 1 with `context="math"` at `site="homepage"`, `target="button"` +4. Mark user 2 with `context="science"` at the same `site`/`target` + +**Before fix:** Both marks hit the same `MARK_KEY_PREFIX-homepage-button` cache entry, so one of the users gets the wrong experiment's data in their monitored decision point record. +**After fix:** Each mark uses `EXPERIMENT_KEY_PREFIX-{context}`, so user 1's mark references experiment A and user 2's references experiment B. + +--- + +#### 7c. Pooling regression — verifies correct behavior is preserved (Phase 0) + +Since pooling consolidation is already complete, this test documents the now-correct behavior and would catch a regression if the code were reverted. + +**Setup:** + +1. Create two experiments in the same pool at `site`/`target`, where pool assignment logic selects experiment A for user 1 +2. Call `getAllExperimentConditions` for user 1 → verify experiment A is assigned +3. Call `markExperimentPoint` for user 1 at the same `site`/`target` with no `experimentId` + +**Assert:** The mark record's `experimentId` matches the experiment returned by assign (experiment A). Under the old code, mark used ad-hoc filtering rather than `processExperimentPools` and could select a different experiment. + +--- + +## Order Dependencies + +Phases 1–4 are purely backend and can be done together. Phase 5 (JS) and Phase 6 (Java) depend on Phases 1–4 being finalized so the wire format is stable before client changes are made. Phase 7 follows Phases 1–4. diff --git a/packages/backend/src/api/controllers/ExperimentClientController.v5.ts b/packages/backend/src/api/controllers/ExperimentClientController.v5.ts index f14f168321..fc84c20761 100644 --- a/packages/backend/src/api/controllers/ExperimentClientController.v5.ts +++ b/packages/backend/src/api/controllers/ExperimentClientController.v5.ts @@ -480,6 +480,7 @@ export class ExperimentClientController { experiment.status, experiment.data.assignedCondition?.conditionCode ?? null, request.logger, + experiment.context ?? '', experiment.data.assignedCondition?.experimentId ?? null, experiment.data.target, experiment.uniquifier ? experiment.uniquifier : null, diff --git a/packages/backend/src/api/controllers/ExperimentClientController.v6.ts b/packages/backend/src/api/controllers/ExperimentClientController.v6.ts index d6f30baced..e2bf917bcb 100644 --- a/packages/backend/src/api/controllers/ExperimentClientController.v6.ts +++ b/packages/backend/src/api/controllers/ExperimentClientController.v6.ts @@ -450,6 +450,7 @@ export class ExperimentClientController { experiment.status, experiment.data.assignedCondition?.conditionCode ?? null, request.logger, + experiment.context, experiment.data.assignedCondition?.experimentId ?? null, experiment.data.target, experiment.uniquifier ? experiment.uniquifier : null, diff --git a/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v5.ts b/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v5.ts index 3803fe0944..43c5ec1992 100644 --- a/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v5.ts +++ b/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v5.ts @@ -34,6 +34,10 @@ export class MarkExperimentValidatorv5 { @IsNotEmpty() public userId: string; + @IsString() + @IsOptional() + public context?: string; + @IsDefined() @ValidateNested() @Type(() => Data) diff --git a/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v6.ts b/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v6.ts index 5ed0280128..94f23f0beb 100644 --- a/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v6.ts +++ b/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v6.ts @@ -31,6 +31,10 @@ class Data { } export class MarkExperimentValidatorv6 { + @IsString() + @IsNotEmpty() + public context: string; + @IsDefined() @ValidateNested() @Type(() => Data) diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index acf41dd06c..ed0886af44 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -22,7 +22,6 @@ import { SUPPORTED_CALIPER_PROFILES, SUPPORTED_CALIPER_EVENTS, IExperimentAssignmentv5, - CACHE_PREFIX, ASSIGNMENT_ALGORITHM, IPayload, PAYLOAD_TYPE, @@ -127,26 +126,108 @@ export class ExperimentAssignmentService { public moocletExperimentService: MoocletExperimentService ) {} - private async getCachedExperiments(site: string, target: string): Promise<[DecisionPoint[], Experiment[]]> { - const cacheKey = CACHE_PREFIX.MARK_KEY_PREFIX + '-' + site + '-' + target; - const dpExperiments = await this.cacheService.wrap(cacheKey, () => - this.decisionPointRepository.find({ - where: { - site: site, - target: target, - }, - relations: [ - 'experiment', - 'experiment.conditions', - 'experiment.conditions.conditionPayloads', - 'experiment.partitions', - 'experiment.experimentSegmentInclusion.segment', - 'experiment.experimentSegmentExclusion.segment', - ], - }) + /** + * Shared experiment-selection logic used by both markExperimentPoint and (indirectly) getAllExperimentConditions. + * + * When experimentId is supplied the method validates it and returns that single experiment after running + * exclusion checks. When experimentId is absent it applies the full filtering and pooling pipeline + * (global exclusion → group-experiment filtering → experiment-level inclusion/exclusion → processExperimentPools) + * so the selected experiment is guaranteed to match what getAllExperimentConditions would have returned for + * the same user and decision point. + */ + private async resolveExperimentForMarkPoint( + site: string, + target: string, + context: string, + experimentId: string, + userDoc: RequestedExperimentUser, + previewUser: PreviewUser, + logger: UpgradeLogger + ): Promise<{ + experiments: Experiment[]; + experimentId: string | null; + isUserExcluded: boolean; + isGroupExcluded: boolean; + exclusionReason: { experiment: Experiment; reason: string; matchedGroup: boolean }[]; + }> { + const allExperimentsForContext = await this.experimentService.getCachedValidExperiments(context); + const allExperimentsAtDP = allExperimentsForContext.filter((exp) => + exp.partitions.some((p) => p.site === site && p.target === target) + ); + + if (allExperimentsAtDP.length === 0) { + return { + experiments: [], + experimentId, + isUserExcluded: false, + isGroupExcluded: false, + exclusionReason: [], + }; + } + + const [isUserExcluded, isGroupExcluded] = await this.checkUserOrGroupIsGloballyExcluded(userDoc, context); + + if (isUserExcluded || isGroupExcluded) { + return { experiments: [], experimentId, isUserExcluded, isGroupExcluded, exclusionReason: [] }; + } + + if (experimentId) { + // Experiment ID provided: validate it exists at this decision point then run exclusion checks. + const dpExpExists = allExperimentsAtDP.filter((exp) => exp.id === experimentId); + if (!dpExpExists.length) { + const error = new Error( + `Experiment ID not provided for shared Decision Point in markExperimentPoint: ${userDoc.id}` + ); + (error as any).type = SERVER_ERROR.INVALID_EXPERIMENT_ID_FOR_SHARED_DECISIONPOINT; + (error as any).httpCode = 404; + logger.error(error); + throw error; + } + const [, exclusionReason] = await this.experimentLevelExclusionInclusion(dpExpExists, userDoc); + return { + experiments: dpExpExists, + experimentId, + isUserExcluded, + isGroupExcluded, + exclusionReason, + }; + } + + // No experiment ID: mirror the full filtering and pooling pipeline from getAllExperimentConditions so that + // the experiment selected here is guaranteed to match what getAllExperimentConditions would return. + const validExperiments = await this.filterAndProcessGroupExperiments(allExperimentsAtDP, userDoc, logger); + const [filteredExperiments, exclusionReason] = await this.experimentLevelExclusionInclusion( + validExperiments, + userDoc ); - return [dpExperiments, dpExperiments.map((dp) => dp.experiment)]; + if (filteredExperiments.length === 0) { + return { experiments: [], experimentId: null, isUserExcluded, isGroupExcluded, exclusionReason }; + } + + const experimentIds = filteredExperiments.map((exp) => exp.id); + const [individualEnrollments, groupEnrollments, individualExclusions, groupExclusions] = + await this.getAssignmentsAndExclusionsForUser(userDoc, experimentIds); + + const selectedExperiments = this.processExperimentPools( + filteredExperiments, + individualEnrollments, + groupEnrollments, + individualExclusions, + groupExclusions, + userDoc, + previewUser + ); + + const experiments = selectedExperiments.length > 0 ? [selectedExperiments[0]] : []; + const resolvedExperimentId = experiments[0]?.id ?? null; + return { + experiments, + experimentId: resolvedExperimentId, + isUserExcluded, + isGroupExcluded, + exclusionReason, + }; } public async markExperimentPoint( @@ -155,6 +236,7 @@ export class ExperimentAssignmentService { status: MARKED_DECISION_POINT_STATUS | undefined, condition: string | null, logger: UpgradeLogger, + context: string, experimentId: string, target?: string, uniquifier?: string, @@ -180,56 +262,21 @@ export class ExperimentAssignmentService { const previewUser: PreviewUser = await this.previewUserService.findOneFromCache(userId, logger); const { workingGroup } = userDoc; - // 1. Search decision points in experiments cache and return relevant experiments and decisionPoints data - const experimentsResult = await this.getCachedExperiments(site, target); - const dpExperiments = experimentsResult[0]; - let experiments = experimentsResult[1]; - logger.info({ message: `markExperimentPoint: Site: ${site}, Target: ${target}, Condition: ${condition}, Status: "${status}" for User: ${userId}`, }); - if (experiments.length) { - if (experimentId) { - const dpExpExists = experiments.filter((exp) => exp.id === experimentId); - - if (!dpExpExists.length) { - const error = new Error( - `Experiment ID not provided for shared Decision Point in markExperimentPoint: ${userId}` - ); - (error as any).type = SERVER_ERROR.INVALID_EXPERIMENT_ID_FOR_SHARED_DECISIONPOINT; - (error as any).httpCode = 404; - logger.error(error); - throw error; - } - experiments = dpExpExists; - } else { - const random = seedrandom(userId)(); - experiments = [experiments[Math.floor(random * experiments.length)]]; - experimentId = experiments[0]?.id; - } - } - - // We are sure that either experiment length will not be greater than 1 - const context: string | null = experiments?.[0]?.context?.[0] ?? null; - - // 2. Check if user or group is globally excluded - const [isUserExcluded, isGroupExcluded] = await this.checkUserOrGroupIsGloballyExcluded(userDoc, context); - - // empty assignments if the user or group is excluded from the experiment - if (isUserExcluded || isGroupExcluded) { - experiments = []; - } - - const experimentIds = experiments.map((experiment) => experiment.id); - - // no experiments - if (experimentIds.length === 0) { - experiments = []; - } - - // 3. Check for experiment level inclusion and exclusion - const [, exclusionReason] = await this.experimentLevelExclusionInclusion(experiments, userDoc); + // 1-3. Resolve which experiment to mark, applying the same filtering and pooling logic as getAllExperimentConditions + // so that the two methods are guaranteed to agree on the selected experiment. + const { + experiments: resolvedExperiments, + experimentId: resolvedExperimentId, + isUserExcluded, + isGroupExcluded, + exclusionReason, + } = await this.resolveExperimentForMarkPoint(site, target, context, experimentId, userDoc, previewUser, logger); + const experiments = resolvedExperiments; + experimentId = resolvedExperimentId; // find monitored document let monitoredDocument: MonitoredDecisionPoint = await this.monitoredDecisionPointRepository.findOne({ @@ -246,7 +293,7 @@ export class ExperimentAssignmentService { } if (experiments.length) { - const selectedExperimentDP = dpExperiments.find((dp) => dp.experiment.id === experimentId); + const selectedExperimentDP = experiments[0]?.partitions.find((p) => p.site === site && p.target === target); const experiment = experiments[0]; const { conditions } = experiment; const payloadCondition = conditions.flatMap((condition) => condition.conditionPayloads); diff --git a/packages/backend/src/api/services/ExperimentService.ts b/packages/backend/src/api/services/ExperimentService.ts index 70e55ca0df..4b4cc62c8e 100644 --- a/packages/backend/src/api/services/ExperimentService.ts +++ b/packages/backend/src/api/services/ExperimentService.ts @@ -361,12 +361,7 @@ export class ExperimentService { const experiment = await this.experimentRepository.findOneExperiment(experimentId); if (experiment) { - await this.clearExperimentCacheDetail( - experiment.context[0], - experiment.partitions.map((partition) => { - return { site: partition.site, target: partition.target }; - }) - ); + await this.clearExperimentCacheDetail(experiment.context[0]); const deletedExperiment = await this.experimentRepository.deleteById(experimentId, transactionalEntityManager); @@ -478,12 +473,7 @@ export class ExperimentService { entityManager?: EntityManager ): Promise { const oldExperiment = await this.experimentRepository.findOneExperiment(experimentId); - await this.clearExperimentCacheDetail( - oldExperiment.context[0], - oldExperiment.partitions.map((partition) => { - return { site: partition.site, target: partition.target }; - }) - ); + await this.clearExperimentCacheDetail(oldExperiment.context[0]); state = EXPERIMENT_STATE_INTERNAL_NAME_OVERRIDES[state] || state; // Exclude the user only when the experiment is enrolling. For Preview state we don't need to exclude the user. The client need to provide explicit assignment for preview user to work correctly. @@ -753,12 +743,7 @@ export class ExperimentService { ): Promise { const entityManager = existingEntityManager || this.dataSource.manager; - await this.clearExperimentCacheDetail( - experiment.context[0], - experiment.partitions.map((partition) => { - return { site: partition.site, target: partition.target }; - }) - ); + await this.clearExperimentCacheDetail(experiment.context[0]); experiment.state = EXPERIMENT_STATE_INTERNAL_NAME_OVERRIDES[experiment.state] || experiment.state; // get old experiment document @@ -1301,12 +1286,7 @@ export class ExperimentService { logger: UpgradeLogger, entityManager: EntityManager ): Promise { - await this.clearExperimentCacheDetail( - experiment.context[0], - experiment.partitions.map((partition) => { - return { site: partition.site, target: partition.target }; - }) - ); + await this.clearExperimentCacheDetail(experiment.context[0]); const createdExperiment = await entityManager.transaction(async (transactionalEntityManager) => { experiment.id = experiment.id || crypto.randomUUID(); @@ -2500,16 +2480,8 @@ export class ExperimentService { }; } - private async clearExperimentCacheDetail( - context: string, - partitions: { site: string; target: string }[] - ): Promise { + private async clearExperimentCacheDetail(context: string): Promise { await this.cacheService.delCache(CACHE_PREFIX.EXPERIMENT_KEY_PREFIX + context); - const deletedCache = partitions.map(async (partition) => { - await this.cacheService.delCache(CACHE_PREFIX.MARK_KEY_PREFIX + '-' + partition.site + '-' + partition.target); - }); - await Promise.all(deletedCache); - return; } private async deleteAllListsFromExperiment( diff --git a/packages/backend/test/integration/UserNotDefined/index.ts b/packages/backend/test/integration/UserNotDefined/index.ts index 6eda67f2cc..f9756b5ee9 100644 --- a/packages/backend/test/integration/UserNotDefined/index.ts +++ b/packages/backend/test/integration/UserNotDefined/index.ts @@ -47,6 +47,7 @@ export const UserNotDefined = async () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, null, new UpgradeLogger(), + '', null ) ).rejects.toThrow(); From b1e33b8e9437a8fff892e58b3f7f806e70b5dba2 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Fri, 5 Jun 2026 16:16:00 -0400 Subject: [PATCH 02/10] update js client library --- clientlibs/js/jest.config.ts | 2 +- .../js/src/ApiService/ApiService.spec.ts | 80 +++++++++++++++++++ clientlibs/js/src/ApiService/ApiService.ts | 1 + clientlibs/js/src/types/requests.ts | 1 + docs/mark-context-refactor-plan.md | 2 +- 5 files changed, 84 insertions(+), 2 deletions(-) diff --git a/clientlibs/js/jest.config.ts b/clientlibs/js/jest.config.ts index 0781c40e5b..441f80a806 100644 --- a/clientlibs/js/jest.config.ts +++ b/clientlibs/js/jest.config.ts @@ -16,7 +16,7 @@ const config: Config.InitialOptions = { API_VERSION: 6, }, moduleNameMapper: { - upgrade_types: '/../../types', + upgrade_types: '/../../packages/types', }, coverageReporters: ['html'], }; diff --git a/clientlibs/js/src/ApiService/ApiService.spec.ts b/clientlibs/js/src/ApiService/ApiService.spec.ts index ab6e5e92fa..bd12d96479 100644 --- a/clientlibs/js/src/ApiService/ApiService.spec.ts +++ b/clientlibs/js/src/ApiService/ApiService.spec.ts @@ -318,6 +318,86 @@ describe('ApiService', () => { }); }); + describe('#markDecisionPoint', () => { + const expectedUrl = `${defaultConfig.hostURL}/api/${defaultConfig.apiVersion}/mark`; + const expectedOptions = { + headers: { + 'Content-Type': 'application/json', + 'Session-Id': 'testClientSessionId', + URL: expectedUrl, + 'User-Id': defaultConfig.userId, + Authorization: 'Bearer testToken', + }, + withCredentials: false, + }; + + const mockAssignment = { + site: 'testSite', + target: 'testTarget', + assignedCondition: [{ conditionCode: 'variant_x', experimentId: 'exp123' }], + assignedFactor: null, + experimentType: 'Simple', + }; + + beforeEach(() => { + MockDataService.findExperimentAssignmentBySiteAndTarget.mockReturnValue(mockAssignment); + // eslint-disable-next-line @typescript-eslint/no-empty-function + MockDataService.rotateAssignmentList.mockImplementation(() => {}); + }); + + it('should include context from config in the request body', async () => { + await apiService.markDecisionPoint({ + site: 'testSite', + target: 'testTarget', + condition: 'variant_x', + status: 'condition applied' as any, + }); + + const postedBody = mockHttpClient.doPost.mock.calls[mockHttpClient.doPost.mock.calls.length - 1][1]; + expect(postedBody.context).toEqual(defaultConfig.context); + }); + + it('should include status and data in the request body', async () => { + await apiService.markDecisionPoint({ + site: 'testSite', + target: 'testTarget', + condition: 'variant_x', + status: 'condition applied' as any, + }); + + const postedBody = mockHttpClient.doPost.mock.calls[mockHttpClient.doPost.mock.calls.length - 1][1]; + expect(postedBody.status).toEqual('condition applied'); + expect(postedBody.data.site).toEqual('testSite'); + expect(postedBody.data.target).toEqual('testTarget'); + }); + + it('should include uniquifier when provided', async () => { + await apiService.markDecisionPoint({ + site: 'testSite', + target: 'testTarget', + condition: 'variant_x', + status: 'condition applied' as any, + uniquifier: 'unique123', + }); + + const postedBody = mockHttpClient.doPost.mock.calls[mockHttpClient.doPost.mock.calls.length - 1][1]; + expect(postedBody.uniquifier).toEqual('unique123'); + }); + + it('should include clientError when provided', async () => { + await apiService.markDecisionPoint({ + site: 'testSite', + target: 'testTarget', + condition: 'variant_x', + status: 'condition not applied' as any, + clientError: 'variant not recognized', + }); + + const postedBody = mockHttpClient.doPost.mock.calls[mockHttpClient.doPost.mock.calls.length - 1][1]; + expect(postedBody.clientError).toEqual('variant not recognized'); + }); + }); + describe('#logCaliper', () => { const expectedUrl = `${defaultConfig.hostURL}/api/${defaultConfig.apiVersion}/log/caliper`; const expectedOptions = { diff --git a/clientlibs/js/src/ApiService/ApiService.ts b/clientlibs/js/src/ApiService/ApiService.ts index ef88627ab0..49c7e458ae 100644 --- a/clientlibs/js/src/ApiService/ApiService.ts +++ b/clientlibs/js/src/ApiService/ApiService.ts @@ -241,6 +241,7 @@ export default class ApiService { }; let requestBody: UpGradeClientRequests.IMarkDecisionPointRequestBody = { + context: this.context, status, data, }; diff --git a/clientlibs/js/src/types/requests.ts b/clientlibs/js/src/types/requests.ts index 6076b01fbc..0731e08173 100644 --- a/clientlibs/js/src/types/requests.ts +++ b/clientlibs/js/src/types/requests.ts @@ -35,6 +35,7 @@ export namespace UpGradeClientRequests { }; export interface IMarkDecisionPointRequestBody { + context: string; status: MARKED_DECISION_POINT_STATUS; data: { site: string; diff --git a/docs/mark-context-refactor-plan.md b/docs/mark-context-refactor-plan.md index f6293ad474..cb4b9715e5 100644 --- a/docs/mark-context-refactor-plan.md +++ b/docs/mark-context-refactor-plan.md @@ -92,7 +92,7 @@ This guarantees that mark and assign always agree on the selected experiment for --- -## Phase 5 — JS Client Library +## Phase 5 — JS Client Library ✅ COMPLETED **Files:** `UpGradeClient.types.ts`, `types/requests.ts`, `ApiService.ts`, `UpgradeClient.ts`, `Assignment.ts` From 5d7ac7bf52cd1c321fdb6d57bbc2dd1bec7285f7 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Fri, 5 Jun 2026 16:22:53 -0400 Subject: [PATCH 03/10] java client library --- .../client/ExperimentClient.java | 2 +- .../requestbeans/MarkExperimentRequest.java | 19 ++++++++++++++++--- docs/mark-context-refactor-plan.md | 2 +- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java b/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java index 2481c48ccb..9c03ee0cfb 100644 --- a/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java +++ b/clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java @@ -424,7 +424,7 @@ public void markDecisionPoint(MarkedDecisionPointStatus status, MarkExperimentRe public void markDecisionPoint(MarkedDecisionPointStatus status, MarkExperimentRequestData data, String clientError, String uniquifier, final ResponseCallback callbacks) { - MarkExperimentRequest markExperimentRequest = new MarkExperimentRequest(status, data, clientError, uniquifier); + MarkExperimentRequest markExperimentRequest = new MarkExperimentRequest(status, data, clientError, uniquifier, this.context); AsyncInvoker invocation = this.apiService.prepareRequest(MARK_EXPERIMENT_POINT); Entity requestContent = Entity.json(markExperimentRequest); diff --git a/clientlibs/java/src/main/java/org/upgradeplatform/requestbeans/MarkExperimentRequest.java b/clientlibs/java/src/main/java/org/upgradeplatform/requestbeans/MarkExperimentRequest.java index d02ba4c1b7..4c625740f6 100644 --- a/clientlibs/java/src/main/java/org/upgradeplatform/requestbeans/MarkExperimentRequest.java +++ b/clientlibs/java/src/main/java/org/upgradeplatform/requestbeans/MarkExperimentRequest.java @@ -8,28 +8,33 @@ public class MarkExperimentRequest { private MarkExperimentRequestData data; private String clientError; private String uniquifier; + private String context; public MarkExperimentRequest() {} - public MarkExperimentRequest(MarkedDecisionPointStatus status, MarkExperimentRequestData data) { + public MarkExperimentRequest(MarkedDecisionPointStatus status, MarkExperimentRequestData data, String context) { super(); this.status = status.toString(); this.data = data; + this.context = context; } - public MarkExperimentRequest(MarkedDecisionPointStatus status, MarkExperimentRequestData data, String clientError) { + public MarkExperimentRequest(MarkedDecisionPointStatus status, MarkExperimentRequestData data, String clientError, + String context) { super(); this.status = status.toString(); this.data = data; this.clientError = clientError; + this.context = context; } public MarkExperimentRequest(MarkedDecisionPointStatus status, MarkExperimentRequestData data, - String clientError, String uniquifier) { + String clientError, String uniquifier, String context) { this.status = status.toString(); this.data = data; this.clientError = clientError; this.uniquifier = uniquifier; + this.context = context; } public String getStatus() { @@ -63,4 +68,12 @@ public String getUniquifier() { public void setUniquifier(String uniquifier) { this.uniquifier = uniquifier; } + + public String getContext() { + return context; + } + + public void setContext(String context) { + this.context = context; + } } diff --git a/docs/mark-context-refactor-plan.md b/docs/mark-context-refactor-plan.md index cb4b9715e5..695151f6fb 100644 --- a/docs/mark-context-refactor-plan.md +++ b/docs/mark-context-refactor-plan.md @@ -104,7 +104,7 @@ This guarantees that mark and assign always agree on the selected experiment for --- -## Phase 6 — Java Client Library +## Phase 6 — Java Client Library ✅ COMPLETED **Files:** `MarkExperimentRequest.java`, `ExperimentClient.java` From 4eb7b24d79c103ec94dd1cbdb88c700d43915a8c Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Fri, 5 Jun 2026 18:06:30 -0400 Subject: [PATCH 04/10] fix existing integration tests --- .../experimentContext/ExperimentContextAssignments.ts | 6 ++++-- .../Experiment/markExperimentPoint/NoExperiment.ts | 3 ++- .../Experiment/stratification/MetricQueriesCheck.ts | 7 +++++++ .../Experiment/withinSubject/MetricQueriesCheck.ts | 7 +++++++ packages/backend/test/integration/utils/index.ts | 2 ++ 5 files changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/backend/test/integration/Experiment/experimentContext/ExperimentContextAssignments.ts b/packages/backend/test/integration/Experiment/experimentContext/ExperimentContextAssignments.ts index 585b3d5587..5b3c82e481 100644 --- a/packages/backend/test/integration/Experiment/experimentContext/ExperimentContextAssignments.ts +++ b/packages/backend/test/integration/Experiment/experimentContext/ExperimentContextAssignments.ts @@ -114,7 +114,8 @@ export default async function testCase(): Promise { experimentPoint1, condition1, firstExperiment.id, - new UpgradeLogger() + new UpgradeLogger(), + context1 ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[1].id, experimentName1, experimentPoint1); @@ -136,7 +137,8 @@ export default async function testCase(): Promise { experimentPoint2, condition2, secondExperimentCreated.id, - new UpgradeLogger() + new UpgradeLogger(), + context2 ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[1].id, experimentName2, experimentPoint2); diff --git a/packages/backend/test/integration/Experiment/markExperimentPoint/NoExperiment.ts b/packages/backend/test/integration/Experiment/markExperimentPoint/NoExperiment.ts index 6e56ebffdd..3a05ab3312 100644 --- a/packages/backend/test/integration/Experiment/markExperimentPoint/NoExperiment.ts +++ b/packages/backend/test/integration/Experiment/markExperimentPoint/NoExperiment.ts @@ -35,7 +35,8 @@ export default async function NoExperiment(): Promise { experimentPoint, condition, experimentID, - new UpgradeLogger() + new UpgradeLogger(), + experimentObject.context[0] ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentName, experimentPoint); diff --git a/packages/backend/test/integration/Experiment/stratification/MetricQueriesCheck.ts b/packages/backend/test/integration/Experiment/stratification/MetricQueriesCheck.ts index b34d39a6f6..32505013e5 100644 --- a/packages/backend/test/integration/Experiment/stratification/MetricQueriesCheck.ts +++ b/packages/backend/test/integration/Experiment/stratification/MetricQueriesCheck.ts @@ -440,6 +440,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '1' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -479,6 +480,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '2' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -517,6 +519,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '3' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -556,6 +559,7 @@ export default async function MetricQueriesCheck(): Promise { condition2, experimentId, new UpgradeLogger(), + 'home', '4' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -595,6 +599,7 @@ export default async function MetricQueriesCheck(): Promise { condition2, experimentId, new UpgradeLogger(), + 'home', '5' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -633,6 +638,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '6' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[1].id, experimentTarget, experimentPoint); @@ -672,6 +678,7 @@ export default async function MetricQueriesCheck(): Promise { condition2, experimentId, new UpgradeLogger(), + 'home', '7' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[1].id, experimentTarget, experimentPoint); diff --git a/packages/backend/test/integration/Experiment/withinSubject/MetricQueriesCheck.ts b/packages/backend/test/integration/Experiment/withinSubject/MetricQueriesCheck.ts index 1cd12a93f3..49efaef54c 100644 --- a/packages/backend/test/integration/Experiment/withinSubject/MetricQueriesCheck.ts +++ b/packages/backend/test/integration/Experiment/withinSubject/MetricQueriesCheck.ts @@ -436,6 +436,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '1' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -475,6 +476,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '2' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -513,6 +515,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '3' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -552,6 +555,7 @@ export default async function MetricQueriesCheck(): Promise { condition2, experimentId, new UpgradeLogger(), + 'home', '4' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -591,6 +595,7 @@ export default async function MetricQueriesCheck(): Promise { condition2, experimentId, new UpgradeLogger(), + 'home', '5' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[0].id, experimentTarget, experimentPoint); @@ -629,6 +634,7 @@ export default async function MetricQueriesCheck(): Promise { condition, experimentId, new UpgradeLogger(), + 'home', '6' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[1].id, experimentTarget, experimentPoint); @@ -668,6 +674,7 @@ export default async function MetricQueriesCheck(): Promise { condition2, experimentId, new UpgradeLogger(), + 'home', '7' ); checkMarkExperimentPointForUser(markedExperimentPoint, experimentUsers[1].id, experimentTarget, experimentPoint); diff --git a/packages/backend/test/integration/utils/index.ts b/packages/backend/test/integration/utils/index.ts index ef6abe894e..2d274260ff 100644 --- a/packages/backend/test/integration/utils/index.ts +++ b/packages/backend/test/integration/utils/index.ts @@ -105,6 +105,7 @@ export async function markExperimentPoint( condition: string | null, experimentId: string, logger: UpgradeLogger, + context = 'home', uniquifier?: string ): Promise { const experimentAssignmentService = Container.get(ExperimentAssignmentService); @@ -119,6 +120,7 @@ export async function markExperimentPoint( MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, logger, + context, experimentId, target, uniquifier From cc4132f4d3bdf1981fe68645d7d3c3fc65f3cc73 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Mon, 8 Jun 2026 10:38:25 -0400 Subject: [PATCH 05/10] Phase 7a - state filter integration tests --- docs/mark-context-refactor-plan.md | 2 +- .../CancelledExperimentStateFilter.ts | 81 +++++++++++++++++++ .../Experiment/markExperimentPoint/index.ts | 6 ++ .../backend/test/integration/index.test.ts | 6 +- .../backend/test/integration/utils/index.ts | 2 +- 5 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 packages/backend/test/integration/Experiment/markExperimentPoint/CancelledExperimentStateFilter.ts diff --git a/docs/mark-context-refactor-plan.md b/docs/mark-context-refactor-plan.md index 695151f6fb..7957ed648c 100644 --- a/docs/mark-context-refactor-plan.md +++ b/docs/mark-context-refactor-plan.md @@ -122,7 +122,7 @@ This guarantees that mark and assign always agree on the selected experiment for ### New tests to add -#### 7a. State filter — testable against the unfixed code (demonstrates bug, then verifies fix) +#### 7a. State filter — testable against the unfixed code (demonstrates bug, then verifies fix) ✅ COMPLETED This is the cleanest test to write before Phases 1–4 are complete, since it doesn't require the `context` wire changes to observe the behavioral difference. diff --git a/packages/backend/test/integration/Experiment/markExperimentPoint/CancelledExperimentStateFilter.ts b/packages/backend/test/integration/Experiment/markExperimentPoint/CancelledExperimentStateFilter.ts new file mode 100644 index 0000000000..7d650ace2c --- /dev/null +++ b/packages/backend/test/integration/Experiment/markExperimentPoint/CancelledExperimentStateFilter.ts @@ -0,0 +1,81 @@ +import { individualAssignmentExperiment } from '../../mockData/experiment/index'; +import { ExperimentService } from '../../../../src/api/services/ExperimentService'; +import { Container } from 'typedi'; +import { UserService } from '../../../../src/api/services/UserService'; +import { systemUser } from '../../mockData/user'; +import { experimentUsers } from '../../mockData/experimentUsers'; +import { EXPERIMENT_STATE } from 'upgrade_types'; +import { getAllExperimentCondition, markExperimentPoint } from '../../utils'; +import { UpgradeLogger } from '../../../../src/lib/logger/UpgradeLogger'; +import { CheckService } from '../../../../src/api/services/CheckService'; + +/** + * Phase 7a — State filter test + * + * Verifies that getCachedValidExperiments (used by resolveExperimentForMarkPoint) filters out + * CANCELLED experiments. Before this fix, DecisionPointRepository.find() returned experiments + * in any state, so a cancelled experiment could appear in the mark result and get recorded as + * the experimentId on the MonitoredDecisionPoint. After the fix, only ENROLLING / + * ENROLLMENT_COMPLETE experiments are considered, so marking at a decision point with no active + * experiment records a null experimentId and creates no individual enrollment. + */ +export default async function CancelledExperimentStateFilter(): Promise { + const experimentService = Container.get(ExperimentService); + const userService = Container.get(UserService); + const checkService = Container.get(CheckService); + + const user = await userService.upsertUser(systemUser as any, new UpgradeLogger()); + + // Use a fresh clone so mutations don't bleed into other tests + const experimentObject = structuredClone(individualAssignmentExperiment); + const context = experimentObject.context[0]; + const site = experimentObject.partitions[0].site; + const target = experimentObject.partitions[0].target; + + // 1. Create experiment and transition to ENROLLING (RUNNING). + // experimentUsers[1] (student2) is used for assign checks — student1 is excluded by the mock + // experiment's segmentExclusion rule, so student2 is the first non-excluded user. + await experimentService.create(experimentObject as any, user, new UpgradeLogger()); + let experiments = await experimentService.find(new UpgradeLogger()); + const experimentId = experiments[0].id; + + await experimentService.updateState(experimentId, EXPERIMENT_STATE.RUNNING, user, new UpgradeLogger()); + + experiments = await experimentService.find(new UpgradeLogger()); + // ENROLLING is stored internally; find() applies the display-name override → RUNNING + expect(experiments[0].state).toEqual(EXPERIMENT_STATE.RUNNING); + + // Confirm the experiment is visible to the assign path while active + const conditionsWhileActive = await getAllExperimentCondition(experimentUsers[1].id, new UpgradeLogger(), context); + expect(conditionsWhileActive).toEqual(expect.arrayContaining([expect.objectContaining({ site, target })])); + + // 2. Cancel the experiment — updateState clears the experiment cache for the context. + // CANCELLED has no internal-name override, so it is stored as-is. + // find() applies the display-name override CANCELLED → COMPLETED. + await experimentService.updateState(experimentId, EXPERIMENT_STATE.CANCELLED, user, new UpgradeLogger()); + + experiments = await experimentService.find(new UpgradeLogger()); + expect(experiments[0].state).toEqual(EXPERIMENT_STATE.COMPLETED); + + // Confirm the assign path no longer sees the experiment after cancellation + const conditionsAfterCancel = await getAllExperimentCondition(experimentUsers[1].id, new UpgradeLogger(), context); + expect(conditionsAfterCancel).not.toEqual(expect.arrayContaining([expect.objectContaining({ site, target })])); + + // 3. Mark the decision point without an experimentId so the service resolves it. + // getCachedValidExperiments only returns ENROLLING / ENROLLMENT_COMPLETE experiments, so the + // cancelled experiment is not found and the monitored point records no experiment reference. + await markExperimentPoint(experimentUsers[1].id, target, site, null, null, new UpgradeLogger(), context); + + // No individual enrollment should be created — the cancelled experiment was filtered out + const individualAssignments = await checkService.getAllIndividualAssignment(); + expect(individualAssignments.length).toEqual(0); + + // The monitored decision point is still written (mark always logs), + // but with no experiment linked because the state filter excluded the cancelled experiment + const markedPoints = await checkService.getAllMarkedExperimentPoints(); + expect(markedPoints.length).toBeGreaterThan(0); + + const markedPoint = markedPoints.find((mp) => mp.site === site && mp.target === target); + expect(markedPoint).toBeDefined(); + expect(markedPoint.experimentId).toBeNull(); +} diff --git a/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts b/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts index c80bb15850..b6ffef76dd 100644 --- a/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts +++ b/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts @@ -3,6 +3,7 @@ import { experimentUsers } from '../../mockData/experimentUsers/index'; import { ExperimentUserService } from '../../../../src/api/services/ExperimentUserService'; import { CheckService } from '../../../../src/api/services/CheckService'; import TestCase1 from './NoExperiment'; +import TestCase2 from './CancelledExperimentStateFilter'; import { UpgradeLogger } from '../../../../src/lib/logger/UpgradeLogger'; const initialChecks = async () => { @@ -43,3 +44,8 @@ export const NoExperiment = async () => { await initialChecks(); await TestCase1(); }; + +export const CancelledExperimentStateFilter = async () => { + await initialChecks(); + await TestCase2(); +}; diff --git a/packages/backend/test/integration/index.test.ts b/packages/backend/test/integration/index.test.ts index 6679633430..7cb65268fb 100644 --- a/packages/backend/test/integration/index.test.ts +++ b/packages/backend/test/integration/index.test.ts @@ -33,7 +33,7 @@ import { import { MainAuditLog } from './Experiment/auditLogs'; import { NoPartitionPoint } from './Experiment/onlyExperimentPoint'; // import { StatsGroupExperiment } from './ExperimentStats'; -import { NoExperiment } from './Experiment/markExperimentPoint'; +import { NoExperiment, CancelledExperimentStateFilter } from './Experiment/markExperimentPoint'; import { NoPreviewUser, PreviewAssignments, @@ -165,6 +165,10 @@ describe('Integration Tests', () => { return NoExperiment(); }); + test('Mark Experiment with cancelled experiment is excluded by state filter', () => { + return CancelledExperimentStateFilter(); + }); + test('No Group for Experiment', () => { return NoGroup(); }); diff --git a/packages/backend/test/integration/utils/index.ts b/packages/backend/test/integration/utils/index.ts index 2d274260ff..9a21ebac27 100644 --- a/packages/backend/test/integration/utils/index.ts +++ b/packages/backend/test/integration/utils/index.ts @@ -103,7 +103,7 @@ export async function markExperimentPoint( target: string, site: string, condition: string | null, - experimentId: string, + experimentId: string | null, logger: UpgradeLogger, context = 'home', uniquifier?: string From d116f3a9a0fa57f0e464b72a91debfe6db00bce1 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Mon, 8 Jun 2026 11:45:57 -0400 Subject: [PATCH 06/10] Phase 7b - context contamination integration tests --- docs/mark-context-refactor-plan.md | 6 +- .../ContextContamination.ts | 135 ++++++++++++++++++ .../Experiment/markExperimentPoint/index.ts | 6 + .../backend/test/integration/index.test.ts | 6 +- 4 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 packages/backend/test/integration/Experiment/markExperimentPoint/ContextContamination.ts diff --git a/docs/mark-context-refactor-plan.md b/docs/mark-context-refactor-plan.md index 7957ed648c..fc232e3d9e 100644 --- a/docs/mark-context-refactor-plan.md +++ b/docs/mark-context-refactor-plan.md @@ -137,7 +137,7 @@ This is the cleanest test to write before Phases 1–4 are complete, since it do --- -#### 7b. Context contamination — testable once Phase 1 adds `context` to the wire format +#### 7b. Context contamination — testable once Phase 1 adds `context` to the wire format ✅ COMPLETED **Setup:** @@ -146,8 +146,8 @@ This is the cleanest test to write before Phases 1–4 are complete, since it do 3. Mark user 1 with `context="math"` at `site="homepage"`, `target="button"` 4. Mark user 2 with `context="science"` at the same `site`/`target` -**Before fix:** Both marks hit the same `MARK_KEY_PREFIX-homepage-button` cache entry, so one of the users gets the wrong experiment's data in their monitored decision point record. -**After fix:** Each mark uses `EXPERIMENT_KEY_PREFIX-{context}`, so user 1's mark references experiment A and user 2's references experiment B. +**Before fix:** `DecisionPointRepository.find({ where: { site, target } })` has no context filter, so both experiments A and B are included in the pool for any mark at that decision point. With no basis to exclude the wrong experiment, a user marking in context "math" may have experiment B selected — regardless of whether the result was cached or freshly queried. The cache makes this wrong behavior more consistent but is not the root cause. +**After fix:** `getCachedValidExperiments(context)` scopes the fetch to the given context before any filtering, so experiment B is never in the pool when marking for context "math". User 1's mark correctly references experiment A and user 2's references experiment B. --- diff --git a/packages/backend/test/integration/Experiment/markExperimentPoint/ContextContamination.ts b/packages/backend/test/integration/Experiment/markExperimentPoint/ContextContamination.ts new file mode 100644 index 0000000000..570b94afe1 --- /dev/null +++ b/packages/backend/test/integration/Experiment/markExperimentPoint/ContextContamination.ts @@ -0,0 +1,135 @@ +import { individualAssignmentExperiment } from '../../mockData/experiment/index'; +import { ExperimentService } from '../../../../src/api/services/ExperimentService'; +import { Container } from 'typedi'; +import { UserService } from '../../../../src/api/services/UserService'; +import { systemUser } from '../../mockData/user'; +import { experimentUsers } from '../../mockData/experimentUsers'; +import { EXPERIMENT_STATE } from 'upgrade_types'; +import { getAllExperimentCondition, markExperimentPoint } from '../../utils'; +import { UpgradeLogger } from '../../../../src/lib/logger/UpgradeLogger'; +import { CheckService } from '../../../../src/api/services/CheckService'; + +/** + * Phase 7b — Context contamination test + * + * Verifies that getCachedValidExperiments(context) scopes the experiment pool to the given + * context before any filtering. Before this fix, DecisionPointRepository.find({ where: { site, target } }) + * had no context dimension — both experiments A and B would appear in the pool for any mark at the + * shared decision point, and the wrong experiment could be selected. After the fix, the context + * parameter is first-class: marking in context "math" only sees experiment A, and marking in + * context "science" only sees experiment B. + * + * Setup: + * Experiment A — context="math", site="homepage", target="button" + * Experiment B — context="science", site="homepage", target="button" + * + * Assert: + * user1 marks in context "math" → monitored point experimentId === experiment A's id + * user2 marks in context "science" → monitored point experimentId === experiment B's id + */ +export default async function ContextContamination(): Promise { + const experimentService = Container.get(ExperimentService); + const userService = Container.get(UserService); + const checkService = Container.get(CheckService); + + const user = await userService.upsertUser(systemUser as any, new UpgradeLogger()); + + const contextMath = 'add'; + const contextScience = 'mul'; + // Both experiments share the same decision point — this is what makes contamination possible + // without context-scoped caching. + const sharedSite = 'homepage'; + const sharedTarget = 'button'; + + // Experiment A — context "math" + const experimentA = structuredClone(individualAssignmentExperiment); + experimentA.id = 'aabb0000-0000-0000-0000-000000000001'; + experimentA.context = [contextMath]; + experimentA.partitions = [ + { + ...experimentA.partitions[0], + id: 'aabb0000-0000-0000-0000-000000000002', + site: sharedSite, + target: sharedTarget, + twoCharacterId: 'MA', + }, + ]; + experimentA.conditions = [ + { ...experimentA.conditions[0], id: 'aabb0000-0000-0000-0000-000000000003' }, + { ...experimentA.conditions[1], id: 'aabb0000-0000-0000-0000-000000000004' }, + ]; + experimentA.conditionPayloads = []; + + // Experiment B — context "science", same site/target as A + const experimentB = structuredClone(individualAssignmentExperiment); + experimentB.id = 'bbcc0000-0000-0000-0000-000000000001'; + experimentB.context = [contextScience]; + experimentB.partitions = [ + { + ...experimentB.partitions[0], + id: 'bbcc0000-0000-0000-0000-000000000002', + site: sharedSite, + target: sharedTarget, + twoCharacterId: 'SC', + }, + ]; + experimentB.conditions = [ + { ...experimentB.conditions[0], id: 'bbcc0000-0000-0000-0000-000000000005' }, + { ...experimentB.conditions[1], id: 'bbcc0000-0000-0000-0000-000000000006' }, + ]; + experimentB.conditionPayloads = []; + + // 1. Create and activate both experiments + await experimentService.create(experimentA as any, user, new UpgradeLogger()); + await experimentService.create(experimentB as any, user, new UpgradeLogger()); + + await experimentService.updateState(experimentA.id, EXPERIMENT_STATE.RUNNING, user, new UpgradeLogger()); + await experimentService.updateState(experimentB.id, EXPERIMENT_STATE.RUNNING, user, new UpgradeLogger()); + + // 2. Assign each user in their respective context so the cache is warm and enrollments exist. + // experimentUsers[1] = student2 (student1 is excluded by the mock experiment's segmentExclusion rule). + // experimentUsers[2] = student3 + const conditionsMath = await getAllExperimentCondition(experimentUsers[1].id, new UpgradeLogger(), contextMath); + expect(conditionsMath.filter((c) => c.site === sharedSite && c.target === sharedTarget).length).toEqual(1); + + const conditionsScience = await getAllExperimentCondition(experimentUsers[2].id, new UpgradeLogger(), contextScience); + expect(conditionsScience.filter((c) => c.site === sharedSite && c.target === sharedTarget).length).toEqual(1); + + // 3. Mark user1 in context "math" — resolveExperimentForMarkPoint must return only experiment A. + // Pass null for condition and experimentId so the service resolves the experiment by context/site/target. + await markExperimentPoint( + experimentUsers[1].id, + sharedTarget, + sharedSite, + null, + null, + new UpgradeLogger(), + contextMath + ); + + // The monitored decision point for user1 must reference experiment A, not experiment B. + let markedPoints = await checkService.getAllMarkedExperimentPoints(); + const markForUser1 = markedPoints.find( + (mp) => mp.site === sharedSite && mp.target === sharedTarget && mp.user.id === experimentUsers[1].id + ); + expect(markForUser1).toBeDefined(); + expect(markForUser1.experimentId).toEqual(experimentA.id); + + // 4. Mark user2 in context "science" — resolveExperimentForMarkPoint must return only experiment B. + await markExperimentPoint( + experimentUsers[2].id, + sharedTarget, + sharedSite, + null, + null, + new UpgradeLogger(), + contextScience + ); + + markedPoints = await checkService.getAllMarkedExperimentPoints(); + const markForUser2 = markedPoints.find( + (mp) => mp.site === sharedSite && mp.target === sharedTarget && mp.user.id === experimentUsers[2].id + ); + expect(markForUser2).toBeDefined(); + expect(markForUser2.experimentId).toEqual(experimentB.id); +} diff --git a/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts b/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts index b6ffef76dd..a7822f22e1 100644 --- a/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts +++ b/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts @@ -4,6 +4,7 @@ import { ExperimentUserService } from '../../../../src/api/services/ExperimentUs import { CheckService } from '../../../../src/api/services/CheckService'; import TestCase1 from './NoExperiment'; import TestCase2 from './CancelledExperimentStateFilter'; +import TestCase3 from './ContextContamination'; import { UpgradeLogger } from '../../../../src/lib/logger/UpgradeLogger'; const initialChecks = async () => { @@ -49,3 +50,8 @@ export const CancelledExperimentStateFilter = async () => { await initialChecks(); await TestCase2(); }; + +export const ContextContamination = async () => { + await initialChecks(); + await TestCase3(); +}; diff --git a/packages/backend/test/integration/index.test.ts b/packages/backend/test/integration/index.test.ts index 7cb65268fb..0422f25381 100644 --- a/packages/backend/test/integration/index.test.ts +++ b/packages/backend/test/integration/index.test.ts @@ -33,7 +33,7 @@ import { import { MainAuditLog } from './Experiment/auditLogs'; import { NoPartitionPoint } from './Experiment/onlyExperimentPoint'; // import { StatsGroupExperiment } from './ExperimentStats'; -import { NoExperiment, CancelledExperimentStateFilter } from './Experiment/markExperimentPoint'; +import { NoExperiment, CancelledExperimentStateFilter, ContextContamination } from './Experiment/markExperimentPoint'; import { NoPreviewUser, PreviewAssignments, @@ -169,6 +169,10 @@ describe('Integration Tests', () => { return CancelledExperimentStateFilter(); }); + test('Mark Experiment scopes experiment pool to context — no cross-context contamination', () => { + return ContextContamination(); + }); + test('No Group for Experiment', () => { return NoGroup(); }); From 6f8d1d2c36b2cbc7c6b22675c62cc57ad6d878ce Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Tue, 9 Jun 2026 16:49:08 -0400 Subject: [PATCH 07/10] remove non-useful test --- docs/mark-context-refactor-plan.md | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/docs/mark-context-refactor-plan.md b/docs/mark-context-refactor-plan.md index fc232e3d9e..7354372956 100644 --- a/docs/mark-context-refactor-plan.md +++ b/docs/mark-context-refactor-plan.md @@ -151,20 +151,6 @@ This is the cleanest test to write before Phases 1–4 are complete, since it do --- -#### 7c. Pooling regression — verifies correct behavior is preserved (Phase 0) - -Since pooling consolidation is already complete, this test documents the now-correct behavior and would catch a regression if the code were reverted. - -**Setup:** - -1. Create two experiments in the same pool at `site`/`target`, where pool assignment logic selects experiment A for user 1 -2. Call `getAllExperimentConditions` for user 1 → verify experiment A is assigned -3. Call `markExperimentPoint` for user 1 at the same `site`/`target` with no `experimentId` - -**Assert:** The mark record's `experimentId` matches the experiment returned by assign (experiment A). Under the old code, mark used ad-hoc filtering rather than `processExperimentPools` and could select a different experiment. - ---- - ## Order Dependencies Phases 1–4 are purely backend and can be done together. Phase 5 (JS) and Phase 6 (Java) depend on Phases 1–4 being finalized so the wire format is stable before client changes are made. Phase 7 follows Phases 1–4. From c901f733eea9eed6bf2bf5e974895d8f4da21448 Mon Sep 17 00:00:00 2001 From: Ben Blanchard Date: Thu, 18 Jun 2026 14:16:11 -0400 Subject: [PATCH 08/10] null experimentId for non-mark monitored points Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../backend/src/api/services/ExperimentAssignmentService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index c174f06ef3..7312bc2f6f 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -158,7 +158,7 @@ export class ExperimentAssignmentService { if (allExperimentsAtDP.length === 0) { return { experiments: [], - experimentId, + experimentId: null, isUserExcluded: false, isGroupExcluded: false, exclusionReason: [], From 39fc6c1a296c0a25bd20e49c9c8198d625491439 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Thu, 18 Jun 2026 14:40:52 -0400 Subject: [PATCH 09/10] add unit tests for resolveExperimentForMarkPoint --- .../ExperimentAssignmentService.test.ts | 187 ++++++++++++++---- 1 file changed, 154 insertions(+), 33 deletions(-) diff --git a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts index 28588d8d21..9b526b0806 100644 --- a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts @@ -1080,6 +1080,7 @@ describe('Experiment Assignment Service Test', () => { target, condition, loggerMock, + 'context', undefined, undefined, undefined, @@ -1113,7 +1114,7 @@ describe('Experiment Assignment Service Test', () => { findOne: sandbox.stub().resolves(monitoredDocument), }; - testedModule.cacheService.wrap = sandbox.stub().resolves([]); + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([]); testedModule.monitoredDecisionPointRepository = monitoredDecisionPointRepositoryMock; testedModule.experimentService.getPayloadAndFactorialObject = sandbox .stub() @@ -1125,6 +1126,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.NO_CONDITION_ASSIGNED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1193,6 +1195,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1258,6 +1261,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1324,6 +1328,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1403,23 +1408,6 @@ describe('Experiment Assignment Service Test', () => { expect(result).toMatchObject(mergedLog); }); - it('[getCachedExperiments] should return empty experiment for particular site-target if no experiment is cached', async () => { - const site = 'CurriculumSequence'; - const target = 'W1'; - - const result = await testedModule.getCachedExperiments(site, target); - expect(result[0]).toEqual([]); - }); - - it('[getCachedExperiments] should return cached experiment for particular site-target', async () => { - const site = 'CurriculumSequence'; - const target = 'W1'; - testedModule.cacheService.wrap = sandbox.stub().resolves([simpleIndividualAssignmentExperiment]); - - const result = await testedModule.getCachedExperiments(site, target); - expect(result[0]).toEqual([simpleIndividualAssignmentExperiment]); - }); - it('[saveGroupExclusionDoc] should not be called if [experimentLevelExclusionInclusion] call returns empty exclusions', async () => { const userId = 'user123'; const userDoc = { id: userId, group: { schoolId: ['school1'] }, workingGroup: {} }; @@ -1454,6 +1442,7 @@ describe('Experiment Assignment Service Test', () => { testedModule.decisionPointRepository = decisionPointRepositoryMock; testedModule.monitoredDecisionPointRepository = monitoredDecisionPointRepositoryMock; + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([]); const markResult = await testedModule.markExperimentPoint( { id: userId }, @@ -1461,6 +1450,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1498,6 +1488,9 @@ describe('Experiment Assignment Service Test', () => { testedModule.decisionPointRepository = decisionPointRepositoryMock; testedModule.monitoredDecisionPointRepository = monitoredDecisionPointRepositoryMock; + testedModule.experimentService.getCachedValidExperiments = sandbox + .stub() + .resolves([simpleIndividualAssignmentExperiment]); const markResult = await testedModule.markExperimentPoint( { id: userId }, @@ -1505,6 +1498,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1520,14 +1514,6 @@ describe('Experiment Assignment Service Test', () => { const condition = 'Shape=Circle; Color=Red'; factorialIndividualExperiment.state = 'inactive'; - const modifiedPartitions = factorialIndividualExperiment.partitions.map((partition) => ({ - ...partition, - experiment: factorialIndividualExperiment, // Adding experiment key - })); - - testedModule.cacheService.wrap = sandbox.stub().resolves([factorialIndividualExperiment]); - testedModule.getCachedExperiments = sandbox.stub().resolves([modifiedPartitions, [factorialIndividualExperiment]]); - testedModule.updateEnrollmentExclusionDocumentsAndCheckEndingCriteria = sandbox.stub().resolves([]); const monitoredDocument = { @@ -1548,6 +1534,8 @@ describe('Experiment Assignment Service Test', () => { }; testedModule.experimentService = experimentServiceMock; + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([factorialIndividualExperiment]); + testedModule.experimentLevelExclusionInclusion = sandbox.stub().resolves([[factorialIndividualExperiment], []]); testedModule.previewUserService = previewUserServiceMock; testedModule.monitoredDecisionPointRepository = monitoredDecisionPointRepositoryMock; @@ -1557,6 +1545,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', factorialIndividualExperiment.id, target, undefined @@ -1571,13 +1560,6 @@ describe('Experiment Assignment Service Test', () => { const target = 'color_shape'; const condition = 'Shape=Circle; Color=Red'; factorialIndividualExperiment.state = 'enrolling'; - const modifiedPartitions = factorialIndividualExperiment.partitions.map((partition) => ({ - ...partition, - experiment: factorialIndividualExperiment, // Adding experiment key - })); - - testedModule.cacheService.wrap = sandbox.stub().resolves([factorialIndividualExperiment]); - testedModule.getCachedExperiments = sandbox.stub().resolves([modifiedPartitions, [factorialIndividualExperiment]]); testedModule.updateEnrollmentExclusionDocumentsAndCheckEndingCriteria = sandbox.stub().resolves([]); @@ -1597,6 +1579,8 @@ describe('Experiment Assignment Service Test', () => { }; testedModule.experimentService = experimentServiceMock; + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([factorialIndividualExperiment]); + testedModule.experimentLevelExclusionInclusion = sandbox.stub().resolves([[factorialIndividualExperiment], []]); testedModule.monitoredDecisionPointRepository = monitoredDecisionPointRepositoryMock; const markResult = await testedModule.markExperimentPoint( @@ -1605,6 +1589,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', factorialIndividualExperiment.id, target, undefined @@ -1613,6 +1598,142 @@ describe('Experiment Assignment Service Test', () => { sinon.assert.calledOnce(testedModule.updateEnrollmentExclusionDocumentsAndCheckEndingCriteria); }); + describe('[resolveExperimentForMarkPoint]', () => { + const site = 'CurriculumSequence'; + const target = 'W1'; + const context = 'context'; + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + it('should return empty result when no experiments exist at the given site and target', async () => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([]); + + const result = await (testedModule as any).resolveExperimentForMarkPoint( + site, + target, + context, + undefined, + userDoc, + undefined, + loggerMock + ); + + expect(result.experiments).toEqual([]); + expect(result.experimentId).toBeNull(); + expect(result.isUserExcluded).toBe(false); + expect(result.isGroupExcluded).toBe(false); + expect(result.exclusionReason).toEqual([]); + }); + + it('should return excluded flags and empty experiments when user is globally excluded', async () => { + const exp = structuredClone(simpleIndividualAssignmentExperiment); + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.checkUserOrGroupIsGloballyExcluded = sandbox.stub().resolves([true, false]); + + const result = await (testedModule as any).resolveExperimentForMarkPoint( + site, + target, + context, + undefined, + userDoc, + undefined, + loggerMock + ); + + expect(result.experiments).toEqual([]); + expect(result.isUserExcluded).toBe(true); + expect(result.isGroupExcluded).toBe(false); + }); + + it('should return the experiment when a valid experimentId is provided', async () => { + const exp = structuredClone(simpleIndividualAssignmentExperiment); + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentLevelExclusionInclusion = sandbox.stub().resolves([[exp], []]); + + const result = await (testedModule as any).resolveExperimentForMarkPoint( + site, + target, + context, + exp.id, + userDoc, + undefined, + loggerMock + ); + + expect(result.experiments).toEqual([exp]); + expect(result.experimentId).toEqual(exp.id); + expect(result.isUserExcluded).toBe(false); + expect(result.isGroupExcluded).toBe(false); + expect(result.exclusionReason).toEqual([]); + }); + + it('should throw when the provided experimentId is not found at the decision point', async () => { + const exp = structuredClone(simpleIndividualAssignmentExperiment); + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + + await expect( + (testedModule as any).resolveExperimentForMarkPoint( + site, + target, + context, + 'non-existent-id', + userDoc, + undefined, + loggerMock + ) + ).rejects.toThrow(); + + sinon.assert.calledOnce(loggerMock.error); + }); + + it('should run the full pipeline and return the selected experiment when no experimentId is provided', async () => { + const exp = structuredClone(simpleIndividualAssignmentExperiment); + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.filterAndProcessGroupExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentLevelExclusionInclusion = sandbox.stub().resolves([[exp], []]); + testedModule.getAssignmentsAndExclusionsForUser = sandbox.stub().resolves([[], [], [], []]); + testedModule.processExperimentPools = sandbox.stub().returns([exp]); + + const result = await (testedModule as any).resolveExperimentForMarkPoint( + site, + target, + context, + undefined, + userDoc, + undefined, + loggerMock + ); + + expect(result.experiments).toEqual([exp]); + expect(result.experimentId).toEqual(exp.id); + expect(result.isUserExcluded).toBe(false); + expect(result.isGroupExcluded).toBe(false); + expect(result.exclusionReason).toEqual([]); + }); + + it('should return empty experiments and an exclusion reason when user is excluded at experiment level', async () => { + const exp = structuredClone(simpleIndividualAssignmentExperiment); + const exclusion = { experiment: exp, reason: 'group', matchedGroup: true }; + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.filterAndProcessGroupExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentLevelExclusionInclusion = sandbox.stub().resolves([[], [exclusion]]); + + const result = await (testedModule as any).resolveExperimentForMarkPoint( + site, + target, + context, + undefined, + userDoc, + undefined, + loggerMock + ); + + expect(result.experiments).toEqual([]); + expect(result.experimentId).toBeNull(); + expect(result.exclusionReason).toHaveLength(1); + expect(result.exclusionReason[0]).toEqual(exclusion); + }); + }); + describe('getBatchExperimentConditions', () => { it('should return empty object if there are no user docs and check [getBatchExperimentConditions] function', async () => { const userDocs = []; From 5bbd084b5a5cc5ac55bee10d16a03d6329741145 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Fri, 10 Jul 2026 15:46:27 -0400 Subject: [PATCH 10/10] consolidate mark and assign logic further --- .../services/ExperimentAssignmentService.ts | 157 ++++++++++-------- 1 file changed, 90 insertions(+), 67 deletions(-) diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index fc747adddc..42806e2a88 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -127,13 +127,87 @@ export class ExperimentAssignmentService { ) {} /** - * Shared experiment-selection logic used by both markExperimentPoint and (indirectly) getAllExperimentConditions. + * Runs the shared experiment-selection pipeline: group-experiment filtering → + * enrollment/exclusion queries → preview-assignment merging → + * experiment-level inclusion/exclusion → pool selection. + * + * Used by both getAllExperimentConditions and resolveExperimentForMarkPoint so the + * two code paths are guaranteed to agree on which experiment is selected. + */ + private async selectExperimentsForUser( + experiments: Experiment[], + userDoc: RequestedExperimentUser, + previewUser: PreviewUser | null, + logger: UpgradeLogger + ): Promise<{ + selectedExperiments: Experiment[]; + exclusionReason: { experiment: Experiment; reason: string; matchedGroup: boolean }[]; + mergedIndividualEnrollments: IndividualEnrollment[]; + groupEnrollments: GroupEnrollment[]; + individualExclusions: IndividualExclusion[]; + groupExclusions: GroupExclusion[]; + }> { + const validExperiments = await this.filterAndProcessGroupExperiments(experiments, userDoc, logger); + + if (validExperiments.length === 0) { + return { + selectedExperiments: [], + exclusionReason: [], + mergedIndividualEnrollments: [], + groupEnrollments: [], + individualExclusions: [], + groupExclusions: [], + }; + } + + const experimentIds = validExperiments.map((experiment) => experiment.id); + const [individualEnrollments, groupEnrollments, individualExclusions, groupExclusions] = + await this.getAssignmentsAndExclusionsForUser(userDoc, experimentIds); + + let mergedIndividualEnrollments = individualEnrollments; + if (previewUser && previewUser.assignments) { + const previewAssignment: IndividualEnrollment[] = previewUser.assignments.map((assignment) => { + return { + user: userDoc, + condition: assignment.experimentCondition, + ...assignment, + } as any; // any is used because we don't have decisionPoint in the preview assignment + }); + mergedIndividualEnrollments = [...previewAssignment, ...mergedIndividualEnrollments]; + } + + const [filteredExperiments, exclusionReason] = await this.experimentLevelExclusionInclusion( + validExperiments, + userDoc + ); + + const selectedExperiments = this.processExperimentPools( + filteredExperiments, + mergedIndividualEnrollments, + groupEnrollments, + individualExclusions, + groupExclusions, + userDoc, + previewUser + ); + + return { + selectedExperiments, + exclusionReason, + mergedIndividualEnrollments, + groupEnrollments, + individualExclusions, + groupExclusions, + }; + } + + /** + * Shared experiment-selection logic used by markExperimentPoint. * * When experimentId is supplied the method validates it and returns that single experiment after running - * exclusion checks. When experimentId is absent it applies the full filtering and pooling pipeline - * (global exclusion → group-experiment filtering → experiment-level inclusion/exclusion → processExperimentPools) - * so the selected experiment is guaranteed to match what getAllExperimentConditions would have returned for - * the same user and decision point. + * exclusion checks. When experimentId is absent it delegates to selectExperimentsForUser so the selected + * experiment is guaranteed to match what getAllExperimentConditions would return for the same user and + * decision point. */ private async resolveExperimentForMarkPoint( site: string, @@ -176,7 +250,7 @@ export class ExperimentAssignmentService { const dpExpExists = allExperimentsAtDP.filter((exp) => exp.id === experimentId); if (!dpExpExists.length) { const error = new Error( - `Experiment ID not provided for shared Decision Point in markExperimentPoint: ${userDoc.id}` + `Experiment ID not found among valid experiments at this decision point in markExperimentPoint: ${userDoc.id}` ); (error as any).type = SERVER_ERROR.INVALID_EXPERIMENT_ID_FOR_SHARED_DECISIONPOINT; (error as any).httpCode = 404; @@ -193,30 +267,12 @@ export class ExperimentAssignmentService { }; } - // No experiment ID: mirror the full filtering and pooling pipeline from getAllExperimentConditions so that - // the experiment selected here is guaranteed to match what getAllExperimentConditions would return. - const validExperiments = await this.filterAndProcessGroupExperiments(allExperimentsAtDP, userDoc, logger); - const [filteredExperiments, exclusionReason] = await this.experimentLevelExclusionInclusion( - validExperiments, - userDoc - ); - - if (filteredExperiments.length === 0) { - return { experiments: [], experimentId: null, isUserExcluded, isGroupExcluded, exclusionReason }; - } - - const experimentIds = filteredExperiments.map((exp) => exp.id); - const [individualEnrollments, groupEnrollments, individualExclusions, groupExclusions] = - await this.getAssignmentsAndExclusionsForUser(userDoc, experimentIds); - - const selectedExperiments = this.processExperimentPools( - filteredExperiments, - individualEnrollments, - groupEnrollments, - individualExclusions, - groupExclusions, + // No experiment ID: delegate to the shared selection pipeline so this method always agrees with getAllExperimentConditions. + const { selectedExperiments, exclusionReason } = await this.selectExperimentsForUser( + allExperimentsAtDP, userDoc, - previewUser + previewUser, + logger ); const experiments = selectedExperiments.length > 0 ? [selectedExperiments[0]] : []; @@ -395,48 +451,15 @@ export class ExperimentAssignmentService { return []; } - // 3. Filter out valid group experiments that doesn't have invalid group/workingGroup which are not enrolled yet - const validExperiments = await this.filterAndProcessGroupExperiments(experiments, experimentUserDoc, logger); - - // 4. Process assignments and exclusions + // 3-5. Filter group experiments, check exclusions, and select from pools via the shared pipeline try { - // return empty assignments if there are no valid experiments - if (validExperiments.length === 0) { - return []; - } - - const experimentIds = validExperiments.map((experiment) => experiment.id); - - // Query assignments and exclusions for the user - const [individualEnrollments, groupEnrollments, individualExclusions, groupExclusions] = - await this.getAssignmentsAndExclusionsForUser(experimentUserDoc, experimentIds); - - let mergedIndividualAssignment = individualEnrollments; - // add assignments for individual assignments if preview user - if (previewUser && previewUser.assignments) { - const previewAssignment: IndividualEnrollment[] = previewUser.assignments.map((assignment) => { - return { - user: experimentUserDoc, - condition: assignment.experimentCondition, - ...assignment, - } as any; // any is used because we don't have decisionPoint in the preview assignment - }); - mergedIndividualAssignment = [...previewAssignment, ...mergedIndividualAssignment]; - } - - // Check for experiment level inclusion and exclusion and return valid inclusion experiments - let [filteredExperiments] = await this.experimentLevelExclusionInclusion(validExperiments, experimentUserDoc); - - // 5. Process experiment pools on filtered experiments - filteredExperiments = this.processExperimentPools( - filteredExperiments, - mergedIndividualAssignment, + const { + selectedExperiments: filteredExperiments, + mergedIndividualEnrollments: mergedIndividualAssignment, groupEnrollments, individualExclusions, groupExclusions, - experimentUserDoc, - previewUser - ); + } = await this.selectExperimentsForUser(experiments, experimentUserDoc, previewUser, logger); // return empty if no experiments if (filteredExperiments.length === 0) {