Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

add assign mooclet proxy to assignment service #2169

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

danoswaltCL
Copy link
Collaborator

This adds assignment proxy to experimentAssignmentService for Mooclet experiments.

Experiments that are created with a Mooclet assignment algorithm will generate two entities in the db to map the relationship between the Upgrade Experiment entities and its Mooclet counterparts, "MoocletExperimentRef" and "MoocletVersionConditionMap", which is joined to the MoocletExperimentRef document as an array of direct relationships between Mooclet "versions" and UpGrade "conditions".

On /assign, if a user has not already been enrolled in the mooclet experiment they have been assigned, a call will be proxied to the Mooclet API to be given a new "version" (condition).

When fetching a new version, the MoocletExperimentRef will be fetched via UpGrade experimentId to find the condition that this version is mapped to, which is what is returned in the response as normal Upgrade condition assignment.

When this assignment is marked, the user will be enrolled as normal in the UpGrade experiment and subsequent calls to /assign will not reach out to the Mooclet server.

Note! the assignment retrieved from Mooclet for ts_configurable is not constant like it is in upgrade, you could get any of the versions involved on each call until it is marked, so this is something we should discuss.

@danoswaltCL danoswaltCL marked this pull request as ready for review January 23, 2025 23:26
@danoswaltCL danoswaltCL changed the title [WIP] add assign mooclet proxy to assignment service add assign mooclet proxy to assignment service Jan 23, 2025
@danoswaltCL
Copy link
Collaborator Author

@bcb37 this is ready for review but I am asking the Mooclet team folks if sending the "learner" is needed when getting condition. I can see that it will add null for learner id in the value table when not sending it, and it adds the learner id when i do, but I can't tell if it actually affects anything (for ts_configurable).

private assignRandom(
experiment: Experiment,
user: ExperimentUser,
enrollmentCount?: { conditionId: string; userCount: number }[]
): ExperimentCondition {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to keep the return type here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes probably, i can add that back, not sure why removed

private async getConditionFromMoocletProxy(experiment: Experiment, user: ExperimentUser) {
const userId = user.id;

const condition = await this.moocletExperimentService.getConditionFromMoocletProxy(experiment, userId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to create this variable just to return it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have to, I guess I like to be explicit vs saving a couple of characters... it's also slightly easier to debug later if need be, all the data is assembled there in the local scope to spy or log out.


// public async getVersionForNewLearner(moocletId: number, userId: string) {
// const endpoint = `/mooclet/${moocletId}/run?learner=${userId}`;
public async getVersionForNewLearner(moocletId: number, userId: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the return type (Promise<MoocletVersionResponseDetails>) inferred here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can add a tyep.

Copy link
Collaborator

@bcb37 bcb37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok to me. Just a couple questions.

@danoswaltCL
Copy link
Collaborator Author

Got a response and it seems that we not only should send the learner for this /mooclet/:id/run call, but also when sending a "reward", which is not something I was testing. I will need to check that out next week to see what the affect is, but I think we do in fact want what is currently coded up after all.

@danoswaltCL danoswaltCL merged commit 8913713 into dev Jan 29, 2025
14 of 15 checks passed
@danoswaltCL danoswaltCL deleted the feature/add-assign-mooclet-proxy branch January 29, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants