From 7216ccf6f08397fec9fcbfea890d45e8aa0988cf Mon Sep 17 00:00:00 2001 From: uzairr Date: Thu, 31 Aug 2023 12:11:02 +0500 Subject: [PATCH] feat: skip the history object creation This change will add the modifications in which the history object is created only if the model instance is changed not the otherwise. PROD-3525 --- .../apps/course_metadata/admin.py | 3 +- ...est_deduplicate_course_metadata_history.py | 22 +++++++----- .../apps/course_metadata/model_utils.py | 16 +++++++++ .../apps/course_metadata/models.py | 18 +++++----- .../apps/course_metadata/tests/test_models.py | 34 +++++++++++++++++++ 5 files changed, 74 insertions(+), 19 deletions(-) diff --git a/course_discovery/apps/course_metadata/admin.py b/course_discovery/apps/course_metadata/admin.py index 3ce4e417608..c8ac59e6f6e 100644 --- a/course_discovery/apps/course_metadata/admin.py +++ b/course_discovery/apps/course_metadata/admin.py @@ -10,6 +10,7 @@ from django.utils.translation import gettext_lazy as _ from django_object_actions import DjangoObjectActions from parler.admin import TranslatableAdmin +from simple_history.admin import SimpleHistoryAdmin from waffle import get_waffle_flag_model # lint-amnesty, pylint: disable=invalid-django-waffle-import from course_discovery.apps.course_metadata.algolia_forms import SearchDefaultResultsConfigurationForm @@ -122,7 +123,7 @@ class ProductValueAdmin(admin.ModelAdmin): @admin.register(Course) -class CourseAdmin(DjangoObjectActions, admin.ModelAdmin): +class CourseAdmin(DjangoObjectActions, SimpleHistoryAdmin): form = CourseAdminForm list_display = ('uuid', 'key', 'key_for_reruns', 'title', 'draft',) list_filter = ('partner', 'product_source') diff --git a/course_discovery/apps/course_metadata/management/commands/tests/test_deduplicate_course_metadata_history.py b/course_discovery/apps/course_metadata/management/commands/tests/test_deduplicate_course_metadata_history.py index 7e55c15f2c7..4f8ccf1c612 100644 --- a/course_discovery/apps/course_metadata/management/commands/tests/test_deduplicate_course_metadata_history.py +++ b/course_discovery/apps/course_metadata/management/commands/tests/test_deduplicate_course_metadata_history.py @@ -30,13 +30,17 @@ def _assert_normal_case_pre_command(self): """ Verify the history count before running the clean-up command. """ - # Induce a few history records: - # - 2 updates for courserun1 - # - 0 updates for courserun2 - # - 3 updates for courserun3 + # In PROD-3525, history is only created if the instance is changed rather than saving it multiple times + self.courserun1.title = 'test title' self.courserun1.save() + + self.courserun3.title = 'test title' self.courserun3.save() + + self.courserun1.title = 'test title again' self.courserun1.save() + + self.courserun3.title = 'test title again' self.courserun3.save() factories.CourseRunFactory() # Toss in a fourth create to mix things up. self.courserun3.save() @@ -48,9 +52,9 @@ def _assert_normal_case_pre_command(self): # Ensure that there are multiple history records for each course run. For each # course run, there should be 2 (baseline) + the amount we added at the # beginning of this test * 2 for the double save for enterprise inclusion boolean - assert courserun1_count_initial == ((2 + 2) * 2) - assert courserun2_count_initial == ((2 + 0) * 2) - assert courserun3_count_initial == ((2 + 3) * 2) + assert courserun1_count_initial == 3 + assert courserun2_count_initial == 1 + assert courserun3_count_initial == 3 def _assert_normal_case_post_command(self): """ @@ -61,9 +65,9 @@ def _assert_normal_case_post_command(self): courserun3_count_final = CourseRun.history.filter(id=self.courserun3.id).count() # pylint: disable=no-member # Ensure that the only history records left are the 3 original creates. - assert courserun1_count_final == 1 + assert courserun1_count_final == 3 assert courserun2_count_final == 1 - assert courserun3_count_final == 1 + assert courserun3_count_final == 3 def test_normal_case(self): """ diff --git a/course_discovery/apps/course_metadata/model_utils.py b/course_discovery/apps/course_metadata/model_utils.py index 82db02bae17..62f727a2b67 100644 --- a/course_discovery/apps/course_metadata/model_utils.py +++ b/course_discovery/apps/course_metadata/model_utils.py @@ -24,3 +24,19 @@ def has_model_changed(field_tracker, external_keys=None, excluded_fields=None): return len(changed) or any( item.has_changed for item in external_keys if hasattr(item, 'has_changed') ) + + +def should_history_be_skipped_on_save(mro, obj, *args, **kwargs): + """ + Sets the parameter 'skip_history_on_save' if the object is not changed + Args: + mro: method resolution order of the model + obj: Model instance + """ + if not obj.has_changed: + setattr(obj, 'skip_history_when_saving', True) # pylint: disable=literal-used-as-attribute + + mro.save(*args, **kwargs) + + if hasattr(obj, 'skip_history_when_saving'): + delattr(obj, 'skip_history_when_saving') # pylint: disable=literal-used-as-attribute diff --git a/course_discovery/apps/course_metadata/models.py b/course_discovery/apps/course_metadata/models.py index 7376da7c903..b9961b25146 100644 --- a/course_discovery/apps/course_metadata/models.py +++ b/course_discovery/apps/course_metadata/models.py @@ -48,7 +48,7 @@ from course_discovery.apps.course_metadata.constants import SUBDIRECTORY_SLUG_FORMAT_REGEX, PathwayType from course_discovery.apps.course_metadata.fields import AutoSlugWithSlashesField, HtmlField, NullHtmlField from course_discovery.apps.course_metadata.managers import DraftManager -from course_discovery.apps.course_metadata.model_utils import has_model_changed +from course_discovery.apps.course_metadata.model_utils import has_model_changed, should_history_be_skipped_on_save from course_discovery.apps.course_metadata.people import MarketingSitePeople from course_discovery.apps.course_metadata.publishers import ( CourseRunMarketingSitePublisher, ProgramMarketingSitePublisher @@ -1459,11 +1459,11 @@ def save(self, *args, **kwargs): and then need to update the boolean in the record based on conditional logic """ self.update_data_modified_timestamp() - super().save(*args, **kwargs) + should_history_be_skipped_on_save(super(), self, *args, **kwargs) self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion() kwargs['force_insert'] = False kwargs['force_update'] = True - super().save(*args, **kwargs) + should_history_be_skipped_on_save(super(), self, *args, **kwargs) # Course runs calculate enterprise subscription inclusion based off of their parent's status, so we need to # force a recalculation @@ -2637,11 +2637,11 @@ def save(self, suppress_publication=False, send_emails=True, **kwargs): if push_to_marketing: previous_obj = CourseRun.objects.get(id=self.id) if self.id else None - super().save(**kwargs) + should_history_be_skipped_on_save(super(), self, **kwargs) self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion() kwargs['force_insert'] = False kwargs['force_update'] = True - super().save(**kwargs) + should_history_be_skipped_on_save(super(), self, **kwargs) self.handle_status_change(send_emails) if push_to_marketing: @@ -3543,18 +3543,18 @@ def save(self, *args, **kwargs): # subscription catalog, we need the id that is created on save to access the many-to-many fields # and then need to update the boolean in the record based on conditional logic with transaction.atomic(): - super().save(*args, **kwargs) + should_history_be_skipped_on_save(super(), self, *args, **kwargs) self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion() kwargs['force_insert'] = False kwargs['force_update'] = True - super().save(**kwargs) + should_history_be_skipped_on_save(super(), self, *args, **kwargs) publisher.publish_obj(self, previous_obj=previous_obj) else: - super().save(*args, **kwargs) + should_history_be_skipped_on_save(super(), self, *args, **kwargs) self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion() kwargs['force_insert'] = False kwargs['force_update'] = True - super().save(**kwargs) + should_history_be_skipped_on_save(super(), self, *args, **kwargs) @property def is_2u_degree_program(self): diff --git a/course_discovery/apps/course_metadata/tests/test_models.py b/course_discovery/apps/course_metadata/tests/test_models.py index 2cf1b67ad77..100c538f74c 100644 --- a/course_discovery/apps/course_metadata/tests/test_models.py +++ b/course_discovery/apps/course_metadata/tests/test_models.py @@ -92,6 +92,40 @@ def test_image_url(self): course.image = None assert course.image_url == course.card_image_url + def test_validate_history_created_only_on_change(self): + """ + Validate that course history object would be created if the object is changed otherwise not. + """ + course = factories.CourseFactory() + course_run = factories.CourseRunFactory() + program = factories.ProgramFactory() + + assert len(course.history.all()) == 1 + assert len(course_run.history.all()) == 1 + assert len(program.history.all()) == 1 + + course.title = 'Test course title' + course.save() + assert len(course.history.all()) == 2 + + course_run.title = 'Test course run title' + course_run.save() + assert len(course_run.history.all()) == 2 + + program.title = 'Test program title' + program.save() + assert len(program.history.all()) == 2 + + # explicitly calling save to validate the history is not created + course.save() + assert len(course.history.all()) == 2 + + course_run.save() + assert len(course_run.history.all()) == 2 + + program.save() + assert len(program.history.all()) == 2 + def test_watchers(self): """ Verify watchers field is properly set and returns correct list of watchers emails addresses