diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2e51f0b83467..44b6b3844745 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,15 @@ LMS: Enable beta instructor dashboard. The beta dashboard is a rearchitecture of the existing instructor dashboard and is available by clicking a link at the top right of the existing dashboard. +Common: CourseEnrollment has new fields `is_active` and `mode`. The mode will be +used to differentiate different kinds of enrollments (currently, all enrollments +are honor certificate enrollments). The `is_active` flag will be used to +deactivate enrollments without deleting them, so that we know what course you +*were* enrolled in. Because of the latter change, enrollment and unenrollment +logic has been consolidated into the model -- you should use new class methods +to `enroll()`, `unenroll()`, and to check `is_enrolled()`, instead of creating +CourseEnrollment objects or querying them directly. + Studio: Email will be sent to admin address when a user requests course creator privileges for Studio (edge only). diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 86369f73d95a..e70df4164a7e 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -49,7 +49,7 @@ from pytz import UTC from uuid import uuid4 from pymongo import MongoClient -from student.views import is_enrolled_in_course +from student.models import CourseEnrollment TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex @@ -1168,7 +1168,7 @@ def assert_created_course(self, number_suffix=None): self.assertNotIn('ErrMsg', data) self.assertEqual(data['id'], 'i4x://MITx/{0}/course/2013_Spring'.format(test_course_data['number'])) # Verify that the creator is now registered in the course. - self.assertTrue(is_enrolled_in_course(self.user, self._get_course_id(test_course_data))) + self.assertTrue(CourseEnrollment.is_enrolled(self.user, self._get_course_id(test_course_data))) return test_course_data def test_create_course_check_forum_seeding(self): @@ -1190,14 +1190,14 @@ def assert_course_creation_failed(self, error_message): Checks that the course did not get created """ course_id = self._get_course_id(self.course_data) - initially_enrolled = is_enrolled_in_course(self.user, course_id) + initially_enrolled = CourseEnrollment.is_enrolled(self.user, course_id) resp = self.client.post(reverse('create_new_course'), self.course_data) self.assertEqual(resp.status_code, 200) data = parse_json(resp) self.assertEqual(data['ErrMsg'], error_message) # One test case involves trying to create the same course twice. Hence for that course, # the user will be enrolled. In the other cases, initially_enrolled will be False. - self.assertEqual(initially_enrolled, is_enrolled_in_course(self.user, course_id)) + self.assertEqual(initially_enrolled, CourseEnrollment.is_enrolled(self.user, course_id)) def test_create_course_duplicate_number(self): """Test new course creation - error path""" diff --git a/cms/djangoapps/contentstore/tests/test_users.py b/cms/djangoapps/contentstore/tests/test_users.py index a9216da6122d..cbb8aa8b01fe 100644 --- a/cms/djangoapps/contentstore/tests/test_users.py +++ b/cms/djangoapps/contentstore/tests/test_users.py @@ -6,7 +6,7 @@ from django.contrib.auth.models import User, Group from django.core.urlresolvers import reverse from auth.authz import get_course_groupname_for_role -from student.views import is_enrolled_in_course +from student.models import CourseEnrollment class UsersTestCase(CourseTestCase): @@ -372,13 +372,13 @@ def test_staff_to_instructor_still_enrolled(self): def assert_not_enrolled(self): """ Asserts that self.ext_user is not enrolled in self.course. """ self.assertFalse( - is_enrolled_in_course(self.ext_user, self.course.location.course_id), + CourseEnrollment.is_enrolled(self.ext_user, self.course.location.course_id), 'Did not expect ext_user to be enrolled in course' ) def assert_enrolled(self): """ Asserts that self.ext_user is enrolled in self.course. """ self.assertTrue( - is_enrolled_in_course(self.ext_user, self.course.location.course_id), + CourseEnrollment.is_enrolled(self.ext_user, self.course.location.course_id), 'User ext_user should have been enrolled in the course' ) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 8ac1d223cbe2..a6b1b29aabd9 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -44,7 +44,7 @@ from django_comment_common.utils import seed_permissions_roles -from student.views import enroll_in_course +from student.models import CourseEnrollment from xmodule.html_module import AboutDescriptor __all__ = ['course_index', 'create_new_course', 'course_info', @@ -165,7 +165,7 @@ def create_new_course(request): seed_permissions_roles(new_course.location.course_id) # auto-enroll the course creator in the course so that "View Live" will work. - enroll_in_course(request.user, new_course.location.course_id) + CourseEnrollment.enroll(request.user, new_course.location.course_id) return JsonResponse({'id': new_course.location.url()}) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index 8b92107e885b..d98931f65e85 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -24,7 +24,7 @@ from .access import has_access -from student.views import enroll_in_course +from student.models import CourseEnrollment @login_required @@ -208,7 +208,7 @@ def course_team_user(request, org, course, name, email): user.groups.add(groups["instructor"]) user.save() # auto-enroll the course creator in the course so that "View Live" will work. - enroll_in_course(user, location.course_id) + CourseEnrollment.enroll(user, location.course_id) elif role == "staff": # if we're trying to downgrade a user from "instructor" to "staff", # make sure we have at least one other instructor in the course team. @@ -223,7 +223,7 @@ def course_team_user(request, org, course, name, email): user.groups.add(groups["staff"]) user.save() # auto-enroll the course creator in the course so that "View Live" will work. - enroll_in_course(user, location.course_id) + CourseEnrollment.enroll(user, location.course_id) return JsonResponse() diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index ec722b718a72..7878f1b453cf 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -19,6 +19,20 @@ @receiver(post_save, sender=CourseEnrollment) def assign_default_role(sender, instance, **kwargs): + # The code below would remove all forum Roles from a user when they unenroll + # from a course. Concerns were raised that it should apply only to students, + # or that even the history of student roles is important for research + # purposes. Since this was new functionality being added in this release, + # I'm just going to comment it out for now and let the forums team deal with + # implementing the right behavior. + # + # # We've unenrolled the student, so remove all roles for this course + # if not instance.is_active: + # course_roles = list(Role.objects.filter(course_id=instance.course_id)) + # instance.user.roles.remove(*course_roles) + # return + + # We've enrolled the student, so make sure they have a default role if instance.user.is_staff: role = Role.objects.get_or_create(course_id=instance.course_id, name="Moderator")[0] else: diff --git a/common/djangoapps/django_comment_common/tests.py b/common/djangoapps/django_comment_common/tests.py new file mode 100644 index 000000000000..47790f1e1edd --- /dev/null +++ b/common/djangoapps/django_comment_common/tests.py @@ -0,0 +1,58 @@ +from django.test import TestCase + +from django_comment_common.models import Role +from student.models import CourseEnrollment, User + +class RoleAssignmentTest(TestCase): + """ + Basic checks to make sure our Roles get assigned and unassigned as students + are enrolled and unenrolled from a course. + """ + + def setUp(self): + self.staff_user = User.objects.create_user( + "patty", + "patty@fake.edx.org", + ) + self.staff_user.is_staff = True + + self.student_user = User.objects.create_user( + "hacky", + "hacky@fake.edx.org" + ) + self.course_id = "edX/Fake101/2012" + CourseEnrollment.enroll(self.staff_user, self.course_id) + CourseEnrollment.enroll(self.student_user, self.course_id) + + def test_enrollment_auto_role_creation(self): + moderator_role = Role.objects.get( + course_id=self.course_id, + name="Moderator" + ) + student_role = Role.objects.get( + course_id=self.course_id, + name="Student" + ) + self.assertIn(moderator_role, self.staff_user.roles.all()) + + self.assertIn(student_role, self.student_user.roles.all()) + self.assertNotIn(moderator_role, self.student_user.roles.all()) + + # The following was written on the assumption that unenrolling from a course + # should remove all forum Roles for that student for that course. This is + # not necessarily the case -- please see comments at the top of + # django_comment_client.models.assign_default_role(). Leaving it for the + # forums team to sort out. + # + # def test_unenrollment_auto_role_removal(self): + # another_student = User.objects.create_user("sol", "sol@fake.edx.org") + # CourseEnrollment.enroll(another_student, self.course_id) + # + # CourseEnrollment.unenroll(self.student_user, self.course_id) + # # Make sure we didn't delete the actual Role + # student_role = Role.objects.get( + # course_id=self.course_id, + # name="Student" + # ) + # self.assertNotIn(student_role, self.student_user.roles.all()) + # self.assertIn(student_role, another_student.roles.all()) diff --git a/common/djangoapps/external_auth/tests/test_shib.py b/common/djangoapps/external_auth/tests/test_shib.py index 428119b88650..6bb9c38e6f2e 100644 --- a/common/djangoapps/external_auth/tests/test_shib.py +++ b/common/djangoapps/external_auth/tests/test_shib.py @@ -431,12 +431,12 @@ def test_enrollment_limit_by_domain(self): # If course is not limited or student has correct shib extauth then enrollment should be allowed if course is open_enroll_course or student is shib_student: self.assertEqual(response.status_code, 200) - self.assertEqual(CourseEnrollment.objects.filter(user=student, course_id=course.id).count(), 1) + self.assertTrue(CourseEnrollment.is_enrolled(student, course.id)) # Clean up - CourseEnrollment.objects.filter(user=student, course_id=course.id).delete() + CourseEnrollment.unenroll(student, course.id) else: self.assertEqual(response.status_code, 400) - self.assertEqual(CourseEnrollment.objects.filter(user=student, course_id=course.id).count(), 0) + self.assertFalse(CourseEnrollment.is_enrolled(student, course.id)) @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), True) def test_shib_login_enrollment(self): @@ -462,7 +462,7 @@ def test_shib_login_enrollment(self): # use django test client for sessions and url processing # no enrollment before trying - self.assertEqual(CourseEnrollment.objects.filter(user=student, course_id=course.id).count(), 0) + self.assertFalse(CourseEnrollment.is_enrolled(student, course.id)) self.client.logout() request_kwargs = {'path': '/shib-login/', 'data': {'enrollment_action': 'enroll', 'course_id': course.id}, @@ -474,4 +474,4 @@ def test_shib_login_enrollment(self): self.assertEqual(response.status_code, 302) self.assertEqual(response['location'], 'http://testserver/') # now there is enrollment - self.assertEqual(CourseEnrollment.objects.filter(user=student, course_id=course.id).count(), 1) + self.assertTrue(CourseEnrollment.is_enrolled(student, course.id)) diff --git a/common/djangoapps/student/management/commands/create_random_users.py b/common/djangoapps/student/management/commands/create_random_users.py index 3000c8660155..db4bb796cc71 100644 --- a/common/djangoapps/student/management/commands/create_random_users.py +++ b/common/djangoapps/student/management/commands/create_random_users.py @@ -12,7 +12,7 @@ def create(n, course_id): for i in range(n): (user, user_profile, _) = _do_create_account(get_random_post_override()) if course_id is not None: - CourseEnrollment.objects.create(user=user, course_id=course_id) + CourseEnrollment.enroll(user, course_id) class Command(BaseCommand): diff --git a/common/djangoapps/student/migrations/0027_add_active_flag_and_mode_to_courseware_enrollment.py b/common/djangoapps/student/migrations/0027_add_active_flag_and_mode_to_courseware_enrollment.py new file mode 100644 index 000000000000..bba8cc6e344d --- /dev/null +++ b/common/djangoapps/student/migrations/0027_add_active_flag_and_mode_to_courseware_enrollment.py @@ -0,0 +1,183 @@ +# -*- coding: utf-8 -*- +import datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding field 'CourseEnrollment.is_active' + db.add_column('student_courseenrollment', 'is_active', + self.gf('django.db.models.fields.BooleanField')(default=True), + keep_default=False) + + # Adding field 'CourseEnrollment.mode' + db.add_column('student_courseenrollment', 'mode', + self.gf('django.db.models.fields.CharField')(default='honor', max_length=100), + keep_default=False) + + + def backwards(self, orm): + # Deleting field 'CourseEnrollment.is_active' + db.delete_column('student_courseenrollment', 'is_active') + + # Deleting field 'CourseEnrollment.mode' + db.delete_column('student_courseenrollment', 'mode') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, + 'student.courseenrollment': { + 'Meta': {'ordering': "('user', 'course_id')", 'unique_together': "(('user', 'course_id'),)", 'object_name': 'CourseEnrollment'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'null': 'True', 'db_index': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'mode': ('django.db.models.fields.CharField', [], {'default': "'honor'", 'max_length': '100'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) + }, + 'student.courseenrollmentallowed': { + 'Meta': {'unique_together': "(('email', 'course_id'),)", 'object_name': 'CourseEnrollmentAllowed'}, + 'auto_enroll': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'null': 'True', 'db_index': 'True', 'blank': 'True'}), + 'email': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'student.pendingemailchange': { + 'Meta': {'object_name': 'PendingEmailChange'}, + 'activation_key': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'new_email': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['auth.User']", 'unique': 'True'}) + }, + 'student.pendingnamechange': { + 'Meta': {'object_name': 'PendingNameChange'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'new_name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}), + 'rationale': ('django.db.models.fields.CharField', [], {'max_length': '1024', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['auth.User']", 'unique': 'True'}) + }, + 'student.registration': { + 'Meta': {'object_name': 'Registration', 'db_table': "'auth_registration'"}, + 'activation_key': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'unique': 'True'}) + }, + 'student.testcenterregistration': { + 'Meta': {'object_name': 'TestCenterRegistration'}, + 'accommodation_code': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}), + 'accommodation_request': ('django.db.models.fields.CharField', [], {'max_length': '1024', 'blank': 'True'}), + 'authorization_id': ('django.db.models.fields.IntegerField', [], {'null': 'True', 'db_index': 'True'}), + 'client_authorization_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '20', 'db_index': 'True'}), + 'confirmed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'db_index': 'True', 'blank': 'True'}), + 'eligibility_appointment_date_first': ('django.db.models.fields.DateField', [], {'db_index': 'True'}), + 'eligibility_appointment_date_last': ('django.db.models.fields.DateField', [], {'db_index': 'True'}), + 'exam_series_code': ('django.db.models.fields.CharField', [], {'max_length': '15', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'processed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'testcenter_user': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['student.TestCenterUser']"}), + 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'db_index': 'True', 'blank': 'True'}), + 'upload_error_message': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), + 'upload_status': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '20', 'blank': 'True'}), + 'uploaded_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'user_updated_at': ('django.db.models.fields.DateTimeField', [], {'db_index': 'True'}) + }, + 'student.testcenteruser': { + 'Meta': {'object_name': 'TestCenterUser'}, + 'address_1': ('django.db.models.fields.CharField', [], {'max_length': '40'}), + 'address_2': ('django.db.models.fields.CharField', [], {'max_length': '40', 'blank': 'True'}), + 'address_3': ('django.db.models.fields.CharField', [], {'max_length': '40', 'blank': 'True'}), + 'candidate_id': ('django.db.models.fields.IntegerField', [], {'null': 'True', 'db_index': 'True'}), + 'city': ('django.db.models.fields.CharField', [], {'max_length': '32', 'db_index': 'True'}), + 'client_candidate_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '50', 'db_index': 'True'}), + 'company_name': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '50', 'blank': 'True'}), + 'confirmed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'country': ('django.db.models.fields.CharField', [], {'max_length': '3', 'db_index': 'True'}), + 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'db_index': 'True', 'blank': 'True'}), + 'extension': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '8', 'blank': 'True'}), + 'fax': ('django.db.models.fields.CharField', [], {'max_length': '35', 'blank': 'True'}), + 'fax_country_code': ('django.db.models.fields.CharField', [], {'max_length': '3', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '50', 'db_index': 'True'}), + 'middle_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'phone': ('django.db.models.fields.CharField', [], {'max_length': '35'}), + 'phone_country_code': ('django.db.models.fields.CharField', [], {'max_length': '3', 'db_index': 'True'}), + 'postal_code': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '16', 'blank': 'True'}), + 'processed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'salutation': ('django.db.models.fields.CharField', [], {'max_length': '50', 'blank': 'True'}), + 'state': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '20', 'blank': 'True'}), + 'suffix': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}), + 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'db_index': 'True', 'blank': 'True'}), + 'upload_error_message': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), + 'upload_status': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '20', 'blank': 'True'}), + 'uploaded_at': ('django.db.models.fields.DateTimeField', [], {'db_index': 'True', 'null': 'True', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['auth.User']", 'unique': 'True'}), + 'user_updated_at': ('django.db.models.fields.DateTimeField', [], {'db_index': 'True'}) + }, + 'student.userprofile': { + 'Meta': {'object_name': 'UserProfile', 'db_table': "'auth_userprofile'"}, + 'allow_certificate': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'courseware': ('django.db.models.fields.CharField', [], {'default': "'course.xml'", 'max_length': '255', 'blank': 'True'}), + 'gender': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '6', 'null': 'True', 'blank': 'True'}), + 'goals': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'language': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'level_of_education': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '6', 'null': 'True', 'blank': 'True'}), + 'location': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'mailing_address': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'meta': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'profile'", 'unique': 'True', 'to': "orm['auth.User']"}), + 'year_of_birth': ('django.db.models.fields.IntegerField', [], {'db_index': 'True', 'null': 'True', 'blank': 'True'}) + }, + 'student.usertestgroup': { + 'Meta': {'object_name': 'UserTestGroup'}, + 'description': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '32', 'db_index': 'True'}), + 'users': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.User']", 'db_index': 'True', 'symmetrical': 'False'}) + } + } + + complete_apps = ['student'] \ No newline at end of file diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 34278c55810f..6b5897e97d25 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -11,11 +11,11 @@ 3. Add the migration file created in edx-platform/common/djangoapps/student/migrations/ """ from datetime import datetime +from random import randint import hashlib import json import logging import uuid -from random import randint from django.conf import settings @@ -645,16 +645,223 @@ class PendingEmailChange(models.Model): class CourseEnrollment(models.Model): + """ + Represents a Student's Enrollment record for a single Course. You should + generally not manipulate CourseEnrollment objects directly, but use the + classmethods provided to enroll, unenroll, or check on the enrollment status + of a given student. + + We're starting to consolidate course enrollment logic in this class, but + more should be brought in (such as checking against CourseEnrollmentAllowed, + checking course dates, user permissions, etc.) This logic is currently + scattered across our views. + """ user = models.ForeignKey(User) course_id = models.CharField(max_length=255, db_index=True) - created = models.DateTimeField(auto_now_add=True, null=True, db_index=True) + # If is_active is False, then the student is not considered to be enrolled + # in the course (is_enrolled() will return False) + is_active = models.BooleanField(default=True) + + # Represents the modes that are possible. We'll update this later with a + # list of possible values. + mode = models.CharField(default="honor", max_length=100) + + class Meta: unique_together = (('user', 'course_id'),) + ordering = ('user', 'course_id') def __unicode__(self): - return "[CourseEnrollment] %s: %s (%s)" % (self.user, self.course_id, self.created) + return ( + "[CourseEnrollment] {}: {} ({}); active: ({})" + ).format(self.user, self.course_id, self.created, self.is_active) + + @classmethod + def create_enrollment(cls, user, course_id, mode="honor", is_active=False): + """ + Create an enrollment for a user in a class. By default *this enrollment + is not active*. This is useful for when an enrollment needs to go + through some sort of approval process before being activated. If you + don't need this functionality, just call `enroll()` instead. + + Returns a CoursewareEnrollment object. + + `user` is a Django User object. If it hasn't been saved yet (no `.id` + attribute), this method will automatically save it before + adding an enrollment for it. + + `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) + + `mode` is a string specifying what kind of enrollment this is. The + default is "honor", meaning honor certificate. Future options + may include "audit", "verified_id", etc. Please don't use it + until we have these mapped out. + + `is_active` is a boolean. If the CourseEnrollment object has + `is_active=False`, then calling + `CourseEnrollment.is_enrolled()` for that user/course_id + will return False. + + It is expected that this method is called from a method which has already + verified the user authentication and access. + """ + # 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 + # into our CourseEnrollment object. Otherwise, we'll get an + # IntegrityError for having a null user_id. + if user.id is None: + user.save() + + enrollment, _ = CourseEnrollment.objects.get_or_create( + user=user, + course_id=course_id, + ) + # In case we're reactivating a deactivated enrollment, or changing the + # enrollment mode. + if enrollment.mode != mode or enrollment.is_active != is_active: + enrollment.mode = mode + enrollment.is_active = is_active + enrollment.save() + + return enrollment + + @classmethod + def enroll(cls, user, course_id, mode="honor"): + """ + Enroll a user in a course. This saves immediately. + + Returns a CoursewareEnrollment object. + + `user` is a Django User object. If it hasn't been saved yet (no `.id` + attribute), this method will automatically save it before + adding an enrollment for it. + + `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) + + `mode` is a string specifying what kind of enrollment this is. The + default is "honor", meaning honor certificate. Future options + may include "audit", "verified_id", etc. Please don't use it + until we have these mapped out. + + It is expected that this method is called from a method which has already + verified the user authentication and access. + """ + return cls.create_enrollment(user, course_id, mode, is_active=True) + + @classmethod + def enroll_by_email(cls, email, course_id, mode="honor", ignore_errors=True): + """ + Enroll a user in a course given their email. This saves immediately. + + Note that enrolling by email is generally done in big batches and the + 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 + `ignore_errors` is set to `True`, it will return None. + + `email` Email address of the User to add to enroll in the course. + + `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) + + `mode` is a string specifying what kind of enrollment this is. The + default is "honor", meaning honor certificate. Future options + may include "audit", "verified_id", etc. Please don't use it + until we have these mapped out. + + `ignore_errors` is a boolean indicating whether we should suppress + `User.DoesNotExist` errors (returning None) or let it + bubble up. + + It is expected that this method is called from a method which has already + verified the user authentication and access. + """ + try: + user = User.objects.get(email=email) + return cls.enroll(user, course_id, mode) + except User.DoesNotExist: + err_msg = u"Tried to enroll email {} into course {}, but user not found" + log.error(err_msg.format(email, course_id)) + if ignore_errors: + return None + raise + + @classmethod + def unenroll(cls, user, course_id): + """ + Remove the user from a given course. If the relevant `CourseEnrollment` + object doesn't exist, we log an error but don't throw an exception. + + `user` is a Django User object. If it hasn't been saved yet (no `.id` + attribute), this method will automatically save it before + adding an enrollment for it. + + `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) + """ + try: + record = CourseEnrollment.objects.get(user=user, course_id=course_id) + record.is_active = False + record.save() + except cls.DoesNotExist: + log.error("Tried to unenroll student {} from {} but they were not enrolled") + + @classmethod + def unenroll_by_email(cls, email, course_id): + """ + Unenroll a user from a course given their email. This saves immediately. + User lookup errors are logged but will not throw an exception. + + `email` Email address of the User to unenroll from the course. + + `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) + """ + try: + user = User.objects.get(email=email) + return cls.unenroll(user, course_id) + except User.DoesNotExist: + err_msg = u"Tried to unenroll email {} from course {}, but user not found" + log.error(err_msg.format(email, course_id)) + + @classmethod + def is_enrolled(cls, user, course_id): + """ + Remove the user from a given course. If the relevant `CourseEnrollment` + object doesn't exist, we log an error but don't throw an exception. + + Returns True if the user is enrolled in the course (the entry must exist + and it must have `is_active=True`). Otherwise, returns False. + + `user` is a Django User object. If it hasn't been saved yet (no `.id` + attribute), this method will automatically save it before + adding an enrollment for it. + + `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) + """ + try: + record = CourseEnrollment.objects.get(user=user, course_id=course_id) + return record.is_active + except cls.DoesNotExist: + return False + + @classmethod + def enrollments_for_user(cls, user): + return CourseEnrollment.objects.filter(user=user, is_active=1) + + def activate(self): + """Makes this `CourseEnrollment` record active. Saves immediately.""" + if not self.is_active: + self.is_active = True + self.save() + + def deactivate(self): + """Makes this `CourseEnrollment` record inactive. Saves immediately. An + inactive record means that the student is not enrolled in this course. + """ + if self.is_active: + self.is_active = False + self.save() class CourseEnrollmentAllowed(models.Model): diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 513216ba1744..397816ec00ab 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -21,9 +21,8 @@ from mock import Mock, patch from textwrap import dedent -from student.models import unique_id_for_user +from student.models import unique_id_for_user, CourseEnrollment from student.views import process_survey_link, _cert_info, password_reset, password_reset_confirm_wrapper -from student.views import enroll_in_course, is_enrolled_in_course from student.tests.factories import UserFactory from student.tests.test_email import mock_render_to_string COURSE_1 = 'edX/toy/2012_Fall' @@ -209,12 +208,127 @@ def test_cert_info(self): class EnrollInCourseTest(TestCase): - """ Tests the helper method for enrolling a user in a class """ + """Tests enrolling and unenrolling in courses.""" - def test_enroll_in_course(self): + def test_enrollment(self): user = User.objects.create_user("joe", "joe@joe.com", "password") - user.save() - course_id = "course_id" - self.assertFalse(is_enrolled_in_course(user, course_id)) - enroll_in_course(user, course_id) - self.assertTrue(is_enrolled_in_course(user, course_id)) + course_id = "edX/Test101/2013" + + # Test basic enrollment + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + CourseEnrollment.enroll(user, course_id) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + + # Enrolling them again should be harmless + CourseEnrollment.enroll(user, course_id) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + + # Now unenroll the user + CourseEnrollment.unenroll(user, course_id) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # Unenrolling them again should also be harmless + CourseEnrollment.unenroll(user, course_id) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # The enrollment record should still exist, just be inactive + enrollment_record = CourseEnrollment.objects.get( + user=user, + course_id=course_id + ) + self.assertFalse(enrollment_record.is_active) + + def test_enrollment_non_existent_user(self): + # Testing enrollment of newly unsaved user (i.e. no database entry) + user = User(username="rusty", email="rusty@fake.edx.org") + course_id = "edX/Test101/2013" + + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # Unenroll does nothing + CourseEnrollment.unenroll(user, course_id) + + # Implicit save() happens on new User object when enrolling, so this + # should still work + CourseEnrollment.enroll(user, course_id) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + + def test_enrollment_by_email(self): + user = User.objects.create(username="jack", email="jack@fake.edx.org") + course_id = "edX/Test101/2013" + + CourseEnrollment.enroll_by_email("jack@fake.edx.org", course_id) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + + # This won't throw an exception, even though the user is not found + self.assertIsNone( + CourseEnrollment.enroll_by_email("not_jack@fake.edx.org", course_id) + ) + + self.assertRaises( + User.DoesNotExist, + CourseEnrollment.enroll_by_email, + "not_jack@fake.edx.org", + course_id, + ignore_errors=False + ) + + # Now unenroll them by email + CourseEnrollment.unenroll_by_email("jack@fake.edx.org", course_id) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # Harmless second unenroll + CourseEnrollment.unenroll_by_email("jack@fake.edx.org", course_id) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # Unenroll on non-existent user shouldn't throw an error + CourseEnrollment.unenroll_by_email("not_jack@fake.edx.org", course_id) + + def test_enrollment_multiple_classes(self): + user = User(username="rusty", email="rusty@fake.edx.org") + course_id1 = "edX/Test101/2013" + course_id2 = "MITx/6.003z/2012" + + CourseEnrollment.enroll(user, course_id1) + CourseEnrollment.enroll(user, course_id2) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id1)) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id2)) + + CourseEnrollment.unenroll(user, course_id1) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id1)) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id2)) + + CourseEnrollment.unenroll(user, course_id2) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id1)) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id2)) + + def test_activation(self): + user = User.objects.create(username="jack", email="jack@fake.edx.org") + course_id = "edX/Test101/2013" + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # Creating an enrollment doesn't actually enroll a student + # (calling CourseEnrollment.enroll() would have) + enrollment = CourseEnrollment.create_enrollment(user, course_id) + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # Until you explicitly activate it + enrollment.activate() + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + + # Activating something that's already active does nothing + enrollment.activate() + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) + + # Now deactive + enrollment.deactivate() + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # Deactivating something that's already inactive does nothing + enrollment.deactivate() + self.assertFalse(CourseEnrollment.is_enrolled(user, course_id)) + + # A deactivated enrollment should be activated if enroll() is called + # for that user/course_id combination + CourseEnrollment.enroll(user, course_id) + self.assertTrue(CourseEnrollment.is_enrolled(user, course_id)) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 9b96b90deef6..7795a13c4783 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -254,13 +254,12 @@ def register_user(request, extra_context=None): @ensure_csrf_cookie def dashboard(request): user = request.user - enrollments = CourseEnrollment.objects.filter(user=user) # Build our courses list for the user, but ignore any courses that no longer # exist (because the course IDs have changed). Still, we don't delete those # enrollments, because it could have been a data push snafu. courses = [] - for enrollment in enrollments: + for enrollment in CourseEnrollment.enrollments_for_user(user): try: courses.append(course_from_id(enrollment.course_id)) except ItemNotFoundError: @@ -377,18 +376,13 @@ def change_enrollment(request): "course:{0}".format(course_num), "run:{0}".format(run)]) - try: - enroll_in_course(user, course.id) - except IntegrityError: - # If we've already created this enrollment in a separate transaction, - # then just continue - pass + CourseEnrollment.enroll(user, course.id) + return HttpResponse() elif action == "unenroll": try: - enrollment = CourseEnrollment.objects.get(user=user, course_id=course_id) - enrollment.delete() + CourseEnrollment.unenroll(user, course_id) org, course_num, run = course_id.split("/") statsd.increment("common.student.unenrollment", @@ -402,30 +396,10 @@ def change_enrollment(request): else: return HttpResponseBadRequest(_("Enrollment action is invalid")) - -def enroll_in_course(user, course_id): - """ - Helper method to enroll a user in a particular class. - - It is expected that this method is called from a method which has already - verified the user authentication and access. - """ - CourseEnrollment.objects.get_or_create(user=user, course_id=course_id) - - -def is_enrolled_in_course(user, course_id): - """ - Helper method that returns whether or not the user is enrolled in a particular course. - """ - return CourseEnrollment.objects.filter(user=user, course_id=course_id).count() > 0 - - @ensure_csrf_cookie def accounts_login(request, error=""): - return render_to_response('login.html', {'error': error}) - # Need different levels of logging @ensure_csrf_cookie def login_user(request, error=""): @@ -1008,13 +982,21 @@ def activate_account(request, key): ceas = CourseEnrollmentAllowed.objects.filter(email=student[0].email) for cea in ceas: if cea.auto_enroll: - course_id = cea.course_id - _enrollment, _created = CourseEnrollment.objects.get_or_create(user_id=student[0].id, course_id=course_id) - - resp = render_to_response("registration/activation_complete.html", {'user_logged_in': user_logged_in, 'already_active': already_active}) + CourseEnrollment.enroll(student[0], cea.course_id) + + resp = render_to_response( + "registration/activation_complete.html", + { + 'user_logged_in': user_logged_in, + 'already_active': already_active + } + ) return resp if len(r) == 0: - return render_to_response("registration/activation_invalid.html", {'csrf': csrf(request)['csrf_token']}) + return render_to_response( + "registration/activation_invalid.html", + {'csrf': csrf(request)['csrf_token']} + ) return HttpResponse(_("Unknown error. Please e-mail us to let us know how it happened.")) diff --git a/common/djangoapps/terrain/course_helpers.py b/common/djangoapps/terrain/course_helpers.py index afef8bf2e153..eca329008084 100644 --- a/common/djangoapps/terrain/course_helpers.py +++ b/common/djangoapps/terrain/course_helpers.py @@ -54,7 +54,7 @@ def register_by_course_id(course_id, is_staff=False): if is_staff: u.is_staff = True u.save() - CourseEnrollment.objects.get_or_create(user=u, course_id=course_id) + CourseEnrollment.enroll(u, course_id) @world.absorb diff --git a/docs/data/source/internal_data_formats/sql_schema.rst b/docs/data/source/internal_data_formats/sql_schema.rst index 92c5c4fa0eee..6c5a7eab8c96 100644 --- a/docs/data/source/internal_data_formats/sql_schema.rst +++ b/docs/data/source/internal_data_formats/sql_schema.rst @@ -347,7 +347,7 @@ There is an important split in demographic data gathered for the students who si `student_courseenrollment` ========================== -A row in this table represents a student's enrollment for a particular course run. If they decide to unenroll in the course, we delete their entry in this table, but we still leave all their state in `courseware_studentmodule` untouched. +A row in this table represents a student's enrollment for a particular course run. If they decide to unenroll in the course, we set `is_active` to `False`. We still leave all their state in `courseware_studentmodule` untouched, so they will not lose courseware state if they unenroll and reenroll. `id` ---- @@ -365,6 +365,13 @@ A row in this table represents a student's enrollment for a particular course ru --------- Datetime of enrollment, UTC. +`is_active` +----------- + Boolean indicating whether this enrollment is active. If an enrollment is not active, a student is not enrolled in that course. This lets us unenroll students without losing a record of what courses they were enrolled in previously. This was introduced in the 2013-08-20 release. Before this release, unenrolling a student simply deleted the row in `student_courseenrollment`. + +`mode` +------ + String indicating what kind of enrollment this was. The default is "honor" (honor certificate) and all enrollments prior to 2013-08-20 will be of that type. Other types being considered are "audit" and "verified_id". ******************* diff --git a/lms/djangoapps/analytics/basic.py b/lms/djangoapps/analytics/basic.py index 0a8c6fec0930..3600d0e39385 100644 --- a/lms/djangoapps/analytics/basic.py +++ b/lms/djangoapps/analytics/basic.py @@ -25,8 +25,10 @@ def enrolled_students_features(course_id, features): {'username': 'username3', 'first_name': 'firstname3'} ] """ - students = User.objects.filter(courseenrollment__course_id=course_id)\ - .order_by('username').select_related('profile') + students = User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=1, + ).order_by('username').select_related('profile') def extract_student(student, features): """ convert student to dictionary """ diff --git a/lms/djangoapps/analytics/tests/test_basic.py b/lms/djangoapps/analytics/tests/test_basic.py index e0251c456776..91d6ed45e952 100644 --- a/lms/djangoapps/analytics/tests/test_basic.py +++ b/lms/djangoapps/analytics/tests/test_basic.py @@ -15,7 +15,8 @@ class TestAnalyticsBasic(TestCase): def setUp(self): self.course_id = 'some/robot/course/id' self.users = tuple(UserFactory() for _ in xrange(30)) - self.ces = tuple(CourseEnrollment.objects.create(course_id=self.course_id, user=user) for user in self.users) + self.ces = tuple(CourseEnrollment.enroll(user, self.course_id) + for user in self.users) def test_enrolled_students_features_username(self): self.assertIn('username', AVAILABLE_FEATURES) diff --git a/lms/djangoapps/analytics/tests/test_distributions.py b/lms/djangoapps/analytics/tests/test_distributions.py index 61f948c26d8a..6d314e4a499f 100644 --- a/lms/djangoapps/analytics/tests/test_distributions.py +++ b/lms/djangoapps/analytics/tests/test_distributions.py @@ -19,10 +19,8 @@ def setUp(self): profile__year_of_birth=i + 1930 ) for i in xrange(30)] - self.ces = [CourseEnrollment.objects.create( - course_id=self.course_id, - user=user - ) for user in self.users] + self.ces = [CourseEnrollment.enroll(user, self.course_id) + for user in self.users] @raises(ValueError) def test_profile_distribution_bad_feature(self): @@ -68,7 +66,8 @@ def setUp(self): self.users += self.nodata_users - self.ces = tuple(CourseEnrollment.objects.create(course_id=self.course_id, user=user) for user in self.users) + self.ces = tuple(CourseEnrollment.enroll(user, self.course_id) + for user in self.users) def test_profile_distribution_easy_choice_nodata(self): feature = 'gender' diff --git a/lms/djangoapps/courseware/features/common.py b/lms/djangoapps/courseware/features/common.py index 8b934e435d4a..bf58cbc6fe0b 100644 --- a/lms/djangoapps/courseware/features/common.py +++ b/lms/djangoapps/courseware/features/common.py @@ -53,7 +53,7 @@ def i_am_registered_for_the_course(step, course): # If the user is not already enrolled, enroll the user. # TODO: change to factory - CourseEnrollment.objects.get_or_create(user=u, course_id=course_id(course)) + CourseEnrollment.enroll(u, course_id(course)) world.log_in(username='robot', password='test') diff --git a/lms/djangoapps/courseware/features/navigation.py b/lms/djangoapps/courseware/features/navigation.py index d4ac7060c456..fcbf21b0956a 100644 --- a/lms/djangoapps/courseware/features/navigation.py +++ b/lms/djangoapps/courseware/features/navigation.py @@ -149,7 +149,7 @@ def create_user_and_visit_course(): world.create_user('robot', 'test') u = User.objects.get(username='robot') - CourseEnrollment.objects.get_or_create(user=u, course_id=course_id(world.scenario_dict['COURSE'].number)) + CourseEnrollment.enroll(u, course_id(world.scenario_dict['COURSE'].number)) world.log_in(username='robot', password='test') chapter_name = (TEST_SECTION_NAME + "1").replace(" ", "_") diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index a5121042d112..6f665f734585 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -67,9 +67,9 @@ def setUp(self): email='test@mit.edu') self.date = datetime.datetime(2013, 1, 22, tzinfo=UTC) self.course_id = 'edX/toy/2012_Fall' - self.enrollment = CourseEnrollment.objects.get_or_create(user=self.user, - course_id=self.course_id, - created=self.date)[0] + self.enrollment = CourseEnrollment.enroll(self.user, self.course_id) + self.enrollment.created = self.date + self.enrollment.save() self.location = ['tag', 'org', 'course', 'category', 'name'] self._MODULESTORES = {} # This is a CourseDescriptor object diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 78151882278f..fefa09288a7a 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -568,12 +568,12 @@ def syllabus(request, course_id): def registered_for_course(course, user): """ - Return CourseEnrollment if user is registered for course, else False + Return True if user is registered for course, else False """ if user is None: return False if user.is_authenticated(): - return CourseEnrollment.objects.filter(user=user, course_id=course.id).exists() + return CourseEnrollment.is_enrolled(user, course.id) else: return False diff --git a/lms/djangoapps/dashboard/views.py b/lms/djangoapps/dashboard/views.py index e04588fff48f..630222d7bfe2 100644 --- a/lms/djangoapps/dashboard/views.py +++ b/lms/djangoapps/dashboard/views.py @@ -47,7 +47,7 @@ def dashboard(request): results["scalars"]["Activated Usernames"]=User.objects.filter(is_active=1).count() # count how many enrollments we have - results["scalars"]["Total Enrollments Across All Courses"]=CourseEnrollment.objects.count() + results["scalars"]["Total Enrollments Across All Courses"] = CourseEnrollment.objects.filter(is_active=1).count() # establish a direct connection to the database (for executing raw SQL) cursor = connection.cursor() @@ -56,20 +56,22 @@ def dashboard(request): # table queries need not take the form of raw SQL, but do in this case since # the MySQL backend for django isn't very friendly with group by or distinct table_queries = {} - table_queries["course enrollments"]= \ - "select "+ \ - "course_id as Course, "+ \ - "count(user_id) as Students " + \ - "from student_courseenrollment "+ \ - "group by course_id "+ \ - "order by students desc;" - table_queries["number of students in each number of classes"]= \ - "select registrations as 'Registered for __ Classes' , "+ \ - "count(registrations) as Users "+ \ - "from (select count(user_id) as registrations "+ \ - "from student_courseenrollment "+ \ - "group by user_id) as registrations_per_user "+ \ - "group by registrations;" + table_queries["course enrollments"] = """ + select + course_id as Course, + count(user_id) as Students + from student_courseenrollment + where is_active=1 + group by course_id + order by students desc;""" + table_queries["number of students in each number of classes"] = """ + select registrations as 'Registered for __ Classes' , + count(registrations) as Users + from (select count(user_id) as registrations + from student_courseenrollment + where is_active=1 + group by user_id) as registrations_per_user + group by registrations;""" # add the result for each of the table_queries to the results object for query in table_queries.keys(): diff --git a/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py b/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py index 9ef4f3d0b118..2a58e370af15 100644 --- a/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py +++ b/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py @@ -22,7 +22,7 @@ def handle(self, *args, **options): course_id = args[0] print "Updated roles for ", - for i, enrollment in enumerate(CourseEnrollment.objects.filter(course_id=course_id), start=1): + for i, enrollment in enumerate(CourseEnrollment.objects.filter(course_id=course_id, is_active=1), start=1): assign_default_role(None, enrollment) if i % 1000 == 0: print "{0}...".format(i), diff --git a/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py b/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py index 037bb292ec60..0516c61c7c7a 100644 --- a/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py +++ b/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py @@ -19,7 +19,7 @@ def handle(self, *args, **options): raise CommandError("This Command takes no arguments") print "Updated roles for ", - for i, enrollment in enumerate(CourseEnrollment.objects.all(), start=1): + for i, enrollment in enumerate(CourseEnrollment.objects.filter(is_active=1), start=1): assign_default_role(None, enrollment) if i % 1000 == 0: print "{0}...".format(i), diff --git a/lms/djangoapps/django_comment_client/tests.py b/lms/djangoapps/django_comment_client/tests.py index 8c6a48d8c179..d9f601ad0bce 100644 --- a/lms/djangoapps/django_comment_client/tests.py +++ b/lms/djangoapps/django_comment_client/tests.py @@ -26,8 +26,8 @@ def setUp(self): password="123456", email="staff@edx.org") self.moderator.is_staff = True self.moderator.save() - self.student_enrollment = CourseEnrollment.objects.create(user=self.student, course_id=self.course_id) - self.moderator_enrollment = CourseEnrollment.objects.create(user=self.moderator, course_id=self.course_id) + self.student_enrollment = CourseEnrollment.enroll(self.student, self.course_id) + self.moderator_enrollment = CourseEnrollment.enroll(self.moderator, self.course_id) def tearDown(self): self.student_enrollment.delete() diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index 0b462957ecd9..7205cffe06cd 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -14,7 +14,11 @@ class EmailEnrollmentState(object): """ Store the complete enrollment state of an email in a class """ def __init__(self, course_id, email): exists_user = User.objects.filter(email=email).exists() - exists_ce = CourseEnrollment.objects.filter(course_id=course_id, user__email=email).exists() + if exists_user: + user = User.objects.get(email=email) + exists_ce = CourseEnrollment.is_enrolled(user, course_id) + else: + exists_ce = False ceas = CourseEnrollmentAllowed.objects.filter(course_id=course_id, email=email).all() exists_allowed = len(ceas) > 0 state_auto_enroll = exists_allowed and ceas[0].auto_enroll @@ -66,8 +70,7 @@ def enroll_email(course_id, student_email, auto_enroll=False): previous_state = EmailEnrollmentState(course_id, student_email) if previous_state.user: - user = User.objects.get(email=student_email) - CourseEnrollment.objects.get_or_create(course_id=course_id, user=user) + CourseEnrollment.enroll_by_email(student_email, course_id) else: cea, _ = CourseEnrollmentAllowed.objects.get_or_create(course_id=course_id, email=student_email) cea.auto_enroll = auto_enroll @@ -91,7 +94,7 @@ def unenroll_email(course_id, student_email): previous_state = EmailEnrollmentState(course_id, student_email) if previous_state.enrollment: - CourseEnrollment.objects.get(course_id=course_id, user__email=student_email).delete() + CourseEnrollment.unenroll_by_email(student_email, course_id) if previous_state.allowed: CourseEnrollmentAllowed.objects.get(course_id=course_id, email=student_email).delete() diff --git a/lms/djangoapps/instructor/offline_gradecalc.py b/lms/djangoapps/instructor/offline_gradecalc.py index 97f252e3a9d6..da5fb3f7d46c 100644 --- a/lms/djangoapps/instructor/offline_gradecalc.py +++ b/lms/djangoapps/instructor/offline_gradecalc.py @@ -31,7 +31,10 @@ def offline_grade_calculation(course_id): ''' tstart = time.time() - enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).prefetch_related("groups").order_by('username') + enrolled_students = User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=1 + ).prefetch_related("groups").order_by('username') enc = MyEncoder() diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 51cf682aa56c..155a8a2c9fef 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -98,7 +98,7 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): def setUp(self): self.user = UserFactory.create() self.course = CourseFactory.create() - CourseEnrollment.objects.create(user=self.user, course_id=self.course.id) + CourseEnrollment.enroll(self.user, self.course.id) self.client.login(username=self.user.username, password='test') def test_deny_students_update_enrollment(self): @@ -161,9 +161,9 @@ def setUp(self): self.client.login(username=self.instructor.username, password='test') self.enrolled_student = UserFactory() - CourseEnrollment.objects.create( - user=self.enrolled_student, - course_id=self.course.id + CourseEnrollment.enroll( + self.enrolled_student, + self.course.id ) self.notenrolled_student = UserFactory() @@ -237,7 +237,8 @@ def test_unenroll(self): self.assertEqual( self.enrolled_student.courseenrollment_set.filter( - course_id=self.course.id + course_id=self.course.id, + is_active=1, ).count(), 0 ) @@ -425,7 +426,7 @@ def setUp(self): self.students = [UserFactory() for _ in xrange(6)] for student in self.students: - CourseEnrollment.objects.create(user=student, course_id=self.course.id) + CourseEnrollment.enroll(student, self.course.id) def test_get_students_features(self): """ @@ -535,7 +536,7 @@ def setUp(self): self.client.login(username=self.instructor.username, password='test') self.student = UserFactory() - CourseEnrollment.objects.create(course_id=self.course.id, user=self.student) + CourseEnrollment.enroll(self.student, self.course.id) self.problem_urlname = 'robot-some-problem-urlname' self.module_to_reset = StudentModule.objects.create( @@ -678,7 +679,7 @@ def setUp(self): self.client.login(username=self.instructor.username, password='test') self.student = UserFactory() - CourseEnrollment.objects.create(course_id=self.course.id, user=self.student) + CourseEnrollment.enroll(self.student, self.course.id) self.problem_urlname = 'robot-some-problem-urlname' self.module = StudentModule.objects.create( diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 46f677fe8833..3af71bb466ef 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -353,10 +353,7 @@ def create_user(self, course_id=None): user = UserFactory() email = user.email if self.enrollment: - cenr = CourseEnrollment.objects.create( - user=user, - course_id=course_id - ) + cenr = CourseEnrollment.enroll(user, course_id) return EnrollmentObjects(email, user, cenr, None) else: return EnrollmentObjects(email, user, None, None) diff --git a/lms/djangoapps/instructor/tests/test_legacy_enrollment.py b/lms/djangoapps/instructor/tests/test_legacy_enrollment.py index c6ee82bfb13e..1f5ea8ad5671 100644 --- a/lms/djangoapps/instructor/tests/test_legacy_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_legacy_enrollment.py @@ -51,7 +51,13 @@ def test_unenrollment_email_off(self): # Run the Un-enroll students command url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) - response = self.client.post(url, {'action': 'Unenroll multiple students', 'multiple_students': 'student0@test.com student1@test.com'}) + response = self.client.post( + url, + { + 'action': 'Unenroll multiple students', + 'multiple_students': 'student0@test.com student1@test.com' + } + ) # Check the page output self.assertContains(response, 'student0@test.com') @@ -60,12 +66,10 @@ def test_unenrollment_email_off(self): # Check the enrollment table user = User.objects.get(email='student0@test.com') - ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(0, len(ce)) + self.assertFalse(CourseEnrollment.is_enrolled(user, course.id)) user = User.objects.get(email='student1@test.com') - ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(0, len(ce)) + self.assertFalse(CourseEnrollment.is_enrolled(user, course.id)) # Check the outbox self.assertEqual(len(mail.outbox), 0) @@ -96,7 +100,7 @@ def test_enrollment_new_student_autoenroll_on_email_off(self): self.assertEqual(1, cea[0].auto_enroll) # Check there is no enrollment db entry other than for the other students - ce = CourseEnrollment.objects.filter(course_id=course.id) + ce = CourseEnrollment.objects.filter(course_id=course.id, is_active=1) self.assertEqual(4, len(ce)) # Create and activate student accounts with same email @@ -111,12 +115,10 @@ def test_enrollment_new_student_autoenroll_on_email_off(self): # Check students are enrolled user = User.objects.get(email='student1_1@test.com') - ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(1, len(ce)) + self.assertTrue(CourseEnrollment.is_enrolled(user, course.id)) user = User.objects.get(email='student1_2@test.com') - ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(1, len(ce)) + self.assertTrue(CourseEnrollment.is_enrolled(user, course.id)) def test_repeat_enroll(self): """ @@ -156,7 +158,7 @@ def test_enrollmemt_new_student_autoenroll_off_email_off(self): self.assertEqual(0, cea[0].auto_enroll) # Check there is no enrollment db entry other than for the setup instructor and students - ce = CourseEnrollment.objects.filter(course_id=course.id) + ce = CourseEnrollment.objects.filter(course_id=course.id, is_active=1) self.assertEqual(4, len(ce)) # Create and activate student accounts with same email @@ -171,11 +173,10 @@ def test_enrollmemt_new_student_autoenroll_off_email_off(self): # Check students are not enrolled user = User.objects.get(email='student2_1@test.com') - ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(0, len(ce)) + self.assertFalse(CourseEnrollment.is_enrolled(user, course.id)) + user = User.objects.get(email='student2_2@test.com') - ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(0, len(ce)) + self.assertFalse(CourseEnrollment.is_enrolled(user, course.id)) def test_get_and_clean_student_list(self): """ diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index 42f399c14347..dd4c77644699 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -93,7 +93,7 @@ def get_course_stats_table(): datatable = {'header': ['Statistic', 'Value'], 'title': 'Course Statistics At A Glance', } - data = [['# Enrolled', CourseEnrollment.objects.filter(course_id=course_id).count()]] + data = [['# Enrolled', CourseEnrollment.objects.filter(course_id=course_id, is_active=1).count()]] data += [['Date', timezone.now().isoformat()]] data += compute_course_stats(course).items() if request.user.is_staff: @@ -530,7 +530,10 @@ def domatch(x): # DataDump elif 'Download CSV of all student profile data' in action: - enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username').select_related("profile") + enrolled_students = User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=1, + ).order_by('username').select_related("profile") profkeys = ['name', 'language', 'location', 'year_of_birth', 'gender', 'level_of_education', 'mailing_address', 'goals'] datatable = {'header': ['username', 'email'] + profkeys} @@ -1002,7 +1005,10 @@ def get_student_grade_summary_data(request, course, course_id, get_grades=True, If get_raw_scores=True, then instead of grade summaries, the raw grades for all graded modules are returned. ''' - enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).prefetch_related("groups").order_by('username') + enrolled_students = User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=1, + ).prefetch_related("groups").order_by('username') header = ['ID', 'Username', 'Full Name', 'edX email', 'External email'] assignments = [] @@ -1053,7 +1059,10 @@ def gradebook(request, course_id): """ course = get_course_with_access(request.user, course_id, 'staff', depth=None) - enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username').select_related("profile") + enrolled_students = User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=1 + ).order_by('username').select_related("profile") # TODO (vshnayder): implement pagination. enrolled_students = enrolled_students[:1000] # HACK! @@ -1110,7 +1119,7 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll for ce in todelete: if not has_access(ce.user, course, 'staff') and ce.user.email.lower() not in new_students_lc: status[ce.user.email] = 'deleted' - ce.delete() + ce.deactivate() else: status[ce.user.email] = 'is staff' ceaset = CourseEnrollmentAllowed.objects.filter(course_id=course_id) @@ -1162,14 +1171,13 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll continue #Student has already registered - if CourseEnrollment.objects.filter(user=user, course_id=course_id): + if CourseEnrollment.is_enrolled(user, course_id): status[student] = 'already enrolled' continue try: #Not enrolled yet - ce = CourseEnrollment(user=user, course_id=course_id) - ce.save() + ce = CourseEnrollment.enroll(user, course_id) status[student] = 'added' if email_students: @@ -1239,11 +1247,10 @@ def _do_unenroll_students(course_id, students, email_students=False): continue - ce = CourseEnrollment.objects.filter(user=user, course_id=course_id) #Will be 0 or 1 records as there is a unique key on user + course_id - if ce: + if CourseEnrollment.is_enrolled(user, course_id): try: - ce[0].delete() + CourseEnrollment.unenroll(user, course_id) status[student] = "un-enrolled" if email_students: #User was enrolled