fix: issues

courses/management/commands/sync_external_course_runs.py

@@ -141,10 +141,10 @@ def log_stats(self, stats):
             f"External Course Run Codes: {stats.get('course_runs_skipped') or 0}.\n"
         )
         self.log_style_success(
-            f"Number of Expired Course Runs {len(stats['course_runs_expired'])}."
+            f"Number of Expired Course Runs {len(stats['course_runs_with_invalid_dates'])}."
         )
         self.log_style_success(
-            f"External Course Run Codes: {stats.get('course_runs_expired') or 0}.\n"
+            f"External Course Run Codes: {stats.get('course_runs_with_invalid_dates') or 0}.\n"
         )
         self.log_style_success(
             f"Number of Course Runs Deactivated {len(stats['course_runs_deactivated'])}."

courses/models.py

@@ -30,6 +30,7 @@
     first_matching_item,
     now_in_utc,
     serialize_model_object,
+    get_courserun_date_errors,
 )
 
 User = get_user_model()
@@ -749,30 +750,19 @@ def __str__(self):
 
     def clean(self):
         """
-        If expiration_date is not set:
-        1. If end_date is provided: set expiration_date to default end_date + 90 days.
-        2. If end_date is None, don't do anything.
-
-        Validate that the expiration date is:
-        1. Later than end_date if end_date is set
-        2. Later than start_date if start_date is set
+        Validate that the course run dates follow these rules:
+        - start_date or enrollment_end must be provided.
+        - start_date and enrollment_end (if provided) must be in the future.
+        - end_date must be later than start_date.
+        - enrollment_end must be later than enrollment_start.
+        - expiration_date must be later than start_date and end_date (if provided).
+
+        Raises:
+            ValidationError: If any of the date rules are violated.
         """
-        now = now_in_utc()
-
-        if not self.start_date and not self.enrollment_end:
-            raise ValidationError("Either start_date or enrollment_end must be provided.")  # noqa: EM101
-
-        if (not self.start_date or self.start_date < now) and (not self.enrollment_end or self.enrollment_end < now):
-            raise ValidationError("Either start_date or enrollment_end must be in the future.")  # noqa: EM101
-
-        if self.start_date and self.end_date and self.start_date > self.end_date:
-            raise ValidationError("End date must be later than start date.")  # noqa: EM101
-
-        if self.expiration_date:
-            if self.start_date and self.expiration_date < self.start_date:
-                raise ValidationError("Expiration date must be later than start date.")  # noqa: EM101
-            if self.end_date and self.expiration_date < self.end_date:
-                raise ValidationError("Expiration date must be later than end date.")  # noqa: EM101
+        errorMsg = get_courserun_date_errors(self.start_date, self.end_date, self.enrollment_end, self.enrollment_start, self.expiration_date)
+        if errorMsg:
+            raise ValidationError(errorMsg) # noqa: EM101
 
     def save(
         self,

courses/models_test.py

@@ -4,6 +4,7 @@
 
 import factory
 import pytest
+from datetime import timedelta
 from django.core.exceptions import ValidationError
 from django.db.models.deletion import ProtectedError
 from django.db.utils import IntegrityError
@@ -226,32 +227,27 @@ def test_courseware_url(settings):
     assert course_run_no_path.courseware_url is None
 
 
-@pytest.mark.parametrize(
-    "start_date, enrollment_end, end_date, expiration_date, expected_error",
-    [
-        (None, None, None, None, "Either start_date or enrollment_end must be provided."),
-        (now - timedelta(days=2), now - timedelta(days=2), None, None, "Either start_date or enrollment_end must be in the future."),
-        (now + timedelta(days=2), now + timedelta(days=3), now + timedelta(days=4), now + timedelta(days=5), None),
-        (now + timedelta(days=4), now + timedelta(days=3), now + timedelta(days=2), now + timedelta(days=5), "End date must be later than start date."),
-        (now + timedelta(days=2), now + timedelta(days=3), now + timedelta(days=4), now - timedelta(days=2), "Expiration date must be later than start date."),
-        (now + timedelta(days=2), now + timedelta(days=3), now + timedelta(days=4), now + timedelta(days=3), "Expiration date must be later than end date."),
-    ]
-)
-def test_course_run_clean(start_date, enrollment_end, end_date, expiration_date, expected_error):
-    course_run = CourseRunFactory.build(
-        start_date=start_date,
-        enrollment_end=enrollment_end,
-        end_date=end_date,
-        expiration_date=expiration_date,
+def test_clean_calls_get_courserun_date_errors(mocker):  
+    """  
+    Test that the `clean` method calls `get_courserun_date_errors` with the correct arguments.  
+    """  
+    course_run = CourseRunFactory.create(  
+        start_date=now + timedelta(-2),  
+        enrollment_start=now + timedelta(-2),  
+        enrollment_end=now + timedelta(2),  
+        end_date=now + timedelta(2),  
+        expiration_date=now + timedelta(2),  
+    )  
+
+    mock_validate = mocker.patch("courses.models.get_courserun_date_errors", return_value=None)
+    course_run.clean()
+    mock_validate.assert_called_once_with(
+        course_run.start_date,
+        course_run.end_date,
+        course_run.enrollment_end,
+        course_run.enrollment_start,
+        course_run.expiration_date
     )
-
-    if expected_error:
-        with pytest.raises(ValidationError) as exc_info:
-            course_run.clean()
-        assert expected_error in str(exc_info.value)
-    else:
-        course_run.clean()  # Should not raise any error
-
 
 @pytest.mark.parametrize("end_days,expected", [[-1, True], [1, False], [None, False]])  # noqa: PT006, PT007
 def test_course_run_past(end_days, expected):

courses/sync_external_courses/external_course_sync_api.py

@@ -32,7 +32,7 @@
     ExternalCourseSyncAPIClient,
 )
 from ecommerce.models import Product, ProductVersion
-from mitxpro.utils import clean_url, now_in_utc, strip_datetime
+from mitxpro.utils import clean_url, now_in_utc, strip_datetime, get_courserun_date_errors
 
 log = logging.getLogger(__name__)
 
@@ -206,11 +206,11 @@ def validate_list_currency(self):
             return False
         return True
 
-    def validate_end_date(self):
+    def validate_dates(self):
         """
-        Validates that the course end date is in the future.
+        Validates that the course run dates are valid.
         """
-        return self.end_date and now_in_utc() < self.end_date
+        return not bool(get_courserun_date_errors(self.start_date, self.end_date, self.enrollment_end))
 
 
 def fetch_external_courses(keymap):
@@ -301,7 +301,7 @@ def update_external_course_runs(external_courses, keymap):  # noqa: C901, PLR091
         "course_pages_created": set(),
         "course_pages_updated": set(),
         "course_runs_skipped": set(),
-        "course_runs_expired": set(),
+        "course_runs_with_invalid_dates": set(),
         "products_created": set(),
         "product_versions_created": set(),
         "course_runs_without_prices": set(),
@@ -340,11 +340,11 @@ def update_external_course_runs(external_courses, keymap):  # noqa: C901, PLR091
             stats["course_runs_skipped"].add(external_course.course_run_code)
             continue
 
-        if not external_course.validate_end_date():
+        if not external_course.validate_dates():
             log.info(
-                f"Course run is expired, Skipping... Course data: {json.dumps(external_course_json)}"  # noqa: G004
+                f"Course run has invalid dates, Skipping... Course data: {json.dumps(external_course_json)}"  # noqa: G004
             )
-            stats["course_runs_expired"].add(external_course.course_run_code)
+            stats["course_runs_with_invalid_dates"].add(external_course.course_run_code)
             continue
 
         with transaction.atomic():
@@ -377,46 +377,46 @@ def update_external_course_runs(external_courses, keymap):  # noqa: C901, PLR091
             course_run, course_run_created, course_run_updated = (
                 create_or_update_external_course_run(course, external_course)
             )
-            if course_run:
-                if course_run_created:
-                    stats["course_runs_created"].add(course_run.external_course_run_id)
-                    log.info(
-                        f"Created Course Run, title: {external_course.course_title}, external_course_run_id: {course_run.external_course_run_id}"  # noqa: G004
-                    )
-                elif course_run_updated:
-                    stats["course_runs_updated"].add(course_run.external_course_run_id)
-                    log.info(
-                        f"Updated Course Run, title: {external_course.course_title}, external_course_run_id: {course_run.external_course_run_id}"  # noqa: G004
-                    )
 
+            if course_run_created:
+                stats["course_runs_created"].add(course_run.external_course_run_id)
                 log.info(
-                    f"Creating or Updating Product and Product Version, course run courseware_id: {course_run.external_course_run_id}, Price: {external_course.price}"  # noqa: G004
+                    f"Created Course Run, title: {external_course.course_title}, external_course_run_id: {course_run.external_course_run_id}"  # noqa: G004
+                )
+            elif course_run_updated:
+                stats["course_runs_updated"].add(course_run.external_course_run_id)
+                log.info(
+                    f"Updated Course Run, title: {external_course.course_title}, external_course_run_id: {course_run.external_course_run_id}"  # noqa: G004
+                )
+
+            log.info(
+                f"Creating or Updating Product and Product Version, course run courseware_id: {course_run.external_course_run_id}, Price: {external_course.price}"  # noqa: G004
+            )
+
+            if external_course.price:
+                product_created, product_version_created = (
+                    create_or_update_product_and_product_version(
+                        external_course, course_run
+                    )
                 )
+                if product_created:
+                    stats["products_created"].add(course_run.external_course_run_id)
+                    log.info(
+                        f"Created Product for course run: {course_run.courseware_id}"  # noqa: G004
+                    )
 
-                if external_course.price:
-                    product_created, product_version_created = (
-                        create_or_update_product_and_product_version(
-                            external_course, course_run
-                        )
+                if product_version_created:
+                    stats["product_versions_created"].add(
+                        course_run.external_course_run_id
                     )
-                    if product_created:
-                        stats["products_created"].add(course_run.external_course_run_id)
-                        log.info(
-                            f"Created Product for course run: {course_run.courseware_id}"  # noqa: G004
-                        )
-
-                    if product_version_created:
-                        stats["product_versions_created"].add(
-                            course_run.external_course_run_id
-                        )
-                        log.info(
-                            f"Created Product Version for course run: {course_run.courseware_id}, Price: {external_course.price}"  # noqa: G004
-                        )
-                else:
                     log.info(
-                        f"Price is Null for course run code: {external_course.course_run_code}"  # noqa: G004
+                        f"Created Product Version for course run: {course_run.courseware_id}, Price: {external_course.price}"  # noqa: G004
                     )
-                    stats["course_runs_without_prices"].add(external_course.course_run_code)
+            else:
+                log.info(
+                    f"Price is Null for course run code: {external_course.course_run_code}"  # noqa: G004
+                )
+                stats["course_runs_without_prices"].add(external_course.course_run_code)
 
             log.info(
                 f"Creating or Updating course page, title: {external_course.course_title}, course_code: {external_course.course_run_code}"  # noqa: G004

courses/sync_external_courses/external_course_sync_api_test.py

@@ -589,7 +589,7 @@ def test_update_external_course_runs(  # noqa: PLR0915, PLR0913
     num_product_versions_created = 2 if create_existing_data else 4
     assert len(courses) == 4
     assert len(stats["course_runs_skipped"]) == 2
-    assert len(stats["course_runs_expired"]) == 1
+    assert len(stats["course_runs_with_invalid_dates"]) == 1
     assert len(stats["courses_created"]) == num_courses_created
     assert len(stats["existing_courses"]) == num_existing_courses
     assert len(stats["course_runs_created"]) == num_course_runs_created
@@ -603,7 +603,7 @@ def test_update_external_course_runs(  # noqa: PLR0915, PLR0913
     for external_course_run in external_course_runs:
         if (
             external_course_run["course_run_code"] in stats["course_runs_skipped"]
-            or external_course_run["course_run_code"] in stats["course_runs_expired"]
+            or external_course_run["course_run_code"] in stats["course_runs_with_invalid_dates"]
         ):
             continue
 
@@ -956,22 +956,22 @@ def test_external_course_validate_list_currency(
     [{"platform": EMERITUS_PLATFORM_NAME}, {"platform": GLOBAL_ALUMNI_PLATFORM_NAME}],
     indirect=True,
 )
-@pytest.mark.parametrize(
-    ("end_date", "is_valid"),
-    [
-        (now_in_utc() + timedelta(days=1), True),
-        (now_in_utc() - timedelta(days=1), False),
-    ],
-)
-def test_external_course_validate_end_date(external_course_data, end_date, is_valid):
+def test_external_course_validate_dates(mocker, external_course_data):
     """
-    Tests that the valid end date is in the future for External courses.
+    Tests that the new external course run dates are valid.
     """
     keymap = get_keymap(external_course_data["course_run_code"])
     external_course = ExternalCourse(external_course_data, keymap=keymap)
-    external_course.end_date = end_date
-    assert external_course.validate_end_date() == is_valid
-
+    mock_validate = mocker.patch("courses.sync_external_courses.external_course_sync_api.get_courserun_date_errors")
+    mock_validate.return_value = None
+    assert external_course.validate_dates() is True
+    mock_validate.assert_called_with(
+        external_course.start_date,
+        external_course.end_date,
+        external_course.enrollment_end
+    )
+    mock_validate.return_value = "Some error message"
+    assert external_course.validate_dates() is False
 
 @pytest.mark.parametrize(
     (

mitxpro/utils.py

@@ -646,3 +646,47 @@ def strip_datetime(date_str, date_format, date_timezone=None):
 
     date_timezone = date_timezone if date_timezone else datetime.UTC
     return datetime.datetime.strptime(date_str, date_format).astimezone(date_timezone)
+
+def get_courserun_date_errors(start_date, end_date, enrollment_end, enrollment_start=None, expiration_date=None):
+    """
+    Check course run dates for validity and return an error message if invalid.
+
+    Rules:
+    - start_date or enrollment_end must be provided.
+    - start_date and enrollment_end (if provided) must be in the future.
+    - end_date must be later than start_date.
+    - enrollment_end must be later than enrollment_start.
+    - expiration_date must be later than start_date and end_date (if provided).
+
+    Args:
+        start_date (datetime or None): The start date of the course run.
+        end_date (datetime or None): The end date of the course run.
+        enrollment_start (datetime or None): The enrollment start date.
+        enrollment_end (datetime or None): The enrollment end date.
+        expiration_date (datetime or None): The expiration date of the course run.
+
+    Returns:
+        str or None: An error message if validation fails, otherwise None.
+    """
+    now = now_in_utc()
+    error_msg = None
+
+    if not start_date and not enrollment_end:
+        error_msg = "start_date or enrollment_end must be provided."
+
+    elif (not start_date or start_date < now) and (not enrollment_end or enrollment_end < now):
+        error_msg = "start_date or enrollment_end must be in the future."
+
+    elif start_date and end_date and start_date > end_date:
+        error_msg = "end_date must be later than start_date."
+
+    elif enrollment_start and enrollment_end and enrollment_start > enrollment_end:
+        error_msg = "enrollment_end must be later than enrollment_start."
+
+    elif expiration_date:
+        if start_date and expiration_date < start_date:
+            error_msg = "expiration_date must be later than start_date."
+        elif end_date and expiration_date < end_date:
+            error_msg = "expiration_date must be later than end_date."
+
+    return error_msg