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

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

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

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

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

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