From 19f5833c2c34138588696ccb7e5a2c3db81b2495 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Tue, 22 Oct 2024 16:57:05 -0400 Subject: [PATCH 1/2] fix: updates to ENT-9579 * Updates Django Admin fields for models.DefaultEnterpriseEnrollmentIntention * Ensures DefaultEnterpriseEnrollmentIntentionAdmin uses SoftDeletableModel's all_objects model manager to be able to view/restore soft-deleted instances. * Adds help text in Django Admin for SoftDeletableModel's is_removed boolean field. * Alters content_type model field to allow blank/null. * Adds unique constraint for enterprise_customer and content_key to prevent duplicates. * Remove `current_*` prefixes on computed properties since a configured content_key might not be the *current* course run. * Updates models.DefaultEnterpriseEnrollmentIntention `clean` method to check for soft-deleted duplicates to raise ValidationError. * Ensures `self.content_type` is already updated on save, even if already set (e.g., if content_key changes to a different content type). * Updates related tests. --- enterprise/admin/__init__.py | 61 +++++-- enterprise/content_metadata/api.py | 8 + ...rollmentintention_content_type_and_more.py | 27 +++ enterprise/models.py | 156 +++++++++++++----- tests/test_admin/test_view.py | 1 - tests/test_enterprise/api/test_views.py | 1 - tests/test_models.py | 119 ++++++------- 7 files changed, 259 insertions(+), 114 deletions(-) create mode 100644 enterprise/migrations/0225_alter_defaultenterpriseenrollmentintention_content_type_and_more.py diff --git a/enterprise/admin/__init__.py b/enterprise/admin/__init__.py index a15608b5b..06070dd5e 100644 --- a/enterprise/admin/__init__.py +++ b/enterprise/admin/__init__.py @@ -118,21 +118,24 @@ class EnterpriseCustomerDefaultEnterpriseEnrollmentIntentionInline(admin.Tabular """ model = models.DefaultEnterpriseEnrollmentIntention - fields = ('content_key', 'course_run_key_for_enrollment',) - readonly_fields = ('course_run_key_for_enrollment',) + fields = ('content_key', 'course_key', 'course_run_key_for_enrollment',) + readonly_fields = ('course_key', 'course_run_key_for_enrollment',) extra = 0 can_delete = True + @admin.display(description='Course key') + def course_key(self, obj): + """ + Returns the course run key. + """ + return obj.course_key + @admin.display(description='Course run key for enrollment') def course_run_key_for_enrollment(self, obj): """ - Returns the course run key based on the content type. - If the content type is a course, we retrieve the advertised_course_run key. + Returns the course run key. """ - content_type = obj.content_type - if content_type == 'course_run': - return obj.content_key - return obj.current_course_run_key + return obj.course_run_key class PendingEnterpriseCustomerAdminUserInline(admin.TabularInline): @@ -1352,13 +1355,32 @@ class DefaultEnterpriseEnrollmentIntentionAdmin(admin.ModelAdmin): list_display = ( 'uuid', 'enterprise_customer', + 'content_key', 'content_type', + 'is_removed', + ) + + list_filter = ('is_removed',) + + fields = ( + 'enterprise_customer', 'content_key', + 'uuid', + 'is_removed', + 'content_type', + 'course_key', + 'course_run_key', + 'created', + 'modified', ) readonly_fields = ( - 'current_course_run_key', - 'is_removed', + 'uuid', + 'content_type', + 'course_key', + 'course_run_key', + 'created', + 'modified', ) search_fields = ( @@ -1370,5 +1392,22 @@ class DefaultEnterpriseEnrollmentIntentionAdmin(admin.ModelAdmin): ordering = ('-modified',) class Meta: - fields = '__all__' model = models.DefaultEnterpriseEnrollmentIntention + + def get_queryset(self, request): # pylint: disable=unused-argument + """ + Return a QuerySet of all model instances. + """ + return self.model.all_objects.get_queryset() + + def formfield_for_dbfield(self, db_field, request, **kwargs): + """ + Customize the form field for the `is_removed` field. + """ + formfield = super().formfield_for_dbfield(db_field, request, **kwargs) + + if db_field.name == 'is_removed': + formfield.help_text = 'Whether this record is soft-deleted. Soft-deleted records ' \ + 'are not used but may be re-enabled if needed.' + + return formfield diff --git a/enterprise/content_metadata/api.py b/enterprise/content_metadata/api.py index d00d1a8e5..4023c1603 100644 --- a/enterprise/content_metadata/api.py +++ b/enterprise/content_metadata/api.py @@ -42,6 +42,14 @@ def get_and_cache_customer_content_metadata(enterprise_customer_uuid, content_ke except HTTPError as exc: raise exc + if not result: + logger.warning( + 'No content found for customer %s and content_key %s', + enterprise_customer_uuid, + content_key, + ) + return {} + logger.info( 'Fetched catalog for customer %s and content_key %s. Result = %s', enterprise_customer_uuid, diff --git a/enterprise/migrations/0225_alter_defaultenterpriseenrollmentintention_content_type_and_more.py b/enterprise/migrations/0225_alter_defaultenterpriseenrollmentintention_content_type_and_more.py new file mode 100644 index 000000000..852eed975 --- /dev/null +++ b/enterprise/migrations/0225_alter_defaultenterpriseenrollmentintention_content_type_and_more.py @@ -0,0 +1,27 @@ +# Generated by Django 4.2.16 on 2024-10-22 18:19 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('enterprise', '0224_alter_enterprisegroup_applies_to_all_contexts_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='defaultenterpriseenrollmentintention', + name='content_type', + field=models.CharField(blank=True, choices=[('course', 'Course'), ('course_run', 'Course Run')], help_text='The type of content (e.g. a course vs. a course run).', max_length=127, null=True), + ), + migrations.AlterField( + model_name='historicaldefaultenterpriseenrollmentintention', + name='content_type', + field=models.CharField(blank=True, choices=[('course', 'Course'), ('course_run', 'Course Run')], help_text='The type of content (e.g. a course vs. a course run).', max_length=127, null=True), + ), + migrations.AddConstraint( + model_name='defaultenterpriseenrollmentintention', + constraint=models.UniqueConstraint(fields=('enterprise_customer', 'content_key'), name='unique_default_enrollment_intention'), + ), + ] diff --git a/enterprise/models.py b/enterprise/models.py index ce3987b19..937677554 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -2470,9 +2470,11 @@ class DefaultEnterpriseEnrollmentIntention(TimeStampedModel, SoftDeletableModel) .. no_pii: """ + COURSE = 'course' + COURSE_RUN = 'course_run' DEFAULT_ENROLLMENT_CONTENT_TYPE_CHOICES = [ - ('course', 'Course'), - ('course_run', 'Course Run'), + (COURSE, 'Course'), + (COURSE_RUN, 'Course Run'), ] uuid = models.UUIDField( primary_key=True, @@ -2491,8 +2493,8 @@ class DefaultEnterpriseEnrollmentIntention(TimeStampedModel, SoftDeletableModel) ) content_type = models.CharField( max_length=127, - blank=False, - null=False, + blank=True, + null=True, choices=DEFAULT_ENROLLMENT_CONTENT_TYPE_CHOICES, help_text=_( "The type of content (e.g. a course vs. a course run)." @@ -2513,78 +2515,149 @@ class DefaultEnterpriseEnrollmentIntention(TimeStampedModel, SoftDeletableModel) ) history = HistoricalRecords() + class Meta: + constraints = [ + models.UniqueConstraint( + fields=['enterprise_customer', 'content_key'], + name='unique_default_enrollment_intention', + ) + ] + @property def content_metadata_for_content_key(self): """ - Retrieves the cached content metadata for the instance content_key - Determines catalog from the catalogs associated to the enterprise customer + Retrieves the content metadata for the instance's enterprise customer and content key. """ - return get_and_cache_customer_content_metadata( - enterprise_customer_uuid=self.enterprise_customer.uuid, - content_key=self.content_key, - ) + try: + return get_and_cache_customer_content_metadata( + enterprise_customer_uuid=self.enterprise_customer.uuid, + content_key=self.content_key, + ) + except HTTPError as e: + LOGGER.error( + f"Error retrieving content metadata for content key {self.content_key} " + f"and enterprise customer {self.enterprise_customer}: {e}" + ) + return {} @property - def current_course_run(self): # pragma: no cover + def course_run(self): """ Metadata describing the current course run for this default enrollment intention. """ - content_metadata = self.content_metadata_for_content_key - if not content_metadata: - return {} - if self.content_type == 'course': - if course_run := get_advertised_course_run(content_metadata): - return course_run + if not (content_metadata := self.content_metadata_for_content_key): return {} + + if self.determine_content_type() == self.COURSE: + course_run = get_advertised_course_run(content_metadata) + return course_run or {} + course_runs = content_metadata.get('course_runs', []) return next( - (course_run for course_run in course_runs if course_run['key'].lower() == self.content_key.lower()), {} + (course_run for course_run in course_runs if course_run['key'].lower() == self.content_key.lower()), + {} ) @property - def current_course_run_key(self): # pragma: no cover + def course_key(self): """ - The current course run key to use for realized course enrollments. + The resolved course key derived from the content_key. """ - return self.current_course_run.get('key') + return self.content_metadata_for_content_key.get('key') @property - def get_content_type(self): + def course_run_key(self): """ - Determines the content_type for a given content_key by validating a return value - from `get_content_metadata_content_identifier` and first checking if the passed content_key - is a top level course key, if not and the API call resolved correctly, we can assume the content_type - is a `course_run` + The resolved course run key derived from the content_key. This property will return the advertised + course run key if the configured content_key is a course; otherwise, it will return the key of the + course run that matches the content_key (i.e., course_run_key == content_key). """ - content_metadata = self.content_metadata_for_content_key - if not content_metadata: - return None - content_metadata_key = content_metadata.get('key').lower() - if content_metadata_key == self.content_key.lower(): - return 'course' - return 'course_run' + return self.course_run.get('key') @property - def current_course_run_enrollable(self): # pragma: no cover + def is_course_run_enrollable(self): # pragma: no cover """ - Whether the current course run is enrollable. + Whether the course run is enrollable. """ return False @property - def current_course_run_enroll_by_date(self): # pragma: no cover + def course_run_enroll_by_date(self): # pragma: no cover """ - The enrollment deadline for this course. + The enrollment deadline for the course run. """ return datetime.datetime.min + def determine_content_type(self): + """ + Determines the content_type for a given content_key by validating the return value + from `content_metadata_for_content_key`. First determines if the configured content_key + matches the returned key, then checks if it matches any of the returned course runs. + + Returns either COURSE, COURSE_RUN, or None (if neither can be determined). + """ + content_metadata = self.content_metadata_for_content_key + if not content_metadata: + return None + + # Determine whether the returned key matches the configured content_key and + # the returned metadata denotes the content type as a course. + content_metadata_key = content_metadata.get('key', '') + content_metadata_content_type = content_metadata.get('content_type', '') + if content_metadata_key.lower() == self.content_key.lower() and content_metadata_content_type == self.COURSE: + return self.COURSE + + # Determine if the content_key matches any of the course runs + # in the content metadata. + course_runs = content_metadata.get('course_runs', []) + course_run = next( + (course_run for course_run in course_runs if course_run['key'].lower() == self.content_key.lower()), + None + ) + return self.COURSE_RUN if course_run is not None else None + def clean(self): - if not (self.content_type or self.get_content_type): - raise ValidationError('The content key did match a valid course or course-run') + """ + Raise ValidationError if no course run or content type exists. + """ + super().clean() + + existing_record = DefaultEnterpriseEnrollmentIntention.all_objects.filter( + enterprise_customer=self.enterprise_customer, + content_key=self.content_key, + ).exclude(uuid=self.uuid).first() + + if existing_record and existing_record.is_removed: + existing_record_admin_url = reverse( + 'admin:enterprise_defaultenterpriseenrollmentintention_change', + args=[existing_record.uuid], + ) + message = _( + f'A default enrollment intention with this enterprise customer and ' + f'content key already exists, but is soft-deleted. Please restore ' + f'it here.', + ) + raise ValidationError({ + 'content_key': mark_safe(message) + }) + + if not self.course_run: + raise ValidationError({ + 'content_key': _('The content key did not resolve to a valid course run.') + }) def save(self, *args, **kwargs): - if not self.content_type: - self.content_type = self.get_content_type + """ + Override save to ensure that the content_type is set correctly before saving. + """ + # Ensure the model instance is cleaned before saving + self.full_clean() + + # Set content_type field + if content_type := self.determine_content_type(): + self.content_type = content_type + + # Call the superclass save method super().save(*args, **kwargs) @@ -2685,7 +2758,6 @@ def clean(self): 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}) diff --git a/tests/test_admin/test_view.py b/tests/test_admin/test_view.py index 128e24178..7bb7ade0e 100644 --- a/tests/test_admin/test_view.py +++ b/tests/test_admin/test_view.py @@ -1680,7 +1680,6 @@ def test_post_create_course_enrollments( bulk_upload_errors = [] if manage_learners_form: bulk_upload_errors = manage_learners_form.errors[ManageLearnersForm.Fields.BULK_UPLOAD] - print('bulk_upload_errors', bulk_upload_errors) if valid_course_id and course_in_catalog: # valid input, no errors diff --git a/tests/test_enterprise/api/test_views.py b/tests/test_enterprise/api/test_views.py index 8c7fd276b..10e6c9d49 100644 --- a/tests/test_enterprise/api/test_views.py +++ b/tests/test_enterprise/api/test_views.py @@ -6806,7 +6806,6 @@ def test_reporting_config_enterprise_catalogs_update(self, request_or_stub_mock) format='json', ) # validate the existing associated catalogs. - print(response.content) assert response.status_code == status.HTTP_200_OK ec_catalog_uuids = [item['uuid'] for item in response.json()['enterprise_customer_catalogs']] assert [str(enterprise_catalog.uuid)] == ec_catalog_uuids diff --git a/tests/test_models.py b/tests/test_models.py index e6cfce77c..1b9b5f23d 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -10,7 +10,6 @@ import unittest from datetime import timedelta from unittest import mock -from unittest.mock import PropertyMock from urllib.parse import urljoin from uuid import UUID @@ -22,6 +21,7 @@ from pytest import mark, raises from slumber.exceptions import HttpClientError from testfixtures import LogCapture +from requests.exceptions import HTTPError from django.conf import settings from django.core.exceptions import ValidationError @@ -29,6 +29,7 @@ from django.core.files.storage import Storage from django.db.utils import IntegrityError from django.http import QueryDict +from django.test import override_settings from django.test.testcases import TransactionTestCase from django.urls import reverse @@ -218,15 +219,15 @@ def setUp(self): self.advertised_course_run_uuid = self.faker.uuid4() self.course_run_1_uuid = self.faker.uuid4() self.mock_course_run_1 = { - 'key': 'course-v1:edX+demoX+2T2023', + 'key': 'course-v1:edX+DemoX+2T2023', 'title': 'Demo Course', - 'parent_content_key': 'edX+demoX', + 'parent_content_key': 'edX+DemoX', 'uuid': self.course_run_1_uuid } self.mock_advertised_course_run = { - 'key': 'course-v1:edX+demoX+3T2024', + 'key': 'course-v1:edX+DemoX+3T2024', 'title': 'Demo Course', - 'parent_content_key': 'edX+demoX', + 'parent_content_key': 'edX+DemoX', 'uuid': self.advertised_course_run_uuid } self.mock_course_runs = [ @@ -234,8 +235,8 @@ def setUp(self): self.mock_advertised_course_run, ] self.mock_course = { - 'key': 'edX+demoX', - 'content_type': 'course', + 'key': 'edX+DemoX', + 'content_type': DefaultEnterpriseEnrollmentIntention.COURSE, 'course_runs': self.mock_course_runs, 'advertised_course_run_uuid': self.advertised_course_run_uuid } @@ -246,76 +247,77 @@ def tearDown(self): DefaultEnterpriseEnrollmentIntention.objects.all().delete() @mock.patch( - 'enterprise.models.DefaultEnterpriseEnrollmentIntention.content_metadata_for_content_key', - new_callable=PropertyMock + 'enterprise.models.get_and_cache_customer_content_metadata', + return_value=mock.MagicMock(), ) - def test_retrieve_current_course_run_advertised_course_run(self, mock_content_metadata_for_content_key): - mock_content_metadata_for_content_key.return_value = self.mock_course + def test_retrieve_course_run_advertised_course_run(self, mock_get_and_cache_customer_content_metadata): + mock_get_and_cache_customer_content_metadata.return_value = self.mock_course default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( enterprise_customer=self.test_enterprise_customer_1, - content_key='edX+demoX', + content_key='edX+DemoX', ) - assert default_enterprise_enrollment_intention.current_course_run == self.mock_advertised_course_run + assert default_enterprise_enrollment_intention.course_run == self.mock_advertised_course_run + assert default_enterprise_enrollment_intention.course_key == self.mock_course['key'] + assert default_enterprise_enrollment_intention.course_run_key == self.mock_advertised_course_run['key'] + assert default_enterprise_enrollment_intention.content_type == DefaultEnterpriseEnrollmentIntention.COURSE @mock.patch( - 'enterprise.models.DefaultEnterpriseEnrollmentIntention.content_metadata_for_content_key', - new_callable=PropertyMock + 'enterprise.models.get_and_cache_customer_content_metadata', + return_value=mock.MagicMock(), ) - def test_retrieve_current_course_run_with_course_run(self, mock_content_metadata_for_content_key): - mock_content_metadata_for_content_key.return_value = self.mock_course + def test_retrieve_course_run_with_explicit_course_run(self, mock_get_and_cache_customer_content_metadata): + mock_get_and_cache_customer_content_metadata.return_value = self.mock_course default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( enterprise_customer=self.test_enterprise_customer_1, - content_key='course-v1:edX+demoX+2T2023', + content_key='course-v1:edX+DemoX+2T2023', ) - assert default_enterprise_enrollment_intention.current_course_run == self.mock_course_run_1 + assert default_enterprise_enrollment_intention.course_run == self.mock_course_run_1 + assert default_enterprise_enrollment_intention.course_key == self.mock_course['key'] + assert default_enterprise_enrollment_intention.course_run_key == self.mock_course_run_1['key'] + assert default_enterprise_enrollment_intention.content_type == DefaultEnterpriseEnrollmentIntention.COURSE_RUN @mock.patch( - 'enterprise.models.DefaultEnterpriseEnrollmentIntention.content_metadata_for_content_key', - new_callable=PropertyMock + 'enterprise.models.get_and_cache_customer_content_metadata', + return_value=mock.MagicMock(), ) - def test_retrieve_current_course_run_key_advertised_course_run(self, mock_content_metadata_for_content_key): - mock_content_metadata_for_content_key.return_value = self.mock_course - default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( - enterprise_customer=self.test_enterprise_customer_1, - content_key='edX+demoX', - ) - assert default_enterprise_enrollment_intention.current_course_run_key == self.mock_advertised_course_run['key'] - - @mock.patch( - 'enterprise.models.DefaultEnterpriseEnrollmentIntention.content_metadata_for_content_key', - new_callable=PropertyMock - ) - def test_retrieve_current_course_run_key_with_course_run(self, mock_content_metadata_for_content_key): - mock_content_metadata_for_content_key.return_value = self.mock_course - default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( - enterprise_customer=self.test_enterprise_customer_1, - content_key='course-v1:edX+demoX+2T2023', - ) - assert default_enterprise_enrollment_intention.current_course_run_key == self.mock_course_run_1['key'] + def test_validate_missing_course_run(self, mock_get_and_cache_customer_content_metadata): + # Simulate a 404 Not Found by raising an HTTPError exception + mock_get_and_cache_customer_content_metadata.side_effect = HTTPError + with self.assertRaises(ValidationError) as exc_info: + DefaultEnterpriseEnrollmentIntention.objects.create( + enterprise_customer=self.test_enterprise_customer_1, + content_key='invalid_content_key', + ) + self.assertIn('The content key did not resolve to a valid course run.', str(exc_info.exception)) @mock.patch( - 'enterprise.models.DefaultEnterpriseEnrollmentIntention.content_metadata_for_content_key', - new_callable=PropertyMock + 'enterprise.models.get_and_cache_customer_content_metadata', + return_value=mock.MagicMock(), ) - def test_get_content_type_course(self, mock_content_metadata_for_content_key): - mock_content_metadata_for_content_key.return_value = self.mock_course - default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( + @override_settings(ROOT_URLCONF="test_utils.admin_urls") + def test_validate_existing_soft_deleted_record(self, mock_get_and_cache_customer_content_metadata): + mock_get_and_cache_customer_content_metadata.return_value = self.mock_course + existing_record = DefaultEnterpriseEnrollmentIntention.objects.create( enterprise_customer=self.test_enterprise_customer_1, - content_key='edX+demoX', + content_key='edX+DemoX', + is_removed=True, + ) + # Attempt to re-create the above entry + with self.assertRaises(ValidationError) as exc_info: + DefaultEnterpriseEnrollmentIntention.objects.create( + enterprise_customer=self.test_enterprise_customer_1, + content_key='edX+DemoX', + ) + existing_record_admin_url = reverse( + 'admin:enterprise_defaultenterpriseenrollmentintention_change', + args=[existing_record.uuid], ) - assert default_enterprise_enrollment_intention.get_content_type == 'course' - - @mock.patch( - 'enterprise.models.DefaultEnterpriseEnrollmentIntention.content_metadata_for_content_key', - new_callable=PropertyMock - ) - def test_get_content_type_course_run(self, mock_content_metadata_for_content_key): - mock_content_metadata_for_content_key.return_value = self.mock_course - default_enterprise_enrollment_intention = DefaultEnterpriseEnrollmentIntention.objects.create( - enterprise_customer=self.test_enterprise_customer_1, - content_key='course-v1:edX+demoX+2T2023', + message = ( + f'A default enrollment intention with this enterprise customer and ' + f'content key already exists, but is soft-deleted. Please restore ' + f'it here.' ) - assert default_enterprise_enrollment_intention.get_content_type == 'course_run' + self.assertIn(message, str(exc_info.exception)) @mark.django_db @@ -1199,7 +1201,6 @@ def test_logo_path(self): storage_mock = mock.MagicMock(spec=Storage, name="StorageMock") with mock.patch("django.core.files.storage.default_storage._wrapped", storage_mock): path = logo_path(branding_config, branding_config.logo.name) - print(path) self.assertTrue(re.search(self.BRANDING_PATH_REGEX, path)) def test_logo_path_after_save(self): From 6389d2afc4c6b3e07b544e6479be78cf1822fc5d Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Thu, 24 Oct 2024 09:42:19 -0400 Subject: [PATCH 2/2] chore: pr feedback --- enterprise/models.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/enterprise/models.py b/enterprise/models.py index 937677554..5c85d8f41 100644 --- a/enterprise/models.py +++ b/enterprise/models.py @@ -2596,8 +2596,7 @@ def determine_content_type(self): Returns either COURSE, COURSE_RUN, or None (if neither can be determined). """ - content_metadata = self.content_metadata_for_content_key - if not content_metadata: + if not (content_metadata := self.content_metadata_for_content_key): return None # Determine whether the returned key matches the configured content_key and @@ -2642,6 +2641,8 @@ def clean(self): }) if not self.course_run: + # NOTE: This validation check also acts as an inferred check on the derived content_type + # from the content metadata. raise ValidationError({ 'content_key': _('The content key did not resolve to a valid course run.') })