From 906ef14f1396fb89d11a238ff9eee17ffc11b6cf Mon Sep 17 00:00:00 2001 From: Kyrylo Kholodenko Date: Mon, 9 Feb 2026 15:11:14 +0200 Subject: [PATCH 1/3] feat: [FC-7879] add signal handler to save assignment dates to ContentDate model --- edx_when/api.py | 74 +++++++++++++++- edx_when/tests/__init__.py | 0 edx_when/tests/test_api.py | 173 +++++++++++++++++++++++++++++++++++++ tox.ini | 1 + 4 files changed, 247 insertions(+), 1 deletion(-) create mode 100644 edx_when/tests/__init__.py create mode 100644 edx_when/tests/test_api.py diff --git a/edx_when/api.py b/edx_when/api.py index 358747b7..98ce55d1 100644 --- a/edx_when/api.py +++ b/edx_when/api.py @@ -3,7 +3,8 @@ """ import logging -from datetime import timedelta +from dataclasses import dataclass +from datetime import datetime, timedelta from django.core.exceptions import ValidationError from django.db import transaction @@ -543,6 +544,77 @@ def get_schedules_with_due_date(course_id, assignment_date): return schedules +@dataclass +class Assignment: + """ + Represents an assignment with a title, date, block key, assignment type, and optional subsection name. + """ + + title: str + date: datetime | None + block_key: UsageKey + assignment_type: str + subsection_name: str = '' + + def __post_init__(self): + """ + Validate the assignment object. + """ + if self.date is not None and not isinstance(self.date, datetime): + raise TypeError("date must be a datetime object or None") + if not isinstance(self.block_key, UsageKey): + raise TypeError("block_key must be a UsageKey object") + + +def update_or_create_assignments_due_dates(course_key, assignments: list[Assignment]): + """ + Update or create due dates for a list of assignments in a course. + + Each assignment's due date is written to ContentDate. If a ContentDate already + exists for (course_key, assignment.block_key, 'due'), it is updated; otherwise + a new one is created. All operations are performed inside a single database + transaction. + + Arguments: + course_key: CourseKey or string representation of the course. + 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. + + Returns: + None + """ + course_key_str = str(course_key) + with transaction.atomic(): + for assignment in assignments: + log.info( + "Updating assignment '%s' with due date '%s' for course %s", + assignment.title, + assignment.date, + course_key_str + ) + if not all((assignment.date, assignment.title)): + log.warning( + "Skipping assignment '%s' for course %s because it has no date or title", + assignment, + course_key_str + ) + continue + models.ContentDate.objects.update_or_create( + course_id=course_key, + location=assignment.block_key, + field='due', + block_type=assignment.assignment_type, + defaults={ + 'policy': models.DatePolicy.objects.get_or_create(abs_date=assignment.date)[0], + 'assignment_title': assignment.title, + 'course_name': course_key.course, + 'subsection_name': assignment.subsection_name, + } + ) + + class BaseWhenException(Exception): pass diff --git a/edx_when/tests/__init__.py b/edx_when/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/edx_when/tests/test_api.py b/edx_when/tests/test_api.py new file mode 100644 index 00000000..7c5c3d81 --- /dev/null +++ b/edx_when/tests/test_api.py @@ -0,0 +1,173 @@ +""" +Tests for course date signals tasks. +""" +from unittest.mock import patch +from datetime import datetime + +from django.contrib.auth import get_user_model +from django.test import TestCase +from opaque_keys.edx.keys import CourseKey, UsageKey + +from edx_when.api import Assignment, update_or_create_assignments_due_dates +from edx_when.models import ContentDate, DatePolicy + +User = get_user_model() + + +class TestUpdateAssignmentDatesForCourse(TestCase): + """ + Tests for the update_assignment_dates_for_course task. + """ + + def setUp(self): + self.course_key = CourseKey.from_string('course-v1:edX+DemoX+Demo_Course') + self.course_key_str = str(self.course_key) + self.staff_user = User.objects.create_user( + username='staff_user', + email='staff@example.com', + is_staff=True + ) + self.block_key = UsageKey.from_string( + 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@test1' + ) + self.due_date = datetime(2024, 12, 31, 23, 59, 59) + self.assignments = [ + Assignment( + title='Test Assignment', + date=self.due_date, + block_key=self.block_key, + assignment_type='Homework', + subsection_name='Test Subsection', + ) + ] + + def test_update_assignment_dates_new_records(self): + """ + Test inserting new records when missing. + """ + update_or_create_assignments_due_dates(self.course_key, self.assignments) + + content_date = ContentDate.objects.get( + course_id=self.course_key, + location=self.block_key + ) + self.assertEqual(content_date.assignment_title, 'Test Assignment') + self.assertEqual(content_date.subsection_name, 'Test Subsection') + self.assertEqual(content_date.block_type, 'Homework') + self.assertEqual(content_date.policy.abs_date, self.due_date) + + def test_update_assignment_dates_existing_records(self): + """ + Test updating existing records when values differ. + """ + existing_policy = DatePolicy.objects.create( + abs_date=datetime(2024, 6, 1) + ) + ContentDate.objects.create( + course_id=self.course_key, + location=self.block_key, + field='due', + block_type='Homework', + policy=existing_policy, + assignment_title='Old Title', + course_name=self.course_key.course, + subsection_name='Old Title' + ) + new_assignment = Assignment( + title='Updated Assignment', + date=self.due_date, + block_key=self.block_key, + assignment_type='Homework', + subsection_name='Updated Subsection', + ) + + update_or_create_assignments_due_dates(self.course_key, [new_assignment]) + + content_date = ContentDate.objects.get( + course_id=self.course_key, + location=self.block_key + ) + self.assertEqual(content_date.assignment_title, 'Updated Assignment') + self.assertEqual(content_date.subsection_name, 'Updated Subsection') + self.assertEqual(content_date.policy.abs_date, self.due_date) + + def test_assignment_with_null_date(self): + """ + Test handling assignments with null dates. + """ + null_date_assignment = Assignment( + title='Null Date Assignment', + date=None, + block_key=self.block_key, + assignment_type='Homework', + ) + update_or_create_assignments_due_dates(self.course_key, [null_date_assignment]) + + content_date_exists = ContentDate.objects.filter( + course_id=self.course_key, + location=self.block_key + ).exists() + self.assertFalse(content_date_exists) + + def test_assignment_with_missing_metadata(self): + """ + Test handling assignments with missing metadata (no title). + """ + assignment = Assignment( + title='', + date=self.due_date, + block_key=self.block_key, + assignment_type='Homework', + ) + update_or_create_assignments_due_dates(self.course_key, [assignment]) + + content_date_exists = ContentDate.objects.filter( + course_id=self.course_key, + location=self.block_key + ).exists() + self.assertFalse(content_date_exists) + + def test_multiple_assignments(self): + """ + Test processing multiple assignments. + """ + assignment1 = Assignment( + title='Assignment 1', + date=self.due_date, + block_key=self.block_key, + assignment_type='Gradeable', + ) + + assignment2 = Assignment( + title='Assignment 2', + date=datetime(2025, 1, 15), + block_key=UsageKey.from_string( + 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@test2' + ), + assignment_type='Homework', + ) + update_or_create_assignments_due_dates(self.course_key, [assignment1, assignment2]) + self.assertEqual(ContentDate.objects.count(), 2) + + def test_empty_assignments_list(self): + """ + Test handling empty assignments list. + """ + update_or_create_assignments_due_dates(self.course_key, []) + self.assertEqual(ContentDate.objects.count(), 0) + + @patch('edx_when.models.DatePolicy.objects.get_or_create') + def test_date_policy_creation_exception(self, mock_policy_create): + """ + Test handling exception during DatePolicy creation. + """ + assignment = Assignment( + title='Test Assignment', + date=self.due_date, + block_key=self.block_key, + assignment_type='problem', + ) + mock_policy_create.side_effect = Exception('Database Error') + + with self.assertRaises(Exception): + update_or_create_assignments_due_dates(self.course_key, [assignment]) diff --git a/tox.ini b/tox.ini index e536b744..de59e2aa 100644 --- a/tox.ini +++ b/tox.ini @@ -20,6 +20,7 @@ norecursedirs = .* docs requirements [testenv] deps = wheel + setuptools django42: Django>=4.2,<5.0 django52: Django<=5.2 -r{toxinidir}/requirements/test.txt From c875945f14100f5634a8d1d5f9c237cc2f59b74b Mon Sep 17 00:00:00 2001 From: Kyrylo Kholodenko Date: Mon, 1 Jun 2026 23:06:10 +0300 Subject: [PATCH 2/3] fix: address review on assignment due dates handler --- edx_when/api.py | 3 ++- tests/test_api.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/edx_when/api.py b/edx_when/api.py index ea351cfd..5647a2bc 100644 --- a/edx_when/api.py +++ b/edx_when/api.py @@ -590,6 +590,7 @@ def update_or_create_assignments_due_dates(course_key, assignments: list[Assignm Returns: None """ + course_key = _ensure_key(CourseKey, course_key) course_key_str = str(course_key) with transaction.atomic(): for assignment in assignments: @@ -610,9 +611,9 @@ def update_or_create_assignments_due_dates(course_key, assignments: list[Assignm course_id=course_key, location=assignment.block_key, field='due', - block_type=assignment.assignment_type, defaults={ 'policy': models.DatePolicy.objects.get_or_create(abs_date=assignment.date)[0], + 'block_type': assignment.block_key.block_type, 'assignment_title': assignment.title, 'course_name': course_key.course, 'subsection_name': assignment.subsection_name, diff --git a/tests/test_api.py b/tests/test_api.py index ffc6316e..ab069517 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -790,7 +790,7 @@ def test_update_assignment_dates_new_records(self): ) self.assertEqual(content_date.assignment_title, 'Test Assignment') self.assertEqual(content_date.subsection_name, 'Test Subsection') - self.assertEqual(content_date.block_type, 'Homework') + self.assertEqual(content_date.block_type, 'sequential') self.assertEqual(content_date.policy.abs_date, self.due_date) def test_update_assignment_dates_existing_records(self): From 79d5acee1467340d0978c5e158e47031ca53b770 Mon Sep 17 00:00:00 2001 From: Kyrylo Kholodenko Date: Fri, 3 Jul 2026 02:17:01 +0300 Subject: [PATCH 3/3] fix: [AXM-2300] address review on update_or_create_assignments_due_dates - 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. --- edx_when/api.py | 65 +++++++++++++++++------------ tests/test_api.py | 102 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 115 insertions(+), 52 deletions(-) diff --git a/edx_when/api.py b/edx_when/api.py index 5647a2bc..1b362193 100644 --- a/edx_when/api.py +++ b/edx_when/api.py @@ -401,6 +401,19 @@ def get_overrides_for_course(course_id): return dates +def _set_content_date_policy(date_kwargs, content_date): + """ + Assign a DatePolicy matching ``date_kwargs`` to ``content_date``. + + Reuses an existing DatePolicy with the same values when present, otherwise creates one. + """ + existing_policies = list(models.DatePolicy.objects.filter(**date_kwargs).order_by('id')) + if existing_policies: + content_date.policy = existing_policies[0] + else: + content_date.policy = models.DatePolicy.objects.create(**date_kwargs) + + def set_date_for_block( course_id, block_id, field, date_or_timedelta, user=None, reason='', actor=None @@ -426,14 +439,6 @@ def set_date_for_block( else: date_kwargs = {'abs_date': date_or_timedelta} - def _set_content_date_policy(date_kwargs, existing_content_date): - # Race conditions were creating multiple DatePolicies w/ the same values. Handle that case. - existing_policies = list(models.DatePolicy.objects.filter(**date_kwargs).order_by('id')) - if existing_policies: - existing_content_date.policy = existing_policies[0] - else: - existing_content_date.policy = models.DatePolicy.objects.create(**date_kwargs) - with transaction.atomic(savepoint=False): # this is frequently called in a loop, let's avoid the savepoints try: existing_date = models.ContentDate.objects.select_related('policy').get( @@ -552,13 +557,12 @@ def get_schedules_with_due_date(course_id, assignment_date): @dataclass class Assignment: """ - Represents an assignment with a title, date, block key, assignment type, and optional subsection name. + Represents an assignment with a title, due date, block key, and optional subsection name. """ title: str date: datetime | None block_key: UsageKey - assignment_type: str subsection_name: str = '' def __post_init__(self): @@ -580,6 +584,11 @@ def update_or_create_assignments_due_dates(course_key, assignments: list[Assignm a new one is created. All operations are performed inside a single database transaction. + Row identity is (course_id, location, field='due'); ``block_type`` stores the + structural XBlock type (e.g. 'sequential') and is not part of the lookup. The + DatePolicy is resolved via `_set_content_date_policy` so concurrent calls + do not create duplicate DatePolicy rows. + Arguments: course_key: CourseKey or string representation of the course. assignments: List of Assignment instances. Assignments with missing date or @@ -591,34 +600,40 @@ def update_or_create_assignments_due_dates(course_key, assignments: list[Assignm None """ course_key = _ensure_key(CourseKey, course_key) - course_key_str = str(course_key) with transaction.atomic(): for assignment in assignments: log.info( "Updating assignment '%s' with due date '%s' for course %s", assignment.title, assignment.date, - course_key_str + course_key ) if not all((assignment.date, assignment.title)): log.warning( "Skipping assignment '%s' for course %s because it has no date or title", assignment, - course_key_str + course_key ) continue - models.ContentDate.objects.update_or_create( - course_id=course_key, - location=assignment.block_key, - field='due', - defaults={ - 'policy': models.DatePolicy.objects.get_or_create(abs_date=assignment.date)[0], - 'block_type': assignment.block_key.block_type, - 'assignment_title': assignment.title, - 'course_name': course_key.course, - 'subsection_name': assignment.subsection_name, - } - ) + try: + content_date = models.ContentDate.objects.select_related('policy').get( + course_id=course_key, + location=assignment.block_key, + field='due', + ) + except models.ContentDate.DoesNotExist: + content_date = models.ContentDate( + course_id=course_key, + location=assignment.block_key, + field='due', + ) + _set_content_date_policy({'abs_date': assignment.date}, content_date) + content_date.block_type = assignment.block_key.block_type + content_date.assignment_title = assignment.title + content_date.course_name = course_key.course + content_date.subsection_name = assignment.subsection_name + content_date.active = True + content_date.save() class BaseWhenException(Exception): diff --git a/tests/test_api.py b/tests/test_api.py index ab069517..3b22bb25 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -751,19 +751,13 @@ def test_relative_dates_import_error(self): assert not api._are_relative_dates_enabled() # pylint: disable=protected-access -class TestUpdateAssignmentDatesForCourse(TestCase): +class TestUpdateOrCreateAssignmentsDueDates(TestCase): """ - Tests for the update_assignment_dates_for_course task. + Tests for the update_or_create_assignments_due_dates API function. """ def setUp(self): self.course_key = CourseKey.from_string('course-v1:edX+DemoX+Demo_Course') - self.course_key_str = str(self.course_key) - self.staff_user = User.objects.create_user( - username='staff_user', - email='staff@example.com', - is_staff=True - ) self.block_key = UsageKey.from_string( 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@test1' ) @@ -773,7 +767,6 @@ def setUp(self): title='Test Assignment', date=self.due_date, block_key=self.block_key, - assignment_type='Homework', subsection_name='Test Subsection', ) ] @@ -790,8 +783,10 @@ def test_update_assignment_dates_new_records(self): ) self.assertEqual(content_date.assignment_title, 'Test Assignment') self.assertEqual(content_date.subsection_name, 'Test Subsection') + # block_type stores the structural XBlock type, taken from block_key. self.assertEqual(content_date.block_type, 'sequential') self.assertEqual(content_date.policy.abs_date, self.due_date) + self.assertTrue(content_date.active) def test_update_assignment_dates_existing_records(self): """ @@ -804,7 +799,7 @@ def test_update_assignment_dates_existing_records(self): course_id=self.course_key, location=self.block_key, field='due', - block_type='Homework', + block_type='sequential', policy=existing_policy, assignment_title='Old Title', course_name=self.course_key.course, @@ -814,7 +809,6 @@ def test_update_assignment_dates_existing_records(self): title='Updated Assignment', date=self.due_date, block_key=self.block_key, - assignment_type='Homework', subsection_name='Updated Subsection', ) @@ -827,16 +821,17 @@ def test_update_assignment_dates_existing_records(self): self.assertEqual(content_date.assignment_title, 'Updated Assignment') self.assertEqual(content_date.subsection_name, 'Updated Subsection') self.assertEqual(content_date.policy.abs_date, self.due_date) + # No duplicate ContentDate row created for the same (course, location, field). + self.assertEqual(models.ContentDate.objects.filter(location=self.block_key).count(), 1) def test_assignment_with_null_date(self): """ - Test handling assignments with null dates. + Test handling assignments with null dates (no existing row). """ null_date_assignment = Assignment( title='Null Date Assignment', date=None, block_key=self.block_key, - assignment_type='Homework', ) api.update_or_create_assignments_due_dates(self.course_key, [null_date_assignment]) @@ -846,6 +841,32 @@ def test_assignment_with_null_date(self): ).exists() self.assertFalse(content_date_exists) + def test_existing_row_with_null_date_is_left_unchanged(self): + """ + A null-date assignment must not modify or remove an existing ContentDate row. + """ + existing_policy = models.DatePolicy.objects.create(abs_date=self.due_date) + models.ContentDate.objects.create( + course_id=self.course_key, + location=self.block_key, + field='due', + block_type='sequential', + policy=existing_policy, + assignment_title='Keep Me', + course_name=self.course_key.course, + ) + null_date_assignment = Assignment( + title='Null Date Assignment', + date=None, + block_key=self.block_key, + ) + + api.update_or_create_assignments_due_dates(self.course_key, [null_date_assignment]) + + content_date = models.ContentDate.objects.get(location=self.block_key) + self.assertEqual(content_date.assignment_title, 'Keep Me') + self.assertEqual(content_date.policy.abs_date, self.due_date) + def test_assignment_with_missing_metadata(self): """ Test handling assignments with missing metadata (no title). @@ -854,7 +875,6 @@ def test_assignment_with_missing_metadata(self): title='', date=self.due_date, block_key=self.block_key, - assignment_type='Homework', ) api.update_or_create_assignments_due_dates(self.course_key, [assignment]) @@ -872,7 +892,6 @@ def test_multiple_assignments(self): title='Assignment 1', date=self.due_date, block_key=self.block_key, - assignment_type='Gradeable', ) assignment2 = Assignment( @@ -881,7 +900,6 @@ def test_multiple_assignments(self): block_key=UsageKey.from_string( 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@test2' ), - assignment_type='Homework', ) api.update_or_create_assignments_due_dates(self.course_key, [assignment1, assignment2]) self.assertEqual(models.ContentDate.objects.count(), 2) @@ -893,18 +911,48 @@ def test_empty_assignments_list(self): api.update_or_create_assignments_due_dates(self.course_key, []) self.assertEqual(models.ContentDate.objects.count(), 0) - @patch('edx_when.models.DatePolicy.objects.get_or_create') - def test_date_policy_creation_exception(self, mock_policy_create): + def test_reuses_existing_date_policy_no_duplicates(self): """ - Test handling exception during DatePolicy creation. + Existing DatePolicy rows with matching values are reused, never duplicated. + + Simulates DatePolicy rows left over from a prior race and asserts the function + reuses one of them instead of creating another (regression for the get_or_create race). """ - assignment = Assignment( - title='Test Assignment', - date=self.due_date, - block_key=self.block_key, - assignment_type='problem', + models.DatePolicy.objects.create(abs_date=self.due_date) + models.DatePolicy.objects.create(abs_date=self.due_date) + + api.update_or_create_assignments_due_dates(self.course_key, self.assignments) + + # No third DatePolicy created; the existing (lowest-id) one is reused. + self.assertEqual(models.DatePolicy.objects.filter(abs_date=self.due_date).count(), 2) + content_date = models.ContentDate.objects.get(location=self.block_key) + self.assertEqual(content_date.policy.abs_date, self.due_date) + + def test_atomic_rollback_on_failure(self): + """ + A failure mid-loop rolls back the whole batch (single transaction boundary). + """ + second_date = datetime(2025, 1, 15) + assignment1 = self.assignments[0] + assignment2 = Assignment( + title='Assignment 2', + date=second_date, + block_key=UsageKey.from_string( + 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@test2' + ), ) - mock_policy_create.side_effect = Exception('Database Error') - with self.assertRaises(Exception): - api.update_or_create_assignments_due_dates(self.course_key, [assignment]) + def fake_set_policy(date_kwargs, content_date): + if date_kwargs.get('abs_date') == second_date: + raise RuntimeError('boom') + content_date.policy = models.DatePolicy.objects.create(**date_kwargs) + + with patch('edx_when.api._set_content_date_policy', side_effect=fake_set_policy): + with self.assertRaises(RuntimeError): + api.update_or_create_assignments_due_dates( + self.course_key, [assignment1, assignment2] + ) + + # First assignment's write must have been rolled back. + self.assertEqual(models.ContentDate.objects.count(), 0) + self.assertEqual(models.DatePolicy.objects.count(), 0)