From f3258b22de1e9c08ca2780df816e1a8690bcecce Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Tue, 26 Apr 2022 18:26:09 -0400 Subject: [PATCH] Update SDM historic course count calculation * Old system wasn't working, was getting course counts as of the current date * Added code to estimate when a course was started from its first enrollment created date * So basically we now tracking counts of when a course had it first enrollment and basing historical course count as of a date based on that * This is to improve accuracy for Figures SDM backfill --- figures/pipeline/site_daily_metrics.py | 35 +++++++- tests/pipeline/test_site_daily_metrics.py | 4 + .../test_site_daily_metrics_functions.py | 89 +++++++++++++++++++ 3 files changed, 125 insertions(+), 3 deletions(-) create mode 100644 tests/pipeline/test_site_daily_metrics_functions.py diff --git a/figures/pipeline/site_daily_metrics.py b/figures/pipeline/site_daily_metrics.py index 582ad557..7bf78138 100644 --- a/figures/pipeline/site_daily_metrics.py +++ b/figures/pipeline/site_daily_metrics.py @@ -10,10 +10,12 @@ from django.db.models import Sum +from figures.course import Course from figures.helpers import as_course_key, as_datetime, next_day from figures.mau import site_mau_1g_for_month_as_of_day from figures.models import CourseDailyMetrics, SiteDailyMetrics from figures.sites import ( + site_course_ids, get_courses_for_site, get_users_for_site, get_student_modules_for_site, @@ -54,6 +56,33 @@ def missing_course_daily_metrics(site, date_for): # Standalone methods to extract data/aggregate data for use in SiteDailyMetrics # + +def get_course_ids_enrolled_on_or_before(site, date_for): + """Best guess to get site courses created on or before the specified date + + CourseOverview does not have a reliable 'created' field + So we need to get the first enrollment for the courses + Unfortunately, we need to iterate over the courses OR create a complex and + slow query, so our implementation is the safe brute force to iterate over + courses, get the first enrollment and see if that enrollment is on or before + the date. + + We will first implement this using `figures.course.Course` as it can + provide the first enrollment date for a course. There is a slight overhead + of an extra query in that `Course`'s contructor retrieves the site for the + course. If this turns out to be expensive, then we can furter optimize. But + first, let's see if we can do this withoutg duplicating code. + """ + found_course_ids = [] + compare_date = next_day(as_datetime(date_for)) + for course_id in site_course_ids(site): + course = Course(course_id) + course_fe_ts = course.first_enrollment_timestamp() + if course_fe_ts and course_fe_ts < compare_date: + found_course_ids.append(course_id) + return found_course_ids + + def get_site_active_users_for_date(site, date_for): ''' Get the active users ids for the given site and date @@ -123,9 +152,9 @@ def extract(self, site, date_for, **kwargs): # pylint: disable=unused-argument site_users = get_users_for_site(site) user_count = site_users.filter( date_joined__lt=as_datetime(next_day(date_for))).count() - site_courses = get_courses_for_site(site) - course_count = site_courses.filter( - created__lt=as_datetime(next_day(date_for))).count() + + course_ids = get_course_ids_enrolled_on_or_before(site, date_for) + course_count = len(course_ids) todays_active_users = get_site_active_users_for_date(site, date_for) todays_active_user_count = todays_active_users.count() diff --git a/tests/pipeline/test_site_daily_metrics.py b/tests/pipeline/test_site_daily_metrics.py index d5f57656..45f0d062 100644 --- a/tests/pipeline/test_site_daily_metrics.py +++ b/tests/pipeline/test_site_daily_metrics.py @@ -277,6 +277,10 @@ def mock_get_previous_cumulative_active_user_count(site, date_for): monkeypatch.setattr(pipeline_sdm, 'get_previous_cumulative_active_user_count', mock_get_previous_cumulative_active_user_count) + course_ids = [str(co.id) for co in self.course_overviews] + monkeypatch.setattr(pipeline_sdm, 'get_course_ids_enrolled_on_or_before', + lambda site, date_for: course_ids) + for course in figures.sites.get_courses_for_site(self.site): assert course.created.date() < self.date_for for user in figures.sites.get_users_for_site(self.site): diff --git a/tests/pipeline/test_site_daily_metrics_functions.py b/tests/pipeline/test_site_daily_metrics_functions.py new file mode 100644 index 00000000..2cc30ff2 --- /dev/null +++ b/tests/pipeline/test_site_daily_metrics_functions.py @@ -0,0 +1,89 @@ +"""Test the module level functions in figures.pipeline.site_daily_metrics + +First +""" +from __future__ import absolute_import +import pytest + +from django.utils.timezone import utc +from faker import Faker + +from figures.helpers import days_from +from figures.pipeline.site_daily_metrics import ( + get_course_ids_enrolled_on_or_before +) + +from tests.factories import ( + CourseEnrollmentFactory, + CourseOverviewFactory, + SiteFactory, + ) + + +fake = Faker() + + +@pytest.mark.django_db +class TestGetCoursesEnrolledOnOrBefore(object): + """Tests the `get_course_ids_enrolled_on_or_before` function + """ + @pytest.fixture(autouse=True) + def setup(self, db, settings): + # self.course_overview = CourseOverviewFactory() + self.site = SiteFactory() + # Try for any datetime with `fake.date_time()` as the code we test here + # should be agnostic to "any when". If it turns up to fail, then we + # need to inspect the code as to why + self.date_for = fake.date_time(tzinfo=utc) + self.site_course_ids = 'figures.pipeline.site_daily_metrics.site_course_ids', + + def test_with_no_courses(self, monkeypatch): + monkeypatch.setattr('figures.pipeline.site_daily_metrics.site_course_ids', + lambda site: []) + found_cids = get_course_ids_enrolled_on_or_before(self.site, + self.date_for) + assert found_cids == [] + + def test_with_no_enrollments(self, monkeypatch): + course_overview = CourseOverviewFactory() + monkeypatch.setattr('figures.pipeline.site_daily_metrics.site_course_ids', + lambda site: [str(course_overview.id)]) + found_cids = get_course_ids_enrolled_on_or_before(self.site, + self.date_for) + assert found_cids == [] + + def test_mix(self, monkeypatch): + """ + This should cover our cases. We shouldn't need + `test_all_before` or `test_all_after` + """ + + # Construct enrollments on days before date_for + ce_before = [ + CourseEnrollmentFactory(created=days_from(self.date_for, days)) + for days in range(-2, 0) + ] + ce_before_cids = [str(ce.course_id) for ce in ce_before] + + # Construct enrollments on date for + ce_date_for = [CourseEnrollmentFactory(created=self.date_for)] + ce_date_for_cids = [str(ce.course_id) for ce in ce_date_for] + + # Construct enrollments on days after date_for + ce_after = [CourseEnrollmentFactory(created=days_from(self.date_for, days)) + for days in range(1, 3)] + ce_after_cids = [str(ce.course_id) for ce in ce_after] + + # adaptable checks to make sure we don't have duplicate course ids + assert not set.intersection(set(ce_before_cids), set(ce_after_cids)) + assert not set.intersection(set(ce_before_cids), set(ce_date_for_cids)) + assert not set.intersection(set(ce_date_for_cids), set(ce_after_cids)) + + all_cids = ce_before_cids + ce_date_for_cids + ce_after_cids + monkeypatch.setattr('figures.pipeline.site_daily_metrics.site_course_ids', + lambda site: all_cids) + monkeypatch.setattr('figures.course.get_site_for_course', + lambda course_id: self.site) + found_cids = get_course_ids_enrolled_on_or_before(self.site, + self.date_for) + assert set(found_cids) == set(ce_before_cids + ce_date_for_cids)