-
Notifications
You must be signed in to change notification settings - Fork 9
Initial implementation #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4948958
43b1a87
43193a7
d14ad36
b11d2bf
04b9e98
dbe5469
d139a10
68a4359
d872ded
a3c5bff
781f858
82ff355
322a080
dc4a23a
4b9a5a1
d5032dd
f0d009c
29bb31c
072ab3e
5d2bc7d
28d5a46
bbc750c
7507f88
706d937
1777066
2c8ea90
812df3a
4de8375
41486ac
b8548d9
5125445
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| __pycache__/ | ||
| *.py[cod] | ||
| activetable_xblock.egg-info/** | ||
| .coverage | ||
| tests.integration.*.log | ||
| tests.integration.*.png | ||
| var/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| language: python | ||
| python: | ||
| - 2.7 | ||
| before_install: | ||
| - export DISPLAY=:99 | ||
| - sh -e /etc/init.d/xvfb start | ||
| install: | ||
| - pip install -r test-requirements.txt | ||
| - pip install -r $VIRTUAL_ENV/src/xblock-sdk/requirements/base.txt | ||
| - pip install -r $VIRTUAL_ENV/src/xblock-sdk/requirements/test.txt | ||
| - pip install -r $VIRTUAL_ENV/src/xblock/requirements.txt | ||
| script: | ||
| - pep8 --max-line-length=100 activetable | ||
| - pylint activetable | ||
| - ./run_tests.py --with-coverage --cover-package=activetable | ||
| notifications: | ||
| email: false | ||
| addons: | ||
| firefox: 36.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| ActiveTable XBlock | ||
| ================== | ||
|
|
||
| This XBlock provides a tabular problem type, where students have to fill in some of the cells of a | ||
| table. | ||
|
|
||
|
|
||
| Running the tests | ||
| ----------------- | ||
|
|
||
| Install the test prerequisites: | ||
|
|
||
| pip install -r test-requirements | ||
|
|
||
| Run pep8: | ||
|
|
||
| pep8 --max-line-length=100 activetable | ||
|
|
||
| Run pylint: | ||
|
|
||
| pylint activetable | ||
|
|
||
| Run the unit and integration tests: | ||
|
|
||
| ./run-tests.sh --with-coverage --cover-package=activetable | ||
|
|
||
|
|
||
| The table definition | ||
| -------------------- | ||
|
|
||
| The table definition is entered in a Python-like syntax (actually in a strict subset of Python). It | ||
| must be a list of lists, with all inner lists having the same lengths. The elements of the inner | ||
| lists correspond to the cells of the table. The first line contains the column headers and can only | ||
| contain string literals. All further lines represent the table body. Cells can be either string | ||
| literals, e.g. `'a string'`, numbers, e.g. `6.23`, or response cell declarations. There are two | ||
| types of response cells: | ||
|
|
||
| Numeric(answer=<correct_answer>, tolerance=<tolerance in percent>, | ||
| min_significant_digits=<number>, max_significant_digits=<number>) | ||
|
|
||
| A cell that expects a numeric answer. The tolerance is optional, and will default to the default | ||
| tolerance specified above. The restrictions for the number of significant digits are optional as | ||
| well. Significant digits are counted started from the first non-zero digit specified by the | ||
| student, and include trailing zeros. | ||
|
|
||
| Text(answer='<correct answer>') | ||
|
|
||
| A cell that expects a string answer. | ||
|
|
||
| An example of a table definition: | ||
|
|
||
| [ | ||
| ['Event', 'Year'], | ||
| ['French Revolution', Numeric(answer=1789)], | ||
| ['Krakatoa volcano explosion', Numeric(answer=1883)], | ||
| ["Proof of Fermat's last theorem", Numeric(answer=1994)], | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| """ActiveTable XBlock top-level package. | ||
|
|
||
| See activetable.activetable for more information. | ||
| """ | ||
| from .activetable import ActiveTableXBlock |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,298 @@ | ||
| # -*- coding: utf-8 -*- | ||
| """An XBlock with a tabular problem type that requires students to fill in some cells.""" | ||
| from __future__ import absolute_import, division, unicode_literals | ||
|
|
||
| import textwrap | ||
|
|
||
| from xblock.core import XBlock | ||
| from xblock.fields import Dict, Float, Integer, Scope, String | ||
| from xblock.fragment import Fragment | ||
| from xblock.validation import ValidationMessage | ||
| from xblockutils.resources import ResourceLoader | ||
| from xblockutils.studio_editable import StudioEditableXBlockMixin | ||
|
|
||
| from .cells import NumericCell | ||
| from .parsers import ParseError, parse_table, parse_number_list | ||
|
|
||
| loader = ResourceLoader(__name__) # pylint: disable=invalid-name | ||
|
|
||
|
|
||
| class ActiveTableXBlock(StudioEditableXBlockMixin, XBlock): | ||
| """An XBlock with a tabular problem type that requires students to fill in some cells.""" | ||
|
|
||
| display_name = String( | ||
| display_name='Display Name', | ||
| help='The title Studio uses for the component.', | ||
| scope=Scope.settings, | ||
| default='ActiveTable problem' | ||
| ) | ||
| content = String( | ||
| display_name='Table definition', | ||
| help='The definition of the table in Python-like syntax. Note that changing the table ' | ||
| 'definition of a live problem will invalidate all student answers.', | ||
| scope=Scope.content, | ||
| multiline_editor=True, | ||
| resettable_editor=False, | ||
| default=textwrap.dedent("""\ | ||
| [ | ||
| ['Column header 1', 'Column header 2'], | ||
| ['Enter "answer" here:', Text(answer='answer')], | ||
| [42, Numeric(answer=42, tolerance=0.0)], | ||
| ] | ||
| """) | ||
| ) | ||
| help_text = String( | ||
| display_name='Help text', | ||
| help='The text that gets displayed when clicking the "+help" button. If you remove the ' | ||
| 'help text, the help feature is disabled.', | ||
| scope=Scope.content, | ||
| multiline_editor=True, | ||
| resettable_editor=False, | ||
| default='Fill in the cells highlighted in yellow with the correct answers. ' | ||
| 'When you are done, you can check your answers using the button below the table.', | ||
| ) | ||
| column_widths = String( | ||
| display_name='Column widths', | ||
| help='Set the width of the columns in pixels. The value should be a Python-like list of ' | ||
| 'numerical values. The total width of the table should not be more than 800. Omitting ' | ||
| 'this value will result in equal-width columns with a total width of 800 pixels.', | ||
| scope=Scope.content, | ||
| resettable_editor=False, | ||
| ) | ||
| row_heights = String( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The row heights and column widths fields have a reset button next to them in the interface. However, this button does not seem to work. Clicking it makes the field appear empty, but saving it does not actually save an empty string-- it just reverts to whatever it was before you hit the button. Putting in a new value does get saved, though. |
||
| display_name='Row heights', | ||
| help='Set the heights of the rows in pixels. The value should be a Python-like list of ' | ||
| 'numerical values. Rows may grow higher than the specified value if the text in some cells ' | ||
| 'in the row is long enough to get wrapped in more than one line.', | ||
| scope=Scope.content, | ||
| resettable_editor=False, | ||
| ) | ||
| default_tolerance = Float( | ||
| display_name='Default tolerance', | ||
| help='The tolerance in percent that is used for numerical response cells you did not ' | ||
| 'specify an explicit tolerance for.', | ||
| scope=Scope.content, | ||
| default=1.0, | ||
| ) | ||
| max_score = Float( | ||
| display_name='Maximum score', | ||
| help='The number of points students will be awarded when solving all fields correctly. ' | ||
| 'For partially correct attempts, the score will be pro-rated.', | ||
| scope=Scope.settings, | ||
| default=1.0, | ||
| ) | ||
| max_attempts = Integer( | ||
| display_name='Maximum attempts', | ||
| help='Defines the number of times a student can try to answer this problem. If the value ' | ||
| 'is not set, infinite attempts are allowed.', | ||
| scope=Scope.settings, | ||
| ) | ||
|
|
||
| editable_fields = [ | ||
| 'display_name', | ||
| 'content', | ||
| 'help_text', | ||
| 'column_widths', | ||
| 'row_heights', | ||
| 'default_tolerance', | ||
| 'max_score', | ||
| 'max_attempts', | ||
| ] | ||
|
|
||
| # Dictionary mapping cell ids to the student answers. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are cell IDs stable while editing? Inserting a row in the table will make students' existing answers mismatched, right? Is this understood by the course team? Can we say something in the help text?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, cell IDs are really just strings like |
||
| answers = Dict(scope=Scope.user_state) | ||
| # Dictionary mapping cell ids to Boolean values indicating whether the cell was answered | ||
| # correctly at the last check. | ||
| answers_correct = Dict(scope=Scope.user_state, default=None) | ||
| # The number of points awarded. | ||
| score = Float(scope=Scope.user_state) | ||
| # The number of attempts used. | ||
| attempts = Integer(scope=Scope.user_state, default=0) | ||
|
|
||
| has_score = True | ||
|
|
||
| @property | ||
| def num_correct_answers(self): | ||
| """The number of correct answers during the last check.""" | ||
| if self.answers_correct is None: | ||
| return None | ||
| return sum(self.answers_correct.itervalues()) | ||
|
|
||
| @property | ||
| def num_total_answers(self): | ||
| """The total number of answers during the last check.""" | ||
| if self.answers_correct is None: | ||
| return None | ||
| return len(self.answers_correct) | ||
|
|
||
| def parse_fields(self): | ||
| """Parse the user-provided fields into more processing-friendly structured data.""" | ||
| if self.content: | ||
| self.thead, self.tbody = parse_table(self.content) | ||
| else: | ||
| self.thead = self.tbody = None | ||
| return | ||
| if self.column_widths: | ||
| self._column_widths = parse_number_list(self.column_widths) | ||
| else: | ||
| self._column_widths = [800 / len(self.thead)] * len(self.thead) | ||
| if self.row_heights: | ||
| self._row_heights = parse_number_list(self.row_heights) | ||
| else: | ||
| self._row_heights = [36] * (len(self.tbody) + 1) | ||
|
|
||
| def postprocess_table(self): | ||
| """Augment the parsed table definition with further information. | ||
|
|
||
| The additional information is taken from other content and student state fields. | ||
| """ | ||
| self.response_cells = {} | ||
| for row, height in zip(self.tbody, self._row_heights[1:]): | ||
| row['height'] = height | ||
| if row['index'] % 2: | ||
| row['class'] = 'even' | ||
| else: | ||
| row['class'] = 'odd' | ||
| for cell, cell.col_label in zip(row['cells'], self.thead): | ||
| cell.id = 'cell_{}_{}'.format(row['index'], cell.index) | ||
| cell.classes = '' | ||
| if not cell.is_static: | ||
| self.response_cells[cell.id] = cell | ||
| cell.classes = 'active' | ||
| cell.value = self.answers.get(cell.id) | ||
| cell.height = height - 2 | ||
| if isinstance(cell, NumericCell) and cell.abs_tolerance is None: | ||
| cell.set_tolerance(self.default_tolerance) | ||
|
|
||
| def get_status(self): | ||
| """Status dictionary passed to the frontend code.""" | ||
| return dict( | ||
| answers_correct=self.answers_correct, | ||
| num_correct_answers=self.num_correct_answers, | ||
| num_total_answers=self.num_total_answers, | ||
| score=self.score, | ||
| max_score=self.max_score, | ||
| attempts=self.attempts, | ||
| max_attempts=self.max_attempts, | ||
| ) | ||
|
|
||
| def student_view(self, context=None): | ||
| """Render the table.""" | ||
| self.parse_fields() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm surprised to see the fields being parsed each time. Not sure why that bothers me, but it seems like significant work that could be done just once.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The result of parsing is a hierachical structure of Python data types that are not JSON-serializable, so I don't see an easy way of doing the parsing only once. I initially added the parsing step to |
||
| self.postprocess_table() | ||
|
|
||
| context = dict( | ||
| help_text=self.help_text, | ||
| total_width=sum(self._column_widths) if self._column_widths else None, | ||
| column_widths=self._column_widths, | ||
| head_height=self._row_heights[0] if self._row_heights else None, | ||
| thead=self.thead, | ||
| tbody=self.tbody, | ||
| max_attempts=self.max_attempts, | ||
| ) | ||
| html = loader.render_template('templates/html/activetable.html', context) | ||
|
|
||
| css_context = dict( | ||
| correct_icon=self.runtime.local_resource_url(self, 'public/img/correct-icon.png'), | ||
| incorrect_icon=self.runtime.local_resource_url(self, 'public/img/incorrect-icon.png'), | ||
| unanswered_icon=self.runtime.local_resource_url(self, 'public/img/unanswered-icon.png'), | ||
| ) | ||
| css = loader.render_template('templates/css/activetable.css', css_context) | ||
|
|
||
| frag = Fragment(html) | ||
| frag.add_css(css) | ||
| frag.add_javascript(loader.load_unicode('static/js/src/activetable.js')) | ||
| frag.initialize_js('ActiveTableXBlock', self.get_status()) | ||
| return frag | ||
|
|
||
| def check_and_save_answers(self, data): | ||
| """Common implementation for the check and save handlers.""" | ||
| if self.max_attempts and self.attempts >= self.max_attempts: | ||
| # The "Check" button is hidden when the maximum number of attempts has been reached, so | ||
| # we can only get here by manually crafted requests. We simply return the current | ||
| # status without rechecking or storing the answers in that case. | ||
| return self.get_status() | ||
| self.parse_fields() | ||
| self.postprocess_table() | ||
| answers_correct = { | ||
| cell_id: self.response_cells[cell_id].check_response(value) | ||
| for cell_id, value in data.iteritems() | ||
| } | ||
| # Since the previous statement executed without error, the data is well-formed enough to be | ||
| # stored. We now know it's a dictionary and all the keys are valid cell ids. | ||
| self.answers = data | ||
| return answers_correct | ||
|
|
||
| @XBlock.json_handler | ||
| def check_answers(self, data, unused_suffix=''): | ||
| """Check the answers given by the student. | ||
|
|
||
| This handler is called when the "Check" button is clicked. | ||
| """ | ||
| self.answers_correct = self.check_and_save_answers(data) | ||
| self.attempts += 1 | ||
| self.score = self.num_correct_answers * self.max_score / len(self.answers_correct) | ||
| self.runtime.publish(self, 'grade', dict(value=self.score, max_value=self.max_score)) | ||
| return self.get_status() | ||
|
|
||
| @XBlock.json_handler | ||
| def save_answers(self, data, unused_suffix=''): | ||
| """Save the answers given by the student without checking them.""" | ||
| self.check_and_save_answers(data) | ||
| self.answers_correct = None | ||
| return self.get_status() | ||
|
|
||
| def validate_field_data(self, validation, data): | ||
| """Validate the data entered by the user. | ||
|
|
||
| This handler is called when the "Save" button is clicked in Studio after editing the | ||
| properties of this XBlock. | ||
| """ | ||
| def add_error(msg): | ||
| """Add a validation error.""" | ||
| validation.add(ValidationMessage(ValidationMessage.ERROR, msg)) | ||
| try: | ||
| thead, tbody = parse_table(data.content) | ||
| except ParseError as exc: | ||
| add_error('Problem with table definition: ' + exc.message) | ||
| thead = tbody = None | ||
| if data.column_widths: | ||
| try: | ||
| column_widths = parse_number_list(data.column_widths) | ||
| except ParseError as exc: | ||
| add_error('Problem with column widths: ' + exc.message) | ||
| else: | ||
| if thead is not None and len(column_widths) != len(thead): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you want to check the 800px limit?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I intentionally didn't check it. The 800px limit is a rule of thumb that works well when using this XBlock on edx-platform on desktop, but people might have other requirements anyway, so it's really up to them to decide whether it looks good enough for them. |
||
| add_error( | ||
| 'The number of list entries in the Column widths field must match the ' | ||
| 'number of columns in the table.' | ||
| ) | ||
| if data.row_heights: | ||
| try: | ||
| row_heights = parse_number_list(data.row_heights) | ||
| except ParseError as exc: | ||
| add_error('Problem with row heights: ' + exc.message) | ||
| else: | ||
| if tbody is not None and len(row_heights) != len(tbody) + 1: | ||
| add_error( | ||
| 'The number of list entries in the Row heights field must match the number ' | ||
| 'of rows in the table.' | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def workbench_scenarios(): | ||
| """A canned scenario for display in the workbench.""" | ||
| return [ | ||
| ("ActiveTableXBlock", | ||
| """<vertical_demo> | ||
| <activetable url_name="basic"> | ||
| [ | ||
| ['Event', 'Year'], | ||
| ['French Revolution', Numeric(answer=1789)], | ||
| ['Krakatoa volcano explosion', Numeric(answer=1883)], | ||
| ["Proof of Fermat's last theorem", Numeric(answer=1994)], | ||
| ] | ||
| </activetable> | ||
| </vertical_demo> | ||
| """), | ||
| ] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're using XBlocks, we can (and should) be designing these sorts of things with Child blocks if we can. This work of doing custom parsing is a lot to maintain when we have things like XBlock-utils to automate much of our form building. If we don't do this now, we're going to be stuck with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kelketek The client explicitly requested to not turn this into a point-and-click interface. See Xavier's comments on the ticket for details.
I think child XBlocks would be a bad fit for this anyway. Making every cell an XBlock would make creating tables really awkward, and it wouldn't be particularly efficient performance-wise either. What I could imagine is to design a tailored graphical interface in JavaScript, but, as mentioned above, we were explicitly asked not to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well.