From 24873c3205a3740d4a7df6f190cb86f79bd8becc Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Mon, 2 Dec 2024 16:02:33 -0500 Subject: [PATCH] fix: account for revoked licenses and staff users in auto-apply logic (#605) --- .../apps/api/v1/tests/test_bff_views.py | 144 +++++++++++++++--- enterprise_access/apps/bffs/api.py | 3 - enterprise_access/apps/bffs/context.py | 16 ++ enterprise_access/apps/bffs/handlers.py | 73 ++++++--- .../apps/bffs/tests/test_context.py | 3 + enterprise_access/apps/bffs/tests/utils.py | 5 + 6 files changed, 202 insertions(+), 42 deletions(-) diff --git a/enterprise_access/apps/api/v1/tests/test_bff_views.py b/enterprise_access/apps/api/v1/tests/test_bff_views.py index d6114d4b..5f642aaa 100644 --- a/enterprise_access/apps/api/v1/tests/test_bff_views.py +++ b/enterprise_access/apps/api/v1/tests/test_bff_views.py @@ -352,73 +352,142 @@ def test_dashboard_with_subscriptions_license_activation( self.assertEqual(response.json(), expected_response_data) @ddt.data( - # No identity provider, no universal link auto-apply, no plan for auto-apply. + # No existing licenses, identity provider, no universal link auto-apply, no plan for auto-apply, + # and not a staff request user. # Expected: Should not auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': False, 'auto_apply_with_universal_link': False, 'has_plan_for_auto_apply': False, 'should_auto_apply': False, }, - # Identity provider exists, but no universal link auto-apply, no plan for auto-apply. + # No existing licenses, identity provider exists, but no universal link auto-apply, no plan for auto-apply, + # and not a staff request user. # Expected: Should not auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': True, 'auto_apply_with_universal_link': False, 'has_plan_for_auto_apply': False, 'should_auto_apply': False, }, - # No identity provider, universal link auto-apply is enabled, no plan for auto-apply. + # No existing licenses, no identity provider, universal link auto-apply is enabled, no plan for auto-apply, + # and not a staff request user. # Expected: Should not auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': False, 'auto_apply_with_universal_link': True, 'has_plan_for_auto_apply': False, 'should_auto_apply': False, }, - # Identity provider exists, universal link auto-apply is enabled, no plan for auto-apply. + # No existing licenses, identity provider exists, universal link auto-apply is enabled, no plan for auto-apply, + # and not a staff request user. # Expected: Should not auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': True, 'auto_apply_with_universal_link': True, 'has_plan_for_auto_apply': False, 'should_auto_apply': False, }, - # No identity provider, no universal link auto-apply, but a plan for auto-apply exists. + # No existing licenses, no identity provider, no universal link auto-apply, a plan for auto-apply exists, + # and not a staff request user. # Expected: Should not auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': False, 'auto_apply_with_universal_link': False, 'has_plan_for_auto_apply': True, 'should_auto_apply': False, }, - # Identity provider exists, no universal link auto-apply, but a plan for auto-apply exists. + # No existing licenses, identity provider exists, no universal link auto-apply, + # a plan for auto-apply exists, and not a staff request user. # Expected: Should auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': True, 'auto_apply_with_universal_link': False, 'has_plan_for_auto_apply': True, 'should_auto_apply': True, }, - # No identity provider, universal link auto-apply is enabled, and a plan for auto-apply exists. + # No existing licenses, no identity provider, universal link auto-apply is enabled, + # a plan for auto-apply exists, and not a staff request user. # Expected: Should auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': False, 'auto_apply_with_universal_link': True, 'has_plan_for_auto_apply': True, 'should_auto_apply': True, }, - # Identity provider exists, universal link auto-apply is enabled, and a plan for auto-apply exists. + # No existing licenses, identity provider exists, universal link auto-apply is enabled, + # a plan for auto-apply exists, and not a staff request user. # Expected: Should auto-apply. { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, 'identity_provider': True, 'auto_apply_with_universal_link': True, 'has_plan_for_auto_apply': True, 'should_auto_apply': True, }, + # Existing activated license, identity provider exists, universal link auto-apply is enabled, + # a plan for auto-apply exists, and not a staff request user. + # Expected: Should not auto-apply. + { + 'has_existing_activated_license': True, + 'has_existing_revoked_license': False, + 'is_staff_request_user': False, + 'identity_provider': True, + 'auto_apply_with_universal_link': True, + 'has_plan_for_auto_apply': True, + 'should_auto_apply': False, + }, + # Existing revoked license, identity provider exists, universal link auto-apply is enabled, + # a plan for auto-apply exists, and not a staff request user. + # Expected: Should not auto-apply. + { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': True, + 'is_staff_request_user': False, + 'identity_provider': True, + 'auto_apply_with_universal_link': True, + 'has_plan_for_auto_apply': True, + 'should_auto_apply': False, + }, + # No existing licenses, identity provider exists, universal link auto-apply is enabled, + # a plan for auto-apply exists, and is a staff request user. + # Expected: Should not auto-apply. + { + 'has_existing_activated_license': False, + 'has_existing_revoked_license': False, + 'is_staff_request_user': True, + 'identity_provider': True, + 'auto_apply_with_universal_link': True, + 'has_plan_for_auto_apply': True, + 'should_auto_apply': False, + }, ) @ddt.unpack @mock_dashboard_dependencies + @mock.patch('enterprise_access.apps.api_client.lms_client.LmsApiClient.get_enterprise_customer_data') @mock.patch( 'enterprise_access.apps.api_client.license_manager_client.LicenseManagerUserApiClient.auto_apply_license' ) @@ -429,6 +498,10 @@ def test_dashboard_with_subscriptions_license_auto_apply( mock_get_subscription_licenses_for_learner, mock_get_enterprise_course_enrollments, mock_auto_apply_license, + mock_get_enterprise_customer_data, + has_existing_activated_license, + has_existing_revoked_license, + is_staff_request_user, identity_provider, auto_apply_with_universal_link, has_plan_for_auto_apply, @@ -442,6 +515,13 @@ def test_dashboard_with_subscriptions_license_auto_apply( 'system_wide_role': SYSTEM_ENTERPRISE_LEARNER_ROLE, 'context': self.mock_enterprise_customer_uuid, }]) + + if is_staff_request_user: + # Set the request user as a staff user and mock the enterprise customer response data. + self.user.is_staff = True + self.user.save() + mock_get_enterprise_customer_data.return_value = self.mock_enterprise_customer + mock_identity_provider = 'mock_idp' if identity_provider else None mock_identity_providers = ( [ @@ -458,14 +538,17 @@ def test_dashboard_with_subscriptions_license_auto_apply( 'identity_provider': mock_identity_provider, 'identity_providers': mock_identity_providers, } + mock_linked_enterprise_customer_users = ( + [] + if is_staff_request_user + else [{ + 'active': True, + 'enterprise_customer': mock_enterprise_customer_with_auto_apply, + }] + ) mock_enterprise_learner_response_data = { **self.mock_enterprise_learner_response_data, - 'results': [ - { - 'active': True, - 'enterprise_customer': mock_enterprise_customer_with_auto_apply, - }, - ], + 'results': mock_linked_enterprise_customer_users, } mock_get_enterprise_customers_for_user.return_value = mock_enterprise_learner_response_data mock_auto_applied_subscription_license = { @@ -484,6 +567,17 @@ def test_dashboard_with_subscriptions_license_auto_apply( ), }, } + if has_existing_activated_license: + mock_subscription_licenses_data['results'].append({ + **self.mock_subscription_license, + 'status': 'activated', + 'activation_date': '2024-01-01T00:00:00Z', + }) + if has_existing_revoked_license: + mock_subscription_licenses_data['results'].append({ + **self.mock_subscription_license, + 'status': 'revoked', + }) mock_get_subscription_licenses_for_learner.return_value = mock_subscription_licenses_data mock_auto_apply_license.return_value = mock_auto_applied_subscription_license mock_get_default_enrollment_intentions_learner_status.return_value =\ @@ -511,7 +605,23 @@ def test_dashboard_with_subscriptions_license_auto_apply( 'status': 'activated', 'activation_date': '2024-01-01T00:00:00Z', } - expected_licenses = [expected_activated_subscription_license] if should_auto_apply else [] + expected_activated_licenses = ( + [expected_activated_subscription_license] + if should_auto_apply or has_existing_activated_license + else [] + ) + expected_revoked_subscription_license = { + **self.expected_subscription_license, + 'status': 'revoked', + } + expected_revoked_licenses = ( + [expected_revoked_subscription_license] + if has_existing_revoked_license + else [] + ) + expected_licenses = [] + expected_licenses.extend(expected_activated_licenses) + expected_licenses.extend(expected_revoked_licenses) expected_response_data = self.mock_dashboard_route_response_data.copy() expected_show_integration_warning = bool(identity_provider) expected_response_data.update({ @@ -526,9 +636,9 @@ def test_dashboard_with_subscriptions_license_auto_apply( 'customer_agreement': expected_customer_agreement, 'subscription_licenses': expected_licenses, 'subscription_licenses_by_status': { - 'activated': expected_licenses, + 'activated': expected_activated_licenses, 'assigned': [], - 'revoked': [], + 'revoked': expected_revoked_licenses, }, }, }, diff --git a/enterprise_access/apps/bffs/api.py b/enterprise_access/apps/bffs/api.py index 5445b33f..94f21eb1 100644 --- a/enterprise_access/apps/bffs/api.py +++ b/enterprise_access/apps/bffs/api.py @@ -229,9 +229,6 @@ def _get_staff_enterprise_customer( if has_enterprise_customer_slug_or_uuid and user.is_staff: try: staff_enterprise_customer = get_and_cache_enterprise_customer( - - ) - staff_enterprise_customer = LmsApiClient().get_enterprise_customer_data( enterprise_customer_uuid=enterprise_customer_uuid, enterprise_customer_slug=enterprise_customer_slug, ) diff --git a/enterprise_access/apps/bffs/context.py b/enterprise_access/apps/bffs/context.py index 4dedfaa3..be547611 100644 --- a/enterprise_access/apps/bffs/context.py +++ b/enterprise_access/apps/bffs/context.py @@ -98,6 +98,22 @@ def active_enterprise_customer(self): def all_linked_enterprise_customer_users(self): return self.data.get('all_linked_enterprise_customer_users') + @property + def is_request_user_linked_to_enterprise_customer(self): + """ + Returns True if the request user is linked to the resolved enterprise customer, False otherwise. + Primary use case is to determine if the request user is explicitly linked to the enterprise customer versus + being a staff user with access. + """ + if not self.enterprise_customer: + return False + + enterprise_customer_uuid = self.enterprise_customer.get('uuid') + return any( + enterprise_customer_user.get('enterprise_customer', {}).get('uuid') == enterprise_customer_uuid + for enterprise_customer_user in self.all_linked_enterprise_customer_users + ) + @property def staff_enterprise_customer(self): return self.data.get('staff_enterprise_customer') diff --git a/enterprise_access/apps/bffs/handlers.py b/enterprise_access/apps/bffs/handlers.py index 727de13f..a8b6a758 100644 --- a/enterprise_access/apps/bffs/handlers.py +++ b/enterprise_access/apps/bffs/handlers.py @@ -238,19 +238,47 @@ def transform_subscriptions_result(self, subscriptions_result): 'subscription_licenses_by_status': subscription_licenses_by_status, } + def _current_subscription_licenses_for_status(self, status): + """ + Filter subscription licenses by license status and current subscription plan. + """ + current_licenses_for_status = [ + _license for _license in self.subscription_licenses_by_status.get(status, []) + if _license['subscription_plan']['is_current'] + ] + return current_licenses_for_status + + @property + def current_activated_licenses(self): + """ + Returns list of current, activated licenses, if any, for the user. + """ + activated_licenses = self._current_subscription_licenses_for_status('activated') + return activated_licenses + @property - def current_active_license(self): + def current_activated_license(self): """ Returns an activated license for the user iff the related subscription plan is current, otherwise returns None. """ - current_active_licenses = [ - _license for _license in self.subscription_licenses_by_status.get('activated', []) - if _license['subscription_plan']['is_current'] - ] - if current_active_licenses: - return current_active_licenses[0] - return None + return self.current_activated_licenses[0] if self.current_activated_licenses else None + + @property + def current_revoked_licenses(self): + """ + Returns a revoked license for the user iff the related subscription plan is current, + otherwise returns None. + """ + return self._current_subscription_licenses_for_status('revoked') + + @property + def current_assigned_licenses(self): + """ + Returns an assigned license for the user iff the related subscription plan is current, + otherwise returns None. + """ + return self._current_subscription_licenses_for_status('assigned') def process_subscription_licenses(self): """ @@ -262,10 +290,10 @@ def process_subscription_licenses(self): This method is called after `load_subscription_licenses` to handle further actions based on the loaded data. """ - if not self.subscriptions or self.current_active_license: + if not self.subscriptions or self.current_activated_license: # Skip processing if: # - there is no subscriptions data - # - user already has an activated license + # - user already has an activated license(s) return # Check if there are 'assigned' licenses that need to be activated @@ -290,9 +318,8 @@ def check_and_activate_assigned_license(self): Check if there are assigned licenses that need to be activated. """ subscription_licenses_by_status = self.subscription_licenses_by_status - assigned_licenses = subscription_licenses_by_status.get('assigned', []) activated_licenses = [] - for subscription_license in assigned_licenses: + for subscription_license in self.current_assigned_licenses: activation_key = subscription_license.get('activation_key') if activation_key: try: @@ -325,7 +352,7 @@ def check_and_activate_assigned_license(self): ) # Update the subscription_licenses_by_status data with the activated licenses - updated_activated_licenses = subscription_licenses_by_status.get('activated', []) + updated_activated_licenses = self.current_activated_licenses updated_activated_licenses.extend(activated_licenses) if updated_activated_licenses: subscription_licenses_by_status['activated'] = updated_activated_licenses @@ -333,7 +360,7 @@ def check_and_activate_assigned_license(self): activated_license_uuids = {license['uuid'] for license in activated_licenses} remaining_assigned_licenses = [ subscription_license - for subscription_license in assigned_licenses + for subscription_license in self.current_assigned_licenses if subscription_license['uuid'] not in activated_license_uuids ] if remaining_assigned_licenses: @@ -363,13 +390,15 @@ def check_and_auto_apply_license(self): """ Check if auto-apply licenses are available and apply them to the user. """ - subscription_licenses_by_status = self.subscription_licenses_by_status - assigned_licenses = subscription_licenses_by_status.get('assigned', []) - - if assigned_licenses or self.current_active_license: - # Skip auto-apply if user already has assigned license(s) or an already-activated license + if (self.subscription_licenses or not self.context.is_request_user_linked_to_enterprise_customer): + # Skip auto-apply if: + # - User has assigned/current license(s) + # - User has activated/current license(s) + # - User has revoked/current license(s) + # - User is not explicitly linked to the enterprise customer (e.g., staff request user) return + subscription_licenses_by_status = self.subscription_licenses_by_status customer_agreement = self.subscriptions.get('customer_agreement') or {} has_subscription_plan_for_auto_apply = ( bool(customer_agreement.get('subscription_for_auto_applied_licenses')) and @@ -435,19 +464,19 @@ def enroll_in_redeemable_default_enterprise_enrollment_intentions(self): needs_enrollment = enrollment_statuses.get('needs_enrollment', {}) needs_enrollment_enrollable = needs_enrollment.get('enrollable', []) - if not (needs_enrollment_enrollable and self.current_active_license): + if not (needs_enrollment_enrollable and self.current_activated_license): return license_uuids_by_course_run_key = {} for enrollment_intention in needs_enrollment_enrollable: - subscription_plan = self.current_active_license.get('subscription_plan', {}) + subscription_plan = self.current_activated_license.get('subscription_plan', {}) subscription_catalog = subscription_plan.get('enterprise_catalog_uuid') applicable_catalog_to_enrollment_intention = enrollment_intention.get( 'applicable_enterprise_catalog_uuids' ) if subscription_catalog in applicable_catalog_to_enrollment_intention: course_run_key = enrollment_intention['course_run_key'] - license_uuids_by_course_run_key[course_run_key] = self.current_active_license['uuid'] + license_uuids_by_course_run_key[course_run_key] = self.current_activated_license['uuid'] response_payload = self._request_default_enrollment_realizations(license_uuids_by_course_run_key) diff --git a/enterprise_access/apps/bffs/tests/test_context.py b/enterprise_access/apps/bffs/tests/test_context.py index e62f1e96..e1843c38 100644 --- a/enterprise_access/apps/bffs/tests/test_context.py +++ b/enterprise_access/apps/bffs/tests/test_context.py @@ -78,6 +78,8 @@ def test_handler_context_init(self, mock_get_enterprise_customers_for_user, rais self.assertEqual(context.lms_user_id, self.mock_user.lms_user_id) expected_enterprise_customer = None if raises_exception else self.mock_enterprise_customer self.assertEqual(context.enterprise_customer, expected_enterprise_customer) + expected_is_linked_user = False if raises_exception else True + self.assertEqual(context.is_request_user_linked_to_enterprise_customer, expected_is_linked_user) @ddt.data( {'raises_exception': False}, @@ -141,6 +143,7 @@ def test_handler_context_init_staff_user_unlinked( self.assertEqual(context.lms_user_id, self.mock_staff_user.lms_user_id) expected_enterprise_customer = None if raises_exception else self.mock_enterprise_customer self.assertEqual(context.enterprise_customer, expected_enterprise_customer) + self.assertEqual(context.is_request_user_linked_to_enterprise_customer, False) @ddt.data( # No enterprise customer uuid/slug in the request; returns active enterprise customer user diff --git a/enterprise_access/apps/bffs/tests/utils.py b/enterprise_access/apps/bffs/tests/utils.py index d9774c5f..551b2484 100644 --- a/enterprise_access/apps/bffs/tests/utils.py +++ b/enterprise_access/apps/bffs/tests/utils.py @@ -170,6 +170,8 @@ def get_mock_handler_context(self, **kwargs): 'enterprise_customer_uuid': getattr(mock_handler_context, '_enterprise_customer_uuid'), 'enterprise_customer_slug': getattr(mock_handler_context, '_enterprise_customer_slug'), 'enterprise_customer': mock_property_enterprise_customer, + 'active_enterprise_customer': mock_property_enterprise_customer, + 'staff_enterprise_customer': None, 'lms_user_id': getattr(mock_handler_context, '_lms_user_id'), 'enterprise_features': getattr(mock_handler_context, '_enterprise_features'), } @@ -186,6 +188,9 @@ def get_mock_handler_context(self, **kwargs): def mock_enterprise_learner_dependency(func): + """ + Mock the enterprise customer related service dependencies. + """ @mock.patch('enterprise_access.apps.api_client.lms_client.LmsUserApiClient.get_enterprise_customers_for_user') def wrapper(self, *args, **kwargs): return func(self, *args, **kwargs)