From eca596ea029a7da0eae44b97bad1c88119c5076e Mon Sep 17 00:00:00 2001 From: Muhammad Afaq Shuaib <78806673+AfaqShuaib09@users.noreply.github.com> Date: Tue, 10 Oct 2023 14:17:56 +0500 Subject: [PATCH] feat: add field tracker timebased filtering to organization model (#4138) --- course_discovery/apps/api/filters.py | 1 + course_discovery/apps/api/serializers.py | 4 +-- .../apps/api/tests/test_serializers.py | 1 + .../v1/tests/test_views/test_organizations.py | 28 +++++++++++++++++++ course_discovery/apps/core/models.py | 11 ++++++++ .../apps/course_metadata/admin.py | 7 +++-- .../migrations/0338_auto_20231010_0648.py | 23 +++++++++++++++ .../apps/course_metadata/models.py | 16 ++++++++++- .../apps/course_metadata/tests/test_models.py | 10 +++++++ 9 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 course_discovery/apps/course_metadata/migrations/0338_auto_20231010_0648.py diff --git a/course_discovery/apps/api/filters.py b/course_discovery/apps/api/filters.py index 8c3bee0adb..49e846d2ce 100644 --- a/course_discovery/apps/api/filters.py +++ b/course_discovery/apps/api/filters.py @@ -192,6 +192,7 @@ class Meta: class OrganizationFilter(filters.FilterSet): tags = CharListFilter(field_name='tags__name', lookup_expr='in') uuids = UUIDListFilter() + timestamp = filters.DateTimeFilter(field_name='data_modified_timestamp', lookup_expr='gte') class Meta: model = Organization diff --git a/course_discovery/apps/api/serializers.py b/course_discovery/apps/api/serializers.py index 70f3cbc457..ece80a0ddf 100644 --- a/course_discovery/apps/api/serializers.py +++ b/course_discovery/apps/api/serializers.py @@ -409,9 +409,9 @@ class Meta: model = Organization fields = ( 'uuid', 'key', 'name', 'auto_generate_course_run_keys', 'certificate_logo_image_url', 'logo_image_url', - 'organization_hex_color' + 'organization_hex_color', 'data_modified_timestamp' ) - read_only_fields = ('auto_generate_course_run_keys',) + read_only_fields = ('auto_generate_course_run_keys', 'data_modified_timestamp') class OrganizationSerializer(TaggitSerializer, MinimalOrganizationSerializer): diff --git a/course_discovery/apps/api/tests/test_serializers.py b/course_discovery/apps/api/tests/test_serializers.py index 66f6262f28..8a2a5bc03b 100644 --- a/course_discovery/apps/api/tests/test_serializers.py +++ b/course_discovery/apps/api/tests/test_serializers.py @@ -1727,6 +1727,7 @@ def get_expected_data(cls, organization): 'certificate_logo_image_url': certificate_logo_image_url, 'logo_image_url': logo_image_url, 'organization_hex_color': organization.organization_hex_color, + 'data_modified_timestamp': json_date_format(organization.data_modified_timestamp), } def test_data(self): diff --git a/course_discovery/apps/api/v1/tests/test_views/test_organizations.py b/course_discovery/apps/api/v1/tests/test_views/test_organizations.py index 87daa91673..bc4ef9333c 100644 --- a/course_discovery/apps/api/v1/tests/test_views/test_organizations.py +++ b/course_discovery/apps/api/v1/tests/test_views/test_organizations.py @@ -1,4 +1,5 @@ import uuid +from datetime import datetime from django.urls import reverse from guardian.shortcuts import assign_perm @@ -69,6 +70,33 @@ def test_list(self): assert response.status_code == 200 self.assert_response_data_valid(response, Organization.objects.all()) + def test_list_organization__timestamped_filtering(self): + """ + Verify the organizations returned are filtered when timestamp is provided. + """ + org1 = OrganizationFactory.create(partner=self.partner, name='org1') + org2 = OrganizationFactory.create(partner=self.partner, name='org2') + org3 = OrganizationFactory.create(partner=self.partner, name='org3') + + timestamp_now = datetime.now() + for org in [org1, org2, org3]: + org.description = 'test description update' + org.save() + + url = f'{self.list_path}?timestamp={timestamp_now}' + response = self.client.get(url) + assert response.status_code == 200 + assert len(response.data['results']) == 3 + + timestamp_now = datetime.now().isoformat() + for org in [org1, org2, org3]: + org.save() + + url = f'{self.list_path}?timestamp={timestamp_now}' + response = self.client.get(url) + assert response.status_code == 200 + assert len(response.data['results']) == 0 + def test_list_not_staff(self): """ Verify the endpoint returns a list of all organizations. """ org1 = OrganizationFactory.create(partner=self.partner) diff --git a/course_discovery/apps/core/models.py b/course_discovery/apps/core/models.py index d005d2a1da..2676f58c43 100644 --- a/course_discovery/apps/core/models.py +++ b/course_discovery/apps/core/models.py @@ -8,6 +8,7 @@ from django_extensions.db.models import TimeStampedModel from edx_rest_api_client.client import OAuthAPIClient from guardian.mixins import GuardianUserMixin +from model_utils import FieldTracker from simple_history.models import HistoricalRecords @@ -84,6 +85,7 @@ class Partner(TimeStampedModel): analytics_token = models.CharField(max_length=255, blank=True, verbose_name=_('Analytics Access Token'), default='') history = HistoricalRecords() + field_tracker = FieldTracker() def __str__(self): return self.name @@ -111,6 +113,15 @@ def oauth_api_client(self): timeout=settings.OAUTH_API_TIMEOUT, ) + @property + def has_changed(self): + """ + Returns True if any of the fields tracked by field tracker have changed. + """ + if not self.pk: + return False + return self.field_tracker.changed() + class SalesforceConfiguration(models.Model): partner = models.OneToOneField(Partner, models.CASCADE, related_name='salesforce') diff --git a/course_discovery/apps/course_metadata/admin.py b/course_discovery/apps/course_metadata/admin.py index 5f82334330..4997a0c6a1 100644 --- a/course_discovery/apps/course_metadata/admin.py +++ b/course_discovery/apps/course_metadata/admin.py @@ -613,6 +613,7 @@ class OrganizationAdmin(admin.ModelAdmin): inlines = [OrganizationUserRoleInline, ] list_filter = ('partner',) search_fields = ('uuid', 'name', 'key',) + readonly_fields = ['data_modified_timestamp'] def get_readonly_fields(self, request, obj=None): """ @@ -622,10 +623,10 @@ def get_readonly_fields(self, request, obj=None): flag_name = f'{obj._meta.app_label}.{obj.__class__.__name__}.make_uuid_editable' flag = get_waffle_flag_model().get(flag_name) if flag.is_active(request): - return ['key', ] - return ['uuid', 'key', ] + return ['key', ] + self.readonly_fields + return ['uuid', 'key', ] + self.readonly_fields else: - return ['uuid', ] + return ['uuid', ] + self.readonly_fields @admin.register(Subject) diff --git a/course_discovery/apps/course_metadata/migrations/0338_auto_20231010_0648.py b/course_discovery/apps/course_metadata/migrations/0338_auto_20231010_0648.py new file mode 100644 index 0000000000..193322b1ae --- /dev/null +++ b/course_discovery/apps/course_metadata/migrations/0338_auto_20231010_0648.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.22 on 2023-10-10 06:48 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_metadata', '0337_alter_historicalcourse_options_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='historicalorganization', + name='data_modified_timestamp', + field=models.DateTimeField(blank=True, default=None, help_text='The timestamp of the last time the organization data was modified.', null=True), + ), + migrations.AddField( + model_name='organization', + name='data_modified_timestamp', + field=models.DateTimeField(blank=True, default=None, help_text='The timestamp of the last time the organization data was modified.', null=True), + ), + ] diff --git a/course_discovery/apps/course_metadata/models.py b/course_discovery/apps/course_metadata/models.py index bca49cef83..b90bffc01d 100644 --- a/course_discovery/apps/course_metadata/models.py +++ b/course_discovery/apps/course_metadata/models.py @@ -291,6 +291,12 @@ class Organization(ManageHistoryMixin, CachedMixin, TimeStampedModel): null=True, max_length=6, ) + data_modified_timestamp = models.DateTimeField( + default=None, + null=True, + blank=True, + help_text=_('The timestamp of the last time the organization data was modified.'), + ) # Do not record the slug field in the history table because AutoSlugField is not compatible with # django-simple-history. Background: https://github.com/openedx/course-discovery/pull/332 history = HistoricalRecords(excluded_fields=['slug']) @@ -301,7 +307,7 @@ class Organization(ManageHistoryMixin, CachedMixin, TimeStampedModel): def has_changed(self): if not self.pk: return False - return self.has_model_changed() + return self.has_model_changed(excluded_fields=['data_modified_timestamp']) def clean(self): if not VALID_CHARS_IN_COURSE_NUM_AND_ORG_KEY.match(self.key): @@ -332,12 +338,20 @@ def marketing_url(self): def user_organizations(cls, user): return cls.objects.filter(organization_extension__group__in=user.groups.all()) + def update_data_modified_timestamp(self): + """ + Update the data_modified_timestamp field to the current time if the organization data has changed. + """ + if not self.data_modified_timestamp or self.has_changed: + self.data_modified_timestamp = datetime.datetime.now(pytz.UTC) + def save(self, *args, **kwargs): """ We cache the key here before saving the record so that we can hit the correct endpoint in lms. """ key = self._cache['key'] + self.update_data_modified_timestamp() super().save(*args, **kwargs) key = key or self.key partner = self.partner diff --git a/course_discovery/apps/course_metadata/tests/test_models.py b/course_discovery/apps/course_metadata/tests/test_models.py index 7611e97db2..6d0fa0ac61 100644 --- a/course_discovery/apps/course_metadata/tests/test_models.py +++ b/course_discovery/apps/course_metadata/tests/test_models.py @@ -1778,6 +1778,16 @@ def test_user_organizations(self): assert len(Organization.user_organizations(user)) == 1 + def test_data_modified_timestamp_model_field_change(self): + """ + Verify data modified timestamp changes on direct organization model field changes. + """ + org = factories.OrganizationFactory(name='test org') + data_modified_timestamp = org.data_modified_timestamp + org.description = 'test description' + org.save() + assert data_modified_timestamp < org.data_modified_timestamp + def test_org_enterprise_subscription_inclusion_toggle_course(self): """Test that toggling an org's enterprise_subscription_inclusion value will turn courses in the org on""" org = factories.OrganizationFactory(enterprise_subscription_inclusion=True)