Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions lms/djangoapps/completion/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from opaque_keys.edx.keys import CourseKey

from openedx.core.djangoapps.xmodule_django.models import CourseKeyField, UsageKeyField
from . import waffle

# pylint: disable=ungrouped-imports
try:
Expand Down Expand Up @@ -52,7 +53,8 @@ def submit_completion(self, user, course_key, block_key, completion):

Return Value:
(BlockCompletion, bool): A tuple comprising the created or updated
BlockCompletion object and a boolean value indicating whether the value
BlockCompletion object and a boolean value indicating whether the
object was newly created by this call.

Raises:

Expand Down Expand Up @@ -84,17 +86,23 @@ def submit_completion(self, user, course_key, block_key, completion):
"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()
if waffle.waffle().is_enabled(waffle.ENABLE_COMPLETION_TRACKING):
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()
else:
# If the feature is not enabled, this method should not be called. Error out with a RuntimeError.
raise RuntimeError(
"BlockCompletion.objects.submit_completion should not be called when the feature is disabled."
)
return obj, isnew


Expand Down
54 changes: 47 additions & 7 deletions lms/djangoapps/completion/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@
Test models, managers, and validators.
"""

from __future__ import absolute_import, division, print_function, unicode_literals

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
from .. import waffle


class PercentValidatorTestCase(TestCase):
Expand All @@ -24,13 +27,8 @@ def test_invalid_percent(self):
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()
class CompletionSetUpMixin(object):
def set_up_completion(self):
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(
Expand All @@ -41,6 +39,19 @@ def setUp(self):
completion=0.5,
)


class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase):
"""
Test that BlockCompletion.objects.submit_completion has the desired
semantics.
"""
def setUp(self):
super(SubmitCompletionTestCase, self).setUp()
self._overrider = waffle.waffle().override(waffle.ENABLE_COMPLETION_TRACKING, True)
self._overrider.__enter__()
self.addCleanup(self._overrider.__exit__, None, None, None)
self.set_up_completion()

def test_changed_value(self):
with self.assertNumQueries(4): # Get, update, 2 * savepoints
completion, isnew = models.BlockCompletion.objects.submit_completion(
Expand Down Expand Up @@ -102,3 +113,32 @@ def test_invalid_completion(self):
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)


class CompletionDisabledTestCase(CompletionSetUpMixin, TestCase):

@classmethod
def setUpClass(cls):
super(CompletionDisabledTestCase, cls).setUpClass()
cls.overrider = waffle.waffle().override(waffle.ENABLE_COMPLETION_TRACKING, False)
cls.overrider.__enter__()

@classmethod
def tearDownClass(cls):
cls.overrider.__exit__(None, None, None)
super(CompletionDisabledTestCase, cls).tearDownClass()

def setUp(self):
super(CompletionDisabledTestCase, self).setUp()
self.set_up_completion()

def test_cannot_call_submit_completion(self):
self.assertEqual(models.BlockCompletion.objects.count(), 1)
with self.assertRaises(RuntimeError):
models.BlockCompletion.objects.submit_completion(
user=self.user,
course_key=self.block_key.course_key,
block_key=self.block_key,
completion=0.9,
)
self.assertEqual(models.BlockCompletion.objects.count(), 1)
20 changes: 20 additions & 0 deletions lms/djangoapps/completion/waffle.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
"""
This module contains various configuration settings via
waffle switches for the completion app.
"""
from __future__ import absolute_import, division, print_function, unicode_literals

from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace

# Namespace
WAFFLE_NAMESPACE = 'completion'

# Switches
ENABLE_COMPLETION_TRACKING = 'enable_completion_tracking'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcdyer: The old Mercury team tended to use Waffle Flags (rather than switches) for everything. I like that the flags enable you to turn things on for single users, or beta-testers, or an individual course (see CourseWaffleFlag), that isn't available with switches. Also, I think our waffle_utils code for flags is a bit cleaner than with switches.

Would you mind reviewing some of the flags here and let let me know what you think?
https://github.com/edx/edx-platform/blob/5ae2bee17f77e345fadef3a0935dc85a890ec405/openedx/features/course_experience/__init__.py

@jcdyer jcdyer Oct 23, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I misunderstand something, waffle flags require a request object to be present. A lot of this code is happening in signals and on models, and so doesn't have a request available. Would you recommend refactoring to block things at the view level or passing a fake request object with the relevant user attached to it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had debated using RequestCache.get_current_request() internally and taking request out of the signature, but wanted to continue to make this explicit.

Would it work to use RequestCache.get_current_request()? I've seen a couple of other examples of using the request cache in models.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you think it's very important, I'd rather not do that. Another client had me spend the last several weeks removing code that was doing something similar in a different codebase, because it was making refactoring and testing a nightmare. There isn't always a current request available (tests, celery tasks, and management commands are three examples), and then to use any of this code in any of those contexts, a fake request needs to be set up by the caller and injected into the request cache. Worse, the fact that this is needed is not evident from the signatures of the methods that check the waffle flag, so you can't work with the code without intimate knowledge of its internals.

If we have enough information available inside all the contexts where the flag is checked to be able to create a fake response inside the public interfaces, then it would make sense. I haven't looked closely enough at waffle to know exactly what it needs from a request. I know we'll have a user and a course available, because it's already part of the submit_completion signature. But I don't know if that's enough, or if not, how much extra work it will be to create a suitable request.

A waffle switch gives us the simple ability to turn the feature on and off without the extra complication, and my understanding was that this was all we needed for this--to be sure we don't start saving data until the feature is complete (or complete enough).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. A switch covers the most important need for now, and hopefully the only need. :)



def waffle():
"""
Returns the namespaced, cached, audited Waffle class for completion.
"""
return WaffleSwitchNamespace(name=WAFFLE_NAMESPACE, log_prefix='completion: ')
106 changes: 89 additions & 17 deletions lms/djangoapps/courseware/module_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
from courseware.model_data import DjangoKeyValueStore, FieldDataCache
from edxmako.shortcuts import render_to_string
from eventtracking import tracker
from lms.djangoapps.completion.models import BlockCompletion
from lms.djangoapps.completion import waffle as completion_waffle
from lms.djangoapps.grades.signals.signals import SCORE_PUBLISHED
from lms.djangoapps.lms_xblock.field_data import LmsFieldData
from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig
Expand Down Expand Up @@ -384,12 +386,23 @@ def get_module_for_descriptor(user, request, descriptor, field_data_cache, cours
)


def get_module_system_for_user(user, student_data, # TODO # pylint: disable=too-many-statements
# Arguments preceding this comment have user binding, those following don't
descriptor, course_id, track_function, xqueue_callback_url_prefix,
request_token, position=None, wrap_xmodule_display=True, grade_bucket_type=None,
static_asset_path='', user_location=None, disable_staff_debug_info=False,
course=None):
def get_module_system_for_user(
user,
student_data, # TODO # pylint: disable=too-many-statements
# Arguments preceding this comment have user binding, those following don't
descriptor,
course_id,
track_function,
xqueue_callback_url_prefix,
request_token,
position=None,
wrap_xmodule_display=True,
grade_bucket_type=None,
static_asset_path='',
user_location=None,
disable_staff_debug_info=False,
course=None
):
"""
Helper function that returns a module system and student_data bound to a user and a descriptor.

Expand Down Expand Up @@ -461,18 +474,26 @@ def inner_get_module(descriptor):
course=course
)

def get_event_handler(event_type):
"""
Return an appropriate function to handle the event.

Returns None if no special processing is required.
"""
handlers = {
'completion': handle_completion_event,
'grade': handle_grade_event,
'progress': handle_deprecated_progress_event,
}
return handlers.get(event_type)

def publish(block, event_type, event):
"""A function that allows XModules to publish events."""
if event_type == 'grade' and not is_masquerading_as_specific_student(user, course_id):
SCORE_PUBLISHED.send(
sender=None,
block=block,
user=user,
raw_earned=event['value'],
raw_possible=event['max_value'],
only_if_higher=event.get('only_if_higher'),
score_deleted=event.get('score_deleted'),
)
"""
A function that allows XModules to publish events.
"""
handle_event = get_event_handler(event_type)
if handle_event and not is_masquerading_as_specific_student(user, course_id):
handle_event(block, event)
else:
context = contexts.course_context_from_course_id(course_id)
if block.runtime.user_id:
Expand All @@ -486,6 +507,57 @@ def publish(block, event_type, event):
with tracker.get_tracker().context(event_type, context):
track_function(event_type, event)

def handle_completion_event(block, event):
"""
Submit a completion object for the block.
"""
if not completion_waffle.waffle().is_enabled(completion_waffle.ENABLE_COMPLETION_TRACKING):
raise Http404
else:
BlockCompletion.objects.submit_completion(
user=user,
course_key=course_id,
block_key=block.scope_ids.usage_id,
completion=event['completion'],
)

def handle_grade_event(block, event):
"""
Submit a grade for the block.
"""
SCORE_PUBLISHED.send(
sender=None,
block=block,
user=user,
raw_earned=event['value'],
raw_possible=event['max_value'],
only_if_higher=event.get('only_if_higher'),
score_deleted=event.get('score_deleted'),
)

def handle_deprecated_progress_event(block, event):
"""
DEPRECATED: Submit a completion for the block represented by the
progress event.

This exists to support the legacy progress extension used by
edx-solutions. New XBlocks should not emit these events, but instead
emit completion events directly.
"""
if not completion_waffle.waffle().is_enabled(completion_waffle.ENABLE_COMPLETION_TRACKING):
raise Http404
else:
requested_user_id = event.get('user_id', user.id)
if requested_user_id != user.id:
log.warning("{} tried to submit a completion on behalf of {}".format(user, requested_user_id))
return
BlockCompletion.objects.submit_completion(
user=user,
course_key=course_id,
block_key=block.scope_ids.usage_id,
completion=1.0,
)

def rebind_noauth_module_to_user(module, real_user):
"""
A function that allows a module to get re-bound to a real user if it was previously bound to an AnonymousUser.
Expand Down
Loading