CarnegieLearningWeb / UpGrade

Framework for adding A/B testing to education applications
https://www.upgradeplatform.org/
BSD 3-Clause "New" or "Revised" License
25 stars 13 forks source link

Different Response Types for Similar Data #1755

Open kawcl opened 1 month ago

kawcl commented 1 month ago

I'm currently implementing Upgrade using the Javascript client-library. I'm running into a bit of headache with the different response types for getDecisionPointAssignment and getAllExperimentConditions. The data coming from both are very similar, if not the same, but it is difficult to convert between the two.

It looks like you could convert from an IExperimentAssignmentv5 object to an Assignment object but for my implementation I don't have use for the apiService code within Assignment. I don't want to convert to an Assignment object because I don't want that service code available with how I'm writing my implementation, it would disrupt the pattern I'm creating for api calls. Otherwise trying to convert from an Assignment object to a IExperimentAssignmentv5 object doesn't seem possible.

Thoughts to improve this situation might be to have a class that has the Assignment data but without the service code and then another class that extends that first class and includes the service code there. Otherwise provide a way to convert to an IExperimentAssignmentv5 object from an Assignment object or maybe even consolidate the responses into just one.

danoswaltCL commented 1 month ago

So the way I'm thinking about this is that Assignment is best kept as a simple read-only DTO and the mark method should be removed.

getAllExperimentConditions should return an array of Assignments that are created as soon as they are fetched, so end-users will only ever deal with simple Assignments.

getDecisionPointAssignment should always return an Assignment also even when null, currently we return null but it would be better to instantiate an Assignmentwith a null condition to we can always be dealing with an Assignment.

An additional benefit is that we are removing versioned data structure implementation details from the client library consumer's code, as the versioned IExperimentAssignmentV5 raw response can be kept private to the internals of the client.

One of the original reasons for allowing the user to do the Assignment.markDecisionPoint() was because Client.markDecisionPoint() required explicitly passing the assignment details of site, target, and condition. If we are refactoring in the above way, we get the same benefit: Client.markDecisionPoint(assignment, options) is pretty clear, recalling that a null assignment can be accomplished the same way, with an Assignment object that has a null condition.

I don't know if there are other use cases for needing to keep Assignment.mark as an option other than flexibility for the end-user. I think the end user is going to be better served by keeping the usage straightforward client as the API interface and Assignment as a read-only DTO.

(I created an imaginary options object there to encapsulate the status, clientError, and uniquifier values, but that is also a refactor that is necessary here, an options object would be way simpler all around).

kawcl commented 1 month ago

I think this makes sense to me. It will resolve the issue that I was having since you are consolidating the response types. I like the change as well because the Client.markDecisionPoint can take that assignment object and all the information you would need is present. As per removing the mark call in the Assignment object, I can't think of a use case where it would be that much more beneficial over the client call. At least in my experience with implementing the library I haven't used it or found it more convenient. I typically like to create a consistent pattern for how api calls get made and that mark call in the assignment object is just a little atypical and breaks pattern from the rest of the calls.