From a83c6a320d7038aaffc269206c5edc82c7988a58 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Wed, 6 May 2020 13:51:11 -0400 Subject: [PATCH 1/5] Added testing for LearnerCourseGradeMetrics manager method --- ...test_learner_course_grade_metrics_model.py | 49 ++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/tests/models/test_learner_course_grade_metrics_model.py b/tests/models/test_learner_course_grade_metrics_model.py index e3255055..83731858 100644 --- a/tests/models/test_learner_course_grade_metrics_model.py +++ b/tests/models/test_learner_course_grade_metrics_model.py @@ -6,10 +6,55 @@ import pytest from django.contrib.sites.models import Site +from django.utils.timezone import utc +from figures.helpers import as_date from figures.models import LearnerCourseGradeMetrics -from tests.factories import CourseEnrollmentFactory +from tests.factories import ( + CourseEnrollmentFactory, + CourseOverviewFactory, + LearnerCourseGradeMetricsFactory, + UserFactory +) + + +@pytest.mark.django_db +def test_most_recent_with_data(db): + """Make sure the query works with a couple of existing models + + We create two LearnerCourseGradeMetrics models and test that the function + retrieves the newer one + """ + user = UserFactory() + first_date = as_date('2020-02-02') + second_date = as_date('2020-04-01') + course_overview = CourseOverviewFactory() + older_lcgm = LearnerCourseGradeMetricsFactory(user=user, + course_id=str(course_overview.id), + date_for=first_date) + newer_lcgm = LearnerCourseGradeMetricsFactory(user=user, + course_id=str(course_overview.id), + date_for=second_date) + + obj = LearnerCourseGradeMetrics.objects.most_recent_for_learner_course( + user=user, course_id=course_overview.id) + assert obj == newer_lcgm + + +@pytest.mark.django_db +def test_most_recent_with_empty_table(db): + """Make sure the query works when there are no models to find + + Tests that the function returns None and does not fail when it cannot find + any LearnerCourseGradeMetrics model instances + """ + assert not LearnerCourseGradeMetrics.objects.count() + user = UserFactory() + course_overview = CourseOverviewFactory() + obj = LearnerCourseGradeMetrics.objects.most_recent_for_learner_course( + user=user, course_id=course_overview.id) + assert not obj @pytest.mark.django_db @@ -69,7 +114,7 @@ def test_most_recent_for_learner_course_multiple_dates(self): def test_progress_percent(self): expected = (self.grade_data['sections_worked'] / - self.grade_data['sections_possible']) + self.grade_data['sections_possible']) obj = LearnerCourseGradeMetrics(**self.create_rec) assert obj.progress_percent == expected From d2d5c93155c6a8ae97588b8e2dabd62cb3713f4a Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Wed, 6 May 2020 20:39:29 -0400 Subject: [PATCH 2/5] Fix migration to work with Ginnkgo Django post 1.8 introduced a site model change that is not backward compatible, so we have a conditional dependency depending on the Django version --- .../migrations/0010_site_monthly_metrics.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/figures/migrations/0010_site_monthly_metrics.py b/figures/migrations/0010_site_monthly_metrics.py index 66052ab0..4fe60457 100644 --- a/figures/migrations/0010_site_monthly_metrics.py +++ b/figures/migrations/0010_site_monthly_metrics.py @@ -1,7 +1,10 @@ # -*- coding: utf-8 -*- # Generated by Django 1.11.15 on 2020-04-08 21:46 +# Manually updated to support Django 1.8 as well as 1.11 + from __future__ import unicode_literals +from django import VERSION as DJANGO_VERSION from django.db import migrations, models import django.db.models.deletion import django.utils.timezone @@ -9,11 +12,16 @@ class Migration(migrations.Migration): - - dependencies = [ - ('sites', '0002_alter_domain_unique'), - ('figures', '0009_mau_metrics'), - ] + if DJANGO_VERSION[0:2] == (1,8): + dependencies = [ + ('sites', '0001_initial'), + ('figures', '0009_mau_metrics'), + ] + else: # Assuming 1.11+ + dependencies = [ + ('sites', '0002_alter_domain_unique'), + ('figures', '0009_mau_metrics'), + ] operations = [ migrations.CreateModel( From 070f86aeac04a87424a68ec211aad52899a31e00 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Wed, 6 May 2020 21:01:15 -0400 Subject: [PATCH 3/5] Disable pipeline.site_monthly_metrtics test for Ginkgo Test fails because a datetime based query returns empty results when freezegun is enabled in Ginkgo. Skipping test if environment is Ginkgo until we can make time to investigate and fix --- tests/pipeline/test_site_monthly_metrics.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/pipeline/test_site_monthly_metrics.py b/tests/pipeline/test_site_monthly_metrics.py index b6816dba..3ff65cf8 100644 --- a/tests/pipeline/test_site_monthly_metrics.py +++ b/tests/pipeline/test_site_monthly_metrics.py @@ -11,6 +11,7 @@ from courseware.models import StudentModule +from figures.compat import RELEASE_LINE from figures.models import SiteMonthlyMetrics from figures.pipeline.site_monthly_metrics import fill_month, fill_last_month @@ -70,13 +71,24 @@ def mock_get_student_modules_for_site(site): assert obj.month_for == smm_test_data['month_before'].date() +@pytest.mark.skipif(RELEASE_LINE=='ginkgo', + reason='Freezegun breaks the StudentModule query') def test_fill_last_month_wo_overwrite(monkeypatch, smm_test_data): """ + This test breaks when run in the Ginkgo environment. The problem is that + after `freezer.start()`, StudentModule queries on `modified` fields return + empty results when there are data for the modified fields set for the filter + values. See tests/test_ginkgo.py """ assert SiteMonthlyMetrics.objects.count() == 0 mock_today = smm_test_data['mock_today'] freezer = freeze_time(mock_today) + year_check = smm_test_data['last_month_sm'][0].modified.year + assert StudentModule.objects.filter(modified__year=year_check), \ + 'before freezegun start' freezer.start() + assert StudentModule.objects.filter(modified__year=year_check), \ + 'after freezegun start' site = smm_test_data['site'] From bbefd3e30a0aca97cda8fce73c56d6d12c42f2b2 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Wed, 6 May 2020 21:03:45 -0400 Subject: [PATCH 4/5] Fixed LearnerCourseGradesDetailsSerializer.get_progress_data method If there aren't any matching LearnerCourseGradeMetrics records matching the filtering criteria then an exception is raised and trapped, triggering an error log. This does not change the behvior of the method. What is does is reduce logging noise to the log files and also does not create a `PipelineError` table log enty, saving database space --- figures/serializers.py | 19 +++++++------ tests/test_serializers.py | 58 +++++++++++++++++++++++++++++++++------ 2 files changed, 60 insertions(+), 17 deletions(-) diff --git a/figures/serializers.py b/figures/serializers.py index 7dce7e34..cae6e155 100644 --- a/figures/serializers.py +++ b/figures/serializers.py @@ -611,17 +611,21 @@ def get_progress_data(self, course_enrollment): else: course_completed = False + # Default values if we can't retrieve progress data + progress_percent = 0.0 + course_progress_details = None + try: obj = LearnerCourseGradeMetrics.objects.most_recent_for_learner_course( user=course_enrollment.user, course_id=str(course_enrollment.course_id)) - course_progress = dict( - progress_percent=obj.progress_percent, - course_progress_details=obj.progress_details) + if obj: + progress_percent = obj.progress_percent + course_progress_details = obj.progress_details except Exception as e: # pylint: disable=broad-except # TODO: Use more specific database-related exception error_data = dict( - msg='Unable to get learner course metrics', + msg='Exception trying to get learner course metrics', username=course_enrollment.user.username, course_id=str(course_enrollment.course_id), exception=str(e) @@ -630,9 +634,6 @@ def get_progress_data(self, course_enrollment): error_data=error_data, error_type=PipelineError.UNSPECIFIED_DATA, ) - course_progress = dict( - progress_percent=0.0, - course_progress_details=None) # Empty list initially, then will fill after we implement capturing # learner specific progress @@ -640,8 +641,8 @@ def get_progress_data(self, course_enrollment): data = dict( course_completed=course_completed, - course_progress=course_progress['progress_percent'], - course_progress_details=course_progress['course_progress_details'], + course_progress=progress_percent, + course_progress_details=course_progress_details, course_progress_history=course_progress_history, ) return data diff --git a/tests/test_serializers.py b/tests/test_serializers.py index 5b18d981..6aaeb64f 100644 --- a/tests/test_serializers.py +++ b/tests/test_serializers.py @@ -18,6 +18,7 @@ from figures.models import ( CourseDailyMetrics, CourseMauMetrics, + LearnerCourseGradeMetrics, SiteDailyMetrics, SiteMauMetrics, ) @@ -45,6 +46,7 @@ CourseMauMetricsFactory, CourseOverviewFactory, GeneratedCertificateFactory, + LearnerCourseGradeMetricsFactory, SiteDailyMetricsFactory, SiteMauMetricsFactory, UserFactory, @@ -395,7 +397,7 @@ def test_has_fields(self): data = self.serializer.data assert set(data.keys()) == set(self.expected_fields) - + # This is to make sure that the serializer retrieves the correct nested # model (UserProfile) data assert data['username'] == 'alpha_one' @@ -413,13 +415,6 @@ class TestLearnerCourseDetailsSerializer(object): ''' @pytest.fixture(autouse=True) def setup(self, db): - # self.model = CourseEnrollment - # self.user_attributes = { - # 'username': 'alpha_one', - # 'profile__name': 'Alpha One', - # 'profile__country': 'CA', - # } - #self.user = UserFactory(**self.user_attributes) self.site = Site.objects.first() self.certificate_date = datetime.datetime(2018, 4, 1, tzinfo=utc) self.course_enrollment = CourseEnrollmentFactory( @@ -442,6 +437,53 @@ def test_has_fields(self): data = self.serializer.data assert set(data.keys()) == expected_fields + def test_get_progress_data(self): + """ + Method should return data of the form: + {'course_progress_history': [], + 'course_progress_details': { + 'sections_worked': 5, + 'points_possible': 30.0, + 'sections_possible': 10, + 'points_earned': 15.0 + }, + 'course_progress': (0.5,), + 'course_completed': datetime.datetime(2018, 4, 1, 0, 0, tzinfo=) + } + """ + metrics_data = dict( + points_possible=1, + points_earned=2, + sections_worked=3, + sections_possible=4) + lcgm = LearnerCourseGradeMetricsFactory( + user=self.course_enrollment.user, + course_id=str(self.course_enrollment.course_id), + **metrics_data + ) + + data = self.serializer.get_progress_data(self.course_enrollment) + details = data['course_progress_details'] + for key, val in metrics_data.items(): + assert details[key] == val + assert data['course_progress'] == lcgm.progress_percent + assert data['course_completed'] == self.generated_certificate.created_date + + def test_get_progress_data_with_no_data(self): + """Tests that the serializer method succeeds when no learner course + grade metrics records + """ + expected_data = { + 'course_progress_history': [], + 'course_progress_details': None, + 'course_progress': 0.0, + 'course_completed': False + } + assert not LearnerCourseGradeMetrics.objects.count() + course_enrollment = CourseEnrollmentFactory() + data = self.serializer.get_progress_data(course_enrollment) + assert data == expected_data + @pytest.mark.django_db class TestLearnerDetailsSerializer(object): From 3f12fa31f13da773fe7fd7ced61a158b22f74bd1 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Sat, 16 May 2020 15:41:19 -0400 Subject: [PATCH 5/5] Code cleanup - Remove pdb traces and remarked out code from test code --- tests/models/test_course_daily_metrics_model.py | 7 ------- tests/views/test_learner_details_view.py | 1 - tests/views/test_site_monthly_metrics_viewset.py | 5 ++++- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/models/test_course_daily_metrics_model.py b/tests/models/test_course_daily_metrics_model.py index d81337ef..39992bcd 100644 --- a/tests/models/test_course_daily_metrics_model.py +++ b/tests/models/test_course_daily_metrics_model.py @@ -130,13 +130,11 @@ def test_with_valid_average_progress(self, average_progress): course_id='course-v1:SomeOrg+ABC01+2121', enrollment_count=11, active_learners_today=1, - # average_progress=Decimal(str(average_progress)), average_progress=str(average_progress), average_days_to_complete=5, num_learners_completed=10 ) metrics = CourseDailyMetrics.objects.create(**rec) - # assert metrics.average_progress == Decimal(str(average_progress)) assert metrics.average_progress == average_progress metrics.clean_fields() @@ -154,7 +152,6 @@ def test_with_valid_average_progress_2(self, average_progress): defaults=dict( enrollment_count=11, active_learners_today=1, - # average_progress=Decimal(str(average_progress)), average_progress=str(average_progress), average_days_to_complete=5, num_learners_completed=10, @@ -163,10 +160,6 @@ def test_with_valid_average_progress_2(self, average_progress): cdm, created = CourseDailyMetrics.objects.update_or_create(**rec) cdm.save() - # import pdb; pdb.set_trace() - - - # assert metrics.average_progress == Decimal(str(average_progress)) assert cdm.average_progress == str(average_progress) cdm.clean_fields() diff --git a/tests/views/test_learner_details_view.py b/tests/views/test_learner_details_view.py index a96c642f..0cb8a746 100644 --- a/tests/views/test_learner_details_view.py +++ b/tests/views/test_learner_details_view.py @@ -301,7 +301,6 @@ def test_serializer(self): queryset = enrollments.filter(user=self.my_site_users[0]) assert queryset serializer = LearnerCourseDetailsSerializer(queryset[0]) - # import pdb; pdb.set_trace() # We're asserting that we can get the serializer `data` property without # error, not checking the contents of the data. That should be done in # the serializer specific tests (see tests/test_serializers.py). diff --git a/tests/views/test_site_monthly_metrics_viewset.py b/tests/views/test_site_monthly_metrics_viewset.py index 73245113..c042ac54 100644 --- a/tests/views/test_site_monthly_metrics_viewset.py +++ b/tests/views/test_site_monthly_metrics_viewset.py @@ -105,7 +105,10 @@ def check_response(self, response, endpoint): return True def run_request(self, endpoint, user_reg_test_data): - import pdb; pdb.set_trace() + """ + Stub + """ + pass @pytest.mark.skip(reason='We need to confirm who is in total registered users') def test_registered_learners(self, monkeypatch, user_reg_test_data):