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 55e3407ebab2..cb740c894e6d 100644 --- a/common/djangoapps/student/admin.py +++ b/common/djangoapps/student/admin.py @@ -465,7 +465,7 @@ def clean_email(self): 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 7a5f363b3e24..21840fe2451f 100644 --- a/common/djangoapps/student/tests/test_admin_views.py +++ b/common/djangoapps/student/tests/test_admin_views.py @@ -454,7 +454,7 @@ def setUpClass(cls): 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 @@ def test_form_with_invalid_site_configuration(self): 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 8fdcf40b9f06..565ad10d606a 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 @@ def test_migrations_are_in_sync(self): 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 8b2ffc95d28a..1f11b2da8148 100644 --- a/lms/djangoapps/branding/tests/test_views.py +++ b/lms/djangoapps/branding/tests/test_views.py @@ -297,7 +297,7 @@ def test_index_redirects_to_marketing_site_with_site_override(self): 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 @@ def test_header_logo_links_to_marketing_site_with_site_override(self): 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 e303bc11c9be..14d588714415 100644 --- a/lms/djangoapps/discussion/tests/test_signals.py +++ b/lms/djangoapps/discussion/tests/test_signals.py @@ -30,7 +30,6 @@ def test_comment_created_signal_sends_message(self, mock_send_message, mock_get_ 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 @@ def test_comment_created_signal_msg_not_sent_with_site_config_disabled( 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 c6a8b64ebbc0..4161792a98e7 100644 --- a/lms/djangoapps/discussion/tests/test_tasks.py +++ b/lms/djangoapps/discussion/tests/test_tasks.py @@ -193,7 +193,7 @@ def test_send_discussion_email_notification(self, user_subscribed): 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 5fa668f87f2b..4424cd6338e5 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -4000,8 +4000,8 @@ def test_send_email_no_message(self): 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 @@ def test_send_email_with_org_template_and_from_addr(self): 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 58d29ae5c1b9..3bbfdbf54ffa 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 @@ def test_membership_reason_field_visibility(self, enbale_reason_field): SiteConfiguration.objects.create( site=site, site_values=configuration_values, - values=configuration_values, enabled=True ) @@ -202,7 +201,6 @@ def test_membership_site_configuration_role(self): 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 1412acf25283..62013620551d 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 @@ def test_site_config_override(self): 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 41874667c1ee..a6c1ba370498 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 abf6b5a038d7..5a46541a939e 100644 --- a/openedx/core/djangoapps/config_model_utils/models.py +++ b/openedx/core/djangoapps/config_model_utils/models.py @@ -244,7 +244,7 @@ def all_current_course_configs(cls): """ 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 @@ def all_current_course_configs(cls): 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 d84ad0669bac..63ed6e710122 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 @@ def test_page_size(self): @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 ef2a8196d525..311be58cc06d 100644 --- a/openedx/core/djangoapps/credentials/tests/test_signals.py +++ b/openedx/core/djangoapps/credentials/tests/test_signals.py @@ -110,8 +110,7 @@ def test_send_grade_without_issuance_enabled(self, _mock_is_course_run_in_a_prog 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 @@ def test_send_grade_records_enabled(self, _mock_is_course_run_in_a_program, mock 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 6fa7c40de111..230dc13862a2 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 @@ def test_awarding_certs_with_skip_program_certificate( 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 82c5299cb7b5..364dc089f632 100644 --- a/openedx/core/djangoapps/programs/tests/test_signals.py +++ b/openedx/core/djangoapps/programs/tests/test_signals.py @@ -146,8 +146,7 @@ def test_records_enabled(self, mock_is_learner_issuance_enabled, mock_task): 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 @@ def test_records_enabled(self, mock_is_learner_issuance_enabled, mock_task): 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 9da5d0e81108..b76054d8fb30 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 @@ def test_site_config(self, this_org_list, other_org_list, expected_message_count 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 4c792b860fbd..aa60ec7cab5c 100644 --- a/openedx/core/djangoapps/schedules/tests/test_resolvers.py +++ b/openedx/core/djangoapps/schedules/tests/test_resolvers.py @@ -61,7 +61,7 @@ def setUp(self): '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 @@ def test_get_course_org_filter_equal(self, course_org_filter): (['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 @@ def test_get_course_org_filter_include__in(self, course_org_filter, expected_org ) 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 6fda05f472f7..d8b52647b625 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 07cb3ae9a9bb..45663497f3a9 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 2c670b513fbd..edcb0f19a417 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 @@ def handle(self, *args, **options): 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 0a0d4974ad3f..9a686ae0cd30 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 @@ def get_site_configuration(self): 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 @@ def test_site_configuration_created_when_non_existent(self): 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 @@ def test_site_configuration_enabled_disabled(self, flag, enabled): 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 @@ def test_site_configuration_created_with_parameters(self): 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 @@ def test_site_configuration_created_with_json_file_parameters(self): 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 @@ def test_site_configuration_updated_with_parameters(self, enabled): 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 @@ def test_site_configuration_updated_from_json_file(self, enabled): 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 000000000000..8993f724fd49 --- /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 683fae6af5ef..e7c64fbd6925 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 @@ def get_value(self, name, default=None): """ 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 @@ def get_configuration_for_org(cls, org, select_related=None): 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 @@ def get_all_orgs(cls): """ 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 @@ def has_org(cls, org): """ 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 @@ def __repr__(self): @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 4943ca3670c4..db1bb7377f4e 100644 --- a/openedx/core/djangoapps/site_configuration/tests/factories.py +++ b/openedx/core/djangoapps/site_configuration/tests/factories.py @@ -33,5 +33,5 @@ class Meta(object): 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 95294d289192..f08aca35415f 100644 --- a/openedx/core/djangoapps/site_configuration/tests/mixins.py +++ b/openedx/core/djangoapps/site_configuration/tests/mixins.py @@ -22,9 +22,7 @@ def setUp(self): } 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 @@ def setUp(self): } 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 @@ def set_up_site(self, domain, site_configuration_values): ) __ = 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 c1dcc8dd37e7..124ba1f4a0a8 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 @@ def setUp(self): site=self.site, site_values={ "SESSION_COOKIE_DOMAIN": self.site.domain, - }, - values={ - "SESSION_COOKIE_DOMAIN": self.site.domain, } ) @@ -78,9 +75,6 @@ def setUp(self): 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 4b69d57f5870..ea83f8c2719b 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 @@ def test_site_configuration_post_update_receiver(self): ) 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 @@ def test_site_configuration_post_update_receiver(self): 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 @@ def test_get_value(self): # 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 @@ def test_invalid_data_error_on_get_value(self): # 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 @@ def test_get_value_for_org(self): # 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 @@ def test_get_site_for_org(self): # 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 @@ def test_get_all_orgs(self): # 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 @@ def test_get_all_orgs_returns_only_enabled(self): 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 6c13a5fadc14..6bfb5b760e79 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 _decorated(*args, **kwargs): # pylint: disable=missing-docstring 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 41e3ad59de46..e83a2b38b8f0 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 @@ def _create_sites(self, site_domain, theme_dir_name, site_configuration): 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 d61bd05f484a..2a966f48c94a 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 @@ def setUpClass(cls): 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 @@ def test_without_any_hubspot_api_key(self): """ 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 @@ def test_without_any_hubspot_api_key(self): 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 c6cf276bdcfb..89c66668d0b1 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 @@ def test_config_overrides(self, global_setting, site_setting, org_setting, cours 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 @@ def test_config_overrides(self, global_setting, site_setting, org_setting, cours # 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 @@ def test_all_current_course_configs(self): 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 @@ def test_caching_site(self): 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 @@ def test_caching_org(self): 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 fe4865fd774c..16414054a367 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 @@ def test_config_overrides(self, global_setting, site_setting, org_setting, cours 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 @@ def test_config_overrides(self, global_setting, site_setting, org_setting, cours # 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 @@ def test_all_current_course_configs(self): 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 @@ def test_all_current_course_configs(self): 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 @@ def test_caching_site(self): 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 @@ def test_caching_org(self): 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 1de2ca7c7202..1288d202e329 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 @@ def test_config_overrides(self, global_setting, site_setting, org_setting, cours 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 @@ def test_config_overrides(self, global_setting, site_setting, org_setting, cours # 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 8993f724fd49..23559ec7ed39 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 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 e7c64fbd6925..8aad073efd35 100644 --- a/openedx/core/djangoapps/site_configuration/models.py +++ b/openedx/core/djangoapps/site_configuration/models.py @@ -141,20 +141,21 @@ def has_org(cls, org): """ 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 ea83f8c2719b..db1115106847 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 @@ def test_site_configuration_post_update_receiver(self): 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):