From 1247dbcb39a28ece4ff0278d331cde15a704eab5 Mon Sep 17 00:00:00 2001 From: Eugene Dyudyunov Date: Wed, 7 Sep 2022 15:11:45 +0300 Subject: [PATCH 1/5] fix: main page course listing The course is visible on the main page right after creation. So anonymous users can see them and access the course about page for the courses without valid data (e.g. they will see the default course overview) When courses list filtering is processed it checks the `see_exists` permission for the anonymous user. Actually, `see_exists` means `can_load` OR `can_enroll`. `can_load` is False in our case because the course start in the future. But `can_enroll` returns True because the course's enrollment_start and enrollment_end dates are blank: ``` enrollment_start = courselike.enrollment_start or datetime.min.replace(tzinfo=UTC) enrollment_end = courselike.enrollment_end or datetime.max.replace(tzinfo=UTC) if enrollment_start < now < enrollment_end: debug("Allow: in enrollment period") return ACCESS_GRANTED ``` Set the enrollment_start the same as a course start by default - this fix will work for the course catalog page when the course discovery is disabled (FEATURES['ENABLE_COURSE_DISCOVERY'] = False) - I wonder why DEFAULT_START_DATE is hardcoded (1 Jan 2030)? How and when is it updated? --- .../core/djangoapps/models/tests/test_course_details.py | 5 ++--- xmodule/course_block.py | 6 +++++- xmodule/modulestore/tests/factories.py | 5 +++++ xmodule/tests/test_course_block.py | 8 ++++++-- 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/models/tests/test_course_details.py b/openedx/core/djangoapps/models/tests/test_course_details.py index 98e3c4b15df0..d183c1a4b2aa 100644 --- a/openedx/core/djangoapps/models/tests/test_course_details.py +++ b/openedx/core/djangoapps/models/tests/test_course_details.py @@ -29,7 +29,7 @@ class CourseDetailsTestCase(ModuleStoreTestCase): def setUp(self): super().setUp() - self.course = CourseFactory.create() + self.course = CourseFactory.create(default_enrollment_start=True) def test_virgin_fetch(self): details = CourseDetails.fetch(self.course.id) @@ -39,8 +39,7 @@ def test_virgin_fetch(self): assert details.course_image_name == self.course.course_image assert details.start_date.tzinfo is not None assert details.end_date is None, ('end date somehow initialized ' + str(details.end_date)) - assert details.enrollment_start is None,\ - ('enrollment_start date somehow initialized ' + str(details.enrollment_start)) + assert details.enrollment_start == self.course.enrollment_start, 'enrollment_start not copied into' assert details.enrollment_end is None,\ ('enrollment_end date somehow initialized ' + str(details.enrollment_end)) assert details.certificate_available_date is None,\ diff --git a/xmodule/course_block.py b/xmodule/course_block.py index 6ef625086bdd..89916d4230fd 100644 --- a/xmodule/course_block.py +++ b/xmodule/course_block.py @@ -315,7 +315,11 @@ class CourseFields: # lint-amnesty, pylint: disable=missing-class-docstring ) wiki_slug = String(help=_("Slug that points to the wiki for this course"), scope=Scope.content) - enrollment_start = Date(help=_("Date that enrollment for this class is opened"), scope=Scope.settings) + enrollment_start = Date( + help=_("Date that enrollment for this class is opened"), + default=DEFAULT_START_DATE, + scope=Scope.settings + ) enrollment_end = Date(help=_("Date that enrollment for this class is closed"), scope=Scope.settings) start = Date( help=_("Start time when this block is visible"), diff --git a/xmodule/modulestore/tests/factories.py b/xmodule/modulestore/tests/factories.py index b740b10e053f..9e0a2aa5cfd1 100644 --- a/xmodule/modulestore/tests/factories.py +++ b/xmodule/modulestore/tests/factories.py @@ -123,6 +123,11 @@ def _create(cls, target_class, **kwargs): # lint-amnesty, pylint: disable=argum run = kwargs.pop('run', name) user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test) emit_signals = kwargs.pop('emit_signals', False) + # By default course has enrollment_start in the future which means course is closed for enrollment. + # We're setting the 'enrollment_start' field to None to reduce number of arguments needed to setup course. + # Use the 'default_enrollment_start=True' kwarg to skip this and use the default enrollment_start date. + if not kwargs.get('enrollment_start', kwargs.pop('default_enrollment_start', False)): + kwargs['enrollment_start'] = None # Pass the metadata just as field=value pairs kwargs.update(kwargs.pop('metadata', {})) diff --git a/xmodule/tests/test_course_block.py b/xmodule/tests/test_course_block.py index 0adca423695a..94b3cc0ef48d 100644 --- a/xmodule/tests/test_course_block.py +++ b/xmodule/tests/test_course_block.py @@ -18,6 +18,7 @@ from openedx.core.lib.teams_config import TeamsConfig, DEFAULT_COURSE_RUN_MAX_TEAM_SIZE import xmodule.course_block +from xmodule.course_metadata_utils import DEFAULT_START_DATE from xmodule.data import CertificatesDisplayBehaviors from xmodule.modulestore.xml import ImportSystem, XMLModuleStore from xmodule.modulestore.exceptions import InvalidProctoringProvider @@ -32,10 +33,13 @@ _NEXT_WEEK = _TODAY + timedelta(days=7) -class CourseFieldsTestCase(unittest.TestCase): +class CourseFieldsTestCase(unittest.TestCase): # lint-amnesty, pylint: disable=missing-class-docstring def test_default_start_date(self): - assert xmodule.course_block.CourseFields.start.default == datetime(2030, 1, 1, tzinfo=utc) + assert xmodule.course_block.CourseFields.start.default == DEFAULT_START_DATE + + def test_default_enrollment_start_date(self): + assert xmodule.course_block.CourseFields.enrollment_start.default == DEFAULT_START_DATE class DummySystem(ImportSystem): # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring From af6b86ddba00b68c44700c7d458f8d4c526e71ce Mon Sep 17 00:00:00 2001 From: Eugene Dyudyunov Date: Fri, 23 Jun 2023 11:10:54 +0300 Subject: [PATCH 2/5] feat: make default enrollment start togglable --- .../models/tests/test_course_details.py | 48 ++++++++++++------- xmodule/course_block.py | 14 +++++- xmodule/tests/test_course_block.py | 16 +++++-- 3 files changed, 56 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/models/tests/test_course_details.py b/openedx/core/djangoapps/models/tests/test_course_details.py index d183c1a4b2aa..86301740c7f1 100644 --- a/openedx/core/djangoapps/models/tests/test_course_details.py +++ b/openedx/core/djangoapps/models/tests/test_course_details.py @@ -4,6 +4,7 @@ import datetime +from django.test import override_settings import pytest import ddt from pytz import UTC @@ -31,24 +32,35 @@ def setUp(self): super().setUp() self.course = CourseFactory.create(default_enrollment_start=True) - def test_virgin_fetch(self): - details = CourseDetails.fetch(self.course.id) - assert details.org == self.course.location.org, 'Org not copied into' - assert details.course_id == self.course.location.course, 'Course_id not copied into' - assert details.run == self.course.location.run, 'Course run not copied into' - assert details.course_image_name == self.course.course_image - assert details.start_date.tzinfo is not None - assert details.end_date is None, ('end date somehow initialized ' + str(details.end_date)) - assert details.enrollment_start == self.course.enrollment_start, 'enrollment_start not copied into' - assert details.enrollment_end is None,\ - ('enrollment_end date somehow initialized ' + str(details.enrollment_end)) - assert details.certificate_available_date is None,\ - ('certificate_available_date date somehow initialized ' + str(details.certificate_available_date)) - assert details.syllabus is None, ('syllabus somehow initialized' + str(details.syllabus)) - assert details.intro_video is None, ('intro_video somehow initialized' + str(details.intro_video)) - assert details.effort is None, ('effort somehow initialized' + str(details.effort)) - assert details.language is None, ('language somehow initialized' + str(details.language)) - assert not details.self_paced + @ddt.data(True, False) + def test_virgin_fetch(self, should_have_default_enroll_start): + features = settings.FEATURES.copy() + features['CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE'] = should_have_default_enroll_start + + with override_settings(FEATURES=features): + course = CourseFactory.create(default_enrollment_start=should_have_default_enroll_start) + details = CourseDetails.fetch(course.id) + wrong_enrollment_start_msg = ( + 'enrollment_start not copied into' + if should_have_default_enroll_start + else f'enrollment_start date somehow initialized {str(details.enrollment_start)}' + ) + assert details.org == course.location.org, 'Org not copied into' + assert details.course_id == course.location.course, 'Course_id not copied into' + assert details.run == course.location.run, 'Course run not copied into' + assert details.course_image_name == course.course_image + assert details.start_date.tzinfo is not None + assert details.end_date is None, ('end date somehow initialized ' + str(details.end_date)) + assert details.enrollment_start == course.enrollment_start, wrong_enrollment_start_msg + assert details.enrollment_end is None,\ + ('enrollment_end date somehow initialized ' + str(details.enrollment_end)) + assert details.certificate_available_date is None,\ + ('certificate_available_date date somehow initialized ' + str(details.certificate_available_date)) + assert details.syllabus is None, ('syllabus somehow initialized' + str(details.syllabus)) + assert details.intro_video is None, ('intro_video somehow initialized' + str(details.intro_video)) + assert details.effort is None, ('effort somehow initialized' + str(details.effort)) + assert details.language is None, ('language somehow initialized' + str(details.language)) + assert not details.self_paced def test_update_and_fetch(self): jsondetails = CourseDetails.fetch(self.course.id) diff --git a/xmodule/course_block.py b/xmodule/course_block.py index 89916d4230fd..e46f6b256ff4 100644 --- a/xmodule/course_block.py +++ b/xmodule/course_block.py @@ -11,6 +11,7 @@ import requests from django.conf import settings from django.core.validators import validate_email +from edx_toggles.toggles import SettingDictToggle from lazy import lazy from lxml import etree from path import Path as path @@ -57,6 +58,17 @@ COURSE_VIDEO_SHARING_PER_VIDEO = 'per-video' COURSE_VIDEO_SHARING_ALL_VIDEOS = 'all-on' COURSE_VIDEO_SHARING_NONE = 'all-off' +# .. toggle_name: FEATURES['CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE'] +# .. toggle_implementation: SettingDictToggle +# .. toggle_default: False +# .. toggle_description: When enabled the newly created courses will have the enrollment_start_date +# set to DEFAULT_START_DATE. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2023-06-22 +# .. toggle_target_removal_date: None +CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE = SettingDictToggle( + "FEATURES", "CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE", default=False, module_name=__name__ +) class StringOrDate(Date): # lint-amnesty, pylint: disable=missing-class-docstring @@ -317,7 +329,7 @@ class CourseFields: # lint-amnesty, pylint: disable=missing-class-docstring wiki_slug = String(help=_("Slug that points to the wiki for this course"), scope=Scope.content) enrollment_start = Date( help=_("Date that enrollment for this class is opened"), - default=DEFAULT_START_DATE, + default=DEFAULT_START_DATE if CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE.is_enabled() else None, scope=Scope.settings ) enrollment_end = Date(help=_("Date that enrollment for this class is closed"), scope=Scope.settings) diff --git a/xmodule/tests/test_course_block.py b/xmodule/tests/test_course_block.py index 94b3cc0ef48d..6b689e39f41e 100644 --- a/xmodule/tests/test_course_block.py +++ b/xmodule/tests/test_course_block.py @@ -4,15 +4,16 @@ import itertools import unittest from datetime import datetime, timedelta +import sys from unittest.mock import Mock, patch -import pytest import ddt from dateutil import parser from django.conf import settings from django.test import override_settings from fs.memoryfs import MemoryFS from opaque_keys.edx.keys import CourseKey +import pytest from pytz import utc from xblock.runtime import DictKeyValueStore, KvsFieldData @@ -33,13 +34,22 @@ _NEXT_WEEK = _TODAY + timedelta(days=7) +@ddt.ddt() class CourseFieldsTestCase(unittest.TestCase): # lint-amnesty, pylint: disable=missing-class-docstring def test_default_start_date(self): assert xmodule.course_block.CourseFields.start.default == DEFAULT_START_DATE - def test_default_enrollment_start_date(self): - assert xmodule.course_block.CourseFields.enrollment_start.default == DEFAULT_START_DATE + @ddt.data(True, False) + def test_default_enrollment_start_date(self, should_have_default_enroll_start): + features = settings.FEATURES.copy() + features['CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE'] = should_have_default_enroll_start + with override_settings(FEATURES=features): + # reimport, so settings override could take effect + del sys.modules['xmodule.course_block'] + import xmodule.course_block + expected = DEFAULT_START_DATE if should_have_default_enroll_start else None + assert xmodule.course_block.CourseFields.enrollment_start.default == expected class DummySystem(ImportSystem): # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring From fe84a4eeaff37a44059b247b8ea969af7d764920 Mon Sep 17 00:00:00 2001 From: Eugene Dyudyunov Date: Fri, 23 Jun 2023 14:17:01 +0300 Subject: [PATCH 3/5] fix: disable linting warnings for reimport --- xmodule/tests/test_course_block.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xmodule/tests/test_course_block.py b/xmodule/tests/test_course_block.py index 6b689e39f41e..c863fa07670e 100644 --- a/xmodule/tests/test_course_block.py +++ b/xmodule/tests/test_course_block.py @@ -47,7 +47,7 @@ def test_default_enrollment_start_date(self, should_have_default_enroll_start): with override_settings(FEATURES=features): # reimport, so settings override could take effect del sys.modules['xmodule.course_block'] - import xmodule.course_block + import xmodule.course_block # lint-amnesty, pylint: disable=redefined-outer-name, reimported expected = DEFAULT_START_DATE if should_have_default_enroll_start else None assert xmodule.course_block.CourseFields.enrollment_start.default == expected From caee0a4521fbc255296666b44c4ca352fa07d5c8 Mon Sep 17 00:00:00 2001 From: Eugene Dyudyunov Date: Wed, 16 Aug 2023 10:09:27 +0300 Subject: [PATCH 4/5] docs: format toggle annotation --- xmodule/course_block.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/xmodule/course_block.py b/xmodule/course_block.py index e46f6b256ff4..102b428647fe 100644 --- a/xmodule/course_block.py +++ b/xmodule/course_block.py @@ -61,11 +61,12 @@ # .. toggle_name: FEATURES['CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE'] # .. toggle_implementation: SettingDictToggle # .. toggle_default: False -# .. toggle_description: When enabled the newly created courses will have the enrollment_start_date -# set to DEFAULT_START_DATE. +# .. toggle_description: The default behavior, when this is disabled, is that newly created course has no +# enrollment_start date set. When the feature is enabled - the newly created courses will have the +# enrollment_start_date set to DEFAULT_START_DATE. # .. toggle_use_cases: open_edx # .. toggle_creation_date: 2023-06-22 -# .. toggle_target_removal_date: None +# .. toggle_target_removal_date: This is intended to be a permanent option. CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE = SettingDictToggle( "FEATURES", "CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE", default=False, module_name=__name__ ) From a581223747a82339e0595f0991a7231347838883 Mon Sep 17 00:00:00 2001 From: Eugene Dyudyunov Date: Wed, 16 Aug 2023 17:44:53 +0300 Subject: [PATCH 5/5] docs: update toggle docstring --- xmodule/course_block.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/xmodule/course_block.py b/xmodule/course_block.py index 102b428647fe..bb08e252b0f7 100644 --- a/xmodule/course_block.py +++ b/xmodule/course_block.py @@ -61,12 +61,15 @@ # .. toggle_name: FEATURES['CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE'] # .. toggle_implementation: SettingDictToggle # .. toggle_default: False -# .. toggle_description: The default behavior, when this is disabled, is that newly created course has no +# .. toggle_description: The default behavior, when this is disabled, is that a newly created course has no # enrollment_start date set. When the feature is enabled - the newly created courses will have the -# enrollment_start_date set to DEFAULT_START_DATE. +# enrollment_start_date set to DEFAULT_START_DATE. This is intended to be a permanent option. +# This toggle affects the course listing pages (platform's index page, /courses page) when course search is +# performed using the `lms.djangoapp.branding.get_visible_courses` method and the +# COURSE_CATALOG_VISIBILITY_PERMISSION setting is set to 'see_exists'. Switching the toggle to True will prevent +# the newly created (empty) course from appearing in the course listing. # .. toggle_use_cases: open_edx # .. toggle_creation_date: 2023-06-22 -# .. toggle_target_removal_date: This is intended to be a permanent option. CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE = SettingDictToggle( "FEATURES", "CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE", default=False, module_name=__name__ )