Merge pull request #242 from appsembler/john/update-monthly-task

John/update monthly task

devsite/requirements/ginkgo.txt

@@ -50,15 +50,16 @@ recommonmark==0.4.0
 ##
 
 edx-opaque-keys==0.4
-#edx-drf-extensions==1.2.2
+# edx-organizations 0.4.4 requires edx-drf-extensions<1.0.0,>=0.5.1, but you'll have edx-drf-extensions 1.2.2 which is incompatible.
+edx-drf-extensions==1.2.2
 edx-organizations==0.4.4
 
 
 ##
 ## Devsite 
 ##
 
-django-debug-toolbar==1.11
+django-debug-toolbar==1.9.1
 
 
 ##
@@ -69,10 +70,13 @@ coverage==4.5.1
 factory-boy==2.5.1
 pylint==1.8.2
 pylint-django==0.9.1
-pytest==3.1.3
+pytest==3.6.2
 pytest-django==3.1.2
 pytest-mock==1.7.1
 pytest-pythonpath==0.7.2
 pytest-cov==2.6.0
 tox==3.7.0
 freezegun==0.3.12
+
+# Added to address: TypeError: attrib() got an unexpected keyword argument 'convert'
+attrs==19.1.0

devsite/requirements/hawthorn_community.txt

@@ -48,7 +48,7 @@ recommonmark==0.4.0
 ##
 
 edx-opaque-keys==0.4.4
-#edx-drf-extensions==1.5.2
+edx-drf-extensions==1.5.2
 
 edx-organizations==0.4.10
 
@@ -66,13 +66,22 @@ django-debug-toolbar==1.11
 coverage==4.5.4
 factory-boy==2.5.1
 flake8==3.7.9
-pylint==1.9.5
-pylint-django==0.11.1
-pytest==3.1.3
+# To address: edx-lint 0.5.5 requires pylint==1.7.1, but you'll have pylint 1.9.5 which is incompatible.
+pylint==1.7.1
+# To address edx-lint 0.5.5 requires pylint-django==0.7.2, but you'll have pylint-django 0.11.1 which is incompatible.
+pylint-django==0.7.2
+pytest==3.6.2
 pytest-django==3.1.2
 pytest-mock==1.7.1
 pytest-pythonpath==0.7.2
 pytest-cov==2.6.0
-tox==3.14.2
+# To address: tox 3.14.2 requires pluggy<1,>=0.12.0, but you'll have pluggy 0.6.0 which is incompatible.
+tox==3.1.0
 freezegun==0.3.12
 edx-lint==0.5.5
+
+# Added to address: TypeError: attrib() got an unexpected keyword argument 'convert'
+attrs==19.1.0
+
+# To address: edx-lint 0.5.5 requires astroid==1.5.2, but you'll have astroid 1.6.6 which is incompatible.
+astroid==1.5.2

devsite/requirements/hawthorn_multisite.txt

@@ -49,7 +49,7 @@ recommonmark==0.4.0
 ##
 
 edx-opaque-keys==0.4.4
-#edx-drf-extensions==1.5.2
+edx-drf-extensions==1.5.2
 
 #edx-organizations==0.4.10
 # * Organization/site mapping requires Appsembler's fork
@@ -72,10 +72,14 @@ factory-boy==2.5.1
 flake8==3.7.9
 pylint==1.9.5
 pylint-django==0.11.1
-pytest==3.1.3
+pytest==3.6.2
 pytest-django==3.1.2
 pytest-mock==1.7.1
 pytest-pythonpath==0.7.2
 pytest-cov==2.6.0
-tox==3.14.2
+# tox==3.14.5
+tox==3.1.0
 freezegun==0.3.12
+
+# Added to address: TypeError: attrib() got an unexpected keyword argument 'convert'
+attrs==19.1.0

figures/helpers.py

@@ -1,6 +1,49 @@
-'''
-Helper functions to make data handling and conversions easier
-'''
+"""Helper functions to make data handling and conversions easier
+
+# Figures 0.3.13 - Defining scope of this module
+
+The purpose of this module is to provide conveniece methods around commonly
+executed statements. These convenience methods should serve as shorthand for
+code we would repeatedly execute which don't yet have a module of their own.
+
+The idea is that if there is not yet a module for some helper function, we put
+it in here until we see a pattern. We then either identify a new module and
+transplate these like functions out of this module into the new one. Or we
+identify that functions in here actually should go in an existing module.
+
+The purposes for this are:
+
+1. Reduce development time cost by not having to stop and design (informally or
+   formally) a new module or inspect all the existing modules to find an
+   appropriate home. The second point is helpful for those not intimately
+   familiar with the Figures codebase
+
+2. Avoid premature optimization in building out new module functionality because
+   we are adding a single method (Avoid the desire to fill this new module with
+   more than the new functionality that serves our immediate needs)
+
+3. Avoid over specificifity, which can results in an explosion of tiny modules
+   that may be too specific in their context
+
+
+## Background
+
+Originally this module served as a variant of a 'utils' module, but with the
+express purpose of providing convenience "helper" functions to help DRY (do not
+repeat yourself) the code and make the code more readable
+
+## What does not belong here?
+
+* "Feature" functionality does not belong here
+* Long functions do not belong here
+* Code that communicates outside of Figures does not belong here. No database,
+  filesystem, or network connectiviely functionality belongs here
+
+This is not an exhaustive list. We'll grow it as needed.
+
+An important point is that we should not expect this module to be a permanent
+home for functionality.
+"""
 
 import calendar
 import datetime
@@ -19,6 +62,8 @@ def is_multisite():
     A naive but reliable check on whether Open edX is using multi-site setup or not.
 
     Override by setting ``FIGURES_IS_MULTISITE`` to true in the Open edX FEATURES.
+
+    TODO: Move to `figures.sites`
     """
     return bool(settings.FEATURES.get('FIGURES_IS_MULTISITE', False))
 
@@ -28,19 +73,23 @@ def log_pipeline_errors_to_db():
     Capture pipeline errors to the figures.models.PipelineError model.
 
     Override by setting ``FIGURES_LOG_PIPELINE_ERRORS_TO_DB`` to false in the Open edX FEATURES.
+
+    TODO: This is an environment/setting "getter". Should be moved to `figures.settings`
     """
     return bool(settings.FEATURES.get('FIGURES_LOG_PIPELINE_ERRORS_TO_DB', True))
 
 
 def as_course_key(course_id):
-    '''Returns course id as a CourseKey instance
+    """Returns course id as a CourseKey instance
 
     Convenience method to return the paramater unchanged if it is of type
     ``CourseKey`` or attempts to convert to ``CourseKey`` if of type str or
     unicode.
 
     Raises TypeError if an unsupported type is provided
-    '''
+
+    NOTE: This is a good example of a helper method that belongs here
+    """
     if isinstance(course_id, CourseKey):
         return course_id
     elif isinstance(course_id, basestring):  # noqa: F821
@@ -54,6 +103,11 @@ def as_datetime(val):
     '''
     TODO: Add arg flag to say if caller wants end of day, beginning of day
     or a particular time of day if the param is a datetime.date obj
+
+
+    NOTE: The date functions here could be in a `figures.datetools` module.
+
+    Not set on the name `datetools` but some date specific module
     '''
     if isinstance(val, datetime.datetime):
         return val
@@ -74,10 +128,15 @@ def as_datetime(val):
 
 
 def as_date(val):
-    '''Casts the value to a ``datetime.date`` object if possible
+    """Casts the value to a ``datetime.date`` object if possible
 
     Else raises ``TypeError``
-    '''
+
+    NOTE: This is a good example of a helper method that belongs here
+          We could also move this and the other date helpers to a "date"
+          labeled module in Figures. Then at some future time, move those out
+          into a "toolbox" package to abstrac
+    """
     # Important to check if datetime first because datetime.date objects
     # pass the isinstance(obj, datetime.date) test
     if isinstance(val, datetime.datetime):
@@ -117,13 +176,11 @@ def days_in_month(month_for):
 
 # TODO: Consider changing name to 'months_back_iterator' or similar
 def previous_months_iterator(month_for, months_back):
-    '''Iterator returns a year,month tuple for n months including the month_for
+    """Iterator returns a year,month tuple for n months including the month_for
 
     month_for is either a date, datetime, or tuple with year and month
     months back is the number of months to iterate
-
-    includes the month_for
-    '''
+    """
 
     if isinstance(month_for, tuple):
         # TODO make sure we've got just two values in the tuple

figures/log.py

@@ -0,0 +1,38 @@
+"""Provides logging and instrumentation functionality for Figures
+
+"""
+
+from contextlib import contextmanager
+import logging
+import timeit
+
+
+default_logger = logging.getLogger(__name__)
+
+
+@contextmanager
+def log_exec_time(description, logger=None):
+    """Context handler to log execution time info for a block
+
+    Parameters:
+    description : The text to add to the log statement
+    logger : The logger to receive the log statement
+
+    If `logger' is not provided, then the default logger is used,
+
+        `logging.getLogger(__name__)`
+
+    Example:
+
+    ```
+    with log_exec_time('Collect grades for courses in site',logger=my_logger):
+        do_grades_collection(site=my_site)
+    ```
+    """
+    logger = logger if logger else default_logger
+    start_time = timeit.default_timer()
+    yield
+    elapsed = timeit.default_timer() - start_time
+    msg = '{}: {} s'.format(description, elapsed)
+
+    logger.info(msg)

figures/models.py

@@ -191,7 +191,7 @@ class LearnerCourseGradeMetricsManager(models.Manager):
     def most_recent_for_learner_course(self, user, course_id):
         queryset = self.filter(user=user, course_id=str(course_id))
         if queryset:
-            return queryset.order_by('-date_for')[0]  # pylint: disable=E1101
+            return queryset.order_by('-date_for')[0]
         else:
             return None
 
@@ -234,7 +234,7 @@ def completed_for_site(self, site, **_kwargs):
             # We do the string casting in case couse_ids are CourseKey instance
             filter_args['course_id__in'] = [str(key) for key in course_ids]
         if filter_args:
-            qs = qs.filter(**filter_args)  # pylint: disable=E1101
+            qs = qs.filter(**filter_args)
         return qs
 
     def completed_ids_for_site(self, site, **_kwargs):
@@ -400,7 +400,7 @@ def latest_for_site_month(self, site, year, month):
         If no record found, returns 'None'
         """
         queryset = self.filter(site=site, date_for__year=year, date_for__month=month)
-        return queryset.order_by('-modified').first()  # pylint: disable=no-member
+        return queryset.order_by('-modified').first()
 
 
 @python_2_unicode_compatible
@@ -450,7 +450,7 @@ def latest_for_course_month(self, site, course_id, year, month):
             date_for__year=year,
             date_for__month=month,
         )
-        return queryset.order_by('-modified').first()  # pylint: disable=no-member
+        return queryset.order_by('-modified').first()
 
 
 @python_2_unicode_compatible

figures/pipeline/enrollment_metrics.py

@@ -166,7 +166,7 @@ def collect_metrics_for_enrollment(site, course_enrollment, course_sm, date_for=
     lcgm = LearnerCourseGradeMetrics.objects.filter(
         user=course_enrollment.user,
         course_id=str(course_enrollment.course_id))
-    most_recent_lcgm = lcgm.order_by('date_for').last()  # pylint: disable=E1101
+    most_recent_lcgm = lcgm.order_by('date_for').last()
 
     if _enrollment_metrics_needs_update(most_recent_lcgm, most_recent_sm):
         progress_data = _collect_progress_data(most_recent_sm)

figures/settings/lms_production.py

@@ -49,7 +49,7 @@ def update_celerybeat_schedule(celerybeat_schedule_settings, figures_env_tokens)
                 ),
             }
 
-    if figures_env_tokens.get('ENABLE_FIGURES_MONTHLY_METRICS', False):
+    if figures_env_tokens.get('ENABLE_FIGURES_MONTHLY_METRICS', True):
         celerybeat_schedule_settings['figures-monthly-metrics'] = {
             'task': 'figures.tasks.run_figures_monthly_metrics',
             'schedule': crontab(0, 0, day_of_month=1),

figures/tasks.py

@@ -16,6 +16,7 @@
 from student.models import CourseEnrollment  # pylint: disable=import-error
 
 from figures.helpers import as_course_key, as_date
+from figures.log import log_exec_time
 from figures.models import PipelineError
 from figures.pipeline.course_daily_metrics import CourseDailyMetricsLoader
 from figures.pipeline.site_daily_metrics import SiteDailyMetricsLoader
@@ -232,6 +233,8 @@ def populate_mau_metrics_for_site(site_id, month_for=None, force_update=False):
     TODO: Check results of 'store_mau_metrics' to log unexpected results
     """
     site = Site.objects.get(id=site_id)
+    msg = 'Starting figures'
+    logger.info(msg)
     for course_key in figures.sites.get_course_keys_for_site(site):
         populate_course_mau(site_id=site_id,
                             course_id=str(course_key),
@@ -253,8 +256,11 @@ def populate_all_mau():
 
 @shared_task
 def populate_monthly_metrics_for_site(site_id):
+
     site = Site.objects.get(id=site_id)
-    fill_last_smm_month(site=site)
+    msg = 'Ran populate_monthly_metrics_for_site. [{}]:{}'
+    with log_exec_time(msg.format(site.id, site.domain)):
+        fill_last_smm_month(site=site)
 
 
 @shared_task