From c64f0b1a180897feae71c47ae52df237d4124427 Mon Sep 17 00:00:00 2001 From: dcadams Date: Mon, 29 Apr 2013 09:38:10 -0700 Subject: [PATCH 001/192] 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/192] 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/192] 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/192] 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/192] 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/192] 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 74866a38b00c2ce5a593dc509dcb23f22febaaf0 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Wed, 29 May 2013 12:55:11 -0400 Subject: [PATCH 007/192] Move parseActions and statics out of evaluator() --- common/lib/calc/calc.py | 144 ++++++++++++++++++++-------------------- 1 file changed, 73 insertions(+), 71 deletions(-) diff --git a/common/lib/calc/calc.py b/common/lib/calc/calc.py index 2ee82e2fb4..0ab02e413b 100644 --- a/common/lib/calc/calc.py +++ b/common/lib/calc/calc.py @@ -37,16 +37,33 @@ default_variables = {'j': numpy.complex(0, 1), 'q': scipy.constants.e } + +ops = {"^": operator.pow, + "*": operator.mul, + "/": operator.truediv, + "+": operator.add, + "-": operator.sub, +} +# We eliminated extreme ones, since they're rarely used, and potentially +# confusing. They may also conflict with variables if we ever allow e.g. +# 5R instead of 5*R +suffixes = {'%': 0.01, 'k': 1e3, 'M': 1e6, 'G': 1e9, + 'T': 1e12, # 'P':1e15,'E':1e18,'Z':1e21,'Y':1e24, + 'c': 1e-2, 'm': 1e-3, 'u': 1e-6, + 'n': 1e-9, 'p': 1e-12} # ,'f':1e-15,'a':1e-18,'z':1e-21,'y':1e-24} + log = logging.getLogger("mitx.courseware.capa") class UndefinedVariable(Exception): - def raiseself(self): - ''' Helper so we can use inside of a lambda ''' - raise self + pass + # unused for now + # def raiseself(self): + # ''' Helper so we can use inside of a lambda ''' + # raise self -general_whitespace = re.compile('[^\w]+') +general_whitespace = re.compile('[^\\w]+') def check_variables(string, variables): @@ -65,13 +82,61 @@ def check_variables(string, variables): for v in possible_variables: if len(v) == 0: continue - if v[0] <= '9' and '0' <= 'v': # Skip things that begin with numbers + if v[0] <= '9' and '0' <= v: # Skip things that begin with numbers continue if v not in variables: bad_variables.append(v) if len(bad_variables) > 0: raise UndefinedVariable(' '.join(bad_variables)) +def lower_dict(d): + return dict([(k.lower(), d[k]) for k in d]) + +def super_float(text): + ''' Like float, but with si extensions. 1k goes to 1000''' + if text[-1] in suffixes: + return float(text[:-1]) * suffixes[text[-1]] + else: + return float(text) + +def number_parse_action(x): # [ '7' ] -> [ 7 ] + return [super_float("".join(x))] + +def exp_parse_action(x): # [ 2 ^ 3 ^ 2 ] -> 512 + x = [e for e in x if isinstance(e, numbers.Number)] # Ignore ^ + x.reverse() + x = reduce(lambda a, b: b ** a, x) + return x + +def parallel(x): # Parallel resistors [ 1 2 ] => 2/3 + # convert from pyparsing.ParseResults, which doesn't support '0 in x' + x = list(x) + if len(x) == 1: + return x[0] + if 0 in x: + return float('nan') + x = [1. / e for e in x if isinstance(e, numbers.Number)] # Ignore || + return 1. / sum(x) + +def sum_parse_action(x): # [ 1 + 2 - 3 ] -> 0 + total = 0.0 + op = ops['+'] + for e in x: + if e in set('+-'): + op = ops[e] + else: + total = op(total, e) + return total + +def prod_parse_action(x): # [ 1 * 2 / 3 ] => 0.66 + prod = 1.0 + op = ops['*'] + for e in x: + if e in set('*/'): + op = ops[e] + else: + prod = op(prod, e) + return prod def evaluator(variables, functions, string, cs=False): ''' @@ -86,12 +151,12 @@ def evaluator(variables, functions, string, cs=False): # log.debug("functions: {0}".format(functions)) # log.debug("string: {0}".format(string)) - def lower_dict(d): - return dict([(k.lower(), d[k]) for k in d]) - all_variables = copy.copy(default_variables) all_functions = copy.copy(default_functions) + def func_parse_action(x): + return [all_functions[x[0]](x[1])] + if not cs: all_variables = lower_dict(all_variables) all_functions = lower_dict(all_functions) @@ -113,69 +178,6 @@ def evaluator(variables, functions, string, cs=False): if string.strip() == "": return float('nan') - ops = {"^": operator.pow, - "*": operator.mul, - "/": operator.truediv, - "+": operator.add, - "-": operator.sub, - } - # We eliminated extreme ones, since they're rarely used, and potentially - # confusing. They may also conflict with variables if we ever allow e.g. - # 5R instead of 5*R - suffixes = {'%': 0.01, 'k': 1e3, 'M': 1e6, 'G': 1e9, - 'T': 1e12, # 'P':1e15,'E':1e18,'Z':1e21,'Y':1e24, - 'c': 1e-2, 'm': 1e-3, 'u': 1e-6, - 'n': 1e-9, 'p': 1e-12} # ,'f':1e-15,'a':1e-18,'z':1e-21,'y':1e-24} - - def super_float(text): - ''' Like float, but with si extensions. 1k goes to 1000''' - if text[-1] in suffixes: - return float(text[:-1]) * suffixes[text[-1]] - else: - return float(text) - - def number_parse_action(x): # [ '7' ] -> [ 7 ] - return [super_float("".join(x))] - - def exp_parse_action(x): # [ 2 ^ 3 ^ 2 ] -> 512 - x = [e for e in x if isinstance(e, numbers.Number)] # Ignore ^ - x.reverse() - x = reduce(lambda a, b: b ** a, x) - return x - - def parallel(x): # Parallel resistors [ 1 2 ] => 2/3 - # convert from pyparsing.ParseResults, which doesn't support '0 in x' - x = list(x) - if len(x) == 1: - return x[0] - if 0 in x: - return float('nan') - x = [1. / e for e in x if isinstance(e, numbers.Number)] # Ignore || - return 1. / sum(x) - - def sum_parse_action(x): # [ 1 + 2 - 3 ] -> 0 - total = 0.0 - op = ops['+'] - for e in x: - if e in set('+-'): - op = ops[e] - else: - total = op(total, e) - return total - - def prod_parse_action(x): # [ 1 * 2 / 3 ] => 0.66 - prod = 1.0 - op = ops['*'] - for e in x: - if e in set('*/'): - op = ops[e] - else: - prod = op(prod, e) - return prod - - def func_parse_action(x): - return [all_functions[x[0]](x[1])] - # SI suffixes and percent number_suffix = reduce(lambda a, b: a | b, map(Literal, suffixes.keys()), NoMatch()) (dot, minus, plus, times, div, lpar, rpar, exp) = map(Literal, ".-+*/()^") From ed45c505a39cf3a8aa094ee6c64591da1c604773 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Wed, 29 May 2013 12:55:51 -0400 Subject: [PATCH 008/192] Simpler pyparsing --- common/lib/calc/calc.py | 48 +++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/common/lib/calc/calc.py b/common/lib/calc/calc.py index 0ab02e413b..64053d6ca5 100644 --- a/common/lib/calc/calc.py +++ b/common/lib/calc/calc.py @@ -8,11 +8,11 @@ import numpy import numbers import scipy.constants -from pyparsing import Word, alphas, nums, oneOf, Literal -from pyparsing import ZeroOrMore, OneOrMore, StringStart -from pyparsing import StringEnd, Optional, Forward -from pyparsing import CaselessLiteral, Group, StringEnd -from pyparsing import NoMatch, stringEnd, alphanums +from pyparsing import Word, nums, Literal +from pyparsing import ZeroOrMore, MatchFirst +from pyparsing import Optional, Forward +from pyparsing import CaselessLiteral +from pyparsing import NoMatch, stringEnd, Suppress, Combine default_functions = {'sin': numpy.sin, 'cos': numpy.cos, @@ -179,17 +179,19 @@ def evaluator(variables, functions, string, cs=False): return float('nan') # SI suffixes and percent - number_suffix = reduce(lambda a, b: a | b, map(Literal, suffixes.keys()), NoMatch()) - (dot, minus, plus, times, div, lpar, rpar, exp) = map(Literal, ".-+*/()^") + number_suffix = MatchFirst([Literal(k) for k in suffixes.keys()]) + plus_minus = Literal('+') | Literal('-') + times_div = Literal('*') | Literal('/') number_part = Word(nums) # 0.33 or 7 or .34 or 16. inner_number = (number_part + Optional("." + Optional(number_part))) | ("." + number_part) + inner_number = Combine(inner_number) # by default pyparsing allows spaces between tokens--this prevents that # 0.33k or -17 - number = (Optional(minus | plus) + inner_number - + Optional(CaselessLiteral("E") + Optional((plus | minus)) + number_part) + number = (inner_number + + Optional(CaselessLiteral("E") + Optional(plus_minus) + number_part) + Optional(number_suffix)) number = number.setParseAction(number_parse_action) # Convert to number @@ -197,40 +199,34 @@ def evaluator(variables, functions, string, cs=False): expr = Forward() factor = Forward() - def sreduce(f, l): - ''' Same as reduce, but handle len 1 and len 0 lists sensibly ''' - if len(l) == 0: - return NoMatch() - if len(l) == 1: - return l[0] - return reduce(f, l) - # Handle variables passed in. E.g. if we have {'R':0.5}, we make the substitution. # Special case for no variables because of how we understand PyParsing is put together if len(all_variables) > 0: # We sort the list so that var names (like "e2") match before # mathematical constants (like "e"). This is kind of a hack. all_variables_keys = sorted(all_variables.keys(), key=len, reverse=True) - varnames = sreduce(lambda x, y: x | y, map(lambda x: CasedLiteral(x), all_variables_keys)) - varnames.setParseAction(lambda x: map(lambda y: all_variables[y], x)) + literal_all_vars = [CasedLiteral(k) for k in all_variables_keys] + varnames = MatchFirst(literal_all_vars) + varnames.setParseAction(lambda x: [all_variables[k] for k in x]) else: varnames = NoMatch() # Same thing for functions. if len(all_functions) > 0: - funcnames = sreduce(lambda x, y: x | y, - map(lambda x: CasedLiteral(x), all_functions.keys())) - function = funcnames + lpar.suppress() + expr + rpar.suppress() + funcnames = MatchFirst([CasedLiteral(k) for k in all_functions.keys()]) + function = funcnames + Suppress("(") + expr + Suppress(")") function.setParseAction(func_parse_action) else: function = NoMatch() - atom = number | function | varnames | lpar + expr + rpar - factor << (atom + ZeroOrMore(exp + atom)).setParseAction(exp_parse_action) # 7^6 + atom = number | function | varnames | Suppress("(") + expr + Suppress(")") + + # Do the following in the correct order to preserve order of operation + factor << (atom + ZeroOrMore("^" + atom)).setParseAction(exp_parse_action) # 7^6 paritem = factor + ZeroOrMore(Literal('||') + factor) # 5k || 4k paritem = paritem.setParseAction(parallel) - term = paritem + ZeroOrMore((times | div) + paritem) # 7 * 5 / 4 - 3 + term = paritem + ZeroOrMore(times_div + paritem) # 7 * 5 / 4 - 3 term = term.setParseAction(prod_parse_action) - expr << Optional((plus | minus)) + term + ZeroOrMore((plus | minus) + term) # -5 + 4 - 3 + expr << Optional(plus_minus) + term + ZeroOrMore(plus_minus + term) # -5 + 4 - 3 expr = expr.setParseAction(sum_parse_action) return (expr + stringEnd).parseString(string)[0] From 72d149caae1c5cd3909b59e850d94cb8ffc95c59 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Wed, 29 May 2013 13:25:48 -0400 Subject: [PATCH 009/192] Add docstrings and comments --- common/lib/calc/calc.py | 81 +++++++++++++++++++++++---- common/lib/capa/capa/responsetypes.py | 1 + 2 files changed, 71 insertions(+), 11 deletions(-) diff --git a/common/lib/calc/calc.py b/common/lib/calc/calc.py index 64053d6ca5..5d0aeb3fd1 100644 --- a/common/lib/calc/calc.py +++ b/common/lib/calc/calc.py @@ -1,3 +1,9 @@ +""" +Parser and evaluator for FormulaResponse and NumericalResponse + +Uses pyparsing to parse. Main function as of now is evaluator(). +""" + import copy import logging import math @@ -56,6 +62,10 @@ log = logging.getLogger("mitx.courseware.capa") class UndefinedVariable(Exception): + """ + Used to indicate the student input of a variable, which was unused by the + instructor. + """ pass # unused for now # def raiseself(self): @@ -67,7 +77,8 @@ general_whitespace = re.compile('[^\\w]+') def check_variables(string, variables): - '''Confirm the only variables in string are defined. + """ + Confirm the only variables in string are defined. Pyparsing uses a left-to-right parser, which makes the more elegant approach pretty hopeless. @@ -76,7 +87,7 @@ def check_variables(string, variables): undefined_variable = achar + Word(alphanums) undefined_variable.setParseAction(lambda x:UndefinedVariable("".join(x)).raiseself()) varnames = varnames | undefined_variable - ''' + """ possible_variables = re.split(general_whitespace, string) # List of all alnums in string bad_variables = list() for v in possible_variables: @@ -90,26 +101,59 @@ def check_variables(string, variables): raise UndefinedVariable(' '.join(bad_variables)) def lower_dict(d): + """ + takes each key in the dict and makes it lowercase, still mapping to the + same value. + + keep in mind that it is possible (but not useful?) to define different + variables that have the same lowercase representation. It would be hard to + tell which is used in the final dict and which isn't. + """ return dict([(k.lower(), d[k]) for k in d]) +# The following few functions define parse actions, which are run on lists of +# results from each parse component. They convert the strings and (previously +# calculated) numbers into the number that component represents. + def super_float(text): - ''' Like float, but with si extensions. 1k goes to 1000''' + """ + Like float, but with si extensions. 1k goes to 1000 + """ if text[-1] in suffixes: return float(text[:-1]) * suffixes[text[-1]] else: return float(text) -def number_parse_action(x): # [ '7' ] -> [ 7 ] +def number_parse_action(x): + """ + Create a float out of its string parts + + e.g. [ '7', '.', '13' ] -> [ 7.13 ] + Calls super_float above + """ return [super_float("".join(x))] -def exp_parse_action(x): # [ 2 ^ 3 ^ 2 ] -> 512 +def exp_parse_action(x): + """ + Take a list of numbers and exponentiate them, right to left + + e.g. [ 3, 2, 3 ] (which is 3^2^3 = 3^(2^3)) -> 6561 + """ x = [e for e in x if isinstance(e, numbers.Number)] # Ignore ^ x.reverse() x = reduce(lambda a, b: b ** a, x) return x -def parallel(x): # Parallel resistors [ 1 2 ] => 2/3 - # convert from pyparsing.ParseResults, which doesn't support '0 in x' +def parallel(x): + """ + Compute numbers according to the parallel resistors operator + + BTW it is commutative. Its formula is given by + out = 1 / (1/in1 + 1/in2 + ...) + e.g. [ 1, 2 ] => 2/3 + + Return NaN if there is a zero among the inputs + """ x = list(x) if len(x) == 1: return x[0] @@ -119,6 +163,13 @@ def parallel(x): # Parallel resistors [ 1 2 ] => 2/3 return 1. / sum(x) def sum_parse_action(x): # [ 1 + 2 - 3 ] -> 0 + """ + Add the inputs + + [ 1, '+', 2, '-', 3 ] -> 0 + + Allow a leading + or - + """ total = 0.0 op = ops['+'] for e in x: @@ -129,6 +180,11 @@ def sum_parse_action(x): # [ 1 + 2 - 3 ] -> 0 return total def prod_parse_action(x): # [ 1 * 2 / 3 ] => 0.66 + """ + Multiply the inputs + + [ 1, '*', 2, '/', 3 ] => 0.66 + """ prod = 1.0 op = ops['*'] for e in x: @@ -139,14 +195,13 @@ def prod_parse_action(x): # [ 1 * 2 / 3 ] => 0.66 return prod def evaluator(variables, functions, string, cs=False): - ''' + """ Evaluate an expression. Variables are passed as a dictionary from string to value. Unary functions are passed as a dictionary from string to function. Variables must be floats. cs: Case sensitive - TODO: Fix it so we can pass integers and complex numbers in variables dict - ''' + """ # log.debug("variables: {0}".format(variables)) # log.debug("functions: {0}".format(functions)) # log.debug("string: {0}".format(string)) @@ -187,7 +242,8 @@ def evaluator(variables, functions, string, cs=False): # 0.33 or 7 or .34 or 16. inner_number = (number_part + Optional("." + Optional(number_part))) | ("." + number_part) - inner_number = Combine(inner_number) # by default pyparsing allows spaces between tokens--this prevents that + # by default pyparsing allows spaces between tokens--Combine prevents that + inner_number = Combine(inner_number) # 0.33k or -17 number = (inner_number @@ -209,6 +265,8 @@ def evaluator(variables, functions, string, cs=False): varnames = MatchFirst(literal_all_vars) varnames.setParseAction(lambda x: [all_variables[k] for k in x]) else: + # all_variables includes DEFAULT_VARIABLES, which isn't empty + # this is unreachable. Get rid of it? varnames = NoMatch() # Same thing for functions. @@ -217,6 +275,7 @@ def evaluator(variables, functions, string, cs=False): function = funcnames + Suppress("(") + expr + Suppress(")") function.setParseAction(func_parse_action) else: + # see note above (this is unreachable) function = NoMatch() atom = number | function | varnames | Suppress("(") + expr + Suppress(")") diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 0fa50079de..314d01e7e8 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -1717,6 +1717,7 @@ class FormulaResponse(LoncapaResponse): student_variables = dict() # ranges give numerical ranges for testing for var in ranges: + # TODO: allow specified ranges (i.e. integers and complex numbers) for random variables value = random.uniform(*ranges[var]) instructor_variables[str(var)] = value student_variables[str(var)] = value From 1ea65455565f2369c220c0a9ce4dfa5376740411 Mon Sep 17 00:00:00 2001 From: dcadams Date: Tue, 4 Jun 2013 14:13:13 -0700 Subject: [PATCH 010/192] 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 011/192] 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 1ac6e1230434e8c8de7c3ebb4f0079c30ed45cbc Mon Sep 17 00:00:00 2001 From: Slater-Victoroff Date: Wed, 5 Jun 2013 10:22:18 -0400 Subject: [PATCH 012/192] Pyes working, considering switch to raw requests, phonetic and fuzzy search both working --- common/djangoapps/search/__init__.py | 0 common/djangoapps/search/index.py | 82 ++++++++++++++++++++++++++ common/djangoapps/search/mapping.json | 24 ++++++++ common/djangoapps/search/views.py | 83 +++++++++++++++++++++++++++ 4 files changed, 189 insertions(+) create mode 100644 common/djangoapps/search/__init__.py create mode 100644 common/djangoapps/search/index.py create mode 100644 common/djangoapps/search/mapping.json create mode 100644 common/djangoapps/search/views.py diff --git a/common/djangoapps/search/__init__.py b/common/djangoapps/search/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/search/index.py b/common/djangoapps/search/index.py new file mode 100644 index 0000000000..7012def8cb --- /dev/null +++ b/common/djangoapps/search/index.py @@ -0,0 +1,82 @@ +import os +import os.path as pt +import json +import re +import string + +from pyes import * +import nltk.stem.snowball as snowball +import fuzzy + + +def grab_transcripts(sjson_directory): + """Returns referenes to all of the files contained within a subs directory""" + all_children = [child for child in os.listdir(sjson_directory)] + all_transcripts = [child for child in all_children if pt.isfile(pt.join(sjson_directory, child))] + # . is not a valid character for a youtube id, so it can be reliably used to pick up the start + # of the file extension + uuids = [transcript_id[:transcript_id.find(".")] for transcript_id in all_transcripts] + parsed_transcripts = [open(pt.join(sjson_directory, transcript)).read() for transcript in all_transcripts] + return zip([clean_transcript(transcript) for transcript in parsed_transcripts], uuids) + + +def clean_transcript(transcript_string): + """Tries to parse and clean a raw transcript. Errors for invalid sjson""" + transcript_list = filter(None, json.loads(transcript_string)['text']) + relevant_text = " ".join([phrase.encode('utf-8').strip() for phrase in transcript_list]) + relevant_text = relevant_text.lower().translate(None, string.punctuation) + cleanedText = re.sub('\n', " ", relevant_text) + return cleanedText + + +def phonetic_transcript(clean_transcript, stemmer): + return " ".join([phoneticize(word, stemmer) for word in clean_transcript.split(" ")]) + + +def phoneticize(word, stemmer): + encode = lambda word: word.decode('utf-8').encode('ascii', 'ignore') + phonetic = lambda word: fuzzy.nysiis(stemmer.stem(encode(word))) + return phonetic(word) + + +def initialize_transcripts(database, mapping): + database.indices.create_index("transcript-index") + + +def index_course(database, sjson_directory, course_name, mapping): + stemmer = snowball.EnglishStemmer() + database.put_mapping(course_name, {'properties': mapping}, "transcript-index") + all_transcripts = grab_transcripts(sjson_directory) + video_counter = 0 + for transcript_tuple in all_transcripts: + data_map = {"searchable_text": transcript_tuple[0], "uuid": transcript_tuple[1]} + data_map['phonetic_text'] = phonetic_transcript(transcript_tuple[0], stemmer) + database.index(data_map, "transcript-index", course_name) + video_counter += 1 + database.indices.refresh("transcript-index") + + +def fuzzy_search(database, query, course_name): + search_query = FuzzyLikeThisFieldQuery("searchable_text", query) + return database.search(query=search_query, indices="transcript-index") + + +def phonetic_search(database, query, course_name): + stemmer = snowball.EnglishStemmer() + search_query = TextQuery("phonetic_text", phoneticize(query, stemmer)) + return database.search(query=search_query, indices="transcript-index") + + +data_directory = '/Users/climatologist/edx_all/data/content-mit-6002x/static/subs/' +mapping_directory = 'mapping.json' +database = ES('127.0.0.1:9200') +mapping = json.loads(open(mapping_directory, 'rb').read()) + +#initialize_transcripts(database, mapping) +#index_course(database, data_directory, "test-course", mapping) +fuzzy_results = fuzzy_search(database, "gaussian", "test-course") +phonetic_results = phonetic_search(database, "gaussian", "test-course") +for r in fuzzy_results: + print "Fuzzy: " + r['uuid'] +for r in phonetic_results: + print "Phonetic: " + r['uuid'] diff --git a/common/djangoapps/search/mapping.json b/common/djangoapps/search/mapping.json new file mode 100644 index 0000000000..8d4deade6d --- /dev/null +++ b/common/djangoapps/search/mapping.json @@ -0,0 +1,24 @@ +{ + + "searchable_text": { + "boost": 1.0, + "index": "analyzed", + "store": "yes", + "type": "string", + "term_vector": "with_positions_offsets" + }, + + "phonetic_text": { + "boost": 1.0, + "index": "analyzed", + "store": "yes", + "type": "string", + "term_vector": "with_positions_offsets" + }, + + "uuid": { + "index": "not_analyzed", + "store": "yes", + "type": "string" + } +} \ No newline at end of file diff --git a/common/djangoapps/search/views.py b/common/djangoapps/search/views.py new file mode 100644 index 0000000000..f15dc9266b --- /dev/null +++ b/common/djangoapps/search/views.py @@ -0,0 +1,83 @@ +from django.http import HttpResponse +from django.template.loader import get_template +from django.template import Context +from django.contrib.auth.models import User +from django.contrib.staticfiles import finders +from courseware.courses import get_courses +from courseware.model_data import ModelDataCache +from courseware.module_render import get_module_for_descriptor + +from courseware.views import registered_for_course +#import logging +import lxml +import re +import posixpath +import urllib +from os import listdir +from os.path import isfile +from os.path import join + + +def test(request): + user = User.objects.prefetch_related("groups").get(id=request.user.id) + request.user = user + + course_list = get_courses(user, request.META.get('HTTP_HOST')) + + all_modules = [get_module(request, user, course) for course in course_list if registered_for_course(course, user)] + child_modules = [] + for module in all_modules: + child_modules.extend(module.get_children()) + bottom_modules = [] + for module in child_modules: + bottom_modules.extend(module.get_children()) + asset_divs = get_asset_div(convert_to_valid_html(bottom_modules[2].get_html())) + strings = [get_transcript_directory(lxml.html.tostring(div)) for div in asset_divs] + search_template = get_template('search.html') + html = search_template.render(Context({'course_list': strings})) + return HttpResponse(html) + + +def get_children(course): + """Returns the children of a given course""" + attributes = [child.location for child in course._child_instances] + return attributes + + +def convert_to_valid_html(html): + replacement = {"<": "<", ">": ">", """: "\"", "'": "'"} + for i, j in replacement.iteritems(): + html = html.replace(i, j) + return html + + +def get_asset_div(html_page): + return lxml.html.find_class(html_page, "video") + + +def get_module(request, user, course): + model_data_cache = ModelDataCache.cache_for_descriptor_descendents(course.id, user, course, depth=2) + course_module = get_module_for_descriptor(user, request, course, model_data_cache, course.id) + return course_module + + +def get_youtube_code(module_html): + youtube_snippet = re.sub(r'(.*?)(1\.0:)(.*?)(,1\.25)(.*)', r'\3', module_html) + sliced_youtube_code = youtube_snippet[:youtube_snippet.find('\n')] + return sliced_youtube_code + + +def get_transcript_directory(module_html): + directory_snippet = re.sub(r'(.*?)(data-caption-asset-path=\")(.*?)(\">.*)', r'\3', module_html) + sliced_directory = directory_snippet[:directory_snippet.find('\n')] + return resolve_to_absolute_path(sliced_directory) + + +def resolve_to_absolute_path(transcript_directory): + normalized_path = posixpath.normpath(urllib.unquote(transcript_directory)).lstrip('/') + return all_transcript_files(normalized_path) + + +def all_transcript_files(normalized_path): + files = [transcript for transcript in listdir(normalized_path) if isfile(join(normalized_path, transcript))] + return files From 16fc7b37fb2da07ea26db98139a0c2094235a290 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 5 Jun 2013 11:12:47 -0400 Subject: [PATCH 013/192] 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 014/192] 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 015/192] 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 016/192] 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 017/192] 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 018/192] 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 a85a7f71df6c0bc889b2d5cbe40926b3663d375e Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Wed, 29 May 2013 13:34:58 -0400 Subject: [PATCH 019/192] Rename variables; get rid of OPS --- common/lib/calc/calc.py | 170 ++++++++++++++++++++-------------------- 1 file changed, 87 insertions(+), 83 deletions(-) diff --git a/common/lib/calc/calc.py b/common/lib/calc/calc.py index 5d0aeb3fd1..f862b41542 100644 --- a/common/lib/calc/calc.py +++ b/common/lib/calc/calc.py @@ -11,16 +11,15 @@ import operator import re import numpy -import numbers import scipy.constants -from pyparsing import Word, nums, Literal -from pyparsing import ZeroOrMore, MatchFirst -from pyparsing import Optional, Forward -from pyparsing import CaselessLiteral -from pyparsing import NoMatch, stringEnd, Suppress, Combine +from pyparsing import (Word, nums, Literal, + ZeroOrMore, MatchFirst, + Optional, Forward, + CaselessLiteral, + NoMatch, stringEnd, Suppress, Combine) -default_functions = {'sin': numpy.sin, +DEFAULT_FUNCTIONS = {'sin': numpy.sin, 'cos': numpy.cos, 'tan': numpy.tan, 'sqrt': numpy.sqrt, @@ -34,7 +33,7 @@ default_functions = {'sin': numpy.sin, 'fact': math.factorial, 'factorial': math.factorial } -default_variables = {'j': numpy.complex(0, 1), +DEFAULT_VARIABLES = {'j': numpy.complex(0, 1), 'e': numpy.e, 'pi': numpy.pi, 'k': scipy.constants.k, @@ -43,22 +42,15 @@ default_variables = {'j': numpy.complex(0, 1), 'q': scipy.constants.e } - -ops = {"^": operator.pow, - "*": operator.mul, - "/": operator.truediv, - "+": operator.add, - "-": operator.sub, -} # We eliminated extreme ones, since they're rarely used, and potentially # confusing. They may also conflict with variables if we ever allow e.g. # 5R instead of 5*R -suffixes = {'%': 0.01, 'k': 1e3, 'M': 1e6, 'G': 1e9, +SUFFIXES = {'%': 0.01, 'k': 1e3, 'M': 1e6, 'G': 1e9, 'T': 1e12, # 'P':1e15,'E':1e18,'Z':1e21,'Y':1e24, 'c': 1e-2, 'm': 1e-3, 'u': 1e-6, 'n': 1e-9, 'p': 1e-12} # ,'f':1e-15,'a':1e-18,'z':1e-21,'y':1e-24} -log = logging.getLogger("mitx.courseware.capa") +LOG = logging.getLogger("mitx.courseware.capa") class UndefinedVariable(Exception): @@ -73,13 +65,12 @@ class UndefinedVariable(Exception): # raise self -general_whitespace = re.compile('[^\\w]+') - - def check_variables(string, variables): """ Confirm the only variables in string are defined. + Otherwise, raise an UndefinedVariable containing all bad variables. + Pyparsing uses a left-to-right parser, which makes the more elegant approach pretty hopeless. @@ -88,19 +79,22 @@ def check_variables(string, variables): undefined_variable.setParseAction(lambda x:UndefinedVariable("".join(x)).raiseself()) varnames = varnames | undefined_variable """ - possible_variables = re.split(general_whitespace, string) # List of all alnums in string + general_whitespace = re.compile('[^\\w]+') + # List of all alnums in string + possible_variables = re.split(general_whitespace, string) bad_variables = list() - for v in possible_variables: - if len(v) == 0: + for var in possible_variables: + if len(var) == 0: continue - if v[0] <= '9' and '0' <= v: # Skip things that begin with numbers + if var[0] <= '9' and '0' <= var: # Skip things that begin with numbers continue - if v not in variables: - bad_variables.append(v) + if var not in variables: + bad_variables.append(var) if len(bad_variables) > 0: raise UndefinedVariable(' '.join(bad_variables)) -def lower_dict(d): + +def lower_dict(input_dict): """ takes each key in the dict and makes it lowercase, still mapping to the same value. @@ -109,7 +103,8 @@ def lower_dict(d): variables that have the same lowercase representation. It would be hard to tell which is used in the final dict and which isn't. """ - return dict([(k.lower(), d[k]) for k in d]) + return dict([(k.lower(), input_dict[k]) for k in input_dict]) + # The following few functions define parse actions, which are run on lists of # results from each parse component. They convert the strings and (previously @@ -119,32 +114,37 @@ def super_float(text): """ Like float, but with si extensions. 1k goes to 1000 """ - if text[-1] in suffixes: - return float(text[:-1]) * suffixes[text[-1]] + if text[-1] in SUFFIXES: + return float(text[:-1]) * SUFFIXES[text[-1]] else: return float(text) -def number_parse_action(x): + +def number_parse_action(parse_result): """ Create a float out of its string parts e.g. [ '7', '.', '13' ] -> [ 7.13 ] Calls super_float above """ - return [super_float("".join(x))] + return super_float("".join(parse_result)) -def exp_parse_action(x): + +def exp_parse_action(parse_result): """ Take a list of numbers and exponentiate them, right to left e.g. [ 3, 2, 3 ] (which is 3^2^3 = 3^(2^3)) -> 6561 """ - x = [e for e in x if isinstance(e, numbers.Number)] # Ignore ^ - x.reverse() - x = reduce(lambda a, b: b ** a, x) - return x + # pyparsing.ParseResults doesn't play well with reverse() + parse_result = parse_result.asList() + parse_result.reverse() + # the result of an exponentiation is called a power + power = reduce(lambda a, b: b ** a, parse_result) + return power -def parallel(x): + +def parallel(parse_result): """ Compute numbers according to the parallel resistors operator @@ -154,15 +154,17 @@ def parallel(x): Return NaN if there is a zero among the inputs """ - x = list(x) - if len(x) == 1: - return x[0] - if 0 in x: + # convert from pyparsing.ParseResults, which doesn't support '0 in parse_result' + parse_result = parse_result.asList() + if len(parse_result) == 1: + return parse_result[0] + if 0 in parse_result: return float('nan') - x = [1. / e for e in x if isinstance(e, numbers.Number)] # Ignore || - return 1. / sum(x) + reciprocals = [1. / e for e in parse_result] + return 1. / sum(reciprocals) -def sum_parse_action(x): # [ 1 + 2 - 3 ] -> 0 + +def sum_parse_action(parse_result): """ Add the inputs @@ -171,29 +173,35 @@ def sum_parse_action(x): # [ 1 + 2 - 3 ] -> 0 Allow a leading + or - """ total = 0.0 - op = ops['+'] - for e in x: - if e in set('+-'): - op = ops[e] + current_op = operator.add + for token in parse_result: + if token is '+': + current_op = operator.add + elif token is '-': + current_op = operator.sub else: - total = op(total, e) + total = current_op(total, token) return total -def prod_parse_action(x): # [ 1 * 2 / 3 ] => 0.66 + +def prod_parse_action(parse_result): """ Multiply the inputs [ 1, '*', 2, '/', 3 ] => 0.66 """ prod = 1.0 - op = ops['*'] - for e in x: - if e in set('*/'): - op = ops[e] + current_op = operator.mul + for token in parse_result: + if token is '*': + current_op = operator.mul + elif token is '/': + current_op = operator.truediv else: - prod = op(prod, e) + prod = current_op(prod, token) return prod + def evaluator(variables, functions, string, cs=False): """ Evaluate an expression. Variables are passed as a dictionary @@ -202,20 +210,12 @@ def evaluator(variables, functions, string, cs=False): cs: Case sensitive """ - # log.debug("variables: {0}".format(variables)) - # log.debug("functions: {0}".format(functions)) - # log.debug("string: {0}".format(string)) - - all_variables = copy.copy(default_variables) - all_functions = copy.copy(default_functions) - - def func_parse_action(x): - return [all_functions[x[0]](x[1])] - - if not cs: - all_variables = lower_dict(all_variables) - all_functions = lower_dict(all_functions) + # LOG.debug("variables: {0}".format(variables)) + # LOG.debug("functions: {0}".format(functions)) + # LOG.debug("string: {0}".format(string)) + all_variables = copy.copy(DEFAULT_VARIABLES) + all_functions = copy.copy(DEFAULT_FUNCTIONS) all_variables.update(variables) all_functions.update(functions) @@ -234,7 +234,7 @@ def evaluator(variables, functions, string, cs=False): return float('nan') # SI suffixes and percent - number_suffix = MatchFirst([Literal(k) for k in suffixes.keys()]) + number_suffix = MatchFirst([Literal(k) for k in SUFFIXES.keys()]) plus_minus = Literal('+') | Literal('-') times_div = Literal('*') | Literal('/') @@ -249,11 +249,10 @@ def evaluator(variables, functions, string, cs=False): number = (inner_number + Optional(CaselessLiteral("E") + Optional(plus_minus) + number_part) + Optional(number_suffix)) - number = number.setParseAction(number_parse_action) # Convert to number + number.setParseAction(number_parse_action) # Convert to number # Predefine recursive variables expr = Forward() - factor = Forward() # Handle variables passed in. E.g. if we have {'R':0.5}, we make the substitution. # Special case for no variables because of how we understand PyParsing is put together @@ -261,9 +260,10 @@ def evaluator(variables, functions, string, cs=False): # We sort the list so that var names (like "e2") match before # mathematical constants (like "e"). This is kind of a hack. all_variables_keys = sorted(all_variables.keys(), key=len, reverse=True) - literal_all_vars = [CasedLiteral(k) for k in all_variables_keys] - varnames = MatchFirst(literal_all_vars) - varnames.setParseAction(lambda x: [all_variables[k] for k in x]) + varnames = MatchFirst([CasedLiteral(k) for k in all_variables_keys]) + varnames.setParseAction( + lambda x: [all_variables[k] for k in x] + ) else: # all_variables includes DEFAULT_VARIABLES, which isn't empty # this is unreachable. Get rid of it? @@ -273,7 +273,9 @@ def evaluator(variables, functions, string, cs=False): if len(all_functions) > 0: funcnames = MatchFirst([CasedLiteral(k) for k in all_functions.keys()]) function = funcnames + Suppress("(") + expr + Suppress(")") - function.setParseAction(func_parse_action) + function.setParseAction( + lambda x: [all_functions[x[0]](x[1])] + ) else: # see note above (this is unreachable) function = NoMatch() @@ -281,11 +283,13 @@ def evaluator(variables, functions, string, cs=False): atom = number | function | varnames | Suppress("(") + expr + Suppress(")") # Do the following in the correct order to preserve order of operation - factor << (atom + ZeroOrMore("^" + atom)).setParseAction(exp_parse_action) # 7^6 - paritem = factor + ZeroOrMore(Literal('||') + factor) # 5k || 4k - paritem = paritem.setParseAction(parallel) - term = paritem + ZeroOrMore(times_div + paritem) # 7 * 5 / 4 - 3 - term = term.setParseAction(prod_parse_action) - expr << Optional(plus_minus) + term + ZeroOrMore(plus_minus + term) # -5 + 4 - 3 - expr = expr.setParseAction(sum_parse_action) + pow_term = atom + ZeroOrMore(Suppress("^") + atom) + pow_term.setParseAction(exp_parse_action) # 7^6 + par_term = pow_term + ZeroOrMore(Suppress('||') + pow_term) # 5k || 4k + par_term.setParseAction(parallel) + prod_term = par_term + ZeroOrMore(times_div + par_term) # 7 * 5 / 4 - 3 + prod_term.setParseAction(prod_parse_action) + sum_term = Optional(plus_minus) + prod_term + ZeroOrMore(plus_minus + prod_term) # -5 + 4 - 3 + sum_term.setParseAction(sum_parse_action) + expr << sum_term # finish the recursion return (expr + stringEnd).parseString(string)[0] From 83f1f9c2fc78442c77376457094ba674bca59c49 Mon Sep 17 00:00:00 2001 From: Peter Baratta Date: Wed, 5 Jun 2013 15:50:35 -0400 Subject: [PATCH 020/192] Set numpy so it does not print out warnings on student input --- common/lib/calc/calc.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/common/lib/calc/calc.py b/common/lib/calc/calc.py index f862b41542..cc3a883221 100644 --- a/common/lib/calc/calc.py +++ b/common/lib/calc/calc.py @@ -13,6 +13,10 @@ import re import numpy import scipy.constants +# have numpy raise errors on functions outside its domain +# See http://docs.scipy.org/doc/numpy/reference/generated/numpy.seterr.html +numpy.seterr(all='ignore') # Also: 'ignore', 'warn' (default), 'raise' + from pyparsing import (Word, nums, Literal, ZeroOrMore, MatchFirst, Optional, Forward, From d147103be0d5b02799fbd2a27e8b5ccc55915b27 Mon Sep 17 00:00:00 2001 From: JonahStanley Date: Wed, 5 Jun 2013 15:55:12 -0400 Subject: [PATCH 021/192] 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 022/192] 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 023/192] 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 024/192] 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 1b81d06337346fc5243b406d0fbeea1690e53962 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 5 Jun 2013 15:47:36 -0400 Subject: [PATCH 025/192] Re-insert "if in course" check for displaying paragraph about forums. Accidentally got removed in pull request 13. --- lms/templates/help_modal.html | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lms/templates/help_modal.html b/lms/templates/help_modal.html index 350f858334..facd74e9ae 100644 --- a/lms/templates/help_modal.html +++ b/lms/templates/help_modal.html @@ -21,9 +21,12 @@ discussion_link = get_discussion_link(course) if course else None %> +% if discussion_link:

For questions on course lectures, homework, tools, or materials for this course, post in the course discussion forum.

+% endif +

Have general questions about edX? You can find lots of helpful information in the edX FAQ.

From 7e8c06caf366830d12c75d10d8138e2fa43b7243 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Wed, 5 Jun 2013 16:07:17 -0400 Subject: [PATCH 026/192] Developer-private settings files Developers can have private settings files by creating lms/envs/private.py or cms/envs/private.py. They are imported at the end of dev.py. Note that they won't be imported if you are using one of the other dev*.py variants. --- .gitignore | 2 ++ cms/envs/dev.py | 8 ++++++++ lms/envs/dev.py | 8 ++++++++ 3 files changed, 18 insertions(+) diff --git a/.gitignore b/.gitignore index 05e76c4caa..8019dd70a7 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,8 @@ .AppleDouble database.sqlite requirements/private.txt +lms/envs/private.py +cms/envs/private.py courseware/static/js/mathjax/* flushdb.sh build diff --git a/cms/envs/dev.py b/cms/envs/dev.py index e63968d338..eea236f0e2 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -165,3 +165,11 @@ MITX_FEATURES['ENABLE_SERVICE_STATUS'] = True # segment-io key for dev SEGMENT_IO_KEY = 'mty8edrrsg' + + +##################################################################### +# Lastly, see if the developer has any local overrides. +try: + from .private import * +except ImportError: + pass diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 9d7c0b3ac2..ac0117ad28 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -243,3 +243,11 @@ MITX_FEATURES['ENABLE_PEARSON_LOGIN'] = False ANALYTICS_SERVER_URL = "http://127.0.0.1:9000/" ANALYTICS_API_KEY = "" + + +##################################################################### +# Lastly, see if the developer has any local overrides. +try: + from .private import * +except ImportError: + pass From 359e61d0f22815cdfd8c3744047877c8f803f2ce Mon Sep 17 00:00:00 2001 From: Will Daly Date: Wed, 5 Jun 2013 16:25:34 -0400 Subject: [PATCH 027/192] Fixed issue with xargs on Mac OS X. Removed cleaning of reports folders, since this was interferring with coverage reports generated sequentially by rake test. --- rakefiles/tests.rake | 6 ------ 1 file changed, 6 deletions(-) diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index 904ea4a54e..1c576f2cbb 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -45,12 +45,6 @@ end directory REPORT_DIR task :clean_test_files do - - # Delete all files in the reports directory, while preserving - # the directory structure. - sh("find #{REPORT_DIR} -type f -print0 | xargs --no-run-if-empty -0 rm") - - # Reset the test fixtures sh("git clean -fqdx test_root") end From 1f284c56c161c3ccffab3ddefacd2034f528c03e Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Wed, 5 Jun 2013 18:15:16 -0400 Subject: [PATCH 028/192] Skip a test that is causing intermittent failures due to the way it is overriding urls --- common/djangoapps/mitxmako/tests.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/mitxmako/tests.py b/common/djangoapps/mitxmako/tests.py index 21866eb9b5..f419daa681 100644 --- a/common/djangoapps/mitxmako/tests.py +++ b/common/djangoapps/mitxmako/tests.py @@ -4,13 +4,15 @@ from django.core.urlresolvers import reverse from django.conf import settings from mitxmako.shortcuts import marketing_link from mock import patch - +from nose.plugins.skip import SkipTest class ShortcutsTests(TestCase): """ Test the mitxmako shortcuts file """ - + # TODO: fix this test. It is causing intermittent test failures on + # subsequent tests due to the way urls are loaded + raise SkipTest() @override_settings(MKTG_URLS={'ROOT': 'dummy-root', 'ABOUT': '/about-us'}) @override_settings(MKTG_URL_LINK_MAP={'ABOUT': 'login'}) def test_marketing_link(self): From 3b165da17047b13c696ee319fa8798d20f8cff04 Mon Sep 17 00:00:00 2001 From: Slater-Victoroff Date: Thu, 6 Jun 2013 08:56:16 -0400 Subject: [PATCH 029/192] Replaced troublesome pyes integration with direct calls made to elasticsearch rest api --- common/djangoapps/search/analyzer.json | 36 +++++++ common/djangoapps/search/es_requests.py | 104 ++++++++++++++++++++ common/djangoapps/search/mapping.json | 17 +--- common/djangoapps/search/protectedWords.txt | 15 +++ common/djangoapps/search/settings.json | 8 ++ 5 files changed, 165 insertions(+), 15 deletions(-) create mode 100644 common/djangoapps/search/analyzer.json create mode 100644 common/djangoapps/search/es_requests.py create mode 100644 common/djangoapps/search/protectedWords.txt create mode 100644 common/djangoapps/search/settings.json diff --git a/common/djangoapps/search/analyzer.json b/common/djangoapps/search/analyzer.json new file mode 100644 index 0000000000..2221d7ad19 --- /dev/null +++ b/common/djangoapps/search/analyzer.json @@ -0,0 +1,36 @@ +{ +"analyzer": { + + "transcript_analyzer": { + "type": "custom", + "tokenizer": "standard", + "filter": ["protected", "asciifolding", "custom_word_delimiter", "lowercase", "custom_stemmer", "shingle"], + "char_filter": ["custom_mapping"] + } +}, + +"filter" : { + + "custom_word_delimiter":{ + "type": "word_delimiter", + "preserve_original": "true" + }, + + "custom_stemmer": { + "type": "stemmer", + "name": "english" + }, + + "protected": { + "type": "keyword_marker", + "keywords_path": "protectedWords.txt" + } +}, + +"char_filter": { + "custom_mapping": { + "type": "mapping", + "mappings": ["\n=>-"] + } +} +} \ No newline at end of file diff --git a/common/djangoapps/search/es_requests.py b/common/djangoapps/search/es_requests.py new file mode 100644 index 0000000000..7c4a81faf5 --- /dev/null +++ b/common/djangoapps/search/es_requests.py @@ -0,0 +1,104 @@ +import requests +import json + + +class ElasticDatabase: + + def __init__(self, url, index_settings_file, *args): + """ + Will initialize elastic search object with any indices specified by args + + specifically the url should be something of the form `http://localhost:9200` + importantly do not include a slash at the end of the url name. + + args should be a list of dictionaries, each dictionary specifying a JSON mapping + to be used for a specific type. + + Example Dictionary: + {"index": "transcript", "type": "6-002x", "mapping": + { + "properties" : { + "searchable_text": { + "type": "string", + "store": "yes", + "index": "analyzed" + } + } + } + } + + Eventually we will support different configuration files for different indices, but + since this is only indexing transcripts right now it seems excessive""" + + self.url = url + self.args = args + self.index_settings = open(index_settings_file, 'rb').read() + + def parse_args(self): + for mapping in self.args: + try: + json_mapping = json.loads(mapping) + except ValueError: + print "Badly formed JSON args, please check your mappings file" + break + + try: + index = json_mapping['index'] + type_ = json_mapping['type'] + mapping = json_mapping['mapping'] + self.setup_index(index) + self.setup_type(index, type_, mapping) + except KeyError: + print "Could not find needed keys. Keys found: " + print mapping.keys() + continue + + def setup_type(self, index, type_, json_mapping): + """ + json_mapping should be a dictionary starting at the properties level of a mapping. + + The type level will be added, so if you include it things will break. The purpose of this + is to encourage loose coupling between types and mappings for better code + """ + + full_url = "/".join([self.url, index, type_, "_mapping"]) + json_put_body = {type_: json_mapping} + requests.put(full_url, data=json_put_body) + + def has_index(self, index): + """Checks to see if a given index exists in the database returns existance boolean, + + If this returns something other than a 200 or a 404 something is wrong and so we error""" + full_url = "/".join([self.url, index]) + status = requests.head(full_url).status_code + if status == 200: + return True + if status == 404: + return False + else: + print "Got an unexpected reponse code: " + str(status) + raise + + def setup_index(self, index): + """Creates a new elasticsearch index, returns the response it gets""" + full_url = "/".join(self.url, index) + "/" + return requests.put(full_url, data=self.index_settings) + + def index_data(self, index, type_, id_, data): + """Data should be passed in as a dictionary, assumes it matches the given mapping""" + full_url = "/".join([self.url, index, type_, id_]) + response = requests.put(full_url, json.dumps(data)) + return json.loads(response)['ok'] + + def get_index_settings(self, index): + """Returns the current settings of """ + full_url = "/".join([self.url, index, "_settings"]) + return json.loads(requests.get(full_url)._content) + + def get_type_mapping(self, index, type_): + full_url = "/".join([self.url, index, type_, "_mapping"]) + return json.loads(requests.get(full_url)._content) + + def index_data(self, index, type_, id_, json_data): + full_url = "/".join([self.url, index, type_, id_]) + requests.put(full_url, data=json_data) diff --git a/common/djangoapps/search/mapping.json b/common/djangoapps/search/mapping.json index 8d4deade6d..8c4ac045ac 100644 --- a/common/djangoapps/search/mapping.json +++ b/common/djangoapps/search/mapping.json @@ -5,20 +5,7 @@ "index": "analyzed", "store": "yes", "type": "string", - "term_vector": "with_positions_offsets" - }, - - "phonetic_text": { - "boost": 1.0, - "index": "analyzed", - "store": "yes", - "type": "string", - "term_vector": "with_positions_offsets" - }, - - "uuid": { - "index": "not_analyzed", - "store": "yes", - "type": "string" + "term_vector": "with_positions_offsets", + "analyzer": "transcript_analyzer" } } \ No newline at end of file diff --git a/common/djangoapps/search/protectedWords.txt b/common/djangoapps/search/protectedWords.txt new file mode 100644 index 0000000000..5f3fce5203 --- /dev/null +++ b/common/djangoapps/search/protectedWords.txt @@ -0,0 +1,15 @@ +"gauss", +"stokes", +"navier", +"einstein", +"goddard", +"oppenheimer", +"bloch", +"hawkings", +"newton", +"bohr", +"darwin", +"planck", +"rontgen", +"tesla", +"franklin" \ No newline at end of file diff --git a/common/djangoapps/search/settings.json b/common/djangoapps/search/settings.json new file mode 100644 index 0000000000..929ec092c9 --- /dev/null +++ b/common/djangoapps/search/settings.json @@ -0,0 +1,8 @@ +{ + "settings": { + "index": { + "number_of_replicas": 2, + "number_of_shards": 3 + } + } +} \ No newline at end of file From 7eb18fe01991654aee2fdca68ea3916bc51cb0ee Mon Sep 17 00:00:00 2001 From: Anton Stupak Date: Mon, 27 May 2013 11:37:01 +0300 Subject: [PATCH 030/192] adds test files for video --- .../xmodule/xmodule/js/fixtures/video.html | 27 +++-- .../lib/xmodule/xmodule/js/spec/helper.coffee | 10 +- .../video/display/video_caption_spec.coffee | 82 ++++++++----- .../video/display/video_control_spec.coffee | 47 ++++---- .../video/display/video_player_spec.coffee | 53 ++++++--- .../display/video_progress_slider_spec.coffee | 108 ++++++++++-------- .../display/video_speed_control_spec.coffee | 7 +- .../display/video_volume_control_spec.coffee | 3 +- .../xmodule/js/spec/video/display_spec.coffee | 48 ++++---- .../js/src/video/display/video_caption.coffee | 2 +- .../display/video_progress_slider.coffee | 2 +- 11 files changed, 219 insertions(+), 170 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/fixtures/video.html b/common/lib/xmodule/xmodule/js/fixtures/video.html index 15404a89d1..e86a24dc5c 100644 --- a/common/lib/xmodule/xmodule/js/fixtures/video.html +++ b/common/lib/xmodule/xmodule/js/fixtures/video.html @@ -1,12 +1,21 @@
-
-
-
-
-
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
-
+
\ No newline at end of file diff --git a/common/lib/xmodule/xmodule/js/spec/helper.coffee b/common/lib/xmodule/xmodule/js/spec/helper.coffee index fbc89f7bd9..5cf75366d8 100644 --- a/common/lib/xmodule/xmodule/js/spec/helper.coffee +++ b/common/lib/xmodule/xmodule/js/spec/helper.coffee @@ -28,7 +28,7 @@ jasmine.stubRequests = -> spyOn($, 'ajax').andCallFake (settings) -> if match = settings.url.match /youtube\.com\/.+\/videos\/(.+)\?v=2&alt=jsonc/ settings.success data: jasmine.stubbedMetadata[match[1]] - else if match = settings.url.match /static\/subs\/(.+)\.srt\.sjson/ + else if match = settings.url.match /static(\/.*)?\/subs\/(.+)\.srt\.sjson/ settings.success jasmine.stubbedCaption else if settings.url.match /.+\/problem_get$/ settings.success html: readFixtures('problem_content.html') @@ -47,19 +47,15 @@ jasmine.stubYoutubePlayer = -> jasmine.stubVideoPlayer = (context, enableParts, createPlayer=true) -> enableParts = [enableParts] unless $.isArray(enableParts) - suite = context.suite currentPartName = suite.description while suite = suite.parentSuite enableParts.push currentPartName - for part in ['VideoCaption', 'VideoSpeedControl', 'VideoVolumeControl', 'VideoProgressSlider'] - unless $.inArray(part, enableParts) >= 0 - spyOn window, part - loadFixtures 'video.html' jasmine.stubRequests() YT.Player = undefined - context.video = new Video 'example', '.75:slowerSpeedYoutubeId,1.0:normalSpeedYoutubeId' + videosDefinition = '0.75:slowerSpeedYoutubeId,1.0:normalSpeedYoutubeId' + context.video = new Video '#example', videosDefinition jasmine.stubYoutubePlayer() if createPlayer return new VideoPlayer(video: context.video) diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee index 90e026e57e..ddf6be18db 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee @@ -1,23 +1,25 @@ -# TODO: figure out why failing -xdescribe 'VideoCaption', -> +describe 'VideoCaption', -> + beforeEach -> - jasmine.stubVideoPlayer @ - $('.subtitles').remove() + spyOn(VideoCaption.prototype, 'fetchCaption').andCallThrough() + spyOn($, 'ajaxWithPrefix').andCallThrough() + window.onTouchBasedDevice = jasmine.createSpy('onTouchBasedDevice').andReturn false afterEach -> YT.Player = undefined $.fn.scrollTo.reset() + $('.subtitles').remove() describe 'constructor', -> - beforeEach -> - spyOn($, 'getWithPrefix').andCallThrough() describe 'always', -> + beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption it 'set the youtube id', -> - expect(@caption.youtubeId).toEqual 'def456' + expect(@caption.youtubeId).toEqual 'normalSpeedYoutubeId' it 'create the caption element', -> expect($('.video')).toContain 'ol.subtitles' @@ -26,7 +28,12 @@ xdescribe 'VideoCaption', -> expect($('.video')).toContain 'a.hide-subtitles' it 'fetch the caption', -> - expect($.getWithPrefix).toHaveBeenCalledWith @caption.captionURL(), jasmine.any(Function) + expect(@caption.loaded).toBeTruthy() + expect(@caption.fetchCaption).toHaveBeenCalled() + expect($.ajaxWithPrefix).toHaveBeenCalledWith + url: @caption.captionURL() + notifyOnError: false + success: jasmine.any(Function) it 'bind window resize event', -> expect($(window)).toHandleWith 'resize', @caption.resize @@ -42,9 +49,10 @@ xdescribe 'VideoCaption', -> expect($('.subtitles')).toHandleWith 'DOMMouseScroll', @caption.onMovement describe 'when on a non touch-based device', -> + beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn false - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption it 'render the caption', -> expect($('.subtitles').html()).toMatch new RegExp(''' @@ -66,9 +74,11 @@ xdescribe 'VideoCaption', -> expect(@caption.rendered).toBeTruthy() describe 'when on a touch-based device', -> + beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn true - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + window.onTouchBasedDevice.andReturn true + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption it 'show explaination message', -> expect($('.subtitles li')).toHaveHtml "Caption will be displayed when you start playing the video." @@ -77,12 +87,15 @@ xdescribe 'VideoCaption', -> expect(@caption.rendered).toBeFalsy() describe 'mouse movement', -> + beforeEach -> - spyOn(window, 'setTimeout').andReturn 100 + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption + window.setTimeout.andReturn(100) spyOn window, 'clearTimeout' - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' describe 'when cursor is outside of the caption box', -> + beforeEach -> $(window).trigger jQuery.Event 'mousemove' @@ -90,6 +103,7 @@ xdescribe 'VideoCaption', -> expect(@caption.frozen).toBeFalsy() describe 'when cursor is in the caption box', -> + beforeEach -> $('.subtitles').trigger jQuery.Event 'mouseenter' @@ -143,8 +157,10 @@ xdescribe 'VideoCaption', -> expect($.fn.scrollTo).not.toHaveBeenCalled() describe 'search', -> + beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption it 'return a correct caption index', -> expect(@caption.search(0)).toEqual 0 @@ -157,8 +173,9 @@ xdescribe 'VideoCaption', -> describe 'play', -> describe 'when the caption was not rendered', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn true - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + window.onTouchBasedDevice.andReturn true + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption @caption.play() it 'render the caption', -> @@ -185,7 +202,8 @@ xdescribe 'VideoCaption', -> describe 'pause', -> beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption @caption.playing = true @caption.pause() @@ -193,8 +211,10 @@ xdescribe 'VideoCaption', -> expect(@caption.playing).toBeFalsy() describe 'updatePlayTime', -> + beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption describe 'when the video speed is 1.0x', -> beforeEach -> @@ -240,13 +260,15 @@ xdescribe 'VideoCaption', -> expect($('.subtitles li[data-index=1]')).toHaveClass 'current' describe 'resize', -> + beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption $('.subtitles li[data-index=1]').addClass 'current' @caption.resize() it 'set the height of caption container', -> - expect(parseInt($('.subtitles').css('maxHeight'))).toEqual $('.video-wrapper').height() + expect(parseInt($('.subtitles').css('maxHeight'))).toBeCloseTo $('.video-wrapper').height(), 5 it 'set the height of caption spacing', -> expect(parseInt($('.subtitles .spacing:first').css('height'))).toEqual( @@ -258,8 +280,10 @@ xdescribe 'VideoCaption', -> expect($.fn.scrollTo).toHaveBeenCalled() describe 'scrollCaption', -> + beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption describe 'when frozen', -> beforeEach -> @@ -291,15 +315,17 @@ xdescribe 'VideoCaption', -> offset: - ($('.video-wrapper').height() / 2 - $('.subtitles .current:first').height() / 2) describe 'seekPlayer', -> + beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption @time = null $(@caption).bind 'seek', (event, time) => @time = time describe 'when the video speed is 1.0x', -> beforeEach -> @caption.currentSpeed = '1.0' - $('.subtitles li[data-start="30000"]').click() + $('.subtitles li[data-start="30000"]').trigger('click') it 'trigger seek event with the correct time', -> expect(@time).toEqual 30.000 @@ -307,14 +333,15 @@ xdescribe 'VideoCaption', -> describe 'when the video speed is not 1.0x', -> beforeEach -> @caption.currentSpeed = '0.75' - $('.subtitles li[data-start="30000"]').click() + $('.subtitles li[data-start="30000"]').trigger('click') it 'trigger seek event with the correct time', -> expect(@time).toEqual 40.000 describe 'toggle', -> beforeEach -> - @caption = new VideoCaption el: $('.video'), youtubeId: 'def456', currentSpeed: '1.0' + @player = jasmine.stubVideoPlayer @ + @caption = @player.caption $('.subtitles li[data-index=1]').addClass 'current' describe 'when the caption is visible', -> @@ -325,7 +352,6 @@ xdescribe 'VideoCaption', -> it 'hide the caption', -> expect(@caption.el).toHaveClass 'closed' - describe 'when the caption is hidden', -> beforeEach -> @caption.el.addClass 'closed' diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_control_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_control_spec.coffee index 7603d5777f..e15b0c856a 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_control_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_control_spec.coffee @@ -1,53 +1,44 @@ -# TODO: figure out why failing -xdescribe 'VideoControl', -> +describe 'VideoControl', -> beforeEach -> - jasmine.stubVideoPlayer @ + window.onTouchBasedDevice = jasmine.createSpy('onTouchBasedDevice').andReturn false + loadFixtures 'video.html' $('.video-controls').html '' describe 'constructor', -> + it 'render the video controls', -> - new VideoControl(el: $('.video-controls')) - expect($('.video-controls').html()).toContain ''' -
-
-
    -
  • Play
    • -
  • -
    0:00 / 0:00
    -
    • -
-
- Fill Browser -
-
- ''' + @control = new window.VideoControl(el: $('.video-controls')) + expect($('.video-controls')).toContain + ['.slider', 'ul.vcr', 'a.play', '.vidtime', '.add-fullscreen'].join(',') + expect($('.video-controls').find('.vidtime')).toHaveText '0:00 / 0:00' it 'bind the playback button', -> - control = new VideoControl(el: $('.video-controls')) - expect($('.video_control')).toHandleWith 'click', control.togglePlayback + @control = new window.VideoControl(el: $('.video-controls')) + expect($('.video_control')).toHandleWith 'click', @control.togglePlayback describe 'when on a touch based device', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn true + window.onTouchBasedDevice.andReturn true + @control = new window.VideoControl(el: $('.video-controls')) it 'does not add the play class to video control', -> - new VideoControl(el: $('.video-controls')) expect($('.video_control')).not.toHaveClass 'play' expect($('.video_control')).not.toHaveHtml 'Play' describe 'when on a non-touch based device', -> + beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn false + @control = new window.VideoControl(el: $('.video-controls')) it 'add the play class to video control', -> - new VideoControl(el: $('.video-controls')) expect($('.video_control')).toHaveClass 'play' expect($('.video_control')).toHaveHtml 'Play' describe 'play', -> + beforeEach -> - @control = new VideoControl(el: $('.video-controls')) + @control = new window.VideoControl(el: $('.video-controls')) @control.play() it 'switch playback button to play state', -> @@ -56,8 +47,9 @@ xdescribe 'VideoControl', -> expect($('.video_control')).toHaveHtml 'Pause' describe 'pause', -> + beforeEach -> - @control = new VideoControl(el: $('.video-controls')) + @control = new window.VideoControl(el: $('.video-controls')) @control.pause() it 'switch playback button to pause state', -> @@ -66,8 +58,9 @@ xdescribe 'VideoControl', -> expect($('.video_control')).toHaveHtml 'Play' describe 'togglePlayback', -> + beforeEach -> - @control = new VideoControl(el: $('.video-controls')) + @control = new window.VideoControl(el: $('.video-controls')) describe 'when the control does not have play or pause class', -> beforeEach -> diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee index b6c562c88a..0fa2c6f515 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee @@ -1,6 +1,9 @@ -# TODO: figure out why failing -xdescribe 'VideoPlayer', -> +describe 'VideoPlayer', -> beforeEach -> + window.onTouchBasedDevice = jasmine.createSpy('onTouchBasedDevice').andReturn false + # It tries to call methods of VideoProgressSlider on Spy + for part in ['VideoCaption', 'VideoSpeedControl', 'VideoVolumeControl', 'VideoProgressSlider', 'VideoControl'] + spyOn(window[part].prototype, 'initialize').andCallThrough() jasmine.stubVideoPlayer @, [], false afterEach -> @@ -8,7 +11,6 @@ xdescribe 'VideoPlayer', -> describe 'constructor', -> beforeEach -> - spyOn window, 'VideoControl' spyOn YT, 'Player' $.fn.qtip.andCallFake -> $(this).data('qtip', true) @@ -22,32 +24,47 @@ xdescribe 'VideoPlayer', -> expect(@player.currentTime).toEqual 0 it 'set the element', -> - expect(@player.el).toBe '#video_example' + expect(@player.el).toHaveId 'video_id' it 'create video control', -> - expect(window.VideoControl).toHaveBeenCalledWith el: $('.video-controls', @player.el) + expect(window.VideoControl.prototype.initialize).toHaveBeenCalled() + expect(@player.control).toBeDefined() + expect(@player.control.el).toBe $('.video-controls', @player.el) it 'create video caption', -> - expect(window.VideoCaption).toHaveBeenCalledWith el: @player.el, youtubeId: 'normalSpeedYoutubeId', currentSpeed: '1.0' + expect(window.VideoCaption.prototype.initialize).toHaveBeenCalled() + expect(@player.caption).toBeDefined() + expect(@player.caption.el).toBe @player.el + expect(@player.caption.youtubeId).toEqual 'normalSpeedYoutubeId' + expect(@player.caption.currentSpeed).toEqual '1.0' + expect(@player.caption.captionAssetPath).toEqual '/static/subs/' it 'create video speed control', -> - expect(window.VideoSpeedControl).toHaveBeenCalledWith el: $('.secondary-controls', @player.el), speeds: ['0.75', '1.0'], currentSpeed: '1.0' + expect(window.VideoSpeedControl.prototype.initialize).toHaveBeenCalled() + expect(@player.speedControl).toBeDefined() + expect(@player.speedControl.el).toBe $('.secondary-controls', @player.el) + expect(@player.speedControl.speeds).toEqual ['0.75', '1.0'] + expect(@player.speedControl.currentSpeed).toEqual '1.0' it 'create video progress slider', -> - expect(window.VideoProgressSlider).toHaveBeenCalledWith el: $('.slider', @player.el) + expect(window.VideoSpeedControl.prototype.initialize).toHaveBeenCalled() + expect(@player.progressSlider).toBeDefined() + expect(@player.progressSlider.el).toBe $('.slider', @player.el) it 'create Youtube player', -> - expect(YT.Player).toHaveBeenCalledWith('example', { + expect(YT.Player).toHaveBeenCalledWith('id', { playerVars: controls: 0 wmode: 'transparent' rel: 0 showinfo: 0 enablejsapi: 1 + modestbranding: 1 videoId: 'normalSpeedYoutubeId' events: onReady: @player.onReady onStateChange: @player.onStateChange + onPlaybackQualityChange: @player.onPlaybackQualityChange }) it 'bind to video control play event', -> @@ -76,7 +93,6 @@ xdescribe 'VideoPlayer', -> describe 'when not on a touch based device', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn false $('.add-fullscreen, .hide-subtitles').removeData 'qtip' @player = new VideoPlayer video: @video @@ -85,11 +101,13 @@ xdescribe 'VideoPlayer', -> expect($('.hide-subtitles')).toHaveData 'qtip' it 'create video volume control', -> - expect(window.VideoVolumeControl).toHaveBeenCalledWith el: $('.secondary-controls', @player.el) + expect(window.VideoVolumeControl.prototype.initialize).toHaveBeenCalled() + expect(@player.volumeControl).toBeDefined() + expect(@player.volumeControl.el).toBe $('.secondary-controls', @player.el) describe 'when on a touch based device', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn true + window.onTouchBasedDevice.andReturn true $('.add-fullscreen, .hide-subtitles').removeData 'qtip' @player = new VideoPlayer video: @video @@ -98,7 +116,8 @@ xdescribe 'VideoPlayer', -> expect($('.hide-subtitles')).not.toHaveData 'qtip' it 'does not create video volume control', -> - expect(window.VideoVolumeControl).not.toHaveBeenCalled() + expect(window.VideoVolumeControl.prototype.initialize).not.toHaveBeenCalled() + expect(@player.volumeControl).not.toBeDefined() describe 'onReady', -> beforeEach -> @@ -110,7 +129,6 @@ xdescribe 'VideoPlayer', -> describe 'when not on a touch based device', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn false spyOn @player, 'play' @player.onReady() @@ -119,7 +137,7 @@ xdescribe 'VideoPlayer', -> describe 'when on a touch based device', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn true + window.onTouchBasedDevice.andReturn true spyOn @player, 'play' @player.onReady() @@ -347,9 +365,6 @@ xdescribe 'VideoPlayer', -> it 'replace the full screen button tooltip', -> expect($('.add-fullscreen')).toHaveAttr 'title', 'Exit fill browser' - it 'add a new exit from fullscreen button', -> - expect(@player.el).toContain 'a.exit' - it 'add the fullscreen class', -> expect(@player.el).toHaveClass 'fullscreen' @@ -438,7 +453,7 @@ xdescribe 'VideoPlayer', -> describe 'volume', -> beforeEach -> - @player = new VideoPlayer @video + @player = new VideoPlayer video: @video @player.player.getVolume.andReturn 42 describe 'without value', -> diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_progress_slider_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_progress_slider_spec.coffee index 99b675b1d7..bf6dada93b 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_progress_slider_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_progress_slider_spec.coffee @@ -1,31 +1,30 @@ -# TODO: figure out why failing -xdescribe 'VideoProgressSlider', -> +describe 'VideoProgressSlider', -> beforeEach -> - jasmine.stubVideoPlayer @ + window.onTouchBasedDevice = jasmine.createSpy('onTouchBasedDevice').andReturn false describe 'constructor', -> describe 'on a non-touch based device', -> beforeEach -> spyOn($.fn, 'slider').andCallThrough() - spyOn(window, 'onTouchBasedDevice').andReturn false - @slider = new VideoProgressSlider el: $('.slider') + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider it 'build the slider', -> - expect(@slider.slider).toBe '.slider' + expect(@progressSlider.slider).toBe '.slider' expect($.fn.slider).toHaveBeenCalledWith range: 'min' - change: @slider.onChange - slide: @slider.onSlide - stop: @slider.onStop + change: @progressSlider.onChange + slide: @progressSlider.onSlide + stop: @progressSlider.onStop it 'build the seek handle', -> - expect(@slider.handle).toBe '.slider .ui-slider-handle' + expect(@progressSlider.handle).toBe '.slider .ui-slider-handle' expect($.fn.qtip).toHaveBeenCalledWith content: "0:00" position: my: 'bottom center' at: 'top center' - container: @slider.handle + container: @progressSlider.handle hide: delay: 700 style: @@ -34,47 +33,51 @@ xdescribe 'VideoProgressSlider', -> describe 'on a touch-based device', -> beforeEach -> + window.onTouchBasedDevice.andReturn true spyOn($.fn, 'slider').andCallThrough() - spyOn(window, 'onTouchBasedDevice').andReturn true - @slider = new VideoProgressSlider el: $('.slider') + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider it 'does not build the slider', -> - expect(@slider.slider).toBeUndefined + expect(@progressSlider.slider).toBeUndefined expect($.fn.slider).not.toHaveBeenCalled() describe 'play', -> beforeEach -> - @slider = new VideoProgressSlider el: $('.slider') - spyOn($.fn, 'slider').andCallThrough() + spyOn(VideoProgressSlider.prototype, 'buildSlider').andCallThrough() + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider describe 'when the slider was already built', -> + beforeEach -> - @slider.play() + @progressSlider.play() it 'does not build the slider', -> - expect($.fn.slider).not.toHaveBeenCalled + expect(@progressSlider.buildSlider.calls.length).toEqual 1 describe 'when the slider was not already built', -> beforeEach -> - @slider.slider = null - @slider.play() + spyOn($.fn, 'slider').andCallThrough() + @progressSlider.slider = null + @progressSlider.play() it 'build the slider', -> - expect(@slider.slider).toBe '.slider' + expect(@progressSlider.slider).toBe '.slider' expect($.fn.slider).toHaveBeenCalledWith range: 'min' - change: @slider.onChange - slide: @slider.onSlide - stop: @slider.onStop + change: @progressSlider.onChange + slide: @progressSlider.onSlide + stop: @progressSlider.onStop it 'build the seek handle', -> - expect(@slider.handle).toBe '.ui-slider-handle' + expect(@progressSlider.handle).toBe '.ui-slider-handle' expect($.fn.qtip).toHaveBeenCalledWith content: "0:00" position: my: 'bottom center' at: 'top center' - container: @slider.handle + container: @progressSlider.handle hide: delay: 700 style: @@ -83,21 +86,23 @@ xdescribe 'VideoProgressSlider', -> describe 'updatePlayTime', -> beforeEach -> - @slider = new VideoProgressSlider el: $('.slider') - spyOn($.fn, 'slider').andCallThrough() + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider describe 'when frozen', -> beforeEach -> - @slider.frozen = true - @slider.updatePlayTime 20, 120 + spyOn($.fn, 'slider').andCallThrough() + @progressSlider.frozen = true + @progressSlider.updatePlayTime 20, 120 it 'does not update the slider', -> expect($.fn.slider).not.toHaveBeenCalled() describe 'when not frozen', -> beforeEach -> - @slider.frozen = false - @slider.updatePlayTime 20, 120 + spyOn($.fn, 'slider').andCallThrough() + @progressSlider.frozen = false + @progressSlider.updatePlayTime 20, 120 it 'update the max value of the slider', -> expect($.fn.slider).toHaveBeenCalledWith 'option', 'max', 120 @@ -107,55 +112,58 @@ xdescribe 'VideoProgressSlider', -> describe 'onSlide', -> beforeEach -> - @slider = new VideoProgressSlider el: $('.slider') + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider @time = null - $(@slider).bind 'seek', (event, time) => @time = time - spyOnEvent @slider, 'seek' - @slider.onSlide {}, value: 20 + $(@progressSlider).bind 'seek', (event, time) => @time = time + spyOnEvent @progressSlider, 'seek' + @progressSlider.onSlide {}, value: 20 it 'freeze the slider', -> - expect(@slider.frozen).toBeTruthy() + expect(@progressSlider.frozen).toBeTruthy() it 'update the tooltip', -> expect($.fn.qtip).toHaveBeenCalled() it 'trigger seek event', -> - expect('seek').toHaveBeenTriggeredOn @slider + expect('seek').toHaveBeenTriggeredOn @progressSlider expect(@time).toEqual 20 describe 'onChange', -> beforeEach -> - @slider = new VideoProgressSlider el: $('.slider') - @slider.onChange {}, value: 20 + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider + @progressSlider.onChange {}, value: 20 it 'update the tooltip', -> expect($.fn.qtip).toHaveBeenCalled() describe 'onStop', -> beforeEach -> - @slider = new VideoProgressSlider el: $('.slider') + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider @time = null - $(@slider).bind 'seek', (event, time) => @time = time - spyOnEvent @slider, 'seek' - spyOn(window, 'setTimeout') - @slider.onStop {}, value: 20 + $(@progressSlider).bind 'seek', (event, time) => @time = time + spyOnEvent @progressSlider, 'seek' + @progressSlider.onStop {}, value: 20 it 'freeze the slider', -> - expect(@slider.frozen).toBeTruthy() + expect(@progressSlider.frozen).toBeTruthy() it 'trigger seek event', -> - expect('seek').toHaveBeenTriggeredOn @slider + expect('seek').toHaveBeenTriggeredOn @progressSlider expect(@time).toEqual 20 it 'set timeout to unfreeze the slider', -> expect(window.setTimeout).toHaveBeenCalledWith jasmine.any(Function), 200 window.setTimeout.mostRecentCall.args[0]() - expect(@slider.frozen).toBeFalsy() + expect(@progressSlider.frozen).toBeFalsy() describe 'updateTooltip', -> beforeEach -> - @slider = new VideoProgressSlider el: $('.slider') - @slider.updateTooltip 90 + @player = jasmine.stubVideoPlayer @ + @progressSlider = @player.progressSlider + @progressSlider.updateTooltip 90 it 'set the tooltip value', -> expect($.fn.qtip).toHaveBeenCalledWith 'option', 'content.text', '1:30' diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee index a7af239094..ac321b8e97 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee @@ -1,6 +1,6 @@ -# TODO: figure out why failing -xdescribe 'VideoSpeedControl', -> +describe 'VideoSpeedControl', -> beforeEach -> + window.onTouchBasedDevice = jasmine.createSpy('onTouchBasedDevice').andReturn false jasmine.stubVideoPlayer @ $('.speeds').remove() @@ -25,7 +25,7 @@ xdescribe 'VideoSpeedControl', -> describe 'when running on touch based device', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn true + window.onTouchBasedDevice.andReturn true $('.speeds').removeClass 'open' @speedControl = new VideoSpeedControl el: $('.secondary-controls'), speeds: @video.speeds, currentSpeed: '1.0' @@ -37,7 +37,6 @@ xdescribe 'VideoSpeedControl', -> describe 'when running on non-touch based device', -> beforeEach -> - spyOn(window, 'onTouchBasedDevice').andReturn false $('.speeds').removeClass 'open' @speedControl = new VideoSpeedControl el: $('.secondary-controls'), speeds: @video.speeds, currentSpeed: '1.0' diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_volume_control_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_volume_control_spec.coffee index 41ac5dd3e4..a2b14afa55 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_volume_control_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_volume_control_spec.coffee @@ -1,5 +1,4 @@ -# TODO: figure out why failing -xdescribe 'VideoVolumeControl', -> +describe 'VideoVolumeControl', -> beforeEach -> jasmine.stubVideoPlayer @ $('.volume').remove() diff --git a/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee index ac90310519..a83fa3905c 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee @@ -1,12 +1,20 @@ -# TODO: figure out why failing -xdescribe 'Video', -> +describe 'Video', -> + metadata = undefined + beforeEach -> loadFixtures 'video.html' jasmine.stubRequests() - @videosDefinition = '.75:slowerSpeedYoutubeId,1.0:normalSpeedYoutubeId' + @videosDefinition = '0.75:slowerSpeedYoutubeId,1.0:normalSpeedYoutubeId' @slowerSpeedYoutubeId = 'slowerSpeedYoutubeId' @normalSpeedYoutubeId = 'normalSpeedYoutubeId' + metadata = + slowerSpeedYoutubeId: + id: @slowerSpeedYoutubeId + duration: 300 + normalSpeedYoutubeId: + id: @normalSpeedYoutubeId + duration: 200 afterEach -> window.player = undefined @@ -16,17 +24,18 @@ xdescribe 'Video', -> beforeEach -> @stubVideoPlayer = jasmine.createSpy('VideoPlayer') $.cookie.andReturn '0.75' - window.player = 100 + window.player = undefined describe 'by default', -> beforeEach -> - @video = new Video 'example', @videosDefinition - + spyOn(window.Video.prototype, 'fetchMetadata').andCallFake -> + @metadata = metadata + @video = new Video '#example', @videosDefinition it 'reset the current video player', -> expect(window.player).toBeNull() it 'set the elements', -> - expect(@video.el).toBe '#video_example' + expect(@video.el).toBe '#video_id' it 'parse the videos', -> expect(@video.videos).toEqual @@ -34,13 +43,8 @@ xdescribe 'Video', -> '1.0': @normalSpeedYoutubeId it 'fetch the video metadata', -> - expect(@video.metadata).toEqual - slowerSpeedYoutubeId: - id: @slowerSpeedYoutubeId - duration: 300 - normalSpeedYoutubeId: - id: @normalSpeedYoutubeId - duration: 200 + expect(@video.fetchMetadata).toHaveBeenCalled + expect(@video.metadata).toEqual metadata it 'parse available video speeds', -> expect(@video.speeds).toEqual ['0.75', '1.0'] @@ -56,7 +60,7 @@ xdescribe 'Video', -> @originalYT = window.YT window.YT = { Player: true } spyOn(window, 'VideoPlayer').andReturn(@stubVideoPlayer) - @video = new Video 'example', @videosDefinition + @video = new Video '#example', @videosDefinition afterEach -> window.YT = @originalYT @@ -69,7 +73,7 @@ xdescribe 'Video', -> beforeEach -> @originalYT = window.YT window.YT = {} - @video = new Video 'example', @videosDefinition + @video = new Video '#example', @videosDefinition afterEach -> window.YT = @originalYT @@ -82,7 +86,7 @@ xdescribe 'Video', -> @originalYT = window.YT window.YT = {} spyOn(window, 'VideoPlayer').andReturn(@stubVideoPlayer) - @video = new Video 'example', @videosDefinition + @video = new Video '#example', @videosDefinition window.onYouTubePlayerAPIReady() afterEach -> @@ -95,7 +99,7 @@ xdescribe 'Video', -> describe 'youtubeId', -> beforeEach -> $.cookie.andReturn '1.0' - @video = new Video 'example', @videosDefinition + @video = new Video '#example', @videosDefinition describe 'with speed', -> it 'return the video id for given speed', -> @@ -108,7 +112,7 @@ xdescribe 'Video', -> describe 'setSpeed', -> beforeEach -> - @video = new Video 'example', @videosDefinition + @video = new Video '#example', @videosDefinition describe 'when new speed is available', -> beforeEach -> @@ -129,14 +133,14 @@ xdescribe 'Video', -> describe 'getDuration', -> beforeEach -> - @video = new Video 'example', @videosDefinition + @video = new Video '#example', @videosDefinition it 'return duration for current video', -> expect(@video.getDuration()).toEqual 200 describe 'log', -> beforeEach -> - @video = new Video 'example', @videosDefinition + @video = new Video '#example', @videosDefinition @video.setSpeed '1.0' spyOn Logger, 'log' @video.player = { currentTime: 25 } @@ -144,7 +148,7 @@ xdescribe 'Video', -> it 'call the logger with valid parameters', -> expect(Logger.log).toHaveBeenCalledWith 'someEvent', - id: 'example' + id: 'id' code: @normalSpeedYoutubeId currentTime: 25 speed: '1.0' diff --git a/common/lib/xmodule/xmodule/js/src/video/display/video_caption.coffee b/common/lib/xmodule/xmodule/js/src/video/display/video_caption.coffee index bf3ec1e102..c72067b0dc 100644 --- a/common/lib/xmodule/xmodule/js/src/video/display/video_caption.coffee +++ b/common/lib/xmodule/xmodule/js/src/video/display/video_caption.coffee @@ -37,7 +37,7 @@ class @VideoCaption extends Subview @loaded = true if onTouchBasedDevice() - $('.subtitles li').html "Caption will be displayed when you start playing the video." + $('.subtitles').html "
  • Caption will be displayed when you start playing the video.
    • " else @renderCaption() diff --git a/common/lib/xmodule/xmodule/js/src/video/display/video_progress_slider.coffee b/common/lib/xmodule/xmodule/js/src/video/display/video_progress_slider.coffee index 874756cb71..ef2f38698b 100644 --- a/common/lib/xmodule/xmodule/js/src/video/display/video_progress_slider.coffee +++ b/common/lib/xmodule/xmodule/js/src/video/display/video_progress_slider.coffee @@ -11,7 +11,7 @@ class @VideoProgressSlider extends Subview @buildHandle() buildHandle: -> - @handle = @$('.slider .ui-slider-handle') + @handle = @$('.ui-slider-handle') @handle.qtip content: "#{Time.format(@slider.slider('value'))}" position: From 8b06916eb6e6d15723d1fe66695a2537e1b46a51 Mon Sep 17 00:00:00 2001 From: Anto Stupak Date: Wed, 29 May 2013 15:53:00 +0300 Subject: [PATCH 031/192] Fix tests for firefox --- .../video/display/video_caption_spec.coffee | 31 +++++++++---------- .../video/display/video_player_spec.coffee | 2 +- .../display/video_speed_control_spec.coffee | 19 ++++++------ .../js/src/video/display/video_player.coffee | 2 +- 4 files changed, 26 insertions(+), 28 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee index ddf6be18db..8c63751f07 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee @@ -55,12 +55,11 @@ describe 'VideoCaption', -> @caption = @player.caption it 'render the caption', -> - expect($('.subtitles').html()).toMatch new RegExp(''' -
  • Caption at 0
    • -
  • Caption at 10000
    • -
  • Caption at 20000
    • -
  • Caption at 30000
    • - '''.replace(/\n/g, '')) + captionsData = jasmine.stubbedCaption + $('.subtitles li[data-index]').each (index, link) => + expect($(link)).toHaveData 'index', index + expect($(link)).toHaveData 'start', captionsData.start[index] + expect($(link)).toHaveText captionsData.text[index] it 'add a padding element to caption', -> expect($('.subtitles li:first')).toBe '.spacing' @@ -179,12 +178,11 @@ describe 'VideoCaption', -> @caption.play() it 'render the caption', -> - expect($('.subtitles').html()).toMatch new RegExp( - '''
  • Caption at 0
    • ''' + - '''
  • Caption at 10000
    • ''' + - '''
  • Caption at 20000
    • ''' + - '''
  • Caption at 30000
    • ''' - ) + captionsData = jasmine.stubbedCaption + $('.subtitles li[data-index]').each (index, link) => + expect($(link)).toHaveData 'index', index + expect($(link)).toHaveData 'start', captionsData.start[index] + expect($(link)).toHaveText captionsData.text[index] it 'add a padding element to caption', -> expect($('.subtitles li:first')).toBe '.spacing' @@ -268,13 +266,12 @@ describe 'VideoCaption', -> @caption.resize() it 'set the height of caption container', -> - expect(parseInt($('.subtitles').css('maxHeight'))).toBeCloseTo $('.video-wrapper').height(), 5 + expect(parseInt($('.subtitles').css('maxHeight'))).toBeCloseTo $('.video-wrapper').height(), 2 it 'set the height of caption spacing', -> - expect(parseInt($('.subtitles .spacing:first').css('height'))).toEqual( - $('.video-wrapper').height() / 2 - $('.subtitles li:not(.spacing):first').height() / 2) - expect(parseInt($('.subtitles .spacing:last').css('height'))).toEqual( - $('.video-wrapper').height() / 2 - $('.subtitles li:not(.spacing):last').height() / 2) + expect(Math.abs(parseInt($('.subtitles .spacing:first').css('height')) - @caption.topSpacingHeight())).toBeLessThan 1 + expect(Math.abs(parseInt($('.subtitles .spacing:last').css('height')) - @caption.bottomSpacingHeight())).toBeLessThan 1 + it 'scroll caption to new position', -> expect($.fn.scrollTo).toHaveBeenCalled() diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee index 0fa2c6f515..dab8c0815a 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee @@ -86,7 +86,7 @@ describe 'VideoPlayer', -> expect($(@player.volumeControl)).toHandleWith 'volumeChange', @player.onVolumeChange it 'bind to key press', -> - expect($(document)).toHandleWith 'keyup', @player.bindExitFullScreen + expect($(document.documentElement)).toHandleWith 'keyup', @player.bindExitFullScreen it 'bind to fullscreen switching button', -> expect($('.add-fullscreen')).toHandleWith 'click', @player.toggleFullScreen diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee index ac321b8e97..687f90e030 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_speed_control_spec.coffee @@ -10,15 +10,16 @@ describe 'VideoSpeedControl', -> @speedControl = new VideoSpeedControl el: $('.secondary-controls'), speeds: @video.speeds, currentSpeed: '1.0' it 'add the video speed control to player', -> - expect($('.secondary-controls').html()).toContain ''' -
    - -

    Speed

    -

    1.0x

    -     -
    1. 1.0x
    2. 0.75x
    -
    - ''' + secondaryControls = $('.secondary-controls') + li = secondaryControls.find('.video_speeds li') + expect(secondaryControls).toContain '.speeds' + expect(secondaryControls).toContain '.video_speeds' + expect(secondaryControls.find('p.active').text()).toBe '1.0x' + expect(li.filter('.active')).toHaveData 'speed', @speedControl.currentSpeed + expect(li.length).toBe @speedControl.speeds.length + $.each li.toArray().reverse(), (index, link) => + expect($(link)).toHaveData 'speed', @speedControl.speeds[index] + expect($(link).find('a').text()).toBe @speedControl.speeds[index] + 'x' it 'bind to change video speed link', -> expect($('.video_speeds a')).toHandleWith 'click', @speedControl.changeVideoSpeed diff --git a/common/lib/xmodule/xmodule/js/src/video/display/video_player.coffee b/common/lib/xmodule/xmodule/js/src/video/display/video_player.coffee index 561ca07c8a..73ff3512e2 100644 --- a/common/lib/xmodule/xmodule/js/src/video/display/video_player.coffee +++ b/common/lib/xmodule/xmodule/js/src/video/display/video_player.coffee @@ -15,7 +15,7 @@ class @VideoPlayer extends Subview $(@progressSlider).bind('seek', @onSeek) if @volumeControl $(@volumeControl).bind('volumeChange', @onVolumeChange) - $(document).keyup @bindExitFullScreen + $(document.documentElement).keyup @bindExitFullScreen @$('.add-fullscreen').click @toggleFullScreen @addToolTip() unless onTouchBasedDevice() From b25710346f44c3e0bdbc69928eba39fc8abc3106 Mon Sep 17 00:00:00 2001 From: Vasyl Nakvasiuk Date: Thu, 30 May 2013 11:22:20 +0300 Subject: [PATCH 032/192] add python video tests --- lms/djangoapps/courseware/tests/__init__.py | 97 +++++++++++++ .../courseware/tests/test_video_mongo.py | 49 +++++++ .../courseware/tests/test_video_xml.py | 137 ++++++++++++++++++ 3 files changed, 283 insertions(+) create mode 100644 lms/djangoapps/courseware/tests/test_video_mongo.py create mode 100644 lms/djangoapps/courseware/tests/test_video_xml.py diff --git a/lms/djangoapps/courseware/tests/__init__.py b/lms/djangoapps/courseware/tests/__init__.py index e69de29bb2..dd3c4dc2b3 100644 --- a/lms/djangoapps/courseware/tests/__init__.py +++ b/lms/djangoapps/courseware/tests/__init__.py @@ -0,0 +1,97 @@ +""" +integration tests for xmodule + +Contains: + + 1. BaseTestXmodule class provides course and users + for testing Xmodules with mongo store. +""" + +from django.test.utils import override_settings +from django.core.urlresolvers import reverse +from django.test.client import Client + +from student.tests.factories import UserFactory, CourseEnrollmentFactory +from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE +from xmodule.modulestore import Location +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class BaseTestXmodule(ModuleStoreTestCase): + """Base class for testing Xmodules with mongo store. + + This class prepares course and users for tests: + 1. create test course + 2. create, enrol and login users for this course + + Any xmodule should overwrite only next parameters for test: + 1. TEMPLATE_NAME + 2. DATA + 3. COURSE_DATA and USER_COUNT if needed + + This class should not contain any tests, because TEMPLATE_NAME + should be defined in child class. + """ + USER_COUNT = 2 + COURSE_DATA = {} + + # Data from YAML common/lib/xmodule/xmodule/templates/NAME/default.yaml + TEMPLATE_NAME = "" + DATA = {} + + def setUp(self): + + self.course = CourseFactory.create(data=self.COURSE_DATA) + + # Turn off cache. + modulestore().request_cache = None + modulestore().metadata_inheritance_cache_subsystem = None + + chapter = ItemFactory.create( + parent_location=self.course.location, + template="i4x://edx/templates/sequential/Empty", + ) + section = ItemFactory.create( + parent_location=chapter.location, + template="i4x://edx/templates/sequential/Empty" + ) + + # username = robot{0}, password = 'test' + self.users = [ + UserFactory.create(username='robot%d' % i, email='robot+test+%d@edx.org' % i) + for i in range(self.USER_COUNT) + ] + + for user in self.users: + CourseEnrollmentFactory.create(user=user, course_id=self.course.id) + + item = ItemFactory.create( + parent_location=section.location, + template=self.TEMPLATE_NAME, + data=self.DATA + ) + self.item_url = Location(item.location).url() + + # login all users for acces to Xmodule + self.clients = {user.username: Client() for user in self.users} + self.login_statuses = [ + self.clients[user.username].login( + username=user.username, password='test') + for user in self.users + ] + + self.assertTrue(all(self.login_statuses)) + + def get_url(self, dispatch): + """Return word cloud url with dispatch.""" + return reverse( + 'modx_dispatch', + args=(self.course.id, self.item_url, dispatch) + ) + + def tearDown(self): + for user in self.users: + user.delete() diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py new file mode 100644 index 0000000000..f979ae2686 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -0,0 +1,49 @@ +# -*- coding: utf-8 -*- +"""Video xmodule tests in mongo.""" + +import json + +from . import BaseTestXmodule + + +class TestVideo(BaseTestXmodule): + """Integration tests: web client + mongo.""" + + TEMPLATE_NAME = "i4x://edx/templates/video/default" + DATA = '