From 66e9819c0f21d655335d5b69b8c37335cf7f13fe Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Mon, 10 Aug 2015 17:47:10 -0400 Subject: [PATCH] Make status use ConfigurationModel instead. --- .../status/migrations/0001_initial.py | 93 +++++++++++++ .../djangoapps/status/migrations/__init__.py | 0 common/djangoapps/status/models.py | 60 +++++++++ common/djangoapps/status/status.py | 44 ++---- common/djangoapps/status/tests.py | 127 ++++++------------ .../tests/test_field_override_performance.py | 72 +++++----- lms/envs/common.py | 3 + lms/templates/navigation-edx.html | 2 +- lms/templates/navigation.html | 2 +- 9 files changed, 248 insertions(+), 155 deletions(-) create mode 100644 common/djangoapps/status/migrations/0001_initial.py create mode 100644 common/djangoapps/status/migrations/__init__.py create mode 100644 common/djangoapps/status/models.py diff --git a/common/djangoapps/status/migrations/0001_initial.py b/common/djangoapps/status/migrations/0001_initial.py new file mode 100644 index 0000000000..48a4d1eeff --- /dev/null +++ b/common/djangoapps/status/migrations/0001_initial.py @@ -0,0 +1,93 @@ +# -*- coding: utf-8 -*- +from south.utils import datetime_utils as datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'GlobalStatusMessage' + db.create_table('status_globalstatusmessage', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('change_date', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), + ('changed_by', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], null=True, on_delete=models.PROTECT)), + ('enabled', self.gf('django.db.models.fields.BooleanField')(default=False)), + ('message', self.gf('django.db.models.fields.TextField')(null=True, blank=True)), + )) + db.send_create_signal('status', ['GlobalStatusMessage']) + + # Adding model 'CourseMessage' + db.create_table('status_coursemessage', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('global_message', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['status.GlobalStatusMessage'])), + ('course_key', self.gf('xmodule_django.models.CourseKeyField')(db_index=True, max_length=255, blank=True)), + ('message', self.gf('django.db.models.fields.TextField')(null=True, blank=True)), + )) + db.send_create_signal('status', ['CourseMessage']) + + + def backwards(self, orm): + # Deleting model 'GlobalStatusMessage' + db.delete_table('status_globalstatusmessage') + + # Deleting model 'CourseMessage' + db.delete_table('status_coursemessage') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, + 'status.coursemessage': { + 'Meta': {'object_name': 'CourseMessage'}, + 'course_key': ('xmodule_django.models.CourseKeyField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), + 'global_message': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['status.GlobalStatusMessage']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}) + }, + 'status.globalstatusmessage': { + 'Meta': {'ordering': "('-change_date',)", 'object_name': 'GlobalStatusMessage'}, + 'change_date': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'changed_by': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'null': 'True', 'on_delete': 'models.PROTECT'}), + 'enabled': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'message': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}) + } + } + + complete_apps = ['status'] \ No newline at end of file diff --git a/common/djangoapps/status/migrations/__init__.py b/common/djangoapps/status/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/status/models.py b/common/djangoapps/status/models.py new file mode 100644 index 0000000000..766b3bac65 --- /dev/null +++ b/common/djangoapps/status/models.py @@ -0,0 +1,60 @@ +""" +Store status messages in the database. +""" + +from django.db import models +from django.contrib import admin +from django.core.cache import cache + +from xmodule_django.models import CourseKeyField + +from config_models.models import ConfigurationModel +from config_models.admin import ConfigurationModelAdmin + + +class GlobalStatusMessage(ConfigurationModel): + """ + Model that represents the current status message. + """ + message = models.TextField(blank=True, null=True) + + def full_message(self, course_key): + """ Returns the full status message, including any course-specific status messages. """ + cache_key = "status_message.{course_id}".format(course_id=unicode(course_key)) + if cache.get(cache_key): + return cache.get(cache_key) + + msg = self.message + if course_key: + try: + course_message = self.coursemessage_set.get(course_key=course_key) + # Don't add the message if course_message is blank. + if course_message: + msg = u"{}
{}".format(msg, course_message.message) + except CourseMessage.DoesNotExist: + # We don't have a course-specific message, so pass. + pass + cache.set(cache_key, msg) + return msg + + def __unicode__(self): + return "{} - {} - {}".format(self.change_date, self.enabled, self.message) + + +class CourseMessage(models.Model): + """ + Model that allows the user to specify messages for individual courses. + + This is not a ConfigurationModel because using it's not designed to support multiple configurations at once, + which would be problematic if separate courses need separate error messages. + """ + global_message = models.ForeignKey(GlobalStatusMessage) + course_key = CourseKeyField(max_length=255, blank=True, db_index=True) + message = models.TextField(blank=True, null=True) + + def __unicode__(self): + return unicode(self.course_key) + + +admin.site.register(GlobalStatusMessage, ConfigurationModelAdmin) +admin.site.register(CourseMessage) diff --git a/common/djangoapps/status/status.py b/common/djangoapps/status/status.py index 4e57930108..dd525601d7 100644 --- a/common/djangoapps/status/status.py +++ b/common/djangoapps/status/status.py @@ -3,49 +3,27 @@ A tiny app that checks for a status message. """ from django.conf import settings -from django.core.cache import cache -import json import logging -import os + +from .models import GlobalStatusMessage log = logging.getLogger(__name__) -def get_site_status_msg(course_id): +def get_site_status_msg(course_key): """ - Look for a file settings.STATUS_MESSAGE_PATH. If found, read it, - parse as json, and do the following: + Pull the status message from the database. - * if there is a key 'global', include that in the result list. - * if course is not None, and there is a key for course.id, add that to the result list. - * return "
".join(result) - - Otherwise, return None. - - If something goes wrong, returns None. ("is there a status msg?" logic is - not allowed to break the entire site). + Caches the message by course. """ try: - # first check for msg in cache - msg = cache.get('site_status_msg') - if msg is not None: - return msg - - if os.path.isfile(settings.STATUS_MESSAGE_PATH): - with open(settings.STATUS_MESSAGE_PATH) as f: - content = f.read() - else: + # The current() value for GlobalStatusMessage is cached. + if not GlobalStatusMessage.current().enabled: return None - status_dict = json.loads(content) - msg = status_dict.get('global', None) - if course_id in status_dict: - msg = msg + "
" if msg else '' - msg += status_dict[course_id] - - # set msg to cache, with expiry 5 mins - cache.set('site_status_msg', msg, 60 * 5) - return msg - except: + return GlobalStatusMessage.current().full_message(course_key) + # Make as general as possible, because something broken here should not + # bring down the whole site. + except: # pylint: disable=bare-except log.exception("Error while getting a status message.") return None diff --git a/common/djangoapps/status/tests.py b/common/djangoapps/status/tests.py index f31f2442f1..5e37e722c3 100644 --- a/common/djangoapps/status/tests.py +++ b/common/djangoapps/status/tests.py @@ -1,99 +1,58 @@ -from django.conf import settings -from django.core.cache import cache -from django.test import TestCase -import os -from django.test.utils import override_settings -from tempfile import NamedTemporaryFile +# -*- coding: utf-8 -*- +""" Tests for setting and displaying the site status message. """ import ddt +import unittest + +from django.test import TestCase +from django.core.cache import cache +from django.conf import settings +from opaque_keys.edx.locations import CourseLocator from .status import get_site_status_msg - -# Get a name where we can put test files -TMP_FILE = NamedTemporaryFile(delete=False) -TMP_NAME = TMP_FILE.name -# Close it--we just want the path. -TMP_FILE.close() +from .models import GlobalStatusMessage, CourseMessage @ddt.ddt -@override_settings(STATUS_MESSAGE_PATH=TMP_NAME) class TestStatus(TestCase): """Test that the get_site_status_msg function does the right thing""" - no_file = None - - invalid_json = """{ - "global" : "Hello, Globe", - }""" - - global_only = """{ - "global" : "Hello, Globe" - }""" - - toy_only = """{ - "edX/toy/2012_Fall" : "A toy story" - }""" - - global_and_toy = """{ - "global" : "Hello, Globe", - "edX/toy/2012_Fall" : "A toy story" - }""" - - # json to use, expected results for course=None (e.g. homepage), - # for toy course, for full course. Note that get_site_status_msg - # is supposed to return global message even if course=None. The - # template just happens to not display it outside the courseware - # at the moment... - checks = [ - (no_file, None, None, None), - (invalid_json, None, None, None), - (global_only, "Hello, Globe", "Hello, Globe", "Hello, Globe"), - (toy_only, None, "A toy story", None), - (global_and_toy, "Hello, Globe", "Hello, Globe
A toy story", "Hello, Globe"), - ] - def setUp(self): - """ - Fake course ids, since we don't have to have full django - settings (common tests run without the lms settings imported) - """ super(TestStatus, self).setUp() - self.full_id = 'edX/full/2012_Fall' - self.toy_id = 'edX/toy/2012_Fall' - self.addCleanup(self.remove_status_file) + # Clear the cache between test runs. + cache.clear() + self.course_key = CourseLocator(org='TestOrg', course='TestCourse', run='TestRun') - def create_status_file(self, contents): - """ - Write contents to settings.STATUS_MESSAGE_PATH. - """ - with open(settings.STATUS_MESSAGE_PATH, 'w') as f: - f.write(contents) - - def clear_status_cache(self): - """ - Remove the cached status message, if found - """ - if cache.get('site_status_msg') is not None: - cache.delete('site_status_msg') - - def remove_status_file(self): - """Delete the status file if it exists""" - if os.path.exists(settings.STATUS_MESSAGE_PATH): - os.remove(settings.STATUS_MESSAGE_PATH) - - @ddt.data(*checks) + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') + @ddt.data( + ("Test global message", "Test course message"), + (u" Ŧɇsŧ sŧȺŧᵾs", u"Ṫëṡẗ ċöüṛṡë ṡẗäẗüṡ "), + (u"", u"Ṫëṡẗ ċöüṛṡë ṡẗäẗüṡ "), + (u" Ŧɇsŧ sŧȺŧᵾs", u""), + ) @ddt.unpack - def test_get_site_status_msg(self, json_str, exp_none, exp_toy, exp_full): - """run the tests""" + def test_get_site_status_msg(self, test_global_message, test_course_message): + """Test status messages in a variety of situations.""" - self.remove_status_file() - if json_str: - self.create_status_file(json_str) + # When we don't have any data set. + self.assertEqual(get_site_status_msg(None), None) + self.assertEqual(get_site_status_msg(self.course_key), None) - for course_id, expected_msg in [(None, exp_none), (self.toy_id, exp_toy), (self.full_id, exp_full)]: - self.assertEqual(get_site_status_msg(course_id), expected_msg) - self.assertEqual(cache.get('site_status_msg'), expected_msg) - # check that `get_site_status_msg` works as expected when the cache - # is warmed, too - self.assertEqual(get_site_status_msg(course_id), expected_msg) - self.clear_status_cache() + msg = GlobalStatusMessage.objects.create(message=test_global_message, enabled=True) + msg.save() + + self.assertEqual(get_site_status_msg(None), test_global_message) + + course_msg = CourseMessage.objects.create( + global_message=msg, message=test_course_message, course_key=self.course_key + ) + course_msg.save() + self.assertEqual( + get_site_status_msg(self.course_key), + u"{}
{}".format(test_global_message, test_course_message) + ) + + msg = GlobalStatusMessage.objects.create(message="", enabled=False) + msg.save() + + self.assertEqual(get_site_status_msg(None), None) + self.assertEqual(get_site_status_msg(self.course_key), None) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 77f9a061ef..4b92a2b5f3 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -214,24 +214,24 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): TEST_DATA = { # (providers, course_width, enable_ccx, view_as_ccx): # of sql queries, # of mongo queries, # of xblocks - ('no_overrides', 1, True, False): (23, 7, 14), - ('no_overrides', 2, True, False): (68, 7, 85), - ('no_overrides', 3, True, False): (263, 7, 336), - ('ccx', 1, True, False): (23, 7, 14), - ('ccx', 2, True, False): (68, 7, 85), - ('ccx', 3, True, False): (263, 7, 336), - ('ccx', 1, True, True): (23, 7, 14), - ('ccx', 2, True, True): (68, 7, 85), - ('ccx', 3, True, True): (263, 7, 336), - ('no_overrides', 1, False, False): (23, 7, 14), - ('no_overrides', 2, False, False): (68, 7, 85), - ('no_overrides', 3, False, False): (263, 7, 336), - ('ccx', 1, False, False): (23, 7, 14), - ('ccx', 2, False, False): (68, 7, 85), - ('ccx', 3, False, False): (263, 7, 336), - ('ccx', 1, False, True): (23, 7, 14), - ('ccx', 2, False, True): (68, 7, 85), - ('ccx', 3, False, True): (263, 7, 336), + ('no_overrides', 1, True, False): (24, 7, 14), + ('no_overrides', 2, True, False): (69, 7, 85), + ('no_overrides', 3, True, False): (264, 7, 336), + ('ccx', 1, True, False): (24, 7, 14), + ('ccx', 2, True, False): (69, 7, 85), + ('ccx', 3, True, False): (264, 7, 336), + ('ccx', 1, True, True): (24, 7, 14), + ('ccx', 2, True, True): (69, 7, 85), + ('ccx', 3, True, True): (264, 7, 336), + ('no_overrides', 1, False, False): (24, 7, 14), + ('no_overrides', 2, False, False): (69, 7, 85), + ('no_overrides', 3, False, False): (264, 7, 336), + ('ccx', 1, False, False): (24, 7, 14), + ('ccx', 2, False, False): (69, 7, 85), + ('ccx', 3, False, False): (264, 7, 336), + ('ccx', 1, False, True): (24, 7, 14), + ('ccx', 2, False, True): (69, 7, 85), + ('ccx', 3, False, True): (264, 7, 336), } @@ -243,22 +243,22 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (23, 4, 9), - ('no_overrides', 2, True, False): (68, 19, 54), - ('no_overrides', 3, True, False): (263, 84, 215), - ('ccx', 1, True, False): (23, 4, 9), - ('ccx', 2, True, False): (68, 19, 54), - ('ccx', 3, True, False): (263, 84, 215), - ('ccx', 1, True, True): (25, 4, 13), - ('ccx', 2, True, True): (70, 19, 84), - ('ccx', 3, True, True): (265, 84, 335), - ('no_overrides', 1, False, False): (23, 4, 9), - ('no_overrides', 2, False, False): (68, 19, 54), - ('no_overrides', 3, False, False): (263, 84, 215), - ('ccx', 1, False, False): (23, 4, 9), - ('ccx', 2, False, False): (68, 19, 54), - ('ccx', 3, False, False): (263, 84, 215), - ('ccx', 1, False, True): (23, 4, 9), - ('ccx', 2, False, True): (68, 19, 54), - ('ccx', 3, False, True): (263, 84, 215), + ('no_overrides', 1, True, False): (24, 4, 9), + ('no_overrides', 2, True, False): (69, 19, 54), + ('no_overrides', 3, True, False): (264, 84, 215), + ('ccx', 1, True, False): (24, 4, 9), + ('ccx', 2, True, False): (69, 19, 54), + ('ccx', 3, True, False): (264, 84, 215), + ('ccx', 1, True, True): (26, 4, 13), + ('ccx', 2, True, True): (71, 19, 84), + ('ccx', 3, True, True): (266, 84, 335), + ('no_overrides', 1, False, False): (24, 4, 9), + ('no_overrides', 2, False, False): (69, 19, 54), + ('no_overrides', 3, False, False): (264, 84, 215), + ('ccx', 1, False, False): (24, 4, 9), + ('ccx', 2, False, False): (69, 19, 54), + ('ccx', 3, False, False): (264, 84, 215), + ('ccx', 1, False, True): (24, 4, 9), + ('ccx', 2, False, True): (69, 19, 54), + ('ccx', 3, False, True): (264, 84, 215), } diff --git a/lms/envs/common.py b/lms/envs/common.py index 2add2dd585..3e9d5a3040 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1802,6 +1802,9 @@ INSTALLED_APPS = ( # Monitor the status of services 'service_status', + # Display status message to students + 'status', + # For asset pipelining 'edxmako', 'pipeline', diff --git a/lms/templates/navigation-edx.html b/lms/templates/navigation-edx.html index d0c17fb22b..b905840951 100644 --- a/lms/templates/navigation-edx.html +++ b/lms/templates/navigation-edx.html @@ -20,7 +20,7 @@ from status.status import get_site_status_msg <%block> <% try: - course_id = course.id.to_deprecated_string() + course_id = course.id except: # can't figure out a better way to get at a possibly-defined course var course_id = None diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index 10b68e32a0..809f4b07e8 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -21,7 +21,7 @@ from status.status import get_site_status_msg <%block> <% try: - course_id = course.id.to_deprecated_string() + course_id = course.id except: # can't figure out a better way to get at a possibly-defined course var course_id = None