Skip to content

Split Testing #829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rlucioni merged 3 commits into from
Sep 25, 2013
Merged

Split Testing #829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1ccf1c
Integrate split testing and LMS tabs experiments
rlucioni Sep 9, 2013
e7e03f1
Refactor tab tests
rlucioni Sep 18, 2013
ac0e65a
Update CHANGELOG
rlucioni Sep 25, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes,
in roughly chronological order, most recent first. Add your entries at or near
the top. Include a label indicating the component affected.

LMS: Add split testing functionality for internal use.

LMS: Improved accessibility of parts of forum navigation sidebar.

LMS: enhanced accessibility labeling and aria support for the discussion forum new post dropdown as well as response and comment area labeling.
Expand Down
99 changes: 65 additions & 34 deletions lms/djangoapps/courseware/tabs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

from open_ended_grading import open_ended_notifications

import waffle

log = logging.getLogger(__name__)


Expand Down Expand Up @@ -55,32 +57,46 @@ def CourseTab(name, link, is_active, has_img=False, img=""):


##### Generators for various tabs.

def _courseware(tab, user, course, active_page):
def _courseware(tab, user, course, active_page, request):

@cpennington cpennington Aug 30, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until this change, tab generators had the same signature. I think I'd rather have that remain true and have the rest of them not use the request argument then to have a conditional that changes the arguments passed based on the tab name.

@rlucioni rlucioni Aug 30, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is meant to be a short-term experiment, would you have me make this change before we know that this use of the request argument is going to make it into the codebase permanently?

@cpennington cpennington Aug 30, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd still rather keep the parameters consistent, even if it's only temporary. @nedbat, thoughts on the matter?

@rlucioni rlucioni Sep 4, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nedbat, what are your thoughts on this?

P.S. I made this:
nedbat

@nedbat nedbat Sep 4, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for "courseware" below that adds the request to the args list is definitely gross :) Other options: 1) as Cale suggests, change the signature of all of the tabs. 2) use inspect to see if the function has a request parameter, and provide the request if it does. 3) access the request implicitly inside the tab function using one of these techniques: http://nedbatchelder.com/blog/201008/global_django_requests.html (also icky)

The image is very nice! :) And it seems to have worked, because here I am!

"""
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.
"""
Expand All @@ -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.
"""
Expand All @@ -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.
"""
Expand All @@ -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.
"""
Expand All @@ -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])

Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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


#### Validators
def key_checker(expected_keys):
"""
Returns a function that checks that specified keys are present in a dict
Expand Down Expand Up @@ -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}"
Expand All @@ -280,25 +301,31 @@ 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
# shouldn't) know about the details of what tabs are supported, etc.
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'):
Expand All @@ -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
Expand All @@ -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])
Expand All @@ -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)
Expand Down
Loading