Sjang92/advanced settings feedback

cms/djangoapps/contentstore/features/advanced_settings.py

@@ -82,8 +82,8 @@ def it_is_formatted(step):
 @step('I get an error on save$')
 def error_on_save(step):
     assert_regexp_matches(
-        world.css_text('#notification-error-description'),
-        "Incorrect format for field '{}'.".format(DISPLAY_NAME_KEY)
+        world.css_text('.error-item-message'),
+        "Value stored in a .* must be .*, found .*"
     )
 
 

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -458,6 +458,69 @@ def test_fetch_initial_fields(self):
         self.assertIn('showanswer', test_model, 'showanswer field ')
         self.assertIn('xqa_key', test_model, 'xqa_key field ')
 
+    def test_validate_and_update_from_json_correct_inputs(self):
+        is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json(
+            self.course,
+            {
+                "advertised_start": {"value": "start A"},
+                "days_early_for_beta": {"value": 2},
+                "advanced_modules": {"value": ['combinedopenended']},
+            },
+            user=self.user
+        )
+        self.assertTrue(is_valid)
+        self.assertTrue(len(errors) == 0)
+        self.update_check(test_model)
+
+        # fresh fetch to ensure persistence
+        fresh = modulestore().get_course(self.course.id)
+        test_model = CourseMetadata.fetch(fresh)
+        self.update_check(test_model)
+
+        # Tab gets tested in test_advanced_settings_munge_tabs
+        self.assertIn('advanced_modules', test_model, 'Missing advanced_modules')
+        self.assertEqual(test_model['advanced_modules']['value'], ['combinedopenended'], 'advanced_module is not updated')
+
+    def test_validate_and_update_from_json_wrong_inputs(self):
+        # input incorrectly formatted data
+        is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json(
+            self.course,
+            {
+                "advertised_start": {"value": 1, "display_name": "Course Advertised Start Date", },
+                "days_early_for_beta": {"value": "supposed to be an integer",
+                                        "display_name": "Days Early for Beta Users", },
+                "advanced_modules": {"value": 1, "display_name": "Advanced Module List", },
+            },
+            user=self.user
+        )
+
+        # Check valid results from validate_and_update_from_json
+        self.assertFalse(is_valid)
+        self.assertEqual(len(errors), 3)
+        self.assertFalse(test_model)
+
+        error_keys = set([error_obj['model']['display_name'] for error_obj in errors])
+        test_keys = set(['Advanced Module List', 'Course Advertised Start Date', 'Days Early for Beta Users'])
+        self.assertEqual(error_keys, test_keys)
+
+        # try fresh fetch to ensure no update happened
+        fresh = modulestore().get_course(self.course.id)
+        test_model = CourseMetadata.fetch(fresh)
+
+        self.assertNotEqual(test_model['advertised_start']['value'], 1, 'advertised_start should not be updated to a wrong value')
+        self.assertNotEqual(test_model['days_early_for_beta']['value'], "supposed to be an integer",
+                            'days_early_for beta should not be updated to a wrong value')
+
+    def test_correct_http_status(self):
+        json_data = json.dumps({
+                        "advertised_start": {"value": 1, "display_name": "Course Advertised Start Date", },
+                        "days_early_for_beta": {"value": "supposed to be an integer",
+                                        "display_name": "Days Early for Beta Users", },
+                        "advanced_modules": {"value": 1, "display_name": "Advanced Module List", },
+                    })
+        response = self.client.ajax_post(self.course_setting_url, json_data)
+        self.assertEqual(400, response.status_code)
+
     def test_update_from_json(self):
         test_model = CourseMetadata.update_from_json(
             self.course,
@@ -487,6 +550,9 @@ def test_update_from_json(self):
         self.assertEqual(test_model['advertised_start']['value'], 'start B', "advertised_start not expected value")
 
     def update_check(self, test_model):
+        """
+        checks that updates were made
+        """
         self.assertIn('display_name', test_model, 'Missing editable metadata field')
         self.assertEqual(test_model['display_name']['value'], 'Robot Super Course', "not expected value")
         self.assertIn('advertised_start', test_model, 'Missing new advertised_start metadata field')

cms/djangoapps/contentstore/views/course.py

@@ -13,7 +13,7 @@
 from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseBadRequest, HttpResponseNotFound, HttpResponse
-from util.json_request import JsonResponse
+from util.json_request import JsonResponse, JsonResponseBadRequest
 from util.date_utils import get_default_time_display
 from edxmako.shortcuts import render_to_response
 
@@ -833,18 +833,26 @@ def _config_course_advanced_components(request, course_module):
             component_types = tab_component_map.get(tab_type)
             found_ac_type = False
             for ac_type in component_types:
-                if ac_type in request.json[ADVANCED_COMPONENT_POLICY_KEY]["value"] and ac_type in ADVANCED_COMPONENT_TYPES:
-                    # Add tab to the course if needed
-                    changed, new_tabs = add_extra_panel_tab(tab_type, course_module)
-                    # If a tab has been added to the course, then send the
-                    # metadata along to CourseMetadata.update_from_json
-                    if changed:
-                        course_module.tabs = new_tabs
-                        request.json.update({'tabs': {'value': new_tabs}})
-                        # Indicate that tabs should not be filtered out of
-                        # the metadata
-                        filter_tabs = False  # Set this flag to avoid the tab removal code below.
-                    found_ac_type = True  # break
+
+                # Check if the user has incorrectly failed to put the value in an iterable.
+                new_advanced_component_list = request.json[ADVANCED_COMPONENT_POLICY_KEY]['value']
+                if hasattr(new_advanced_component_list, '__iter__'):
+                    if ac_type in new_advanced_component_list and ac_type in ADVANCED_COMPONENT_TYPES:
+
+                        # Add tab to the course if needed
+                        changed, new_tabs = add_extra_panel_tab(tab_type, course_module)
+                        # If a tab has been added to the course, then send the
+                        # metadata along to CourseMetadata.update_from_json
+                        if changed:
+                            course_module.tabs = new_tabs
+                            request.json.update({'tabs': {'value': new_tabs}})
+                            # Indicate that tabs should not be filtered out of
+                            # the metadata
+                            filter_tabs = False  # Set this flag to avoid the tab removal code below.
+                        found_ac_type = True  # break
+                else:
+                    # If not iterable, return immediately and let validation handle.
+                    return
 
             # If we did not find a module type in the advanced settings,
             # we may need to remove the tab from the course.
@@ -890,12 +898,21 @@ def advanced_settings_handler(request, course_key_string):
             try:
                 # Whether or not to filter the tabs key out of the settings metadata
                 filter_tabs = _config_course_advanced_components(request, course_module)
-                return JsonResponse(CourseMetadata.update_from_json(
+
+                # validate data formats and update
+                is_valid, errors, updated_data = CourseMetadata.validate_and_update_from_json(
                     course_module,
                     request.json,
                     filter_tabs=filter_tabs,
                     user=request.user,
-                ))
+                )
+
+                if is_valid:
+                    return JsonResponse(updated_data)
+                else:
+                    return JsonResponseBadRequest(errors)
+
+            # Handle all errors that validation doesn't catch
             except (TypeError, ValueError) as err:
                 return HttpResponseBadRequest(
                     django.utils.html.escape(err.message),

cms/djangoapps/models/settings/course_metadata.py

@@ -82,10 +82,55 @@ def update_from_json(cls, descriptor, jsondict, user, filter_tabs=True):
                 raise ValueError(_("Incorrect format for field '{name}'. {detailed_message}".format(
                     name=model['display_name'], detailed_message=err.message)))
 
+        return cls.update_from_dict(key_values, descriptor, user)
+
+    @classmethod
+    def validate_and_update_from_json(cls, descriptor, jsondict, user, filter_tabs=True):
+        """
+        Validate the values in the json dict (validated by xblock fields from_json method)
+
+        If all fields validate, go ahead and update those values in the database.
+        If not, return the error objects list.
+
+        Returns:
+            did_validate: whether values pass validation or not
+            errors: list of error objects
+            result: the updated course metadata or None if error
+        """
+
+        filtered_list = list(cls.FILTERED_LIST)
+        if not filter_tabs:
+            filtered_list.remove("tabs")
+        filtered_dict = dict((k, v) for k, v in jsondict.iteritems() if k not in filtered_list)
+        did_validate = True
+        errors = []
+        key_values = {}
+        updated_data = None
+
+        for key, model in filtered_dict.iteritems():
+            try:
+                val = model['value']
+                if hasattr(descriptor, key) and getattr(descriptor, key) != val:
+                    key_values[key] = descriptor.fields[key].from_json(val)
+            except (TypeError, ValueError) as err:
+                did_validate = False
+                errors.append({'message': err.message, 'model': model})
+
+        # If did validate, go ahead and update the metadata
+        if did_validate:
+            updated_data = cls.update_from_dict(key_values, descriptor, user)
+
+        return did_validate, errors, updated_data
+
+    @classmethod
+    def update_from_dict(cls, key_values, descriptor, user):
+        """
+        Update metadata descriptor in modulestore from key_values.
+        """
         for key, value in key_values.iteritems():
             setattr(descriptor, key, value)
 
-        if len(key_values) > 0:
+        if len(key_values):
             modulestore().update_item(descriptor, user.id)
 
         return cls.fetch(descriptor)

cms/static/coffee/spec/main.coffee

@@ -238,6 +238,7 @@ define([
 
     "js/spec/views/modals/base_modal_spec",
     "js/spec/views/modals/edit_xblock_spec",
+    "js/spec/views/modals/validation_error_modal_spec",
 
     "js/spec/xblock/cms.runtime.v1_spec",
 

cms/static/js/spec/views/modals/validation_error_modal_spec.js

@@ -0,0 +1,86 @@
+define(['jquery', 'underscore', 'js/spec_helpers/validation_helpers', 'js/views/modals/validation_error_modal'],
+    function ($, _, validation_helpers, ValidationErrorModal) {
+
+        describe('ValidationErrorModal', function() {
+            var modal, showModal;
+
+            showModal = function (jsonContent, callback) {
+                modal = new ValidationErrorModal();
+                modal.setResetCallback(callback);
+                modal.setContent(jsonContent);
+                modal.show();
+            };
+
+            /* Before each, install templates required for the base modal
+               and validation error modal. */
+            beforeEach(function () {
+                validation_helpers.installValidationTemplates();
+            });
+
+            afterEach(function() {
+                validation_helpers.hideModalIfShowing(modal);
+            });
+
+            it('is visible after show is called', function () {
+                showModal([]);
+                expect(validation_helpers.isShowingModal(modal)).toBeTruthy();
+            });
+
+            it('displays none if no error given', function () {
+                var errorObjects = [];
+
+                showModal(errorObjects);
+                expect(validation_helpers.isShowingModal(modal)).toBeTruthy();
+                validation_helpers.checkErrorContents(modal, errorObjects);
+            });
+
+            it('correctly displays json error message objects', function () {
+                var errorObjects = [
+                    {
+                        model: {display_name: 'test_attribute1'},
+                        message: 'Encountered an error while saving test_attribute1'
+                    },
+                    {
+                        model: {display_name: 'test_attribute2'},
+                        message: 'Encountered an error while saving test_attribute2'
+                    }
+                ];
+
+                showModal(errorObjects);
+                expect(validation_helpers.isShowingModal(modal)).toBeTruthy();
+                validation_helpers.checkErrorContents(modal, errorObjects);
+            });
+
+            it('run callback when undo changes button is clicked', function () {
+                var errorObjects = [
+                    {
+                        model: {display_name: 'test_attribute1'},
+                        message: 'Encountered an error while saving test_attribute1'
+                    },
+                    {
+                        model: {display_name: 'test_attribute2'},
+                        message: 'Encountered an error while saving test_attribute2'
+                    }
+                ];
+
+                var callback = function() {
+                    return true;
+                };
+
+                // Show Modal and click undo changes
+                showModal(errorObjects, callback);
+                expect(validation_helpers.isShowingModal(modal)).toBeTruthy();
+                validation_helpers.undoChanges(modal);
+
+                // Wait for the callback to be fired
+                waitsFor(function () {
+                    return callback();
+                }, 'the callback to be called', 5000);
+
+                // After checking callback fire, check modal hide
+                runs(function () {
+                    expect(validation_helpers.isShowingModal(modal)).toBe(false);
+                });
+            });
+        });
+    });

cms/static/js/spec_helpers/validation_helpers.js

@@ -0,0 +1,34 @@
+/**
+ * Provides helper methods for invoking Validation modal in Jasmine tests.
+ */
+define(['jquery', 'js/spec_helpers/modal_helpers', 'js/spec_helpers/view_helpers'],
+    function($, modal_helpers, view_helpers) {
+        var installValidationTemplates, checkErrorContents, undoChanges;
+
+        installValidationTemplates = function () {
+            modal_helpers.installModalTemplates();
+            view_helpers.installTemplate('validation-error-modal');
+        };
+
+        checkErrorContents = function(validationModal, errorObjects) {
+            var errorItems = validationModal.$('.error-item-message');
+            var i, item;
+            var num_items = errorItems.length;
+            expect(num_items).toBe(errorObjects.length);
+
+            for (i = 0; i < num_items; i++) {
+                item = errorItems[i];
+                expect(item.value).toBe(errorObjects[i].message);
+            }
+        };
+
+        undoChanges = function(validationModal) {
+            modal_helpers.pressModalButton('.action-undo', validationModal);
+        };
+
+        return $.extend(modal_helpers, {
+            'installValidationTemplates': installValidationTemplates,
+            'checkErrorContents': checkErrorContents,
+            'undoChanges': undoChanges,
+        });
+    });