-
Notifications
You must be signed in to change notification settings - Fork 2
Change v2 req_tree to not return root node, improve OpenAPI spec #3010
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
Conversation
| course = serializers.CharField(source="course_id", allow_null=True, default=None) | ||
| program = serializers.CharField(source="program_id", required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API currently returns integers for this.
I believe we are only using this serializer in read-only viewsets (only v2 ProgramViewSet?), and iirc DRF does not run validation on response data, only request data, which is why it hasn't caused runtime problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Switching to IntegerField fixes the openapi spec, too)
courses/serializers/v2/programs.py
Outdated
| ProgramRequirementNodeType.OPERATOR, | ||
| ProgramRequirementNodeType.COURSE, | ||
| ) | ||
| choices=[x.value for x in ProgramRequirementNodeType] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are four node types:
class ProgramRequirementNodeType(models.TextChoices):
PROGRAM_ROOT = "program_root", "Program Root" # Present since beginning
OPERATOR = "operator", "Operator" # Present since beginning
COURSE = "course", "Course" # Present since beginning
PROGRAM = "program", "Program" # Recently AddedI think PROGRAM should have been added to the serializer in #2772.
PROGRAM_ROOT has been excluded since requirements were first added.
@rhysyngsun Do you recall why PROGRAM_ROOT is excluded? I can understand we might not want people creating / adding new PROGRAM_ROOT nodes via a (not currently exposed) creation/update API. Is that the reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(My goal here for the OpenAPI spec accuracy; currently spec thinks req_tree can only have course and operator nodes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PROGRAM_ROOT would never show up, it's just an anchor node for the root of the tree in the database, it's basically a pointer to the program itself. So yeah, it shouldn't be in the API schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does show up in the API response, e.g., https://mitxonline.mit.edu/api/v2/programs/1/, at the root:
{
"id": 1,
"title": "Data, Economics, and Design of Policy: International Development",
"readable_id": "program-v1:MITx+DEDP",
"req_tree": [
{
"data": {
"node_type": "program_root",
"operator": null,
"operator_value": null,
"program": 1,
"course": null,
"required_program": null,
"title": "",
"elective_flag": false
},
"id": 1,
"children": [
{
"data": {
"node_type": "operator",
"operator": "all_of",
"operator_value": null,
"program": 1,
"course": null,
"required_program": null,
"title": "Required Courses",
"elective_flag": false
},
"id": 2,
"children": [
{
"data": {
"node_type": "course",
"operator": null,
"operator_value": null,
"program": 1,
"course": 1,
"required_program": null,
"title": null,
"elective_flag": false
},
"id": 4
},
{
"data": {
"node_type": "course",
"operator": null,
"operator_value": null,
"program": 1,
"course": 2,
"required_program": null,
"title": null,
"elective_flag": false
},
"id": 5
},
{
"data": {
"node_type": "course",
"operator": null,
"operator_value": null,
"program": 1,
"course": 3,
"required_program": null,
"title": null,
"elective_flag": false
},
"id": 6
}
]
},
{
"data": {
"node_type": "operator",
"operator": "min_number_of",
"operator_value": "2",
"program": 1,
"course": null,
"required_program": null,
"title": "Elective Courses",
"elective_flag": true
},
"id": 3,
"children": [
{
"data": {
"node_type": "course",
"operator": null,
"operator_value": null,
"program": 1,
"course": 5,
"required_program": null,
"title": null,
"elective_flag": false
},
"id": 7
},
{
"data": {
"node_type": "course",
"operator": null,
"operator_value": null,
"program": 1,
"course": 6,
"required_program": null,
"title": null,
"elective_flag": false
},
"id": 8
},
{
"data": {
"node_type": "course",
"operator": null,
"operator_value": null,
"program": 1,
"course": 4,
"required_program": null,
"title": null,
"elective_flag": false
},
"id": 20
}
]
}
]
}
]
// Omitting other fields for brevity
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be returned, it's a bit silly to have an array of a single requirements tree like that. Ideally we'd redesign that but it might be too late for that unless the integration you're doing is the first usage of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but it might be too late for that unless the integration you're doing is the first usage of it.
The v1 programs and enrollments apis returns the same response (for req_tree, at least) and is used on mitxonline frontend. mitodl/mit-learn#2590 will be the first usage of req_tree from the v2 enrollments API in learn or mitxonline, as far as I can tell.
So if we want to change it in the v2 api, that would be straightforward. I agree it's a bit silly for the response to be array with 1 item. I'd prefer:
- no array,
req_tree = root node, or - array,
req_tree = root_node.children
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhysyngsun I've updated the v2/programs response so that req_tree is an array of nodes, program.req_tree = root_node.children.
Looking back a bit at https://github.com/mitodl/mitxonline/pull/1070/files#diff-97a17b4355029f455bcde3c73bdc32a4579fc81f007ee6fb8594d0f9656effe1R390 this seems to have been the intent, and it's what the update methods on serializers expect. I don't think the update methods are actually exposed anywhere, but now the response data matches what they would use.
d62269f to
dde7c6d
Compare
dde7c6d to
5604d75
Compare
5604d75 to
8b6a106
Compare
| required_program = serializers.IntegerField( | ||
| source="required_program_id", allow_null=True, default=None | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
required_program is not new—it was added in #2772 and can be seen in the current response at https://mitxonline.mit.edu/api/v2/programs/1
In #2772 it was added to the v1 serializer but not the v2 serializer
rhysyngsun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What are the relevant tickets?
Description (What does it do?)
req_treeproperty onv2/programsresponse to NOT include the root node.req_tree.How can this be tested?
v1/programsandv2/programsresponses:req_treeshould include the root node; this was not the original intent when req_tree was added, but maintains backward compatibilityreq_treeshould not include the root node.v2/programs; This PR changes thereq_treeproperty on the response. However, the req_tree property was not used anywhere on the frontend from this response. (Only fromv1/programsandv1/enrollments).v1/programsapi; this was not changed, but good to check.If you'd like to generate the API client locally, I suggest:
- check out branch
cc/local-generationfrom mitodl/mitxonline-api-clients#48- run
BRANCH_NAME=cc/improve-req_tree-spec ./scripts/local-generate.shAdditional Information
New code in MIT Learn that uses
req_treecurrently works around the inaccuracies fixed in this PR via@ts-expect-error; Upgrading the generated client will cause typechecking errors in MIT Learn, but in all cases the errors can be fixed by removing@ts-expect-error.