Merge pull request #563 from edx/sanchez/fix-extra-training-alerts

Adding checks to the alerts for student training.

openassessment/xblock/static/js/fixtures/templates.json

@@ -590,6 +590,10 @@
                         ]
                     }
                 },
+                "student_training": {
+                    "start": "",
+                    "due": ""
+                },
                 "peer_assessment": {
                     "start": "2014-01-02T00:00",
                     "due": "2020-01-01T12:34",

openassessment/xblock/static/js/spec/studio/oa_edit_listeners.js

@@ -273,6 +273,48 @@ describe("OpenAssessment.StudentTrainingListener", function() {
         // The alert should be displayed
         expect(listener.alert.isVisible()).toBe(true);
     });
+
+    it("does not display an alert when student training is disabled", function() {
+        var studentTrainingView =
+            new OpenAssessment.EditStudentTrainingView($('#oa_student_training_editor'));
+        studentTrainingView.isEnabled(false);
+        // Initial state, set by the fixture
+        assertExampleLabels(
+            listener.examplesCriteriaLabels(),
+            { criterion_with_two_options: "Criterion with two options" }
+        );
+        expect(listener.alert.isVisible()).toBe(false);
+
+        // Remove the criterion
+        listener.criterionRemove({
+            criterionName: "criterion_with_two_options"
+        });
+
+        // The criterion should no longer be displayed
+        assertExampleLabels(listener.examplesCriteriaLabels(), {}, 1);
+
+        // The alert should not be displayed.
+        expect(listener.alert.isVisible()).toBe(false);
+    });
+
+    it("does not display an alert with no training examples", function() {
+        // Clear out all examples.
+        var studentTrainingView =
+            new OpenAssessment.EditStudentTrainingView($('#oa_student_training_editor'));
+        var items = studentTrainingView.exampleContainer.getAllItems();
+        $(items).each(function(){
+            studentTrainingView.exampleContainer.remove(this);
+        });
+        expect(listener.alert.isVisible()).toBe(false);
+
+        // Remove the criterion
+        listener.criterionRemove({
+            criterionName: "criterion_with_two_options"
+        });
+
+        // The alert should not be displayed.
+        expect(listener.alert.isVisible()).toBe(false);
+    });
 });
 
 

openassessment/xblock/static/js/src/studio/oa_edit_listeners.js

@@ -20,10 +20,7 @@ OpenAssessment.StudentTrainingListener.prototype = {
          points (int): The point value for the option.
      */
     optionUpdated: function(data) {
-        var view = this;
-        var sel = '.openassessment_training_example_criterion[data-criterion="' + data.criterionName + '"]';
-
-        $(sel, this.element).each(
+        this._optionSel(data.criterionName).each(
             function() {
                 var criterion = this;
                 var option = $('option[value="' + data.name + '"]', criterion)
@@ -52,34 +49,28 @@ OpenAssessment.StudentTrainingListener.prototype = {
      */
     optionAdd: function(data) {
         // First, check to see if the criterion exists on the training examples
-        var options = $('.openassessment_training_example_criterion_option[data-criterion="' + data.criterionName + '"]');
-        var view = this;
         var criterionAdded = false;
-        var examplesUpdated = false;
-        if (options.length === 0) {
+        if (this._optionSel(data.criterionName).length === 0) {
             this.criterionAdd(data);
             criterionAdded = true;
         }
 
-        $('.openassessment_training_example_criterion_option', this.element).each(function() {
-            if ($(this).data('criterion') === data.criterionName) {
-                var criterion = this;
-                // Risky; making an assumption that options will remain simple.
-                // updates could cause this to get out of sync with templates,
-                // but this avoids overly complex templating code.
-                var option = $("<option></option>")
-                    .attr("value", data.name)
-                    .data("points", data.points)
-                    .data("label", data.label);
-
-                // Sets the option's text description, and adds it to the criterion.
-                OpenAssessment.ItemUtilities.refreshOptionString(option);
-                $(criterion).append(option);
-                examplesUpdated = true;
-            }
+        this._optionSel(data.criterionName).each(function() {
+            var criterion = this;
+            // Risky; making an assumption that options will remain simple.
+            // updates could cause this to get out of sync with templates,
+            // but this avoids overly complex templating code.
+            var option = $("<option></option>")
+                .attr("value", data.name)
+                .data("points", data.points)
+                .data("label", data.label);
+
+            // Sets the option's text description, and adds it to the criterion.
+            OpenAssessment.ItemUtilities.refreshOptionString(option);
+            $(criterion).append(option);
         });
 
-        if (criterionAdded && examplesUpdated) {
+        if (criterionAdded) {
             this.displayAlertMsg(
                 gettext("Criterion Addition requires Training Example Updates"),
                 gettext("Because you added a criterion, student training examples will have to be updated.")
@@ -105,26 +96,24 @@ OpenAssessment.StudentTrainingListener.prototype = {
     optionRemove: function(data) {
         var handler = this;
         var invalidated = false;
-        $('.openassessment_training_example_criterion_option', this.element).each(function() {
+        this._optionSel(data.criterionName).each(function() {
             var criterionOption = this;
-            if ($(criterionOption).data('criterion') === data.criterionName) {
-                if ($(criterionOption).val() === data.name.toString()) {
-                    $(criterionOption).val("");
-                    $(criterionOption).addClass("openassessment_highlighted_field");
-                    $(criterionOption).click(function() {
-                        $(criterionOption).removeClass("openassessment_highlighted_field");
-                    });
-                    invalidated = true;
-                }
-
-                $('option[value="' + data.name + '"]', criterionOption).remove();
-
-                // If all options have been removed from the Criterion, remove
-                // the criterion entirely.
-                if ($("option", criterionOption).length == 1) {
-                    handler.removeAllOptions(data);
-                    invalidated = false;
-                }
+            if ($(criterionOption).val() === data.name.toString()) {
+                $(criterionOption).val("")
+                .addClass("openassessment_highlighted_field")
+                .click(function() {
+                    $(criterionOption).removeClass("openassessment_highlighted_field");
+                });
+                invalidated = true;
+            }
+
+            $('option[value="' + data.name + '"]', criterionOption).remove();
+
+            // If all options have been removed from the Criterion, remove
+            // the criterion entirely.
+            if ($("option", criterionOption).length == 1) {
+                handler.removeAllOptions(data);
+                invalidated = false;
             }
         });
 
@@ -136,6 +125,10 @@ OpenAssessment.StudentTrainingListener.prototype = {
         }
     },
 
+    _optionSel: function(criterionName) {
+        return $('.openassessment_training_example_criterion_option[data-criterion="' + criterionName + '"]', this.element);
+    },
+
     /**
      Event handler for when all options are removed from a criterion. Right now,
      the logic is the same as if the criterion was removed. When all options are
@@ -192,7 +185,8 @@ OpenAssessment.StudentTrainingListener.prototype = {
     },
 
     /**
-     Sets up the alert window based on a change message.
+     Sets up the alert window based on a change message. Checks that there is
+     at least one training example, and that student training is enabled.
 
      Args:
          title (str): Title of the alert message.
@@ -200,7 +194,11 @@ OpenAssessment.StudentTrainingListener.prototype = {
 
      */
     displayAlertMsg: function(title, msg) {
-        this.alert.setMessage(title, msg).show();
+        if ($("#include_student_training", this.element).is(":checked") &&
+                // Check for at least more than one example, to exclude the template
+                $(".openassessment_training_example", this.element).length > 1) {
+            this.alert.setMessage(title, msg).show();
+        }
     },
 
     /**