diff --git a/enterprise_subsidy/apps/api/v1/serializers.py b/enterprise_subsidy/apps/api/v1/serializers.py index 383eaa5a..f8e3c1d7 100644 --- a/enterprise_subsidy/apps/api/v1/serializers.py +++ b/enterprise_subsidy/apps/api/v1/serializers.py @@ -128,7 +128,9 @@ class Meta: "state", "idempotency_key", "lms_user_id", + "lms_user_email", "content_key", + "content_title", "quantity", "unit", # Manually fetch from parent ledger via get_unit(). "fulfillment_identifier", diff --git a/enterprise_subsidy/apps/api/v1/tests/test_views.py b/enterprise_subsidy/apps/api/v1/tests/test_views.py index e79478a1..a3b47215 100644 --- a/enterprise_subsidy/apps/api/v1/tests/test_views.py +++ b/enterprise_subsidy/apps/api/v1/tests/test_views.py @@ -51,7 +51,10 @@ class APITestBase(APITestMixin): subsidy_access_policy_1_uuid = str(uuid.uuid4()) subsidy_access_policy_2_uuid = str(uuid.uuid4()) content_key_1 = "course-v1:edX+test+course.1" + content_title_1 = "edx: Test Course 1" content_key_2 = "course-v1:edX+test+course.2" + content_title_2 = "edx: Test Course 2" + lms_user_email = 'edx@example.com' def setUp(self): super().setUp() @@ -69,8 +72,10 @@ def setUp(self): quantity=-1000, ledger=self.subsidy_1.ledger, lms_user_id=STATIC_LMS_USER_ID, # This is the only transaction belonging to the requester. + lms_user_email=self.lms_user_email, subsidy_access_policy_uuid=self.subsidy_access_policy_1_uuid, content_key=self.content_key_1, + content_title=self.content_title_1, ) self.subsidy_1_transaction_2 = TransactionFactory( uuid=self.subsidy_1_transaction_2_uuid, @@ -78,8 +83,10 @@ def setUp(self): quantity=-1000, ledger=self.subsidy_1.ledger, lms_user_id=STATIC_LMS_USER_ID+1000, + lms_user_email=self.lms_user_email, subsidy_access_policy_uuid=self.subsidy_access_policy_2_uuid, content_key=self.content_key_2, + content_title=self.content_title_2, ) # Create an extra subsidy with the same enterprise_customer_uuid, but the learner does not have any transactions @@ -96,6 +103,7 @@ def setUp(self): quantity=-1000, ledger=self.subsidy_2.ledger, lms_user_id=STATIC_LMS_USER_ID+1000, + lms_user_email=self.lms_user_email, ) TransactionFactory( uuid=self.subsidy_2_transaction_2_uuid, @@ -103,6 +111,7 @@ def setUp(self): quantity=-1000, ledger=self.subsidy_2.ledger, lms_user_id=STATIC_LMS_USER_ID+1000, + lms_user_email=self.lms_user_email, ) # Create third subsidy with a different enterprise_customer_uuid. Neither test learner nor the test admin @@ -119,6 +128,7 @@ def setUp(self): quantity=-1000, ledger=self.subsidy_3.ledger, lms_user_id=STATIC_LMS_USER_ID+1000, + lms_user_email=self.lms_user_email, ) TransactionFactory( uuid=self.subsidy_3_transaction_2_uuid, @@ -126,6 +136,7 @@ def setUp(self): quantity=-1000, ledger=self.subsidy_3.ledger, lms_user_id=STATIC_LMS_USER_ID+1000, + lms_user_email=self.lms_user_email, ) self.all_initial_transactions = set([ @@ -156,7 +167,7 @@ def test_get_one_subsidy(self): expected_result = { "uuid": str(self.subsidy_1.uuid), "title": self.subsidy_1.title, - "enterprise_customer_uuid": self.subsidy_1.enterprise_customer_uuid, + "enterprise_customer_uuid": str(self.subsidy_1.enterprise_customer_uuid), "active_datetime": self.subsidy_1.active_datetime.strftime(SERIALIZED_DATE_PATTERN), "expiration_datetime": self.subsidy_1.expiration_datetime.strftime(SERIALIZED_DATE_PATTERN), "unit": self.subsidy_1.unit, @@ -254,14 +265,18 @@ def test_can_redeem_bad_request(self, query_params): self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) @mock.patch('enterprise_subsidy.apps.api.v1.views.subsidy.can_redeem') + @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client") @ddt.data(False, True) - def test_can_redeem_happy_path(self, has_existing_transaction, mock_can_redeem): + def test_can_redeem_happy_path(self, has_existing_transaction, mock_lms_user_client, mock_can_redeem): """ Tests that the result of ``api.can_redeem()`` is returned as the response payload for a POST to the can_redeem action, including any relevant existing transaction. """ self.set_up_admin(enterprise_uuids=[self.subsidy_1.enterprise_customer_uuid]) + mock_lms_user_client.return_value.best_effort_user_data.return_value = { + 'email': self.lms_user_email, + } expected_redeemable = True expected_active = True expected_price = 350 @@ -292,8 +307,10 @@ def test_can_redeem_happy_path(self, has_existing_transaction, mock_can_redeem): 'state': TransactionStateChoices.COMMITTED, 'quantity': -1000, 'lms_user_id': STATIC_LMS_USER_ID, + 'lms_user_email': self.lms_user_email, 'subsidy_access_policy_uuid': str(self.subsidy_access_policy_1_uuid), 'content_key': self.content_key_1, + 'content_title': self.content_title_1, 'external_reference': [], 'transaction_status_api_url': f"{self.transaction_status_api_url}/{self.subsidy_1_transaction_1_uuid}/", 'courseware_url': f"http://localhost:2000/course/{self.content_key_1}/home", @@ -959,10 +976,18 @@ def test_retrieve_invalid_uuid(self): @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.enterprise_client") @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.price_for_content") @mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary") - def test_create(self, mock_get_content_summary, mock_price_for_content, mock_enterprise_client): + @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client") + def test_create( + self, + mock_lms_user_client, + mock_get_content_summary, + mock_price_for_content, + mock_enterprise_client, + ): """ Test create Transaction, happy case. """ + mock_lms_user_client.return_value.best_effort_user_data.return_value = {'email': 'edx@example.com'} url = reverse("api:v1:transaction-list") test_enroll_enterprise_fulfillment_uuid = "test-enroll-reference-id" mock_enterprise_client.enroll.return_value = test_enroll_enterprise_fulfillment_uuid @@ -970,6 +995,7 @@ def test_create(self, mock_get_content_summary, mock_price_for_content, mock_ent mock_get_content_summary.return_value = { 'content_uuid': 'course-v1:edX-test-course', 'content_key': 'course-v1:edX-test-course', + 'content_title': 'edX: Test Course', 'source': 'edX', 'mode': 'verified', 'content_price': 10000, @@ -994,7 +1020,9 @@ def test_create(self, mock_get_content_summary, mock_price_for_content, mock_ent f"{self.transaction_status_api_url}/{create_response_data['uuid']}/" ) assert create_response_data["content_key"] == post_data["content_key"] + assert create_response_data["content_title"] == 'edX: Test Course' assert create_response_data["lms_user_id"] == post_data["lms_user_id"] + assert create_response_data["lms_user_email"] == 'edx@example.com' assert create_response_data["subsidy_access_policy_uuid"] == post_data["subsidy_access_policy_uuid"] assert create_response_data["courseware_url"] == f"http://localhost:2000/course/{post_data['content_key']}/home" self.assertDictEqual(create_response_data["metadata"], {}) @@ -1011,6 +1039,7 @@ def test_create(self, mock_get_content_summary, mock_price_for_content, mock_ent retrieve_response_data = retrieve_response.json() assert retrieve_response_data["uuid"] == create_response_data["uuid"] assert retrieve_response_data["idempotency_key"] == create_response_data["idempotency_key"] + # assert retrieve_response_data["content_title"] == 'edX: Test Course' # Uncomment after Segment events are setup: # @@ -1030,10 +1059,18 @@ def test_create(self, mock_get_content_summary, mock_price_for_content, mock_ent @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.enterprise_client") @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.price_for_content") @mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary") - def test_create_with_metadata(self, mock_get_content_summary, mock_price_for_content, mock_enterprise_client): + @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client") + def test_create_with_metadata( + self, + mock_lms_user_client, + mock_get_content_summary, + mock_price_for_content, + mock_enterprise_client + ): """ Test create Transaction, happy case. """ + mock_lms_user_client.return_value.best_effort_user_data.return_value = {'email': 'edx@example.com'} url = reverse("api:v1:transaction-list") test_enroll_enterprise_fulfillment_uuid = "test-enroll-reference-id" mock_enterprise_client.enroll.return_value = test_enroll_enterprise_fulfillment_uuid @@ -1085,10 +1122,27 @@ def test_create_with_metadata(self, mock_get_content_summary, mock_price_for_con @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.enterprise_client") @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.price_for_content") - def test_create_ledger_locked(self, mock_price_for_content, mock_enterprise_client): + @mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary") + @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client") + def test_create_ledger_locked( + self, + mock_lms_user_client, + mock_get_content_summary, + mock_price_for_content, + mock_enterprise_client, + ): """ Test create Transaction, 429 response due to the ledger being locked. """ + mock_get_content_summary.return_value = { + 'content_uuid': 'course-v1:edX-test-course', + 'content_key': 'course-v1:edX-test-course', + 'source': 'edX', + 'mode': 'verified', + 'content_price': 10000, + 'geag_variant_id': None, + } + mock_lms_user_client.return_value.best_effort_user_data.return_value = {'email': 'edx@example.com'} url = reverse("api:v1:transaction-list") test_enroll_enterprise_fulfillment_uuid = "test-enroll-reference-id" mock_enterprise_client.enroll.return_value = test_enroll_enterprise_fulfillment_uuid @@ -1171,29 +1225,40 @@ def test_create_content_not_in_catalog(self, mock_content_metadata_api): @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.enterprise_client") @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.content_metadata_api") @mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary") + @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client") def test_create_external_enroll_failed( self, + mock_lms_user_client, mock_get_content_summary, - mock_content_metadata_api, - mock_enterprise_client + mock_subsidy_content_metadata_api, + mock_enterprise_client, ): """ Test create Transaction, 5xx response due to the external enrollment failing. Check that a transaction is created, then rolled back to "failed" state. """ mock_get_content_summary.return_value = { - 'content_uuid': 'course-v1:edX+test+course.enroll.failed', - 'content_key': 'course-v1:edX+test+course.enroll.failed', + 'content_uuid': 'course-v1:edX-test-course', + 'content_key': 'course-v1:edX-test-course', 'source': 'edX', 'mode': 'verified', - 'content_price': 10000, + 'content_price': 100, + 'geag_variant_id': None, + } + mock_subsidy_content_metadata_api().get_content_summary.return_value = { + 'content_uuid': 'course-v1:edX-test-course', + 'content_key': 'course-v1:edX-test-course', + 'source': 'edX', + 'mode': 'verified', + 'content_price': 100, 'geag_variant_id': None, } + mock_subsidy_content_metadata_api().get_course_price.return_value = 100 + mock_lms_user_client.return_value.best_effort_user_data.return_value = {'email': 'edx@example.com'} # Create privileged staff user that should be able to create Transactions. self.set_up_operator() url = reverse("api:v1:transaction-list") mock_enterprise_client.enroll.side_effect = HTTPError() - mock_content_metadata_api().get_course_price.return_value = 100 test_content_key = "course-v1:edX+test+course.enroll.failed" test_lms_user_id = 1234 post_data = { @@ -1430,7 +1495,11 @@ def test_successful_get( self.set_up_admin(enterprise_uuids=[str(customer_uuid)]) mock_oauth_client.return_value.get.return_value = MockResponse(mock_metadata, 200) url = reverse('api:v1:content-metadata', kwargs={'content_identifier': expected_content_key}) + + # Make a first call that should not run into a cached response + # from the cached EnterpriseCustomerViewSet.get view. response = self.client.get(url + f'?enterprise_customer_uuid={str(customer_uuid)}') + assert response.status_code == 200 assert response.json() == { 'content_title': expected_content_title, @@ -1444,10 +1513,14 @@ def test_successful_get( 'geag_variant_id': expected_geag_variant_id, } - # Everything after this line is testing the view's cache - # If this mock response is ever hit, the test will fail, caching prevents it. + # Now make a second call to validate that the view-level cache is utilized. + # This means we won't make a second request via the enterprise catalog API client. + # Adding an exception side effect proves that we don't actually make + # the call with the client. mock_oauth_client.return_value.get.side_effect = Exception("Does not reach this") + response = self.client.get(url + f'?enterprise_customer_uuid={str(customer_uuid)}') + assert response.status_code == 200 assert response.json() == { 'content_title': expected_content_title, @@ -1460,6 +1533,15 @@ def test_successful_get( 'mode': expected_mode, 'geag_variant_id': expected_geag_variant_id, } + # Validate that, in the first, non-cached request, we call + # the enterprise catalog endpoint via the client, and that + # a `skip_customer_fetch` parameter is included in the request. + mock_oauth_client.return_value.get.assert_called_once_with( + f'api/v1/enterprise-customer/{customer_uuid}/content-metadata/{expected_content_key}/', + params={ + 'skip_customer_fetch': True, + }, + ) def test_failure_no_permission(self): self.set_up_admin(enterprise_uuids=[str(uuid.uuid4())]) @@ -1494,6 +1576,14 @@ def test_failure_exception_while_gather_metadata(self, catalog_status_code, expe url=self.mock_http_error_url ) url = reverse('api:v1:content-metadata', kwargs={'content_identifier': 'content_key'}) + response = self.client.get(url + f'?enterprise_customer_uuid={str(customer_uuid)}') + assert response.status_code == catalog_status_code assert response.json() == expected_response + mock_oauth_client.return_value.get.assert_called_once_with( + f'api/v1/enterprise-customer/{customer_uuid}/content-metadata/content_key/', + params={ + 'skip_customer_fetch': True, + }, + ) diff --git a/enterprise_subsidy/apps/api/v1/views/content_metadata.py b/enterprise_subsidy/apps/api/v1/views/content_metadata.py index 90a1c81f..76d80dfd 100644 --- a/enterprise_subsidy/apps/api/v1/views/content_metadata.py +++ b/enterprise_subsidy/apps/api/v1/views/content_metadata.py @@ -106,7 +106,8 @@ def get(self, request, content_identifier, enterprise_customer_uuid): try: content_summary = self.content_metadata_api().get_content_summary( enterprise_customer_uuid[0], - content_identifier + content_identifier, + skip_customer_fetch=True, ) if not content_summary.get('content_price'): logger.warning(f"Could not find course price in metadata for {content_identifier}") diff --git a/enterprise_subsidy/apps/api/v1/views/transaction.py b/enterprise_subsidy/apps/api/v1/views/transaction.py index 2d2ee2ad..ecfb217c 100644 --- a/enterprise_subsidy/apps/api/v1/views/transaction.py +++ b/enterprise_subsidy/apps/api/v1/views/transaction.py @@ -217,6 +217,10 @@ def base_queryset(self): for learner_only_context in learner_only_contexts: # For each context (enterprise_customer_uuid) that the requester only has learner access to, filter # transactions related to that context to only include their own transactions. + # AED 2023-10-31: locally, pylint complains of the binary operation. + # In github actions, pylint complains of a useless-suppression. + # Suppressing both and letting the code gods sort it out. + # pylint: disable=unsupported-binary-operation,useless-suppression if request_jwt.get('user_id'): queryset = queryset.filter( ( diff --git a/enterprise_subsidy/apps/api/v2/tests/test_transaction_views.py b/enterprise_subsidy/apps/api/v2/tests/test_transaction_views.py index ddad4ff5..c13cfe38 100644 --- a/enterprise_subsidy/apps/api/v2/tests/test_transaction_views.py +++ b/enterprise_subsidy/apps/api/v2/tests/test_transaction_views.py @@ -33,6 +33,7 @@ class APITestBase(APITestMixin): Contains boilerplate to create a couple of subsidies with related ledgers and starting transactions. """ + lms_user_email = 'edx@example.com' enterprise_1_uuid = STATIC_ENTERPRISE_UUID enterprise_2_uuid = str(uuid.uuid4()) @@ -56,6 +57,8 @@ class APITestBase(APITestMixin): content_key_1 = "course-v1:edX+test+course.1" content_key_2 = "course-v1:edX+test+course.2" + content_title_1 = "edX: Test Course 1" + content_title_2 = "edx: Test Course 2" @classmethod def setUpClass(cls): @@ -108,8 +111,10 @@ def _setup_transactions(cls): quantity=-1000, ledger=cls.subsidy_1.ledger, lms_user_id=STATIC_LMS_USER_ID+1000, + lms_user_email=cls.lms_user_email, subsidy_access_policy_uuid=cls.subsidy_access_policy_2_uuid, content_key=cls.content_key_2, + content_title=cls.content_title_2, ) # Also create a reversed transaction, and also include metadata on both the transaction and reversal. cls.subsidy_1_transaction_3 = TransactionFactory( @@ -118,8 +123,10 @@ def _setup_transactions(cls): quantity=-1000, ledger=cls.subsidy_1.ledger, lms_user_id=STATIC_LMS_USER_ID, + lms_user_email=cls.lms_user_email, subsidy_access_policy_uuid=cls.subsidy_access_policy_2_uuid, content_key=cls.content_key_2, + content_title=cls.content_title_2, metadata={"bin": "baz"}, ) cls.subsidy_1_transaction_3_reversal = ReversalFactory( @@ -256,8 +263,10 @@ def setUpClass(cls): quantity=0, ledger=cls.subsidy_1.ledger, lms_user_id=STATIC_LMS_USER_ID, # This is the only transaction belonging to the requester. + lms_user_email=cls.lms_user_email, subsidy_access_policy_uuid=cls.subsidy_access_policy_1_uuid, content_key=cls.content_key_1, + content_title=cls.content_title_1, ) def test_list_transactions_metadata_format(self): @@ -705,8 +714,10 @@ def test_operator_creation_happy_path_transaction_exists(self, mock_price_for_co @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.enterprise_client") @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.price_for_content") @mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary") + @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client") def test_operator_creation_happy_path_201( self, + mock_lms_user_client, mock_get_content_summary, mock_price_for_content, mock_enterprise_client @@ -717,11 +728,15 @@ def test_operator_creation_happy_path_201( """ self.set_up_operator() + mock_lms_user_client.return_value.best_effort_user_data.return_value = { + 'email': self.lms_user_email, + } mock_enterprise_client.enroll.return_value = 'my-fulfillment-id' mock_price_for_content.return_value = 1000 mock_get_content_summary.return_value = { 'content_uuid': self.content_key_1, 'content_key': self.content_key_1, + 'content_title': self.content_title_1, 'source': 'edX', 'mode': 'verified', 'content_price': 10000, diff --git a/enterprise_subsidy/apps/api_client/enterprise_catalog.py b/enterprise_subsidy/apps/api_client/enterprise_catalog.py index 721577cd..c8846b49 100644 --- a/enterprise_subsidy/apps/api_client/enterprise_catalog.py +++ b/enterprise_subsidy/apps/api_client/enterprise_catalog.py @@ -31,7 +31,9 @@ def content_metadata_url(self, enterprise_customer_uuid, content_identifier): f'content-metadata/{content_identifier}/' ) - def get_content_metadata_for_customer(self, enterprise_customer_uuid, content_identifier): + def get_content_metadata_for_customer( + self, enterprise_customer_uuid, content_identifier, skip_customer_fetch=False, **kwargs + ): """ Returns Enterprise Customer related data for a specified piece on content. @@ -40,6 +42,12 @@ def get_content_metadata_for_customer(self, enterprise_customer_uuid, content_id content_identifier (str): **Either** the content UUID or content key identifier for a content record. Note: the content needs to be owned by a catalog associated with the provided customer else this method will throw an HTTPError. + skip_customer_fetch (bool): Forces enterprise-catalog to skip a sub-call to an edx-enterprise + API endpoint running in the edx-platform runtime. This sub-call helps the catalog service + understand the last time a catalog's customer record was modified, and also helps + to construct course and course run enrollment URLs that are usually not needed + in the context of enterprise-subsidy or callers of the EnterpriseCustomerViewSet. + Defaults to False. Returns: response (dict): JSON response object associated with a content metadata record Raises: @@ -48,8 +56,11 @@ def get_content_metadata_for_customer(self, enterprise_customer_uuid, content_id does not exist, or is not present in a catalog associated with the customer. """ content_metadata_url = self.content_metadata_url(enterprise_customer_uuid, content_identifier) + query_params = {} + if skip_customer_fetch: + query_params['skip_customer_fetch'] = skip_customer_fetch try: - response = self.client.get(content_metadata_url) + response = self.client.get(content_metadata_url, params=query_params) response.raise_for_status() return response.json() except requests.exceptions.HTTPError as exc: diff --git a/enterprise_subsidy/apps/content_metadata/api.py b/enterprise_subsidy/apps/content_metadata/api.py index df8c603b..bd80b4bc 100644 --- a/enterprise_subsidy/apps/content_metadata/api.py +++ b/enterprise_subsidy/apps/content_metadata/api.py @@ -136,13 +136,14 @@ def get_course_run(self, content_identifier, content_data): return course_run return {} - def get_content_summary(self, enterprise_customer_uuid, content_identifier): + def get_content_summary(self, enterprise_customer_uuid, content_identifier, **kwargs): """ Returns a summary dict some content metadata, makes the client call """ course_details = self.get_content_metadata( enterprise_customer_uuid, - content_identifier + content_identifier, + **kwargs, ) return self.summary_data_for_content(content_identifier, course_details) @@ -201,7 +202,7 @@ def get_geag_variant_id(self, enterprise_customer_uuid, content_identifier): return self.get_content_summary(enterprise_customer_uuid, content_identifier).get('geag_variant_id') @staticmethod - def get_content_metadata(enterprise_customer_uuid, content_identifier): + def get_content_metadata(enterprise_customer_uuid, content_identifier, **kwargs): """ Fetches details about the given content from a tiered (request + django) cache; or it fetches from the enterprise-catalog API if not present in the cache, @@ -214,7 +215,8 @@ def get_content_metadata(enterprise_customer_uuid, content_identifier): course_details = EnterpriseCatalogApiClient().get_content_metadata_for_customer( enterprise_customer_uuid, - content_identifier + content_identifier, + **kwargs, ) if course_details: TieredCache.set_all_tiers( diff --git a/enterprise_subsidy/apps/subsidy/migrations/0019_alter_historicalsubsidy_options.py b/enterprise_subsidy/apps/subsidy/migrations/0019_alter_historicalsubsidy_options.py new file mode 100644 index 00000000..fb48272b --- /dev/null +++ b/enterprise_subsidy/apps/subsidy/migrations/0019_alter_historicalsubsidy_options.py @@ -0,0 +1,17 @@ +# Generated by Django 3.2.19 on 2023-10-31 13:02 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('subsidy', '0018_alter_historicalsubsidy_options'), + ] + + operations = [ + migrations.AlterModelOptions( + name='historicalsubsidy', + options={'get_latest_by': 'history_date', 'ordering': ('-history_date', '-history_id'), 'verbose_name': 'historical subsidy'}, + ), + ] diff --git a/enterprise_subsidy/apps/subsidy/migrations/0020_remove_subsidy_unique_reference_id_non_internal.py b/enterprise_subsidy/apps/subsidy/migrations/0020_remove_subsidy_unique_reference_id_non_internal.py new file mode 100644 index 00000000..fe4d43c0 --- /dev/null +++ b/enterprise_subsidy/apps/subsidy/migrations/0020_remove_subsidy_unique_reference_id_non_internal.py @@ -0,0 +1,17 @@ +# Generated by Django 3.2.19 on 2023-10-31 13:04 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('subsidy', '0019_alter_historicalsubsidy_options'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='subsidy', + name='unique_reference_id_non_internal', + ), + ] diff --git a/enterprise_subsidy/apps/subsidy/models.py b/enterprise_subsidy/apps/subsidy/models.py index 709b021b..2ae374a8 100644 --- a/enterprise_subsidy/apps/subsidy/models.py +++ b/enterprise_subsidy/apps/subsidy/models.py @@ -13,8 +13,8 @@ from unittest import mock from uuid import uuid4 +from django.core.exceptions import ValidationError from django.db import models -from django.db.models import Q from django.utils.functional import cached_property from edx_rbac.models import UserRole, UserRoleAssignment from edx_rbac.utils import ALL_ACCESS_CONTEXT @@ -105,13 +105,6 @@ class Meta: """ Metaclass for Subsidy. """ - constraints = [ - models.UniqueConstraint( - condition=Q(internal_only=False), # Allow more flexibility for internal/test subsidies. - fields=["reference_id", "reference_type"], - name="unique_reference_id_non_internal", - ) - ] ordering = ['-created'] # Please reserve the "subsidy_type" field name for the future when we use it to distinguish between @@ -219,6 +212,30 @@ class Meta: objects = ActiveSubsidyManager() all_objects = models.Manager() + def clean(self): + """ + Ensures that non-internal-only subsidies are unique + on (reference_id, reference_type). This is necessary + because MySQL does not support conditional unique constraints. + """ + if not self.internal_only: + other_record = Subsidy.objects.filter( + reference_id=self.reference_id, + reference_type=self.reference_type, + ).exclude(uuid=self.uuid).first() + if other_record: + raise ValidationError( + f'Subsidy {other_record.uuid} already exists with the same ' + f'reference_id {self.reference_id} and reference_type {self.reference_type}' + ) + + def save(self, *args, **kwargs): + """ + Overrides default save() method to run full_clean. + """ + self.full_clean() + super().save(*args, **kwargs) + @property def is_active(self): """ @@ -268,7 +285,7 @@ def email_for_learner(self, lms_user_id): if isinstance(user_data, dict): return user_data.get('email') return None - + def title_for_content(self, content_key): """ Best effort return the title of the given content. @@ -278,15 +295,18 @@ def title_for_content(self, content_key): """ content_title = None try: - content_summary = self.content_metadata_api().get_content_summary(self.enterprise_customer_uuid, content_key) + content_summary = self.content_metadata_api().get_content_summary( + self.enterprise_customer_uuid, + content_key + ) if content_summary: - content_title = content_summary.get('title') + content_title = content_summary.get('content_title') except HTTPError as exc: if exc.response.status_code == status.HTTP_404_NOT_FOUND: raise ContentNotFoundForCustomerException( 'The given content_key is not in any catalog for this customer.' ) from exc - return content_title + return content_title def price_for_content(self, content_key): """ diff --git a/enterprise_subsidy/apps/subsidy/tests/test_api.py b/enterprise_subsidy/apps/subsidy/tests/test_api.py index 6ff06ac7..f65fe183 100644 --- a/enterprise_subsidy/apps/subsidy/tests/test_api.py +++ b/enterprise_subsidy/apps/subsidy/tests/test_api.py @@ -1,11 +1,13 @@ """ Tests for functions defined in the ``api.py`` module. """ +from datetime import timedelta from unittest import mock from uuid import uuid4 import pytest from django.test import TestCase +from django.utils import timezone from openedx_ledger.models import Reversal, TransactionStateChoices, UnitChoices from openedx_ledger.test_utils.factories import TransactionFactory @@ -25,8 +27,8 @@ def learner_credit_fixture(): default_enterprise_customer_uuid=uuid4(), default_unit=UnitChoices.USD_CENTS, default_starting_balance=1000000, - default_active_datetime=None, - default_expiration_datetime=None, + default_active_datetime=timezone.now() - timedelta(days=365), + default_expiration_datetime=timezone.now() + timedelta(days=365), ) return subsidy @@ -72,8 +74,8 @@ def test_create_internal_only_subsidy_record(learner_credit_fixture): # pylint: default_enterprise_customer_uuid=other_customer_uuid, default_unit=UnitChoices.USD_CENTS, default_starting_balance=42, - default_active_datetime=None, - default_expiration_datetime=None, + default_active_datetime=timezone.now() - timedelta(days=365), + default_expiration_datetime=timezone.now() + timedelta(days=365), default_internal_only=True ) assert created diff --git a/enterprise_subsidy/apps/subsidy/tests/test_models.py b/enterprise_subsidy/apps/subsidy/tests/test_models.py index 3d4a30b4..17714daf 100644 --- a/enterprise_subsidy/apps/subsidy/tests/test_models.py +++ b/enterprise_subsidy/apps/subsidy/tests/test_models.py @@ -1,12 +1,14 @@ """ Tests for functionality provided in the ``models.py`` module. """ +import random from itertools import product from unittest import mock from uuid import uuid4 import ddt import pytest +from django.core.exceptions import ValidationError from django.test import TestCase from openedx_ledger.models import Transaction, TransactionStateChoices from openedx_ledger.test_utils.factories import ( @@ -99,6 +101,73 @@ def test_price_for_content_not_in_catalog(self): with self.assertRaises(ContentNotFoundForCustomerException): self.subsidy.price_for_content('some-content-key') + def test_reference_uniqueness(self): + """ + Tests that not soft-deleted, non-internal-only subsidies + are validated to be unique on (reference_id, reference_type). + """ + reference_id = random.randint(1, 10000000) + existing_record = SubsidyFactory.create( + enterprise_customer_uuid=self.enterprise_customer_uuid, + reference_id=reference_id, + internal_only=False, + ) + existing_record.save() + + with self.assertRaisesRegex(ValidationError, 'already exists with the same reference_id'): + new_record = SubsidyFactory.create( + enterprise_customer_uuid=self.enterprise_customer_uuid, + reference_id=reference_id, + internal_only=False, + ) + new_record.save() + + def test_reference_uniqueness_not_constrained_on_internal_only(self): + """ + Tests that not soft-deleted, internal-only subsidies + are allowed to not be unique on (reference_id, reference_type). + """ + reference_id = random.randint(1, 100000000) + existing_record = SubsidyFactory.create( + enterprise_customer_uuid=self.enterprise_customer_uuid, + reference_id=reference_id, + internal_only=True, + ) + existing_record.save() + + new_record = SubsidyFactory.create( + enterprise_customer_uuid=self.enterprise_customer_uuid, + reference_id=reference_id, + internal_only=True, + ) + new_record.save() + self.assertEqual(existing_record.reference_id, new_record.reference_id) + self.assertEqual(existing_record.reference_type, new_record.reference_type) + + def test_reference_uniqueness_not_constrained_when_soft_deleted(self): + """ + Tests that soft-deleted, non-internal-only subsidies + are allowed to not be unique on (reference_id, reference_type). + """ + reference_id = random.randint(1, 100000000) + existing_record = SubsidyFactory.create( + enterprise_customer_uuid=self.enterprise_customer_uuid, + reference_id=reference_id, + internal_only=False, + is_soft_deleted=True, + ) + existing_record.save() + + new_record = SubsidyFactory.create( + enterprise_customer_uuid=self.enterprise_customer_uuid, + reference_id=reference_id, + internal_only=False, + is_soft_deleted=False, + ) + new_record.save() + self.assertEqual(existing_record.reference_id, new_record.reference_id) + self.assertEqual(existing_record.reference_type, new_record.reference_type) + @ddt.data(True, False) def test_is_redeemable(self, expected_to_be_redeemable): """ @@ -129,6 +198,8 @@ def setUp(self): self.subsidy = SubsidyFactory.create( enterprise_customer_uuid=self.enterprise_customer_uuid, ) + self.subsidy.lms_user_client = mock.MagicMock() + self.subsidy.lms_user_client.return_value.best_effort_user_data.return_value = {'email': 'edx@example.com'} super().setUp() def test_get_committed_transaction_no_reversal(self): @@ -231,6 +302,7 @@ def test_redeem_with_metadata(self, mock_get_content_summary, mock_enterprise_cl mock_get_content_summary.return_value = { 'content_uuid': 'course-v1:edX+test+course', 'content_key': 'course-v1:edX+test+course', + 'content_title': 'edx: Test Course', 'source': 'edX', 'mode': 'verified', 'content_price': 10000, diff --git a/enterprise_subsidy/settings/base.py b/enterprise_subsidy/settings/base.py index 4ad601b1..c020dfaa 100644 --- a/enterprise_subsidy/settings/base.py +++ b/enterprise_subsidy/settings/base.py @@ -360,7 +360,7 @@ def root(*path_fragments): # per-view cache timeout settings # We can disable caching on this view by setting the value below to 0. -CONTENT_METADATA_VIEW_CACHE_TIMEOUT_SECONDS = 60 +CONTENT_METADATA_VIEW_CACHE_TIMEOUT_SECONDS = 60 * 15 # disable indexing on history_date SIMPLE_HISTORY_DATE_INDEX = False diff --git a/requirements/base.txt b/requirements/base.txt index 1d52ee67..4f8dab16 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -173,7 +173,7 @@ openedx-events==8.6.0 # via # -r requirements/base.in # openedx-ledger -openedx-ledger==1.3.0 +openedx-ledger==1.3.2 # via -r requirements/base.in packaging==23.2 # via drf-yasg diff --git a/requirements/ci.in b/requirements/ci.in index 37978497..3586cbe3 100644 --- a/requirements/ci.in +++ b/requirements/ci.in @@ -3,4 +3,3 @@ -c constraints.txt tox # Virtualenv management for tests -tox-battery # Makes tox aware of requirements file changes diff --git a/requirements/ci.txt b/requirements/ci.txt index 3b584191..a8caf1c7 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -26,8 +26,5 @@ tox==3.28.0 # via # -c requirements/common_constraints.txt # -r requirements/ci.in - # tox-battery -tox-battery==0.6.2 - # via -r requirements/ci.in virtualenv==20.24.5 # via tox diff --git a/requirements/dev.txt b/requirements/dev.txt index 76c7a550..975b79d7 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -344,7 +344,7 @@ openedx-events==8.6.0 # via # -r requirements/validation.txt # openedx-ledger -openedx-ledger==1.3.0 +openedx-ledger==1.3.2 # via -r requirements/validation.txt packaging==23.2 # via diff --git a/requirements/doc.txt b/requirements/doc.txt index 04dddd47..385bf157 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -331,7 +331,7 @@ openedx-events==8.6.0 # via # -r requirements/test.txt # openedx-ledger -openedx-ledger==1.3.0 +openedx-ledger==1.3.2 # via -r requirements/test.txt packaging==23.2 # via diff --git a/requirements/production.txt b/requirements/production.txt index c0192774..334db2d4 100644 --- a/requirements/production.txt +++ b/requirements/production.txt @@ -213,7 +213,7 @@ openedx-events==8.6.0 # via # -r requirements/base.txt # openedx-ledger -openedx-ledger==1.3.0 +openedx-ledger==1.3.2 # via -r requirements/base.txt packaging==23.2 # via diff --git a/requirements/quality.txt b/requirements/quality.txt index 808b6fce..ada9b80a 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -310,7 +310,7 @@ openedx-events==8.6.0 # via # -r requirements/test.txt # openedx-ledger -openedx-ledger==1.3.0 +openedx-ledger==1.3.2 # via -r requirements/test.txt packaging==23.2 # via diff --git a/requirements/test.txt b/requirements/test.txt index ee6d1f57..bf41fc90 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -258,7 +258,7 @@ openedx-events==8.6.0 # via # -r requirements/base.txt # openedx-ledger -openedx-ledger==1.3.0 +openedx-ledger==1.3.2 # via -r requirements/base.txt packaging==23.2 # via diff --git a/requirements/validation.txt b/requirements/validation.txt index 536472b6..b4371938 100644 --- a/requirements/validation.txt +++ b/requirements/validation.txt @@ -399,7 +399,7 @@ openedx-events==8.6.0 # -r requirements/quality.txt # -r requirements/test.txt # openedx-ledger -openedx-ledger==1.3.0 +openedx-ledger==1.3.2 # via # -r requirements/quality.txt # -r requirements/test.txt diff --git a/tox.ini b/tox.ini index 75e00e4a..c69547ee 100644 --- a/tox.ini +++ b/tox.ini @@ -46,7 +46,7 @@ commands = setenv = DJANGO_SETTINGS_MODULE = enterprise_subsidy.settings.test PYTHONPATH = {toxinidir} -whitelist_externals = +allowlist_externals = make rm deps = @@ -59,7 +59,7 @@ commands = make -e -C docs html [testenv:translations] -whitelist_externals = +allowlist_externals = make deps = -r{toxinidir}/requirements/dev.txt @@ -67,7 +67,7 @@ commands = make validate_translations [testenv:quality] -whitelist_externals = +allowlist_externals = make deps = -r{toxinidir}/requirements/quality.txt