diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c66f0fa766..8ef5f4cd2d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -17,6 +17,10 @@ Unreleased ---------- * nothing unreleased +[4.23.1] +--------- +* fix: don't allow saving duplicate EnterpriseCatalogQuery content_filter + [4.22.6] ---------- * feat: add created time to reponse diff --git a/docs/decisions/0014-enterprise-catalog-sync-catalogs.rst b/docs/decisions/0014-enterprise-catalog-sync-catalogs.rst new file mode 100644 index 0000000000..5edc7b0103 --- /dev/null +++ b/docs/decisions/0014-enterprise-catalog-sync-catalogs.rst @@ -0,0 +1,43 @@ +Catalog syncing behavior with enterprise-catalog service +------------------------------------------------------------------------------ + +Status +====== + +Accepted + +Context +======= +We would like to have a way to reuse a saved set of courses configured by a query and use them in multiple catalogs across customers. + + +Decisions +========= + +- **Sync catalog/customer data with enterprise-catalog** +As the catalog data is saved in both edx-enterprise and enterprise-catalog, we need to make sure that catalog data is consistent across both. +In the context of edx-enterprise, changes to EnterpriseCatalogQuery and EnterpriseCustomerCatalog will be propagated to their counterparts +in the enterprise-catalog service. + + +Consequences +============ + +- **EnterpriseCatalogQuery Sync Process** +The enterprise-catalog service enforces a uniqueness constraint for the `content_filter` field, and so edx-enterprise must enforce it as well +or the sync operation will fail and the entities will be out of sync with each other. + +In the Django Admin console, whenever a new EnterpriseCatalogQuery is created, or an existing one edited, we first query enterprise-catalog to see if its +`content_filter` is already in use (by calculating the `content_filter` hash via a call to enterprise-catalog, and then attempting to retrieve +an existing catalog query from enterprise-catalog by that hash). If the new `content_filter` is unique, we will save the changes and propagate +them to enterprise-catalog. + +If the `content_filter` is *not* unique, we display an error on the EnterpriseCatalogQuery edit page and don't commit the change. + + +- **EnterpriseCustomerCatalog Sync Process** +In the Django Admin console, whenever a EnterpriseCustomerCatalog is created, or an existing one is edited, we simply pass the changes on to +enterprise-catalog without doing any checks for duplicates. + +When it comes to custom `content_filter` fields attached to EnterpriseCustomerCatalog objects, enterprise-catalog doesn't care if they are +non-unique, and will simply update the corresponding catalog entity to point to the catalog query that matches the `content_filter`. diff --git a/enterprise/__init__.py b/enterprise/__init__.py index 6302f876c9..6a8c5b9ed9 100644 --- a/enterprise/__init__.py +++ b/enterprise/__init__.py @@ -2,4 +2,4 @@ Your project description goes here. """ -__version__ = "4.22.6" +__version__ = "4.23.1" diff --git a/enterprise/api_client/client.py b/enterprise/api_client/client.py index 2e2855c7c5..37a042491f 100644 --- a/enterprise/api_client/client.py +++ b/enterprise/api_client/client.py @@ -31,14 +31,15 @@ class APIClientMixin: APPEND_SLASH = False API_BASE_URL = "" - def get_api_url(self, path): + def get_api_url(self, path, append_slash=None): """ Helper method to construct the API URL. Args: path (str): the API endpoint path string. """ - return urljoin(f"{self.API_BASE_URL}/", f"{path.strip('/')}{'/' if self.APPEND_SLASH else ''}") + _APPEND_SLASH = self.APPEND_SLASH if append_slash is None else append_slash + return urljoin(f"{self.API_BASE_URL}/", f"{path.strip('/')}{'/' if _APPEND_SLASH else ''}") class BackendServiceAPIClient(APIClientMixin): diff --git a/enterprise/api_client/enterprise_catalog.py b/enterprise/api_client/enterprise_catalog.py index 9800a444d0..a3e41f9609 100644 --- a/enterprise/api_client/enterprise_catalog.py +++ b/enterprise/api_client/enterprise_catalog.py @@ -31,6 +31,9 @@ class EnterpriseCatalogApiClient(UserAPIClient): ENTERPRISE_CUSTOMER_ENDPOINT = 'enterprise-customer' CONTENT_METADATA_IDENTIFIER_ENDPOINT = ENTERPRISE_CUSTOMER_ENDPOINT + \ "/{}/content-metadata/" + "{}" + CATALOG_QUERIES_ENDPOINT = 'catalog-queries' + GET_CONTENT_FILTER_HASH_ENDPOINT = CATALOG_QUERIES_ENDPOINT + '/get_content_filter_hash' + GET_QUERY_BY_HASH_ENDPOINT = CATALOG_QUERIES_ENDPOINT + '/get_query_by_hash?hash={}' APPEND_SLASH = True GET_CONTENT_METADATA_PAGE_SIZE = getattr(settings, 'ENTERPRISE_CATALOG_GET_CONTENT_METADATA_PAGE_SIZE', 50) @@ -110,6 +113,60 @@ def get_enterprise_catalog(self, catalog_uuid, should_raise_exception=True): ) return {} + @UserAPIClient.refresh_token + def get_enterprise_catalog_by_hash(self, catalog_query_hash, should_raise_exception=True): + """ + Gets an enterprise catalog by its content query hash. + + Arguments: + catalog_query_hash (str): The hash of an EnterpriseCatalog instance's query + should_raise_exception (bool): Whether an exception should be logged if + a catalog does not yet exist in enterprise-catalog. Defaults to True. + + Returns: + dict: a dictionary representing an enterprise catalog + """ + api_url = self.get_api_url(self.GET_QUERY_BY_HASH_ENDPOINT.format(catalog_query_hash), append_slash=False) + try: + response = self.client.get(api_url) + response.raise_for_status() + return response.json() + except (RequestException, ConnectionError, Timeout) as exc: + LOGGER.exception( + 'Failed to get EnterpriseCustomer Catalog by hash [%s] in enterprise-catalog due to: [%s]', + catalog_query_hash, str(exc) + ) + if should_raise_exception: + raise + return {} + + @UserAPIClient.refresh_token + def get_catalog_query_hash(self, catalog_query, should_raise_exception=True): + """ + Gets hash for content query. + + Arguments: + catalog_query (obj): content query json + should_raise_exception (bool): Whether an exception should be logged if + a catalog does not yet exist in enterprise-catalog. Defaults to True. + + Returns: + dict: a dictionary representing an enterprise catalog + """ + api_url = self.get_api_url(self.GET_CONTENT_FILTER_HASH_ENDPOINT, append_slash=False) + try: + response = self.client.get(api_url, data=catalog_query) + response.raise_for_status() + return response.json() + except (RequestException, ConnectionError, Timeout) as exc: + LOGGER.exception( + 'Failed to get catalog query hash for "[%s]" due to: [%s]', + catalog_query, str(exc) + ) + if should_raise_exception: + raise + return {} + @UserAPIClient.refresh_token def get_catalog_diff(self, enterprise_customer_catalog, content_keys, should_raise_exception=True): """ diff --git a/enterprise/models.py b/enterprise/models.py index 2adea0b44d..f161048af1 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -18,6 +18,7 @@ from jsonfield.encoder import JSONEncoder from jsonfield.fields import JSONField from multi_email_field.fields import MultiEmailField +from requests.exceptions import HTTPError from simple_history.models import HistoricalRecords from django.apps import apps @@ -2391,7 +2392,7 @@ class EnterpriseCatalogQuery(TimeStampedModel): help_text=_( "Query parameters which will be used to filter the discovery service's search/all endpoint results, " "specified as a JSON object. An empty JSON object means that all available content items will be " - "included in the catalog." + "included in the catalog. Must be unique." ), validators=[validate_content_filter_fields] ) @@ -2422,6 +2423,33 @@ def __str__(self): """ return "".format(title=self.title) + def clean(self): + """ + Before saving (and syncing with enterprise-catalog), check whether we're attempting to change + the content_filter to one that is a duplicate of an existing entry in enterprise-catalog + """ + previous_values = EnterpriseCatalogQuery.objects.filter(id=self.id).first() + if previous_values: + old_filter = previous_values.content_filter + new_filter = self.content_filter + if not old_filter == new_filter: + catalog_client = EnterpriseCatalogApiClient() + hash_catalog_response = None + try: + old_hash = catalog_client.get_catalog_query_hash(old_filter) + new_hash = catalog_client.get_catalog_query_hash(new_filter) + if not old_hash == new_hash: + hash_catalog_response = catalog_client.get_enterprise_catalog_by_hash(new_hash) + except HTTPError: + # If no results returned for querying by hash, we're safe to commit + return + except Exception as exc: + raise ValidationError({'content_filter': f'Failed to validate with exception: {exc}'}) from exc + if hash_catalog_response: + print(f'hash_catalog_response: {hash_catalog_response}') + err_msg = f'Duplicate value, see {hash_catalog_response["uuid"]}({hash_catalog_response["title"]})' + raise ValidationError({'content_filter': err_msg}) + def delete(self, *args, **kwargs): """ Deletes this ``EnterpriseCatalogQuery``. diff --git a/tests/test_enterprise/api_client/test_enterprise_catalog.py b/tests/test_enterprise/api_client/test_enterprise_catalog.py index 1df2532e69..a3a830a7b7 100644 --- a/tests/test_enterprise/api_client/test_enterprise_catalog.py +++ b/tests/test_enterprise/api_client/test_enterprise_catalog.py @@ -17,6 +17,7 @@ TEST_ENTERPRISE_ID = '1840e1dc-59cf-4a78-82c5-c5bbc0b5df0f' TEST_ENTERPRISE_CATALOG_UUID = 'cde8287a-1001-457c-b9d5-f9f5bd535427' +TEST_ENTERPRISE_CATALOG_NAME = 'Test Catalog' TEST_ENTERPRISE_NAME = 'Test Enterprise' @@ -100,6 +101,89 @@ def test_get_enterprise_catalog(): assert actual_response == expected_response +@responses.activate +@mock.patch('enterprise.api_client.client.JwtBuilder', mock.Mock()) +def test_get_enterprise_catalog_by_hash(): + TEST_CONTENT_FILTER_HASH = 'abcd1234' + expected_response = { + 'uuid': TEST_ENTERPRISE_CATALOG_UUID, + 'title': TEST_ENTERPRISE_CATALOG_NAME, + 'content_filter': {"content_type": "course"}, + 'content_filter_hash': TEST_CONTENT_FILTER_HASH, + } + responses.add( + responses.GET, + _url(f"catalog-queries/get_query_by_hash?hash={TEST_CONTENT_FILTER_HASH}"), + json=expected_response, + ) + client = enterprise_catalog.EnterpriseCatalogApiClient('staff-user-goes-here') + actual_response = client.get_enterprise_catalog_by_hash(TEST_CONTENT_FILTER_HASH) + assert actual_response == expected_response + + +@responses.activate +@mock.patch('enterprise.api_client.client.JwtBuilder', mock.Mock()) +@mark.parametrize('exception', (RequestException, ConnectionError, Timeout)) +def test_get_enterprise_catalog_by_hash_error(exception): + TEST_CONTENT_FILTER_HASH = 'abcd1234' + responses.add_callback( + responses.GET, + _url(f"catalog-queries/get_query_by_hash?hash={TEST_CONTENT_FILTER_HASH}"), + callback=exception, + content_type='application/json' + ) + client = enterprise_catalog.EnterpriseCatalogApiClient('staff-user-goes-here') + logger = logging.getLogger('enterprise.api_client.enterprise_catalog') + handler = MockLoggingHandler(level="ERROR") + logger.addHandler(handler) + expected_message = ( + f"Failed to get EnterpriseCustomer Catalog by hash [{TEST_CONTENT_FILTER_HASH}]" + " in enterprise-catalog due to: [" + ) + + with raises(exception): + client.get_enterprise_catalog_by_hash(TEST_CONTENT_FILTER_HASH) + assert expected_message in handler.messages['error'][0] + + +@responses.activate +@mock.patch('enterprise.api_client.client.JwtBuilder', mock.Mock()) +def test_get_catalog_query_hash(): + TEST_CONTENT_FILTER_HASH = 'abcd1234' + responses.add( + responses.GET, + _url("catalog-queries/get_content_filter_hash"), + json=TEST_CONTENT_FILTER_HASH, + ) + client = enterprise_catalog.EnterpriseCatalogApiClient('staff-user-goes-here') + actual_response = client.get_catalog_query_hash({"content_type": "course"}) + assert actual_response == TEST_CONTENT_FILTER_HASH + + +@responses.activate +@mock.patch('enterprise.api_client.client.JwtBuilder', mock.Mock()) +@mark.parametrize('exception', (RequestException, ConnectionError, Timeout)) +def test_get_catalog_query_hash_error(exception): + TEST_QUERY_HASH = {"content_type": "course"} + responses.add_callback( + responses.GET, + _url("catalog-queries/get_content_filter_hash"), + callback=exception, + content_type='application/json' + ) + client = enterprise_catalog.EnterpriseCatalogApiClient('staff-user-goes-here') + logger = logging.getLogger('enterprise.api_client.enterprise_catalog') + handler = MockLoggingHandler(level="ERROR") + logger.addHandler(handler) + expected_message = ( + f"Failed to get catalog query hash for \"[{TEST_QUERY_HASH}]\" due to: [" + ) + + with raises(exception): + client.get_catalog_query_hash(TEST_QUERY_HASH) + assert expected_message in handler.messages['error'][0] + + @responses.activate @mock.patch('enterprise.api_client.client.JwtBuilder', mock.Mock()) def test_get_catalog_diff(): diff --git a/tests/test_models.py b/tests/test_models.py index e5e8a0560a..c665f010f6 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -2566,6 +2566,34 @@ def test_save_content_filter_success(self, content_filter): catalog_query = EnterpriseCatalogQuery(content_filter=content_filter) catalog_query.full_clean() + @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_catalog_query_hash') + @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_enterprise_catalog_by_hash') + def test_clean_duplicate_query_check(self, mock_get_enterprise_catalog_by_hash, mock_get_catalog_query_hash): + mock_get_enterprise_catalog_by_hash.return_value = {'uuid': 'abcd', 'title': 'test catalog'} + mock_get_catalog_query_hash.side_effect = ['hash', 'differenthash'] + content_filter = '{"a":"b"}' + catalog_query = EnterpriseCatalogQuery(content_filter=content_filter) + catalog_query.save() + saved_catalog_query = EnterpriseCatalogQuery.objects.filter(id=catalog_query.id).first() + saved_catalog_query.content_filter = '{"c":"d"}' + with self.assertRaises(ValidationError) as cm: + saved_catalog_query.clean() + expected_exception = "{'content_filter': ['Duplicate value, see abcd(test catalog)']}" + assert str(cm.exception) == expected_exception + + @mock.patch('enterprise.api_client.enterprise_catalog.EnterpriseCatalogApiClient.get_catalog_query_hash') + def test_clean_call_failure(self, mock_get_catalog_query_hash): + mock_get_catalog_query_hash.side_effect = [Exception('Exception!')] + content_filter = '{"a":"b"}' + catalog_query = EnterpriseCatalogQuery(content_filter=content_filter) + catalog_query.save() + saved_catalog_query = EnterpriseCatalogQuery.objects.filter(id=catalog_query.id).first() + saved_catalog_query.content_filter = '{"c":"d"}' + with self.assertRaises(ValidationError) as cm: + saved_catalog_query.clean() + expected_exception = "{'content_filter': ['Failed to validate with exception: Exception!']}" + assert str(cm.exception) == expected_exception + @mock.patch('enterprise.signals.EnterpriseCatalogApiClient', return_value=mock.MagicMock()) def test_sync_catalog_query_content_filter(self, mock_api_client): # pylint: disable=unused-argument enterprise_customer = factories.EnterpriseCustomerFactory()