-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Block completion model #16047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block completion model #16047
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| """ | ||
| Completion App | ||
| """ | ||
|
|
||
| default_app_config = 'lms.djangoapps.completion.apps.CompletionAppConfig' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| """ | ||
| App Configuration for Completion | ||
| """ | ||
|
|
||
| from __future__ import absolute_import, division, print_function, unicode_literals | ||
| from django.apps import AppConfig | ||
|
|
||
|
|
||
| class CompletionAppConfig(AppConfig): | ||
| """ | ||
| App Configuration for Completion | ||
| """ | ||
| name = 'lms.djangoapps.completion' | ||
| verbose_name = 'Completion' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| # -*- coding: utf-8 -*- | ||
| from __future__ import unicode_literals | ||
|
|
||
| from django.db import migrations, models | ||
| import django.utils.timezone | ||
| from django.conf import settings | ||
| import model_utils.fields | ||
|
|
||
| import lms.djangoapps.completion.models | ||
| import openedx.core.djangoapps.xmodule_django.models | ||
|
|
||
| # pylint: disable=ungrouped-imports | ||
| try: | ||
| from django.models import BigAutoField # New in django 1.10 | ||
| except ImportError: | ||
| from openedx.core.djangolib.fields import BigAutoField | ||
| # pylint: enable=ungrouped-imports | ||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='BlockCompletion', | ||
| fields=[ | ||
| ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, verbose_name='created', editable=False)), | ||
| ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, verbose_name='modified', editable=False)), | ||
| ('id', BigAutoField(serialize=False, primary_key=True)), | ||
| ('course_key', openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255)), | ||
| ('block_key', openedx.core.djangoapps.xmodule_django.models.UsageKeyField(max_length=255)), | ||
| ('block_type', models.CharField(max_length=64)), | ||
| ('completion', models.FloatField(validators=[lms.djangoapps.completion.models.validate_percent])), | ||
| ('user', models.ForeignKey(to=settings.AUTH_USER_MODEL)), | ||
| ], | ||
| ), | ||
| migrations.AlterUniqueTogether( | ||
| name='blockcompletion', | ||
| unique_together=set([('course_key', 'block_key', 'user')]), | ||
| ), | ||
| migrations.AlterIndexTogether( | ||
| name='blockcompletion', | ||
| index_together=set([('course_key', 'block_type', 'user'), ('user', 'course_key', 'modified')]), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| """ | ||
| Completion tracking and aggregation models. | ||
| """ | ||
|
|
||
| from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
|
||
| from django.contrib.auth.models import User | ||
| from django.core.exceptions import ValidationError | ||
| from django.db import models | ||
| from django.utils.translation import ugettext as _ | ||
| from model_utils.models import TimeStampedModel | ||
| from opaque_keys.edx.keys import CourseKey | ||
|
|
||
| from openedx.core.djangoapps.xmodule_django.models import CourseKeyField, UsageKeyField | ||
|
|
||
| # pylint: disable=ungrouped-imports | ||
| try: | ||
| from django.models import BigAutoField # New in django 1.10 | ||
| except ImportError: | ||
| from openedx.core.djangolib.fields import BigAutoField | ||
| # pylint: enable=ungrouped-imports | ||
|
|
||
|
|
||
| def validate_percent(value): | ||
| """ | ||
| Verify that the passed value is between 0.0 and 1.0. | ||
| """ | ||
| if not 0.0 <= value <= 1.0: | ||
| raise ValidationError(_('{value} must be between 0.0 and 1.0').format(value=value)) | ||
|
|
||
|
|
||
| class BlockCompletionManager(models.Manager): | ||
| """ | ||
| Custom manager for BlockCompletion model. | ||
|
|
||
| Adds submit_completion method. | ||
| """ | ||
|
|
||
| def submit_completion(self, user, course_key, block_key, completion): | ||
| """ | ||
| Update the completion value for the specified record. | ||
|
|
||
| Parameters: | ||
| * user (django.contrib.auth.models.User): The user for whom the | ||
| completion is being submitted. | ||
| * course_key (opaque_keys.edx.keys.CourseKey): The course in | ||
| which the submitted block is found. | ||
| * block_key (opaque_keys.edx.keys.UsageKey): The block that has had | ||
| its completion changed. | ||
| * completion (float in range [0.0, 1.0]): The fractional completion | ||
| value of the block (0.0 = incomplete, 1.0 = complete). | ||
|
|
||
| Return Value: | ||
| (BlockCompletion, bool): A tuple comprising the created or updated | ||
| BlockCompletion object and a boolean value indicating whether the value | ||
|
|
||
| Raises: | ||
|
|
||
| ValueError: | ||
| If the wrong type is passed for one of the parameters. | ||
|
|
||
| django.core.exceptions.ValidationError: | ||
| If a float is passed that is not between 0.0 and 1.0. | ||
|
|
||
| django.db.DatabaseError: | ||
| If there was a problem getting, creating, or updating the | ||
| BlockCompletion record in the database. | ||
|
|
||
| This will also be a more specific error, as described here: | ||
| https://docs.djangoproject.com/en/1.11/ref/exceptions/#database-exceptions. | ||
| IntegrityError and OperationalError are relatively common | ||
| subclasses. | ||
| """ | ||
|
|
||
| # Raise ValueError to match normal django semantics for wrong type of field. | ||
| if not isinstance(course_key, CourseKey): | ||
| raise ValueError( | ||
| "course_key must be an instance of `opaque_keys.edx.keys.CourseKey`. Got {}".format(type(course_key)) | ||
| ) | ||
| try: | ||
| block_type = block_key.block_type | ||
| except AttributeError: | ||
| raise ValueError( | ||
| "block_key must be an instance of `opaque_keys.edx.keys.UsageKey`. Got {}".format(type(block_key)) | ||
| ) | ||
|
|
||
| obj, isnew = self.get_or_create( | ||
| user=user, | ||
| course_key=course_key, | ||
| block_type=block_type, | ||
| block_key=block_key, | ||
| defaults={'completion': completion}, | ||
| ) | ||
| if not isnew and obj.completion != completion: | ||
| obj.completion = completion | ||
| obj.full_clean() | ||
| obj.save() | ||
| return obj, isnew | ||
|
|
||
|
|
||
| class BlockCompletion(TimeStampedModel, models.Model): | ||
| """ | ||
| Track completion of completable blocks. | ||
|
|
||
| A completion is unique for each (user, course_key, block_key). | ||
|
|
||
| The block_type field is included separately from the block_key to | ||
| facilitate distinct aggregations of the completion of particular types of | ||
| block. | ||
|
|
||
| The completion value is stored as a float in the range [0.0, 1.0], and all | ||
| calculations are performed on this float, though current practice is to | ||
| only track binary completion, where 1.0 indicates that the block is | ||
| complete, and 0.0 indicates that the block is incomplete. | ||
| """ | ||
| id = BigAutoField(primary_key=True) # pylint: disable=invalid-name | ||
| user = models.ForeignKey(User) | ||
| course_key = CourseKeyField(max_length=255) | ||
| block_key = UsageKeyField(max_length=255) | ||
| block_type = models.CharField(max_length=64) | ||
| completion = models.FloatField(validators=[validate_percent]) | ||
|
|
||
| objects = BlockCompletionManager() | ||
|
|
||
| class Meta(object): | ||
| index_together = [ | ||
| ('course_key', 'block_type', 'user'), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can imagine your comments about the indices living in the code. Thoughts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's useful information that I think might not be as widely known as it should be, but it's fundamental to how indexes work, and is by no means unique to this particular model (the indexes on the grades models were designed with this in mind, for certain. I would think a wiki page, or other training documentation would be a better place for it. |
||
| ('user', 'course_key', 'modified'), | ||
| ] | ||
|
|
||
| unique_together = [ | ||
| ('course_key', 'block_key', 'user') | ||
| ] | ||
|
|
||
| def __unicode__(self): | ||
| return 'BlockCompletion: {username}, {course_key}, {block_key}: {completion}'.format( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Possible repeat comment, but I don't see what Github did with mine.]
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I responded to the other version of this comment. Intentional. |
||
| username=self.user.username, | ||
| course_key=self.course_key, | ||
| block_key=self.block_key, | ||
| completion=self.completion, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| """ | ||
| Test models, managers, and validators. | ||
| """ | ||
|
|
||
| from django.core.exceptions import ValidationError | ||
| from django.test import TestCase | ||
| from opaque_keys.edx.keys import UsageKey | ||
|
|
||
| from student.tests.factories import UserFactory | ||
|
|
||
| from .. import models | ||
|
|
||
|
|
||
| class PercentValidatorTestCase(TestCase): | ||
| """ | ||
| Test that validate_percent only allows floats (and ints) between 0.0 and 1.0. | ||
| """ | ||
| def test_valid_percents(self): | ||
| for value in [1.0, 0.0, 1, 0, 0.5, 0.333081348071397813987230871]: | ||
| models.validate_percent(value) | ||
|
|
||
| def test_invalid_percent(self): | ||
| for value in [-0.00000000001, 1.0000000001, 47.1, 1000, None, float('inf'), float('nan')]: | ||
| self.assertRaises(ValidationError, models.validate_percent, value) | ||
|
|
||
|
|
||
| class SubmitCompletionTestCase(TestCase): | ||
| """ | ||
| Test that BlockCompletion.objects.submit_completion has the desired | ||
| semantics. | ||
| """ | ||
| def setUp(self): | ||
| super(SubmitCompletionTestCase, self).setUp() | ||
| self.user = UserFactory() | ||
| self.block_key = UsageKey.from_string(u'block-v1:edx+test+run+type@video+block@doggos') | ||
| self.completion = models.BlockCompletion.objects.create( | ||
| user=self.user, | ||
| course_key=self.block_key.course_key, | ||
| block_type=self.block_key.block_type, | ||
| block_key=self.block_key, | ||
| completion=0.5, | ||
| ) | ||
|
|
||
| def test_changed_value(self): | ||
| with self.assertNumQueries(4): # Get, update, 2 * savepoints | ||
| completion, isnew = models.BlockCompletion.objects.submit_completion( | ||
| user=self.user, | ||
| course_key=self.block_key.course_key, | ||
| block_key=self.block_key, | ||
| completion=0.9, | ||
| ) | ||
| completion.refresh_from_db() | ||
| self.assertEqual(completion.completion, 0.9) | ||
| self.assertFalse(isnew) | ||
| self.assertEqual(models.BlockCompletion.objects.count(), 1) | ||
|
|
||
| def test_unchanged_value(self): | ||
| with self.assertNumQueries(1): # Get | ||
| completion, isnew = models.BlockCompletion.objects.submit_completion( | ||
| user=self.user, | ||
| course_key=self.block_key.course_key, | ||
| block_key=self.block_key, | ||
| completion=0.5, | ||
| ) | ||
| completion.refresh_from_db() | ||
| self.assertEqual(completion.completion, 0.5) | ||
| self.assertFalse(isnew) | ||
| self.assertEqual(models.BlockCompletion.objects.count(), 1) | ||
|
|
||
| def test_new_user(self): | ||
| newuser = UserFactory() | ||
| with self.assertNumQueries(4): # Get, update, 2 * savepoints | ||
| _, isnew = models.BlockCompletion.objects.submit_completion( | ||
| user=newuser, | ||
| course_key=self.block_key.course_key, | ||
| block_key=self.block_key, | ||
| completion=0.0, | ||
| ) | ||
| self.assertTrue(isnew) | ||
| self.assertEqual(models.BlockCompletion.objects.count(), 2) | ||
|
|
||
| def test_new_block(self): | ||
| newblock = UsageKey.from_string(u'block-v1:edx+test+run+type@video+block@puppers') | ||
| with self.assertNumQueries(4): # Get, update, 2 * savepoints | ||
| _, isnew = models.BlockCompletion.objects.submit_completion( | ||
| user=self.user, | ||
| course_key=newblock.course_key, | ||
| block_key=newblock, | ||
| completion=1.0, | ||
| ) | ||
| self.assertTrue(isnew) | ||
| self.assertEqual(models.BlockCompletion.objects.count(), 2) | ||
|
|
||
| def test_invalid_completion(self): | ||
| with self.assertRaises(ValidationError): | ||
| models.BlockCompletion.objects.submit_completion( | ||
| user=self.user, | ||
| course_key=self.block_key.course_key, | ||
| block_key=self.block_key, | ||
| completion=1.2 | ||
| ) | ||
| completion = models.BlockCompletion.objects.get(user=self.user, block_key=self.block_key) | ||
| self.assertEqual(completion.completion, 0.5) | ||
| self.assertEqual(models.BlockCompletion.objects.count(), 1) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,9 @@ | |
|
|
||
|
|
||
| class CharNullField(models.CharField): | ||
| """CharField that stores NULL but returns ''""" | ||
| """ | ||
| CharField that stores NULL but returns '' | ||
| """ | ||
|
|
||
| description = "CharField that stores NULL but returns ''" | ||
|
|
||
|
|
@@ -26,3 +28,31 @@ def get_db_prep_value(self, value, connection, prepared=False): | |
| return None | ||
| else: | ||
| return value | ||
|
|
||
|
|
||
| class BigAutoField(models.AutoField): | ||
| """ | ||
| AutoField that uses BigIntegers. | ||
|
|
||
| This exists in Django as of version 1.10. | ||
| """ | ||
|
|
||
| def db_type(self, connection): | ||
| """ | ||
| The type of the field to insert into the database. | ||
| """ | ||
| conn_module = type(connection).__module__ | ||
| if "mysql" in conn_module: | ||
| return "bigint AUTO_INCREMENT" | ||
| elif "postgres" in conn_module: | ||
| return "bigserial" | ||
| else: | ||
| return super(BigAutoField, self).db_type(connection) | ||
|
|
||
| def rel_db_type(self, connection): | ||
| """ | ||
| The type to be used by relations pointing to this field. | ||
|
|
||
| Not used until Django 1.10. | ||
| """ | ||
| return "bigint" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @jcdyer ! Questions:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @doctoryes Good point. The django BigAutoField is signed. It uses @jcdyer I'll give a final review once the tests are fixed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like @doctoryes has asked most of the questions I would care about.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @doctoryes! The back-of-napkin calculation that @bradenmacdonald came up with was 10M users taking 3 courses eaches with 100 completable blocks per course = 3B entries. It will probably take longer than that to get there for a couple reasons:
So we aren't likely to actually hit the cap for significantly longer if at all, but it's close enough to the right ballpark that I think a better-safe-than-sorry policy is warranted. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a note somewhere about the purpose of block_type in the model and its relation to upcoming "aggregation models"?