Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions common/lib/xmodule/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
"word_cloud = xmodule.word_cloud_module:WordCloudDescriptor",
"hidden = xmodule.hidden_module:HiddenDescriptor",
"raw = xmodule.raw_module:RawDescriptor",
"crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor",
],
'console_scripts': [
'xmodule_assets = xmodule.static_content:main',
Expand Down
307 changes: 307 additions & 0 deletions common/lib/xmodule/xmodule/crowdsource_hinter.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,307 @@
"""
Adds crowdsourced hinting functionality to lon-capa numerical response problems.

Currently experimental - not for instructor use, yet.
"""

import logging

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add an intro docstring, describing what the module does in very general terms. Also make add a line at the beginning that highlights that the module is experimental and should not be used to create content at the moment (because we may change things, etc).

import json
import random

from pkg_resources import resource_string

from lxml import etree

from xmodule.x_module import XModule
from xmodule.xml_module import XmlDescriptor
from xblock.core import Scope, String, Integer, Boolean, Dict, List

from django.utils.html import escape

log = logging.getLogger(__name__)


class CrowdsourceHinterFields(object):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is good practice to add a docstring to the class, even if it is short as in this case.

"""Defines fields for the crowdsource hinter module."""
has_children = True

moderate = String(help='String "True"/"False" - activates moderation', scope=Scope.content,
default='False')
debug = String(help='String "True"/"False" - allows multiple voting', scope=Scope.content,
default='False')
# hints[answer] = {str(pk): [hint_text, #votes]}
hints = Dict(help='A dictionary containing all the active hints.', scope=Scope.content, default={})
mod_queue = Dict(help='A dictionary containing hints still awaiting approval', scope=Scope.content,
default={})
hint_pk = Integer(help='Used to index hints.', scope=Scope.content, default=0)
# A list of previous answers this student made to this problem.
# Of the form (answer, (hint_pk_1, hint_pk_2, hint_pk_3)) for each problem. hint_pk's are
# None if the hint was not given.
previous_answers = List(help='A list of previous submissions.', scope=Scope.user_state, default=[])
user_voted = Boolean(help='Specifies if the user has voted on this problem or not.',
scope=Scope.user_state, default=False)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It think you may the able to reorder the fields to make it more readable. Put the ones that can be configured (like moderate) first, and then the rest trying to put related fields close to each other.


class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
"""
An Xmodule that makes crowdsourced hints.
Currently, only works on capa problems with exactly one numerical response,
and no other parts.

Example usage:
<crowdsource_hinter>
<problem blah blah />
</crowdsource_hinter>

XML attributes:
-moderate="True" will not display hints until staff approve them in the hint manager.
-debug="True" will let users vote as often as they want.
"""
icon_class = 'crowdsource_hinter'
js = {'coffee': [resource_string(__name__, 'js/src/crowdsource_hinter/display.coffee')],
'js': []}
js_module_name = "Hinter"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PEP 8 will complain for double space.

def __init__(self, *args, **kwargs):
XModule.__init__(self, *args, **kwargs)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

def get_html(self):
"""
Puts a wrapper around the problem html. This wrapper includes ajax urls of the
hinter and of the problem.
- Dependent on lon-capa problem.
"""
if self.debug == 'True':
# Reset the user vote, for debugging only!
self.user_voted = False
if self.hints == {}:
# Force self.hints to be written into the database. (When an xmodule is initialized,
# fields are not added to the db until explicitly changed at least once.)
self.hints = {}

try:
child = self.get_display_items()[0]
out = child.get_html()
# The event listener uses the ajax url to find the child.
child_url = child.system.ajax_url
except IndexError:
out = 'Error in loading crowdsourced hinter - can\'t find child problem.'
child_url = ''

# Wrap the module in a <section>. This lets us pass data attributes to the javascript.
out += '<section class="crowdsource-wrapper" data-url="' + self.system.ajax_url +\
'" data-child-url = "' + child_url + '"> </section>'
return out

def capa_answer_to_str(self, answer):
"""
Converts capa answer format to a string representation
of the answer.
-Lon-capa dependent.
-Assumes that the problem only has one part.
"""
return str(float(answer.values()[0]))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The assumption here is that is the problem only has one numerical response correct? If so please add a comment here and to the class.


def handle_ajax(self, dispatch, get):
"""
This is the landing method for AJAX calls.
"""
if dispatch == 'get_hint':

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In python this series of ifs is normally done with a dictionary. Look at capa_module handle_ajax for an example. Also that example handles the case when one of the functions called raises an exception.

out = self.get_hint(get)
elif dispatch == 'get_feedback':
out = self.get_feedback(get)
elif dispatch == 'vote':
out = self.tally_vote(get)
elif dispatch == 'submit_hint':
out = self.submit_hint(get)
else:
return json.dumps({'contents': 'Error - invalid operation.'})

if out is None:
out = {'op': 'empty'}
else:
out.update({'op': dispatch})
return json.dumps({'contents': self.system.render_template('hinter_display.html', out)})

def get_hint(self, get):
"""
The student got the incorrect answer found in get. Give him a hint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify the docstring a bit? Who class this? When it gets called? What is the format of the input and output. In a separate pull request I renamed the "get" paramater for all handle_ajax functions to "data".


Called by hinter javascript after a problem is graded as incorrect.
Args:
`get` -- must be interpretable by capa_answer_to_str.
Output keys:
- 'best_hint' is the hint text with the most votes.
- 'rand_hint_1' and 'rand_hint_2' are two random hints to the answer in `get`.
- 'answer' is the parsed answer that was submitted.
"""
answer = self.capa_answer_to_str(get)
# Look for a hint to give.
# Make a local copy of self.hints - this means we only need to do one json unpacking.
# (This is because xblocks storage makes the following command a deep copy.)
local_hints = self.hints

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a comment here saying that it is because of the what XBlock storages works. Normally in python when you see a =self.b you don't expect a copy, which is normally done by a = self.b.copy().

if (answer not in local_hints) or (len(local_hints[answer]) == 0):
# No hints to give. Return.
self.previous_answers += [[answer, [None, None, None]]]
return

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it valid to return nothing here?

# Get the top hint, plus two random hints.
n_hints = len(local_hints[answer])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can refactor this a bit to make it more redable. For example, in the code below there are many instances of local_hints[answer]. For another day: it would be nice to have a way to configure how hints get chosen, for example how many random, or change the criteria completely.

best_hint_index = max(local_hints[answer], key=lambda key: local_hints[answer][key][1])
best_hint = local_hints[answer][best_hint_index][0]
if len(local_hints[answer]) == 1:
rand_hint_1 = ''
rand_hint_2 = ''
self.previous_answers += [[answer, [best_hint_index, None, None]]]
elif n_hints == 2:
best_hint = local_hints[answer].values()[0][0]
best_hint_index = local_hints[answer].keys()[0]
rand_hint_1 = local_hints[answer].values()[1][0]
hint_index_1 = local_hints[answer].keys()[1]
rand_hint_2 = ''
self.previous_answers += [[answer, [best_hint_index, hint_index_1, None]]]
else:
(hint_index_1, rand_hint_1), (hint_index_2, rand_hint_2) =\
random.sample(local_hints[answer].items(), 2)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can get a repeated hint here, correct? I mean, there is a chance the best_hint could be picked by the sample. We probably would need something more robust later.

rand_hint_1 = rand_hint_1[0]
rand_hint_2 = rand_hint_2[0]
self.previous_answers += [(answer, (best_hint_index, hint_index_1, hint_index_2))]

return {'best_hint': best_hint,
'rand_hint_1': rand_hint_1,
'rand_hint_2': rand_hint_2,
'answer': answer}

def get_feedback(self, get):
"""
The student got it correct. Ask him to vote on hints, or submit a hint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same comment I made for the docstring of the previous function.


Args:
`get` -- not actually used. (It is assumed that the answer is correct.)
Output keys:
- 'index_to_hints' maps previous answer indices to hints that the user saw earlier.
- 'index_to_answer' maps previous answer indices to the actual answer submitted.
"""
# The student got it right.
# Did he submit at least one wrong answer?
out = ''
if len(self.previous_answers) == 0:
# No. Nothing to do here.
return

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it ok to return none here?

# Make a hint-voting interface for each wrong answer. The student will only
# be allowed to make one vote / submission, but he can choose which wrong answer
# he wants to look at.
# index_to_hints[previous answer #] = [(hint text, hint pk), + ]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a little bit more about what index_to_hints is

index_to_hints = {}
# index_to_answer[previous answer #] = answer text

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above

index_to_answer = {}

# Go through each previous answer, and populate index_to_hints and index_to_answer.
for i in xrange(len(self.previous_answers)):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In python is more common to use the enumerate iterator:

for index, answer in enumerate(self.previous_answers):
...

answer, hints_offered = self.previous_answers[i]
index_to_hints[i] = []
index_to_answer[i] = answer
if answer in self.hints:
# Go through each hint, and add to index_to_hints
for hint_id in hints_offered:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a comment of what this loop is doing, and why. It makes reading it and verifying it easier.

if hint_id is not None:
try:
index_to_hints[i].append((self.hints[answer][str(hint_id)][0], hint_id))
except KeyError:
# Sometimes, the hint that a user saw will have been deleted by the instructor.
continue

return {'index_to_hints': index_to_hints, 'index_to_answer': index_to_answer}

def tally_vote(self, get):
"""
Tally a user's vote on his favorite hint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same comment as docstring for previous function and PEP 257


Args:
`get` -- expected to have the following keys:
'answer': ans_no (index in previous_answers)
'hint': hint_pk
Returns key 'hint_and_votes', a list of (hint_text, #votes) pairs.
"""
if self.user_voted:
return json.dumps({'contents': 'Sorry, but you have already voted!'})
ans_no = int(get['answer'])
hint_no = str(get['hint'])
answer = self.previous_answers[ans_no][0]
# We use temp_dict because we need to do a direct write for the database to update.
temp_dict = self.hints

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a comment of how this is making a copy, XBlock, etc.

temp_dict[answer][hint_no][1] += 1
self.hints = temp_dict
# Don't let the user vote again!
self.user_voted = True

# Return a list of how many votes each hint got.
hint_and_votes = []
for hint_no in self.previous_answers[ans_no][1]:
if hint_no is None:
continue
hint_and_votes.append(temp_dict[answer][str(hint_no)])

# Reset self.previous_answers.
self.previous_answers = []
return {'hint_and_votes': hint_and_votes}

def submit_hint(self, get):
"""
Take a hint submission and add it to the database.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same comment about docstring of previous function


Args:
`get` -- expected to have the following keys:
'answer': answer index in previous_answers
'hint': text of the new hint that the user is adding
Returns a thank-you message.
"""
# Do html escaping. Perhaps in the future do profanity filtering, etc. as well.
hint = escape(get['hint'])
answer = self.previous_answers[int(get['answer'])][0]
# Only allow a student to vote or submit a hint once.
if self.user_voted:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a comment (could be in the docstring) saying that a student is maked as voted when he submits a hint.

return {'message': 'Sorry, but you have already voted!'}
# Add the new hint to self.hints or self.mod_queue. (Awkward because a direct write
# is necessary.)
if self.moderate == 'True':
temp_dict = self.mod_queue
else:
temp_dict = self.hints
if answer in temp_dict:
temp_dict[answer][self.hint_pk] = [hint, 1] # With one vote (the user himself).
else:
temp_dict[answer] = {self.hint_pk: [hint, 1]}
self.hint_pk += 1
if self.moderate == 'True':
self.mod_queue = temp_dict
else:
self.hints = temp_dict

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a comment about XBLocks, starage, etc, as I have mentioned before.

# Mark the user has having voted; reset previous_answers
self.user_voted = True
self.previous_answers = []
return {'message': 'Thank you for your hint!'}


class CrowdsourceHinterDescriptor(CrowdsourceHinterFields, XmlDescriptor):
module_class = CrowdsourceHinterModule

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Add a docstring, even if it is very short.

stores_state = True

@classmethod
def definition_from_xml(cls, xml_object, system):
children = []
for child in xml_object:
try:
children.append(system.process_xml(etree.tostring(child, encoding='unicode')).location.url())
except Exception as e:
log.exception("Unable to load child when parsing CrowdsourceHinter. Continuing...")
if system.error_tracker is not None:
system.error_tracker("ERROR: " + str(e))
continue
return {}, children

def definition_to_xml(self, resource_fs):
xml_object = etree.Element('crowdsource_hinter')
for child in self.get_children():
xml_object.append(
etree.fromstring(child.export_to_xml(resource_fs)))
return xml_object
3 changes: 2 additions & 1 deletion common/lib/xmodule/xmodule/js/src/capa/display.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class @Problem
# maybe preferable to consolidate all dispatches to use FormData
###
check_fd: =>
Logger.log 'problem_check', @answers
Logger.log 'problem_check_file', @answers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you sure we can change this line? It looks like the new name it is the proper one, but I am not sure if there is anything else expecting it to remain the same, specially on the analytics side.


# If there are no file inputs in the problem, we can fall back on @check
if $('input:file').length == 0
Expand Down Expand Up @@ -222,6 +222,7 @@ class @Problem
@el.removeClass 'showed'
else
@gentle_alert response.success
Logger.log 'problem_graded', [@answers, response.contents], @url

reset: =>
Logger.log 'problem_reset', @answers
Expand Down
Loading