Merge pull request #269 from edx/will/reset-scores-2

Reset scores (null submission approach)

apps/openassessment/xblock/grade_mixin.py

@@ -76,8 +76,14 @@ class GradeMixin(object):
         self_assessment = self_api.get_assessment(student_submission['uuid'])
         has_submitted_feedback = peer_api.get_assessment_feedback(workflow['submission_uuid']) is not None
 
+        # We retrieve the score from the workflow, which in turn retrieves
+        # the score for our current submission UUID.
+        # We look up the score by submission UUID instead of student item
+        # to ensure that the score always matches the rubric.
+        score = workflow['score']
+
         context = {
-            'score': workflow['score'],
+            'score': score,
             'feedback_text': feedback_text,
             'student_submission': student_submission,
             'peer_assessments': peer_assessments,

apps/submissions/api.py

@@ -370,7 +370,12 @@ def get_score(student_item):
     except (ScoreSummary.DoesNotExist, StudentItem.DoesNotExist):
         return None
 
-    return ScoreSerializer(score).data
+    # By convention, scores are hidden if "points possible" is set to 0.
+    # This can occur when an instructor has reset scores for a student.
+    if score.is_hidden():
+        return None
+    else:
+        return ScoreSerializer(score).data
 
 
 def get_scores(course_id, student_id):
@@ -379,6 +384,9 @@ def get_scores(course_id, student_id):
     This method would be used by an LMS to find all the scores for a given
     student in a given course.
 
+    Scores that are "hidden" (because they have points earned set to zero)
+    are excluded from the results.
+
     Args:
         course_id (str): Course ID, used to do a lookup on the `StudentItem`.
         student_id (str): Student ID, used to do a lookup on the `StudentItem`.
@@ -391,6 +399,9 @@ def get_scores(course_id, student_id):
         `student_id`, we simply return an empty dictionary. This is not
         considered an error because there might be many queries for the progress
         page of a person who has never submitted anything.
+
+    Raises:
+        SubmissionInternalError: An unexpected error occurred while resetting scores.
     """
     try:
         score_summaries = ScoreSummary.objects.filter(
@@ -407,21 +418,81 @@ def get_scores(course_id, student_id):
         summary.student_item.item_id:
             (summary.latest.points_earned, summary.latest.points_possible)
         for summary in score_summaries
+        if not summary.latest.is_hidden()
     }
     return scores
 
 
 def get_latest_score_for_submission(submission_uuid):
+    """
+    Retrieve the latest score for a particular submission.
+
+    Args:
+        submission_uuid (str): The UUID of the submission to retrieve.
+
+    Returns:
+        dict: The serialized score model, or None if no score is available.
+
+    """
     try:
         score = Score.objects.filter(
             submission__uuid=submission_uuid
         ).order_by("-id").select_related("submission")[0]
+        if score.is_hidden():
+            return None
     except IndexError:
         return None
 
     return ScoreSerializer(score).data
 
 
+def reset_score(student_id, course_id, item_id):
+    """
+    Reset scores for a specific student on a specific problem.
+
+    Note: this does *not* delete `Score` models from the database,
+    since these are immutable.  It simply creates a new score with
+    the "reset" flag set to True.
+
+    Args:
+        student_id (unicode): The ID of the student for whom to reset scores.
+        course_id (unicode): The ID of the course containing the item to reset.
+        item_id (unicode): The ID of the item for which to reset scores.
+
+    Returns:
+        None
+
+    Raises:
+        SubmissionInternalError: An unexpected error occurred while resetting scores.
+
+    """
+    # Retrieve the student item
+    try:
+        student_item = StudentItem.objects.get(
+            student_id=student_id, course_id=course_id, item_id=item_id
+        )
+    except StudentItem.DoesNotExist:
+        # If there is no student item, then there is no score to reset,
+        # so we can return immediately.
+        return
+
+    # Create a "reset" score
+    try:
+        Score.create_reset_score(student_item)
+    except DatabaseError:
+        msg = (
+            u"Error occurred while reseting scores for"
+            u" item {item_id} in course {course_id} for student {student_id}"
+        ).format(item_id=item_id, course_id=course_id, student_id=student_id)
+        logger.exception(msg)
+        raise SubmissionInternalError(msg)
+    else:
+        msg = u"Score reset for item {item_id} in course {course_id} for student {student_id}".format(
+            item_id=item_id, course_id=course_id, student_id=student_id
+        )
+        logger.info(msg)
+
+
 def set_score(submission_uuid, points_earned, points_possible):
     """Set a score for a particular submission.
 
@@ -429,9 +500,6 @@ def set_score(submission_uuid, points_earned, points_possible):
     externally to the API.
 
     Args:
-        student_item (dict): The student item associated with this score. This
-            dictionary must contain a course_id, student_id, and item_id.
-        submission_uuid (str): The submission associated with this score.
         submission_uuid (str): UUID for the submission (must exist).
         points_earned (int): The earned points for this submission.
         points_possible (int): The total points possible for this particular

apps/submissions/migrations/0004_auto__add_field_score_reset.py

+# -*- coding: utf-8 -*-
+import datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        # Adding field 'Score.reset'
+        db.add_column('submissions_score', 'reset',
+                      self.gf('django.db.models.fields.BooleanField')(default=False),
+                      keep_default=False)
+
+
+    def backwards(self, orm):
+        # Deleting field 'Score.reset'
+        db.delete_column('submissions_score', 'reset')
+
+
+    models = {
+        'submissions.score': {
+            'Meta': {'object_name': 'Score'},
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'points_earned': ('django.db.models.fields.PositiveIntegerField', [], {'default': '0'}),
+            'points_possible': ('django.db.models.fields.PositiveIntegerField', [], {'default': '0'}),
+            'reset': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'student_item': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.StudentItem']"}),
+            'submission': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.Submission']", 'null': 'True'})
+        },
+        'submissions.scoresummary': {
+            'Meta': {'object_name': 'ScoreSummary'},
+            'highest': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'+'", 'to': "orm['submissions.Score']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'latest': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'+'", 'to': "orm['submissions.Score']"}),
+            'student_item': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.StudentItem']", 'unique': 'True'})
+        },
+        'submissions.studentitem': {
+            'Meta': {'unique_together': "(('course_id', 'student_id', 'item_id'),)", 'object_name': 'StudentItem'},
+            'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'item_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'item_type': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'student_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'})
+        },
+        'submissions.submission': {
+            'Meta': {'ordering': "['-submitted_at', '-id']", 'object_name': 'Submission'},
+            'attempt_number': ('django.db.models.fields.PositiveIntegerField', [], {}),
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'raw_answer': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
+            'student_item': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.StudentItem']"}),
+            'submitted_at': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'db_index': 'True'}),
+            'uuid': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '36', 'blank': 'True'})
+        }
+    }
+
+    complete_apps = ['submissions']
\ No newline at end of file

apps/submissions/models.py

@@ -112,11 +112,36 @@ class Score(models.Model):
     points_possible = models.PositiveIntegerField(default=0)
     created_at = models.DateTimeField(editable=False, default=now, db_index=True)
 
+    # Flag to indicate that this score should reset the current "highest" score
+    reset = models.BooleanField(default=False)
+
     @property
     def submission_uuid(self):
-        return self.submission.uuid
+        """
+        Retrieve the submission UUID associated with this score.
+        If the score isn't associated with a submission (for example, if this is
+        a "reset" score or a a non-courseware item like "class participation"),
+        then this will return None.
+
+        Returns:
+            str or None
+
+        """
+        if self.submission is not None:
+            return self.submission.uuid
+        else:
+            return None
 
     def to_float(self):
+        """
+        Calculate (points earned) / (points possible).
+        If points possible is None (e.g. this is a "hidden" score)
+        then return None.
+
+        Returns:
+            float or None
+
+        """
         if self.points_possible == 0:
             return None
         return float(self.points_earned) / self.points_possible
@@ -130,23 +155,81 @@ class Score(models.Model):
             points_possible=self.points_possible,
         ))
 
+    def is_hidden(self):
+        """
+        By convention, a score of 0/0 is not displayed to users.
+        Hidden scores are filtered by the submissions API.
+
+        Returns:
+            bool: Whether the score should be hidden.
+
+        """
+        return self.points_possible == 0
+
+    @classmethod
+    def create_reset_score(cls, student_item):
+        """
+        Create a "reset" score (a score with a null submission).
+
+        Only scores created after the most recent "reset" score
+        should be used to determine a student's effective score.
+
+        Args:
+            student_item (StudentItem): The student item model.
+
+        Returns:
+            Score: The newly created "reset" score.
+
+        Raises:
+            DatabaseError: An error occurred while creating the score
+
+        """
+        # By setting the "reset" flag, we ensure that the "highest"
+        # score in the score summary will point to this score.
+        # By setting points earned and points possible to 0,
+        # we ensure that this score will be hidden from the user.
+        return cls.objects.create(
+            student_item=student_item,
+            submission=None,
+            points_earned=0,
+            points_possible=0,
+            reset=True,
+        )
+
 
 class ScoreSummary(models.Model):
     """Running store of the highest and most recent Scores for a StudentItem."""
     student_item = models.ForeignKey(StudentItem, unique=True)
+
     highest = models.ForeignKey(Score, related_name="+")
     latest = models.ForeignKey(Score, related_name="+")
 
     @receiver(post_save, sender=Score)
     def update_score_summary(sender, **kwargs):
-        """Listen for new Scores and update the relevant ScoreSummary."""
+        """
+        Listen for new Scores and update the relevant ScoreSummary.
+
+        Args:
+            sender: not used
+
+        Kwargs:
+            instance (Score): The score model whose save triggered this receiver.
+
+        """
         score = kwargs['instance']
         try:
             score_summary = ScoreSummary.objects.get(
                 student_item=score.student_item
             )
             score_summary.latest = score
-            if score.to_float() > score_summary.highest.to_float():
+
+            # A score with the "reset" flag set will always replace the current highest score
+            if score.reset:
+                score_summary.highest = score
+            # The conversion to a float may return None if points possible is zero
+            # In Python, None is always less than an integer, so any score
+            # with non-null points possible will take precedence.
+            elif score.to_float() > score_summary.highest.to_float():
                 score_summary.highest = score
             score_summary.save()
         except ScoreSummary.DoesNotExist:
@@ -160,4 +243,3 @@ class ScoreSummary(models.Model):
                 u"Error while updating score summary for student item {}"
                 .format(score.student_item)
             )
-

apps/submissions/tests/test_api.py

@@ -233,6 +233,16 @@ class TestSubmissionsApi(TestCase):
         self._assert_score(score, 11, 12)
         self.assertEqual(score['submission_uuid'], submission['uuid'])
 
+    def test_get_score_for_submission_hidden_score(self):
+        # Create a "hidden" score for the submission
+        # (by convention, a score with points possible set to 0)
+        submission = api.create_submission(STUDENT_ITEM, ANSWER_ONE)
+        api.set_score(submission["uuid"], 0, 0)
+
+        # Expect that the retrieved score is None
+        score = api.get_latest_score_for_submission(submission['uuid'])
+        self.assertIs(score, None)
+
     def test_get_score_no_student_id(self):
         student_item = copy.deepcopy(STUDENT_ITEM)
         student_item['student_id'] = None

apps/submissions/tests/test_models.py

@@ -3,7 +3,7 @@ Tests for submission models.
 """
 
 from django.test import TestCase
-from submissions.models import Score, ScoreSummary, StudentItem
+from submissions.models import Submission, Score, ScoreSummary, StudentItem
 
 
 class TestScoreSummary(TestCase):
@@ -43,7 +43,6 @@ class TestScoreSummary(TestCase):
         # Low score is higher than no score...
         low_score = Score.objects.create(
             student_item=item,
-            submission=None,
             points_earned=0,
             points_possible=0,
         )
@@ -55,7 +54,6 @@ class TestScoreSummary(TestCase):
         # Medium score should supplant low score
         med_score = Score.objects.create(
             student_item=item,
-            submission=None,
             points_earned=8,
             points_possible=10,
         )
@@ -68,7 +66,6 @@ class TestScoreSummary(TestCase):
         # should win because it's 4/4 as opposed to 8/10.
         high_score = Score.objects.create(
             student_item=item,
-            submission=None,
             points_earned=4,
             points_possible=4,
         )
@@ -80,7 +77,6 @@ class TestScoreSummary(TestCase):
         # Put another medium score to make sure it doesn't get set back down
         med_score2 = Score.objects.create(
             student_item=item,
-            submission=None,
             points_earned=5,
             points_possible=10,
         )
@@ -92,3 +88,79 @@ class TestScoreSummary(TestCase):
             med_score2,
             ScoreSummary.objects.get(student_item=item).latest
         )
+
+    def test_reset_score_highest(self):
+        item = StudentItem.objects.create(
+            student_id="score_test_student",
+            course_id="score_test_course",
+            item_id="i4x://mycourse/special_presentation"
+        )
+
+        # Reset score with no score
+        Score.create_reset_score(item)
+        highest = ScoreSummary.objects.get(student_item=item).highest
+        self.assertEqual(highest.points_earned, 0)
+        self.assertEqual(highest.points_possible, 0)
+
+        # Non-reset score after a reset score
+        submission = Submission.objects.create(student_item=item, attempt_number=1)
+        Score.objects.create(
+            student_item=item,
+            submission=submission,
+            points_earned=2,
+            points_possible=3,
+        )
+        highest = ScoreSummary.objects.get(student_item=item).highest
+        self.assertEqual(highest.points_earned, 2)
+        self.assertEqual(highest.points_possible, 3)
+
+        # Reset score after a non-reset score
+        Score.create_reset_score(item)
+        highest = ScoreSummary.objects.get(student_item=item).highest
+        self.assertEqual(highest.points_earned, 0)
+        self.assertEqual(highest.points_possible, 0)
+
+    def test_highest_score_hidden(self):
+        item = StudentItem.objects.create(
+            student_id="score_test_student",
+            course_id="score_test_course",
+            item_id="i4x://mycourse/special_presentation"
+        )
+
+        # Score with points possible set to 0
+        # (by convention a "hidden" score)
+        submission = Submission.objects.create(student_item=item, attempt_number=1)
+        Score.objects.create(
+            student_item=item,
+            submission=submission,
+            points_earned=0,
+            points_possible=0,
+        )
+        highest = ScoreSummary.objects.get(student_item=item).highest
+        self.assertEqual(highest.points_earned, 0)
+        self.assertEqual(highest.points_possible, 0)
+
+        # Score with points
+        submission = Submission.objects.create(student_item=item, attempt_number=1)
+        Score.objects.create(
+            student_item=item,
+            submission=submission,
+            points_earned=1,
+            points_possible=2,
+        )
+        highest = ScoreSummary.objects.get(student_item=item).highest
+        self.assertEqual(highest.points_earned, 1)
+        self.assertEqual(highest.points_possible, 2)
+
+        # Another score with points possible set to 0
+        # The previous score should remain the highest score.
+        submission = Submission.objects.create(student_item=item, attempt_number=1)
+        Score.objects.create(
+            student_item=item,
+            submission=submission,
+            points_earned=0,
+            points_possible=0,
+        )
+        highest = ScoreSummary.objects.get(student_item=item).highest
+        self.assertEqual(highest.points_earned, 1)
+        self.assertEqual(highest.points_possible, 2)

apps/submissions/tests/test_reset_score.py

+"""
+Test reset scores.
+"""
+
+import copy
+from mock import patch
+from django.test import TestCase
+import ddt
+from django.core.cache import cache
+from django.db import DatabaseError
+from submissions import api as sub_api
+from submissions.models import Score
+
+
+@ddt.ddt
+class TestResetScore(TestCase):
+    """
+    Test resetting scores for a specific student on a specific problem.
+    """
+
+    STUDENT_ITEM = {
+        'student_id': 'Test student',
+        'course_id': 'Test course',
+        'item_id': 'Test item',
+        'item_type': 'Test item type',
+    }
+
+    def setUp(self):
+        """
+        Clear the cache.
+        """
+        cache.clear()
+
+    def test_reset_with_no_scores(self):
+        sub_api.reset_score(
+            self.STUDENT_ITEM['student_id'],
+            self.STUDENT_ITEM['course_id'],
+            self.STUDENT_ITEM['item_id'],
+        )
+        self.assertIs(sub_api.get_score(self.STUDENT_ITEM), None)
+
+        scores = sub_api.get_scores(self.STUDENT_ITEM['course_id'], self.STUDENT_ITEM['student_id'])
+        self.assertEqual(len(scores), 0)
+
+    def test_reset_with_one_score(self):
+        # Create a submission for the student and score it
+        submission = sub_api.create_submission(self.STUDENT_ITEM, 'test answer')
+        sub_api.set_score(submission['uuid'], 1, 2)
+
+        # Reset scores
+        sub_api.reset_score(
+            self.STUDENT_ITEM['student_id'],
+            self.STUDENT_ITEM['course_id'],
+            self.STUDENT_ITEM['item_id'],
+        )
+
+        # Expect that no scores are available for the student
+        self.assertIs(sub_api.get_score(self.STUDENT_ITEM), None)
+        scores = sub_api.get_scores(self.STUDENT_ITEM['course_id'], self.STUDENT_ITEM['student_id'])
+        self.assertEqual(len(scores), 0)
+
+    def test_reset_with_multiple_scores(self):
+        # Create a submission for the student and score it
+        submission = sub_api.create_submission(self.STUDENT_ITEM, 'test answer')
+        sub_api.set_score(submission['uuid'], 1, 2)
+        sub_api.set_score(submission['uuid'], 2, 2)
+
+        # Reset scores
+        sub_api.reset_score(
+            self.STUDENT_ITEM['student_id'],
+            self.STUDENT_ITEM['course_id'],
+            self.STUDENT_ITEM['item_id'],
+        )
+
+        # Expect that no scores are available for the student
+        self.assertIs(sub_api.get_score(self.STUDENT_ITEM), None)
+        scores = sub_api.get_scores(self.STUDENT_ITEM['course_id'], self.STUDENT_ITEM['student_id'])
+        self.assertEqual(len(scores), 0)
+
+    @ddt.data(
+        {'student_id': 'other student'},
+        {'course_id': 'other course'},
+        {'item_id': 'other item'},
+    )
+    def test_reset_different_student_item(self, changed):
+        # Create a submissions for two students
+        submission = sub_api.create_submission(self.STUDENT_ITEM, 'test answer')
+        sub_api.set_score(submission['uuid'], 1, 2)
+
+        other_student = copy.copy(self.STUDENT_ITEM)
+        other_student.update(changed)
+        submission = sub_api.create_submission(other_student, 'other test answer')
+        sub_api.set_score(submission['uuid'], 3, 4)
+
+        # Reset the score for the first student
+        sub_api.reset_score(
+            self.STUDENT_ITEM['student_id'],
+            self.STUDENT_ITEM['course_id'],
+            self.STUDENT_ITEM['item_id'],
+        )
+
+        # The first student's scores should be reset
+        self.assertIs(sub_api.get_score(self.STUDENT_ITEM), None)
+        scores = sub_api.get_scores(self.STUDENT_ITEM['course_id'], self.STUDENT_ITEM['student_id'])
+        self.assertNotIn(self.STUDENT_ITEM['item_id'], scores)
+
+        # But the second student should still have a score
+        score = sub_api.get_score(other_student)
+        self.assertEqual(score['points_earned'], 3)
+        self.assertEqual(score['points_possible'], 4)
+        scores = sub_api.get_scores(other_student['course_id'], other_student['student_id'])
+        self.assertIn(other_student['item_id'], scores)
+
+    def test_reset_then_add_score(self):
+        # Create a submission for the student and score it
+        submission = sub_api.create_submission(self.STUDENT_ITEM, 'test answer')
+        sub_api.set_score(submission['uuid'], 1, 2)
+
+        # Reset scores
+        sub_api.reset_score(
+            self.STUDENT_ITEM['student_id'],
+            self.STUDENT_ITEM['course_id'],
+            self.STUDENT_ITEM['item_id'],
+        )
+
+        # Score the student again
+        sub_api.set_score(submission['uuid'], 3, 4)
+
+        # Expect that the new score is available
+        score = sub_api.get_score(self.STUDENT_ITEM)
+        self.assertEqual(score['points_earned'], 3)
+        self.assertEqual(score['points_possible'], 4)
+
+        scores = sub_api.get_scores(self.STUDENT_ITEM['course_id'], self.STUDENT_ITEM['student_id'])
+        self.assertIn(self.STUDENT_ITEM['item_id'], scores)
+        self.assertEqual(scores[self.STUDENT_ITEM['item_id']], (3, 4))
+
+    def test_reset_then_get_score_for_submission(self):
+        # Create a submission for the student and score it
+        submission = sub_api.create_submission(self.STUDENT_ITEM, 'test answer')
+        sub_api.set_score(submission['uuid'], 1, 2)
+
+        # Reset scores
+        sub_api.reset_score(
+            self.STUDENT_ITEM['student_id'],
+            self.STUDENT_ITEM['course_id'],
+            self.STUDENT_ITEM['item_id'],
+        )
+
+        # If we're retrieving the score for a particular submission,
+        # instead of a student item, then we should STILL get a score.
+        self.assertIsNot(sub_api.get_latest_score_for_submission(submission['uuid']), None)
+
+    @patch.object(Score.objects, 'create')
+    def test_database_error(self, create_mock):
+        # Create a submission for the student and score it
+        submission = sub_api.create_submission(self.STUDENT_ITEM, 'test answer')
+        sub_api.set_score(submission['uuid'], 1, 2)
+
+        # Simulate a database error when creating the reset score
+        create_mock.side_effect = DatabaseError("Test error")
+        with self.assertRaises(sub_api.SubmissionInternalError):
+            sub_api.reset_score(
+                self.STUDENT_ITEM['student_id'],
+                self.STUDENT_ITEM['course_id'],
+                self.STUDENT_ITEM['item_id'],
+            )

apps/submissions/tests/test_serializers.py

+"""
+Tests for submissions serializers.
+"""
+from django.test import TestCase
+from submissions.models import Score, StudentItem
+from submissions.serializers import ScoreSerializer
+
+
+class ScoreSerializerTest(TestCase):
+    """
+    Tests for the score serializer.
+    """
+
+    def test_score_with_null_submission(self):
+        item = StudentItem.objects.create(
+            student_id="score_test_student",
+            course_id="score_test_course",
+            item_id="i4x://mycourse/special_presentation"
+        )
+
+        # Create a score with a null submission
+        score = Score.objects.create(
+            student_item=item,
+            submission=None,
+            points_earned=2,
+            points_possible=6
+        )
+        score_dict = ScoreSerializer(score).data
+
+        self.assertIs(score_dict['submission_uuid'], None)
+        self.assertEqual(score_dict['points_earned'], 2)
+        self.assertEqual(score_dict['points_possible'], 6)