Merge pull request #2179 from openedx/mkeating/ENT-8661

fix: don't allow saving duplicate EnterpriseCatalogQuery content_filter

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

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`.

enterprise/__init__.py

@@ -2,4 +2,4 @@
 Your project description goes here.
 """
 
-__version__ = "4.22.6"
+__version__ = "4.23.1"

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):

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):
         """

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 "<EnterpriseCatalogQuery '{title}' >".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``.

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():

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()