Skip to content

E
edx-ora2
Commit fd435437 by Will Daly
Options

Merge pull request #253 from edx/will/relax-date-validation 

Gracefully handle Studio authors setting invalid problem dates
parents 9dfb1928 14ca6c25
Showing with 260 additions and 54 deletions
apps/openassessment/xblock/openassessmentblock.py
......@@ -177,6 +177,11 @@ class OpenAssessmentBlock(
LmsCompatibilityMixin):
"""Displays a question and gives an area where students can compose a response."""
submission_start = String(
default=None, scope=Scope.settings,
help="ISO-8601 formatted string representing the submission start date."
)
submission_due = String(
default=None, scope=Scope.settings,
help="ISO-8601 formatted string representing the submission due date."
......@@ -483,7 +488,7 @@ class OpenAssessmentBlock(
True, "start", datetime.datetime(2014, 3, 27, 22, 7, 38, 788861), datetime.datetime(2015, 3, 27, 22, 7, 38, 788861)
"""
submission_range = (self.start, self.submission_due)
submission_range = (self.submission_start, self.submission_due)
assessment_ranges = [
(asmnt.get('start'), asmnt.get('due'))
for asmnt in self.rubric_assessments
......
apps/openassessment/xblock/resolve_dates.py
......@@ -62,6 +62,27 @@ def resolve_dates(start, end, date_ranges):
(The first submission defaults to the problem start date.)
4) Unset end dates default to the end date of the following assessment/submission.
(The last assessment defaults to the problem end date.)
5) `start` resolves to the earliest start date.
6) `end` resolves to the latest end date.
7) If `start` is later than `end`, move `start` to just before `end`.
Overriding start/end dates:
* Rules 5, 6, and 7 may seem strange, but they're necessary. Unlike `date_ranges`,
the `start` and `end` values are inherited by the XBlock from the LMS.
This means that you can set `start` and `end` in Studio, effectively bypassing
our validation rules.
* On the other hand, we *need* the start/due dates so we can resolve unspecified
date ranges to an actual date. For example,
if the problem closes on April 15th, 2014, but the course author hasn't specified
a due date for a submission, we need ensure the submission closes on April 15th.
* For this reason, we use `start` and `end` only if they satisfy our validation
rules. If not (because a course author has changed them to something invalid in Studio),
we use the dates that the course author specified in the problem definition,
which (a) MUST satisfy our ordering constraints, and (b) are probably
what the author intended.
Example:
......@@ -117,12 +138,31 @@ def resolve_dates(start, end, date_ranges):
resolved_starts = []
resolved_ends = []
# Validate the problem start/end dates
# Amazingly, Studio allows the release date to be after the due date!
# This can cause a problem if the course author has configured:
#
# 1) Problem start >= problem due, and
# 2) Start/due dates that resolve to the problem start/due date.
#
# In this case, all submission/assessment start dates
# could default to the problem start while
# due dates default to the problem due date, violating
# the constraint that start dates always precede due dates.
# If we detect that the author has done this,
# we set the start date to just before
# the due date, so we (just barely) satify the validation rules.
if start >= end:
msg = _(u"Problem start date '{start}' cannot be later than the problem due date '{due}'.").format(
start=start, due=end
)
raise DateValidationError(msg)
start = end - dt.timedelta(milliseconds=1)
# Override start/end dates if they fail to satisfy our validation rules
# These are the only parameters a course author can change in Studio
# without triggering our validation rules, so we need to use sensible
# defaults. See the docstring above for a more detailed justification.
for step_start, step_end in date_ranges:
if step_start is not None:
start = min(start, _parse_date(step_start))
if step_end is not None:
end = max(end, _parse_date(step_end))
# Iterate through the list forwards and backwards simultaneously
# As we iterate forwards, resolve start dates.
......@@ -172,4 +212,4 @@ def resolve_dates(start, end, date_ranges):
)
raise DateValidationError(msg)
return start, end, resolved_ranges
\ No newline at end of file
return start, end, resolved_ranges
apps/openassessment/xblock/test/data/invalid_dates.json
{
"xblock_start_past_xblock_due": {
"xblock_start": 11,
"submission_start": 1,
"submission_due": 2,
"peer_start": 3,
"peer_due": 4,
"self_start": 5,
"self_due": 6,
"xblock_due": 10
},
"xblock_start_equals_xblock_due": {
"xblock_start": 10,
"submission_start": 1,
"submission_due": 2,
"peer_start": 3,
"peer_due": 4,
"self_start": 5,
"self_due": 6,
"xblock_due": 10
},
"submission_start_past_submission_due": {
"xblock_start": 0,
"submission_start": 3,
......@@ -79,16 +59,6 @@
"self_due": 6,
"xblock_due": 10
},
"xblock_start_past_submission_start": {
"xblock_start": 2,
"submission_start": 1,
"submission_due": 3,
"peer_start": 4,
"peer_due": 5,
"self_start": 6,
"self_due": 7,
"xblock_due": 10
},
"submission_start_past_peer_start": {
"xblock_start": 0,
"submission_start": 4,
......@@ -109,16 +79,6 @@
"self_due": 7,
"xblock_due": 10
},
"xblock_due_before_self_due": {
"xblock_start": 0,
"submission_start": 1,
"submission_due": 2,
"peer_start": 3,
"peer_due": 4,
"self_start": 5,
"self_due": 7,
"xblock_due": 6
},
"self_due_before_peer_due": {
"xblock_start": 0,
"submission_start": 1,
......
apps/openassessment/xblock/test/data/serialize.json
......@@ -4,6 +4,7 @@
"prompt": "Test prompt",
"start": null,
"due": null,
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -65,6 +66,7 @@
"prompt": "Ṫëṡẗ ṗṛöṁṗẗ",
"start": null,
"due": null,
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -120,6 +122,7 @@
"prompt": "Test prompt",
"start": null,
"due": null,
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -175,6 +178,7 @@
"prompt": "Test prompt",
"start": null,
"due": null,
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -248,6 +252,7 @@
"prompt": "Test prompt",
"start": null,
"due": null,
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -311,6 +316,7 @@
"prompt": "Test prompt",
"start": "2010-04-01T00:00:00",
"due": "2030-05-01T00:00:00",
"submission_start": null,
"submission_due": "2020-04-15T00:00:00",
"criteria": [
{
......
apps/openassessment/xblock/test/data/update_from_xml.json
......@@ -22,6 +22,7 @@
"prompt": "Test prompt",
"start": "2000-01-01T00:00:00",
"due": "3000-01-01T00:00:00",
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -81,6 +82,7 @@
"title": "िѻѻ",
"start": "2000-01-01T00:00:00",
"due": "3000-01-01T00:00:00",
"submission_start": null,
"submission_due": null,
"prompt": "ՇєรՇ קг๏๓קՇ",
"criteria": [
......@@ -142,6 +144,7 @@
"prompt": "Test prompt",
"start": "2000-01-01T00:00:00",
"due": "3000-01-01T00:00:00",
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -210,6 +213,7 @@
"prompt": "Test prompt",
"start": "2000-01-01T00:00:00",
"due": "3000-01-01T00:00:00",
"submission_start": null,
"submission_due": null,
"criteria": [
{
......@@ -265,6 +269,7 @@
"prompt": "Test prompt",
"start": "2000-01-01T00:00:00",
"due": "3000-01-01T00:00:00",
"submission_start": null,
"submission_due": "2014-01-01T00:00:00",
"criteria": [
{
......@@ -296,5 +301,61 @@
"must_be_graded_by": 3
}
]
},
"submission_start": {
"xml": [
"<openassessment submission_start=\"2000-01-02T00:00:00\">",
"<title>Foo</title>",
"<assessments>",
"<assessment name=\"peer-assessment\" must_grade=\"5\" must_be_graded_by=\"3\" />",
"</assessments>",
"<rubric>",
"<prompt>Test prompt</prompt>",
"<criterion>",
"<name>Test criterion</name>",
"<prompt>Test criterion prompt</prompt>",
"<option points=\"0\"><name>No</name><explanation>No explanation</explanation></option>",
"<option points=\"2\"><name>Yes</name><explanation>Yes explanation</explanation></option>",
"</criterion>",
"</rubric>",
"</openassessment>"
],
"title": "Foo",
"prompt": "Test prompt",
"start": "2000-01-01T00:00:00",
"due": "3000-01-01T00:00:00",
"submission_start": "2000-01-02T00:00:00",
"submission_due": null,
"criteria": [
{
"order_num": 0,
"name": "Test criterion",
"prompt": "Test criterion prompt",
"options": [
{
"order_num": 0,
"points": 0,
"name": "No",
"explanation": "No explanation"
},
{
"order_num": 1,
"points": 2,
"name": "Yes",
"explanation": "Yes explanation"
}
]
}
],
"assessments": [
{
"name": "peer-assessment",
"start": null,
"due": null,
"must_grade": 5,
"must_be_graded_by": 3
}
]
}
}
apps/openassessment/xblock/test/data/update_from_xml_error.json
......@@ -183,6 +183,25 @@
]
},
"invalid_submission_start_date": {
"xml": [
"<openassessment submission_start=\"non-date\">",
"<title>Foo</title>",
"<assessments>",
"<assessment name=\"peer-assessment\" start=\"2014-03-01T00:00:00\" must_grade=\"2\" must_be_graded_by=\"5\" />",
"</assessments>",
"<rubric>",
"<prompt>Test prompt</prompt>",
"<criterion>",
"<name>Test criterion</name>",
"<prompt>Test criterion prompt</prompt>",
"<option points=\"5\"><name>Yes</name><explanation>Yes explanation</explanation></option>",
"</criterion>",
"</rubric>",
"</openassessment>"
]
},
"missing_rubric_prompt": {
"xml": [
"<openassessment>",
......
apps/openassessment/xblock/test/data/valid_dates.json
......@@ -98,5 +98,45 @@
"peer_due": null,
"self_start": null,
"self_due": null
},
"xblock_due_before_self_due": {
"xblock_start": 0,
"submission_start": 1,
"submission_due": 2,
"peer_start": 3,
"peer_due": 4,
"self_start": 5,
"self_due": 7,
"xblock_due": 6
},
"xblock_start_equals_xblock_due": {
"xblock_start": 10,
"submission_start": 1,
"submission_due": 2,
"peer_start": 3,
"peer_due": 4,
"self_start": 5,
"self_due": 6,
"xblock_due": 10
},
"xblock_start_past_submission_start": {
"xblock_start": 2,
"submission_start": 1,
"submission_due": 3,
"peer_start": 4,
"peer_due": 5,
"self_start": 6,
"self_due": 7,
"xblock_due": 10
},
"xblock_start_past_xblock_due": {
"xblock_start": 11,
"submission_start": 1,
"submission_due": 2,
"peer_start": 3,
"peer_due": 4,
"self_start": 5,
"self_due": 6,
"xblock_due": 10
}
}
apps/openassessment/xblock/test/test_resolve_dates.py
......@@ -45,4 +45,55 @@ class ResolveDatesTest(TestCase):
(self.DATES[start], self.DATES[end])
for start, end in tuple(data['resolved_ranges'])
]
)
\ No newline at end of file
)
def test_min_start_date(self):
# Start date should resolve to the min of all start dates
# See the detailed comment in the docstring of `resolve_dates`
# for the reasoning behind this.
resolved_start, __, __ = resolve_dates(
"2013-01-01", None,
[
("1999-01-01", "1999-02-03"),
("2003-01-01", "2003-02-03"),
("3234-01-01", "3234-02-03"),
]
)
# Should default to the min of all specified start dates
self.assertEqual(
resolved_start,
datetime.datetime(1999, 1, 1).replace(tzinfo=pytz.UTC)
)
def test_max_due_date(self):
# End date should resolve to the max of all end dates
# See the detailed comment in the docstring of `resolve_dates`
# for the reasoning behind this.
__, resolved_end, __ = resolve_dates(
None, "2013-01-01",
[
("1999-01-01", "1999-02-03"),
("2003-01-01", "2003-02-03"),
("3234-01-01", "3234-02-03"),
]
)
# Should default to the max of all specified end dates
self.assertEqual(
resolved_end,
datetime.datetime(3234, 2, 3).replace(tzinfo=pytz.UTC)
)
def test_start_greater_than_end(self):
# Handle the special case in which the problem's release
# date is after the problem's start date, and we've
# specified only one deadline.
resolve_dates(
"2040-01-01", "2013-01-02",
[
(None, "2014-08-01"),
(None, None),
(None, None)
]
)
apps/openassessment/xblock/test/test_validation.py
......@@ -68,6 +68,15 @@ class DateValidationTest(TestCase):
}
self.DATES[None] = None
# There are a few test cases here that might seem incorrect:
# * xblock_due_before_self_due
# * xblock_start_equals_xblock_due
# * xblock_start_past_submission_start
# * xblock_start_past_xblock_due
#
# We count these as valid because the start/due date are inherited
# from the LMS, thus bypassing our validation rules.
# See the docstring for `resolve_dates` for a more detailed justification.
@ddt.file_data('data/valid_dates.json')
def test_valid_dates(self, data):
......
apps/openassessment/xblock/test/test_xml.py
......@@ -82,6 +82,7 @@ class TestSerializeContent(TestCase):
self.oa_block.prompt = data['prompt']
self.oa_block.start = _parse_date(data['start'])
self.oa_block.due = _parse_date(data['due'])
self.oa_block.submission_start = data['submission_start']
self.oa_block.submission_due = data['submission_due']
self.oa_block.rubric_criteria = data['criteria']
self.oa_block.rubric_assessments = data['assessments']
......@@ -132,6 +133,7 @@ class TestSerializeContent(TestCase):
self.oa_block.rubric_assessments = self.BASIC_ASSESSMENTS
self.oa_block.start = None
self.oa_block.due = None
self.oa_block.submission_start = None
self.oa_block.submission_due = None
# We have to be really permissive with the data we'll accept.
......@@ -156,6 +158,7 @@ class TestSerializeContent(TestCase):
self.oa_block.rubric_criteria = self.BASIC_CRITERIA
self.oa_block.start = None
self.oa_block.due = None
self.oa_block.submission_start = None
self.oa_block.submission_due = None
for assessment_dict in self.BASIC_ASSESSMENTS:
......@@ -169,12 +172,13 @@ class TestSerializeContent(TestCase):
msg = "Could not parse mutated assessment dict {assessment}\n{ex}".format(assessment=mutated_dict, ex=ex)
self.fail(msg)
@data("title", "prompt", "start", "due", "submission_due")
@data("title", "prompt", "start", "due", "submission_due", "submission_start")
def test_mutated_field(self, field):
self.oa_block.rubric_criteria = self.BASIC_CRITERIA
self.oa_block.rubric_assessments = self.BASIC_ASSESSMENTS
self.oa_block.start = None
self.oa_block.due = None
self.oa_block.submission_start = None
self.oa_block.submission_due = None
for mutated_value in [0, u"\u9282", None]:
......@@ -300,6 +304,7 @@ class TestUpdateFromXml(TestCase):
self.oa_block.start = dt.datetime(2000, 1, 1).replace(tzinfo=pytz.utc)
self.oa_block.due = dt.datetime(3000, 1, 1).replace(tzinfo=pytz.utc)
self.oa_block.submission_start = "2000-01-01T00:00:00"
self.oa_block.submission_due = "2000-01-01T00:00:00"
@file_data('data/update_from_xml.json')
......@@ -316,6 +321,7 @@ class TestUpdateFromXml(TestCase):
self.assertEqual(self.oa_block.prompt, data['prompt'])
self.assertEqual(self.oa_block.start, _parse_date(data['start']))
self.assertEqual(self.oa_block.due, _parse_date(data['due']))
self.assertEqual(self.oa_block.submission_start, data['submission_start'])
self.assertEqual(self.oa_block.submission_due, data['submission_due'])
self.assertEqual(self.oa_block.rubric_criteria, data['criteria'])
self.assertEqual(self.oa_block.rubric_assessments, data['assessments'])
......@@ -334,4 +340,4 @@ class TestUpdateFromXml(TestCase):
update_from_xml_str(
self.oa_block, "".join(data['xml']),
validator=lambda *args: (False, '')
)
\ No newline at end of file
)
apps/openassessment/xblock/xml.py
......@@ -379,6 +379,10 @@ def serialize_content_to_xml(oa_block, root):
"""
root.tag = 'openassessment'
# Set the submission start date
if oa_block.submission_start is not None:
root.set('submission_start', unicode(oa_block.submission_start))
# Set submission due date
if oa_block.submission_due is not None:
root.set('submission_due', unicode(oa_block.submission_due))
......@@ -464,8 +468,15 @@ def update_from_xml(oa_block, root, validator=DEFAULT_VALIDATOR):
if root.tag != 'openassessment':
raise UpdateFromXmlError(_("XML content must contain an 'openassessment' root element."))
# Retrieve the start date for the submission
# Set it to None by default; we will update it to the latest start date later on
submission_start = None
if 'submission_start' in root.attrib:
submission_start = _parse_date(unicode(root.attrib['submission_start']))
if submission_start is None:
raise UpdateFromXmlError(_("Invalid date format for submission start date"))
# Retrieve the due date for the submission
# (assume that the start date of submission is the same as the start date of the problem)
# Set it to None by default; we will update it to the earliest deadline later on
submission_due = None
if 'submission_due' in root.attrib:
......@@ -505,6 +516,7 @@ def update_from_xml(oa_block, root, validator=DEFAULT_VALIDATOR):
oa_block.prompt = rubric['prompt']
oa_block.rubric_criteria = rubric['criteria']
oa_block.rubric_assessments = assessments
oa_block.submission_start = submission_start
oa_block.submission_due = submission_due
return oa_block
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment