Merge pull request #213 from appsembler/john/fix_progress_percent

Figures bug fixes

Author: johnbaldwin

figures/migrations/0010_site_monthly_metrics.py

@@ -1,19 +1,27 @@
 # -*- 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
 import model_utils.fields
 
 
 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(

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,18 +634,15 @@ 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
         course_progress_history = []
 
         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

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()
 

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
 

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']
 

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=<UTC>)
+            }
+        """
+        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):

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).

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):