Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: skip the history object creation #4071

Merged
merged 1 commit into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions 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):
Copy link
Contributor

@DawoudSheraz DawoudSheraz Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

form = CourseAdminForm
list_display = ('uuid', 'key', 'key_for_reruns', 'title', 'draft',)
list_filter = ('partner', 'product_source')
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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 = (
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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',)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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):
"""
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def test_normal_case(self):
"""
Expand Down
26 changes: 0 additions & 26 deletions course_discovery/apps/course_metadata/model_utils.py

This file was deleted.

Loading