From 9dd182800b46f781ef56a0a15ca620021f0f385d Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 8 Nov 2013 11:56:12 -0500 Subject: [PATCH 01/69] Set empty aws credentials to None. --- cms/envs/aws.py | 7 ++++++- lms/envs/aws.py | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/cms/envs/aws.py b/cms/envs/aws.py index 2e53a555a5..9cdfd79b34 100644 --- a/cms/envs/aws.py +++ b/cms/envs/aws.py @@ -156,9 +156,14 @@ SEGMENT_IO_KEY = AUTH_TOKENS.get('SEGMENT_IO_KEY') if SEGMENT_IO_KEY: MITX_FEATURES['SEGMENT_IO'] = ENV_TOKENS.get('SEGMENT_IO', False) - AWS_ACCESS_KEY_ID = AUTH_TOKENS["AWS_ACCESS_KEY_ID"] +if AWS_ACCESS_KEY_ID == "": + AWS_ACCESS_KEY_ID = None + AWS_SECRET_ACCESS_KEY = AUTH_TOKENS["AWS_SECRET_ACCESS_KEY"] +if AWS_SECRET_ACCESS_KEY == "": + AWS_SECRET_ACCESS_KEY = None + DATABASES = AUTH_TOKENS['DATABASES'] MODULESTORE = AUTH_TOKENS['MODULESTORE'] CONTENTSTORE = AUTH_TOKENS['CONTENTSTORE'] diff --git a/lms/envs/aws.py b/lms/envs/aws.py index d524474d5b..0262c9a5d4 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -243,7 +243,13 @@ CC_PROCESSOR = AUTH_TOKENS.get('CC_PROCESSOR', CC_PROCESSOR) SECRET_KEY = AUTH_TOKENS['SECRET_KEY'] AWS_ACCESS_KEY_ID = AUTH_TOKENS["AWS_ACCESS_KEY_ID"] +if AWS_ACCESS_KEY_ID == "": + AWS_ACCESS_KEY_ID = None + AWS_SECRET_ACCESS_KEY = AUTH_TOKENS["AWS_SECRET_ACCESS_KEY"] +if AWS_SECRET_ACCESS_KEY == "": + AWS_SECRET_ACCESS_KEY = None + AWS_STORAGE_BUCKET_NAME = AUTH_TOKENS.get('AWS_STORAGE_BUCKET_NAME', 'edxuploads') DATABASES = AUTH_TOKENS['DATABASES'] From 2a31e3567e623feffd74cb861d40c792d5909344 Mon Sep 17 00:00:00 2001 From: John Jarvis Date: Wed, 6 Nov 2013 17:07:02 -0500 Subject: [PATCH 02/69] sending template pdf with certificate request --- .../management/commands/ungenerated_certs.py | 1 + .../0015_adding_mode_for_verified_certs.py | 86 +++++++++++++++++++ lms/djangoapps/certificates/models.py | 5 ++ lms/djangoapps/certificates/queue.py | 75 +++++++++------- 4 files changed, 136 insertions(+), 31 deletions(-) create mode 100644 lms/djangoapps/certificates/migrations/0015_adding_mode_for_verified_certs.py diff --git a/lms/djangoapps/certificates/management/commands/ungenerated_certs.py b/lms/djangoapps/certificates/management/commands/ungenerated_certs.py index 5fb9c53718..5aa223acab 100644 --- a/lms/djangoapps/certificates/management/commands/ungenerated_certs.py +++ b/lms/djangoapps/certificates/management/commands/ungenerated_certs.py @@ -93,6 +93,7 @@ class Command(BaseCommand): total = enrolled_students.count() count = 0 start = datetime.datetime.now(UTC) + for student in enrolled_students: count += 1 if count % STATUS_INTERVAL == 0: diff --git a/lms/djangoapps/certificates/migrations/0015_adding_mode_for_verified_certs.py b/lms/djangoapps/certificates/migrations/0015_adding_mode_for_verified_certs.py new file mode 100644 index 0000000000..c16d51b8ee --- /dev/null +++ b/lms/djangoapps/certificates/migrations/0015_adding_mode_for_verified_certs.py @@ -0,0 +1,86 @@ +# -*- 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 'GeneratedCertificate.mode' + db.add_column('certificates_generatedcertificate', 'mode', + self.gf('django.db.models.fields.CharField')(default='honor', max_length=32), + keep_default=False) + + + def backwards(self, orm): + # Deleting field 'GeneratedCertificate.mode' + db.delete_column('certificates_generatedcertificate', 'mode') + + + 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'}) + }, + 'certificates.certificatewhitelist': { + 'Meta': {'object_name': 'CertificateWhitelist'}, + 'course_id': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '255', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}), + 'whitelist': ('django.db.models.fields.BooleanField', [], {'default': 'False'}) + }, + 'certificates.generatedcertificate': { + 'Meta': {'unique_together': "(('user', 'course_id'),)", 'object_name': 'GeneratedCertificate'}, + 'course_id': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '255', 'blank': 'True'}), + 'created_date': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'auto_now_add': 'True', 'blank': 'True'}), + 'distinction': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'download_url': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '128', 'blank': 'True'}), + 'download_uuid': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '32', 'blank': 'True'}), + 'error_reason': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '512', 'blank': 'True'}), + 'grade': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '5', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'key': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '32', 'blank': 'True'}), + 'mode': ('django.db.models.fields.CharField', [], {'default': "'honor'", 'max_length': '32'}), + 'modified_date': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'auto_now': 'True', 'blank': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'blank': 'True'}), + 'status': ('django.db.models.fields.CharField', [], {'default': "'unavailable'", 'max_length': '32'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}), + 'verify_uuid': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '32', 'blank': 'True'}) + }, + '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'}) + } + } + + complete_apps = ['certificates'] \ No newline at end of file diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 8cd1a292c4..36ff18618e 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -62,6 +62,10 @@ class CertificateStatuses(object): restricted = 'restricted' unavailable = 'unavailable' +class CertificateModes(object): + verified = 'verified' + honor = 'honor' + audit = 'audit' class CertificateWhitelist(models.Model): """ @@ -86,6 +90,7 @@ class GeneratedCertificate(models.Model): key = models.CharField(max_length=32, blank=True, default='') distinction = models.BooleanField(default=False) status = models.CharField(max_length=32, default='unavailable') + mode = models.CharField(max_length=32, default=CertificateModes.honor) name = models.CharField(blank=True, max_length=255) created_date = models.DateTimeField( auto_now_add=True, default=datetime.now) diff --git a/lms/djangoapps/certificates/queue.py b/lms/djangoapps/certificates/queue.py index 5f63bbf1e2..33db940c44 100644 --- a/lms/djangoapps/certificates/queue.py +++ b/lms/djangoapps/certificates/queue.py @@ -9,7 +9,7 @@ from capa.xqueue_interface import XQueueInterface from capa.xqueue_interface import make_xheader, make_hashkey from django.conf import settings from requests.auth import HTTPBasicAuth -from student.models import UserProfile +from student.models import UserProfile, CourseEnrollment import json import random @@ -57,7 +57,7 @@ class XQueueCertInterface(object): if settings.XQUEUE_INTERFACE.get('basic_auth') is not None: requests_auth = HTTPBasicAuth( - *settings.XQUEUE_INTERFACE['basic_auth']) + *settings.XQUEUE_INTERFACE['basic_auth']) else: requests_auth = None @@ -68,10 +68,10 @@ class XQueueCertInterface(object): self.request = request self.xqueue_interface = XQueueInterface( - settings.XQUEUE_INTERFACE['url'], - settings.XQUEUE_INTERFACE['django_auth'], - requests_auth, - ) + settings.XQUEUE_INTERFACE['url'], + settings.XQUEUE_INTERFACE['django_auth'], + requests_auth, + ) self.whitelist = CertificateWhitelist.objects.all() self.restricted = UserProfile.objects.filter(allow_certificate=False) self.use_https = True @@ -84,7 +84,7 @@ class XQueueCertInterface(object): course_id - courseenrollment.course_id (string) WARNING: this command will leave the old certificate, if one exists, - laying around in AWS taking up space. If this is a problem, + laying around in AWS taking up space. If this is a problem, take pains to clean up storage before running this command. Change the certificate status to unavailable (if it exists) and request @@ -92,7 +92,7 @@ class XQueueCertInterface(object): Return the status object. """ - # TODO: when del_cert is implemented and plumbed through certificates + # TODO: when del_cert is implemented and plumbed through certificates # repo also, do a deletion followed by a creation r/t a simple # recreation. XXX: this leaves orphan cert files laying around in # AWS. See note in the docstring too. @@ -149,13 +149,15 @@ class XQueueCertInterface(object): """ VALID_STATUSES = [status.generating, - status.unavailable, - status.deleted, + status.unavailable, + status.deleted, status.error, status.notpassing] cert_status = certificate_status_for_student(student, course_id)['status'] + new_status = cert_status + if cert_status in VALID_STATUSES: # grade the student @@ -165,9 +167,6 @@ class XQueueCertInterface(object): course = courses.get_course_by_id(course_id) profile = UserProfile.objects.get(user=student) - cert, created = GeneratedCertificate.objects.get_or_create( - user=student, course_id=course_id) - # Needed self.request.user = student self.request.session = {} @@ -175,45 +174,59 @@ class XQueueCertInterface(object): grade = grades.grade(student, self.request, course) is_whitelisted = self.whitelist.filter( user=student, course_id=course_id, whitelist=True).exists() + enrollment = CourseEnrollment.objects.get(user=student) + org = course_id.split('/')[0] + course_num = course_id.split('/')[1] + if enrolment.mode == CertificateModes.verified: + template_pdf = "certificate-template-{0}-{1}-verified.pdf".format( + org, course_num) + else: + # honor code and audit students + template_pdf = "certificate-template-{0}-{1}.pdf".format( + org, course_num) + + cert, created = GeneratedCertificate.objects.get_or_create( + user=student, course_id=course_id) + + cert.mode = enrollment.mode + + cert.user = student + cert.grade = grade['percent'] + cert.course_id = course_id + cert.name = profile.name if is_whitelisted or grade['grade'] is not None: - key = make_hashkey(random.random()) - - cert.grade = grade['percent'] - cert.user = student - cert.course_id = course_id - cert.key = key - cert.name = profile.name - # check to see whether the student is on the # the embargoed country restricted list # otherwise, put a new certificate request # on the queue + if self.restricted.filter(user=student).exists(): - cert.status = status.restricted + new_status = status.restricted + cert.status = new_status cert.save() else: + key = make_hashkey(random.random()) + cert.key = key contents = { 'action': 'create', 'username': student.username, 'course_id': course_id, 'name': profile.name, 'grade': grade['grade'], + 'template_pdf': template_pdf, } - cert.status = status.generating + new_status = status.generating + cert.status = new_status cert.save() self._send_to_xqueue(contents, key) else: - cert_status = status.notpassing - cert.grade = grade['percent'] - cert.user = student - cert.course_id = course_id - cert.name = profile.name - cert.status = cert_status + new_status = status.notpassing + cert.status = new_status cert.save() - return cert_status + return new_status def _send_to_xqueue(self, contents, key): @@ -227,7 +240,7 @@ class XQueueCertInterface(object): proto, settings.SITE_NAME, key), key, settings.CERT_QUEUE) (error, msg) = self.xqueue_interface.send_to_queue( - header=xheader, body=json.dumps(contents)) + header=xheader, body=json.dumps(contents)) if error: logger.critical('Unable to add a request to the queue: {} {}'.format(error, msg)) raise Exception('Unable to send queue message') From 2fed61814a4ee63e2753933e816b8680647608b9 Mon Sep 17 00:00:00 2001 From: John Jarvis Date: Wed, 6 Nov 2013 17:32:57 -0500 Subject: [PATCH 03/69] fixing some typos --- lms/djangoapps/certificates/queue.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/certificates/queue.py b/lms/djangoapps/certificates/queue.py index 33db940c44..8dffa7ee24 100644 --- a/lms/djangoapps/certificates/queue.py +++ b/lms/djangoapps/certificates/queue.py @@ -1,7 +1,7 @@ from certificates.models import GeneratedCertificate from certificates.models import certificate_status_for_student from certificates.models import CertificateStatuses as status -from certificates.models import CertificateWhitelist +from certificates.models import CertificateWhitelist, CertificateModes from courseware import grades, courses from django.test.client import RequestFactory @@ -177,12 +177,12 @@ class XQueueCertInterface(object): enrollment = CourseEnrollment.objects.get(user=student) org = course_id.split('/')[0] course_num = course_id.split('/')[1] - if enrolment.mode == CertificateModes.verified: - template_pdf = "certificate-template-{0}-{1}-verified.pdf".format( + if enrollment.mode == CertificateModes.verified: + template_pdf = "certificate-template-{0}-{1}-verified.pdf".format( org, course_num) else: # honor code and audit students - template_pdf = "certificate-template-{0}-{1}.pdf".format( + template_pdf = "certificate-template-{0}-{1}.pdf".format( org, course_num) cert, created = GeneratedCertificate.objects.get_or_create( From 1b7a871926848beecd88c86db55c7e2d36296fe9 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Fri, 15 Nov 2013 22:18:31 +0000 Subject: [PATCH 04/69] Fixed password reset message LMS-1507 --- common/djangoapps/student/views.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index f92ffe9d3e..d4a03dca37 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -1229,11 +1229,8 @@ def password_reset(request): from_email=settings.DEFAULT_FROM_EMAIL, request=request, domain_override=request.get_host()) - return HttpResponse(json.dumps({'success': True, + return HttpResponse(json.dumps({'success': True, 'value': render_to_string('registration/password_reset_done.html', {})})) - else: - return HttpResponse(json.dumps({'success': False, - 'error': _('Invalid e-mail or user')})) def password_reset_confirm_wrapper( @@ -1515,4 +1512,4 @@ def change_email_settings(request): log.info(u"User {0} ({1}) opted out of receiving emails from course {2}".format(user.username, user.email, course_id)) track.views.server_track(request, "change-email-settings", {"receive_emails": "no", "course": course_id}, page='dashboard') - return HttpResponse(json.dumps({'success': True})) \ No newline at end of file + return HttpResponse(json.dumps({'success': True})) \ No newline at end of file From c8a98051dd99b68da27d4e276b964b1c85beee04 Mon Sep 17 00:00:00 2001 From: Jason Bau Date: Fri, 15 Nov 2013 21:28:03 -0800 Subject: [PATCH 05/69] CSV Reporting of Shopping Cart Purchases, with tests squashing to one commit to make cherry-picking by feature possible --- lms/djangoapps/shoppingcart/admin.py | 7 + ...nannotation__add_field_orderitem_report.py | 132 +++++++++++++++ lms/djangoapps/shoppingcart/models.py | 90 ++++++++++- .../shoppingcart/tests/test_models.py | 85 +++++++++- .../shoppingcart/tests/test_views.py | 150 +++++++++++++++++- lms/djangoapps/shoppingcart/urls.py | 1 + lms/djangoapps/shoppingcart/views.py | 74 +++++++++ lms/envs/aws.py | 4 + lms/envs/common.py | 8 + lms/static/sass/views/_shoppingcart.scss | 8 + .../shoppingcart/download_report.html | 29 ++++ requirements/edx/base.txt | 1 + 12 files changed, 582 insertions(+), 7 deletions(-) create mode 100644 lms/djangoapps/shoppingcart/admin.py create mode 100644 lms/djangoapps/shoppingcart/migrations/0005_auto__add_paidcourseregistrationannotation__add_field_orderitem_report.py create mode 100644 lms/templates/shoppingcart/download_report.html diff --git a/lms/djangoapps/shoppingcart/admin.py b/lms/djangoapps/shoppingcart/admin.py new file mode 100644 index 0000000000..199a39d7c0 --- /dev/null +++ b/lms/djangoapps/shoppingcart/admin.py @@ -0,0 +1,7 @@ +""" +Allows django admin site to add PaidCourseRegistrationAnnotations +""" +from ratelimitbackend import admin +from shoppingcart.models import PaidCourseRegistrationAnnotation + +admin.site.register(PaidCourseRegistrationAnnotation) diff --git a/lms/djangoapps/shoppingcart/migrations/0005_auto__add_paidcourseregistrationannotation__add_field_orderitem_report.py b/lms/djangoapps/shoppingcart/migrations/0005_auto__add_paidcourseregistrationannotation__add_field_orderitem_report.py new file mode 100644 index 0000000000..04d37c730a --- /dev/null +++ b/lms/djangoapps/shoppingcart/migrations/0005_auto__add_paidcourseregistrationannotation__add_field_orderitem_report.py @@ -0,0 +1,132 @@ +# -*- 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 model 'PaidCourseRegistrationAnnotation' + db.create_table('shoppingcart_paidcourseregistrationannotation', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('course_id', self.gf('django.db.models.fields.CharField')(unique=True, max_length=128, db_index=True)), + ('annotation', self.gf('django.db.models.fields.TextField')(null=True)), + )) + db.send_create_signal('shoppingcart', ['PaidCourseRegistrationAnnotation']) + + # Adding field 'OrderItem.report_comments' + db.add_column('shoppingcart_orderitem', 'report_comments', + self.gf('django.db.models.fields.TextField')(default=''), + keep_default=False) + + + def backwards(self, orm): + # Deleting model 'PaidCourseRegistrationAnnotation' + db.delete_table('shoppingcart_paidcourseregistrationannotation') + + # Deleting field 'OrderItem.report_comments' + db.delete_column('shoppingcart_orderitem', 'report_comments') + + + 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'}) + }, + 'shoppingcart.certificateitem': { + 'Meta': {'object_name': 'CertificateItem', '_ormbases': ['shoppingcart.OrderItem']}, + 'course_enrollment': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['student.CourseEnrollment']"}), + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'mode': ('django.db.models.fields.SlugField', [], {'max_length': '50'}), + 'orderitem_ptr': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['shoppingcart.OrderItem']", 'unique': 'True', 'primary_key': 'True'}) + }, + 'shoppingcart.order': { + 'Meta': {'object_name': 'Order'}, + 'bill_to_cardtype': ('django.db.models.fields.CharField', [], {'max_length': '32', 'blank': 'True'}), + 'bill_to_ccnum': ('django.db.models.fields.CharField', [], {'max_length': '8', 'blank': 'True'}), + 'bill_to_city': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}), + 'bill_to_country': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}), + 'bill_to_first': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}), + 'bill_to_last': ('django.db.models.fields.CharField', [], {'max_length': '64', 'blank': 'True'}), + 'bill_to_postalcode': ('django.db.models.fields.CharField', [], {'max_length': '16', 'blank': 'True'}), + 'bill_to_state': ('django.db.models.fields.CharField', [], {'max_length': '8', 'blank': 'True'}), + 'bill_to_street1': ('django.db.models.fields.CharField', [], {'max_length': '128', 'blank': 'True'}), + 'bill_to_street2': ('django.db.models.fields.CharField', [], {'max_length': '128', 'blank': 'True'}), + 'currency': ('django.db.models.fields.CharField', [], {'default': "'usd'", 'max_length': '8'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'processor_reply_dump': ('django.db.models.fields.TextField', [], {'blank': 'True'}), + 'purchase_time': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'blank': 'True'}), + 'status': ('django.db.models.fields.CharField', [], {'default': "'cart'", 'max_length': '32'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) + }, + 'shoppingcart.orderitem': { + 'Meta': {'object_name': 'OrderItem'}, + 'currency': ('django.db.models.fields.CharField', [], {'default': "'usd'", 'max_length': '8'}), + 'fulfilled_time': ('django.db.models.fields.DateTimeField', [], {'null': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'line_desc': ('django.db.models.fields.CharField', [], {'default': "'Misc. Item'", 'max_length': '1024'}), + 'order': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['shoppingcart.Order']"}), + 'qty': ('django.db.models.fields.IntegerField', [], {'default': '1'}), + 'report_comments': ('django.db.models.fields.TextField', [], {'default': "''"}), + 'status': ('django.db.models.fields.CharField', [], {'default': "'cart'", 'max_length': '32'}), + 'unit_cost': ('django.db.models.fields.DecimalField', [], {'default': '0.0', 'max_digits': '30', 'decimal_places': '2'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) + }, + 'shoppingcart.paidcourseregistration': { + 'Meta': {'object_name': 'PaidCourseRegistration', '_ormbases': ['shoppingcart.OrderItem']}, + 'course_id': ('django.db.models.fields.CharField', [], {'max_length': '128', 'db_index': 'True'}), + 'mode': ('django.db.models.fields.SlugField', [], {'default': "'honor'", 'max_length': '50'}), + 'orderitem_ptr': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['shoppingcart.OrderItem']", 'unique': 'True', 'primary_key': 'True'}) + }, + 'shoppingcart.paidcourseregistrationannotation': { + 'Meta': {'object_name': 'PaidCourseRegistrationAnnotation'}, + 'annotation': ('django.db.models.fields.TextField', [], {'null': 'True'}), + 'course_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '128', 'db_index': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}) + }, + 'student.courseenrollment': { + 'Meta': {'ordering': "('user', 'course_id')", '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'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'mode': ('django.db.models.fields.CharField', [], {'default': "'honor'", 'max_length': '100'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}) + } + } + + complete_apps = ['shoppingcart'] \ No newline at end of file diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index 03be80861a..f8422dbefc 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/lms/djangoapps/shoppingcart/models.py @@ -2,6 +2,7 @@ from datetime import datetime import pytz import logging import smtplib +import unicodecsv from model_utils.managers import InheritanceManager from collections import namedtuple @@ -207,6 +208,8 @@ class OrderItem(models.Model): line_desc = models.CharField(default="Misc. Item", max_length=1024) currency = models.CharField(default="usd", max_length=8) # lower case ISO currency codes fulfilled_time = models.DateTimeField(null=True) + # general purpose field, not user-visible. Used for reporting + report_comments = models.TextField(default="") @property def line_cost(self): @@ -254,6 +257,66 @@ class OrderItem(models.Model): """ return self.pk_with_subclass, set([]) + @classmethod + def purchased_items_btw_dates(cls, start_date, end_date): + """ + Returns a QuerySet of the purchased items between start_date and end_date inclusive. + """ + return cls.objects.filter( + status="purchased", + fulfilled_time__gte=start_date, + fulfilled_time__lt=end_date, + ) + + @classmethod + def csv_purchase_report_btw_dates(cls, filelike, start_date, end_date): + """ + Outputs a CSV report into "filelike" (a file-like python object, such as an actual file, an HttpRequest, + or sys.stdout) of purchased items between start_date and end_date inclusive. + Opening and closing filelike (if applicable) should be taken care of by the caller + """ + items = cls.purchased_items_btw_dates(start_date, end_date).order_by("fulfilled_time") + + writer = unicodecsv.writer(filelike, encoding="utf-8") + writer.writerow(OrderItem.csv_report_header_row()) + + for item in items: + writer.writerow(item.csv_report_row) + + @classmethod + def csv_report_header_row(cls): + """ + Returns the "header" row for a csv report of purchases + """ + return [ + "Purchase Time", + "Order ID", + "Status", + "Quantity", + "Unit Cost", + "Total Cost", + "Currency", + "Description", + "Comments" + ] + + @property + def csv_report_row(self): + """ + Returns an array which can be fed into csv.writer to write out one csv row + """ + return [ + self.fulfilled_time, + self.order_id, # pylint: disable=no-member + self.status, + self.qty, + self.unit_cost, + self.line_cost, + self.currency, + self.line_desc, + self.report_comments, + ] + @property def pk_with_subclass(self): """ @@ -345,13 +408,13 @@ class PaidCourseRegistration(OrderItem): item, created = cls.objects.get_or_create(order=order, user=order.user, course_id=course_id) item.status = order.status - item.mode = course_mode.slug item.qty = 1 item.unit_cost = cost item.line_desc = 'Registration for Course: {0}'.format(course.display_name_with_default) item.currency = currency order.currency = currency + item.report_comments = item.csv_report_comments order.save() item.save() log.info("User {} added course registration {} to cart: order {}" @@ -391,6 +454,31 @@ class PaidCourseRegistration(OrderItem): return self.pk_with_subclass, set([notification]) + @property + def csv_report_comments(self): + """ + Tries to fetch an annotation associated with the course_id from the database. If not found, returns u"". + Otherwise returns the annotation + """ + try: + return PaidCourseRegistrationAnnotation.objects.get(course_id=self.course_id).annotation + except PaidCourseRegistrationAnnotation.DoesNotExist: + return u"" + + +class PaidCourseRegistrationAnnotation(models.Model): + """ + A model that maps course_id to an additional annotation. This is specifically needed because when Stanford + generates report for the paid courses, each report item must contain the payment account associated with a course. + And unfortunately we didn't have the concept of a "SKU" or stock item where we could keep this association, + so this is to retrofit it. + """ + course_id = models.CharField(unique=True, max_length=128, db_index=True) + annotation = models.TextField(null=True) + + def __unicode__(self): + return u"{} : {}".format(self.course_id, self.annotation) + class CertificateItem(OrderItem): """ diff --git a/lms/djangoapps/shoppingcart/tests/test_models.py b/lms/djangoapps/shoppingcart/tests/test_models.py index ecb76ac941..a7196aa2a1 100644 --- a/lms/djangoapps/shoppingcart/tests/test_models.py +++ b/lms/djangoapps/shoppingcart/tests/test_models.py @@ -2,6 +2,8 @@ Tests for the Shopping Cart Models """ import smtplib +import StringIO +from textwrap import dedent from boto.exception import BotoServerError # this is a super-class of SESError and catches connection errors from mock import patch, MagicMock @@ -15,7 +17,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE from shoppingcart.models import (Order, OrderItem, CertificateItem, InvalidCartItem, PaidCourseRegistration, - OrderItemSubclassPK) + OrderItemSubclassPK, PaidCourseRegistrationAnnotation) from student.tests.factories import UserFactory from student.models import CourseEnrollment from course_modes.models import CourseMode @@ -321,6 +323,87 @@ class PaidCourseRegistrationTest(ModuleStoreTestCase): self.assertFalse(CourseEnrollment.is_enrolled(self.user, self.course_id)) +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class PurchaseReportTest(ModuleStoreTestCase): + + FIVE_MINS = datetime.timedelta(minutes=5) + TEST_ANNOTATION = u'Ba\xfc\u5305' + + def setUp(self): + self.user = UserFactory.create() + self.course_id = "MITx/999/Robot_Super_Course" + self.cost = 40 + self.course = CourseFactory.create(org='MITx', number='999', display_name=u'Robot Super Course') + course_mode = CourseMode(course_id=self.course_id, + mode_slug="honor", + mode_display_name="honor cert", + min_price=self.cost) + course_mode.save() + course_mode2 = CourseMode(course_id=self.course_id, + mode_slug="verified", + mode_display_name="verified cert", + min_price=self.cost) + course_mode2.save() + self.annotation = PaidCourseRegistrationAnnotation(course_id=self.course_id, annotation=self.TEST_ANNOTATION) + self.annotation.save() + self.cart = Order.get_cart_for_user(self.user) + self.reg = PaidCourseRegistration.add_to_order(self.cart, self.course_id) + self.cert_item = CertificateItem.add_to_order(self.cart, self.course_id, self.cost, 'verified') + self.cart.purchase() + self.now = datetime.datetime.now(pytz.UTC) + + def test_purchased_items_btw_dates(self): + purchases = OrderItem.purchased_items_btw_dates(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS) + self.assertEqual(len(purchases), 2) + self.assertIn(self.reg.orderitem_ptr, purchases) + self.assertIn(self.cert_item.orderitem_ptr, purchases) + no_purchases = OrderItem.purchased_items_btw_dates(self.now + self.FIVE_MINS, + self.now + self.FIVE_MINS + self.FIVE_MINS) + self.assertFalse(no_purchases) + + test_time = datetime.datetime.now(pytz.UTC) + + CORRECT_CSV = dedent(""" + Purchase Time,Order ID,Status,Quantity,Unit Cost,Total Cost,Currency,Description,Comments + {time_str},1,purchased,1,40,40,usd,Registration for Course: Robot Super Course,Ba\xc3\xbc\xe5\x8c\x85 + {time_str},1,purchased,1,40,40,usd,"Certificate of Achievement, verified cert for course Robot Super Course", + """.format(time_str=str(test_time))) + + def test_purchased_csv(self): + """ + Tests that a generated purchase report CSV is as we expect + """ + # coerce the purchase times to self.test_time so that the test can match. + # It's pretty hard to patch datetime.datetime b/c it's a python built-in, which is immutable, so we + # make the times match this way + for item in OrderItem.purchased_items_btw_dates(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS): + item.fulfilled_time = self.test_time + item.save() + + # add annotation to the + csv_file = StringIO.StringIO() + OrderItem.csv_purchase_report_btw_dates(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS) + csv = csv_file.getvalue() + csv_file.close() + # Using excel mode csv, which automatically ends lines with \r\n, so need to convert to \n + self.assertEqual(csv.replace('\r\n', '\n').strip(), self.CORRECT_CSV.strip()) + + def test_csv_report_no_annotation(self): + """ + Fill in gap in test coverage. csv_report_comments for PaidCourseRegistration instance with no + matching annotation + """ + # delete the matching annotation + self.annotation.delete() + self.assertEqual(u"", self.reg.csv_report_comments) + + def test_paidcourseregistrationannotation_unicode(self): + """ + Fill in gap in test coverage. __unicode__ method of PaidCourseRegistrationAnnotation + """ + self.assertEqual(unicode(self.annotation), u'{} : {}'.format(self.course_id, self.TEST_ANNOTATION)) + + @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) class CertificateItemTest(ModuleStoreTestCase): """ diff --git a/lms/djangoapps/shoppingcart/tests/test_views.py b/lms/djangoapps/shoppingcart/tests/test_views.py index d60cab78d9..0451277ce2 100644 --- a/lms/djangoapps/shoppingcart/tests/test_views.py +++ b/lms/djangoapps/shoppingcart/tests/test_views.py @@ -3,23 +3,23 @@ Tests for Shopping Cart views """ from urlparse import urlparse +from django.conf import settings from django.test import TestCase from django.test.utils import override_settings from django.core.urlresolvers import reverse from django.utils.translation import ugettext as _ +from django.contrib.auth.models import Group from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.exceptions import ItemNotFoundError from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE -from shoppingcart.views import add_course_to_cart -from shoppingcart.models import Order, OrderItem, CertificateItem, InvalidCartItem, PaidCourseRegistration +from shoppingcart.views import _can_download_report, _get_date_from_str +from shoppingcart.models import Order, CertificateItem, PaidCourseRegistration, OrderItem from student.tests.factories import UserFactory from student.models import CourseEnrollment from course_modes.models import CourseMode -from ..exceptions import PurchasedCallbackException from mitxmako.shortcuts import render_to_response -from shoppingcart.processors import render_purchase_form_html, process_postpay_callback +from shoppingcart.processors import render_purchase_form_html from mock import patch, Mock @@ -232,3 +232,143 @@ class ShoppingCartViewsTests(ModuleStoreTestCase): self.assertEqual(resp.status_code, 200) ((template, _context), _tmp) = render_mock.call_args self.assertEqual(template, cert_item.single_item_receipt_template) + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class CSVReportViewsTest(ModuleStoreTestCase): + """ + Test suite for CSV Purchase Reporting + """ + def setUp(self): + self.user = UserFactory.create() + self.user.set_password('password') + self.user.save() + self.course_id = "MITx/999/Robot_Super_Course" + self.cost = 40 + self.course = CourseFactory.create(org='MITx', number='999', display_name='Robot Super Course') + self.course_mode = CourseMode(course_id=self.course_id, + mode_slug="honor", + mode_display_name="honor cert", + min_price=self.cost) + self.course_mode.save() + self.verified_course_id = 'org/test/Test_Course' + CourseFactory.create(org='org', number='test', run='course1', display_name='Test Course') + self.cart = Order.get_cart_for_user(self.user) + self.dl_grp = Group(name=settings.PAYMENT_REPORT_GENERATOR_GROUP) + self.dl_grp.save() + + def login_user(self): + """ + Helper fn to login self.user + """ + self.client.login(username=self.user.username, password="password") + + def add_to_download_group(self, user): + """ + Helper fn to add self.user to group that's allowed to download report CSV + """ + user.groups.add(self.dl_grp) + + def test_report_csv_no_access(self): + self.login_user() + response = self.client.get(reverse('payment_csv_report')) + self.assertEqual(response.status_code, 403) + + def test_report_csv_bad_method(self): + self.login_user() + self.add_to_download_group(self.user) + response = self.client.put(reverse('payment_csv_report')) + self.assertEqual(response.status_code, 400) + + @patch('shoppingcart.views.render_to_response', render_mock) + def test_report_csv_get(self): + self.login_user() + self.add_to_download_group(self.user) + response = self.client.get(reverse('payment_csv_report')) + + ((template, context), unused_kwargs) = render_mock.call_args + self.assertEqual(template, 'shoppingcart/download_report.html') + self.assertFalse(context['total_count_error']) + self.assertFalse(context['date_fmt_error']) + self.assertIn(_("Download Purchase Report"), response.content) + + @patch('shoppingcart.views.render_to_response', render_mock) + def test_report_csv_bad_date(self): + self.login_user() + self.add_to_download_group(self.user) + response = self.client.post(reverse('payment_csv_report'), {'start_date': 'BAD', 'end_date': 'BAD'}) + + ((template, context), unused_kwargs) = render_mock.call_args + self.assertEqual(template, 'shoppingcart/download_report.html') + self.assertFalse(context['total_count_error']) + self.assertTrue(context['date_fmt_error']) + self.assertIn(_("There was an error in your date input. It should be formatted as YYYY-MM-DD"), + response.content) + + @patch('shoppingcart.views.render_to_response', render_mock) + @override_settings(PAYMENT_REPORT_MAX_ITEMS=0) + def test_report_csv_too_long(self): + PaidCourseRegistration.add_to_order(self.cart, self.course_id) + self.cart.purchase() + self.login_user() + self.add_to_download_group(self.user) + response = self.client.post(reverse('payment_csv_report'), {'start_date': '1970-01-01', + 'end_date': '2100-01-01'}) + + ((template, context), unused_kwargs) = render_mock.call_args + self.assertEqual(template, 'shoppingcart/download_report.html') + self.assertTrue(context['total_count_error']) + self.assertFalse(context['date_fmt_error']) + self.assertIn(_("There are too many results in your report.") + " (>0)", response.content) + + # just going to ignored the date in this test, since we already deal with date testing + # in test_models.py + CORRECT_CSV_NO_DATE = ",1,purchased,1,40,40,usd,Registration for Course: Robot Super Course," + + def test_report_csv(self): + PaidCourseRegistration.add_to_order(self.cart, self.course_id) + self.cart.purchase() + self.login_user() + self.add_to_download_group(self.user) + response = self.client.post(reverse('payment_csv_report'), {'start_date': '1970-01-01', + 'end_date': '2100-01-01'}) + self.assertEqual(response['Content-Type'], 'text/csv') + self.assertIn(",".join(OrderItem.csv_report_header_row()), response.content) + self.assertIn(self.CORRECT_CSV_NO_DATE, response.content) + + +class UtilFnsTest(TestCase): + """ + Tests for utility functions in views.py + """ + def setUp(self): + self.user = UserFactory.create() + + def test_can_download_report_no_group(self): + """ + Group controlling perms is not present + """ + self.assertFalse(_can_download_report(self.user)) + + def test_can_download_report_not_member(self): + """ + User is not part of group controlling perms + """ + Group(name=settings.PAYMENT_REPORT_GENERATOR_GROUP).save() + self.assertFalse(_can_download_report(self.user)) + + def test_can_download_report(self): + """ + User is part of group controlling perms + """ + grp = Group(name=settings.PAYMENT_REPORT_GENERATOR_GROUP) + grp.save() + self.user.groups.add(grp) + self.assertTrue(_can_download_report(self.user)) + + def test_get_date_from_str(self): + test_str = "2013-10-01" + date = _get_date_from_str(test_str) + self.assertEqual(2013, date.year) + self.assertEqual(10, date.month) + self.assertEqual(1, date.day) diff --git a/lms/djangoapps/shoppingcart/urls.py b/lms/djangoapps/shoppingcart/urls.py index 9522d15298..3653c91524 100644 --- a/lms/djangoapps/shoppingcart/urls.py +++ b/lms/djangoapps/shoppingcart/urls.py @@ -12,6 +12,7 @@ if settings.MITX_FEATURES['ENABLE_SHOPPING_CART']: url(r'^clear/$', 'clear_cart'), url(r'^remove_item/$', 'remove_item'), url(r'^add/course/(?P[^/]+/[^/]+/[^/]+)/$', 'add_course_to_cart', name='add_course_to_cart'), + url(r'^csv_report/$', 'csv_report', name='payment_csv_report'), ) if settings.MITX_FEATURES.get('ENABLE_PAYMENT_FAKE'): diff --git a/lms/djangoapps/shoppingcart/views.py b/lms/djangoapps/shoppingcart/views.py index 8c6d61d532..ad7ef6b080 100644 --- a/lms/djangoapps/shoppingcart/views.py +++ b/lms/djangoapps/shoppingcart/views.py @@ -1,4 +1,8 @@ import logging +import datetime +import pytz +from django.conf import settings +from django.contrib.auth.models import Group from django.http import (HttpResponse, HttpResponseRedirect, HttpResponseNotFound, HttpResponseBadRequest, HttpResponseForbidden, Http404) from django.utils.translation import ugettext as _ @@ -121,3 +125,73 @@ def show_receipt(request, ordernum): context.update(order_items[0].single_item_receipt_context) return render_to_response(receipt_template, context) + + +def _can_download_report(user): + """ + Tests if the user can download the payments report, based on membership in a group whose name is determined + in settings. If the group does not exist, denies all access + """ + try: + access_group = Group.objects.get(name=settings.PAYMENT_REPORT_GENERATOR_GROUP) + except Group.DoesNotExist: + return False + return access_group in user.groups.all() + + +def _get_date_from_str(date_input): + """ + Gets date from the date input string. Lets the ValueError raised by invalid strings be processed by the caller + """ + return datetime.datetime.strptime(date_input.strip(), "%Y-%m-%d").replace(tzinfo=pytz.UTC) + + +def _render_report_form(start_str, end_str, total_count_error=False, date_fmt_error=False): + """ + Helper function that renders the purchase form. Reduces repetition + """ + context = { + 'total_count_error': total_count_error, + 'date_fmt_error': date_fmt_error, + 'start_date': start_str, + 'end_date': end_str, + } + return render_to_response('shoppingcart/download_report.html', context) + + +@login_required +def csv_report(request): + """ + Downloads csv reporting of orderitems + """ + if not _can_download_report(request.user): + return HttpResponseForbidden(_('You do not have permission to view this page.')) + + if request.method == 'POST': + start_str = request.POST.get('start_date', '') + end_str = request.POST.get('end_date', '') + try: + start_date = _get_date_from_str(start_str) + end_date = _get_date_from_str(end_str) + datetime.timedelta(days=1) + except ValueError: + # Error case: there was a badly formatted user-input date string + return _render_report_form(start_str, end_str, date_fmt_error=True) + + items = OrderItem.purchased_items_btw_dates(start_date, end_date) + if items.count() > settings.PAYMENT_REPORT_MAX_ITEMS: + # Error case: too many items would be generated in the report and we're at risk of timeout + return _render_report_form(start_str, end_str, total_count_error=True) + + response = HttpResponse(mimetype='text/csv') + filename = "purchases_report_{}.csv".format(datetime.datetime.now(pytz.UTC).strftime("%Y-%m-%d-%H-%M-%S")) + response['Content-Disposition'] = 'attachment; filename="{}"'.format(filename) + OrderItem.csv_purchase_report_btw_dates(response, start_date, end_date) + return response + + elif request.method == 'GET': + end_date = datetime.datetime.now(pytz.UTC) + start_date = end_date - datetime.timedelta(days=30) + return _render_report_form(start_date.strftime("%Y-%m-%d"), end_date.strftime("%Y-%m-%d")) + + else: + return HttpResponseBadRequest("HTTP Method Not Supported") diff --git a/lms/envs/aws.py b/lms/envs/aws.py index d524474d5b..204f317d8e 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -148,6 +148,10 @@ PAYMENT_SUPPORT_EMAIL = ENV_TOKENS.get('PAYMENT_SUPPORT_EMAIL', PAYMENT_SUPPORT_ PAID_COURSE_REGISTRATION_CURRENCY = ENV_TOKENS.get('PAID_COURSE_REGISTRATION_CURRENCY', PAID_COURSE_REGISTRATION_CURRENCY) +# Payment Report Settings +PAYMENT_REPORT_GENERATOR_GROUP = ENV_TOKENS.get('PAYMENT_REPORT_GENERATOR_GROUP', PAYMENT_REPORT_GENERATOR_GROUP) +PAYMENT_REPORT_MAX_ITEMS = ENV_TOKENS.get('PAYMENT_REPORT_MAX_ITEMS', PAYMENT_REPORT_MAX_ITEMS) + # Bulk Email overrides BULK_EMAIL_DEFAULT_FROM_EMAIL = ENV_TOKENS.get('BULK_EMAIL_DEFAULT_FROM_EMAIL', BULK_EMAIL_DEFAULT_FROM_EMAIL) BULK_EMAIL_EMAILS_PER_TASK = ENV_TOKENS.get('BULK_EMAIL_EMAILS_PER_TASK', BULK_EMAIL_EMAILS_PER_TASK) diff --git a/lms/envs/common.py b/lms/envs/common.py index c698ce24be..376fdb2405 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -542,6 +542,12 @@ CC_PROCESSOR = { } # Setting for PAID_COURSE_REGISTRATION, DOES NOT AFFECT VERIFIED STUDENTS PAID_COURSE_REGISTRATION_CURRENCY = ['usd', '$'] + +# Members of this group are allowed to generate payment reports +PAYMENT_REPORT_GENERATOR_GROUP = 'shoppingcart_report_access' +# Maximum number of rows the report can contain +PAYMENT_REPORT_MAX_ITEMS = 10000 + ################################# open ended grading config ##################### #By setting up the default settings with an incorrect user name and password, @@ -899,6 +905,8 @@ BULK_EMAIL_LOG_SENT_EMAILS = False # parallel, and what the SES rate is. BULK_EMAIL_RETRY_DELAY_BETWEEN_SENDS = 0.02 + + ################################### APPS ###################################### INSTALLED_APPS = ( # Standard ones that are always installed... diff --git a/lms/static/sass/views/_shoppingcart.scss b/lms/static/sass/views/_shoppingcart.scss index d6861fb456..1b3da66893 100644 --- a/lms/static/sass/views/_shoppingcart.scss +++ b/lms/static/sass/views/_shoppingcart.scss @@ -5,6 +5,14 @@ padding: 30px 30px 0 30px; } +.error_msg { + margin: 20px; + padding: 5px; + color: $red; + border: 1px solid $red; + +} + .cart-list { padding: 30px; margin-top: 40px; diff --git a/lms/templates/shoppingcart/download_report.html b/lms/templates/shoppingcart/download_report.html new file mode 100644 index 0000000000..838b07f145 --- /dev/null +++ b/lms/templates/shoppingcart/download_report.html @@ -0,0 +1,29 @@ +<%! from django.utils.translation import ugettext as _ %> +<%! from django.core.urlresolvers import reverse %> +<%inherit file="../main.html" /> + +<%block name="title">${_("Download Purchase Report")} + + +
+

${_("Download CSV of purchase data")}

+ % if date_fmt_error: +
+ ${_("There was an error in your date input. It should be formatted as YYYY-MM-DD")} +
+ % endif + % if total_count_error: +
+ ${_("There are too many results in your report.")} (>${settings.PAYMENT_REPORT_MAX_ITEMS}). + ${_("Try making the date range smaller.")} +
+ % endif +
+ + + + + + +
+
diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index a0199378e7..f198b3ae10 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -70,6 +70,7 @@ South==0.7.6 sympy==0.7.1 xmltodict==0.4.1 django-ratelimit-backend==0.6 +unicodecsv==0.9.4 # Used for debugging ipython==0.13.1 From 41b73d8f482ae218bb1c1795b3879541de835e40 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Mon, 18 Nov 2013 20:03:01 +0000 Subject: [PATCH 06/69] Basic test fix --- common/djangoapps/student/tests/tests.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 06d61c0425..b90ba3a165 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -59,23 +59,28 @@ class ResetPasswordTests(TestCase): self.user_bad_passwd.password = UNUSABLE_PASSWORD self.user_bad_passwd.save() + @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) def test_user_bad_password_reset(self): """Tests password reset behavior for user with password marked UNUSABLE_PASSWORD""" bad_pwd_req = self.request_factory.post('/password_reset/', {'email': self.user_bad_passwd.email}) bad_pwd_resp = password_reset(bad_pwd_req) + # If they've got an unusable password, fine, we should let them reset it self.assertEquals(bad_pwd_resp.status_code, 200) - self.assertEquals(bad_pwd_resp.content, json.dumps({'success': False, - 'error': 'Invalid e-mail or user'})) + self.assertEquals(bad_pwd_resp.content, json.dumps({'success': True, + 'value': "('registration/password_reset_done.html', [])"})) + @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) def test_nonexist_email_password_reset(self): """Now test the exception cases with of reset_password called with invalid email.""" bad_email_req = self.request_factory.post('/password_reset/', {'email': self.user.email+"makeItFail"}) bad_email_resp = password_reset(bad_email_req) + # Note: even if the email is bad, we return a successful response code + # This prevents someone potentially trying to "brute-force" find out which emails are and aren't registered with edX self.assertEquals(bad_email_resp.status_code, 200) - self.assertEquals(bad_email_resp.content, json.dumps({'success': False, - 'error': 'Invalid e-mail or user'})) + self.assertEquals(bad_email_resp.content, json.dumps({'success': True, + 'value': "('registration/password_reset_done.html', [])"})) @unittest.skipUnless(not settings.MITX_FEATURES.get('DISABLE_PASSWORD_RESET_EMAIL_TEST', False), dedent("""Skipping Test because CMS has not provided necessary templates for password reset. From 2e31ff8c35e75489cf89fa653af154052eac2fc8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 14 Nov 2013 17:25:34 -0500 Subject: [PATCH 07/69] Recover from error loading forum thread list When a user attempts to load more threads in the forum navigation sidebar, reset the state of the world so the user can retry, and alert the user appropriately. --- CHANGELOG.rst | 2 ++ common/static/coffee/src/discussion/discussion.coffee | 6 +++--- .../src/discussion/views/discussion_thread_list_view.coffee | 6 +++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 69522b0584..46fc3f9cdf 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Add error recovery when a user loads more threads in the forum sidebar. + LMS: Add a user-visible alert modal when a forums AJAX request fails. Blades: Add template for checkboxes response to studio. BLD-193. diff --git a/common/static/coffee/src/discussion/discussion.coffee b/common/static/coffee/src/discussion/discussion.coffee index 5a52cd4de0..954dd80129 100644 --- a/common/static/coffee/src/discussion/discussion.coffee +++ b/common/static/coffee/src/discussion/discussion.coffee @@ -25,9 +25,8 @@ if Backbone? @add model model - retrieveAnotherPage: (mode, options={}, sort_options={})-> - @current_page += 1 - data = { page: @current_page } + retrieveAnotherPage: (mode, options={}, sort_options={}, error=null)-> + data = { page: @current_page + 1 } switch mode when 'search' url = DiscussionUtil.urlFor 'search' @@ -59,6 +58,7 @@ if Backbone? @reset new_collection @pages = response.num_pages @current_page = response.page + error: error sortByDate: (thread) -> # diff --git a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee index 5a1e3fa9ae..ad527cf20b 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee @@ -156,7 +156,11 @@ if Backbone? $(".post-list a").first()?.focus() ) - @collection.retrieveAnotherPage(@mode, options, {sort_key: @sortBy}) + error = => + @renderThreads() + DiscussionUtil.discussionAlert("Sorry", "We had some trouble loading more threads. Please try again.") + + @collection.retrieveAnotherPage(@mode, options, {sort_key: @sortBy}, error) renderThread: (thread) => content = $(_.template($("#thread-list-item-template").html())(thread.toJSON())) From 95932610a7fe02fd416385314cfe23d4fbfacd9a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 14 Nov 2013 17:03:30 -0500 Subject: [PATCH 08/69] Add focus trap on forum navigation thread loading For accessibility purposes, it is bad to allow a user to initiate loading of additional threads in the navigation sidebar and then shift focus away from the sidebar, only to have focus snap back when the additional threads are loaded. Now, we trap focus on the loading element as recommended by our accessibility consultant. JIRA: FOR-238 --- CHANGELOG.rst | 3 +++ common/static/coffee/src/discussion/utils.coffee | 14 ++++++++------ .../views/discussion_thread_list_view.coffee | 5 ++++- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 46fc3f9cdf..91eac8f611 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,9 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Trap focus on the loading element when a user loads more threads +in the forum sidebar to improve accessibility. + LMS: Add error recovery when a user loads more threads in the forum sidebar. LMS: Add a user-visible alert modal when a forums AJAX request fails. diff --git a/common/static/coffee/src/discussion/utils.coffee b/common/static/coffee/src/discussion/utils.coffee index 73cfde8a06..89014c5f57 100644 --- a/common/static/coffee/src/discussion/utils.coffee +++ b/common/static/coffee/src/discussion/utils.coffee @@ -87,6 +87,13 @@ class @DiscussionUtil "notifications_status" : "/notification_prefs/status" }[name] + @makeFocusTrap: (elem) -> + elem.keydown( + (event) -> + if event.which == 9 # Tab + event.preventDefault() + ) + @discussionAlert: (header, body) -> if $("#discussion-alert").length == 0 alertDiv = $("" ) - # Capture focus - alertDiv.find("button").keydown( - (event) -> - if event.which == 9 # Tab - event.preventDefault() - ) + @makeFocusTrap(alertDiv.find("button")) alertTrigger = $("").css("display", "none") alertTrigger.leanModal({closeButton: "#discussion-alert .dismiss", overlay: 1, top: 200}) $("body").append(alertDiv).append(alertTrigger) diff --git a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee index ad527cf20b..57385c15bd 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee @@ -124,8 +124,11 @@ if Backbone? loadMorePages: (event) -> if event event.preventDefault() - @$(".more-pages").html('
Loading more threads
') + @$(".more-pages").html('
Loading more threads
') @$(".more-pages").addClass("loading") + loadingDiv = @$(".more-pages .loading-animation") + DiscussionUtil.makeFocusTrap(loadingDiv) + loadingDiv.focus() options = {} switch @mode when 'search' From e3b8ce708f6fd2431a5cd5e412f508a676585e9d Mon Sep 17 00:00:00 2001 From: RobertMarks Date: Mon, 18 Nov 2013 09:20:00 -0800 Subject: [PATCH 09/69] changes to allow multiple choicetextresponses in one problem --- common/lib/capa/capa/templates/choicetext.html | 2 +- common/static/js/capa/choicetextinput.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/lib/capa/capa/templates/choicetext.html b/common/lib/capa/capa/templates/choicetext.html index 5f587e214a..e74e9f71e5 100644 --- a/common/lib/capa/capa/templates/choicetext.html +++ b/common/lib/capa/capa/templates/choicetext.html @@ -55,7 +55,7 @@ % else: <% my_id = content_node.get('contents','') %> <% my_val = value.get(my_id,'') %> - + %endif ${content_node['tail_text']} diff --git a/common/static/js/capa/choicetextinput.js b/common/static/js/capa/choicetextinput.js index 4d7540f938..514e3f67f5 100644 --- a/common/static/js/capa/choicetextinput.js +++ b/common/static/js/capa/choicetextinput.js @@ -1,13 +1,13 @@ (function () { var update = function () { // Whenever a value changes create a new serialized version of this - // problem's inputs and set the hidden input fields value to equal it. - var parent = $(this).closest('.problems-wrapper'); + // problem's inputs and set the hidden input field's value to equal it. + var parent = $(this).closest('section.choicetextinput'); // find the closest parent problems-wrapper and use that as the problem // grab the input id from the input // real_input is the hidden input field var real_input = $('input.choicetextvalue', parent); - var all_inputs = $('.choicetextinput .ctinput', parent); + var all_inputs = $('input.ctinput', parent); var user_inputs = {}; $(all_inputs).each(function (index, elt) { var node = $(elt); From fc46efb6c77033d1af0ec06f2dadfb625783ec0c Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Tue, 19 Nov 2013 14:03:05 -0500 Subject: [PATCH 10/69] Fix bug in grabbing course enrollments. LMS-1475 --- lms/djangoapps/certificates/queue.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/certificates/queue.py b/lms/djangoapps/certificates/queue.py index 8dffa7ee24..8ab9759b8c 100644 --- a/lms/djangoapps/certificates/queue.py +++ b/lms/djangoapps/certificates/queue.py @@ -10,6 +10,7 @@ from capa.xqueue_interface import make_xheader, make_hashkey from django.conf import settings from requests.auth import HTTPBasicAuth from student.models import UserProfile, CourseEnrollment +from verify_student.models import SoftwareSecurePhotoVerification import json import random @@ -174,7 +175,7 @@ class XQueueCertInterface(object): grade = grades.grade(student, self.request, course) is_whitelisted = self.whitelist.filter( user=student, course_id=course_id, whitelist=True).exists() - enrollment = CourseEnrollment.objects.get(user=student) + enrollment = CourseEnrollment.objects.get(user=student, course_id=course_id) org = course_id.split('/')[0] course_num = course_id.split('/')[1] if enrollment.mode == CertificateModes.verified: From 954ca83c9000327a15b99f05466d0e0322148cf9 Mon Sep 17 00:00:00 2001 From: danielcebrian Date: Tue, 19 Nov 2013 14:37:07 -0500 Subject: [PATCH 11/69] Update AUTHORS --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 9326b6781a..60ac912d49 100644 --- a/AUTHORS +++ b/AUTHORS @@ -97,3 +97,4 @@ Iain Dunning Olivier Marquez Florian Dufour Manuel Freire +Daniel Cebrián Robles From 87238e6d938e3b972b3404cfe3c00b2c74793e9f Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Tue, 19 Nov 2013 15:54:20 +0000 Subject: [PATCH 12/69] Removed null bits --- common/djangoapps/student/tests/tests.py | 2 +- common/djangoapps/student/views.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index b90ba3a165..9aa5ad8279 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -65,7 +65,7 @@ class ResetPasswordTests(TestCase): bad_pwd_req = self.request_factory.post('/password_reset/', {'email': self.user_bad_passwd.email}) bad_pwd_resp = password_reset(bad_pwd_req) - # If they've got an unusable password, fine, we should let them reset it + # If they've got an unusable password, we return a successful response code self.assertEquals(bad_pwd_resp.status_code, 200) self.assertEquals(bad_pwd_resp.content, json.dumps({'success': True, 'value': "('registration/password_reset_done.html', [])"})) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index d4a03dca37..1702d7145e 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -1512,4 +1512,4 @@ def change_email_settings(request): log.info(u"User {0} ({1}) opted out of receiving emails from course {2}".format(user.username, user.email, course_id)) track.views.server_track(request, "change-email-settings", {"receive_emails": "no", "course": course_id}, page='dashboard') - return HttpResponse(json.dumps({'success': True})) \ No newline at end of file + return HttpResponse(json.dumps({'success': True})) \ No newline at end of file From bcb5f1b3689bbf9b76e23997caa4f713cda8aea1 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 19 Nov 2013 16:34:01 -0500 Subject: [PATCH 13/69] Increased timeout for element count in HTML test --- .../features/component_settings_editor_helpers.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py index 5473438571..d3293c474e 100644 --- a/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py +++ b/cms/djangoapps/contentstore/features/component_settings_editor_helpers.py @@ -6,14 +6,6 @@ from nose.tools import assert_equal, assert_in # pylint: disable=E0611 from terrain.steps import reload_the_page -def _is_expected_element_count(css, expected_number): - """ - Returns whether the number of elements found on the page by css locator - the same number that you expected. - """ - return len(world.css_find(css)) == expected_number - - @world.absorb def create_component_instance(step, category, component_type=None, is_advanced=False): """ @@ -47,8 +39,11 @@ def create_component_instance(step, category, component_type=None, is_advanced=F world.wait_for_invisible(component_button_css) click_component_from_menu(category, component_type, is_advanced) - world.wait_for(lambda _: _is_expected_element_count(module_css, - module_count_before + 1)) + expected_count = module_count_before + 1 + world.wait_for( + lambda _: len(world.css_find(module_css)) == expected_count, + timeout=20 + ) @world.absorb From 6e0140b65a91f30096c60f3870a455ab41a85260 Mon Sep 17 00:00:00 2001 From: Brian Talbot Date: Tue, 19 Nov 2013 16:28:46 -0500 Subject: [PATCH 14/69] revises .gitignore file to include static css directories and remnants of devstack setup --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index b3f7473dc0..d864ecf7d0 100644 --- a/.gitignore +++ b/.gitignore @@ -48,14 +48,18 @@ reports/ .prereqs_cache .vagrant/ node_modules +.bundle/ +bin/ ### Static assets pipeline artifacts *.scssc +lms/static/css/ lms/static/sass/*.css lms/static/sass/application.scss lms/static/sass/application-extend1.scss lms/static/sass/application-extend2.scss lms/static/sass/course.scss +cms/static/css/ cms/static/sass/*.css ### Logging artifacts From e7c06e3ab17b77e1be57ede079835130db9c21bf Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 15 Nov 2013 11:20:12 -0500 Subject: [PATCH 15/69] Change preview view method to use RESTful URL. STUD-848 --- .../contentstore/tests/test_contentstore.py | 39 +++++++++------- .../contentstore/views/component.py | 9 ++-- cms/djangoapps/contentstore/views/item.py | 28 +++++++++++- cms/djangoapps/contentstore/views/preview.py | 45 ++++--------------- cms/djangoapps/contentstore/views/tabs.py | 9 ++-- .../coffee/spec/views/module_edit_spec.coffee | 11 ++--- .../coffee/src/views/module_edit.coffee | 6 +-- cms/static/coffee/src/views/tabs.coffee | 3 +- cms/static/coffee/src/views/unit.coffee | 1 - cms/templates/edit-tabs.html | 4 +- cms/templates/unit.html | 4 +- cms/urls.py | 1 - 12 files changed, 74 insertions(+), 86 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 58cf3be70b..2b773e991d 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -454,31 +454,36 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): @override_settings(COURSES_WITH_UNSAFE_CODE=['edX/toy/.*']) def test_module_preview_in_whitelist(self): - ''' + """ Tests the ajax callback to render an XModule - ''' - direct_store = modulestore('direct') - import_from_xml(direct_store, 'common/test/data/', ['toy']) - - # also try a custom response which will trigger the 'is this course in whitelist' logic - problem_module_location = Location(['i4x', 'edX', 'toy', 'vertical', 'vertical_test', None]) - url = reverse('preview_component', kwargs={'location': problem_module_location.url()}) - resp = self.client.get_html(url) - self.assertEqual(resp.status_code, 200) + """ + resp = self._test_preview(Location(['i4x', 'edX', 'toy', 'vertical', 'vertical_test', None])) + # These are the data-ids of the xblocks contained in the vertical. + # Ultimately, these must be converted to new locators. + self.assertContains(resp, 'i4x://edX/toy/video/sample_video') + self.assertContains(resp, 'i4x://edX/toy/video/separate_file_video') + self.assertContains(resp, 'i4x://edX/toy/video/video_with_end_time') + self.assertContains(resp, 'i4x://edX/toy/poll_question/T1_changemind_poll_foo_2') def test_video_module_caption_asset_path(self): - ''' + """ This verifies that a video caption url is as we expect it to be - ''' + """ + resp = self._test_preview(Location(['i4x', 'edX', 'toy', 'video', 'sample_video', None])) + self.assertContains(resp, 'data-caption-asset-path="/c4x/edX/toy/asset/subs_"') + + def _test_preview(self, location): + """ Preview test case. """ direct_store = modulestore('direct') - import_from_xml(direct_store, 'common/test/data/', ['toy']) + _, course_items = import_from_xml(direct_store, 'common/test/data/', ['toy']) # also try a custom response which will trigger the 'is this course in whitelist' logic - video_module_location = Location(['i4x', 'edX', 'toy', 'video', 'sample_video', None]) - url = reverse('preview_component', kwargs={'location': video_module_location.url()}) - resp = self.client.get_html(url) + locator = loc_mapper().translate_location( + course_items[0].location.course_id, location, False, True + ) + resp = self.client.get_html(locator.url_reverse('xblock')) self.assertEqual(resp.status_code, 200) - self.assertContains(resp, 'data-caption-asset-path="/c4x/edX/toy/asset/subs_"') + return resp def test_delete(self): direct_store = modulestore('direct') diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 327e75c7f4..70d7bef7ce 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -249,12 +249,9 @@ def edit_unit(request, location): ) components = [ - [ - component.location.url(), - loc_mapper().translate_location( - course.location.course_id, component.location, False, True - ) - ] + loc_mapper().translate_location( + course.location.course_id, component.location, False, True + ) for component in item.get_children() ] diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 97bfde3b82..d33d00377b 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -3,7 +3,9 @@ import logging from uuid import uuid4 +from functools import partial from static_replace import replace_static_urls +from xmodule_modifiers import wrap_xblock from django.core.exceptions import PermissionDenied from django.contrib.auth.decorators import login_required @@ -27,6 +29,8 @@ from xmodule.modulestore.locator import BlockUsageLocator from student.models import CourseEnrollment from django.http import HttpResponseBadRequest from xblock.fields import Scope +from preview import handler_prefix, get_preview_html +from mitxmako.shortcuts import render_to_response, render_to_string __all__ = ['orphan_handler', 'xblock_handler'] @@ -51,6 +55,7 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= all children and "all_versions" to delete from all (mongo) versions. GET json: returns representation of the xblock (locator id, data, and metadata). + html: returns HTML for rendering the xblock (which includes both the "preview" view and the "editor" view) PUT or POST json: if xblock location is specified, update the xblock instance. The json payload can contain these fields, all optional: @@ -76,8 +81,27 @@ def xblock_handler(request, tag=None, course_id=None, branch=None, version_guid= old_location = loc_mapper().translate_locator_to_location(location) if request.method == 'GET': - rsp = _get_module_info(location) - return JsonResponse(rsp) + if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + rsp = _get_module_info(location) + return JsonResponse(rsp) + else: + component = modulestore().get_item(old_location) + # Wrap the generated fragment in the xmodule_editor div so that the javascript + # can bind to it correctly + component.runtime.wrappers.append(partial(wrap_xblock, handler_prefix)) + + try: + content = component.render('studio_view').content + # catch exceptions indiscriminately, since after this point they escape the + # dungeon and surface as uneditable, unsaveable, and undeletable + # component-goblins. + except Exception as exc: # pylint: disable=W0703 + content = render_to_string('html_error.html', {'message': str(exc)}) + + return render_to_response('component.html', { + 'preview': get_preview_html(request, component), + 'editor': content + }) elif request.method == 'DELETE': delete_children = str_to_bool(request.REQUEST.get('recurse', 'False')) delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', 'False')) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 6e86a6485b..3b2ec85326 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -3,7 +3,7 @@ from functools import partial from django.conf import settings from django.core.urlresolvers import reverse -from django.http import Http404, HttpResponseBadRequest, HttpResponseForbidden +from django.http import Http404, HttpResponseBadRequest from django.contrib.auth.decorators import login_required from mitxmako.shortcuts import render_to_response, render_to_string @@ -24,10 +24,9 @@ from util.sandboxing import can_execute_unsafe_code import static_replace from .session_kv_store import SessionKeyValueStore from .helpers import render_from_lms -from .access import has_access from ..utils import get_course_for_item -__all__ = ['preview_handler', 'preview_component'] +__all__ = ['preview_handler'] log = logging.getLogger(__name__) @@ -53,13 +52,13 @@ def preview_handler(request, usage_id, handler, suffix=''): usage_id: The usage-id of the block to dispatch to, passed through `quote_slashes` handler: The handler to execute - suffix: The remaineder of the url to be passed to the handler + suffix: The remainder of the url to be passed to the handler """ location = unquote_slashes(usage_id) descriptor = modulestore().get_item(location) - instance = load_preview_module(request, descriptor) + instance = _load_preview_module(request, descriptor) # Let the module handle the AJAX req = django_to_webob_request(request) try: @@ -85,32 +84,6 @@ def preview_handler(request, usage_id, handler, suffix=''): return webob_to_django_response(resp) -@login_required -def preview_component(request, location): - "Return the HTML preview of a component" - # TODO (vshnayder): change name from id to location in coffee+html as well. - if not has_access(request.user, location): - return HttpResponseForbidden() - - component = modulestore().get_item(location) - # Wrap the generated fragment in the xmodule_editor div so that the javascript - # can bind to it correctly - component.runtime.wrappers.append(partial(wrap_xblock, handler_prefix)) - - try: - content = component.render('studio_view').content - # catch exceptions indiscriminately, since after this point they escape the - # dungeon and surface as uneditable, unsaveable, and undeletable - # component-goblins. - except Exception as exc: # pylint: disable=W0703 - content = render_to_string('html_error.html', {'message': str(exc)}) - - return render_to_response('component.html', { - 'preview': get_preview_html(request, component), - 'editor': content - }) - - class PreviewModuleSystem(ModuleSystem): # pylint: disable=abstract-method """ An XModule ModuleSystem for use in Studio previews @@ -119,7 +92,7 @@ class PreviewModuleSystem(ModuleSystem): # pylint: disable=abstract-method return handler_prefix(block, handler_name, suffix) + '?' + query -def preview_module_system(request, descriptor): +def _preview_module_system(request, descriptor): """ Returns a ModuleSystem for the specified descriptor that is specialized for rendering module previews. @@ -135,7 +108,7 @@ def preview_module_system(request, descriptor): # TODO (cpennington): Do we want to track how instructors are using the preview problems? track_function=lambda event_type, event: None, filestore=descriptor.runtime.resources_fs, - get_module=partial(load_preview_module, request), + get_module=partial(_load_preview_module, request), render_template=render_from_lms, debug=True, replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_id=course_id), @@ -162,7 +135,7 @@ def preview_module_system(request, descriptor): ) -def load_preview_module(request, descriptor): +def _load_preview_module(request, descriptor): """ Return a preview XModule instantiated from the supplied descriptor. @@ -171,7 +144,7 @@ def load_preview_module(request, descriptor): """ student_data = DbModel(SessionKeyValueStore(request)) descriptor.bind_for_student( - preview_module_system(request, descriptor), + _preview_module_system(request, descriptor), LmsFieldData(descriptor._field_data, student_data), # pylint: disable=protected-access ) return descriptor @@ -182,7 +155,7 @@ def get_preview_html(request, descriptor): Returns the HTML returned by the XModule's student_view, specified by the descriptor and idx. """ - module = load_preview_module(request, descriptor) + module = _load_preview_module(request, descriptor) try: content = module.render("student_view").content except Exception as exc: # pylint: disable=W0703 diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 277445e3b9..40ea9bfd6b 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -125,12 +125,9 @@ def edit_tabs(request, org, course, coursename): static_tabs.append(modulestore('direct').get_item(static_tab_loc)) components = [ - [ - static_tab.location.url(), - loc_mapper().translate_location( - course_item.location.course_id, static_tab.location, False, True - ) - ] + loc_mapper().translate_location( + course_item.location.course_id, static_tab.location, False, True + ) for static_tab in static_tabs ] diff --git a/cms/static/coffee/spec/views/module_edit_spec.coffee b/cms/static/coffee/spec/views/module_edit_spec.coffee index 22d1052fa3..36716668d3 100644 --- a/cms/static/coffee/spec/views/module_edit_spec.coffee +++ b/cms/static/coffee/spec/views/module_edit_spec.coffee @@ -1,12 +1,9 @@ -define ["coffee/src/views/module_edit", "xmodule"], (ModuleEdit) -> +define ["coffee/src/views/module_edit", "js/models/module_info", "xmodule"], (ModuleEdit, ModuleModel) -> describe "ModuleEdit", -> beforeEach -> - @stubModule = jasmine.createSpy("Module") - @stubModule.id = 'stub-id' - @stubModule.get = (param)-> - if param == 'old_id' - return 'stub-old-id' + @stubModule = new ModuleModel + id: "stub-id" setFixtures """
  • @@ -62,7 +59,7 @@ define ["coffee/src/views/module_edit", "xmodule"], (ModuleEdit) -> @moduleEdit.render() it "loads the module preview and editor via ajax on the view element", -> - expect(@moduleEdit.$el.load).toHaveBeenCalledWith("/preview_component/#{@moduleEdit.model.get('old_id')}", jasmine.any(Function)) + expect(@moduleEdit.$el.load).toHaveBeenCalledWith("/xblock/#{@moduleEdit.model.id}", jasmine.any(Function)) @moduleEdit.$el.load.mostRecentCall.args[1]() expect(@moduleEdit.loadDisplay).toHaveBeenCalled() expect(@moduleEdit.delegateEvents).toHaveBeenCalled() diff --git a/cms/static/coffee/src/views/module_edit.coffee b/cms/static/coffee/src/views/module_edit.coffee index a13e572887..729a17615e 100644 --- a/cms/static/coffee/src/views/module_edit.coffee +++ b/cms/static/coffee/src/views/module_edit.coffee @@ -69,15 +69,13 @@ define ["backbone", "jquery", "underscore", "gettext", "xblock/runtime.v1", payload (data) => @model.set(id: data.locator) - @model.set(old_id: data.id) - @$el.data('id', data.id) @$el.data('locator', data.locator) @render() ) render: -> - if @model.get('old_id') - @$el.load("/preview_component/#{@model.get('old_id')}", => + if @model.id + @$el.load(@model.url(), => @loadDisplay() @delegateEvents() ) diff --git a/cms/static/coffee/src/views/tabs.coffee b/cms/static/coffee/src/views/tabs.coffee index 0f72e8bddb..a85b3b7863 100644 --- a/cms/static/coffee/src/views/tabs.coffee +++ b/cms/static/coffee/src/views/tabs.coffee @@ -6,8 +6,7 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views initialize: => @$('.component').each((idx, element) => model = new ModuleModel({ - id: $(element).data('locator'), - old_id:$(element).data('id') + id: $(element).data('locator') }) new ModuleEditView( diff --git a/cms/static/coffee/src/views/unit.coffee b/cms/static/coffee/src/views/unit.coffee index 075b56d1b0..e6ba0e1382 100644 --- a/cms/static/coffee/src/views/unit.coffee +++ b/cms/static/coffee/src/views/unit.coffee @@ -63,7 +63,6 @@ define ["jquery", "jquery.ui", "gettext", "backbone", @$('.component').each (idx, element) => model = new ModuleModel id: $(element).data('locator') - old_id: $(element).data('id') new ModuleEditView el: element, onDelete: @deleteComponent, diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index f6b8e7b77a..54a30217ea 100644 --- a/cms/templates/edit-tabs.html +++ b/cms/templates/edit-tabs.html @@ -61,8 +61,8 @@ require(["backbone", "coffee/src/views/tabs"], function(Backbone, TabsEditView)
      - % for id, locator in components: -
    1. + % for locator in components: +
    2. % endfor
    3. diff --git a/cms/templates/unit.html b/cms/templates/unit.html index cc7827c7d3..e83dd45da0 100644 --- a/cms/templates/unit.html +++ b/cms/templates/unit.html @@ -48,8 +48,8 @@ require(["domReady!", "jquery", "js/models/module_info", "coffee/src/views/unit"

        - % for id, locator in components: -
      1. + % for locator in components: +
      2. % endfor
      3. diff --git a/cms/urls.py b/cms/urls.py index 99e9cbfaba..343e9f4d04 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -14,7 +14,6 @@ urlpatterns = patterns('', # nopep8 url(r'^$', 'contentstore.views.howitworks', name='homepage'), url(r'^edit/(?P.*?)$', 'contentstore.views.edit_unit', name='edit_unit'), url(r'^subsection/(?P.*?)$', 'contentstore.views.edit_subsection', name='edit_subsection'), - url(r'^preview_component/(?P.*?)$', 'contentstore.views.preview_component', name='preview_component'), url(r'^transcripts/upload$', 'contentstore.views.upload_transcripts', name='upload_transcripts'), url(r'^transcripts/download$', 'contentstore.views.download_transcripts', name='download_transcripts'), From 16568766994bc77b0f543dfdf4cbed5e39a51ecc Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Tue, 19 Nov 2013 17:12:49 -0500 Subject: [PATCH 16/69] If student has not passed verification, issue an honor code cert. Also, display a message on their dashboard. --- lms/djangoapps/certificates/models.py | 9 +++++---- lms/djangoapps/certificates/queue.py | 11 ++++++++--- .../dashboard/_dashboard_certificate_information.html | 8 +++++++- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 36ff18618e..4e948d4b06 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -93,9 +93,9 @@ class GeneratedCertificate(models.Model): mode = models.CharField(max_length=32, default=CertificateModes.honor) name = models.CharField(blank=True, max_length=255) created_date = models.DateTimeField( - auto_now_add=True, default=datetime.now) + auto_now_add=True, default=datetime.now) modified_date = models.DateTimeField( - auto_now=True, default=datetime.now) + auto_now=True, default=datetime.now) error_reason = models.CharField(max_length=512, blank=True, default='') class Meta: @@ -133,8 +133,9 @@ def certificate_status_for_student(student, course_id): try: generated_certificate = GeneratedCertificate.objects.get( - user=student, course_id=course_id) - d = {'status': generated_certificate.status} + user=student, course_id=course_id) + d = {'status': generated_certificate.status, + 'mode': generated_certificate.mode} if generated_certificate.grade: d['grade'] = generated_certificate.grade if generated_certificate.status == CertificateStatuses.downloadable: diff --git a/lms/djangoapps/certificates/queue.py b/lms/djangoapps/certificates/queue.py index 8ab9759b8c..03a0947aa9 100644 --- a/lms/djangoapps/certificates/queue.py +++ b/lms/djangoapps/certificates/queue.py @@ -178,9 +178,15 @@ class XQueueCertInterface(object): enrollment = CourseEnrollment.objects.get(user=student, course_id=course_id) org = course_id.split('/')[0] course_num = course_id.split('/')[1] - if enrollment.mode == CertificateModes.verified: + cert_mode = enrollment.mode + if enrollment.mode == CertificateModes.verified and SoftwareSecurePhotoVerification.user_is_verified(student): template_pdf = "certificate-template-{0}-{1}-verified.pdf".format( org, course_num) + elif (enrollment.mode == CertificateModes.verified and not + SoftwareSecurePhotoVerification.user_is_verified(student)): + template_pdf = "certificate-template-{0}-{1}.pdf".format( + org, course_num) + cert_mode = CertificateModes.honor else: # honor code and audit students template_pdf = "certificate-template-{0}-{1}.pdf".format( @@ -189,8 +195,7 @@ class XQueueCertInterface(object): cert, created = GeneratedCertificate.objects.get_or_create( user=student, course_id=course_id) - cert.mode = enrollment.mode - + cert.mode = cert_mode cert.user = student cert.grade = grade['percent'] cert.course_id = course_id diff --git a/lms/templates/dashboard/_dashboard_certificate_information.html b/lms/templates/dashboard/_dashboard_certificate_information.html index ea5171c0ef..3222b6aae8 100644 --- a/lms/templates/dashboard/_dashboard_certificate_information.html +++ b/lms/templates/dashboard/_dashboard_certificate_information.html @@ -19,7 +19,7 @@ else: % elif cert_status['status'] in ('generating', 'ready', 'notpassing', 'restricted'):

        ${_("Your final grade:")} ${"{0:.0f}%".format(float(cert_status['grade'])*100)}. - % if cert_status['status'] == 'notpassing': + % if cert_status['status'] == 'notpassing' and enrollment.mode != 'audit': ${_("Grade required for a certificate:")} ${"{0:.0f}%".format(float(course.lowest_passing_grade)*100)}. % elif cert_status['status'] == 'restricted' and enrollment.mode == 'verified': @@ -44,6 +44,12 @@ else: ${_("Download Your Certificate (PDF)")}

        4. + % elif cert_status['show_download_url'] and enrollment.mode == 'verified' and cert_status['mode'] == 'honor': +
      4. +

        ${_('Since we did not have a valid set of verification photos from you when certificates were generated, we could not grant you a verified certificate. An honor code certificate has been granted instead.')}

        +         + ${_("Download Your Certificate (PDF)")}
        5. % elif cert_status['show_download_url'] and enrollment.mode == 'verified':
      5. Date: Tue, 19 Nov 2013 17:30:27 -0500 Subject: [PATCH 17/69] Use class methods to find the enrollment mode. --- lms/djangoapps/certificates/queue.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/certificates/queue.py b/lms/djangoapps/certificates/queue.py index 03a0947aa9..0c07ed6e6f 100644 --- a/lms/djangoapps/certificates/queue.py +++ b/lms/djangoapps/certificates/queue.py @@ -175,14 +175,14 @@ class XQueueCertInterface(object): grade = grades.grade(student, self.request, course) is_whitelisted = self.whitelist.filter( user=student, course_id=course_id, whitelist=True).exists() - enrollment = CourseEnrollment.objects.get(user=student, course_id=course_id) + enrollment_mode = CourseEnrollment.enrollment_mode_for_user(student, course_id) org = course_id.split('/')[0] course_num = course_id.split('/')[1] - cert_mode = enrollment.mode - if enrollment.mode == CertificateModes.verified and SoftwareSecurePhotoVerification.user_is_verified(student): + cert_mode = enrollment_mode + if enrollment_mode == CertificateModes.verified and SoftwareSecurePhotoVerification.user_is_verified(student): template_pdf = "certificate-template-{0}-{1}-verified.pdf".format( org, course_num) - elif (enrollment.mode == CertificateModes.verified and not + elif (enrollment_mode == CertificateModes.verified and not SoftwareSecurePhotoVerification.user_is_verified(student)): template_pdf = "certificate-template-{0}-{1}.pdf".format( org, course_num) From cb113deade37141e65c2bceac2ddf3fff5ca10e0 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 11 Oct 2013 17:37:04 -0400 Subject: [PATCH 18/69] Separate all db ops from modulestore ops --- .../xmodule/modulestore/split_migrator.py | 2 +- .../split_mongo/definition_lazy_loader.py | 3 +- .../split_mongo/mongo_connection.py | 116 +++++++++++++++++ .../xmodule/modulestore/split_mongo/split.py | 122 ++++++------------ .../modulestore/tests/test_split_migrator.py | 6 +- .../tests/test_split_modulestore.py | 34 ++--- 6 files changed, 170 insertions(+), 113 deletions(-) create mode 100644 common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 27a758c083..355f5bfa62 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -88,7 +88,7 @@ class SplitMigrator(object): index_info = self.split_modulestore.get_course_index_info(course_version_locator) versions = index_info['versions'] versions['draft'] = versions['published'] - self.split_modulestore.update_course_index(course_version_locator, {'versions': versions}, update_versions=True) + self.split_modulestore.update_course_index(index_info) # clean up orphans in published version: in old mongo, parents pointed to the union of their published and draft # children which meant some pointers were to non-existent locations in 'direct' diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py index 9b2a652a95..ded67104b4 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py @@ -22,5 +22,4 @@ class DefinitionLazyLoader(object): Fetch the definition. Note, the caller should replace this lazy loader pointer with the result so as not to fetch more than once """ - return self.modulestore.definitions.find_one( - {'_id': self.definition_locator.definition_id}) + return self.modulestore.db_connection.get_definition(self.definition_locator.definition_id) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py new file mode 100644 index 0000000000..510c100048 --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -0,0 +1,116 @@ +""" +Segregation of pymongo functions from the data modeling mechanisms for split modulestore. +""" +import pymongo + +class MongoConnection(object): + """ + Segregation of pymongo functions from the data modeling mechanisms for split modulestore. + """ + def __init__( + self, db, collection, host, port=27017, tz_aware=True, user=None, password=None, **kwargs + ): + """ + Create & open the connection, authenticate, and provide pointers to the collections + """ + self.database = pymongo.database.Database( + pymongo.MongoClient( + host=host, + port=port, + tz_aware=tz_aware, + **kwargs + ), + db + ) + + if user is not None and password is not None: + self.database.authenticate(user, password) + + self.course_index = self.database[collection + '.active_versions'] + self.structures = self.database[collection + '.structures'] + self.definitions = self.database[collection + '.definitions'] + + # every app has write access to the db (v having a flag to indicate r/o v write) + # Force mongo to report errors, at the expense of performance + # pymongo docs suck but explanation: + # http://api.mongodb.org/java/2.10.1/com/mongodb/WriteConcern.html + self.course_index.write_concern = {'w': 1} + self.structures.write_concern = {'w': 1} + self.definitions.write_concern = {'w': 1} + + def get_structure(self, key): + """ + Get the structure from the persistence mechanism whose id is the given key + """ + return self.structures.find_one({'_id': key}) + + def find_matching_structures(self, query): + """ + Find the structure matching the query. Right now the query must be a legal mongo query + :param query: a mongo-style query of {key: [value|{$in ..}|..], ..} + """ + return self.structures.find(query) + + def insert_structure(self, structure): + """ + Create the structure in the db + """ + self.structures.insert(structure) + + def update_structure(self, structure): + """ + Update the db record for structure + """ + self.structures.update({'_id': structure['_id']}, structure) + + def get_course_index(self, key): + """ + Get the course_index from the persistence mechanism whose id is the given key + """ + return self.course_index.find_one({'_id': key}) + + def find_matching_course_indexes(self, query): + """ + Find the course_index matching the query. Right now the query must be a legal mongo query + :param query: a mongo-style query of {key: [value|{$in ..}|..], ..} + """ + return self.course_index.find(query) + + def insert_course_index(self, course_index): + """ + Create the course_index in the db + """ + self.course_index.insert(course_index) + + def update_course_index(self, course_index): + """ + Update the db record for course_index + """ + self.course_index.update({'_id': course_index['_id']}, course_index) + + def delete_course_index(self, key): + """ + Delete the course_index from the persistence mechanism whose id is the given key + """ + return self.course_index.remove({'_id': key}) + + def get_definition(self, key): + """ + Get the definition from the persistence mechanism whose id is the given key + """ + return self.definitions.find_one({'_id': key}) + + def find_matching_definitions(self, query): + """ + Find the definitions matching the query. Right now the query must be a legal mongo query + :param query: a mongo-style query of {key: [value|{$in ..}|..], ..} + """ + return self.definitions.find(query) + + def insert_definition(self, definition): + """ + Create the definition in the db + """ + self.definitions.insert(definition) + + diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 6152416596..a6349e6113 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1,7 +1,6 @@ import threading import datetime import logging -import pymongo import re from importlib import import_module from path import path @@ -21,6 +20,7 @@ from .caching_descriptor_system import CachingDescriptorSystem from xblock.fields import Scope from xblock.runtime import Mixologist from bson.objectid import ObjectId +from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection log = logging.getLogger(__name__) #============================================================================== @@ -49,7 +49,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): A Mongodb backed ModuleStore supporting versions, inheritance, and sharing. """ - # pylint: disable=W0201 def __init__(self, doc_store_config, fs_root, render_template, default_class=None, error_tracker=null_error_tracker, @@ -62,44 +61,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): super(SplitMongoModuleStore, self).__init__(**kwargs) self.loc_mapper = loc_mapper - def do_connection( - db, collection, host, port=27017, tz_aware=True, user=None, password=None, **kwargs - ): - """ - Create & open the connection, authenticate, and provide pointers to the collections - """ - self.db = pymongo.database.Database( - pymongo.MongoClient( - host=host, - port=port, - tz_aware=tz_aware, - **kwargs - ), - db - ) - - if user is not None and password is not None: - self.db.authenticate(user, password) - - self.course_index = self.db[collection + '.active_versions'] - self.structures = self.db[collection + '.structures'] - self.definitions = self.db[collection + '.definitions'] - - do_connection(**doc_store_config) + self.db_connection = MongoConnection(**doc_store_config) + self.db = self.db_connection.database # Code review question: How should I expire entries? # _add_cache could use a lru mechanism to control the cache size? self.thread_cache = threading.local() - - # every app has write access to the db (v having a flag to indicate r/o v write) - # Force mongo to report errors, at the expense of performance - # pymongo docs suck but explanation: - # http://api.mongodb.org/java/2.10.1/com/mongodb/WriteConcern.html - self.course_index.write_concern = {'w': 1} - self.structures.write_concern = {'w': 1} - self.definitions.write_concern = {'w': 1} - if default_class is not None: module_path, _, class_name = default_class.rpartition('.') class_ = getattr(import_module(module_path), class_name) @@ -138,7 +106,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): block['definition'] = DefinitionLazyLoader(self, block['definition']) else: # Load all descendants by id - descendent_definitions = self.definitions.find({ + descendent_definitions = self.db_connection.find_matching_definitions({ '_id': {'$in': [block['definition'] for block in new_module_data.itervalues()]}}) # turn into a map @@ -226,7 +194,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if course_locator.course_id is not None and course_locator.branch is not None: # use the course_id - index = self.course_index.find_one({'_id': course_locator.course_id}) + index = self.db_connection.get_course_index(course_locator.course_id) if index is None: raise ItemNotFoundError(course_locator) if course_locator.branch not in index['versions']: @@ -241,7 +209,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # cast string to ObjectId if necessary version_guid = course_locator.as_object_id(version_guid) - entry = self.structures.find_one({'_id': version_guid}) + entry = self.db_connection.get_structure(version_guid) # b/c more than one course can use same structure, the 'course_id' and 'branch' are not intrinsic to structure # and the one assoc'd w/ it by another fetch may not be the one relevant to this fetch; so, @@ -269,7 +237,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if qualifiers is None: qualifiers = {} qualifiers.update({"versions.{}".format(branch): {"$exists": True}}) - matching = self.course_index.find(qualifiers) + matching = self.db_connection.find_matching_course_indexes(qualifiers) # collect ids and then query for those version_guids = [] @@ -279,7 +247,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): version_guids.append(version_guid) id_version_map[version_guid] = structure['_id'] - course_entries = self.structures.find({'_id': {'$in': version_guids}}) + course_entries = self.db_connection.find_matching_structures({'_id': {'$in': version_guids}}) # get the block for the course element (s/b the root) result = [] @@ -455,7 +423,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ if course_locator.course_id is None: return None - index = self.course_index.find_one({'_id': course_locator.course_id}) + index = self.db_connection.get_course_index(course_locator.course_id) return index # TODO figure out a way to make this info accessible from the course descriptor @@ -487,7 +455,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'edited_on': when the change was made } """ - definition = self.definitions.find_one({'_id': definition_locator.definition_id}) + definition = self.db_connection.get_definition(definition_locator.definition_id) if definition is None: return None return definition['edit_info'] @@ -509,14 +477,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # TODO if depth is significant, it may make sense to get all that have the same original_version # and reconstruct the subtree from version_guid - next_entries = self.structures.find({'previous_version' : version_guid}) + next_entries = self.db_connection.find_matching_structures({'previous_version' : version_guid}) # must only scan cursor's once next_versions = [struct for struct in next_entries] result = {version_guid: [CourseLocator(version_guid=struct['_id']) for struct in next_versions]} depth = 1 while depth < version_history_depth and len(next_versions) > 0: depth += 1 - next_entries = self.structures.find({'previous_version': + next_entries = self.db_connection.find_matching_structures({'previous_version': {'$in': [struct['_id'] for struct in next_versions]}}) next_versions = [struct for struct in next_entries] for course_structure in next_versions: @@ -537,7 +505,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): course_struct = self._lookup_course(block_locator.version_agnostic())['structure'] usage_id = block_locator.usage_id update_version_field = 'blocks.{}.edit_info.update_version'.format(usage_id) - all_versions_with_block = self.structures.find({'original_version': course_struct['original_version'], + all_versions_with_block = self.db_connection.find_matching_structures({'original_version': course_struct['original_version'], update_version_field: {'$exists': True}}) # find (all) root versions and build map previous: [successors] possible_roots = [] @@ -596,7 +564,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): "original_version": new_id, } } - self.definitions.insert(document) + self.db_connection.insert_definition(document) definition_locator = DefinitionLocator(new_id) return definition_locator @@ -618,7 +586,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # if this looks in cache rather than fresh fetches, then it will probably not detect # actual change b/c the descriptor and cache probably point to the same objects - old_definition = self.definitions.find_one({'_id': definition_locator.definition_id}) + old_definition = self.db_connection.get_definition(definition_locator.definition_id) if old_definition is None: raise ItemNotFoundError(definition_locator.url()) @@ -630,7 +598,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) # previous version id old_definition['edit_info']['previous_version'] = definition_locator.definition_id - self.definitions.insert(old_definition) + self.db_connection.insert_definition(old_definition) return DefinitionLocator(old_definition['_id']), True else: return definition_locator, False @@ -657,7 +625,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param course_blocks: the current list of blocks. :param category: """ - existing_uses = self.course_index.find({"_id": {"$regex": id_root}}) + existing_uses = self.db_connection.find_matching_course_indexes({"_id": {"$regex": id_root}}) if existing_uses.count() > 0: max_found = 0 matcher = re.compile(id_root + r'(\d+)') @@ -779,11 +747,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): parent['edit_info']['update_version'] = new_id if continue_version: # db update - self.structures.update({'_id': new_id}, new_structure) + self.db_connection.update_structure(new_structure) # clear cache so things get refetched and inheritance recomputed self._clear_cache(new_id) else: - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) # update the index entry if appropriate if index_entry is not None: @@ -856,7 +824,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'original_version': definition_id, } } - self.definitions.insert(definition_entry) + self.db_connection.insert_definition(definition_entry) new_id = ObjectId() draft_structure = { @@ -880,7 +848,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): } } } - self.structures.insert(draft_structure) + self.db_connection.insert_structure(draft_structure) if versions_dict is None: versions_dict = {master_branch: new_id} @@ -898,20 +866,20 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if block_fields is not None: root_block['fields'].update(block_fields) if definition_fields is not None: - definition = self.definitions.find_one({'_id': root_block['definition']}) + definition = self.db_connection.get_definition(root_block['definition']) definition['fields'].update(definition_fields) definition['edit_info']['previous_version'] = definition['_id'] definition['edit_info']['edited_by'] = user_id definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) definition['_id'] = ObjectId() - self.definitions.insert(definition) + self.db_connection.insert_definition(definition) root_block['definition'] = definition['_id'] root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) root_block['edit_info']['edited_by'] = user_id root_block['edit_info']['previous_version'] = root_block['edit_info'].get('update_version') root_block['edit_info']['update_version'] = new_id - self.structures.insert(draft_structure) + self.db_connection.insert_structure(draft_structure) versions_dict[master_branch] = new_id # create the index entry @@ -926,7 +894,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'edited_by': user_id, 'edited_on': datetime.datetime.now(UTC), 'versions': versions_dict} - self.course_index.insert(index_entry) + self.db_connection.insert_course_index(index_entry) return self.get_course(CourseLocator(course_id=new_id, branch=master_branch)) def update_item(self, descriptor, user_id, force=False): @@ -978,7 +946,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'previous_version': block_data['edit_info']['update_version'], 'update_version': new_id, } - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) # update the index entry if appropriate if index_entry is not None: self._update_head(index_entry, descriptor.location.branch, new_id) @@ -1016,7 +984,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): is_updated = self._persist_subdag(xblock, user_id, new_structure['blocks'], new_id) if is_updated: - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) # update the index entry if appropriate if index_entry is not None: @@ -1115,31 +1083,18 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): '''Deprecated, use update_item.''' raise NotImplementedError('use update_item') - def update_course_index(self, course_locator, new_values_dict, update_versions=False): + def update_course_index(self, updated_index_entry): """ - Change the given course's index entry for the given fields. new_values_dict - should be a subset of the dict returned by get_course_index_info. - It cannot include '_id' (will raise IllegalArgument). - Provide update_versions=True if you intend this to replace the versions hash. + Change the given course's index entry. + Note, this operation can be dangerous and break running courses. - If the dict includes versions and not update_versions, it will raise an exception. - - If the dict includes edited_on or edited_by, it will raise an exception - Does not return anything useful. """ # TODO how should this log the change? edited_on and edited_by for this entry # has the semantic of who created the course and when; so, changing those will lose # that information. - if '_id' in new_values_dict: - raise ValueError("Cannot override _id") - if 'edited_on' in new_values_dict or 'edited_by' in new_values_dict: - raise ValueError("Cannot set edited_on or edited_by") - if not update_versions and 'versions' in new_values_dict: - raise ValueError("Cannot override versions without setting update_versions") - self.course_index.update({'_id': course_locator.course_id}, - {'$set': new_values_dict}) + self.db_connection.update_course_index(updated_index_entry) def delete_item(self, usage_locator, user_id, delete_children=False, force=False): """ @@ -1182,7 +1137,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): remove_subtree(usage_locator.usage_id) # update index if appropriate and structures - self.structures.insert(new_structure) + self.db_connection.insert_structure(new_structure) result = CourseLocator(version_guid=new_id) @@ -1204,11 +1159,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param course_id: uses course_id rather than locator to emphasize its global effect """ - index = self.course_index.find_one({'_id': course_id}) + index = self.db_connection.get_course_index(course_id) if index is None: raise ItemNotFoundError(course_id) # this is the only real delete in the system. should it do something else? - self.course_index.remove(index['_id']) + self.db_connection.delete_course_index(index['_id']) def get_errored_courses(self): """ @@ -1296,7 +1251,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): block['fields']["children"] = [ usage_id for usage_id in block['fields']["children"] if usage_id in original_structure['blocks'] ] - self.structures.update({'_id': original_structure['_id']}, original_structure) + self.db_connection.update_structure(original_structure) # clear cache again b/c inheritance may be wrong over orphans self._clear_cache(original_structure['_id']) @@ -1379,7 +1334,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): else: return None else: - index_entry = self.course_index.find_one({'_id': locator.course_id}) + index_entry = self.db_connection.get_course_index(locator.course_id) is_head = ( locator.version_guid is None or index_entry['versions'][locator.branch] == locator.version_guid @@ -1424,9 +1379,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param course_locator: :param new_id: """ - self.course_index.update( - {"_id": index_entry["_id"]}, - {"$set": {"versions.{}".format(branch): new_id}}) + index_entry['versions'][branch] = new_id + self.db_connection.update_course_index(index_entry) def _partition_fields_by_scope(self, category, fields): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py index 049fbd2ef8..74975f2896 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py @@ -59,9 +59,9 @@ class TestMigration(unittest.TestCase): dbref = self.loc_mapper.db dbref.drop_collection(self.loc_mapper.location_map) split_db = self.split_mongo.db - split_db.drop_collection(split_db.course_index) - split_db.drop_collection(split_db.structures) - split_db.drop_collection(split_db.definitions) + split_db.drop_collection(self.split_mongo.db_connection.course_index) + split_db.drop_collection(self.split_mongo.db_connection.structures) + split_db.drop_collection(self.split_mongo.db_connection.definitions) # old_mongo doesn't give a db attr, but all of the dbs are the same dbref.drop_collection(self.old_mongo.collection) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index aa095d8e4c..92de35e39f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -1018,41 +1018,29 @@ class TestCourseCreation(SplitModuleTest): Test changing the org, pretty id, etc of a course. Test that it doesn't allow changing the id, etc. """ locator = CourseLocator(course_id="GreekHero", branch='draft') - modulestore().update_course_index(locator, {'org': 'funkyU'}) + course_info = modulestore().get_course_index_info(locator) + course_info['org'] = 'funkyU' + modulestore().update_course_index(course_info) course_info = modulestore().get_course_index_info(locator) self.assertEqual(course_info['org'], 'funkyU') - modulestore().update_course_index(locator, {'org': 'moreFunky', 'prettyid': 'Ancient Greek Demagods'}) + course_info['org'] = 'moreFunky' + course_info['prettyid'] = 'Ancient Greek Demagods' + modulestore().update_course_index(course_info) course_info = modulestore().get_course_index_info(locator) self.assertEqual(course_info['org'], 'moreFunky') self.assertEqual(course_info['prettyid'], 'Ancient Greek Demagods') - self.assertRaises(ValueError, modulestore().update_course_index, locator, {'_id': 'funkygreeks'}) - - with self.assertRaises(ValueError): - modulestore().update_course_index( - locator, - {'edited_on': datetime.datetime.now(UTC)} - ) - with self.assertRaises(ValueError): - modulestore().update_course_index( - locator, - {'edited_by': 'sneak'} - ) - - self.assertRaises(ValueError, modulestore().update_course_index, locator, - {'versions': {'draft': self.GUID_D1}}) - # an allowed but not necessarily recommended way to revert the draft version versions = course_info['versions'] versions['draft'] = self.GUID_D1 - modulestore().update_course_index(locator, {'versions': versions}, update_versions=True) + modulestore().update_course_index(course_info) course = modulestore().get_course(locator) self.assertEqual(str(course.location.version_guid), self.GUID_D1) # an allowed but not recommended way to publish a course versions['published'] = self.GUID_D1 - modulestore().update_course_index(locator, {'versions': versions}, update_versions=True) + modulestore().update_course_index(course_info) course = modulestore().get_course(CourseLocator(course_id=locator.course_id, branch="published")) self.assertEqual(str(course.location.version_guid), self.GUID_D1) @@ -1068,9 +1056,9 @@ class TestCourseCreation(SplitModuleTest): self.assertEqual(new_course.location.usage_id, 'top') self.assertEqual(new_course.category, 'chapter') # look at db to verify - db_structure = modulestore().structures.find_one({ - '_id': new_course.location.as_object_id(new_course.location.version_guid) - }) + db_structure = modulestore().db_connection.get_structure( + new_course.location.as_object_id(new_course.location.version_guid) + ) self.assertIsNotNone(db_structure, "Didn't find course") self.assertNotIn('course', db_structure['blocks']) self.assertIn('top', db_structure['blocks']) From 783e4b223fbd8c9837e39d16085b18b05bd6fb2c Mon Sep 17 00:00:00 2001 From: Zubair Afzal Date: Thu, 7 Nov 2013 17:08:58 +0500 Subject: [PATCH 19/69] Show error on invalid html in course handout edit + Added tests STUD-293 --- .../features/course-updates.feature | 14 ++++++ .../contentstore/features/course-updates.py | 29 ++++++++++++ .../coffee/spec/views/course_info_spec.coffee | 19 ++++++++ cms/static/js/views/course_info_handout.js | 46 ++++++++++++------- cms/static/js/views/course_info_helper.js | 5 +- .../js/course_info_handouts.underscore | 3 +- 6 files changed, 97 insertions(+), 19 deletions(-) diff --git a/cms/djangoapps/contentstore/features/course-updates.feature b/cms/djangoapps/contentstore/features/course-updates.feature index 6f24fba68c..152da9c349 100644 --- a/cms/djangoapps/contentstore/features/course-updates.feature +++ b/cms/djangoapps/contentstore/features/course-updates.feature @@ -76,3 +76,17 @@ Feature: CMS.Course updates Then I see the handout "/c4x/MITx/999/asset/modified.jpg" And when I reload the page Then I see the handout "/c4x/MITx/999/asset/modified.jpg" + + Scenario: Users cannot save handouts with bad html until edit or update it properly + Given I have opened a new course in Studio + And I go to the course updates page + When I modify the handout to "

        [LINK TEXT]

        " + Then I see the handout error text + And I see handout save button disabled + When I edit the handout to "

        home

        " + Then I see handout save button re-enabled + When I save handout edit + # Can only do partial text matches because of the quotes with in quotes (and regexp step matching). + Then I see the handout "https://www.google.com.pk/" + And when I reload the page + Then I see the handout "https://www.google.com.pk/" diff --git a/cms/djangoapps/contentstore/features/course-updates.py b/cms/djangoapps/contentstore/features/course-updates.py index da74f5aa4b..b41578c907 100644 --- a/cms/djangoapps/contentstore/features/course-updates.py +++ b/cms/djangoapps/contentstore/features/course-updates.py @@ -90,6 +90,35 @@ def check_handout(_step, handout): assert handout in world.css_html(handout_css) +@step(u'I see the handout error text') +def check_handout_error(_step): + handout_error_css = 'div#handout_error' + assert world.css_has_class(handout_error_css, 'is-shown') + + +@step(u'I see handout save button disabled') +def check_handout_error(_step): + handout_save_button = 'form.edit-handouts-form a.save-button' + assert world.css_has_class(handout_save_button, 'is-disabled') + + +@step(u'I edit the handout to "([^"]*)"$') +def edit_handouts(_step, text): + type_in_codemirror(0, text) + + +@step(u'I see handout save button re-enabled') +def check_handout_error(_step): + handout_save_button = 'form.edit-handouts-form a.save-button' + assert not world.css_has_class(handout_save_button, 'is-disabled') + + +@step(u'I save handout edit') +def check_handout_error(_step): + save_css = 'a.save-button' + world.css_click(save_css) + + def change_text(text): type_in_codemirror(0, text) save_css = 'a.save-button' diff --git a/cms/static/coffee/spec/views/course_info_spec.coffee b/cms/static/coffee/spec/views/course_info_spec.coffee index 3c388fa593..1e843d59fb 100644 --- a/cms/static/coffee/spec/views/course_info_spec.coffee +++ b/cms/static/coffee/spec/views/course_info_spec.coffee @@ -196,3 +196,22 @@ define ["js/views/course_info_handout", "js/views/course_info_update", "js/model @handoutsEdit.$el.find('.edit-button').click() expect(@handoutsEdit.$codeMirror.getValue().trim()).toEqual('/static/fromServer.jpg') + it "can open course handouts with bad html on edit", -> + # Enter some bad html in handouts section, verifying that the + # model/handoutform opens when "Edit" is clicked + + @model = new ModuleInfo({ + id: 'handouts-id', + data: '

        [LINK TEXT]

        ') + expect($('.edit-handouts-form').is(':hidden')).toEqual(false) \ No newline at end of file diff --git a/cms/static/js/views/course_info_handout.js b/cms/static/js/views/course_info_handout.js index f9804d03a4..9309deda1b 100644 --- a/cms/static/js/views/course_info_handout.js +++ b/cms/static/js/views/course_info_handout.js @@ -30,6 +30,7 @@ define(["backbone", "underscore", "codemirror", "js/views/feedback_notification" model: this.model })) ); + $('.handouts-content').html(this.model.get('data')); this.$preview = this.$el.find('.handouts-content'); this.$form = this.$el.find(".edit-handouts-form"); this.$editor = this.$form.find('.handouts-content-editor'); @@ -50,32 +51,43 @@ define(["backbone", "underscore", "codemirror", "js/views/feedback_notification" }, onSave: function(event) { - this.model.set('data', this.$codeMirror.getValue()); - var saving = new NotificationView.Mini({ - title: gettext('Saving…') - }); - saving.show(); - this.model.save({}, { - success: function() { - saving.hide(); - } - }); - this.render(); - this.$form.hide(); - this.closeEditor(); - - analytics.track('Saved Course Handouts', { - 'course': course_location_analytics - }); + $('#handout_error').removeClass('is-shown'); + $('.save-button').removeClass('is-disabled'); + if ($('.CodeMirror-lines').find('.cm-error').length == 0){ + this.model.set('data', this.$codeMirror.getValue()); + var saving = new NotificationView.Mini({ + title: gettext('Saving…') + }); + saving.show(); + this.model.save({}, { + success: function() { + saving.hide(); + } + }); + this.render(); + this.$form.hide(); + this.closeEditor(); + analytics.track('Saved Course Handouts', { + 'course': course_location_analytics + }); + }else{ + $('#handout_error').addClass('is-shown'); + $('.save-button').addClass('is-disabled'); + event.preventDefault(); + } }, onCancel: function(event) { + $('#handout_error').removeClass('is-shown'); + $('.save-button').removeClass('is-disabled'); this.$form.hide(); this.closeEditor(); }, closeEditor: function() { + $('#handout_error').removeClass('is-shown'); + $('.save-button').removeClass('is-disabled'); this.$form.hide(); ModalUtils.hideModalCover(); this.$form.find('.CodeMirror').remove(); diff --git a/cms/static/js/views/course_info_helper.js b/cms/static/js/views/course_info_helper.js index ec4a6ba550..fb3474cdb0 100644 --- a/cms/static/js/views/course_info_helper.js +++ b/cms/static/js/views/course_info_helper.js @@ -6,7 +6,10 @@ define(["codemirror", "utility"], var $codeMirror = CodeMirror.fromTextArea(textArea, { mode: "text/html", lineNumbers: true, - lineWrapping: true + lineWrapping: true, + onChange: function () { + $('.save-button').removeClass('is-disabled'); + } }); $codeMirror.setValue(content); $codeMirror.clearHistory(); diff --git a/cms/templates/js/course_info_handouts.underscore b/cms/templates/js/course_info_handouts.underscore index 7fbbe9bc33..6ce8518a32 100644 --- a/cms/templates/js/course_info_handouts.underscore +++ b/cms/templates/js/course_info_handouts.underscore @@ -3,12 +3,13 @@

        Course Handouts

        <%if (model.get('data') != null) { %>
        - <%= model.get('data') %> +
        <% } else {%>

        ${_("You have no handouts defined")}

        <% } %>
        +
        <%=gettext("There is invalid code in your content. Please check to make sure it is valid HTML.")%>
        From cc2f1e73bfd754cba2c6b875ce93df281d9d9780 Mon Sep 17 00:00:00 2001 From: polesye Date: Wed, 20 Nov 2013 15:48:15 +0200 Subject: [PATCH 20/69] BLD-524: Add missing assert in video test. --- cms/djangoapps/contentstore/features/video.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/features/video.py b/cms/djangoapps/contentstore/features/video.py index 5408c48290..c97dba10b9 100644 --- a/cms/djangoapps/contentstore/features/video.py +++ b/cms/djangoapps/contentstore/features/video.py @@ -181,7 +181,7 @@ def click_on_the_caption(_step, index): @step('I see caption line with data-index "([^"]*)" has class "([^"]*)"$') def caption_line_has_class(_step, index, className): SELECTOR = ".subtitles > li[data-index='{index}']".format(index=int(index.strip())) - world.css_has_class(SELECTOR, className.strip()) + assert world.css_has_class(SELECTOR, className.strip()) @step('I see a range on slider$') From f01b36b5d42923155a15ed7a86e78e125a4c4191 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 18 Nov 2013 14:48:11 -0500 Subject: [PATCH 21/69] Test for i4x on returned pages. STUD-941 --- .../contentstore/tests/test_contentstore.py | 146 ++++++++++++++---- cms/static/js/models/course_info.js | 5 +- cms/templates/asset_index.html | 2 +- cms/templates/course_info.html | 1 - cms/templates/settings.html | 2 +- cms/templates/widgets/segment-io.html | 15 +- 6 files changed, 131 insertions(+), 40 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 2b773e991d..34f15b6db8 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1,6 +1,5 @@ #pylint: disable=E1101 -import json import shutil import mock @@ -15,6 +14,7 @@ from fs.osfs import OSFS import copy from json import loads from datetime import timedelta +from django.test import TestCase from django.contrib.auth.models import User from django.dispatch import Signal @@ -53,6 +53,7 @@ from pytz import UTC from uuid import uuid4 from pymongo import MongoClient from student.models import CourseEnrollment +import re from contentstore.utils import delete_course_and_groups from xmodule.modulestore.django import loc_mapper @@ -135,6 +136,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()})) self.assertEqual(resp.status_code, 200) + # TODO: uncomment after edit_unit no longer using locations. + # _test_no_locations(self, resp) for expected in expected_types: self.assertIn(expected, resp.content) @@ -160,15 +163,21 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): resp = self.client.get_html(reverse('edit_unit', kwargs={'location': location.url()})) self.assertEqual(resp.status_code, 400) + _test_no_locations(self, resp, status_code=400) def check_edit_unit(self, test_course_name): import_from_xml(modulestore('direct'), 'common/test/data/', [test_course_name]) - for descriptor in modulestore().get_items(Location(None, None, 'vertical', None, None)): + items = modulestore().get_items(Location('i4x', 'edX', test_course_name, 'vertical', None, None)) + # Assert is here to make sure that the course being tested actually has verticals. + self.assertGreater(len(items), 0) + for descriptor in items: print "Checking ", descriptor.location.url() print descriptor.__class__, descriptor.location resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()})) self.assertEqual(resp.status_code, 200) + # TODO: uncomment after edit_unit not using locations. + # _test_no_locations(self, resp) def lockAnAsset(self, content_store, course_location): """ @@ -483,6 +492,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): ) resp = self.client.get_html(locator.url_reverse('xblock')) self.assertEqual(resp.status_code, 200) + # TODO: uncomment when preview no longer has locations being returned. + # _test_no_locations(self, resp) return resp def test_delete(self): @@ -841,6 +852,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): def test_bad_contentstore_request(self): resp = self.client.get_html('http://localhost:8001/c4x/CDX/123123/asset/&images_circuits_Lab7Solution2.png') self.assertEqual(resp.status_code, 400) + _test_no_locations(self, resp, 400) def test_rewrite_nonportable_links_on_import(self): module_store = modulestore('direct') @@ -1026,12 +1038,11 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): items = module_store.get_items(stub_location.replace(category='vertical', name=None)) self.assertGreater(len(items), 0) for descriptor in items: - # don't try to look at private verticals. Right now we're running - # the service in non-draft aware - if getattr(descriptor, 'is_draft', False): - print "Checking {0}....".format(descriptor.location.url()) - resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()})) - self.assertEqual(resp.status_code, 200) + print "Checking {0}....".format(descriptor.location.url()) + resp = self.client.get_html(reverse('edit_unit', kwargs={'location': descriptor.location.url()})) + self.assertEqual(resp.status_code, 200) + # TODO: uncomment when edit_unit no longer has locations. + # _test_no_locations(self, resp) # verify that we have the content in the draft store as well vertical = draft_store.get_item( @@ -1508,6 +1519,7 @@ class ContentStoreTest(ModuleStoreTestCase): status_code=200, html=True ) + _test_no_locations(self, resp) def test_course_factory(self): """Test that the course factory works correctly.""" @@ -1530,6 +1542,8 @@ class ContentStoreTest(ModuleStoreTestCase): status_code=200, html=True ) + # TODO: uncomment when course index no longer has locations being returned. + # _test_no_locations(self, resp) def test_course_overview_view_with_course(self): """Test viewing the course overview page with an existing course""" @@ -1589,6 +1603,13 @@ class ContentStoreTest(ModuleStoreTestCase): Import and walk through some common URL endpoints. This just verifies non-500 and no other correct behavior, so it is not a deep test """ + def test_get_html(page): + # Helper function for getting HTML for a page in Studio and + # checking that it does not error. + resp = self.client.get_html(new_location.url_reverse(page)) + self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) + import_from_xml(modulestore('direct'), 'common/test/data/', ['simple']) loc = Location(['i4x', 'edX', 'simple', 'course', '2012_Fall', None]) new_location = loc_mapper().translate_location(loc.course_id, loc, False, True) @@ -1598,42 +1619,46 @@ class ContentStoreTest(ModuleStoreTestCase): self.assertContains(resp, 'Chapter 2') # go to various pages - - # import page - resp = self.client.get_html(new_location.url_reverse('import/', '')) - self.assertEqual(resp.status_code, 200) - - # export page - resp = self.client.get_html(new_location.url_reverse('export/', '')) - self.assertEqual(resp.status_code, 200) - - # course team - url = new_location.url_reverse('course_team/', '') - resp = self.client.get_html(url) - self.assertEqual(resp.status_code, 200) - - # course info - resp = self.client.get(new_location.url_reverse('course_info')) - self.assertEqual(resp.status_code, 200) + test_get_html('import') + test_get_html('export') + test_get_html('course_team') + test_get_html('course_info') + test_get_html('checklists') + test_get_html('assets') # settings_details - resp = self.client.get(reverse('settings_details', + resp = self.client.get_html(reverse('settings_details', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) # settings_details - resp = self.client.get(reverse('settings_grading', + resp = self.client.get_html(reverse('settings_grading', kwargs={'org': loc.org, 'course': loc.course, 'name': loc.name})) self.assertEqual(resp.status_code, 200) + # TODO: uncomment when grading is not using old locations. + # _test_no_locations(self, resp) - # assets_handler (HTML for full page content) - url = new_location.url_reverse('assets/', '') - resp = self.client.get_html(url) + # advanced settings + resp = self.client.get_html(reverse('course_advanced_settings', + kwargs={'org': loc.org, + 'course': loc.course, + 'name': loc.name})) self.assertEqual(resp.status_code, 200) + # TODO: uncomment when advanced settings not using old locations. + # _test_no_locations(self, resp) + + # textbook index + resp = self.client.get_html(reverse('textbook_index', + kwargs={'org': loc.org, + 'course': loc.course, + 'name': loc.name})) + self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) # go look at a subsection page subsection_location = loc.replace(category='sequential', name='test_sequence') @@ -1641,12 +1666,23 @@ class ContentStoreTest(ModuleStoreTestCase): reverse('edit_subsection', kwargs={'location': subsection_location.url()}) ) self.assertEqual(resp.status_code, 200) + # TODO: uncomment when grading and outline not using old locations. + # _test_no_locations(self, resp) # go look at the Edit page unit_location = loc.replace(category='vertical', name='test_vertical') resp = self.client.get_html( reverse('edit_unit', kwargs={'location': unit_location.url()})) self.assertEqual(resp.status_code, 200) + # TODO: uncomment when edit_unit not using old locations. + # _test_no_locations(self, resp) + + resp = self.client.get_html(reverse('edit_tabs', + kwargs={'org': loc.org, + 'course': loc.course, + 'coursename': loc.name})) + self.assertEqual(resp.status_code, 200) + _test_no_locations(self, resp) def delete_item(category, name): """ Helper method for testing the deletion of an xblock item. """ @@ -1654,6 +1690,7 @@ class ContentStoreTest(ModuleStoreTestCase): del_location = loc_mapper().translate_location(loc.course_id, del_loc, False, True) resp = self.client.delete(del_location.url_reverse('xblock')) self.assertEqual(resp.status_code, 204) + _test_no_locations(self, resp, status_code=204, html=False) # delete a component delete_item(category='html', name='test_html') @@ -1853,7 +1890,10 @@ class ContentStoreTest(ModuleStoreTestCase): Show the course overview page. """ new_location = loc_mapper().translate_location(location.course_id, location, False, True) - return self.client.get_html(new_location.url_reverse('course/', '')) + resp = self.client.get_html(new_location.url_reverse('course/', '')) + # TODO: uncomment when i4x no longer in overview. + # _test_no_locations(self, resp) + return resp @override_settings(MODULESTORE=TEST_MODULESTORE) @@ -1920,6 +1960,32 @@ class MetadataSaveTestCase(ModuleStoreTestCase): pass +class EntryPageTestCase(TestCase): + """ + Tests entry pages that aren't specific to a course. + """ + def setUp(self): + self.client = AjaxEnabledTestClient() + + def _test_page(self, page, status_code=200): + resp = self.client.get_html(reverse(page)) + self.assertEqual(resp.status_code, status_code) + _test_no_locations(self, resp, status_code) + + def test_how_it_works(self): + self._test_page("howitworks") + + def test_signup(self): + self._test_page("signup") + + def test_login(self): + self._test_page("login") + + def test_logout(self): + # Logout redirects. + self._test_page("logout", 302) + + def _create_course(test, course_data): """ Creates a course via an AJAX request and verifies the URL returned in the response. @@ -1945,3 +2011,21 @@ def _course_factory_create_course(): def _get_course_id(test_course_data): """Returns the course ID (org/number/run).""" return "{org}/{number}/{run}".format(**test_course_data) + + +def _test_no_locations(test, resp, status_code=200, html=True): + """ + Verifies that "i4x", which appears in old locations, but not + new locators, does not appear in the HTML response output. + Used to verify that database refactoring is complete. + """ + test.assertNotContains(resp, 'i4x', status_code=status_code, html=html) + if html: + # For HTML pages, it is nice to call the method with html=True because + # it checks that the HTML properly parses. However, it won't find i4x usages + # in JavaScript blocks. + content = resp.content + num_jump_to = len(re.findall(r"8000(\S)*jump_to/i4x", content)) + total_i4x = len(re.findall(r"i4x", content)) + + test.assertEqual(total_i4x - num_jump_to, 0, "i4x found outside of LMS jump-to links") diff --git a/cms/static/js/models/course_info.js b/cms/static/js/models/course_info.js index e5a6114dff..e4c816ccf3 100644 --- a/cms/static/js/models/course_info.js +++ b/cms/static/js/models/course_info.js @@ -5,12 +5,9 @@ define(["backbone"], function(Backbone) { url: '', defaults: { - "courseId": "", // the location url "updates" : null, // UpdateCollection "handouts": null // HandoutCollection - }, - - idAttribute : "courseId" + } }); return CourseInfo; }); diff --git a/cms/templates/asset_index.html b/cms/templates/asset_index.html index 5576664df7..4f6f14a466 100644 --- a/cms/templates/asset_index.html +++ b/cms/templates/asset_index.html @@ -187,7 +187,7 @@ require(["domReady", "jquery", "gettext", "js/models/asset", "js/collections/ass ${_('close')}

        ${_("Upload New File")}

        -

        +

        diff --git a/cms/templates/course_info.html b/cms/templates/course_info.html index 401d32ccb2..8cddb8a8c5 100644 --- a/cms/templates/course_info.html +++ b/cms/templates/course_info.html @@ -33,7 +33,6 @@ require(["domReady!", "jquery", "js/collections/course_update", "js/models/modul var editor = new CourseInfoEditView({ el: $('.main-wrapper'), model : new CourseInfoModel({ - courseId : '${context_course.location}', updates : course_updates, base_asset_url : '${base_asset_url}', handouts : course_handouts diff --git a/cms/templates/settings.html b/cms/templates/settings.html index 33a5360da0..ca60bb2e90 100644 --- a/cms/templates/settings.html +++ b/cms/templates/settings.html @@ -87,7 +87,7 @@ require(["domReady!", "jquery", "js/models/settings/course_details", "js/views/s

        ${_("Course Summary Page")} ${_("(for student enrollment and access)")}

        -

        https:${utils.get_lms_link_for_about_page(course_location)}

        +

        https:${utils.get_lms_link_for_about_page(course_location)}

          diff --git a/cms/templates/widgets/segment-io.html b/cms/templates/widgets/segment-io.html index a6a186b2a3..d250e9c23b 100644 --- a/cms/templates/widgets/segment-io.html +++ b/cms/templates/widgets/segment-io.html @@ -1,9 +1,20 @@ +<%! +from xmodule.modulestore.django import loc_mapper +%> + +% if context_course: +<% + ctx_loc = context_course.location + locator = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True) +%> +% endif + % if settings.MITX_FEATURES.get('SEGMENT_IO'): - + But in this there should be
          Great ideas require offsetting. - + bad tests require drivel
          - + [code] Code should be nicely monospaced. [/code] @@ -322,7 +363,7 @@ describe 'MarkdownEditingDescriptor', -> expect(data).toEqual("""

          Not a header

          A header

          - +

          Multiple choice w/ parentheticals

          @@ -332,7 +373,7 @@ describe 'MarkdownEditingDescriptor', -> no space b4 close paren - +

          Choice checks

          @@ -343,25 +384,25 @@ describe 'MarkdownEditingDescriptor', -> no space - +

          Option with multiple correct ones

          - + - +

          Option with embedded parens

          - + - +

          What happens w/ empty correct options?

          - + - +

          Explanation

          @@ -379,14 +420,14 @@ describe 'MarkdownEditingDescriptor', -> console.log(two * 2); - +

          But in this there should be

          Great ideas require offsetting.

          - +

          bad tests require drivel

          - + 
          
         Code should be nicely monospaced.
          diff --git a/common/lib/xmodule/xmodule/js/src/problem/edit.coffee b/common/lib/xmodule/xmodule/js/src/problem/edit.coffee index ba85db574b..9f5d361009 100644 --- a/common/lib/xmodule/xmodule/js/src/problem/edit.coffee +++ b/common/lib/xmodule/xmodule/js/src/problem/edit.coffee @@ -228,11 +228,13 @@ class @MarkdownEditingDescriptor extends XModule.Descriptor }); // replace string and numerical - xml = xml.replace(/^\=\s*(.*?$)/gm, function(match, p) { - var string; - var floatValue = parseFloat(p); + xml = xml.replace(/(^\=\s*(.*?$)(\n*or\=\s*(.*?$))*)+/gm, function(match, p) { + var string, + answersList = p.replace(/^(or)?=\s*/gm, '').split('\n'), + floatValue = parseFloat(answersList[0]); + if(!isNaN(floatValue)) { - var params = /(.*?)\+\-\s*(.*?$)/.exec(p); + var params = /(.*?)\+\-\s*(.*?$)/.exec(answersList[0]); if(params) { string = '\n'; string += ' \n'; @@ -242,10 +244,16 @@ class @MarkdownEditingDescriptor extends XModule.Descriptor string += ' \n'; string += '\n\n'; } else { - string = '\n \n\n\n'; + var answers = []; + + for(var i = 0; i < answersList.length; i++) { + answers.push(answersList[i]) + } + + string = '\n \n\n\n'; } return string; - }); + }); // replace selects xml = xml.replace(/\[\[(.+?)\]\]/g, function(match, p) { @@ -262,13 +270,13 @@ class @MarkdownEditingDescriptor extends XModule.Descriptor selectString += '\n\n'; return selectString; }); - + // replace explanations xml = xml.replace(/\[explanation\]\n?([^\]]*)\[\/?explanation\]/gmi, function(match, p1) { var selectString = '\n
          \nExplanation\n\n' + p1 + '\n
          \n          '; return selectString; }); - + // replace code blocks xml = xml.replace(/\[code\]\n?([^\]]*)\[\/?code\]/gmi, function(match, p1) { var selectString = '
          \n' + p1 + '
          '; @@ -293,7 +301,7 @@ class @MarkdownEditingDescriptor extends XModule.Descriptor // rid white space xml = xml.replace(/\n\n\n/g, '\n'); - + // surround w/ problem tag xml = '\n' + xml + '\n'; From 1b2be30c45aeb434728216c26d7870c85399a19e Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 14 Nov 2013 15:45:16 -0500 Subject: [PATCH 32/69] Convert tabs to RESTful URL. STUD-850 --- .../features/static-pages.feature | 19 +- .../contentstore/features/static-pages.py | 44 ++++ .../contentstore/tests/test_contentstore.py | 104 +++++---- cms/djangoapps/contentstore/views/tabs.py | 200 +++++++++--------- cms/static/coffee/spec/main.coffee | 3 +- cms/static/coffee/src/views/tabs.coffee | 7 +- cms/static/js/models/explicit_url.js | 14 ++ .../js/spec/models/explicit_url_spec.js | 12 ++ cms/templates/edit-tabs.html | 11 +- cms/templates/widgets/header.html | 17 +- cms/urls.py | 5 +- 11 files changed, 272 insertions(+), 164 deletions(-) create mode 100644 cms/static/js/models/explicit_url.js create mode 100644 cms/static/js/spec/models/explicit_url_spec.js diff --git a/cms/djangoapps/contentstore/features/static-pages.feature b/cms/djangoapps/contentstore/features/static-pages.feature index 54d23d985d..39399fb207 100644 --- a/cms/djangoapps/contentstore/features/static-pages.feature +++ b/cms/djangoapps/contentstore/features/static-pages.feature @@ -9,10 +9,8 @@ Feature: CMS.Static Pages Then I should see a static page named "Empty" Scenario: Users can delete static pages - Given I have opened a new course in Studio - And I go to the static pages page - And I add a new page - And I "delete" the static page + Given I have created a static page + When I "delete" the static page Then I am shown a prompt When I confirm the prompt Then I should not see any static pages @@ -20,9 +18,16 @@ Feature: CMS.Static Pages # Safari won't update the name properly @skip_safari Scenario: Users can edit static pages - Given I have opened a new course in Studio - And I go to the static pages page - And I add a new page + Given I have created a static page When I "edit" the static page And I change the name to "New" Then I should see a static page named "New" + + # Safari won't update the name properly + @skip_safari + Scenario: Users can reorder static pages + Given I have created two different static pages + When I reorder the tabs + Then the tabs are in the reverse order + And I reload the page + Then the tabs are in the reverse order diff --git a/cms/djangoapps/contentstore/features/static-pages.py b/cms/djangoapps/contentstore/features/static-pages.py index 58932ad8e2..0adb4b1e54 100644 --- a/cms/djangoapps/contentstore/features/static-pages.py +++ b/cms/djangoapps/contentstore/features/static-pages.py @@ -48,3 +48,47 @@ def change_name(step, new_name): world.trigger_event(input_css) save_button = 'a.save-button' world.css_click(save_button) + + +@step(u'I reorder the tabs') +def reorder_tabs(_step): + # For some reason, the drag_and_drop method did not work in this case. + draggables = world.css_find('.drag-handle') + source = draggables.first + target = draggables.last + source.action_chains.click_and_hold(source._element).perform() + source.action_chains.move_to_element_with_offset(target._element, 0, 50).perform() + source.action_chains.release().perform() + + +@step(u'I have created a static page') +def create_static_page(step): + step.given('I have opened a new course in Studio') + step.given('I go to the static pages page') + step.given('I add a new page') + + +@step(u'I have created two different static pages') +def create_two_pages(step): + step.given('I have created a static page') + step.given('I "edit" the static page') + step.given('I change the name to "First"') + step.given('I add a new page') + # Verify order of tabs + _verify_tab_names('First', 'Empty') + + +@step(u'the tabs are in the reverse order') +def tabs_in_reverse_order(step): + _verify_tab_names('Empty', 'First') + + +def _verify_tab_names(first, second): + world.wait_for( + func=lambda _: len(world.css_find('.xmodule_StaticTabModule')) == 2, + timeout=200, + timeout_msg="Timed out waiting for two tabs to be present" + ) + tabs = world.css_find('.xmodule_StaticTabModule') + assert tabs[0].text == first + assert tabs[1].text == second diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 34f15b6db8..cc0e50fbf6 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -179,7 +179,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # TODO: uncomment after edit_unit not using locations. # _test_no_locations(self, resp) - def lockAnAsset(self, content_store, course_location): + def _lock_an_asset(self, content_store, course_location): """ Lock an arbitrary asset in the course :param course_location: @@ -407,15 +407,63 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertEqual(course.tabs, expected_tabs) def test_static_tab_reordering(self): - def get_tab_locator(tab): - tab_location = 'i4x://MITx/999/static_tab/{0}'.format(tab['url_slug']) - return unicode(loc_mapper().translate_location( - course.location.course_id, Location(tab_location), False, True - )) + module_store, course_location, new_location = self._create_static_tabs() + course = module_store.get_item(course_location) + + # reverse the ordering + reverse_tabs = [] + for tab in course.tabs: + if tab['type'] == 'static_tab': + reverse_tabs.insert(0, unicode(self._get_tab_locator(course, tab))) + + self.client.ajax_post(new_location.url_reverse('tabs'), {'tabs': reverse_tabs}) + + course = module_store.get_item(course_location) + + # compare to make sure that the tabs information is in the expected order after the server call + course_tabs = [] + for tab in course.tabs: + if tab['type'] == 'static_tab': + course_tabs.append(unicode(self._get_tab_locator(course, tab))) + + self.assertEqual(reverse_tabs, course_tabs) + + def test_static_tab_deletion(self): + module_store, course_location, _ = self._create_static_tabs() + + course = module_store.get_item(course_location) + num_tabs = len(course.tabs) + last_tab = course.tabs[num_tabs - 1] + url_slug = last_tab['url_slug'] + delete_url = self._get_tab_locator(course, last_tab).url_reverse('xblock') + + self.client.delete(delete_url) + + course = module_store.get_item(course_location) + self.assertEqual(num_tabs - 1, len(course.tabs)) + + def tab_matches(tab): + """ Checks if the tab matches the one we deleted """ + return tab['type'] == 'static_tab' and tab['url_slug'] == url_slug + + tab_found = any(tab_matches(tab) for tab in course.tabs) + + self.assertFalse(tab_found, "tab should have been deleted") + + def _get_tab_locator(self, course, tab): + """ Returns the locator for a given tab. """ + tab_location = 'i4x://MITx/999/static_tab/{0}'.format(tab['url_slug']) + return loc_mapper().translate_location( + course.location.course_id, Location(tab_location), False, True + ) + + def _create_static_tabs(self): + """ Creates two static tabs in a dummy course. """ module_store = modulestore('direct') - locator = _course_factory_create_course() - course_location = loc_mapper().translate_locator_to_location(locator) + CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') + course_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None]) + new_location = loc_mapper().translate_location(course_location.course_id, course_location, False, True) ItemFactory.create( parent_location=course_location, @@ -426,25 +474,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): category="static_tab", display_name="Static_2") - course = module_store.get_item(course_location) - - # reverse the ordering - reverse_tabs = [] - for tab in course.tabs: - if tab['type'] == 'static_tab': - reverse_tabs.insert(0, get_tab_locator(tab)) - - self.client.ajax_post(reverse('reorder_static_tabs'), {'tabs': reverse_tabs}) - - course = module_store.get_item(course_location) - - # compare to make sure that the tabs information is in the expected order after the server call - course_tabs = [] - for tab in course.tabs: - if tab['type'] == 'static_tab': - course_tabs.append(get_tab_locator(tab)) - - self.assertEqual(reverse_tabs, course_tabs) + return module_store, course_location, new_location def test_import_polls(self): module_store = modulestore('direct') @@ -633,7 +663,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): thumbnail = content_store.find(thumbnail_location, throw_on_not_found=False) self.assertIsNotNone(thumbnail) - def _delete_asset_in_course (self): + def _delete_asset_in_course(self): """ Helper method for: 1) importing course from xml @@ -972,7 +1002,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertIn(private_location_no_draft.url(), sequential.children) - locked_asset = self.lockAnAsset(content_store, location) + locked_asset = self._lock_an_asset(content_store, location) locked_asset_attrs = content_store.get_attrs(locked_asset) # the later import will reupload del locked_asset_attrs['uploadDate'] @@ -1027,7 +1057,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): shutil.rmtree(root_dir) def check_import(self, module_store, root_dir, draft_store, content_store, stub_location, course_location, - locked_asset, locked_asset_attrs): + locked_asset, locked_asset_attrs): # reimport import_from_xml( module_store, root_dir, ['test_export'], draft_store=draft_store, @@ -1226,7 +1256,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): handouts_locator = loc_mapper().translate_location('edX/toy/2012_Fall', handout_location) # get module info (json) - resp = self.client.get(handouts_locator.url_reverse('/xblock', '')) + resp = self.client.get(handouts_locator.url_reverse('/xblock')) # make sure we got a successful response self.assertEqual(resp.status_code, 200) @@ -1403,7 +1433,7 @@ class ContentStoreTest(ModuleStoreTestCase): second_course_data = self.assert_created_course(number_suffix=uuid4().hex) # unseed the forums for the first course - course_id =_get_course_id(test_course_data) + course_id = _get_course_id(test_course_data) delete_course_and_groups(course_id, commit=True) self.assertFalse(are_permissions_roles_seeded(course_id)) @@ -1625,6 +1655,7 @@ class ContentStoreTest(ModuleStoreTestCase): test_get_html('course_info') test_get_html('checklists') test_get_html('assets') + test_get_html('tabs') # settings_details resp = self.client.get_html(reverse('settings_details', @@ -1677,13 +1708,6 @@ class ContentStoreTest(ModuleStoreTestCase): # TODO: uncomment when edit_unit not using old locations. # _test_no_locations(self, resp) - resp = self.client.get_html(reverse('edit_tabs', - kwargs={'org': loc.org, - 'course': loc.course, - 'coursename': loc.name})) - self.assertEqual(resp.status_code, 200) - _test_no_locations(self, resp) - def delete_item(category, name): """ Helper method for testing the deletion of an xblock item. """ del_loc = loc.replace(category=category, name=name) @@ -1997,7 +2021,7 @@ def _create_course(test, course_data): test.assertEqual(response.status_code, 200) data = parse_json(response) test.assertNotIn('ErrMsg', data) - test.assertEqual(data['url'], new_location.url_reverse("course/", "")) + test.assertEqual(data['url'], new_location.url_reverse("course")) def _course_factory_create_course(): diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 40ea9bfd6b..46791ddc26 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -2,12 +2,13 @@ Views related to course tabs """ from access import has_access -from util.json_request import expect_json +from util.json_request import expect_json, JsonResponse -from django.http import HttpResponse, HttpResponseBadRequest +from django.http import HttpResponseNotFound from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django_future.csrf import ensure_csrf_cookie +from django.views.decorators.http import require_http_methods from mitxmako.shortcuts import render_to_response from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata @@ -19,7 +20,7 @@ from ..utils import get_modulestore from django.utils.translation import ugettext as _ -__all__ = ['edit_tabs', 'reorder_static_tabs'] +__all__ = ['tabs_handler'] def initialize_course_tabs(course): @@ -43,104 +44,113 @@ def initialize_course_tabs(course): modulestore('direct').update_metadata(course.location.url(), own_metadata(course)) - -@login_required @expect_json -def reorder_static_tabs(request): - "Order the static tabs in the requested order" - def get_location_for_tab(tab): - tab_locator = BlockUsageLocator(tab) - return loc_mapper().translate_locator_to_location(tab_locator) - - tabs = request.json['tabs'] - course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(tabs[0]), get_course=True) - - if not has_access(request.user, course_location): - raise PermissionDenied() - - course = get_modulestore(course_location).get_item(course_location) - - # get list of existing static tabs in course - # make sure they are the same lengths (i.e. the number of passed in tabs equals the number - # that we know about) otherwise we can drop some! - - existing_static_tabs = [t for t in course.tabs if t['type'] == 'static_tab'] - if len(existing_static_tabs) != len(tabs): - return HttpResponseBadRequest() - - # load all reference tabs, return BadRequest if we can't find any of them - tab_items = [] - for tab in tabs: - item = modulestore('direct').get_item(get_location_for_tab(tab)) - if item is None: - return HttpResponseBadRequest() - - tab_items.append(item) - - # now just go through the existing course_tabs and re-order the static tabs - reordered_tabs = [] - static_tab_idx = 0 - for tab in course.tabs: - if tab['type'] == 'static_tab': - reordered_tabs.append({'type': 'static_tab', - 'name': tab_items[static_tab_idx].display_name, - 'url_slug': tab_items[static_tab_idx].location.name}) - static_tab_idx += 1 - else: - reordered_tabs.append(tab) - - # OK, re-assemble the static tabs in the new order - course.tabs = reordered_tabs - # Save the data that we've just changed to the underlying - # MongoKeyValueStore before we update the mongo datastore. - course.save() - modulestore('direct').update_metadata(course.location, own_metadata(course)) - # TODO: above two lines are used for the primitive-save case. Maybe factor them out? - return HttpResponse() - - @login_required @ensure_csrf_cookie -def edit_tabs(request, org, course, coursename): - "Edit tabs" - location = ['i4x', org, course, 'course', coursename] - store = get_modulestore(location) - course_item = store.get_item(location) +@require_http_methods(("GET", "POST", "PUT")) +def tabs_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): + """ + The restful handler for static tabs. - # check that logged in user has permissions to this item - if not has_access(request.user, location): + GET + html: return page for editing static tabs + json: not supported + PUT or POST + json: update the tab order. It is expected that the request body contains a JSON-encoded dict with entry "tabs". + The value for "tabs" is an array of tab locators, indicating the desired order of the tabs. + + Creating a tab, deleting a tab, or changing its contents is not supported through this method. + Instead use the general xblock URL (see item.xblock_handler). + """ + locator = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + if not has_access(request.user, locator): raise PermissionDenied() - # see tabs have been uninitialized (e.g. supporing courses created before tab support in studio) - if course_item.tabs is None or len(course_item.tabs) == 0: - initialize_course_tabs(course_item) + old_location = loc_mapper().translate_locator_to_location(locator) + store = get_modulestore(old_location) + course_item = store.get_item(old_location) - # first get all static tabs from the tabs list - # we do this because this is also the order in which items are displayed in the LMS - static_tabs_refs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + if request.method == 'GET': + raise NotImplementedError('coming soon') + else: + if 'tabs' in request.json: + def get_location_for_tab(tab): + """ Returns the location (old-style) for a tab. """ + return loc_mapper().translate_locator_to_location(BlockUsageLocator(tab)) - static_tabs = [] - for static_tab_ref in static_tabs_refs: - static_tab_loc = Location(location)._replace(category='static_tab', name=static_tab_ref['url_slug']) - static_tabs.append(modulestore('direct').get_item(static_tab_loc)) + tabs = request.json['tabs'] - components = [ - loc_mapper().translate_location( - course_item.location.course_id, static_tab.location, False, True - ) - for static_tab - in static_tabs - ] + # get list of existing static tabs in course + # make sure they are the same lengths (i.e. the number of passed in tabs equals the number + # that we know about) otherwise we will inadvertently drop some! + existing_static_tabs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + if len(existing_static_tabs) != len(tabs): + return JsonResponse( + {"error": "number of tabs must be {}".format(len(existing_static_tabs))}, status=400 + ) - course_locator = loc_mapper().translate_location( - course_item.location.course_id, course_item.location, False, True - ) + # load all reference tabs, return BadRequest if we can't find any of them + tab_items = [] + for tab in tabs: + item = modulestore('direct').get_item(get_location_for_tab(tab)) + if item is None: + return JsonResponse( + {"error": "no tab for found location {}".format(tab)}, status=400 + ) - return render_to_response('edit-tabs.html', { - 'context_course': course_item, - 'components': components, - 'locator': course_locator - }) + tab_items.append(item) + + # now just go through the existing course_tabs and re-order the static tabs + reordered_tabs = [] + static_tab_idx = 0 + for tab in course_item.tabs: + if tab['type'] == 'static_tab': + reordered_tabs.append( + {'type': 'static_tab', + 'name': tab_items[static_tab_idx].display_name, + 'url_slug': tab_items[static_tab_idx].location.name, + } + ) + static_tab_idx += 1 + else: + reordered_tabs.append(tab) + + # OK, re-assemble the static tabs in the new order + course_item.tabs = reordered_tabs + modulestore('direct').update_metadata(course_item.location, own_metadata(course_item)) + return JsonResponse() + else: + raise NotImplementedError('Creating or changing tab content is not supported.') + elif request.method == 'GET': # assume html + # see tabs have been uninitialized (e.g. supporting courses created before tab support in studio) + if course_item.tabs is None or len(course_item.tabs) == 0: + initialize_course_tabs(course_item) + + # first get all static tabs from the tabs list + # we do this because this is also the order in which items are displayed in the LMS + static_tabs_refs = [t for t in course_item.tabs if t['type'] == 'static_tab'] + + static_tabs = [] + for static_tab_ref in static_tabs_refs: + static_tab_loc = old_location.replace(category='static_tab', name=static_tab_ref['url_slug']) + static_tabs.append(modulestore('direct').get_item(static_tab_loc)) + + components = [ + loc_mapper().translate_location( + course_item.location.course_id, static_tab.location, False, True + ) + for static_tab + in static_tabs + ] + + return render_to_response('edit-tabs.html', { + 'context_course': course_item, + 'components': components, + 'course_locator': locator + }) + else: + return HttpResponseNotFound() # "primitive" tab edit functions driven by the command line. @@ -164,7 +174,7 @@ def primitive_delete(course, num): # Note for future implementations: if you delete a static_tab, then Chris Dodge # points out that there's other stuff to delete beyond this element. # This code happens to not delete static_tab so it doesn't come up. - primitive_save(course) + modulestore('direct').update_metadata(course.location, own_metadata(course)) def primitive_insert(course, num, tab_type, name): @@ -173,11 +183,5 @@ def primitive_insert(course, num, tab_type, name): new_tab = {u'type': unicode(tab_type), u'name': unicode(name)} tabs = course.tabs tabs.insert(num, new_tab) - primitive_save(course) - - -def primitive_save(course): - "Saves the course back to modulestore." - # This code copied from reorder_static_tabs above - course.save() modulestore('direct').update_metadata(course.location, own_metadata(course)) + diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 2ee8d17979..c84b60be61 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -197,7 +197,8 @@ define([ "js/spec/transcripts/videolist_spec", "js/spec/transcripts/message_manager_spec", "js/spec/transcripts/file_uploader_spec", - "js/spec/utils/module_spec" + "js/spec/utils/module_spec", + "js/spec/models/explicit_url_spec" # these tests are run separate in the cms-squire suite, due to process # isolation issues with Squire.js diff --git a/cms/static/coffee/src/views/tabs.coffee b/cms/static/coffee/src/views/tabs.coffee index a85b3b7863..83ca7dc2fe 100644 --- a/cms/static/coffee/src/views/tabs.coffee +++ b/cms/static/coffee/src/views/tabs.coffee @@ -37,14 +37,17 @@ define ["jquery", "jquery.ui", "backbone", "js/views/feedback_prompt", "js/views analytics.track "Reordered Static Pages", course: course_location_analytics + saving = new NotificationView.Mini({title: gettext("Saving…")}) + saving.show() + $.ajax({ type:'POST', - url: '/reorder_static_tabs', + url: @model.url(), data: JSON.stringify({ tabs : tabs }), contentType: 'application/json' - }) + }).success(=> saving.hide()) addNewTab: (event) => event.preventDefault() diff --git a/cms/static/js/models/explicit_url.js b/cms/static/js/models/explicit_url.js new file mode 100644 index 0000000000..aae69608af --- /dev/null +++ b/cms/static/js/models/explicit_url.js @@ -0,0 +1,14 @@ +/** + * A model that simply allows the update URL to be passed + * in as an argument. + */ +define(["backbone"], function(Backbone){ + return Backbone.Model.extend({ + defaults: { + "explicit_url": "" + }, + url: function() { + return this.get("explicit_url"); + } + }); +}); diff --git a/cms/static/js/spec/models/explicit_url_spec.js b/cms/static/js/spec/models/explicit_url_spec.js new file mode 100644 index 0000000000..df70d47b63 --- /dev/null +++ b/cms/static/js/spec/models/explicit_url_spec.js @@ -0,0 +1,12 @@ +define(['js/models/explicit_url'], + function (Model) { + describe('Model ', function () { + it('allows url to be passed in constructor', function () { + expect(new Model({'explicit_url': '/fancy/url'}).url()).toBe('/fancy/url'); + }); + it('returns empty string if url not set', function () { + expect(new Model().url()).toBe(''); + }); + }); + } +); diff --git a/cms/templates/edit-tabs.html b/cms/templates/edit-tabs.html index ca6e0259ab..70c3e61801 100644 --- a/cms/templates/edit-tabs.html +++ b/cms/templates/edit-tabs.html @@ -9,12 +9,15 @@ <%block name="jsextra"> +{% endaddtoblock %} + + {% include "wiki/includes/editor.html" %}
          - - + + {% trans "Delete article" %} - +
          - + {% include "wiki/includes/cheatsheet.html" %} {% endblock %} - - - diff --git a/lms/templates/wiki/history.html b/lms/templates/wiki/history.html index 5488abb97d..798b030c03 100644 --- a/lms/templates/wiki/history.html +++ b/lms/templates/wiki/history.html @@ -10,7 +10,7 @@ {% endaddtoblock %} @@ -81,7 +95,7 @@ {% trans "Click each revision to see a list of edited lines. Click the Preview button to see how the article looked at this stage. At the bottom of this page, you can change to a particular revision or merge an old revision with the current one." %}

          -
          +
          {% for revision in revisions %}
          @@ -107,16 +121,29 @@
          {% if not revision == article.current_revision %} - + + + {% trans "Preview this revision" %} + + + {% if article|can_write:user %} + + {% endif %} + {% endif %} - - {% if article|can_write:user %} - - {% endif %} - +
          @@ -140,84 +167,103 @@
        {% endfor %} - + {% include "wiki/includes/pagination.html" %} - + {% if revisions.count > 1 %}
        {% if article|can_write:user %} - + {% else %} {% endif %} -
        - {% endif %} - +
        -