fix: add error handling for potential issues calculating course progress (#36660)

* fix: add error handling for potential issues calculating course progress

Realized there is potential in a few places for exceptions to be thrown. I'm adding some more error handling in the course progress task and function to handle potential issues.

Author: justinhynes

lms/djangoapps/course_home_api/progress/api.py

@@ -7,7 +7,6 @@
 
 from lms.djangoapps.courseware.courses import get_course_blocks_completion_summary
 
-
 User = get_user_model()
 
 
@@ -28,9 +27,12 @@ def calculate_progress_for_learner_in_course(course_key: CourseKey, user: User)
     # refactored in the future so that the calculation is handled solely on the backend, eliminating the need for it to
     # be done in the frontend.
     num_total_units = complete_count + incomplete_count + locked_count
-    complete_percentage = round(complete_count / num_total_units, 2)
-    locked_percentage = round(locked_count / num_total_units, 2)
-    incomplete_percentage = 1.00 - complete_percentage - locked_percentage
+    if num_total_units == 0:
+        complete_percentage = locked_percentage = incomplete_percentage = 0.0
+    else:
+        complete_percentage = round(complete_count / num_total_units, 2)
+        locked_percentage = round(locked_count / num_total_units, 2)
+        incomplete_percentage = 1.00 - complete_percentage - locked_percentage
 
     return {
         "complete_count": complete_count,

lms/djangoapps/course_home_api/progress/tests/test_api.py

@@ -13,17 +13,17 @@ class ProgressApiTests(TestCase):
     """
     Tests for the progress calculation functions.
     """
+
     @patch("lms.djangoapps.course_home_api.progress.api.get_course_blocks_completion_summary")
     def test_calculate_progress_for_learner_in_course(self, mock_get_summary):
         """
         A test to verify functionality of the function under test.
         """
-        get_summary_return_val = {
+        mock_get_summary.return_value = {
             "complete_count": 5,
             "incomplete_count": 2,
             "locked_count": 1,
         }
-        mock_get_summary.return_value = get_summary_return_val
 
         expected_data = {
             "complete_count": 5,
@@ -39,6 +39,31 @@ def test_calculate_progress_for_learner_in_course(self, mock_get_summary):
         assert mock_get_summary.called_once_with("some_course", "some_user")
         assert results == expected_data
 
+    @patch("lms.djangoapps.course_home_api.progress.api.get_course_blocks_completion_summary")
+    def test_handle_division_by_zero(self, mock_get_summary):
+        """
+        A test to verify that we're avoiding division-by-zero errors if the total number of units is 0.
+        """
+        mock_get_summary.return_value = {
+            "complete_count": 0,
+            "incomplete_count": 0,
+            "locked_count": 0,
+        }
+
+        expected_data = {
+            "complete_count": 0,
+            "incomplete_count": 0,
+            "locked_count": 0,
+            "total_count": 0,
+            "complete_percentage": 0.0,
+            "locked_percentage": 0.0,
+            "incomplete_percentage": 0.0,
+        }
+
+        results = calculate_progress_for_learner_in_course("some_course", "some_user")
+        assert mock_get_summary.called_once_with("some_course", "some_user")
+        assert results == expected_data
+
     @patch("lms.djangoapps.course_home_api.progress.api.get_course_blocks_completion_summary")
     def test_calculate_progress_for_learner_in_course_summary_empty(self, mock_get_summary):
         """

lms/djangoapps/course_home_api/tasks.py

@@ -37,12 +37,19 @@ def collect_progress_for_user_in_course(course_id: str, user_id: str) -> None:
         log.warning(f"Could not retrieve a user with id {user_id}, aborting task.")
         return
 
+    try:
+        enrollment = get_course_enrollment(user, course_key)
+        enrollment_mode = enrollment.mode
+    except AttributeError:
+        log.warning(f"Could not retrieve enrollment info for user {user.id} in course {course_id}")
+        return
+
     progress = calculate_progress_for_learner_in_course(course_key, user)
-    enrollment = get_course_enrollment(user, course_key)
+
     # add a few extra fields to the returned data to make the event payload a bit more usable
     progress["user_id"] = user.id
     progress["course_id"] = course_id
-    progress["enrollment_mode"] = enrollment.mode
+    progress["enrollment_mode"] = enrollment_mode
 
     tracker.emit(
         COURSE_COMPLETION_FOR_USER_EVENT_NAME,

lms/djangoapps/course_home_api/tests/test_tasks.py

@@ -5,6 +5,7 @@
 from unittest.mock import patch
 
 from opaque_keys.edx.keys import CourseKey
+from testfixtures import LogCapture
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory
@@ -14,6 +15,8 @@
 )
 from openedx.core.djangoapps.catalog.tests.factories import CourseFactory, CourseRunFactory
 
+LOG_PATH = 'lms.djangoapps.course_home_api.tasks'
+
 
 class CalculateCompletionTaskTests(ModuleStoreTestCase):
     """
@@ -67,6 +70,27 @@ def test_successful_event_emission(self, mock_tracker, mock_progress):
             expected_data,
         )
 
+    @patch("lms.djangoapps.course_home_api.tasks.calculate_progress_for_learner_in_course")
+    @patch("lms.djangoapps.course_home_api.tasks.get_course_enrollment")
+    @patch("lms.djangoapps.course_home_api.tasks.tracker.emit")
+    def test_cannot_retrieve_enrollment_info(self, mock_tracker, mock_get_enrollment, mock_progress):
+        """
+        Test to ensure the task is aborted if we cannot retrieve enrollment info for the user in the specified course.
+        """
+        mock_get_enrollment.return_value = None
+
+        expected_message = (
+            f"Could not retrieve enrollment info for user {self.user.id} in course {self.course_run_key_string}"
+        )
+
+        with LogCapture() as log:
+            collect_progress_for_user_in_course(self.course_run_key_string, self.user.id)
+
+        mock_get_enrollment.assert_called_once_with(self.user, CourseKey.from_string(self.course_run_key_string))
+        log.check_present((LOG_PATH, "WARNING", expected_message),)
+        mock_progress.assert_not_called()
+        mock_tracker.assert_not_called()
+
     @patch("lms.djangoapps.course_home_api.tasks.calculate_progress_for_learner_in_course")
     @patch("lms.djangoapps.course_home_api.tasks.tracker.emit")
     def test_aborted_task_user_dne(self, mock_tracker, mock_progress):