Conversation
switching from calling other djangoapps via direct model access to calling from API. This included adding an API in the Student app. FIXES: APER-3972
| course_enrollments = get_course_enrollments(user, True, (enterprise_enrollment_course_ids)) | ||
|
|
||
| return list(course_enrollments) | ||
| return course_enrollments |
There was a problem hiding this comment.
I'm not a big fan of passing around lists, when you could pass around a QuerySet, so I converted this to return the QuerySet. If I had my druthers the ProgramProgressMeter would also take a QuerySet, but honestly at some point that whole thing is going to get re-factored in some way or another.
| enterprise_enrollment_course_ids = list( | ||
| EnterpriseCourseEnrollment.objects.filter( | ||
| enterprise_customer_user__user_id=user.id, | ||
| enterprise_customer_user__enterprise_customer__uuid=enterprise_uuid, | ||
| ).values_list("course_id", flat=True) | ||
| enterprise_enrollment_course_ids = ( | ||
| get_enterprise_course_enrollments(user) | ||
| .filter(enterprise_customer_user__enterprise_customer__uuid=enterprise_uuid) | ||
| .values_list("course_id", flat=True) | ||
| ) | ||
|
|
||
| course_enrollments = ( | ||
| CourseEnrollment.enrollments_for_user(user) | ||
| .filter(course_id__in=enterprise_enrollment_course_ids) | ||
| .select_related("course") | ||
| ) | ||
| course_enrollments = get_course_enrollments(user, True, (enterprise_enrollment_course_ids)) |
There was a problem hiding this comment.
Quick question -- It looks like the new get_course_enrollments function is expecting course_ids to be a list (type-hint: course_ids: list[str | None] | None = None,), but we no longer are casting enterprise_enrollment_course_ids to a list on line 183 in the updated _get_enterprise_course_enrollments function.
It looks like enterprise_enrollment_course_ids will now be a QuerySet. Is this okay? I think it's fine? since the course_enrollments = course_enrollments.filter(course_id__in=course_ids) line just needs an iterable? I don't feel strongly either way in terms of fixing the type hint or explicitly casting enterprise_enrollment_course_ids as a list.
Let me know what you think!
There was a problem hiding this comment.
I think you're right, and it should be a List. In fact, I think that's probably my original intention, which is why there is a meaningless set of parens around the passed parameter – I think I started to cast it and then left off the word list.
I thought about sending it as a QuerySet, which I prefer, but the reality is I don't want the caller to have an idea about what kind of course ID list might be getting sent over. In theory, it could be someone just sending over a list of a couple of course IDs to search through.
( python's loose typing is very forgiving of treating QuerySets as list switches I assume why this worked, but it really shouldn't be. If I had enabled the type checker on student/api.py i'm sure it would have yelled at me about that.)
Casting the query set to list, because the caller is not making any assumptions about what kind of IDs are being sent.
…emove-direct-links-to-models
deborahgu
deleted the
dkaplan1/APER-3972_program-dashboard-api-remove-direct-links-to-models
branch
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
switching from calling other djangoapps via direct model access to calling from API. This included adding an API in the Student app. FIXES: APER-3972
switching from calling other djangoapps via direct model access to calling from API. This included adding an API in the Student app. FIXES: APER-3972
3 participants
Description
i converted the Programs rest API views from calling other djangoapps via direct model access to calling from API. This included adding an API in the Student app.
Should be invisible to all learners. Other developers will now have access to the new API method.
FIXES: APER-3972