diff --git a/edx_when/api.py b/edx_when/api.py index e3e9b87..1b36219 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 @@ -400,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 @@ -425,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( @@ -548,6 +554,88 @@ def get_schedules_with_due_date(course_id, assignment_date): return schedules +@dataclass +class Assignment: + """ + Represents an assignment with a title, due date, block key, and optional subsection name. + """ + + title: str + date: datetime | None + block_key: UsageKey + 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. + + 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 + 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 = _ensure_key(CourseKey, 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 + ) + 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 + ) + continue + 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): pass diff --git a/tests/test_api.py b/tests/test_api.py index a4591ab..3b22bb2 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -11,9 +11,11 @@ from django.test import TestCase from django.urls import reverse from edx_django_utils.cache.utils import RequestCache, TieredCache +from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import CourseLocator from edx_when import api, models +from edx_when.api import Assignment from test_utils import make_block_id, make_items from tests.test_models_app.models import DummyCourse, DummyEnrollment, DummySchedule @@ -747,3 +749,210 @@ def test_relative_dates_disabled(self): @patch.dict(sys.modules, {'openedx.features.course_experience': None}) def test_relative_dates_import_error(self): assert not api._are_relative_dates_enabled() # pylint: disable=protected-access + + +class TestUpdateOrCreateAssignmentsDueDates(TestCase): + """ + 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.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, + subsection_name='Test Subsection', + ) + ] + + def test_update_assignment_dates_new_records(self): + """ + Test inserting new records when missing. + """ + api.update_or_create_assignments_due_dates(self.course_key, self.assignments) + + content_date = models.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') + # 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): + """ + Test updating existing records when values differ. + """ + existing_policy = models.DatePolicy.objects.create( + abs_date=datetime(2024, 6, 1) + ) + models.ContentDate.objects.create( + course_id=self.course_key, + location=self.block_key, + field='due', + block_type='sequential', + 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, + subsection_name='Updated Subsection', + ) + + api.update_or_create_assignments_due_dates(self.course_key, [new_assignment]) + + content_date = models.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) + # 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 (no existing row). + """ + 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_exists = models.ContentDate.objects.filter( + course_id=self.course_key, + location=self.block_key + ).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). + """ + assignment = Assignment( + title='', + date=self.due_date, + block_key=self.block_key, + ) + api.update_or_create_assignments_due_dates(self.course_key, [assignment]) + + content_date_exists = models.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, + ) + + 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' + ), + ) + api.update_or_create_assignments_due_dates(self.course_key, [assignment1, assignment2]) + self.assertEqual(models.ContentDate.objects.count(), 2) + + def test_empty_assignments_list(self): + """ + Test handling empty assignments list. + """ + api.update_or_create_assignments_due_dates(self.course_key, []) + self.assertEqual(models.ContentDate.objects.count(), 0) + + def test_reuses_existing_date_policy_no_duplicates(self): + """ + 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). + """ + 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' + ), + ) + + 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) diff --git a/tox.ini b/tox.ini index e536b74..de59e2a 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