-
Notifications
You must be signed in to change notification settings - Fork 37
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
figures.course is a new module to simplify Figures
This PR adds `figures.course`, a new module that has the `Course` class The goal of this class is to simplify Figures code This class provides data specific to and associated with a course. Our initial version provides only the essential data needed for our pipeline performance improvement
- Loading branch information
1 parent
906e5da
commit 4cd78a7
Showing
2 changed files
with
227 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
"""Course specific module for Figures | ||
## This module defined a `Course` class for data retrieval | ||
Initialy created to do the following: | ||
1. Reduce duplication in Figures "pipeline" | ||
2. Build stronger course context to make Figures programming easier | ||
## Background summary | ||
A course id is globally unique as it has the identify of the organization and | ||
organizations are globally unique. | ||
## Design to think about - Enrollment class | ||
Build on Django's lazy eval for querysets to create also an `Enrollment` class | ||
that provides interfaces for `enrollment.date_for` and abstracts this bit of | ||
mess that enrollments and student modules do NOT associate and instead we need | ||
to query back and forth with `user_id` and `course_id`. | ||
""" | ||
from __future__ import absolute_import | ||
from django.db.models import Q | ||
from figures.compat import CourseEnrollment, StudentModule | ||
from figures.helpers import ( | ||
as_course_key, | ||
as_date, | ||
) | ||
from figures.sites import ( | ||
get_site_for_course, | ||
) | ||
|
||
|
||
class Course(object): | ||
"""Representation of a Course. | ||
The impetus for this class was dealing with querying for course enrollment | ||
and student module records for a specific course and for dates and date | ||
ranges for the course | ||
## Architecture goal | ||
**Start simple and don't build the kitchen sink into here right away just | ||
because this class exists** | ||
## Data under consideration to have this class handle | ||
* enrollments created on a date, before, after, between. However this would | ||
just be a convenience as the `.enrollments` property returns a queryset that | ||
can be filtered on `.created` | ||
""" | ||
def __init__(self, course_id): | ||
""" | ||
Initial version, we pass in a course ID and cast to a course key as an | ||
instance attribute. Later on, add `CourseLike` to abstract course identity | ||
so we can stop worrying about "Is it a string repretentation of a course or | ||
is it a CourseKey?" | ||
""" | ||
self.course_key = as_course_key(course_id) | ||
|
||
# Improvement: Consider doing lazy evaluation | ||
self.site = get_site_for_course(self.course_id) | ||
|
||
def __repr__(self): | ||
return '{}.{} <{}>'.format(self.__module__, | ||
self.__class__.__name__, | ||
str(self.course_key)) | ||
|
||
def __str__(self): | ||
return self.__repr__() | ||
|
||
@property | ||
def course_id(self): | ||
"""Returns string representation of the course id | ||
""" | ||
return str(self.course_key) | ||
|
||
@property | ||
def enrollments(self): | ||
"""Returns CourseEnrollment queryset for the course | ||
""" | ||
return CourseEnrollment.objects.filter(course_id=self.course_key) | ||
|
||
@property | ||
def student_modules(self): | ||
"""Returns StudentModule queryset for enrollments in the course | ||
""" | ||
return StudentModule.objects.filter(course_id=self.course_key) | ||
|
||
def student_modules_active_on_date(self, date_for): | ||
"""Returns StudentModule queryset active on the date | ||
Active is if there was a `created` or `modified` field for the given date | ||
NOTE: We need to do this instead of simplly `modified__date=date_for` | ||
because we still have to support Django 1.8/Ginkgo | ||
""" | ||
date_for = as_date(date_for) | ||
q_created = Q(created__year=date_for.year, | ||
created__month=date_for.month, | ||
created__day=date_for.day) | ||
q_modified = Q(modified__year=date_for.year, | ||
modified__month=date_for.month, | ||
modified__day=date_for.day) | ||
return self.student_modules.filter(q_created | q_modified) | ||
|
||
def enrollments_active_on_date(self, date_for): | ||
"""Return CourseEnrollment queryset for enrollments active on the date | ||
Looks for student modules modified on the specified date and returns | ||
matching CourseEnrollment records | ||
""" | ||
sm = self.student_modules_active_on_date(date_for) | ||
user_ids = sm.values('student_id').distinct() | ||
return CourseEnrollment.objects.filter(course_id=self.course_key, | ||
user_id__in=user_ids) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
"""Test `figures.course` module | ||
""" | ||
from __future__ import absolute_import | ||
import pytest | ||
from faker import Faker | ||
from opaque_keys.edx.keys import CourseKey | ||
|
||
from figures.course import Course | ||
from figures.helpers import days_from | ||
from figures.sites import get_site_for_course | ||
|
||
from tests.factories import ( | ||
CourseEnrollmentFactory, | ||
CourseOverviewFactory, | ||
StudentModuleFactory, | ||
) | ||
|
||
fake = Faker() | ||
|
||
|
||
@pytest.mark.django_db | ||
class TestCourse: | ||
""" | ||
Starting with just basic tests | ||
""" | ||
@pytest.fixture(autouse=True) | ||
def setup(self, db): | ||
self.course_overview = CourseOverviewFactory() | ||
# Little sanity check making sure our Factory has the right class type | ||
# for the course key | ||
assert isinstance(self.course_overview.id, CourseKey) | ||
|
||
def assert_construction(self, course): | ||
assert course.course_key == self.course_overview.id | ||
assert course.course_id == str(self.course_overview.id) | ||
assert course.site == get_site_for_course(self.course_overview.id) | ||
|
||
def simple_test_property_course_id_association(self, factory_class, property_name): | ||
"""Helper method to DRY tests for property methods on simple associations | ||
This method can be used to test the Course class property methods return | ||
expected results for property methods that return objects that can be | ||
associated with a course with just the course id. Initially implemented | ||
to test the enrollments and student modules property methods. Could also | ||
be used to test for other models like CourseAccessGroup, CourseAccessRole, | ||
Figures CourseDailyMetrics, | ||
EnrollmentData, or LearnerCourseGradeMetrics if we wanted to implement | ||
property methods for those | ||
If we did use this for models that use a string course id instead of a | ||
CourseKey, then we'll need to make sure the handled course id form is | ||
used. We could do this with a method parameter that either casts to the | ||
expected form or defines which one to use and we can explicitly cast in | ||
this method | ||
""" | ||
our_objects = [factory_class(course_id=self.course_overview.id) | ||
for _ in range(2)] | ||
other_object = factory_class() | ||
assert not other_object.course_id == self.course_overview.id | ||
course = Course(self.course_overview.id) | ||
assert set(our_objects) == set(getattr(course, property_name)) | ||
|
||
def test_str_constructor(self): | ||
self.assert_construction(Course(str(self.course_overview.id))) | ||
|
||
def test_course_key_constructor(self): | ||
self.assert_construction(Course(self.course_overview.id)) | ||
|
||
def test_enrollments(self): | ||
self.simple_test_property_course_id_association(CourseEnrollmentFactory, | ||
'enrollments') | ||
|
||
def test_student_modules(self): | ||
self.simple_test_property_course_id_association(StudentModuleFactory, | ||
'student_modules') | ||
|
||
def test_student_modules_active_on_date(self): | ||
our_date_for = fake.date_this_year() | ||
our_created_sm = [StudentModuleFactory(course_id=self.course_overview.id, | ||
created=our_date_for) for _ in range(2)] | ||
our_modified_sm = [StudentModuleFactory(course_id=self.course_overview.id, | ||
modified=our_date_for) for _ in range(2)] | ||
# Create record with a different date | ||
StudentModuleFactory(course_id=self.course_overview.id, | ||
created=days_from(our_date_for, -2), | ||
modified=days_from(our_date_for, -1)) | ||
course = Course(self.course_overview.id) | ||
found_sm = course.student_modules_active_on_date(our_date_for) | ||
assert set(our_created_sm + our_modified_sm) == set(found_sm) | ||
|
||
def test_enrollments_active_on_date(self): | ||
our_date_for = fake.date_this_year() | ||
other_date_for = days_from(our_date_for, -1) | ||
our_ce = [CourseEnrollmentFactory(course_id=self.course_overview.id) | ||
for _ in range(2)] | ||
our_sm = [] | ||
for ce in our_ce: | ||
our_sm.extend([StudentModuleFactory(course_id=ce.course_id, | ||
student=ce.user, | ||
modified=our_date_for), | ||
StudentModuleFactory(course_id=ce.course_id, | ||
student=ce.user, | ||
created=our_date_for)]) | ||
# Create enrollment we should not get in our results | ||
other_ce = CourseEnrollmentFactory(course_id=self.course_overview.id) | ||
StudentModuleFactory.from_course_enrollment(other_ce, | ||
created=other_date_for, | ||
modified=other_date_for) | ||
course = Course(self.course_overview.id) | ||
found_ce = course.enrollments_active_on_date(our_date_for) | ||
assert set(found_ce) == set(our_ce) |