Skip to content

Select problem by type - #24

Closed
e-kolpakov wants to merge 11 commits into
content_libraries/3-library-course-blockfrom
content_libraries/3.1-select-by-problem-type
Closed

Select problem by type#24
e-kolpakov wants to merge 11 commits into
content_libraries/3-library-course-blockfrom
content_libraries/3.1-select-by-problem-type

Conversation

@e-kolpakov

Copy link
Copy Markdown

Internal/temporary PR for review.

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.

@bradenmacdonald
bradenmacdonald force-pushed the content_libraries/3-library-course-block branch from ad29a90 to 03a9ebb Compare December 17, 2014 20:15
@bradenmacdonald

Copy link
Copy Markdown
Member

@e-kolpakov Two things:

  1. This needs to be rebased.
  2. It would be much better to do the actual filtering in common/xmodule/xmodule/library_tools.py:update_children(). Because then it just happens once, for all students. Whereas if you're filtering in selected_children(), then the system has to load all possible XBlocks and filter them every time a new student views the block.
    (Also change the editor_saved() method of LibraryContentDescriptor so that it will refresh the children whenever the capa_type setting is changed.)

Otherwise, your approach looks good here from what I can see (well, as good as it can be, given that this task is a bit of a hack).

@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch from 5aa4397 to c0998b2 Compare December 19, 2014 12:20
@e-kolpakov

Copy link
Copy Markdown
Author

@bradenmacdonald thanks for the hint. I'm trying to do that, but there's a problem - filtering requires CapaProblem introspection. It's available as lcp attribute on CapaModule. However, update_children operates on XModule Descriptors.

So, surprisingly, CapaDescriptor does not have lcp attribute. It actually throws UndefinedContext in common/lib/xmodule/xmodule/x_module.py:1032. Looks like the environment does not provide runtime_system to the Descriptor, so it can't instantiate XModule. I recall you have faced this problem once - have you found the solution?

@bradenmacdonald

Copy link
Copy Markdown
Member

The descriptor needs a reference to a ModuleSystem (the CMS uses PreviewModuleSystem as its ModuleSystem). You can convert the Descriptor child to an XModule by calling child.bind_for_student(module_system, child._field_data) where module_system is either the result of _preview_module_system(...) (in cms views/preview.py) or get_test_system() (in xmodule/tests/init.py). I would suggest that you just do the following since you don't need a full modulesystem as you aren't rendering templates or persisting student data or anything:

from xmodule.tests import get_test_system
child.bind_for_student(get_test_system(), child._field_data)
# Now child.lcp should be accessible...

If that's not working and you need the big picture of how these work, check out _load_preview_module in preview.py for a more full-featured example.

@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch 4 times, most recently from c0afe58 to 102047e Compare December 22, 2014 16:25
@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch from 102047e to c051785 Compare December 22, 2014 17:50
@bradenmacdonald

Copy link
Copy Markdown
Member

My only other feedback here is that you have added five new bok choy tests (if you count the ddt variants). I seem to recall Steve or someone else from upstream saying that because bok choy tests are so slow, we should aim for closer to one new bok choy test per story. You may want to try to consolidate these tests somehow.

@bradenmacdonald
bradenmacdonald force-pushed the content_libraries/3-library-course-block branch from 82b7bb8 to 0cc3b33 Compare December 23, 2014 19:31
@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch from 0f0821c to ea03428 Compare December 24, 2014 08:39

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

@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch from 6a6f3c2 to d6fa6c0 Compare December 29, 2014 13:39
@e-kolpakov

Copy link
Copy Markdown
Author

@bradenmacdonald all notes are addressed - thanks for suggestion of using response types rather than input types - it's more intuitive. Please note my comments on "Custom" type though - basically three "Add Component" templates fall into this filter: "Custom Javascript Display and Grading", "Custom Python-Evaluated Input" and (surprisingly) "Drag and Drop"

@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch from d6fa6c0 to e19e166 Compare December 29, 2014 13:58
@bradenmacdonald

Copy link
Copy Markdown
Member

@e-kolpakov Sounds good.

… problems

than libraries and filtering allow
@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch from 8056ce2 to a487da4 Compare December 30, 2014 09:27
@e-kolpakov
e-kolpakov force-pushed the content_libraries/3.1-select-by-problem-type branch from a487da4 to 9fc92ac Compare December 30, 2014 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants