From c422dec08318dd56d5c6f02bf6bd129b9a5c5265 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Wed, 26 Feb 2020 12:54:24 -0500 Subject: [PATCH 1/2] Revert "Revert "Rename values in SiteConfiguration (2/3)"" This reverts commit b85aa4b3fbe7c9c0561125bc8b2eacae7ed6b9dd. --- common/djangoapps/student/admin.py | 2 +- .../student/tests/test_admin_views.py | 4 +- common/djangoapps/util/tests/test_db.py | 8 ++- lms/djangoapps/branding/tests/test_views.py | 4 +- .../discussion/tests/test_signals.py | 2 - lms/djangoapps/discussion/tests/test_tasks.py | 2 +- lms/djangoapps/instructor/tests/test_api.py | 6 +- .../tests/views/test_instructor_dashboard.py | 2 - .../ace_common/tests/test_tracking.py | 3 - openedx/core/djangoapps/catalog/views.py | 3 +- .../djangoapps/config_model_utils/models.py | 4 +- .../commands/tests/test_notify_credentials.py | 3 +- .../credentials/tests/test_signals.py | 5 +- .../programs/tasks/v1/tests/test_tasks.py | 2 +- .../djangoapps/programs/tests/test_signals.py | 5 +- .../commands/tests/send_email_base.py | 6 +- .../schedules/tests/test_resolvers.py | 7 +-- .../djangoapps/site_configuration/admin.py | 6 +- .../djangoapps/site_configuration/helpers.py | 2 +- .../create_or_update_site_configuration.py | 4 -- ...est_create_or_update_site_configuration.py | 14 ++--- .../0005_copy_values_to_site_values.py | 40 ++++++++++++ .../djangoapps/site_configuration/models.py | 58 ++++++++++------- .../site_configuration/tests/factories.py | 2 +- .../site_configuration/tests/mixins.py | 12 +--- .../tests/test_middleware.py | 6 -- .../site_configuration/tests/test_models.py | 63 +++++++++---------- .../site_configuration/tests/test_util.py | 6 +- .../create_sites_and_configurations.py | 1 - .../tests/test_sync_hubspot_contacts.py | 9 ++- .../content_type_gating/tests/test_models.py | 20 +++--- .../tests/test_models.py | 20 +++--- .../tests/test_discount_restriction_models.py | 9 +-- 33 files changed, 171 insertions(+), 169 deletions(-) create mode 100644 openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py diff --git a/common/djangoapps/student/admin.py b/common/djangoapps/student/admin.py index 55e3407eba..cb740c894e 100644 --- a/common/djangoapps/student/admin.py +++ b/common/djangoapps/student/admin.py @@ -465,7 +465,7 @@ class AllowedAuthUserForm(forms.ModelForm): if not allowed_site_email_domain: raise forms.ValidationError( _("Please add a key/value 'THIRD_PARTY_AUTH_ONLY_DOMAIN/{site_email_domain}' in SiteConfiguration " - "model's values field.") + "model's site_values field.") ) elif email_domain != allowed_site_email_domain: raise forms.ValidationError( diff --git a/common/djangoapps/student/tests/test_admin_views.py b/common/djangoapps/student/tests/test_admin_views.py index 7a5f363b3e..21840fe245 100644 --- a/common/djangoapps/student/tests/test_admin_views.py +++ b/common/djangoapps/student/tests/test_admin_views.py @@ -454,7 +454,7 @@ class AllowedAuthUserFormTest(SiteMixin, TestCase): def _update_site_configuration(self): """ Updates the site's configuration """ - self.site.configuration.values = {'THIRD_PARTY_AUTH_ONLY_DOMAIN': self.email_domain_name} + self.site.configuration.site_values = {'THIRD_PARTY_AUTH_ONLY_DOMAIN': self.email_domain_name} self.site.configuration.save() def _assert_form(self, site, email, is_valid_form=False): @@ -480,7 +480,7 @@ class AllowedAuthUserFormTest(SiteMixin, TestCase): self.assertEqual( error, "Please add a key/value 'THIRD_PARTY_AUTH_ONLY_DOMAIN/{site_email_domain}' in SiteConfiguration " - "model's values field." + "model's site_values field." ) def test_form_with_invalid_domain_name(self): diff --git a/common/djangoapps/util/tests/test_db.py b/common/djangoapps/util/tests/test_db.py index 8fdcf40b9f..565ad10d60 100644 --- a/common/djangoapps/util/tests/test_db.py +++ b/common/djangoapps/util/tests/test_db.py @@ -222,7 +222,9 @@ class MigrationTests(TestCase): Tests for migrations. """ - @unittest.skip("Need to skip as part of renaming a field in schedules app. This will be unskipped in DE-1825") + @unittest.skip( + "Need to skip as part of renaming a field in schedules app. This will be unskipped in DE-1825. ALSO need to skip as part of renaming a field in the site_configuration app. This will be unskipped in DENG-18." + ) @override_settings(MIGRATION_MODULES={}) def test_migrations_are_in_sync(self): """ @@ -237,6 +239,6 @@ class MigrationTests(TestCase): release afterwards, this test doesn't fail. """ out = StringIO() - call_command('makemigrations', dry_run=True, verbosity=3, stdout=out) + call_command("makemigrations", dry_run=True, verbosity=3, stdout=out) output = out.getvalue() - self.assertIn('No changes detected', output) + self.assertIn("No changes detected", output) diff --git a/lms/djangoapps/branding/tests/test_views.py b/lms/djangoapps/branding/tests/test_views.py index 8b2ffc95d2..1f11b2da81 100644 --- a/lms/djangoapps/branding/tests/test_views.py +++ b/lms/djangoapps/branding/tests/test_views.py @@ -297,7 +297,7 @@ class TestIndex(SiteMixin, TestCase): response = self.client.get(reverse("root")) self.assertRedirects( response, - self.site_configuration_other.values["MKTG_URLS"]["ROOT"], + self.site_configuration_other.site_values["MKTG_URLS"]["ROOT"], fetch_redirect_response=False ) @@ -309,4 +309,4 @@ class TestIndex(SiteMixin, TestCase): self.use_site(self.site_other) self.client.login(username=self.user.username, password="password") response = self.client.get(reverse("dashboard")) - self.assertIn(self.site_configuration_other.values["MKTG_URLS"]["ROOT"], response.content.decode('utf-8')) + self.assertIn(self.site_configuration_other.site_values["MKTG_URLS"]["ROOT"], response.content.decode('utf-8')) diff --git a/lms/djangoapps/discussion/tests/test_signals.py b/lms/djangoapps/discussion/tests/test_signals.py index e303bc11c9..14d5887144 100644 --- a/lms/djangoapps/discussion/tests/test_signals.py +++ b/lms/djangoapps/discussion/tests/test_signals.py @@ -30,7 +30,6 @@ class SendMessageHandlerTestCase(TestCase): site_config = SiteConfigurationFactory.create(site=self.site) enable_notifications_cfg = {ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY: True} site_config.site_values = enable_notifications_cfg - site_config.values = enable_notifications_cfg site_config.save() mock_get_current_site.return_value = self.site signals.comment_created.send(sender=self.sender, user=self.user, post=self.post) @@ -60,7 +59,6 @@ class SendMessageHandlerTestCase(TestCase): site_config = SiteConfigurationFactory.create(site=self.site) enable_notifications_cfg = {ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY: False} site_config.site_values = enable_notifications_cfg - site_config.values = enable_notifications_cfg site_config.save() mock_get_current_site.return_value = self.site signals.comment_created.send(sender=self.sender, user=self.user, post=self.post) diff --git a/lms/djangoapps/discussion/tests/test_tasks.py b/lms/djangoapps/discussion/tests/test_tasks.py index c6a8b64ebb..4161792a98 100644 --- a/lms/djangoapps/discussion/tests/test_tasks.py +++ b/lms/djangoapps/discussion/tests/test_tasks.py @@ -193,7 +193,7 @@ class TaskTestCase(ModuleStoreTestCase): comment = cc.Comment.find(id=self.comment['id']).retrieve() site = Site.objects.get_current() site_config = SiteConfigurationFactory.create(site=site) - site_config.values[ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY] = True + site_config.site_values[ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY] = True site_config.save() with mock.patch('lms.djangoapps.discussion.signals.handlers.get_current_site', return_value=site): comment_created.send(sender=None, user=user, post=comment) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 5fa668f87f..4424cd6338 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -4000,8 +4000,8 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm self.assertEqual(response.status_code, 400) def test_send_email_with_site_template_and_from_addr(self): - site_email = self.site_configuration.values.get('course_email_from_addr') - site_template = self.site_configuration.values.get('course_email_template_name') + site_email = self.site_configuration.site_values.get('course_email_from_addr') + site_template = self.site_configuration.site_values.get('course_email_template_name') CourseEmailTemplate.objects.create(name=site_template) url = reverse('send_email', kwargs={'course_id': text_type(self.course.id)}) response = self.client.post(url, self.full_test_message) @@ -4019,7 +4019,7 @@ class TestInstructorSendEmail(SiteMixin, SharedModuleStoreTestCase, LoginEnrollm org_email = 'fake_org@example.com' org_template = 'fake_org_email_template' CourseEmailTemplate.objects.create(name=org_template) - self.site_configuration.values.update({ + self.site_configuration.site_values.update({ 'course_email_from_addr': {self.course.id.org: org_email}, 'course_email_template_name': {self.course.id.org: org_template} }) diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 58d29ae5c1..3bbfdbf54f 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -169,7 +169,6 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssT SiteConfiguration.objects.create( site=site, site_values=configuration_values, - values=configuration_values, enabled=True ) @@ -202,7 +201,6 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssT SiteConfiguration.objects.create( site=site, site_values=configuration_values, - values=configuration_values, enabled=True ) url = reverse( diff --git a/openedx/core/djangoapps/ace_common/tests/test_tracking.py b/openedx/core/djangoapps/ace_common/tests/test_tracking.py index 1412acf252..6201362055 100644 --- a/openedx/core/djangoapps/ace_common/tests/test_tracking.py +++ b/openedx/core/djangoapps/ace_common/tests/test_tracking.py @@ -124,9 +124,6 @@ class TestGoogleAnalyticsTrackingPixel(QueryStringAssertionMixin, CacheIsolation site_config = SiteConfigurationFactory.create( site_values=dict( GOOGLE_ANALYTICS_ACCOUNT='UA-654321-1' - ), - values=dict( - GOOGLE_ANALYTICS_ACCOUNT='UA-654321-1' ) ) pixel = GoogleAnalyticsTrackingPixel(site=site_config.site) diff --git a/openedx/core/djangoapps/catalog/views.py b/openedx/core/djangoapps/catalog/views.py index 41874667c1..a6c1ba3704 100644 --- a/openedx/core/djangoapps/catalog/views.py +++ b/openedx/core/djangoapps/catalog/views.py @@ -27,8 +27,7 @@ def cache_programs(request): SiteConfiguration.objects.create( site=request.site, enabled=True, - site_values={"COURSE_CATALOG_API_URL": "{catalog_url}/api/v1/".format(catalog_url=CATALOG_STUB_URL)}, - values={"COURSE_CATALOG_API_URL": "{catalog_url}/api/v1/".format(catalog_url=CATALOG_STUB_URL)} + site_values={"COURSE_CATALOG_API_URL": "{catalog_url}/api/v1/".format(catalog_url=CATALOG_STUB_URL)} ) if settings.FEATURES.get('EXPOSE_CACHE_PROGRAMS_ENDPOINT'): diff --git a/openedx/core/djangoapps/config_model_utils/models.py b/openedx/core/djangoapps/config_model_utils/models.py index abf6b5a038..5a46541a93 100644 --- a/openedx/core/djangoapps/config_model_utils/models.py +++ b/openedx/core/djangoapps/config_model_utils/models.py @@ -244,7 +244,7 @@ class StackedConfigurationModel(ConfigurationModel): """ all_courses = CourseOverview.objects.all() all_site_configs = SiteConfiguration.objects.filter( - values__contains='course_org_filter', enabled=True + site_values__contains='course_org_filter', enabled=True ).select_related('site') try: @@ -254,7 +254,7 @@ class StackedConfigurationModel(ConfigurationModel): sites_by_org = defaultdict(lambda: default_site) site_cfg_org_filters = ( - (site_cfg.site, site_cfg.values['course_org_filter']) + (site_cfg.site, site_cfg.site_values['course_org_filter']) for site_cfg in all_site_configs ) sites_by_org.update({ diff --git a/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py b/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py index d84ad0669b..63ed6e7101 100644 --- a/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py +++ b/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py @@ -157,8 +157,7 @@ class TestNotifyCredentials(TestCase): @mock.patch(COMMAND_MODULE + '.handle_cert_change') def test_site(self, mock_grade_interesting, mock_cert_change): site_config = SiteConfigurationFactory.create( - site_values={'course_org_filter': ['testX']}, - values={'course_org_filter': ['testX']}, + site_values={'course_org_filter': ['testX']} ) call_command(Command(), '--site', site_config.site.domain, '--start-date', '2017-01-01') diff --git a/openedx/core/djangoapps/credentials/tests/test_signals.py b/openedx/core/djangoapps/credentials/tests/test_signals.py index ef2a8196d5..311be58cc0 100644 --- a/openedx/core/djangoapps/credentials/tests/test_signals.py +++ b/openedx/core/djangoapps/credentials/tests/test_signals.py @@ -110,8 +110,7 @@ class TestCredentialsSignalsSendGrade(TestCase): def test_send_grade_records_enabled(self, _mock_is_course_run_in_a_program, mock_send_grade_to_credentials, _mock_is_learner_issuance_enabled): site_config = SiteConfigurationFactory.create( - site_values={'course_org_filter': [self.key.org]}, - values={'course_org_filter': [self.key.org]}, + site_values={'course_org_filter': [self.key.org]} ) # Correctly sent @@ -120,7 +119,7 @@ class TestCredentialsSignalsSendGrade(TestCase): mock_send_grade_to_credentials.delay.reset_mock() # Correctly not sent - site_config.values['ENABLE_LEARNER_RECORDS'] = False + site_config.site_values['ENABLE_LEARNER_RECORDS'] = False site_config.save() send_grade_if_interesting(self.user, self.key, 'verified', 'downloadable', None, None) self.assertFalse(mock_send_grade_to_credentials.delay.called) diff --git a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py index 6fa7c40de1..230dc13862 100644 --- a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py @@ -196,7 +196,7 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo mock_get_certified_programs.return_value = [1] # programs to be skipped - self.site_configuration.values = { + self.site_configuration.site_values = { "programs_without_certificates": [2] } self.site_configuration.save() diff --git a/openedx/core/djangoapps/programs/tests/test_signals.py b/openedx/core/djangoapps/programs/tests/test_signals.py index 82c5299cb7..364dc089f6 100644 --- a/openedx/core/djangoapps/programs/tests/test_signals.py +++ b/openedx/core/djangoapps/programs/tests/test_signals.py @@ -146,8 +146,7 @@ class CertChangedReceiverTest(TestCase): mock_is_learner_issuance_enabled.return_value = True site_config = SiteConfigurationFactory.create( - site_values={'course_org_filter': ['edX']}, - values={'course_org_filter': ['edX']}, + site_values={'course_org_filter': ['edX']} ) # Correctly sent @@ -156,7 +155,7 @@ class CertChangedReceiverTest(TestCase): mock_task.reset_mock() # Correctly not sent - site_config.values['ENABLE_LEARNER_RECORDS'] = False + site_config.site_values['ENABLE_LEARNER_RECORDS'] = False site_config.save() handle_course_cert_changed(**self.signal_kwargs) self.assertFalse(mock_task.called) diff --git a/openedx/core/djangoapps/schedules/management/commands/tests/send_email_base.py b/openedx/core/djangoapps/schedules/management/commands/tests/send_email_base.py index 9da5d0e811..b76054d8fb 100644 --- a/openedx/core/djangoapps/schedules/management/commands/tests/send_email_base.py +++ b/openedx/core/djangoapps/schedules/management/commands/tests/send_email_base.py @@ -308,12 +308,10 @@ class ScheduleSendEmailTestMixin(FilteredQueryCountMixin): filtered_org = 'filtered_org' unfiltered_org = 'unfiltered_org' this_config = SiteConfigurationFactory.create( - site_values={'course_org_filter': this_org_list}, - values={'course_org_filter': this_org_list} + site_values={'course_org_filter': this_org_list} ) other_config = SiteConfigurationFactory.create( - site_values={'course_org_filter': other_org_list}, - values={'course_org_filter': other_org_list} + site_values={'course_org_filter': other_org_list} ) for config in (this_config, other_config): diff --git a/openedx/core/djangoapps/schedules/tests/test_resolvers.py b/openedx/core/djangoapps/schedules/tests/test_resolvers.py index 4c792b860f..aa60ec7cab 100644 --- a/openedx/core/djangoapps/schedules/tests/test_resolvers.py +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -61,7 +61,7 @@ class TestBinnedSchedulesBaseResolver(SchedulesResolverTestMixin, TestCase): 'course1' ) def test_get_course_org_filter_equal(self, course_org_filter): - self.site_config.values['course_org_filter'] = course_org_filter + self.site_config.site_values['course_org_filter'] = course_org_filter self.site_config.save() mock_query = Mock() result = self.resolver.filter_by_org(mock_query) @@ -73,7 +73,7 @@ class TestBinnedSchedulesBaseResolver(SchedulesResolverTestMixin, TestCase): (['course1', 'course2'], ['course1', 'course2']) ) def test_get_course_org_filter_include__in(self, course_org_filter, expected_org_list): - self.site_config.values['course_org_filter'] = course_org_filter + self.site_config.site_values['course_org_filter'] = course_org_filter self.site_config.save() mock_query = Mock() result = self.resolver.filter_by_org(mock_query) @@ -88,8 +88,7 @@ class TestBinnedSchedulesBaseResolver(SchedulesResolverTestMixin, TestCase): ) def test_get_course_org_filter_exclude__in(self, course_org_filter, expected_org_list): SiteConfigurationFactory.create( - site_values={'course_org_filter': course_org_filter}, - values={'course_org_filter': course_org_filter}, + site_values={'course_org_filter': course_org_filter} ) mock_query = Mock() result = self.resolver.filter_by_org(mock_query) diff --git a/openedx/core/djangoapps/site_configuration/admin.py b/openedx/core/djangoapps/site_configuration/admin.py index 6fda05f472..d8b52647b6 100644 --- a/openedx/core/djangoapps/site_configuration/admin.py +++ b/openedx/core/djangoapps/site_configuration/admin.py @@ -12,8 +12,8 @@ class SiteConfigurationAdmin(admin.ModelAdmin): """ Admin interface for the SiteConfiguration object. """ - list_display = ('site', 'enabled', 'values') - search_fields = ('site__domain', 'values') + list_display = ('site', 'enabled', 'site_values') + search_fields = ('site__domain', 'site_values') class Meta(object): """ @@ -29,7 +29,7 @@ class SiteConfigurationHistoryAdmin(admin.ModelAdmin): Admin interface for the SiteConfigurationHistory object. """ list_display = ('site', 'enabled', 'created', 'modified') - search_fields = ('site__domain', 'values', 'created', 'modified') + search_fields = ('site__domain', 'site_values', 'created', 'modified') ordering = ['-created'] diff --git a/openedx/core/djangoapps/site_configuration/helpers.py b/openedx/core/djangoapps/site_configuration/helpers.py index 07cb3ae9a9..45663497f3 100644 --- a/openedx/core/djangoapps/site_configuration/helpers.py +++ b/openedx/core/djangoapps/site_configuration/helpers.py @@ -53,7 +53,7 @@ def has_configuration_override(name): (bool): True if given key is present in the configuration. """ configuration = get_current_site_configuration() - if configuration and name in configuration.values: + if configuration and name in configuration.site_values: return True return False diff --git a/openedx/core/djangoapps/site_configuration/management/commands/create_or_update_site_configuration.py b/openedx/core/djangoapps/site_configuration/management/commands/create_or_update_site_configuration.py index 2c670b513f..edcb0f19a4 100644 --- a/openedx/core/djangoapps/site_configuration/management/commands/create_or_update_site_configuration.py +++ b/openedx/core/djangoapps/site_configuration/management/commands/create_or_update_site_configuration.py @@ -103,10 +103,6 @@ class Command(BaseCommand): site_configuration_values = configuration or config_file_data if site_configuration_values: - if site_configuration.values: - site_configuration.values.update(site_configuration_values) - else: - site_configuration.values = site_configuration_values if site_configuration.site_values: site_configuration.site_values.update(site_configuration_values) else: diff --git a/openedx/core/djangoapps/site_configuration/management/commands/tests/test_create_or_update_site_configuration.py b/openedx/core/djangoapps/site_configuration/management/commands/tests/test_create_or_update_site_configuration.py index 0a0d4974ad..9a686ae0cd 100644 --- a/openedx/core/djangoapps/site_configuration/management/commands/tests/test_create_or_update_site_configuration.py +++ b/openedx/core/djangoapps/site_configuration/management/commands/tests/test_create_or_update_site_configuration.py @@ -56,7 +56,7 @@ class CreateOrUpdateSiteConfigurationTest(TestCase): def create_fixture_site_configuration(self, enabled): SiteConfiguration.objects.update_or_create( site=self.site, - defaults={'enabled': enabled, 'values': {'ABC': 'abc', 'B': 'b'}} + defaults={'enabled': enabled, 'site_values': {'ABC': 'abc', 'B': 'b'}} ) def test_command_no_args(self): @@ -105,7 +105,7 @@ class CreateOrUpdateSiteConfigurationTest(TestCase): call_command(self.command, *self.site_id_arg) site_configuration = SiteConfiguration.objects.get(site=self.site) - self.assertFalse(site_configuration.values) + self.assertFalse(site_configuration.site_values) self.assertFalse(site_configuration.enabled) def test_both_enabled_disabled_flags(self): @@ -126,7 +126,7 @@ class CreateOrUpdateSiteConfigurationTest(TestCase): self.assert_site_configuration_does_not_exist() call_command(self.command, '--{}'.format(flag), *self.site_id_arg) site_configuration = SiteConfiguration.objects.get(site=self.site) - self.assertFalse(site_configuration.values) + self.assertFalse(site_configuration.site_values) self.assertEqual(enabled, site_configuration.enabled) def test_site_configuration_created_with_parameters(self): @@ -136,7 +136,7 @@ class CreateOrUpdateSiteConfigurationTest(TestCase): self.assert_site_configuration_does_not_exist() call_command(self.command, '--configuration', json.dumps(self.input_configuration), *self.site_id_arg) site_configuration = self.get_site_configuration() - self.assertDictEqual(site_configuration.values, self.input_configuration) + self.assertDictEqual(site_configuration.site_values, self.input_configuration) def test_site_configuration_created_with_json_file_parameters(self): """ @@ -145,7 +145,7 @@ class CreateOrUpdateSiteConfigurationTest(TestCase): self.assert_site_configuration_does_not_exist() call_command(self.command, '-f', str(self.json_file_path.abspath()), *self.site_id_arg) site_configuration = self.get_site_configuration() - self.assertEqual(site_configuration.values, {'ABC': 123, 'XYZ': '789'}) + self.assertEqual(site_configuration.site_values, {'ABC': 123, 'XYZ': '789'}) @ddt.data(True, False) def test_site_configuration_updated_with_parameters(self, enabled): @@ -156,7 +156,7 @@ class CreateOrUpdateSiteConfigurationTest(TestCase): call_command(self.command, '--configuration', json.dumps(self.input_configuration), *self.site_id_arg) site_configuration = self.get_site_configuration() self.assertEqual( - site_configuration.values, + site_configuration.site_values, {'ABC': 123, 'B': 'b', 'FEATURE_FLAG': True, 'SERVICE_URL': 'https://foo.bar'} ) self.assertEqual(site_configuration.enabled, enabled) @@ -172,5 +172,5 @@ class CreateOrUpdateSiteConfigurationTest(TestCase): expected_site_configuration = {'ABC': 'abc', 'B': 'b'} with codecs.open(self.json_file_path, encoding='utf-8') as f: expected_site_configuration.update(json.load(f)) - self.assertEqual(site_configuration.values, expected_site_configuration) + self.assertEqual(site_configuration.site_values, expected_site_configuration) self.assertEqual(site_configuration.enabled, enabled) diff --git a/openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py b/openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py new file mode 100644 index 0000000000..8993f724fd --- /dev/null +++ b/openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py @@ -0,0 +1,40 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.27 on 2020-01-03 20:57 +from __future__ import unicode_literals + +from django.db import migrations + + +def copy_column_values(apps, schema_editor): + """ + Copy the contents of the values field into the site_values field in both + SiteConfiguration and SiteConfigurationHistory. + """ + # Update all values in the model. + SiteConfiguration = apps.get_model('site_configuration', 'SiteConfiguration') + for site_configuration in SiteConfiguration.objects.all(): + site_configuration.site_values = site_configuration.values + # It would be incorrect to record these saves in the history table since it is + # just backfilling data. Use save_without_historical_record() instead of + # save(). + site_configuration.save_without_historical_record() + + # Update all values in the history model. + SiteConfigurationHistory = apps.get_model('site_configuration', 'SiteConfigurationHistory') + for historical_site_configuration in SiteConfigurationHistory.objects.all(): + historical_site_configuration.site_values = historical_site_configuration.values + historical_site_configuration.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('site_configuration', '0004_add_site_values_field'), + ] + + operations = [ + migrations.RunPython( + copy_column_values, + reverse_code=migrations.RunPython.noop, # Allow reverse migrations, but make it a no-op. + ), + ] diff --git a/openedx/core/djangoapps/site_configuration/models.py b/openedx/core/djangoapps/site_configuration/models.py index 683fae6af5..e7c64fbd69 100644 --- a/openedx/core/djangoapps/site_configuration/models.py +++ b/openedx/core/djangoapps/site_configuration/models.py @@ -40,13 +40,6 @@ class SiteConfiguration(models.Model): default=dict, load_kwargs={'object_pairs_hook': collections.OrderedDict} ) - # TODO: Delete this deprecated field during the later stages of the rollout - # which renames 'values' to 'site_values'. - values = JSONField( - null=False, - blank=True, - load_kwargs={'object_pairs_hook': collections.OrderedDict} - ) def __str__(self): return u"".format(site=self.site) # xss-lint: disable=python-wrap-html @@ -69,7 +62,7 @@ class SiteConfiguration(models.Model): """ if self.enabled: try: - return self.values.get(name, default) + return self.site_values.get(name, default) except AttributeError as error: logger.exception(u'Invalid JSON data. \n [%s]', error) else: @@ -87,7 +80,7 @@ class SiteConfiguration(models.Model): org (str): Org to use to filter SiteConfigurations select_related (list or None): A list of values to pass as arguments to select_related """ - query = cls.objects.filter(values__contains=org, enabled=True).all() + query = cls.objects.filter(site_values__contains=org, enabled=True).all() if select_related is not None: query = query.select_related(*select_related) for configuration in query: @@ -131,7 +124,7 @@ class SiteConfiguration(models.Model): """ org_filter_set = set() - for configuration in cls.objects.filter(values__contains='course_org_filter', enabled=True).all(): + for configuration in cls.objects.filter(site_values__contains='course_org_filter', enabled=True).all(): course_org_filter = configuration.get_value('course_org_filter', []) if not isinstance(course_org_filter, list): course_org_filter = [course_org_filter] @@ -148,6 +141,21 @@ class SiteConfiguration(models.Model): """ return org in cls.get_all_orgs() + def save_without_historical_record(self, *args, **kwargs): + """ + Save model without saving a historical record + + Make sure you know what you're doing before you use this method. + + Note: this method is copied verbatim from django-simple-history. + """ + self.skip_history_when_saving = True # pylint: disable=attribute-defined-outside-init + try: + ret = self.save(*args, **kwargs) + finally: + del self.skip_history_when_saving + return ret + @python_2_unicode_compatible class SiteConfigurationHistory(TimeStampedModel): @@ -168,13 +176,6 @@ class SiteConfigurationHistory(TimeStampedModel): blank=True, load_kwargs={'object_pairs_hook': collections.OrderedDict} ) - # TODO: Delete this deprecated field during the later stages of the rollout - # which renames 'values' to 'site_values'. - values = JSONField( - null=False, - blank=True, - load_kwargs={'object_pairs_hook': collections.OrderedDict} - ) class Meta: get_latest_by = 'modified' @@ -192,18 +193,27 @@ class SiteConfigurationHistory(TimeStampedModel): @receiver(post_save, sender=SiteConfiguration) -def update_site_configuration_history(sender, instance, **kwargs): # pylint: disable=unused-argument +def update_site_configuration_history(sender, instance, created, **kwargs): # pylint: disable=unused-argument """ Add site configuration changes to site configuration history. + Recording history on updates and deletes can be skipped by first setting + the `skip_history_when_saving` attribute on the instace, e.g.: + + site_config.skip_history_when_saving = True + site_config.save() + Args: sender: sender of the signal i.e. SiteConfiguration model instance: SiteConfiguration instance associated with the current signal + created (bool): True if a new record was created. **kwargs: extra key word arguments """ - SiteConfigurationHistory.objects.create( - site=instance.site, - site_values=instance.values, - values=instance.values, - enabled=instance.enabled, - ) + # Skip writing history when asked by the caller. This skip feature only + # works for non-creates. + if created or not hasattr(instance, "skip_history_when_saving"): + SiteConfigurationHistory.objects.create( + site=instance.site, + site_values=instance.site_values, + enabled=instance.enabled, + ) diff --git a/openedx/core/djangoapps/site_configuration/tests/factories.py b/openedx/core/djangoapps/site_configuration/tests/factories.py index 4943ca3670..db1bb7377f 100644 --- a/openedx/core/djangoapps/site_configuration/tests/factories.py +++ b/openedx/core/djangoapps/site_configuration/tests/factories.py @@ -33,5 +33,5 @@ class SiteConfigurationFactory(DjangoModelFactory): site = SubFactory(SiteFactory) @lazy_attribute - def values(self): + def site_values(self): return {} diff --git a/openedx/core/djangoapps/site_configuration/tests/mixins.py b/openedx/core/djangoapps/site_configuration/tests/mixins.py index 95294d2891..f08aca3541 100644 --- a/openedx/core/djangoapps/site_configuration/tests/mixins.py +++ b/openedx/core/djangoapps/site_configuration/tests/mixins.py @@ -22,9 +22,7 @@ class SiteMixin(object): } self.site_configuration = SiteConfigurationFactory.create( site=self.site, - site_values=site_config, - # TODO: Remove this deprecated value eventually. - values=site_config + site_values=site_config ) self.site_other = SiteFactory.create( @@ -44,9 +42,7 @@ class SiteMixin(object): } self.site_configuration_other = SiteConfigurationFactory.create( site=self.site_other, - site_values=site_config_other, - # TODO: Remove this deprecated value eventually. - values=site_config_other + site_values=site_config_other ) # Initialize client with default site domain @@ -62,9 +58,7 @@ class SiteMixin(object): ) __ = SiteConfigurationFactory.create( site=site, - site_values=site_configuration_values, - # TODO: Remove this deprecated value eventually. - values=site_configuration_values + site_values=site_configuration_values ) self.use_site(site) return site diff --git a/openedx/core/djangoapps/site_configuration/tests/test_middleware.py b/openedx/core/djangoapps/site_configuration/tests/test_middleware.py index c1dcc8dd37..124ba1f4a0 100644 --- a/openedx/core/djangoapps/site_configuration/tests/test_middleware.py +++ b/openedx/core/djangoapps/site_configuration/tests/test_middleware.py @@ -40,9 +40,6 @@ class SessionCookieDomainTests(TestCase): site=self.site, site_values={ "SESSION_COOKIE_DOMAIN": self.site.domain, - }, - values={ - "SESSION_COOKIE_DOMAIN": self.site.domain, } ) @@ -78,9 +75,6 @@ class SessionCookieDomainSiteConfigurationOverrideTests(TestCase): site=self.site, site_values={ "SESSION_COOKIE_DOMAIN": self.site.domain, - }, - values={ - "SESSION_COOKIE_DOMAIN": self.site.domain, } ) self.client = Client() diff --git a/openedx/core/djangoapps/site_configuration/tests/test_models.py b/openedx/core/djangoapps/site_configuration/tests/test_models.py index 4b69d57f58..ea83f8c271 100644 --- a/openedx/core/djangoapps/site_configuration/tests/test_models.py +++ b/openedx/core/djangoapps/site_configuration/tests/test_models.py @@ -83,8 +83,6 @@ class SiteConfigurationTests(TestCase): ) site_configuration.site_values = {'test': 'test'} - # TODO: Remove this deprecated value eventually. - site_configuration.values = {'test': 'test'} site_configuration.save() # Verify an entry to SiteConfigurationHistory was added. @@ -92,9 +90,30 @@ class SiteConfigurationTests(TestCase): site=site_configuration.site, ).all() - # Make sure two entries (one for save and one for update) are saved for SiteConfiguration + # Make sure two entries (one for create and one for update) are saved for SiteConfiguration self.assertEqual(len(site_configuration_history), 2) + def test_site_configuration_post_update_receiver_with_skip(self): + """ + Test that and entry is NOT added to SiteConfigurationHistory each time a + SiteConfiguration is updated with save_without_historical_record(). + """ + # add SiteConfiguration to database + site_configuration = SiteConfigurationFactory.create( + site=self.site, + ) + + site_configuration.site_values = {'test': 'test'} + site_configuration.save_without_historical_record() # Instead of save(). + + # Verify an entry to SiteConfigurationHistory was NOT added. + site_configuration_history = SiteConfigurationHistory.objects.filter( + site=site_configuration.site, + ).all() + + # Make sure one entry (one for create and NONE for update) is saved for SiteConfiguration + self.assertEqual(len(site_configuration_history), 1) + def test_no_entry_is_saved_for_errors(self): """ Test that and entry is not added to SiteConfigurationHistory if there is an error while @@ -133,9 +152,7 @@ class SiteConfigurationTests(TestCase): # add SiteConfiguration to database site_configuration = SiteConfigurationFactory.create( site=self.site, - site_values=self.test_config1, - # TODO: Remove this deprecated value eventually. - values=self.test_config1, + site_values=self.test_config1 ) # Make sure entry is saved and retrieved correctly @@ -183,9 +200,7 @@ class SiteConfigurationTests(TestCase): # add SiteConfiguration to database site_configuration = SiteConfigurationFactory.create( site=self.site, - site_values=invalid_data, - # TODO: Remove this deprecated value eventually. - values=invalid_data, + site_values=invalid_data ) # make sure get_value logs an error for invalid json data @@ -206,15 +221,11 @@ class SiteConfigurationTests(TestCase): # add SiteConfiguration to database SiteConfigurationFactory.create( site=self.site, - site_values=self.test_config1, - # TODO: Remove this deprecated value eventually. - values=self.test_config1, + site_values=self.test_config1 ) SiteConfigurationFactory.create( site=self.site2, - site_values=self.test_config2, - # TODO: Remove this deprecated value eventually. - values=self.test_config2, + site_values=self.test_config2 ) # Make sure entry is saved and retrieved correctly @@ -295,15 +306,11 @@ class SiteConfigurationTests(TestCase): # add SiteConfiguration to database config1 = SiteConfigurationFactory.create( site=self.site, - site_values=self.test_config1, - # TODO: Remove this deprecated value eventually. - values=self.test_config1, + site_values=self.test_config1 ) config2 = SiteConfigurationFactory.create( site=self.site2, - site_values=self.test_config2, - # TODO: Remove this deprecated value eventually. - values=self.test_config2, + site_values=self.test_config2 ) # Make sure entry is saved and retrieved correctly @@ -328,15 +335,11 @@ class SiteConfigurationTests(TestCase): # add SiteConfiguration to database SiteConfigurationFactory.create( site=self.site, - site_values=self.test_config1, - # TODO: Remove this deprecated value eventually. - values=self.test_config1, + site_values=self.test_config1 ) SiteConfigurationFactory.create( site=self.site2, - site_values=self.test_config2, - # TODO: Remove this deprecated value eventually. - values=self.test_config2, + site_values=self.test_config2 ) # Test that the default value is returned if the value for the given key is not found in the configuration @@ -351,15 +354,11 @@ class SiteConfigurationTests(TestCase): SiteConfigurationFactory.create( site=self.site, site_values=self.test_config1, - # TODO: Remove this deprecated value eventually. - values=self.test_config1, enabled=False, ) SiteConfigurationFactory.create( site=self.site2, - site_values=self.test_config2, - # TODO: Remove this deprecated value eventually. - values=self.test_config2, + site_values=self.test_config2 ) # Test that the default value is returned if the value for the given key is not found in the configuration diff --git a/openedx/core/djangoapps/site_configuration/tests/test_util.py b/openedx/core/djangoapps/site_configuration/tests/test_util.py index 6c13a5fadc..6bfb5b760e 100644 --- a/openedx/core/djangoapps/site_configuration/tests/test_util.py +++ b/openedx/core/djangoapps/site_configuration/tests/test_util.py @@ -29,11 +29,10 @@ def with_site_configuration(domain="test.localhost", configuration=None): site, __ = Site.objects.get_or_create(domain=domain, name=domain) site_configuration, created = SiteConfiguration.objects.get_or_create( site=site, - defaults={"enabled": True, "site_values": configuration, "values": configuration}, + defaults={"enabled": True, "site_values": configuration}, ) if not created: site_configuration.site_values = configuration - site_configuration.values = configuration site_configuration.save() with patch('openedx.core.djangoapps.site_configuration.helpers.get_current_site_configuration', @@ -57,11 +56,10 @@ def with_site_configuration_context(domain="test.localhost", configuration=None) site, __ = Site.objects.get_or_create(domain=domain, name=domain) site_configuration, created = SiteConfiguration.objects.get_or_create( site=site, - defaults={"enabled": True, "site_values": configuration, "values": configuration}, + defaults={"enabled": True, "site_values": configuration}, ) if not created: site_configuration.site_values = configuration - site_configuration.values = configuration site_configuration.save() with patch('openedx.core.djangoapps.site_configuration.helpers.get_current_site_configuration', diff --git a/openedx/core/djangoapps/theming/management/commands/create_sites_and_configurations.py b/openedx/core/djangoapps/theming/management/commands/create_sites_and_configurations.py index 41e3ad59de..e83a2b38b8 100644 --- a/openedx/core/djangoapps/theming/management/commands/create_sites_and_configurations.py +++ b/openedx/core/djangoapps/theming/management/commands/create_sites_and_configurations.py @@ -116,7 +116,6 @@ class Command(BaseCommand): SiteConfiguration.objects.create( site=site, site_values=site_configuration, - values=site_configuration, enabled=True ) else: diff --git a/openedx/core/djangoapps/user_api/management/tests/test_sync_hubspot_contacts.py b/openedx/core/djangoapps/user_api/management/tests/test_sync_hubspot_contacts.py index d61bd05f48..2a966f48c9 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_sync_hubspot_contacts.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_sync_hubspot_contacts.py @@ -31,8 +31,7 @@ class TestHubspotSyncCommand(TestCase): super(TestHubspotSyncCommand, cls).setUpClass() cls.site_config = SiteConfigurationFactory() cls.hubspot_site_config = SiteConfigurationFactory.create( - site_values={'HUBSPOT_API_KEY': 'test_key'}, - values={'HUBSPOT_API_KEY': 'test_key'}, + site_values={'HUBSPOT_API_KEY': 'test_key'} ) cls.users = [] cls._create_users(cls.hubspot_site_config) # users for a site with hubspot integration enabled @@ -64,8 +63,8 @@ class TestHubspotSyncCommand(TestCase): """ Test no _sync_site call is made if hubspot integration is not enabled for any site """ - orig_values = self.hubspot_site_config.values - self.hubspot_site_config.values = {} + orig_values = self.hubspot_site_config.site_values + self.hubspot_site_config.site_values = {} self.hubspot_site_config.save() sync_site = patch.object(sync_command, '_sync_site') mock_sync_site = sync_site.start() @@ -73,7 +72,7 @@ class TestHubspotSyncCommand(TestCase): self.assertFalse(mock_sync_site.called, "_sync_site should not be called") sync_site.stop() # put values back - self.hubspot_site_config.values = orig_values + self.hubspot_site_config.site_values = orig_values self.hubspot_site_config.save() def test_with_initial_sync_days(self): diff --git a/openedx/features/content_type_gating/tests/test_models.py b/openedx/features/content_type_gating/tests/test_models.py index c6cf276bdc..89c66668d0 100644 --- a/openedx/features/content_type_gating/tests/test_models.py +++ b/openedx/features/content_type_gating/tests/test_models.py @@ -132,12 +132,10 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): non_test_course_enabled = CourseOverviewFactory.create(org='non-test-org-enabled') non_test_course_disabled = CourseOverviewFactory.create(org='non-test-org-disabled') non_test_site_cfg_enabled = SiteConfigurationFactory.create( - site_values={'course_org_filter': non_test_course_enabled.org}, - values={'course_org_filter': non_test_course_enabled.org} + site_values={'course_org_filter': non_test_course_enabled.org} ) non_test_site_cfg_disabled = SiteConfigurationFactory.create( - site_values={'course_org_filter': non_test_course_disabled.org}, - values={'course_org_filter': non_test_course_disabled.org} + site_values={'course_org_filter': non_test_course_disabled.org} ) ContentTypeGatingConfig.objects.create(course=non_test_course_enabled, enabled=True, enabled_as_of=datetime(2018, 1, 1)) @@ -150,8 +148,7 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): # Set up test objects test_course = CourseOverviewFactory.create(org='test-org') test_site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': test_course.org}, - values={'course_org_filter': test_course.org} + site_values={'course_org_filter': test_course.org} ) ContentTypeGatingConfig.objects.create(enabled=global_setting, enabled_as_of=datetime(2018, 1, 1)) @@ -176,14 +173,13 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): ContentTypeGatingConfig.objects.create(enabled=global_setting, enabled_as_of=datetime(2018, 1, 1)) for site_setting in (True, False, None): test_site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': []}, - values={'course_org_filter': []} + site_values={'course_org_filter': []} ) ContentTypeGatingConfig.objects.create(site=test_site_cfg.site, enabled=site_setting, enabled_as_of=datetime(2018, 1, 1)) for org_setting in (True, False, None): test_org = "{}-{}".format(test_site_cfg.id, org_setting) - test_site_cfg.values['course_org_filter'].append(test_org) + test_site_cfg.site_values['course_org_filter'].append(test_org) test_site_cfg.save() ContentTypeGatingConfig.objects.create(org=test_org, enabled=org_setting, enabled_as_of=datetime(2018, 1, 1)) @@ -292,8 +288,7 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): def test_caching_org(self): course = CourseOverviewFactory.create(org='test-org') site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': course.org}, - values={'course_org_filter': course.org} + site_values={'course_org_filter': course.org} ) org_config = ContentTypeGatingConfig(org=course.org, enabled=True, enabled_as_of=datetime(2018, 1, 1)) org_config.save() @@ -340,8 +335,7 @@ class TestContentTypeGatingConfig(CacheIsolationTestCase): def test_caching_course(self): course = CourseOverviewFactory.create(org='test-org') site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': course.org}, - values={'course_org_filter': course.org} + site_values={'course_org_filter': course.org} ) course_config = ContentTypeGatingConfig(course=course, enabled=True, enabled_as_of=datetime(2018, 1, 1)) course_config.save() diff --git a/openedx/features/course_duration_limits/tests/test_models.py b/openedx/features/course_duration_limits/tests/test_models.py index fe4865fd77..16414054a3 100644 --- a/openedx/features/course_duration_limits/tests/test_models.py +++ b/openedx/features/course_duration_limits/tests/test_models.py @@ -142,12 +142,10 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): non_test_course_enabled = CourseOverviewFactory.create(org='non-test-org-enabled') non_test_course_disabled = CourseOverviewFactory.create(org='non-test-org-disabled') non_test_site_cfg_enabled = SiteConfigurationFactory.create( - site_values={'course_org_filter': non_test_course_enabled.org}, - values={'course_org_filter': non_test_course_enabled.org} + site_values={'course_org_filter': non_test_course_enabled.org} ) non_test_site_cfg_disabled = SiteConfigurationFactory.create( - site_values={'course_org_filter': non_test_course_disabled.org}, - values={'course_org_filter': non_test_course_disabled.org} + site_values={'course_org_filter': non_test_course_disabled.org} ) CourseDurationLimitConfig.objects.create(course=non_test_course_enabled, enabled=True) @@ -160,8 +158,7 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): # Set up test objects test_course = CourseOverviewFactory.create(org='test-org') test_site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': test_course.org}, - values={'course_org_filter': test_course.org} + site_values={'course_org_filter': test_course.org} ) if reverse_order: @@ -191,8 +188,7 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): CourseDurationLimitConfig.objects.create(enabled=global_setting, enabled_as_of=datetime(2018, 1, 1)) for site_setting in (True, False, None): test_site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': []}, - values={'course_org_filter': []} + site_values={'course_org_filter': []} ) CourseDurationLimitConfig.objects.create( site=test_site_cfg.site, enabled=site_setting, enabled_as_of=datetime(2018, 1, 1) @@ -200,7 +196,7 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): for org_setting in (True, False, None): test_org = "{}-{}".format(test_site_cfg.id, org_setting) - test_site_cfg.values['course_org_filter'].append(test_org) + test_site_cfg.site_values['course_org_filter'].append(test_org) test_site_cfg.save() CourseDurationLimitConfig.objects.create( @@ -310,8 +306,7 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): def test_caching_org(self): course = CourseOverviewFactory.create(org='test-org') site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': course.org}, - values={'course_org_filter': course.org} + site_values={'course_org_filter': course.org} ) org_config = CourseDurationLimitConfig(org=course.org, enabled=True, enabled_as_of=datetime(2018, 1, 1)) org_config.save() @@ -358,8 +353,7 @@ class TestCourseDurationLimitConfig(CacheIsolationTestCase): def test_caching_course(self): course = CourseOverviewFactory.create(org='test-org') site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': course.org}, - values={'course_org_filter': course.org} + site_values={'course_org_filter': course.org} ) course_config = CourseDurationLimitConfig(course=course, enabled=True, enabled_as_of=datetime(2018, 1, 1)) course_config.save() diff --git a/openedx/features/discounts/tests/test_discount_restriction_models.py b/openedx/features/discounts/tests/test_discount_restriction_models.py index 1de2ca7c72..1288d202e3 100644 --- a/openedx/features/discounts/tests/test_discount_restriction_models.py +++ b/openedx/features/discounts/tests/test_discount_restriction_models.py @@ -59,12 +59,10 @@ class TestDiscountRestrictionConfig(CacheIsolationTestCase): non_test_course_disabled = CourseOverviewFactory.create(org='non-test-org-disabled') non_test_course_enabled = CourseOverviewFactory.create(org='non-test-org-enabled') non_test_site_cfg_disabled = SiteConfigurationFactory.create( - site_values={'course_org_filter': non_test_course_disabled.org}, - values={'course_org_filter': non_test_course_disabled.org} + site_values={'course_org_filter': non_test_course_disabled.org} ) non_test_site_cfg_enabled = SiteConfigurationFactory.create( - site_values={'course_org_filter': non_test_course_enabled.org}, - values={'course_org_filter': non_test_course_enabled.org} + site_values={'course_org_filter': non_test_course_enabled.org} ) DiscountRestrictionConfig.objects.create(course=non_test_course_disabled, disabled=True) @@ -77,8 +75,7 @@ class TestDiscountRestrictionConfig(CacheIsolationTestCase): # Set up test objects test_course = CourseOverviewFactory.create(org='test-org') test_site_cfg = SiteConfigurationFactory.create( - site_values={'course_org_filter': test_course.org}, - values={'course_org_filter': test_course.org} + site_values={'course_org_filter': test_course.org} ) DiscountRestrictionConfig.objects.create(disabled=global_setting) From 3400326e502c9d83d2856700d51211b408eb525f Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Wed, 26 Feb 2020 13:43:23 -0500 Subject: [PATCH 2/2] Fix migration issue with the missing method. This commit fixes an issue originally in 3541643d where an instance method on a model was missing in a migration. The problem was that Django is smarter than we thought, and is somehow able to construct an older version of the model before the commit, where there was no such method. The solution is just to pull the method out of the model. DENG-18 --- .../0005_copy_values_to_site_values.py | 3 +- .../djangoapps/site_configuration/models.py | 25 ++++++++-------- .../site_configuration/tests/test_models.py | 30 +++++++++++-------- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py b/openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py index 8993f724fd..23559ec7ed 100644 --- a/openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py +++ b/openedx/core/djangoapps/site_configuration/migrations/0005_copy_values_to_site_values.py @@ -4,6 +4,7 @@ from __future__ import unicode_literals from django.db import migrations +from ..models import save_siteconfig_without_historical_record def copy_column_values(apps, schema_editor): """ @@ -17,7 +18,7 @@ def copy_column_values(apps, schema_editor): # It would be incorrect to record these saves in the history table since it is # just backfilling data. Use save_without_historical_record() instead of # save(). - site_configuration.save_without_historical_record() + save_siteconfig_without_historical_record(site_configuration) # Update all values in the history model. SiteConfigurationHistory = apps.get_model('site_configuration', 'SiteConfigurationHistory') diff --git a/openedx/core/djangoapps/site_configuration/models.py b/openedx/core/djangoapps/site_configuration/models.py index e7c64fbd69..8aad073efd 100644 --- a/openedx/core/djangoapps/site_configuration/models.py +++ b/openedx/core/djangoapps/site_configuration/models.py @@ -141,20 +141,21 @@ class SiteConfiguration(models.Model): """ return org in cls.get_all_orgs() - def save_without_historical_record(self, *args, **kwargs): - """ - Save model without saving a historical record - Make sure you know what you're doing before you use this method. +def save_siteconfig_without_historical_record(siteconfig, *args, **kwargs): + """ + Save model without saving a historical record - Note: this method is copied verbatim from django-simple-history. - """ - self.skip_history_when_saving = True # pylint: disable=attribute-defined-outside-init - try: - ret = self.save(*args, **kwargs) - finally: - del self.skip_history_when_saving - return ret + Make sure you know what you're doing before you use this method. + + Note: this method is copied verbatim from django-simple-history. + """ + siteconfig.skip_history_when_saving = True + try: + ret = siteconfig.save(*args, **kwargs) + finally: + del siteconfig.skip_history_when_saving + return ret @python_2_unicode_compatible diff --git a/openedx/core/djangoapps/site_configuration/tests/test_models.py b/openedx/core/djangoapps/site_configuration/tests/test_models.py index ea83f8c271..db11151068 100644 --- a/openedx/core/djangoapps/site_configuration/tests/test_models.py +++ b/openedx/core/djangoapps/site_configuration/tests/test_models.py @@ -1,16 +1,17 @@ """ Tests for site configuration's django models. """ - - -from mock import patch import six -from django.test import TestCase -from django.db import IntegrityError, transaction from django.contrib.sites.models import Site - -from openedx.core.djangoapps.site_configuration.models import SiteConfigurationHistory, SiteConfiguration +from django.db import IntegrityError, transaction +from django.test import TestCase +from mock import patch +from openedx.core.djangoapps.site_configuration.models import ( + SiteConfiguration, + SiteConfigurationHistory, + save_siteconfig_without_historical_record +) from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory @@ -96,22 +97,25 @@ class SiteConfigurationTests(TestCase): def test_site_configuration_post_update_receiver_with_skip(self): """ Test that and entry is NOT added to SiteConfigurationHistory each time a - SiteConfiguration is updated with save_without_historical_record(). + SiteConfiguration is updated with save_siteconfig_without_historical_record(). """ - # add SiteConfiguration to database + # Add SiteConfiguration to database. By default, the site_valutes field contains only "{}". site_configuration = SiteConfigurationFactory.create( site=self.site, ) - site_configuration.site_values = {'test': 'test'} - site_configuration.save_without_historical_record() # Instead of save(). + # Update the SiteConfiguration we just created. + site_configuration.site_values = {"test": "test"} + save_siteconfig_without_historical_record(site_configuration) # Instead of .save(). + + # Verify that the SiteConfiguration has been updated. + self.assertEqual(site_configuration.get_value("test"), "test") # Verify an entry to SiteConfigurationHistory was NOT added. + # Make sure one entry (one for create and NONE for update) is saved for SiteConfiguration. site_configuration_history = SiteConfigurationHistory.objects.filter( site=site_configuration.site, ).all() - - # Make sure one entry (one for create and NONE for update) is saved for SiteConfiguration self.assertEqual(len(site_configuration_history), 1) def test_no_entry_is_saved_for_errors(self):