John/learner metrics multi course filtering #248
Conversation
Adds two functions to figures.sites. This is to support multi-course filtering for the learner-metrics endpoint 1. enrollments_for_course_ids This retrieves a CourseEnrollment queryset for enrollments that belong to the list of course ids argument 2. users_enrolled_in_courses This retrieves a User queryset for users that are enrolled in the courses identified by the course ids argument Added tests for the new functionality
The `learner-metrics` endpoint now filters on one or more courses using 'course=some-course-id` query parameters. To filter on multiple courses, simply do multiple parameters of the same "course" key. Example ``` ?course-my-first-course-id&course=my-second-course-id ``` * Added query param tests to the learner metrics viewset test class
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
==========================================
+ Coverage 91.79% 91.86% +0.06%
==========================================
Files 41 41
Lines 2133 2150 +17
==========================================
+ Hits 1958 1975 +17
Misses 175 175
Continue to review full report at Codecov.
|
| def query_param_course_keys(self): | ||
| """ | ||
| TODO: Mixin this | ||
| """ | ||
| cid_list = self.request.GET.getlist('course') | ||
| return [CourseKey.from_string(elem.replace(' ', '+')) for elem in cid_list] |
There was a problem hiding this comment.
The .replace(' ', '+') is an anti pattern in the backend and is error-prune. The frontend should send properly escaped course keys with encodeURIComponent or use fetch() to take care of that.
| def query_param_course_keys(self): | |
| """ | |
| TODO: Mixin this | |
| """ | |
| cid_list = self.request.GET.getlist('course') | |
| return [CourseKey.from_string(elem.replace(' ', '+')) for elem in cid_list] | |
| def query_param_course_keys(self): | |
| """ | |
| TODO: Mixin this | |
| """ | |
| cid_list = self.request.GET.getlist('course') | |
| return [CourseKey.from_string(elem) for elem in cid_list] |
There was a problem hiding this comment.
I"ll make this change right after I have some coffee :)
There was a problem hiding this comment.
@OmarIthawi Well, I would love to make this change, but when I try it, I get this. So the anti-pattern appears to be required still.
>>> cid = 'course-v1:StarFleetAcademy SFA01 2161'
>>> from opaque_keys.edx.keys import CourseKey
>>> CourseKey.from_string(cid)
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "/Users/jbaldwin/.virtualenvs/figtahoe2/lib/python2.7/site-packages/opaque_keys/__init__.py", line 197, in from_string
return cls.deprecated_fallback._from_deprecated_string(serialized)
File "/Users/jbaldwin/.virtualenvs/figtahoe2/lib/python2.7/site-packages/opaque_keys/edx/locator.py", line 379, in _from_deprecated_string
raise InvalidKeyError(cls, serialized)
InvalidKeyError: <class 'opaque_keys.edx.locator.CourseLocator'>: course-v1:StarFleetAcademy SFA01 2161
and
>>> cid = 'course-v1:StarFleetAcademy+SFA01+2161'
>>> CourseKey.from_string(cid)
CourseLocator(u'StarFleetAcademy', u'SFA01', u'2161', None, None)
To show that the key is otherwise valid.
I know you are saying the front end should handle it, but this was done because we've needed to be defensive in the past on this. Otherwise, this little hack would not exist. And we don't have the bandwidth to make sure 100% this won't happen at the absolute worst possible time. So hack stays for now.
However, this is something that warrants a revisit to do a decorator or mixin for the course key handling
There was a problem hiding this comment.
@johnbaldwin that's right, course-v1:StarFleetAcademy SFA01 2161 is not a valid key. Therefore the frontend should not send unescaped keys which what would make those keys invalid.
It's as if the frontend keeps adding a 999 to numbers and asking the API to subtract 999 before using the page argument. This will be more of an issue in the cases like:
- One of our customers tries to use the figures API through a library that escapes the parameters properly like Python
requestsor pretty much every HTTP client out there. - Or if it's us working with Figures APIs through such libraries.
It's bothering that we're doing an extra step every time we make a Figures API to fix something in the wrong place.
This PR adds multiple course filtering for the
learner-metricsendpoint.New functions to support filtering
Adds two functions to figures.sites. This is to support multi-course
filtering for the learner-metrics endpoint
This retrieves a CourseEnrollment queryset for enrollments that belong
to the list of course ids argument
This retrieves a User queryset for users that are enrolled in the
courses identified by the course ids argument
Added tests for the new functionality
New learner-metrics query parameter, "course="
The
learner-metricsendpoint now filters on one or more courses using'course=some-course-id` query parameters. To filter on multiple courses,
simply do multiple parameters of the same "course" key. Example