MIT: CCX. Fix issues identified in code review

Original Commit Messages:

use edx's own get_parent method, rather than our own.

add field to unique constraint to avoid MultipleObjectsReturned in case of multiple browser clicks on submit

fix 0011 migration, inherit from TimeStampedField and add composite index (migration only)

fix bug where adding an already registered user to a ccx would cause a crash due to an undefined variable

add assertNumQueries tests to test modules where override field providers are used

remove unnecessary teardown

implement recommended style for checking empty list

import utility methods rather than use duplicate code

added comment explaining date conversion to string for json

add logging for invalid users or emails when enrolling students

add comment about xmodule user state

avoid using get_or_create, which seems to be causing a race condition on schedule change save

relocate badly placed edvent handlers to fix multiple submit problem

common/lib/xmodule/xmodule/course_module.py

@@ -1213,12 +1213,15 @@ class CourseDescriptor(CourseFields, SequenceDescriptor):
 
 
         """
+        # If this descriptor has been bound to a student, return the corresponding
+        # XModule. If not, just use the descriptor itself
         try:
             module = getattr(self, '_xmodule', None)
             if not module:
                 module = self
         except UndefinedContext:
             module = self
+
         all_descriptors = []
         graded_sections = {}
 

lms/djangoapps/ccx/overrides.py

@@ -7,6 +7,8 @@ import threading
 
 from contextlib import contextmanager
 
+from django.db import transaction, IntegrityError
+
 from courseware.field_overrides import FieldOverrideProvider
 from ccx import ACTIVE_CCX_KEY
 
@@ -97,20 +99,29 @@ def _get_overrides_for_ccx(ccx, block):
     return overrides
 
 
+@transaction.commit_on_success
 def override_field_for_ccx(ccx, block, name, value):
     """
     Overrides a field for the `ccx`.  `block` and `name` specify the block
     and the name of the field on that block to override.  `value` is the
     value to set for the given field.
     """
-    override, created = CcxFieldOverride.objects.get_or_create(
+    field = block.fields[name]
+    value = json.dumps(field.to_json(value))
+    try:
+        override = CcxFieldOverride.objects.create(
+        ccx=ccx,
+        location=block.location,
+        field=name,
+        value=value)
+    except IntegrityError:
+        transaction.commit()
+        override = CcxFieldOverride.objects.get(
         ccx=ccx,
         location=block.location,
         field=name)
-    field = block.fields[name]
-    override.value = json.dumps(field.to_json(value))
+        override.value = value
     override.save()
-
     if hasattr(block, '_ccx_overrides'):
         del block._ccx_overrides[ccx.id]
 

lms/djangoapps/ccx/tests/test_overrides.py

@@ -11,6 +11,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from ..models import CustomCourseForEdX
 from ..overrides import override_field_for_ccx
 
+from .test_views import flatten, iter_blocks
 
 @override_settings(FIELD_OVERRIDE_PROVIDERS=(
     'ccx.overrides.CustomCoursesForEdxOverrideProvider',))
@@ -76,6 +77,28 @@ class TestFieldOverrides(ModuleStoreTestCase):
         override_field_for_ccx(self.ccx, chapter, 'start', ccx_start)
         self.assertEquals(chapter.start, ccx_start)
 
+    def test_override_num_queries(self):
+        """
+        Test that overriding and accessing a field produce same number of queries.
+        """
+        ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC)
+        chapter = self.course.get_children()[0]
+        with self.assertNumQueries(4):
+            override_field_for_ccx(self.ccx, chapter, 'start', ccx_start)
+            dummy = chapter.start
+
+    def test_overriden_field_access_produces_no_extra_queries(self):
+        """
+        Test no extra queries when accessing an overriden field more than once.
+        """
+        ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC)
+        chapter = self.course.get_children()[0]
+        with self.assertNumQueries(4):
+            override_field_for_ccx(self.ccx, chapter, 'start', ccx_start)
+            dummy1 = chapter.start
+            dummy2 = chapter.start
+            dummy3 = chapter.start
+
     def test_override_is_inherited(self):
         """
         Test that sequentials inherit overridden start date from chapter.
@@ -98,22 +121,3 @@ class TestFieldOverrides(ModuleStoreTestCase):
         override_field_for_ccx(self.ccx, chapter, 'due', ccx_due)
         vertical = chapter.get_children()[0].get_children()[0]
         self.assertEqual(vertical.due, ccx_due)
-
-
-def flatten(seq):
-    """
-    For [[1, 2], [3, 4]] returns [1, 2, 3, 4].  Does not recurse.
-    """
-    return [x for sub in seq for x in sub]
-
-
-def iter_blocks(course):
-    """
-    Returns an iterator over all of the blocks in a course.
-    """
-    def visit(block):
-        yield block
-        for child in block.get_children():
-            for descendant in visit(child):  # wish they'd backport yield from
-                yield descendant
-    return visit(course)

lms/djangoapps/ccx/tests/test_utils.py

@@ -253,7 +253,7 @@ class TestEnrollEmail(ModuleStoreTestCase):
         """enroll a non-user email and send an enrollment email to them
         """
         # ensure no emails are in the outbox now
-        self.assertEqual(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         test_email = "nobody@nowhere.com"
         before, after = self.call_FUT(
             student_email=test_email, email_students=True
@@ -273,7 +273,7 @@ class TestEnrollEmail(ModuleStoreTestCase):
         """
         self.create_user()
         # ensure no emails are in the outbox now
-        self.assertEqual(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         before, after = self.call_FUT(email_students=True)
 
         # there should be a membership set for this email address now
@@ -290,7 +290,7 @@ class TestEnrollEmail(ModuleStoreTestCase):
         """
         self.register_user_in_ccx()
         # ensure no emails are in the outbox now
-        self.assertEqual(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         before, after = self.call_FUT(email_students=True)
 
         # there should be a membership set for this email address now
@@ -306,7 +306,7 @@ class TestEnrollEmail(ModuleStoreTestCase):
         """register a non-user via email address but send no email
         """
         # ensure no emails are in the outbox now
-        self.assertEqual(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         test_email = "nobody@nowhere.com"
         before, after = self.call_FUT(
             student_email=test_email, email_students=False
@@ -317,13 +317,13 @@ class TestEnrollEmail(ModuleStoreTestCase):
         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(len(self.outbox), 0)
+        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(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         before, after = self.call_FUT(email_students=False)
 
         # there should be a membership set for this email address now
@@ -331,14 +331,14 @@ class TestEnrollEmail(ModuleStoreTestCase):
         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(len(self.outbox), 0)
+        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(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         before, after = self.call_FUT(email_students=False)
 
         # there should be a membership set for this email address now
@@ -346,7 +346,7 @@ class TestEnrollEmail(ModuleStoreTestCase):
         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(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
 
 
 # TODO: deal with changes in behavior for auto_enroll
@@ -364,11 +364,6 @@ class TestUnenrollEmail(ModuleStoreTestCase):
         self.ccx = CcxFactory(course_id=course.id, coach=coach)
         self.outbox = self.get_outbox()
 
-    def tearDown(self):
-        for attr in ['user', 'email']:
-            if hasattr(self, attr):
-                delattr(self, attr)
-
     def get_outbox(self):
         """Return the django mail outbox"""
         from django.core import mail
@@ -428,7 +423,7 @@ class TestUnenrollEmail(ModuleStoreTestCase):
         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(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         # unenroll the student
         before, after = self.call_FUT(email_students=True)
 
@@ -447,7 +442,7 @@ class TestUnenrollEmail(ModuleStoreTestCase):
         self.make_ccx_membership()
         # assert that a membership exists and that no emails have been sent
         self.assertTrue(self.check_membership())
-        self.assertEqual(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         # unenroll the student
         before, after = self.call_FUT(email_students=True)
 
@@ -467,7 +462,7 @@ class TestUnenrollEmail(ModuleStoreTestCase):
         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(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         # unenroll the student
         before, after = self.call_FUT()
 
@@ -477,7 +472,7 @@ class TestUnenrollEmail(ModuleStoreTestCase):
         for state in [before, after]:
             self.check_enrollment_state(state, False, None, False)
         # no email was sent to the student
-        self.assertEqual(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
 
     def test_unenroll_member_no_email(self):
         """unenroll a current member but send no email
@@ -485,7 +480,7 @@ class TestUnenrollEmail(ModuleStoreTestCase):
         self.make_ccx_membership()
         # assert that a membership exists and that no emails have been sent
         self.assertTrue(self.check_membership())
-        self.assertEqual(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
         # unenroll the student
         before, after = self.call_FUT()
 
@@ -495,7 +490,7 @@ class TestUnenrollEmail(ModuleStoreTestCase):
         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(len(self.outbox), 0)
+        self.assertEqual(self.outbox, [])
 
 
 class TestUserCCXList(ModuleStoreTestCase):
@@ -530,16 +525,16 @@ class TestUserCCXList(ModuleStoreTestCase):
 
     def test_anonymous_sees_no_ccx(self):
         memberships = self.call_FUT(self.anonymous)
-        self.assertEqual(len(memberships), 0)
+        self.assertEqual(memberships, [])
 
     def test_unenrolled_sees_no_ccx(self):
         memberships = self.call_FUT(self.user)
-        self.assertEqual(len(memberships), 0)
+        self.assertEqual(memberships, [])
 
     def test_enrolled_inactive_sees_no_ccx(self):
         self.register_user_in_ccx()
         memberships = self.call_FUT(self.user)
-        self.assertEqual(len(memberships), 0)
+        self.assertEqual(memberships, [])
 
     def test_enrolled_sees_a_ccx(self):
         self.register_user_in_ccx(active=True)

lms/djangoapps/ccx/tests/test_views.py

@@ -223,7 +223,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase):
         enrollment = CourseEnrollmentFactory(course_id=self.course.id)
         student = enrollment.user
         outbox = self.get_outbox()
-        self.assertEqual(len(outbox), 0)
+        self.assertEqual(outbox, [])
 
         url = reverse(
             'ccx_invite',
@@ -254,7 +254,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase):
         enrollment = CourseEnrollmentFactory(course_id=self.course.id)
         student = enrollment.user
         outbox = self.get_outbox()
-        self.assertEqual(len(outbox), 0)
+        self.assertEqual(outbox, [])
         # student is member of CCX:
         CcxMembershipFactory(ccx=ccx, student=student)
 
@@ -286,7 +286,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.make_coach()
         ccx = self.make_ccx()
         outbox = self.get_outbox()
-        self.assertEqual(len(outbox), 0)
+        self.assertEqual(outbox, [])
 
         url = reverse(
             'ccx_invite',
@@ -318,7 +318,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase):
         ccx = self.make_ccx()
         outbox = self.get_outbox()
         CcxFutureMembershipFactory(ccx=ccx, email=test_email)
-        self.assertEqual(len(outbox), 0)
+        self.assertEqual(outbox, [])
 
         url = reverse(
             'ccx_invite',
@@ -351,7 +351,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase):
         student = enrollment.user
         # no emails have been sent so far
         outbox = self.get_outbox()
-        self.assertEqual(len(outbox), 0)
+        self.assertEqual(outbox, [])
 
         url = reverse(
             'ccx_manage_student',
@@ -366,7 +366,7 @@ 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), 0)
+        self.assertEqual(outbox, [])
         # a CcxMembership exists for this student
         self.assertTrue(
             CcxMembership.objects.filter(ccx=ccx, student=student).exists()
@@ -382,7 +382,7 @@ class TestCoachDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase):
         CcxMembershipFactory(ccx=ccx, student=student)
         # no emails have been sent so far
         outbox = self.get_outbox()
-        self.assertEqual(len(outbox), 0)
+        self.assertEqual(outbox, [])
 
         url = reverse(
             'ccx_manage_student',
@@ -397,7 +397,7 @@ 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), 0)
+        self.assertEqual(outbox, [])
         # a CcxMembership exists for this student
         self.assertFalse(
             CcxMembership.objects.filter(ccx=ccx, student=student).exists()

lms/djangoapps/ccx/utils.py

@@ -65,8 +65,8 @@ def enroll_email(ccx, student_email, auto_enroll=False, email_students=False, em
     previous_state = EmailEnrollmentState(ccx, student_email)
 
     if previous_state.user:
+        user = User.objects.get(email=student_email)
         if not previous_state.in_ccx:
-            user = User.objects.get(email=student_email)
             membership = CcxMembership(
                 ccx=ccx, student=user, active=True
             )

lms/djangoapps/ccx/views.py

@@ -32,7 +32,7 @@ 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 opaque_keys.edx.locations import SlashSeparatedCourseKey
+from opaque_keys.edx.keys import CourseKey
 from student.roles import CourseCcxCoachRole
 
 from instructor.offline_gradecalc import student_grades
@@ -69,7 +69,7 @@ def coach_dashboard(view):
         Wraps the view function, performing access check, loading the course,
         and modifying the view's call signature.
         """
-        course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+        course_key = CourseKey.from_string(course_id)
         role = CourseCcxCoachRole(course_key)
         if not role.has_user(request.user):
             return HttpResponseForbidden(
@@ -283,6 +283,8 @@ def get_ccx_schedule(course, ccx):
     def visit(node, depth=1):
         """
         Recursive generator function which yields CCX schedule nodes.
+        We convert dates to string to get them ready for use by the js date
+        widgets, which use text inputs.
         """
         for child in node.get_children():
             start = get_override_for_ccx(ccx, child, 'start', None)
@@ -358,7 +360,7 @@ def ccx_invite(request, course):
             if action == "Unenroll":
                 unenroll_email(ccx, email, email_students=email_students)
         except ValidationError:
-            pass  # maybe log this?
+            log.info('Invalid user name or email when trying to invite students: %s', email)
     url = reverse('ccx_coach_dashboard', kwargs={'course_id': course.id})
     return redirect(url)
 
@@ -389,7 +391,7 @@ def ccx_student_management(request, course):
         elif action == 'revoke':
             unenroll_email(ccx, email, email_students=False)
     except ValidationError:
-        pass  # XXX: log, report?
+        log.info('Invalid user name or email when trying to enroll student: %s', email)
 
     url = reverse('ccx_coach_dashboard', kwargs={'course_id': course.id})
     return redirect(url)
@@ -499,8 +501,7 @@ def ccx_grades_csv(request, course):
 def switch_active_ccx(request, course_id, ccx_id=None):
     """set the active CCX for the logged-in user
     """
-    user = request.user
-    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+    course_key = CourseKey.from_string(course_id)
     # will raise Http404 if course_id is bad
     course = get_course_by_id(course_key)
     course_url = reverse(
@@ -509,7 +510,7 @@ def switch_active_ccx(request, course_id, ccx_id=None):
     if ccx_id is not None:
         try:
             requested_ccx = CustomCourseForEdX.objects.get(pk=ccx_id)
-            assert requested_ccx.course_id.to_deprecated_string() == course_id
+            assert unicode(requested_ccx.course_id) == course_id
             if not CcxMembership.objects.filter(
                 ccx=requested_ccx, student=request.user, active=True
             ).exists():

lms/djangoapps/courseware/field_overrides.py

@@ -199,20 +199,7 @@ def _lineage(block):
     Returns an iterator over all ancestors of the given block, starting with
     its immediate parent and ending at the root of the block tree.
     """
-    parent = _get_parent(block)
+    parent = block.get_parent()
     while parent:
         yield parent
-        parent = _get_parent(parent)
-
-
-def _get_parent(block):
-    """
-    Gets parent of a block, trying to cache result.
-    """
-    if not hasattr(block, '_parent'):
-        store = modulestore()
-        # Call to get_parent_location is fairly expensive.  Is there a more
-        # performant way to get at the ancestors of a block?
-        location = store.get_parent_location(block.location)
-        block._parent = store.get_item(location) if location else None
-    return block._parent
+        parent = parent.get_parent()

lms/djangoapps/courseware/models.py

@@ -18,6 +18,8 @@ from django.db import models
 from django.db.models.signals import post_save
 from django.dispatch import receiver
 
+from model_utils.models import TimeStampedModel
+
 from xmodule_django.models import CourseKeyField, LocationKeyField, BlockTypeKeyField
 
 
@@ -232,7 +234,7 @@ class OfflineComputedGradeLog(models.Model):
         return "[OCGLog] %s: %s" % (self.course_id.to_deprecated_string(), self.created)  # pylint: disable=no-member
 
 
-class StudentFieldOverride(models.Model):
+class StudentFieldOverride(TimeStampedModel):
     """
     Holds the value of a specific field overriden for a student.  This is used
     by the code in the `courseware.student_field_overrides` module to provide
@@ -243,7 +245,7 @@ class StudentFieldOverride(models.Model):
     student = models.ForeignKey(User, db_index=True)
 
     class Meta:   # pylint: disable=missing-docstring
-        unique_together = (('course_id', 'location', 'student'),)
+        unique_together = (('course_id', 'field', 'location', 'student'),)
 
     field = models.CharField(max_length=255)
     value = models.TextField(default='null')

lms/djangoapps/instructor/tests/test_tools.py

@@ -242,6 +242,12 @@ class TestSetDueDateExtension(ModuleStoreTestCase):
         self.assertEqual(self.homework.due, extended)
         self.assertEqual(self.assignment.due, extended)
 
+    def test_set_due_date_extension_num_queries(self):
+        extended = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=utc)
+        with self.assertNumQueries(4):
+            tools.set_due_date_extension(self.course, self.week1, self.user, extended)
+            self._clear_field_data_cache()
+
     def test_set_due_date_extension_invalid_date(self):
         extended = datetime.datetime(2009, 1, 1, 0, 0, tzinfo=utc)
         with self.assertRaises(tools.DashboardError):

lms/static/js/ccx/schedule.js

@@ -54,55 +54,6 @@ var edx = edx || {};
 		self.schedule_collection.set(self.schedule);
 	        self.render();
 	    });
-        },
-
-        render: function() {
-	    self.schedule = this.schedule_collection.toJSON();
-            self.hidden = this.pruned(self.schedule, function(node) {
-              return node.hidden || node.category !== 'vertical'});
-            this.showing = this.pruned(self.schedule, function(node) {
-              return !node.hidden});
-            this.$el.html(schedule_template({chapters: this.showing}));
-            $('table.ccx-schedule .sequential,.vertical').hide();
-            $('table.ccx-schedule .toggle-collapse').on('click', this.toggle_collapse);
-	    //
-	    // Hidden hover fields for empty date fields
-	    $('table.ccx-schedule .date a').each(function() {
-	      if (! $(this).text()) {
-		$(this).text('Set date').addClass('empty');
-	      }
-	    });
-	    
-	    // Handle date edit clicks
-	    $('table.ccx-schedule .date a').attr('href', '#enter-date-modal')
-	      .leanModal({closeButton: '.close-modal'});
-	    $('table.ccx-schedule .due-date a').on('click', this.enterNewDate('due'));
-	    $('table.ccx-schedule .start-date a').on('click', this.enterNewDate('start'));
-	    // Click handler for remove all
-	    $('table.ccx-schedule a#remove-all').on('click', function(event) {
-	      event.preventDefault();
-	      self.schedule_apply(self.schedule, self.hide);
-	      self.dirty = true;
-	      self.schedule_collection.set(self.schedule);
-	      self.render();
-	    });
-
-	    // Show or hide form
-	    if (this.hidden.length) {
-	      // Populate chapters select, depopulate others
-	      this.chapter_select.html('')
-		.append('<option value="none">'+gettext("Select a chapter")+'...</option>')
-		.append(self.schedule_options(this.hidden));
-	      this.sequential_select.html('').prop('disabled', true);
-	      this.vertical_select.html('').prop('disabled', true);
-	      $('form#add-unit').show();
-	      $('#all-units-added').hide();
-	      $('#add-unit-button').prop('disabled', true);
-	    }
-	    else {
-	      $('form#add-unit').hide();
-	      $('#all-units-added').show();
-	    }
 
 	    // Add unit handlers
 	    this.chapter_select.on('change', function(event) {
@@ -174,6 +125,44 @@ var edx = edx || {};
 	      self.render();
 	    });
 
+	    // Handle save button
+	    $('#dirty-schedule #save-changes').on('click', function(event) {
+		event.preventDefault();
+		self.save();
+	    });
+
+        },
+
+        render: function() {
+	    self.schedule = this.schedule_collection.toJSON();
+            self.hidden = this.pruned(self.schedule, function(node) {
+              return node.hidden || node.category !== 'vertical'});
+            this.showing = this.pruned(self.schedule, function(node) {
+              return !node.hidden});
+            this.$el.html(schedule_template({chapters: this.showing}));
+            $('table.ccx-schedule .sequential,.vertical').hide();
+            $('table.ccx-schedule .toggle-collapse').on('click', this.toggle_collapse);
+	    //
+	    // Hidden hover fields for empty date fields
+	    $('table.ccx-schedule .date a').each(function() {
+	      if (! $(this).text()) {
+		$(this).text('Set date').addClass('empty');
+	      }
+	    });
+	    
+	    // Handle date edit clicks
+	    $('table.ccx-schedule .date a').attr('href', '#enter-date-modal')
+	      .leanModal({closeButton: '.close-modal'});
+	    $('table.ccx-schedule .due-date a').on('click', this.enterNewDate('due'));
+	    $('table.ccx-schedule .start-date a').on('click', this.enterNewDate('start'));
+	    // Click handler for remove all
+	    $('table.ccx-schedule a#remove-all').on('click', function(event) {
+	      event.preventDefault();
+	      self.schedule_apply(self.schedule, self.hide);
+	      self.dirty = true;
+	      self.schedule_collection.set(self.schedule);
+	      self.render();
+	    });
 	    // Remove unit handler
 	    $('table.ccx-schedule a.remove-unit').on('click', function(event) {
 	      var row = $(this).closest('tr'),
@@ -185,16 +174,28 @@ var edx = edx || {};
 	      self.render(); 
 	    });
 
+
+	    // Show or hide form
+	    if (this.hidden.length) {
+	      // Populate chapters select, depopulate others
+	      this.chapter_select.html('')
+		.append('<option value="none">'+gettext("Select a chapter")+'...</option>')
+		.append(self.schedule_options(this.hidden));
+	      this.sequential_select.html('').prop('disabled', true);
+	      this.vertical_select.html('').prop('disabled', true);
+	      $('form#add-unit').show();
+	      $('#all-units-added').hide();
+	      $('#add-unit-button').prop('disabled', true);
+	    }
+	    else {
+	      $('form#add-unit').hide();
+	      $('#all-units-added').show();
+	    }
+
 	    // Show or hide save button
 	    if (this.dirty) $('#dirty-schedule').show()
 	    else $('#dirty-schedule').hide();
 
-	    // Handle save button
-	    $('#dirty-schedule #save-changes').on('click', function(event) {
-		event.preventDefault();
-		self.save();
-	    });
-
 	    $('#ajax-error').hide();
 
 	    return this;