Merge pull request #651 from edx/ormsbee/enrollment_modes

Add mode and is_active to CourseEnrollment, shift enrollment logic to model

CHANGELOG.rst

@@ -11,6 +11,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).
 

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"""

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'
         )

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()})
 

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()
 

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:

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())

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))

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):