diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index c7fbdb20ea..815e4b3f47 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -830,6 +830,13 @@ def user_post_save_callback(sender, **kwargs): cea.save() continue + # Skip auto enrollment of user if enrollment is not open for the course + # We are checking this here instead of passing check_access=True to CourseEnrollment.enroll() + # as we want to skip course full check. + if CourseEnrollment.is_enrollment_closed(user, CourseOverview.get_from_id(cea.course_id)): + log.info(f'Skipping auto enrollment of user as enrollment for course {cea.course_id} has ended') + continue + enrollment = CourseEnrollment.enroll(user, cea.course_id) manual_enrollment_audit = ManualEnrollmentAudit.get_manual_enrollment_by_email(user.email) diff --git a/common/djangoapps/student/tests/test_events.py b/common/djangoapps/student/tests/test_events.py index a8b70ea4b8..ac86cb1770 100644 --- a/common/djangoapps/student/tests/test_events.py +++ b/common/djangoapps/student/tests/test_events.py @@ -26,6 +26,7 @@ from openedx_events.learning.signals import ( # lint-amnesty, pylint: disable=w COURSE_UNENROLLMENT_COMPLETED, ) from openedx_events.tests.utils import OpenEdxEventsTestMixin # lint-amnesty, pylint: disable=wrong-import-order +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangolib.testing.utils import skip_unless_lms from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order @@ -181,6 +182,10 @@ class TestUserEvents(UserSettingsEventTestMixin, TestCase): """ pending_enrollment = CourseEnrollmentAllowedFactory(auto_enroll=True) # lint-amnesty, pylint: disable=unused-variable + # Create a CourseOverview for the enrollment course + course_overview = CourseOverviewFactory.create(id=pending_enrollment.course_id) + course_overview.save() + # the e-mail will change to test@edx.org (from something else) assert self.user.email != 'test@edx.org' diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index fd90a2f37e..470135f609 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -8,6 +8,7 @@ import pytz from crum import set_current_request from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user from django.core.cache import cache +from django.conf import settings from django.db.models import signals # pylint: disable=unused-import from django.db.models.functions import Lower from django.test import TestCase, override_settings @@ -39,6 +40,7 @@ from lms.djangoapps.courseware.toggles import ( COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES_STREAK_CELEBRATION, ) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.schedules.models import Schedule from openedx.core.djangoapps.user_api.preferences.api import set_user_preference from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -760,6 +762,42 @@ class TestUserPostSaveCallback(SharedModuleStoreTestCase): assert actual_student.is_active is True assert actual_cea.user == student + @ddt.data(False, True) + def test_auto_enrollment_if_course_enrollment_closed(self, feature_enabled): + """ + Test the following scenarios + + 1. Invited students who register when enrollment is closed are not enrolled if + DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED is True + + 2. Invited students who register when enrollment is closed are enrolled if + DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED is False + """ + + def register_and_enroll_student(): + student = self._set_up_invited_student( + course=self.course, + active=False, + enrolled=False + ) + student.is_active = True + # trigger the post_save callback + student.save() + return CourseEnrollment.get_enrollment(student, self.course.id) + + # Set enrollment end date to a past date so that enrollment is ended + enrollment_end = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=2) + course_overview = CourseOverviewFactory.create(id=self.course.id, enrollment_end=enrollment_end) + course_overview.save() + + if feature_enabled: + with override_settings( + FEATURES={**settings.FEATURES, 'DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED': True} + ): + assert register_and_enroll_student() is None + else: + assert register_and_enroll_student() is not None + def test_verified_student_not_downgraded_when_changing_email(self): """ Make sure that verified students do not get downgrade if they are active + changing their email. diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index fe9db5db56..a6750122cd 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -13,13 +13,11 @@ Note: The access control logic in this file does NOT check for enrollment in import logging -from datetime import datetime from django.conf import settings # pylint: disable=unused-import from django.contrib.auth.models import AnonymousUser from edx_django_utils.monitoring import function_trace from opaque_keys.edx.keys import CourseKey, UsageKey -from pytz import UTC from xblock.core import XBlock from lms.djangoapps.courseware.access_response import ( @@ -250,22 +248,35 @@ def _can_enroll_courselike(user, courselike): # which actually points to a CourseKey. Sigh. course_key = courselike.id + course_enrollment_open = courselike.is_enrollment_open() + + user_has_staff_access = _has_staff_access_to_descriptor(user, courselike, course_key) + # If the user appears in CourseEnrollmentAllowed paired with the given course key, # they may enroll, except if the CEA has already been used by a different user. # Note that as dictated by the legacy database schema, the filter call includes # a `course_id` kwarg which requires a CourseKey. if user is not None and user.is_authenticated: cea = CourseEnrollmentAllowed.objects.filter(email=user.email, course_id=course_key).first() - if cea and cea.valid_for_user(user): - return ACCESS_GRANTED - elif cea: - debug("Deny: CEA was already consumed by a different user {} and can't be used again by {}".format( - cea.user.id, - user.id, - )) - return ACCESS_DENIED + if cea: + # DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED flag is used to disable enrollment for user invited + # to a course if user is registering when the course enrollment is closed + if ( + settings.FEATURES.get('DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED') and + not course_enrollment_open and + not user_has_staff_access + ): + return ACCESS_DENIED + elif cea.valid_for_user(user): + return ACCESS_GRANTED + else: + debug("Deny: CEA was already consumed by a different user {} and can't be used again by {}".format( + cea.user.id, + user.id, + )) + return ACCESS_DENIED - if _has_staff_access_to_descriptor(user, courselike, course_key): + if user_has_staff_access: return ACCESS_GRANTED # Access denied when the course requires an invitation @@ -273,10 +284,7 @@ def _can_enroll_courselike(user, courselike): debug("Deny: invitation only") return ACCESS_DENIED - now = datetime.now(UTC) - 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: + if course_enrollment_open: debug("Allow: in enrollment period") return ACCESS_GRANTED diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 2da5954966..9b6e0e718f 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -11,6 +11,7 @@ import pytest import ddt import pytz from ccx_keys.locator import CCXLocator +from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.test import TestCase from django.test.client import RequestFactory @@ -26,6 +27,7 @@ from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase, mas from lms.djangoapps.courseware.toggles import course_is_invitation_only from lms.djangoapps.ccx.models import CustomCourseForEdX from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES from openedx.features.content_type_gating.models import ContentTypeGatingConfig from common.djangoapps.student.models import CourseEnrollment @@ -459,10 +461,11 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase, MilestonesTes # Non-staff can enroll if authenticated and specifically allowed for that course # even outside the open enrollment period user = UserFactory.create() - course = Mock( - enrollment_start=tomorrow, enrollment_end=tomorrow, - id=CourseLocator('edX', 'test', '2012_Fall'), enrollment_domain='' - ) + course = CourseOverviewFactory.create(id=CourseLocator('edX', 'test', '2012_Fall')) + course.enrollment_start = tomorrow + course.enrollment_end = tomorrow + course.enrollment_domain = '' + course.save() CourseEnrollmentAllowedFactory(email=user.email, course_id=course.id) assert access._has_access_course(user, 'enroll', course) @@ -472,30 +475,62 @@ class AccessTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase, MilestonesTes # Non-staff cannot enroll if it is between the start and end dates and invitation only # and not specifically allowed - course = Mock( - enrollment_start=yesterday, enrollment_end=tomorrow, - id=CourseLocator('edX', 'test', '2012_Fall'), enrollment_domain='', - invitation_only=True - ) + course.enrollment_start = yesterday + course.enrollment_end = tomorrow + course.invitation_only = True + course.save() user = UserFactory.create() assert not access._has_access_course(user, 'enroll', course) # Non-staff can enroll if it is between the start and end dates and not invitation only - course = Mock( - enrollment_start=yesterday, enrollment_end=tomorrow, - id=CourseLocator('edX', 'test', '2012_Fall'), enrollment_domain='', - invitation_only=False - ) + course.invitation_only = False + course.save() assert access._has_access_course(user, 'enroll', course) # Non-staff cannot enroll outside the open enrollment period if not specifically allowed - course = Mock( - enrollment_start=tomorrow, enrollment_end=tomorrow, - id=CourseLocator('edX', 'test', '2012_Fall'), enrollment_domain='', - invitation_only=False - ) + course.enrollment_start = tomorrow + course.enrollment_end = tomorrow + course.invitation_only = False + course.save() assert not access._has_access_course(user, 'enroll', course) + @override_settings(FEATURES={**settings.FEATURES, 'DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED': True}) + def test__has_access_course_with_disable_allowed_enrollment_flag(self): + yesterday = datetime.datetime.now(pytz.utc) - datetime.timedelta(days=1) + tomorrow = datetime.datetime.now(pytz.utc) + datetime.timedelta(days=1) + + # Non-staff user invited to course, cannot enroll outside the open enrollment period + # if DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED is True + user = UserFactory.create() + course = CourseOverviewFactory.create(id=CourseLocator('edX', 'test', '2012_Fall')) + course.enrollment_start = tomorrow + course.enrollment_end = tomorrow + course.enrollment_domain = '' + course.save() + CourseEnrollmentAllowedFactory(email=user.email, course_id=course.id) + assert not access._has_access_course(user, 'enroll', course) + + # Staff can always enroll even outside the open enrollment period + user = StaffFactory.create(course_key=course.id) + assert access._has_access_course(user, 'enroll', course) + + # Non-staff cannot enroll outside the open enrollment period if not specifically allowed + user = UserFactory.create() + assert not access._has_access_course(user, 'enroll', course) + + # Non-staff cannot enroll if it is between the start and end dates and invitation only + # and not specifically allowed + course.enrollment_start = yesterday + course.enrollment_end = tomorrow + course.invitation_only = True + course.save() + assert not access._has_access_course(user, 'enroll', course) + + # Non-staff can enroll if it is between the start and end dates and not invitation only + course.invitation_only = False + course.save() + assert access._has_access_course(user, 'enroll', course) + @override_settings(COURSES_INVITE_ONLY=False) def test__course_default_invite_only_flag_false(self): """ diff --git a/lms/envs/common.py b/lms/envs/common.py index 4361838e4d..166fc640c8 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1018,6 +1018,16 @@ FEATURES = { # .. toggle_target_removal_date: None # .. toggle_tickets: 'https://openedx.atlassian.net/browse/MST-1458' 'ENABLE_CERTIFICATES_IDV_REQUIREMENT': False, + + # .. toggle_name: FEATURES['DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Set to True to disable enrollment for user invited to a course + # .. if user is registering before enrollment start date or after enrollment end date + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2022-06-06 + # .. toggle_tickets: 'https://github.com/edx/edx-platform/pull/29538' + 'DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED': False, } # Specifies extra XBlock fields that should available when requested via the Course Blocks API diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index a7565c7a4d..d16defff36 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -534,6 +534,12 @@ class CourseOverview(TimeStampedModel): """ return course_metadata_utils.has_course_ended(self.end) + def is_enrollment_open(self): + """ + Returns True if course enrollment is open + """ + return course_metadata_utils.is_enrollment_open(self.enrollment_start, self.enrollment_end) + def has_marketing_url(self): """ Returns whether the course has marketing url. diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 2c856ae70a..74b704a303 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -141,6 +141,7 @@ class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase, Cache ('clean_id', ('#',)), ('has_ended', ()), ('has_started', ()), + ('is_enrollment_open', ()), ] for method_name, method_args in methods_to_test: course_value = getattr(course, method_name)(*method_args) diff --git a/xmodule/course_metadata_utils.py b/xmodule/course_metadata_utils.py index d1583f3895..d8cccb7248 100644 --- a/xmodule/course_metadata_utils.py +++ b/xmodule/course_metadata_utils.py @@ -110,6 +110,20 @@ def has_course_ended(end_date): return datetime.now(utc) > end_date if end_date is not None else False +def is_enrollment_open(enrollment_start_date, enrollment_end_date): + """ + Given a course's enrollment start and end datetime, returns if enrollment is open + + Arguments: + enrollment_start_date (datetime): The enrollment start datetime of the course. + enrollment_end_date (datetime): The enrollment end datetime of the course. + """ + now = datetime.now(utc) + enrollment_start_date = enrollment_start_date or datetime.min.replace(tzinfo=utc) + enrollment_end_date = enrollment_end_date or datetime.max.replace(tzinfo=utc) + return enrollment_start_date < now < enrollment_end_date + + def course_starts_within(start_date, look_ahead_days): """ Given a course's start datetime and look ahead days, returns True if diff --git a/xmodule/course_module.py b/xmodule/course_module.py index 7d186c60b2..99ff1e84cd 100644 --- a/xmodule/course_module.py +++ b/xmodule/course_module.py @@ -1206,6 +1206,12 @@ class CourseBlock( def has_started(self): return course_metadata_utils.has_course_started(self.start) + def is_enrollment_open(self): + """ + Returns True if course enrollment is open + """ + return course_metadata_utils.is_enrollment_open(self.enrollment_start, self.enrollment_end) + @property def grader(self): return grader_from_conf(self.raw_grader) diff --git a/xmodule/tests/test_course_metadata_utils.py b/xmodule/tests/test_course_metadata_utils.py index f1bd4b6f8d..36795955ea 100644 --- a/xmodule/tests/test_course_metadata_utils.py +++ b/xmodule/tests/test_course_metadata_utils.py @@ -20,6 +20,7 @@ from xmodule.course_metadata_utils import ( course_start_date_is_default, has_course_ended, has_course_started, + is_enrollment_open, number_for_course_location ) from xmodule.modulestore.tests.utils import ( @@ -59,7 +60,21 @@ class CourseMetadataUtilsTestCase(TestCase): user_id=-3, # -3 refers to a "testing user" fields={ "start": _LAST_MONTH, - "end": _LAST_WEEK + "end": _LAST_WEEK, + "enrollment_start": _LAST_MONTH, + "enrollment_end": _LAST_WEEK + } + ) + with mixed_store.default_store('mongo'): + self.demo_course_enrollment_end = mixed_store.create_course( + org="edX", + course="DemoX.2", + run="Fall_2014_1", + user_id=-3, # -3 refers to a "testing user" + fields={ + "start": _LAST_MONTH, + "end": _LAST_WEEK, + "enrollment_end": _LAST_WEEK } ) with mixed_store.default_store('split'): @@ -70,7 +85,20 @@ class CourseMetadataUtilsTestCase(TestCase): user_id=-3, # -3 refers to a "testing user" fields={ "start": _NEXT_WEEK, - "display_name": "Intro to