From ce943bced27e33af0b6c22d9d8d9ef83dc4147dd Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Thu, 22 Aug 2019 16:25:59 -0400 Subject: [PATCH] Add the correct UNIQUE constraints to the ProgramEnrollment model. --- ...0001-program-enrollment-data-integrity.rst | 47 +++++++++++++++++++ .../0006_add_the_correct_constraints.py | 21 +++++++++ lms/djangoapps/program_enrollments/models.py | 4 +- .../program_enrollments/tests/factories.py | 4 +- .../program_enrollments/tests/test_models.py | 26 +++++++++- 5 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 lms/djangoapps/program_enrollments/docs/decisions/0001-program-enrollment-data-integrity.rst create mode 100644 lms/djangoapps/program_enrollments/migrations/0006_add_the_correct_constraints.py diff --git a/lms/djangoapps/program_enrollments/docs/decisions/0001-program-enrollment-data-integrity.rst b/lms/djangoapps/program_enrollments/docs/decisions/0001-program-enrollment-data-integrity.rst new file mode 100644 index 0000000000..16a1a9a817 --- /dev/null +++ b/lms/djangoapps/program_enrollments/docs/decisions/0001-program-enrollment-data-integrity.rst @@ -0,0 +1,47 @@ +ProgramEnrollment Model Data Integrity +-------------------------------------- + +Status +====== + +Accepted (circa August 2019) + + +Context +======= + +For the sake of fundamental data integrity, we are introducing 2 unique +constraints on the ``program_enrollments.ProgramEnrollment`` model. + +Decisions +========= + +The unique constraints are on the following column sets: + +* ``('user', 'program_uuid', 'curriculum_uuid')`` +* ``('external_user_key', 'program_uuid', 'curriculum_uuid')`` + +Note that either the ``user`` column or the ``external_user_key`` column may be null. +In the future, it would be nice to add a validation step at the Django model layer +that restricts a model instance from having null values for both of these fields. + +Consequences +============ + +The first constraint supports the cases in which we save program enrollment records +that don't have any association with an external organization, e.g. our MicroMasters programs. +Non-realized enrollments, where the ``user`` value is null, are not affected by this constraint. + +As for the second constraint , we want to disallow the ability of anyone to register a learner, +as identified by ``external_user_key``, into the same program and curriculum more than once. +No enrollment record with a null ``external_user_key`` is affected by this constraint. + +Together, these constraints restrict the duplication of learner records in a specific +program/curriculum, where the learner is identified either by their ``auth.User.id`` or +some ``external_user_key``. + +This constraint set does NOT support the use case of a single ``auth.User`` being enrolled +in the same program/curriculum with two or more different ``external_user_keys``. Supporting +this case leads to problematic situations, e.g. how to decide which of these program enrollment +records to link to a program-course enrollment? If needed, we could introduce an additional +set of models to support this situation. diff --git a/lms/djangoapps/program_enrollments/migrations/0006_add_the_correct_constraints.py b/lms/djangoapps/program_enrollments/migrations/0006_add_the_correct_constraints.py new file mode 100644 index 0000000000..78d8e8f717 --- /dev/null +++ b/lms/djangoapps/program_enrollments/migrations/0006_add_the_correct_constraints.py @@ -0,0 +1,21 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.23 on 2019-08-23 15:37 +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('program_enrollments', '0005_canceled_not_withdrawn'), + ] + + operations = [ + migrations.AlterUniqueTogether( + name='programenrollment', + unique_together=set([('user', 'program_uuid', 'curriculum_uuid'), ('external_user_key', 'program_uuid', 'curriculum_uuid')]), + ), + ] diff --git a/lms/djangoapps/program_enrollments/models.py b/lms/djangoapps/program_enrollments/models.py index e7ed710ade..4307658c06 100644 --- a/lms/djangoapps/program_enrollments/models.py +++ b/lms/djangoapps/program_enrollments/models.py @@ -39,12 +39,12 @@ class ProgramEnrollment(TimeStampedModel): # pylint: disable=model-missing-unic class Meta(object): app_label = "program_enrollments" - unique_together = ('external_user_key', 'program_uuid', 'curriculum_uuid') # A student enrolled in a given (program, curriculum) should always # have a non-null ``user`` or ``external_user_key`` field (or both). unique_together = ( - ('user', 'external_user_key', 'program_uuid', 'curriculum_uuid'), + ('user', 'program_uuid', 'curriculum_uuid'), + ('external_user_key', 'program_uuid', 'curriculum_uuid'), ) user = models.ForeignKey( diff --git a/lms/djangoapps/program_enrollments/tests/factories.py b/lms/djangoapps/program_enrollments/tests/factories.py index 120ba875a2..ac0c75f36d 100644 --- a/lms/djangoapps/program_enrollments/tests/factories.py +++ b/lms/djangoapps/program_enrollments/tests/factories.py @@ -20,8 +20,8 @@ class ProgramEnrollmentFactory(DjangoModelFactory): user = factory.SubFactory(UserFactory) external_user_key = None - program_uuid = uuid4() - curriculum_uuid = uuid4() + program_uuid = factory.LazyFunction(uuid4) + curriculum_uuid = factory.LazyFunction(uuid4) status = 'enrolled' diff --git a/lms/djangoapps/program_enrollments/tests/test_models.py b/lms/djangoapps/program_enrollments/tests/test_models.py index cf12471b0e..1f1181e2ed 100644 --- a/lms/djangoapps/program_enrollments/tests/test_models.py +++ b/lms/djangoapps/program_enrollments/tests/test_models.py @@ -6,6 +6,7 @@ from __future__ import absolute_import, unicode_literals from uuid import uuid4 import ddt +from django.db.utils import IntegrityError from django.test import TestCase from opaque_keys.edx.keys import CourseKey from six.moves import range @@ -32,14 +33,37 @@ class ProgramEnrollmentModelTests(TestCase): self.user = UserFactory.create() self.program_uuid = uuid4() self.other_program_uuid = uuid4() + self.curriculum_uuid = uuid4() self.enrollment = ProgramEnrollment.objects.create( user=self.user, external_user_key='abc', program_uuid=self.program_uuid, - curriculum_uuid=uuid4(), + curriculum_uuid=self.curriculum_uuid, status='enrolled' ) + def test_unique_external_key_program_curriculum(self): + """ A record with the same (external_user_key, program_uuid, curriculum_uuid) cannot be duplicated. """ + with self.assertRaises(IntegrityError): + _ = ProgramEnrollment.objects.create( + user=None, + external_user_key='abc', + program_uuid=self.program_uuid, + curriculum_uuid=self.curriculum_uuid, + status='pending', + ) + + def test_unique_user_program_curriculum(self): + """ A record with the same (user, program_uuid, curriculum_uuid) cannot be duplicated. """ + with self.assertRaises(IntegrityError): + _ = ProgramEnrollment.objects.create( + user=self.user, + external_user_key=None, + program_uuid=self.program_uuid, + curriculum_uuid=self.curriculum_uuid, + status='suspended', + ) + def test_bulk_read_by_student_key(self): curriculum_a = uuid4() curriculum_b = uuid4()