feat: [FC-7879] add signal handler to save assignment dates to Conten… by kyrylo-kh · Pull Request #347 · openedx/edx-when

kyrylo-kh · 2026-02-09T13:14:15Z

NOTE: Depends on #343

Description

Adds a new signal handler to support saving assignment due dates to the ContentDate model in the edx-when service.

Key Changes:

Introduces the _Assignment data class to standardize assignment metadata (title, date, block key, type).
Implements update_or_create_assignments_due_dates() to:
- Create or update ContentDate entries with assignment titles, dates, and metadata.
- Store related information like block_type, subsection_name, and course_name.
- Ensure DatePolicy entries are reused or created as needed.
Adds logging and input validation to handle incomplete or invalid assignment data gracefully.

Testing:

Includes a full test suite verifying:

New entries are inserted correctly.
Existing entries are updated when data changes.
Graceful handling of:
- Assignments with missing title or date.
- Multiple assignments at once.
- Failures during DatePolicy creation.

openedx-webhooks · 2026-02-09T13:14:20Z

Thanks for the pull request, @kyrylo-kh!

This repository is currently maintained by @openedx/openedx-unmaintained.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

e0d · 2026-02-10T15:59:02Z

@kyrylo-kh the branch is reporting as out of date, but I don't have the ability to pull in the latest from base. Can you do so an push it up.

e0d · 2026-02-10T15:59:57Z

@kyrylo-kh sorry, there also seem to be a bunch of failing tests. Perhaps a rebase will also fix this, but please have a look.

e0d · 2026-02-10T17:59:47Z

+            block_type=assignment.assignment_type,
+            defaults={
+                'policy': models.DatePolicy.objects.get_or_create(abs_date=assignment.date)[0],
+                'assignment_title': assignment.title,


This section is a little strange to me. assignment.title is used twice and is unlikely to be the reliably the subsection_name.

Do you mean it can confuse having subsection_name in model for different type of content?
We wrote description of this in argument:

assignments: List of Assignment instances. Assignments with missing date or title are skipped (with a warning). Use Assignment.subsection_name for the containing subsection when available; it is stored separately from assignment_title.

e0d · 2026-02-10T18:00:27Z

+                course_key_str
+            )
+            continue
+        models.ContentDate.objects.update_or_create(


There's no transactional boundary set here.

It exists - https://github.com/openedx/edx-when/pull/347/changes#diff-c3d98af45b99fefbf88d009b7b89c13a1fdea947cb57f3523c1af0c5fa8c4899R594

…tDate model

…d-process

edschema · 2026-06-01T16:40:39Z

+                defaults={
+                    'policy': models.DatePolicy.objects.get_or_create(abs_date=assignment.date)[0],
+                    'assignment_title': assignment.title,
+                    'course_name': course_key.course,


Docstring says course_key can be a string; if a string is passed to this fn, a string caller will raise an attribute error. Suggestion: normalize course_key with " _ensure_key(CourseKey, course_key)"

Fixed - c875945

edschema · 2026-06-01T16:42:28Z

+                course_id=course_key,
+                location=assignment.block_key,
+                field='due',
+                block_type=assignment.assignment_type,


block_type is used elsewhere as the structural XBlock type. Suggestion: define assignment_type if we need. But a more general question: Is the block_type or assignment_type needed to identify an existing date row?

Identity question: No. Row identity = (location, field) (unique_together policy,location,field; location encodes course+block). block_type/assignment_type not needed to match.
I moved block_type out of lookup to defaults. c875945
Value now assignment.block_key.block_type (structural: 'sequential'), matching usage everywhere else

pkulkark · 2026-06-16T14:22:08Z

+                location=assignment.block_key,
+                field='due',
+                defaults={
+                    'policy': models.DatePolicy.objects.get_or_create(abs_date=assignment.date)[0],


Won't this reintroduce the race condition noted in line 430?

Agree, as @pkulkark mentioned, this get_or_create for policy may be problematic due to race condition with DatePolicy creation mentioned in set_date_for_block.

I believe it's necessary to reuse _set_content_date_policy from set_date_for_block.
Also, it will be good to add a test case against DatePolicy duplicate.

pkulkark · 2026-06-16T14:28:10Z

+    title: str
+    date: datetime | None
+    block_key: UsageKey
+    assignment_type: str


This field doesn't seem to be persisted. Instead assignment.block_key.block_type is used. Either persist it or remove it from the dataclass.

GlugovGrGlib

Thanks, left several suggestions.

In addition, if this PR is planned to be merged after the #346, it's necessary to include version bump and a changelog update for both changes.

GlugovGrGlib · 2026-06-17T09:42:59Z

+    """
+    course_key = _ensure_key(CourseKey, course_key)
+    course_key_str = str(course_key)
+    with transaction.atomic():


Will be good to have a test case for the atomic boundary in this method

GlugovGrGlib · 2026-06-17T10:43:03Z



+@dataclass
+class Assignment:


suggestion (optional): this dataclass can be extracted into data.py

GlugovGrGlib · 2026-06-17T11:20:51Z

+                location=assignment.block_key,
+                field='due',
+                defaults={
+                    'policy': models.DatePolicy.objects.get_or_create(abs_date=assignment.date)[0],


Agree, as @pkulkark mentioned, this get_or_create for policy may be problematic due to race condition with DatePolicy creation mentioned in set_date_for_block.

I believe it's necessary to reuse _set_content_date_policy from set_date_for_block.
Also, it will be good to add a test case against DatePolicy duplicate.

GlugovGrGlib · 2026-06-17T11:36:42Z

+
+class TestUpdateAssignmentDatesForCourse(TestCase):
+    """
+    Tests for the update_assignment_dates_for_course task.


Looks like the classname and docstring are not correct as this test cases are for the update_or_create_assignments_due_dates

GlugovGrGlib · 2026-06-17T11:50:37Z

+
+    def test_assignment_with_null_date(self):
+        """
+        Test handling assignments with null dates.


Suggest adding a case when row already exists and null date is passed

GlugovGrGlib · 2026-06-17T11:57:09Z

+
+    def setUp(self):
+        self.course_key = CourseKey.from_string('course-v1:edX+DemoX+Demo_Course')
+        self.course_key_str = str(self.course_key)


self.course_key_str and self.staff_user are not used in the test cases, consider removing them

- Fix DatePolicy race: reuse extracted module-level _set_content_date_policy (filter-existing-then-create) instead of DatePolicy.get_or_create, and take block_type out of the ContentDate lookup (identity is course_id/location/field). - block_type now stores the structural XBlock type (block_key.block_type), matching platform usage; drop the unused assignment_type field from Assignment. - Normalize course_key via _ensure_key so a string course_key is accepted. - Tests: rename class to TestUpdateOrCreateAssignmentsDueDates and fix docstring, add DatePolicy-duplicate regression, atomic-rollback, and existing-row+null-date cases; drop unused setUp attributes.

kyrylo-kh · 2026-07-05T21:30:51Z

Please, take a look at latest commit

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 9, 2026

openedx-webhooks added this to Contributions Feb 9, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Feb 9, 2026

kyrylo-kh requested a review from e0d February 9, 2026 18:23

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Feb 9, 2026

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Feb 9, 2026

e0d requested changes Feb 10, 2026

View reviewed changes

e0d reviewed Feb 10, 2026

View reviewed changes

Comment thread edx_when/tests/test_api.py Outdated

kyrylo-kh force-pushed the rg/axm-dates-listen-to-course-published-and-process branch 5 times, most recently from 900b00c to 7c7451a Compare February 16, 2026 19:46

mphilbrick211 moved this from Waiting on Author to In Eng Review in Contributions Feb 25, 2026

feat: [FC-7879] add signal handler to save assignment dates to Conten…

906ef14

…tDate model

kyrylo-kh force-pushed the rg/axm-dates-listen-to-course-published-and-process branch from 7c7451a to 906ef14 Compare March 2, 2026 19:31

kyrylo-kh force-pushed the rg/axm-dates-listen-to-course-published-and-process branch from bd16f7c to f1229a9 Compare March 12, 2026 00:32

Merge branch 'master' into rg/axm-dates-listen-to-course-published-an…

9e50d46

…d-process

kyrylo-kh force-pushed the rg/axm-dates-listen-to-course-published-and-process branch from f1229a9 to 9e50d46 Compare March 12, 2026 00:35

edschema reviewed Jun 1, 2026

View reviewed changes

fix: address review on assignment due dates handler

c875945

pkulkark reviewed Jun 16, 2026

View reviewed changes

GlugovGrGlib suggested changes Jun 17, 2026

View reviewed changes

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jun 25, 2026

Uh oh!

Conversation

kyrylo-kh commented Feb 9, 2026

Description

Key Changes:

Testing:

Uh oh!

openedx-webhooks commented Feb 9, 2026

Uh oh!

e0d commented Feb 10, 2026

Uh oh!

e0d commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyrylo-kh Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GlugovGrGlib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyrylo-kh commented Jul 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kyrylo-kh Jun 1, 2026 •

edited

Loading