diff --git a/.pylintrc b/.pylintrc index 6690bb7df0..a9f19ca667 100644 --- a/.pylintrc +++ b/.pylintrc @@ -41,7 +41,8 @@ disable= # R0902: Too many instance attributes # R0903: Too few public methods (1/2) # R0904: Too many public methods - W0141,W0142,R0201,R0901,R0902,R0903,R0904 +# R0913: Too many arguments + W0141,W0142,R0201,R0901,R0902,R0903,R0904,R0913 [REPORTS] diff --git a/cms/static/js/views/course_info_edit.js b/cms/static/js/views/course_info_edit.js index 8382fb15eb..ce959fd443 100644 --- a/cms/static/js/views/course_info_edit.js +++ b/cms/static/js/views/course_info_edit.js @@ -142,8 +142,11 @@ CMS.Views.ClassInfoUpdateView = Backbone.View.extend({ onDelete: function(event) { event.preventDefault(); - // TODO ask for confirmation - // remove the dom element and delete the model + + if (!confirm('Are you sure you want to delete this update? This action cannot be undone.')) { + return; + } + var targetModel = this.eventModel(event); this.modelDom(event).remove(); var cacheThis = this; diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index c362ed4e89..7924012bfe 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/common/djangoapps/course_groups/cohorts.py @@ -15,6 +15,24 @@ from .models import CourseUserGroup log = logging.getLogger(__name__) +# tl;dr: global state is bad. capa reseeds random every time a problem is loaded. Even +# if and when that's fixed, it's a good idea to have a local generator to avoid any other +# code that messes with the global random module. +_local_random = None + +def local_random(): + """ + Get the local random number generator. In a function so that we don't run + random.Random() at import time. + """ + # ironic, isn't it? + global _local_random + + if _local_random is None: + _local_random = random.Random() + + return _local_random + def is_course_cohorted(course_id): """ Given a course id, return a boolean for whether or not the course is @@ -129,13 +147,7 @@ def get_cohort(user, course_id): return None # Put user in a random group, creating it if needed - choice = random.randrange(0, n) - group_name = choices[choice] - - # Victor: we are seeing very strange behavior on prod, where almost all users - # end up in the same group. Log at INFO to try to figure out what's going on. - log.info("DEBUG: adding user {0} to cohort {1}. choice={2}".format( - user, group_name,choice)) + group_name = local_random().choice(choices) group, created = CourseUserGroup.objects.get_or_create( course_id=course_id, diff --git a/common/djangoapps/terrain/steps.py b/common/djangoapps/terrain/steps.py index 52eeb23c4a..371496f823 100644 --- a/common/djangoapps/terrain/steps.py +++ b/common/djangoapps/terrain/steps.py @@ -9,6 +9,7 @@ from bs4 import BeautifulSoup import time import re import os.path +from selenium.common.exceptions import WebDriverException from logging import getLogger logger = getLogger(__name__) @@ -214,3 +215,15 @@ def save_the_course_content(path='/tmp'): f = open('%s/%s' % (path, filename), 'w') f.write(output) f.close + +@world.absorb +def css_click(css_selector): + try: + world.browser.find_by_css(css_selector).click() + + except WebDriverException: + # Occassionally, MathJax or other JavaScript can cover up + # an element temporarily. + # If this happens, wait a second, then try again + time.sleep(1) + world.browser.find_by_css(css_selector).click() diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 1bf4763723..f6fa98fc28 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -8,7 +8,7 @@ from collections import namedtuple from fs.osfs import OSFS from itertools import repeat from path import path -from datetime import datetime, timedelta +from datetime import datetime from importlib import import_module from xmodule.errortracker import null_error_tracker, exc_info_to_str @@ -246,6 +246,7 @@ class MongoModuleStore(ModuleStoreBase): self.fs_root = path(fs_root) self.error_tracker = error_tracker self.render_template = render_template + self.ignore_write_events_on_courses = [] def get_metadata_inheritance_tree(self, location): ''' @@ -330,6 +331,11 @@ class MongoModuleStore(ModuleStoreBase): return tree + def refresh_cached_metadata_inheritance_tree(self, location): + pseudo_course_id = '/'.join([location.org, location.course]) + if pseudo_course_id not in self.ignore_write_events_on_courses: + self.get_cached_metadata_inheritance_tree(location, force_refresh = True) + def clear_cached_metadata_inheritance_tree(self, location): key_name = '{0}/{1}'.format(location.org, location.course) if self.metadata_inheritance_cache is not None: @@ -376,7 +382,7 @@ class MongoModuleStore(ModuleStoreBase): return data - def _load_item(self, item, data_cache): + def _load_item(self, item, data_cache, should_apply_metadata_inheritence=True): """ Load an XModuleDescriptor from item, using the children stored in data_cache """ @@ -388,7 +394,10 @@ class MongoModuleStore(ModuleStoreBase): resource_fs = OSFS(root) - metadata_inheritance_tree = self.get_cached_metadata_inheritance_tree(Location(item['location'])) + metadata_inheritance_tree = None + + if should_apply_metadata_inheritence: + metadata_inheritance_tree = self.get_cached_metadata_inheritance_tree(Location(item['location'])) # TODO (cdodge): When the 'split module store' work has been completed, we should remove # the 'metadata_inheritance_tree' parameter @@ -410,7 +419,10 @@ class MongoModuleStore(ModuleStoreBase): """ data_cache = self._cache_children(items, depth) - return [self._load_item(item, data_cache) for item in items] + # if we are loading a course object, if we're not prefetching children (depth != 0) then don't + # bother with the metadata inheritence + return [self._load_item(item, data_cache, + should_apply_metadata_inheritence=(item['location']['category'] != 'course' or depth != 0)) for item in items] def get_courses(self): ''' @@ -493,10 +505,12 @@ class MongoModuleStore(ModuleStoreBase): try: source_item = self.collection.find_one(location_to_query(source)) source_item['_id'] = Location(location).dict() - self.collection.insert(source_item, + self.collection.insert( + source_item, # Must include this to avoid the django debug toolbar (which defines the deprecated "safe=False") # from overriding our default value set in the init method. - safe=self.collection.safe) + safe=self.collection.safe + ) item = self._load_items([source_item])[0] # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so @@ -518,7 +532,7 @@ class MongoModuleStore(ModuleStoreBase): raise DuplicateItemError(location) # recompute (and update) the metadata inheritance tree which is cached - self.get_cached_metadata_inheritance_tree(Location(location), force_refresh = True) + self.refresh_cached_metadata_inheritance_tree(Location(location)) def get_course_for_item(self, location, depth=0): ''' @@ -588,7 +602,7 @@ class MongoModuleStore(ModuleStoreBase): self._update_single_item(location, {'definition.children': children}) # recompute (and update) the metadata inheritance tree which is cached - self.get_cached_metadata_inheritance_tree(Location(location), force_refresh = True) + self.refresh_cached_metadata_inheritance_tree(Location(location)) def update_metadata(self, location, metadata): """ @@ -614,7 +628,7 @@ class MongoModuleStore(ModuleStoreBase): self._update_single_item(location, {'metadata': metadata}) # recompute (and update) the metadata inheritance tree which is cached - self.get_cached_metadata_inheritance_tree(loc, force_refresh = True) + self.refresh_cached_metadata_inheritance_tree(loc) def delete_item(self, location): """ @@ -637,8 +651,7 @@ class MongoModuleStore(ModuleStoreBase): # from overriding our default value set in the init method. safe=self.collection.safe) # recompute (and update) the metadata inheritance tree which is cached - self.get_cached_metadata_inheritance_tree(Location(location), force_refresh = True) - + self.refresh_cached_metadata_inheritance_tree(Location(location)) def get_parent_locations(self, location, course_id): '''Find all locations that are the parents of this location in this diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index fa232596f2..89ec1116ae 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -201,100 +201,117 @@ def import_from_xml(store, data_dir, course_dirs=None, course_items = [] for course_id in module_store.modules.keys(): - course_data_path = None - course_location = None + if target_location_namespace is not None: + pseudo_course_id = '/'.join([target_location_namespace.org, target_location_namespace.course]) + else: + course_id_components = course_id.split('/') + pseudo_course_id = '/'.join([course_id_components[0], course_id_components[1]]) - if verbose: - log.debug("Scanning {0} for course module...".format(course_id)) + try: + # turn off all write signalling while importing as this is a high volume operation + if pseudo_course_id not in store.ignore_write_events_on_courses: + store.ignore_write_events_on_courses.append(pseudo_course_id) - # Quick scan to get course module as we need some info from there. Also we need to make sure that the - # course module is committed first into the store - for module in module_store.modules[course_id].itervalues(): - if module.category == 'course': - course_data_path = path(data_dir) / module.data_dir - course_location = module.location - - module = remap_namespace(module, target_location_namespace) - - # cdodge: more hacks (what else). Seems like we have a problem when importing a course (like 6.002) which - # does not have any tabs defined in the policy file. The import goes fine and then displays fine in LMS, - # but if someone tries to add a new tab in the CMS, then the LMS barfs because it expects that - - # if there is *any* tabs - then there at least needs to be some predefined ones - if module.tabs is None or len(module.tabs) == 0: - module.tabs = [{"type": "courseware"}, - {"type": "course_info", "name": "Course Info"}, - {"type": "discussion", "name": "Discussion"}, - {"type": "wiki", "name": "Wiki"}] # note, add 'progress' when we can support it on Edge - - - if hasattr(module, 'data'): - store.update_item(module.location, module.data) - store.update_children(module.location, module.children) - store.update_metadata(module.location, dict(own_metadata(module))) - - # a bit of a hack, but typically the "course image" which is shown on marketing pages is hard coded to /images/course_image.jpg - # so let's make sure we import in case there are no other references to it in the modules - verify_content_links(module, course_data_path, static_content_store, '/static/images/course_image.jpg') - - course_items.append(module) - - - # then import all the static content - if static_content_store is not None: - _namespace_rename = target_location_namespace if target_location_namespace is not None else course_location - - # first pass to find everything in /static/ - import_static_content(module_store.modules[course_id], course_location, course_data_path, static_content_store, - _namespace_rename, subpath='static', verbose=verbose) - - # finally loop through all the modules - for module in module_store.modules[course_id].itervalues(): - - if module.category == 'course': - # we've already saved the course module up at the top of the loop - # so just skip over it in the inner loop - continue - - # remap module to the new namespace - if target_location_namespace is not None: - module = remap_namespace(module, target_location_namespace) + course_data_path = None + course_location = None if verbose: - log.debug('importing module location {0}'.format(module.location)) + log.debug("Scanning {0} for course module...".format(course_id)) - if hasattr(module, 'data'): - module_data = module.data + # Quick scan to get course module as we need some info from there. Also we need to make sure that the + # course module is committed first into the store + for module in module_store.modules[course_id].itervalues(): + if module.category == 'course': + course_data_path = path(data_dir) / module.data_dir + course_location = module.location - # cdodge: now go through any link references to '/static/' and make sure we've imported - # it as a StaticContent asset - try: - remap_dict = {} + module = remap_namespace(module, target_location_namespace) - # use the rewrite_links as a utility means to enumerate through all links - # in the module data. We use that to load that reference into our asset store - # IMPORTANT: There appears to be a bug in lxml.rewrite_link which makes us not be able to - # do the rewrites natively in that code. - # For example, what I'm seeing is -> - # Note the dropped element closing tag. This causes the LMS to fail when rendering modules - that's - # no good, so we have to do this kludge - if isinstance(module_data, str) or isinstance(module_data, unicode): # some module 'data' fields are non strings which blows up the link traversal code - lxml_rewrite_links(module_data, lambda link: verify_content_links(module, course_data_path, - static_content_store, link, remap_dict)) + # cdodge: more hacks (what else). Seems like we have a problem when importing a course (like 6.002) which + # does not have any tabs defined in the policy file. The import goes fine and then displays fine in LMS, + # but if someone tries to add a new tab in the CMS, then the LMS barfs because it expects that - + # if there is *any* tabs - then there at least needs to be some predefined ones + if module.tabs is None or len(module.tabs) == 0: + module.tabs = [{"type": "courseware"}, + {"type": "course_info", "name": "Course Info"}, + {"type": "discussion", "name": "Discussion"}, + {"type": "wiki", "name": "Wiki"}] # note, add 'progress' when we can support it on Edge - for key in remap_dict.keys(): - module_data = module_data.replace(key, remap_dict[key]) - except Exception, e: - logging.exception("failed to rewrite links on {0}. Continuing...".format(module.location)) + if hasattr(module, 'data'): + store.update_item(module.location, module.data) + store.update_children(module.location, module.children) + store.update_metadata(module.location, dict(own_metadata(module))) - store.update_item(module.location, module_data) + # a bit of a hack, but typically the "course image" which is shown on marketing pages is hard coded to /images/course_image.jpg + # so let's make sure we import in case there are no other references to it in the modules + verify_content_links(module, course_data_path, static_content_store, '/static/images/course_image.jpg') - if hasattr(module, 'children') and module.children != []: - store.update_children(module.location, module.children) + course_items.append(module) - # NOTE: It's important to use own_metadata here to avoid writing - # inherited metadata everywhere. - store.update_metadata(module.location, dict(own_metadata(module))) + + # then import all the static content + if static_content_store is not None: + _namespace_rename = target_location_namespace if target_location_namespace is not None else course_location + + # first pass to find everything in /static/ + import_static_content(module_store.modules[course_id], course_location, course_data_path, static_content_store, + _namespace_rename, subpath='static', verbose=verbose) + + # finally loop through all the modules + for module in module_store.modules[course_id].itervalues(): + + if module.category == 'course': + # we've already saved the course module up at the top of the loop + # so just skip over it in the inner loop + continue + + # remap module to the new namespace + if target_location_namespace is not None: + module = remap_namespace(module, target_location_namespace) + + if verbose: + log.debug('importing module location {0}'.format(module.location)) + + if hasattr(module, 'data'): + module_data = module.data + + # cdodge: now go through any link references to '/static/' and make sure we've imported + # it as a StaticContent asset + try: + remap_dict = {} + + # use the rewrite_links as a utility means to enumerate through all links + # in the module data. We use that to load that reference into our asset store + # IMPORTANT: There appears to be a bug in lxml.rewrite_link which makes us not be able to + # do the rewrites natively in that code. + # For example, what I'm seeing is -> + # Note the dropped element closing tag. This causes the LMS to fail when rendering modules - that's + # no good, so we have to do this kludge + if isinstance(module_data, str) or isinstance(module_data, unicode): # some module 'data' fields are non strings which blows up the link traversal code + lxml_rewrite_links(module_data, lambda link: verify_content_links(module, course_data_path, + static_content_store, link, remap_dict)) + + for key in remap_dict.keys(): + module_data = module_data.replace(key, remap_dict[key]) + + except Exception, e: + logging.exception("failed to rewrite links on {0}. Continuing...".format(module.location)) + + store.update_item(module.location, module_data) + + if hasattr(module, 'children') and module.children != []: + store.update_children(module.location, module.children) + + # NOTE: It's important to use own_metadata here to avoid writing + # inherited metadata everywhere. + store.update_metadata(module.location, dict(own_metadata(module))) + finally: + # turn back on all write signalling + if pseudo_course_id in store.ignore_write_events_on_courses: + store.ignore_write_events_on_courses.remove(pseudo_course_id) + store.refresh_cached_metadata_inheritance_tree(target_location_namespace if + target_location_namespace is not None else course_location) return module_store, course_items diff --git a/lms/djangoapps/courseware/features/problems.feature b/lms/djangoapps/courseware/features/problems.feature index a7fbac49c7..efeb338c45 100644 --- a/lms/djangoapps/courseware/features/problems.feature +++ b/lms/djangoapps/courseware/features/problems.feature @@ -1,10 +1,11 @@ -Feature: Answer choice problems +Feature: Answer problems As a student in an edX course In order to test my understanding of the material - I want to answer choice based problems + I want to answer problems Scenario: I can answer a problem correctly - Given I am viewing a "" problem + Given External graders respond "correct" + And I am viewing a "" problem When I answer a "" problem "correctly" Then My "" answer is marked "correct" @@ -17,9 +18,11 @@ Feature: Answer choice problems | numerical | | formula | | script | + | code | Scenario: I can answer a problem incorrectly - Given I am viewing a "" problem + Given External graders respond "incorrect" + And I am viewing a "" problem When I answer a "" problem "incorrectly" Then My "" answer is marked "incorrect" @@ -32,6 +35,7 @@ Feature: Answer choice problems | numerical | | formula | | script | + | code | Scenario: I can submit a blank answer Given I am viewing a "" problem diff --git a/lms/djangoapps/courseware/features/problems.py b/lms/djangoapps/courseware/features/problems.py index a6575c3d22..715e2689fb 100644 --- a/lms/djangoapps/courseware/features/problems.py +++ b/lms/djangoapps/courseware/features/problems.py @@ -1,14 +1,15 @@ from lettuce import world, step from lettuce.django import django_url -from selenium.webdriver.support.ui import Select import random import textwrap +import time from common import i_am_registered_for_the_course, TEST_SECTION_NAME, section_location from terrain.factories import ItemFactory from capa.tests.response_xml_factory import OptionResponseXMLFactory, \ ChoiceResponseXMLFactory, MultipleChoiceResponseXMLFactory, \ StringResponseXMLFactory, NumericalResponseXMLFactory, \ - FormulaResponseXMLFactory, CustomResponseXMLFactory + FormulaResponseXMLFactory, CustomResponseXMLFactory, \ + CodeResponseXMLFactory # Factories from capa.tests.response_xml_factory that we will use # to generate the problem XML, with the keyword args used to configure @@ -78,6 +79,12 @@ PROBLEM_FACTORY_DICT = { a2=0 return (a1+a2)==int(expect) """)}}, + 'code': { + 'factory': CodeResponseXMLFactory(), + 'kwargs': { + 'question_text': 'Submit code to an external grader', + 'initial_display': 'print "Hello world!"', + 'grader_payload': '{"grader": "ps1/Spring2013/test_grader.py"}', }}, } @@ -116,6 +123,19 @@ def view_problem(step, problem_type): world.browser.visit(url) +@step(u'External graders respond "([^"]*)"') +def set_external_grader_response(step, correctness): + assert(correctness in ['correct', 'incorrect']) + + response_dict = {'correct': True if correctness == 'correct' else False, + 'score': 1 if correctness == 'correct' else 0, + 'msg': 'Your problem was graded %s' % correctness} + + # Set the fake xqueue server to always respond + # correct/incorrect when asked to grade a problem + world.xqueue_server.set_grade_response(response_dict) + + @step(u'I answer a "([^"]*)" problem "([^"]*)ly"') def answer_problem(step, problem_type, correctness): """ Mark a given problem type correct or incorrect, then submit it. @@ -169,18 +189,29 @@ def answer_problem(step, problem_type, correctness): inputfield('script', input_num=1).fill(str(first_addend)) inputfield('script', input_num=2).fill(str(second_addend)) + elif problem_type == 'code': + # The fake xqueue server is configured to respond + # correct / incorrect no matter what we submit. + # Furthermore, since the inline code response uses + # JavaScript to make the code display nicely, it's difficult + # to programatically input text + # (there's not