From 9f8a6867ee2a2911efc0e4b0293400538c8a9c6e Mon Sep 17 00:00:00 2001 From: 0x29a Date: Sun, 5 Feb 2023 19:06:47 +0100 Subject: [PATCH 1/8] feat: allow enrollment api admin to see all enrollments Co-authored-by: Maxim Beder test: EnterpriseCourseEnrollmentFilterBackend Co-authored-by: Maxim Beder feat: add additional fields to EnterpriseCourseEnrollmentViewSet Created new manager and serializer for EnterpriseCourseEnrollment model, which add additional fields to the model, which allow to reduce the number of requests to the API by providing the necessary data in the feilds along side the model. Use the new manager and serializer in EnterpriseCourseEnrollmentViewSet. Co-authored-by: Maxim Beder perf: order enterprise course enrollments by id perf: strip annotations for paginator's count perf: use read replica or default --- enterprise/api/filters.py | 32 ++++++++- enterprise/api/v1/serializers.py | 26 +++++++ .../v1/views/enterprise_course_enrollment.py | 63 ++++++++++++++++- enterprise/models.py | 58 ++++++++++++++- test_utils/factories.py | 35 ++++++++++ tests/test_enterprise/api/test_filters.py | 70 ++++++++++++++++++- tests/test_enterprise/api/test_views.py | 5 ++ 7 files changed, 284 insertions(+), 5 deletions(-) diff --git a/enterprise/api/filters.py b/enterprise/api/filters.py index 0ea4115085..92c1bb4551 100644 --- a/enterprise/api/filters.py +++ b/enterprise/api/filters.py @@ -7,7 +7,7 @@ from django.contrib import auth -from enterprise.models import EnterpriseCustomerUser, SystemWideEnterpriseUserRoleAssignment +from enterprise.models import EnterpriseCustomer, EnterpriseCustomerUser, SystemWideEnterpriseUserRoleAssignment User = auth.get_user_model() @@ -33,6 +33,36 @@ def filter_queryset(self, request, queryset, view): return queryset +class EnterpriseCourseEnrollmentFilterBackend(filters.BaseFilterBackend): + """ + Filter backend to return enrollments under the user's enterprise(s) only. + + * Staff users will bypass this filter. + * Non-staff users will receive enrollments under their linked enterprises, + only if they have the `enterprise.can_enroll_learners` permission. + * Non-staff users without the `enterprise.can_enroll_learners` permission + will receive only their own enrollments. + """ + + def filter_queryset(self, request, queryset, view): + """ + Filter out enrollments if learner is not linked + """ + + if request.user.is_staff: + return queryset + + if request.user.has_perm('enterprise.can_enroll_learners'): + enterprise_customers = EnterpriseCustomer.objects.filter(enterprise_customer_users__user_id=request.user.id) + return queryset.filter(enterprise_customer_user__enterprise_customer__in=enterprise_customers) + + filter_kwargs = { + view.USER_ID_FILTER: request.user.id, + } + + return queryset.filter(**filter_kwargs) + + class EnterpriseCustomerUserFilterBackend(filters.BaseFilterBackend): """ Allow filtering on the enterprise customer user api endpoint. diff --git a/enterprise/api/v1/serializers.py b/enterprise/api/v1/serializers.py index d44ce837cd..a0b09a446a 100644 --- a/enterprise/api/v1/serializers.py +++ b/enterprise/api/v1/serializers.py @@ -356,6 +356,32 @@ class Meta: ) +class EnterpriseCourseEnrollmentWithAdditionalFieldsReadOnlySerializer(EnterpriseCourseEnrollmentReadOnlySerializer): + """ + Serializer for EnterpriseCourseEnrollment model with additional fields. + """ + + class Meta: + model = models.EnterpriseCourseEnrollment + fields = ( + 'enterprise_customer_user', + 'course_id', + 'created', + 'unenrolled_at', + 'enrollment_date', + 'enrollment_track', + 'user_email', + 'course_start', + 'course_end', + ) + + enrollment_track = serializers.CharField() + enrollment_date = serializers.DateTimeField() + user_email = serializers.EmailField() + course_start = serializers.DateTimeField() + course_end = serializers.DateTimeField() + + class EnterpriseCourseEnrollmentWriteSerializer(serializers.ModelSerializer): """ Serializer for writing to the EnterpriseCourseEnrollment model. diff --git a/enterprise/api/v1/views/enterprise_course_enrollment.py b/enterprise/api/v1/views/enterprise_course_enrollment.py index c7aef5ffc0..59ebf75c01 100644 --- a/enterprise/api/v1/views/enterprise_course_enrollment.py +++ b/enterprise/api/v1/views/enterprise_course_enrollment.py @@ -1,17 +1,68 @@ """ Views for the ``enterprise-course-enrollment`` API endpoint. """ +from django_filters.rest_framework import DjangoFilterBackend +from edx_rest_framework_extensions.paginators import DefaultPagination +from rest_framework import filters + +from django.core.paginator import Paginator +from django.utils.functional import cached_property + from enterprise import models +from enterprise.api.filters import EnterpriseCourseEnrollmentFilterBackend from enterprise.api.v1 import serializers from enterprise.api.v1.views.base_views import EnterpriseReadWriteModelViewSet +try: + from common.djangoapps.util.query import read_replica_or_default +except ImportError: + def read_replica_or_default(): + return None + + +class PaginatorWithOptimizedCount(Paginator): + """ + Django < 4.2 ORM doesn't strip unused annotations from count queries. + + For example, if we execute this query: + + Book.objects.annotate(Count('chapters')).count() + + it will generate SQL like this: + + SELECT COUNT(*) FROM (SELECT COUNT(...), ... FROM ...) subquery + + This isn't optimal on its own, but it's not a big deal. However, this + becomes problematic when annotations use subqueries, because it's terribly + inefficient to execute the subquery for every row in the outer query. + + This class overrides the count() method of Django's Paginator to strip + unused annotations from the query by requesting only the primary key + instead of all fields. + + This is a temporary workaround until Django is updated to 4.2, which will + include a fix for this issue. + + See https://code.djangoproject.com/ticket/32169 for more details. + + TODO: remove this class once Django is updated to 4.2 or higher. + """ + @cached_property + def count(self): + return self.object_list.values("pk").count() + + +class EnterpriseCourseEnrollmentPagination(DefaultPagination): + django_paginator_class = PaginatorWithOptimizedCount + class EnterpriseCourseEnrollmentViewSet(EnterpriseReadWriteModelViewSet): """ API views for the ``enterprise-course-enrollment`` API endpoint. """ - queryset = models.EnterpriseCourseEnrollment.objects.all() + queryset = models.EnterpriseCourseEnrollment.with_additional_fields.all() + filter_backends = (filters.OrderingFilter, DjangoFilterBackend, EnterpriseCourseEnrollmentFilterBackend) USER_ID_FILTER = 'enterprise_customer_user__user_id' FIELDS = ( @@ -20,10 +71,18 @@ class EnterpriseCourseEnrollmentViewSet(EnterpriseReadWriteModelViewSet): filterset_fields = FIELDS ordering_fields = FIELDS + pagination_class = EnterpriseCourseEnrollmentPagination + + def get_queryset(self): + queryset = super().get_queryset() + if self.request.method == 'GET': + queryset = queryset.using(read_replica_or_default()) + return queryset + def get_serializer_class(self): """ Use a special serializer for any requests that aren't read-only. """ if self.request.method in ('GET',): - return serializers.EnterpriseCourseEnrollmentReadOnlySerializer + return serializers.EnterpriseCourseEnrollmentWithAdditionalFieldsReadOnlySerializer return serializers.EnterpriseCourseEnrollmentWriteSerializer diff --git a/enterprise/models.py b/enterprise/models.py index a976cf40b2..7f27902990 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -92,6 +92,11 @@ except ImportError: CourseEntitlement = None +try: + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +except ImportError: + CourseOverview = None + LOGGER = getEnterpriseLogger(__name__) User = auth.get_user_model() mark_safe_lazy = lazy(mark_safe, str) @@ -1855,11 +1860,61 @@ def get_queryset(self): """ Override to return only those enrollment records for which learner is linked to an enterprise. """ + return super().get_queryset().select_related('enterprise_customer_user').filter( enterprise_customer_user__linked=True ) +class EnterpriseCourseEnrollmentWithAdditionalFieldsManager(models.Manager): + """ + Model manager for `EnterpriseCourseEnrollment`. + """ + + def get_queryset(self): + """ + Override to return only those enrollment records for which learner is linked to an enterprise. + """ + + return super().get_queryset().select_related('enterprise_customer_user').filter( + enterprise_customer_user__linked=True + ).annotate(**self._get_additional_data_annotations()) + + def _get_additional_data_annotations(self): + """ + Return annotations with additional data for the queryset. + Additional fields are None in the test environment, where platform models are not available. + """ + + if not CourseEnrollment or not CourseOverview: + return { + 'enrollment_track': models.Value(None, output_field=models.CharField()), + 'enrollment_date': models.Value(None, output_field=models.DateTimeField()), + 'user_email': models.Value(None, output_field=models.EmailField()), + 'course_start': models.Value(None, output_field=models.DateTimeField()), + 'course_end': models.Value(None, output_field=models.DateTimeField()), + } + + enrollment_subquery = CourseEnrollment.objects.filter( + user=models.OuterRef('enterprise_customer_user__user_id'), + course_id=models.OuterRef('course_id'), + ) + user_subquery = auth.get_user_model().objects.filter( + id=models.OuterRef('enterprise_customer_user__user_id'), + ).values('email')[:1] + course_subquery = CourseOverview.objects.filter( + id=models.OuterRef('course_id'), + ) + + return { + 'enrollment_track': models.Subquery(enrollment_subquery.values('mode')[:1]), + 'enrollment_date': models.Subquery(enrollment_subquery.values('created')[:1]), + 'user_email': models.Subquery(user_subquery), + 'course_start': models.Subquery(course_subquery.values('start')[:1]), + 'course_end': models.Subquery(course_subquery.values('end')[:1]), + } + + class EnterpriseCourseEnrollment(TimeStampedModel): """ Store information about the enrollment of a user in a course. @@ -1879,11 +1934,12 @@ class EnterpriseCourseEnrollment(TimeStampedModel): """ objects = EnterpriseCourseEnrollmentManager() + with_additional_fields = EnterpriseCourseEnrollmentWithAdditionalFieldsManager() class Meta: unique_together = (('enterprise_customer_user', 'course_id',),) app_label = 'enterprise' - ordering = ['created'] + ordering = ['id'] enterprise_customer_user = models.ForeignKey( EnterpriseCustomerUser, diff --git a/test_utils/factories.py b/test_utils/factories.py index 1d6998d92c..ea0152df87 100644 --- a/test_utils/factories.py +++ b/test_utils/factories.py @@ -27,6 +27,8 @@ EnterpriseCustomerReportingConfiguration, EnterpriseCustomerSsoConfiguration, EnterpriseCustomerUser, + EnterpriseFeatureRole, + EnterpriseFeatureUserRoleAssignment, LearnerCreditEnterpriseCourseEnrollment, LicensedEnterpriseCourseEnrollment, PendingEnrollment, @@ -272,6 +274,39 @@ class Meta: invite_key = None +class EnterpriseFeatureRoleFactory(factory.django.DjangoModelFactory): + """ + EnterpriseFeatureRole factory. + Creates an instance of EnterpriseFeatureRole with minimal boilerplate. + """ + + class Meta: + """ + Meta for EnterpriseFeatureRoleFactory. + """ + + model = EnterpriseFeatureRole + + name = factory.LazyAttribute(lambda x: FAKER.word()) + + +class EnterpriseFeatureUserRoleAssignmentFactory(factory.django.DjangoModelFactory): + """ + EnterpriseFeatureUserRoleAssignment factory. + Creates an instance of EnterpriseFeatureUserRoleAssignment with minimal boilerplate. + """ + + class Meta: + """ + Meta for EnterpriseFeatureUserRoleAssignmentFactory. + """ + + model = EnterpriseFeatureUserRoleAssignment + + role = factory.SubFactory(EnterpriseFeatureRoleFactory) + user = factory.SubFactory(UserFactory) + + class AnonymousUserFactory(factory.Factory): """ Anonymous User factory. diff --git a/tests/test_enterprise/api/test_filters.py b/tests/test_enterprise/api/test_filters.py index b37b90790c..1076c58f4b 100644 --- a/tests/test_enterprise/api/test_filters.py +++ b/tests/test_enterprise/api/test_filters.py @@ -10,7 +10,8 @@ from django.conf import settings -from enterprise.constants import ENTERPRISE_ADMIN_ROLE +from enterprise.constants import ENTERPRISE_ADMIN_ROLE, ENTERPRISE_ENROLLMENT_API_ADMIN_ROLE +from enterprise.models import EnterpriseFeatureRole from test_utils import FAKE_UUIDS, TEST_EMAIL, TEST_USERNAME, APITest, factories ENTERPRISE_CUSTOMER_LIST_ENDPOINT = reverse('enterprise-customer-list') @@ -80,6 +81,73 @@ def test_filter_for_detail(self, is_staff, is_linked, expected_content_in_respon assert data[key] == value +@ddt.ddt +@mark.django_db +class TestEnterpriseCourseEnrollmentFilterBackend(APITest): + """ + Test suite for the ``EnterpriseCourseEnrollmentFilterBackend`` filter. + """ + + def setUp(self): + super().setUp() + + self._setup_enterprise_customer_and_enrollments( + uuid=FAKE_UUIDS[0], + users=[self.user, factories.UserFactory()] + ) + self._setup_enterprise_customer_and_enrollments( + uuid=FAKE_UUIDS[1], + users=[factories.UserFactory(), factories.UserFactory()] + ) + + self.url = settings.TEST_SERVER + reverse('enterprise-course-enrollment-list') + + def _setup_enterprise_customer_and_enrollments(self, uuid, users): + """ + Creates an enterprise customer with the uuid and enrolls passed users. + """ + enterprise_customer = factories.EnterpriseCustomerFactory(uuid=uuid) + + for user in users: + enterprise_customer_user = factories.EnterpriseCustomerUserFactory( + enterprise_customer=enterprise_customer, + user_id=user.id + ) + factories.EnterpriseCourseEnrollmentFactory( + enterprise_customer_user=enterprise_customer_user + ) + + def _setup_user_privileges_by_role(self, user, role): + """ + Sets up privileges for the passed user based on the role. + """ + if role == "staff": + user.is_staff = True + user.save() + elif role == "enrollment_api_admin": + factories.EnterpriseFeatureUserRoleAssignmentFactory( + user=user, + role=EnterpriseFeatureRole.objects.get(name=ENTERPRISE_ENROLLMENT_API_ADMIN_ROLE) + ) + + @ddt.data( + ("regular", 1), + ("enrollment_api_admin", 2), + ("staff", 4), + ) + @ddt.unpack + def test_filter_for_list(self, user_role, expected_course_enrollment_count): + """ + Filter objects based off whether the user is a staff, enterprise enrollment api admin, or neither. + """ + self._setup_user_privileges_by_role(self.user, user_role) + + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + data = self.load_json(response.content) + assert len(data['results']) == expected_course_enrollment_count + + @ddt.ddt @mark.django_db class TestEnterpriseCustomerUserFilterBackend(APITest): diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 0c35a915b5..a01f7c666e 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -1276,6 +1276,11 @@ class TestEnterpriseCustomerViewSet(BaseTestEnterpriseAPIViews): 'course_id': 'course-v1:edX+DemoX+DemoCourse', 'created': '2021-10-20T19:01:31Z', 'unenrolled_at': None, + 'enrollment_date': None, + 'enrollment_track': None, + 'user_email': None, + 'course_start': None, + 'course_end': None, }], ), ( From 447cc3baf9c418c8cbf4196886713e75ba3ca22b Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Sat, 6 May 2023 13:59:37 +0530 Subject: [PATCH 2/8] feat: create CourseEnrollmentAllowed entries for pending enrollments When creating pending enrollments for non-existant users, we also check to see if the course is "invite_only". If the course is invite only, then we create corresponding CourseEnrollmentAllowed objects. This fixes the issue when the enterprise creates pending enrollment, but the user cannot enroll to the course as platform rejects the enrollment request due to missing CEA for the user. (cherry picked from commit 2d8c2d4969bb6939857a6529b57f9cf2c30065cf) --- enterprise/admin/forms.py | 6 + enterprise/admin/views.py | 12 +- enterprise/api_client/lms.py | 14 +- enterprise/models.py | 17 ++- .../static/enterprise/js/manage_learners.js | 36 ++++- enterprise/utils.py | 12 +- test_utils/fake_enrollment_api.py | 3 +- tests/test_admin/test_view.py | 132 +++++++++++++++--- tests/test_enterprise/api/test_views.py | 78 +++++++---- tests/test_enterprise/api_client/test_lms.py | 3 +- tests/test_enterprise/test_utils.py | 6 +- 11 files changed, 258 insertions(+), 61 deletions(-) diff --git a/enterprise/admin/forms.py b/enterprise/admin/forms.py index fd076aa4d4..034e8e14fd 100644 --- a/enterprise/admin/forms.py +++ b/enterprise/admin/forms.py @@ -68,6 +68,11 @@ class ManageLearnersForm(forms.Form): label=_("Enroll these learners in this course"), required=False, help_text=_("To enroll learners in a course, enter a course ID."), ) + force_enrollment = forms.BooleanField( + label=_("Force Enrollment"), + help_text=_("The selected course is 'Invite Only'. Only staff can enroll learners to this course."), + required=False, + ) course_mode = forms.ChoiceField( label=_("Course enrollment track"), required=False, choices=BLANK_CHOICE_DASH + [ @@ -130,6 +135,7 @@ class Fields: REASON = "reason" SALES_FORCE_ID = "sales_force_id" DISCOUNT = "discount" + FORCE_ENROLLMENT = "force_enrollment" class CsvColumns: """ diff --git a/enterprise/admin/views.py b/enterprise/admin/views.py index da997683b5..c1e9979e67 100644 --- a/enterprise/admin/views.py +++ b/enterprise/admin/views.py @@ -678,7 +678,8 @@ def _enroll_users( notify=True, enrollment_reason=None, sales_force_id=None, - discount=0.0 + discount=0.0, + force_enrollment=False, ): """ Enroll the users with the given email addresses to the course. @@ -691,6 +692,7 @@ def _enroll_users( mode: The enrollment mode the users will be enrolled in the course with course_id: The ID of the course in which we want to enroll notify: Whether to notify (by email) the users that have been enrolled + force_enrollment: Force enrollment into "Invite Only" courses """ pending_messages = [] paid_modes = ['verified', 'professional'] @@ -704,6 +706,7 @@ def _enroll_users( enrollment_reason=enrollment_reason, discount=discount, sales_force_id=sales_force_id, + force_enrollment=force_enrollment, ) all_successes = succeeded + pending if notify: @@ -820,6 +823,7 @@ def post(self, request, customer_uuid): sales_force_id = manage_learners_form.cleaned_data.get(ManageLearnersForm.Fields.SALES_FORCE_ID) course_mode = manage_learners_form.cleaned_data.get(ManageLearnersForm.Fields.COURSE_MODE) course_id = None + force_enrollment = manage_learners_form.cleaned_data.get(ManageLearnersForm.Fields.FORCE_ENROLLMENT) if not course_id_with_emails: course_details = manage_learners_form.cleaned_data.get(ManageLearnersForm.Fields.COURSE) or {} @@ -834,7 +838,8 @@ def post(self, request, customer_uuid): notify=notify, enrollment_reason=manual_enrollment_reason, sales_force_id=sales_force_id, - discount=discount + discount=discount, + force_enrollment=force_enrollment, ) else: for course_id, emails in course_id_with_emails.items(): @@ -849,7 +854,8 @@ def post(self, request, customer_uuid): notify=notify, enrollment_reason=manual_enrollment_reason, sales_force_id=sales_force_id, - discount=discount + discount=discount, + force_enrollment=force_enrollment, ) # Redirect to GET if everything went smooth. diff --git a/enterprise/api_client/lms.py b/enterprise/api_client/lms.py index 47e08edb49..cb06742e69 100644 --- a/enterprise/api_client/lms.py +++ b/enterprise/api_client/lms.py @@ -128,7 +128,15 @@ def has_course_mode(self, course_run_id, mode): course_modes = self.get_course_modes(course_run_id) return any(course_mode for course_mode in course_modes if course_mode['slug'] == mode) - def enroll_user_in_course(self, username, course_id, mode, cohort=None, enterprise_uuid=None): + def enroll_user_in_course( + self, + username, + course_id, + mode, + cohort=None, + enterprise_uuid=None, + force_enrollment=False, + ): """ Call the enrollment API to enroll the user in the course specified by course_id. @@ -138,6 +146,7 @@ def enroll_user_in_course(self, username, course_id, mode, cohort=None, enterpri mode (str): The enrollment mode which should be used for the enrollment cohort (str): Add the user to this named cohort enterprise_uuid (str): Add course enterprise uuid + force_enrollment (bool): Force the enrollment even if course is Invite Only Returns: dict: A dictionary containing details of the enrollment, including course details, mode, username, etc. @@ -152,7 +161,8 @@ def enroll_user_in_course(self, username, course_id, mode, cohort=None, enterpri 'is_active': True, 'mode': mode, 'cohort': cohort, - 'enterprise_uuid': str(enterprise_uuid) + 'enterprise_uuid': str(enterprise_uuid), + 'force_enrollment': force_enrollment, } ) response.raise_for_status() diff --git a/enterprise/models.py b/enterprise/models.py index 7f27902990..d8282a139d 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -83,9 +83,10 @@ ) try: - from common.djangoapps.student.models import CourseEnrollment + from common.djangoapps.student.models import CourseEnrollment, CourseEnrollmentAllowed except ImportError: CourseEnrollment = None + CourseEnrollmentAllowed = None try: from common.djangoapps.entitlements.models import CourseEntitlement @@ -749,7 +750,21 @@ def enroll_user_pending_registration_with_status(self, email, course_mode, *cour license_uuid = None new_enrollments = {} + enrollment_api_client = EnrollmentApiClient() + for course_id in course_ids: + # Check if the course is "Invite Only" and add CEA if it is. + course_details = enrollment_api_client.get_course_details(course_id) + + if course_details["invite_only"]: + if not CourseEnrollmentAllowed: + raise NotConnectedToOpenEdX() + + CourseEnrollmentAllowed.objects.update_or_create( + email=email, + course_id=course_id + ) + __, created = PendingEnrollment.objects.update_or_create( user=pending_ecu, course_id=course_id, diff --git a/enterprise/static/enterprise/js/manage_learners.js b/enterprise/static/enterprise/js/manage_learners.js index 5b12d4ad0b..940092467b 100644 --- a/enterprise/static/enterprise/js/manage_learners.js +++ b/enterprise/static/enterprise/js/manage_learners.js @@ -9,7 +9,7 @@ function makeOption(name, value) { return $("").text(name).val(value); } -function fillModeDropdown(data) { +function updateCourseData(data) { /* Given a set of data fetched from the enrollment API, populate the Course Mode dropdown with those options that are valid for the course entered in the @@ -19,6 +19,11 @@ function fillModeDropdown(data) { var previous_value = $course_mode.val(); applyModes(data.course_modes); $course_mode.val(previous_value); + + // If the course is invite-only, show the force enrollment box. + if (data.invite_only) { + $("#id_force_enrollment").parent().show(); + } } function applyModes(modes) { @@ -43,7 +48,7 @@ function loadCourseModes(success, failure) { return; } $.ajax({method: 'get', url: enrollmentApiRoot + "course/" + courseId}) - .done(success || fillModeDropdown) + .done(success || updateCourseData) .fail(failure || function (err, jxHR, errstat) { disableMode(disableReason); }); }); } @@ -134,11 +139,38 @@ function loadPage() { programEnrollment.$control.oldValue = null; }); + // NOTE: As the course details won't be fetched for course id in the CSV + // file, this has a potential side-effect of enrolling learners into the courses + // which might be marked as closed for reasons other then being "Invite Only". + // + // This is considered as a reasonable tradeoff at the time of this addition. + // Currently, the EnrollmentListView does not support invitation only courses. + // This problem does not happen in the Instructor Dashboard because it doesn't + // invoke access checks when calling the enroll method. Modifying the enroll method + // is a high-risk change, and it seems that the API will need some changes in + // the near future anyway - when the Instructor Dashboard is converted into an + // MFE (it could be an excellent opportunity to eliminate many legacy behaviors + // there, too). + $("#id_bulk_upload_csv").change(function(e) { + if (e.target.value) { + var force_enrollment = $("#id_force_enrollment"); + force_enrollment.parent().show(); + force_enrollment.siblings(".helptext")[0].innerHTML = gettext( + "If any of the courses in the CSV file are marked 'Invite Only', " + + "this should be enabled for the enrollments to go through in those courses." + ); + } + }); + if (courseEnrollment.$control.val()) { courseEnrollment.$control.trigger("input"); } else if (programEnrollment.$control.val()) { programEnrollment.$control.trigger("input"); } + + // hide the force_invite_only checkbox by default + $("#id_force_enrollment").parent().hide(); + $("#learner-management-form").submit(addCheckedLearnersToEnrollBox); } diff --git a/enterprise/utils.py b/enterprise/utils.py index 28725480c9..2b26fe67a6 100644 --- a/enterprise/utils.py +++ b/enterprise/utils.py @@ -1739,12 +1739,15 @@ def enroll_user(enterprise_customer, user, course_mode, *course_ids, **kwargs): user: The user model object who needs to be enrolled in the course course_mode: The string representation of the mode with which the enrollment should be created *course_ids: An iterable containing any number of course IDs to eventually enroll the user in. - kwargs: Should contain enrollment_client if it's already been instantiated and should be passed in. + kwargs: Contains optional params such as: + - enrollment_client, if it's already been instantiated and should be passed in + - force_enrollment, if the course is "Invite Only" and the "force_enrollment" is needed Returns: Boolean: Whether or not enrollment succeeded for all courses specified """ enrollment_client = kwargs.pop('enrollment_client', None) + force_enrollment = kwargs.pop('force_enrollment', False) if not enrollment_client: from enterprise.api_client.lms import EnrollmentApiClient # pylint: disable=import-outside-toplevel enrollment_client = EnrollmentApiClient() @@ -1759,7 +1762,8 @@ def enroll_user(enterprise_customer, user, course_mode, *course_ids, **kwargs): user.username, course_id, course_mode, - enterprise_uuid=str(enterprise_customer_user.enterprise_customer.uuid) + enterprise_uuid=str(enterprise_customer_user.enterprise_customer.uuid), + force_enrollment=force_enrollment, ) except HttpClientError as exc: # Check if user is already enrolled then we should ignore exception @@ -2112,6 +2116,7 @@ def enroll_users_in_course( enrollment_reason=None, discount=0.0, sales_force_id=None, + force_enrollment=False, ): """ Enroll existing users in a course, and create a pending enrollment for nonexisting users. @@ -2125,6 +2130,7 @@ def enroll_users_in_course( enrollment_reason (str): A reason for enrollment. discount (Decimal): Percentage discount for enrollment. sales_force_id (str): Salesforce opportunity id. + force_enrollment (bool): Force enrollment into 'Invite Only' courses. Returns: successes: A list of users who were successfully enrolled in the course. @@ -2141,7 +2147,7 @@ def enroll_users_in_course( failures = [] for user in existing_users: - succeeded = enroll_user(enterprise_customer, user, course_mode, course_id) + succeeded = enroll_user(enterprise_customer, user, course_mode, course_id, force_enrollment=force_enrollment) if succeeded: successes.append(user) if enrollment_requester and enrollment_reason: diff --git a/test_utils/fake_enrollment_api.py b/test_utils/fake_enrollment_api.py index 700cc31d38..7db03d5f80 100644 --- a/test_utils/fake_enrollment_api.py +++ b/test_utils/fake_enrollment_api.py @@ -150,7 +150,8 @@ def get_course_details(course_id): return None -def enroll_user_in_course(user, course_id, mode, cohort=None, enterprise_uuid=None): +def enroll_user_in_course(user, course_id, mode, cohort=None, enterprise_uuid=None, force_enrollment=False): # pylint: disable=unused-argument + """ Fake implementation. """ diff --git a/tests/test_admin/test_view.py b/tests/test_admin/test_view.py index 87288b60d3..006baf90bf 100644 --- a/tests/test_admin/test_view.py +++ b/tests/test_admin/test_view.py @@ -894,7 +894,16 @@ def test_post_existing_pending_record_with_another_enterprise_customer(self): self._test_post_existing_record_response(response) assert PendingEnterpriseCustomerUser.objects.filter(user_email=email).count() == 2 - def _enroll_user_request(self, user, mode, course_id="", notify=True, reason="tests", discount=0.0): + def _enroll_user_request( + self, + user, + mode, + course_id="", + notify=True, + reason="tests", + discount=0.0, + force_enrollment=False + ): """ Perform post request to log in and submit the form to enroll a user. """ @@ -919,6 +928,7 @@ def _enroll_user_request(self, user, mode, course_id="", notify=True, reason="te ManageLearnersForm.Fields.NOTIFY: notify, ManageLearnersForm.Fields.REASON: reason, ManageLearnersForm.Fields.DISCOUNT: discount, + ManageLearnersForm.Fields.FORCE_ENROLLMENT: force_enrollment, }) return response @@ -977,7 +987,8 @@ def test_post_enroll_user( user.username, course_id, mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) if enrollment_exists: track_enrollment.assert_not_called() @@ -1050,7 +1061,8 @@ def _post_multi_enroll( user.username, course_id, mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False, ) track_enrollment.assert_called_with('admin-enrollment', user.id, course_id) self._assert_django_messages(response, { @@ -1111,20 +1123,71 @@ def test_post_multi_enroll_pending_user( """ Test that a pending learner can be enrolled in multiple courses. """ - self._post_multi_enroll( + with mock.patch( + 'enterprise.models.EnrollmentApiClient.get_course_details', + wraps=fake_enrollment_api.get_course_details, + ): + self._post_multi_enroll( + enterprise_catalog_client, + enrollment_client, + course_catalog_client, + track_enrollment, + False, + ) + + @mock.patch("enterprise.utils.track_enrollment") + @mock.patch("enterprise.models.CourseCatalogApiClient") + @mock.patch("enterprise.api_client.lms.EnrollmentApiClient") + @mock.patch("enterprise.models.EnterpriseCatalogApiClient") + def test_post_enroll_no_course_detail( + self, enterprise_catalog_client, enrollment_client, course_catalog_client, track_enrollment, - False, + ): + catalog_instance = course_catalog_client.return_value + catalog_instance.get_course_run.return_value = {} + enrollment_instance = enrollment_client.return_value + enrollment_instance.enroll_user_in_course.side_effect = fake_enrollment_api.enroll_user_in_course + enrollment_instance.get_course_details.side_effect = fake_enrollment_api.get_course_details + enterprise_catalog_instance = enterprise_catalog_client.return_value + enterprise_catalog_instance.enterprise_contains_content_items.return_value = True + + user = UserFactory() + course_id = "course-v1:HarvardX+CoolScience+2016" + mode = "verified" + response = self._enroll_user_request(user, mode, course_id=course_id) + enrollment_instance.enroll_user_in_course.assert_called_once_with( + user.username, + course_id, + mode, + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) + track_enrollment.assert_called_once_with('admin-enrollment', user.id, course_id) + self._assert_django_messages(response, { + (messages.SUCCESS, "1 learner was enrolled in {}.".format(course_id)), + }) + all_enterprise_enrollments = EnterpriseCourseEnrollment.objects.all() + num_enterprise_enrollments = len(all_enterprise_enrollments) + assert num_enterprise_enrollments == 1 + enrollment = all_enterprise_enrollments[0] + assert enrollment.enterprise_customer_user.user == user + assert enrollment.course_id == course_id + assert enrollment.source is not None + assert enrollment.source.slug == EnterpriseEnrollmentSource.MANUAL + num_messages = len(mail.outbox) + assert num_messages == 0 @mock.patch("enterprise.utils.track_enrollment") @mock.patch("enterprise.models.CourseCatalogApiClient") @mock.patch("enterprise.api_client.lms.EnrollmentApiClient") @mock.patch("enterprise.models.EnterpriseCatalogApiClient") - def test_post_enroll_no_course_detail( + @ddt.data(True, False) + def test_post_enroll_force_enrollment( self, + force_enrollment, enterprise_catalog_client, enrollment_client, course_catalog_client, @@ -1141,12 +1204,13 @@ def test_post_enroll_no_course_detail( user = UserFactory() course_id = "course-v1:HarvardX+CoolScience+2016" mode = "verified" - response = self._enroll_user_request(user, mode, course_id=course_id) + response = self._enroll_user_request(user, mode, course_id=course_id, force_enrollment=force_enrollment) enrollment_instance.enroll_user_in_course.assert_called_once_with( user.username, course_id, mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=force_enrollment ) track_enrollment.assert_called_once_with('admin-enrollment', user.id, course_id) self._assert_django_messages(response, { @@ -1160,8 +1224,6 @@ def test_post_enroll_no_course_detail( assert enrollment.course_id == course_id assert enrollment.source is not None assert enrollment.source.slug == EnterpriseEnrollmentSource.MANUAL - num_messages = len(mail.outbox) - assert num_messages == 0 @mock.patch("enterprise.utils.track_enrollment") @mock.patch("enterprise.models.CourseCatalogApiClient") @@ -1211,7 +1273,8 @@ def test_post_enroll_course_when_enrollment_closed( user.username, course_id, mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) @mock.patch("enterprise.utils.track_enrollment") @@ -1245,7 +1308,8 @@ def test_post_enroll_course_when_enrollment_closed_mode_changed( user.username, course_id, mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) track_enrollment.assert_not_called() self._assert_django_messages(response, { @@ -1286,7 +1350,8 @@ def test_post_enroll_course_when_enrollment_closed_no_sce_exists( user.username, course_id, mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) track_enrollment.assert_not_called() self._assert_django_messages(response, { @@ -1331,7 +1396,8 @@ def test_post_enroll_with_missing_course_start_date( user.username, course_id, mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) track_enrollment.assert_called_once_with('admin-enrollment', user.id, course_id) self._assert_django_messages(response, { @@ -1671,6 +1737,7 @@ def test_post_create_course_enrollments( enrollment_requester=ANY, enterprise_customer=ANY, sales_force_id=ANY, + force_enrollment=ANY, ) enroll_users_in_course_mock.assert_any_call( course_id=second_course_id, @@ -1681,6 +1748,7 @@ def test_post_create_course_enrollments( enrollment_requester=ANY, enterprise_customer=ANY, sales_force_id=ANY, + force_enrollment=ANY, ) else: enroll_users_in_course_mock.assert_not_called() @@ -1765,8 +1833,10 @@ def test_post_successful_test(self): @mock.patch("enterprise.models.CourseCatalogApiClient") @mock.patch("enterprise.api_client.lms.EnrollmentApiClient") @mock.patch("enterprise.models.EnterpriseCatalogApiClient") + @mock.patch("enterprise.models.CourseEnrollmentAllowed") def test_post_link_and_enroll( self, + mock_cea, enterprise_catalog_client, enrollment_client, course_catalog_client, @@ -1799,13 +1869,18 @@ def test_post_link_and_enroll( course_id = "course-v1:EnterpriseX+Training+2017" course_mode = "professional" - response = self._perform_request(columns, data, course=course_id, course_mode=course_mode) + with mock.patch( + 'enterprise.models.EnrollmentApiClient.get_course_details', + wraps=fake_enrollment_api.get_course_details, + ): + response = self._perform_request(columns, data, course=course_id, course_mode=course_mode) enrollment_instance.enroll_user_in_course.assert_called_once_with( user.username, course_id, course_mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) track_enrollment.assert_called_once_with('admin-enrollment', user.id, course_id) pending_user_message = ( @@ -1824,13 +1899,16 @@ def test_post_link_and_enroll( assert pending_enrollment.sales_force_id == sales_force_id num_messages = len(mail.outbox) assert num_messages == 2 + mock_cea.objects.update_or_create.assert_called_once() @mock.patch("enterprise.utils.track_enrollment") @mock.patch("enterprise.models.CourseCatalogApiClient") @mock.patch("enterprise.api_client.lms.EnrollmentApiClient") @mock.patch("enterprise.models.EnterpriseCatalogApiClient") + @mock.patch("enterprise.models.CourseEnrollmentAllowed") def test_post_link_and_enroll_no_course_details( self, + mock_cea, enterprise_catalog_client, enrollment_client, course_catalog_client, @@ -1855,13 +1933,18 @@ def test_post_link_and_enroll_no_course_details( course_id = "course-v1:EnterpriseX+Training+2017" course_mode = "professional" - response = self._perform_request(columns, data, course=course_id, course_mode=course_mode) + with mock.patch( + 'enterprise.models.EnrollmentApiClient.get_course_details', + wraps=fake_enrollment_api.get_course_details, + ): + response = self._perform_request(columns, data, course=course_id, course_mode=course_mode) enrollment_instance.enroll_user_in_course.assert_called_once_with( user.username, course_id, course_mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) track_enrollment.assert_called_once_with('admin-enrollment', user.id, course_id) pending_user_message = ( @@ -1877,12 +1960,15 @@ def test_post_link_and_enroll_no_course_details( assert PendingEnterpriseCustomerUser.objects.all()[0].pendingenrollment_set.all()[0].course_id == course_id num_messages = len(mail.outbox) assert num_messages == 0 + mock_cea.objects.update_or_create.assert_called_once() @mock.patch("enterprise.utils.track_enrollment") @mock.patch("enterprise.api_client.lms.EnrollmentApiClient") @mock.patch("enterprise.models.EnterpriseCatalogApiClient") + @mock.patch("enterprise.models.CourseEnrollmentAllowed") def test_post_link_and_enroll_no_notification( self, + mock_cea, enterprise_catalog_client, enrollment_client, track_enrollment, @@ -1904,13 +1990,18 @@ def test_post_link_and_enroll_no_notification( course_id = "course-v1:EnterpriseX+Training+2017" course_mode = "professional" - response = self._perform_request(columns, data, course=course_id, course_mode=course_mode, notify=False) + with mock.patch( + 'enterprise.models.EnrollmentApiClient.get_course_details', + wraps=fake_enrollment_api.get_course_details, + ): + response = self._perform_request(columns, data, course=course_id, course_mode=course_mode, notify=False) enrollment_instance.enroll_user_in_course.assert_called_once_with( user.username, course_id, course_mode, - enterprise_uuid=str(self.enterprise_customer.uuid) + enterprise_uuid=str(self.enterprise_customer.uuid), + force_enrollment=False ) track_enrollment.assert_called_once_with('admin-enrollment', user.id, course_id) pending_user_message = ( @@ -1925,6 +2016,7 @@ def test_post_link_and_enroll_no_notification( assert PendingEnterpriseCustomerUser.objects.all()[0].pendingenrollment_set.all()[0].course_id == course_id num_messages = len(mail.outbox) assert num_messages == 0 + mock_cea.objects.update_or_create.assert_called_once() @mark.django_db diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index a01f7c666e..489818837b 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -90,6 +90,7 @@ PendingEnterpriseCustomerUserFactory, UserFactory, ) +from test_utils.fake_enrollment_api import get_course_details from test_utils.fake_enterprise_api import get_default_branding_object Application = get_application_model() @@ -2997,6 +2998,7 @@ def test_enterprise_customer_course_enrollments_detail_success( True, enable_autocohorting=True ) + mock_enrollment_client.return_value.get_course_details = get_course_details # Make the call! response = self.client.post( @@ -3194,7 +3196,8 @@ def test_enterprise_customer_course_enrollments_detail_multiple( get_course_enrollment=mock.Mock( side_effect=[None, {'is_active': True, 'mode': VERIFIED_SUBSCRIPTION_COURSE_MODE}] ), - enroll_user_in_course=mock.Mock() + enroll_user_in_course=mock.Mock(), + get_course_details=get_course_details ) # Set up catalog_contains_course response. @@ -4138,6 +4141,7 @@ def tearDown(self): }, 'expected_num_pending_licenses': 1, 'expected_events': [mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v1:edX+DemoX+Demo_Course')], + 'expected_cea': 0, }, # Validation failure cases { @@ -4146,6 +4150,7 @@ def tearDown(self): 'expected_response': {'non_field_errors': ['Must include the `enrollment_info` parameter in request.']}, 'expected_num_pending_licenses': 0, 'expected_events': None, + 'expected_cea': 0, }, { 'body': { @@ -4157,6 +4162,7 @@ def tearDown(self): }, 'expected_num_pending_licenses': 0, 'expected_events': None, + 'expected_cea': 0, }, { 'body': { @@ -4174,6 +4180,7 @@ def tearDown(self): }, 'expected_num_pending_licenses': 0, 'expected_events': None, + 'expected_cea': 0, }, { 'body': { @@ -4199,6 +4206,7 @@ def tearDown(self): }, 'expected_num_pending_licenses': 0, 'expected_events': None, + 'expected_cea': 0, }, { 'body': { @@ -4217,6 +4225,7 @@ def tearDown(self): }, 'expected_num_pending_licenses': 0, 'expected_events': None, + 'expected_cea': 0, }, { 'body': { @@ -4232,6 +4241,7 @@ def tearDown(self): }, 'expected_num_pending_licenses': 0, 'expected_events': None, + 'expected_cea': 0, }, # Single learner, single course success { @@ -4255,6 +4265,7 @@ def tearDown(self): }, 'expected_num_pending_licenses': 1, 'expected_events': [mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v1:edX+DemoX+Demo_Course')], + 'expected_cea': 0, }, # Multi-learner, single course success { @@ -4295,6 +4306,7 @@ def tearDown(self): 'expected_events': [ mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v1:edX+DemoX+Demo_Course'), ], + 'expected_cea': 0, }, # Multi-learner, multi-course success { @@ -4312,12 +4324,12 @@ def tearDown(self): }, { 'email': 'abc@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'license_uuid': '5a88bdcade7c4ecb838f8111b68e18ac' }, { 'email': 'xyz@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'license_uuid': '2c58acdade7c4ede838f7111b42e18ac' }, ] @@ -4340,13 +4352,13 @@ def tearDown(self): }, { 'email': 'abc@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'created': True, 'activation_link': None, }, { 'email': 'xyz@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'created': True, 'activation_link': None, } @@ -4356,8 +4368,9 @@ def tearDown(self): 'expected_num_pending_licenses': 4, 'expected_events': [ mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v1:edX+DemoX+Demo_Course'), - mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v2:edX+DemoX+Second_Demo_Course') + mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v1:EnterpriseX+Training+2017') ], + 'expected_cea': 2, }, { 'body': { @@ -4374,12 +4387,12 @@ def tearDown(self): }, { 'email': 'abc@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'license_uuid': '5a88bdcade7c4ecb838f8111b68e18ac' }, { 'email': 'xyz@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'license_uuid': '2c58acdade7c4ede838f7111b42e18ac' }, ] @@ -4402,13 +4415,13 @@ def tearDown(self): }, { 'email': 'abc@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'created': True, 'activation_link': None, }, { 'email': 'xyz@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:EnterpriseX+Training+2017', 'created': True, 'activation_link': None, } @@ -4418,16 +4431,19 @@ def tearDown(self): 'expected_num_pending_licenses': 4, 'expected_events': [ mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v1:edX+DemoX+Demo_Course'), - mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v2:edX+DemoX+Second_Demo_Course') + mock.call(PATHWAY_CUSTOMER_ADMIN_ENROLLMENT, 1, 'course-v1:EnterpriseX+Training+2017') ], + 'expected_cea': 2, }, ) @ddt.unpack @mock.patch('enterprise.api.v1.views.enterprise_customer.get_best_mode_from_course_key') @mock.patch('enterprise.api.v1.views.enterprise_customer.track_enrollment') @mock.patch("enterprise.models.EnterpriseCustomer.notify_enrolled_learners") + @mock.patch("enterprise.models.CourseEnrollmentAllowed") def test_bulk_enrollment_in_bulk_courses_pending_licenses( self, + mock_cea, mock_notify_task, mock_track_enroll, mock_get_course_mode, @@ -4436,6 +4452,7 @@ def test_bulk_enrollment_in_bulk_courses_pending_licenses( expected_response, expected_num_pending_licenses, expected_events, + expected_cea, ): """ Tests the bulk enrollment endpoint at enroll_learners_in_courses. @@ -4452,11 +4469,17 @@ def test_bulk_enrollment_in_bulk_courses_pending_licenses( mock_get_course_mode.return_value = VERIFIED_SUBSCRIPTION_COURSE_MODE self.assertEqual(len(PendingEnrollment.objects.all()), 0) - response = self.client.post( - settings.TEST_SERVER + ENTERPRISE_CUSTOMER_BULK_ENROLL_LEARNERS_IN_COURSES_ENDPOINT, - data=json.dumps(body), - content_type='application/json', - ) + + with mock.patch( + "enterprise.models.EnrollmentApiClient.get_course_details", + wraps=get_course_details + ): + response = self.client.post( + settings.TEST_SERVER + ENTERPRISE_CUSTOMER_BULK_ENROLL_LEARNERS_IN_COURSES_ENDPOINT, + data=json.dumps(body), + content_type='application/json', + ) + self.assertEqual(response.status_code, expected_code) if expected_response: response_json = response.json() @@ -4471,6 +4494,8 @@ def test_bulk_enrollment_in_bulk_courses_pending_licenses( else: mock_track_enroll.assert_not_called() + self.assertEqual(mock_cea.objects.update_or_create.call_count, expected_cea) + # no notifications to be sent unless 'notify' specifically asked for in payload mock_notify_task.assert_not_called() @@ -4815,12 +4840,12 @@ def test_bulk_enrollment_includes_fulfillment_source_uuid( }, { 'email': 'abc@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:HarvardX+CoolScience+2016', 'license_uuid': '5a88bdcade7c4ecb838f8111b68e18ac' }, { 'email': 'xyz@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:HarvardX+CoolScience+2016', 'license_uuid': '2c58acdade7c4ede838f7111b42e18ac' }, ] @@ -4843,13 +4868,13 @@ def test_bulk_enrollment_includes_fulfillment_source_uuid( }, { 'email': 'abc@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:HarvardX+CoolScience+2016', 'created': True, 'activation_link': None, }, { 'email': 'xyz@test.com', - 'course_run_key': 'course-v2:edX+DemoX+Second_Demo_Course', + 'course_run_key': 'course-v1:HarvardX+CoolScience+2016', 'created': True, 'activation_link': None, } @@ -4894,13 +4919,14 @@ def test_bulk_enrollment_with_notification( self.assertEqual(len(PendingEnrollment.objects.all()), 0) - response = self.client.post( - settings.TEST_SERVER + ENTERPRISE_CUSTOMER_BULK_ENROLL_LEARNERS_IN_COURSES_ENDPOINT, - data=json.dumps(body), - content_type='application/json', - ) - self.assertEqual(response.status_code, expected_code) + with mock.patch("enterprise.models.EnrollmentApiClient.get_course_details", wraps=get_course_details): + response = self.client.post( + settings.TEST_SERVER + ENTERPRISE_CUSTOMER_BULK_ENROLL_LEARNERS_IN_COURSES_ENDPOINT, + data=json.dumps(body), + content_type='application/json', + ) + self.assertEqual(response.status_code, expected_code) response_json = response.json() self.assertEqual(expected_response, response_json) self.assertEqual(len(PendingEnrollment.objects.all()), expected_num_pending_licenses) diff --git a/tests/test_enterprise/api_client/test_lms.py b/tests/test_enterprise/api_client/test_lms.py index abaa1307d0..d15f27725b 100644 --- a/tests/test_enterprise/api_client/test_lms.py +++ b/tests/test_enterprise/api_client/test_lms.py @@ -99,7 +99,8 @@ def test_enroll_user_in_course(): 'mode': mode, 'cohort': cohort, 'is_active': True, - 'enterprise_uuid': 'None' + 'enterprise_uuid': 'None', + 'force_enrollment': False } responses.add( responses.POST, diff --git a/tests/test_enterprise/test_utils.py b/tests/test_enterprise/test_utils.py index d98921e6c0..c77d2113ac 100644 --- a/tests/test_enterprise/test_utils.py +++ b/tests/test_enterprise/test_utils.py @@ -24,6 +24,7 @@ serialize_notification_content, ) from test_utils import FAKE_UUIDS, TEST_PASSWORD, TEST_USERNAME, factories +from test_utils.fake_enrollment_api import get_course_details LMS_BASE_URL = 'https://lms.base.url' @@ -420,11 +421,12 @@ def test_enroll_pending_licensed_users_in_courses_succeeds(self): ) licensed_users_info = [{ 'email': 'pending-user-email@example.com', - 'course_run_key': 'course-key-v1', + 'course_run_key': 'course-v1:edX+DemoX+Demo_Course', 'course_mode': 'verified', 'license_uuid': '5b77bdbade7b4fcb838f8111b68e18ae' }] - result = enroll_subsidy_users_in_courses(ent_customer, licensed_users_info) + with mock.patch("enterprise.models.EnrollmentApiClient.get_course_details", wraps=get_course_details): + result = enroll_subsidy_users_in_courses(ent_customer, licensed_users_info) self.assertEqual(result['pending'][0]['email'], 'pending-user-email@example.com') self.assertFalse(result['successes']) From fa66def8c8501299d34285792e2759225fcae7e2 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Tue, 4 Jul 2023 13:44:14 +0530 Subject: [PATCH 3/8] feat: adds attribute to hide course prices when zero This adds a new attribute `hide_course_price_when_zero` to the EnterpriseCustomer model, which will hide the pricing information from the enrollment page when the final price of a premium course mode is Zero. fix: removed unused variable from template context fix: quality issue and add the new attribute to test fix: update the help text in the db migration feat: adds final price to logging fix: change the attribute in the utils test causing test failure refactor: renamed the migration file chore: rebase on master and update db migration fix: remove empty lines causing linting failure fix: remove the empty line in tests file ------------------------- Additionally includes this fix: https://tasks.opencraft.com/browse/BB-7541?focusedCommentId=282910&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-282910 --- enterprise/admin/__init__.py | 2 +- enterprise/admin/forms.py | 1 + enterprise/models.py | 5 ++++ .../enterprise_course_enrollment_page.html | 24 ++++++++-------- .../enterprise/templatetags/course_modal.html | 2 +- enterprise/utils.py | 25 +++++++++++++++++ enterprise/views.py | 4 +++ tests/test_enterprise/test_utils.py | 28 +++++++++++++++++++ tests/test_utilities.py | 1 + 9 files changed, 79 insertions(+), 13 deletions(-) diff --git a/enterprise/admin/__init__.py b/enterprise/admin/__init__.py index 665745c1d0..e8416fefd1 100644 --- a/enterprise/admin/__init__.py +++ b/enterprise/admin/__init__.py @@ -202,7 +202,7 @@ class EnterpriseCustomerAdmin(DjangoObjectActions, SimpleHistoryAdmin): ('Integration and learning platform settings', { 'fields': ('enable_portal_lms_configurations_screen', 'enable_portal_saml_configuration_screen', 'enable_slug_login', 'replace_sensitive_sso_username', 'hide_course_original_price', - 'enable_generation_of_api_credentials') + 'hide_course_price_when_zero', 'enable_generation_of_api_credentials') }), ('Recommended default settings for all enterprise customers', { 'fields': ('site', 'customer_type', 'enable_learner_portal', diff --git a/enterprise/admin/forms.py b/enterprise/admin/forms.py index 034e8e14fd..f610a9643e 100644 --- a/enterprise/admin/forms.py +++ b/enterprise/admin/forms.py @@ -401,6 +401,7 @@ class Meta: "enable_audit_data_reporting", "replace_sensitive_sso_username", "hide_course_original_price", + "hide_course_price_when_zero", "enable_portal_code_management_screen", "enable_portal_subscription_management_screen", "enable_learner_portal", diff --git a/enterprise/models.py b/enterprise/models.py index d8282a139d..d7801484df 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -476,6 +476,11 @@ class Meta: help_text=_("Email address that will receive learner replies to automated edX emails.") ) + hide_course_price_when_zero = models.BooleanField( + default=False, + help_text=_("Specify whether course cost should be hidden in the landing page when the final price is zero.") + ) + enable_generation_of_api_credentials = models.BooleanField( verbose_name="Allow generation of API credentials", default=False, diff --git a/enterprise/templates/enterprise/enterprise_course_enrollment_page.html b/enterprise/templates/enterprise/enterprise_course_enrollment_page.html index 1d4a777bdf..efb644a519 100644 --- a/enterprise/templates/enterprise/enterprise_course_enrollment_page.html +++ b/enterprise/templates/enterprise/enterprise_course_enrollment_page.html @@ -77,19 +77,21 @@

{{ confirmation_text }}

diff --git a/enterprise/templates/enterprise/templatetags/course_modal.html b/enterprise/templates/enterprise/templatetags/course_modal.html index 41e6f694e0..bc7965b9f3 100644 --- a/enterprise/templates/enterprise/templatetags/course_modal.html +++ b/enterprise/templates/enterprise/templatetags/course_modal.html @@ -25,7 +25,7 @@

{{ course_title
    {% if premium_modes|length > 0 %} {% with course_mode=premium_modes.0 %} - {% if course_mode.original_price %} + {% if course_mode.original_price and not course_mode.hide_price %}
  • diff --git a/enterprise/utils.py b/enterprise/utils.py index 2b26fe67a6..567d814c9d 100644 --- a/enterprise/utils.py +++ b/enterprise/utils.py @@ -2371,3 +2371,28 @@ def camelCase(string): """ output = ''.join(x for x in string.title() if x.isalnum()) return output[0].lower() + output[1:] + + +def hide_price_when_zero(enterprise_customer, course_modes): + """ + Adds a "hide_price" flag to the course modes if price is zero and "Hide course price when zero" flag is set. + + Arguments: + enterprise_customer: The EnterpriseCustomer that the enrollemnt is being done. + course_modes: iterable with dictionaries containing a required 'final_price' key + """ + if not enterprise_customer.hide_course_price_when_zero: + return course_modes + + for mode in course_modes: + mode['hide_price'] = False + try: + numbers = re.findall(r'\d+', mode['final_price']) + mode['hide_price'] = int(''.join(numbers)) == 0 + except ValueError: + LOGGER.warning( + 'hide_price_when_zero: Could not convert price "%s" of course mode "%s" to int.', + mode['final_price'], + mode['title'] + ) + return course_modes diff --git a/enterprise/views.py b/enterprise/views.py index 7c93bbb224..8d1129104b 100644 --- a/enterprise/views.py +++ b/enterprise/views.py @@ -76,6 +76,7 @@ get_enterprise_customer_user, get_platform_logo_url, get_program_type_description, + hide_price_when_zero, is_course_run_enrollable, localized_utcnow, track_enrollment, @@ -1542,6 +1543,9 @@ def get_enterprise_course_enrollment_page( # Filter audit course modes. course_modes = filter_audit_course_modes(enterprise_customer, course_modes) + # Set a flag to hide the $0 when the customer doesn't want it to be shown + course_modes = hide_price_when_zero(enterprise_customer, course_modes) + # Allows automatic assignment to a cohort upon enrollment. cohort = request.GET.get('cohort') # Add a message to the message display queue if the learner diff --git a/tests/test_enterprise/test_utils.py b/tests/test_enterprise/test_utils.py index c77d2113ac..cee847a942 100644 --- a/tests/test_enterprise/test_utils.py +++ b/tests/test_enterprise/test_utils.py @@ -18,6 +18,7 @@ get_default_invite_key_expiration_date, get_idiff_list, get_platform_logo_url, + hide_price_when_zero, is_pending_user, localized_utcnow, parse_lms_api_datetime, @@ -518,3 +519,30 @@ def test_get_default_invite_key_expiration_date(self): expiration_date = get_default_invite_key_expiration_date() expected_expiration_date = current_time + timedelta(days=365) self.assertEqual(expiration_date.date(), expected_expiration_date.date()) + + @ddt.data(True, False) + def test_hide_course_price_when_zero(self, hide_price): + customer = factories.EnterpriseCustomerFactory() + zero_modes = [ + {"final_price": "$0"}, + {"final_price": "$0.000"}, + {"final_price": "Rs. 0.00"}, + {"final_price": "0.00 EURO"}, + ] + non_zero_modes = [ + {"final_price": "$100"}, + {"final_price": "$73.50"}, + {"final_price": "Rs.8000.00"}, + {"final_price": "4000 Euros"}, + ] + customer.hide_course_price_when_zero = hide_price + + processed_zero_modes = hide_price_when_zero(customer, zero_modes) + processed_non_zero_modes = hide_price_when_zero(customer, non_zero_modes) + + if hide_price: + self.assertTrue(all(mode["hide_price"] for mode in processed_zero_modes)) + self.assertFalse(all(mode["hide_price"] for mode in processed_non_zero_modes)) + else: + self.assertEqual(zero_modes, processed_zero_modes) + self.assertEqual(non_zero_modes, processed_non_zero_modes) diff --git a/tests/test_utilities.py b/tests/test_utilities.py index 070a1e817a..dd998dddfc 100644 --- a/tests/test_utilities.py +++ b/tests/test_utilities.py @@ -169,6 +169,7 @@ def setUp(self): "system_wide_role_assignments", "reply_to", "hide_labor_market_data", + "hide_course_price_when_zero", "chat_gpt_prompts", "enable_generation_of_api_credentials", "career_engagement_network_message", From 0069d2c3163ce3b9e0a35cbf3c17af3836b40268 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Fri, 14 Jul 2023 14:48:58 +0530 Subject: [PATCH 4/8] temp: add utility function to add CEA objects feat: adds allow invite only enrollment flag feat: adds allow invite only enrollment flag feat: create cea only when customer has invite-only enrollments enabled fix: simplify the cea creation logic, update tests fix: remove a stray empty line feat: adds the invite-only flag to customer admin fix: typo in the fuction docstring Co-authored-by: Piotr Surowiec refactor: convert the user consent flow handler method to static fix: move migrations to avoid conflicts feat: add typing to the ensure cea utility method Revert "feat: add typing to the ensure cea utility method" This reverts commit b6b2f2590e23263a2671b469f139c2c12abcac3b. refactor: rename the migration with a custom name instead of the auto one fix: remove typing that breaks causes import failures chore: move the DB migration to latest version ----------------------- Additionally includes this fix: https://tasks.opencraft.com/browse/BB-7619?focusedCommentId=282911&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-282911 --- enterprise/admin/__init__.py | 3 +- enterprise/admin/forms.py | 1 + enterprise/models.py | 8 ++ enterprise/utils.py | 23 +++- enterprise/views.py | 110 ++++++++++-------- tests/test_enterprise/test_utils.py | 21 ++++ .../views/test_course_enrollment_view.py | 52 +++++++++ tests/test_utilities.py | 1 + 8 files changed, 171 insertions(+), 48 deletions(-) diff --git a/enterprise/admin/__init__.py b/enterprise/admin/__init__.py index e8416fefd1..5bb4906973 100644 --- a/enterprise/admin/__init__.py +++ b/enterprise/admin/__init__.py @@ -202,7 +202,8 @@ class EnterpriseCustomerAdmin(DjangoObjectActions, SimpleHistoryAdmin): ('Integration and learning platform settings', { 'fields': ('enable_portal_lms_configurations_screen', 'enable_portal_saml_configuration_screen', 'enable_slug_login', 'replace_sensitive_sso_username', 'hide_course_original_price', - 'hide_course_price_when_zero', 'enable_generation_of_api_credentials') + 'hide_course_price_when_zero', 'allow_enrollment_in_invite_only_courses', + 'enable_generation_of_api_credentials') }), ('Recommended default settings for all enterprise customers', { 'fields': ('site', 'customer_type', 'enable_learner_portal', diff --git a/enterprise/admin/forms.py b/enterprise/admin/forms.py index f610a9643e..787fc7c0c3 100644 --- a/enterprise/admin/forms.py +++ b/enterprise/admin/forms.py @@ -402,6 +402,7 @@ class Meta: "replace_sensitive_sso_username", "hide_course_original_price", "hide_course_price_when_zero", + "allow_enrollment_in_invite_only_courses", "enable_portal_code_management_screen", "enable_portal_subscription_management_screen", "enable_learner_portal", diff --git a/enterprise/models.py b/enterprise/models.py index d7801484df..b4bcc763f5 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -493,6 +493,14 @@ class Meta: ), ) + allow_enrollment_in_invite_only_courses = models.BooleanField( + default=False, + help_text=_( + "Specifies if learners are allowed to enroll into courses marked as 'invitation-only', " + "when they attempt to enroll from the landing page." + ) + ) + @property def enterprise_customer_identity_provider(self): """ diff --git a/enterprise/utils.py b/enterprise/utils.py index 567d814c9d..70f7ad6381 100644 --- a/enterprise/utils.py +++ b/enterprise/utils.py @@ -59,8 +59,10 @@ try: from common.djangoapps.course_modes.models import CourseMode + from common.djangoapps.student.models import CourseEnrollmentAllowed except ImportError: CourseMode = None + CourseEnrollmentAllowed = None try: from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -2357,7 +2359,6 @@ def get_md5_hash(content): Get the MD5 hash digest of the given content. Arguments: - content (str): Content in string format for calculating MD5 hash digest. Returns: (str): MD5 hash digest. @@ -2396,3 +2397,23 @@ def hide_price_when_zero(enterprise_customer, course_modes): mode['title'] ) return course_modes + + +def ensure_course_enrollment_is_allowed(course_id, email, enrollment_api_client): + """ + Create a CourseEnrollmentAllowed object for invitation-only courses. + + Arguments: + course_id (str): ID of the course to allow enrollment + email (str): email of the user whose enrollment should be allowed + enrollment_api_client (:class:`enterprise.api_client.lms.EnrollmentApiClient`): Enrollment API Client + """ + if not CourseEnrollmentAllowed: + raise NotConnectedToOpenEdX() + + course_details = enrollment_api_client.get_course_details(course_id) + if course_details["invite_only"]: + CourseEnrollmentAllowed.objects.update_or_create( + course_id=course_id, + email=email, + ) diff --git a/enterprise/views.py b/enterprise/views.py index 8d1129104b..e3166efab8 100644 --- a/enterprise/views.py +++ b/enterprise/views.py @@ -63,6 +63,7 @@ CourseEnrollmentPermissionError, NotConnectedToOpenEdX, clean_html_for_template_rendering, + ensure_course_enrollment_is_allowed, filter_audit_course_modes, format_price, get_active_course_runs, @@ -1681,12 +1682,17 @@ def post(self, request, enterprise_uuid, course_id): enterprise_customer.uuid, course_id=course_id ).consent_required() + + client = EnrollmentApiClient() + if enterprise_customer.allow_enrollment_in_invite_only_courses: + # Make sure a enrollment is allowed if the course is marked "invite-only" + ensure_course_enrollment_is_allowed(course_id, request.user.email, client) + if not selected_course_mode.get('premium') and not user_consent_needed: # For the audit course modes (audit, honor), where DSC is not # required, enroll the learner directly through enrollment API # client and redirect the learner to LMS courseware page. succeeded = True - client = EnrollmentApiClient() try: client.enroll_user_in_course( request.user.username, @@ -1731,51 +1737,12 @@ def post(self, request, enterprise_uuid, course_id): return redirect(LMS_COURSEWARE_URL.format(course_id=course_id)) if user_consent_needed: - # For the audit course modes (audit, honor) or for the premium - # course modes (Verified, Prof Ed) where DSC is required, redirect - # the learner to course specific DSC with enterprise UUID from - # there the learner will be directed to the ecommerce flow after - # providing DSC. - query_string_params = { - 'course_mode': selected_course_mode_name, - } - if enterprise_catalog_uuid: - query_string_params.update({'catalog': enterprise_catalog_uuid}) - - next_url = '{handle_consent_enrollment_url}?{query_string}'.format( - handle_consent_enrollment_url=reverse( - 'enterprise_handle_consent_enrollment', args=[enterprise_customer.uuid, course_id] - ), - query_string=urlencode(query_string_params) - ) - - failure_url = reverse('enterprise_course_run_enrollment_page', args=[enterprise_customer.uuid, course_id]) - if request.META['QUERY_STRING']: - # Preserve all querystring parameters in the request to build - # failure url, so that learner views the same enterprise course - # enrollment page (after redirect) as for the first time. - # Since this is a POST view so use `request.META` to get - # querystring instead of `request.GET`. - # https://docs.djangoproject.com/en/1.11/ref/request-response/#django.http.HttpRequest.META - failure_url = '{course_enrollment_url}?{query_string}'.format( - course_enrollment_url=reverse( - 'enterprise_course_run_enrollment_page', args=[enterprise_customer.uuid, course_id] - ), - query_string=request.META['QUERY_STRING'] - ) - - return redirect( - '{grant_data_sharing_url}?{params}'.format( - grant_data_sharing_url=reverse('grant_data_sharing_permissions'), - params=urlencode( - { - 'next': next_url, - 'failure_url': failure_url, - 'enterprise_customer_uuid': enterprise_customer.uuid, - 'course_id': course_id, - } - ) - ) + return self._handle_user_consent_flow( + request, + enterprise_customer, + enterprise_catalog_uuid, + course_id, + selected_course_mode_name ) # For the premium course modes (Verified, Prof Ed) where DSC is @@ -1790,6 +1757,57 @@ def post(self, request, enterprise_uuid, course_id): return redirect(premium_flow) + @staticmethod + def _handle_user_consent_flow(request, enterprise_customer, enterprise_catalog_uuid, course_id, course_mode): + """ + For the audit course modes (audit, honor) or for the premium + course modes (Verified, Prof Ed) where DSC is required, redirect + the learner to course specific DSC with enterprise UUID from + there the learner will be directed to the ecommerce flow after + providing DSC. + """ + query_string_params = { + 'course_mode': course_mode, + } + if enterprise_catalog_uuid: + query_string_params.update({'catalog': enterprise_catalog_uuid}) + + next_url = '{handle_consent_enrollment_url}?{query_string}'.format( + handle_consent_enrollment_url=reverse( + 'enterprise_handle_consent_enrollment', args=[enterprise_customer.uuid, course_id] + ), + query_string=urlencode(query_string_params) + ) + + failure_url = reverse('enterprise_course_run_enrollment_page', args=[enterprise_customer.uuid, course_id]) + if request.META['QUERY_STRING']: + # Preserve all querystring parameters in the request to build + # failure url, so that learner views the same enterprise course + # enrollment page (after redirect) as for the first time. + # Since this is a POST view so use `request.META` to get + # querystring instead of `request.GET`. + # https://docs.djangoproject.com/en/1.11/ref/request-response/#django.http.HttpRequest.META + failure_url = '{course_enrollment_url}?{query_string}'.format( + course_enrollment_url=reverse( + 'enterprise_course_run_enrollment_page', args=[enterprise_customer.uuid, course_id] + ), + query_string=request.META['QUERY_STRING'] + ) + + return redirect( + '{grant_data_sharing_url}?{params}'.format( + grant_data_sharing_url=reverse('grant_data_sharing_permissions'), + params=urlencode( + { + 'next': next_url, + 'failure_url': failure_url, + 'enterprise_customer_uuid': enterprise_customer.uuid, + 'course_id': course_id, + } + ) + ) + ) + @method_decorator(enterprise_login_required) @method_decorator(force_fresh_session) def get(self, request, enterprise_uuid, course_id): diff --git a/tests/test_enterprise/test_utils.py b/tests/test_enterprise/test_utils.py index cee847a942..0151c4113d 100644 --- a/tests/test_enterprise/test_utils.py +++ b/tests/test_enterprise/test_utils.py @@ -15,6 +15,7 @@ from enterprise.models import EnterpriseCourseEnrollment, LicensedEnterpriseCourseEnrollment from enterprise.utils import ( enroll_subsidy_users_in_courses, + ensure_course_enrollment_is_allowed, get_default_invite_key_expiration_date, get_idiff_list, get_platform_logo_url, @@ -546,3 +547,23 @@ def test_hide_course_price_when_zero(self, hide_price): else: self.assertEqual(zero_modes, processed_zero_modes) self.assertEqual(non_zero_modes, processed_non_zero_modes) + + @ddt.data(True, False) + @mock.patch("enterprise.utils.CourseEnrollmentAllowed") + def test_ensure_course_enrollment_is_allowed(self, invite_only, mock_cea): + """ + Test that the CourseEnrollmentAllowed is created only for the "invite_only" courses. + """ + self.create_user() + mock_enrollment_api = mock.Mock() + mock_enrollment_api.get_course_details.return_value = {"invite_only": invite_only} + + ensure_course_enrollment_is_allowed("test-course-id", self.user.email, mock_enrollment_api) + + if invite_only: + mock_cea.objects.update_or_create.assert_called_with( + course_id="test-course-id", + email=self.user.email + ) + else: + mock_cea.objects.update_or_create.assert_not_called() diff --git a/tests/test_enterprise/views/test_course_enrollment_view.py b/tests/test_enterprise/views/test_course_enrollment_view.py index cd26459f01..8ed1819d5a 100644 --- a/tests/test_enterprise/views/test_course_enrollment_view.py +++ b/tests/test_enterprise/views/test_course_enrollment_view.py @@ -1618,6 +1618,58 @@ def test_post_course_specific_enrollment_view_premium_mode( fetch_redirect_response=False ) + @mock.patch('enterprise.views.render', side_effect=fake_render) + @mock.patch('enterprise.api_client.discovery.CourseCatalogApiServiceClient') + @mock.patch('enterprise.views.EnrollmentApiClient') + @mock.patch('enterprise.views.get_data_sharing_consent') + @mock.patch('enterprise.utils.Registry') + @mock.patch('enterprise.utils.CourseEnrollmentAllowed') + def test_post_course_specific_enrollment_view_invite_only_courses( + self, + mock_cea, + registry_mock, + get_data_sharing_consent_mock, + enrollment_api_client_mock, + catalog_api_client_mock, + *args + ): + course_id = self.demo_course_id + get_data_sharing_consent_mock.return_value = mock.MagicMock(consent_required=mock.MagicMock(return_value=False)) + setup_course_catalog_api_client_mock(catalog_api_client_mock) + self._setup_enrollment_client(enrollment_api_client_mock) + enrollment_api_client_mock.return_value.get_course_details.return_value = {"invite_only": True} + + enterprise_customer = EnterpriseCustomerFactory( + name='Starfleet Academy', + enable_data_sharing_consent=False, + enable_audit_enrollment=False, + allow_enrollment_in_invite_only_courses=True, + ) + EnterpriseCustomerCatalogFactory(enterprise_customer=enterprise_customer) + self._setup_registry_mock(registry_mock, self.provider_id) + EnterpriseCustomerIdentityProviderFactory(provider_id=self.provider_id, enterprise_customer=enterprise_customer) + self._login() + course_enrollment_page_url = self._append_fresh_login_param( + reverse( + 'enterprise_course_run_enrollment_page', + args=[enterprise_customer.uuid, course_id], + ) + ) + enterprise_catalog_uuid = str(enterprise_customer.enterprise_customer_catalogs.first().uuid) + + response = self.client.post( + course_enrollment_page_url, { + 'course_mode': 'professional', + 'catalog': enterprise_catalog_uuid + } + ) + + mock_cea.objects.update_or_create.assert_called_with( + course_id=course_id, + email=self.user.email + ) + assert response.status_code == 302 + @mock.patch('enterprise.api_client.ecommerce.configuration_helpers') @mock.patch('enterprise.views.render', side_effect=fake_render) @mock.patch('enterprise.api_client.lms.embargo_api') diff --git a/tests/test_utilities.py b/tests/test_utilities.py index dd998dddfc..39487f6b0d 100644 --- a/tests/test_utilities.py +++ b/tests/test_utilities.py @@ -134,6 +134,7 @@ def setUp(self): "uuid", "name", "slug", + "allow_enrollment_in_invite_only_courses", "auth_org_id", "active", "country", From db8659f20474278a9f8c9889d0da9562b4405fcc Mon Sep 17 00:00:00 2001 From: 0x29a Date: Wed, 6 Dec 2023 13:19:41 +0100 Subject: [PATCH 5/8] feat: per-user secured Algolia API keys (cherry picked from commit 6247ed4ead9128c06f027d86fa17f06e1edbcf44) --- .../api/v1/views/enterprise_customer.py | 44 ++++++++++++++++- enterprise/settings/test.py | 2 + requirements/base.in | 1 + requirements/dev.txt | 6 +++ requirements/doc.txt | 3 ++ requirements/test-master.txt | 5 ++ requirements/test.txt | 3 ++ tests/test_enterprise/api/test_views.py | 48 +++++++++++++++++++ 8 files changed, 111 insertions(+), 1 deletion(-) diff --git a/enterprise/api/v1/views/enterprise_customer.py b/enterprise/api/v1/views/enterprise_customer.py index 1685304d17..fcd61cc80f 100644 --- a/enterprise/api/v1/views/enterprise_customer.py +++ b/enterprise/api/v1/views/enterprise_customer.py @@ -4,10 +4,11 @@ from urllib.parse import quote_plus, unquote +from algoliasearch.search_client import SearchClient from edx_rbac.decorators import permission_required from rest_framework import permissions from rest_framework.decorators import action -from rest_framework.exceptions import ValidationError +from rest_framework.exceptions import NotFound, ValidationError from rest_framework.response import Response from rest_framework.status import ( HTTP_200_OK, @@ -17,12 +18,15 @@ HTTP_409_CONFLICT, ) +from django.conf import settings from django.contrib import auth from django.core import exceptions from django.db import transaction from django.db.models import Q +from django.http import Http404 from django.shortcuts import get_object_or_404 from django.utils.decorators import method_decorator +from django.utils.translation import gettext_lazy as _ from enterprise import models from enterprise.api.filters import EnterpriseLinkedUserFilterBackend @@ -436,3 +440,41 @@ def unlink_users(self, request, pk=None): # pylint: disable=unused-argument raise UnlinkUserFromEnterpriseError(msg) from exc return Response(status=HTTP_200_OK) + + @action(detail=False) + def algolia_key(self, request, *args, **kwargs): + """ + Returns an Algolia API key that is secured to only allow searching for + objects associated with enterprise customers that the user is linked to. + + This endpoint is used with `frontend-app-learner-portal-enterprise` MFE + currently. + """ + + if not (api_key := getattr(settings, "ENTERPRISE_ALGOLIA_SEARCH_API_KEY", "")): + LOGGER.warning("Algolia search API key is not configured. To enable this view, " + "set `ENTERPRISE_ALGOLIA_SEARCH_API_KEY` in settings.") + raise Http404 + + queryset = self.queryset.filter( + **{ + self.USER_ID_FILTER: request.user.id, + "enterprise_customer_users__linked": True + } + ).values_list("uuid", flat=True) + + if len(queryset) == 0: + raise NotFound(_("User is not linked to any enterprise customers.")) + + secured_key = SearchClient.generate_secured_api_key( + api_key, + { + "filters": " OR ".join( + f"enterprise_customer_uuids:{enterprise_customer_uuid}" + for enterprise_customer_uuid + in queryset + ), + } + ) + + return Response({"key": secured_key}, status=HTTP_200_OK) diff --git a/enterprise/settings/test.py b/enterprise/settings/test.py index 20c1490ccd..2691b83660 100644 --- a/enterprise/settings/test.py +++ b/enterprise/settings/test.py @@ -224,6 +224,8 @@ def root(*args): 'status': 'published' } +ENTERPRISE_ALGOLIA_SEARCH_API_KEY = 'test' + SNOWFLAKE_SERVICE_USER = 'TEST@EDX.ORG' SNOWFLAKE_SERVICE_USER_PASSWORD = 'secret' diff --git a/requirements/base.in b/requirements/base.in index b54f17646e..650a0fdc86 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -2,6 +2,7 @@ # This file contains the dependencies explicitly needed for this library. # # Packages directly used by this library that we do not need pinned to a specific version. +algoliasearch bleach celery code-annotations diff --git a/requirements/dev.txt b/requirements/dev.txt index f1c86e6f3a..a482a81a32 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -24,6 +24,11 @@ alabaster==0.7.13 # via # -r requirements/doc.txt # sphinx +algoliasearch==2.6.3 + # via + # -r requirements/doc.txt + # -r requirements/test-master.txt + # -r requirements/test.txt amqp==5.1.1 # via # -r requirements/doc.txt @@ -719,6 +724,7 @@ requests==2.31.0 # -r requirements/doc.txt # -r requirements/test-master.txt # -r requirements/test.txt + # algoliasearch # django-oauth-toolkit # edx-drf-extensions # edx-rest-api-client diff --git a/requirements/doc.txt b/requirements/doc.txt index 79578958ea..b4d29d1586 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -16,6 +16,8 @@ aiosignal==1.3.1 # aiohttp alabaster==0.7.13 # via sphinx +algoliasearch==2.6.3 + # via -r requirements/test-master.txt amqp==5.1.1 # via # -r requirements/test-master.txt @@ -404,6 +406,7 @@ readme-renderer==42.0 requests==2.31.0 # via # -r requirements/test-master.txt + # algoliasearch # django-oauth-toolkit # edx-drf-extensions # edx-rest-api-client diff --git a/requirements/test-master.txt b/requirements/test-master.txt index 64a5d89599..1352026bb0 100644 --- a/requirements/test-master.txt +++ b/requirements/test-master.txt @@ -12,6 +12,10 @@ aiosignal==1.3.1 # via # -c requirements/edx-platform-constraints.txt # aiohttp +algoliasearch==2.6.3 + # via + # -c requirements/edx-platform-constraints.txt + # -r requirements/base.in amqp==5.1.1 # via kombu aniso8601==9.0.1 @@ -388,6 +392,7 @@ requests==2.31.0 # via # -c requirements/edx-platform-constraints.txt # -r requirements/base.in + # algoliasearch # django-oauth-toolkit # edx-drf-extensions # edx-rest-api-client diff --git a/requirements/test.txt b/requirements/test.txt index 1d80e77236..f013f35a1d 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -15,6 +15,8 @@ aiosignal==1.3.1 # via # -r requirements/test-master.txt # kombu +algoliasearch==2.6.3 + # via -r requirements/test-master.txt aniso8601==9.0.1 # via # -r requirements/test-master.txt @@ -383,6 +385,7 @@ pyyaml==6.0.1 requests==2.31.0 # via # -r requirements/test-master.txt + # algoliasearch # django-oauth-toolkit # edx-drf-extensions # edx-rest-api-client diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 489818837b..a72ef7ccc0 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -2,6 +2,7 @@ Tests for the `edx-enterprise` api module. """ +import base64 import copy import json import logging @@ -140,6 +141,7 @@ ENTERPRISE_LEARNER_LIST_ENDPOINT = reverse('enterprise-learner-list') ENTERPRISE_CUSTOMER_WITH_ACCESS_TO_ENDPOINT = reverse('enterprise-customer-with-access-to') ENTERPRISE_CUSTOMER_UNLINK_USERS_ENDPOINT = reverse('enterprise-customer-unlink-users', kwargs={'pk': FAKE_UUIDS[0]}) +ENTERPRISE_CUSTOMER_ALGOLIA_KEY_ENDPOINT = reverse('enterprise-customer-algolia-key') PENDING_ENTERPRISE_LEARNER_LIST_ENDPOINT = reverse('pending-enterprise-learner-list') LICENSED_ENTERPRISE_COURSE_ENROLLMENTS_REVOKE_ENDPOINT = reverse( 'licensed-enterprise-course-enrollment-license-revoke' @@ -1857,6 +1859,52 @@ def test_unlink_users(self, enterprise_role, enterprise_uuid_for_role, is_relink assert enterprise_customer_user_2.is_relinkable == is_relinkable assert enterprise_customer_user_2.is_relinkable == is_relinkable + def test_algolia_key(self): + """ + Tests that the endpoint algolia_key endpoint returns the correct secured key. + """ + + # Test that the endpoint returns 401 if the user is not logged in. + self.client.logout() + response = self.client.get(ENTERPRISE_CUSTOMER_ALGOLIA_KEY_ENDPOINT) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + username = 'test_learner_portal_user' + self.create_user(username=username, is_staff=False) + self.client.login(username=username, password=TEST_PASSWORD) + + # Test that the endpoint returns 404 if the Algolia Search API key is not set. + with override_settings(ENTERPRISE_ALGOLIA_SEARCH_API_KEY=None): + response = self.client.get(ENTERPRISE_CUSTOMER_ALGOLIA_KEY_ENDPOINT) + assert response.status_code == status.HTTP_404_NOT_FOUND + + # Test that the endpoint returns 404 if the user is not linked to any enterprise. + response = self.client.get(ENTERPRISE_CUSTOMER_ALGOLIA_KEY_ENDPOINT) + assert response.status_code == status.HTTP_404_NOT_FOUND + + # Test that the endpoint returns 200 if the user is linked to at least one enterprise. + enterprise_customer_1 = factories.EnterpriseCustomerFactory(uuid=FAKE_UUIDS[0]) + enterprise_customer_2 = factories.EnterpriseCustomerFactory(uuid=FAKE_UUIDS[1]) + factories.EnterpriseCustomerFactory(uuid=FAKE_UUIDS[2]) # extra unlinked enterprise + + factories.EnterpriseCustomerUserFactory( + user_id=self.user.id, + enterprise_customer=enterprise_customer_1 + ) + factories.EnterpriseCustomerUserFactory( + user_id=self.user.id, + enterprise_customer=enterprise_customer_2 + ) + + response = self.client.get(ENTERPRISE_CUSTOMER_ALGOLIA_KEY_ENDPOINT) + assert response.status_code == status.HTTP_200_OK + + # Test that the endpoint returns the key encoding correct filters. + decoded_key = base64.b64decode(response.json()["key"]).decode("utf-8") + assert decoded_key.endswith( + f"filters=enterprise_customer_uuids%3A{FAKE_UUIDS[0]}+OR+enterprise_customer_uuids%3A{FAKE_UUIDS[1]}" + ) + @ddt.ddt @mark.django_db From 8b48b1145ceffb4b41a0d0882ac51c85f7e95a55 Mon Sep 17 00:00:00 2001 From: 0x29a Date: Sat, 30 Dec 2023 12:17:16 +0100 Subject: [PATCH 6/8] fix: renamed migrations This commit contains renamed migrations that were extracted from other pull requests. We do this because upstream PRs depend on migrations that don't exist in v4.6.0 IMPORTANT: if you've previously had `opencraft-release/palm.1` installed, you'll have to fake `0194_hide_course_price_when_zero` and `0195_allow_enrollment_in_invite_only_courses` migrations from this commit, as changes are most likely already applied to your DB. See this and further comments for details: https://tasks.opencraft.com/browse/BB-7619?focusedCommentId=274168&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-274168 Also see the similar commit for nutmeg: https://github.com/open-craft/edx-enterprise/pull/10/commits/9840c0bb4457a63ad1b6e476845c7481ecbd2f58 --- ...lter_enterprisecourseenrollment_options.py | 17 ++++++++++++++ .../0194_hide_course_price_when_zero.py | 23 +++++++++++++++++++ ...allow_enrollment_in_invite_only_courses.py | 23 +++++++++++++++++++ 3 files changed, 63 insertions(+) create mode 100644 enterprise/migrations/0193_alter_enterprisecourseenrollment_options.py create mode 100644 enterprise/migrations/0194_hide_course_price_when_zero.py create mode 100644 enterprise/migrations/0195_allow_enrollment_in_invite_only_courses.py diff --git a/enterprise/migrations/0193_alter_enterprisecourseenrollment_options.py b/enterprise/migrations/0193_alter_enterprisecourseenrollment_options.py new file mode 100644 index 0000000000..54a3c3eb87 --- /dev/null +++ b/enterprise/migrations/0193_alter_enterprisecourseenrollment_options.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2 on 2023-12-29 17:03 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('enterprise', '0192_auto_20231009_1302'), + ] + + operations = [ + migrations.AlterModelOptions( + name='enterprisecourseenrollment', + options={'ordering': ['id']}, + ), + ] diff --git a/enterprise/migrations/0194_hide_course_price_when_zero.py b/enterprise/migrations/0194_hide_course_price_when_zero.py new file mode 100644 index 0000000000..9e5ce7ab5e --- /dev/null +++ b/enterprise/migrations/0194_hide_course_price_when_zero.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.23 on 2023-12-08 05:36 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('enterprise', '0193_alter_enterprisecourseenrollment_options'), + ] + + operations = [ + migrations.AddField( + model_name='enterprisecustomer', + name='hide_course_price_when_zero', + field=models.BooleanField(default=False, help_text='Specify whether course cost should be hidden in the landing page when the final price is zero.'), + ), + migrations.AddField( + model_name='historicalenterprisecustomer', + name='hide_course_price_when_zero', + field=models.BooleanField(default=False, help_text='Specify whether course cost should be hidden in the landing page when the final price is zero.'), + ), + ] diff --git a/enterprise/migrations/0195_allow_enrollment_in_invite_only_courses.py b/enterprise/migrations/0195_allow_enrollment_in_invite_only_courses.py new file mode 100644 index 0000000000..36038e48a5 --- /dev/null +++ b/enterprise/migrations/0195_allow_enrollment_in_invite_only_courses.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.23 on 2023-12-08 09:54 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('enterprise', '0194_hide_course_price_when_zero'), + ] + + operations = [ + migrations.AddField( + model_name='enterprisecustomer', + name='allow_enrollment_in_invite_only_courses', + field=models.BooleanField(default=False, help_text="Specifies if learners are allowed to enroll into courses marked as 'invitation-only', when they attempt to enroll from the landing page."), + ), + migrations.AddField( + model_name='historicalenterprisecustomer', + name='allow_enrollment_in_invite_only_courses', + field=models.BooleanField(default=False, help_text="Specifies if learners are allowed to enroll into courses marked as 'invitation-only', when they attempt to enroll from the landing page."), + ), + ] From 724cdc164a05a9f31752d1bc933aa2f823277c5e Mon Sep 17 00:00:00 2001 From: 0x29a Date: Sat, 30 Dec 2023 12:22:46 +0100 Subject: [PATCH 7/8] chore: version bump, changelog entry --- CHANGELOG.rst | 5 +++++ enterprise/__init__.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 74b6247217..52f0ff02c8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -15,6 +15,11 @@ Change Log Unreleased ---------- +[4.6.1] +--------- +This version is based on v4.6.0 and contains some backports needed to OpenCraft's clients. There is no "official" v4.6.0. +See this PR for more details: https://github.com/open-craft/edx-enterprise/pull/13 + [4.6.0] ------- feat: Added enable_source_demo_data_for_analytics_and_lpr field to EnterpriseCustomer. diff --git a/enterprise/__init__.py b/enterprise/__init__.py index c629ee4f9b..2364a6257c 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.6.0" +__version__ = "4.6.1" From ff5da2dd551d1fd1e2cd946dfdf8a5bd678b3b86 Mon Sep 17 00:00:00 2001 From: 0x29a Date: Sat, 30 Dec 2023 13:37:14 +0100 Subject: [PATCH 8/8] build: enable CI for pull requests --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c77c86867..c28ba1bb71 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,6 @@ on: push: branches: [master] pull_request: - branches: [master] concurrency: group: ci-${{ github.event.pull_request.number || github.ref }}