fix: add courserun date validation #3400

Anas12091101 · 2025-02-11T14:12:49Z

What are the relevant tickets?

https://github.com/mitodl/hq/issues/5823

Description (What does it do?)

This PR adds the following date validation at course run creation:

Either start_date or enrollment_end should be present.
Either start_date or enrollment_end must be in the future.
End date must be later than start date.

Note: This PR also updates the tests which started to break due to validation changes

How can this be tested?

Creating a Course Run via Django Admin:
- Try creating a course run without any dates → You will get this error:
  Either start_date or enrollment_end must be provided.
- Add a start_date or enrollment_end in the past → You will get this error:
  Either start_date or enrollment_end must be in the future.
- Set the start_date to a future date and end_date before the start_date → You will get this error:
  End date must be later than start date.
- Set the start_date to a future date and end_date after start_date → The course run will be created successfully.
- Open the newly created course run, remove the start_date and set the enrollment_end to a future date → The course run will be updated successfully.
For external courses:
- The API provides course run data, some of which may have a past start date.
- Run the task_sync_external_course_runs from shell. Now, two scenarios can occur:
  - Course Run already exists in the system:
    - If the data from the API matches the existing data, no update is needed.
    - If the data from the API differs, the task will update it.
      - If the API data meets the validation rules, the Course Run will be updated.
      - Otherwise, the following error will be logged:
      Error updating course run for course: course-v1:xPRO+MCPO, course run code: MXP-MCPO-24-12#1, error: e
  - Course Run doesn't exist in the system and the data from the API fails to satisfy the validations. In this case, Error creating course run for course: course-v1:xPRO+MCPO, course run code: MXP-MCPO-24-12#1, error: e will be logged.
- Verify that the task doesn't break for all the cases.

marslanabdulrauf · 2025-02-18T09:04:05Z

courses/models.py

+            not self.enrollment_end or self.enrollment_end < now
+        ):
+            raise ValidationError(
+                "Either start_date or enrollment_end must be in the future."


Suggested change

"Either start_date or enrollment_end must be in the future."

"Either start date or enrollment end must be in the future."

marslanabdulrauf · 2025-02-18T09:09:13Z

courses/models.py

+
+        if not self.start_date and not self.enrollment_end:
+            raise ValidationError(
+                "Either start_date or enrollment_end must be provided."


Suggested change

"Either start_date or enrollment_end must be provided."

"Either start date or enrollment end must be provided."

marslanabdulrauf · 2025-02-18T09:20:36Z

courses/sync_external_courses/external_course_sync_api.py

+            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"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
+                    f"Creating or Updating Product and Product Version, course run courseware_id: {course_run.external_course_run_id}, Price: {external_course.price}"  # 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 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)
+                    if product_created:


nit: may be we can move this into separate functions to reduce nested blocks in single function

asadali145

@arslanashraf7 We are not validating the enrollment start date as it is not mentioned in the issue. With the current implementation, I can create a course run with enrollment start greater than enrollment end (both in future).

asadali145 · 2025-02-18T13:32:03Z

courses/sync_external_courses/external_course_sync_api.py

+            course_run = CourseRun.objects.create(
+                external_course_run_id=external_course.course_run_code,
+                course=course,
+                title=external_course.course_title,
+                courseware_id=course_run_courseware_id,
+                run_tag=external_course.course_run_tag,
+                start_date=external_course.start_date,
+                end_date=external_course.end_date,
+                enrollment_end=external_course.enrollment_end,
+                live=True,
+            )
+            is_created = True
+        except ValidationError as e:
+            log.error(
+                f"Error creating course run for course: {course.readable_id}, course run code: {external_course.course_run_code}, error: {e}"  # noqa: G004
+            )


Instead of trying to create a course run with bad data, can we validate the dates like we are validating the end date in update_external_course_runs? We won't need these extra changes and it would be much simpler. What are your thoughts?

asadali145 · 2025-02-18T13:57:18Z

courses/models.py

@@ -757,14 +757,28 @@ def clean(self):
        1. Later than end_date if end_date is set
        2. Later than start_date if start_date is set


We should update docs as per current logic.

arslanashraf7 · 2025-02-18T14:28:40Z

@arslanashraf7 We are not validating the enrollment start date as it is not mentioned in the issue. With the current implementation, I can create a course run with enrollment start greater than enrollment end (both in future).

At the time of the creation of the ticket, The main goal was to keep the catalog consistent and the catalog course criteria take the start date and enrollment end date into consideration while deciding if the course should be visible on the catalog or not.

I do remember discussing this problem of date validation somewhere but don't remember where, or if was it verbal or written at that time. I would defer to @cachob and @pdpinch to make a decision on this.

pdpinch · 2025-02-21T13:32:40Z

Can you help me understand this from a systems level? Do we have a history of bad data from the external partners? I understand the validation will raise an error. Will those errors manifest in Sentry? Do we know who will watch out for them and take action?

Anas12091101 · 2025-02-21T16:28:00Z

@pdpinch This PR mainly adds validations for course runs created manually via Django admin. Data from external vendors is usually correct, but we’ve added extra date checks before creating an external course run to prevent validation-related task failures.

Because of this, no validation errors will be raised or logged in Sentry when the task runs, even if the API sends bad data, it will just skip those course runs. Also, validation errors from the clean() method won’t show up in Sentry since Django already handles them and displays them on the form.

pdpinch · 2025-02-21T17:56:56Z

That's fine for the django admin.

Silent failures from the external partners' APIs might lead to confusion, but I'm not sure. @cachob maybe be able to comment, although it's probably out of scope for this PR.

marslanabdulrauf

Now that we've added validation to prevent courses from being created with invalid dates, we should update our test cases accordingly.

Bypassing validation to create courses without these dates no longer makes sense.
@arslanashraf7 any thoughts ?

marslanabdulrauf · 2025-02-24T15:45:57Z

mitxpro/utils.py

+    elif (not start_date or start_date < now) and (
+        not enrollment_end or enrollment_end < now
+    ):


Suggested change

elif (not start_date or start_date < now) and (

not enrollment_end or enrollment_end < now

):

elif (start_date and start_date < now) or (enrollment_end and enrollment_end < now):

marslanabdulrauf · 2025-02-24T15:48:34Z

mitxpro/utils.py

+    now = now_in_utc()
+    error_msg = None
+
+    if not start_date and not enrollment_end:


Its equivalent to:

Suggested change

if not start_date and not enrollment_end:

if not (start_date or enrollment_end):

marslanabdulrauf · 2025-02-24T15:58:24Z

courses/sync_external_courses/external_course_sync_api.py

+        if not (
+            external_course.validate_end_date()
+            and not bool(
+                get_courserun_date_errors(
+                    external_course.start_date,
+                    external_course.end_date,
+                    external_course.enrollment_end,
+                )
+            )
+        ):


Suggested change

if not (

external_course.validate_end_date()

and not bool(

get_courserun_date_errors(

external_course.start_date,

external_course.end_date,

external_course.enrollment_end,

)

)

):

if not external_course.validate_end_date() or get_courserun_date_errors(

external_course.start_date,

external_course.end_date,

external_course.enrollment_end,

):

Anas12091101 · 2025-02-24T18:42:44Z

Bypassing validation to create courses without these dates no longer makes sense.

Sometimes we need to test for scenarios when course run is expired (having past dates). For this, I have added the clean_disabled attribute which helps us in creating a course run without running the clean() fn.

asadali145 · 2025-02-26T11:25:25Z

mitxpro/utils.py

@@ -646,3 +646,52 @@ 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(


A better name would be validate_courserun_dates. It can return a tuple of True/False, Message

asadali145 · 2025-02-26T11:26:40Z

courses/sync_external_courses/external_course_sync_api.py

@@ -339,11 +344,20 @@ 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_end_date()


We are validating this in get_courserun_date_errors, Do we need this now?

We are validating this in get_courserun_date_errors, Do we need this now?

I think we are not validating end_date in get_courserun_date_errors. Should we add this validation there as well? Initially the ticket asked validations for start and enrollment_end date and that's why I didn't add the end date validations in get_courserun_date_errors

asadali145 · 2025-02-26T11:26:55Z

courses/models.py

-
-        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(


Suggested change

errorMsg = get_courserun_date_errors(

error_msg = get_courserun_date_errors(

cachob · 2025-02-26T16:24:58Z

@pdpinch / @Anas12091101 - I believe with the checks in place that we have for external courses should suffice. I don't see any urgent reason why these should take up Sentry overhead - if ever there is bad data in these endpoints it should be cleaned up by the vendors before they send it over as a best practice. I agree that this is out scope, and this could be dealt with the reporting tools we're looking to develop for our vendor APIs

odlbot temporarily deployed to xpro-ci-pr-3400 February 11, 2025 14:14 Inactive

odlbot temporarily deployed to xpro-ci-pr-3400 February 11, 2025 14:17 Inactive

Anas12091101 requested a review from arslanashraf7 February 11, 2025 14:47

Anas12091101 added Needs Review Waiting on Author and removed Needs Review labels Feb 11, 2025

Anas12091101 added 3 commits February 13, 2025 16:19

feat: add validations for dates in course runs

e78739a

fix: tests

2bee0f3

fix: handling an edge case

90185d0

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from 29f8bbf to 90185d0 Compare February 13, 2025 11:32

odlbot temporarily deployed to xpro-ci-pr-3400 February 13, 2025 11:32 Inactive

odlbot temporarily deployed to xpro-ci-pr-3400 February 13, 2025 11:33 Inactive

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from ab801c5 to 7e0b918 Compare February 13, 2025 12:46

odlbot temporarily deployed to xpro-ci-pr-3400 February 13, 2025 12:46 Inactive

odlbot temporarily deployed to xpro-ci-pr-3400 February 13, 2025 12:47 Inactive

fix: flaky tests

c2178ee

arslanashraf7 changed the title ~~Anas/add courserun date validation~~ fix: add courserun date validation Feb 14, 2025

fix: flaky tests

ff9fb44

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from 31b782e to ff9fb44 Compare February 14, 2025 07:38

odlbot deployed to xpro-ci-pr-3400 February 14, 2025 07:39 View deployment

Anas12091101 added Needs Review and removed Waiting on Author labels Feb 14, 2025

marslanabdulrauf requested review from marslanabdulrauf and removed request for arslanashraf7 February 18, 2025 08:46

marslanabdulrauf requested changes Feb 18, 2025

View reviewed changes

marslanabdulrauf added Waiting on Author and removed Needs Review labels Feb 18, 2025

asadali145 reviewed Feb 18, 2025

View reviewed changes

Anas12091101 force-pushed the anas/add-courserun-date-validation branch 2 times, most recently from 225498f to 953ba0b Compare February 21, 2025 11:19

fix: issues

01585ab

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from fe39cb1 to 01585ab Compare February 21, 2025 11:24

Anas12091101 added 2 commits February 21, 2025 11:52

fix: tests

8ecc8c4

fix: issues

e8adab1

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from a8b09f1 to e8adab1 Compare February 21, 2025 13:11

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from 9c1e654 to c7edd99 Compare February 21, 2025 16:00

fix: issues

935a91d

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from 52be026 to 935a91d Compare February 21, 2025 16:12

fix: flaky tests

1661ed4

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from 4ad2365 to 1661ed4 Compare February 24, 2025 09:17

Anas12091101 added Needs Review and removed Waiting on Author labels Feb 24, 2025

Anas12091101 requested review from asadali145 and marslanabdulrauf February 24, 2025 09:58

marslanabdulrauf requested changes Feb 24, 2025

View reviewed changes

asadali145 requested changes Feb 26, 2025

View reviewed changes

fix: issues

ecfa920

Anas12091101 force-pushed the anas/add-courserun-date-validation branch 2 times, most recently from 1ac7031 to e270dbf Compare February 27, 2025 13:07

fix: issues

0bd3168

Anas12091101 force-pushed the anas/add-courserun-date-validation branch from dcf3f2a to 0bd3168 Compare February 27, 2025 14:41

Anas12091101 requested review from marslanabdulrauf and asadali145 February 27, 2025 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add courserun date validation #3400

fix: add courserun date validation #3400

Anas12091101 commented Feb 11, 2025 •

edited

Loading

marslanabdulrauf Feb 18, 2025

marslanabdulrauf Feb 18, 2025

marslanabdulrauf Feb 18, 2025

asadali145 left a comment

asadali145 Feb 18, 2025

asadali145 Feb 18, 2025

arslanashraf7 commented Feb 18, 2025

pdpinch commented Feb 21, 2025

Anas12091101 commented Feb 21, 2025

pdpinch commented Feb 21, 2025

marslanabdulrauf left a comment

marslanabdulrauf Feb 24, 2025

marslanabdulrauf Feb 24, 2025

marslanabdulrauf Feb 24, 2025

Anas12091101 commented Feb 24, 2025

asadali145 Feb 26, 2025

asadali145 Feb 26, 2025

Anas12091101 Feb 26, 2025

asadali145 Feb 26, 2025

cachob commented Feb 26, 2025

	"Either start_date or enrollment_end must be in the future."
	"Either start date or enrollment end must be in the future."

		@@ -757,14 +757,28 @@ def clean(self):
		1. Later than end_date if end_date is set
		2. Later than start_date if start_date is set

	if not start_date and not enrollment_end:
	if not (start_date or enrollment_end):

	errorMsg = get_courserun_date_errors(
	error_msg = get_courserun_date_errors(

fix: add courserun date validation #3400

Are you sure you want to change the base?

fix: add courserun date validation #3400

Conversation

Anas12091101 commented Feb 11, 2025 • edited Loading

What are the relevant tickets?

Description (What does it do?)

How can this be tested?

Creating a Course Run via Django Admin:

For external courses:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asadali145 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 commented Feb 18, 2025

pdpinch commented Feb 21, 2025

Anas12091101 commented Feb 21, 2025

pdpinch commented Feb 21, 2025

marslanabdulrauf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anas12091101 commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cachob commented Feb 26, 2025

Anas12091101 commented Feb 11, 2025 •

edited

Loading