Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions common/lib/xmodule/xmodule/capa_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
import json
import logging
import sys
from lxml import etree

from pkg_resources import resource_string

from .capa_base import CapaMixin, CapaFields, ComplexEncoder
from capa import inputtypes
from .progress import Progress
from xmodule.x_module import XModule, module_attr
from xmodule.raw_module import RawDescriptor
Expand Down Expand Up @@ -172,6 +174,13 @@ def non_editable_metadata_fields(self):
])
return non_editable_fields

@property
def problem_types(self):
""" Low-level problem type introspection for content libraries filtering by problem type """
tree = etree.XML(self.data)
registered_tas = inputtypes.registry.registered_tags()
return set([node.tag for node in tree.iter() if node.tag in registered_tas])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Variable is called _tas instead of tags


# Proxy to CapaModule for access to any of its attributes
answer_available = module_attr('answer_available')
check_button_name = module_attr('check_button_name')
Expand Down
53 changes: 52 additions & 1 deletion common/lib/xmodule/xmodule/library_content_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from bson.objectid import ObjectId, InvalidId
from collections import namedtuple
from copy import copy

from .mako_module import MakoModuleDescriptor
from opaque_keys import InvalidKeyError
from opaque_keys.edx.locator import LibraryLocator
Expand All @@ -19,15 +20,57 @@
from .xml_module import XmlDescriptor
from pkg_resources import resource_string


# Make '_' a no-op so we can scrape strings
_ = lambda text: text


ANY_CAPA_TYPE_VALUE = 'any'
CAPA_BLOCK_TYPE = 'problem'


def enum(**enums):
""" enum helper in lieu of enum34 """
return type('Enum', (), enums)


def _get_capa_types():
"""
Gets capa types tags and labels
"""
capa_types = {
'annotationinput': _('Annotation'),
'checkboxgroup': _('Checkbox Group'),
'checkboxtextgroup': _('Checkbox Text Group'),
'chemicalequationinput': _('Chemical Equation'),
'choicegroup': _('Choice Group'),
'codeinput': _('Code Input'),
'crystallography': _('Crystallography'),
'designprotein2dinput': _('Design Protein 2D'),
'drag_and_drop_input': _('Drag and Drop'),
'editageneinput': _('Edit A Gene'),
'editamoleculeinput': _('Edit A Molecule'),
'filesubmission': _('File Submission'),
'formulaequationinput': _('Formula Equation'),
'imageinput': _('Image'),
'javascriptinput': _('Javascript Input'),
'jsinput': _('JS Input'),
'matlabinput': _('Matlab'),
'optioninput': _('Select Option'),
'radiogroup': _('Radio Group'),
'radiotextgroup': _('Radio Text Group'),
'schematic': _('Schematic'),
'textbox': _('Code Text Input'),
'textline': _('Text Line'),
'vsepr_input': _('VSEPR'),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names should match the names used in Studio... I just tried to find "Numerical Response" and it wasn't on this list - eventually I realized that it's called "Formula Equation" here. I think you should change all these to be the same as in the "Add Component" menu. You might also want to change them to look at the response type tag rather than the input type tag?? (Not sure which is "better")

e.g. Below the response type is numericalresponse and the input type is formulaequation

<problem>
<p>Calculate the total kinetic energy released in the β decay of a single free electron.</p>

<p>Enter K in MeV:</p>
<numericalresponse answer="0.783">
  <responseparam type="tolerance" default=".001" />
  <formulaequationinput label="Enter K in MeV:" />
</numericalresponse>

</problem>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradenmacdonald "Add Component" menu maps almost one-to-one to responses. However, there are CustomResponse (and children) which allow virtually any input type tag and can represent anything. Also, it's used for two add component templates and it's not possible to distinguish between them. I'll switch to responses though, but there won't be "Custom Javascript Display and Grading" and "CUSTOM PYTHON-EVALUATED INPUT" - just "Custom" which would filter for both. I can't think of better approach achievable in time window we have, so if you have a better ideas I suggest keeping them for post-MVP era.

Btw, "Drag-and-Drop" appears to be custom script as well.

}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns out to be least invasive and complex solution. Alternatives were:

  • Add problem_type class method to every CAPA Input type: results in shotgun change - a couple of lines into ~20 classes.
  • Determine problem name by CAPA input class name: some classes support multiple modes (e.g. checkboxgroup and radiogroup) - results in three/four exceptions to the rule that cover ~20 cases. Plus the rule itself would be a bit non-trivial - something like "split into words by capital letters, remove stopwords (e.g. Input), join by space, but don't split JS"

On the other hand, this approach have no "moving parts" (i.e. trivial logic), localized and does not require using reflection in production code. I'm going to try covering this method with test that would check if all the CAPA input type tags are present, so it would still be robust to changes/additions/deletions of CAPA input types.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird that something like this doesn't exist already.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradenmacdonald unfortunately, it doesn't. Labels displayed in "Add XBlock" -> "Problem" menu are names of templates, but since it's possible to have more than one control in a problem there're no such thing as "Problem Type". And controls themselves does not have display names.


return [{'value': ANY_CAPA_TYPE_VALUE, 'display_name': _('Any Type')}] + sorted([
{'value': capa_type, 'display_name': caption}
for capa_type, caption in capa_types.items()
], key=lambda item: item.get('display_name'))


class LibraryVersionReference(namedtuple("LibraryVersionReference", "library_id version")):
"""
A reference to a specific library, with an optional version.
Expand Down Expand Up @@ -146,6 +189,13 @@ class LibraryContentFields(object):
default=1,
scope=Scope.settings,
)
capa_type = String(
display_name=_("Problem Type"),
help=_("The type of components to include in this block"),
default=ANY_CAPA_TYPE_VALUE,
values=_get_capa_types(),
scope=Scope.settings,
)
filters = String(default="") # TBD
has_score = Boolean(
display_name=_("Scored"),
Expand Down Expand Up @@ -362,7 +412,8 @@ def editor_saved(self, user, old_metadata, old_content):
If source_libraries has been edited, refresh_children automatically.
"""
old_source_libraries = LibraryList().from_json(old_metadata.get('source_libraries', []))
if set(old_source_libraries) != set(self.source_libraries):
if set(old_source_libraries) != set(self.source_libraries) or \
old_metadata.get('capa_type', ANY_CAPA_TYPE_VALUE) != self.capa_type:
try:
self.refresh_children(None, None, update_db=False) # update_db=False since update_item() is about to be called anyways
except ValueError:
Expand Down
22 changes: 19 additions & 3 deletions common/lib/xmodule/xmodule/library_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
import hashlib
from opaque_keys.edx.locator import LibraryLocator
from xblock.fields import Scope
from xmodule.library_content_module import LibraryVersionReference
from xmodule.library_content_module import LibraryVersionReference, ANY_CAPA_TYPE_VALUE
from xmodule.modulestore.exceptions import ItemNotFoundError
from xmodule.capa_module import CapaDescriptor


class LibraryToolsService(object):
Expand Down Expand Up @@ -54,6 +55,18 @@ def get_library_display_name(self, lib_key):
return library.display_name
return None

def _filter_child(self, dest_block, child_descriptor):
"""
Filters children by CAPA problem type, if configured
"""
if dest_block.capa_type == ANY_CAPA_TYPE_VALUE:
return True

if not isinstance(child_descriptor, CapaDescriptor):
return False

return dest_block.capa_type in child_descriptor.problem_types

def update_children(self, dest_block, user_id, update_db=True):
"""
This method is to be used when any of the libraries that a LibraryContentModule
Expand Down Expand Up @@ -96,13 +109,16 @@ def update_children(self, dest_block, user_id, update_db=True):
new_libraries = []
for library_key, library in libraries:

def copy_children_recursively(from_block):
def copy_children_recursively(from_block, filter_problem_type=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should add filter_problem_type as an argument, if it's always True.

Perhaps just create a local variable at the beginning of update_children where filter_problem_type = (self.capa_type != ANY_CAPA_TYPE_VALUE). It's better to do that test once than to always call _filter_child for each block and decide if we're filtering once that helper method has already been called.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradenmacdonald oooough, thanks for noticing it - it was supposed to be False. The reason behind this is to filter only top-level xblocks so that if CapaModule ever had children blocks those children blocks are included if parent block is included, and don't filtered by problem type.

"""
Internal method to copy blocks from the library recursively
"""
new_children = []
for child_key in from_block.children:
child = self.store.get_item(child_key, depth=9)

if filter_problem_type and not self._filter_child(dest_block, child):
continue
# We compute a block_id for each matching child block found in the library.
# block_ids are unique within any branch, but are not unique per-course or globally.
# We need our block_ids to be consistent when content in the library is updated, so
Expand Down Expand Up @@ -130,7 +146,7 @@ def copy_children_recursively(from_block):
)
new_children.append(new_child_info.location)
return new_children
root_children.extend(copy_children_recursively(from_block=library))
root_children.extend(copy_children_recursively(from_block=library, filter_problem_type=True))
new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
dest_block.source_libraries = new_libraries
dest_block.children = root_children
Expand Down
11 changes: 11 additions & 0 deletions common/test/acceptance/pages/lms/library.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ def __init__(self, browser, locator):
self.locator = locator

def is_browser_on_page(self):
"""
Checks if page is opened
"""
return self.q(css='{}[data-id="{}"]'.format(self.BODY_SELECTOR, self.locator)).present

def _bounded_selector(self, selector):
Expand All @@ -35,3 +38,11 @@ def children_contents(self):
"""
child_blocks = self.q(css=self._bounded_selector("div[data-id]"))
return frozenset(child.text for child in child_blocks)

@property
def children_headers(self):
"""
Gets headers of all child XBlocks as list of strings
"""
child_blocks_headers = self.q(css=self._bounded_selector("div[data-id] h2.problem-header"))
return frozenset(child.text for child in child_blocks_headers)
19 changes: 19 additions & 0 deletions common/test/acceptance/pages/studio/library.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ class StudioLibraryContentXBlockEditModal(CourseOutlineModal, PageObject):
LIBRARY_LABEL = "Libraries"
COUNT_LABEL = "Count"
SCORED_LABEL = "Scored"
PROBLEM_TYPE_LABEL = "Problem Type"

def is_browser_on_page(self):
"""
Expand Down Expand Up @@ -196,6 +197,24 @@ def scored(self, scored):
scored_select.select_by_value(str(scored))
EmptyPromise(lambda: self.scored == scored, "scored is updated in modal.").fulfill()

@property
def capa_type(self):
"""
Gets value of CAPA type select
"""
return self.get_metadata_input(self.PROBLEM_TYPE_LABEL).get_attribute('value')

@capa_type.setter
def capa_type(self, value):
"""
Sets value of CAPA type select
"""
select_element = self.get_metadata_input(self.PROBLEM_TYPE_LABEL)
select_element.click()
problem_type_select = Select(select_element)
problem_type_select.select_by_value(value)
EmptyPromise(lambda: self.capa_type == value, "problem type is updated in modal.").fulfill()

def _add_library_key(self):
"""
Adds library key input
Expand Down
Loading