Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clientlibs/python/src/upgrade_client_lib/api_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions clientlibs/python/src/upgrade_client_lib/types/requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class MarkDecisionPointData(BaseModel):


class MarkDecisionPointRequest(BaseModel):
context: str
status: MarkedDecisionPointStatus
data: MarkDecisionPointData
uniquifier: str | None = None
Expand Down
7 changes: 6 additions & 1 deletion clientlibs/python/tests/test_api_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -290,14 +291,18 @@ 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",
condition_code="control",
status=MarkedDecisionPointStatus.CONDITION_APPLIED,
)
assert result.userId == USER_ID
import json

body = json.loads(route.calls[0].request.content)
assert body["context"] == CONTEXT


# ---------------------------------------------------------------------------
Expand Down
20 changes: 20 additions & 0 deletions clientlibs/python/tests/test_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,19 +117,23 @@ 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",
target="banner",
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",
Expand All @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,17 +146,21 @@ export class ExperimentAssignmentService {
logger: UpgradeLogger
): Promise<{
selectedExperiments: Experiment[];
invalidGroupExperiments: Experiment[];
exclusionReason: { experiment: Experiment; reason: string; matchedGroup: boolean }[];
mergedIndividualEnrollments: IndividualEnrollment[];
groupEnrollments: GroupEnrollment[];
individualExclusions: IndividualExclusion[];
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: [],
Expand Down Expand Up @@ -198,6 +202,7 @@ export class ExperimentAssignmentService {

return {
selectedExperiments,
invalidGroupExperiments,
exclusionReason,
mergedIndividualEnrollments,
groupEnrollments,
Expand Down Expand Up @@ -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,
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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';
Expand Down