Merge pull request #1172 from MITx/bug/dhm/dec12

Gives warning on type name change

cms/djangoapps/contentstore/course_info_model.py

@@ -4,6 +4,7 @@ from xmodule.modulestore.django import modulestore
 from lxml import etree
 import re
 from django.http import HttpResponseBadRequest
+import logging
 
 ## TODO store as array of { date, content } and override  course_info_module.definition_from_xml
 ## This should be in a class which inherits from XmlDescriptor
@@ -64,10 +65,9 @@ def update_course_updates(location, update, passed_id=None):
     except etree.XMLSyntaxError:
         course_html_parsed = etree.fromstring("<ol></ol>")
 
-    try:
-        new_html_parsed = etree.fromstring(update['content'], etree.XMLParser(remove_blank_text=True))
-    except etree.XMLSyntaxError:
-        new_html_parsed = None
+    # No try/catch b/c failure generates an error back to client
+    new_html_parsed = etree.fromstring('<li><h2>' + update['date'] + '</h2>' + update['content'] + '</li>', 
+                                       etree.XMLParser(remove_blank_text=True))
         
     # Confirm that root is <ol>, iterate over <li>, pull out <h2> subs and then rest of val
     if course_html_parsed.tag == 'ol':
@@ -75,29 +75,9 @@ def update_course_updates(location, update, passed_id=None):
         if passed_id:
             idx = get_idx(passed_id)
             # idx is count from end of list
-            element = course_html_parsed[-idx]
-            element[0].text = update['date']
-            if (len(element) == 1):
-                if new_html_parsed is not None:
-                    element[0].tail = None
-                    element.append(new_html_parsed)
-                else:
-                    element[0].tail = update['content']
-            else:
-                if new_html_parsed is not None:
-                    element[1] = new_html_parsed
-                else:
-                    element.pop(1)
-                    element[0].tail = update['content']
+            course_html_parsed[-idx] = new_html_parsed
         else:
-            element =  etree.Element("li")
-            course_html_parsed.insert(0, element)
-            date_element = etree.SubElement(element, "h2")
-            date_element.text = update['date']
-            if new_html_parsed is not None:
-                element.append(new_html_parsed)
-            else:
-                date_element.tail = update['content']
+            course_html_parsed.insert(0, new_html_parsed)
 
             idx = len(course_html_parsed)
             passed_id = course_updates.location.url() + "/" + str(idx)

cms/djangoapps/contentstore/views.py

@@ -54,6 +54,7 @@ from cms.djangoapps.models.settings.course_details import CourseDetails,\
     CourseSettingsEncoder
 from cms.djangoapps.models.settings.course_grading import CourseGradingModel
 from cms.djangoapps.contentstore.utils import get_modulestore
+from lxml import etree
 
 # to install PIL on MacOSX: 'easy_install http://dist.repoze.org/PIL-1.1.6.tar.gz'
 
@@ -979,13 +980,13 @@ def course_info_updates(request, org, course, provided_id=None):
         
     if request.method == 'GET':
         return HttpResponse(json.dumps(get_course_updates(location)), mimetype="application/json")
-    elif real_method == 'POST':
-        # new instance (unless django makes PUT a POST): updates are coming as POST. Not sure why.
-        return HttpResponse(json.dumps(update_course_updates(location, request.POST, provided_id)), mimetype="application/json")
-    elif real_method == 'PUT':
-        return HttpResponse(json.dumps(update_course_updates(location, request.POST, provided_id)), mimetype="application/json")
     elif real_method == 'DELETE':  # coming as POST need to pull from Request Header X-HTTP-Method-Override    DELETE
         return HttpResponse(json.dumps(delete_course_update(location, request.POST, provided_id)), mimetype="application/json")
+    elif request.method == 'POST':
+        try:
+            return HttpResponse(json.dumps(update_course_updates(location, request.POST, provided_id)), mimetype="application/json")
+        except etree.XMLSyntaxError:
+            return HttpResponse("Failed to save: malformed html", status=515, content_type="text/plain")
 
 
 @expect_json

cms/djangoapps/models/settings/course_details.py

@@ -8,6 +8,9 @@ from contentstore.utils import get_modulestore
 from util.converters import jsdate_to_time, time_to_date
 from cms.djangoapps.models.settings import course_grading
 from cms.djangoapps.contentstore.utils import update_item
+import re
+import logging
+
 
 class CourseDetails:
     def __init__(self, location):
@@ -58,7 +61,8 @@ class CourseDetails:
         
         temploc = temploc._replace(name='video')
         try:
-            course.intro_video = get_modulestore(temploc).get_item(temploc).definition['data']
+            raw_video = get_modulestore(temploc).get_item(temploc).definition['data']
+            course.intro_video = CourseDetails.parse_video_tag(raw_video) 
         except ItemNotFoundError:
             pass
         
@@ -127,12 +131,43 @@ class CourseDetails:
         update_item(temploc, jsondict['effort'])
         
         temploc = temploc._replace(name='video')
-        update_item(temploc, jsondict['intro_video'])
+        recomposed_video_tag = CourseDetails.recompose_video_tag(jsondict['intro_video'])
+        update_item(temploc, recomposed_video_tag)
         
                     
         # Could just generate and return a course obj w/o doing any db reads, but I put the reads in as a means to confirm
         # it persisted correctly
         return CourseDetails.fetch(course_location)
+    
+    @staticmethod
+    def parse_video_tag(raw_video):
+        """
+        Because the client really only wants the author to specify the youtube key, that's all we send to and get from the client.
+        The problem is that the db stores the html markup as well (which, of course, makes any sitewide changes to how we do videos
+        next to impossible.)
+        """
+        if not raw_video:
+            return None
+         
+        keystring_matcher = re.search('(?<=embed/)[a-zA-Z0-9_-]+', raw_video)
+        if keystring_matcher is None:
+            keystring_matcher = re.search('<?=\d+:[a-zA-Z0-9_-]+', raw_video)
+        
+        if keystring_matcher:
+            return keystring_matcher.group(0)
+        else:
+            logging.warn("ignoring the content because it doesn't not conform to expected pattern: " + raw_video)
+            return None
+    
+    @staticmethod
+    def recompose_video_tag(video_key):
+        # TODO should this use a mako template? Of course, my hope is that this is a short-term workaround for the db not storing
+        # the right thing
+        result = '<iframe width="560" height="315" src="http://www.youtube.com/embed/' + \
+            video_key + '?autoplay=1&rel=0" frameborder="0" allowfullscreen=""></iframe>'
+        return result
+
+    
 
 # TODO move to a more general util? Is there a better way to do the isinstance model check?
 class CourseSettingsEncoder(json.JSONEncoder):

cms/static/js/models/settings/course_details.js

@@ -49,20 +49,11 @@ CMS.Models.Settings.CourseDetails = Backbone.Model.extend({
 		if (newattrs.end_date && newattrs.enrollment_end && newattrs.end_date < newattrs.enrollment_end) {
 			errors.enrollment_end = "The enrollment end date cannot be after the course end date.";
 		}
-		if (newattrs.intro_video && newattrs.intro_video != this.get('intro_video')) {
-			var videos = this.parse_videosource(newattrs.intro_video);
-			var vid_errors = new Array();
-			var cachethis = this;
-			for (var i=0; i<videos.length; i++) {
-				// doesn't call parseFloat or Number b/c they stop on first non parsable and return what they have
-				if (!isFinite(videos[i].speed)) vid_errors.push(videos[i].speed + " is not a valid speed.");
-				else if (!videos[i].key) vid_errors.push(videos[i].speed + " does not have a video id");
-				// can't use get from client to test if video exists b/c of CORS (crossbrowser get not allowed)
-				// GET "http://gdata.youtube.com/feeds/api/videos/" + videokey
-			}
-			if (!_.isEmpty(vid_errors)) {
-				errors.intro_video = vid_errors.join(' ');
+		if (newattrs.intro_video && newattrs.intro_video !== this.get('intro_video')) {
+			if (this._videokey_illegal_chars.exec(newattrs.intro_video)) {
+				errors.intro_video = "Key should only contain letters, numbers, _, or -";
 			}
+			// TODO check if key points to a real video using google's youtube api
 		}
 		if (!_.isEmpty(errors)) return errors;
 		// NOTE don't return empty errors as that will be interpreted as an error state
@@ -73,98 +64,20 @@ CMS.Models.Settings.CourseDetails = Backbone.Model.extend({
 		return '/' + location.get('org') + "/" + location.get('course') + '/settings/' + location.get('name') + '/section/details';
 	},
 	
-	_videoprefix : /\s*<video\s*youtube="/g,
-	// the below is lax to enable validation
-	_videospeedparse : /[^:]*/g, // /\d+\.?\d*(?=:)/g,
-	_videokeyparse : /([^,\/>]+)/g,
-	_videonosuffix : /[^"\/>]+/g,
-	_getNextMatch : function (regex, string, cursor) {
-		regex.lastIndex = cursor;
-		var result = regex.exec(string);
-		if (_.isArray(result)) return result[0];
-		else return result;
-	},
-	// the whole string for editing (put in edit box)
-	getVideoSource: function() {
-		if (this.get('intro_video')) {
-			var cursor = 0;
-			var videostring = this.get('intro_video');
-			this._getNextMatch(this._videoprefix, videostring, cursor);
-			cursor = this._videoprefix.lastIndex;
-			return this._getNextMatch(this._videonosuffix, videostring, cursor);
-		}
-		else return "";
-	},
-	// the source closest to 1.0 speed
-	videosourceSample: function() {
-		if (this.get('intro_video')) {
-			var cursor = 0;
-			var videostring = this.get('intro_video');
-			this._getNextMatch(this._videoprefix, videostring, cursor);
-			cursor = this._videoprefix.lastIndex;
-			
-			// parse from [speed:id,/s?]* to find 1.0 or take first
-			var parsedspeed = this._getNextMatch(this._videospeedparse, videostring, cursor);
-			var bestkey;
-			if (parsedspeed) {
-				cursor = this._videospeedparse.lastIndex + 1;
-				var bestspeed = Number(parsedspeed);
-				bestkey = this._getNextMatch(this._videokeyparse, videostring, cursor);
-				cursor = this._videokeyparse.lastIndex + 1;
-				while (cursor < videostring.length && bestspeed != 1.0) {
-					parsedspeed = this._getNextMatch(this._videospeedparse, videostring, cursor);
-					if (parsedspeed) cursor = this._videospeedparse.lastIndex + 1;
-					else break;
-					if (Math.abs(Number(parsedspeed) - 1.0) < Math.abs(bestspeed - 1.0)) {
-						bestspeed = Number(parsedspeed);
-						bestkey = this._getNextMatch(this._videokeyparse, videostring, cursor);
-					}
-					else this._getNextMatch(this._videokeyparse, videostring, cursor);
-					if (this._videokeyparse.lastIndex > cursor)	cursor = this._videokeyparse.lastIndex + 1;
-					else cursor++;
-				}
-			}
-			else {
-				bestkey = this._getNextMatch(this._videokeyparse, videostring, cursor);
-			}
-			if (bestkey) {
-				// WTF? for some reason bestkey is an array [key, key] (same one repeated)
-				if (_.isArray(bestkey)) bestkey = bestkey[0];
-				return "http://www.youtube.com/embed/" + bestkey;
-			}
-			else return "";
-		}
-	},
-	parse_videosource: function(videostring) {
-		// used to validate before set so cannot get from model attr. Returns [{ speed: fff, key: sss }]
-		var cursor = 0;
-		this._getNextMatch(this._videoprefix, videostring, cursor);
-		cursor = this._videoprefix.lastIndex;
-		videostring = this._getNextMatch(this._videonosuffix, videostring, cursor);
-		cursor = 0;
-		// parsed to "fff:kkk,fff:kkk"
-		var result = new Array();
-		if (!videostring || videostring.length == 0) return result;
-		while (cursor < videostring.length) {
-			var speed = this._getNextMatch(this._videospeedparse, videostring, cursor);
-			if (speed) cursor = this._videospeedparse.lastIndex + 1;
-			else return result;
-			var key = this._getNextMatch(this._videokeyparse, videostring, cursor);
-			if (key) cursor = this._videokeyparse.lastIndex + 1;
-			// See the WTF above
-			if (_.isArray(key)) key = key[0];
-			result.push({speed: speed, key: key});
-		}
-		return result;
-	},
+	_videokey_illegal_chars : /[^a-zA-Z0-9_-]/g,
 	save_videosource: function(newsource) {
 		// newsource either is <video youtube="speed:key, *"/> or just the "speed:key, *" string
 		// returns the videosource for the preview which iss the key whose speed is closest to 1
-		if (newsource == null) this.save({'intro_video': null}); 
+		if (_.isEmpty(newsource) && !_.isEmpty(this.get('intro_video'))) this.save({'intro_video': null}); 
 		// TODO remove all whitespace w/in string
-		else if (this._getNextMatch(this._videoprefix, newsource, 0)) this.save('intro_video', newsource);
-		else this.save('intro_video', '<video youtube="' + newsource + '"/>');
+		else {
+			if (this.get('intro_video') !== newsource) this.save('intro_video', newsource);
+		}
 		
 		return this.videosourceSample();
+	},
+	videosourceSample : function() {
+		 if (this.has('intro_video')) return "http://www.youtube.com/embed/" + this.get('intro_video');
+		 else return "";
 	}
 });

cms/static/js/views/course_info_edit.js

@@ -99,7 +99,10 @@ CMS.Views.ClassInfoUpdateView = Backbone.View.extend({
         var targetModel = this.eventModel(event);
         targetModel.set({ date : this.dateEntry(event).val(), content : this.$codeMirror.getValue() });
         // push change to display, hide the editor, submit the change        
-        targetModel.save();
+        targetModel.save({}, {error : function(model, xhr) {
+        	// TODO use a standard component
+        	window.alert(xhr.responseText);
+        }});
         this.closeEditor(this);
     },
     

cms/static/js/views/settings/main_settings_view.js

@@ -46,6 +46,19 @@ CMS.Views.ValidatingView = Backbone.View.extend({
 			else $(ele).removeClass('error');
 			$(ele).nextAll('.message-error').remove();
 		}
+	},
+	
+	saveIfChanged : function(event) {
+		// returns true if the value changed and was thus sent to server
+		var field = this.selectorToField[event.currentTarget.id];
+		var currentVal = this.model.get(field);
+		var newVal = $(event.currentTarget).val();
+		if (currentVal != newVal) {
+			this.clearValidationErrors();
+			this.model.save(field, newVal);
+			return true;
+		}
+		else return false;
 	}
 });
 
@@ -170,38 +183,38 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
 			this.$el.find('.remove-course-syllabus').show();
 		}
 		else {
-			this.$el.find(this.fieldToSelectorMap['syllabus']).html("");
+			this.$el.find('#' + this.fieldToSelectorMap['syllabus']).html("");
 			this.$el.find('.remove-course-syllabus').hide();
 		}
 		
-		this.$el.find(this.fieldToSelectorMap['overview']).val(this.model.get('overview'));
+		this.$el.find('#' + this.fieldToSelectorMap['overview']).val(this.model.get('overview'));
 		this.codeMirrorize(null, $('#course-overview')[0]);
 		
 		this.$el.find('.current-course-introduction-video iframe').attr('src', this.model.videosourceSample());
 		if (this.model.has('intro_video')) {
 			this.$el.find('.remove-course-introduction-video').show();
-			this.$el.find(this.fieldToSelectorMap['intro_video']).val(this.model.getVideoSource());
+			this.$el.find('#' + this.fieldToSelectorMap['intro_video']).val(this.model.get('intro_video'));
 		}
 		else this.$el.find('.remove-course-introduction-video').hide();
 		
-		this.$el.find(this.fieldToSelectorMap['effort']).val(this.model.get('effort'));
+		this.$el.find('#' + this.fieldToSelectorMap['effort']).val(this.model.get('effort'));
 		
 		return this;
 	},
 	fieldToSelectorMap : {
-		'start_date' : "#course-start",
-		'end_date' : '#course-end',
-		'enrollment_start' : '#enrollment-start',
-		'enrollment_end' : '#enrollment-end',
+		'start_date' : "course-start",
+		'end_date' : 'course-end',
+		'enrollment_start' : 'enrollment-start',
+		'enrollment_end' : 'enrollment-end',
 		'syllabus' : '.current-course-syllabus .doc-filename',
-		'overview' : '#course-overview',
-		'intro_video' : '#course-introduction-video',
-		'effort' : "#course-effort"
+		'overview' : 'course-overview',
+		'intro_video' : 'course-introduction-video',
+		'effort' : "course-effort"
 	},
 	
 	setupDatePicker : function(fieldName) {
 		var cacheModel = this.model;
-		var div = this.$el.find(this.fieldToSelectorMap[fieldName]);
+		var div = this.$el.find('#' + this.fieldToSelectorMap[fieldName]);
 		var datefield = $(div).find(".date");
 		var timefield = $(div).find(".time");
 		var cachethis = this;
@@ -213,7 +226,8 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
                 if (!time) {
                     time = 0;
                 }
-                cacheModel.save(fieldName, new Date(date.getTime() + time * 1000));
+                var newVal = new Date(date.getTime() + time * 1000);
+                if (cacheModel.get(fieldName) != newVal) cacheModel.save(fieldName, newVal);
             }
 		};
 		
@@ -237,18 +251,22 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
 			break;
 
 		case 'course-overview':
-			this.clearValidationErrors();
-			this.model.save('overview', $(event.currentTarget).val());
+			// handled via code mirror
 			break;
 
 		case 'course-effort':
-			this.clearValidationErrors();
-			this.model.save('effort', $(event.currentTarget).val());
+			this.saveIfChanged(event);
 			break;
 		case 'course-introduction-video':
 			this.clearValidationErrors();
 			var previewsource = this.model.save_videosource($(event.currentTarget).val());
 			this.$el.find(".current-course-introduction-video iframe").attr("src", previewsource);
+			if (this.model.has('intro_video')) {
+				this.$el.find('.remove-course-introduction-video').show();
+			}
+			else {
+				this.$el.find('.remove-course-introduction-video').hide();
+			}
 			break;
 			
 		default:
@@ -269,7 +287,8 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
 		if (this.model.has('intro_video')) {
 			this.model.save_videosource(null);
 			this.$el.find(".current-course-introduction-video iframe").attr("src", "");
-			this.$el.find(this.fieldToSelectorMap['intro_video']).val("");
+			this.$el.find('#' + this.fieldToSelectorMap['intro_video']).val("");
+			this.$el.find('.remove-course-introduction-video').hide();
 		}
 	},
 	codeMirrors : {},
@@ -283,13 +302,14 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
 
 		if (!this.codeMirrors[thisTarget.id]) {
 			var cachethis = this;
-			var field = this.selectorToField['#' + thisTarget.id];
+			var field = this.selectorToField[thisTarget.id];
 			this.codeMirrors[thisTarget.id] = CodeMirror.fromTextArea(thisTarget, {
 				mode: "text/html", lineNumbers: true, lineWrapping: true,
 				onBlur : function(mirror) {
 					mirror.save();
 					cachethis.clearValidationErrors();
-					cachethis.model.save(field, mirror.getValue());
+					var newVal = mirror.getValue();
+					if (cachethis.model.get(field) != newVal) cachethis.model.save(field, newVal);
 				}
 			});
 		}
@@ -340,6 +360,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         );
 		this.model.on('error', this.handleValidationError, this);
 		this.model.get('graders').on('remove', this.render, this);
+		this.model.get('graders').on('reset', this.render, this);
 		this.model.get('graders').on('add', this.render, this);
 		this.selectorToField = _.invert(this.fieldToSelectorMap);
 	},
@@ -353,10 +374,12 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
 		var gradelist = this.$el.find('.course-grading-assignment-list');
 		// Undo the double invocation error. At some point, fix the double invocation
 		$(gradelist).empty();
-		this.model.get('graders').each(function(gradeModel) {
+		var gradeCollection = this.model.get('graders');
+		gradeCollection.each(function(gradeModel) {
 			$(gradelist).append(self.template({model : gradeModel }));
 			var newEle = gradelist.children().last();
-			var newView = new CMS.Views.Settings.GraderView({el: newEle, model : gradeModel});
+			var newView = new CMS.Views.Settings.GraderView({el: newEle, 
+				model : gradeModel, collection : gradeCollection });
 		});
 		
 		// render the grade cutoffs
@@ -381,12 +404,12 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
 		switch (this.selectorToField[event.currentTarget.id]) {
 		case 'grace_period':
 			this.clearValidationErrors();
-			this.model.save('grace_period',	this.model.dateToGracePeriod($(event.currentTarget).timepicker('getTime')));
+			var newVal = this.model.dateToGracePeriod($(event.currentTarget).timepicker('getTime'));
+			if (this.model.get('grace_period') != newVal) this.model.save('grace_period', newVal);
 			break;
 
 		default:
-			this.clearValidationErrors();
-			this.model.save(this.selectorToField[event.currentTarget.id], $(event.currentTarget).val());
+			this.saveIfChanged(event);
 			break;
 		}
 	},
@@ -615,15 +638,31 @@ CMS.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
 		'weight' : 'course-grading-assignment-gradeweight'
 	},
 	updateModel : function(event) {
+		// HACK to fix model sometimes losing its pointer to the collection [I think I fixed this but leaving
+		// this in out of paranoia. If this error ever happens, the user will get a warning that they cannot
+		// give 2 assignments the same name.]
+		if (!this.model.collection) {
+			this.model.collection = this.collection;
+		}
+		
 		switch (event.currentTarget.id) {
 		case 'course-grading-assignment-totalassignments':
 			this.$el.find('#course-grading-assignment-droppable').attr('max', $(event.currentTarget).val());
-			// no break b/c want to use the default save
+			this.saveIfChanged(event);
+			break;
+		case 'course-grading-assignment-name':
+			var oldName = this.model.get('type');
+			if (this.saveIfChanged(event) && !_.isEmpty(oldName)) {
+				// overload the error display logic
+				this._cacheValidationErrors.push(event.currentTarget);
+				$(event.currentTarget).parent().append(
+						this.errorTemplate({message : 'For grading to work, you must change all "' + oldName +
+							'" subsections to "' + this.model.get('type') + '".'}));
+			};
+			break;
 		default:
-			this.clearValidationErrors();
-			this.model.save(this.selectorToField[event.currentTarget.id], $(event.currentTarget).val());
+			this.saveIfChanged(event);
 			break;
-
 		}
 	},
 	deleteModel : function(e) {

cms/templates/settings.html

@@ -230,7 +230,7 @@ from contentstore import utils
                   </div>
 
                   <div class="input">
-                    <input type="text" class="long new-course-introduction-video add-video-data" id="course-introduction-video" value="" placeholder="speed:id,speed:id" autocomplete="off">
+                    <input type="text" class="long new-course-introduction-video add-video-data" id="course-introduction-video" value="" placeholder="id" autocomplete="off">
                     <span class="tip tip-stacked">Video restrictions go here</span>
                   </div>
                 </div>

lms/templates/staff_problem_info.html

@@ -61,16 +61,18 @@ category = ${category | h}
 
 <script type="text/javascript">
 // assumes courseware.html's loaded this method.
-setup_debug('${element_id}', 
-	%if edit_link:
-		'${edit_link}',
-	%else:
-		null,
-	%endif
-	{
-        'location': '${location}',
-        'xqa_key': '${xqa_key}',
-        'category': '${category}',
-        'user': '${user}'
-     });
+ % if staff_access:
+    setup_debug('${element_id}', 
+    	%if edit_link:
+    		'${edit_link}',
+    	%else:
+       		null,
+    	%endif
+    	{
+           'location': '${location}',
+           'xqa_key': '${xqa_key}',
+          'category': '${category}',
+          'user': '${user}'
+       });
+ % endif
 </script>