From f1ccf1c06b12acd4c74db693363c1fc6abd1f2a0 Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Mon, 9 Sep 2013 14:52:54 -0400 Subject: [PATCH 1/3] Integrate split testing and LMS tabs experiments --- lms/djangoapps/courseware/tabs.py | 99 ++++++++++++------- lms/djangoapps/courseware/tests/test_tabs.py | 57 +++++------ lms/djangoapps/courseware/views.py | 1 + lms/envs/common.py | 16 ++- lms/envs/dev.py | 2 +- .../course/layout/_courseware_header.scss | 11 +++ .../courseware/course_navigation.html | 11 ++- lms/templates/courseware/welcome-back.html | 34 ++++++- lms/templates/dashboard.html | 8 +- lms/templates/widgets/segment-io.html | 16 ++- lms/urls.py | 1 + requirements/edx/github.txt | 1 + 12 files changed, 184 insertions(+), 73 deletions(-) diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index ce49e5a201..6579e631d6 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/lms/djangoapps/courseware/tabs.py @@ -25,6 +25,8 @@ from courseware.model_data import FieldDataCache from open_ended_grading import open_ended_notifications +import waffle + log = logging.getLogger(__name__) @@ -55,32 +57,46 @@ TabImpl = namedtuple('TabImpl', 'validator generator') ##### Generators for various tabs. - -def _courseware(tab, user, course, active_page): +def _courseware(tab, user, course, active_page, request): + """ + This returns a tab containing the course content. + """ link = reverse('courseware', args=[course.id]) - return [CourseTab('Courseware', link, active_page == "courseware")] + if waffle.flag_is_active(request, 'merge_course_tabs'): + return [CourseTab('Course Content', link, active_page == "courseware")] + else: + return [CourseTab('Courseware', link, active_page == "courseware")] -def _course_info(tab, user, course, active_page): +def _course_info(tab, user, course, active_page, request): + """ + This returns a tab containing information about the course. + """ link = reverse('info', args=[course.id]) return [CourseTab(tab['name'], link, active_page == "info")] -def _progress(tab, user, course, active_page): +def _progress(tab, user, course, active_page, request): + """ + This returns a tab containing information about the authenticated user's progress. + """ if user.is_authenticated(): link = reverse('progress', args=[course.id]) return [CourseTab(tab['name'], link, active_page == "progress")] return [] -def _wiki(tab, user, course, active_page): +def _wiki(tab, user, course, active_page, request): + """ + This returns a tab containing the course wiki. + """ if settings.WIKI_ENABLED: link = reverse('course_wiki', args=[course.id]) return [CourseTab(tab['name'], link, active_page == 'wiki')] return [] -def _discussion(tab, user, course, active_page): +def _discussion(tab, user, course, active_page, request): """ This tab format only supports the new Berkeley discussion forums. """ @@ -91,25 +107,25 @@ def _discussion(tab, user, course, active_page): return [] -def _external_discussion(tab, user, course, active_page): +def _external_discussion(tab, user, course, active_page, request): """ This returns a tab that links to an external discussion service """ return [CourseTab('Discussion', tab['link'], active_page == 'discussion')] -def _external_link(tab, user, course, active_page): +def _external_link(tab, user, course, active_page, request): # external links are never active return [CourseTab(tab['name'], tab['link'], False)] -def _static_tab(tab, user, course, active_page): +def _static_tab(tab, user, course, active_page, request): link = reverse('static_tab', args=[course.id, tab['url_slug']]) active_str = 'static_tab_{0}'.format(tab['url_slug']) return [CourseTab(tab['name'], link, active_page == active_str)] -def _textbooks(tab, user, course, active_page): +def _textbooks(tab, user, course, active_page, request): """ Generates one tab per textbook. Only displays if user is authenticated. """ @@ -120,7 +136,8 @@ def _textbooks(tab, user, course, active_page): for index, textbook in enumerate(course.textbooks)] return [] -def _pdf_textbooks(tab, user, course, active_page): + +def _pdf_textbooks(tab, user, course, active_page, request): """ Generates one tab per textbook. Only displays if user is authenticated. """ @@ -131,7 +148,8 @@ def _pdf_textbooks(tab, user, course, active_page): for index, textbook in enumerate(course.pdf_textbooks)] return [] -def _html_textbooks(tab, user, course, active_page): + +def _html_textbooks(tab, user, course, active_page, request): """ Generates one tab per textbook. Only displays if user is authenticated. """ @@ -142,7 +160,8 @@ def _html_textbooks(tab, user, course, active_page): for index, textbook in enumerate(course.html_textbooks)] return [] -def _staff_grading(tab, user, course, active_page): + +def _staff_grading(tab, user, course, active_page, request): if has_access(user, course, 'staff'): link = reverse('staff_grading', args=[course.id]) @@ -157,14 +176,13 @@ def _staff_grading(tab, user, course, active_page): return [] -def _syllabus(tab, user, course, active_page): +def _syllabus(tab, user, course, active_page, request): """Display the syllabus tab""" link = reverse('syllabus', args=[course.id]) return [CourseTab('Syllabus', link, active_page == 'syllabus')] -def _peer_grading(tab, user, course, active_page): - +def _peer_grading(tab, user, course, active_page, request): if user.is_authenticated(): link = reverse('peer_grading', args=[course.id]) tab_name = "Peer grading" @@ -178,7 +196,7 @@ def _peer_grading(tab, user, course, active_page): return [] -def _combined_open_ended_grading(tab, user, course, active_page): +def _combined_open_ended_grading(tab, user, course, active_page, request): if user.is_authenticated(): link = reverse('open_ended_notifications', args=[course.id]) tab_name = "Open Ended Panel" @@ -191,15 +209,15 @@ def _combined_open_ended_grading(tab, user, course, active_page): return tab return [] -def _notes_tab(tab, user, course, active_page): + +def _notes_tab(tab, user, course, active_page, request): if user.is_authenticated() and settings.MITX_FEATURES.get('ENABLE_STUDENT_NOTES'): link = reverse('notes', args=[course.id]) return [CourseTab(tab['name'], link, active_page == 'notes')] return [] + #### Validators - - def key_checker(expected_keys): """ Returns a function that checks that specified keys are present in a dict @@ -263,12 +281,15 @@ def validate_tabs(course): if len(tabs) < 2: raise InvalidTabsException("Expected at least two tabs. tabs: '{0}'".format(tabs)) + if tabs[0]['type'] != 'courseware': raise InvalidTabsException( "Expected first tab to have type 'courseware'. tabs: '{0}'".format(tabs)) + if tabs[1]['type'] != 'course_info': raise InvalidTabsException( "Expected second tab to have type 'course_info'. tabs: '{0}'".format(tabs)) + for t in tabs: if t['type'] not in VALID_TAB_TYPES: raise InvalidTabsException("Unknown tab type {0}. Known types: {1}" @@ -280,12 +301,12 @@ def validate_tabs(course): # are actually unique (otherwise, will break active tag code) -def get_course_tabs(user, course, active_page): +def get_course_tabs(user, course, active_page, request): """ Return the tabs to show a particular user, as a list of CourseTab items. """ if not hasattr(course, 'tabs') or not course.tabs: - return get_default_tabs(user, course, active_page) + return get_default_tabs(user, course, active_page, request) # TODO (vshnayder): There needs to be a place to call this right after course # load, but not from inside xmodule, since that doesn't (and probably @@ -293,12 +314,18 @@ def get_course_tabs(user, course, active_page): validate_tabs(course) tabs = [] - for tab in course.tabs: + + if waffle.flag_is_active(request, 'merge_course_tabs'): + course_tabs = [tab for tab in course.tabs if tab['type'] != "course_info"] + else: + course_tabs = course.tabs + + for tab in course_tabs: # expect handlers to return lists--handles things that are turned off # via feature flags, and things like 'textbook' which might generate # multiple tabs. gen = VALID_TAB_TYPES[tab['type']].generator - tabs.extend(gen(tab, user, course, active_page)) + tabs.extend(gen(tab, user, course, active_page, request)) # Instructor tab is special--automatically added if user is staff for the course if has_access(user, course, 'staff'): @@ -314,7 +341,7 @@ def get_discussion_link(course): Return the URL for the discussion tab for the given `course`. If they have a discussion link specified, use that even if we disable - discussions. Disabling discsussions is mostly a server safety feature at + discussions. Disabling discussions is mostly a server safety feature at this point, and we don't need to worry about external sites. Otherwise, if the course has a discussion tab or uses the default tabs, return the discussion view URL. Otherwise, return None to indicate the lack of a @@ -330,28 +357,33 @@ def get_discussion_link(course): return reverse('django_comment_client.forum.views.forum_form_discussion', args=[course.id]) -def get_default_tabs(user, course, active_page): - +def get_default_tabs(user, course, active_page, request): + """ + Return the default set of tabs. + """ # When calling the various _tab methods, can omit the 'type':'blah' from the # first arg, since that's only used for dispatch tabs = [] - tabs.extend(_courseware({''}, user, course, active_page)) - tabs.extend(_course_info({'name': 'Course Info'}, user, course, active_page)) + + tabs.extend(_courseware({''}, user, course, active_page, request)) + + if not waffle.flag_is_active(request, 'merge_course_tabs'): + tabs.extend(_course_info({'name': 'Course Info'}, user, course, active_page, request)) if hasattr(course, 'syllabus_present') and course.syllabus_present: link = reverse('syllabus', args=[course.id]) tabs.append(CourseTab('Syllabus', link, active_page == 'syllabus')) - tabs.extend(_textbooks({}, user, course, active_page)) + tabs.extend(_textbooks({}, user, course, active_page, request)) discussion_link = get_discussion_link(course) if discussion_link: tabs.append(CourseTab('Discussion', discussion_link, active_page == 'discussion')) - tabs.extend(_wiki({'name': 'Wiki', 'type': 'wiki'}, user, course, active_page)) + tabs.extend(_wiki({'name': 'Wiki', 'type': 'wiki'}, user, course, active_page, request)) if user.is_authenticated() and not course.hide_progress_tab: - tabs.extend(_progress({'name': 'Progress'}, user, course, active_page)) + tabs.extend(_progress({'name': 'Progress'}, user, course, active_page, request)) if has_access(user, course, 'staff'): link = reverse('instructor_dashboard', args=[course.id]) @@ -376,7 +408,6 @@ def get_static_tab_by_slug(course, tab_slug): def get_static_tab_contents(request, course, tab): - loc = Location(course.location.tag, course.location.org, course.location.course, 'static_tab', tab['url_slug']) field_data_cache = FieldDataCache.cache_for_descriptor_descendents(course.id, request.user, modulestore().get_instance(course.id, loc), depth=0) diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index 5de7a39f63..54264121d1 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/lms/djangoapps/courseware/tests/test_tabs.py @@ -11,6 +11,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE +FAKE_REQUEST = None class ProgressTestCase(TestCase): @@ -29,20 +30,20 @@ class ProgressTestCase(TestCase): def test_progress(self): self.assertEqual(tabs._progress(self.tab, self.mockuser0, self.course, - self.active_page0), []) + self.active_page0, FAKE_REQUEST), []) self.assertEqual(tabs._progress(self.tab, self.mockuser1, self.course, - self.active_page1)[0].name, 'same') + self.active_page1, FAKE_REQUEST)[0].name, 'same') self.assertEqual(tabs._progress(self.tab, self.mockuser1, self.course, - self.active_page1)[0].link, + self.active_page1, FAKE_REQUEST)[0].link, reverse('progress', args=[self.course.id])) self.assertEqual(tabs._progress(self.tab, self.mockuser1, self.course, - self.active_page0)[0].is_active, False) + self.active_page0, FAKE_REQUEST)[0].is_active, False) self.assertEqual(tabs._progress(self.tab, self.mockuser1, self.course, - self.active_page1)[0].is_active, True) + self.active_page1, FAKE_REQUEST)[0].is_active, True) class WikiTestCase(TestCase): @@ -60,26 +61,26 @@ class WikiTestCase(TestCase): def test_wiki_enabled(self): self.assertEqual(tabs._wiki(self.tab, self.user, - self.course, self.active_page1)[0].name, + self.course, self.active_page1, FAKE_REQUEST)[0].name, 'same') self.assertEqual(tabs._wiki(self.tab, self.user, - self.course, self.active_page1)[0].link, + self.course, self.active_page1, FAKE_REQUEST)[0].link, reverse('course_wiki', args=[self.course.id])) self.assertEqual(tabs._wiki(self.tab, self.user, - self.course, self.active_page1)[0].is_active, + self.course, self.active_page1, FAKE_REQUEST)[0].is_active, True) self.assertEqual(tabs._wiki(self.tab, self.user, - self.course, self.active_page0)[0].is_active, + self.course, self.active_page0, FAKE_REQUEST)[0].is_active, False) @override_settings(WIKI_ENABLED=False) def test_wiki_enabled_false(self): self.assertEqual(tabs._wiki(self.tab, self.user, - self.course, self.active_page1), []) + self.course, self.active_page1, FAKE_REQUEST), []) class ExternalLinkTestCase(TestCase): @@ -95,19 +96,19 @@ class ExternalLinkTestCase(TestCase): def test_external_link(self): self.assertEqual(tabs._external_link(self.tabby, self.user, - self.course, self.active_page0)[0].name, + self.course, self.active_page0, FAKE_REQUEST)[0].name, 'same') self.assertEqual(tabs._external_link(self.tabby, self.user, - self.course, self.active_page0)[0].link, + self.course, self.active_page0, FAKE_REQUEST)[0].link, 'blink') self.assertEqual(tabs._external_link(self.tabby, self.user, - self.course, self.active_page0)[0].is_active, + self.course, self.active_page0, FAKE_REQUEST)[0].is_active, False) self.assertEqual(tabs._external_link(self.tabby, self.user, - self.course, self.active_page00)[0].is_active, + self.course, self.active_page00, FAKE_REQUEST)[0].is_active, False) @@ -125,20 +126,20 @@ class StaticTabTestCase(TestCase): def test_static_tab(self): self.assertEqual(tabs._static_tab(self.tabby, self.user, - self.course, self.active_page1)[0].name, + self.course, self.active_page1, FAKE_REQUEST)[0].name, 'same') self.assertEqual(tabs._static_tab(self.tabby, self.user, - self.course, self.active_page1)[0].link, + self.course, self.active_page1, FAKE_REQUEST)[0].link, reverse('static_tab', args=[self.course.id, self.tabby['url_slug']])) self.assertEqual(tabs._static_tab(self.tabby, self.user, - self.course, self.active_page1)[0].is_active, + self.course, self.active_page1, FAKE_REQUEST)[0].is_active, True) self.assertEqual(tabs._static_tab(self.tabby, self.user, - self.course, self.active_page0)[0].is_active, + self.course, self.active_page0, FAKE_REQUEST)[0].is_active, False) @@ -166,45 +167,45 @@ class TextbooksTestCase(TestCase): def test_textbooks1(self): self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_page0)[0].name, + self.course, self.active_page0, FAKE_REQUEST)[0].name, 'Algebra') self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_page0)[0].link, + self.course, self.active_page0, FAKE_REQUEST)[0].link, reverse('book', args=[self.course.id, 0])) self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_page0)[0].is_active, + self.course, self.active_page0, FAKE_REQUEST)[0].is_active, True) self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_pageX)[0].is_active, + self.course, self.active_pageX, FAKE_REQUEST)[0].is_active, False) self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_page1)[1].name, + self.course, self.active_page1, FAKE_REQUEST)[1].name, 'Topology') self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_page1)[1].link, + self.course, self.active_page1, FAKE_REQUEST)[1].link, reverse('book', args=[self.course.id, 1])) self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_page1)[1].is_active, + self.course, self.active_page1, FAKE_REQUEST)[1].is_active, True) self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_pageX)[1].is_active, + self.course, self.active_pageX, FAKE_REQUEST)[1].is_active, False) @override_settings(MITX_FEATURES={'ENABLE_TEXTBOOK': False}) def test_textbooks0(self): self.assertEqual(tabs._textbooks(self.tab, self.mockuser1, - self.course, self.active_pageX), []) + self.course, self.active_pageX, FAKE_REQUEST), []) self.assertEqual(tabs._textbooks(self.tab, self.mockuser0, - self.course, self.active_pageX), []) + self.course, self.active_pageX, FAKE_REQUEST), []) class KeyCheckerTestCase(TestCase): diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 695d7ea55d..66c91f1b9b 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -728,6 +728,7 @@ def submission_history(request, course_id, student_username, location): Right now this only works for problems because that's all StudentModuleHistory records. """ + course = get_course_with_access(request.user, course_id, 'load') staff_access = has_access(request.user, course, 'staff') diff --git a/lms/envs/common.py b/lms/envs/common.py index 466ec262f9..47f0083957 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -80,7 +80,7 @@ MITX_FEATURES = { 'ENABLE_PSYCHOMETRICS': False, # real-time psychometrics (eg item response theory analysis in instructor dashboard) - 'ENABLE_DJANGO_ADMIN_SITE': False, # set true to enable django's admin site, even on prod (e.g. for course ops) + 'ENABLE_DJANGO_ADMIN_SITE': True, # set true to enable django's admin site, even on prod (e.g. for course ops) 'ENABLE_SQL_TRACKING_LOGS': False, 'ENABLE_LMS_MIGRATION': False, 'ENABLE_MANUAL_GIT_RELOAD': False, @@ -523,6 +523,14 @@ MOCK_STAFF_GRADING = False ################################# Jasmine ################################### JASMINE_TEST_DIRECTORY = PROJECT_ROOT + '/static/coffee' +################################# Waffle ################################### + +# Name prepended to cookies set by Waffle +WAFFLE_COOKIE = "waffle_flag_%s" + +# Two weeks (in sec) +WAFFLE_MAX_AGE = 1209600 + ################################# Middleware ################################### # List of finder classes that know how to find static files in # various locations. @@ -570,6 +578,9 @@ MIDDLEWARE_CLASSES = ( # catches any uncaught RateLimitExceptions and returns a 403 instead of a 500 'ratelimitbackend.middleware.RateLimitMiddleware', + + # For A/B testing + 'waffle.middleware.WaffleMiddleware', ) ############################### Pipeline ####################################### @@ -832,6 +843,9 @@ INSTALLED_APPS = ( # Foldit integration 'foldit', + # For A/B testing + 'waffle', + # For testing 'django.contrib.admin', # only used in DEBUG mode 'django_nose', diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 1ec5030f7a..c596208b3f 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -255,7 +255,7 @@ ANALYTICS_API_KEY = "" ##### segment-io ###### -# If there's an environment variable set, grab it and turn on segment io +# If there's an environment variable set, grab it and turn on Segment.io SEGMENT_IO_LMS_KEY = os.environ.get('SEGMENT_IO_LMS_KEY') if SEGMENT_IO_LMS_KEY: MITX_FEATURES['SEGMENT_IO_LMS'] = True diff --git a/lms/static/sass/course/layout/_courseware_header.scss b/lms/static/sass/course/layout/_courseware_header.scss index 77a80b481c..95765fc93c 100644 --- a/lms/static/sass/course/layout/_courseware_header.scss +++ b/lms/static/sass/course/layout/_courseware_header.scss @@ -23,6 +23,17 @@ nav.course-material { list-style: none; margin-right: 6px; + &.prominent { + margin-right: 16px; + background: rgba(255, 255, 255, .5); + border-radius: 3px; + } + + &.prominent + li { + padding-left: 15px; + border-left: 1px solid #333; + } + a { border-radius: 3px; color: #555; diff --git a/lms/templates/courseware/course_navigation.html b/lms/templates/courseware/course_navigation.html index 303a12f142..8cd5368ad0 100644 --- a/lms/templates/courseware/course_navigation.html +++ b/lms/templates/courseware/course_navigation.html @@ -13,19 +13,24 @@ def url_class(is_active): %> <%! from courseware.tabs import get_course_tabs %> <%! from django.utils.translation import ugettext as _ %> +<% import waffle %>