From 0c69ec2f3301258d09f432a726722fa2fe558150 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sun, 7 Oct 2012 20:02:18 -0400 Subject: [PATCH 1/3] Add support for per-course site status. Now looks for a status_message.json file, looks for 'global' and ${course.id} keys. Return any global message and any course-specific message, joined by
. fab files need to be updated to use this new format (new filename, possibly also help manage per-course messages, or at least test for valid json) --- common/djangoapps/status/status.py | 24 ++++++-- common/djangoapps/status/tests.py | 91 ++++++++++++++++++++++++++++++ lms/envs/common.py | 2 +- lms/envs/test.py | 15 +++++ lms/templates/navigation.html | 7 ++- 5 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 common/djangoapps/status/tests.py diff --git a/common/djangoapps/status/status.py b/common/djangoapps/status/status.py index d8cb3f5e8f14..7ca89d2f4e70 100644 --- a/common/djangoapps/status/status.py +++ b/common/djangoapps/status/status.py @@ -3,27 +3,41 @@ """ from django.conf import settings +import json import logging import os import sys log = logging.getLogger(__name__) -def get_site_status_msg(): +def get_site_status_msg(course): """ - Look for a file settings.STATUS_MESSAGE_PATH. If found, return the - contents. Otherwise, return None. + Look for a file settings.STATUS_MESSAGE_PATH. If found, read it, + parse as json, and do the following: + + * 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). """ try: - content = None if os.path.isfile(settings.STATUS_MESSAGE_PATH): with open(settings.STATUS_MESSAGE_PATH) as f: content = f.read() + else: + return None + + status_dict = json.loads(content) + msg = status_dict.get('global', None) + if course and course.id in status_dict: + msg = msg + "
" if msg else '' + msg += status_dict[course.id] - return content + return msg 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 new file mode 100644 index 000000000000..604cfe3e375a --- /dev/null +++ b/common/djangoapps/status/tests.py @@ -0,0 +1,91 @@ +from django.conf import settings +from django.test import TestCase +from tempfile import NamedTemporaryFile +import os +from override_settings import override_settings + +from status import get_site_status_msg + +import xmodule.modulestore.django +from xmodule.modulestore.django import modulestore +from xmodule.modulestore import Location +from xmodule.modulestore.xml_importer import import_from_xml + + +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): + xmodule.modulestore.django._MODULESTORES = {} + courses = modulestore().get_courses() + + def find_course(course_id): + """Assumes the course is present""" + return [c for c in courses if c.id==course_id][0] + + self.full = find_course("edX/full/6.002_Spring_2012") + self.toy = find_course("edX/toy/2012_Fall") + + 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 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) + + def tearDown(self): + self.remove_status_file() + + def test_get_site_status_msg(self): + """run the tests""" + for (json_str, exp_none, exp_toy, exp_full) in self.checks: + + self.remove_status_file() + if json_str: + self.create_status_file(json_str) + + print "checking results for {0}".format(json_str) + print "course=None:" + self.assertEqual(get_site_status_msg(None), exp_none) + print "course=toy:" + self.assertEqual(get_site_status_msg(self.toy), exp_toy) + print "course=full:" + self.assertEqual(get_site_status_msg(self.full), exp_full) diff --git a/lms/envs/common.py b/lms/envs/common.py index e4f087286fd6..a927da8e9825 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -129,7 +129,7 @@ # Where to look for a status message -STATUS_MESSAGE_PATH = ENV_ROOT / "status_message.html" +STATUS_MESSAGE_PATH = ENV_ROOT / "status_message.json" ############################ OpenID Provider ################################## OPENID_PROVIDER_TRUSTED_ROOTS = ['cs50.net', '*.cs50.net'] diff --git a/lms/envs/test.py b/lms/envs/test.py index af48690dd3f0..892019736cda 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -40,6 +40,8 @@ # Want static files in the same dir for running on jenkins. STATIC_ROOT = TEST_ROOT / "staticfiles" +STATUS_MESSAGE_PATH = TEST_ROOT / "status_message.json" + COURSES_ROOT = TEST_ROOT / "data" DATA_DIR = COURSES_ROOT @@ -77,6 +79,19 @@ if os.path.isdir(COMMON_TEST_DATA_ROOT / course_dir) ] +# point tests at the test courses by default + +MODULESTORE = { + 'default': { + 'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore', + 'OPTIONS': { + 'data_dir': COMMON_TEST_DATA_ROOT, + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + } + } +} + + DATABASES = { 'default': { 'ENGINE': 'django.db.backends.sqlite3', diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index 90c9e2d3836f..7a4f4da0f742 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -14,7 +14,12 @@ <%block cached="False"> <% -site_status_msg = get_site_status_msg() +try: + c = course +except: + # can't figure out a better way to get at a possibly-defined course var + c = None +site_status_msg = get_site_status_msg(c) %> % if site_status_msg:
From 3feb24f9977795f540d623a9221f0bb1bdd6a42b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Sun, 7 Oct 2012 23:06:27 -0400 Subject: [PATCH 2/3] make tests pass without LMS settings --- common/djangoapps/status/tests.py | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/common/djangoapps/status/tests.py b/common/djangoapps/status/tests.py index 604cfe3e375a..b0535c243f17 100644 --- a/common/djangoapps/status/tests.py +++ b/common/djangoapps/status/tests.py @@ -1,17 +1,20 @@ from django.conf import settings from django.test import TestCase -from tempfile import NamedTemporaryFile +from mock import Mock import os from override_settings import override_settings +from tempfile import NamedTemporaryFile from status import get_site_status_msg -import xmodule.modulestore.django -from xmodule.modulestore.django import modulestore -from xmodule.modulestore import Location -from xmodule.modulestore.xml_importer import import_from_xml +# 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() +@override_settings(STATUS_MESSAGE_PATH=TMP_NAME) class TestStatus(TestCase): """Test that the get_site_status_msg function does the right thing""" @@ -49,15 +52,14 @@ class TestStatus(TestCase): ] def setUp(self): - xmodule.modulestore.django._MODULESTORES = {} - courses = modulestore().get_courses() - - def find_course(course_id): - """Assumes the course is present""" - return [c for c in courses if c.id==course_id][0] - - self.full = find_course("edX/full/6.002_Spring_2012") - self.toy = find_course("edX/toy/2012_Fall") + """ + Mock courses, since we don't have to have full django + settings (common tests run without the lms settings imported) + """ + self.full = Mock() + self.full.id = 'edX/full/2012_Fall' + self.toy = Mock() + self.toy.id = 'edX/toy/2012_Fall' def create_status_file(self, contents): """ From 7e1ca6e3d001ff3623956d81a76e3120c13c8d00 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 8 Oct 2012 14:06:58 -0400 Subject: [PATCH 3/3] Pass course_id instead of course to get_site_status_msg * makes testing easier -- don't need course objects --- common/djangoapps/status/status.py | 6 +++--- common/djangoapps/status/tests.py | 13 +++++-------- lms/templates/navigation.html | 6 +++--- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/common/djangoapps/status/status.py b/common/djangoapps/status/status.py index 7ca89d2f4e70..c06a70f5a106 100644 --- a/common/djangoapps/status/status.py +++ b/common/djangoapps/status/status.py @@ -10,7 +10,7 @@ log = logging.getLogger(__name__) -def get_site_status_msg(course): +def get_site_status_msg(course_id): """ Look for a file settings.STATUS_MESSAGE_PATH. If found, read it, parse as json, and do the following: @@ -33,9 +33,9 @@ def get_site_status_msg(course): status_dict = json.loads(content) msg = status_dict.get('global', None) - if course and course.id in status_dict: + if course_id in status_dict: msg = msg + "
" if msg else '' - msg += status_dict[course.id] + msg += status_dict[course_id] return msg except: diff --git a/common/djangoapps/status/tests.py b/common/djangoapps/status/tests.py index b0535c243f17..98a36f433ae3 100644 --- a/common/djangoapps/status/tests.py +++ b/common/djangoapps/status/tests.py @@ -1,6 +1,5 @@ from django.conf import settings from django.test import TestCase -from mock import Mock import os from override_settings import override_settings from tempfile import NamedTemporaryFile @@ -53,13 +52,11 @@ class TestStatus(TestCase): def setUp(self): """ - Mock courses, since we don't have to have full django + Fake course ids, since we don't have to have full django settings (common tests run without the lms settings imported) """ - self.full = Mock() - self.full.id = 'edX/full/2012_Fall' - self.toy = Mock() - self.toy.id = 'edX/toy/2012_Fall' + self.full_id = 'edX/full/2012_Fall' + self.toy_id = 'edX/toy/2012_Fall' def create_status_file(self, contents): """ @@ -88,6 +85,6 @@ def test_get_site_status_msg(self): print "course=None:" self.assertEqual(get_site_status_msg(None), exp_none) print "course=toy:" - self.assertEqual(get_site_status_msg(self.toy), exp_toy) + self.assertEqual(get_site_status_msg(self.toy_id), exp_toy) print "course=full:" - self.assertEqual(get_site_status_msg(self.full), exp_full) + self.assertEqual(get_site_status_msg(self.full_id), exp_full) diff --git a/lms/templates/navigation.html b/lms/templates/navigation.html index 7a4f4da0f742..d574bc3f6eca 100644 --- a/lms/templates/navigation.html +++ b/lms/templates/navigation.html @@ -15,11 +15,11 @@ <%block cached="False"> <% try: - c = course + course_id = course.id except: # can't figure out a better way to get at a possibly-defined course var - c = None -site_status_msg = get_site_status_msg(c) + course_id = None +site_status_msg = get_site_status_msg(course_id) %> % if site_status_msg: