From 3ce87583ab6be7fde4ccfa5aa7be360ff451f505 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 12 Aug 2013 09:20:13 -0400 Subject: [PATCH] Shift enroll/unenroll logic to CourseEnrollment model, add is_active and mode. Features coming down the pipe will want to be able to: * Refer to enrollments before they are actually activated (approval step). * See what courses a user used to be enrolled in for when they re-enroll in the same course, or a different run of that course. * Have different "modes" of enrolling in a course, representing things like honor certificate enrollment, auditing (no certs), etc. This change adds an is_active flag and mode (with default being "honor"). The commit is only as large as it is because many parts of the codebase were manipulating enrollments by adding and removing CourseEnrollment objects directly. It was necessary to create classmethods on CourseEnrollment to encapsulate this functionality and then port everything over to using them. The migration to add columns has been tested on a prod replica, and seems to be fine for running on a live system with single digit millions of rows of enrollments. --- CHANGELOG.rst | 9 + .../contentstore/tests/test_contentstore.py | 8 +- .../contentstore/tests/test_users.py | 6 +- cms/djangoapps/contentstore/views/course.py | 4 +- cms/djangoapps/contentstore/views/user.py | 6 +- .../django_comment_common/models.py | 14 ++ .../djangoapps/django_comment_common/tests.py | 58 +++++ .../external_auth/tests/test_shib.py | 10 +- .../commands/create_random_users.py | 2 +- ..._flag_and_mode_to_courseware_enrollment.py | 183 +++++++++++++++ common/djangoapps/student/models.py | 213 +++++++++++++++++- common/djangoapps/student/tests/tests.py | 132 ++++++++++- common/djangoapps/student/views.py | 52 ++--- common/djangoapps/terrain/course_helpers.py | 2 +- .../internal_data_formats/sql_schema.rst | 9 +- lms/djangoapps/analytics/basic.py | 6 +- lms/djangoapps/analytics/tests/test_basic.py | 3 +- .../analytics/tests/test_distributions.py | 9 +- lms/djangoapps/courseware/features/common.py | 2 +- .../courseware/features/navigation.py | 2 +- lms/djangoapps/courseware/tests/test_views.py | 6 +- lms/djangoapps/courseware/views.py | 4 +- lms/djangoapps/dashboard/views.py | 32 +-- .../commands/assign_roles_for_course.py | 2 +- .../commands/create_roles_for_existing.py | 2 +- lms/djangoapps/django_comment_client/tests.py | 4 +- lms/djangoapps/instructor/enrollment.py | 11 +- .../instructor/offline_gradecalc.py | 5 +- lms/djangoapps/instructor/tests/test_api.py | 17 +- .../instructor/tests/test_enrollment.py | 5 +- .../tests/test_legacy_enrollment.py | 31 +-- lms/djangoapps/instructor/views/legacy.py | 29 ++- 32 files changed, 734 insertions(+), 144 deletions(-) create mode 100644 common/djangoapps/django_comment_common/tests.py create mode 100644 common/djangoapps/student/migrations/0027_add_active_flag_and_mode_to_courseware_enrollment.py 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