feat: skip the history object creation #4071

uzairr · 2023-08-31T07:13:51Z

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

DawoudSheraz · 2023-08-31T09:20:25Z

course_discovery/apps/course_metadata/models.py

+        if not self.has_changed:
+            setattr(self, 'skip_history_when_saving', True)

-        # Course runs calculate enterprise subscription inclusion based off of their parent's status, so we need to
-        # force a recalculation
-        course_runs = self.course_runs.all()
-        for course_run in course_runs:
-            course_run.save()
+        try:
+            self.update_data_modified_timestamp()
+            super().save(*args, **kwargs)
+            self.enterprise_subscription_inclusion = self._check_enterprise_subscription_inclusion()
+            kwargs['force_insert'] = False
+            kwargs['force_update'] = True
+            super().save(*args, **kwargs)
+
+            # Course runs calculate enterprise subscription inclusion based off of their parent's status, so we need to
+            # force a recalculation
+            course_runs = self.course_runs.all()
+            for course_run in course_runs:
+                course_run.save()
+        finally:
+            if hasattr(self, 'skip_history_when_saving'):
+                delattr(self, 'skip_history_when_saving')


Rather than doing this, add a new method that is called in place of super. save. Inside that method, we can decide that if self.has_changed is true, call super.save as it is. Otherwise, set skip history attribute to true. This way, the history logic will be contained in a single place. Also, since the new method is being called inside the overridden save, we wont need any changes across the codebase.

AliAdnanSohail

Just a suggestion: can we create a Mixin which will have save() method in it. And we will add
logic of should_history_be_skipped_on_save in there. By doing this we don't have to change logic in model level.
We just have to inherit that mixin for that model and rest will be handled in overridden save method.

WDYT?

uzairr · 2023-09-05T11:40:37Z

by and large, it is the same thing that you are suggesting, right now should_history_be_skipped_on_save is part of model utils and is independent of any particular model. it is written in this way by taking the reference from the edx-platform which is also implemented as a stand-alone method.
https://github.com/openedx/edx-platform/blob/705ff1ba7cebc01728d66a8ab556ba372ecbab54/openedx/core/djangoapps/site_configuration/models.py#L144

AliAdnanSohail · 2023-09-05T12:01:13Z

But we have to update all save() method calling in specific model. If we create something like mixin, we just have to Inherit that mixin and it will override all save() methods

uzairr · 2023-09-05T12:40:36Z

this change is aimed to target super().save() instead of default save().Maybe i am not able to follow the scenario that you are trying to build here. we can discuss it over a call about it.

AfaqShuaib09 · 2023-09-05T14:03:57Z

course_discovery/apps/course_metadata/model_utils.py

+        obj: Any Model instance
+        parent_obj: parent object of the instance obj
+    """
+    if not obj.has_changed:


Aren't we planning to use a field tracker here instead of simple has_changed method to check for changes in attributes and associated models?

FieldTracker() is already implemented there! has_changed is working on top of it.
https://github.com/openedx/course-discovery/blob/master/course_discovery/apps/course_metadata/models.py#L1407

DawoudSheraz · 2023-09-07T09:47:01Z

...y/apps/course_metadata/management/commands/tests/test_deduplicate_course_metadata_history.py

-        # - 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


There is no need to mention an internal ticket in the comments.

DawoudSheraz · 2023-09-07T09:48:12Z

...y/apps/course_metadata/management/commands/tests/test_deduplicate_course_metadata_history.py

        assert courserun2_count_final == 1
-        assert courserun3_count_final == 1
+        assert courserun3_count_final == 3


Please ensure that inline comments are updated to reflect the changes.

yes, a comment was added just above the assert statement(s).

DawoudSheraz · 2023-09-07T09:51:11Z

course_discovery/apps/course_metadata/model_utils.py

+    """
+    Sets the parameter 'skip_history_on_save' if the object is not changed
+    Args:
+        mro: method resolution order of the model


nit: Why is the first argument named mro? It is evaluated to super(). Wouldn't super() take care of MRO based on the inheritance structure?

This code is refactored and now it is much more readable!

DawoudSheraz · 2023-09-07T09:51:53Z

course_discovery/apps/course_metadata/tests/test_models.py

+        course = factories.CourseFactory()
+        course_run = factories.CourseRunFactory()
+        program = factories.ProgramFactory()


Missing Seat & CourseEntitlement

DawoudSheraz · 2023-09-11T09:20:11Z

course_discovery/apps/course_metadata/admin.py

@@ -122,7 +123,7 @@ class ProductValueAdmin(admin.ModelAdmin):


 @admin.register(Course)
-class CourseAdmin(DjangoObjectActions, admin.ModelAdmin):
+class CourseAdmin(DjangoObjectActions, SimpleHistoryAdmin):


Can this increase loading time on admin? There are million of rows on prod, just be careful here.

I have to add this change because of the difference in working nature of django model admin and simple history admin. Changes in this PR can better be observed via SimpleHistoryAdmin.

Your concern is valid.After its deployment, i ll monitor it and will take any followup action accordingly.

DawoudSheraz

While I have not tested it out on the local, CI being green gives a confidence boost on the changes here. Love the mixin approach, much cleaner now.

AliAdnanSohail

LGTM! thanks for considering the suggestion

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

DawoudSheraz reviewed Aug 31, 2023

View reviewed changes

uzairr force-pushed the skip-history-if-not-changed branch 3 times, most recently from 889c701 to ec4b6b3 Compare September 5, 2023 07:08

uzairr changed the title ~~[WIP] feat: skip the history object creation~~ feat: skip the history object creation Sep 5, 2023

AliAdnanSohail reviewed Sep 5, 2023

View reviewed changes

AfaqShuaib09 reviewed Sep 5, 2023

View reviewed changes

uzairr force-pushed the skip-history-if-not-changed branch 3 times, most recently from 3d05017 to 7216ccf Compare September 7, 2023 09:38

DawoudSheraz reviewed Sep 7, 2023

View reviewed changes

uzairr force-pushed the skip-history-if-not-changed branch 7 times, most recently from 32e98c9 to 20bed96 Compare September 8, 2023 14:01

DawoudSheraz reviewed Sep 11, 2023

View reviewed changes

DawoudSheraz approved these changes Sep 11, 2023

View reviewed changes

AliAdnanSohail approved these changes Sep 11, 2023

View reviewed changes

feat: skip the history object creation

c9ff964

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

uzairr force-pushed the skip-history-if-not-changed branch from 20bed96 to c9ff964 Compare September 12, 2023 05:53

uzairr merged commit 5925b57 into master Sep 12, 2023
19 checks passed

uzairr deleted the skip-history-if-not-changed branch September 12, 2023 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: skip the history object creation #4071

feat: skip the history object creation #4071

uzairr commented Aug 31, 2023

DawoudSheraz Aug 31, 2023

AliAdnanSohail left a comment

uzairr commented Sep 5, 2023

AliAdnanSohail commented Sep 5, 2023

uzairr commented Sep 5, 2023

AfaqShuaib09 Sep 5, 2023 •

edited

Loading

uzairr Sep 5, 2023

DawoudSheraz Sep 7, 2023

DawoudSheraz Sep 7, 2023

uzairr Sep 7, 2023

DawoudSheraz Sep 7, 2023

uzairr Sep 7, 2023

DawoudSheraz Sep 7, 2023

DawoudSheraz Sep 11, 2023 •

edited

Loading

uzairr Sep 12, 2023

DawoudSheraz left a comment

AliAdnanSohail left a comment •

edited

Loading

feat: skip the history object creation #4071

feat: skip the history object creation #4071

Conversation

uzairr commented Aug 31, 2023

Choose a reason for hiding this comment

AliAdnanSohail left a comment

Choose a reason for hiding this comment

uzairr commented Sep 5, 2023

AliAdnanSohail commented Sep 5, 2023

uzairr commented Sep 5, 2023

AfaqShuaib09 Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz left a comment

Choose a reason for hiding this comment

AliAdnanSohail left a comment • edited Loading

Choose a reason for hiding this comment

AfaqShuaib09 Sep 5, 2023 •

edited

Loading

DawoudSheraz Sep 11, 2023 •

edited

Loading

AliAdnanSohail left a comment •

edited

Loading