Commit d08cdd80 by Stephen Sanchez

Avoid creating duplicate workflow items

Conflicts:
	openassessment/xblock/static/js/openassessment.min.js

Update minified JS

adding a new unit test to verify the correct number of peer render calls

Updating workflow item 'find' functions to exclude duplicate items
parent f7fffa30
...@@ -249,7 +249,7 @@ def create_assessment( ...@@ -249,7 +249,7 @@ def create_assessment(
try: try:
# Retrieve workflow information # Retrieve workflow information
scorer_workflow = PeerWorkflow.objects.get(submission_uuid=scorer_submission_uuid) scorer_workflow = PeerWorkflow.objects.get(submission_uuid=scorer_submission_uuid)
peer_workflow_item = scorer_workflow.get_latest_open_workflow_item() peer_workflow_item = scorer_workflow.find_active_assessments()
if peer_workflow_item is None: if peer_workflow_item is None:
message = ( message = (
u"There are no open assessments associated with the scorer's " u"There are no open assessments associated with the scorer's "
...@@ -257,7 +257,7 @@ def create_assessment( ...@@ -257,7 +257,7 @@ def create_assessment(
).format(scorer_submission_uuid) ).format(scorer_submission_uuid)
logger.warning(message) logger.warning(message)
raise PeerAssessmentWorkflowError(message) raise PeerAssessmentWorkflowError(message)
peer_submission_uuid = peer_workflow_item.author.submission_uuid peer_submission_uuid = peer_workflow_item.submission_uuid
assessment = _complete_assessment( assessment = _complete_assessment(
rubric_dict, rubric_dict,
...@@ -666,7 +666,8 @@ def get_submission_to_assess(submission_uuid, graded_by): ...@@ -666,7 +666,8 @@ def get_submission_to_assess(submission_uuid, graded_by):
u"A Peer Assessment Workflow does not exist for the student " u"A Peer Assessment Workflow does not exist for the student "
u"with submission UUID {}".format(submission_uuid) u"with submission UUID {}".format(submission_uuid)
) )
peer_submission_uuid = workflow.find_active_assessments() open_item = workflow.find_active_assessments()
peer_submission_uuid = open_item.submission_uuid if open_item else None
# If there is an active assessment for this user, get that submission, # If there is an active assessment for this user, get that submission,
# otherwise, get the first assessment for review, otherwise, # otherwise, get the first assessment for review, otherwise,
# get the first submission available for over grading ("over-grading"). # get the first submission available for over grading ("over-grading").
......
...@@ -216,13 +216,29 @@ class PeerWorkflow(models.Model): ...@@ -216,13 +216,29 @@ class PeerWorkflow(models.Model):
for this PeerWorkflow. for this PeerWorkflow.
Returns: Returns:
submission_uuid (str): The submission_uuid for the submission that the (PeerWorkflowItem) The PeerWorkflowItem for the submission that the
student has open for active assessment. student has open for active assessment.
""" """
oldest_acceptable = now() - self.TIME_LIMIT oldest_acceptable = now() - self.TIME_LIMIT
workflows = self.graded.filter(assessment__isnull=True, started_at__gt=oldest_acceptable) # pylint:disable=E1101 items = list(self.graded.all().order_by("-started_at", "-id"))
return workflows[0].submission_uuid if workflows else None valid_open_items = []
completed_sub_uuids = []
# First, remove all completed items.
for item in items:
if item.assessment is not None:
completed_sub_uuids.append(item.submission_uuid)
else:
valid_open_items.append(item)
# Remove any open items which have a submission which has been completed.
for item in valid_open_items:
if (item.started_at < oldest_acceptable or
item.submission_uuid in completed_sub_uuids):
valid_open_items.remove(item)
return valid_open_items[0] if valid_open_items else None
def get_submission_for_review(self, graded_by): def get_submission_for_review(self, graded_by):
""" """
...@@ -331,19 +347,6 @@ class PeerWorkflow(models.Model): ...@@ -331,19 +347,6 @@ class PeerWorkflow(models.Model):
logger.exception(error_message) logger.exception(error_message)
raise PeerAssessmentInternalError(error_message) raise PeerAssessmentInternalError(error_message)
def get_latest_open_workflow_item(self):
"""
Return the latest open workflow item for this workflow.
Returns:
A PeerWorkflowItem that is open for assessment.
None if no item is found.
"""
workflow_query = self.graded.filter(assessment__isnull=True).order_by("-started_at", "-id") # pylint:disable=E1101
items = list(workflow_query[:1])
return items[0] if items else None
def close_active_assessment(self, submission_uuid, assessment, num_required_grades): def close_active_assessment(self, submission_uuid, assessment, num_required_grades):
""" """
Updates a workflow item on the student's workflow with the associated Updates a workflow item on the student's workflow with the associated
......
...@@ -151,7 +151,7 @@ class TestPeerApi(CacheResetTest): ...@@ -151,7 +151,7 @@ class TestPeerApi(CacheResetTest):
Tests for the peer assessment API functions. Tests for the peer assessment API functions.
""" """
CREATE_ASSESSMENT_NUM_QUERIES = 59 CREATE_ASSESSMENT_NUM_QUERIES = 58
def test_create_assessment_points(self): def test_create_assessment_points(self):
self._create_student_and_submission("Tim", "Tim's answer") self._create_student_and_submission("Tim", "Tim's answer")
...@@ -879,8 +879,8 @@ class TestPeerApi(CacheResetTest): ...@@ -879,8 +879,8 @@ class TestPeerApi(CacheResetTest):
PeerWorkflow.create_item(buffy_workflow, xander_answer["uuid"]) PeerWorkflow.create_item(buffy_workflow, xander_answer["uuid"])
# Check to see if Buffy is still actively reviewing Xander's submission. # Check to see if Buffy is still actively reviewing Xander's submission.
submission_uuid = buffy_workflow.find_active_assessments() item = buffy_workflow.find_active_assessments()
self.assertEqual(xander_answer["uuid"], submission_uuid) self.assertEqual(xander_answer["uuid"], item.submission_uuid)
def test_get_workflow_by_uuid(self): def test_get_workflow_by_uuid(self):
buffy_answer, _ = self._create_student_and_submission("Buffy", "Buffy's answer") buffy_answer, _ = self._create_student_and_submission("Buffy", "Buffy's answer")
...@@ -1212,6 +1212,70 @@ class TestPeerApi(CacheResetTest): ...@@ -1212,6 +1212,70 @@ class TestPeerApi(CacheResetTest):
MONDAY, MONDAY,
) )
def test_ignore_duplicate_workflow_items(self):
"""
A race condition may cause two workflow items to be opened for a single
submission. In this case, we want to be defensive in the API, such that
no open workflow item is acknowledged if an assessment has already been
made against the associated submission.
"""
bob_sub, bob = self._create_student_and_submission('Bob', 'Bob submission')
tim_sub, tim = self._create_student_and_submission('Tim', 'Tim submission')
sally_sub, sally = self._create_student_and_submission('Sally', 'Sally submission')
jane_sub, jane = self._create_student_and_submission('Jane', 'Jane submission')
# Create two workflow items.
peer_api.create_peer_workflow_item(bob_sub['uuid'], tim_sub['uuid'])
peer_api.create_peer_workflow_item(bob_sub['uuid'], tim_sub['uuid'])
# Assess the submission, then get the next submission.
peer_api.create_assessment(
bob_sub['uuid'],
bob['student_id'],
ASSESSMENT_DICT['options_selected'],
ASSESSMENT_DICT['criterion_feedback'],
ASSESSMENT_DICT['overall_feedback'],
RUBRIC_DICT,
REQUIRED_GRADED_BY,
MONDAY
)
# Verify the next submission is not Tim again, but Sally.
next_sub = peer_api.get_submission_to_assess(bob_sub['uuid'], REQUIRED_GRADED_BY)
self.assertEqual(next_sub['uuid'], sally_sub['uuid'])
# Request another peer submission. Should pick up Sally again.
next_sub = peer_api.get_submission_to_assess(bob_sub['uuid'], REQUIRED_GRADED_BY)
self.assertEqual(next_sub['uuid'], sally_sub['uuid'])
# Ensure that the next assessment made is against Sally, not Tim.
# Assess the submission, then get the next submission.
peer_api.create_assessment(
bob_sub['uuid'],
bob['student_id'],
ASSESSMENT_DICT['options_selected'],
ASSESSMENT_DICT['criterion_feedback'],
ASSESSMENT_DICT['overall_feedback'],
RUBRIC_DICT,
REQUIRED_GRADED_BY,
MONDAY
)
# Make sure Tim has one assessment.
tim_assessments = peer_api.get_assessments(tim_sub['uuid'], scored_only=False)
self.assertEqual(1, len(tim_assessments))
# Make sure Sally has one assessment.
sally_assessments = peer_api.get_assessments(sally_sub['uuid'], scored_only=False)
self.assertEqual(1, len(sally_assessments))
# Make sure Jane has no assessment.
jane_assessments = peer_api.get_assessments(jane_sub['uuid'], scored_only=False)
self.assertEqual(0, len(jane_assessments))
def test_get_submission_to_assess_no_workflow(self): def test_get_submission_to_assess_no_workflow(self):
# Try to retrieve a submission to assess when the student # Try to retrieve a submission to assess when the student
# doing the assessment hasn't yet submitted. # doing the assessment hasn't yet submitted.
......
...@@ -26,6 +26,16 @@ describe("OpenAssessment.BaseView", function() { ...@@ -26,6 +26,16 @@ describe("OpenAssessment.BaseView", function() {
defer.resolveWith(this, [server.fragments[component]]); defer.resolveWith(this, [server.fragments[component]]);
}).promise(); }).promise();
}; };
var successPromise = $.Deferred(
function(defer) {
defer.resolve();
}
).promise();
this.peerAssess = function(optionsSelected, feedback) {
return successPromise;
};
}; };
// Stub runtime // Stub runtime
...@@ -76,4 +86,20 @@ describe("OpenAssessment.BaseView", function() { ...@@ -76,4 +86,20 @@ describe("OpenAssessment.BaseView", function() {
expect(server.fragmentsLoaded).toContain("grade"); expect(server.fragmentsLoaded).toContain("grade");
}); });
}); });
it("Only load the peer section once on submit", function() {
loadSubviews(function() {
// Simulate a server error
view.peerView.peerAssess();
var numPeerLoads = 0;
for (var i = 0; i < server.fragmentsLoaded.length; i++) {
if (server.fragmentsLoaded[i] == 'peer_assessment') {
numPeerLoads++;
}
}
// Peer should be called twice, once when loading the views,
// and again after the peer has been assessed.
expect(numPeerLoads).toBe(2);
});
});
}); });
...@@ -158,7 +158,6 @@ OpenAssessment.PeerView.prototype = { ...@@ -158,7 +158,6 @@ OpenAssessment.PeerView.prototype = {
var view = this; var view = this;
var baseView = view.baseView; var baseView = view.baseView;
this.peerAssessRequest(function() { this.peerAssessRequest(function() {
view.load();
baseView.loadAssessmentModules(); baseView.loadAssessmentModules();
baseView.scrollToTop(); baseView.scrollToTop();
}); });
......
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