Feedback changes

milestones/api.py

@@ -70,6 +70,13 @@ def _validate_user(user):
 
 
 # PUBLIC FUNCTIONS
+def get_milestone_relationship_types():
+    """
+    Exposes the available relationship type choices without exposing the data layer
+    """
+    return data.RELATIONSHIP_TYPES
+
+
 def add_milestone(milestone):
     """
     Passes a new milestone to the data layer for storage
@@ -155,7 +162,11 @@ def get_course_required_milestones(course_key, user):
     """
     _validate_course_key(course_key)
     _validate_user(user)
-    required_milestones = data.fetch_courses_milestones([course_key], 'requires', user)
+    required_milestones = data.fetch_courses_milestones(
+        [course_key],
+        get_milestone_relationship_types()['REQUIRES'],
+        user
+    )
     return required_milestones
 
 
@@ -167,17 +178,27 @@ def get_course_milestones_fulfillment_paths(course_key, user):
     _validate_user(user)
 
     # Retrieve the outstanding milestones for this course, for this user
-    required_milestones = data.fetch_courses_milestones([course_key], 'requires', user)
+    required_milestones = data.fetch_courses_milestones(
+        [course_key],
+        get_milestone_relationship_types()['REQUIRES'],
+        user
+    )
 
     # Build the set of fulfillment paths for the outstanding milestones
     fulfillment_paths = {}
     for milestone in required_milestones:
         dict_key = 'milestone_{}'.format(milestone['id'])
         fulfillment_paths[dict_key] = {}
-        milestone_courses = data.fetch_milestone_courses(milestone, 'fulfills')
+        milestone_courses = data.fetch_milestone_courses(
+            milestone,
+            get_milestone_relationship_types()['FULFILLS']
+        )
         if len(milestone_courses):
             fulfillment_paths[dict_key]['courses'] = milestone_courses
-        milestone_course_content = data.fetch_milestone_course_content(milestone, 'fulfills')
+        milestone_course_content = data.fetch_milestone_course_content(
+            milestone,
+            get_milestone_relationship_types()['FULFILLS']
+        )
         if len(milestone_course_content):
             fulfillment_paths[dict_key]['content'] = milestone_course_content
     return fulfillment_paths
@@ -187,6 +208,7 @@ def get_courses_milestones(course_keys, relationship=None, user=None):
     """
     Retrieves the set of milestones for list of courses
     'relationship': optional filter on milestone relationship type (string, eg: 'requires')
+    'user': optional filter to constrain the set to those milestones which a user has already collected
     Returns an array of dicts containing milestones
     """
     [_validate_course_key(course_key) for course_key in course_keys]  # pylint: disable=expression-not-assigned

milestones/data.py

@@ -28,6 +28,11 @@
 from . import models as internal
 from . import serializers
 
+RELATIONSHIP_TYPES = {
+    'FULFILLS': "fulfills",
+    'REQUIRES': "requires",
+}
+
 
 # PRIVATE/INTERNAL METHODS
 def _get_milestone_relationship_type(relationship):
@@ -40,13 +45,11 @@ def _get_milestone_relationship_type(relationship):
             active=True
         )
     except internal.MilestoneRelationshipType.DoesNotExist:
-        if relationship in ['requires', 'fulfills']:
-            return internal.MilestoneRelationshipType.objects.create(
-                name=relationship,
-                active=True
-            )
-        else:
-            raise exceptions.InvalidMilestoneRelationshipTypeException()
+        exceptions.raise_exception(
+            "MilestoneRelationshipType",
+            {'name': relationship},
+            exceptions.InvalidMilestoneRelationshipTypeException
+        )
 
 
 # PUBLIC METHODS
@@ -80,7 +83,7 @@ def update_milestone(milestone):
         milestone.description = milestone_obj.description
         milestone.active = milestone_obj.active
     except internal.Milestone.DoesNotExist:
-        raise exceptions.InvalidMilestoneException()
+        exceptions.raise_exception("Milestone", milestone, exceptions.InvalidMilestoneException)
     return serializers.serialize_milestone(milestone)
 
 
@@ -114,7 +117,7 @@ def fetch_milestones(milestone):
     Returns a list-of-dicts representation of the object
     """
     if milestone is None:
-        raise exceptions.InvalidMilestoneException()
+        exceptions.raise_exception("Milestone", milestone, exceptions.InvalidMilestoneException)
     milestone_obj = serializers.deserialize_milestone(milestone)
     if milestone_obj.id is not None:
         return serializers.serialize_milestones(internal.Milestone.objects.filter(
@@ -163,6 +166,7 @@ def fetch_courses_milestones(course_keys, relationship=None, user=None):
     """
     Retrieves the set of milestones currently linked to the specified courses
     Optionally pass in 'relationship' (ex. 'fulfills') to filter down the set
+    Optionally pass in 'user' to constrain the set to those which the user has collected
     """
     queryset = internal.CourseMilestone.objects.filter(
         course_id__in=course_keys,
@@ -179,7 +183,7 @@ def fetch_courses_milestones(course_keys, relationship=None, user=None):
     # To pull the list of milestones a user HAS, use get_user_milestones
     # Use fetch_courses_milestones to pull the list of milestones that a user does not yet
     # have for the specified course
-    if relationship == 'requires' and user and user.get('id', 0) > 0:
+    if relationship == RELATIONSHIP_TYPES['REQUIRES'] and user and user.get('id', 0) > 0:
         queryset = queryset.exclude(milestone__usermilestone__user_id=user['id'])
 
     # Assemble the response container
@@ -223,31 +227,30 @@ def delete_course_content_milestone(course_key, content_key, milestone):
         pass
 
 
-def fetch_course_content_milestones(course_key, content_key, relationship=None):
+def fetch_course_content_milestones(content_key, course_key=None, relationship=None):
     """
     Retrieves the set of milestones currently linked to the specified course content
     Optionally pass in 'relationship' (ex. 'fulfills') to filter down the set
     Optionally pass in 'user' to further-filter the set (ex. for retrieving unfulfilled milestones)
     """
-    queryset = internal.Milestone.objects.filter(active=True)
+    queryset = internal.CourseContentMilestone.objects.filter(
+        active=True
+    ).select_related('milestone')
 
-    if course_key:
-        queryset = queryset.filter(coursecontentmilestone__course_id=unicode(course_key))
+    if course_key is not None:
+        queryset = queryset.filter(course_id=unicode(course_key)).select_related('milestone')
 
-    if content_key:
-        queryset = queryset.filter(coursecontentmilestone__content_id=unicode(content_key))
+    if content_key is not None:
+        queryset = queryset.filter(content_id=unicode(content_key)).select_related('milestone')
 
-    if relationship:
+    if relationship is not None:
         mrt = _get_milestone_relationship_type(relationship)
-        queryset = internal.Milestone.objects.filter(
-            coursecontentmilestone__milestone_relationship_type=mrt.id,
-            active=True,
-        )
+        queryset = queryset.filter(milestone_relationship_type=mrt.id).select_related('milestone')
 
     course_content_milestones = []
     if len(queryset):
-        for milestone in queryset:
-            course_content_milestones.append(serializers.serialize_milestone(milestone))
+        for ccm in queryset:
+            course_content_milestones.append(serializers.serialize_milestone(ccm.milestone))
 
     return course_content_milestones
 
@@ -272,9 +275,8 @@ def fetch_milestone_courses(milestone, relationship=None):
 
     # Assemble the response container
     milestone_courses = []
-    if len(queryset):
-        for milestone in queryset:
-            milestone_courses.append(serializers.serialize_milestone_with_course(milestone))
+    for milestone in queryset:
+        milestone_courses.append(serializers.serialize_milestone_with_course(milestone))
 
     return milestone_courses
 
@@ -362,7 +364,8 @@ def delete_content_references(content_key):
     Removes references to content keys within this app (ref: api.py)
     Supports the 'delete entrance exam' Studio use case, when Milestones is enabled
     """
-    internal.CourseContentMilestone.objects.filter(content_id=unicode(content_key)).delete()
+    references = internal.CourseContentMilestone.objects.filter(content_id=unicode(content_key))
+    references.delete()
 
 
 def delete_course_references(course_key):

milestones/exceptions.py

@@ -1,37 +1,38 @@
 """
 Application-specific exception classes used throughout the implementation
 """
+from django.core.exceptions import ValidationError
 
 
-class InvalidCourseKeyException(Exception):
+class InvalidCourseKeyException(ValidationError):
     """
     CourseKey validation exception class
     """
     pass
 
 
-class InvalidContentKeyException(Exception):
+class InvalidContentKeyException(ValidationError):
     """
     Course content/module/usage key validation exception class
     """
     pass
 
 
-class InvalidMilestoneException(Exception):
+class InvalidMilestoneException(ValidationError):
     """
     Milestone validation exception class
     """
     pass
 
 
-class InvalidMilestoneRelationshipTypeException(Exception):
+class InvalidMilestoneRelationshipTypeException(ValidationError):
     """
     Milestone Relationship Type validation exception class
     """
     pass
 
 
-class InvalidUserException(Exception):
+class InvalidUserException(ValidationError):
     """
     User validation exception class
     """

milestones/migrations/0001_initial.py

@@ -13,10 +13,10 @@ def forwards(self, orm):
             ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
             ('created', self.gf('model_utils.fields.AutoCreatedField')(default=datetime.datetime.now)),
             ('modified', self.gf('model_utils.fields.AutoLastModifiedField')(default=datetime.datetime.now)),
-            ('namespace', self.gf('django.db.models.fields.CharField')(max_length=255)),
-            ('name', self.gf('django.db.models.fields.CharField')(max_length=255)),
+            ('namespace', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)),
+            ('name', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)),
             ('description', self.gf('django.db.models.fields.TextField')()),
-            ('active', self.gf('django.db.models.fields.BooleanField')(default=True)),
+            ('active', self.gf('django.db.models.fields.BooleanField')(default=True, db_index=True)),
         ))
         db.send_create_signal('milestones', ['Milestone'])
 
@@ -27,7 +27,7 @@ def forwards(self, orm):
             ('modified', self.gf('model_utils.fields.AutoLastModifiedField')(default=datetime.datetime.now)),
             ('name', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)),
             ('description', self.gf('django.db.models.fields.TextField')(blank=True)),
-            ('active', self.gf('django.db.models.fields.BooleanField')(default=True)),
+            ('active', self.gf('django.db.models.fields.BooleanField')(default=True, db_index=True)),
         ))
         db.send_create_signal('milestones', ['MilestoneRelationshipType'])
 
@@ -39,7 +39,7 @@ def forwards(self, orm):
             ('course_id', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)),
             ('milestone', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['milestones.Milestone'])),
             ('milestone_relationship_type', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['milestones.MilestoneRelationshipType'])),
-            ('active', self.gf('django.db.models.fields.BooleanField')(default=True)),
+            ('active', self.gf('django.db.models.fields.BooleanField')(default=True, db_index=True)),
         ))
         db.send_create_signal('milestones', ['CourseMilestone'])
 
@@ -55,7 +55,7 @@ def forwards(self, orm):
             ('content_id', self.gf('django.db.models.fields.CharField')(max_length=255, db_index=True)),
             ('milestone', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['milestones.Milestone'])),
             ('milestone_relationship_type', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['milestones.MilestoneRelationshipType'])),
-            ('active', self.gf('django.db.models.fields.BooleanField')(default=True)),
+            ('active', self.gf('django.db.models.fields.BooleanField')(default=True, db_index=True)),
         ))
         db.send_create_signal('milestones', ['CourseContentMilestone'])
 
@@ -70,7 +70,8 @@ def forwards(self, orm):
             ('user_id', self.gf('django.db.models.fields.IntegerField')(db_index=True)),
             ('milestone', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['milestones.Milestone'])),
             ('source', self.gf('django.db.models.fields.TextField')(blank=True)),
-            ('active', self.gf('django.db.models.fields.BooleanField')(default=True)),
+            ('collected', self.gf('django.db.models.fields.DateTimeField')(null=True, blank=True)),
+            ('active', self.gf('django.db.models.fields.BooleanField')(default=True, db_index=True)),
         ))
         db.send_create_signal('milestones', ['UserMilestone'])
 
@@ -105,7 +106,7 @@ def backwards(self, orm):
     models = {
         'milestones.coursecontentmilestone': {
             'Meta': {'unique_together': "(('course_id', 'content_id', 'milestone'),)", 'object_name': 'CourseContentMilestone'},
-            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True', 'db_index': 'True'}),
             'content_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
             'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
             'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}),
@@ -116,7 +117,7 @@ def backwards(self, orm):
         },
         'milestones.coursemilestone': {
             'Meta': {'unique_together': "(('course_id', 'milestone'),)", 'object_name': 'CourseMilestone'},
-            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True', 'db_index': 'True'}),
             'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
             'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
@@ -126,17 +127,17 @@ def backwards(self, orm):
         },
         'milestones.milestone': {
             'Meta': {'object_name': 'Milestone'},
-            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True', 'db_index': 'True'}),
             'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}),
             'description': ('django.db.models.fields.TextField', [], {}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
             'modified': ('model_utils.fields.AutoLastModifiedField', [], {'default': 'datetime.datetime.now'}),
-            'name': ('django.db.models.fields.CharField', [], {'max_length': '255'}),
-            'namespace': ('django.db.models.fields.CharField', [], {'max_length': '255'})
+            'name': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'namespace': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'})
         },
         'milestones.milestonerelationshiptype': {
             'Meta': {'object_name': 'MilestoneRelationshipType'},
-            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True', 'db_index': 'True'}),
             'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}),
             'description': ('django.db.models.fields.TextField', [], {'blank': 'True'}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
@@ -145,7 +146,8 @@ def backwards(self, orm):
         },
         'milestones.usermilestone': {
             'Meta': {'unique_together': "(('user_id', 'milestone'),)", 'object_name': 'UserMilestone'},
-            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}),
+            'active': ('django.db.models.fields.BooleanField', [], {'default': 'True', 'db_index': 'True'}),
+            'collected': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'blank': 'True'}),
             'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
             'milestone': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['milestones.Milestone']"}),