Implement a new view: Learners Progress Overview

[grozdanowski]

Per requests by our users, we found that we are missing a central point of overview for learner progress. That is why we are working on implementing this new view: Learners Progress Overview. It allows to view the progress of all learners in all courses at the same time. It also allows to filter out certain courses to only show the learners that have enrolled in them, as well as having a search enabled (by email, username or full name). After creating your filtered and sorted view, you can export the data from this view as a CSV on-the-fly from the frontend.

Note: while the view is ready for testing, we need to figure out how to test this out (especially CSV generation) in production against a serious amount of data. My recommendation is to temporarily remove the menu link for this new view and push it to production, then access it directly using the URL and test it out in real world. I'm also open for other suggestions.

[codecov-commenter]

Codecov Report

Merging #229 into master will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   91.33%   91.52%   +0.18%     
==========================================
  Files          38       38              
  Lines        1950     2111     +161     
==========================================
+ Hits         1781     1932     +151     
- Misses        169      179      +10     

Impacted Files	Coverage Δ
figures/filters.py	98.76% <100.00%> (+0.08%)	⬆️
figures/views.py	92.99% <100.00%> (+1.24%)	⬆️
figures/metrics.py	87.58% <0.00%> (-0.40%)	⬇️
figures/pipeline/enrollment_metrics.py	97.95% <0.00%> (-0.38%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a542aa...d9770a9. Read the comment docs.

[johnbaldwin]

@grozdanowski So if I understand right, one suggestion you have is to hide this view with only a direct link?

Would be handy if we could feature flag this for specific users for testing.

Regardless, we should label this page as "Beta" after we've proved basic operational functionality on staging and we're into scalability testing on prod.

I'll comment specifically in the new filter code about the filter implementation

[johnbaldwin]

@grozdanowski First, cheers for diving into Python and the back end of Figures! I really want to encourage you,

but now for the comments:

Style nit

I don't believe I've formally written contribution docs to identify style guidelines for Figures, pretty much we use Google's python style guide: https://google.github.io/styleguide/pyguide.html#316-naming

Which means please use course_id and course_ids instead of courseId and courseIDs for local var names

Data safety

This introduces a potential latent vulnerability: Course ids from any site can be in the query string. so we can have users who belong to other sites in total_user_ids. Now this is potential and not realized because queryset contains user ids only for this site (phew!), so we have a white list that will just ignore non-site user ids in total_user_ids. This work for now. However it is latent because if this code is then refactored or reused without queryset behaving as it does or if some future query filtering drops queryset, or if someone likes this code and does copy/paste programming without proper understanding of site isolation, data leak vulnerability is introduced.

We can fix this by validating course ids belong to the site. One easy way to do this would be something like the following:

# Top of module
from figures.sites import get_course_keys_for_site
from django.contrib.sites.shortcuts get_current_site

# in our Filter method

param_course_keys = [as_course_key(val.replace(' ', '+')) for val in value.split('|')]
site = get_current_site(self.request)
if not set(param_course_keys) <= set(get_course_keys_for_site(site)):
    raise InvalidCourseIdError("some message")  # this would be a new exception class
else:
    # handle code normally
    ...

However, this brings us to point 'B':

Data correctness / consistency and expectations and performance

Let's say User X is an admin in Course X and a learner in Course B. If this is called with Course A and Course B in the params, should we include or not include User X? Now, this is not a big deal, but we want to make sure we are clear on what we want with regard to including/excluding course staff.

Also, the code is making N SQL calls, one for each course. Now get_enrolled_in_exclude_admins is pretty fast, I tested it in prod with a big site and the range I got was 0.004 to 0.01 sec per call.

Not too bad, and even if we have a lot of courses in the query string, we can likely skip worrying about O(n) performance because in reality, the query string size is going to be a problem before we have any kind of worry about asymptotic analysis (https://stackoverflow.com/questions/812925/what-is-the-maximum-possible-length-of-a-query-string)

So if we decide that it is ok to return course staff, we can then do some easy query optimization, like the following:

# Top of figures/filter.py module
from django.contrib.sites.shortcuts get_current_site

# in our Filter class method:

    course_keys = [as_course_key(val.replace(' ', '+')) for val in course_query_str.split('|')]

    if not set(course_keys) <= set(get_course_keys_for_site(site)):
        raise InvalidCourseError('Request gave us invalid course keys')

    ce = CourseEnrollment.objects.filter(course_id__in=course_keys)

    # `.distinct()` is probably not needed, might have additional overhead, but
    # adding it gets us a list without duplicate user_ids, since users can be
    # enrolled in multiple courses in a site
    return queryset.filter(id__in=ce.values('user_id').distinct())

If we DO need to exclude course staff unless there is a course for which the admin is enrolled as a learner, we can adapt the get_enrolled_in_exclude_admins to handle multiple courses so we've got one SQL call to retrieve user ids for the whole list of courses. We might need to do some RAW nested SQL to get there, but we'll do the cleanest code that is in the same order of magnitude performance.

[johnbaldwin]

@grozdanowski I commented. Also I posted a PR in our scripts repo and added you as a reviewer. There I have demo code for doing user filtering. That can be a sandbox you can run with production data to test query performance. We can chat in slack on that or hop on a vid-chat

[OmarIthawi]

Thanks @grozdanowski! Looks great! Thank you for jumping into Python!

I've suggested two things:

• Please always use encodeURIComponent when sending GET parameters to backend. POST requests automatically does this for you.
• Please favor comma (',') instead of the pipe (|) mostly just for consistency and developer sanity.

or even better since we have fetch:

    var url = new URL('https://api.tahoe.com')

    var params = {lat:35.696233, long:139.570431, course_ids: ['course-v1:edX+AC104+3T2019', 'course-v1:omarrustici+Hello+Hello']}
    
    url.search = new URLSearchParams(params).toString();  // does all the magic and encodeURIComponent for you.

    console.log('' + url)
    // Nicely encoded without needing any hack on the backend 
    // https://sl.se/?lat=35.696233&long=139.570431&course_ids=course-v1%3AedX%2BAC104%2B3T2019%2Ccourse-v1%3Aomarrustici%2BHello%2BHello

In addition to what John said about the security, linting and performance comments.

[OmarIthawi]

Suggested change

	courseIds = value.split('|')
	courseIds = value.split(',')

[OmarIthawi]

let's please use fetch().search to avoid this hack.

Suggested change

	course_key = CourseKey.from_string(courseId.replace(' ', '+'))
	course_key = CourseKey.from_string(courseId)

[OmarIthawi]

Suggested change

	const selectedCourseIds = selectedIdsList.join('|');
	const selectedCourseIds = selectedIdsList.join(',');

[OmarIthawi]

Suggested change

	requestUrl += '&enrolled_in_course_id=' + selectedCourseIds;
	requestUrl += '&enrolled_in_course_id=' + encodeURIComponent(selectedCourseIds);

[OmarIthawi]

Suggested change

	requestUrl += '?search=' + searchQuery;
	requestUrl += '?search=' + ecodeURIComponent(searchQuery);

[OmarIthawi]

Suggested change

	requestUrl += '&ordering=' + orderingType;
	requestUrl += '&ordering=' + encodeURIComponent(orderingType);

[grozdanowski]

This PR is to me dismissed, as per new work replacing this one. New PR is here: #240