From a227b14fdd879156c63599f957f5b53f3503a200 Mon Sep 17 00:00:00 2001
From: cahrens <christina@edx.org>
Date: Mon, 5 Aug 2013 13:58:43 -0400
Subject: [PATCH 1/2] Auto-enroll course staff to fix "View Live".

STUD-554

Code review feedback.
---
 CHANGELOG.rst                                 |  3 +
 .../contentstore/tests/test_contentstore.py   | 13 +++-
 .../contentstore/tests/test_users.py          | 66 ++++++++++++++++++-
 cms/djangoapps/contentstore/tests/utils.py    | 10 +++
 cms/djangoapps/contentstore/views/course.py   |  5 ++
 cms/djangoapps/contentstore/views/user.py     |  7 ++
 6 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index cba3d2dbf8..9100e05c68 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -5,6 +5,9 @@ These are notable changes in edx-platform.  This is a rolling list of changes,
 in roughly chronological order, most recent first.  Add your entries at or near
 the top.  Include a label indicating the component affected.
 
+Studio: Studio course authors (both instructors and staff) will be auto-enrolled
+for their courses so that "View Live" works.
+
 Common: Added ratelimiting to our authentication backend.
 
 Common: Add additional logging to cover login attempts and logouts.
diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py
index 4c9fcf7f81..3b2cb7dcc9 100644
--- a/cms/djangoapps/contentstore/tests/test_contentstore.py
+++ b/cms/djangoapps/contentstore/tests/test_contentstore.py
@@ -49,7 +49,7 @@ import datetime
 from pytz import UTC
 from uuid import uuid4
 from pymongo import MongoClient
-
+from .utils import get_enrollment_count
 
 TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
 TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex
@@ -1041,12 +1041,18 @@ class ContentStoreTest(ModuleStoreTestCase):
         data = parse_json(resp)
         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.assertEqual(1, get_enrollment_count(self.user, self._get_course_id(test_course_data)))
         return test_course_data
 
     def test_create_course_check_forum_seeding(self):
         """Test new course creation and verify forum seeding """
         test_course_data = self.assert_created_course(number_suffix=uuid4().hex)
-        self.assertTrue(are_permissions_roles_seeded('MITx/{0}/2013_Spring'.format(test_course_data['number'])))
+        self.assertTrue(are_permissions_roles_seeded(self._get_course_id(test_course_data)))
+
+    def _get_course_id(self, test_course_data):
+        """Returns the course ID (org/number/run)."""
+        return "{org}/{number}/{run}".format(**test_course_data)
 
     def test_create_course_duplicate_course(self):
         """Test new course creation - error path"""
@@ -1057,10 +1063,13 @@ class ContentStoreTest(ModuleStoreTestCase):
         """
         Checks that the course did not get created
         """
+        course_id = self._get_course_id(self.course_data)
+        initially_enrolled_count = get_enrollment_count(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)
+        self.assertEqual(initially_enrolled_count, get_enrollment_count(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 4b9dcf487f..54981b3c49 100644
--- a/cms/djangoapps/contentstore/tests/test_users.py
+++ b/cms/djangoapps/contentstore/tests/test_users.py
@@ -1,5 +1,8 @@
+"""
+Tests for contentstore/views/user.py.
+"""
 import json
-from .utils import CourseTestCase
+from .utils import CourseTestCase, get_enrollment_count
 from django.contrib.auth.models import User, Group
 from django.core.urlresolvers import reverse
 from auth.authz import get_course_groupname_for_role
@@ -90,6 +93,7 @@ class UsersTestCase(CourseTestCase):
         # no content: should not be in any roles
         self.assertNotIn(self.staff_groupname, groups)
         self.assertNotIn(self.inst_groupname, groups)
+        self.assert_not_enrolled()
 
     def test_detail_post_staff(self):
         resp = self.client.post(
@@ -104,6 +108,7 @@ class UsersTestCase(CourseTestCase):
         groups = [g.name for g in ext_user.groups.all()]
         self.assertIn(self.staff_groupname, groups)
         self.assertNotIn(self.inst_groupname, groups)
+        self.assert_enrolled()
 
     def test_detail_post_staff_other_inst(self):
         inst_group, _ = Group.objects.get_or_create(name=self.inst_groupname)
@@ -122,6 +127,7 @@ class UsersTestCase(CourseTestCase):
         groups = [g.name for g in ext_user.groups.all()]
         self.assertIn(self.staff_groupname, groups)
         self.assertNotIn(self.inst_groupname, groups)
+        self.assert_enrolled()
         # check that other user is unchanged
         user = User.objects.get(email=self.user.email)
         groups = [g.name for g in user.groups.all()]
@@ -141,6 +147,7 @@ class UsersTestCase(CourseTestCase):
         groups = [g.name for g in ext_user.groups.all()]
         self.assertNotIn(self.staff_groupname, groups)
         self.assertIn(self.inst_groupname, groups)
+        self.assert_enrolled()
 
     def test_detail_post_missing_role(self):
         resp = self.client.post(
@@ -152,6 +159,7 @@ class UsersTestCase(CourseTestCase):
         self.assert4XX(resp.status_code)
         result = json.loads(resp.content)
         self.assertIn("error", result)
+        self.assert_not_enrolled()
 
     def test_detail_post_bad_json(self):
         resp = self.client.post(
@@ -163,6 +171,7 @@ class UsersTestCase(CourseTestCase):
         self.assert4XX(resp.status_code)
         result = json.loads(resp.content)
         self.assertIn("error", result)
+        self.assert_not_enrolled()
 
     def test_detail_post_no_json(self):
         resp = self.client.post(
@@ -176,6 +185,7 @@ class UsersTestCase(CourseTestCase):
         groups = [g.name for g in ext_user.groups.all()]
         self.assertIn(self.staff_groupname, groups)
         self.assertNotIn(self.inst_groupname, groups)
+        self.assert_enrolled()
 
     def test_detail_delete_staff(self):
         group, _ = Group.objects.get_or_create(name=self.staff_groupname)
@@ -317,3 +327,57 @@ class UsersTestCase(CourseTestCase):
         ext_user = User.objects.get(email=self.ext_user.email)
         groups = [g.name for g in ext_user.groups.all()]
         self.assertIn(self.staff_groupname, groups)
+
+    def test_user_not_initially_enrolled(self):
+        # Verify that ext_user is not enrolled in the new course before being added as a staff member.
+        self.assert_not_enrolled()
+
+    def test_remove_staff_does_not_enroll(self):
+        # Add user with staff permissions.
+        self.client.post(
+            self.detail_url,
+            data=json.dumps({"role": "staff"}),
+            content_type="application/json",
+            HTTP_ACCEPT="application/json",
+        )
+        self.assert_enrolled()
+        # Remove user from staff on course. Will not un-enroll them from the course.
+        resp = self.client.delete(
+            self.detail_url,
+            HTTP_ACCEPT="application/json",
+        )
+        self.assert2XX(resp.status_code)
+        self.assert_enrolled()
+
+    def test_staff_to_instructor_still_enrolled(self):
+        # Add user with staff permission.
+        self.client.post(
+            self.detail_url,
+            data=json.dumps({"role": "staff"}),
+            content_type="application/json",
+            HTTP_ACCEPT="application/json",
+        )
+        self.assert_enrolled()
+        # Now add with instructor permission. Verify still enrolled.
+        resp = self.client.post(
+            self.detail_url,
+            data=json.dumps({"role": "instructor"}),
+            content_type="application/json",
+            HTTP_ACCEPT="application/json",
+        )
+        self.assert2XX(resp.status_code)
+        self.assert_enrolled()
+
+    def assert_not_enrolled(self):
+        """ Asserts that self.ext_user is not enrolled in self.course. """
+        self.assertEqual(
+            0, get_enrollment_count(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.assertEqual(
+            1, get_enrollment_count(self.ext_user, self.course.location.course_id),
+            'User ext_user should have been enrolled in the course'
+        )
diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py
index a3f211a703..88ff08c3bd 100644
--- a/cms/djangoapps/contentstore/tests/utils.py
+++ b/cms/djangoapps/contentstore/tests/utils.py
@@ -5,6 +5,7 @@ Utilities for contentstore tests
 import json
 
 from student.models import Registration
+from student.models import CourseEnrollment
 from django.contrib.auth.models import User
 from django.test.client import Client
 
@@ -27,6 +28,15 @@ def registration(email):
     return Registration.objects.get(user__email=email)
 
 
+def get_enrollment_count(user, course_id):
+    """
+    Gets the number of enrolled registrants for given course and user=self.user.
+
+    This should always be 0 or 1.
+    """
+    return CourseEnrollment.objects.filter(user=user, course_id=course_id).count()
+
+
 class CourseTestCase(ModuleStoreTestCase):
     def setUp(self):
         """
diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py
index e68210dea4..b35f9bcd56 100644
--- a/cms/djangoapps/contentstore/views/course.py
+++ b/cms/djangoapps/contentstore/views/course.py
@@ -44,6 +44,8 @@ from .component import (
 
 from django_comment_common.utils import seed_permissions_roles
 
+from student.models import CourseEnrollment
+
 from xmodule.html_module import AboutDescriptor
 __all__ = ['course_index', 'create_new_course', 'course_info',
            'course_info_updates', 'get_course_settings',
@@ -162,6 +164,9 @@ def create_new_course(request):
     # seed the forums
     seed_permissions_roles(new_course.location.course_id)
 
+    # auto-enroll the course creator in the course so that "View Live" will work.
+    CourseEnrollment.objects.get_or_create(user=request.user, course_id=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 e1c75bad0f..22376b36db 100644
--- a/cms/djangoapps/contentstore/views/user.py
+++ b/cms/djangoapps/contentstore/views/user.py
@@ -23,6 +23,8 @@ from course_creators.views import (
 
 from .access import has_access
 
+from student.models import CourseEnrollment
+
 
 @login_required
 @ensure_csrf_cookie
@@ -201,6 +203,8 @@ def course_team_user(request, org, course, name, email):
             return JsonResponse(msg, 400)
         user.groups.add(groups["instructor"])
         user.save()
+        # auto-enroll the course creator in the course so that "View Live" will work.
+        CourseEnrollment.objects.get_or_create(user=user, course_id=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.
@@ -214,6 +218,9 @@ def course_team_user(request, org, course, name, email):
             user.groups.remove(groups["instructor"])
         user.groups.add(groups["staff"])
         user.save()
+        # auto-enroll the course creator in the course so that "View Live" will work.
+        CourseEnrollment.objects.get_or_create(user=user, course_id=location.course_id)
+
     return JsonResponse()
 
 

From 3732d418c9d25a246fd4f912559a041c37036639 Mon Sep 17 00:00:00 2001
From: cahrens <christina@edx.org>
Date: Mon, 5 Aug 2013 16:15:18 -0400
Subject: [PATCH 2/2] Add helper methods to hide internals of how users are
 enrolled in courses.

Remove _

Minor cleanup.
---
 .../contentstore/tests/test_contentstore.py   | 10 ++++++----
 .../contentstore/tests/test_users.py          | 13 +++++++------
 cms/djangoapps/contentstore/tests/utils.py    | 10 ----------
 cms/djangoapps/contentstore/views/course.py   |  4 ++--
 cms/djangoapps/contentstore/views/user.py     |  6 +++---
 common/djangoapps/student/tests/tests.py      | 13 +++++++++++++
 common/djangoapps/student/views.py            | 19 ++++++++++++++++++-
 7 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py
index 3b2cb7dcc9..e1e6c6dc4a 100644
--- a/cms/djangoapps/contentstore/tests/test_contentstore.py
+++ b/cms/djangoapps/contentstore/tests/test_contentstore.py
@@ -49,7 +49,7 @@ import datetime
 from pytz import UTC
 from uuid import uuid4
 from pymongo import MongoClient
-from .utils import get_enrollment_count
+from student.views import is_enrolled_in_course
 
 TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
 TEST_DATA_CONTENTSTORE['OPTIONS']['db'] = 'test_xcontent_%s' % uuid4().hex
@@ -1042,7 +1042,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         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.assertEqual(1, get_enrollment_count(self.user, self._get_course_id(test_course_data)))
+        self.assertTrue(is_enrolled_in_course(self.user, self._get_course_id(test_course_data)))
         return test_course_data
 
     def test_create_course_check_forum_seeding(self):
@@ -1064,12 +1064,14 @@ class ContentStoreTest(ModuleStoreTestCase):
         Checks that the course did not get created
         """
         course_id = self._get_course_id(self.course_data)
-        initially_enrolled_count = get_enrollment_count(self.user, course_id)
+        initially_enrolled = is_enrolled_in_course(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)
-        self.assertEqual(initially_enrolled_count, get_enrollment_count(self.user, course_id))
+        # 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))
 
     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 54981b3c49..a9216da612 100644
--- a/cms/djangoapps/contentstore/tests/test_users.py
+++ b/cms/djangoapps/contentstore/tests/test_users.py
@@ -2,10 +2,11 @@
 Tests for contentstore/views/user.py.
 """
 import json
-from .utils import CourseTestCase, get_enrollment_count
+from .utils import CourseTestCase
 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
 
 
 class UsersTestCase(CourseTestCase):
@@ -332,7 +333,7 @@ class UsersTestCase(CourseTestCase):
         # Verify that ext_user is not enrolled in the new course before being added as a staff member.
         self.assert_not_enrolled()
 
-    def test_remove_staff_does_not_enroll(self):
+    def test_remove_staff_does_not_unenroll(self):
         # Add user with staff permissions.
         self.client.post(
             self.detail_url,
@@ -370,14 +371,14 @@ class UsersTestCase(CourseTestCase):
 
     def assert_not_enrolled(self):
         """ Asserts that self.ext_user is not enrolled in self.course. """
-        self.assertEqual(
-            0, get_enrollment_count(self.ext_user, self.course.location.course_id),
+        self.assertFalse(
+            is_enrolled_in_course(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.assertEqual(
-            1, get_enrollment_count(self.ext_user, self.course.location.course_id),
+        self.assertTrue(
+            is_enrolled_in_course(self.ext_user, self.course.location.course_id),
             'User ext_user should have been enrolled in the course'
         )
diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py
index 88ff08c3bd..a3f211a703 100644
--- a/cms/djangoapps/contentstore/tests/utils.py
+++ b/cms/djangoapps/contentstore/tests/utils.py
@@ -5,7 +5,6 @@ Utilities for contentstore tests
 import json
 
 from student.models import Registration
-from student.models import CourseEnrollment
 from django.contrib.auth.models import User
 from django.test.client import Client
 
@@ -28,15 +27,6 @@ def registration(email):
     return Registration.objects.get(user__email=email)
 
 
-def get_enrollment_count(user, course_id):
-    """
-    Gets the number of enrolled registrants for given course and user=self.user.
-
-    This should always be 0 or 1.
-    """
-    return CourseEnrollment.objects.filter(user=user, course_id=course_id).count()
-
-
 class CourseTestCase(ModuleStoreTestCase):
     def setUp(self):
         """
diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py
index b35f9bcd56..8ac1d223cb 100644
--- a/cms/djangoapps/contentstore/views/course.py
+++ b/cms/djangoapps/contentstore/views/course.py
@@ -44,7 +44,7 @@ from .component import (
 
 from django_comment_common.utils import seed_permissions_roles
 
-from student.models import CourseEnrollment
+from student.views import enroll_in_course
 
 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.
-    CourseEnrollment.objects.get_or_create(user=request.user, course_id=new_course.location.course_id)
+    enroll_in_course(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 22376b36db..7233770b02 100644
--- a/cms/djangoapps/contentstore/views/user.py
+++ b/cms/djangoapps/contentstore/views/user.py
@@ -23,7 +23,7 @@ from course_creators.views import (
 
 from .access import has_access
 
-from student.models import CourseEnrollment
+from student.views import enroll_in_course
 
 
 @login_required
@@ -204,7 +204,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.
-        CourseEnrollment.objects.get_or_create(user=user, course_id=location.course_id)
+        enroll_in_course(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.
@@ -219,7 +219,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.
-        CourseEnrollment.objects.get_or_create(user=user, course_id=location.course_id)
+        enroll_in_course(user, location.course_id)
 
     return JsonResponse()
 
diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py
index 3e36016316..513216ba17 100644
--- a/common/djangoapps/student/tests/tests.py
+++ b/common/djangoapps/student/tests/tests.py
@@ -23,6 +23,7 @@ from textwrap import dedent
 
 from student.models import unique_id_for_user
 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'
@@ -205,3 +206,15 @@ class CourseEndingTest(TestCase):
                           'show_survey_button': False,
                           'grade': '67'
                           })
+
+
+class EnrollInCourseTest(TestCase):
+    """ Tests the helper method for enrolling a user in a class """
+
+    def test_enroll_in_course(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))
diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py
index 223c124e46..bc6afdce0b 100644
--- a/common/djangoapps/student/views.py
+++ b/common/djangoapps/student/views.py
@@ -378,7 +378,7 @@ def change_enrollment(request):
                                "run:{0}".format(run)])
 
         try:
-            enrollment, _created = CourseEnrollment.objects.get_or_create(user=user, course_id=course.id)
+            enroll_in_course(user, course.id)
         except IntegrityError:
             # If we've already created this enrollment in a separate transaction,
             # then just continue
@@ -403,6 +403,23 @@ def change_enrollment(request):
         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=""):