From fa6ef2a11d9371074113676a049fdd6329ce2b7c Mon Sep 17 00:00:00 2001 From: MueezKhan246 Date: Thu, 4 Jul 2024 22:27:55 +0500 Subject: [PATCH 1/6] feat: refactored validation logic for bulk license enrollment endpoint --- license_manager/apps/api/serializers.py | 4 +++ .../apps/api/v1/tests/test_views.py | 25 ++++++++++--- license_manager/apps/api/v1/views.py | 35 ++++++++----------- 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/license_manager/apps/api/serializers.py b/license_manager/apps/api/serializers.py index 72e179a1..8758a298 100644 --- a/license_manager/apps/api/serializers.py +++ b/license_manager/apps/api/serializers.py @@ -716,6 +716,10 @@ class Meta: 'subscription_uuid', ] + def validate(self, data): + if data.get('enroll_all') and not data.get('subscription_uuid'): + raise serializers.ValidationError({'subscription_id': 'This field is required when enroll_all is True.'}) + return data class EnterpriseEnrollmentWithLicenseSubsidyRequestSerializer(serializers.Serializer): # pylint: disable=abstract-method """ diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index d57590c4..66c0c9a1 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -4,6 +4,7 @@ """ import datetime import random +import string from math import ceil, sqrt from unittest import mock from uuid import uuid4 @@ -66,6 +67,18 @@ assert_pii_cleared, ) from license_manager.apps.subscriptions.utils import localized_utcnow +import os +import django + +os.environ['DJANGO_SETTINGS_MODULE'] = 'license_manager.settings.test' +django.setup() + + +def generate_random_email(): + name = ''.join(random.choices(string.ascii_lowercase + string.digits, k=10)) + domain = ''.join(random.choices(string.ascii_lowercase, k=5)) + + return f"{name}@{domain}.com" def _jwt_payload_from_role_context_pairs(user, role_context_pairs): @@ -3979,6 +3992,7 @@ def test_bulk_enroll_all(self, mock_get_decoded_jwt, mock_send_task): self._assign_learner_roles() mock_get_decoded_jwt.return_value = self._decoded_jwt data = { + 'emails': [self.user.email], 'course_run_keys': [self.course_key], 'notify': True, } @@ -4012,6 +4026,7 @@ def test_bulk_enroll_all_ignores_revoked_licenses( self._assign_learner_roles() mock_get_decoded_jwt.return_value = self._decoded_jwt data = { + 'emails': [self.user.email], 'course_run_keys': [self.course_key], 'notify': True, } @@ -4054,6 +4069,7 @@ def test_bulk_enroll_all_uses_assigned_and_active_licenses( self._assign_learner_roles() mock_get_decoded_jwt.return_value = self._decoded_jwt data = { + 'emails': [self.user.email], 'course_run_keys': [self.course_key], 'notify': True, } @@ -4085,8 +4101,8 @@ def test_bulk_licensed_enrollment_with_enroll_all_no_sub_uuid(self): url = self._get_url_with_params(enroll_all=True) response = self.api_client.post(url, data) assert response.status_code == 400 - - assert response.json() == "Missing the following required request data: ['subscription_id']" + expected_json = {'subscription_id': ['This field is required when enroll_all is True.']} + assert response.json() == expected_json def test_bulk_licensed_enrollment_with_missing_emails(self): """ @@ -4102,7 +4118,8 @@ def test_bulk_licensed_enrollment_with_missing_emails(self): url = self._get_url_with_params() response = self.api_client.post(url, empty_email_data) assert response.status_code == 400 - assert response.json() == "Missing the following required request data: ['emails']" + expected_json = {'emails': ['This list may not be empty.']} + assert response.json() == expected_json def test_bulk_enroll_too_many_enrollments(self): # Unnecessary math time! @@ -4111,7 +4128,7 @@ def test_bulk_enroll_too_many_enrollments(self): self._assign_multi_learner_roles() payload = { - 'emails': random.sample(range(1, 100), enrollment_split), + 'emails': [generate_random_email() for _ in range(enrollment_split)], 'course_run_keys': random.sample(range(1, 100), enrollment_split), 'notify': True, } diff --git a/license_manager/apps/api/v1/views.py b/license_manager/apps/api/v1/views.py index 338fa5b5..72ec047a 100644 --- a/license_manager/apps/api/v1/views.py +++ b/license_manager/apps/api/v1/views.py @@ -1518,29 +1518,24 @@ def _validate_enrollment_request_params(self): """ Helper function to validate both the existence of required params and their typing. """ - self.validation_errors = [] - self.missing_params = [] - if self.requested_notify_learners is None: - self.missing_params.append('notify') + query_params_serializer = serializers.EnterpriseEnrollmentWithLicenseSubsidyQueryParamsSerializer( + data=self.request.query_params + ) + request_body_serializer = serializers.EnterpriseEnrollmentWithLicenseSubsidyRequestSerializer( + data=self.request.data + ) - # Gather all missing and incorrect typing validation errors - if not self.requested_course_run_keys: - self.missing_params.append('course_run_keys') - if not self.requested_enroll_all: - if not self.requested_user_emails: - self.missing_params.append('emails') - if self.requested_enroll_all and not self.requested_subscription_id: - self.missing_params.append('subscription_id') - if not self.requested_enterprise_id: - self.missing_params.append('enterprise_customer_uuid') + # Validate the query parameters + if not query_params_serializer.is_valid(): + raise ValidationError(query_params_serializer.errors) - # Report param type errors - if self.validation_errors: - return 'Received invalid types for the following required params: {}'.format(self.validation_errors) + # Validate the request body + if not request_body_serializer.is_valid(): + raise ValidationError(request_body_serializer.errors) - # Report required params type errors - if self.missing_params: - return 'Missing the following required request data: {}'.format(self.missing_params) + if not self.requested_enroll_all: + if not self.requested_user_emails: + return 'Missing the following required request data: {}'.format(['emails']) return '' From c1ff464855fdb2e65511e6b599ff4e38dcedf44b Mon Sep 17 00:00:00 2001 From: MueezKhan246 Date: Fri, 5 Jul 2024 12:30:49 +0500 Subject: [PATCH 2/6] refactor: fixed linting issues --- license_manager/apps/api/serializers.py | 6 +++--- license_manager/apps/api/v1/tests/test_views.py | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/license_manager/apps/api/serializers.py b/license_manager/apps/api/serializers.py index 8758a298..3190d5a6 100644 --- a/license_manager/apps/api/serializers.py +++ b/license_manager/apps/api/serializers.py @@ -716,10 +716,10 @@ class Meta: 'subscription_uuid', ] - def validate(self, data): - if data.get('enroll_all') and not data.get('subscription_uuid'): + def validate(self, attrs): + if attrs.get('enroll_all') and not attrs.get('subscription_uuid'): raise serializers.ValidationError({'subscription_id': 'This field is required when enroll_all is True.'}) - return data + return attrs class EnterpriseEnrollmentWithLicenseSubsidyRequestSerializer(serializers.Serializer): # pylint: disable=abstract-method """ diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index 66c0c9a1..d123fe95 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -3,6 +3,7 @@ Tests for the Subscription and License V1 API view sets. """ import datetime +import os import random import string from math import ceil, sqrt @@ -10,6 +11,7 @@ from uuid import uuid4 import ddt +import django import pytest from django.conf import settings from django.contrib.auth import get_user_model @@ -67,8 +69,7 @@ assert_pii_cleared, ) from license_manager.apps.subscriptions.utils import localized_utcnow -import os -import django + os.environ['DJANGO_SETTINGS_MODULE'] = 'license_manager.settings.test' django.setup() From 59c4f68d6154718c1d610a46a75bb5fdfd095aef Mon Sep 17 00:00:00 2001 From: MueezKhan246 Date: Fri, 5 Jul 2024 12:35:01 +0500 Subject: [PATCH 3/6] refactor: provided correct amount of blank space --- license_manager/apps/api/serializers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/license_manager/apps/api/serializers.py b/license_manager/apps/api/serializers.py index 3190d5a6..a100f8d3 100644 --- a/license_manager/apps/api/serializers.py +++ b/license_manager/apps/api/serializers.py @@ -721,6 +721,7 @@ def validate(self, attrs): raise serializers.ValidationError({'subscription_id': 'This field is required when enroll_all is True.'}) return attrs + class EnterpriseEnrollmentWithLicenseSubsidyRequestSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer for the enterprise enrollment with license subsidy request From da4b00c0f5b5f21d058e6e0f375f810a4e9c5668 Mon Sep 17 00:00:00 2001 From: MueezKhan246 Date: Fri, 5 Jul 2024 13:17:40 +0500 Subject: [PATCH 4/6] refactor: removed redundant django test environment variable --- license_manager/apps/api/v1/tests/test_views.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index d123fe95..a7eccae3 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -71,10 +71,6 @@ from license_manager.apps.subscriptions.utils import localized_utcnow -os.environ['DJANGO_SETTINGS_MODULE'] = 'license_manager.settings.test' -django.setup() - - def generate_random_email(): name = ''.join(random.choices(string.ascii_lowercase + string.digits, k=10)) domain = ''.join(random.choices(string.ascii_lowercase, k=5)) From d11bfccd44f759d4c8d2d70dd5a7ee074b968b61 Mon Sep 17 00:00:00 2001 From: MueezKhan246 Date: Fri, 5 Jul 2024 13:20:39 +0500 Subject: [PATCH 5/6] refactor: removed redundant imports --- license_manager/apps/api/v1/tests/test_views.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index a7eccae3..dc0458f2 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -3,7 +3,6 @@ Tests for the Subscription and License V1 API view sets. """ import datetime -import os import random import string from math import ceil, sqrt @@ -11,7 +10,6 @@ from uuid import uuid4 import ddt -import django import pytest from django.conf import settings from django.contrib.auth import get_user_model From 67f98825b4083020e4605ab0f2747937756b8093 Mon Sep 17 00:00:00 2001 From: MueezKhan246 Date: Tue, 9 Jul 2024 17:29:35 +0500 Subject: [PATCH 6/6] refactor: removed nested redundant validation check --- license_manager/apps/api/v1/views.py | 42 ++++++++++------------------ 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/license_manager/apps/api/v1/views.py b/license_manager/apps/api/v1/views.py index 72ec047a..85c8ef44 100644 --- a/license_manager/apps/api/v1/views.py +++ b/license_manager/apps/api/v1/views.py @@ -1514,31 +1514,6 @@ def _validate_status_request_params(self): return '' - def _validate_enrollment_request_params(self): - """ - Helper function to validate both the existence of required params and their typing. - """ - query_params_serializer = serializers.EnterpriseEnrollmentWithLicenseSubsidyQueryParamsSerializer( - data=self.request.query_params - ) - request_body_serializer = serializers.EnterpriseEnrollmentWithLicenseSubsidyRequestSerializer( - data=self.request.data - ) - - # Validate the query parameters - if not query_params_serializer.is_valid(): - raise ValidationError(query_params_serializer.errors) - - # Validate the request body - if not request_body_serializer.is_valid(): - raise ValidationError(request_body_serializer.errors) - - if not self.requested_enroll_all: - if not self.requested_user_emails: - return 'Missing the following required request data: {}'.format(['emails']) - - return '' - @permission_required( constants.SUBSCRIPTIONS_ADMIN_LEARNER_ACCESS_PERMISSION, fn=lambda request: utils.get_context_for_customer_agreement_from_request(request), # pylint: disable=unnecessary-lambda @@ -1571,9 +1546,20 @@ def post(self, request): - Exceeding BULK_ENROLL_REQUEST_LIMIT by passing too many learners + course keys, 400 - Enterprise UUID without a valid CustomerAgreement, 404 """ - param_validation = self._validate_enrollment_request_params() - if param_validation: - return Response(param_validation, status=status.HTTP_400_BAD_REQUEST) + query_params_serializer = serializers.EnterpriseEnrollmentWithLicenseSubsidyQueryParamsSerializer( + data=self.request.query_params + ) + request_body_serializer = serializers.EnterpriseEnrollmentWithLicenseSubsidyRequestSerializer( + data=self.request.data + ) + + # Validate the query parameters + if not query_params_serializer.is_valid(): + raise ValidationError(query_params_serializer.errors) + + # Validate the request body + if not request_body_serializer.is_valid(): + raise ValidationError(request_body_serializer.errors) num_enrollments = len(self.requested_course_run_keys) * len(self.request.data.get('emails', [])) if settings.BULK_ENROLL_REQUEST_LIMIT < num_enrollments: