Update backfill enrollment management command

* Add ability to load course ids from a file
* Add test coverage

devsite/requirements/ginkgo.txt

@@ -83,3 +83,6 @@ freezegun==0.3.12
 
 # Added to address: TypeError: attrib() got an unexpected keyword argument 'convert'
 attrs==19.1.0
+
+# Added to enable mock.mock_open using 'builtins.open'
+future==0.18.2

devsite/requirements/hawthorn_base.txt

@@ -95,3 +95,6 @@ edx-lint==0.5.5
 
 # 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
+
+# Added to enable mock.mock_open using 'builtins.open'
+future==0.18.2

figures/management/commands/backfill_figures_enrollment_data.py

@@ -37,6 +37,7 @@
 from __future__ import print_function
 from __future__ import absolute_import
 
+import argparse
 from textwrap import dedent
 from django.contrib.sites.models import Site
 from django.core.management.base import BaseCommand, CommandError
@@ -82,7 +83,7 @@ def get_sites(self, options):
             sites.append(Site.objects.get(domain=site_identifier))
         return set(sites)
 
-    def get_course_ids(self, options):
+    def get_course_ids_from_courses_param(self, options):
         """Return a validated list of course id strings.
 
         If no courses are specified, returns an empty list
@@ -101,23 +102,47 @@ def get_course_ids(self, options):
             course_ids.append(str(course_overview.id))
         return course_ids
 
-    def update_enrollments(self, course_ids, use_celery=True):
+    def get_course_ids_from_file(self, options):
+        """"Gets a list of verified course ids from a flat file
+        Initially, we start with a flat file of course ids
+        The '#' sign is a comment
+        """
+        course_ids = []
+        lines = options['courses_file'].read().splitlines()
+        for line in lines:
+            line = line.strip()
+            if line and not line.startswith('#'):
+                course_overview = CourseOverview.objects.get(id=as_course_key(line))
+                course_ids.append(str(course_overview.id))
+        return course_ids
+
+    def update_enrollments(self, course_ids, use_celery):
+        """This method calls the Celery task in delay or immediate mode
+
+        TODO: If running as Celery tasks, improve the function, collect the
+        task ids, return them to the caller (Currently it is the `handle`
+        function) and report on processing state. Allow the user to quit and
+        the tasks keep running.
+        """
         for course_id in course_ids:
+            print('Updating enrollment data for course "{}"'.format(str(course_id)))
             if use_celery:
                 # Call the Celery task with delay
                 backfill_enrollment_data_for_course.delay(str(course_id))
+
             else:
                 # Call the Celery task immediately
                 backfill_enrollment_data_for_course(str(course_id))
 
     def add_arguments(self, parser):
         """
-
         If site domain or id or set of ides are provided, but no course or
         courses provided, runs for all courses in the site
 
         If a course id or list of course ids provided, processes those courses
-        If a site
+        If a site domain or list of site domains, process those, unless course_ids
+        are provided. If course ids are provided, those are processed and sites
+        ignored.
         """
         parser.add_argument('--sites',
                             nargs='+',
@@ -127,15 +152,28 @@ def add_arguments(self, parser):
                             nargs='+',
                             default=[],
                             help='backfill a specific course. provide course id string')
+        parser.add_argument('--courses-file', type=argparse.FileType('r'),
+                            help='Course IDs file. A flat file with course ids')
+        # TODO: document this
+        # we want to allow explicit --use-celery false
         parser.add_argument('--use-celery',
                             action='store_true',
-                            default=True,
-                            help='Run with Celery worker. Set to false to run in app space ')
+                            default=False,
+                            help='Run with Celery worker. Default is to run in immediate mode')
 
     def handle(self, *args, **options):
+
         print('BEGIN: Backfill Figures EnrollmentData')
         sites = self.get_sites(options)
-        course_ids = self.get_course_ids(options)
+
+        if options['courses_file']:
+            # Implmented two different methods just to keep them separate for
+            # this release and make it clear where the course ids are from
+            # Later on, we might abstract this to just `seof.get_course_ids(options)`
+            course_ids = self.get_course_ids_from_file(options)
+        else:
+            course_ids = self.get_course_ids_from_courses_param(options)
+
         use_celery = options['use_celery']
 
         # Would it benefit to implement a generator method to yield the course id?

tests/commands/test_backfill_figures_enrollment_data_command.py

@@ -1,7 +1,6 @@
-"""Test Figures Django management commands
+"""Test Figures Django management command, 'backfill_figures_enrollment_data'
 """
 from __future__ import absolute_import
-
 import pytest
 import mock
 
@@ -24,17 +23,24 @@ class TestBackfillEnrollmentDataCommand(object):
         * has one course id, has two course ids
         * has one or more site domains, and one or more courses
             * expects to ignore the sites arg and only process the courses arg
+
+    ## Test class notes:
+
+    We really only need to test immediate or delay mode once, as the comand
+    class's `update_enrollments` method is called whether site or course identifiers
+    are provided.
     """
     TASK = 'figures.tasks.backfill_enrollment_data_for_course'
     MANAGEMENT_COMMAND = 'backfill_figures_enrollment_data'
 
+    @pytest.mark.skip()
     @pytest.mark.parametrize('domains', [
         ['alpha.test'], ['alpha.test', 'bravo.test']
     ])
     @pytest.mark.parametrize('delay_suffix, do_delay', [
         ('', False), ('.delay', True)
     ])
-    def test_backfill_with_sites_arg(self, domains, delay_suffix, do_delay):
+    def test_with_sites_arg(self, domains, delay_suffix, do_delay):
         sites = [SiteFactory(domain=domain) for domain in domains]
         courses = [CourseOverviewFactory() for _ in range(2)]
         course_ids = [str(obj.id) for obj in courses]
@@ -51,11 +57,15 @@ def test_backfill_with_sites_arg(self, domains, delay_suffix, do_delay):
                 call_command(self.MANAGEMENT_COMMAND, **kwargs)
                 assert mock_task.has_calls(calls)
 
+    @pytest.mark.skip()
     @pytest.mark.parametrize('course_ids', [
         ['course-v1:TestOrg+T01+run'],
         ['course-v1:TestOrg+T01+run', 'course-v1:TestOrg+T02+run'],
     ])
-    def test_backfill_with_courses_arg_immediate(self, course_ids):
+    @pytest.mark.parametrize('delay_suffix, use_celery', [
+        ('', False), ('.delay', True)
+    ])
+    def test_with_courses_arg(self, course_ids, delay_suffix, use_celery):
         """Test called with courses arg and not run in Celery worker
 
         This tests that the expected task function is called with specific
@@ -80,27 +90,18 @@ def test_backfill_with_courses_arg_immediate(self, course_ids):
         kwargs = {'courses': course_ids, 'use_celery': False}
         calls = [mock.call(course_id) for course_id in course_ids]
 
-        with mock.patch(self.TASK) as mock_task:
-            with mock.patch(self.TASK + '.delay') as mock_task_delay:
-                call_command(self.MANAGEMENT_COMMAND, **kwargs)
-                assert mock_task.has_calls(calls)
-                assert not mock_task_delay.called
+        with mock.patch(self.TASK + delay_suffix) as mock_task:
+            call_command(self.MANAGEMENT_COMMAND, **kwargs)
+            assert mock_task.has_calls(calls)
 
-    @pytest.mark.parametrize('course_ids', [
-        ['course-v1:TestOrg+T01+run'],
-        ['course-v1:TestOrg+T01+run', 'course-v1:TestOrg+T02+run'],
-    ])
-    def test_backfill_with_courses_arg_delay(self, course_ids):
-        """Test called with courses arg and run in Celery worker
+    def test_with_courses_simple_list_file(self):
+        course_ids = ['course-v1:TestOrg+T0{}+run'.format(i) for i in range(3)]
 
-        See comment in the test method immediate above this one.
-        """
         [CourseOverviewFactory(id=cid) for cid in course_ids]
-        kwargs = {'courses': course_ids, 'use_celery': True}
-        calls = [mock.call(course_id) for course_id in course_ids]
+        file_contents = "# some comment here. Blah, blah, blah"
+        for course_id in course_ids:
+            file_contents += '\n' + course_id
 
-        with mock.patch(self.TASK) as mock_task:
-            with mock.patch(self.TASK + '.delay') as mock_task_delay:
-                call_command(self.MANAGEMENT_COMMAND, **kwargs)
-                assert mock_task_delay.has_calls(calls)
-                assert not mock_task.called
+        with mock.patch('builtins.open', mock.mock_open(read_data=file_contents)) as mo:
+            kwargs = {'courses_file': mo.return_value}
+            call_command(self.MANAGEMENT_COMMAND, **kwargs)