Commit 9ddee934 by Carlos de la Guardia Committed by cewing

MIT: CCX. Code Quality fixes

add doc strings; fix pep8 warning
address some minor issues brought up by the code review process
parent b3da2a54
......@@ -1821,12 +1821,12 @@ def activate_account(request, key):
# 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
pfms = CcxFutureMembership.objects.filter(
ccxfms = CcxFutureMembership.objects.filter(
email=student[0].email
)
for pfm in pfms:
if pfm.auto_enroll:
CcxMembership.auto_enroll(student[0], pfm)
for ccxfm in ccxfms:
if ccxfm.auto_enroll:
CcxMembership.auto_enroll(student[0], ccxfm)
resp = render_to_response(
"registration/activation_complete.html",
......
......@@ -1225,25 +1225,17 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
all_descriptors = []
graded_sections = {}
def yield_descendents(module):
for child in module.get_children():
def yield_descriptor_descendents(module_descriptor):
for child in module_descriptor.get_children():
yield child
for module_descriptor in yield_descendents(child):
for module_descriptor in yield_descriptor_descendents(child):
yield module_descriptor
<<<<<<< HEAD
for chapter in self.get_children():
for section in chapter.get_children():
if section.graded:
xmoduledescriptors = list(yield_descriptor_descendents(section))
xmoduledescriptors.append(section)
=======
for c in module.get_children():
for s in c.get_children():
if s.graded:
xmoduledescriptors = list(yield_descendents(s))
xmoduledescriptors.append(s)
>>>>>>> Hide course blocks not in the CCX from view for coaches and students
# The xmoduledescriptors included here are only the ones that have scores.
section_description = {
......
......@@ -377,7 +377,7 @@ class DiscussionTab(EnrolledOrStaffTab):
def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled):
if settings.FEATURES.get('CUSTOM_COURSES_EDX', False):
from ccx.overrides import get_current_ccx
from ccx.overrides import get_current_ccx # pylint: disable=import-error
if get_current_ccx():
return False
super_can_display = super(DiscussionTab, self).can_display(
......@@ -752,11 +752,16 @@ class CcxCoachTab(CourseTab):
)
def can_display(self, course, settings, *args, **kw):
"""
Since we don't get the user here, we use a thread local defined in the ccx
overrides to get it, then use the course to get the coach role and find out if
the user is one.
"""
user_is_coach = False
if settings.FEATURES.get('CUSTOM_COURSES_EDX', False):
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from student.roles import CourseCcxCoachRole
from ccx.overrides import get_current_request
from student.roles import CourseCcxCoachRole # pylint: disable=import-error
from ccx.overrides import get_current_request # pylint: disable=import-error
course_id = course.id.to_deprecated_string()
course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
role = CourseCcxCoachRole(course_key)
......@@ -766,7 +771,7 @@ class CcxCoachTab(CourseTab):
super_can_display = super(CcxCoachTab, self).can_display(
course, settings, *args, **kw
)
return (user_is_coach and super_can_display)
return user_is_coach and super_can_display
class CourseTabList(List):
......
"""
we use this to mark the active ccx, for use by ccx middleware and some views
"""
ACTIVE_CCX_KEY = '_ccx_id'
"""
Models for the custom course feature
"""
from django.contrib.auth.models import User
from django.db import models
from student.models import CourseEnrollment, AlreadyEnrolledError
from xmodule_django.models import CourseKeyField, LocationKeyField
from student.models import CourseEnrollment, AlreadyEnrolledError # pylint: disable=import-error
from xmodule_django.models import CourseKeyField, LocationKeyField # pylint: disable=import-error
class CustomCourseForEdX(models.Model):
......@@ -45,6 +48,9 @@ class CcxMembership(models.Model):
@classmethod
def memberships_for_user(cls, user, active=True):
"""
active memberships for a user
"""
return cls.objects.filter(student=user, active__exact=active)
......@@ -65,7 +71,7 @@ class CcxFieldOverride(models.Model):
location = LocationKeyField(max_length=255, db_index=True)
field = models.CharField(max_length=255)
class Meta:
class Meta: # pylint: disable=missing-docstring,old-style-class
unique_together = (('ccx', 'location', 'field'),)
value = models.TextField(default='null')
"""
API related to providing field overrides for individual students. This is used
by the individual due dates feature.
by the individual custom courses feature.
"""
import json
import threading
......@@ -9,8 +9,8 @@ from contextlib import contextmanager
from django.db import transaction, IntegrityError
from courseware.field_overrides import FieldOverrideProvider
from ccx import ACTIVE_CCX_KEY
from courseware.field_overrides import FieldOverrideProvider # pylint: disable=import-error
from ccx import ACTIVE_CCX_KEY # pylint: disable=import-error
from .models import CcxMembership, CcxFieldOverride
......@@ -22,6 +22,9 @@ class CustomCoursesForEdxOverrideProvider(FieldOverrideProvider):
overrides to be made on a per user basis.
"""
def get(self, block, name, default):
"""
Just call the get_override_for_ccx method if there is a ccx
"""
ccx = get_current_ccx()
if ccx:
return get_override_for_ccx(ccx, block, name, default)
......@@ -58,15 +61,18 @@ def get_current_ccx():
"""
Return the ccx that is active for this request.
"""
ccx = _CCX_CONTEXT.ccx
if ccx:
return ccx
return _CCX_CONTEXT.ccx
def get_current_request():
"""
Return the active request, so that we can get context information in places
where it is limited, like in the tabs.
"""
request = _CCX_CONTEXT.request
return request
def get_override_for_ccx(ccx, block, name, default=None):
"""
Gets the value of the overridden field for the `ccx`. `block` and `name`
......@@ -74,11 +80,11 @@ def get_override_for_ccx(ccx, block, name, default=None):
overridden for the given ccx, returns `default`.
"""
if not hasattr(block, '_ccx_overrides'):
block._ccx_overrides = {}
overrides = block._ccx_overrides.get(ccx.id)
block._ccx_overrides = {} # pylint: disable=protected-access
overrides = block._ccx_overrides.get(ccx.id) # pylint: disable=protected-access
if overrides is None:
overrides = _get_overrides_for_ccx(ccx, block)
block._ccx_overrides[ccx.id] = overrides
block._ccx_overrides[ccx.id] = overrides # pylint: disable=protected-access
return overrides.get(name, default)
......@@ -110,20 +116,20 @@ def override_field_for_ccx(ccx, block, name, value):
value = json.dumps(field.to_json(value))
try:
override = CcxFieldOverride.objects.create(
ccx=ccx,
location=block.location,
field=name,
value=value)
ccx=ccx,
location=block.location,
field=name,
value=value)
except IntegrityError:
transaction.commit()
override = CcxFieldOverride.objects.get(
ccx=ccx,
location=block.location,
field=name)
ccx=ccx,
location=block.location,
field=name)
override.value = value
override.save()
if hasattr(block, '_ccx_overrides'):
del block._ccx_overrides[ccx.id]
del block._ccx_overrides[ccx.id] # pylint: disable=protected-access
def clear_override_for_ccx(ccx, block, name):
......@@ -140,7 +146,7 @@ def clear_override_for_ccx(ccx, block, name):
field=name).delete()
if hasattr(block, '_ccx_overrides'):
del block._ccx_overrides[ccx.id]
del block._ccx_overrides[ccx.id] # pylint: disable=protected-access
except CcxFieldOverride.DoesNotExist:
pass
......@@ -169,7 +175,7 @@ class CcxMiddleware(object):
_CCX_CONTEXT.request = request
def process_response(self, request, response):
def process_response(self, request, response): # pylint: disable=unused-argument
"""
Clean up afterwards.
"""
......
"""
Dummy factories for tests
"""
from factory.django import DjangoModelFactory
from ccx.models import CustomCourseForEdX
from ccx.models import CcxMembership
from ccx.models import CcxFutureMembership
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):
class CcxFactory(DjangoModelFactory): # pylint: disable=missing-docstring
FACTORY_FOR = CustomCourseForEdX
display_name = "Test CCX"
id = None # pylint: disable=redefined-builtin, invalid-name
class CcxMembershipFactory(DjangoModelFactory):
class CcxMembershipFactory(DjangoModelFactory): # pylint: disable=missing-docstring
FACTORY_FOR = CcxMembership
active = False
class CcxFutureMembershipFactory(DjangoModelFactory):
class CcxFutureMembershipFactory(DjangoModelFactory): # pylint: disable=missing-docstring
FACTORY_FOR = CcxFutureMembership
from student.models import CourseEnrollment
from student.roles import CourseCcxCoachRole
from student.tests.factories import (
"""
tests for the models
"""
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,
......@@ -10,7 +13,6 @@ from xmodule.modulestore.tests.factories import CourseFactory
from .factories import (
CcxFactory,
CcxMembershipFactory,
CcxFutureMembershipFactory,
)
from ..models import (
......@@ -36,6 +38,9 @@ class TestCcxMembership(ModuleStoreTestCase):
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,
......@@ -44,24 +49,36 @@ class TestCcxMembership(ModuleStoreTestCase):
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):
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):
......@@ -75,7 +92,7 @@ class TestCcxMembership(ModuleStoreTestCase):
self.assertTrue(self.has_ccx_future_membership(user))
self.assertFalse(self.has_course_enrollment(user))
# auto_enroll user
self.call_MUT(user, pfm)
self.call_mut(user, pfm)
self.assertTrue(self.has_course_enrollment(user))
self.assertTrue(self.has_ccx_membership(user))
......@@ -89,7 +106,7 @@ class TestCcxMembership(ModuleStoreTestCase):
self.assertTrue(self.has_ccx_future_membership(user))
self.assertTrue(self.has_course_enrollment(user))
self.call_MUT(user, pfm)
self.call_mut(user, pfm)
self.assertTrue(self.has_course_enrollment(user))
self.assertTrue(self.has_ccx_membership(user))
......@@ -103,7 +120,7 @@ class TestCcxMembership(ModuleStoreTestCase):
self.assertTrue(self.has_ccx_future_membership(user))
self.assertFalse(self.has_course_enrollment(user))
self.assertRaises(ValueError, self.call_MUT, user, pfm)
self.assertRaises(ValueError, self.call_mut, user, pfm)
self.assertFalse(self.has_course_enrollment(user))
self.assertFalse(self.has_ccx_membership(user))
......
"""
tests for overrides
"""
import datetime
import mock
import pytz
from courseware.field_overrides import OverrideFieldData
from courseware.field_overrides import OverrideFieldData # pylint: disable=import-error
from django.test.utils import override_settings
from student.tests.factories import AdminFactory
from student.tests.factories import AdminFactory # pylint: disable=import-error
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
......@@ -13,6 +16,7 @@ from ..overrides import override_field_for_ccx
from .test_views import flatten, iter_blocks
@override_settings(FIELD_OVERRIDE_PROVIDERS=(
'ccx.overrides.CustomCoursesForEdxOverrideProvider',))
class TestFieldOverrides(ModuleStoreTestCase):
......@@ -39,7 +43,7 @@ class TestFieldOverrides(ModuleStoreTestCase):
verticals = flatten([
[ItemFactory.create(due=due, parent=sequential) for _ in xrange(2)]
for sequential in sequentials])
blocks = flatten([
blocks = flatten([ # pylint: disable=unused-variable
[ItemFactory.create(parent=vertical) for _ in xrange(2)]
for vertical in verticals])
......@@ -59,12 +63,14 @@ class TestFieldOverrides(ModuleStoreTestCase):
# just inject the override field storage in this brute force manner.
OverrideFieldData.provider_classes = None
for block in iter_blocks(course):
block._field_data = OverrideFieldData.wrap( # pylint: disable=protected-access
AdminFactory.create(), block._field_data) # pylint: disable=protected-access
block._field_data = OverrideFieldData.wrap( # pylint: disable=protected-access
AdminFactory.create(), block._field_data) # pylint: disable=protected-access
# and after everything is done, clean up by un-doing the change to the
# OverrideFieldData object that is done during the wrap method.
def cleanup_provider_classes():
"""
After everything is done, clean up by un-doing the change to the
OverrideFieldData object that is done during the wrap method.
"""
OverrideFieldData.provider_classes = None
self.addCleanup(cleanup_provider_classes)
......
......@@ -4,14 +4,14 @@ CCX Enrollment operations for use by Coach APIs.
Does not include any access control, be sure to check access before calling.
"""
import logging
from courseware.courses import get_course_about_section
from courseware.courses import get_course_by_id
from courseware.courses import get_course_about_section # pylint: disable=import-error
from courseware.courses import get_course_by_id # pylint: disable=import-error
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
from microsite_configuration import microsite
from edxmako.shortcuts import render_to_string # pylint: disable=import-error
from microsite_configuration import microsite # pylint: disable=import-error
from xmodule.modulestore.django import modulestore
from xmodule.error_module import ErrorDescriptor
......@@ -44,7 +44,7 @@ class EmailEnrollmentState(object):
self.in_ccx = in_ccx
def __repr__(self):
return "{}(user={}, member={}, in_ccx={}".format(
return "{}(user={}, member={}, in_ccx={})".format(
self.__class__.__name__,
self.user,
self.member,
......@@ -52,6 +52,7 @@ class EmailEnrollmentState(object):
)
def to_dict(self):
""" return dict with membership and ccx info """
return {
'user': self.user,
'member': self.member,
......@@ -60,6 +61,9 @@ class EmailEnrollmentState(object):
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)
......@@ -97,6 +101,9 @@ def enroll_email(ccx, student_email, auto_enroll=False, email_students=False, em
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)
......@@ -112,8 +119,7 @@ def unenroll_email(ccx, student_email, email_students=False, email_params=None):
send_mail_to_student(student_email, email_params)
else:
if CcxFutureMembership.objects.filter(
ccx=ccx, email=student_email
).exists():
ccx=ccx, email=student_email).exists():
CcxFutureMembership.objects.get(
ccx=ccx, email=student_email
).delete()
......@@ -128,6 +134,9 @@ def unenroll_email(ccx, student_email, email_students=False, email_params=None):
def get_email_params(ccx, auto_enroll, secure=True):
"""
get parameters for enrollment emails
"""
protocol = 'https' if secure else 'http'
course_id = ccx.course_id
......@@ -172,6 +181,9 @@ def get_email_params(ccx, auto_enroll, secure=True):
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
......@@ -266,6 +278,7 @@ def get_all_ccx_for_user(user):
})
return memberships
def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set):
"""
Get the relevant set of (CustomCourseForEdX, CcxMembership, Course)
......@@ -289,6 +302,6 @@ def get_ccx_membership_triplets(user, course_org_filter, org_filter_out_set):
yield (ccx, membership, course)
else:
log.error("User {0} enrolled in {2} course {1}".format(
log.error("User {0} enrolled in {2} course {1}".format( # pylint: disable=logging-format-interpolation
user.username, ccx.course_id, "broken" if course else "non-existent"
))
......@@ -22,22 +22,23 @@ from django.core.validators import validate_email
from django.shortcuts import redirect
from django.utils.translation import ugettext as _
from django.views.decorators.cache import cache_control
from django_future.csrf import ensure_csrf_cookie
from django_future.csrf import ensure_csrf_cookie # pylint: disable=import-error
from django.contrib.auth.decorators import login_required
from django.contrib.auth.models import User
from courseware.courses import get_course_by_id
from courseware.field_overrides import disable_overrides
from courseware.grades import iterate_grades_for
from courseware.model_data import FieldDataCache
from courseware.module_render import get_module_for_descriptor
from edxmako.shortcuts import render_to_response
from courseware.courses import get_course_by_id # pylint: disable=import-error
from courseware.field_overrides import disable_overrides # pylint: disable=import-error
from courseware.grades import iterate_grades_for # pylint: disable=import-error
from courseware.model_data import FieldDataCache # pylint: disable=import-error
from courseware.module_render import get_module_for_descriptor # pylint: disable=import-error
from edxmako.shortcuts import render_to_response # pylint: disable=import-error
from opaque_keys.edx.keys import CourseKey
from student.roles import CourseCcxCoachRole
from student.roles import CourseCcxCoachRole # pylint: disable=import-error
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 .models import CustomCourseForEdX, CcxMembership
from .overrides import (
......@@ -50,7 +51,7 @@ from .utils import (
enroll_email,
unenroll_email,
)
from ccx import ACTIVE_CCX_KEY
from ccx import ACTIVE_CCX_KEY # pylint: disable=import-error
log = logging.getLogger(__name__)
......@@ -181,7 +182,7 @@ def save_ccx(request, course):
clear_override_for_ccx(ccx, block, 'due')
if not unit['hidden'] and block.graded:
graded[block.format] = graded.get(block.format, 0) + 1
graded[block.format] = graded.get(block.format, 0) + 1
children = unit.get('children', None)
if children:
......@@ -232,7 +233,9 @@ def set_grading_policy(request, course):
def validate_date(year, month, day, hour, minute):
# avoid corrupting db if bad dates come in
"""
avoid corrupting db if bad dates come in
"""
valid = True
if year < 0:
valid = False
......@@ -320,6 +323,9 @@ def get_ccx_schedule(course, ccx):
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
@coach_dashboard
def ccx_schedule(request, course):
"""
get json representation of ccx schedule
"""
ccx = get_ccx_for_coach(course, request.user)
schedule = get_ccx_schedule(course, ccx)
json_schedule = json.dumps(schedule, indent=4)
......@@ -512,7 +518,7 @@ def switch_active_ccx(request, course_id, ccx_id=None):
requested_ccx = CustomCourseForEdX.objects.get(pk=ccx_id)
assert unicode(requested_ccx.course_id) == course_id
if not CcxMembership.objects.filter(
ccx=requested_ccx, student=request.user, active=True
ccx=requested_ccx, student=request.user, active=True
).exists():
ccx_id = None
except CustomCourseForEdX.DoesNotExist:
......
......@@ -20,7 +20,6 @@ from abc import ABCMeta, abstractmethod
from contextlib import contextmanager
from django.conf import settings
from xblock.field_data import FieldData
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.inheritance import InheritanceMixin
......
......@@ -27,7 +27,6 @@ from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
log = logging.getLogger("edx.courseware")
......
......@@ -20,7 +20,7 @@ from django.dispatch import receiver
from model_utils.models import TimeStampedModel
from xmodule_django.models import CourseKeyField, LocationKeyField, BlockTypeKeyField
from xmodule_django.models import CourseKeyField, LocationKeyField, BlockTypeKeyField # pylint: disable=import-error
class StudentModule(models.Model):
......@@ -37,8 +37,7 @@ class StudentModule(models.Model):
('course', 'course'),
('chapter', 'Section'),
('sequential', 'Subsection'),
('library_content', 'Library Content'),
)
('library_content', 'Library Content'))
## These three are the key for the object
module_type = models.CharField(max_length=32, choices=MODULE_TYPES, default='problem', db_index=True)
......@@ -88,7 +87,7 @@ class StudentModule(models.Model):
return 'StudentModule<%r>' % ({
'course_id': self.course_id,
'module_type': self.module_type,
'student': self.student.username,
'student': self.student.username, # pylint: disable=no-member
'module_state_key': self.module_state_key,
'state': str(self.state)[:20],
},)
......@@ -244,7 +243,7 @@ class StudentFieldOverride(TimeStampedModel):
location = LocationKeyField(max_length=255, db_index=True)
student = models.ForeignKey(User, db_index=True)
class Meta: # pylint: disable=missing-docstring
class Meta(object): # pylint: disable=missing-docstring
unique_together = (('course_id', 'field', 'location', 'student'),)
field = models.CharField(max_length=255)
......
......@@ -25,11 +25,11 @@ def get_override_for_user(user, block, name, default=None):
overridden for the given user, returns `default`.
"""
if not hasattr(block, '_student_overrides'):
block._student_overrides = {}
overrides = block._student_overrides.get(user.id)
block._student_overrides = {} # pylint: disable=protected-access
overrides = block._student_overrides.get(user.id) # pylint: disable=protected-access
if overrides is None:
overrides = _get_overrides_for_user(user, block)
block._student_overrides[user.id] = overrides
block._student_overrides[user.id] = overrides # pylint: disable=protected-access
return overrides.get(name, default)
......
......@@ -26,6 +26,7 @@ class OverrideFieldDataTests(TestCase):
"""
def setUp(self):
super(OverrideFieldDataTests, self).setUp()
OverrideFieldData.provider_classes = None
def tearDown(self):
......
......@@ -720,7 +720,7 @@ def modify_access(request, course_id):
rolename = request.GET.get('rolename')
action = request.GET.get('action')
if not rolename in ROLES:
if rolename not in ROLES:
error = strip_tags("unknown rolename '{}'".format(rolename))
log.error(error)
return HttpResponseBadRequest(error)
......@@ -783,7 +783,7 @@ def list_course_role_members(request, course_id):
rolename = request.GET.get('rolename')
if not rolename in ROLES:
if rolename not in ROLES:
return HttpResponseBadRequest()
def extract_user_info(user):
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment