From 34526ddf6a0be6585715c058025b86ea93e8a57e Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 11 Aug 2015 13:03:58 -0400 Subject: [PATCH 1/4] Revert "Revert "Merge pull request #8986 from jazkarta/remove-ccx-enrollment"" This reverts commit 42e78463a739d3f0ca605dc3cc06e278f336758a. --- common/djangoapps/student/models.py | 6 - common/djangoapps/student/views.py | 18 - .../migrations/0002_convert_memberships.py | 98 +++ lms/djangoapps/ccx/models.py | 47 -- lms/djangoapps/ccx/tests/factories.py | 11 - .../tests/test_field_override_performance.py | 21 +- lms/djangoapps/ccx/tests/test_models.py | 115 ---- lms/djangoapps/ccx/tests/test_utils.py | 562 +----------------- lms/djangoapps/ccx/tests/test_views.py | 61 +- lms/djangoapps/ccx/utils.py | 286 +-------- lms/djangoapps/ccx/views.py | 48 +- lms/templates/ccx/_dashboard_ccx_listing.html | 80 --- lms/templates/ccx/enrollment.html | 4 +- lms/templates/dashboard.html | 8 - .../content/course_overviews/models.py | 19 +- 15 files changed, 210 insertions(+), 1174 deletions(-) create mode 100644 lms/djangoapps/ccx/migrations/0002_convert_memberships.py delete mode 100644 lms/templates/ccx/_dashboard_ccx_listing.html diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index b7f00fbc8b..3cb3f3ab9a 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1200,12 +1200,6 @@ class CourseEnrollment(models.Model): if not user.is_authenticated(): return False - # unwrap CCXLocators so that we use the course as the access control - # source - from ccx_keys.locator import CCXLocator - if isinstance(course_key, CCXLocator): - course_key = course_key.to_course_locator() - try: record = CourseEnrollment.objects.get(user=user, course_id=course_key) return record.is_active diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index eb8d00526c..4a02405154 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -655,13 +655,6 @@ def dashboard(request): ) courses_requirements_not_met = get_pre_requisite_courses_not_completed(user, courses_having_prerequisites) - ccx_membership_triplets = [] - if settings.FEATURES.get('CUSTOM_COURSES_EDX', False): - from ccx.utils import get_ccx_membership_triplets - ccx_membership_triplets = get_ccx_membership_triplets( - user, course_org_filter, org_filter_out_set - ) - if 'notlive' in request.GET: redirect_message = _("The course you are looking for does not start until {date}.").format( date=request.GET['notlive'] @@ -697,7 +690,6 @@ def dashboard(request): 'provider_states': [], 'order_history_list': order_history_list, 'courses_requirements_not_met': courses_requirements_not_met, - 'ccx_membership_triplets': ccx_membership_triplets, 'nav_hidden': True, } @@ -1904,16 +1896,6 @@ def activate_account(request, key): manual_enrollment_audit.reason, enrollment ) - # enroll student in any pending CCXs he/she may have if auto_enroll flag is set - if settings.FEATURES.get('CUSTOM_COURSES_EDX'): - from ccx.models import CcxMembership, CcxFutureMembership - ccxfms = CcxFutureMembership.objects.filter( - email=student[0].email - ) - for ccxfm in ccxfms: - if ccxfm.auto_enroll: - CcxMembership.auto_enroll(student[0], ccxfm) - resp = render_to_response( "registration/activation_complete.html", { diff --git a/lms/djangoapps/ccx/migrations/0002_convert_memberships.py b/lms/djangoapps/ccx/migrations/0002_convert_memberships.py new file mode 100644 index 0000000000..3030c463b0 --- /dev/null +++ b/lms/djangoapps/ccx/migrations/0002_convert_memberships.py @@ -0,0 +1,98 @@ +# -*- coding: utf-8 -*- +from south.utils import datetime_utils as datetime +from south.db import db +from south.v2 import DataMigration +from django.db import models + +class Migration(DataMigration): + + def forwards(self, orm): + "Convert CCX Memberships to Course Enrollments." + from ccx_keys.locator import CCXLocator + memberships = orm['ccx.CcxMembership'].objects.select_related('ccx', 'student').all() + for membership in memberships: + ccx = membership.ccx + course_key = CCXLocator.from_course_locator(ccx.course_id, ccx.id) + enrollment, created = orm['student.CourseEnrollment'].objects.get_or_create( + user=membership.student, + course_id=course_key, + ) + + + def backwards(self, orm): + """In the future, here we will convert back CCX Course Enrollments to CCX + Memberships. + """ + + 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'}) + }, + 'ccx.ccxfieldoverride': { + 'Meta': {'unique_together': "(('ccx', 'location', 'field'),)", 'object_name': 'CcxFieldOverride'}, + 'ccx': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['ccx.CustomCourseForEdX']"}), + 'field': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'location': ('xmodule_django.models.LocationKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'value': ('django.db.models.fields.TextField', [], {'default': "'null'"}) + }, + 'ccx.customcourseforedx': { + 'Meta': {'object_name': 'CustomCourseForEdX'}, + 'coach': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}), + 'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'display_name': ('django.db.models.fields.CharField', [], {'max_length': '255'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'ccx.ccxmembership': { + 'Meta': {'object_name': 'CcxMembership'}, + 'active': ('django.db.models.fields.BooleanField', [], {'default': False}), + 'ccx': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['ccx.CustomCourseForEdX']"}), + 'student': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'student.courseenrollment': { + 'Meta': {'object_name': 'CourseEnrollment'}, + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}), + 'course_id': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': True}), + 'mode': ('django.db.models.fields.CharField', [], {'max_length': '100', 'default': '"honor"'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + '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'}) + } + } + + complete_apps = ['ccx', 'ccx'] + symmetrical = True diff --git a/lms/djangoapps/ccx/models.py b/lms/djangoapps/ccx/models.py index 9639923946..439962b0e6 100644 --- a/lms/djangoapps/ccx/models.py +++ b/lms/djangoapps/ccx/models.py @@ -9,7 +9,6 @@ from django.db import models from django.utils.timezone import UTC from lazy import lazy -from student.models import CourseEnrollment, AlreadyEnrolledError # pylint: disable=import-error from xmodule_django.models import CourseKeyField, LocationKeyField # pylint: disable=import-error from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore @@ -96,52 +95,6 @@ class CustomCourseForEdX(models.Model): return value -class CcxMembership(models.Model): - """ - Which students are in a CCX? - """ - ccx = models.ForeignKey(CustomCourseForEdX, db_index=True) - student = models.ForeignKey(User, db_index=True) - active = models.BooleanField(default=False) - - @classmethod - def auto_enroll(cls, student, future_membership): - """convert future_membership to an active membership - """ - if not future_membership.auto_enroll: - msg = "auto enrollment not allowed for {}" - raise ValueError(msg.format(future_membership)) - membership = cls( - ccx=future_membership.ccx, student=student, active=True - ) - try: - CourseEnrollment.enroll( - student, future_membership.ccx.course_id, check_access=True - ) - except AlreadyEnrolledError: - # if the user is already enrolled in the course, great! - pass - - membership.save() - future_membership.delete() - - @classmethod - def memberships_for_user(cls, user, active=True): - """ - active memberships for a user - """ - return cls.objects.filter(student=user, active__exact=active) - - -class CcxFutureMembership(models.Model): - """ - Which emails for non-users are waiting to be added to CCX on registration - """ - ccx = models.ForeignKey(CustomCourseForEdX, db_index=True) - email = models.CharField(max_length=255) - auto_enroll = models.BooleanField(default=0) - - class CcxFieldOverride(models.Model): """ Field overrides for custom courses. diff --git a/lms/djangoapps/ccx/tests/factories.py b/lms/djangoapps/ccx/tests/factories.py index 507df6b44a..b2a99215c1 100644 --- a/lms/djangoapps/ccx/tests/factories.py +++ b/lms/djangoapps/ccx/tests/factories.py @@ -5,8 +5,6 @@ from factory import SubFactory from factory.django import DjangoModelFactory from student.tests.factories import UserFactory from ccx.models import CustomCourseForEdX # pylint: disable=import-error -from ccx.models import CcxMembership # pylint: disable=import-error -from ccx.models import CcxFutureMembership # pylint: disable=import-error class CcxFactory(DjangoModelFactory): # pylint: disable=missing-docstring @@ -14,12 +12,3 @@ class CcxFactory(DjangoModelFactory): # pylint: disable=missing-docstring display_name = "Test CCX" id = None # pylint: disable=redefined-builtin, invalid-name coach = SubFactory(UserFactory) - - -class CcxMembershipFactory(DjangoModelFactory): # pylint: disable=missing-docstring - FACTORY_FOR = CcxMembership - active = False - - -class CcxFutureMembershipFactory(DjangoModelFactory): # pylint: disable=missing-docstring - FACTORY_FOR = CcxFutureMembership diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 77247650fb..77f9a061ef 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -27,7 +27,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, \ from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, check_sum_of_calls from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin from ccx_keys.locator import CCXLocator -from ccx.tests.factories import CcxFactory, CcxMembershipFactory +from ccx.tests.factories import CcxFactory @attr('shard_1') @@ -65,7 +65,7 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, MakoMiddleware().process_request(self.request) - def setup_course(self, size, enable_ccx): + def setup_course(self, size, enable_ccx, view_as_ccx): """ Build a gradable course where each node has `size` children. """ @@ -112,18 +112,17 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, ) self.populate_course(size) + course_key = self.course.id + if enable_ccx: + self.ccx = CcxFactory.create(course_id=self.course.id) + if view_as_ccx: + course_key = CCXLocator.from_course_locator(self.course.id, self.ccx.id) + CourseEnrollment.enroll( self.student, - self.course.id + course_key ) - if enable_ccx: - self.ccx = CcxFactory.create() - CcxMembershipFactory.create( - student=self.student, - ccx=self.ccx - ) - def grade_course(self, course, view_as_ccx): """ Renders the progress page for the given course. @@ -156,7 +155,7 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, """ Renders the progress page, instrumenting Mongo reads and SQL queries. """ - self.setup_course(course_width, enable_ccx) + self.setup_course(course_width, enable_ccx, view_as_ccx) # Switch to published-only mode to simulate the LMS with self.settings(MODULESTORE_BRANCH='published-only'): diff --git a/lms/djangoapps/ccx/tests/test_models.py b/lms/djangoapps/ccx/tests/test_models.py index 4162a8811e..fd60a53725 100644 --- a/lms/djangoapps/ccx/tests/test_models.py +++ b/lms/djangoapps/ccx/tests/test_models.py @@ -5,12 +5,9 @@ from datetime import datetime, timedelta from django.utils.timezone import UTC from mock import patch from nose.plugins.attrib import attr -from student.models import CourseEnrollment # pylint: disable=import-error from student.roles import CourseCcxCoachRole # pylint: disable=import-error from student.tests.factories import ( # pylint: disable=import-error AdminFactory, - CourseEnrollmentFactory, - UserFactory, ) from util.tests.test_date_utils import fake_ugettext from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -21,122 +18,10 @@ from xmodule.modulestore.tests.factories import ( from .factories import ( CcxFactory, - CcxFutureMembershipFactory, -) -from ..models import ( - CcxMembership, - CcxFutureMembership, ) from ..overrides import override_field_for_ccx -@attr('shard_1') -class TestCcxMembership(ModuleStoreTestCase): - """Unit tests for the CcxMembership model - """ - - def setUp(self): - """common setup for all tests""" - super(TestCcxMembership, self).setUp() - self.course = course = CourseFactory.create() - coach = AdminFactory.create() - role = CourseCcxCoachRole(course.id) - role.add_users(coach) - self.ccx = CcxFactory(course_id=course.id, coach=coach) - enrollment = CourseEnrollmentFactory.create(course_id=course.id) - self.enrolled_user = enrollment.user - self.unenrolled_user = UserFactory.create() - - def create_future_enrollment(self, user, auto_enroll=True): - """ - utility method to create future enrollment - """ - pfm = CcxFutureMembershipFactory.create( - ccx=self.ccx, - email=user.email, - auto_enroll=auto_enroll - ) - return pfm - - def has_course_enrollment(self, user): - """ - utility method to create future enrollment - """ - enrollment = CourseEnrollment.objects.filter( - user=user, course_id=self.course.id - ) - return enrollment.exists() - - def has_ccx_membership(self, user): - """ - verify ccx membership - """ - membership = CcxMembership.objects.filter( - student=user, ccx=self.ccx, active=True - ) - return membership.exists() - - def has_ccx_future_membership(self, user): - """ - verify future ccx membership - """ - future_membership = CcxFutureMembership.objects.filter( - email=user.email, ccx=self.ccx - ) - return future_membership.exists() - - def call_mut(self, student, future_membership): - """ - Call the method undser test - """ - CcxMembership.auto_enroll(student, future_membership) - - def test_ccx_auto_enroll_unregistered_user(self): - """verify auto_enroll works when user is not enrolled in the MOOC - - n.b. After auto_enroll, user will have both a MOOC enrollment and a - CCX membership - """ - user = self.unenrolled_user - pfm = self.create_future_enrollment(user) - self.assertTrue(self.has_ccx_future_membership(user)) - self.assertFalse(self.has_course_enrollment(user)) - # auto_enroll user - self.call_mut(user, pfm) - - self.assertTrue(self.has_course_enrollment(user)) - self.assertTrue(self.has_ccx_membership(user)) - self.assertFalse(self.has_ccx_future_membership(user)) - - def test_ccx_auto_enroll_registered_user(self): - """verify auto_enroll works when user is enrolled in the MOOC - """ - user = self.enrolled_user - pfm = self.create_future_enrollment(user) - self.assertTrue(self.has_ccx_future_membership(user)) - self.assertTrue(self.has_course_enrollment(user)) - - self.call_mut(user, pfm) - - self.assertTrue(self.has_course_enrollment(user)) - self.assertTrue(self.has_ccx_membership(user)) - self.assertFalse(self.has_ccx_future_membership(user)) - - def test_future_membership_disallows_auto_enroll(self): - """verify that the CcxFutureMembership can veto auto_enroll - """ - user = self.unenrolled_user - pfm = self.create_future_enrollment(user, auto_enroll=False) - self.assertTrue(self.has_ccx_future_membership(user)) - self.assertFalse(self.has_course_enrollment(user)) - - self.assertRaises(ValueError, self.call_mut, user, pfm) - - self.assertFalse(self.has_course_enrollment(user)) - self.assertFalse(self.has_ccx_membership(user)) - self.assertTrue(self.has_ccx_future_membership(user)) - - @attr('shard_1') class TestCCX(ModuleStoreTestCase): """Unit tests for the CustomCourseForEdX model diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index 7fa024c924..01c0bc7cc0 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -3,575 +3,49 @@ test utils """ from nose.plugins.attrib import attr -from ccx.models import ( # pylint: disable=import-error - CcxMembership, - CcxFutureMembership, -) from ccx.tests.factories import ( # pylint: disable=import-error CcxFactory, - CcxMembershipFactory, - CcxFutureMembershipFactory, ) from student.roles import CourseCcxCoachRole # pylint: disable=import-error from student.tests.factories import ( # pylint: disable=import-error AdminFactory, - UserFactory, ) from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE) from xmodule.modulestore.tests.factories import CourseFactory + from ccx_keys.locator import CCXLocator @attr('shard_1') -class TestEmailEnrollmentState(ModuleStoreTestCase): - """unit tests for the EmailEnrollmentState class - """ - - def setUp(self): - """ - Set up tests - """ - super(TestEmailEnrollmentState, self).setUp() - # remove user provided by the parent test case so we can make our own - # when needed. - self.user = None - course = CourseFactory.create() - coach = AdminFactory.create() - role = CourseCcxCoachRole(course.id) - role.add_users(coach) - self.ccx = CcxFactory(course_id=course.id, coach=coach) - - def create_user(self): - """provide a legitimate django user for testing - """ - if getattr(self, 'user', None) is None: - self.user = UserFactory() - - def register_user_in_ccx(self): - """create registration of self.user in self.ccx - - registration will be inactive - """ - self.create_user() - CcxMembershipFactory(ccx=self.ccx, student=self.user) - - def create_one(self, email=None): - """Create a single EmailEnrollmentState object and return it - """ - from ccx.utils import EmailEnrollmentState # pylint: disable=import-error - if email is None: - email = self.user.email - return EmailEnrollmentState(self.ccx, email) - - def test_enrollment_state_for_non_user(self): - """verify behavior for non-user email address - """ - ee_state = self.create_one(email='nobody@nowhere.com') - for attribute in ['user', 'member', 'full_name', 'in_ccx']: - value = getattr(ee_state, attribute, 'missing attribute') - self.assertFalse(value, "{}: {}".format(value, attribute)) - - def test_enrollment_state_for_non_member_user(self): - """verify behavior for email address of user who is not a ccx memeber - """ - self.create_user() - ee_state = self.create_one() - self.assertTrue(ee_state.user) - self.assertFalse(ee_state.in_ccx) - self.assertEqual(ee_state.member, self.user) - self.assertEqual(ee_state.full_name, self.user.profile.name) - - def test_enrollment_state_for_member_user(self): - """verify behavior for email address of user who is a ccx member - """ - self.create_user() - self.register_user_in_ccx() - ee_state = self.create_one() - for attribute in ['user', 'in_ccx']: - self.assertTrue( - getattr(ee_state, attribute, False), - "attribute {} is missing or False".format(attribute) - ) - self.assertEqual(ee_state.member, self.user) - self.assertEqual(ee_state.full_name, self.user.profile.name) - - def test_enrollment_state_to_dict(self): - """verify dict representation of EmailEnrollmentState - """ - self.create_user() - self.register_user_in_ccx() - ee_state = self.create_one() - ee_dict = ee_state.to_dict() - expected = { - 'user': True, - 'member': self.user, - 'in_ccx': True, - } - for expected_key, expected_value in expected.iteritems(): - self.assertTrue(expected_key in ee_dict) - self.assertEqual(expected_value, ee_dict[expected_key]) - - def test_enrollment_state_repr(self): - self.create_user() - self.register_user_in_ccx() - ee_state = self.create_one() - representation = repr(ee_state) - self.assertTrue('user=True' in representation) - self.assertTrue('in_ccx=True' in representation) - member = 'member={}'.format(self.user) - self.assertTrue(member in representation) - - -@attr('shard_1') -# TODO: deal with changes in behavior for auto_enroll -class TestGetEmailParams(ModuleStoreTestCase): - """tests for ccx.utils.get_email_params - """ - MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - - def setUp(self): - """ - Set up tests - """ - super(TestGetEmailParams, self).setUp() - course = CourseFactory.create() - coach = AdminFactory.create() - role = CourseCcxCoachRole(course.id) - role.add_users(coach) - self.ccx = CcxFactory(course_id=course.id, coach=coach) - self.all_keys = [ - 'site_name', 'course', 'course_url', 'registration_url', - 'course_about_url', 'auto_enroll' - ] - self.url_keys = [k for k in self.all_keys if 'url' in k] - self.course_keys = [k for k in self.url_keys if 'course' in k] - - def call_fut(self, auto_enroll=False, secure=False): - """ - call function under test - """ - from ccx.utils import get_email_params # pylint: disable=import-error - return get_email_params(self.ccx, auto_enroll, secure) - - def test_params_have_expected_keys(self): - params = self.call_fut() - self.assertFalse(set(params.keys()) - set(self.all_keys)) - - def test_ccx_id_in_params(self): - expected_course_id = unicode(CCXLocator.from_course_locator(self.ccx.course_id, self.ccx.id)) - params = self.call_fut() - self.assertEqual(params['course'], self.ccx) - for url_key in self.url_keys: - self.assertTrue('http://' in params[url_key]) - for url_key in self.course_keys: - self.assertTrue(expected_course_id in params[url_key]) - - def test_security_respected(self): - secure = self.call_fut(secure=True) - for url_key in self.url_keys: - self.assertTrue('https://' in secure[url_key]) - insecure = self.call_fut(secure=False) - for url_key in self.url_keys: - self.assertTrue('http://' in insecure[url_key]) - - def test_auto_enroll_passed_correctly(self): - not_auto = self.call_fut(auto_enroll=False) - self.assertFalse(not_auto['auto_enroll']) - auto = self.call_fut(auto_enroll=True) - self.assertTrue(auto['auto_enroll']) - - -@attr('shard_1') -# TODO: deal with changes in behavior for auto_enroll -class TestEnrollEmail(ModuleStoreTestCase): - """tests for the enroll_email function from ccx.utils - """ - MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - - def setUp(self): - super(TestEnrollEmail, self).setUp() - # unbind the user created by the parent, so we can create our own when - # needed. - self.user = None - course = CourseFactory.create() - coach = AdminFactory.create() - role = CourseCcxCoachRole(course.id) - role.add_users(coach) - self.ccx = CcxFactory(course_id=course.id, coach=coach) - self.outbox = self.get_outbox() - - def create_user(self): - """provide a legitimate django user for testing - """ - if getattr(self, 'user', None) is None: - self.user = UserFactory() - - def register_user_in_ccx(self): - """create registration of self.user in self.ccx - - registration will be inactive - """ - self.create_user() - CcxMembershipFactory(ccx=self.ccx, student=self.user) - - def get_outbox(self): - """Return the django mail outbox""" - from django.core import mail - return mail.outbox - - def check_membership(self, email=None, user=None, future=False): - """Verify tjat an appropriate CCX Membership exists""" - if not email and not user: - self.fail( - "must provide user or email address to check CCX Membership" - ) - if future and email: - membership = CcxFutureMembership.objects.filter( - ccx=self.ccx, email=email - ) - elif not future: - if not user: - user = self.user - membership = CcxMembership.objects.filter( - ccx=self.ccx, student=user - ) - self.assertTrue(membership.exists()) - - def check_enrollment_state(self, state, in_ccx, member, user): - """Verify an enrollment state object against provided arguments - - state.in_ccx will always be a boolean - state.user will always be a boolean - state.member will be a Django user object or None - """ - self.assertEqual(in_ccx, state.in_ccx) - self.assertEqual(member, state.member) - self.assertEqual(user, state.user) - - def call_fut( - self, - student_email=None, - auto_enroll=False, - email_students=False, - email_params=None - ): - """Call function under test""" - from ccx.utils import enroll_email # pylint: disable=import-error - if student_email is None: - student_email = self.user.email - before, after = enroll_email( - self.ccx, student_email, auto_enroll, email_students, email_params - ) - return before, after - - def test_enroll_non_user_sending_email(self): - """enroll a non-user email and send an enrollment email to them - """ - # ensure no emails are in the outbox now - self.assertEqual(self.outbox, []) - test_email = "nobody@nowhere.com" - before, after = self.call_fut( - student_email=test_email, email_students=True - ) - - # there should be a future membership set for this email address now - self.check_membership(email=test_email, future=True) - for state in [before, after]: - self.check_enrollment_state(state, False, None, False) - # mail was sent and to the right person - self.assertEqual(len(self.outbox), 1) - msg = self.outbox[0] - self.assertTrue(test_email in msg.recipients()) - - def test_enroll_non_member_sending_email(self): - """register a non-member and send an enrollment email to them - """ - self.create_user() - # ensure no emails are in the outbox now - self.assertEqual(self.outbox, []) - before, after = self.call_fut(email_students=True) - - # there should be a membership set for this email address now - self.check_membership(email=self.user.email) - self.check_enrollment_state(before, False, self.user, True) - self.check_enrollment_state(after, True, self.user, True) - # mail was sent and to the right person - self.assertEqual(len(self.outbox), 1) - msg = self.outbox[0] - self.assertTrue(self.user.email in msg.recipients()) - - def test_enroll_member_sending_email(self): - """register a member and send an enrollment email to them - """ - self.register_user_in_ccx() - # ensure no emails are in the outbox now - self.assertEqual(self.outbox, []) - before, after = self.call_fut(email_students=True) - - # there should be a membership set for this email address now - self.check_membership(email=self.user.email) - for state in [before, after]: - self.check_enrollment_state(state, True, self.user, True) - # mail was sent and to the right person - self.assertEqual(len(self.outbox), 1) - msg = self.outbox[0] - self.assertTrue(self.user.email in msg.recipients()) - - def test_enroll_non_user_no_email(self): - """register a non-user via email address but send no email - """ - # ensure no emails are in the outbox now - self.assertEqual(self.outbox, []) - test_email = "nobody@nowhere.com" - before, after = self.call_fut( - student_email=test_email, email_students=False - ) - - # there should be a future membership set for this email address now - self.check_membership(email=test_email, future=True) - for state in [before, after]: - self.check_enrollment_state(state, False, None, False) - # ensure there are still no emails in the outbox now - self.assertEqual(self.outbox, []) - - def test_enroll_non_member_no_email(self): - """register a non-member but send no email""" - self.create_user() - # ensure no emails are in the outbox now - self.assertEqual(self.outbox, []) - before, after = self.call_fut(email_students=False) - - # there should be a membership set for this email address now - self.check_membership(email=self.user.email) - self.check_enrollment_state(before, False, self.user, True) - self.check_enrollment_state(after, True, self.user, True) - # ensure there are still no emails in the outbox now - self.assertEqual(self.outbox, []) - - def test_enroll_member_no_email(self): - """enroll a member but send no email - """ - self.register_user_in_ccx() - # ensure no emails are in the outbox now - self.assertEqual(self.outbox, []) - before, after = self.call_fut(email_students=False) - - # there should be a membership set for this email address now - self.check_membership(email=self.user.email) - for state in [before, after]: - self.check_enrollment_state(state, True, self.user, True) - # ensure there are still no emails in the outbox now - self.assertEqual(self.outbox, []) - - -@attr('shard_1') -# TODO: deal with changes in behavior for auto_enroll -class TestUnenrollEmail(ModuleStoreTestCase): - """Tests for the unenroll_email function from ccx.utils""" - MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - - def setUp(self): - super(TestUnenrollEmail, self).setUp() - # unbind the user created by the parent, so we can create our own when - # needed. - self.user = None - course = CourseFactory.create() - coach = AdminFactory.create() - role = CourseCcxCoachRole(course.id) - role.add_users(coach) - self.ccx = CcxFactory(course_id=course.id, coach=coach) - self.outbox = self.get_outbox() - self.email = "nobody@nowhere.com" - - def get_outbox(self): - """Return the django mail outbox""" - from django.core import mail - return mail.outbox - - def create_user(self): - """provide a legitimate django user for testing - """ - if getattr(self, 'user', None) is None: - self.user = UserFactory() - - def make_ccx_membership(self): - """create registration of self.user in self.ccx - - registration will be inactive - """ - self.create_user() - CcxMembershipFactory.create(ccx=self.ccx, student=self.user) - - def make_ccx_future_membership(self): - """create future registration for email in self.ccx""" - CcxFutureMembershipFactory.create( - ccx=self.ccx, email=self.email - ) - - def check_enrollment_state(self, state, in_ccx, member, user): - """Verify an enrollment state object against provided arguments - - state.in_ccx will always be a boolean - state.user will always be a boolean - state.member will be a Django user object or None - """ - self.assertEqual(in_ccx, state.in_ccx) - self.assertEqual(member, state.member) - self.assertEqual(user, state.user) - - def check_membership(self, future=False): - """ - check membership - """ - if future: - membership = CcxFutureMembership.objects.filter( - ccx=self.ccx, email=self.email - ) - else: - membership = CcxMembership.objects.filter( - ccx=self.ccx, student=self.user - ) - return membership.exists() - - def call_fut(self, email_students=False): - """call function under test""" - from ccx.utils import unenroll_email # pylint: disable=import-error - email = getattr(self, 'user', None) and self.user.email or self.email - return unenroll_email(self.ccx, email, email_students=email_students) - - def test_unenroll_future_member_with_email(self): - """unenroll a future member and send an email - """ - self.make_ccx_future_membership() - # assert that a membership exists and that no emails have been sent - self.assertTrue(self.check_membership(future=True)) - self.assertEqual(self.outbox, []) - # unenroll the student - before, after = self.call_fut(email_students=True) - - # assert that membership is now gone - self.assertFalse(self.check_membership(future=True)) - # validate the before and after enrollment states - for state in [before, after]: - self.check_enrollment_state(state, False, None, False) - # check that mail was sent and to the right person - self.assertEqual(len(self.outbox), 1) - msg = self.outbox[0] - self.assertTrue(self.email in msg.recipients()) - - def test_unenroll_member_with_email(self): - """unenroll a current member and send an email""" - self.make_ccx_membership() - # assert that a membership exists and that no emails have been sent - self.assertTrue(self.check_membership()) - self.assertEqual(self.outbox, []) - # unenroll the student - before, after = self.call_fut(email_students=True) - - # assert that membership is now gone - self.assertFalse(self.check_membership()) - # validate the before and after enrollment state - self.check_enrollment_state(after, False, self.user, True) - self.check_enrollment_state(before, True, self.user, True) - # check that mail was sent and to the right person - self.assertEqual(len(self.outbox), 1) - msg = self.outbox[0] - self.assertTrue(self.user.email in msg.recipients()) - - def test_unenroll_future_member_no_email(self): - """unenroll a future member but send no email - """ - self.make_ccx_future_membership() - # assert that a membership exists and that no emails have been sent - self.assertTrue(self.check_membership(future=True)) - self.assertEqual(self.outbox, []) - # unenroll the student - before, after = self.call_fut() - - # assert that membership is now gone - self.assertFalse(self.check_membership(future=True)) - # validate the before and after enrollment states - for state in [before, after]: - self.check_enrollment_state(state, False, None, False) - # no email was sent to the student - self.assertEqual(self.outbox, []) - - def test_unenroll_member_no_email(self): - """unenroll a current member but send no email - """ - self.make_ccx_membership() - # assert that a membership exists and that no emails have been sent - self.assertTrue(self.check_membership()) - self.assertEqual(self.outbox, []) - # unenroll the student - before, after = self.call_fut() - - # assert that membership is now gone - self.assertFalse(self.check_membership()) - # validate the before and after enrollment state - self.check_enrollment_state(after, False, self.user, True) - self.check_enrollment_state(before, True, self.user, True) - # no email was sent to the student - self.assertEqual(self.outbox, []) - - -@attr('shard_1') -class TestGetMembershipTriplets(ModuleStoreTestCase): - """Verify that get_ccx_membership_triplets functions properly""" +class TestGetCCXFromCCXLocator(ModuleStoreTestCase): + """Verify that get_ccx_from_ccx_locator functions properly""" MODULESTORE = TEST_DATA_SPLIT_MODULESTORE def setUp(self): """Set up a course, coach, ccx and user""" - super(TestGetMembershipTriplets, self).setUp() + super(TestGetCCXFromCCXLocator, self).setUp() self.course = CourseFactory.create() - coach = AdminFactory.create() + coach = self.coach = AdminFactory.create() role = CourseCcxCoachRole(self.course.id) role.add_users(coach) - self.ccx = CcxFactory(course_id=self.course.id, coach=coach) - def make_ccx_membership(self, active=True): - """create registration of self.user in self.ccx - - registration will be inactive - """ - CcxMembershipFactory.create(ccx=self.ccx, student=self.user, active=active) - - def call_fut(self, org_filter=None, org_filter_out=()): + def call_fut(self, course_id): """call the function under test in this test case""" - from ccx.utils import get_ccx_membership_triplets - return list( - get_ccx_membership_triplets(self.user, org_filter, org_filter_out) - ) + from ccx.utils import get_ccx_from_ccx_locator + return get_ccx_from_ccx_locator(course_id) - def test_no_membership(self): - """verify that no triplets are returned if there are no memberships + def test_non_ccx_locator(self): + """verify that nothing is returned if locator is not a ccx locator """ - triplets = self.call_fut() - self.assertEqual(len(triplets), 0) + result = self.call_fut(self.course.id) + self.assertEqual(result, None) - def test_has_membership(self): - """verify that a triplet is returned when a membership exists + def test_ccx_locator(self): + """verify that the ccx is retuned if using a ccx locator """ - self.make_ccx_membership() - triplets = self.call_fut() - self.assertEqual(len(triplets), 1) - ccx, membership, course_overview = triplets[0] - self.assertEqual(ccx.id, self.ccx.id) - self.assertEqual(unicode(course_overview.id), unicode(self.course.id)) - self.assertEqual(membership.student, self.user) - - def test_has_membership_org_filtered(self): - """verify that microsite org filter prevents seeing microsite ccx""" - self.make_ccx_membership() - bad_org = self.course.location.org + 'foo' - triplets = self.call_fut(org_filter=bad_org) - self.assertEqual(len(triplets), 0) - - def test_has_membership_org_filtered_out(self): - """verify that microsite ccxs not seen in non-microsite view""" - self.make_ccx_membership() - filter_list = [self.course.location.org] - triplets = self.call_fut(org_filter_out=filter_list) - self.assertEqual(len(triplets), 0) + ccx = CcxFactory(course_id=self.course.id, coach=self.coach) + course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) + result = self.call_fut(course_key) + self.assertEqual(result, ccx) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index d9a8182017..919cd1f52a 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -20,9 +20,12 @@ from django.utils.timezone import UTC from django.test.utils import override_settings from django.test import RequestFactory from edxmako.shortcuts import render_to_response # pylint: disable=import-error -from student.models import CourseEnrollment from request_cache.middleware import RequestCache from student.roles import CourseCcxCoachRole # pylint: disable=import-error +from student.models import ( + CourseEnrollment, + CourseEnrollmentAllowed, +) from student.tests.factories import ( # pylint: disable=import-error AdminFactory, CourseEnrollmentFactory, @@ -42,14 +45,10 @@ from ccx_keys.locator import CCXLocator from ..models import ( CustomCourseForEdX, - CcxMembership, - CcxFutureMembership, ) from ..overrides import get_override_for_ccx, override_field_for_ccx from .factories import ( CcxFactory, - CcxMembershipFactory, - CcxFutureMembershipFactory, ) @@ -280,7 +279,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertTrue(student.email in outbox[0].recipients()) # pylint: disable=no-member # a CcxMembership exists for this student self.assertTrue( - CcxMembership.objects.filter(ccx=ccx, student=student).exists() + CourseEnrollment.objects.filter(course_id=self.course.id, user=student).exists() ) def test_unenroll_member_student(self): @@ -288,16 +287,15 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ self.make_coach() ccx = self.make_ccx() - enrollment = CourseEnrollmentFactory(course_id=self.course.id) + course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) + enrollment = CourseEnrollmentFactory(course_id=course_key) student = enrollment.user outbox = self.get_outbox() self.assertEqual(outbox, []) - # student is member of CCX: - CcxMembershipFactory(ccx=ccx, student=student) url = reverse( 'ccx_invite', - kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} + kwargs={'course_id': course_key} ) data = { 'enrollment-button': 'Unenroll', @@ -311,10 +309,6 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertTrue(302 in response.redirect_chain[0]) self.assertEqual(len(outbox), 1) self.assertTrue(student.email in outbox[0].recipients()) # pylint: disable=no-member - # the membership for this student is gone - self.assertFalse( - CcxMembership.objects.filter(ccx=ccx, student=student).exists() - ) def test_enroll_non_user_student(self): """enroll a list of students who are not users yet @@ -322,12 +316,13 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): test_email = "nobody@nowhere.com" self.make_coach() ccx = self.make_ccx() + course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) outbox = self.get_outbox() self.assertEqual(outbox, []) url = reverse( 'ccx_invite', - kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} + kwargs={'course_id': course_key} ) data = { 'enrollment-button': 'Enroll', @@ -342,8 +337,8 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertEqual(len(outbox), 1) self.assertTrue(test_email in outbox[0].recipients()) self.assertTrue( - CcxFutureMembership.objects.filter( - ccx=ccx, email=test_email + CourseEnrollmentAllowed.objects.filter( + course_id=course_key, email=test_email ).exists() ) @@ -352,14 +347,16 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ test_email = "nobody@nowhere.com" self.make_coach() + course = CourseFactory.create() ccx = self.make_ccx() + course_key = CCXLocator.from_course_locator(course.id, ccx.id) outbox = self.get_outbox() - CcxFutureMembershipFactory(ccx=ccx, email=test_email) + CourseEnrollmentAllowed(course_id=course_key, email=test_email) self.assertEqual(outbox, []) url = reverse( 'ccx_invite', - kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} + kwargs={'course_id': course_key} ) data = { 'enrollment-button': 'Unenroll', @@ -371,11 +368,9 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): # we were redirected to our current location self.assertEqual(len(response.redirect_chain), 1) self.assertTrue(302 in response.redirect_chain[0]) - self.assertEqual(len(outbox), 1) - self.assertTrue(test_email in outbox[0].recipients()) self.assertFalse( - CcxFutureMembership.objects.filter( - ccx=ccx, email=test_email + CourseEnrollmentAllowed.objects.filter( + course_id=course_key, email=test_email ).exists() ) @@ -384,7 +379,8 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ self.make_coach() ccx = self.make_ccx() - enrollment = CourseEnrollmentFactory(course_id=self.course.id) + course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) + enrollment = CourseEnrollmentFactory(course_id=course_key) student = enrollment.user # no emails have been sent so far outbox = self.get_outbox() @@ -392,7 +388,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): url = reverse( 'ccx_manage_student', - kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)} + kwargs={'course_id': course_key} ) data = { 'student-action': 'add', @@ -406,7 +402,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertEqual(outbox, []) # a CcxMembership exists for this student self.assertTrue( - CcxMembership.objects.filter(ccx=ccx, student=student).exists() + CourseEnrollment.objects.filter(course_id=course_key, user=student).exists() ) def test_manage_remove_single_student(self): @@ -414,9 +410,9 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): """ self.make_coach() ccx = self.make_ccx() - enrollment = CourseEnrollmentFactory(course_id=self.course.id) + course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) + enrollment = CourseEnrollmentFactory(course_id=course_key) student = enrollment.user - CcxMembershipFactory(ccx=ccx, student=student) # no emails have been sent so far outbox = self.get_outbox() self.assertEqual(outbox, []) @@ -435,10 +431,6 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): self.assertEqual(len(response.redirect_chain), 1) self.assertTrue(302 in response.redirect_chain[0]) self.assertEqual(outbox, []) - # a CcxMembership exists for this student - self.assertFalse( - CcxMembership.objects.filter(ccx=ccx, student=student).exists() - ) GET_CHILDREN = XModuleMixin.get_children @@ -525,7 +517,6 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): self.student = student = UserFactory.create() CourseEnrollmentFactory.create(user=student, course_id=self.course.id) - CcxMembershipFactory(ccx=ccx, student=student, active=True) # create grades for self.student as if they'd submitted the ccx for chapter in self.course.get_children(): @@ -674,12 +665,14 @@ class TestStudentDashboardWithCCX(ModuleStoreTestCase): self.ccx = CcxFactory(course_id=self.split_course.id, coach=self.coach) last_week = datetime.datetime.now(UTC()) - datetime.timedelta(days=7) override_field_for_ccx(self.ccx, self.split_course, 'start', last_week) # Required by self.ccx.has_started(). - CcxMembershipFactory(ccx=self.ccx, student=self.student, active=True) + course_key = CCXLocator.from_course_locator(self.split_course.id, self.ccx.id) + CourseEnrollment.enroll(self.student, course_key) def test_load_student_dashboard(self): self.client.login(username=self.student.username, password=self.student_password) response = self.client.get(reverse('dashboard')) self.assertEqual(response.status_code, 200) + self.assertTrue(re.search('Test CCX', response.content)) def flatten(seq): diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 5b69cd794b..8cbcb8ece1 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -4,283 +4,23 @@ CCX Enrollment operations for use by Coach APIs. Does not include any access control, be sure to check access before calling. """ import logging -from django.contrib.auth.models import User -from django.conf import settings -from django.core.urlresolvers import reverse -from django.core.mail import send_mail -from edxmako.shortcuts import render_to_string # pylint: disable=import-error -from microsite_configuration import microsite # pylint: disable=import-error -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from xmodule.modulestore.django import modulestore -from xmodule.error_module import ErrorDescriptor -from ccx_keys.locator import CCXLocator -from .models import ( - CcxMembership, - CcxFutureMembership, -) +from .models import CustomCourseForEdX log = logging.getLogger("edx.ccx") -class EmailEnrollmentState(object): - """ Store the complete enrollment state of an email in a class """ - def __init__(self, ccx, email): - exists_user = User.objects.filter(email=email).exists() - if exists_user: - user = User.objects.get(email=email) - ccx_member = CcxMembership.objects.filter(ccx=ccx, student=user) - in_ccx = ccx_member.exists() - full_name = user.profile.name - else: - user = None - in_ccx = False - full_name = None - self.user = exists_user - self.member = user - self.full_name = full_name - self.in_ccx = in_ccx - - def __repr__(self): - return "{}(user={}, member={}, in_ccx={})".format( - self.__class__.__name__, - self.user, - self.member, - self.in_ccx, +def get_ccx_from_ccx_locator(course_id): + """ helper function to allow querying ccx fields from templates """ + ccx_id = getattr(course_id, 'ccx', None) + ccx = None + if ccx_id: + ccx = CustomCourseForEdX.objects.filter(id=ccx_id) + if not ccx: + log.warning( + "CCX does not exist for course with id %s", + course_id ) - - def to_dict(self): - """ return dict with membership and ccx info """ - return { - 'user': self.user, - 'member': self.member, - 'in_ccx': self.in_ccx, - } - - -def enroll_email(ccx, student_email, auto_enroll=False, email_students=False, email_params=None): - """ - Send email to newly enrolled student - """ - if email_params is None: - email_params = get_email_params(ccx, True) - previous_state = EmailEnrollmentState(ccx, student_email) - - if previous_state.user: - user = User.objects.get(email=student_email) - if not previous_state.in_ccx: - membership = CcxMembership( - ccx=ccx, student=user, active=True - ) - membership.save() - elif auto_enroll: - # activate existing memberships - membership = CcxMembership.objects.get(student=user, ccx=ccx) - membership.active = True - membership.save() - if email_students: - email_params['message'] = 'enrolled_enroll' - email_params['email_address'] = student_email - email_params['full_name'] = previous_state.full_name - send_mail_to_student(student_email, email_params) - else: - membership = CcxFutureMembership( - ccx=ccx, auto_enroll=auto_enroll, email=student_email - ) - membership.save() - if email_students: - email_params['message'] = 'allowed_enroll' - email_params['email_address'] = student_email - send_mail_to_student(student_email, email_params) - - after_state = EmailEnrollmentState(ccx, student_email) - - return previous_state, after_state - - -def unenroll_email(ccx, student_email, email_students=False, email_params=None): - """ - send email to unenrolled students - """ - if email_params is None: - email_params = get_email_params(ccx, True) - previous_state = EmailEnrollmentState(ccx, student_email) - - if previous_state.in_ccx: - CcxMembership.objects.get( - ccx=ccx, student=previous_state.member - ).delete() - if email_students: - email_params['message'] = 'enrolled_unenroll' - email_params['email_address'] = student_email - email_params['full_name'] = previous_state.full_name - send_mail_to_student(student_email, email_params) - else: - if CcxFutureMembership.objects.filter( - ccx=ccx, email=student_email).exists(): - CcxFutureMembership.objects.get( - ccx=ccx, email=student_email - ).delete() - if email_students: - email_params['message'] = 'allowed_unenroll' - email_params['email_address'] = student_email - send_mail_to_student(student_email, email_params) - - after_state = EmailEnrollmentState(ccx, student_email) - - return previous_state, after_state - - -def get_email_params(ccx, auto_enroll, secure=True): - """ - get parameters for enrollment emails - """ - protocol = 'https' if secure else 'http' - - stripped_site_name = microsite.get_value( - 'SITE_NAME', - settings.SITE_NAME - ) - registration_url = u'{proto}://{site}{path}'.format( - proto=protocol, - site=stripped_site_name, - path=reverse('register_user') - ) - course_url = u'{proto}://{site}{path}'.format( - proto=protocol, - site=stripped_site_name, - path=reverse( - 'course_root', - kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} - ) - ) - - course_about_url = None - if not settings.FEATURES.get('ENABLE_MKTG_SITE', False): - course_about_url = u'{proto}://{site}{path}'.format( - proto=protocol, - site=stripped_site_name, - path=reverse( - 'about_course', - kwargs={'course_id': CCXLocator.from_course_locator(ccx.course_id, ccx.id)} - ) - ) - - email_params = { - 'site_name': stripped_site_name, - 'registration_url': registration_url, - 'course': ccx, - 'auto_enroll': auto_enroll, - 'course_url': course_url, - 'course_about_url': course_about_url, - } - return email_params - - -def send_mail_to_student(student, param_dict): - """ - Check parameters, set text template and send email to student - """ - if 'course' in param_dict: - param_dict['course_name'] = param_dict['course'].display_name - - param_dict['site_name'] = microsite.get_value( - 'SITE_NAME', - param_dict['site_name'] - ) - - subject = None - message = None - - message_type = param_dict['message'] - - email_template_dict = { - 'allowed_enroll': ( - 'ccx/enroll_email_allowedsubject.txt', - 'ccx/enroll_email_allowedmessage.txt' - ), - 'enrolled_enroll': ( - 'ccx/enroll_email_enrolledsubject.txt', - 'ccx/enroll_email_enrolledmessage.txt' - ), - 'allowed_unenroll': ( - 'ccx/unenroll_email_subject.txt', - 'ccx/unenroll_email_allowedmessage.txt' - ), - 'enrolled_unenroll': ( - 'ccx/unenroll_email_subject.txt', - 'ccx/unenroll_email_enrolledmessage.txt' - ), - } - - subject_template, message_template = email_template_dict.get( - message_type, (None, None) - ) - if subject_template is not None and message_template is not None: - subject = render_to_string(subject_template, param_dict) - message = render_to_string(message_template, param_dict) - - if subject and message: - message = message.strip() - - subject = ''.join(subject.splitlines()) - from_address = microsite.get_value( - 'email_from_address', - settings.DEFAULT_FROM_EMAIL - ) - - send_mail( - subject, - message, - from_address, - [student], - fail_silently=False - ) - - -def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set): - """ - Get the relevant set of (CustomCourseForEdX, CcxMembership, CourseOverview) - triplets to be displayed on a student's dashboard. - """ - error_message_format = "User {0} enrolled in {2} course {1}" - - # only active memberships for now - for membership in CcxMembership.memberships_for_user(user): - ccx = membership.ccx - - try: - course_overview = CourseOverview.get_from_id(ccx.course_id) - except CourseOverview.DoesNotExist: - log.error(error_message_format.format( # pylint: disable=logging-format-interpolation - user.username, ccx.course_id, "non-existent" - )) - continue - except IOError: - log.error(error_message_format.format( # pylint: disable=logging-format-interpolation - user.username, ccx.course_id, "broken" - )) - continue - - # if we are in a Microsite, then filter out anything that is not - # attributed (by ORG) to that Microsite - if course_org_filter and course_org_filter != course_overview.location.org: - continue - # Conversely, if we are not in a Microsite, then let's filter out any enrollments - # with courses attributed (by ORG) to Microsites - elif course_overview.location.org in org_filter_out_set: - continue - - # If, somehow, we've got a ccx that has been created for a - # course with a deprecated ID, we must filter it out. Emit a - # warning to the log so we can clean up. - if course_overview.location.deprecated: - log.warning( - "CCX %s exists for course %s with deprecated id", - ccx, - ccx.course_id - ) - continue - - yield (ccx, membership, course_overview) + return None + return ccx[0] diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 554f0be9b3..931ba33b08 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -36,22 +36,24 @@ from courseware.module_render import get_module_for_descriptor from edxmako.shortcuts import render_to_response from opaque_keys.edx.keys import CourseKey from ccx_keys.locator import CCXLocator -from student.roles import CourseCcxCoachRole +from student.roles import CourseCcxCoachRole # pylint: disable=import-error +from student.models import CourseEnrollment -from instructor.offline_gradecalc import student_grades -from instructor.views.api import _split_input_list -from instructor.views.tools import get_student_from_identifier +from instructor.offline_gradecalc import student_grades # pylint: disable=import-error +from instructor.views.api import _split_input_list # pylint: disable=import-error +from instructor.views.tools import get_student_from_identifier # pylint: disable=import-error +from instructor.enrollment import ( + enroll_email, + unenroll_email, + get_email_params, +) -from .models import CustomCourseForEdX, CcxMembership +from .models import CustomCourseForEdX from .overrides import ( clear_override_for_ccx, get_override_for_ccx, override_field_for_ccx, ) -from .utils import ( - enroll_email, - unenroll_email, -) log = logging.getLogger(__name__) @@ -127,7 +129,7 @@ def dashboard(request, course, ccx=None): context['schedule'] = json.dumps(schedule, indent=4) context['save_url'] = reverse( 'save_ccx', kwargs={'course_id': ccx_locator}) - context['ccx_members'] = CcxMembership.objects.filter(ccx=ccx) + context['ccx_members'] = CourseEnrollment.objects.filter(course_id=ccx_locator) context['gradebook_url'] = reverse( 'ccx_gradebook', kwargs={'course_id': ccx_locator}) context['grades_csv_url'] = reverse( @@ -156,7 +158,7 @@ def create_ccx(request, course, ccx=None): "You cannot create a CCX from a course using a deprecated id. " "Please create a rerun of this course in the studio to allow " "this action.")) - url = reverse('ccx_coach_dashboard', kwargs={'course_id', course.id}) + url = reverse('ccx_coach_dashboard', kwargs={'course_id': course.id}) return redirect(url) ccx = CustomCourseForEdX( @@ -407,15 +409,18 @@ def ccx_invite(request, course, ccx=None): email = user.email try: validate_email(email) + course_key = CCXLocator.from_course_locator(course.id, ccx.id) + email_params = get_email_params(course, auto_enroll) if action == 'Enroll': enroll_email( - ccx, + course_key, email, auto_enroll=auto_enroll, - email_students=email_students + email_students=email_students, + email_params=email_params ) if action == "Unenroll": - unenroll_email(ccx, email, email_students=email_students) + unenroll_email(course_key, email, email_students=email_students, email_params=email_params) except ValidationError: log.info('Invalid user name or email when trying to invite students: %s', email) url = reverse( @@ -444,20 +449,21 @@ def ccx_student_management(request, course, ccx=None): else: email = user.email + course_key = CCXLocator.from_course_locator(course.id, ccx.id) try: validate_email(email) if action == 'add': # by decree, no emails sent to students added this way # by decree, any students added this way are auto_enrolled - enroll_email(ccx, email, auto_enroll=True, email_students=False) + enroll_email(course_key, email, auto_enroll=True, email_students=False) elif action == 'revoke': - unenroll_email(ccx, email, email_students=False) + unenroll_email(course_key, email, email_students=False) except ValidationError: log.info('Invalid user name or email when trying to enroll student: %s', email) url = reverse( 'ccx_coach_dashboard', - kwargs={'course_id': CCXLocator.from_course_locator(course.id, ccx.id)} + kwargs={'course_id': course_key} ) return redirect(url) @@ -496,8 +502,8 @@ def ccx_gradebook(request, course, ccx=None): prep_course_for_grading(course, request) enrolled_students = User.objects.filter( - ccxmembership__ccx=ccx, - ccxmembership__active=1 + courseenrollment__course_id=ccx_key, + courseenrollment__is_active=1 ).order_by('username').select_related("profile") student_info = [ @@ -535,8 +541,8 @@ def ccx_grades_csv(request, course, ccx=None): prep_course_for_grading(course, request) enrolled_students = User.objects.filter( - ccxmembership__ccx=ccx, - ccxmembership__active=1 + courseenrollment__course_id=ccx_key, + courseenrollment__is_active=1 ).order_by('username').select_related("profile") grades = iterate_grades_for(course, enrolled_students) diff --git a/lms/templates/ccx/_dashboard_ccx_listing.html b/lms/templates/ccx/_dashboard_ccx_listing.html deleted file mode 100644 index 1eca7c6eb5..0000000000 --- a/lms/templates/ccx/_dashboard_ccx_listing.html +++ /dev/null @@ -1,80 +0,0 @@ -<%page args="ccx, membership, course_overview, show_courseware_link, is_course_blocked" /> - -<%! -from django.utils.translation import ugettext as _ -from django.core.urlresolvers import reverse -from courseware.courses import course_image_url, get_course_about_section -from ccx_keys.locator import CCXLocator -%> -<% - ccx_target = reverse('info', args=[CCXLocator.from_course_locator(course_overview.id, ccx.id)]) -%> -
  • -
    -
    - -
    -

    - % if show_courseware_link: - % if not is_course_blocked: - ${ccx.display_name} - % else: - ${ccx.display_name} - % endif - % else: - ${ccx.display_name} - % endif -

    -
    - ${get_course_about_section(course_overview, 'university')} - - ${course_overview.display_number_with_default | h} - - % if ccx.has_ended(): - ${_("Ended - {end_date}").format(end_date=ccx.end_datetime_text("SHORT_DATE"))} - % elif ccx.has_started(): - ${_("Started - {start_date}").format(start_date=ccx.start_datetime_text("SHORT_DATE"))} - % else: # hasn't started yet - ${_("Starts - {start_date}").format(start_date=ccx.start_datetime_text("SHORT_DATE"))} - % endif - -
    - % if show_courseware_link: -
    -
    - % if ccx.has_ended(): - % if not is_course_blocked: - ${_('View Archived Custom Course')} ${ccx.display_name} - % else: - ${_('View Archived Custom Course')} ${ccx.display_name} - % endif - % else: - % if not is_course_blocked: - ${_('View Custom Course')} ${ccx.display_name} - % else: - ${_('View Custom Course')} ${ccx.display_name} - % endif - % endif - -
    -
    - % endif -
    -
    -
    -
  • diff --git a/lms/templates/ccx/enrollment.html b/lms/templates/ccx/enrollment.html index f024ea3a68..15ea3d86a3 100644 --- a/lms/templates/ccx/enrollment.html +++ b/lms/templates/ccx/enrollment.html @@ -63,8 +63,8 @@ %for member in ccx_members: - ${member.student} - ${member.student.email} + ${member.user} + ${member.user.email}
    Revoke access
    %endfor diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index b3c53bb6a6..f03cb821d4 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -100,14 +100,6 @@ import json <%include file='dashboard/_dashboard_course_listing.html' args="course_overview=enrollment.course_overview, enrollment=enrollment, show_courseware_link=show_courseware_link, cert_status=cert_status, credit_status=credit_status, show_email_settings=show_email_settings, course_mode_info=course_mode_info, show_refund_option=show_refund_option, is_paid_course=is_paid_course, is_course_blocked=is_course_blocked, verification_status=course_verification_status, course_requirements=course_requirements, dashboard_index=dashboard_index, share_settings=share_settings, user=user" /> % endfor - % if settings.FEATURES.get('CUSTOM_COURSES_EDX', False): - % for ccx, membership, course_overview in ccx_membership_triplets: - <% show_courseware_link = ccx.has_started() %> - <% is_course_blocked = False %> - <%include file='ccx/_dashboard_ccx_listing.html' args="ccx=ccx, membership=membership, course_overview=course_overview, show_courseware_link=show_courseware_link, is_course_blocked=is_course_blocked" /> - % endfor - % endif - % else:
    diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index a6db91fbcc..d98b77f1cb 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -15,6 +15,8 @@ from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from xmodule_django.models import CourseKeyField, UsageKeyField +from ccx_keys.locator import CCXLocator + class CourseOverview(TimeStampedModel): """ @@ -100,17 +102,26 @@ class CourseOverview(TimeStampedModel): except ValueError: lowest_passing_grade = None + display_name = course.display_name + start = course.start + end = course.end + if isinstance(course.id, CCXLocator): + from ccx.utils import get_ccx_from_ccx_locator # pylint: disable=import-error + ccx = get_ccx_from_ccx_locator(course.id) + display_name = ccx.display_name + start = ccx.start + end = ccx.due + return cls( version=cls.VERSION, - id=course.id, _location=course.location, - display_name=course.display_name, + display_name=display_name, display_number_with_default=course.display_number_with_default, display_org_with_default=course.display_org_with_default, - start=course.start, - end=course.end, + start=start, + end=end, advertised_start=course.advertised_start, course_image_url=course_image_url(course), From 912db9910faa76d043cf03f0a7f24ff84f432644 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Fri, 7 Aug 2015 16:56:02 -0400 Subject: [PATCH 2/4] Fix CCX 0002 migration (cherry picked from commit 558b93fdeaaf72d02e050808698b2015e123a01c) --- .../ccx/migrations/0002_convert_memberships.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/ccx/migrations/0002_convert_memberships.py b/lms/djangoapps/ccx/migrations/0002_convert_memberships.py index 3030c463b0..4f595eb312 100644 --- a/lms/djangoapps/ccx/migrations/0002_convert_memberships.py +++ b/lms/djangoapps/ccx/migrations/0002_convert_memberships.py @@ -3,6 +3,7 @@ from south.utils import datetime_utils as datetime from south.db import db from south.v2 import DataMigration from django.db import models +from opaque_keys import InvalidKeyError class Migration(DataMigration): @@ -12,11 +13,14 @@ class Migration(DataMigration): memberships = orm['ccx.CcxMembership'].objects.select_related('ccx', 'student').all() for membership in memberships: ccx = membership.ccx - course_key = CCXLocator.from_course_locator(ccx.course_id, ccx.id) - enrollment, created = orm['student.CourseEnrollment'].objects.get_or_create( + try: + course_key = CCXLocator.from_course_locator(ccx.course_id, ccx.id) + enrollment, created = orm['student.CourseEnrollment'].objects.get_or_create( user=membership.student, course_id=course_key, - ) + ) + except InvalidKeyError: + membership.delete() def backwards(self, orm): From aff849281d9a7dbfeb19afcb2a61e82ab517d750 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Wed, 12 Aug 2015 02:06:09 +0500 Subject: [PATCH 3/4] Revert "Test library failed to export after import" This reverts commit 4c1c4619f0a1aca5878533cf20d116d4424cddf8. --- .../contentstore/tests/test_contentstore.py | 4 +-- cms/djangoapps/contentstore/views/course.py | 2 +- .../views/tests/test_import_export.py | 35 ------------------- .../contentstore/views/tests/test_item.py | 2 +- common/lib/xmodule/xmodule/course_module.py | 2 +- lms/djangoapps/django_comment_client/utils.py | 2 +- lms/djangoapps/mobile_api/users/tests.py | 4 +-- .../content/course_overviews/tests.py | 4 +-- requirements/edx/github.txt | 2 +- 9 files changed, 11 insertions(+), 46 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 768566b053..5c23196e71 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1903,8 +1903,8 @@ class RerunCourseTest(ContentStoreTestCase): source_course = CourseFactory.create(advertised_start="01-12-2015") destination_course_key = self.post_rerun_request(source_course.id) destination_course = self.store.get_course(destination_course_key) - # Advertised_start is String field so it will return empty string if its not set - self.assertEqual('', destination_course.advertised_start) + + self.assertEqual(None, destination_course.advertised_start) def test_rerun_of_rerun(self): source_course = CourseFactory.create() diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 28cbd6c7eb..5c230c13d7 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -787,7 +787,7 @@ def _rerun_course(request, org, number, run, fields): CourseRerunState.objects.initiated(source_course_key, destination_course_key, request.user, fields['display_name']) # Clear the fields that must be reset for the rerun - fields['advertised_start'] = '' + fields['advertised_start'] = None # Rerun the course as a new celery task json_fields = json.dumps(fields, cls=EdxJSONEncoder) diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index b67090005c..41e4681643 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -451,41 +451,6 @@ class ExportTestCase(CourseTestCase): finally: shutil.rmtree(root_dir / name) - def test_library_import_then_export(self): - """ - Verify that a library exports successfully after being imported. - """ - library = LibraryFactory.create(modulestore=self.store) - lib_key = library.location.library_key - name = library.url_name - - # import the library - extract_dir = path(tempfile.mkdtemp(dir=settings.DATA_DIR)) - extract_dir_relative = path.relpath(extract_dir, settings.DATA_DIR) - try: - with tarfile.open(path(TEST_DATA_DIR) / 'imports' / 'library.HhJfPD.tar.gz') as tar: - safetar_extractall(tar, extract_dir) - library_items = import_library_from_xml( - self.store, - self.user.id, - settings.GITHUB_REPO_ROOT, - [extract_dir_relative / 'library'], - load_error_modules=False, - static_content_store=contentstore(), - target_id=lib_key - ) - - # verify library import correctly - self.assertEqual(lib_key, library_items[0].location.library_key) - library = self.store.get_library(lib_key) - self.assertEqual(len(library.children), 3) - - # export library again - export_library_to_xml(self.store, contentstore(), lib_key, extract_dir, name) - - finally: - shutil.rmtree(extract_dir) - def test_export_success_with_custom_tag(self): """ Verify that course export with customtag diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 42ab3cbf38..fec27e64d6 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -625,7 +625,7 @@ class TestEditItem(TestEditItemSetup): data={'nullout': ['markdown']} ) problem = self.get_item_from_modulestore(self.problem_usage_key, verify_is_draft=True) - self.assertEqual(problem.markdown, '') + self.assertIsNone(problem.markdown) def test_date_fields(self): """ diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 808279b20e..694ab758a0 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -948,7 +948,7 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin): super(CourseDescriptor, self).__init__(*args, **kwargs) _ = self.runtime.service(self, "i18n").ugettext - if not self.wiki_slug: + if self.wiki_slug is None: self.wiki_slug = self.location.course if self.due_date_display_format is None and self.show_timezone is False: diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index e0c83e29ec..0374c770de 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -87,7 +87,7 @@ def has_forum_access(uname, course_id, rolename): def has_required_keys(module): """Returns True iff module has the proper attributes for generating metadata with get_discussion_id_map_entry()""" for key in ('discussion_id', 'discussion_category', 'discussion_target'): - if not getattr(module, key, None): + if getattr(module, key, None) is None: log.debug("Required key '%s' not in discussion %s, leaving out of category map", key, module.location) return False return True diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index c3b4f9187e..45fd9801ba 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -146,9 +146,9 @@ class TestUserEnrollmentApi(MobileAPITestCase, MobileAuthUserTestMixin): @ddt.data( (NEXT_WEEK, ADVERTISED_START, ADVERTISED_START, "string"), - (NEXT_WEEK, None, '', "string"), + (NEXT_WEEK, None, defaultfilters.date(NEXT_WEEK, "DATE_FORMAT"), "timestamp"), (DEFAULT_START_DATE, ADVERTISED_START, ADVERTISED_START, "string"), - (DEFAULT_START_DATE, None, '', "string") + (DEFAULT_START_DATE, None, None, "empty") ) @ddt.unpack @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) diff --git a/openedx/core/djangoapps/content/course_overviews/tests.py b/openedx/core/djangoapps/content/course_overviews/tests.py index 9b0500fa24..c55a3e0afc 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests.py +++ b/openedx/core/djangoapps/content/course_overviews/tests.py @@ -191,7 +191,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): "display_name": "", # Empty display name "start": LAST_MONTH, # Course already ended "end": LAST_WEEK, - "advertised_start": '', # No advertised start + "advertised_start": None, # No advertised start "pre_requisite_courses": [], # No pre-requisites "static_asset_path": "", # Empty asset path "certificates_show_before_end": False, @@ -200,7 +200,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): # # Don't set display name "start": DEFAULT_START_DATE, # Default start and end dates "end": None, - "advertised_start": '', # No advertised start + "advertised_start": None, # No advertised start "pre_requisite_courses": [], # No pre-requisites "static_asset_path": None, # No asset path "certificates_show_before_end": False, diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 9bd3e63350..c6ee7ccd9f 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -34,7 +34,7 @@ git+https://github.com/hmarr/django-debug-toolbar-mongo.git@b0686a76f1ce3532088c git+https://github.com/edx/rfc6266.git@v0.0.5-edx#egg=rfc6266==0.0.5-edx # Our libraries: --e git+https://github.com/edx/XBlock.git@0d3d43bd1ef0f882ef70d19d4f652d35dd37eb01#egg=XBlock +-e git+https://github.com/edx/XBlock.git@d1ff8cf31a9b94916ce06ba06d4176bd72e15768#egg=XBlock -e git+https://github.com/edx/codejail.git@6b17c33a89bef0ac510926b1d7fea2748b73aadd#egg=codejail -e git+https://github.com/edx/js-test-tool.git@v0.1.6#egg=js_test_tool -e git+https://github.com/edx/event-tracking.git@0.2.0#egg=event-tracking From 72e6b8aaeb92f0af2d6ff2e8e12af8701fbde20b Mon Sep 17 00:00:00 2001 From: Dennis Jen Date: Tue, 11 Aug 2015 17:06:22 -0400 Subject: [PATCH 4/4] Revert "Optimize OpenID Course Claims for non-global-staff users." This reverts commit f0d1772da39e9c3324a5d331743419571927f146. --- lms/djangoapps/oauth2_handler/handlers.py | 27 ++++------- lms/djangoapps/oauth2_handler/tests.py | 58 ++++++----------------- 2 files changed, 25 insertions(+), 60 deletions(-) diff --git a/lms/djangoapps/oauth2_handler/handlers.py b/lms/djangoapps/oauth2_handler/handlers.py index 4d08519fa7..5989dfb2fe 100644 --- a/lms/djangoapps/oauth2_handler/handlers.py +++ b/lms/djangoapps/oauth2_handler/handlers.py @@ -4,11 +4,12 @@ from django.conf import settings from django.core.cache import cache from xmodule.modulestore.django import modulestore +from courseware.access import has_access from openedx.core.djangoapps.user_api.models import UserPreference from student.models import anonymous_id_for_user from student.models import UserProfile from lang_pref import LANGUAGE_KEY -from student.roles import (GlobalStaff, CourseStaffRole, CourseInstructorRole, UserBasedRole) +from student.roles import GlobalStaff, CourseStaffRole, CourseInstructorRole class OpenIDHandler(object): @@ -189,31 +190,23 @@ class CourseAccessHandler(object): return course_ids + # pylint: disable=missing-docstring def _get_courses_with_access_type(self, user, access_type): - """ - If global staff, returns all courses. Otherwise, returns list of courses - based on role access (e.g. courses that course staff has access to). - """ - # Check the application cache and update if not present. The application # cache is useful since there are calls to different endpoints in close # succession, for example the id_token and user_info endpoints. + key = '-'.join([str(self.__class__), str(user.id), access_type]) course_ids = cache.get(key) if not course_ids: + courses = _get_all_courses() - if GlobalStaff().has_user(user): - # TODO: This code should be optimized in the future to caching - # the list of all courses in the system. - # The modulestore has all courses, while the roles table only has courses - # with roles. Thus, we'll use the modulestore to fetch all courses. - courses = _get_all_courses() - course_ids = [unicode(course.id) for course in courses] - else: - # Getting courses based on roles avoid querying mongo and thus faster - courses = UserBasedRole(user, access_type).courses_with_role() - course_ids = [unicode(course.course_id) for course in courses] + # Global staff have access to all courses. Filter courses for non-global staff. + if not GlobalStaff().has_user(user): + courses = [course for course in courses if has_access(user, access_type, course)] + + course_ids = [unicode(course.id) for course in courses] cache.set(key, course_ids, self.COURSE_CACHE_TIMEOUT) diff --git a/lms/djangoapps/oauth2_handler/tests.py b/lms/djangoapps/oauth2_handler/tests.py index e83d3f4c1a..d34d24072c 100644 --- a/lms/djangoapps/oauth2_handler/tests.py +++ b/lms/djangoapps/oauth2_handler/tests.py @@ -5,10 +5,9 @@ from lang_pref import LANGUAGE_KEY from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_TOY_MODULESTORE -from xmodule.modulestore.tests.factories import check_mongo_calls, check_mongo_calls_range from student.models import anonymous_id_for_user from student.models import UserProfile -from student.roles import CourseStaffRole, CourseInstructorRole, GlobalStaff +from student.roles import CourseStaffRole, CourseInstructorRole from student.tests.factories import UserFactory, UserProfileFactory from openedx.core.djangoapps.user_api.preferences.api import set_user_preference from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -78,8 +77,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): self.assertEqual(language, locale) def test_no_special_course_access(self): - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values('openid course_instructor course_staff') + scopes, claims = self.get_id_token_values('openid course_instructor course_staff') self.assertNotIn('course_staff', scopes) self.assertNotIn('staff_courses', claims) @@ -88,15 +86,17 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): def test_course_staff_courses(self): CourseStaffRole(self.course_key).add_users(self.user) - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values('openid course_staff') + + scopes, claims = self.get_id_token_values('openid course_staff') + self.assertIn('course_staff', scopes) self.assertNotIn('staff_courses', claims) # should not return courses in id_token def test_course_instructor_courses(self): CourseInstructorRole(self.course_key).add_users(self.user) - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values('openid course_instructor') + + scopes, claims = self.get_id_token_values('openid course_instructor') + self.assertIn('course_instructor', scopes) self.assertNotIn('instructor_courses', claims) # should not return courses in id_token @@ -113,8 +113,7 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): } } - with check_mongo_calls(0): - scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims) + scopes, claims = self.get_id_token_values(scope='openid course_staff', claims=claims) self.assertIn('course_staff', scopes) self.assertIn('staff_courses', claims) @@ -134,11 +133,6 @@ class IDTokenTest(BaseTestMixin, IDTokenTestCase): class UserInfoTest(BaseTestMixin, UserInfoTestCase): - def setUp(self): - super(UserInfoTest, self).setUp() - # clear the course ID cache - cache.clear() - def token_for_scope(self, scope): full_scope = 'openid %s' % scope self.set_access_token_scope(full_scope) @@ -164,48 +158,27 @@ class UserInfoTest(BaseTestMixin, UserInfoTestCase): self.assertEqual(result.status_code, 200) return claims - def test_request_global_staff_courses_using_scope(self): - GlobalStaff().add_users(self.user) - with check_mongo_calls_range(min_finds=1): - claims = self.get_with_scope('course_staff') - courses = claims['staff_courses'] - self.assertIn(self.course_id, courses) - self.assertEqual(len(courses), 1) - def test_request_staff_courses_using_scope(self): CourseStaffRole(self.course_key).add_users(self.user) - with check_mongo_calls(0): - claims = self.get_with_scope('course_staff') + claims = self.get_with_scope('course_staff') + courses = claims['staff_courses'] self.assertIn(self.course_id, courses) self.assertEqual(len(courses), 1) def test_request_instructor_courses_using_scope(self): CourseInstructorRole(self.course_key).add_users(self.user) - with check_mongo_calls(0): - claims = self.get_with_scope('course_instructor') + claims = self.get_with_scope('course_instructor') + courses = claims['instructor_courses'] self.assertIn(self.course_id, courses) self.assertEqual(len(courses), 1) - def test_request_global_staff_courses_with_claims(self): - GlobalStaff().add_users(self.user) - - values = [self.course_id, 'some_invalid_course'] - with check_mongo_calls_range(min_finds=1): - claims = self.get_with_claim_value('course_staff', 'staff_courses', values) - self.assertEqual(len(claims), 2) - - courses = claims['staff_courses'] - self.assertIn(self.course_id, courses) - self.assertEqual(len(courses), 1) - def test_request_staff_courses_with_claims(self): CourseStaffRole(self.course_key).add_users(self.user) values = [self.course_id, 'some_invalid_course'] - with check_mongo_calls(0): - claims = self.get_with_claim_value('course_staff', 'staff_courses', values) + claims = self.get_with_claim_value('course_staff', 'staff_courses', values) self.assertEqual(len(claims), 2) courses = claims['staff_courses'] @@ -216,8 +189,7 @@ class UserInfoTest(BaseTestMixin, UserInfoTestCase): CourseInstructorRole(self.course_key).add_users(self.user) values = ['edX/toy/TT_2012_Fall', self.course_id, 'invalid_course_id'] - with check_mongo_calls(0): - claims = self.get_with_claim_value('course_instructor', 'instructor_courses', values) + claims = self.get_with_claim_value('course_instructor', 'instructor_courses', values) self.assertEqual(len(claims), 2) courses = claims['instructor_courses']