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/clientlibs/js/src/ApiService/ApiService.spec.ts b/clientlibs/js/src/ApiService/ApiService.spec.ts index cb42ebf3e5..7746711579 100644 --- a/clientlibs/js/src/ApiService/ApiService.spec.ts +++ b/clientlibs/js/src/ApiService/ApiService.spec.ts @@ -409,6 +409,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 ae88f72df9..bf6efd923f 100644 --- a/clientlibs/js/src/ApiService/ApiService.ts +++ b/clientlibs/js/src/ApiService/ApiService.ts @@ -240,6 +240,7 @@ export default class ApiService { }; let requestBody: UpGradeClientRequests.IMarkDecisionPointRequestBody = { + context: this.context, status, data, }; diff --git a/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts b/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts index 0f366061e1..363ba8385d 100644 --- a/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts +++ b/clientlibs/js/src/UpGradeClient/generateUUID.spec.ts @@ -83,4 +83,3 @@ describe('generateUUID (via constructor clientSessionId)', () => { expect(sessionId).toBe(providedId); }); }); - diff --git a/clientlibs/js/src/types/requests.ts b/clientlibs/js/src/types/requests.ts index b085faca0c..87efddfcb5 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 new file mode 100644 index 0000000000..7354372956 --- /dev/null +++ b/docs/mark-context-refactor-plan.md @@ -0,0 +1,156 @@ +# 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 ✅ COMPLETED + +**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 ✅ COMPLETED + +**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) ✅ 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. + +**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 ✅ COMPLETED + +**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:** `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. + +--- + +## 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 c3f4fc34e3..f260d2a72b 100644 --- a/packages/backend/src/api/controllers/ExperimentClientController.v5.ts +++ b/packages/backend/src/api/controllers/ExperimentClientController.v5.ts @@ -481,6 +481,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 a7e47a989a..5a83108d0a 100644 --- a/packages/backend/src/api/controllers/ExperimentClientController.v6.ts +++ b/packages/backend/src/api/controllers/ExperimentClientController.v6.ts @@ -451,6 +451,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 ccf36078e6..46d91955aa 100644 --- a/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v5.ts +++ b/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v5.ts @@ -35,6 +35,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 41907e7e69..b80d0046fa 100644 --- a/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v6.ts +++ b/packages/backend/src/api/controllers/validators/MarkExperimentValidator.v6.ts @@ -32,6 +32,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 b91baa414a..42806e2a88 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,35 +126,164 @@ 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: { - conditions: { - conditionPayloads: true, - }, + /** + * 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); - partitions: true, + if (validExperiments.length === 0) { + return { + selectedExperiments: [], + exclusionReason: [], + mergedIndividualEnrollments: [], + groupEnrollments: [], + individualExclusions: [], + groupExclusions: [], + }; + } - experimentSegmentInclusion: { - segment: true, - }, + const experimentIds = validExperiments.map((experiment) => experiment.id); + const [individualEnrollments, groupEnrollments, individualExclusions, groupExclusions] = + await this.getAssignmentsAndExclusionsForUser(userDoc, experimentIds); - experimentSegmentExclusion: { - segment: true, - }, - }, - }, - }) + 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 ); - return [dpExperiments, dpExperiments.map((dp) => dp.experiment)]; + 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 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, + 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: null, + 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 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; + logger.error(error); + throw error; + } + const [, exclusionReason] = await this.experimentLevelExclusionInclusion(dpExpExists, userDoc); + return { + experiments: dpExpExists, + experimentId, + isUserExcluded, + isGroupExcluded, + exclusionReason, + }; + } + + // 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, + logger + ); + + const experiments = selectedExperiments.length > 0 ? [selectedExperiments[0]] : []; + const resolvedExperimentId = experiments[0]?.id ?? null; + return { + experiments, + experimentId: resolvedExperimentId, + isUserExcluded, + isGroupExcluded, + exclusionReason, + }; } public async markExperimentPoint( @@ -164,6 +292,7 @@ export class ExperimentAssignmentService { status: MARKED_DECISION_POINT_STATUS | undefined, condition: string | null, logger: UpgradeLogger, + context: string, experimentId: string, target = '', uniquifier?: string, @@ -189,56 +318,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({ @@ -255,7 +349,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); @@ -357,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) { diff --git a/packages/backend/src/api/services/ExperimentService.ts b/packages/backend/src/api/services/ExperimentService.ts index a397b2a53a..f9edeb31ca 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); @@ -467,12 +462,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. @@ -726,12 +716,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 @@ -1231,12 +1216,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(); @@ -2431,16 +2411,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/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/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/ContextContamination.ts b/packages/backend/test/integration/Experiment/markExperimentPoint/ContextContamination.ts new file mode 100644 index 0000000000..c13d7b92ae --- /dev/null +++ b/packages/backend/test/integration/Experiment/markExperimentPoint/ContextContamination.ts @@ -0,0 +1,133 @@ +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, + }, + ]; + 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, + }, + ]; + 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/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/markExperimentPoint/index.ts b/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts index c80bb15850..a7822f22e1 100644 --- a/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts +++ b/packages/backend/test/integration/Experiment/markExperimentPoint/index.ts @@ -3,6 +3,8 @@ 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 TestCase3 from './ContextContamination'; import { UpgradeLogger } from '../../../../src/lib/logger/UpgradeLogger'; const initialChecks = async () => { @@ -43,3 +45,13 @@ export const NoExperiment = async () => { await initialChecks(); await TestCase1(); }; + +export const CancelledExperimentStateFilter = async () => { + await initialChecks(); + await TestCase2(); +}; + +export const ContextContamination = async () => { + await initialChecks(); + await TestCase3(); +}; 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/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(); diff --git a/packages/backend/test/integration/index.test.ts b/packages/backend/test/integration/index.test.ts index 6679633430..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 } from './Experiment/markExperimentPoint'; +import { NoExperiment, CancelledExperimentStateFilter, ContextContamination } from './Experiment/markExperimentPoint'; import { NoPreviewUser, PreviewAssignments, @@ -165,6 +165,14 @@ describe('Integration Tests', () => { return NoExperiment(); }); + test('Mark Experiment with cancelled experiment is excluded by state filter', () => { + return CancelledExperimentStateFilter(); + }); + + test('Mark Experiment scopes experiment pool to context — no cross-context contamination', () => { + return ContextContamination(); + }); + 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 ef6abe894e..9a21ebac27 100644 --- a/packages/backend/test/integration/utils/index.ts +++ b/packages/backend/test/integration/utils/index.ts @@ -103,8 +103,9 @@ export async function markExperimentPoint( target: string, site: string, condition: string | null, - experimentId: string, + experimentId: string | null, 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 diff --git a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts index a0d5d7d789..f1403d76a3 100644 --- a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts @@ -1120,6 +1120,7 @@ describe('Experiment Assignment Service Test', () => { target, condition, loggerMock, + 'context', undefined, undefined, undefined, @@ -1153,7 +1154,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() @@ -1165,6 +1166,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.NO_CONDITION_ASSIGNED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1233,6 +1235,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1298,6 +1301,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1364,6 +1368,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1443,23 +1448,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: {} }; @@ -1494,6 +1482,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 }, @@ -1501,6 +1490,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1538,6 +1528,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 }, @@ -1545,6 +1538,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', undefined, target, undefined @@ -1560,14 +1554,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 = { @@ -1588,6 +1574,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; @@ -1597,6 +1585,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', factorialIndividualExperiment.id, target, undefined @@ -1611,13 +1600,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([]); @@ -1637,6 +1619,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( @@ -1645,6 +1629,7 @@ describe('Experiment Assignment Service Test', () => { MARKED_DECISION_POINT_STATUS.CONDITION_APPLIED, condition, loggerMock, + 'context', factorialIndividualExperiment.id, target, undefined @@ -1653,6 +1638,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 = [];