diff --git a/common/djangoapps/student/migrations/0014_courseenrollmentallowed_user.py b/common/djangoapps/student/migrations/0014_courseenrollmentallowed_user.py new file mode 100644 index 0000000000..72557ddf20 --- /dev/null +++ b/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), + ), + ] diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 51339d4634..4e333c8c98 100644 --- a/common/djangoapps/student/models.py +++ b/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): """ diff --git a/common/djangoapps/student/tests/test_enrollment.py b/common/djangoapps/student/tests/test_enrollment.py index b05891291b..ed69ffec10 100644 --- a/common/djangoapps/student/tests/test_enrollment.py +++ b/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) diff --git a/common/djangoapps/student/tests/test_events.py b/common/djangoapps/student/tests/test_events.py index bc3f9f1023..ccb1f058d2 100644 --- a/common/djangoapps/student/tests/test_events.py +++ b/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) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 31df861188..0a8c3e27db 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/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. """ diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 85af8d6cef..b3491478ff 100644 --- a/common/djangoapps/student/views/management.py +++ b/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", { diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index bf981d497a..c70571ef1c 100644 --- a/lms/djangoapps/courseware/access.py +++ b/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 diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 7811b56117..f8aab91939 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/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