From 3f6b1bc2ea1d3ddc6da578a3d5d753e9b294a73b Mon Sep 17 00:00:00 2001 From: Alexander J Sheehan Date: Mon, 4 Mar 2024 14:59:25 +0000 Subject: [PATCH 1/2] Revert "fix: test fixes" This reverts commit e3074d7b72abc885d5e2901fc45c4be31b4c7131. --- .../apps/api/v1/tests/test_subsidy_access_policy_views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py index 4f80ae0f..fafb1726 100644 --- a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py +++ b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py @@ -45,11 +45,12 @@ PolicyGroupAssociationFactory ) from enterprise_access.apps.subsidy_access_policy.utils import create_idempotency_key_for_transaction -from test_utils import TEST_ENTERPRISE_GROUP_UUID, TEST_USER_RECORD, APITestWithMocks +from test_utils import TEST_USER_RECORD, APITestWithMocks SUBSIDY_ACCESS_POLICY_LIST_ENDPOINT = reverse('api:v1:subsidy-access-policies-list') TEST_ENTERPRISE_UUID = uuid4() +TEST_GROUP_UUID = uuid4() # pylint: disable=missing-function-docstring @@ -1067,14 +1068,13 @@ def setup_mocks(self): self.addCleanup(contains_key_patcher.stop) self.addCleanup(get_content_metadata_patcher.stop) - @mock.patch('enterprise_access.apps.api_client.base_oauth.OAuthAPIClient') @mock.patch('enterprise_access.apps.subsidy_access_policy.models.get_and_cache_transactions_for_learner') - def test_redeem_policy(self, mock_transactions_cache_for_learner, mock_oauth): # pylint: disable=unused-argument + def test_redeem_policy(self, mock_transactions_cache_for_learner): # pylint: disable=unused-argument """ Verify that SubsidyAccessPolicyRedeemViewset redeem endpoint works as expected """ PolicyGroupAssociationFactory( - enterprise_group_uuid=TEST_ENTERPRISE_GROUP_UUID, + enterprise_group_uuid=TEST_GROUP_UUID, subsidy_access_policy=self.redeemable_policy ) self.mock_get_content_metadata.return_value = {'content_price': 123} From a59e68474a0bc084f4b401546da2875991c6b9f9 Mon Sep 17 00:00:00 2001 From: Alexander J Sheehan Date: Mon, 4 Mar 2024 15:19:00 +0000 Subject: [PATCH 2/2] Revert "feat: adding enterprise group checks in can_redeem" This reverts commit fd40488cd3e672f8f2414edda63cfd939be6e67e. --- .../tests/test_subsidy_access_policy_views.py | 14 +----- .../apps/subsidy_access_policy/constants.py | 1 - .../apps/subsidy_access_policy/models.py | 22 +-------- .../tests/test_models.py | 45 ++----------------- 4 files changed, 6 insertions(+), 76 deletions(-) diff --git a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py index fafb1726..a4259608 100644 --- a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py +++ b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py @@ -41,8 +41,7 @@ from enterprise_access.apps.subsidy_access_policy.tests.factories import ( AssignedLearnerCreditAccessPolicyFactory, PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory, - PerLearnerSpendCapLearnerCreditAccessPolicyFactory, - PolicyGroupAssociationFactory + PerLearnerSpendCapLearnerCreditAccessPolicyFactory ) from enterprise_access.apps.subsidy_access_policy.utils import create_idempotency_key_for_transaction from test_utils import TEST_USER_RECORD, APITestWithMocks @@ -50,7 +49,6 @@ SUBSIDY_ACCESS_POLICY_LIST_ENDPOINT = reverse('api:v1:subsidy-access-policies-list') TEST_ENTERPRISE_UUID = uuid4() -TEST_GROUP_UUID = uuid4() # pylint: disable=missing-function-docstring @@ -1073,10 +1071,6 @@ def test_redeem_policy(self, mock_transactions_cache_for_learner): # pylint: di """ Verify that SubsidyAccessPolicyRedeemViewset redeem endpoint works as expected """ - PolicyGroupAssociationFactory( - enterprise_group_uuid=TEST_GROUP_UUID, - subsidy_access_policy=self.redeemable_policy - ) self.mock_get_content_metadata.return_value = {'content_price': 123} mock_transaction_record = { 'uuid': str(uuid4()), @@ -1622,12 +1616,6 @@ def setUp(self): ) self.non_redeemable_policy = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory() - group_uuid = uuid4() - PolicyGroupAssociationFactory( - enterprise_group_uuid=group_uuid, - subsidy_access_policy=self.redeemable_policy - ) - def test_can_redeem_policy_missing_params(self): """ Test that the can_redeem endpoint returns an access policy when one is redeemable. diff --git a/enterprise_access/apps/subsidy_access_policy/constants.py b/enterprise_access/apps/subsidy_access_policy/constants.py index 5693c46b..12c86c2d 100644 --- a/enterprise_access/apps/subsidy_access_policy/constants.py +++ b/enterprise_access/apps/subsidy_access_policy/constants.py @@ -105,7 +105,6 @@ class MissingSubsidyAccessReasonUserMessages: REASON_SUBSIDY_EXPIRED = "subsidy_expired" REASON_CONTENT_NOT_IN_CATALOG = "content_not_in_catalog" REASON_LEARNER_NOT_IN_ENTERPRISE = "learner_not_in_enterprise" -REASON_LEARNER_NOT_IN_ENTERPRISE_GROUP = "learner_not_in_enterprise_group" REASON_NOT_ENOUGH_VALUE_IN_SUBSIDY = "not_enough_value_in_subsidy" REASON_LEARNER_MAX_SPEND_REACHED = "learner_max_spend_reached" REASON_POLICY_SPEND_LIMIT_REACHED = "policy_spend_limit_reached" diff --git a/enterprise_access/apps/subsidy_access_policy/models.py b/enterprise_access/apps/subsidy_access_policy/models.py index 70082515..b19a71c3 100644 --- a/enterprise_access/apps/subsidy_access_policy/models.py +++ b/enterprise_access/apps/subsidy_access_policy/models.py @@ -30,7 +30,6 @@ REASON_LEARNER_MAX_SPEND_REACHED, REASON_LEARNER_NOT_ASSIGNED_CONTENT, REASON_LEARNER_NOT_IN_ENTERPRISE, - REASON_LEARNER_NOT_IN_ENTERPRISE_GROUP, REASON_NOT_ENOUGH_VALUE_IN_SUBSIDY, REASON_POLICY_EXPIRED, REASON_POLICY_SPEND_LIMIT_REACHED, @@ -592,29 +591,10 @@ def can_redeem(self, lms_user_id, content_key, skip_customer_user_check=False): # learner not associated to enterprise if not skip_customer_user_check: - learner_record = self.lms_api_client.get_enterprise_user(self.enterprise_customer_uuid, lms_user_id) - if not learner_record: + if self.lms_api_client.get_enterprise_user(self.enterprise_customer_uuid, lms_user_id) is None: self._log_redeemability(False, REASON_LEARNER_NOT_IN_ENTERPRISE, lms_user_id, content_key) return (False, REASON_LEARNER_NOT_IN_ENTERPRISE, []) - associated_group_uuids = set( # pylint: disable=consider-using-set-comprehension - [str(group["uuid"]) for group in learner_record['user']['enterprise_group']] - ) - - policy_groups = PolicyGroupAssociation.objects.filter(subsidy_access_policy=self).all() - policy_groups_uuids = set( # pylint: disable=consider-using-set-comprehension - [str(group.enterprise_group_uuid) for group in policy_groups] - ) - - # if there are groups associated with the policy, if not we want to be backwards compatible with - # policies which haven't implemented groups yet - if len(policy_groups) > 0: - uuid_overlap = associated_group_uuids.intersection(policy_groups_uuids) - # if any of the user's associated groups are connected to the policy - if len(uuid_overlap) == 0: - self._log_redeemability(False, REASON_LEARNER_NOT_IN_ENTERPRISE_GROUP, lms_user_id, content_key) - return (False, REASON_LEARNER_NOT_IN_ENTERPRISE_GROUP, []) - # no content key in catalog if not self.catalog_contains_content_key(content_key): self._log_redeemability(False, REASON_CONTENT_NOT_IN_CATALOG, lms_user_id, content_key) diff --git a/enterprise_access/apps/subsidy_access_policy/tests/test_models.py b/enterprise_access/apps/subsidy_access_policy/tests/test_models.py index 6e785957..719b7ee9 100644 --- a/enterprise_access/apps/subsidy_access_policy/tests/test_models.py +++ b/enterprise_access/apps/subsidy_access_policy/tests/test_models.py @@ -28,7 +28,6 @@ REASON_LEARNER_MAX_SPEND_REACHED, REASON_LEARNER_NOT_ASSIGNED_CONTENT, REASON_LEARNER_NOT_IN_ENTERPRISE, - REASON_LEARNER_NOT_IN_ENTERPRISE_GROUP, REASON_NOT_ENOUGH_VALUE_IN_SUBSIDY, REASON_POLICY_EXPIRED, REASON_POLICY_SPEND_LIMIT_REACHED, @@ -50,7 +49,7 @@ PolicyGroupAssociationFactory ) from enterprise_access.cache_utils import request_cache -from test_utils import TEST_ENTERPRISE_GROUP_UUID, TEST_USER_RECORD, TEST_USER_RECORD_NO_GROUPS +from test_utils import TEST_USER_RECORD from ..constants import AccessMethods from ..exceptions import PriceValidationError @@ -205,9 +204,9 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): @ddt.data( { - # Happy path: content in catalog, learner in enterprise, learner in group, - # subsidy has value, existing transactions for learner and policy below - # the policy limits. Expected can_redeem result: True + # Happy path: content in catalog, learner in enterprise, subsidy has value, + # existing transactions for learner and policy below the policy limits. + # Expected can_redeem result: True 'policy_active_type': 'active', 'catalog_contains_content': True, 'get_enterprise_user': TEST_USER_RECORD, @@ -242,19 +241,6 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args): 'expect_content_metadata_fetch': False, 'expect_transaction_fetch': False, }, - { - # Learner is not in the enterprise group, every other check would succeed. - # Expected can_redeem result: False - 'policy_active_type': 'active', - 'catalog_contains_content': True, - 'get_enterprise_user': TEST_USER_RECORD_NO_GROUPS, - 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, - 'transactions_for_learner': {'transactions': [], 'aggregates': {}}, - 'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}}, - 'expected_policy_can_redeem': (False, REASON_LEARNER_NOT_IN_ENTERPRISE_GROUP, []), - 'expect_content_metadata_fetch': False, - 'expect_transaction_fetch': False, - }, { # The subsidy is not redeemable, every other check would succeed. # Expected can_redeem result: False @@ -375,11 +361,6 @@ def test_learner_enrollment_cap_policy_can_redeem( elif policy_active_type == 'non_redeemable': policy_record = self.active_non_redeemable_per_learner_enroll_policy - PolicyGroupAssociationFactory( - enterprise_group_uuid=TEST_ENTERPRISE_GROUP_UUID, - subsidy_access_policy=policy_record - ) - can_redeem_result = policy_record.can_redeem(self.lms_user_id, self.course_id) self.assertEqual(can_redeem_result, expected_policy_can_redeem, []) @@ -438,19 +419,6 @@ def test_learner_enrollment_cap_policy_can_redeem( 'expect_content_metadata_fetch': False, 'expect_transaction_fetch': False, }, - { - # Learner is not in the enterprise group, every other check would succeed. - # Expected can_redeem result: False - 'policy_is_active': True, - 'catalog_contains_content': True, - 'get_enterprise_user': TEST_USER_RECORD_NO_GROUPS, - 'subsidy_is_redeemable': {'can_redeem': True, 'active': True}, - 'transactions_for_learner': {'transactions': [], 'aggregates': {}}, - 'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}}, - 'expected_policy_can_redeem': (False, REASON_LEARNER_NOT_IN_ENTERPRISE_GROUP, []), - 'expect_content_metadata_fetch': False, - 'expect_transaction_fetch': False, - }, { # The subsidy is not redeemable, every other check would succeed. # Expected can_redeem result: False @@ -556,11 +524,6 @@ def test_learner_spend_cap_policy_can_redeem( if policy_is_active: policy_record = self.per_learner_spend_policy - PolicyGroupAssociationFactory( - enterprise_group_uuid=TEST_ENTERPRISE_GROUP_UUID, - subsidy_access_policy=policy_record - ) - can_redeem_result = policy_record.can_redeem(self.lms_user_id, self.course_id) self.assertEqual(can_redeem_result, expected_policy_can_redeem)