Skip to content
This repository has been archived by the owner on Jun 18, 2024. It is now read-only.

Commit

Permalink
Address review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
itsjeyd committed Nov 2, 2015
1 parent 6c1b897 commit 6b87e0a
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 29 deletions.
4 changes: 0 additions & 4 deletions vectordraw/grader.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,6 @@ def grade(self, answer):
return {'ok': False, 'msg': result}
return {'ok': True, 'msg': self.success_message}

def cfn(self, e, ans):
answer = json.loads(json.loads(ans)['answer'])
return self.grade(answer)

def _get_vectors(self, answer):
vectors = {}
for name, props in answer['vectors'].iteritems():
Expand Down
25 changes: 10 additions & 15 deletions vectordraw/static/js/src/vectordraw.js
Original file line number Diff line number Diff line change
Expand Up @@ -608,26 +608,21 @@ function VectorDrawXBlock(runtime, element, init_args) {
}
var state = getInput(vectordraw);
checkXHR = $.post(checkHandlerUrl, JSON.stringify(state))
.success(function(response) {
updateStatus(response);
});
.success(updateStatus);
}

// Initialization logic

$(function ($) {

// Initialize exercise
var vectordraw = new VectorDraw('vectordraw', init_args.settings);
// Initialize exercise
var vectordraw = new VectorDraw('vectordraw', init_args.settings);

// Load user state
if (!_.isEmpty(init_args.user_state)) {
vectordraw.setState(init_args.user_state);
updateStatus(init_args.user_state);
}
// Load user state
if (!_.isEmpty(init_args.user_state)) {
vectordraw.setState(init_args.user_state);
updateStatus(init_args.user_state);
}

// Set up click handlers
$('.action .check', element).on('click', function(e) { checkAnswer(vectordraw); });
// Set up click handlers
$('.action .check', element).on('click', function(e) { checkAnswer(vectordraw); });

});
}
56 changes: 46 additions & 10 deletions vectordraw/vectordraw.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

import json
import logging
import pkg_resources

from xblock.core import XBlock
from xblock.exceptions import JsonHandlerError
from xblock.fields import Scope, Boolean, Dict, Float, Integer, String
from xblock.fragment import Fragment
from xblockutils.resources import ResourceLoader
Expand Down Expand Up @@ -48,14 +48,14 @@ class VectorDrawXBlock(StudioEditableXBlockMixin, XBlock):
)

height = Integer(
display_name="",
display_name="Height",
help="The height of the board in pixels",
default=400,
scope=Scope.content
)

bounding_box_size = Integer(
display_name="",
display_name="Bounding box size",
help=(
"Defines the bounding box height of the graph area. "
"The bounding box width is calculated from the width/height ratio."
Expand Down Expand Up @@ -183,7 +183,12 @@ class VectorDrawXBlock(StudioEditableXBlockMixin, XBlock):
)

# User state

# Dictionary containing vectors and points present on the board when user last clicked "Check",
# as well as checks to perform in order to obtain grade
answer = Dict(scope=Scope.user_state)
# Dictionary that represents result returned by the grader for the most recent answer;
# contains info about correctness of answer and feedback message
result = Dict(scope=Scope.user_state)

editable_fields = (
Expand Down Expand Up @@ -254,11 +259,6 @@ def points_json(self):
def expected_result_json(self):
return json.loads(self.expected_result)

def resource_string(self, path):
"""Handy helper for getting resources from our kit."""
data = pkg_resources.resource_string(__name__, path)
return data.decode("utf8")

def student_view(self, context=None):
"""
The primary view of the VectorDrawXBlock, shown to students
Expand All @@ -268,17 +268,53 @@ def student_view(self, context=None):
fragment = Fragment()
fragment.add_content(loader.render_template('static/html/vectordraw.html', context))
fragment.add_css_url("//cdnjs.cloudflare.com/ajax/libs/font-awesome/4.3.0/css/font-awesome.min.css")
fragment.add_css(self.resource_string('static/css/vectordraw.css'))
fragment.add_css(loader.load_unicode('static/css/vectordraw.css'))
fragment.add_javascript_url("//cdnjs.cloudflare.com/ajax/libs/jsxgraph/0.98/jsxgraphcore.js")
fragment.add_javascript(self.resource_string("static/js/src/vectordraw.js"))
fragment.add_javascript(loader.load_unicode("static/js/src/vectordraw.js"))
fragment.initialize_js('VectorDrawXBlock', {"settings": self.settings, "user_state": self.user_state})
return fragment

def is_valid(self, data):

This comment has been minimized.

Copy link
@smarnach

smarnach Nov 3, 2015

This is really thorough! I wasn't even thinking of that kind of detailed validation when I commented. Just running all graders over the data without erroring out might already be a sufficient level of validity, but the explicit tests are nice as well.

This name is a bit too generic for something used internally in one specific handler. How about _validate_check_answer_data() or something like that?

"""
Validate answer data submitted by user.
"""
# Check vectors
vectors = data.get('vectors')
if vectors is None:
return False

This comment has been minimized.

Copy link
@smarnach

smarnach Nov 3, 2015

I don't think this test is necessary. If someone sends you a broken request to an AJAX handler, I find it acceptable for them to get back a 500. If you decide to keep it, change it to

if not isinstance(vectors, dict):

since otherwise the next line will error out.

(What's not acceptable is to allow the broken request to break data consistency, or cause other errors later on. This is the reason why I requested the validation. But the single broken request is allowed to fail.)

for vector_data in vectors.values():
# Validate vector

This comment has been minimized.

Copy link
@smarnach

smarnach Nov 3, 2015

In the same spirit, you could write the loop body as

unused_x, unused_y = vector_data['tail']
unused_x, unused_y = vector_data['tip']

This will simply error out if anything is wrong, which I think is ok. You could even catch the exceptions in the handler and serve a 400 anyway if you want to be nice.

vector_valid = 'tip' in vector_data and 'tail' in vector_data
if not vector_valid:

This comment has been minimized.

Copy link
@smarnach

smarnach Nov 3, 2015

How about

if vector_data.viewkeys() != {'tail', 'tip'}:
    return False

to test for the exact keys?

This comment has been minimized.

Copy link
@itsjeyd

itsjeyd Nov 4, 2015

Author Member

@smarnach Didn't know about dict.viewkeys -- nice!

return False
# Validate tip and tail
tip = vector_data['tip']
tip_valid = type(tip) == list and len(tip) == 2

This comment has been minimized.

Copy link
@smarnach

smarnach Nov 3, 2015

Style issue: PEP8 says it's isinstance(tip, list) instead of type(tip) == list.

tail = vector_data['tail']
tail_valid = type(tail) == list and len(tail) == 2
if not (tip_valid and tail_valid):
return False
# Check points
points = data.get('points')
if points is None:
return False
for coords in points.values():

This comment has been minimized.

Copy link
@smarnach

smarnach Nov 3, 2015

Analogously to the above suggestion, this could be wriiten as

for unused_x, unused_y in points.values():
    pass
# Validate point
point_valid = type(coords) == list and len(coords) == 2
if not point_valid:
break

This comment has been minimized.

Copy link
@smarnach

smarnach Nov 3, 2015

If you decide to keep the current version, this should become

return False

(I definitely added too many comments to this function. Sorry. I appreciate that you wrote it. :) )

This comment has been minimized.

Copy link
@itsjeyd

itsjeyd Nov 4, 2015

Author Member

@smarnach No worries, I appreciate the detailed feedback :)

# If we get to this point, it means that vector and point data is valid;
# the only thing left to check is whether data contains a list of checks:
return 'checks' in data

@XBlock.json_handler
def check_answer(self, data, suffix=''):
"""
Check and persist student's answer to this vector drawing problem.
"""
# Validate data
if not self.is_valid(data):
raise JsonHandlerError(400, "Invalid data")
# Save answer
self.answer = dict(
vectors=data["vectors"],
Expand Down

0 comments on commit 6b87e0a

Please sign in to comment.