From c9ff964be7ec82f0babfc8c0516c4bcf080f0f4e 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 | 11 +- ...est_deduplicate_course_metadata_history.py | 22 ++-- .../apps/course_metadata/model_utils.py | 26 ---- .../apps/course_metadata/models.py | 120 ++++++++++++------ .../apps/course_metadata/tests/test_models.py | 56 ++++++++ 5 files changed, 155 insertions(+), 80 deletions(-) delete mode 100644 course_discovery/apps/course_metadata/model_utils.py diff --git a/course_discovery/apps/course_metadata/admin.py b/course_discovery/apps/course_metadata/admin.py index 3ce4e41760..e4254f7efc 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') @@ -217,7 +218,7 @@ class CourseEditorAdmin(admin.ModelAdmin): @admin.register(CourseEntitlement) -class CourseEntitlementAdmin(admin.ModelAdmin): +class CourseEntitlementAdmin(SimpleHistoryAdmin): list_display = ['course', 'get_course_key', 'mode', 'draft'] def get_course_key(self, obj): @@ -262,7 +263,7 @@ class CourseTypeAdmin(admin.ModelAdmin): @admin.register(CourseRun) -class CourseRunAdmin(admin.ModelAdmin): +class CourseRunAdmin(SimpleHistoryAdmin): inlines = (SeatInline,) list_display = ('uuid', 'key', 'external_key', 'title', 'status', 'draft',) list_filter = ( @@ -351,7 +352,7 @@ class ProgramLocationRestrictionAdmin(admin.ModelAdmin): @admin.register(Program) -class ProgramAdmin(DjangoObjectActions, admin.ModelAdmin): +class ProgramAdmin(DjangoObjectActions, SimpleHistoryAdmin): form = ProgramAdminForm list_display = ('id', 'uuid', 'title', 'type', 'partner', 'status', 'hidden') list_filter = ('partner', 'type', 'product_source', 'status', ProgramEligibilityFilter, 'hidden') @@ -490,7 +491,7 @@ class ProgramTypeAdmin(TranslatableAdmin): @admin.register(Seat) -class SeatAdmin(admin.ModelAdmin): +class SeatAdmin(SimpleHistoryAdmin): list_display = ('course_run', 'type', 'draft', 'upgrade_deadline_override',) raw_id_fields = ('draft_version',) readonly_fields = ('_upgrade_deadline',) 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 7e55c15f2c..cd7f047473 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,16 @@ 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 + 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 +51,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 +64,10 @@ 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 + # count remains same because all history instances are unique. + 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 deleted file mode 100644 index 82db02bae1..0000000000 --- a/course_discovery/apps/course_metadata/model_utils.py +++ /dev/null @@ -1,26 +0,0 @@ -""" -Utilities for models and model fields. -""" - - -def has_model_changed(field_tracker, external_keys=None, excluded_fields=None): - """ - Returns True if the model has changed, False otherwise. - - Args: - field_tracker (FieldTracker): FieldTracker instance - external_keys (list): Names of the Foreign Keys to check - - Returns: - Boolean indicating if model or associated keys have changed. - """ - external_keys = external_keys if external_keys else [] - excluded_fields = excluded_fields if excluded_fields else [] - - changed = field_tracker.changed() - for field in excluded_fields: - changed.pop(field, None) - - return len(changed) or any( - item.has_changed for item in external_keys if hasattr(item, 'has_changed') - ) diff --git a/course_discovery/apps/course_metadata/models.py b/course_discovery/apps/course_metadata/models.py index a7647f744b..35c5ebed2d 100644 --- a/course_discovery/apps/course_metadata/models.py +++ b/course_discovery/apps/course_metadata/models.py @@ -48,7 +48,6 @@ 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.people import MarketingSitePeople from course_discovery.apps.course_metadata.publishers import ( CourseRunMarketingSitePublisher, ProgramMarketingSitePublisher @@ -68,6 +67,47 @@ logger = logging.getLogger(__name__) +class ManageHistoryMixin(models.Model): + """ + Manages the history creation of the models based on the actual changes rather than saving without changes. + """ + + def has_model_changed(self, external_keys=None, excluded_fields=None): + """ + Returns True if the model has changed, False otherwise. + Args: + external_keys (list): Names of the Foreign Keys to check + excluded_fields (list): Names of fields to exclude + + Returns: + Boolean indicating if model or associated keys have changed. + """ + external_keys = external_keys if external_keys else [] + excluded_fields = excluded_fields if excluded_fields else [] + changed = self.field_tracker.changed() + for field in excluded_fields: + changed.pop(field, None) + + return len(changed) or any( + item.has_changed for item in external_keys if hasattr(item, 'has_changed') + ) + + def save(self, *args, **kwargs): + """ + Sets the parameter 'skip_history_on_save' if the object is not changed + """ + if not self.has_changed: + setattr(self, 'skip_history_when_saving', True) # pylint: disable=literal-used-as-attribute + + super().save(*args, **kwargs) + + if hasattr(self, 'skip_history_when_saving'): + delattr(self, 'skip_history_when_saving') # pylint: disable=literal-used-as-attribute + + class Meta: + abstract = True + + class DraftModelMixin(models.Model): """ Defines a draft boolean field and an object manager to make supporting drafts more transparent. @@ -184,7 +224,7 @@ class Meta: abstract = True -class Organization(CachedMixin, TimeStampedModel): +class Organization(ManageHistoryMixin, CachedMixin, TimeStampedModel): """ Organization model. """ partner = models.ForeignKey(Partner, models.CASCADE, null=True, blank=False) uuid = models.UUIDField(blank=False, null=False, default=uuid4, editable=True, verbose_name=_('UUID')) @@ -261,7 +301,7 @@ class Organization(CachedMixin, TimeStampedModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def clean(self): if not VALID_CHARS_IN_COURSE_NUM_AND_ORG_KEY.match(self.key): @@ -347,7 +387,7 @@ class Image(AbstractMediaModel): width = models.IntegerField(null=True, blank=True) -class Video(AbstractMediaModel): +class Video(ManageHistoryMixin, AbstractMediaModel): """ Video model. """ image = models.ForeignKey(Image, models.CASCADE, null=True, blank=True) @@ -357,7 +397,7 @@ class Video(AbstractMediaModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def __str__(self): return f'{self.src}: {self.description}' @@ -676,7 +716,7 @@ class Meta: verbose_name = _('Subject model translations') -class Topic(TranslatableModel, TimeStampedModel): +class Topic(ManageHistoryMixin, TranslatableModel, TimeStampedModel): """ Topic model. """ uuid = models.UUIDField(blank=False, null=False, default=uuid4, editable=False, verbose_name=_('UUID')) banner_image_url = models.URLField(blank=True, null=True) @@ -701,7 +741,7 @@ class Meta: def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def validate_unique(self, *args, **kwargs): super().validate_unique(*args, **kwargs) @@ -723,7 +763,7 @@ class Meta: verbose_name = _('Topic model translations') -class Fact(AbstractHeadingBlurbModel): +class Fact(ManageHistoryMixin, AbstractHeadingBlurbModel): """ Fact Model """ field_tracker = FieldTracker() @@ -732,7 +772,7 @@ class Fact(AbstractHeadingBlurbModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): if self.has_changed: @@ -745,7 +785,7 @@ def update_product_data_modified_timestamp(self): ) -class CertificateInfo(AbstractHeadingBlurbModel): +class CertificateInfo(ManageHistoryMixin, AbstractHeadingBlurbModel): """ Certificate Information Model """ field_tracker = FieldTracker() @@ -754,7 +794,7 @@ class CertificateInfo(AbstractHeadingBlurbModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): if self.has_changed: @@ -764,7 +804,7 @@ def update_product_data_modified_timestamp(self): ) -class ProductMeta(TimeStampedModel): +class ProductMeta(ManageHistoryMixin, TimeStampedModel): """ Model to contain SEO/Meta information for a product. """ @@ -788,7 +828,7 @@ class ProductMeta(TimeStampedModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self, bypass_has_changed=False): if self.has_changed or bypass_has_changed: @@ -804,7 +844,7 @@ def __str__(self): return self.title -class TaxiForm(TimeStampedModel): +class TaxiForm(ManageHistoryMixin, TimeStampedModel): """ Represents the data needed for a single Taxi (2U form library) lead capture form. """ @@ -829,10 +869,10 @@ class TaxiForm(TimeStampedModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() -class AdditionalMetadata(TimeStampedModel): +class AdditionalMetadata(ManageHistoryMixin, TimeStampedModel): """ This model holds 2U related additional fields. """ @@ -910,7 +950,7 @@ def has_changed(self): if not self.pk: return False external_keys = [self.product_meta,] - return has_model_changed(self.field_tracker, external_keys=external_keys) + return self.has_model_changed(external_keys=external_keys) def update_product_data_modified_timestamp(self, bypass_has_changed=False): if self.has_changed or bypass_has_changed: @@ -1102,7 +1142,7 @@ def __str__(self): return f'{self.name}' -class ProductValue(TimeStampedModel): +class ProductValue(ManageHistoryMixin, TimeStampedModel): """ ProductValue model, with fields related to projected value for a product. """ @@ -1139,7 +1179,7 @@ class ProductValue(TimeStampedModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): if self.has_changed: @@ -1150,7 +1190,7 @@ def update_product_data_modified_timestamp(self): self.courses.all().update(data_modified_timestamp=datetime.datetime.now(pytz.UTC)) -class GeoLocation(TimeStampedModel): +class GeoLocation(ManageHistoryMixin, TimeStampedModel): """ Model to keep Geographic location for Products. """ @@ -1197,7 +1237,7 @@ def coordinates(self): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): if self.has_changed: @@ -1226,7 +1266,7 @@ class Meta: abstract = True -class CourseLocationRestriction(AbstractLocationRestrictionModel): +class CourseLocationRestriction(ManageHistoryMixin, AbstractLocationRestrictionModel): """ Course location restriction model. @@ -1238,7 +1278,7 @@ class CourseLocationRestriction(AbstractLocationRestrictionModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): if self.has_changed: @@ -1249,7 +1289,7 @@ def update_product_data_modified_timestamp(self): self.courses.all().update(data_modified_timestamp=datetime.datetime.now(pytz.UTC)) -class Course(DraftModelMixin, PkSearchableMixin, CachedMixin, TimeStampedModel): +class Course(ManageHistoryMixin, DraftModelMixin, PkSearchableMixin, CachedMixin, TimeStampedModel): """ Course model. """ partner = models.ForeignKey(Partner, models.CASCADE) uuid = models.UUIDField(default=uuid4, editable=True, verbose_name=_('UUID')) @@ -1414,7 +1454,7 @@ def has_changed(self): excluded_fields = [ 'data_modified_timestamp', ] - return has_model_changed(self.field_tracker, excluded_fields=excluded_fields) + return self.has_model_changed(excluded_fields=excluded_fields) class Meta: unique_together = ( @@ -1882,7 +1922,7 @@ def set_subdirectory_slug(self): ) -class CourseEditor(TimeStampedModel): +class CourseEditor(ManageHistoryMixin, TimeStampedModel): """ CourseEditor model, defining who can edit a course and its course runs. @@ -1897,7 +1937,7 @@ class CourseEditor(TimeStampedModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() class Meta: unique_together = ('user', 'course',) @@ -1986,7 +2026,7 @@ def editable_course_runs(cls, user, queryset): return queryset.filter(user_can_edit, course__authoring_organizations__in=user_orgs).distinct() -class CourseRun(DraftModelMixin, CachedMixin, TimeStampedModel): +class CourseRun(ManageHistoryMixin, DraftModelMixin, CachedMixin, TimeStampedModel): """ CourseRun model. """ OFAC_RESTRICTION_CHOICES = ( ('', '--'), @@ -2129,7 +2169,7 @@ class CourseRun(DraftModelMixin, CachedMixin, TimeStampedModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): """ @@ -2626,7 +2666,7 @@ def _check_enterprise_subscription_inclusion(self): # there have to be two saves because in order to check for if this is included in the # 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 - def save(self, suppress_publication=False, send_emails=True, **kwargs): + def save(self, *args, suppress_publication=False, send_emails=True, **kwargs): """ Arguments: suppress_publication (bool): if True, we won't push the run data to the marketing site @@ -2641,11 +2681,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) + super().save(*args, **kwargs) self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion() kwargs['force_insert'] = False kwargs['force_update'] = True - super().save(**kwargs) + super().save(*args, **kwargs) self.handle_status_change(send_emails) if push_to_marketing: @@ -2823,7 +2863,7 @@ class CourseReview(TimeStampedModel): ) -class Seat(DraftModelMixin, TimeStampedModel): +class Seat(ManageHistoryMixin, DraftModelMixin, TimeStampedModel): """ Seat model. """ # This set of class variables is historic. Before CourseType and Mode and all that jazz, Seat used to just hold @@ -2883,7 +2923,7 @@ class Meta: def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): """ @@ -2909,7 +2949,7 @@ def upgrade_deadline(self, value): self._upgrade_deadline = value -class CourseEntitlement(DraftModelMixin, TimeStampedModel): +class CourseEntitlement(ManageHistoryMixin, DraftModelMixin, TimeStampedModel): """ Model storing product metadata for a Course. """ PRICE_FIELD_CONFIG = { 'decimal_places': 2, @@ -2931,7 +2971,7 @@ class CourseEntitlement(DraftModelMixin, TimeStampedModel): def has_changed(self): if not self.pk: return False - return has_model_changed(self.field_tracker) + return self.has_model_changed() def update_product_data_modified_timestamp(self): """ @@ -2985,7 +3025,7 @@ def __str__(self): return self.question -class Program(PkSearchableMixin, TimeStampedModel): +class Program(ManageHistoryMixin, PkSearchableMixin, TimeStampedModel): OFAC_RESTRICTION_CHOICES = ( ('', '--'), (True, _('Blocked')), @@ -3191,7 +3231,7 @@ def has_changed(self): self.in_year_value, self.taxi_form, ] - return has_model_changed(self.field_tracker, external_keys, excluded_fields) + return self.has_model_changed(external_keys, excluded_fields) objects = ProgramQuerySet.as_manager() @@ -3551,14 +3591,14 @@ def save(self, *args, **kwargs): self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion() kwargs['force_insert'] = False kwargs['force_update'] = True - super().save(**kwargs) + super().save(*args, **kwargs) publisher.publish_obj(self, previous_obj=previous_obj) else: super().save(*args, **kwargs) self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion() kwargs['force_insert'] = False kwargs['force_update'] = True - super().save(**kwargs) + super().save(*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 4f9697a708..7830263570 100644 --- a/course_discovery/apps/course_metadata/tests/test_models.py +++ b/course_discovery/apps/course_metadata/tests/test_models.py @@ -96,6 +96,62 @@ 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() + + verified_type = factories.SeatTypeFactory.verified() + seat = factories.SeatFactory(course_run=course_run, type=verified_type, price=0, sku='ABCDEF') + + entitlement_mode = SeatTypeFactory.verified() + entitlement = factories.CourseEntitlementFactory(course=course, mode=entitlement_mode, draft=True) + + assert len(course.history.all()) == 1 + assert len(course_run.history.all()) == 1 + assert len(program.history.all()) == 1 + assert len(seat.history.all()) == 1 + assert len(entitlement.history.all()) == 1 + + entitlement.sku = 'ABCDEF' + entitlement.save() + assert len(entitlement.history.all()) == 2 + + seat.price = 10 + seat.save() + assert len(seat.history.all()) == 2 + + 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 + + seat.save() + assert len(seat.history.all()) == 2 + + entitlement.save() + assert len(entitlement.history.all()) == 2 + def test_watchers(self): """ Verify watchers field is properly set and returns correct list of watchers emails addresses