Merge pull request #411 from openedx/knguyen2/ent-8416

feat: update enterprise_contains_learner return value

docs/decisions/0018-access-policy-grouping.rst

@@ -36,11 +36,11 @@ create a new ``EnterpriseGroup`` record. On successful response, the enterprise-
 UUID of the newly created group to the new table ``PolicyGroupAssociation`` with the associated policy's UUID.
 
 ``SubsidyAccessPolicy``'s `can_redeem()` method already makes a request to edx-platform for 
-`enterprise_contains_learner()` in which `lms_user_id` and `enterprise_customer_uuid` are provided to confirm
+`get_enterprise_user()` in which `lms_user_id` and `enterprise_customer_uuid` are provided to confirm
 a learner's membership with the associated organization. Now, instead returning `True` or `False` as a signature, the
-`enterprise_contains_learner()` method will return the learner's serialized EnterpriseCustomerUsers record from the
+`get_enterprise_user()` method will return the learner's serialized EnterpriseCustomerUsers record from the
 `/enterprise-learner/` API or `None` if the user is not a part of the enterprise. This will retain any truthy based
-logic dependent on the old functionality of `enterprise_contains_learner()` but will surface more information usable by
+logic dependent on the old functionality of `get_enterprise_user()` but will surface more information usable by
 new consumers, namely `can_redeem()`.
 
 Consequences

enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py

@@ -44,7 +44,7 @@
     PerLearnerSpendCapLearnerCreditAccessPolicyFactory
 )
 from enterprise_access.apps.subsidy_access_policy.utils import create_idempotency_key_for_transaction
-from test_utils import APITestWithMocks
+from test_utils import TEST_USER_RECORD, APITestWithMocks
 
 SUBSIDY_ACCESS_POLICY_LIST_ENDPOINT = reverse('api:v1:subsidy-access-policies-list')
 
@@ -1059,7 +1059,7 @@ def setup_mocks(self):
         lms_client_patcher = mock.patch('enterprise_access.apps.subsidy_access_policy.models.LmsApiClient')
         lms_client = lms_client_patcher.start()
         self.lms_client_instance = lms_client.return_value
-        self.lms_client_instance.enterprise_contains_learner.return_value = True
+        self.lms_client_instance.get_enterprise_user.return_value = TEST_USER_RECORD
 
         self.addCleanup(lms_client_patcher.stop)
         self.addCleanup(subsidy_client_patcher.stop)
@@ -1249,37 +1249,37 @@ def test_redeem_policy_redemption_idempotency_key_versions(
         {
             'is_subsidy_active': True,
             'has_subsidy_balance_remaining': True,
-            'is_learned_linked': True,
+            'get_enterprise_user': TEST_USER_RECORD,
             'has_learner_exceed_spend_cap': False,
         },
         {
             'is_subsidy_active': True,
             'has_subsidy_balance_remaining': True,
-            'is_learned_linked': False,
+            'get_enterprise_user': None,
             'has_learner_exceed_spend_cap': False,
         },
         {
             'is_subsidy_active': True,
             'has_subsidy_balance_remaining': True,
-            'is_learned_linked': True,
+            'get_enterprise_user': TEST_USER_RECORD,
             'has_learner_exceed_spend_cap': True,
         },
         {
             'is_subsidy_active': False,
             'has_subsidy_balance_remaining': True,
-            'is_learned_linked': True,
+            'get_enterprise_user': TEST_USER_RECORD,
             'has_learner_exceed_spend_cap': False,
         },
         {
             'is_subsidy_active': True,
             'has_subsidy_balance_remaining': False,
-            'is_learned_linked': True,
+            'get_enterprise_user': TEST_USER_RECORD,
             'has_learner_exceed_spend_cap': False,
         },
         {
             'is_subsidy_active': False,
             'has_subsidy_balance_remaining': False,
-            'is_learned_linked': True,
+            'get_enterprise_user': TEST_USER_RECORD,
             'has_learner_exceed_spend_cap': False,
         },
     )
@@ -1290,7 +1290,7 @@ def test_credits_available_endpoint(
         mock_transactions_cache_for_learner,
         is_subsidy_active,
         has_subsidy_balance_remaining,
-        is_learned_linked,
+        get_enterprise_user,
         has_learner_exceed_spend_cap,
     ):
         """
@@ -1317,10 +1317,12 @@ def test_credits_available_endpoint(
         enroll_cap_policy = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory(
             enterprise_customer_uuid=self.enterprise_uuid,
             per_learner_enrollment_limit=5,
+            spend_limit=10000,
         )
         spend_cap_policy = PerLearnerSpendCapLearnerCreditAccessPolicyFactory(
             enterprise_customer_uuid=self.enterprise_uuid,
             per_learner_spend_limit=(5 if has_learner_exceed_spend_cap else 1000),
+            spend_limit=10000,
         )
 
         mock_transaction_record = {
@@ -1369,7 +1371,7 @@ def test_credits_available_endpoint(
             'current_balance': '5000' if has_subsidy_balance_remaining else '0',
             'is_active': is_subsidy_active,
         }
-        self.lms_client_instance.enterprise_contains_learner.return_value = is_learned_linked
+        self.lms_client_instance.get_enterprise_user.return_value = get_enterprise_user
 
         query_params = {
             'enterprise_customer_uuid': str(self.enterprise_uuid),
@@ -1379,7 +1381,7 @@ def test_credits_available_endpoint(
 
         response_json = response.json()
 
-        if is_subsidy_active and has_subsidy_balance_remaining and is_learned_linked:
+        if is_subsidy_active and has_subsidy_balance_remaining and get_enterprise_user is not None:
             # the above generic checks passed, now verify the specific policy-type specific checks.
             if has_learner_exceed_spend_cap:
                 # The spend cap policy should not be returned as the learner has exceeded the spend cap.
@@ -1459,7 +1461,7 @@ def test_credits_available_endpoint_with_content_assignments(
             'current_balance': '5000',
             'is_active': True,
         }
-        self.lms_client_instance.enterprise_contains_learner.return_value = True
+        self.lms_client_instance.get_enterprise_user.return_value = TEST_USER_RECORD
         query_params = {
             'enterprise_customer_uuid': str(self.enterprise_uuid),
             'lms_user_id': 1234,
@@ -1590,7 +1592,7 @@ def setup_mocks(self):
         lms_client_patcher = mock.patch('enterprise_access.apps.subsidy_access_policy.models.LmsApiClient')
         lms_client = lms_client_patcher.start()
         lms_client_instance = lms_client.return_value
-        lms_client_instance.enterprise_contains_learner.return_value = True
+        lms_client_instance.get_enterprise_user.return_value = TEST_USER_RECORD
 
         self.addCleanup(lms_client_patcher.stop)
         self.addCleanup(subsidy_client_patcher.stop)

enterprise_access/apps/api_client/lms_client.py

@@ -113,7 +113,7 @@ def unlink_users_from_enterprise(self, enterprise_customer_uuid, user_emails, is
             logger.exception(msg, enterprise_customer_uuid)
             raise
 
-    def enterprise_contains_learner(self, enterprise_customer_uuid, learner_id):
+    def get_enterprise_user(self, enterprise_customer_uuid, learner_id):
         """
         Verify if `learner_id` is a part of an enterprise represented by `enterprise_customer_uuid`.
 
@@ -122,10 +122,8 @@ def enterprise_contains_learner(self, enterprise_customer_uuid, learner_id):
             learner_id (int): LMS user id of a learner.
 
         Returns:
-            bool: True if learner is linked with enterprise else False
+            None or the enterprise customer user record
         """
-
-        result = False
         ec_uuid = str(enterprise_customer_uuid)
         query_params = {'enterprise_customer_uuid': ec_uuid, 'user_ids': learner_id}
 
@@ -134,16 +132,21 @@ def enterprise_contains_learner(self, enterprise_customer_uuid, learner_id):
             response = self.client.get(url, params=query_params, timeout=settings.LMS_CLIENT_TIMEOUT)
             response.raise_for_status()
             json_response = response.json()
-            results = json_response.get('results')
-            results = results and results[0]
-            if results and results['enterprise_customer']['uuid'] == ec_uuid and results['user']['id'] == learner_id:
-                result = True
+            results = json_response.get('results', [])
+            if isinstance(results, list):
+                for result in results:
+                    returned_customer = result.get('enterprise_customer', {})
+                    returned_user = result.get('user', {})
+                    if returned_customer.get('uuid') == ec_uuid and returned_user.get('id') == learner_id:
+                        return result
+            else:
+                logger.exception(f'get_enterprise_user got unexpected results: {results} from {url} ')
         except requests.exceptions.HTTPError:
             logger.exception('Failed to fetch data from LMS. URL: [%s].', url)
         except KeyError:
             logger.exception('Incorrect data received from LMS. [%s]', url)
 
-        return result
+        return None
 
     def create_pending_enterprise_users(self, enterprise_customer_uuid, user_emails):
         """

enterprise_access/apps/api_client/tests/test_lms_client.py

@@ -13,8 +13,8 @@
 
 from enterprise_access.apps.api_client.lms_client import LmsApiClient
 from enterprise_access.apps.api_client.tests.test_utils import MockResponse
+from test_utils import TEST_ENTERPRISE_UUID, TEST_USER_ID, TEST_USER_RECORD
 
-TEST_ENTERPRISE_UUID = uuid4()
 TEST_USER_EMAILS = [
     '[email protected]',
     '[email protected]',
@@ -168,32 +168,22 @@ def test_unlink_users_from_enterprise(self, mock_oauth_client):
 
     @mock.patch('requests.Response.json')
     @mock.patch('enterprise_access.apps.api_client.base_oauth.OAuthAPIClient')
-    def test_enterprise_contains_learner(self, mock_oauth_client, mock_json):
+    def test_get_enterprise_user(self, mock_oauth_client, mock_json):
         """
-        Verify enterprise_contains_learner works as expected.
+        Verify get_enterprise_user works as expected.
         """
-        mock_enterprise_uuid = str(uuid4())
-        user_id = 1234
         mock_oauth_client.return_value.get.return_value = requests.Response()
         mock_oauth_client.return_value.get.return_value.status_code = 200
 
         mock_json.return_value = {
             'results': [
-                {
-                    'enterprise_customer': {
-                        'uuid': mock_enterprise_uuid
-                    },
-                    'user': {
-                        'id': user_id
-                    }
-                }
+                TEST_USER_RECORD
             ]
         }
-
-        query_params = {'enterprise_customer_uuid': mock_enterprise_uuid, 'user_ids': user_id}
+        query_params = {'enterprise_customer_uuid': str(TEST_ENTERPRISE_UUID), 'user_ids': TEST_USER_ID}
         client = LmsApiClient()
-        enterprise_contains_learner = client.enterprise_contains_learner(mock_enterprise_uuid, user_id)
-        assert enterprise_contains_learner
+        get_enterprise_user = client.get_enterprise_user(str(TEST_ENTERPRISE_UUID), TEST_USER_ID)
+        assert get_enterprise_user == TEST_USER_RECORD
 
         mock_oauth_client.return_value.get.assert_called_with(
             'http://edx-platform.example.com/enterprise/api/v1/enterprise-learner/',

enterprise_access/apps/subsidy_access_policy/models.py

@@ -591,7 +591,7 @@ 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:
-            if not self.lms_api_client.enterprise_contains_learner(self.enterprise_customer_uuid, lms_user_id):
+            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, [])
 
@@ -687,7 +687,7 @@ def credit_available(
 
         # learner not linked to enterprise
         if not skip_customer_user_check:
-            if not self.lms_api_client.enterprise_contains_learner(self.enterprise_customer_uuid, lms_user_id):
+            if self.lms_api_client.get_enterprise_user(self.enterprise_customer_uuid, lms_user_id) is None:
                 logger.info(
                     '[credit_available] learner %s not linked to enterprise %s',
                     lms_user_id,

enterprise_access/apps/subsidy_access_policy/tests/factories.py

@@ -29,15 +29,15 @@ class SubsidyAccessPolicyFactory(factory.django.DjangoModelFactory):
     subsidy_uuid = factory.LazyFunction(uuid4)
     access_method = AccessMethods.DIRECT
     description = 'A generic description'
-    spend_limit = factory.LazyAttribute(lambda _: FAKER.pyint())
+    spend_limit = factory.LazyAttribute(lambda _: FAKER.pyint(min_value=1))
     active = True
 
 
 class PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory(SubsidyAccessPolicyFactory):
     """
     Test factory for the `PerLearnerEnrollmentCreditAccessPolicy` model.
     """
-    per_learner_enrollment_limit = factory.LazyAttribute(lambda _: FAKER.pyint())
+    per_learner_enrollment_limit = factory.LazyAttribute(lambda _: FAKER.pyint(min_value=1))
 
     class Meta:
         model = PerLearnerEnrollmentCreditAccessPolicy
@@ -47,7 +47,7 @@ class PerLearnerSpendCapLearnerCreditAccessPolicyFactory(SubsidyAccessPolicyFact
     """
     Test factory for the `PerLearnerSpendCreditAccessPolicy` model.
     """
-    per_learner_spend_limit = factory.LazyAttribute(lambda _: FAKER.pyint())
+    per_learner_spend_limit = factory.LazyAttribute(lambda _: FAKER.pyint(min_value=1))
 
     class Meta:
         model = PerLearnerSpendCreditAccessPolicy
@@ -62,7 +62,7 @@ class Meta:
         model = AssignedLearnerCreditAccessPolicy
 
     access_method = AccessMethods.ASSIGNED
-    spend_limit = factory.LazyAttribute(lambda _: FAKER.pyint())
+    spend_limit = factory.LazyAttribute(lambda _: FAKER.pyint(min_value=1))
     per_learner_spend_limit = None
     per_learner_enrollment_limit = None