From c64f0b1a180897feae71c47ae52df237d4124427 Mon Sep 17 00:00:00 2001 From: dcadams Date: Mon, 29 Apr 2013 09:38:10 -0700 Subject: [PATCH 001/125] Modifications to instructor dashboard - enrollment tab: - Remove single enrollment/unenrollment - Make unenrollment work with multi-email listbox. - Provide checkbox for auto-enroll on activate if user not yet signed up. - Enroll student when activating if auto-enroll flag set. --- ...eld_courseenrollmentallowed_auto_enroll.py | 173 ++++++++++++++++++ common/djangoapps/student/models.py | 1 + common/djangoapps/student/views.py | 11 +- .../instructor/tests/test_enrollment.py | 160 ++++++++++++++++ lms/djangoapps/instructor/views.py | 114 ++++++++---- .../courseware/instructor_dashboard.html | 11 +- 6 files changed, 424 insertions(+), 46 deletions(-) create mode 100644 common/djangoapps/student/migrations/0025_auto__add_field_courseenrollmentallowed_auto_enroll.py create mode 100644 lms/djangoapps/instructor/tests/test_enrollment.py diff --git a/common/djangoapps/student/migrations/0025_auto__add_field_courseenrollmentallowed_auto_enroll.py b/common/djangoapps/student/migrations/0025_auto__add_field_courseenrollmentallowed_auto_enroll.py new file mode 100644 index 0000000000..8ce1d0cda1 --- /dev/null +++ b/common/djangoapps/student/migrations/0025_auto__add_field_courseenrollmentallowed_auto_enroll.py @@ -0,0 +1,173 @@ +# -*- coding: utf-8 -*- +import datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding field 'CourseEnrollmentAllowed.auto_enroll' + db.add_column('student_courseenrollmentallowed', 'auto_enroll', + self.gf('django.db.models.fields.BooleanField')(default=False), + keep_default=False) + + + def backwards(self, orm): + # Deleting field 'CourseEnrollmentAllowed.auto_enroll' + db.delete_column('student_courseenrollmentallowed', 'auto_enroll') + + + 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'}) + }, + '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'}) + }, + 'student.courseenrollment': { + 'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'CourseEnrollment'}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'null': 'True', 'db_index': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) + }, + 'student.courseenrollmentallowed': { + 'Meta': {'unique_together': "(('email', 'course_id'),)", 'object_name': 'CourseEnrollmentAllowed'}, + 'auto_enroll': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'null': 'True', 'db_index': 'True', 'blank': 'True'}), + 'email': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'student.pendingemailchange': { + 'Meta': {'object_name': 'PendingEmailChange'}, + 'activation_key': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'new_email': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['auth.User']", 'unique': 'True'}) + }, + 'student.pendingnamechange': { + 'Meta': {'object_name': 'PendingNameChange'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'new_name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}), + 'rationale': ('django.db.models.fields.CharField', [], {'max_length': '1024', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['auth.User']", 'unique': 'True'}) + }, + 'student.registration': { + 'Meta': {'object_name': 'Registration', 'db_table': "'auth_registration'"}, + 'activation_key': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '32', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'unique': 'True'}) + }, + 'student.testcenterregistration': { + 'Meta': {'object_name': 'TestCenterRegistration'}, + 'accommodation_code': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}), + 'accommodation_request': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '1024', 'blank': 'True'}), + 'authorization_id': ('django.db.models.fields.IntegerField', [], {'null': 'True', 'db_index': 'True'}), + 'client_authorization_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '20', 'db_index': 'True'}), + 'confirmed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'db_index': 'True', 'blank': 'True'}), + 'eligibility_appointment_date_first': ('django.db.models.fields.DateField', [], {'db_index': 'True'}), + 'eligibility_appointment_date_last': ('django.db.models.fields.DateField', [], {'db_index': 'True'}), + 'exam_series_code': ('django.db.models.fields.CharField', [], {'max_length': '15', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'processed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'testcenter_user': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['student.TestCenterUser']"}), + 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'db_index': 'True', 'blank': 'True'}), + 'upload_error_message': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), + 'upload_status': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '20', 'blank': 'True'}), + 'uploaded_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'user_updated_at': ('django.db.models.fields.DateTimeField', [], {'db_index': 'True'}) + }, + 'student.testcenteruser': { + 'Meta': {'object_name': 'TestCenterUser'}, + 'address_1': ('django.db.models.fields.CharField', [], {'max_length': '40'}), + 'address_2': ('django.db.models.fields.CharField', [], {'max_length': '40', 'blank': 'True'}), + 'address_3': ('django.db.models.fields.CharField', [], {'max_length': '40', 'blank': 'True'}), + 'candidate_id': ('django.db.models.fields.IntegerField', [], {'null': 'True', 'db_index': 'True'}), + 'city': ('django.db.models.fields.CharField', [], {'max_length': '32', 'db_index': 'True'}), + 'client_candidate_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '50', 'db_index': 'True'}), + 'company_name': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '50', 'blank': 'True'}), + 'confirmed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'country': ('django.db.models.fields.CharField', [], {'max_length': '3', 'db_index': 'True'}), + 'created_at': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'db_index': 'True', 'blank': 'True'}), + 'extension': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '8', 'blank': 'True'}), + 'fax': ('django.db.models.fields.CharField', [], {'max_length': '35', 'blank': 'True'}), + 'fax_country_code': ('django.db.models.fields.CharField', [], {'max_length': '3', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '50', 'db_index': 'True'}), + 'middle_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'phone': ('django.db.models.fields.CharField', [], {'max_length': '35'}), + 'phone_country_code': ('django.db.models.fields.CharField', [], {'max_length': '3', 'db_index': 'True'}), + 'postal_code': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '16', 'blank': 'True'}), + 'processed_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}), + 'salutation': ('django.db.models.fields.CharField', [], {'max_length': '50', 'blank': 'True'}), + 'state': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '20', 'blank': 'True'}), + 'suffix': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}), + 'updated_at': ('django.db.models.fields.DateTimeField', [], {'auto_now': 'True', 'db_index': 'True', 'blank': 'True'}), + 'upload_error_message': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), + 'upload_status': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '20', 'blank': 'True'}), + 'uploaded_at': ('django.db.models.fields.DateTimeField', [], {'db_index': 'True', 'null': 'True', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'default': 'None', 'to': "orm['auth.User']", 'unique': 'True'}), + 'user_updated_at': ('django.db.models.fields.DateTimeField', [], {'db_index': 'True'}) + }, + 'student.userprofile': { + 'Meta': {'object_name': 'UserProfile', 'db_table': "'auth_userprofile'"}, + 'allow_certificate': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'courseware': ('django.db.models.fields.CharField', [], {'default': "'course.xml'", 'max_length': '255', 'blank': 'True'}), + 'gender': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '6', 'null': 'True', 'blank': 'True'}), + 'goals': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'language': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'level_of_education': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '6', 'null': 'True', 'blank': 'True'}), + 'location': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'mailing_address': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), + 'meta': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'user': ('django.db.models.fields.related.OneToOneField', [], {'related_name': "'profile'", 'unique': 'True', 'to': "orm['auth.User']"}), + 'year_of_birth': ('django.db.models.fields.IntegerField', [], {'db_index': 'True', 'null': 'True', 'blank': 'True'}) + }, + 'student.usertestgroup': { + 'Meta': {'object_name': 'UserTestGroup'}, + 'description': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '32', 'db_index': 'True'}), + 'users': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.User']", 'db_index': 'True', 'symmetrical': 'False'}) + } + } + + complete_apps = ['student'] \ No newline at end of file diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 56b1293c2d..ab68b05f4b 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -662,6 +662,7 @@ class CourseEnrollmentAllowed(models.Model): """ email = models.CharField(max_length=255, db_index=True) course_id = models.CharField(max_length=255, db_index=True) + auto_enroll = models.BooleanField(default=0) created = models.DateTimeField(auto_now_add=True, null=True, db_index=True) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 98b5265d5f..2b359acc6a 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -31,7 +31,7 @@ from student.models import (Registration, UserProfile, TestCenterUser, TestCente TestCenterRegistration, TestCenterRegistrationForm, PendingNameChange, PendingEmailChange, CourseEnrollment, unique_id_for_user, - get_testcenter_registration) + get_testcenter_registration, CourseEnrollmentAllowed) from certificates.models import CertificateStatuses, certificate_status_for_student @@ -825,6 +825,15 @@ def activate_account(request, key): if not r[0].user.is_active: r[0].activate() already_active = False + + #Enroll student in any pending courses he/she may have if auto_enroll flag is set + student = request.user + ceas = CourseEnrollmentAllowed.objects.filter(email=student.email) + for cea in ceas: + if cea.auto_enroll: + course_id = cea.course_id + enrollment, created = CourseEnrollment.objects.get_or_create(user_id=student.id, course_id=course_id) + resp = render_to_response("registration/activation_complete.html", {'user_logged_in': user_logged_in, 'already_active': already_active}) return resp if len(r) == 0: diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py new file mode 100644 index 0000000000..877f46da32 --- /dev/null +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -0,0 +1,160 @@ +""" Unit tests for enrollment methods in views.py """ + +from django.test.utils import override_settings +from django.contrib.auth.models import Group, User +from django.core.urlresolvers import reverse +from courseware.access import _course_staff_group_name +from courseware.tests.tests import LoginEnrollmentTestCase, TEST_DATA_XML_MODULESTORE, get_user +from xmodule.modulestore.django import modulestore +import xmodule.modulestore.django +from student.models import CourseEnrollment, CourseEnrollmentAllowed + + +@override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) +class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): + + def setUp(self): + xmodule.modulestore.django._MODULESTORES = {} + + self.full = modulestore().get_course("edX/full/6.002_Spring_2012") + self.toy = modulestore().get_course("edX/toy/2012_Fall") + + #Create instructor and student accounts + self.instructor = 'instructor1@test.com' + self.student1 = 'student1@test.com' + self.student2 = 'student2@test.com' + self.password = 'foo' + self.create_account('it1', self.instructor, self.password) + self.create_account('st1', self.student1, self.password) + self.create_account('st2', self.student2, self.password) + self.activate_user(self.instructor) + self.activate_user(self.student1) + self.activate_user(self.student2) + + def make_instructor(course): + group_name = _course_staff_group_name(course.location) + g = Group.objects.create(name=group_name) + g.user_set.add(get_user(self.instructor)) + + make_instructor(self.toy) + + #Enroll Students + self.logout() + self.login(self.student1, self.password) + self.enroll(self.toy) + + self.logout() + self.login(self.student2, self.password) + self.enroll(self.toy) + + #Enroll Instructor + self.logout() + self.login(self.instructor, self.password) + self.enroll(self.toy) + + + def test_unenrollment(self): + #Do un-enrollment + course = self.toy + url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) + response = self.client.post(url, {'action': 'Unenroll multiple students', 'multiple_students': 'student1@test.com, student2@test.com'}) + + #Check the page output + self.assertContains(response, 'student1@test.com') + self.assertContains(response, 'student1@test.com') + self.assertContains(response, 'un-enrolled') + + #Check the enrollment table + user = User.objects.get(email='student1@test.com') + ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) + self.assertEqual(0, len(ce)) + + user = User.objects.get(email='student2@test.com') + ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) + self.assertEqual(0, len(ce)) + + + def test_enrollmemt_new_student_autoenroll_on(self): + + #Run the Enroll students command + course = self.toy + url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) + response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'test1_1@student.com, test1_2@student.com', 'auto_enroll': 'on'}) + + #Check the page output + self.assertContains(response, 'test1_1@student.com') + self.assertContains(response, 'test1_2@student.com') + self.assertContains(response, 'user does not exist, enrollment allowed, pending with auto enrollment on') + + #Check the enrollmentallowed db entries + cea = CourseEnrollmentAllowed.objects.filter(email='test1_1@student.com', course_id=course.id) + self.assertEqual(1, cea[0].auto_enroll) + cea = CourseEnrollmentAllowed.objects.filter(email='test1_2@student.com', course_id=course.id) + self.assertEqual(1, cea[0].auto_enroll) + + #Check there is no enrollment db entry other than for the setup instructor and students + ce = CourseEnrollment.objects.filter(course_id=course.id) + self.assertEqual(3, len(ce)) + + #Create and activate student accounts with same email + self.student1 = 'test1_1@student.com' + self.password = 'bar' + self.create_account('s1_1', self.student1, self.password) + self.activate_user(self.student1) + + self.student2 = 'test1_2@student.com' + self.password = 'bar' + self.create_account('s1_2', self.student2, self.password) + self.activate_user(self.student2) + + #Check students are enrolled + user = User.objects.get(email='test1_1@student.com') + ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) + self.assertEqual(1, len(ce)) + + user = User.objects.get(email='test1_2@student.com') + ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) + self.assertEqual(1, len(ce)) + + + def test_enrollmemt_new_student_autoenroll_off(self): + + #Run the Enroll students command + course = self.toy + url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) + response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'test2_1@student.com, test2_2@student.com'}) + + #Check the page output + self.assertContains(response, 'test2_1@student.com') + self.assertContains(response, 'test2_2@student.com') + self.assertContains(response, 'user does not exist, enrollment allowed, pending with auto enrollment off') + + #Check the enrollmentallowed db entries + cea = CourseEnrollmentAllowed.objects.filter(email='test2_1@student.com', course_id=course.id) + self.assertEqual(0, cea[0].auto_enroll) + cea = CourseEnrollmentAllowed.objects.filter(email='test2_2@student.com', course_id=course.id) + self.assertEqual(0, cea[0].auto_enroll) + + #Check there is no enrollment db entry other than for the setup instructor and students + ce = CourseEnrollment.objects.filter(course_id=course.id) + self.assertEqual(3, len(ce)) + + #Create and activate student accounts with same email + self.student = 'test2_1@student.com' + self.password = 'bar' + self.create_account('s2_1', self.student, self.password) + self.activate_user(self.student) + + self.student = 'test2_2@student.com' + self.password = 'bar' + self.create_account('s2_2', self.student, self.password) + self.activate_user(self.student) + + #Check students are not enrolled + user = User.objects.get(email='test2_1@student.com') + ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) + self.assertEqual(0, len(ce)) + user = User.objects.get(email='test2_2@student.com') + ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) + self.assertEqual(0, len(ce)) + \ No newline at end of file diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index dd6748e691..e36fb3a3ac 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -539,35 +539,17 @@ def instructor_dashboard(request, course_id): datatable['data'] = [[x.email] for x in ceaset] datatable['title'] = action - elif action == 'Enroll student': - - student = request.POST.get('enstudent', '') - ret = _do_enroll_students(course, course_id, student) - datatable = ret['datatable'] - - elif action == 'Un-enroll student': - - student = request.POST.get('enstudent', '') - datatable = {} - isok = False - cea = CourseEnrollmentAllowed.objects.filter(course_id=course_id, email=student) - if cea: - cea.delete() - msg += "Un-enrolled student with email '%s'" % student - isok = True - try: - nce = CourseEnrollment.objects.get(user=User.objects.get(email=student), course_id=course_id) - nce.delete() - msg += "Un-enrolled student with email '%s'" % student - except Exception as err: - if not isok: - msg += "Error! Failed to un-enroll student with email '%s'\n" % student - msg += str(err) + '\n' - elif action == 'Enroll multiple students': - students = request.POST.get('enroll_multiple', '') - ret = _do_enroll_students(course, course_id, students) + students = request.POST.get('multiple_students', '') + auto_enroll = request.POST.get('auto_enroll', False) != False + ret = _do_enroll_students(course_id, students, auto_enroll=auto_enroll) + datatable = ret['datatable'] + + elif action == 'Unenroll multiple students': + + students = request.POST.get('multiple_students', '') + ret = _do_unenroll_students(course_id, students) datatable = ret['datatable'] elif action == 'List sections available in remote gradebook': @@ -985,17 +967,11 @@ def grade_summary(request, course_id): #----------------------------------------------------------------------------- # enrollment -def _do_enroll_students(course, course_id, students, overload=False): +def _do_enroll_students(course_id, students, overload=False, auto_enroll=False): """Do the actual work of enrolling multiple students, presented as a string of emails separated by commas or returns""" - new_students = split_by_comma_and_whitespace(students) - new_students = [str(s.strip()) for s in new_students] - new_students_lc = [x.lower() for x in new_students] - - if '' in new_students: - new_students.remove('') - + new_students = get_and_clean_student_list(students) status = dict([x, 'unprocessed'] for x in new_students) if overload: # delete all but staff @@ -1015,13 +991,20 @@ def _do_enroll_students(course, course_id, students, overload=False): try: user = User.objects.get(email=student) except User.DoesNotExist: - # user not signed up yet, put in pending enrollment allowed table - if CourseEnrollmentAllowed.objects.filter(email=student, course_id=course_id): - status[student] = 'user does not exist, enrollment already allowed, pending' + + #User not signed up yet, put in pending enrollment allowed table + cea = CourseEnrollmentAllowed.objects.filter(email=student, course_id=course_id) + + #If enrollmentallowed already exists, update auto_enroll flag to however it was set in UI + #Will be 0 or 1 records as there is a unique key on email + course_id + if cea: + cea[0].auto_enroll = auto_enroll + cea[0].save() + status[student] = 'user does not exist, enrollment already allowed, pending with auto enrollment ' + ("off", "on")[auto_enroll] continue - cea = CourseEnrollmentAllowed(email=student, course_id=course_id) + cea = CourseEnrollmentAllowed(email=student, course_id=course_id, auto_enroll=auto_enroll) cea.save() - status[student] = 'user does not exist, enrollment allowed, pending' + status[student] = 'user does not exist, enrollment allowed, pending with auto enrollment ' + ("off", "on")[auto_enroll] continue if CourseEnrollment.objects.filter(user=user, course_id=course_id): @@ -1077,6 +1060,57 @@ def enroll_students(request, course_id): 'debug': new_students}) +#Unenrollment +def _do_unenroll_students(course_id, students): + """Do the actual work of un-enrolling multiple students, presented as a string + of emails separated by commas or returns""" + + old_students = get_and_clean_student_list(students) + status = dict([x, 'unprocessed'] for x in old_students) + + for student in old_students: + + isok = False + cea = CourseEnrollmentAllowed.objects.filter(course_id=course_id, email=student) + #Safe to get the first entry as there is a unique key on email + course_id + if cea: + cea[0].delete() + status[student] = "un-enrolled" + isok = True + + try: + user = User.objects.get(email=student) + except User.DoesNotExist: + continue + + nce = CourseEnrollment.objects.filter(user=user, course_id=course_id) + #Safe to get the first entry as there is a unique key on user + course_id + if nce: + try: + nce[0].delete() + status[student] = "un-enrolled" + except Exception as err: + if not isok: + status[student] = "Error! Failed to un-enroll" + + datatable = {'header': ['StudentEmail', 'action']} + datatable['data'] = [[x, status[x]] for x in status] + datatable['title'] = 'Un-enrollment of students' + + data = dict(datatable=datatable) + return data + +def get_and_clean_student_list(students): + + students = split_by_comma_and_whitespace(students) + students = [str(s.strip()) for s in students] + students_lc = [x.lower() for x in students] + + if '' in students: + students.remove('') + + return students + #----------------------------------------------------------------------------- # answer distribution diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html index 4898914d01..4b37004181 100644 --- a/lms/templates/courseware/instructor_dashboard.html +++ b/lms/templates/courseware/instructor_dashboard.html @@ -296,9 +296,6 @@ function goto( mode)

-

- Student Email: -

%if settings.MITX_FEATURES.get('REMOTE_GRADEBOOK_URL','') and instructor_access: @@ -320,9 +317,13 @@ function goto( mode) %endif -

Add students: enter emails, separated by new lines or commas;

- +

Enroll or un-enroll one or many students: enter emails, separated by new lines or commas;

+ +

+ Auto-enroll students when they activate +

+ %endif From aa4b61180c6f5aa36a62eab6c356ab45ec9fadb2 Mon Sep 17 00:00:00 2001 From: dcadams Date: Mon, 29 Apr 2013 10:03:11 -0700 Subject: [PATCH 002/125] Some cleanup. --- .../instructor/tests/test_enrollment.py | 2 -- lms/djangoapps/instructor/views.py | 34 ++----------------- lms/urls.py | 2 -- 3 files changed, 2 insertions(+), 36 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 877f46da32..a33e224e5e 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -103,7 +103,6 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): self.activate_user(self.student1) self.student2 = 'test1_2@student.com' - self.password = 'bar' self.create_account('s1_2', self.student2, self.password) self.activate_user(self.student2) @@ -146,7 +145,6 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): self.activate_user(self.student) self.student = 'test2_2@student.com' - self.password = 'bar' self.create_account('s2_2', self.student, self.password) self.activate_user(self.student) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index e36fb3a3ac..606f875eb8 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -1030,36 +1030,6 @@ def _do_enroll_students(course_id, students, overload=False, auto_enroll=False): return data -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -def enroll_students(request, course_id): - """Allows a staff member to enroll students in a course. - - This is a short-term hack for Berkeley courses launching fall - 2012. In the long term, we would like functionality like this, but - we would like both the instructor and the student to agree. Right - now, this allows any instructor to add students to their course, - which we do not want. - - It is poorly written and poorly tested, but it's designed to be - stripped out. - """ - - course = get_course_with_access(request.user, course_id, 'staff') - existing_students = [ce.user.email for ce in CourseEnrollment.objects.filter(course_id=course_id)] - - new_students = request.POST.get('new_students') - ret = _do_enroll_students(course, course_id, new_students) - added_students = ret['added'] - rejected_students = ret['rejected'] - - return render_to_response("enroll_students.html", {'course': course_id, - 'existing_students': existing_students, - 'added_students': added_students, - 'rejected_students': rejected_students, - 'debug': new_students}) - - #Unenrollment def _do_unenroll_students(course_id, students): """Do the actual work of un-enrolling multiple students, presented as a string @@ -1072,7 +1042,7 @@ def _do_unenroll_students(course_id, students): isok = False cea = CourseEnrollmentAllowed.objects.filter(course_id=course_id, email=student) - #Safe to get the first entry as there is a unique key on email + course_id + #Will be 0 or 1 records as there is a unique key on email + course_id if cea: cea[0].delete() status[student] = "un-enrolled" @@ -1084,7 +1054,7 @@ def _do_unenroll_students(course_id, students): continue nce = CourseEnrollment.objects.filter(user=user, course_id=course_id) - #Safe to get the first entry as there is a unique key on user + course_id + #Will be 0 or 1 records as there is a unique key on user + course_id if nce: try: nce[0].delete() diff --git a/lms/urls.py b/lms/urls.py index 082004c1be..375335e3ea 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -236,8 +236,6 @@ if settings.COURSEWARE_ENABLED: 'instructor.views.gradebook', name='gradebook'), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/grade_summary$', 'instructor.views.grade_summary', name='grade_summary'), - url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/enroll_students$', - 'instructor.views.enroll_students', name='enroll_students'), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/staff_grading$', 'open_ended_grading.views.staff_grading', name='staff_grading'), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/staff_grading/get_next$', From 6a1a907339ece6231dd681f4f96fac144eef098a Mon Sep 17 00:00:00 2001 From: dcadams Date: Tue, 30 Apr 2013 11:48:23 -0700 Subject: [PATCH 003/125] Fix pep8 violations. --- .../instructor/tests/test_enrollment.py | 69 +++++++++++-------- lms/djangoapps/instructor/views.py | 26 +++---- 2 files changed, 52 insertions(+), 43 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index a33e224e5e..d2e31d3eb6 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -1,4 +1,6 @@ -""" Unit tests for enrollment methods in views.py """ +''' +Unit tests for enrollment methods in views.py +''' from django.test.utils import override_settings from django.contrib.auth.models import Group, User @@ -12,9 +14,11 @@ from student.models import CourseEnrollment, CourseEnrollmentAllowed @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): + ''' + Check Enrollment/Unenrollment with/without auto-enrollment on activation + ''' def setUp(self): - xmodule.modulestore.django._MODULESTORES = {} self.full = modulestore().get_course("edX/full/6.002_Spring_2012") self.toy = modulestore().get_course("edX/toy/2012_Fall") @@ -42,112 +46,118 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): self.logout() self.login(self.student1, self.password) self.enroll(self.toy) - + self.logout() self.login(self.student2, self.password) self.enroll(self.toy) - + #Enroll Instructor self.logout() self.login(self.instructor, self.password) self.enroll(self.toy) - def test_unenrollment(self): - #Do un-enrollment + ''' + Do un-enrollment test + ''' + course = self.toy url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) response = self.client.post(url, {'action': 'Unenroll multiple students', 'multiple_students': 'student1@test.com, student2@test.com'}) - + #Check the page output self.assertContains(response, 'student1@test.com') self.assertContains(response, 'student1@test.com') self.assertContains(response, 'un-enrolled') - + #Check the enrollment table user = User.objects.get(email='student1@test.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(0, len(ce)) - + user = User.objects.get(email='student2@test.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(0, len(ce)) - + self.assertEqual(0, len(ce)) def test_enrollmemt_new_student_autoenroll_on(self): - + ''' + Do auto-enroll on test + ''' + #Run the Enroll students command course = self.toy url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'test1_1@student.com, test1_2@student.com', 'auto_enroll': 'on'}) - + #Check the page output self.assertContains(response, 'test1_1@student.com') self.assertContains(response, 'test1_2@student.com') self.assertContains(response, 'user does not exist, enrollment allowed, pending with auto enrollment on') - + #Check the enrollmentallowed db entries cea = CourseEnrollmentAllowed.objects.filter(email='test1_1@student.com', course_id=course.id) self.assertEqual(1, cea[0].auto_enroll) cea = CourseEnrollmentAllowed.objects.filter(email='test1_2@student.com', course_id=course.id) self.assertEqual(1, cea[0].auto_enroll) - + #Check there is no enrollment db entry other than for the setup instructor and students ce = CourseEnrollment.objects.filter(course_id=course.id) self.assertEqual(3, len(ce)) - + #Create and activate student accounts with same email self.student1 = 'test1_1@student.com' self.password = 'bar' self.create_account('s1_1', self.student1, self.password) self.activate_user(self.student1) - + self.student2 = 'test1_2@student.com' self.create_account('s1_2', self.student2, self.password) self.activate_user(self.student2) - + #Check students are enrolled user = User.objects.get(email='test1_1@student.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(1, len(ce)) - + user = User.objects.get(email='test1_2@student.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) - self.assertEqual(1, len(ce)) - + self.assertEqual(1, len(ce)) def test_enrollmemt_new_student_autoenroll_off(self): - + ''' + Do auto-enroll off test + ''' + #Run the Enroll students command course = self.toy url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) response = self.client.post(url, {'action': 'Enroll multiple students', 'multiple_students': 'test2_1@student.com, test2_2@student.com'}) - + #Check the page output self.assertContains(response, 'test2_1@student.com') self.assertContains(response, 'test2_2@student.com') self.assertContains(response, 'user does not exist, enrollment allowed, pending with auto enrollment off') - + #Check the enrollmentallowed db entries cea = CourseEnrollmentAllowed.objects.filter(email='test2_1@student.com', course_id=course.id) self.assertEqual(0, cea[0].auto_enroll) cea = CourseEnrollmentAllowed.objects.filter(email='test2_2@student.com', course_id=course.id) self.assertEqual(0, cea[0].auto_enroll) - + #Check there is no enrollment db entry other than for the setup instructor and students ce = CourseEnrollment.objects.filter(course_id=course.id) self.assertEqual(3, len(ce)) - + #Create and activate student accounts with same email self.student = 'test2_1@student.com' self.password = 'bar' self.create_account('s2_1', self.student, self.password) self.activate_user(self.student) - + self.student = 'test2_2@student.com' self.create_account('s2_2', self.student, self.password) - self.activate_user(self.student) - + self.activate_user(self.student) + #Check students are not enrolled user = User.objects.get(email='test2_1@student.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) @@ -155,4 +165,3 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): user = User.objects.get(email='test2_2@student.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(0, len(ce)) - \ No newline at end of file diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 606f875eb8..756d1a6204 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -542,12 +542,12 @@ def instructor_dashboard(request, course_id): elif action == 'Enroll multiple students': students = request.POST.get('multiple_students', '') - auto_enroll = request.POST.get('auto_enroll', False) != False - ret = _do_enroll_students(course_id, students, auto_enroll=auto_enroll) + auto_enroll = request.POST.get('auto_enroll', False) is not False + ret = _do_enroll_students(course, course_id, students, auto_enroll=auto_enroll) datatable = ret['datatable'] elif action == 'Unenroll multiple students': - + students = request.POST.get('multiple_students', '') ret = _do_unenroll_students(course_id, students) datatable = ret['datatable'] @@ -967,11 +967,11 @@ def grade_summary(request, course_id): #----------------------------------------------------------------------------- # enrollment -def _do_enroll_students(course_id, students, overload=False, auto_enroll=False): +def _do_enroll_students(course, course_id, students, overload=False, auto_enroll=False): """Do the actual work of enrolling multiple students, presented as a string of emails separated by commas or returns""" - new_students = get_and_clean_student_list(students) + new_students, new_students_lc = get_and_clean_student_list(students) status = dict([x, 'unprocessed'] for x in new_students) if overload: # delete all but staff @@ -991,10 +991,10 @@ def _do_enroll_students(course_id, students, overload=False, auto_enroll=False): try: user = User.objects.get(email=student) except User.DoesNotExist: - + #User not signed up yet, put in pending enrollment allowed table cea = CourseEnrollmentAllowed.objects.filter(email=student, course_id=course_id) - + #If enrollmentallowed already exists, update auto_enroll flag to however it was set in UI #Will be 0 or 1 records as there is a unique key on email + course_id if cea: @@ -1035,11 +1035,11 @@ def _do_unenroll_students(course_id, students): """Do the actual work of un-enrolling multiple students, presented as a string of emails separated by commas or returns""" - old_students = get_and_clean_student_list(students) + old_students, old_students_lc = get_and_clean_student_list(students) status = dict([x, 'unprocessed'] for x in old_students) - + for student in old_students: - + isok = False cea = CourseEnrollmentAllowed.objects.filter(course_id=course_id, email=student) #Will be 0 or 1 records as there is a unique key on email + course_id @@ -1047,7 +1047,7 @@ def _do_unenroll_students(course_id, students): cea[0].delete() status[student] = "un-enrolled" isok = True - + try: user = User.objects.get(email=student) except User.DoesNotExist: @@ -1066,7 +1066,7 @@ def _do_unenroll_students(course_id, students): datatable = {'header': ['StudentEmail', 'action']} datatable['data'] = [[x, status[x]] for x in status] datatable['title'] = 'Un-enrollment of students' - + data = dict(datatable=datatable) return data @@ -1079,7 +1079,7 @@ def get_and_clean_student_list(students): if '' in students: students.remove('') - return students + return students, students_lc #----------------------------------------------------------------------------- # answer distribution From 2ef3bcc92dd599125cc5fe34566953717e9ac841 Mon Sep 17 00:00:00 2001 From: dcadams Date: Wed, 8 May 2013 14:52:18 -0700 Subject: [PATCH 004/125] Commit just to get Jenkins to rerun. --- lms/djangoapps/instructor/tests/test_enrollment.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index d2e31d3eb6..775f39f442 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -1,5 +1,6 @@ ''' Unit tests for enrollment methods in views.py + ''' from django.test.utils import override_settings From 5e75e9132204ce086cc6f454e2cd855320231f67 Mon Sep 17 00:00:00 2001 From: dcadams Date: Mon, 13 May 2013 13:20:37 -0700 Subject: [PATCH 005/125] Takes care of Victor's comments on the PR. Note, the students_lc list is used in _do_enroll_students when overload = True so it needs to be left in. --- .../instructor/tests/test_enrollment.py | 13 +++++++-- lms/djangoapps/instructor/views.py | 29 ++++++++++++------- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 775f39f442..5c4cbd4ec5 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -1,6 +1,5 @@ ''' Unit tests for enrollment methods in views.py - ''' from django.test.utils import override_settings @@ -11,6 +10,7 @@ from courseware.tests.tests import LoginEnrollmentTestCase, TEST_DATA_XML_MODULE from xmodule.modulestore.django import modulestore import xmodule.modulestore.django from student.models import CourseEnrollment, CourseEnrollmentAllowed +from instructor.views import get_and_clean_student_list @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) @@ -68,7 +68,7 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): #Check the page output self.assertContains(response, 'student1@test.com') - self.assertContains(response, 'student1@test.com') + self.assertContains(response, 'student2@test.com') self.assertContains(response, 'un-enrolled') #Check the enrollment table @@ -166,3 +166,12 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): user = User.objects.get(email='test2_2@student.com') ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(0, len(ce)) + + def test_get_and_clean_student_list(self): + ''' + Clean user input test + ''' + + string = "abc@test.com, def@test.com ghi@test.com \n \n jkl@test.com " + cleaned_string, cleaned_string_lc = get_and_clean_student_list(string) + self.assertEqual(cleaned_string, ['abc@test.com', 'def@test.com', 'ghi@test.com', 'jkl@test.com']) \ No newline at end of file diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 756d1a6204..a16e5e84c4 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -542,7 +542,7 @@ def instructor_dashboard(request, course_id): elif action == 'Enroll multiple students': students = request.POST.get('multiple_students', '') - auto_enroll = request.POST.get('auto_enroll', False) is not False + auto_enroll = bool(request.POST.get('auto_enroll')) ret = _do_enroll_students(course, course_id, students, auto_enroll=auto_enroll) datatable = ret['datatable'] @@ -1000,11 +1000,12 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll if cea: cea[0].auto_enroll = auto_enroll cea[0].save() - status[student] = 'user does not exist, enrollment already allowed, pending with auto enrollment ' + ("off", "on")[auto_enroll] + status[student] = 'user does not exist, enrollment already allowed, pending with auto enrollment ' \ + + ('on' if auto_enroll else 'off') continue cea = CourseEnrollmentAllowed(email=student, course_id=course_id, auto_enroll=auto_enroll) cea.save() - status[student] = 'user does not exist, enrollment allowed, pending with auto enrollment ' + ("off", "on")[auto_enroll] + status[student] = 'user does not exist, enrollment allowed, pending with auto enrollment ' + ('on' if auto_enroll else 'off') continue if CourseEnrollment.objects.filter(user=user, course_id=course_id): @@ -1018,7 +1019,7 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll status[student] = 'rejected' datatable = {'header': ['StudentEmail', 'action']} - datatable['data'] = [[x, status[x]] for x in status] + datatable['data'] = [[x, status[x]] for x in sorted(status)] datatable['title'] = 'Enrollment of students' def sf(stat): @@ -1050,7 +1051,7 @@ def _do_unenroll_students(course_id, students): try: user = User.objects.get(email=student) - except User.DoesNotExist: + except User.DoesNotExist: continue nce = CourseEnrollment.objects.filter(user=user, course_id=course_id) @@ -1064,21 +1065,29 @@ def _do_unenroll_students(course_id, students): status[student] = "Error! Failed to un-enroll" datatable = {'header': ['StudentEmail', 'action']} - datatable['data'] = [[x, status[x]] for x in status] + datatable['data'] = [[x, status[x]] for x in sorted(status)] datatable['title'] = 'Un-enrollment of students' data = dict(datatable=datatable) return data + def get_and_clean_student_list(students): - + """ + Separate out individual student email from the comma, or space separated string. + + In: + students: string coming from the input text area + Return: + students: list of cleaned student emails + students_lc: list of lower case cleaned student emails + """ + students = split_by_comma_and_whitespace(students) students = [str(s.strip()) for s in students] + students = [s for s in students if s != ''] students_lc = [x.lower() for x in students] - if '' in students: - students.remove('') - return students, students_lc #----------------------------------------------------------------------------- From 67356cdcc816f2cec579c69bd44446e0dba6eb25 Mon Sep 17 00:00:00 2001 From: dcadams Date: Wed, 15 May 2013 09:54:53 -0700 Subject: [PATCH 006/125] Merge of master to get Jenkins running. --- lms/djangoapps/instructor/tests/test_enrollment.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 5c4cbd4ec5..1136b7cd04 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -1,5 +1,6 @@ ''' Unit tests for enrollment methods in views.py + ''' from django.test.utils import override_settings From 1ea65455565f2369c220c0a9ce4dfa5376740411 Mon Sep 17 00:00:00 2001 From: dcadams Date: Tue, 4 Jun 2013 14:13:13 -0700 Subject: [PATCH 007/125] Worked on reducing pep8 violations. --- common/djangoapps/student/views.py | 28 ++++----- .../instructor/tests/test_enrollment.py | 4 +- lms/djangoapps/instructor/views.py | 61 +++++++++---------- 3 files changed, 42 insertions(+), 51 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index c74650b7f4..fd0f643af0 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -264,7 +264,6 @@ def dashboard(request): if not user.is_active: message = render_to_string('registration/activate_account_notice.html', {'email': user.email}) - # Global staff can see what courses errored on their dashboard staff_access = False errored_courses = {} @@ -355,7 +354,7 @@ def change_enrollment(request): course = course_from_id(course_id) except ItemNotFoundError: log.warning("User {0} tried to enroll in non-existent course {1}" - .format(user.username, course_id)) + .format(user.username, course_id)) return HttpResponseBadRequest("Course id is invalid") if not has_access(user, course, 'enroll'): @@ -363,9 +362,9 @@ def change_enrollment(request): org, course_num, run = course_id.split("/") statsd.increment("common.student.enrollment", - tags=["org:{0}".format(org), - "course:{0}".format(course_num), - "run:{0}".format(run)]) + tags=["org:{0}".format(org), + "course:{0}".format(course_num), + "run:{0}".format(run)]) try: enrollment, created = CourseEnrollment.objects.get_or_create(user=user, course_id=course.id) @@ -382,9 +381,9 @@ def change_enrollment(request): org, course_num, run = course_id.split("/") statsd.increment("common.student.unenrollment", - tags=["org:{0}".format(org), - "course:{0}".format(course_num), - "run:{0}".format(run)]) + tags=["org:{0}".format(org), + "course:{0}".format(course_num), + "run:{0}".format(run)]) return HttpResponse() except CourseEnrollment.DoesNotExist: @@ -454,7 +453,6 @@ def login_user(request, error=""): expires_time = time.time() + max_age expires = cookie_date(expires_time) - response.set_cookie(settings.EDXMKTG_COOKIE_NAME, 'true', max_age=max_age, expires=expires, domain=settings.SESSION_COOKIE_DOMAIN, @@ -515,8 +513,8 @@ def _do_create_account(post_vars): Note: this function is also used for creating test users. """ user = User(username=post_vars['username'], - email=post_vars['email'], - is_active=False) + email=post_vars['email'], + is_active=False) user.set_password(post_vars['password']) registration = Registration() # TODO: Rearrange so that if part of the process fails, the whole process fails. @@ -698,7 +696,6 @@ def create_account(request, post_override=None): expires_time = time.time() + max_age expires = cookie_date(expires_time) - response.set_cookie(settings.EDXMKTG_COOKIE_NAME, 'true', max_age=max_age, expires=expires, domain=settings.SESSION_COOKIE_DOMAIN, @@ -708,7 +705,6 @@ def create_account(request, post_override=None): return response - def exam_registration_info(user, course): """ Returns a Registration object if the user is currently registered for a current exam of the course. Returns None if the user is not registered, or if there is no @@ -849,7 +845,6 @@ def create_exam_registration(request, post_override=None): response_data['non_field_errors'] = form.non_field_errors() return HttpResponse(json.dumps(response_data), mimetype="application/json") - # only do the following if there is accommodation text to send, # and a destination to which to send it. # TODO: still need to create the accommodation email templates @@ -872,7 +867,6 @@ def create_exam_registration(request, post_override=None): # response_data['non_field_errors'] = [ 'Could not send accommodation e-mail.', ] # return HttpResponse(json.dumps(response_data), mimetype="application/json") - js = {'success': True} return HttpResponse(json.dumps(js), mimetype="application/json") @@ -916,7 +910,7 @@ def activate_account(request, key): if not r[0].user.is_active: r[0].activate() already_active = False - + #Enroll student in any pending courses he/she may have if auto_enroll flag is set student = request.user ceas = CourseEnrollmentAllowed.objects.filter(email=student.email) @@ -924,7 +918,7 @@ def activate_account(request, key): if cea.auto_enroll: course_id = cea.course_id enrollment, created = CourseEnrollment.objects.get_or_create(user_id=student.id, course_id=course_id) - + resp = render_to_response("registration/activation_complete.html", {'user_logged_in': user_logged_in, 'already_active': already_active}) return resp if len(r) == 0: diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index 1136b7cd04..fcbe1381e6 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -172,7 +172,7 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): ''' Clean user input test ''' - + string = "abc@test.com, def@test.com ghi@test.com \n \n jkl@test.com " cleaned_string, cleaned_string_lc = get_and_clean_student_list(string) - self.assertEqual(cleaned_string, ['abc@test.com', 'def@test.com', 'ghi@test.com', 'jkl@test.com']) \ No newline at end of file + self.assertEqual(cleaned_string, ['abc@test.com', 'def@test.com', 'ghi@test.com', 'jkl@test.com']) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 795865efe2..d46af2269d 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -9,7 +9,6 @@ import os import re import requests from requests.status_codes import codes -import urllib from collections import OrderedDict from StringIO import StringIO @@ -230,13 +229,13 @@ def instructor_dashboard(request, course_id): if student_to_reset is not None: # find the module in question if '/' not in problem_to_reset: # allow state of modules other than problem to be reset - problem_to_reset = "problem/" + problem_to_reset # but problem is the default + problem_to_reset = "problem/" + problem_to_reset # but problem is the default try: (org, course_name, _) = course_id.split("/") module_state_key = "i4x://" + org + "/" + course_name + "/" + problem_to_reset module_to_reset = StudentModule.objects.get(student_id=student_to_reset.id, - course_id=course_id, - module_state_key=module_state_key) + course_id=course_id, + module_state_key=module_state_key) msg += "Found module to reset. " except Exception: msg += "Couldn't find module with that urlname. " @@ -260,19 +259,18 @@ def instructor_dashboard(request, course_id): module_to_reset.state = json.dumps(problem_state) module_to_reset.save() track.views.server_track(request, - '{instructor} reset attempts from {old_attempts} to 0 for {student} on problem {problem} in {course}'.format( - old_attempts=old_number_of_attempts, - student=student_to_reset, - problem=module_to_reset.module_state_key, - instructor=request.user, - course=course_id), - {}, - page='idashboard') + '{instructor} reset attempts from {old_attempts} to 0 for {student} on problem {problem} in {course}'.format( + old_attempts=old_number_of_attempts, + student=student_to_reset, + problem=module_to_reset.module_state_key, + instructor=request.user, + course=course_id), + {}, + page='idashboard') msg += "Module state successfully reset!" except: msg += "Couldn't reset module state. " - elif "Get link to student's progress page" in action: unique_student_identifier = request.POST.get('unique_student_identifier', '') try: @@ -282,12 +280,12 @@ def instructor_dashboard(request, course_id): student_to_reset = User.objects.get(username=unique_student_identifier) progress_url = reverse('student_progress', kwargs={'course_id': course_id, 'student_id': student_to_reset.id}) track.views.server_track(request, - '{instructor} requested progress page for {student} in {course}'.format( - student=student_to_reset, - instructor=request.user, - course=course_id), - {}, - page='idashboard') + '{instructor} requested progress page for {student} in {course}'.format( + student=student_to_reset, + instructor=request.user, + course=course_id), + {}, + page='idashboard') msg += " Progress page for username: {1} with email address: {2}.".format(progress_url, student_to_reset.username, student_to_reset.email) except: msg += "Couldn't find student with that username. " @@ -315,6 +313,7 @@ def instructor_dashboard(request, course_id): msg2, rg_stud_data = _do_remote_gradebook(request.user, course, 'get-membership') datatable = {'header': ['Student email', 'Match?']} rg_students = [x['email'] for x in rg_stud_data['retdata']] + def domatch(x): return 'yes' if x.email in rg_students else 'No' datatable['data'] = [[x.email, domatch(x)] for x in stud_data['students']] @@ -350,7 +349,6 @@ def instructor_dashboard(request, course_id): msg2, _ = _do_remote_gradebook(request.user, course, 'post-grades', files=files) msg += msg2 - #---------------------------------------- # Admin @@ -416,6 +414,7 @@ def instructor_dashboard(request, course_id): profkeys = ['name', 'language', 'location', 'year_of_birth', 'gender', 'level_of_education', 'mailing_address', 'goals'] datatable = {'header': ['username', 'email'] + profkeys} + def getdat(u): p = u.profile return [u.username, u.email] + [getattr(p, x, '') for x in profkeys] @@ -424,9 +423,8 @@ def instructor_dashboard(request, course_id): datatable['title'] = 'Student profile data for course %s' % course_id return return_csv('profiledata_%s.csv' % course_id, datatable) - elif 'Download CSV of all responses to problem' in action: - problem_to_dump = request.POST.get('problem_to_dump','') + problem_to_dump = request.POST.get('problem_to_dump', '') if problem_to_dump[-4:] == ".xml": problem_to_dump = problem_to_dump[:-4] @@ -444,7 +442,7 @@ def instructor_dashboard(request, course_id): if smdat: datatable = {'header': ['username', 'state']} - datatable['data'] = [ [x.student.username, x.state] for x in smdat ] + datatable['data'] = [[x.student.username, x.state] for x in smdat] datatable['title'] = 'Student state for problem %s' % problem_to_dump return return_csv('student_state_from_%s.csv' % problem_to_dump, datatable) @@ -481,7 +479,6 @@ def instructor_dashboard(request, course_id): msg += _list_course_forum_members(course_id, rolename, datatable) track.views.server_track(request, 'list-{0}'.format(rolename), {}, page='idashboard') - elif action == 'Remove forum admin': uname = request.POST['forumadmin'] msg += _update_forum_role_membership(uname, course, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_REMOVE) @@ -571,7 +568,6 @@ def instructor_dashboard(request, course_id): ret = _do_enroll_students(course, course_id, students, overload=overload) datatable = ret['datatable'] - #---------------------------------------- # psychometrics @@ -591,9 +587,9 @@ def instructor_dashboard(request, course_id): logs and swallows errors. """ url = settings.ANALYTICS_SERVER_URL + \ - "get?aname={}&course_id={}&apikey={}".format(analytics_name, - course_id, - settings.ANALYTICS_API_KEY) + "get?aname={}&course_id={}&apikey={}".format(analytics_name, + course_id, + settings.ANALYTICS_API_KEY) try: res = requests.get(url) except Exception: @@ -652,7 +648,7 @@ def instructor_dashboard(request, course_id): 'cohorts_ajax_url': reverse('cohorts', kwargs={'course_id': course_id}), 'analytics_results': analytics_results, - } + } return render_to_response('courseware/instructor_dashboard.html', context) @@ -815,7 +811,7 @@ def _add_or_remove_user_group(request, username_or_email, group, group_title, ev action = "Added" if do_add else "Removed" prep = "to" if do_add else "from" msg = '{action} {0} {prep} {1} group = {2}'.format(user, group_title, group.name, - action=action, prep=prep) + action=action, prep=prep) if do_add: user.groups.add(group) else: @@ -941,7 +937,7 @@ def gradebook(request, course_id): 'grade_summary': student_grades(student, request, course), 'realname': student.profile.name, } - for student in enrolled_students] + for student in enrolled_students] return render_to_response('courseware/gradebook.html', { 'students': student_info, @@ -1093,6 +1089,7 @@ def get_and_clean_student_list(students): #----------------------------------------------------------------------------- # answer distribution + def get_answers_distribution(request, course_id): """ Get the distribution of answers for all graded problems in the course. @@ -1184,5 +1181,5 @@ def dump_grading_context(course): msg += " %s (format=%s, Assignment=%s%s)\n" % (s.display_name, format, aname, notes) msg += "all descriptors:\n" msg += "length=%d\n" % len(gc['all_descriptors']) - msg = '

%s
' % msg.replace('<','<') + msg = '
%s
' % msg.replace('<', '<') return msg From a8ec1ca9d5804823772a6634cbf2cbee6bb8f426 Mon Sep 17 00:00:00 2001 From: dcadams Date: Tue, 4 Jun 2013 17:08:34 -0700 Subject: [PATCH 008/125] Modified code such that non-logged in student activates correctly. --- common/djangoapps/student/views.py | 13 +++++++------ lms/djangoapps/instructor/tests/test_enrollment.py | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index fd0f643af0..661b1da26b 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -912,12 +912,13 @@ def activate_account(request, key): already_active = False #Enroll student in any pending courses he/she may have if auto_enroll flag is set - student = request.user - ceas = CourseEnrollmentAllowed.objects.filter(email=student.email) - for cea in ceas: - if cea.auto_enroll: - course_id = cea.course_id - enrollment, created = CourseEnrollment.objects.get_or_create(user_id=student.id, course_id=course_id) + student = User.objects.filter(id=r[0].user_id) + if student: + ceas = CourseEnrollmentAllowed.objects.filter(email=student[0].email) + for cea in ceas: + if cea.auto_enroll: + course_id = cea.course_id + enrollment, created = CourseEnrollment.objects.get_or_create(user_id=student[0].id, course_id=course_id) resp = render_to_response("registration/activation_complete.html", {'user_logged_in': user_logged_in, 'already_active': already_active}) return resp diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index fcbe1381e6..ce5f2d2e50 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -81,7 +81,7 @@ class TestInstructorEnrollsStudent(LoginEnrollmentTestCase): ce = CourseEnrollment.objects.filter(course_id=course.id, user=user) self.assertEqual(0, len(ce)) - def test_enrollmemt_new_student_autoenroll_on(self): + def test_enrollment_new_student_autoenroll_on(self): ''' Do auto-enroll on test ''' From 16fc7b37fb2da07ea26db98139a0c2094235a290 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 5 Jun 2013 11:12:47 -0400 Subject: [PATCH 009/125] Make xmodule_assets incremental, rather than removing the entire generated asset tree --- common/lib/xmodule/xmodule/static_content.py | 58 +++++++++++++------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/common/lib/xmodule/xmodule/static_content.py b/common/lib/xmodule/xmodule/static_content.py index f433a9d6c9..8929ddebd4 100755 --- a/common/lib/xmodule/xmodule/static_content.py +++ b/common/lib/xmodule/xmodule/static_content.py @@ -58,7 +58,12 @@ def _ensure_dir(dir_): def _write_styles(selector, output_root, classes): - _ensure_dir(output_root) + """ + Write the css fragments from all XModules in `classes` + into `output_root` as individual files, hashed by the contents to remove + duplicates + """ + contents = {} css_fragments = defaultdict(set) for class_ in classes: @@ -73,25 +78,34 @@ def _write_styles(selector, output_root, classes): hash=hashlib.md5(fragment).hexdigest(), type=filetype) # Prepend _ so that sass just includes the files into a single file - with open(output_root / '_' + fragment_name, 'w') as css_file: - css_file.write(fragment) + filename = '_' + fragment_name + contents[filename] = fragment for class_ in classes: css_imports[class_].add(fragment_name) - with open(output_root / '_module-styles.scss', 'w') as module_styles: + module_styles_lines = [] + module_styles_lines.append("@import 'bourbon/bourbon';") + module_styles_lines.append("@import 'bourbon/addons/button';") + for class_, fragment_names in css_imports.items(): + module_styles_lines.append("""{selector}.xmodule_{class_} {{""".format( + class_=class_, selector=selector + )) + module_styles_lines.extend(' @import "{0}";'.format(name) for name in fragment_names) + module_styles_lines.append('}') - module_styles.write("@import 'bourbon/bourbon';\n") - module_styles.write("@import 'bourbon/addons/button';\n") - for class_, fragment_names in css_imports.items(): - imports = "\n".join('@import "{0}";'.format(name) for name in fragment_names) - module_styles.write("""{selector}.xmodule_{class_} {{ {imports} }}\n""".format( - class_=class_, imports=imports, selector=selector - )) + contents['_module-styles.scss'] = '\n'.join(module_styles_lines) + + _write_files(output_root, contents) def _write_js(output_root, classes): - _ensure_dir(output_root) + """ + Write the javascript fragments from all XModules in `classes` + into `output_root` as individual files, hashed by the contents to remove + duplicates + """ + contents = {} js_fragments = set() for class_ in classes: @@ -100,18 +114,25 @@ def _write_js(output_root, classes): for idx, fragment in enumerate(module_js.get(filetype, [])): js_fragments.add((idx, filetype, fragment)) - module_js = [] for idx, filetype, fragment in sorted(js_fragments): - path = output_root / "{idx:0=3d}-{hash}.{type}".format( + filename = "{idx:0=3d}-{hash}.{type}".format( idx=idx, hash=hashlib.md5(fragment).hexdigest(), type=filetype) - with open(path, 'w') as js_file: - js_file.write(fragment) + contents[filename] = fragment - module_js.append(path) + _write_files(output_root, contents) - return module_js + return [output_root / filename for filename in contents.keys()] + + +def _write_files(output_root, contents): + _ensure_dir(output_root) + for extra_file in set(output_root.files()) - set(contents.keys()): + extra_file.remove() + + for filename, file_content in contents.iteritems(): + (output_root / filename).write_bytes(file_content) def main(): @@ -122,7 +143,6 @@ def main(): args = docopt(main.__doc__) root = path(args['']) - root.rmtree(ignore_errors=True) write_descriptor_js(root / 'descriptors/js') write_descriptor_styles(root / 'descriptors/css') write_module_js(root / 'modules/js') From fdf213f9b5bc5b2f50c110d0ffe5ffb4f833d7b1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 5 Jun 2013 13:04:20 -0400 Subject: [PATCH 010/125] Compile all of the coffee and sass in the project, not just the files in */static --- rakefiles/assets.rake | 44 ++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/rakefiles/assets.rake b/rakefiles/assets.rake index a3564c2650..009c87048c 100644 --- a/rakefiles/assets.rake +++ b/rakefiles/assets.rake @@ -10,10 +10,11 @@ def xmodule_cmd(watch=false, debug=false) xmodule_cmd = 'xmodule_assets common/static/xmodule' if watch "watchmedo shell-command " + - "--patterns='*.js;*.coffee;*.sass;*.scss;*.css' " + - "--recursive " + - "--command='#{xmodule_cmd}' " + - "common/lib/xmodule" + "--patterns='*.js;*.coffee;*.sass;*.scss;*.css' " + + "--recursive " + + "--command='#{xmodule_cmd}' " + + "--wait " + + "common/lib/xmodule" else xmodule_cmd end @@ -27,15 +28,16 @@ def coffee_cmd(watch=false, debug=false) # # Ref: https://github.com/joyent/node/issues/2479 # - # Rather than watching all of the directories in one command - # watch each static files subdirectory separately - cmds = [] - ['lms/static/coffee', 'cms/static/coffee', 'common/static/coffee', 'common/static/xmodule'].each do |coffee_folder| - cmds << "node_modules/.bin/coffee --watch --compile #{coffee_folder}" - end - cmds + # So, instead, we use watchmedo, which works around the problem + "watchmedo shell-command " + + "--command 'node_modules/.bin/coffee -c ${watch_src_path}' " + + "--recursive " + + "--patterns '*.coffee' " + + "--ignore-directories " + + "--wait " + + "." else - 'node_modules/.bin/coffee --compile */static' + 'node_modules/.bin/coffee --compile .' end end @@ -75,8 +77,8 @@ namespace :assets do $stdin.gets end - {:xmodule => :install_python_prereqs, - :coffee => :install_node_prereqs, + {:xmodule => [:install_python_prereqs], + :coffee => [:install_node_prereqs], :sass => [:install_ruby_prereqs, :preprocess]}.each_pair do |asset_type, prereq_tasks| desc "Compile all #{asset_type} assets" task asset_type => prereq_tasks do @@ -105,7 +107,8 @@ namespace :assets do $stdin.gets end - task :_watch => prereq_tasks do + # Fully compile before watching for changes + task :_watch => (prereq_tasks + ["assets:#{asset_type}:debug"]) do cmd = send(asset_type.to_s + "_cmd", watch=true, debug=true) if cmd.kind_of?(Array) cmd.each {|c| background_process(c)} @@ -118,19 +121,18 @@ namespace :assets do multitask :sass => 'assets:xmodule' namespace :sass do - # In watch mode, sass doesn't immediately compile out of date files, - # so force a recompile first - # Also force xmodule files to be generated before we start watching anything - task :_watch => ['assets:sass:debug', 'assets:xmodule'] multitask :debug => 'assets:xmodule:debug' end multitask :coffee => 'assets:xmodule' namespace :coffee do - # Force xmodule files to be generated before we start watching anything - task :_watch => 'assets:xmodule' multitask :debug => 'assets:xmodule:debug' end + + namespace :xmodule do + # Only start the xmodule watcher after the coffee and sass watchers have already started + task :_watch => ['assets:coffee:_watch', 'assets:sass:_watch'] + end end # This task does the real heavy lifting to gather all of the static From 0bf7c71ec2880dce39df9f1a5710d5e78a63d5c7 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 5 Jun 2013 13:05:00 -0400 Subject: [PATCH 011/125] Run all tests on jenkins We used to specify specific rake test tasks so that we could run all of them even if early ones failed. However, that meant that as new tasks were added, they weren't being run on jenkins. Now, there is a facility in the rake scripts so that tests can run using the test_sh function, which will delay failure until the end of the rake run, unless the TESTS_FAIL_FAST environment variable is set. Furthermore, this reorganizes the jasmine test tasks so that we can run those as part of `rake test` as well. --- doc/testing.md | 33 ++++++--- jenkins/test.sh | 16 +---- rakefiles/deprecated.rake | 23 ++++++ rakefiles/helpers.rb | 34 ++++++++- rakefiles/jasmine.rake | 143 ++++++++++++++++++++------------------ rakefiles/tests.rake | 36 +++------- 6 files changed, 168 insertions(+), 117 deletions(-) create mode 100644 rakefiles/deprecated.rake diff --git a/doc/testing.md b/doc/testing.md index 4d286b1bcc..c806b4e4ce 100644 --- a/doc/testing.md +++ b/doc/testing.md @@ -141,21 +141,36 @@ Very handy: if you uncomment the `pdb=1` line in `setup.cfg`, it will drop you i ### Running Javascript Unit Tests -These commands start a development server with jasmine testing enabled, and launch your default browser -pointing to those tests +To run all of the javascript unit tests, use - rake browse_jasmine_{lms,cms} + rake jasmine -To run the tests headless, you must install [phantomjs](http://phantomjs.org/download.html), then run: +If the `phantomjs` binary is on the path, or the `PHANTOMJS_PATH` environment variable is +set to point to it, then the tests will be run headless. Otherwise, they will be run in +your default browser - rake phantomjs_jasmine_{lms,cms} + export PATH=/path/to/phantomjs:$PATH + rake jasmine # Runs headless -If the `phantomjs` binary is not on the path, set the `PHANTOMJS_PATH` environment variable to point to it +or - PHANTOMJS_PATH=/path/to/phantomjs rake phantomjs_jasmine_{lms,cms} + PHANTOMJS_PATH=/path/to/phantomjs rake jasmine # Runs headless -Once you have run the `rake` command, your browser should open to -to `http://localhost/_jasmine/`, which displays the test results. +or + + rake jasmine # Runs in browser + +You can also force a run using phantomjs or the browser using the commands + + rake jasmine:browser # Runs in browser + rake jasmine:phantomjs # Runs headless + +You can run tests for a specific subsystems as well + + rake jasmine:lms # Runs all lms javascript unit tests using the default method + rake jasmine:cms:browser # Runs all cms javascript unit tests in the browser + +Use `rake -T` to get a list of all available subsystems **Troubleshooting**: If you get an error message while running the `rake` task, try running `bundle install` to install the required ruby gems. diff --git a/jenkins/test.sh b/jenkins/test.sh index 35be3a0121..8c4df9fac3 100755 --- a/jenkins/test.sh +++ b/jenkins/test.sh @@ -70,23 +70,11 @@ rake clobber rake pep8 > pep8.log || cat pep8.log rake pylint > pylint.log || cat pylint.log -TESTS_FAILED=0 - -# Run the python unit tests -rake test_cms || TESTS_FAILED=1 -rake test_lms || TESTS_FAILED=1 -rake test_common/lib/capa || TESTS_FAILED=1 -rake test_common/lib/xmodule || TESTS_FAILED=1 - -# Run the javascript unit tests -rake phantomjs_jasmine_lms || TESTS_FAILED=1 -rake phantomjs_jasmine_cms || TESTS_FAILED=1 -rake phantomjs_jasmine_common/lib/xmodule || TESTS_FAILED=1 -rake phantomjs_jasmine_common/static/coffee || TESTS_FAILED=1 +# Run the unit tests (use phantomjs for javascript unit tests) +rake test rake coverage:xml coverage:html -[ $TESTS_FAILED == '0' ] rake autodeploy_properties github_status state:success "passed" diff --git a/rakefiles/deprecated.rake b/rakefiles/deprecated.rake new file mode 100644 index 0000000000..00c1987bd5 --- /dev/null +++ b/rakefiles/deprecated.rake @@ -0,0 +1,23 @@ + +require 'colorize' + +def deprecated(deprecated, deprecated_by) + task deprecated do + puts("Task #{deprecated} has been deprecated. Use #{deprecated_by} instead. Waiting 5 seconds...".red) + sleep(5) + Rake::Task[deprecated_by].invoke + end +end + +[:lms, :cms].each do |system| + deprecated("browse_jasmine_#{system}", "jasmine:#{system}:browser") + deprecated("phantomjs_jasmine_#{system}", "jasmine:#{system}:phantomjs") +end + +Dir["common/lib/*"].select{|lib| File.directory?(lib)}.each do |lib| + deprecated("browse_jasmine_#{lib}", "jasmine:#{lib}:browser") + deprecated("phantomjs_jasmine_#{lib}", "jasmine:#{lib}:phantomjs") +end + +deprecated("browse_jasmine_discussion", "jasmine:common/static/coffee:browser") +deprecated("phantomjs_jasmine_discussion", "jasmine:common/static/coffee:phantomjs") \ No newline at end of file diff --git a/rakefiles/helpers.rb b/rakefiles/helpers.rb index f344aa2042..4b10bef709 100644 --- a/rakefiles/helpers.rb +++ b/rakefiles/helpers.rb @@ -1,8 +1,12 @@ require 'digest/md5' +def find_executable(exec) + path = %x(which #{exec}).strip + $?.exitstatus == 0 ? path : nil +end def select_executable(*cmds) - cmds.find_all{ |cmd| system("which #{cmd} > /dev/null 2>&1") }[0] || fail("No executables found from #{cmds.join(', ')}") + cmds.find_all{ |cmd| !find_executable(cmd).nil? }[0] || fail("No executables found from #{cmds.join(', ')}") end def django_admin(system, env, command, *args) @@ -85,3 +89,31 @@ def environments(system) env_file.gsub("#{system}/envs/", '').gsub(/\.py/, '').gsub('/', '.') end end + +$failed_tests = 0 + +# Run sh on args. If TESTS_FAIL_FAST is set, then stop on the first shell failure. +# Otherwise, a final task will be added that will fail if any tests have failed +def test_sh(*args) + sh(*args) do |ok, res| + if ok + return + end + + if ENV['TESTS_FAIL_FAST'] + fail("Test failed!") + else + $failed_tests += 1 + end + end +end + +# Add a task after all other tasks that fails if any tests have failed +if !ENV['TESTS_FAIL_FAST'] + task :fail_tests do + fail("#{$failed_tests} tests failed!") if $failed_tests > 0 + end + + Rake.application.top_level_tasks << :fail_tests +end + diff --git a/rakefiles/jasmine.rake b/rakefiles/jasmine.rake index bd1c7e5d6c..ab3209c9ec 100644 --- a/rakefiles/jasmine.rake +++ b/rakefiles/jasmine.rake @@ -3,6 +3,11 @@ require 'erb' require 'launchy' require 'net/http' +PHANTOMJS_PATH = find_executable(ENV['PHANTOMJS_PATH'] || 'phantomjs') +PREFERRED_METHOD = PHANTOMJS_PATH.nil? ? 'browser' : 'phantomjs' +if PHANTOMJS_PATH.nil? + puts("phantomjs not found on path. Set $PHANTOMJS_PATH. Using browser for jasmine tests".blue) +end def django_for_jasmine(system, django_reload) if !django_reload @@ -35,18 +40,6 @@ def django_for_jasmine(system, django_reload) end def template_jasmine_runner(lib) - case lib - when /common\/lib\/.+/ - coffee_files = Dir["#{lib}/**/js/**/*.coffee", "common/static/coffee/src/**/*.coffee"] - when /common\/static\/coffee/ - coffee_files = Dir["#{lib}/**/*.coffee"] - else - puts('I do not know how to run jasmine tests for #{lib}') - exit - end - if !coffee_files.empty? - sh("node_modules/.bin/coffee -c #{coffee_files.join(' ')}") - end phantom_jasmine_path = File.expand_path("node_modules/phantom-jasmine") jasmine_reporters_path = File.expand_path("node_modules/jasmine-reporters") common_js_root = File.expand_path("common/static/js") @@ -54,8 +47,8 @@ def template_jasmine_runner(lib) # Get arrays of spec and source files, ordered by how deep they are nested below the library # (and then alphabetically) and expanded from a relative to an absolute path - spec_glob = File.join("#{lib}", "**", "spec", "**", "*.js") - src_glob = File.join("#{lib}", "**", "src", "**", "*.js") + spec_glob = File.join(lib, "**", "spec", "**", "*.js") + src_glob = File.join(lib, "**", "src", "**", "*.js") js_specs = Dir[spec_glob].sort_by {|p| [p.split('/').length, p]} .map {|f| File.expand_path(f)} js_source = Dir[src_glob].sort_by {|p| [p.split('/').length, p]} .map {|f| File.expand_path(f)} @@ -68,74 +61,90 @@ def template_jasmine_runner(lib) yield File.expand_path(template_output) end -def run_phantom_js(url) - phantomjs = ENV['PHANTOMJS_PATH'] || 'phantomjs' - sh("#{phantomjs} node_modules/jasmine-reporters/test/phantomjs-testrunner.js #{url}") +def jasmine_browser(url, wait=10) + # Jitter starting the browser so that the tests don't all try and + # start the browser simultaneously + sleep(rand(3)) + sh("python -m webbrowser -t '#{url}'") + sleep(wait) end -# Open jasmine tests for :system in the default browser. The :env -# should (always?) be 'jasmine', but it's passed as an arg so that -# the :assets dependency gets it. -# -# This task should be invoked via the wrapper below, so we don't -# include a description to keep it from showing up in rake -T. -task :browse_jasmine, [:system, :env] => :assets do |t, args| - django_for_jasmine(args.system, true) do |jasmine_url| - Launchy.open(jasmine_url) - puts "Press ENTER to terminate".red - $stdin.gets - end -end - -# Use phantomjs to run jasmine tests from the console. The :env -# should (always?) be 'jasmine', but it's passed as an arg so that -# the :assets dependency gets it. -# -# This task should be invoked via the wrapper below, so we don't -# include a description to keep it from showing up in rake -T. -task :phantomjs_jasmine, [:system, :env] => :assets do |t, args| - django_for_jasmine(args.system, false) do |jasmine_url| - run_phantom_js(jasmine_url) - end +def jasmine_phantomjs(url) + fail("phantomjs not found. Add it to your path, or set $PHANTOMJS_PATH") if PHANTOMJS_PATH.nil? + test_sh("#{PHANTOMJS_PATH} node_modules/jasmine-reporters/test/phantomjs-testrunner.js #{url}") end # Wrapper tasks for the real browse_jasmine and phantomjs_jasmine # tasks above. These have a nicer UI since there's no arg passing. [:lms, :cms].each do |system| - desc "Open jasmine tests for #{system} in your default browser" - task "browse_jasmine_#{system}" do - Rake::Task[:browse_jasmine].invoke(system, 'jasmine') - end + namespace :jasmine do + namespace system do + desc "Open jasmine tests for #{system} in your default browser" + task :browser do + Rake::Task[:assets].invoke(system, 'jasmine') + django_for_jasmine(system, true) do |jasmine_url| + jasmine_browser(jasmine_url) + end + end - desc "Use phantomjs to run jasmine tests for #{system} from the console" - task "phantomjs_jasmine_#{system}" do - Rake::Task[:phantomjs_jasmine].invoke(system, 'jasmine') + desc "Use phantomjs to run jasmine tests for #{system} from the console" + task :phantomjs do + Rake::Task[:assets].invoke(system, 'jasmine') + phantomjs = ENV['PHANTOMJS_PATH'] || 'phantomjs' + django_for_jasmine(system, false) do |jasmine_url| + jasmine_phantomjs(jasmine_url) + end + end + end + + desc "Run jasmine tests for #{system} using #{PREFERRED_METHOD}" + task system => "jasmine:#{system}:#{PREFERRED_METHOD}" + + task :phantomjs => "jasmine:#{system}:phantomjs" + multitask :browser => "jasmine:#{system}:browser" end end -STATIC_JASMINE_TESTS = Dir["common/lib/*"].select{|lib| File.directory?(lib)} -STATIC_JASMINE_TESTS << 'common/static/coffee' +static_js_dirs = Dir["common/lib/*"].select{|lib| File.directory?(lib)} +static_js_dirs << 'common/static/coffee' +static_js_dirs.select!{|lib| !Dir["#{lib}/**/spec"].empty?} -STATIC_JASMINE_TESTS.each do |lib| - desc "Open jasmine tests for #{lib} in your default browser" - task "browse_jasmine_#{lib}" do - template_jasmine_runner(lib) do |f| - sh("python -m webbrowser -t 'file://#{f}'") - puts "Press ENTER to terminate".red - $stdin.gets - end - end +static_js_dirs.each do |dir| + namespace :jasmine do + namespace dir do + desc "Open jasmine tests for #{dir} in your default browser" + task :browser do + # We need to use either CMS or LMS to preprocess files. Use LMS by default + Rake::Task['assets:coffee'].invoke('lms', 'jasmine') + template_jasmine_runner(dir) do |f| + jasmine_browser("file://#{f}") + end + end - desc "Use phantomjs to run jasmine tests for #{lib} from the console" - task "phantomjs_jasmine_#{lib}" do - template_jasmine_runner(lib) do |f| - run_phantom_js(f) + desc "Use phantomjs to run jasmine tests for #{dir} from the console" + task :phantomjs do + # We need to use either CMS or LMS to preprocess files. Use LMS by default + Rake::Task[:assets].invoke('lms', 'jasmine') + template_jasmine_runner(dir) do |f| + jasmine_phantomjs(f) + end + end end + + desc "Run jasmine tests for #{dir} using #{PREFERRED_METHOD}" + task dir => "jasmine:#{dir}:#{PREFERRED_METHOD}" + + task :phantomjs => "jasmine:#{dir}:phantomjs" + multitask :browser => "jasmine:#{dir}:browser" end end -desc "Open jasmine tests for discussion in your default browser" -task "browse_jasmine_discussion" => "browse_jasmine_common/static/coffee" +desc "Run all jasmine tests using #{PREFERRED_METHOD}" +task :jasmine => "jasmine:#{PREFERRED_METHOD}" -desc "Use phantomjs to run jasmine tests for discussion from the console" -task "phantomjs_jasmine_discussion" => "phantomjs_jasmine_common/static/coffee" +['phantomjs', 'browser'].each do |method| + desc "Run all jasmine tests using #{method}" + task "jasmine:#{method}" +end + +task :test => :jasmine diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index 448a482f04..592f777b39 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -1,9 +1,6 @@ - # Set up the clean and clobber tasks CLOBBER.include(REPORT_DIR, 'test_root/*_repo', 'test_root/staticfiles') -$failed_tests = 0 - def run_under_coverage(cmd, root) cmd0, cmd_rest = cmd.split(" ", 2) # We use "python -m coverage" so that the proper python will run the importable coverage @@ -17,12 +14,7 @@ def run_tests(system, report_dir, test_id=nil, stop_on_failure=true) dirs = Dir["common/djangoapps/*"] + Dir["#{system}/djangoapps/*"] test_id = dirs.join(' ') if test_id.nil? or test_id == '' cmd = django_admin(system, :test, 'test', '--logging-clear-handlers', test_id) - sh(run_under_coverage(cmd, system)) do |ok, res| - if !ok and stop_on_failure - abort "Test failed!" - end - $failed_tests += 1 unless ok - end + test_sh(run_under_coverage(cmd, system)) end def run_acceptance_tests(system, report_dir, harvest_args) @@ -38,7 +30,7 @@ def run_acceptance_tests(system, report_dir, harvest_args) end sh(django_admin(system, 'acceptance', 'syncdb', '--noinput')) sh(django_admin(system, 'acceptance', 'migrate', '--noinput')) - sh(django_admin(system, 'acceptance', 'harvest', '--debug-mode', '--tag -skip', harvest_args)) + test_sh(django_admin(system, 'acceptance', 'harvest', '--debug-mode', '--tag -skip', harvest_args)) end @@ -55,13 +47,13 @@ TEST_TASK_DIRS = [] # Per System tasks desc "Run all django tests on our djangoapps for the #{system}" - task "test_#{system}", [:test_id, :stop_on_failure] => ["clean_test_files", :predjango, "#{system}:gather_assets:test", "fasttest_#{system}"] + task "test_#{system}", [:test_id] => ["clean_test_files", :predjango, "#{system}:gather_assets:test", "fasttest_#{system}"] # Have a way to run the tests without running collectstatic -- useful when debugging without # messing with static files. - task "fasttest_#{system}", [:test_id, :stop_on_failure] => [report_dir, :install_prereqs, :predjango] do |t, args| - args.with_defaults(:stop_on_failure => 'true', :test_id => nil) - run_tests(system, report_dir, args.test_id, args.stop_on_failure) + task "fasttest_#{system}", [:test_id] => [report_dir, :install_prereqs, :predjango] do |t, args| + args.with_defaults(:test_id => nil) + run_tests(system, report_dir, args.test_id) end # Run acceptance tests @@ -89,9 +81,7 @@ Dir["common/lib/*"].select{|lib| File.directory?(lib)}.each do |lib| task task_name => report_dir do ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml") cmd = "nosetests #{lib}" - sh(run_under_coverage(cmd, lib)) do |ok, res| - $failed_tests += 1 unless ok - end + test_sh(run_under_coverage(cmd, lib)) end TEST_TASK_DIRS << lib @@ -107,17 +97,11 @@ TEST_TASK_DIRS.each do |dir| report_dir = report_dir_path(dir) directory report_dir task :report_dirs => [REPORT_DIR, report_dir] + task :test => "test_#{dir}" end -task :test do - TEST_TASK_DIRS.each do |dir| - Rake::Task["test_#{dir}"].invoke(nil, false) - end - - if $failed_tests > 0 - abort "Tests failed!" - end -end +desc "Run all tests" +task :test namespace :coverage do desc "Build the html coverage reports" From 3c498f66b39f105c5757c824f613cfb6608c608d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 23 May 2013 16:06:08 -0400 Subject: [PATCH 012/125] Add script to generate lists of changes made by all users in a release --- scripts/release-email-list.sh | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100755 scripts/release-email-list.sh diff --git a/scripts/release-email-list.sh b/scripts/release-email-list.sh new file mode 100755 index 0000000000..92f7a9aef4 --- /dev/null +++ b/scripts/release-email-list.sh @@ -0,0 +1,31 @@ +#! /bin/bash + +LOG_SPEC="$1..$2" +LOG_CMD="git --no-pager log $LOG_SPEC" + +RESPONSIBLE=$(sort -u <($LOG_CMD --format='tformat:%ae' && $LOG_CMD --format='tformat:%ce')) + +echo -n 'To: ' +echo ${RESPONSIBLE} | sed "s/ /, /g" +echo + +echo "You've made changes that are about to be released. All of the commits +that you either authored or committed are listed below. Please verify them on +\$ENVIRONMENT" +echo + +for EMAIL in $RESPONSIBLE; do + AUTHORED_BY="$LOG_CMD --author=<${EMAIL}>" + COMMITTED_BY="$LOG_CMD --committer=<${EMAIL}>" + COMMITTED_NOT_AUTHORED="$COMMITTED_BY $($AUTHORED_BY --format='tformat:^%h')" + + echo $EMAIL "authored the following commits:" + $AUTHORED_BY --format='tformat: %s - https://github.com/edx/edx-platform/commit/%h' + echo + + if [[ $($COMMITTED_NOT_AUTHORED) != "" ]]; then + echo $EMAIL "committed but didn't author the following commits:" + $COMMITTED_NOT_AUTHORED --format='tformat: %s - https://github.com/edx/edx-platform/commit/%h' + echo + fi +done \ No newline at end of file From acc743eea8861b8a5b5cda231d69e5caf61ceb4e Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 5 Jun 2013 15:08:58 -0400 Subject: [PATCH 013/125] Added functionality to bypass alerts This is done with the following steps: 'I confirm all alerts' means that all alert and confirm windows are returned and returned true respectively 'I dismiss all alerts' means that all confirm windows are returned false 'I answer all prompts with "([^"]*)"' means that all prompts are returned with the given string Please note that these settings are on a PER PAGE basis. This means that for best results, the step must be given right before the alert is generated. --- .../studio-overview-togglesection.feature | 26 +++++++++---------- common/djangoapps/terrain/steps.py | 15 +++++++++++ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature b/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature index c9f5b43dfb..3bdf2633d0 100644 --- a/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature +++ b/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature @@ -5,29 +5,27 @@ Feature: Overview Toggle Section Scenario: The default layout for the overview page is to show sections in expanded view Given I have a course with multiple sections - When I navigate to the course overview page - Then I see the "Collapse All Sections" link - And all sections are expanded + When I navigate to the course overview page + Then I see the "Collapse All Sections" link + And all sections are expanded Scenario: Expand /collapse for a course with no sections Given I have a course with no sections - When I navigate to the course overview page - Then I do not see the "Collapse All Sections" link + When I navigate to the course overview page + Then I do not see the "Collapse All Sections" link Scenario: Collapse link appears after creating first section of a course Given I have a course with no sections - When I navigate to the course overview page - And I add a section - Then I see the "Collapse All Sections" link - And all sections are expanded + When I navigate to the course overview page + And I add a section + Then I see the "Collapse All Sections" link + And all sections are expanded - # Skipped because Ubuntu ChromeDriver hangs on alert - @skip Scenario: Collapse link is not removed after last section of a course is deleted Given I have a course with 1 section - And I navigate to the course overview page - When I press the "section" delete icon - And I confirm the alert + And I navigate to the course overview page + When I confirm all alerts + And I press the "section" delete icon Then I see the "Collapse All Sections" link Scenario: Collapsing all sections when all sections are expanded diff --git a/common/djangoapps/terrain/steps.py b/common/djangoapps/terrain/steps.py index 1d9e59cd72..2fe9692ae4 100644 --- a/common/djangoapps/terrain/steps.py +++ b/common/djangoapps/terrain/steps.py @@ -159,3 +159,18 @@ def registered_edx_user(step, uname): @step(u'All dialogs should be closed$') def dialogs_are_closed(step): assert world.dialogs_closed() + + +@step('I confirm all alerts') +def i_confirm_with_ok(step): + world.browser.execute_script('window.confirm = function(){return true;} ; window.alert = function(){return;}') + + +@step('I dismiss all alerts') +def i_dismiss_with_ok(step): + world.browser.execute_script('window.confirm = function(){return false;}') + + +@step('I answer all prompts with "([^"]*)"') +def i_answer_prompts_with(step, prompt): + world.browser.execute_script('window.prompt = function(){return %s;}') % prompt From 0548b119a97b7d0cb63218ecdf35c4776d8c5831 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 5 Jun 2013 15:21:23 -0400 Subject: [PATCH 014/125] Fixed other scenarios that were skipped --- cms/djangoapps/contentstore/features/section.feature | 6 ++---- cms/djangoapps/contentstore/features/section.py | 4 ++-- cms/djangoapps/contentstore/features/subsection.feature | 6 ++---- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/features/section.feature b/cms/djangoapps/contentstore/features/section.feature index 236cf501fc..c738428a43 100644 --- a/cms/djangoapps/contentstore/features/section.feature +++ b/cms/djangoapps/contentstore/features/section.feature @@ -26,11 +26,9 @@ Feature: Create Section And I save a new section release date Then the section release date is updated - # Skipped because Ubuntu ChromeDriver hangs on alert - @skip Scenario: Delete section Given I have opened a new course in Studio And I have added a new section - When I press the "section" delete icon - And I confirm the alert + When I confirm all alerts + And I press the "section" delete icon Then the section does not exist diff --git a/cms/djangoapps/contentstore/features/section.py b/cms/djangoapps/contentstore/features/section.py index 9a896d8ebe..4a628ff72b 100644 --- a/cms/djangoapps/contentstore/features/section.py +++ b/cms/djangoapps/contentstore/features/section.py @@ -69,8 +69,8 @@ def i_see_complete_section_name_with_quote_in_editor(step): @step('the section does not exist$') def section_does_not_exist(step): - css = 'span.section-name-span' - assert world.browser.is_element_not_present_by_css(css) + css = 'h3[data-name="My Section"]' + assert world.is_css_not_present(css) @step('I see a release date for my section$') diff --git a/cms/djangoapps/contentstore/features/subsection.feature b/cms/djangoapps/contentstore/features/subsection.feature index 8bb12467ff..0873a3f428 100644 --- a/cms/djangoapps/contentstore/features/subsection.feature +++ b/cms/djangoapps/contentstore/features/subsection.feature @@ -32,12 +32,10 @@ Feature: Create Subsection And I reload the page Then I see the correct dates - # Skipped because Ubuntu ChromeDriver hangs on alert - @skip Scenario: Delete a subsection Given I have opened a new course section in Studio And I have added a new subsection And I see my subsection on the Courseware page - When I press the "subsection" delete icon - And I confirm the alert + When I confirm all alerts + And I press the "subsection" delete icon Then the subsection does not exist From d147103be0d5b02799fbd2a27e8b5ccc55915b27 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 5 Jun 2013 15:55:12 -0400 Subject: [PATCH 015/125] Fixed tab issue (tabs are now correctly spaces) --- .../studio-overview-togglesection.feature | 72 +++++++++---------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature b/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature index 3bdf2633d0..039faf0dd7 100644 --- a/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature +++ b/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature @@ -1,59 +1,59 @@ Feature: Overview Toggle Section - In order to quickly view the details of a course's section or to scan the inventory of sections + In order to quickly view the details of a course's section or to scan the inventory of sections As a course author I want to toggle the visibility of each section's subsection details in the overview listing - Scenario: The default layout for the overview page is to show sections in expanded view - Given I have a course with multiple sections + Scenario: The default layout for the overview page is to show sections in expanded view + Given I have a course with multiple sections When I navigate to the course overview page Then I see the "Collapse All Sections" link And all sections are expanded - Scenario: Expand /collapse for a course with no sections - Given I have a course with no sections + Scenario: Expand /collapse for a course with no sections + Given I have a course with no sections When I navigate to the course overview page Then I do not see the "Collapse All Sections" link - Scenario: Collapse link appears after creating first section of a course - Given I have a course with no sections + Scenario: Collapse link appears after creating first section of a course + Given I have a course with no sections When I navigate to the course overview page And I add a section Then I see the "Collapse All Sections" link And all sections are expanded - Scenario: Collapse link is not removed after last section of a course is deleted - Given I have a course with 1 section + Scenario: Collapse link is not removed after last section of a course is deleted + Given I have a course with 1 section And I navigate to the course overview page When I confirm all alerts And I press the "section" delete icon - Then I see the "Collapse All Sections" link + Then I see the "Collapse All Sections" link - Scenario: Collapsing all sections when all sections are expanded - Given I navigate to the courseware page of a course with multiple sections - And all sections are expanded - When I click the "Collapse All Sections" link - Then I see the "Expand All Sections" link - And all sections are collapsed + Scenario: Collapsing all sections when all sections are expanded + Given I navigate to the courseware page of a course with multiple sections + And all sections are expanded + When I click the "Collapse All Sections" link + Then I see the "Expand All Sections" link + And all sections are collapsed - Scenario: Collapsing all sections when 1 or more sections are already collapsed - Given I navigate to the courseware page of a course with multiple sections - And all sections are expanded - When I collapse the first section - And I click the "Collapse All Sections" link - Then I see the "Expand All Sections" link - And all sections are collapsed + Scenario: Collapsing all sections when 1 or more sections are already collapsed + Given I navigate to the courseware page of a course with multiple sections + And all sections are expanded + When I collapse the first section + And I click the "Collapse All Sections" link + Then I see the "Expand All Sections" link + And all sections are collapsed - Scenario: Expanding all sections when all sections are collapsed - Given I navigate to the courseware page of a course with multiple sections - And I click the "Collapse All Sections" link - When I click the "Expand All Sections" link - Then I see the "Collapse All Sections" link - And all sections are expanded + Scenario: Expanding all sections when all sections are collapsed + Given I navigate to the courseware page of a course with multiple sections + And I click the "Collapse All Sections" link + When I click the "Expand All Sections" link + Then I see the "Collapse All Sections" link + And all sections are expanded - Scenario: Expanding all sections when 1 or more sections are already expanded - Given I navigate to the courseware page of a course with multiple sections - And I click the "Collapse All Sections" link - When I expand the first section - And I click the "Expand All Sections" link - Then I see the "Collapse All Sections" link - And all sections are expanded + Scenario: Expanding all sections when 1 or more sections are already expanded + Given I navigate to the courseware page of a course with multiple sections + And I click the "Collapse All Sections" link + When I expand the first section + And I click the "Expand All Sections" link + Then I see the "Collapse All Sections" link + And all sections are expanded From 260659cfc6e913e99557d25e225970e52c42af1b Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 5 Jun 2013 15:56:45 -0400 Subject: [PATCH 016/125] Fixed wording in the step --- common/djangoapps/terrain/steps.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/terrain/steps.py b/common/djangoapps/terrain/steps.py index 2fe9692ae4..ce841cc3d8 100644 --- a/common/djangoapps/terrain/steps.py +++ b/common/djangoapps/terrain/steps.py @@ -166,9 +166,9 @@ def i_confirm_with_ok(step): world.browser.execute_script('window.confirm = function(){return true;} ; window.alert = function(){return;}') -@step('I dismiss all alerts') +@step('I cancel all alerts') def i_dismiss_with_ok(step): - world.browser.execute_script('window.confirm = function(){return false;}') + world.browser.execute_script('window.confirm = function(){return false;} ; window.alert = function(){return;}') @step('I answer all prompts with "([^"]*)"') From f652a5d8c4df3eb25cd06f75101ddf543a996872 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 5 Jun 2013 15:57:42 -0400 Subject: [PATCH 017/125] Small wording fix --- common/djangoapps/terrain/steps.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/terrain/steps.py b/common/djangoapps/terrain/steps.py index ce841cc3d8..78d9665cfd 100644 --- a/common/djangoapps/terrain/steps.py +++ b/common/djangoapps/terrain/steps.py @@ -162,12 +162,12 @@ def dialogs_are_closed(step): @step('I confirm all alerts') -def i_confirm_with_ok(step): +def i_confirm_all_alerts(step): world.browser.execute_script('window.confirm = function(){return true;} ; window.alert = function(){return;}') @step('I cancel all alerts') -def i_dismiss_with_ok(step): +def i_cancel_all_alerts(step): world.browser.execute_script('window.confirm = function(){return false;} ; window.alert = function(){return;}') From d226a21eb0508cded67c87cb4e285efed929df52 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 5 Jun 2013 16:01:45 -0400 Subject: [PATCH 018/125] Changed wording to I will --- cms/djangoapps/contentstore/features/section.feature | 2 +- .../features/studio-overview-togglesection.feature | 2 +- cms/djangoapps/contentstore/features/subsection.feature | 2 +- common/djangoapps/terrain/steps.py | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/features/section.feature b/cms/djangoapps/contentstore/features/section.feature index c738428a43..80ccb6cc7a 100644 --- a/cms/djangoapps/contentstore/features/section.feature +++ b/cms/djangoapps/contentstore/features/section.feature @@ -29,6 +29,6 @@ Feature: Create Section Scenario: Delete section Given I have opened a new course in Studio And I have added a new section - When I confirm all alerts + When I will confirm all alerts And I press the "section" delete icon Then the section does not exist diff --git a/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature b/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature index 039faf0dd7..e746f3629a 100644 --- a/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature +++ b/cms/djangoapps/contentstore/features/studio-overview-togglesection.feature @@ -24,7 +24,7 @@ Feature: Overview Toggle Section Scenario: Collapse link is not removed after last section of a course is deleted Given I have a course with 1 section And I navigate to the course overview page - When I confirm all alerts + When I will confirm all alerts And I press the "section" delete icon Then I see the "Collapse All Sections" link diff --git a/cms/djangoapps/contentstore/features/subsection.feature b/cms/djangoapps/contentstore/features/subsection.feature index 0873a3f428..a11467e3f9 100644 --- a/cms/djangoapps/contentstore/features/subsection.feature +++ b/cms/djangoapps/contentstore/features/subsection.feature @@ -36,6 +36,6 @@ Feature: Create Subsection Given I have opened a new course section in Studio And I have added a new subsection And I see my subsection on the Courseware page - When I confirm all alerts + When I will confirm all alerts And I press the "subsection" delete icon Then the subsection does not exist diff --git a/common/djangoapps/terrain/steps.py b/common/djangoapps/terrain/steps.py index 78d9665cfd..c126cae0ad 100644 --- a/common/djangoapps/terrain/steps.py +++ b/common/djangoapps/terrain/steps.py @@ -161,16 +161,16 @@ def dialogs_are_closed(step): assert world.dialogs_closed() -@step('I confirm all alerts') +@step('I will confirm all alerts') def i_confirm_all_alerts(step): world.browser.execute_script('window.confirm = function(){return true;} ; window.alert = function(){return;}') -@step('I cancel all alerts') +@step('I will cancel all alerts') def i_cancel_all_alerts(step): world.browser.execute_script('window.confirm = function(){return false;} ; window.alert = function(){return;}') -@step('I answer all prompts with "([^"]*)"') +@step('I will answer all prompts with "([^"]*)"') def i_answer_prompts_with(step, prompt): world.browser.execute_script('window.prompt = function(){return %s;}') % prompt From 79edee2d9a07b1cf0227ecb7be36bf7bbd2ae492 Mon Sep 17 00:00:00 2001 From: John Jarvis Date: Thu, 6 Jun 2013 09:50:43 -0400 Subject: [PATCH 019/125] read MKTG_URLS from env.json --- cms/envs/aws.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/envs/aws.py b/cms/envs/aws.py index af5f39631d..35b15fe6ba 100644 --- a/cms/envs/aws.py +++ b/cms/envs/aws.py @@ -103,6 +103,7 @@ DEFAULT_FROM_EMAIL = ENV_TOKENS.get('DEFAULT_FROM_EMAIL', DEFAULT_FROM_EMAIL) DEFAULT_FEEDBACK_EMAIL = ENV_TOKENS.get('DEFAULT_FEEDBACK_EMAIL', DEFAULT_FEEDBACK_EMAIL) ADMINS = ENV_TOKENS.get('ADMINS', ADMINS) SERVER_EMAIL = ENV_TOKENS.get('SERVER_EMAIL', SERVER_EMAIL) +MKTG_URLS = ENV_TOKENS.get('MKTG_URLS', MKTG_URLS) #Timezone overrides TIME_ZONE = ENV_TOKENS.get('TIME_ZONE', TIME_ZONE) From e2ee5145da06bd39966428cdac7658ab4e0f6048 Mon Sep 17 00:00:00 2001 From: John Jarvis Date: Thu, 6 Jun 2013 10:05:44 -0400 Subject: [PATCH 020/125] updating mktg defaults in common.py --- cms/envs/common.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cms/envs/common.py b/cms/envs/common.py index 04d5888750..22e69fa08a 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -335,3 +335,14 @@ INSTALLED_APPS = ( ################# EDX MARKETING SITE ################################## EDXMKTG_COOKIE_NAME = 'edxloggedin' +MKTG_URLS = {} +MKTG_URL_LINK_MAP = { + 'ABOUT': 'about_edx', + 'CONTACT': 'contact', + 'FAQ': 'help_edx', + 'COURSES': 'courses', + 'ROOT': 'root', + 'TOS': 'tos', + 'HONOR': 'honor', + 'PRIVACY': 'privacy_edx', +} From 4ffe3c51b54962b3f79eee0bee026f9f9e7b0d5a Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 28 May 2013 08:29:02 -0400 Subject: [PATCH 021/125] Adds segment-io integration into the LMS Tell segment IO whenever a page is loaded. Once this works, we can add more detailed instrumentation for events we care about. Will be turned off in prod until the SEGMENT_IO_LMS feature flag is set to True and a SEGMENT_IO_LMS_KEY is set. --- lms/envs/aws.py | 5 +++++ lms/envs/common.py | 3 +++ lms/envs/dev.py | 7 +++++++ lms/templates/main.html | 2 ++ lms/templates/widgets/segment-io.html | 24 ++++++++++++++++++++++++ 5 files changed, 41 insertions(+) create mode 100644 lms/templates/widgets/segment-io.html diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 788e08a110..1645505dd7 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -172,6 +172,11 @@ for name, value in ENV_TOKENS.get("CODE_JAIL", {}).items(): COURSES_WITH_UNSAFE_CODE = ENV_TOKENS.get("COURSES_WITH_UNSAFE_CODE", []) +# If segment.io key specified, load it and turn on segment IO if the feature flag is set +SEGMENT_IO_LMS_KEY = ENV_TOKENS.get('SEGMENT_IO_LMS_KEY') +if SEGMENT_IO_LMS_KEY: + MITX_FEATURES['SEGMENT_IO_LMS'] = ENV_TOKENS.get('SEGMENT_IO_LMS', False) + ############################## SECURE AUTH ITEMS ############### # Secret things: passwords, access keys, etc. diff --git a/lms/envs/common.py b/lms/envs/common.py index 2029715c60..b2c6f15c39 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -102,6 +102,9 @@ MITX_FEATURES = { # Staff Debug tool. 'ENABLE_STUDENT_HISTORY_VIEW': True, + # segment.io for LMS--need to explicitly turn it on on production. + 'SEGMENT_IO_LMS': False, + # Enables the student notes API and UI. 'ENABLE_STUDENT_NOTES': True, diff --git a/lms/envs/dev.py b/lms/envs/dev.py index ac0117ad28..17bce93991 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -244,6 +244,13 @@ MITX_FEATURES['ENABLE_PEARSON_LOGIN'] = False ANALYTICS_SERVER_URL = "http://127.0.0.1:9000/" ANALYTICS_API_KEY = "" +##### segment-io ###### + +# If there's an environment variable set, grab it and turn on segment io +SEGMENT_IO_LMS_KEY = os.environ.get('SEGMENT_IO_LMS_KEY') +if SEGMENT_IO_LMS_KEY: + MITX_FEATURES['SEGMENT_IO_LMS'] = True + ##################################################################### # Lastly, see if the developer has any local overrides. diff --git a/lms/templates/main.html b/lms/templates/main.html index f97c173d0d..b00446d190 100644 --- a/lms/templates/main.html +++ b/lms/templates/main.html @@ -61,6 +61,8 @@ % endif % endif + <%include file="widgets/segment-io.html" /> + diff --git a/lms/templates/widgets/segment-io.html b/lms/templates/widgets/segment-io.html new file mode 100644 index 0000000000..6b4ace8375 --- /dev/null +++ b/lms/templates/widgets/segment-io.html @@ -0,0 +1,24 @@ +% if settings.MITX_FEATURES.get('SEGMENT_IO_LMS'): + + + +% else: + + + +% endif From 6526146b954f08897424fff3fc5f3073d60168c2 Mon Sep 17 00:00:00 2001 From: Frances Botsford Date: Wed, 5 Jun 2013 15:08:38 -0400 Subject: [PATCH 022/125] updated misused sass body-bg to container-bg variableon account and course views --- lms/static/sass/course/base/_base.scss | 4 ++-- lms/static/sass/multicourse/_account.scss | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/static/sass/course/base/_base.scss b/lms/static/sass/course/base/_base.scss index 584412ca22..6d87b7f554 100644 --- a/lms/static/sass/course/base/_base.scss +++ b/lms/static/sass/course/base/_base.scss @@ -35,7 +35,7 @@ a { width: 100%; border-radius: 3px; border: 1px solid $outer-border-color; - background: $body-bg; + background: $container-bg; @include box-shadow(0 1px 2px rgba(0, 0, 0, 0.05)); } } @@ -50,7 +50,7 @@ textarea, input[type="text"], input[type="email"], input[type="password"] { - background: $body-bg; + background: $white; border: 1px solid $border-color-2; @include border-radius(0); @include box-shadow(0 1px 0 0 rgba(255,255,255, 0.6), inset 0 0 3px 0 rgba(0,0,0, 0.1)); diff --git a/lms/static/sass/multicourse/_account.scss b/lms/static/sass/multicourse/_account.scss index dfd803a22d..e531754aac 100644 --- a/lms/static/sass/multicourse/_account.scss +++ b/lms/static/sass/multicourse/_account.scss @@ -6,7 +6,7 @@ // page-level .view-register, .view-login, .view-passwordreset { - background: $body-bg; + background: $container-bg; @@ -331,7 +331,7 @@ } textarea, input { - background: $body-bg; + background: $container-bg; color: rgba(0,0,0,.25); } } From 86a7a255c316fa81c951201b1c394187bd159898 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Thu, 6 Jun 2013 10:35:44 -0400 Subject: [PATCH 023/125] resolves background color syncing for particular page/container wrappers --- lms/static/sass/base/_variables.scss | 3 ++- lms/static/sass/multicourse/_account.scss | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/static/sass/base/_variables.scss b/lms/static/sass/base/_variables.scss index 5c8c8d18d9..2bbb34a03f 100644 --- a/lms/static/sass/base/_variables.scss +++ b/lms/static/sass/base/_variables.scss @@ -88,9 +88,10 @@ $dashboard-profile-header-color: transparent; $dashboard-profile-color: rgb(252,252,252); $dot-color: $light-gray; -$content-wrapper-bg: shade($body-bg, 2%); +$content-wrapper-bg: $white; $course-bg-color: #d6d6d6; $course-bg-image: url(../images/bg-texture.png); +$account-content-wrapper-bg: shade($body-bg, 2%); $course-profile-bg: rgb(245,245,245); $course-header-bg: rgba(255,255,255, 0.93); diff --git a/lms/static/sass/multicourse/_account.scss b/lms/static/sass/multicourse/_account.scss index e531754aac..7528368986 100644 --- a/lms/static/sass/multicourse/_account.scss +++ b/lms/static/sass/multicourse/_account.scss @@ -98,7 +98,7 @@ // layout .content-wrapper { - background: $content-wrapper-bg; + background: $account-content-wrapper-bg; padding-bottom: 0; } From 9220a4f45940484a31edf61fb599f75d062ac9e4 Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 6 Jun 2013 10:29:10 -0400 Subject: [PATCH 024/125] Bullet-proofing for MKTG_URLS. --- .../contentstore/tests/test_utils.py | 6 ++++++ cms/djangoapps/contentstore/utils.py | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index c4b0f4bb51..0c33c2dcca 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -24,6 +24,12 @@ class LMSLinksTestCase(TestCase): with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': False}): self.assertEquals(self.get_about_page_link(), "//localhost:8000/courses/mitX/101/test/about") + @override_settings(MKTG_URLS={}) + def about_page_marketing_urls_not_set_test(self): + """ Error case. ENABLE_MKTG_SITE is True, but there is either no MKTG_URLS, or no MKTG_URLS Root property. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': True}): + self.assertEquals(self.get_about_page_link(), None) + @override_settings(LMS_BASE=None) def about_page_no_lms_base_test(self): """ No LMS_BASE, nor is ENABLE_MKTG_SITE True """ diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 703e9a266a..83c413ac72 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -4,6 +4,9 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from django.core.urlresolvers import reverse import copy +import logging + +log = logging.getLogger(__name__) DIRECT_ONLY_CATEGORIES = ['course', 'chapter', 'sequential', 'about', 'static_tab', 'course_info'] @@ -108,9 +111,18 @@ def get_lms_link_for_about_page(location): Returns the url to the course about page from the location tuple. """ if settings.MITX_FEATURES.get('ENABLE_MKTG_SITE', False): - # Root will be "www.edx.org". The complete URL will still not be exactly correct, - # but redirects exist from www.edx.org to get to the drupal course about page URL. - about_base = settings.MKTG_URLS.get('ROOT') + if not hasattr(settings, 'MKTG_URLS'): + log.exception("ENABLE_MKTG_SITE is True, but MKTG_URLS is not defined.") + about_base = None + else: + marketing_urls = settings.MKTG_URLS + if marketing_urls.get('ROOT', None) is None: + log.exception('There is no ROOT defined in MKTG_URLS') + about_base = None + else: + # Root will be "www.edx.org". The complete URL will still not be exactly correct, + # but redirects exist from www.edx.org to get to the drupal course about page URL. + about_base = marketing_urls.get('ROOT') elif settings.LMS_BASE is not None: about_base = settings.LMS_BASE else: From e5efdde76a7c7eda45cd64c1cd3ed45e38774314 Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 6 Jun 2013 10:55:41 -0400 Subject: [PATCH 025/125] Strip off https:// --- .../contentstore/tests/test_utils.py | 18 ++++++++++++++++++ cms/djangoapps/contentstore/utils.py | 7 +++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index 0c33c2dcca..fec82db1bb 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -24,6 +24,24 @@ class LMSLinksTestCase(TestCase): with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': False}): self.assertEquals(self.get_about_page_link(), "//localhost:8000/courses/mitX/101/test/about") + @override_settings(MKTG_URLS={'ROOT': 'http://www.dummy'}) + def about_page_marketing_site_remove_http_test(self): + """ Get URL for about page, marketing root present, remove http://. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': True}): + self.assertEquals(self.get_about_page_link(), "//www.dummy/courses/mitX/101/test/about") + + @override_settings(MKTG_URLS={'ROOT': 'https://www.dummy'}) + def about_page_marketing_site_remove_https_test(self): + """ Get URL for about page, marketing root present, remove https://. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': True}): + self.assertEquals(self.get_about_page_link(), "//www.dummy/courses/mitX/101/test/about") + + @override_settings(MKTG_URLS={'ROOT': 'www.dummyhttps://x'}) + def about_page_marketing_site_https__edge_test(self): + """ Get URL for about page, only remove https:// at the beginning of the string. """ + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {'ENABLE_MKTG_SITE': True}): + self.assertEquals(self.get_about_page_link(), "//www.dummyhttps://x/courses/mitX/101/test/about") + @override_settings(MKTG_URLS={}) def about_page_marketing_urls_not_set_test(self): """ Error case. ENABLE_MKTG_SITE is True, but there is either no MKTG_URLS, or no MKTG_URLS Root property. """ diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 83c413ac72..e9b62331ef 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -5,6 +5,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from django.core.urlresolvers import reverse import copy import logging +import re log = logging.getLogger(__name__) @@ -120,9 +121,11 @@ def get_lms_link_for_about_page(location): log.exception('There is no ROOT defined in MKTG_URLS') about_base = None else: - # Root will be "www.edx.org". The complete URL will still not be exactly correct, - # but redirects exist from www.edx.org to get to the drupal course about page URL. + # Root will be "https://www.edx.org". The complete URL will still not be exactly correct, + # but redirects exist from www.edx.org to get to the Drupal course about page URL. about_base = marketing_urls.get('ROOT') + # Strip off https:// (or http://) to be consistent with the formatting of LMS_BASE. + about_base = re.sub(r"^https?://", "", about_base) elif settings.LMS_BASE is not None: about_base = settings.LMS_BASE else: From 7d961bae548727d27b6fb27d5eb8c31136d21c21 Mon Sep 17 00:00:00 2001 From: dcadams Date: Thu, 6 Jun 2013 10:26:30 -0700 Subject: [PATCH 026/125] undid last commit where some coffee files were inadvertantly added. --- lms/djangoapps/instructor/views.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index d46af2269d..d8f9085f62 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -9,6 +9,7 @@ import os import re import requests from requests.status_codes import codes +import urllib from collections import OrderedDict from StringIO import StringIO @@ -1008,8 +1009,8 @@ def _do_enroll_students(course, course_id, students, overload=False, auto_enroll status[student] = 'already enrolled' continue try: - nce = CourseEnrollment(user=user, course_id=course_id) - nce.save() + ce = CourseEnrollment(user=user, course_id=course_id) + ce.save() status[student] = 'added' except: status[student] = 'rejected' @@ -1050,11 +1051,11 @@ def _do_unenroll_students(course_id, students): except User.DoesNotExist: continue - nce = CourseEnrollment.objects.filter(user=user, course_id=course_id) + ce = CourseEnrollment.objects.filter(user=user, course_id=course_id) #Will be 0 or 1 records as there is a unique key on user + course_id - if nce: + if ce: try: - nce[0].delete() + ce[0].delete() status[student] = "un-enrolled" except Exception as err: if not isok: From 9a631fe47654e6c220d02fa7ac9b7dcdace9c48b Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Wed, 29 May 2013 17:36:40 -0400 Subject: [PATCH 027/125] All uses of safe_exec need to get the correct random seed. --- common/lib/capa/capa/responsetypes.py | 32 +++++++- .../lib/capa/capa/tests/test_responsetypes.py | 77 ++++++++++++++++--- 2 files changed, 96 insertions(+), 13 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 0fa50079de..a13ed3ca11 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -288,7 +288,13 @@ class LoncapaResponse(object): } try: - safe_exec.safe_exec(code, globals_dict, python_path=self.context['python_path'], slug=self.id) + safe_exec.safe_exec( + code, + globals_dict, + python_path=self.context['python_path'], + slug=self.id, + random_seed=self.context['seed'], + ) except Exception as err: msg = 'Error %s in evaluating hint function %s' % (err, hintfn) msg += "\nSee XML source line %s" % getattr( @@ -973,7 +979,13 @@ class CustomResponse(LoncapaResponse): 'ans': ans, } globals_dict.update(kwargs) - safe_exec.safe_exec(code, globals_dict, python_path=self.context['python_path'], slug=self.id) + safe_exec.safe_exec( + code, + globals_dict, + python_path=self.context['python_path'], + slug=self.id, + random_seed=self.context['seed'], + ) return globals_dict['cfn_return'] return check_function @@ -1090,7 +1102,13 @@ class CustomResponse(LoncapaResponse): # exec the check function if isinstance(self.code, basestring): try: - safe_exec.safe_exec(self.code, self.context, cache=self.system.cache, slug=self.id) + safe_exec.safe_exec( + self.code, + self.context, + cache=self.system.cache, + slug=self.id, + random_seed=self.context['seed'], + ) except Exception as err: self._handle_exec_exception(err) @@ -1814,7 +1832,13 @@ class SchematicResponse(LoncapaResponse): ] self.context.update({'submission': submission}) try: - safe_exec.safe_exec(self.code, self.context, cache=self.system.cache, slug=self.id) + safe_exec.safe_exec( + self.code, + self.context, + cache=self.system.cache, + slug=self.id, + random_seed=self.context['seed'], + ) except Exception as err: msg = 'Error %s in evaluating SchematicResponse' % err raise ResponseError(msg) diff --git a/common/lib/capa/capa/tests/test_responsetypes.py b/common/lib/capa/capa/tests/test_responsetypes.py index 780c475b09..20de19f567 100644 --- a/common/lib/capa/capa/tests/test_responsetypes.py +++ b/common/lib/capa/capa/tests/test_responsetypes.py @@ -640,6 +640,23 @@ class StringResponseTest(ResponseTest): correct_map = problem.grade_answers(input_dict) self.assertEquals(correct_map.get_hint('1_2_1'), "Hello??") + def test_hint_function_randomization(self): + # The hint function should get the seed from the problem. + problem = self.build_problem( + answer="1", + hintfn="gimme_a_random_hint", + script=textwrap.dedent(""" + def gimme_a_random_hint(answer_ids, student_answers, new_cmap, old_cmap): + answer = str(random.randint(0, 1e9)) + new_cmap.set_hint_and_mode(answer_ids[0], answer, "always") + + """) + ) + correct_map = problem.grade_answers({'1_2_1': '2'}) + hint = correct_map.get_hint('1_2_1') + r = random.Random(problem.seed) + self.assertEqual(hint, str(r.randint(0, 1e9))) + class CodeResponseTest(ResponseTest): from response_xml_factory import CodeResponseXMLFactory @@ -948,7 +965,6 @@ class CustomResponseTest(ResponseTest): xml_factory_class = CustomResponseXMLFactory def test_inline_code(self): - # For inline code, we directly modify global context variables # 'answers' is a list of answers provided to us # 'correct' is a list we fill in with True/False @@ -961,15 +977,14 @@ class CustomResponseTest(ResponseTest): self.assert_grade(problem, '0', 'incorrect') def test_inline_message(self): - # Inline code can update the global messages list # to pass messages to the CorrectMap for a particular input # The code can also set the global overall_message (str) # to pass a message that applies to the whole response inline_script = textwrap.dedent(""" - messages[0] = "Test Message" - overall_message = "Overall message" - """) + messages[0] = "Test Message" + overall_message = "Overall message" + """) problem = self.build_problem(answer=inline_script) input_dict = {'1_2_1': '0'} @@ -983,8 +998,19 @@ class CustomResponseTest(ResponseTest): overall_msg = correctmap.get_overall_message() self.assertEqual(overall_msg, "Overall message") - def test_function_code_single_input(self): + def test_inline_randomization(self): + # Make sure the seed from the problem gets fed into the script execution. + inline_script = """messages[0] = str(random.randint(0, 1e9))""" + problem = self.build_problem(answer=inline_script) + input_dict = {'1_2_1': '0'} + correctmap = problem.grade_answers(input_dict) + + input_msg = correctmap.get_msg('1_2_1') + r = random.Random(problem.seed) + self.assertEqual(input_msg, str(r.randint(0, 1e9))) + + def test_function_code_single_input(self): # For function code, we pass in these arguments: # # 'expect' is the expect attribute of the @@ -1212,6 +1238,29 @@ class CustomResponseTest(ResponseTest): with self.assertRaises(ResponseError): problem.grade_answers({'1_2_1': '42'}) + def test_setup_randomization(self): + # Ensure that the problem setup script gets the random seed from the problem. + script = textwrap.dedent(""" + num = random.randint(0, 1e9) + """) + problem = self.build_problem(script=script) + r = random.Random(problem.seed) + self.assertEqual(r.randint(0, 1e9), problem.context['num']) + + def test_check_function_randomization(self): + # The check function should get random-seeded from the problem. + script = textwrap.dedent(""" + def check_func(expect, answer_given): + return {'ok': True, 'msg': str(random.randint(0, 1e9))} + """) + + problem = self.build_problem(script=script, cfn="check_func", expect="42") + input_dict = {'1_2_1': '42'} + correct_map = problem.grade_answers(input_dict) + msg = correct_map.get_msg('1_2_1') + r = random.Random(problem.seed) + self.assertEqual(msg, str(r.randint(0, 1e9))) + def test_module_imports_inline(self): ''' Check that the correct modules are available to custom @@ -1275,7 +1324,6 @@ class SchematicResponseTest(ResponseTest): xml_factory_class = SchematicResponseXMLFactory def test_grade(self): - # Most of the schematic-specific work is handled elsewhere # (in client-side JavaScript) # The is responsible only for executing the @@ -1290,7 +1338,7 @@ class SchematicResponseTest(ResponseTest): # The actual dictionary would contain schematic information # sent from the JavaScript simulation - submission_dict = {'test': 'test'} + submission_dict = {'test': 'the_answer'} input_dict = {'1_2_1': json.dumps(submission_dict)} correct_map = problem.grade_answers(input_dict) @@ -1299,8 +1347,19 @@ class SchematicResponseTest(ResponseTest): # is what we expect) self.assertEqual(correct_map.get_correctness('1_2_1'), 'correct') - def test_script_exception(self): + def test_check_function_randomization(self): + # The check function should get a random seed from the problem. + script = "correct = ['correct' if (submission[0]['num'] == random.randint(0, 1e9)) else 'incorrect']" + problem = self.build_problem(answer=script) + r = random.Random(problem.seed) + submission_dict = {'num': r.randint(0, 1e9)} + input_dict = {'1_2_1': json.dumps(submission_dict)} + correct_map = problem.grade_answers(input_dict) + + self.assertEqual(correct_map.get_correctness('1_2_1'), 'correct') + + def test_script_exception(self): # Construct a script that will raise an exception script = "raise Exception('test')" problem = self.build_problem(answer=script) From cab49716b56bd1d23e57ffb98805b60cdbfe65f3 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Thu, 6 Jun 2013 14:14:30 -0400 Subject: [PATCH 028/125] Whitelisted courses now run Python code outside the sandbox. --- common/lib/capa/capa/capa_problem.py | 1 + common/lib/capa/capa/responsetypes.py | 4 ++++ common/lib/capa/capa/safe_exec/safe_exec.py | 13 +++++++++-- .../capa/safe_exec/tests/test_safe_exec.py | 22 +++++++++++++++++++ requirements/edx/github.txt | 2 +- 5 files changed, 39 insertions(+), 3 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 150b3b3c9b..7dcd7b925e 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -470,6 +470,7 @@ class LoncapaProblem(object): python_path=python_path, cache=self.system.cache, slug=self.problem_id, + unsafely=self.system.can_execute_unsafe_code(), ) except Exception as err: log.exception("Error while execing script code: " + all_code) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index a13ed3ca11..6183ca2ade 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -294,6 +294,7 @@ class LoncapaResponse(object): python_path=self.context['python_path'], slug=self.id, random_seed=self.context['seed'], + unsafely=self.system.can_execute_unsafe_code(), ) except Exception as err: msg = 'Error %s in evaluating hint function %s' % (err, hintfn) @@ -985,6 +986,7 @@ class CustomResponse(LoncapaResponse): python_path=self.context['python_path'], slug=self.id, random_seed=self.context['seed'], + unsafely=self.system.can_execute_unsafe_code(), ) return globals_dict['cfn_return'] return check_function @@ -1108,6 +1110,7 @@ class CustomResponse(LoncapaResponse): cache=self.system.cache, slug=self.id, random_seed=self.context['seed'], + unsafely=self.system.can_execute_unsafe_code(), ) except Exception as err: self._handle_exec_exception(err) @@ -1838,6 +1841,7 @@ class SchematicResponse(LoncapaResponse): cache=self.system.cache, slug=self.id, random_seed=self.context['seed'], + unsafely=self.system.can_execute_unsafe_code(), ) except Exception as err: msg = 'Error %s in evaluating SchematicResponse' % err diff --git a/common/lib/capa/capa/safe_exec/safe_exec.py b/common/lib/capa/capa/safe_exec/safe_exec.py index 67e93be46f..3ab8f0bf9e 100644 --- a/common/lib/capa/capa/safe_exec/safe_exec.py +++ b/common/lib/capa/capa/safe_exec/safe_exec.py @@ -1,6 +1,7 @@ """Capa's specialized use of codejail.safe_exec.""" from codejail.safe_exec import safe_exec as codejail_safe_exec +from codejail.safe_exec import not_safe_exec as codejail_not_safe_exec from codejail.safe_exec import json_safe, SafeExecException from . import lazymod from statsd import statsd @@ -71,7 +72,7 @@ def update_hash(hasher, obj): @statsd.timed('capa.safe_exec.time') -def safe_exec(code, globals_dict, random_seed=None, python_path=None, cache=None, slug=None): +def safe_exec(code, globals_dict, random_seed=None, python_path=None, cache=None, slug=None, unsafely=False): """ Execute python code safely. @@ -90,6 +91,8 @@ def safe_exec(code, globals_dict, random_seed=None, python_path=None, cache=None `slug` is an arbitrary string, a description that's meaningful to the caller, that will be used in log messages. + If `unsafely` is true, then the code will actually be executed without sandboxing. + """ # Check the cache for a previous result. if cache: @@ -111,9 +114,15 @@ def safe_exec(code, globals_dict, random_seed=None, python_path=None, cache=None # Create the complete code we'll run. code_prolog = CODE_PROLOG % random_seed + # Decide which code executor to use. + if unsafely: + exec_fn = codejail_not_safe_exec + else: + exec_fn = codejail_safe_exec + # Run the code! Results are side effects in globals_dict. try: - codejail_safe_exec( + exec_fn( code_prolog + LAZY_IMPORTS + code, globals_dict, python_path=python_path, slug=slug, ) diff --git a/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py b/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py index 4592af8305..f8a8a32297 100644 --- a/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py +++ b/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py @@ -1,13 +1,17 @@ """Test safe_exec.py""" import hashlib +import os import os.path import random import textwrap import unittest +from nose.plugins.skip import SkipTest + from capa.safe_exec import safe_exec, update_hash from codejail.safe_exec import SafeExecException +from codejail.jail_code import is_configured class TestSafeExec(unittest.TestCase): @@ -68,6 +72,24 @@ class TestSafeExec(unittest.TestCase): self.assertIn("ZeroDivisionError", cm.exception.message) +class TestSafeOrNot(unittest.TestCase): + def test_cant_do_something_forbidden(self): + # Can't test for forbiddenness if CodeJail isn't configured for python. + if not is_configured("python"): + raise SkipTest + + g = {} + with self.assertRaises(SafeExecException) as cm: + safe_exec("import os; files = os.listdir('/')", g) + self.assertIn("OSError", cm.exception.message) + self.assertIn("Permission denied", cm.exception.message) + + def test_can_do_something_forbidden_if_run_unsafely(self): + g = {} + safe_exec("import os; files = os.listdir('/')", g, unsafely=True) + self.assertEqual(g['files'], os.listdir('/')) + + class DictCache(object): """A cache implementation over a simple dict, for testing.""" diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index fc9070bba3..8b5ab8df48 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -9,5 +9,5 @@ # Our libraries: -e git+https://github.com/edx/XBlock.git@2144a25d#egg=XBlock --e git+https://github.com/edx/codejail.git@5fb5fa0#egg=codejail +-e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.1.0#egg=diff_cover From d3965cb3ee30499bef12435cedb8dc625788ef34 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Thu, 6 Jun 2013 11:27:09 -0400 Subject: [PATCH 029/125] removes redundant h2 heading and syncs up h1 value (with broken mak/template syntax) for register view --- lms/templates/login.html | 4 ---- lms/templates/register.html | 6 +----- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/lms/templates/login.html b/lms/templates/login.html index b185b02c84..d3c3384847 100644 --- a/lms/templates/login.html +++ b/lms/templates/login.html @@ -86,10 +86,6 @@