diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 7e4c98c..23ed18d 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -17,10 +17,3 @@ auth.User: ".. pii_retirement:" : consumer_api contenttypes.ContentType: ".. no_pii:": "This model has no PII" -# Via waffle -waffle.Flag: - ".. no_pii:": "No PII" -waffle.Sample: - ".. no_pii:": "No PII" -waffle.Switch: - ".. no_pii:": "No PII" diff --git a/edx_when/__init__.py b/edx_when/__init__.py index 3302d9c..7687aaa 100644 --- a/edx_when/__init__.py +++ b/edx_when/__init__.py @@ -4,6 +4,6 @@ from __future__ import absolute_import, unicode_literals -__version__ = '0.7.1' +__version__ = '1.0.0' default_app_config = 'edx_when.apps.EdxWhenConfig' # pylint: disable=invalid-name diff --git a/edx_when/api.py b/edx_when/api.py index 83a8c60..ee7a893 100644 --- a/edx_when/api.py +++ b/edx_when/api.py @@ -25,19 +25,25 @@ def _ensure_key(key_class, key_obj): return key_obj -def is_enabled_for_course(course_key): +def _are_relative_dates_enabled(course_key=None): """ - Return whether edx-when is enabled for this course. + Return whether it's OK to consider relative dates. If not, pretend those database entries don't exist. """ - return models.ContentDate.objects.filter(course_id=course_key, active=True).exists() + try: + # It's bad form to depend on LMS code from inside a plugin like this. But we gracefully fail, and this is + # temporary code anyway, while we develop this feature. + from openedx.features.course_experience import RELATIVE_DATES_FLAG + except ImportError: + return False + return RELATIVE_DATES_FLAG.is_enabled(course_key) -def override_enabled(): + +def is_enabled_for_course(course_key): """ - Return decorator that enables edx-when. + Return whether edx-when is enabled for this course. """ - from waffle.testutils import override_flag - return override_flag('edx-when-enabled', active=True) + return models.ContentDate.objects.filter(course_id=course_key, active=True).exists() def set_dates_for_course(course_key, items): @@ -72,8 +78,16 @@ def get_dates_for_course(course_id, user=None, use_cached=True, schedule=None): key: block location, field name value: datetime object + + Arguments: + course_id: either a CourseKey or string representation of same + user: None, an int, or a User object + use_cached: will skip cache lookups (but not saves) if False + schedule: optional override for a user's enrollment Schedule, used for relative date calculations """ + course_id = _ensure_key(CourseKey, course_id) log.debug("Getting dates for %s as %s", course_id, user) + allow_relative_dates = _are_relative_dates_enabled(course_id) cache_key = 'course_dates.%s' % course_id if user: @@ -84,13 +98,18 @@ def get_dates_for_course(course_id, user=None, use_cached=True, schedule=None): cache_key += '.%s' % user_id else: user_id = None + if schedule: + cache_key += '.schedule-%s' % schedule.start_date + if allow_relative_dates: + cache_key += '.with-rel-dates' dates = DEFAULT_REQUEST_CACHE.data.get(cache_key, None) if use_cached and dates is not None: return dates - course_id = _ensure_key(CourseKey, course_id) - qset = models.ContentDate.objects.filter(course_id=course_id, active=True).select_related('policy') + rel_lookup = {} if allow_relative_dates else {'policy__rel_date': None} + qset = models.ContentDate.objects.filter(course_id=course_id, active=True, **rel_lookup).select_related('policy') + dates = {} policies = {} for cdate in qset: @@ -106,11 +125,14 @@ def get_dates_for_course(course_id, user=None, use_cached=True, schedule=None): except ValueError: log.warning("Unable to read date for %s", cdate.location, exc_info=True) policies[cdate.id] = key + if user_id: + rel_lookup = {} if allow_relative_dates else {'content_date__policy__rel_date': None, 'rel_date': None} for userdate in models.UserDate.objects.filter( user_id=user_id, content_date__course_id=course_id, - content_date__active=True + content_date__active=True, + **rel_lookup, ).select_related( 'content_date', 'content_date__policy' ).order_by('modified'): @@ -127,6 +149,11 @@ def get_dates_for_course(course_id, user=None, use_cached=True, schedule=None): def get_date_for_block(course_id, block_id, name='due', user=None): """ Return the date for block in the course for the (optional) user. + + Arguments: + course_id: either a CourseKey or string representation of same + block_id: either a UsageKey or string representation of same + user: None, an int, or a User object """ try: return get_dates_for_course(course_id, user).get((_ensure_key(UsageKey, block_id), name), None) @@ -138,15 +165,24 @@ def get_overrides_for_block(course_id, block_id): """ Return list of date overrides for a block. - list of (username, full_name, date) + Arguments: + course_id: either a CourseKey or string representation of same + block_id: either a UsageKey or string representation of same + + Returns: + list of (username, full_name, date) """ course_id = _ensure_key(CourseKey, course_id) block_id = _ensure_key(UsageKey, block_id) + allow_relative_dates = _are_relative_dates_enabled(course_id) + rel_lookup = {} if allow_relative_dates else {'content_date__policy__rel_date': None, 'rel_date': None} query = models.UserDate.objects.filter( - content_date__course_id=course_id, - content_date__location=block_id, - content_date__active=True).order_by('-modified') + content_date__course_id=course_id, + content_date__location=block_id, + content_date__active=True, + **rel_lookup, + ).order_by('-modified') dates = [] users = set() for udate in query: @@ -168,14 +204,23 @@ def get_overrides_for_user(course_id, user): """ Return all user date overrides for a particular course. - iterator of {'location': location, 'actual_date': date} + Arguments: + course_id: either a CourseKey or string representation of same + user: a User object + + Returns: + iterator of {'location': location, 'actual_date': date} """ course_id = _ensure_key(CourseKey, course_id) + allow_relative_dates = _are_relative_dates_enabled(course_id) + rel_lookup = {} if allow_relative_dates else {'abs_date__isnull': False} query = models.UserDate.objects.filter( - content_date__course_id=course_id, - user=user, - content_date__active=True).order_by('-modified') + content_date__course_id=course_id, + user=user, + content_date__active=True, + **rel_lookup, + ).order_by('-modified') blocks = set() for udate in query: if udate.content_date.location in blocks: diff --git a/edx_when/models.py b/edx_when/models.py index 0b01bfd..155e9e3 100644 --- a/edx_when/models.py +++ b/edx_when/models.py @@ -25,7 +25,7 @@ @python_2_unicode_compatible class DatePolicy(TimeStampedModel): """ - TODO: replace with a brief description of the model. + Stores a date (either absolute or relative). .. no_pii: """ @@ -67,7 +67,7 @@ def clean(self): @python_2_unicode_compatible class ContentDate(models.Model): """ - TODO: replace with a brief description of the model. + Ties a DatePolicy to a specific piece of course content. (e.g. a due date for a homework). .. no_pii: """ @@ -110,7 +110,7 @@ def schedule_for_user(self, user): @python_2_unicode_compatible class UserDate(TimeStampedModel): """ - TODO: replace with a brief description of the model. + Stores a user-specific date override for a given ContentDate. .. no_pii: """ diff --git a/test_settings.py b/test_settings.py index a31c83b..1828e87 100644 --- a/test_settings.py +++ b/test_settings.py @@ -32,7 +32,6 @@ def root(*args): 'django.contrib.auth', 'django.contrib.contenttypes', 'edx_when', - 'waffle', 'tests.test_models_app', ) diff --git a/tests/test_api.py b/tests/test_api.py index 2c6210a..3bb0e4d 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -3,6 +3,7 @@ """ from __future__ import absolute_import, unicode_literals +import sys from datetime import datetime, timedelta import ddt @@ -10,7 +11,8 @@ from django.contrib.auth.models import User from django.test import TestCase from edx_django_utils.cache.utils import DEFAULT_REQUEST_CACHE -from mock import patch +from mock import Mock, patch +from opaque_keys.edx.locator import CourseLocator from edx_when import api, models from test_utils import make_block_id, make_items @@ -42,6 +44,10 @@ def setUp(self): dummy_schedule_patcher.start() self.addCleanup(dummy_schedule_patcher.stop) + relative_dates_patcher = patch('edx_when.api._are_relative_dates_enabled', return_value=True) + relative_dates_patcher.start() + self.addCleanup(relative_dates_patcher.stop) + DEFAULT_REQUEST_CACHE.clear() def test_set_dates_for_course(self): @@ -213,3 +219,89 @@ def test_is_enabled(self): assert not api.is_enabled_for_course(course_id) api.set_dates_for_course(course_id, items) assert api.is_enabled_for_course(course_id) + + def test_allow_relative_dates(self): + course_key = CourseLocator('testX', 'tt101', '2019') + block1 = make_block_id(course_key) + date1 = datetime(2019, 3, 22) + block2 = make_block_id(course_key) + date2 = datetime(2019, 3, 23) + date2_override_delta = timedelta(days=10) + date2_override = date2 + date2_override_delta + block3 = make_block_id(course_key) + date3_delta = timedelta(days=1) + date3 = self.schedule.start_date + date3_delta + block4 = make_block_id(course_key) + date4_delta = timedelta(days=2) + date4 = self.schedule.start_date + date4_delta + date4_override = datetime(2019, 4, 24) + items = [ + (block1, {'due': date1}), # absolute + (block2, {'due': date2}), # absolute, to be overwritten by relative date + (block3, {'due': date3_delta}), # relative + (block4, {'due': date4_delta}), # relative, to be overwritten by absolute date + ] + api.set_dates_for_course(course_key, items) + api.set_date_for_block(course_key, block2, 'due', date2_override_delta, user=self.user) + api.set_date_for_block(course_key, block4, 'due', date4_override, user=self.user) + + # get_dates_for_course + dates = [ + ((block1, 'due'), date1), + ((block2, 'due'), date2), + ((block3, 'due'), date3), + ((block4, 'due'), date4), + ] + assert api.get_dates_for_course(course_key, schedule=self.schedule) == dict(dates) + with patch('edx_when.api._are_relative_dates_enabled', return_value=False): + assert api.get_dates_for_course(course_key, schedule=self.schedule) == dict(dates[0:2]) + assert api.get_dates_for_course(course_key, schedule=self.schedule, user=self.user) == dict(dates[0:2]) + + # get_date_for_block + assert api.get_date_for_block(course_key, block2) == date2 + assert api.get_date_for_block(course_key, block4, user=self.user) == date4_override + with patch('edx_when.api._are_relative_dates_enabled', return_value=False): + assert api.get_date_for_block(course_key, block2) == date2 + assert api.get_date_for_block(course_key, block1, user=self.user) == date1 + assert api.get_date_for_block(course_key, block2, user=self.user) == date2 + assert api.get_date_for_block(course_key, block4, user=self.user) is None + + # get_overrides_for_block + block2_overrides = [(self.user.username, 'unknown', date2_override)] + assert api.get_overrides_for_block(course_key, block2) == block2_overrides + with patch('edx_when.api._are_relative_dates_enabled', return_value=False): + assert api.get_overrides_for_block(course_key, block2) == [] + + # get_overrides_for_user + user_overrides = [ + {'location': block4, 'actual_date': date4_override}, + {'location': block2, 'actual_date': date2_override}, + ] + assert list(api.get_overrides_for_user(course_key, self.user)) == user_overrides + with patch('edx_when.api._are_relative_dates_enabled', return_value=False): + assert list(api.get_overrides_for_user(course_key, self.user)) == [user_overrides[0]] + + +class ApiWaffleTests(TestCase): + """ + Tests for edx_when.api waffle usage. + + These are isolated because they have pretty different patch requirements. + """ + @patch.dict(sys.modules, {'openedx.features.course_experience': Mock()}) + def test_relative_dates_enabled(self): + from openedx.features.course_experience import RELATIVE_DATES_FLAG as mock_flag # pylint: disable=import-error + mock_flag.is_enabled.return_value = True + assert api._are_relative_dates_enabled() # pylint: disable=protected-access + assert mock_flag.is_enabled.called + + @patch.dict(sys.modules, {'openedx.features.course_experience': Mock()}) + def test_relative_dates_disabled(self): + from openedx.features.course_experience import RELATIVE_DATES_FLAG as mock_flag # pylint: disable=import-error + mock_flag.is_enabled.return_value = False + assert not api._are_relative_dates_enabled() # pylint: disable=protected-access + assert mock_flag.is_enabled.called + + @patch.dict(sys.modules, {'openedx.features.course_experience': None}) + def test_relative_dates_import_error(self): + assert not api._are_relative_dates_enabled() # pylint: disable=protected-access diff --git a/tests/test_xblock_services.py b/tests/test_xblock_services.py index 334d641..7d22dcf 100644 --- a/tests/test_xblock_services.py +++ b/tests/test_xblock_services.py @@ -61,7 +61,6 @@ class TestFieldData(XblockTests): Tests for the FieldData subclass. """ - @api.override_enabled() def test_field_data_get(self): defaults = mock.MagicMock() dfd = field_data.DateLookupFieldData(defaults, course_id=self.course_id, use_cached=False, user=self.user) @@ -128,7 +127,8 @@ def test_collect(self): field_data.DateOverrideTransformer.collect(block_structure) assert block_structure.request_xblock_fields.called_once_with('due', 'start') - def test_transform(self): + @mock.patch('edx_when.api._are_relative_dates_enabled', return_value=True) + def test_transform(self, _mock): override = datetime.datetime(2020, 1, 1) api.set_date_for_block(self.items[0][0].course_key, self.items[0][0], 'due', override, user=self.user) usage_info = mock.MagicMock()