Merge pull request #16267 from open-craft/haikuginger/allowed-enrollment-signal

Switch to using a signal to manage pending enrollments when a user is activated or changes their email address

common/djangoapps/student/migrations/0014_courseenrollmentallowed_user.py

@@ -0,0 +1,21 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+from django.conf import settings
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('student', '0013_delete_historical_enrollment_records'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='courseenrollmentallowed',
+            name='user',
+            field=models.ForeignKey(blank=True, to=settings.AUTH_USER_MODEL, help_text="First user which enrolled in the specified course through the specified e-mail. Once set, it won't change.", null=True),
+        ),
+    ]

common/djangoapps/student/models.py

@@ -30,7 +30,7 @@ from django.contrib.auth.signals import user_logged_in, user_logged_out
 from django.core.cache import cache
 from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist
 from django.db import IntegrityError, models
-from django.db.models import Count
+from django.db.models import Count, Q
 from django.db.models.signals import post_save, pre_save
 from django.dispatch import receiver
 from django.utils import timezone
@@ -477,9 +477,39 @@ def user_pre_save_callback(sender, **kwargs):
 @receiver(post_save, sender=User)
 def user_post_save_callback(sender, **kwargs):
     """
-    Emit analytics events after saving the User.
+    When a user is modified and either its `is_active` state or email address
+    is changed, and the user is, in fact, active, then check to see if there
+    are any courses that it needs to be automatically enrolled in.
+
+    Additionally, emit analytics events after saving the User.
     """
     user = kwargs['instance']
+
+    changed_fields = user._changed_fields
+
+    if 'is_active' in changed_fields or 'email' in changed_fields:
+        if user.is_active:
+            ceas = CourseEnrollmentAllowed.for_user(user).filter(auto_enroll=True)
+
+            for cea in ceas:
+                enrollment = CourseEnrollment.enroll(user, cea.course_id)
+
+                manual_enrollment_audit = ManualEnrollmentAudit.get_manual_enrollment_by_email(user.email)
+                if manual_enrollment_audit is not None:
+                    # get the enrolled by user and reason from the ManualEnrollmentAudit table.
+                    # then create a new ManualEnrollmentAudit table entry for the same email
+                    # different transition state.
+                    ManualEnrollmentAudit.create_manual_enrollment_audit(
+                        manual_enrollment_audit.enrolled_by,
+                        user.email,
+                        ALLOWEDTOENROLL_TO_ENROLLED,
+                        manual_enrollment_audit.reason,
+                        enrollment
+                    )
+
+    # Because `emit_field_changed_events` removes the record of the fields that
+    # were changed, wait to do that until after we've checked them as part of
+    # the condition on whether we want to check for automatic enrollments.
     # pylint: disable=protected-access
     emit_field_changed_events(
         user,
@@ -1067,7 +1097,7 @@ class CourseEnrollment(models.Model):
         through some sort of approval process before being activated. If you
         don't need this functionality, just call `enroll()` instead.
 
-        Returns a CoursewareEnrollment object.
+        Returns a CourseEnrollment object.
 
         `user` is a Django User object. If it hasn't been saved yet (no `.id`
                attribute), this method will automatically save it before
@@ -1077,6 +1107,9 @@ class CourseEnrollment(models.Model):
 
         It is expected that this method is called from a method which has already
         verified the user authentication and access.
+
+        If the enrollment is done due to a CourseEnrollmentAllowed, the CEA will be
+        linked to the user being enrolled so that it can't be used by other users.
         """
         # If we're passing in a newly constructed (i.e. not yet persisted) User,
         # save it to the database so that it can have an ID that we can throw
@@ -1096,11 +1129,18 @@ class CourseEnrollment(models.Model):
             }
         )
 
+        # If there was an unlinked CEA, it becomes linked now
+        CourseEnrollmentAllowed.objects.filter(
+            email=user.email,
+            course_id=course_key,
+            user__isnull=True
+        ).update(user=user)
+
         return enrollment
 
     @classmethod
     def get_enrollment(cls, user, course_key):
-        """Returns a CoursewareEnrollment object.
+        """Returns a CourseEnrollment object.
 
         Args:
             user (User): The user associated with the enrollment.
@@ -1258,7 +1298,7 @@ class CourseEnrollment(models.Model):
         """
         Enroll a user in a course. This saves immediately.
 
-        Returns a CoursewareEnrollment object.
+        Returns a CourseEnrollment object.
 
         `user` is a Django User object. If it hasn't been saved yet (no `.id`
                attribute), this method will automatically save it before
@@ -1342,7 +1382,7 @@ class CourseEnrollment(models.Model):
         error rate is high. For that reason, we supress User lookup errors by
         default.
 
-        Returns a CoursewareEnrollment object. If the User does not exist and
+        Returns a CourseEnrollment object. If the User does not exist and
         `ignore_errors` is set to `True`, it will return None.
 
         `email` Email address of the User to add to enroll in the course.
@@ -1974,11 +2014,20 @@ class CourseEnrollmentAllowed(models.Model):
     """
     Table of users (specified by email address strings) who are allowed to enroll in a specified course.
     The user may or may not (yet) exist.  Enrollment by users listed in this table is allowed
-    even if the enrollment time window is past.
+    even if the enrollment time window is past.  Once an enrollment from this list effectively happens,
+    the object is marked with the student who enrolled, to prevent students from changing e-mails and
+    enrolling many accounts through the same e-mail.
     """
     email = models.CharField(max_length=255, db_index=True)
     course_id = CourseKeyField(max_length=255, db_index=True)
     auto_enroll = models.BooleanField(default=0)
+    user = models.ForeignKey(
+        User,
+        null=True,
+        blank=True,
+        help_text="First user which enrolled in the specified course through the specified e-mail. "
+                  "Once set, it won't change."
+    )
 
     created = models.DateTimeField(auto_now_add=True, null=True, db_index=True)
 
@@ -1988,6 +2037,21 @@ class CourseEnrollmentAllowed(models.Model):
     def __unicode__(self):
         return "[CourseEnrollmentAllowed] %s: %s (%s)" % (self.email, self.course_id, self.created)
 
+    @classmethod
+    def for_user(cls, user):
+        """
+        Returns the CourseEnrollmentAllowed objects that can effectively be used by a particular `user`.
+        This includes the ones that match the user's e-mail and excludes those CEA which were already consumed
+        by a different user.
+        """
+        return cls.objects.filter(email=user.email).filter(Q(user__isnull=True) | Q(user=user))
+
+    def valid_for_user(self, user):
+        """
+        Returns True if the CEA is usable by the given user, or False if it was already consumed by another user.
+        """
+        return self.user is None or self.user == user
+
     @classmethod
     def may_enroll_and_unenrolled(cls, course_id):
         """

common/djangoapps/student/tests/test_enrollment.py

@@ -12,9 +12,9 @@ from nose.plugins.attrib import attr
 from course_modes.models import CourseMode
 from course_modes.tests.factories import CourseModeFactory
 from openedx.core.djangoapps.embargo.test_utils import restrict_course
-from student.models import CourseEnrollment, CourseFullError
+from student.models import CourseEnrollment, CourseEnrollmentAllowed, CourseFullError, EnrollmentClosedError
 from student.roles import CourseInstructorRole, CourseStaffRole
-from student.tests.factories import UserFactory
+from student.tests.factories import CourseEnrollmentAllowedFactory, UserFactory
 from util.testing import UrlResetMixin
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
@@ -280,3 +280,63 @@ class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase):
             params['email_opt_in'] = email_opt_in
 
         return self.client.post(reverse('change_enrollment'), params)
+
+    def test_cea_enrolls_only_one_user(self):
+        """
+        Tests that a CourseEnrollmentAllowed can be used by just one user.
+        If the user changes e-mail and then a second user tries to enroll with the same accepted e-mail,
+        the second enrollment should fail.
+        However, the original user can reuse the CEA many times.
+        """
+
+        cea = CourseEnrollmentAllowedFactory(
+            email='allowed@edx.org',
+            course_id=self.course.id,
+            auto_enroll=False,
+        )
+        # Still unlinked
+        self.assertIsNone(cea.user)
+
+        user1 = UserFactory.create(username="tester1", email="tester1@e.com", password="test")
+        user2 = UserFactory.create(username="tester2", email="tester2@e.com", password="test")
+
+        self.assertFalse(
+            CourseEnrollment.objects.filter(course_id=self.course.id, user=user1).exists()
+        )
+
+        user1.email = 'allowed@edx.org'
+        user1.save()
+
+        CourseEnrollment.enroll(user1, self.course.id, check_access=True)
+
+        self.assertTrue(
+            CourseEnrollment.objects.filter(course_id=self.course.id, user=user1).exists()
+        )
+
+        # The CEA is now linked
+        cea.refresh_from_db()
+        self.assertEqual(cea.user, user1)
+
+        # user2 wants to enroll too, (ab)using the same allowed e-mail, but cannot
+        user1.email = 'my_other_email@edx.org'
+        user1.save()
+        user2.email = 'allowed@edx.org'
+        user2.save()
+        with self.assertRaises(EnrollmentClosedError):
+            CourseEnrollment.enroll(user2, self.course.id, check_access=True)
+
+        # CEA still linked to user1. Also after unenrolling
+        cea.refresh_from_db()
+        self.assertEqual(cea.user, user1)
+
+        CourseEnrollment.unenroll(user1, self.course.id)
+
+        cea.refresh_from_db()
+        self.assertEqual(cea.user, user1)
+
+        # Enroll user1 again. Because it's the original owner of the CEA, the enrollment is allowed
+        CourseEnrollment.enroll(user1, self.course.id, check_access=True)
+
+        # Still same
+        cea.refresh_from_db()
+        self.assertEqual(cea.user, user1)

common/djangoapps/student/tests/test_events.py

@@ -7,8 +7,8 @@ from django.db.utils import IntegrityError
 from django.test import TestCase
 from django_countries.fields import Country
 
-from student.models import PasswordHistory
-from student.tests.factories import UserFactory
+from student.models import CourseEnrollmentAllowed, PasswordHistory
+from student.tests.factories import UserFactory, CourseEnrollmentAllowedFactory
 from student.tests.tests import UserSettingsEventTestMixin
 
 
@@ -154,3 +154,25 @@ class TestUserEvents(UserSettingsEventTestMixin, TestCase):
         self.user.last_name = "Duck"
         self.user.save()
         self.assert_no_events_were_emitted()
+
+    def test_enrolled_after_email_change(self):
+        """
+        Test that when a user's email changes, the user is enrolled in pending courses.
+        """
+        pending_enrollment = CourseEnrollmentAllowedFactory(auto_enroll=True)
+
+        # the e-mail will change to test@edx.org (from something else)
+        self.assertNotEquals(self.user.email, 'test@edx.org')
+
+        # there's a CEA for the new e-mail
+        self.assertEquals(CourseEnrollmentAllowed.objects.count(), 1)
+        self.assertEquals(CourseEnrollmentAllowed.objects.filter(email='test@edx.org').count(), 1)
+
+        # Changing the e-mail to the enrollment-allowed e-mail should enroll
+        self.user.email = 'test@edx.org'
+        self.user.save()
+        self.assert_user_enrollment_occurred('edX/toy/2012_Fall')
+
+        # CEAs shouldn't have been affected
+        self.assertEquals(CourseEnrollmentAllowed.objects.count(), 1)
+        self.assertEquals(CourseEnrollmentAllowed.objects.filter(email='test@edx.org').count(), 1)

common/djangoapps/student/tests/tests.py

@@ -699,6 +699,12 @@ class UserSettingsEventTestMixin(EventTestMixin):
             **kwargs
         )
 
+    def assert_user_enrollment_occurred(self, course_key):
+        """
+        Helper method to assert that the user is enrolled in the given course.
+        """
+        self.assertTrue(CourseEnrollment.is_enrolled(self.user, CourseKey.from_string(course_key)))
+
 
 class EnrollmentEventTestMixin(EventTestMixin):
     """ Mixin with assertions for validating enrollment events. """

common/djangoapps/student/views/management.py

@@ -83,10 +83,7 @@ from student.helpers import (
     get_next_url_for_login_page
 )
 from student.models import (
-    ALLOWEDTOENROLL_TO_ENROLLED,
     CourseEnrollment,
-    CourseEnrollmentAllowed,
-    ManualEnrollmentAudit,
     PasswordHistory,
     PendingEmailChange,
     Registration,
@@ -784,7 +781,6 @@ def create_account_with_params(request, params):
 
     if skip_email:
         registration.activate()
-        _enroll_user_in_pending_courses(user)  # Enroll student in any pending courses
     else:
         compose_and_send_activation_email(user, profile, registration)
 
@@ -878,25 +874,6 @@ def skip_activation_email(user, do_external_auth, running_pipeline, third_party_
     )
 
 
-def _enroll_user_in_pending_courses(student):
-    """
-    Enroll student in any pending courses he/she may have.
-    """
-    ceas = CourseEnrollmentAllowed.objects.filter(email=student.email)
-    for cea in ceas:
-        if cea.auto_enroll:
-            enrollment = CourseEnrollment.enroll(student, cea.course_id)
-            manual_enrollment_audit = ManualEnrollmentAudit.get_manual_enrollment_by_email(student.email)
-            if manual_enrollment_audit is not None:
-                # get the enrolled by user and reason from the ManualEnrollmentAudit table.
-                # then create a new ManualEnrollmentAudit table entry for the same email
-                # different transition state.
-                ManualEnrollmentAudit.create_manual_enrollment_audit(
-                    manual_enrollment_audit.enrolled_by, student.email, ALLOWEDTOENROLL_TO_ENROLLED,
-                    manual_enrollment_audit.reason, enrollment
-                )
-
-
 def record_affiliate_registration_attribution(request, user):
     """
     Attribute this user's registration to the referring affiliate, if
@@ -1060,9 +1037,6 @@ def activate_account(request, key):
                 extra_tags='account-activation aa-icon',
             )
 
-        # Enroll student in any pending courses he/she may have if auto_enroll flag is set
-        _enroll_user_in_pending_courses(registration.user)
-
     return redirect('dashboard')
 
 
@@ -1088,9 +1062,6 @@ def activate_account_studio(request, key):
             registration.activate()
             already_active = False
 
-        # Enroll student in any pending courses he/she may have if auto_enroll flag is set
-        _enroll_user_in_pending_courses(registration.user)
-
         return render_to_response(
             "registration/activation_complete.html",
             {

lms/djangoapps/courseware/access.py

@@ -260,11 +260,19 @@ def _can_enroll_courselike(user, courselike):
         reg_method_ok = True
 
     # If the user appears in CourseEnrollmentAllowed paired with the given course key,
-    # they may enroll. Note that as dictated by the legacy database schema, the filter
-    # call includes a `course_id` kwarg which requires a CourseKey.
+    # they may enroll, except if the CEA has already been used by a different user.
+    # Note that as dictated by the legacy database schema, the filter call includes
+    # a `course_id` kwarg which requires a CourseKey.
     if user is not None and user.is_authenticated():
-        if CourseEnrollmentAllowed.objects.filter(email=user.email, course_id=course_key):
+        cea = CourseEnrollmentAllowed.objects.filter(email=user.email, course_id=course_key).first()
+        if cea and cea.valid_for_user(user):
             return ACCESS_GRANTED
+        elif cea:
+            debug("Deny: CEA was already consumed by a different user {} and can't be used again by {}".format(
+                cea.user.id,
+                user.id,
+            ))
+            return ACCESS_DENIED
 
     if _has_staff_access_to_descriptor(user, courselike, course_key):
         return ACCESS_GRANTED

lms/djangoapps/instructor/enrollment.py

@@ -51,11 +51,12 @@ class EmailEnrollmentState(object):
             # is_active is `None` if the user is not enrolled in the course
             exists_ce = is_active is not None and is_active
             full_name = user.profile.name
+            ceas = CourseEnrollmentAllowed.for_user(user).filter(course_id=course_id).all()
         else:
             mode = None
             exists_ce = False
             full_name = None
-        ceas = CourseEnrollmentAllowed.objects.filter(course_id=course_id, email=email).all()
+            ceas = CourseEnrollmentAllowed.objects.filter(email=email, course_id=course_id).all()
         exists_allowed = ceas.exists()
         state_auto_enroll = exists_allowed and ceas[0].auto_enroll