Merge pull request #127 from edx/ormsbee/score_summaries

Get scores by course_id and student_id

apps/openassessment/assessment/test/test_peer.py

@@ -171,7 +171,6 @@ class TestPeerApi(TestCase):
                 "must_be_graded_by": REQUIRED_GRADED_BY,
             }
         }
-        # score = sub_api.get_score(STUDENT_ITEM)
         score = workflow_api.get_workflow_for_submission(
             tim_sub["uuid"], requirements
         )["score"]
@@ -188,8 +187,7 @@ class TestPeerApi(TestCase):
 
         # Tim should not have a score, because his submission does not have
         # enough assessments.
-        scores = sub_api.get_score(STUDENT_ITEM)
-        self.assertFalse(scores)
+        self.assertIsNone(sub_api.get_score(STUDENT_ITEM))
 
         sub = peer_api.get_submission_to_assess(bob, REQUIRED_GRADED)
         self.assertEqual(sub["uuid"], tim_sub["uuid"])
@@ -254,7 +252,7 @@ class TestPeerApi(TestCase):
                 "must_be_graded_by": REQUIRED_GRADED_BY,
                 }
         }
-        
+
         # 1) Angel Submits
         angel_sub, angel = self._create_student_and_submission("Angel", "Angel's answer")
 

apps/openassessment/xblock/openassessmentblock.py

@@ -257,11 +257,19 @@ class OpenAssessmentBlock(
                 includes the student id, item id, and course id.
         """
         item_id, student_id = self.get_xblock_trace()
+
+        # This is not the real way course_ids should work, but this is a
+        # temporary expediency for LMS integratino
+        if hasattr(self, "xmodule_runtime"):
+            course_id = self.xmodule_runtime.course_id
+        else:
+            course_id = "edX/Enchantment_101/April_1"
+
         student_item_dict = dict(
             student_id=student_id,
             item_id=item_id,
-            course_id=self.course_id,
-            item_type='openassessment'      # XXX: Is this the tag we want? Why?
+            course_id=course_id,
+            item_type='openassessment'
         )
         return student_item_dict
 

apps/openassessment/xblock/static/xml/censorship_rubric_example.xml

@@ -42,7 +42,7 @@
         </criterion>
         <criterion>
             <name>form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>IRC</name>
                 <explanation></explanation>

apps/openassessment/xblock/static/xml/poverty_rubric_example.xml

@@ -76,7 +76,7 @@
         </criterion>
         <criterion>
             <name>form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>lolcats</name>
                 <explanation></explanation>

apps/openassessment/xblock/submission_mixin.py

@@ -116,24 +116,6 @@ class SubmissionMixin(object):
         return submission
 
     @staticmethod
-    def _get_submission_score(student_item_dict):
-        """Return the most recent score, if any, for student item
-
-        Gets the score, if available.
-
-        Args:
-            student_item_dict (dict): The student item we want to check for a
-                score.
-
-        Returns:
-            (dict): Dictionary representing the score for this particular
-                question.
-
-        """
-        scores = api.get_score(student_item_dict)
-        return scores[0] if scores else None
-
-    @staticmethod
     def get_user_submission(submission_uuid):
         """Return the most recent submission by user in workflow
 

apps/openassessment/xblock/test/data/basic_scenario.xml

@@ -56,7 +56,7 @@
         </criterion>
         <criterion>
             <name>Form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>lolcats</name>
                 <explanation>lolcats explanation</explanation>

apps/openassessment/xblock/test/data/dates_scenario.xml

@@ -56,7 +56,7 @@
         </criterion>
         <criterion>
             <name>Form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>lolcats</name>
                 <explanation>lolcats explanation</explanation>

apps/openassessment/xblock/test/data/invalid_assessment_combo.xml

@@ -56,7 +56,7 @@
         </criterion>
         <criterion>
             <name>Form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>lolcats</name>
                 <explanation>lolcats explanation</explanation>

apps/openassessment/xblock/test/data/invalid_dates.xml

@@ -56,7 +56,7 @@
         </criterion>
         <criterion>
             <name>Form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>lolcats</name>
                 <explanation>lolcats explanation</explanation>

apps/openassessment/xblock/test/data/resolve_dates_scenario.xml

@@ -56,7 +56,7 @@
         </criterion>
         <criterion>
             <name>Form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>lolcats</name>
                 <explanation>lolcats explanation</explanation>

apps/openassessment/xblock/test/data/save_scenario.xml

@@ -56,7 +56,7 @@
         </criterion>
         <criterion>
             <name>Form</name>
-            <prompt>Lastly, how is it's form? Punctuation, grammar, and spelling all count.</prompt>
+            <prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
             <option points="0">
                 <name>lolcats</name>
                 <explanation>lolcats explanation</explanation>

apps/submissions/api.py

@@ -8,8 +8,10 @@ import logging
 from django.db import DatabaseError
 from django.utils.encoding import force_unicode
 
-from submissions.serializers import SubmissionSerializer, StudentItemSerializer, ScoreSerializer
-from submissions.models import Submission, StudentItem, Score
+from submissions.serializers import (
+    SubmissionSerializer, StudentItemSerializer, ScoreSerializer
+)
+from submissions.models import Submission, StudentItem, Score, ScoreSummary
 
 logger = logging.getLogger(__name__)
 
@@ -320,12 +322,49 @@ def get_score(student_item):
     """
     try:
         student_item_model = StudentItem.objects.get(**student_item)
-        scores = Score.objects.filter(student_item=student_item_model)
-
-    except StudentItem.DoesNotExist:
+        score = ScoreSummary.objects.get(student_item=student_item_model).latest
+    except (ScoreSummary.DoesNotExist, StudentItem.DoesNotExist):
         return None
 
-    return ScoreSerializer(scores, many=True).data
+    return ScoreSerializer(score).data
+
+
+def get_scores(course_id, student_id):
+    """Return a dict mapping item_ids -> (points_earned, points_possible).
+
+    This method would be used by an LMS to find all the scores for a given
+    student in a given course.
+
+    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`.
+
+    Returns:
+        dict: The keys are `item_id`s (`str`) and the values are tuples of
+        `(points_earned, points_possible)`. All points are integer values and
+        represent the raw, unweighted scores. Submissions does not have any
+        concept of weights. If there are no entries matching the `course_id` or
+        `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.
+    """
+    try:
+        score_summaries = ScoreSummary.objects.filter(
+            student_item__course_id=course_id,
+            student_item__student_id=student_id,
+        ).select_related('latest', 'student_item')
+    except DatabaseError:
+        msg = u"Could not fetch scores for course {}, student {}".format(
+            course_id, student_id
+        )
+        logger.exception(msg)
+        raise SubmissionInternalError(msg)
+    scores = {
+        summary.student_item.item_id:
+            (summary.latest.points_earned, summary.latest.points_possible)
+        for summary in score_summaries
+    }
+    return scores
 
 
 def get_latest_score_for_submission(submission_uuid):
@@ -341,9 +380,6 @@ def get_latest_score_for_submission(submission_uuid):
 
     return ScoreSerializer(score).data
 
-def get_scores(course_id, student_id, types=None):
-    pass
-
 
 def set_score(submission_uuid, score, points_possible):
     """Set a score for a particular submission.

apps/submissions/migrations/0002_auto__add_scoresummary.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 model 'ScoreSummary'
+        db.create_table('submissions_scoresummary', (
+            ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
+            ('student_item', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['submissions.StudentItem'], unique=True)),
+            ('highest', self.gf('django.db.models.fields.related.ForeignKey')(related_name='+', to=orm['submissions.Score'])),
+            ('latest', self.gf('django.db.models.fields.related.ForeignKey')(related_name='+', to=orm['submissions.Score'])),
+        ))
+        db.send_create_signal('submissions', ['ScoreSummary'])
+
+
+    def backwards(self, orm):
+        # Deleting model 'ScoreSummary'
+        db.delete_table('submissions_scoresummary')
+
+
+    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'}),
+            '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'},
+            'answer': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
+            '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'}),
+            '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

@@ -9,11 +9,18 @@ need to then generate a matching migration for it using:
     ./manage.py schemamigration submissions --auto
 
 """
-from django.db import models
+import logging
+
+from django.db import models, DatabaseError
+from django.db.models.signals import post_save
+from django.dispatch import receiver
 from django.utils.timezone import now
 from django_extensions.db.fields import UUIDField
 
 
+logger = logging.getLogger(__name__)
+
+
 class StudentItem(models.Model):
     """Represents a single item for a single course for a single user.
 
@@ -95,12 +102,7 @@ class Submission(models.Model):
 
 
 class Score(models.Model):
-    """What the user scored for a given StudentItem.
-
-    TODO: Make a ScoreHistory that has more detailed log information so that we
-    can reconstruct what the state was at a given point in time and debug
-    more easily.
-    """
+    """What the user scored for a given StudentItem at a given time."""
     student_item = models.ForeignKey(StudentItem)
     submission = models.ForeignKey(Submission, null=True)
     points_earned = models.PositiveIntegerField(default=0)
@@ -111,6 +113,11 @@ class Score(models.Model):
     def submission_uuid(self):
         return self.submission.uuid
 
+    def to_float(self):
+        if self.points_possible == 0:
+            return None
+        return float(self.points_earned) / self.points_possible
+
     def __repr__(self):
         return repr(dict(
             student_item=self.student_item,
@@ -120,3 +127,33 @@ class Score(models.Model):
             points_possible=self.points_possible,
         ))
 
+
+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."""
+        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():
+                score_summary.highest = score
+            score_summary.save()
+        except ScoreSummary.DoesNotExist:
+            ScoreSummary.objects.create(
+                student_item=score.student_item,
+                highest=score,
+                latest=score,
+            )
+        except DatabaseError as err:
+            logger.exception(
+                u"Error while updating score summary for student item {}"
+                .format(score.student_item)
+            )

apps/submissions/tests/test_api.py

@@ -9,7 +9,7 @@ from mock import patch
 import pytz
 
 from submissions import api as api
-from submissions.models import Submission, StudentItem
+from submissions.models import ScoreSummary, Submission, StudentItem
 from submissions.serializers import StudentItemSerializer
 
 STUDENT_ITEM = dict(
@@ -183,15 +183,55 @@ class TestSubmissionsApi(TestCase):
     def test_get_score(self):
         submission = api.create_submission(STUDENT_ITEM, ANSWER_ONE)
         api.set_score(submission["uuid"], 11, 12)
-        scores = api.get_score(STUDENT_ITEM)
-        self._assert_score(scores[0], 11, 12)
-        self.assertEqual(scores[0]['submission_uuid'], submission['uuid'])
+        score = api.get_score(STUDENT_ITEM)
+        self._assert_score(score, 11, 12)
+        self.assertEqual(score['submission_uuid'], submission['uuid'])
 
     def test_get_score_no_student_id(self):
         student_item = copy.deepcopy(STUDENT_ITEM)
         student_item['student_id'] = None
         self.assertIs(api.get_score(student_item), None)
 
+    def test_get_scores(self):
+        student_item = copy.deepcopy(STUDENT_ITEM)
+        student_item["course_id"] = "get_scores_course"
+
+        student_item["item_id"] = "i4x://a/b/c/s1"
+        s1 = api.create_submission(student_item, "Hello World")
+
+        student_item["item_id"] = "i4x://a/b/c/s2"
+        s2 = api.create_submission(student_item, "Hello World")
+
+        student_item["item_id"] = "i4x://a/b/c/s3"
+        s3 = api.create_submission(student_item, "Hello World")
+
+        api.set_score(s1['uuid'], 3, 5)
+        api.set_score(s1['uuid'], 4, 5)
+        api.set_score(s1['uuid'], 2, 5)  # Should overwrite previous lines
+
+        api.set_score(s2['uuid'], 0, 10)
+        api.set_score(s3['uuid'], 4, 4)
+
+        # Getting the scores for a user should never take more than one query
+        with self.assertNumQueries(1):
+            scores = api.get_scores(
+                student_item["course_id"], student_item["student_id"]
+            )
+            self.assertEqual(
+                scores,
+                {
+                    u"i4x://a/b/c/s1": (2, 5),
+                    u"i4x://a/b/c/s2": (0, 10),
+                    u"i4x://a/b/c/s3": (4, 4),
+                }
+            )
+
+    @patch.object(ScoreSummary.objects, 'filter')
+    @raises(api.SubmissionInternalError)
+    def test_error_on_get_scores(self, mock_filter):
+        mock_filter.side_effect = DatabaseError("Bad things happened")
+        api.get_scores("some_course", "some_student")
+
     def _assert_score(
             self,
             score,

apps/submissions/tests/test_models.py

+"""
+Tests for submission models.
+"""
+
+from django.test import TestCase
+from submissions.models import Score, ScoreSummary, StudentItem
+
+
+class TestScoreSummary(TestCase):
+    """
+    Test selection of options from a rubric.
+    """
+
+    def test_latest(self):
+        item = StudentItem.objects.create(
+            student_id="score_test_student",
+            course_id="score_test_course",
+            item_id="i4x://mycourse/class_participation.section_attendance"
+        )
+        first_score = Score.objects.create(
+            student_item=item,
+            submission=None,
+            points_earned=8,
+            points_possible=10,
+        )
+        second_score = Score.objects.create(
+            student_item=item,
+            submission=None,
+            points_earned=5,
+            points_possible=10,
+        )
+        latest_score = ScoreSummary.objects.get(student_item=item).latest
+        self.assertEqual(second_score, latest_score)
+
+
+    def test_highest(self):
+        item = StudentItem.objects.create(
+            student_id="score_test_student",
+            course_id="score_test_course",
+            item_id="i4x://mycourse/special_presentation"
+        )
+
+        # Low score is higher than no score...
+        low_score = Score.objects.create(
+            student_item=item,
+            submission=None,
+            points_earned=0,
+            points_possible=0,
+        )
+        self.assertEqual(
+            low_score,
+            ScoreSummary.objects.get(student_item=item).highest
+        )
+
+        # Medium score should supplant low score
+        med_score = Score.objects.create(
+            student_item=item,
+            submission=None,
+            points_earned=8,
+            points_possible=10,
+        )
+        self.assertEqual(
+            med_score,
+            ScoreSummary.objects.get(student_item=item).highest
+        )
+
+        # Even though the points_earned is higher in the med_score, high_score
+        # 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,
+        )
+        self.assertEqual(
+            high_score,
+            ScoreSummary.objects.get(student_item=item).highest
+        )
+
+        # 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,
+        )
+        self.assertEqual(
+            high_score,
+            ScoreSummary.objects.get(student_item=item).highest
+        )
+        self.assertEqual(
+            med_score2,
+            ScoreSummary.objects.get(student_item=item).latest
+        )