Merge pull request #5528 from edx/dan-f/data-download-tech-debt

Student profile CSV background task tech debt

common/test/acceptance/pages/lms/instructor_dashboard.py

@@ -146,3 +146,19 @@ class MembershipPage(PageObject):
         The first entry in the array is the title. Any further entries are the details.
         """
         return self._get_cohort_messages("errors")
+
+    def select_data_download(self):
+        """
+        Click on the link to the Data Download Page.
+        """
+        self.q(css="a.link-cross-reference[data-section=data_download]").first.click()
+
+
+class DataDownloadPage(PageObject):
+    """
+    Data Download section of the Instructor dashboard.
+    """
+    url = None
+
+    def is_browser_on_page(self):
+        return self.q(css='a[data-section=data_download].active-section').present

common/test/acceptance/tests/discussion/test_cohort_management.py

@@ -12,7 +12,7 @@ from .helpers import CohortTestMixin
 from ..helpers import UniqueCourseTest
 from ...fixtures.course import CourseFixture
 from ...pages.lms.auto_auth import AutoAuthPage
-from ...pages.lms.instructor_dashboard import InstructorDashboardPage
+from ...pages.lms.instructor_dashboard import InstructorDashboardPage, DataDownloadPage
 from ...pages.studio.settings_advanced import AdvancedSettingsPage
 
 import uuid
@@ -242,3 +242,16 @@ class CohortConfigurationTest(UniqueCourseTest, CohortTestMixin):
             }).count(),
             1
         )
+
+    def test_link_to_data_download(self):
+        """
+        Scenario: a link is present from the cohort configuration in
+        the instructor dashboard to the Data Download section.
+
+        Given I have a course with a cohort defined
+        When I view the cohort in the LMS instructor dashboard
+        There is a link to take me to the Data Download section of the Instructor Dashboard.
+        """
+        self.membership_page.select_data_download()
+        data_download_page = DataDownloadPage(self.browser)
+        data_download_page.wait_for_page()

lms/djangoapps/instructor_task/tasks.py

@@ -30,8 +30,8 @@ from instructor_task.tasks_helper import (
     rescore_problem_module_state,
     reset_attempts_module_state,
     delete_problem_module_state,
-    push_grades_to_s3,
-    push_students_csv_to_s3
+    upload_grades_csv,
+    upload_students_csv
 )
 from bulk_email.tasks import perform_delegate_email_batches
 
@@ -139,7 +139,7 @@ def calculate_grades_csv(entry_id, xmodule_instance_args):
     """
     # Translators: This is a past-tense verb that is inserted into task progress messages as {action}.
     action_name = ugettext_noop('graded')
-    task_fn = partial(push_grades_to_s3, xmodule_instance_args)
+    task_fn = partial(upload_grades_csv, xmodule_instance_args)
     return run_main_task(entry_id, task_fn, action_name)
 
 
@@ -151,5 +151,5 @@ def calculate_students_features_csv(entry_id, xmodule_instance_args):
     """
     # Translators: This is a past-tense verb that is inserted into task progress messages as {action}.
     action_name = ugettext_noop('generated')
-    task_fn = partial(push_students_csv_to_s3, xmodule_instance_args)
+    task_fn = partial(upload_students_csv, xmodule_instance_args)
     return run_main_task(entry_id, task_fn, action_name)

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -19,7 +19,7 @@ from xmodule.modulestore.tests.factories import CourseFactory
 from student.tests.factories import CourseEnrollmentFactory, UserFactory
 
 from instructor_task.models import ReportStore
-from instructor_task.tasks_helper import push_grades_to_s3, push_students_csv_to_s3, UPDATE_STATUS_SUCCEEDED
+from instructor_task.tasks_helper import upload_grades_csv, upload_students_csv
 
 
 class TestReport(ModuleStoreTestCase):
@@ -36,6 +36,7 @@ class TestReport(ModuleStoreTestCase):
     def create_student(self, username, email):
         student = UserFactory.create(username=username, email=email)
         CourseEnrollmentFactory.create(user=student, course_id=self.course.id)
+        return student
 
 
 @ddt.ddt
@@ -55,9 +56,26 @@ class TestInstructorGradeReport(TestReport):
         self.current_task.update_state = Mock()
         with patch('instructor_task.tasks_helper._get_current_task') as mock_current_task:
             mock_current_task.return_value = self.current_task
-            result = push_grades_to_s3(None, None, self.course.id, None, 'graded')
-        #This assertion simply confirms that the generation completed with no errors
-        self.assertEquals(result['succeeded'], result['attempted'])
+            result = upload_grades_csv(None, None, self.course.id, None, 'graded')
+        num_students = len(emails)
+        self.assertDictContainsSubset({'attempted': num_students, 'succeeded': num_students, 'failed': 0}, result)
+
+    @patch('instructor_task.tasks_helper._get_current_task')
+    @patch('instructor_task.tasks_helper.iterate_grades_for')
+    def test_grading_failure(self, mock_iterate_grades_for, _mock_current_task):
+        """
+        Test that any grading errors are properly reported in the
+        progress dict and uploaded to the report store.
+        """
+        # mock an error response from `iterate_grades_for`
+        mock_iterate_grades_for.return_value = [
+            (self.create_student('username', 'student@example.com'), {}, 'Cannot grade student')
+        ]
+        result = upload_grades_csv(None, None, self.course.id, None, 'graded')
+        self.assertDictContainsSubset({'attempted': 1, 'succeeded': 0, 'failed': 1}, result)
+
+        report_store = ReportStore.from_config()
+        self.assertTrue(any('grade_report_err' in item[0] for item in report_store.links_for(self.course.id)))
 
 
 @ddt.ddt
@@ -66,14 +84,15 @@ class TestStudentReport(TestReport):
     Tests that CSV student profile report generation works.
     """
     def test_success(self):
+        self.create_student('student', 'student@example.com')
         task_input = {'features': []}
         with patch('instructor_task.tasks_helper._get_current_task'):
-            result = push_students_csv_to_s3(None, None, self.course.id, task_input, 'calculated')
+            result = upload_students_csv(None, None, self.course.id, task_input, 'calculated')
         report_store = ReportStore.from_config()
         links = report_store.links_for(self.course.id)
 
         self.assertEquals(len(links), 1)
-        self.assertEquals(result, UPDATE_STATUS_SUCCEEDED)
+        self.assertDictContainsSubset({'attempted': 1, 'succeeded': 1, 'failed': 0}, result)
 
     @ddt.data([u'student', u'student\xec'])
     def test_unicode_usernames(self, students):
@@ -95,6 +114,7 @@ class TestStudentReport(TestReport):
         }
         with patch('instructor_task.tasks_helper._get_current_task') as mock_current_task:
             mock_current_task.return_value = self.current_task
-            result = push_students_csv_to_s3(None, None, self.course.id, task_input, 'calculated')
+            result = upload_students_csv(None, None, self.course.id, task_input, 'calculated')
         #This assertion simply confirms that the generation completed with no errors
-        self.assertEquals(result, UPDATE_STATUS_SUCCEEDED)
+        num_students = len(students)
+        self.assertDictContainsSubset({'attempted': num_students, 'succeeded': num_students, 'failed': 0}, result)

lms/static/coffee/src/instructor_dashboard/data_download.coffee

@@ -153,7 +153,7 @@ class ReportDownloads
 
     @$report_downloads_table = @$section.find ".report-downloads-table"
 
-    POLL_INTERVAL = 1000 * 60 * 5 # 5 minutes in ms
+    POLL_INTERVAL = 20000 # 20 seconds, just like the "pending instructor tasks" table
     @downloads_poller = new window.InstructorDashboard.util.IntervalManager(
       POLL_INTERVAL, => @reload_report_downloads()
     )