From 8acbeec3add4e65149f2ccb9dbca44bbc4e4dfc0 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 20 Feb 2013 12:23:19 -0500 Subject: [PATCH 1/2] Comment out validation, update warning message. --- cms/static/js/models/settings/advanced.js | 8 ++- cms/static/js/views/settings/advanced_view.js | 66 +++++++++++-------- cms/templates/settings_advanced.html | 2 +- 3 files changed, 48 insertions(+), 28 deletions(-) diff --git a/cms/static/js/models/settings/advanced.js b/cms/static/js/models/settings/advanced.js index 052f8c9bd6..754be11749 100644 --- a/cms/static/js/models/settings/advanced.js +++ b/cms/static/js/models/settings/advanced.js @@ -1,6 +1,9 @@ if (!CMS.Models['Settings']) CMS.Models.Settings = {}; CMS.Models.Settings.Advanced = Backbone.Model.extend({ + // the key for a newly added policy-- before the user has entered a key value + new_key : "__new_advanced_key__", + defaults: { // the properties are whatever the user types in (in addition to whatever comes originally from the server) }, @@ -11,7 +14,10 @@ CMS.Models.Settings.Advanced = Backbone.Model.extend({ validate: function (attrs) { var errors = {}; for (var key in attrs) { - if (_.contains(this.blacklistKeys, key)) { + if (key === this.new_key || _.isEmpty(key)) { + errors[key] = "A key must be entered."; + } + else if (_.contains(this.blacklistKeys, key)) { errors[key] = key + " is a reserved keyword or has another editor"; } } diff --git a/cms/static/js/views/settings/advanced_view.js b/cms/static/js/views/settings/advanced_view.js index 04dd8ad53d..2ae304e502 100644 --- a/cms/static/js/views/settings/advanced_view.js +++ b/cms/static/js/views/settings/advanced_view.js @@ -1,8 +1,6 @@ if (!CMS.Views['Settings']) CMS.Views.Settings = {}; CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ - // the key for a newly added policy-- before the user has entered a key value - new_key : "__new_advanced_key__", error_saving : "error_saving", successful_changes: "successful_changes", @@ -54,6 +52,13 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ return this; }, attachJSONEditor : function (textarea) { + // Since we are allowing duplicate keys at the moment, it is possible that we will try to attach + // JSON Editor to a value that already has one. Therefore only attach if no CodeMirror peer exists. + var siblings = $(textarea).siblings(); + if (siblings.length >= 1 && $(siblings[0]).hasClass('CodeMirror')) { + return; + } + var self = this; CodeMirror.fromTextArea(textarea, { mode: "application/json", lineNumbers: false, lineWrapping: false, @@ -79,18 +84,20 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ mirror.setValue(stringValue); } catch(quotedE) { // TODO: validation error - console.log("Error with JSON, even after converting to String.") + console.log("Error with JSON, even after converting to String."); console.log(quotedE); JSONValue = undefined; } } else { // TODO: validation error - console.log("Error with JSON, but will not convert to String.") + console.log("Error with JSON, but will not convert to String."); console.log(e); } } if (JSONValue !== undefined) { + // Is it OK to clear all validation errors? If we don't we get problems with errors overlaying. + self.clearValidationErrors(); self.model.set(key, JSONValue, {validate: true}); } } @@ -146,7 +153,7 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ var key = $('.key', li$).attr('id'); delete this.fieldToSelectorMap[key]; - if (key !== this.new_key) { + if (key !== this.model.new_key) { this.model.deleteKeys.push(key); this.model.unset(key); } @@ -182,9 +189,9 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ listEle$.append(newEle); // disable the value entry until there's an acceptable key $(newEle).find('.course-advanced-policy-value').addClass('disabled'); - this.fieldToSelectorMap[this.new_key] = this.new_key; + this.fieldToSelectorMap[this.model.new_key] = this.model.new_key; // need to re-find b/c replaceWith seems to copy rather than use the specific ele instance - var policyValueDivs = this.$el.find('#' + this.new_key).closest('li').find('.json'); + var policyValueDivs = this.$el.find('#' + this.model.new_key).closest('li').find('.json'); // only 1 but hey, let's take advantage of the context mechanism _.each(policyValueDivs, this.attachJSONEditor, this); this.toggleNewButton(false); @@ -194,26 +201,29 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ // old key: either the key as in the model or new_key. // That is, it doesn't change as the val changes until val is accepted. var oldKey = $(event.currentTarget).closest('.key').attr('id'); - var newKey = $(event.currentTarget).val(); + // TODO: validation of keys with spaces. For now at least trim strings to remove initial and + // trailing whitespace + var newKey = $.trim($(event.currentTarget).val()); if (oldKey !== newKey) { // TODO: is it OK to erase other validation messages? this.clearValidationErrors(); if (!this.validateKey(oldKey, newKey)) return; - if (this.model.has(newKey)) { - var error = {}; - error[oldKey] = 'You have already defined "' + newKey + '" in the manual policy definitions.'; - error[newKey] = "You tried to enter a duplicate of this key."; - this.model.trigger("error", this.model, error); - return false; - } +// TODO: re-enable validation +// if (this.model.has(newKey)) { +// var error = {}; +// error[oldKey] = 'You have already defined "' + newKey + '" in the manual policy definitions.'; +// error[newKey] = "You tried to enter a duplicate of this key."; +// this.model.trigger("error", this.model, error); +// return false; +// } // explicitly call validate to determine whether to proceed (relying on triggered error means putting continuation in the success // method which is uglier I think?) var newEntryModel = {}; // set the new key's value to the old one's - newEntryModel[newKey] = (oldKey === this.new_key ? '' : this.model.get(oldKey)); + newEntryModel[newKey] = (oldKey === this.model.new_key ? '' : this.model.get(oldKey)); var validation = this.model.validate(newEntryModel); if (validation) { @@ -231,7 +241,7 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ delete this.fieldToSelectorMap[oldKey]; - if (oldKey !== this.new_key) { + if (oldKey !== this.model.new_key) { // mark the old key for deletion and delete from field maps this.model.deleteKeys.push(oldKey); this.model.unset(oldKey) ; @@ -252,7 +262,9 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ $(event.currentTarget).closest('li').replaceWith(newEle); // need to re-find b/c replaceWith seems to copy rather than use the specific ele instance var policyValueDivs = this.$el.find('#' + newKey).closest('li').find('.json'); - // only 1 but hey, let's take advantage of the context mechanism + + // Because we are not dis-allowing duplicate key definitions, we may get back more than one + // div. But attachJSONEditor will only attach editor if it is not already defined. _.each(policyValueDivs, this.attachJSONEditor, this); this.fieldToSelectorMap[newKey] = newKey; @@ -260,13 +272,15 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({ }, validateKey : function(oldKey, newKey) { // model validation can't handle malformed keys nor notice if 2 fields have same key; so, need to add that chk here - // TODO ensure there's no spaces or illegal chars - if (_.isEmpty(newKey)) { - var error = {}; - error[oldKey] = "Key cannot be an empty string"; - this.model.trigger("error", this.model, error); - return false; - } - else return true; + // TODO ensure there's no spaces or illegal chars (note some checking for spaces currently done in model's + // validate method. +// if (_.isEmpty(newKey)) { +// var error = {}; +// error[oldKey] = "Key cannot be an empty string"; +// this.model.trigger("error", this.model, error); +// return false; +// } +// else return true; + return true; } }); \ No newline at end of file diff --git a/cms/templates/settings_advanced.html b/cms/templates/settings_advanced.html index 5fe1b21a9b..d98e694b8d 100644 --- a/cms/templates/settings_advanced.html +++ b/cms/templates/settings_advanced.html @@ -93,7 +93,7 @@ from contentstore import utils ⚠

Note: Your changes will not take effect until you save your - progress.

+ progress. Be careful with syntax as validation is not implemented.

From 05f32f4bc937c892b1d763a84092ab75baaccbfa Mon Sep 17 00:00:00 2001 From: Jay Zoldak Date: Wed, 20 Feb 2013 14:30:39 -0500 Subject: [PATCH 2/2] Update tests for the advanced policy settings feature. Includes adding a method for css_click_at. --- .../features/advanced-settings.feature | 22 ++++++------ .../features/advanced-settings.py | 33 ++++++++++------- .../contentstore/features/common.py | 35 +++++++++++++++++-- common/djangoapps/terrain/steps.py | 5 +++ 4 files changed, 69 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/features/advanced-settings.feature b/cms/djangoapps/contentstore/features/advanced-settings.feature index a0ad8004cc..4790c2eef7 100644 --- a/cms/djangoapps/contentstore/features/advanced-settings.feature +++ b/cms/djangoapps/contentstore/features/advanced-settings.feature @@ -7,18 +7,18 @@ Feature: Advanced (manual) course policy When I select the Advanced Settings Then I see only the display name - Scenario: A course author sees something sensible if there are no policy settings without existing UI controls + Scenario: Test if there are no policy settings without existing UI controls Given I have opened a new course in Studio When I select the Advanced Settings And I delete the display name - Then There are no advanced policy settings - And I refresh and select the Advanced Settings - Then There are no advanced policy settings + Then there are no advanced policy settings + And I reload the page + Then there are no advanced policy settings - Scenario: A course author can add new entries, and they appear alphabetically after save - Given I have opened a new course in Studio - When I select the Advanced Settings - And Create New Entries - Then They are alphabetized - And I refresh and select the Advanced Settings - Then They are alphabetized \ No newline at end of file + Scenario: Add new entries, and they appear alphabetically after save + Given I have opened a new course in Studio + When I select the Advanced Settings + And I create New Entries + Then they are alphabetized + And I reload the page + Then they are alphabetized \ No newline at end of file diff --git a/cms/djangoapps/contentstore/features/advanced-settings.py b/cms/djangoapps/contentstore/features/advanced-settings.py index 19460220d5..5356a96739 100644 --- a/cms/djangoapps/contentstore/features/advanced-settings.py +++ b/cms/djangoapps/contentstore/features/advanced-settings.py @@ -11,41 +11,43 @@ from selenium.webdriver.common.keys import Keys ############### ACTIONS #################### @step('I select the Advanced Settings$') def i_select_advanced_settings(step): - link_css = 'a#settings-tab' - css_click(link_css) - link_css = "[data-section='advanced']" + expand_icon_css = 'li.nav-course-settings i.icon-expand' + if world.browser.is_element_present_by_css(expand_icon_css): + css_click(expand_icon_css) + link_css = 'li.nav-course-settings-advanced a' css_click(link_css) -@step('I refresh and select the Advanced Settings$') -def refresh_and_select_advanced_settings(step): - reload() - i_select_advanced_settings(step) @step('I see only the display name$') def i_see_only_display_name(step): assert_policy_entries(["display_name"], ['"Robot Super Course"']) + @step('I delete the display name') def i_delete_the_display_name(step): delete_entry(0) click_save() -@step("There are no advanced policy settings$") + +@step('there are no advanced policy settings$') def no_policy_settings(step): assert_policy_entries([], []) -@step("Create New Entries") + +@step('create New Entries$') def create_new_entries(step): create_entry("z", "apple") create_entry("a", "zebra") click_save() -@step("They are alphabetized") + +@step('they are alphabetized$') def they_are_alphabetized(step): assert_policy_entries(["a", "display_name", "z"], ['"zebra"', '"Robot Super Course"', '"apple"']) + def create_entry(key, value): - css_click(".new-advanced-policy-item") + css_click_at('a.new-advanced-policy-item') newKey = css_find('#__new_advanced_key__ input').first newKey.fill(key) # For some reason have to get the instance for each command (get error that it is no longer attached to the DOM) @@ -54,6 +56,7 @@ def create_entry(key, value): css_find('.CodeMirror textarea').last._element.send_keys(Keys.ARROW_LEFT) css_find('.CodeMirror textarea').last.fill(value) + def delete_entry(index): """ index is 0-based """ @@ -63,17 +66,20 @@ def delete_entry(index): assert_true(len(delete_buttons) > index, "no delete button exists for entry " + str(index)) delete_buttons[index].click() + def assert_policy_entries(expected_keys, expected_values): assert_entries('.key input', expected_keys) assert_entries('.json', expected_values) + def assert_entries(css, expected_values): webElements = css_find(css) - assert_equal(len(expected_values),len(webElements)) + assert_equal(len(expected_values), len(webElements)) # Sometimes get stale reference if I hold on to the array of elements for counter in range(len(expected_values)): assert_equal(expected_values[counter], css_find(css)[counter].value) + def click_save(): css = ".save-button" def is_shown(driver): @@ -85,6 +91,7 @@ def click_save(): wait_for(is_shown) css_click(css) + def fill_last_field(value): newValue = css_find('#__new_advanced_key__ input').first - newValue.fill(value) \ No newline at end of file + newValue.fill(value) diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py index d0d007b231..e611d81b4f 100644 --- a/cms/djangoapps/contentstore/features/common.py +++ b/cms/djangoapps/contentstore/features/common.py @@ -10,6 +10,7 @@ from logging import getLogger logger = getLogger(__name__) ########### STEP HELPERS ############## + @step('I (?:visit|access|open) the Studio homepage$') def i_visit_the_studio_homepage(step): # To make this go to port 8001, put @@ -19,14 +20,17 @@ def i_visit_the_studio_homepage(step): signin_css = 'a.action-signin' assert world.browser.is_element_present_by_css(signin_css, 10) + @step('I am logged into Studio$') def i_am_logged_into_studio(step): log_into_studio() + @step('I confirm the alert$') def i_confirm_with_ok(step): world.browser.get_alert().accept() + @step(u'I press the "([^"]*)" delete icon$') def i_press_the_category_delete_icon(step, category): if category == 'section': @@ -37,6 +41,7 @@ def i_press_the_category_delete_icon(step, category): assert False, 'Invalid category: %s' % category css_click(css) + @step('I have opened a new course in Studio$') def i_have_opened_a_new_course(step): clear_courses() @@ -44,13 +49,14 @@ def i_have_opened_a_new_course(step): create_a_course() ####### HELPER FUNCTIONS ############## + def create_studio_user( uname='robot', email='robot+studio@edx.org', password='test', - is_staff=False): + is_staff=False): studio_user = UserFactory.build( - username=uname, + username=uname, email=email, password=password, is_staff=is_staff) @@ -63,6 +69,7 @@ def create_studio_user( user_profile = UserProfileFactory(user=studio_user) + def flush_xmodule_store(): # Flush and initialize the module store # It needs the templates because it creates new records @@ -75,32 +82,53 @@ def flush_xmodule_store(): xmodule.modulestore.django.modulestore().collection.drop() xmodule.templates.update_templates() + def assert_css_with_text(css, text): assert_true(world.browser.is_element_present_by_css(css, 5)) assert_equal(world.browser.find_by_css(css).text, text) + def css_click(css): assert_true(world.browser.is_element_present_by_css(css, 5)) world.browser.find_by_css(css).first.click() + +def css_click_at(css, x=10, y=10): + ''' + A method to click at x,y coordinates of the element + rather than in the center of the element + ''' + assert_true(world.browser.is_element_present_by_css(css, 5)) + e = world.browser.find_by_css(css).first + e.action_chains.move_to_element_with_offset(e._element, x, y) + e.action_chains.click() + e.action_chains.perform() + + def css_fill(css, value): world.browser.find_by_css(css).first.fill(value) + def css_find(css): return world.browser.find_by_css(css) + def wait_for(func): WebDriverWait(world.browser.driver, 10).until(func) + def id_find(id): return world.browser.find_by_id(id) + def reload(): return world.browser.reload() + def clear_courses(): flush_xmodule_store() + def fill_in_course_info( name='Robot Super Course', org='MITx', @@ -109,6 +137,7 @@ def fill_in_course_info( css_fill('.new-course-org', org) css_fill('.new-course-number', num) + def log_into_studio( uname='robot', email='robot+studio@edx.org', @@ -130,6 +159,7 @@ def log_into_studio( assert_true(world.browser.is_element_present_by_css('.new-course-button', 5)) + def create_a_course(): css_click('a.new-course-button') fill_in_course_info() @@ -137,6 +167,7 @@ def create_a_course(): course_title_css = 'span.course-title' assert_true(world.browser.is_element_present_by_css(course_title_css, 5)) + def add_section(name='My Section'): link_css = 'a.new-courseware-section-button' css_click(link_css) diff --git a/common/djangoapps/terrain/steps.py b/common/djangoapps/terrain/steps.py index 6b2a813d8d..a69f8dc7a2 100644 --- a/common/djangoapps/terrain/steps.py +++ b/common/djangoapps/terrain/steps.py @@ -21,6 +21,11 @@ def wait(step, seconds): time.sleep(float(seconds)) +@step('I reload the page$') +def reload_the_page(step): + world.browser.reload() + + @step('I (?:visit|access|open) the homepage$') def i_visit_the_homepage(step): world.browser.visit(django_url('/'))