Skip to content

Commit

Permalink
feat: skip the history object creation
Browse files Browse the repository at this point in the history
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
  • Loading branch information
uzairr committed Sep 6, 2023
1 parent 2f2307f commit 3d05017
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 10 deletions.
3 changes: 2 additions & 1 deletion course_discovery/apps/course_metadata/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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')
Expand Down
16 changes: 16 additions & 0 deletions course_discovery/apps/course_metadata/model_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
18 changes: 9 additions & 9 deletions course_discovery/apps/course_metadata/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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):
Expand Down
34 changes: 34 additions & 0 deletions course_discovery/apps/course_metadata/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 3d05017

Please sign in to comment.