From c64f0b1a180897feae71c47ae52df237d4124427 Mon Sep 17 00:00:00 2001 From: dcadams Date: Mon, 29 Apr 2013 09:38:10 -0700 Subject: [PATCH 001/141] 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/141] 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/141] 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/141] 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/141] 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/141] 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 34d1ea9e63093bfeba1fa1239d16158d364a99af Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 30 May 2013 15:09:39 -0400 Subject: [PATCH 007/141] Consolidated coverage report generation into one rake command. Added diff coverage reports using diff-coverage tool. --- doc/testing.md | 5 +++-- jenkins/test.sh | 3 ++- rakefiles/tests.rake | 31 +++++++++---------------------- 3 files changed, 14 insertions(+), 25 deletions(-) diff --git a/doc/testing.md b/doc/testing.md index e5d035d90e..bc58bc387d 100644 --- a/doc/testing.md +++ b/doc/testing.md @@ -197,9 +197,10 @@ To view test coverage: 2. Generate reports: - rake coverage:html + rake coverage -3. HTML reports are located in the `reports` folder. +3. Reports are located in the `reports` folder. The command +generates HTML and XML (Cobertura format) reports. ## Testing using queue servers diff --git a/jenkins/test.sh b/jenkins/test.sh index 35be3a0121..51566f6fb5 100755 --- a/jenkins/test.sh +++ b/jenkins/test.sh @@ -84,7 +84,8 @@ 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 -rake coverage:xml coverage:html +# Generate coverage reports +rake coverage [ $TESTS_FAILED == '0' ] rake autodeploy_properties diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index 448a482f04..ac7037508c 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -119,30 +119,17 @@ task :test do end end -namespace :coverage do - desc "Build the html coverage reports" - task :html => :report_dirs do - TEST_TASK_DIRS.each do |dir| - report_dir = report_dir_path(dir) +desc "Build the html, xml, and diff coverage reports" +task :coverage => :report_dirs do + TEST_TASK_DIRS.each do |dir| + report_dir = report_dir_path(dir) - if !File.file?("#{report_dir}/.coverage") - next - end - - sh("coverage html --rcfile=#{dir}/.coveragerc") + if !File.file?("#{report_dir}/.coverage") + next end - end - desc "Build the xml coverage reports" - task :xml => :report_dirs do - TEST_TASK_DIRS.each do |dir| - report_dir = report_dir_path(dir) - - if !File.file?("#{report_dir}/.coverage") - next - end - # Why doesn't the rcfile control the xml output file properly?? - sh("coverage xml -o #{report_dir}/coverage.xml --rcfile=#{dir}/.coveragerc") - end + sh("coverage html --rcfile=#{dir}/.coveragerc") + sh("coverage xml -o #{report_dir}/coverage.xml --rcfile=#{dir}/.coveragerc") + sh("diff-cover #{report_dir}/coverage.xml --html-report #{report_dir}/diff_cover.html --git-branch master...HEAD") end end From 76ae9800060d329f7c08ee941c53aff489ad2c8b Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 3 Jun 2013 12:19:13 -0400 Subject: [PATCH 008/141] Clean the report directories before each test run, to ensure we don't get coverage results from a previous test run. --- rakefiles/tests.rake | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index ac7037508c..ee862d631d 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -45,6 +45,11 @@ end directory REPORT_DIR task :clean_test_files do + + # Delete all non-folder files in the reports directory + sh("find #{REPORT_DIR} -type f -print0 | xargs -0 rm") + + # Reset the test fixtures sh("git clean -fqdx test_root") end @@ -81,12 +86,11 @@ TEST_TASK_DIRS = [] end Dir["common/lib/*"].select{|lib| File.directory?(lib)}.each do |lib| - task_name = "test_#{lib}" report_dir = report_dir_path(lib) desc "Run tests for common lib #{lib}" - task task_name => report_dir do + task "test_#{lib}" => ["clean_test_files", 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| @@ -95,10 +99,13 @@ Dir["common/lib/*"].select{|lib| File.directory?(lib)}.each do |lib| end TEST_TASK_DIRS << lib - desc "Run tests for common lib #{lib} (without coverage)" - task "fasttest_#{lib}" do - sh("nosetests #{lib}") - end + # There used to be a fasttest_#{lib} command that ran without coverage. + # However, this is an inconsistent usage of "fast": + # When running tests for lms and cms, "fast" means skipping + # staticfiles collection, but still running under coverage. + # We keep the fasttest_#{lib} command for backwards compatibility, + # but make it an alias to the normal test command. + task "fasttest_#{lib}" => "test_#{lib}" end task :report_dirs From afa6f485fdaf672c0f915f1a203c26320f4fdb85 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 3 Jun 2013 13:26:01 -0400 Subject: [PATCH 009/141] Added diff-cover as a requirement (installed from github) --- rakefiles/tests.rake | 3 ++- requirements/edx/github.txt | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index ee862d631d..7e6d12c40f 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -46,7 +46,8 @@ directory REPORT_DIR task :clean_test_files do - # Delete all non-folder files in the reports directory + # Delete all files in the reports directory, while preserving + # the directory structure. sh("find #{REPORT_DIR} -type f -print0 | xargs -0 rm") # Reset the test fixtures diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index b1aef0a108..bcf1b415d7 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -10,3 +10,4 @@ # Our libraries: -e git+https://github.com/edx/XBlock.git@2144a25d#egg=XBlock -e git+https://github.com/edx/codejail.git@5fb5fa0#egg=codejail +-e git+https://github.com/edx/diff-cover.git@v0.1#egg=diff_cover From 1ea65455565f2369c220c0a9ce4dfa5376740411 Mon Sep 17 00:00:00 2001 From: dcadams Date: Tue, 4 Jun 2013 14:13:13 -0700 Subject: [PATCH 010/141] 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 9c506020d9f8536c756f143f0a7082d36d46b8bb Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 4 Jun 2013 17:51:12 -0400 Subject: [PATCH 011/141] Fixed bug in tests.rake in which rm would be called with no args. --- rakefiles/tests.rake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index 7e6d12c40f..b8919bdf90 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -48,7 +48,7 @@ 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 -0 rm") + sh("find #{REPORT_DIR} -type f -print0 | xargs --no-run-if-empty -0 rm") # Reset the test fixtures sh("git clean -fqdx test_root") @@ -138,6 +138,6 @@ task :coverage => :report_dirs do sh("coverage html --rcfile=#{dir}/.coveragerc") sh("coverage xml -o #{report_dir}/coverage.xml --rcfile=#{dir}/.coveragerc") - sh("diff-cover #{report_dir}/coverage.xml --html-report #{report_dir}/diff_cover.html --git-branch master...HEAD") + sh("diff-cover #{report_dir}/coverage.xml --html-report #{report_dir}/diff_cover.html") end end From 5662ecfde99424cecafa06605d9fc4f9b940a21f Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 4 Jun 2013 18:13:42 -0400 Subject: [PATCH 012/141] Updated to current tag of diff-cover --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index bcf1b415d7..fc9070bba3 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -10,4 +10,4 @@ # Our libraries: -e git+https://github.com/edx/XBlock.git@2144a25d#egg=XBlock -e git+https://github.com/edx/codejail.git@5fb5fa0#egg=codejail --e git+https://github.com/edx/diff-cover.git@v0.1#egg=diff_cover +-e git+https://github.com/edx/diff-cover.git@v0.1.0#egg=diff_cover From 99c185f06bf1d7ef83b5b7c05c56a9c251870856 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 4 Jun 2013 18:39:49 -0400 Subject: [PATCH 013/141] Added diff coverage printout to rake coverage command --- rakefiles/tests.rake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index b8919bdf90..528290cee5 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -139,5 +139,7 @@ task :coverage => :report_dirs do sh("coverage html --rcfile=#{dir}/.coveragerc") sh("coverage xml -o #{report_dir}/coverage.xml --rcfile=#{dir}/.coveragerc") sh("diff-cover #{report_dir}/coverage.xml --html-report #{report_dir}/diff_cover.html") + sh("diff-cover #{report_dir}/coverage.xml") + puts "\n\n" end end From e1d3fb73012b5af3c5ec81f4577b4d9d1fe1927c Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 4 Jun 2013 18:46:45 -0400 Subject: [PATCH 014/141] Added warning message when no coverage information found. Added comments to tests.rake --- rakefiles/tests.rake | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/rakefiles/tests.rake b/rakefiles/tests.rake index 528290cee5..904ea4a54e 100644 --- a/rakefiles/tests.rake +++ b/rakefiles/tests.rake @@ -129,17 +129,33 @@ end desc "Build the html, xml, and diff coverage reports" task :coverage => :report_dirs do + + found_coverage_info = false + TEST_TASK_DIRS.each do |dir| report_dir = report_dir_path(dir) if !File.file?("#{report_dir}/.coverage") next + else + found_coverage_info = true end + # Generate the coverage.py HTML report sh("coverage html --rcfile=#{dir}/.coveragerc") + + # Generate the coverage.py XML report sh("coverage xml -o #{report_dir}/coverage.xml --rcfile=#{dir}/.coveragerc") + + # Generate the diff coverage HTML report, based on the XML report sh("diff-cover #{report_dir}/coverage.xml --html-report #{report_dir}/diff_cover.html") + + # Print the diff coverage report to the console sh("diff-cover #{report_dir}/coverage.xml") - puts "\n\n" + puts "\n" + end + + if not found_coverage_info + puts "No coverage info found. Run `rake test` before running `rake coverage`." end end From a8ec1ca9d5804823772a6634cbf2cbee6bb8f426 Mon Sep 17 00:00:00 2001 From: dcadams Date: Tue, 4 Jun 2013 17:08:34 -0700 Subject: [PATCH 015/141] 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 016/141] 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 4391783248ee1338e086175723910e486745ca5e Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 5 Jun 2013 12:40:09 -0400 Subject: [PATCH 017/141] add exporting of 'about' content as well as adding unit test checks --- cms/djangoapps/contentstore/tests/test_contentstore.py | 3 +++ common/lib/xmodule/xmodule/modulestore/xml_exporter.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index aebfb91126..771871e9bc 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -486,6 +486,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # check for custom_tags self.verify_content_existence(module_store, root_dir, location, 'custom_tags', 'custom_tag_template') + # check for about content + self.verify_content_existence(module_store, root_dir, location, 'about', 'about', '.html') + # check for graiding_policy.json filesystem = OSFS(root_dir / 'test_export/policies/6.002_Spring_2012') self.assertTrue(filesystem.exists('grading_policy.json')) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 50bdf3a252..9fceb51c51 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -28,6 +28,9 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d # export the course updates export_extra_content(export_fs, modulestore, course_location, 'course_info', 'info', '.html') + # export the 'about' data (e.g. overview, etc.) + export_extra_content(export_fs, modulestore, course_location, 'about', 'about', '.html') + # export the grading policy policies_dir = export_fs.makeopendir('policies') course_run_policy_dir = policies_dir.makeopendir(course.location.name) From 16fc7b37fb2da07ea26db98139a0c2094235a290 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 5 Jun 2013 11:12:47 -0400 Subject: [PATCH 018/141] 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 019/141] 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 020/141] 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 f2c40a71fe3b1d775241efa7109dd897fb834521 Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Fri, 31 May 2013 15:17:45 -0700 Subject: [PATCH 021/141] Enable theming in base LMS template Provide the appropriate switches to adjust based on whether or not a theme (in particular, the Stanford theme) is enabled in the settings. For now, these changes are very specific to Stanford. This is because the template architecture needs some reworking to generalize nicely. --- lms/templates/main.html | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/lms/templates/main.html b/lms/templates/main.html index 313025d09a..d6cbe21095 100644 --- a/lms/templates/main.html +++ b/lms/templates/main.html @@ -1,9 +1,28 @@ <%namespace name='static' file='static_content.html'/> <%! from django.utils import html %> + +## Define a couple of helper functions to make life easier when +## embedding theme conditionals into templates. All inheriting +## templates have access to these functions, and we can import these +## into non-inheriting templates via the %namespace tag. +<%def name="theme_enabled()"> + <% return settings.MITX_FEATURES["USE_CUSTOM_THEME"] %> + + +<%def name="stanford_theme_enabled()"> + <% return theme_enabled() and getattr(settings, "THEME_NAME") == "stanford" %> + + - <%block name="title">edX + <%block name="title"> + % if stanford_theme_enabled(): + Home | class.stanford.edu + % else: + edX + % endif + - + <%static:css group='application'/> From f055c6f9893439bdae6148eb313124ef32dc6ae5 Mon Sep 17 00:00:00 2001 From: Nate Hardison Date: Fri, 31 May 2013 15:37:00 -0700 Subject: [PATCH 023/141] Inject theming hooks into navigation bar Allow themes to inherit from the default navigation bar and override pieces of it, including the main logo, the links that display to the right of the logo, and the links inside the dropdown menu (with the exception of the `Log Out` link. In addition, this adds an empty block at the very top so that themes can place a branding bar at the top of the page. (Stanford identity guidelines require this: see https://identity.stanford.edu.) --- lms/templates/navigation.html | 51 ++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index 13c461173b..104366c8c4 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -1,6 +1,6 @@ ## mako <%namespace name='static' file='static_content.html'/> -<%namespace file='main.html' import="login_query"/> +<%namespace file='main.html' import="login_query, stanford_theme_enabled"/> <%! from django.core.urlresolvers import reverse @@ -10,6 +10,9 @@ import branding from status.status import get_site_status_msg %> +## Provide a hook for themes to inject branding on top. +<%block name="navigation_top" /> + <%block cached="False"> <% try: @@ -38,9 +41,12 @@ site_status_msg = get_site_status_msg(course_id)