diff --git a/clientlibs/python/src/upgrade_client_lib/api_service.py b/clientlibs/python/src/upgrade_client_lib/api_service.py index 51a6f266a..c4df53702 100644 --- a/clientlibs/python/src/upgrade_client_lib/api_service.py +++ b/clientlibs/python/src/upgrade_client_lib/api_service.py @@ -228,8 +228,8 @@ def mark_decision_point_sync( data = self._post_sync("mark", body) return MarkDecisionPointResponse.model_validate(data) - @staticmethod def _build_mark_body( + self, site: str, target: str | None, condition_code: str, @@ -257,7 +257,7 @@ def _build_mark_body( if assigned_factor is not None: mark_data["assignedFactor"] = assigned_factor - body: dict[str, Any] = {"status": status.value, "data": mark_data} + body: dict[str, Any] = {"context": self._context, "status": status.value, "data": mark_data} if uniquifier is not None: body["uniquifier"] = uniquifier if client_error is not None: diff --git a/clientlibs/python/src/upgrade_client_lib/types/requests.py b/clientlibs/python/src/upgrade_client_lib/types/requests.py index e76dda61a..70fcc93f7 100644 --- a/clientlibs/python/src/upgrade_client_lib/types/requests.py +++ b/clientlibs/python/src/upgrade_client_lib/types/requests.py @@ -42,6 +42,7 @@ class MarkDecisionPointData(BaseModel): class MarkDecisionPointRequest(BaseModel): + context: str status: MarkedDecisionPointStatus data: MarkDecisionPointData uniquifier: str | None = None diff --git a/clientlibs/python/tests/test_api_service.py b/clientlibs/python/tests/test_api_service.py index 77fabab29..cef3837cd 100644 --- a/clientlibs/python/tests/test_api_service.py +++ b/clientlibs/python/tests/test_api_service.py @@ -248,6 +248,7 @@ async def test_async_full(self) -> None: import json body = json.loads(route.calls[0].request.content) + assert body["context"] == CONTEXT assert body["status"] == "condition applied" assert body["data"]["site"] == "home" assert body["data"]["experimentType"] == "Simple" @@ -290,7 +291,7 @@ async def test_optional_fields_omitted_when_none(self) -> None: @respx.mock def test_sync(self) -> None: - respx.post(f"{BASE}/mark").mock(return_value=Response(200, json=MARK_PAYLOAD)) + route = respx.post(f"{BASE}/mark").mock(return_value=Response(200, json=MARK_PAYLOAD)) result = make_service().mark_decision_point_sync( site="home", target="banner", @@ -298,6 +299,10 @@ def test_sync(self) -> None: status=MarkedDecisionPointStatus.CONDITION_APPLIED, ) assert result.userId == USER_ID + import json + + body = json.loads(route.calls[0].request.content) + assert body["context"] == CONTEXT # --------------------------------------------------------------------------- diff --git a/clientlibs/python/tests/test_types.py b/clientlibs/python/tests/test_types.py index 4c225093c..4cf6aa471 100644 --- a/clientlibs/python/tests/test_types.py +++ b/clientlibs/python/tests/test_types.py @@ -117,6 +117,7 @@ def test_valid(self) -> None: class TestMarkDecisionPointRequest: def test_valid_minimal(self) -> None: req = MarkDecisionPointRequest( + context="my-app", status=MarkedDecisionPointStatus.CONDITION_APPLIED, data=MarkDecisionPointData( site="home", @@ -124,12 +125,15 @@ def test_valid_minimal(self) -> None: assignedCondition=MarkDecisionPointCondition(conditionCode="control"), ), ) + assert req.context == "my-app" + assert req.model_dump(mode="json")["context"] == "my-app" assert req.status == MarkedDecisionPointStatus.CONDITION_APPLIED assert req.uniquifier is None assert req.clientError is None def test_valid_full(self) -> None: req = MarkDecisionPointRequest( + context="my-app", status=MarkedDecisionPointStatus.CONDITION_APPLIED, data=MarkDecisionPointData( site="home", @@ -142,9 +146,25 @@ def test_valid_full(self) -> None: assert req.data.assignedCondition.experimentId == "exp-123" assert req.uniquifier == "abc" + def test_missing_context(self) -> None: + with pytest.raises(ValidationError) as exc_info: + MarkDecisionPointRequest.model_validate( + { + "status": "condition applied", + "data": { + "site": "home", + "target": "banner", + "assignedCondition": {"conditionCode": "control"}, + }, + } + ) + assert exc_info.value.errors()[0]["loc"] == ("context",) + assert exc_info.value.errors()[0]["type"] == "missing" + def test_invalid_status(self) -> None: with pytest.raises(ValidationError): MarkDecisionPointRequest( + context="my-app", status="not-a-status", # type: ignore[arg-type] data=MarkDecisionPointData( site="home", diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index 5f1c4a365..ba0f33067 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -146,6 +146,7 @@ export class ExperimentAssignmentService { logger: UpgradeLogger ): Promise<{ selectedExperiments: Experiment[]; + invalidGroupExperiments: Experiment[]; exclusionReason: { experiment: Experiment; reason: string; matchedGroup: boolean }[]; mergedIndividualEnrollments: IndividualEnrollment[]; groupEnrollments: GroupEnrollment[]; @@ -153,10 +154,13 @@ export class ExperimentAssignmentService { groupExclusions: GroupExclusion[]; }> { const validExperiments = await this.filterAndProcessGroupExperiments(experiments, userDoc, logger); + const validExperimentIds = new Set(validExperiments.map((experiment) => experiment.id)); + const invalidGroupExperiments = experiments.filter((experiment) => !validExperimentIds.has(experiment.id)); if (validExperiments.length === 0) { return { selectedExperiments: [], + invalidGroupExperiments, exclusionReason: [], mergedIndividualEnrollments: [], groupEnrollments: [], @@ -198,6 +202,7 @@ export class ExperimentAssignmentService { return { selectedExperiments, + invalidGroupExperiments, exclusionReason, mergedIndividualEnrollments, groupEnrollments, @@ -274,7 +279,7 @@ export class ExperimentAssignmentService { } // No experiment ID: pool the full context before matching the marked decision point. - const { selectedExperiments, exclusionReason } = await this.selectExperimentsForUser( + const { selectedExperiments, invalidGroupExperiments, exclusionReason } = await this.selectExperimentsForUser( allExperimentsForContext, userDoc, previewUser, @@ -285,6 +290,7 @@ export class ExperimentAssignmentService { const experiment = selectedExperiments.find((selected) => experimentIdsAtDP.has(selected.id)) ?? exclusionReason.find((reason) => experimentIdsAtDP.has(reason.experiment?.id))?.experiment ?? + invalidGroupExperiments.find((invalid) => experimentIdsAtDP.has(invalid.id)) ?? null; const resolvedExperimentId = experiment?.id ?? null; const resolvedExclusionReason = exclusionReason.filter((reason) => reason.experiment?.id === resolvedExperimentId); diff --git a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts index 3e736c8e3..8b8153d78 100644 --- a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts @@ -33,7 +33,9 @@ import { import { GroupEnrollment } from '../../../src/api/models/GroupEnrollment'; import { ASSIGNMENT_ALGORITHM, + CONSISTENCY_RULE, ENROLLMENT_CODE, + EXCLUSION_CODE, EXPERIMENT_STATE, FILTER_MODE, MARKED_DECISION_POINT_STATUS, @@ -2037,6 +2039,60 @@ describe('Experiment Assignment Service Test', () => { sinon.assert.calledOnce(testedModule.updateEnrollmentExclusionDocumentsAndCheckEndingCriteria); }); + describe('/mark records invalid-group exclusions', () => { + it.each([ + [ + 'invalid working group', + { schoolId: ['school1'] }, + { schoolId: 'school2' }, + EXCLUSION_CODE.INVALID_GROUP_OR_WORKING_GROUP, + ], + ['missing working group', { schoolId: ['school1'] }, {}, EXCLUSION_CODE.NO_GROUP_SPECIFIED], + ['missing group data', {}, { schoolId: 'school1' }, EXCLUSION_CODE.NO_GROUP_SPECIFIED], + ])('records %s with and without an experiment ID', async (_name, group, workingGroup, exclusionCode) => { + const exp = structuredClone(simpleGroupAssignmentExperiment); + exp.state = EXPERIMENT_STATE.ENROLLING; + exp.consistencyRule = CONSISTENCY_RULE.GROUP; + exp.group = 'schoolId'; + exp.enrollmentCompleteCondition = null; + const { site, target } = exp.partitions[0]; + const userDoc = { id: 'invalid-group-user', group, workingGroup }; + + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.checkUserOrGroupIsGloballyExcluded = sandbox.stub().resolves([false, false]); + testedModule.experimentLevelExclusionInclusion = sandbox.stub().callsFake(async (exps) => [exps, []]); + groupEnrollmentRepositoryMock.findOne = sandbox.stub().resolves(undefined); + groupExclusionRepositoryMock.findOne = sandbox.stub().resolves(undefined); + groupEnrollmentRepositoryMock.save = sandbox.stub(); + individualEnrollmentRepositoryMock.save = sandbox.stub(); + testedModule.monitoredDecisionPointRepository = { + findOne: sandbox.stub().resolves(undefined), + saveRawJson: sandbox.stub().callsFake(async (document) => document), + }; + + for (const experimentId of [undefined, exp.id]) { + individualExclusionRepositoryMock.saveRawJson.resetHistory(); + await testedModule.markExperimentPoint( + userDoc, + site, + MARKED_DECISION_POINT_STATUS.NO_CONDITION_ASSIGNED, + null, + loggerMock, + 'context', + experimentId, + target + ); + + sinon.assert.calledOnce(individualExclusionRepositoryMock.saveRawJson); + expect(individualExclusionRepositoryMock.saveRawJson.firstCall.args[0]).toEqual([ + { user: userDoc, experiment: exp, exclusionCode }, + ]); + sinon.assert.notCalled(groupEnrollmentRepositoryMock.save); + sinon.assert.notCalled(individualEnrollmentRepositoryMock.save); + } + }); + }); + describe('[resolveExperimentForMarkPoint]', () => { const site = 'CurriculumSequence'; const target = 'W1';