From c9f7558d6d30031327c137fdd46f7a14c37f5f01 Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Fri, 16 Nov 2012 18:34:26 -0500
Subject: [PATCH 1/8] first pass at add/remove moderators

---
 lms/djangoapps/instructor/views.py            | 101 +++++++++++++++---
 .../courseware/instructor_dashboard.html      |  29 +++++
 2 files changed, 117 insertions(+), 13 deletions(-)

diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py
index 0b6392a7fc..0b755145b3 100644
--- a/lms/djangoapps/instructor/views.py
+++ b/lms/djangoapps/instructor/views.py
@@ -1,42 +1,43 @@
 # ======== Instructor views =============================================================================
 
 import csv
-import itertools
-import json
+#import itertools
+#import json
 import logging
 import os
 import urllib
 
 import track.views
 
-from functools import partial
+#from functools import partial
 from collections import defaultdict
 
 from django.conf import settings
-from django.core.context_processors import csrf
-from django.core.urlresolvers import reverse
+#from django.core.context_processors import csrf
+#from django.core.urlresolvers import reverse
 from django.contrib.auth.models import User, Group
-from django.contrib.auth.decorators import login_required
-from django.http import Http404, HttpResponse
-from django.shortcuts import redirect
-from mitxmako.shortcuts import render_to_response, render_to_string
+#from django.contrib.auth.decorators import login_required
+from django.http import HttpResponse
+# from django.shortcuts import redirect
+from mitxmako.shortcuts import render_to_response #, render_to_string
 #from django.views.decorators.csrf import ensure_csrf_cookie
 from django_future.csrf import ensure_csrf_cookie
 from django.views.decorators.cache import cache_control
 
 from courseware import grades
 from courseware.access import has_access, get_access_group_name
-from courseware.courses import (get_course_with_access, get_courses_by_university)
+from courseware.courses import get_course_with_access #, get_courses_by_university)
 from psychometrics import psychoanalyze
-from student.models import UserProfile
+#from student.models import UserProfile
 
-from student.models import UserTestGroup, CourseEnrollment
-from util.cache import cache, cache_if_anonymous
+from student.models import CourseEnrollment
+# from util.cache import cache, cache_if_anonymous
 from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
 from xmodule.modulestore.search import path_to_location
+from django_comment_client.models import Role
 
 log = logging.getLogger("mitx.courseware")
 
@@ -203,6 +204,55 @@ def instructor_dashboard(request, course_id):
             user.groups.remove(group)
             track.views.server_track(request, 'remove-staff %s' % user, {}, page='idashboard')
 
+    #----------------------------------------
+    # forum administration
+  
+
+    elif action == 'List course forum administrators':
+        pass
+    
+    elif action == 'Remove forum admin':
+        uname = request.POST['forumadmin']
+        msg += _update_forum_role_membership(uname, course_id, 'Administrator', 'remove')
+        track.views.server_track(request, '%s %s as %s for %s' % ('remove', uname, 'Administrator', course_id), 
+                                 {}, page='idashboard')
+
+    elif action == 'Add forum admin':
+        uname = request.POST['forumadmin']
+        msg += _update_forum_role_membership(uname, course_id, 'Administrator', 'add')
+        track.views.server_track(request, '%s %s as %s for %s' % ('add', uname, 'Administrator', course_id), 
+                                 {}, page='idashboard')
+
+    elif action == 'List course forum moderators':
+        pass
+    
+    elif action == 'Remove forum moderator':
+        uname = request.POST['forummoderator']
+        msg += _update_forum_role_membership(uname, course_id, 'Moderator', 'remove')
+        track.views.server_track(request, '%s %s as %s for %s' % ('remove', uname, 'Moderator', course_id), 
+                                 {}, page='idashboard')
+    
+    elif action == 'Add forum moderator':
+        uname = request.POST['forummoderator']
+        msg += _update_forum_role_membership(uname, course_id, 'Moderator', 'add')
+        track.views.server_track(request, '%s %s as %s for %s' % ('add', uname, 'Moderator', course_id), 
+                                 {}, page='idashboard')
+    
+    elif action == 'List course forum community TAs':
+        pass
+    
+    elif action == 'Remove forum community TA':
+        uname = request.POST['forumcommunityta']
+        msg += _update_forum_role_membership(uname, course_id, 'Community TA', 'remove')
+        track.views.server_track(request, '%s %s as %s for %s' % ('remove', uname, 'Community TA', course_id), 
+                                 {}, page='idashboard')
+    
+    elif action == 'Add forum community TA':
+        uname = request.POST['forumcommunityta']
+        msg += _update_forum_role_membership(uname, course_id, 'Community TA', 'add')
+        track.views.server_track(request, '%s %s as %s for %s' % ('add', uname, 'Community TA', course_id), 
+                                 {}, page='idashboard')
+
     #----------------------------------------
     # psychometrics
 
@@ -215,6 +265,8 @@ def instructor_dashboard(request, course_id):
     if idash_mode=='Psychometrics':
         problems = psychoanalyze.problems_with_psychometric_data(course_id)
 
+
+
     #----------------------------------------
     # context for rendering
     context = {'course': course,
@@ -232,6 +284,29 @@ def instructor_dashboard(request, course_id):
 
     return render_to_response('courseware/instructor_dashboard.html', context)
 
+def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
+    '''
+    
+    
+    returns message status to append to displayed message
+    '''
+    msg = ''
+    try:
+        user = User.objects.get(username=uname)
+    except User.DoesNotExist:
+        return '<font color="red">Error: unknown username "%s"</font>' % uname
+    
+    if user is not None:
+        role = Role.objects.get(name=rolename, course_id=course_id)
+        log.debug('rolename=%s' % rolename)
+        if (add_or_remove == 'remove'):
+            user.roles.remove(role)
+            msg += '<font color="green">Removed %s from %s forum role = %s</font>' % (user, rolename)
+        else:
+            user.roles.add(role)
+            msg += '<font color="green">Added %s to %s forum role = %s</font>' % (user, rolename)
+    return msg
+    
 
 def get_student_grade_summary_data(request, course, course_id, get_grades=True, get_raw_scores=False):
     '''
diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html
index e822f05f92..4a55e4bf14 100644
--- a/lms/templates/courseware/instructor_dashboard.html
+++ b/lms/templates/courseware/instructor_dashboard.html
@@ -57,6 +57,7 @@ function goto( mode)
             <a href="#" onclick="goto('Psychometrics');" class="${modeflag.get('Psychometrics')}">Psychometrics</a> | 
           %endif
           <a href="#" onclick="goto('Admin');" class="${modeflag.get('Admin')}">Admin</a> ]
+          <a href="#" onclick="goto('Forum Admin');" class="${modeflag.get('Admin')}">Forum Admin</a> ]
     </h2>
 
   <div style="text-align:right" id="djangopid">${djangopid}</div>
@@ -134,6 +135,34 @@ function goto( mode)
   %endif
 %endif
 
+##----------------------------------------------------------------------------- 
+%if modeflag.get('Forum Admin'):
+  %if instructor_access:
+    <hr width="40%" style="align:left">
+    <p>
+    <input type="submit" name="action" value="List course forum administrators">
+    <p>
+    <input type="text" name="forumadmin"> <input type="submit" name="action" value="Remove forum admin">
+    <input type="submit" name="action" value="Add forum admin">
+    <hr width="40%" style="align:left">
+  %endif
+
+  %if instructor_access or forum_admin_access:
+    <p>
+    <input type="submit" name="action" value="List course forum moderators">
+    <p>
+    <input type="text" name="forummoderator"> <input type="submit" name="action" value="Remove forum moderator">
+    <input type="submit" name="action" value="Add forum moderator">
+    <hr width="40%" style="align:left">
+    <p>
+    <input type="submit" name="action" value="List course forum community TAs">
+    <p>
+    <input type="text" name="forumcommunityta"> <input type="submit" name="action" value="Remove forum community TA">
+    <input type="submit" name="action" value="Add forum community TA">
+    <hr width="40%" style="align:left">
+  %endif
+%endif
+
     </form>
 
 ##----------------------------------------------------------------------------- 

From 8b1b63003b901498d0e796fd53e9bde8565997b1 Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Mon, 19 Nov 2012 00:37:43 -0500
Subject: [PATCH 2/8] add listing forum moderator to instructor dash

---
 lms/djangoapps/instructor/views.py | 34 +++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py
index 0b755145b3..5ae92ceb3b 100644
--- a/lms/djangoapps/instructor/views.py
+++ b/lms/djangoapps/instructor/views.py
@@ -46,6 +46,7 @@ template_imports = {'urllib': urllib}
 
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
+
 def instructor_dashboard(request, course_id):
     """Display the instructor dashboard for a course."""
     course = get_course_with_access(request.user, course_id, 'staff')
@@ -209,7 +210,11 @@ def instructor_dashboard(request, course_id):
   
 
     elif action == 'List course forum administrators':
-        pass
+        rolename = 'Administrator'
+        datatable = {}
+        msg += _list_course_forum_members(course_id, rolename, datatable)
+        track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
+        
     
     elif action == 'Remove forum admin':
         uname = request.POST['forumadmin']
@@ -224,7 +229,10 @@ def instructor_dashboard(request, course_id):
                                  {}, page='idashboard')
 
     elif action == 'List course forum moderators':
-        pass
+        rolename = 'Moderator'
+        datatable = {}
+        msg += _list_course_forum_members(course_id, rolename, datatable)
+        track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
     
     elif action == 'Remove forum moderator':
         uname = request.POST['forummoderator']
@@ -239,7 +247,10 @@ def instructor_dashboard(request, course_id):
                                  {}, page='idashboard')
     
     elif action == 'List course forum community TAs':
-        pass
+        rolename = 'Community TA'
+        datatable = {}
+        msg += _list_course_forum_members(course_id, rolename, datatable)
+        track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
     
     elif action == 'Remove forum community TA':
         uname = request.POST['forumcommunityta']
@@ -284,6 +295,19 @@ def instructor_dashboard(request, course_id):
 
     return render_to_response('courseware/instructor_dashboard.html', context)
 
+def _list_course_forum_members(course_id, rolename, datatable):
+    ''' TODO
+    '''
+    role = Role.objects.get(name=rolename, course_id=course_id)
+    uset = role.users.all()
+    msg = 'Role = %s' % rolename
+    log.debug('role=%s' % rolename)
+    datatable['header'] = ['Username', 'Full name', 'Roles']
+    datatable['data'] = [[x.username, x.profile.name, ', '.join([r.name for r in x.roles.all()])] for x in uset]
+    datatable['title'] = 'List of Forum %s in course %s' % (rolename, course_id)
+    return msg
+
+
 def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
     '''
     
@@ -301,10 +325,10 @@ def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
         log.debug('rolename=%s' % rolename)
         if (add_or_remove == 'remove'):
             user.roles.remove(role)
-            msg += '<font color="green">Removed %s from %s forum role = %s</font>' % (user, rolename)
+            msg += '<font color="green">Removed %s from %s forum role = %s</font>' % (user, course_id, rolename)
         else:
             user.roles.add(role)
-            msg += '<font color="green">Added %s to %s forum role = %s</font>' % (user, rolename)
+            msg += '<font color="green">Added %s to %s forum role = %s</font>' % (user, course_id, rolename)
     return msg
     
 

From ee7a8494e6dec37bf41ea89cb6ee79a2b8906fd3 Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Mon, 19 Nov 2012 14:59:34 -0500
Subject: [PATCH 3/8] finish basic implementation of forum admin on instructor
 dash

---
 .../django_comment_client/models.py           |   2 +-
 lms/djangoapps/instructor/views.py            | 102 ++++++++++++------
 .../courseware/instructor_dashboard.html      |  16 +--
 3 files changed, 77 insertions(+), 43 deletions(-)

diff --git a/lms/djangoapps/django_comment_client/models.py b/lms/djangoapps/django_comment_client/models.py
index ff2146afac..b1b8f985c6 100644
--- a/lms/djangoapps/django_comment_client/models.py
+++ b/lms/djangoapps/django_comment_client/models.py
@@ -15,7 +15,7 @@ class Role(models.Model):
     def inherit_permissions(self, role): # TODO the name of this method is a little bit confusing,
                                          # since it's one-off and doesn't handle inheritance later
         if role.course_id and role.course_id != self.course_id:
-            logging.warning("%s cannot inheret permissions from %s due to course_id inconsistency" % 
+            logging.warning("%s cannot inherit permissions from %s due to course_id inconsistency" % 
                             (self, role))
         for per in role.permissions.all():
             self.add_permission(per)
diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py
index 5ae92ceb3b..c78c360b70 100644
--- a/lms/djangoapps/instructor/views.py
+++ b/lms/djangoapps/instructor/views.py
@@ -43,6 +43,11 @@ log = logging.getLogger("mitx.courseware")
 
 template_imports = {'urllib': urllib}
 
+FORUM_ROLE_ADMINISTRATOR = 'Administrator'
+FORUM_ROLE_MODERATOR = 'Moderator'
+FORUM_ROLE_COMMUNITY_TA = 'Community TA'
+FORUM_ROLE_ADD = 'add'
+FORUM_ROLE_REMOVE = 'remove'
 
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
@@ -208,9 +213,8 @@ def instructor_dashboard(request, course_id):
     #----------------------------------------
     # forum administration
   
-
     elif action == 'List course forum administrators':
-        rolename = 'Administrator'
+        rolename = FORUM_ROLE_ADMINISTRATOR
         datatable = {}
         msg += _list_course_forum_members(course_id, rolename, datatable)
         track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
@@ -218,50 +222,50 @@ def instructor_dashboard(request, course_id):
     
     elif action == 'Remove forum admin':
         uname = request.POST['forumadmin']
-        msg += _update_forum_role_membership(uname, course_id, 'Administrator', 'remove')
-        track.views.server_track(request, '%s %s as %s for %s' % ('remove', uname, 'Administrator', course_id), 
+        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_REMOVE)
+        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
                                  {}, page='idashboard')
 
     elif action == 'Add forum admin':
         uname = request.POST['forumadmin']
-        msg += _update_forum_role_membership(uname, course_id, 'Administrator', 'add')
-        track.views.server_track(request, '%s %s as %s for %s' % ('add', uname, 'Administrator', course_id), 
+        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_ADD)
+        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
                                  {}, page='idashboard')
 
     elif action == 'List course forum moderators':
-        rolename = 'Moderator'
+        rolename = FORUM_ROLE_MODERATOR
         datatable = {}
         msg += _list_course_forum_members(course_id, rolename, datatable)
         track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
     
     elif action == 'Remove forum moderator':
         uname = request.POST['forummoderator']
-        msg += _update_forum_role_membership(uname, course_id, 'Moderator', 'remove')
-        track.views.server_track(request, '%s %s as %s for %s' % ('remove', uname, 'Moderator', course_id), 
+        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_MODERATOR, FORUM_ROLE_REMOVE)
+        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_MODERATOR, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'Add forum moderator':
         uname = request.POST['forummoderator']
-        msg += _update_forum_role_membership(uname, course_id, 'Moderator', 'add')
-        track.views.server_track(request, '%s %s as %s for %s' % ('add', uname, 'Moderator', course_id), 
+        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_MODERATOR, FORUM_ROLE_ADD)
+        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_MODERATOR, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'List course forum community TAs':
-        rolename = 'Community TA'
+        rolename = FORUM_ROLE_COMMUNITY_TA
         datatable = {}
         msg += _list_course_forum_members(course_id, rolename, datatable)
         track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
     
     elif action == 'Remove forum community TA':
-        uname = request.POST['forumcommunityta']
-        msg += _update_forum_role_membership(uname, course_id, 'Community TA', 'remove')
-        track.views.server_track(request, '%s %s as %s for %s' % ('remove', uname, 'Community TA', course_id), 
+        uname = request.POST['forummoderator']
+        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_REMOVE)
+        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'Add forum community TA':
-        uname = request.POST['forumcommunityta']
-        msg += _update_forum_role_membership(uname, course_id, 'Community TA', 'add')
-        track.views.server_track(request, '%s %s as %s for %s' % ('add', uname, 'Community TA', course_id), 
+        uname = request.POST['forummoderator']
+        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_ADD)
+        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
                                  {}, page='idashboard')
 
     #----------------------------------------
@@ -296,39 +300,69 @@ def instructor_dashboard(request, course_id):
     return render_to_response('courseware/instructor_dashboard.html', context)
 
 def _list_course_forum_members(course_id, rolename, datatable):
-    ''' TODO
+    ''' 
+    Fills in datatable with forum membership information, for a given role,
+    so that it will be displayed on instructor dashboard.
+    
+      course_ID = course's ID string
+      rolename = one of "Administrator", "Moderator", "Community TA"
+    
+    Returns message status string to append to displayed message, if role is unknown.
     '''
-    role = Role.objects.get(name=rolename, course_id=course_id)
-    uset = role.users.all()
+    # make sure datatable is set up properly for display first, before checking for errors
+    datatable['header'] = ['Username', 'Full name', 'Roles']
+    datatable['title'] = 'List of Forum %ss in course %s' % (rolename, course_id)
+    datatable['data'] = [];
+    try:
+        role = Role.objects.get(name=rolename, course_id=course_id)
+    except Role.DoesNotExist:
+        return '<font color="red">Error: unknown rolename "%s"</font>' % rolename
+    uset = role.users.all().order_by('username')
     msg = 'Role = %s' % rolename
     log.debug('role=%s' % rolename)
-    datatable['header'] = ['Username', 'Full name', 'Roles']
-    datatable['data'] = [[x.username, x.profile.name, ', '.join([r.name for r in x.roles.all()])] for x in uset]
-    datatable['title'] = 'List of Forum %s in course %s' % (rolename, course_id)
+    datatable['data'] = [[x.username, x.profile.name, ', '.join([r.name for r in x.roles.filter(course_id=course_id).order_by('name')])] for x in uset]
     return msg
 
 
 def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
     '''
+    Supports adding a user to a course's forum role
     
-    
-    returns message status to append to displayed message
+      uname = username string for user
+      course_ID = course's ID string
+      rolename = one of "Administrator", "Moderator", "Community TA"
+      add_or_remove = one of "add" or "remove"
+      
+    Returns message status string to append to displayed message,  Status is returned if user 
+    or role is unknown, or if entry already exists when adding, or if entry doesn't exist when removing.
     '''
-    msg = ''
+    # check that username and rolename are valid:
     try:
         user = User.objects.get(username=uname)
     except User.DoesNotExist:
         return '<font color="red">Error: unknown username "%s"</font>' % uname
-    
-    if user is not None:
+    try:
         role = Role.objects.get(name=rolename, course_id=course_id)
-        log.debug('rolename=%s' % rolename)
-        if (add_or_remove == 'remove'):
+    except Role.DoesNotExist:
+        return '<font color="red">Error: unknown rolename "%s"</font>' % rolename
+
+    # check whether role already has the specified user:
+    alreadyexists = role.users.filter(username=uname).exists()
+    msg = ''
+    log.debug('rolename=%s' % rolename)
+    if (add_or_remove == FORUM_ROLE_REMOVE):
+        if (not alreadyexists):
+            msg ='<font color="red">Error: user %s does not have rolename "%s", cannot remove</font>' % (uname, rolename)
+        else: 
             user.roles.remove(role)
-            msg += '<font color="green">Removed %s from %s forum role = %s</font>' % (user, course_id, rolename)
-        else:
+            msg = '<font color="green">Removed %s from %s forum role = %s</font>' % (user, course_id, rolename)
+    else:
+        if (alreadyexists):
+            msg = '<font color="red">Error: user %s already has rolename "%s", cannot add</font>' % (uname, rolename)
+        else: 
             user.roles.add(role)
-            msg += '<font color="green">Added %s to %s forum role = %s</font>' % (user, course_id, rolename)
+            msg = '<font color="green">Added %s to %s forum role = %s</font>' % (user, course_id, rolename)
+
     return msg
     
 
diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html
index 4a55e4bf14..69bfabf64d 100644
--- a/lms/templates/courseware/instructor_dashboard.html
+++ b/lms/templates/courseware/instructor_dashboard.html
@@ -56,8 +56,8 @@ function goto( mode)
           %if settings.MITX_FEATURES.get('ENABLE_PSYCHOMETRICS'):
             <a href="#" onclick="goto('Psychometrics');" class="${modeflag.get('Psychometrics')}">Psychometrics</a> | 
           %endif
-          <a href="#" onclick="goto('Admin');" class="${modeflag.get('Admin')}">Admin</a> ]
-          <a href="#" onclick="goto('Forum Admin');" class="${modeflag.get('Admin')}">Forum Admin</a> ]
+          <a href="#" onclick="goto('Admin');" class="${modeflag.get('Admin')}">Admin</a> |
+          <a href="#" onclick="goto('Forum Admin');" class="${modeflag.get('Forum Admin')}">Forum Admin</a> ]
     </h2>
 
   <div style="text-align:right" id="djangopid">${djangopid}</div>
@@ -150,16 +150,16 @@ function goto( mode)
   %if instructor_access or forum_admin_access:
     <p>
     <input type="submit" name="action" value="List course forum moderators">
-    <p>
-    <input type="text" name="forummoderator"> <input type="submit" name="action" value="Remove forum moderator">
-    <input type="submit" name="action" value="Add forum moderator">
-    <hr width="40%" style="align:left">
-    <p>
     <input type="submit" name="action" value="List course forum community TAs">
     <p>
-    <input type="text" name="forumcommunityta"> <input type="submit" name="action" value="Remove forum community TA">
+    <input type="text" name="forummoderator"> 
+    <input type="submit" name="action" value="Remove forum moderator">
+    <input type="submit" name="action" value="Add forum moderator">
+    <input type="submit" name="action" value="Remove forum community TA">
     <input type="submit" name="action" value="Add forum community TA">
     <hr width="40%" style="align:left">
+  %else:
+  <p>User requires forum administrator privileges to perform administration tasks.  See instructor.</p>
   %endif
 %endif
 

From 7ac7f9ef22dc162df093b5360867192678678da4 Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Mon, 19 Nov 2012 18:12:57 -0500
Subject: [PATCH 4/8] start work on unit test.

---
 lms/djangoapps/instructor/tests.py | 61 ++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py
index e948771d6d..fbe00c4b35 100644
--- a/lms/djangoapps/instructor/tests.py
+++ b/lms/djangoapps/instructor/tests.py
@@ -82,3 +82,64 @@ class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader):
 "2","u2","Fred Weasley","view2@test.com","","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0.0","0.0"
 '''
         self.assertEqual(body, expected_body, msg)
+        
+
+@override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE)
+class TestInstructorDashboardForumAdmin(ct.PageLoader):
+    '''
+    Check for change in forum admin role memberships
+    '''
+
+    def setUp(self):
+        xmodule.modulestore.django._MODULESTORES = {}
+        courses = modulestore().get_courses()
+
+        def find_course(name):
+            """Assumes the course is present"""
+            return [c for c in courses if c.location.course==name][0]
+
+        self.full = find_course("full")
+        self.toy = find_course("toy")
+
+        # Create two accounts
+        self.student = 'view@test.com'
+        self.instructor = 'view2@test.com'
+        self.password = 'foo'
+        self.create_account('u1', self.student, self.password)
+        self.create_account('u2', self.instructor, self.password)
+        self.activate_user(self.student)
+        self.activate_user(self.instructor)
+
+        group_name = _course_staff_group_name(self.toy.location)
+        g = Group.objects.create(name=group_name)
+        g.user_set.add(ct.user(self.instructor))
+
+        self.logout()
+        self.login(self.instructor, self.password)
+        self.enroll(self.toy)
+
+    def test_add_forum_admin(self):
+        print "running test_add_forum_admin"
+        course = self.toy
+        url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
+#        msg = "url = %s\n" % url
+        response = self.client.post(url, {'action': 'Add forum admin',
+                                          'forumadmin': 'u1'})
+#        msg += "instructor dashboard download csv grades: response = '%s'\n" % response
+#
+#        self.assertEqual(response['Content-Type'],'text/csv',msg)
+#
+#        cdisp = response['Content-Disposition'].replace('TT_2012','2012')  # jenkins course_id is TT_2012_Fall instead of 2012_Fall?
+#        msg += "cdisp = '%s'\n" % cdisp
+#        self.assertEqual(cdisp,'attachment; filename=grades_edX/toy/2012_Fall.csv',msg)
+#
+#        body = response.content.replace('\r','')
+#        msg += "body = '%s'\n" % body
+
+# need to make sure the roles actually exist.  They don't seem to yet....        
+   # <p><font color="red">Error: unknown rolename "Administrator"</font></p>
+
+#        print response
+#        context = response.context
+#        self.assertTrue(context.contains("Added %s to %s forum role = %s" % ('u1', course.id, 'Administrator')))
+        

From 5953be746381ee70166ead952fcbcda2f7df9b1c Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Tue, 20 Nov 2012 01:38:33 -0500
Subject: [PATCH 5/8] fix testing for forum admin privileges

---
 lms/djangoapps/instructor/views.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py
index c78c360b70..11cab79c57 100644
--- a/lms/djangoapps/instructor/views.py
+++ b/lms/djangoapps/instructor/views.py
@@ -1,37 +1,27 @@
 # ======== Instructor views =============================================================================
 
 import csv
-#import itertools
-#import json
 import logging
 import os
 import urllib
 
 import track.views
 
-#from functools import partial
 from collections import defaultdict
 
 from django.conf import settings
-#from django.core.context_processors import csrf
-#from django.core.urlresolvers import reverse
 from django.contrib.auth.models import User, Group
-#from django.contrib.auth.decorators import login_required
 from django.http import HttpResponse
-# from django.shortcuts import redirect
-from mitxmako.shortcuts import render_to_response #, render_to_string
-#from django.views.decorators.csrf import ensure_csrf_cookie
+from mitxmako.shortcuts import render_to_response
 from django_future.csrf import ensure_csrf_cookie
 from django.views.decorators.cache import cache_control
 
 from courseware import grades
 from courseware.access import has_access, get_access_group_name
-from courseware.courses import get_course_with_access #, get_courses_by_university)
+from courseware.courses import get_course_with_access 
 from psychometrics import psychoanalyze
-#from student.models import UserProfile
 
 from student.models import CourseEnrollment
-# from util.cache import cache, cache_if_anonymous
 from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
@@ -43,12 +33,21 @@ log = logging.getLogger("mitx.courseware")
 
 template_imports = {'urllib': urllib}
 
+# TODO: move these to views for forum role
 FORUM_ROLE_ADMINISTRATOR = 'Administrator'
 FORUM_ROLE_MODERATOR = 'Moderator'
 FORUM_ROLE_COMMUNITY_TA = 'Community TA'
 FORUM_ROLE_ADD = 'add'
 FORUM_ROLE_REMOVE = 'remove'
 
+def has_forum_access(uname, course_id, rolename):
+    try:
+        role = Role.objects.get(name=rolename, course_id=course_id)
+    except Role.DoesNotExist:
+        return False
+    return role.users.filter(username=uname).exists()
+
+
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 
@@ -57,7 +56,7 @@ def instructor_dashboard(request, course_id):
     course = get_course_with_access(request.user, course_id, 'staff')
 
     instructor_access = has_access(request.user, course, 'instructor')   # an instructor can manage staff lists
-
+    forum_admin_access = has_forum_access(request.user, course_id, FORUM_ROLE_ADMINISTRATOR)
     msg = ''
     #msg += ('POST=%s' % dict(request.POST)).replace('<','&lt;')
 
@@ -288,6 +287,7 @@ def instructor_dashboard(request, course_id):
                'staff_access': True,
                'admin_access': request.user.is_staff,
                'instructor_access': instructor_access,
+               'forum_admin_access': forum_admin_access,
                'datatable': datatable,
                'msg': msg,
                'modeflag': {idash_mode: 'selectedmode'},

From 57cb8c1e215d542d11802469e10bc93c4af1723c Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Tue, 20 Nov 2012 15:56:18 -0500
Subject: [PATCH 6/8] Add unit tests

---
 .../django_comment_client/models.py           |   7 +-
 lms/djangoapps/django_comment_client/utils.py |  41 +++---
 lms/djangoapps/instructor/tests.py            | 130 +++++++++++++-----
 lms/djangoapps/instructor/views.py            |  28 ++--
 .../courseware/instructor_dashboard.html      |   2 +-
 5 files changed, 135 insertions(+), 73 deletions(-)

diff --git a/lms/djangoapps/django_comment_client/models.py b/lms/djangoapps/django_comment_client/models.py
index b1b8f985c6..ebc9f38e96 100644
--- a/lms/djangoapps/django_comment_client/models.py
+++ b/lms/djangoapps/django_comment_client/models.py
@@ -4,6 +4,11 @@ import logging
 
 from courseware.courses import get_course_by_id
 
+FORUM_ROLE_ADMINISTRATOR = 'Administrator'
+FORUM_ROLE_MODERATOR = 'Moderator'
+FORUM_ROLE_COMMUNITY_TA = 'Community TA'
+FORUM_ROLE_STUDENT = 'Student'
+
 class Role(models.Model):
     name = models.CharField(max_length=30, null=False, blank=False)
     users = models.ManyToManyField(User, related_name="roles")
@@ -28,7 +33,7 @@ class Role(models.Model):
         if self.name == "Student" and \
            (permission.startswith('edit') or permission.startswith('update') or permission.startswith('create')) and \
            (not course.forum_posts_allowed):
-           return False
+            return False
         
         return self.permissions.filter(name=permission).exists()
 
diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py
index b3a1626d22..6bd05f1edd 100644
--- a/lms/djangoapps/django_comment_client/utils.py
+++ b/lms/djangoapps/django_comment_client/utils.py
@@ -1,27 +1,21 @@
-import time
 from collections import defaultdict
-from importlib import import_module
-
-from courseware.models import StudentModuleCache
-from courseware.module_render import get_module
+from django.contrib.auth.models import User
+from django.core.urlresolvers import reverse
+from django.db import connection
+from django.http import HttpResponse
+from django.utils import simplejson
+from django_comment_client.models import Role
+from django_comment_client.permissions import check_permissions_by_view
+from mitxmako import middleware
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.search import path_to_location
-from django.http import HttpResponse
-from django.utils import simplejson
-from django.db import connection
-from django.conf import settings
-from django.core.urlresolvers import reverse
-from django.contrib.auth.models import User
-from django_comment_client.permissions import check_permissions_by_view
-from django_comment_client.models import Role
-from mitxmako import middleware
-
 import logging
-import operator
-import itertools
-import urllib
 import pystache_custom as pystache
+import time
+import urllib
+
+
 
 
 # TODO these should be cached via django's caching rather than in-memory globals
@@ -47,9 +41,16 @@ def get_role_ids(course_id):
     staff = list(User.objects.filter(is_staff=True).values_list('id', flat=True))
     roles_with_ids = {'Staff': staff}
     for role in roles:
-      roles_with_ids[role.name] = list(role.users.values_list('id', flat=True))
+        roles_with_ids[role.name] = list(role.users.values_list('id', flat=True))
     return roles_with_ids
 
+def has_forum_access(uname, course_id, rolename):
+    try:
+        role = Role.objects.get(name=rolename, course_id=course_id)
+    except Role.DoesNotExist:
+        return False
+    return role.users.filter(username=uname).exists()
+
 def get_full_modules():
     global _FULLMODULES
     if not _FULLMODULES:
@@ -132,7 +133,7 @@ def initialize_discussion_info(course):
         return
 
     course_id = course.id
-    url_course_id = course_id.replace('/', '_').replace('.', '_')
+    # url_course_id = course_id.replace('/', '_').replace('.', '_')
 
     all_modules = get_full_modules()[course_id]
 
diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py
index fbe00c4b35..07da4d2895 100644
--- a/lms/djangoapps/instructor/tests.py
+++ b/lms/djangoapps/instructor/tests.py
@@ -8,21 +8,20 @@ Notes for running by hand:
 django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/instructor
 """
 
-import courseware.tests.tests as ct
-
-from nose import SkipTest
-from mock import patch, Mock
-from override_settings import override_settings
-
-# Need access to internal func to put users in the right group
 from courseware.access import _course_staff_group_name
-from django.contrib.auth.models import User, Group
-from django.conf import settings
+from django.contrib.auth.models import \
+    Group # Need access to internal func to put users in the right group
 from django.core.urlresolvers import reverse
-
+from django_comment_client.models import Role, FORUM_ROLE_ADMINISTRATOR, \
+    FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA
+from django_comment_client.utils import has_forum_access
+from override_settings import override_settings
+from xmodule.modulestore.django import modulestore
+import courseware.tests.tests as ct
 import xmodule.modulestore.django
 
-from xmodule.modulestore.django import modulestore
+
+
 
 
 @override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE)
@@ -83,13 +82,22 @@ class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader):
 '''
         self.assertEqual(body, expected_body, msg)
         
+FORUM_ROLES = [ FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA ]
+FORUM_ADMIN_ACTION_SUFFIX = { FORUM_ROLE_ADMINISTRATOR : 'admin', FORUM_ROLE_MODERATOR : 'moderator', FORUM_ROLE_COMMUNITY_TA : 'community TA'}
+FORUM_ADMIN_USER = { FORUM_ROLE_ADMINISTRATOR : 'forumadmin', FORUM_ROLE_MODERATOR : 'forummoderator', FORUM_ROLE_COMMUNITY_TA : 'forummoderator'}
+
+def action_name(operation, rolename):
+    if operation == 'List':
+        return '%s course forum %ss' % (operation, FORUM_ADMIN_ACTION_SUFFIX[rolename])
+    else:
+        return '%s forum %s' % (operation, FORUM_ADMIN_ACTION_SUFFIX[rolename])
 
 @override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE)
 class TestInstructorDashboardForumAdmin(ct.PageLoader):
     '''
     Check for change in forum admin role memberships
     '''
-
+    
     def setUp(self):
         xmodule.modulestore.django._MODULESTORES = {}
         courses = modulestore().get_courses()
@@ -118,28 +126,84 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
         self.login(self.instructor, self.password)
         self.enroll(self.toy)
 
-    def test_add_forum_admin(self):
-        print "running test_add_forum_admin"
+    def initialize_roles(self, course_id):
+        self.admin_role = Role.objects.get_or_create(name=FORUM_ROLE_ADMINISTRATOR, course_id=course_id)[0]
+        self.moderator_role = Role.objects.get_or_create(name=FORUM_ROLE_MODERATOR, course_id=course_id)[0]
+        self.community_ta_role = Role.objects.get_or_create(name=FORUM_ROLE_COMMUNITY_TA, course_id=course_id)[0]
+
+    def test_add_forum_admin_users_for_unknown_user(self):
+        print "running test_add_forum_admin_users_for_unknown_user"
         course = self.toy
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
-#        msg = "url = %s\n" % url
-        response = self.client.post(url, {'action': 'Add forum admin',
-                                          'forumadmin': 'u1'})
-#        msg += "instructor dashboard download csv grades: response = '%s'\n" % response
-#
-#        self.assertEqual(response['Content-Type'],'text/csv',msg)
-#
-#        cdisp = response['Content-Disposition'].replace('TT_2012','2012')  # jenkins course_id is TT_2012_Fall instead of 2012_Fall?
-#        msg += "cdisp = '%s'\n" % cdisp
-#        self.assertEqual(cdisp,'attachment; filename=grades_edX/toy/2012_Fall.csv',msg)
-#
-#        body = response.content.replace('\r','')
-#        msg += "body = '%s'\n" % body
+        username = 'unknown'
+        for action in ['Add', 'Remove']:
+            for rolename in FORUM_ROLES:
+                response = self.client.post(url, {'action': action_name(action, rolename), FORUM_ADMIN_USER[rolename]: username})
+                self.assertTrue(response.content.find('Error: unknown username "%s"' % username)>=0)
 
-# need to make sure the roles actually exist.  They don't seem to yet....        
-   # <p><font color="red">Error: unknown rolename "Administrator"</font></p>
+    def test_add_forum_admin_users_for_missing_roles(self):
+        print "test_add_forum_admin_users_for_missing_roles"
+        course = self.toy
+        url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
+        username = 'u1'
+        for action in ['Add', 'Remove']:
+            for rolename in FORUM_ROLES:
+                response = self.client.post(url, {'action': action_name(action, rolename), FORUM_ADMIN_USER[rolename]: username})
+                self.assertTrue(response.content.find('Error: unknown rolename "%s"' % rolename)>=0)
 
-#        print response
-#        context = response.context
-#        self.assertTrue(context.contains("Added %s to %s forum role = %s" % ('u1', course.id, 'Administrator')))
-        
+    def test_remove_forum_admin_users_for_missing_users(self):
+        print "test_remove_forum_admin_users_for_missing_users"
+        course = self.toy
+        self.initialize_roles(course.id)
+        url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
+        username = 'u1'
+        action = 'Remove'
+        for rolename in FORUM_ROLES:
+            response = self.client.post(url, {'action': action_name(action, rolename), FORUM_ADMIN_USER[rolename]: username})
+            self.assertTrue(response.content.find('Error: user "%s" does not have rolename "%s"' % (username, rolename))>=0)
+
+    def test_add_and_remove_forum_admin_users(self):
+        print "test_add_and_remove_forum_admin_users"
+        course = self.toy
+        self.initialize_roles(course.id)
+        url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
+        username = 'u1'
+        for rolename in FORUM_ROLES:
+            response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
+            self.assertTrue(response.content.find('Added "%s" to "%s" forum role = "%s"' % (username, course.id, rolename))>=0)
+            self.assertTrue(has_forum_access(username, course.id, rolename))
+            response = self.client.post(url, {'action': action_name('Remove', rolename), FORUM_ADMIN_USER[rolename]: username})
+            self.assertTrue(response.content.find('Removed "%s" from "%s" forum role = "%s"' % (username, course.id, rolename))>=0)
+            self.assertFalse(has_forum_access(username, course.id, rolename))
+
+    def test_add_and_readd_forum_admin_users(self):
+        print "test_add_and_readd_forum_admin_users"
+        course = self.toy
+        self.initialize_roles(course.id)
+        url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
+        username = 'u1'
+        for rolename in FORUM_ROLES:
+            # perform an add, and follow with a second identical add:
+            self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
+            response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
+            self.assertTrue(response.content.find('Error: user "%s" already has rolename "%s", cannot add' % (username, rolename))>=0)
+            self.assertTrue(has_forum_access(username, course.id, rolename))
+
+    def test_list_forum_admin_users(self):
+        print "test_list_forum_admin_users"
+        course = self.toy
+        self.initialize_roles(course.id)
+        url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
+        username = 'u1'
+        added_roles = []
+        for rolename in FORUM_ROLES:
+            response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
+            response = self.client.post(url, {'action': action_name('List', rolename), FORUM_ADMIN_USER[rolename]: username})
+            for header in ['Username', 'Full name', 'Roles']:
+                self.assertTrue(response.content.find('<th>%s</th>' % header)>0)
+            self.assertTrue(response.content.find('<td>%s</td>' % username)>=0)
+            # concatenate all roles for user, in sorted order:
+            added_roles.append(rolename)
+            added_roles.sort()
+            roles = ', '.join(added_roles)
+            self.assertTrue(response.content.find('<td>%s</td>' % roles)>=0)
diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py
index 11cab79c57..60cc634cde 100644
--- a/lms/djangoapps/instructor/views.py
+++ b/lms/djangoapps/instructor/views.py
@@ -27,27 +27,17 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
 from xmodule.modulestore.search import path_to_location
-from django_comment_client.models import Role
+from django_comment_client.models import Role, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA
+from django_comment_client.utils import has_forum_access
 
 log = logging.getLogger("mitx.courseware")
 
 template_imports = {'urllib': urllib}
 
-# TODO: move these to views for forum role
-FORUM_ROLE_ADMINISTRATOR = 'Administrator'
-FORUM_ROLE_MODERATOR = 'Moderator'
-FORUM_ROLE_COMMUNITY_TA = 'Community TA'
+# internal commands for managing forum roles:
 FORUM_ROLE_ADD = 'add'
 FORUM_ROLE_REMOVE = 'remove'
 
-def has_forum_access(uname, course_id, rolename):
-    try:
-        role = Role.objects.get(name=rolename, course_id=course_id)
-    except Role.DoesNotExist:
-        return False
-    return role.users.filter(username=uname).exists()
-
-
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 
@@ -56,7 +46,9 @@ def instructor_dashboard(request, course_id):
     course = get_course_with_access(request.user, course_id, 'staff')
 
     instructor_access = has_access(request.user, course, 'instructor')   # an instructor can manage staff lists
+    
     forum_admin_access = has_forum_access(request.user, course_id, FORUM_ROLE_ADMINISTRATOR)
+
     msg = ''
     #msg += ('POST=%s' % dict(request.POST)).replace('<','&lt;')
 
@@ -212,7 +204,7 @@ def instructor_dashboard(request, course_id):
     #----------------------------------------
     # forum administration
   
-    elif action == 'List course forum administrators':
+    elif action == 'List course forum admins':
         rolename = FORUM_ROLE_ADMINISTRATOR
         datatable = {}
         msg += _list_course_forum_members(course_id, rolename, datatable)
@@ -352,16 +344,16 @@ def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
     log.debug('rolename=%s' % rolename)
     if (add_or_remove == FORUM_ROLE_REMOVE):
         if (not alreadyexists):
-            msg ='<font color="red">Error: user %s does not have rolename "%s", cannot remove</font>' % (uname, rolename)
+            msg ='<font color="red">Error: user "%s" does not have rolename "%s", cannot remove</font>' % (uname, rolename)
         else: 
             user.roles.remove(role)
-            msg = '<font color="green">Removed %s from %s forum role = %s</font>' % (user, course_id, rolename)
+            msg = '<font color="green">Removed "%s" from "%s" forum role = "%s"</font>' % (user, course_id, rolename)
     else:
         if (alreadyexists):
-            msg = '<font color="red">Error: user %s already has rolename "%s", cannot add</font>' % (uname, rolename)
+            msg = '<font color="red">Error: user "%s" already has rolename "%s", cannot add</font>' % (uname, rolename)
         else: 
             user.roles.add(role)
-            msg = '<font color="green">Added %s to %s forum role = %s</font>' % (user, course_id, rolename)
+            msg = '<font color="green">Added "%s" to "%s" forum role = "%s"</font>' % (user, course_id, rolename)
 
     return msg
     
diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html
index 69bfabf64d..74bc25fcbe 100644
--- a/lms/templates/courseware/instructor_dashboard.html
+++ b/lms/templates/courseware/instructor_dashboard.html
@@ -140,7 +140,7 @@ function goto( mode)
   %if instructor_access:
     <hr width="40%" style="align:left">
     <p>
-    <input type="submit" name="action" value="List course forum administrators">
+    <input type="submit" name="action" value="List course forum admins">
     <p>
     <input type="text" name="forumadmin"> <input type="submit" name="action" value="Remove forum admin">
     <input type="submit" name="action" value="Add forum admin">

From 3b29ab373d758e796ede133b8df3ca5b222c058d Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Mon, 26 Nov 2012 01:27:16 -0500
Subject: [PATCH 7/8] check for staff access before creating forum admin

---
 lms/djangoapps/instructor/tests.py | 22 +++++++++++++++++-----
 lms/djangoapps/instructor/views.py | 27 +++++++++++++++------------
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py
index 07da4d2895..2c498ab6f4 100644
--- a/lms/djangoapps/instructor/tests.py
+++ b/lms/djangoapps/instructor/tests.py
@@ -167,7 +167,7 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
-        username = 'u1'
+        username = 'u2'
         for rolename in FORUM_ROLES:
             response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
             self.assertTrue(response.content.find('Added "%s" to "%s" forum role = "%s"' % (username, course.id, rolename))>=0)
@@ -181,7 +181,7 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
-        username = 'u1'
+        username = 'u2'
         for rolename in FORUM_ROLES:
             # perform an add, and follow with a second identical add:
             self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
@@ -189,15 +189,27 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
             self.assertTrue(response.content.find('Error: user "%s" already has rolename "%s", cannot add' % (username, rolename))>=0)
             self.assertTrue(has_forum_access(username, course.id, rolename))
 
+    def test_add_nonstaff_forum_admin_users(self):
+        print "test_add_and_readd_forum_admin_users"
+        course = self.toy
+        self.initialize_roles(course.id)
+        url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
+        username = 'u1'
+        rolename = FORUM_ROLE_ADMINISTRATOR
+        response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
+        self.assertTrue(response.content.find('Error: user "%s" should first be added as staff' % username)>=0)
+
     def test_list_forum_admin_users(self):
         print "test_list_forum_admin_users"
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
-        username = 'u1'
-        added_roles = []
+        username = 'u2'
+        added_roles = ['Student']  # u2 is already added as a student to the discussion forums
+        self.assertTrue(has_forum_access(username, course.id, 'Student'))
         for rolename in FORUM_ROLES:
             response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
+            self.assertTrue(has_forum_access(username, course.id, rolename))
             response = self.client.post(url, {'action': action_name('List', rolename), FORUM_ADMIN_USER[rolename]: username})
             for header in ['Username', 'Full name', 'Roles']:
                 self.assertTrue(response.content.find('<th>%s</th>' % header)>0)
@@ -206,4 +218,4 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
             added_roles.append(rolename)
             added_roles.sort()
             roles = ', '.join(added_roles)
-            self.assertTrue(response.content.find('<td>%s</td>' % roles)>=0)
+            self.assertTrue(response.content.find('<td>%s</td>' % roles)>=0, 'not finding roles "%s"' % roles)
diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py
index 60cc634cde..2ae383e25b 100644
--- a/lms/djangoapps/instructor/views.py
+++ b/lms/djangoapps/instructor/views.py
@@ -213,13 +213,13 @@ def instructor_dashboard(request, course_id):
     
     elif action == 'Remove forum admin':
         uname = request.POST['forumadmin']
-        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_REMOVE)
+        msg += _update_forum_role_membership(uname, course, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_REMOVE)
         track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
                                  {}, page='idashboard')
 
     elif action == 'Add forum admin':
         uname = request.POST['forumadmin']
-        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_ADD)
+        msg += _update_forum_role_membership(uname, course, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_ADD)
         track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
                                  {}, page='idashboard')
 
@@ -231,13 +231,13 @@ def instructor_dashboard(request, course_id):
     
     elif action == 'Remove forum moderator':
         uname = request.POST['forummoderator']
-        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_MODERATOR, FORUM_ROLE_REMOVE)
+        msg += _update_forum_role_membership(uname, course, FORUM_ROLE_MODERATOR, FORUM_ROLE_REMOVE)
         track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_MODERATOR, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'Add forum moderator':
         uname = request.POST['forummoderator']
-        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_MODERATOR, FORUM_ROLE_ADD)
+        msg += _update_forum_role_membership(uname, course, FORUM_ROLE_MODERATOR, FORUM_ROLE_ADD)
         track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_MODERATOR, course_id), 
                                  {}, page='idashboard')
     
@@ -249,13 +249,13 @@ def instructor_dashboard(request, course_id):
     
     elif action == 'Remove forum community TA':
         uname = request.POST['forummoderator']
-        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_REMOVE)
+        msg += _update_forum_role_membership(uname, course, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_REMOVE)
         track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'Add forum community TA':
         uname = request.POST['forummoderator']
-        msg += _update_forum_role_membership(uname, course_id, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_ADD)
+        msg += _update_forum_role_membership(uname, course, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_ADD)
         track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
                                  {}, page='idashboard')
 
@@ -316,12 +316,12 @@ def _list_course_forum_members(course_id, rolename, datatable):
     return msg
 
 
-def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
+def _update_forum_role_membership(uname, course, rolename, add_or_remove):
     '''
     Supports adding a user to a course's forum role
     
       uname = username string for user
-      course_ID = course's ID string
+      course = course object 
       rolename = one of "Administrator", "Moderator", "Community TA"
       add_or_remove = one of "add" or "remove"
       
@@ -334,7 +334,7 @@ def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
     except User.DoesNotExist:
         return '<font color="red">Error: unknown username "%s"</font>' % uname
     try:
-        role = Role.objects.get(name=rolename, course_id=course_id)
+        role = Role.objects.get(name=rolename, course_id=course.id)
     except Role.DoesNotExist:
         return '<font color="red">Error: unknown rolename "%s"</font>' % rolename
 
@@ -347,13 +347,16 @@ def _update_forum_role_membership(uname, course_id, rolename, add_or_remove):
             msg ='<font color="red">Error: user "%s" does not have rolename "%s", cannot remove</font>' % (uname, rolename)
         else: 
             user.roles.remove(role)
-            msg = '<font color="green">Removed "%s" from "%s" forum role = "%s"</font>' % (user, course_id, rolename)
+            msg = '<font color="green">Removed "%s" from "%s" forum role = "%s"</font>' % (user, course.id, rolename)
     else:
         if (alreadyexists):
             msg = '<font color="red">Error: user "%s" already has rolename "%s", cannot add</font>' % (uname, rolename)
         else: 
-            user.roles.add(role)
-            msg = '<font color="green">Added "%s" to "%s" forum role = "%s"</font>' % (user, course_id, rolename)
+            if (rolename == FORUM_ROLE_ADMINISTRATOR and not has_access(user, course, 'staff')):   
+                msg = '<font color="red">Error: user "%s" should first be added as staff before adding as a forum administrator, cannot add</font>' % uname
+            else:
+                user.roles.add(role)
+                msg = '<font color="green">Added "%s" to "%s" forum role = "%s"</font>' % (user, course.id, rolename)
 
     return msg
     

From 8d0eb7f1a184059e1425f36337ccd38f638fa49f Mon Sep 17 00:00:00 2001
From: Brian Wilson <brian@edx.org>
Date: Mon, 26 Nov 2012 15:43:18 -0500
Subject: [PATCH 8/8] Respond to feedback (esp. use of .format())

---
 .../django_comment_client/models.py           |   9 +-
 lms/djangoapps/django_comment_client/utils.py |  14 +-
 lms/djangoapps/instructor/tests.py            |  58 ++++----
 lms/djangoapps/instructor/views.py            | 127 +++++++++---------
 4 files changed, 96 insertions(+), 112 deletions(-)

diff --git a/lms/djangoapps/django_comment_client/models.py b/lms/djangoapps/django_comment_client/models.py
index ebc9f38e96..628ac21a4a 100644
--- a/lms/djangoapps/django_comment_client/models.py
+++ b/lms/djangoapps/django_comment_client/models.py
@@ -1,6 +1,7 @@
+import logging
+
 from django.db import models
 from django.contrib.auth.models import User
-import logging
 
 from courseware.courses import get_course_by_id
 
@@ -20,8 +21,8 @@ class Role(models.Model):
     def inherit_permissions(self, role): # TODO the name of this method is a little bit confusing,
                                          # since it's one-off and doesn't handle inheritance later
         if role.course_id and role.course_id != self.course_id:
-            logging.warning("%s cannot inherit permissions from %s due to course_id inconsistency" % 
-                            (self, role))
+            logging.warning("{0} cannot inherit permissions from {1} due to course_id inconsistency", \
+                            self, role)
         for per in role.permissions.all():
             self.add_permission(per)
 
@@ -30,7 +31,7 @@ class Role(models.Model):
 
     def has_permission(self, permission):
         course = get_course_by_id(self.course_id)
-        if self.name == "Student" and \
+        if self.name == FORUM_ROLE_STUDENT and \
            (permission.startswith('edit') or permission.startswith('update') or permission.startswith('create')) and \
            (not course.forum_posts_allowed):
             return False
diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py
index 6bd05f1edd..fbb87a1584 100644
--- a/lms/djangoapps/django_comment_client/utils.py
+++ b/lms/djangoapps/django_comment_client/utils.py
@@ -1,4 +1,8 @@
 from collections import defaultdict
+import logging
+import time
+import urllib
+
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.db import connection
@@ -7,15 +11,11 @@ from django.utils import simplejson
 from django_comment_client.models import Role
 from django_comment_client.permissions import check_permissions_by_view
 from mitxmako import middleware
+import pystache_custom as pystache
+
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.search import path_to_location
-import logging
-import pystache_custom as pystache
-import time
-import urllib
-
-
 
 
 # TODO these should be cached via django's caching rather than in-memory globals
@@ -133,8 +133,6 @@ def initialize_discussion_info(course):
         return
 
     course_id = course.id
-    # url_course_id = course_id.replace('/', '_').replace('.', '_')
-
     all_modules = get_full_modules()[course_id]
 
     discussion_id_map = {}
diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py
index 2c498ab6f4..532c0c3f68 100644
--- a/lms/djangoapps/instructor/tests.py
+++ b/lms/djangoapps/instructor/tests.py
@@ -8,22 +8,21 @@ Notes for running by hand:
 django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/instructor
 """
 
-from courseware.access import _course_staff_group_name
+from override_settings import override_settings
+
 from django.contrib.auth.models import \
     Group # Need access to internal func to put users in the right group
 from django.core.urlresolvers import reverse
 from django_comment_client.models import Role, FORUM_ROLE_ADMINISTRATOR, \
-    FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA
+    FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_STUDENT
 from django_comment_client.utils import has_forum_access
-from override_settings import override_settings
-from xmodule.modulestore.django import modulestore
+
+from courseware.access import _course_staff_group_name
 import courseware.tests.tests as ct
+from xmodule.modulestore.django import modulestore
 import xmodule.modulestore.django
 
 
-
-
-
 @override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE)
 class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader):
     '''
@@ -60,22 +59,20 @@ class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader):
 
 
     def test_download_grades_csv(self):
-        print "running test_download_grades_csv"
         course = self.toy
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
-        msg = "url = %s\n" % url
-        response = self.client.post(url, {'action': 'Download CSV of all student grades for this course',
-                                          })
-        msg += "instructor dashboard download csv grades: response = '%s'\n" % response
+        msg = "url = {0}\n".format(url)
+        response = self.client.post(url, {'action': 'Download CSV of all student grades for this course'})
+        msg += "instructor dashboard download csv grades: response = '{0}'\n".format(response)
 
         self.assertEqual(response['Content-Type'],'text/csv',msg)
 
         cdisp = response['Content-Disposition'].replace('TT_2012','2012')  # jenkins course_id is TT_2012_Fall instead of 2012_Fall?
-        msg += "cdisp = '%s'\n" % cdisp
+        msg += "cdisp = '{0}'\n".format(cdisp)
         self.assertEqual(cdisp,'attachment; filename=grades_edX/toy/2012_Fall.csv',msg)
 
         body = response.content.replace('\r','')
-        msg += "body = '%s'\n" % body
+        msg += "body = '{0}'\n".format(body)
 
         expected_body = '''"ID","Username","Full Name","edX email","External email","HW 01","HW 02","HW 03","HW 04","HW 05","HW 06","HW 07","HW 08","HW 09","HW 10","HW 11","HW 12","HW Avg","Lab 01","Lab 02","Lab 03","Lab 04","Lab 05","Lab 06","Lab 07","Lab 08","Lab 09","Lab 10","Lab 11","Lab 12","Lab Avg","Midterm","Final"
 "2","u2","Fred Weasley","view2@test.com","","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0.0","0.0"
@@ -88,9 +85,9 @@ FORUM_ADMIN_USER = { FORUM_ROLE_ADMINISTRATOR : 'forumadmin', FORUM_ROLE_MODERAT
 
 def action_name(operation, rolename):
     if operation == 'List':
-        return '%s course forum %ss' % (operation, FORUM_ADMIN_ACTION_SUFFIX[rolename])
+        return '{0} course forum {1}s'.format(operation, FORUM_ADMIN_ACTION_SUFFIX[rolename])
     else:
-        return '%s forum %s' % (operation, FORUM_ADMIN_ACTION_SUFFIX[rolename])
+        return '{0} forum {1}'.format(operation, FORUM_ADMIN_ACTION_SUFFIX[rolename])
 
 @override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE)
 class TestInstructorDashboardForumAdmin(ct.PageLoader):
@@ -132,27 +129,24 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
         self.community_ta_role = Role.objects.get_or_create(name=FORUM_ROLE_COMMUNITY_TA, course_id=course_id)[0]
 
     def test_add_forum_admin_users_for_unknown_user(self):
-        print "running test_add_forum_admin_users_for_unknown_user"
         course = self.toy
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
         username = 'unknown'
         for action in ['Add', 'Remove']:
             for rolename in FORUM_ROLES:
                 response = self.client.post(url, {'action': action_name(action, rolename), FORUM_ADMIN_USER[rolename]: username})
-                self.assertTrue(response.content.find('Error: unknown username "%s"' % username)>=0)
+                self.assertTrue(response.content.find('Error: unknown username "{0}"'.format(username))>=0)
 
     def test_add_forum_admin_users_for_missing_roles(self):
-        print "test_add_forum_admin_users_for_missing_roles"
         course = self.toy
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
         username = 'u1'
         for action in ['Add', 'Remove']:
             for rolename in FORUM_ROLES:
                 response = self.client.post(url, {'action': action_name(action, rolename), FORUM_ADMIN_USER[rolename]: username})
-                self.assertTrue(response.content.find('Error: unknown rolename "%s"' % rolename)>=0)
+                self.assertTrue(response.content.find('Error: unknown rolename "{0}"'.format(rolename))>=0)
 
     def test_remove_forum_admin_users_for_missing_users(self):
-        print "test_remove_forum_admin_users_for_missing_users"
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
@@ -160,24 +154,22 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
         action = 'Remove'
         for rolename in FORUM_ROLES:
             response = self.client.post(url, {'action': action_name(action, rolename), FORUM_ADMIN_USER[rolename]: username})
-            self.assertTrue(response.content.find('Error: user "%s" does not have rolename "%s"' % (username, rolename))>=0)
+            self.assertTrue(response.content.find('Error: user "{0}" does not have rolename "{1}"'.format(username, rolename))>=0)
 
     def test_add_and_remove_forum_admin_users(self):
-        print "test_add_and_remove_forum_admin_users"
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
         username = 'u2'
         for rolename in FORUM_ROLES:
             response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
-            self.assertTrue(response.content.find('Added "%s" to "%s" forum role = "%s"' % (username, course.id, rolename))>=0)
+            self.assertTrue(response.content.find('Added "{0}" to "{1}" forum role = "{2}"'.format(username, course.id, rolename))>=0)
             self.assertTrue(has_forum_access(username, course.id, rolename))
             response = self.client.post(url, {'action': action_name('Remove', rolename), FORUM_ADMIN_USER[rolename]: username})
-            self.assertTrue(response.content.find('Removed "%s" from "%s" forum role = "%s"' % (username, course.id, rolename))>=0)
+            self.assertTrue(response.content.find('Removed "{0}" from "{1}" forum role = "{2}"'.format(username, course.id, rolename))>=0)
             self.assertFalse(has_forum_access(username, course.id, rolename))
 
     def test_add_and_readd_forum_admin_users(self):
-        print "test_add_and_readd_forum_admin_users"
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
@@ -186,36 +178,34 @@ class TestInstructorDashboardForumAdmin(ct.PageLoader):
             # perform an add, and follow with a second identical add:
             self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
             response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
-            self.assertTrue(response.content.find('Error: user "%s" already has rolename "%s", cannot add' % (username, rolename))>=0)
+            self.assertTrue(response.content.find('Error: user "{0}" already has rolename "{1}", cannot add'.format(username, rolename))>=0)
             self.assertTrue(has_forum_access(username, course.id, rolename))
 
     def test_add_nonstaff_forum_admin_users(self):
-        print "test_add_and_readd_forum_admin_users"
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
         username = 'u1'
         rolename = FORUM_ROLE_ADMINISTRATOR
         response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
-        self.assertTrue(response.content.find('Error: user "%s" should first be added as staff' % username)>=0)
+        self.assertTrue(response.content.find('Error: user "{0}" should first be added as staff'.format(username))>=0)
 
     def test_list_forum_admin_users(self):
-        print "test_list_forum_admin_users"
         course = self.toy
         self.initialize_roles(course.id)
         url = reverse('instructor_dashboard', kwargs={'course_id': course.id})
         username = 'u2'
-        added_roles = ['Student']  # u2 is already added as a student to the discussion forums
+        added_roles = [FORUM_ROLE_STUDENT]  # u2 is already added as a student to the discussion forums
         self.assertTrue(has_forum_access(username, course.id, 'Student'))
         for rolename in FORUM_ROLES:
             response = self.client.post(url, {'action': action_name('Add', rolename), FORUM_ADMIN_USER[rolename]: username})
             self.assertTrue(has_forum_access(username, course.id, rolename))
             response = self.client.post(url, {'action': action_name('List', rolename), FORUM_ADMIN_USER[rolename]: username})
             for header in ['Username', 'Full name', 'Roles']:
-                self.assertTrue(response.content.find('<th>%s</th>' % header)>0)
-            self.assertTrue(response.content.find('<td>%s</td>' % username)>=0)
+                self.assertTrue(response.content.find('<th>{0}</th>'.format(header))>0)
+            self.assertTrue(response.content.find('<td>{0}</td>'.format(username))>=0)
             # concatenate all roles for user, in sorted order:
             added_roles.append(rolename)
             added_roles.sort()
             roles = ', '.join(added_roles)
-            self.assertTrue(response.content.find('<td>%s</td>' % roles)>=0, 'not finding roles "%s"' % roles)
+            self.assertTrue(response.content.find('<td>{0}</td>'.format(roles))>=0, 'not finding roles "{0}"'.format(roles))
diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py
index 2ae383e25b..f985cc43a0 100644
--- a/lms/djangoapps/instructor/views.py
+++ b/lms/djangoapps/instructor/views.py
@@ -1,34 +1,31 @@
 # ======== Instructor views =============================================================================
 
+from collections import defaultdict
 import csv
 import logging
 import os
 import urllib
 
-import track.views
-
-from collections import defaultdict
-
 from django.conf import settings
 from django.contrib.auth.models import User, Group
 from django.http import HttpResponse
-from mitxmako.shortcuts import render_to_response
 from django_future.csrf import ensure_csrf_cookie
 from django.views.decorators.cache import cache_control
+from mitxmako.shortcuts import render_to_response
 
 from courseware import grades
 from courseware.access import has_access, get_access_group_name
 from courseware.courses import get_course_with_access 
+from django_comment_client.models import Role, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA
+from django_comment_client.utils import has_forum_access
 from psychometrics import psychoanalyze
-
 from student.models import CourseEnrollment
 from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
 from xmodule.modulestore.search import path_to_location
-from django_comment_client.models import Role, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA
-from django_comment_client.utils import has_forum_access
+import track.views
 
 log = logging.getLogger("mitx.courseware")
 
@@ -50,8 +47,6 @@ def instructor_dashboard(request, course_id):
     forum_admin_access = has_forum_access(request.user, course_id, FORUM_ROLE_ADMINISTRATOR)
 
     msg = ''
-    #msg += ('POST=%s' % dict(request.POST)).replace('<','&lt;')
-
     problems = []
     plots = []
 
@@ -79,7 +74,7 @@ def instructor_dashboard(request, course_id):
 
     def return_csv(fn, datatable):
         response = HttpResponse(mimetype='text/csv')
-        response['Content-Disposition'] = 'attachment; filename=%s' % fn
+        response['Content-Disposition'] = 'attachment; filename={0}'.format(fn)
         writer = csv.writer(response, dialect='excel', quotechar='"', quoting=csv.QUOTE_ALL)
         writer.writerow(datatable['header'])
         for datarow in datatable['data']:
@@ -102,75 +97,75 @@ def instructor_dashboard(request, course_id):
     if settings.MITX_FEATURES['ENABLE_MANUAL_GIT_RELOAD']:
         if 'GIT pull' in action:
             data_dir = course.metadata['data_dir']
-            log.debug('git pull %s' % (data_dir))
+            log.debug('git pull {0}'.format(data_dir))
             gdir = settings.DATA_DIR / data_dir
             if not os.path.exists(gdir):
-                msg += "====> ERROR in gitreload - no such directory %s" % gdir
+                msg += "====> ERROR in gitreload - no such directory {0}".format(gdir)
             else:
-                cmd = "cd %s; git reset --hard HEAD; git clean -f -d; git pull origin; chmod g+w course.xml" % gdir
-                msg += "git pull on %s:<p>" % data_dir
-                msg += "<pre>%s</pre></p>" % escape(os.popen(cmd).read())
-                track.views.server_track(request, 'git pull %s' % data_dir, {}, page='idashboard')
+                cmd = "cd {0}; git reset --hard HEAD; git clean -f -d; git pull origin; chmod g+w course.xml".format(gdir)
+                msg += "git pull on {0}:<p>".format(data_dir)
+                msg += "<pre>{0}</pre></p>".format(escape(os.popen(cmd).read()))
+                track.views.server_track(request, 'git pull {0}'.format(data_dir), {}, page='idashboard')
 
         if 'Reload course' in action:
-            log.debug('reloading %s (%s)' % (course_id, course))
+            log.debug('reloading {0} ({1})'.format(course_id, course))
             try:
                 data_dir = course.metadata['data_dir']
                 modulestore().try_load_course(data_dir)
-                msg += "<br/><p>Course reloaded from %s</p>" % data_dir
-                track.views.server_track(request, 'reload %s' % data_dir, {}, page='idashboard')
+                msg += "<br/><p>Course reloaded from {0}</p>".format(data_dir)
+                track.views.server_track(request, 'reload {0}'.format(data_dir), {}, page='idashboard')
                 course_errors = modulestore().get_item_errors(course.location)
                 msg += '<ul>'
                 for cmsg, cerr in course_errors:
-                    msg += "<li>%s: <pre>%s</pre>" % (cmsg,escape(cerr))
+                    msg += "<li>{0}: <pre>{1}</pre>".format(cmsg,escape(cerr))
                 msg += '</ul>'
             except Exception as err:
-                msg += '<br/><p>Error: %s</p>' % escape(err)
+                msg += '<br/><p>Error: {0}</p>'.format(escape(err))
 
     if action == 'Dump list of enrolled students':
         log.debug(action)
         datatable = get_student_grade_summary_data(request, course, course_id, get_grades=False)
-        datatable['title'] = 'List of students enrolled in %s' % course_id
+        datatable['title'] = 'List of students enrolled in {0}'.format(course_id)
         track.views.server_track(request, 'list-students', {}, page='idashboard')
 
     elif 'Dump Grades' in action:
         log.debug(action)
         datatable = get_student_grade_summary_data(request, course, course_id, get_grades=True)
-        datatable['title'] = 'Summary Grades of students enrolled in %s' % course_id
+        datatable['title'] = 'Summary Grades of students enrolled in {0}'.format(course_id)
         track.views.server_track(request, 'dump-grades', {}, page='idashboard')
 
     elif 'Dump all RAW grades' in action:
         log.debug(action)
         datatable = get_student_grade_summary_data(request, course, course_id, get_grades=True,
                                                    get_raw_scores=True)
-        datatable['title'] = 'Raw Grades of students enrolled in %s' % course_id
+        datatable['title'] = 'Raw Grades of students enrolled in {0}'.format(course_id)
         track.views.server_track(request, 'dump-grades-raw', {}, page='idashboard')
 
     elif 'Download CSV of all student grades' in action:
         track.views.server_track(request, 'dump-grades-csv', {}, page='idashboard')
-        return return_csv('grades_%s.csv' % course_id,
+        return return_csv('grades_{0}.csv'.format(course_id),
                           get_student_grade_summary_data(request, course, course_id))
 
     elif 'Download CSV of all RAW grades' in action:
         track.views.server_track(request, 'dump-grades-csv-raw', {}, page='idashboard')
-        return return_csv('grades_%s_raw.csv' % course_id,
+        return return_csv('grades_{0}_raw.csv'.format(course_id),
                           get_student_grade_summary_data(request, course, course_id, get_raw_scores=True))
 
     elif 'Download CSV of answer distributions' in action:
         track.views.server_track(request, 'dump-answer-dist-csv', {}, page='idashboard')
-        return return_csv('answer_dist_%s.csv' % course_id, get_answers_distribution(request, course_id))
+        return return_csv('answer_dist_{0}.csv'.format(course_id), get_answers_distribution(request, course_id))
 
     #----------------------------------------
     # Admin
 
     elif 'List course staff' in action:
         group = get_staff_group(course)
-        msg += 'Staff group = %s' % group.name
-        log.debug('staffgrp=%s' % group.name)
+        msg += 'Staff group = {0}'.format(group.name)
+        log.debug('staffgrp={0}'.format(group.name))
         uset = group.user_set.all()
         datatable = {'header': ['Username', 'Full name']}
         datatable['data'] = [[x.username, x.profile.name] for x in uset]
-        datatable['title'] = 'List of Staff in course %s' % course_id
+        datatable['title'] = 'List of Staff in course {0}'.format(course_id)
         track.views.server_track(request, 'list-staff', {}, page='idashboard')
 
     elif action == 'Add course staff':
@@ -178,28 +173,28 @@ def instructor_dashboard(request, course_id):
         try:
             user = User.objects.get(username=uname)
         except User.DoesNotExist:
-            msg += '<font color="red">Error: unknown username "%s"</font>' % uname
+            msg += '<font color="red">Error: unknown username "{0}"</font>'.format(uname)
             user = None
         if user is not None:
             group = get_staff_group(course)
-            msg += '<font color="green">Added %s to staff group = %s</font>' % (user, group.name)
-            log.debug('staffgrp=%s' % group.name)
+            msg += '<font color="green">Added {0} to staff group = {1}</font>'.format(user, group.name)
+            log.debug('staffgrp={0}'.format(group.name))
             user.groups.add(group)
-            track.views.server_track(request, 'add-staff %s' % user, {}, page='idashboard')
+            track.views.server_track(request, 'add-staff {0}'.format(user), {}, page='idashboard')
 
     elif action == 'Remove course staff':
         uname = request.POST['staffuser']
         try:
             user = User.objects.get(username=uname)
         except User.DoesNotExist:
-            msg += '<font color="red">Error: unknown username "%s"</font>' % uname
+            msg += '<font color="red">Error: unknown username "{0}"</font>'.format(uname)
             user = None
         if user is not None:
             group = get_staff_group(course)
-            msg += '<font color="green">Removed %s from staff group = %s</font>' % (user, group.name)
-            log.debug('staffgrp=%s' % group.name)
+            msg += '<font color="green">Removed {0} from staff group = {1}</font>'.format(user, group.name)
+            log.debug('staffgrp={0}'.format(group.name))
             user.groups.remove(group)
-            track.views.server_track(request, 'remove-staff %s' % user, {}, page='idashboard')
+            track.views.server_track(request, 'remove-staff {0}'.format(user), {}, page='idashboard')
 
     #----------------------------------------
     # forum administration
@@ -208,55 +203,55 @@ def instructor_dashboard(request, course_id):
         rolename = FORUM_ROLE_ADMINISTRATOR
         datatable = {}
         msg += _list_course_forum_members(course_id, rolename, datatable)
-        track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
+        track.views.server_track(request, 'list-{0}'.format(rolename), {}, page='idashboard')
         
     
     elif action == 'Remove forum admin':
         uname = request.POST['forumadmin']
         msg += _update_forum_role_membership(uname, course, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_REMOVE)
-        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
+        track.views.server_track(request, '{0} {1} as {2} for {3}'.format(FORUM_ROLE_REMOVE, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
                                  {}, page='idashboard')
 
     elif action == 'Add forum admin':
         uname = request.POST['forumadmin']
         msg += _update_forum_role_membership(uname, course, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_ADD)
-        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
+        track.views.server_track(request, '{0} {1} as {2} for {3}'.format(FORUM_ROLE_ADD, uname, FORUM_ROLE_ADMINISTRATOR, course_id), 
                                  {}, page='idashboard')
 
     elif action == 'List course forum moderators':
         rolename = FORUM_ROLE_MODERATOR
         datatable = {}
         msg += _list_course_forum_members(course_id, rolename, datatable)
-        track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
+        track.views.server_track(request, 'list-{0}'.format(rolename), {}, page='idashboard')
     
     elif action == 'Remove forum moderator':
         uname = request.POST['forummoderator']
         msg += _update_forum_role_membership(uname, course, FORUM_ROLE_MODERATOR, FORUM_ROLE_REMOVE)
-        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_MODERATOR, course_id), 
+        track.views.server_track(request, '{0} {1} as {2} for {3}'.format(FORUM_ROLE_REMOVE, uname, FORUM_ROLE_MODERATOR, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'Add forum moderator':
         uname = request.POST['forummoderator']
         msg += _update_forum_role_membership(uname, course, FORUM_ROLE_MODERATOR, FORUM_ROLE_ADD)
-        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_MODERATOR, course_id), 
+        track.views.server_track(request, '{0} {1} as {2} for {3}'.format(FORUM_ROLE_ADD, uname, FORUM_ROLE_MODERATOR, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'List course forum community TAs':
         rolename = FORUM_ROLE_COMMUNITY_TA
         datatable = {}
         msg += _list_course_forum_members(course_id, rolename, datatable)
-        track.views.server_track(request, 'list-%s' % rolename, {}, page='idashboard')
+        track.views.server_track(request, 'list-{0}'.format(rolename), {}, page='idashboard')
     
     elif action == 'Remove forum community TA':
         uname = request.POST['forummoderator']
         msg += _update_forum_role_membership(uname, course, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_REMOVE)
-        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_REMOVE, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
+        track.views.server_track(request, '{0} {1} as {2} for {3}'.format(FORUM_ROLE_REMOVE, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
                                  {}, page='idashboard')
     
     elif action == 'Add forum community TA':
         uname = request.POST['forummoderator']
         msg += _update_forum_role_membership(uname, course, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_ADD)
-        track.views.server_track(request, '%s %s as %s for %s' % (FORUM_ROLE_ADD, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
+        track.views.server_track(request, '{0} {1} as {2} for {3}'.format(FORUM_ROLE_ADD, uname, FORUM_ROLE_COMMUNITY_TA, course_id), 
                                  {}, page='idashboard')
 
     #----------------------------------------
@@ -266,7 +261,7 @@ def instructor_dashboard(request, course_id):
         problem = request.POST['Problem']
         nmsg, plots = psychoanalyze.generate_plots_for_problem(problem)
         msg += nmsg
-        track.views.server_track(request, 'psychometrics %s' % problem, {}, page='idashboard')
+        track.views.server_track(request, 'psychometrics {0}'.format(problem), {}, page='idashboard')
 
     if idash_mode=='Psychometrics':
         problems = psychoanalyze.problems_with_psychometric_data(course_id)
@@ -303,15 +298,15 @@ def _list_course_forum_members(course_id, rolename, datatable):
     '''
     # make sure datatable is set up properly for display first, before checking for errors
     datatable['header'] = ['Username', 'Full name', 'Roles']
-    datatable['title'] = 'List of Forum %ss in course %s' % (rolename, course_id)
+    datatable['title'] = 'List of Forum {0}s in course {1}'.format(rolename, course_id)
     datatable['data'] = [];
     try:
         role = Role.objects.get(name=rolename, course_id=course_id)
     except Role.DoesNotExist:
-        return '<font color="red">Error: unknown rolename "%s"</font>' % rolename
+        return '<font color="red">Error: unknown rolename "{0}"</font>'.format(rolename)
     uset = role.users.all().order_by('username')
-    msg = 'Role = %s' % rolename
-    log.debug('role=%s' % rolename)
+    msg = 'Role = {0}'.format(rolename)
+    log.debug('role={0}'.format(rolename))
     datatable['data'] = [[x.username, x.profile.name, ', '.join([r.name for r in x.roles.filter(course_id=course_id).order_by('name')])] for x in uset]
     return msg
 
@@ -332,31 +327,31 @@ def _update_forum_role_membership(uname, course, rolename, add_or_remove):
     try:
         user = User.objects.get(username=uname)
     except User.DoesNotExist:
-        return '<font color="red">Error: unknown username "%s"</font>' % uname
+        return '<font color="red">Error: unknown username "{0}"</font>'.format(uname)
     try:
         role = Role.objects.get(name=rolename, course_id=course.id)
     except Role.DoesNotExist:
-        return '<font color="red">Error: unknown rolename "%s"</font>' % rolename
+        return '<font color="red">Error: unknown rolename "{0}"</font>'.format(rolename)
 
     # check whether role already has the specified user:
     alreadyexists = role.users.filter(username=uname).exists()
     msg = ''
-    log.debug('rolename=%s' % rolename)
-    if (add_or_remove == FORUM_ROLE_REMOVE):
-        if (not alreadyexists):
-            msg ='<font color="red">Error: user "%s" does not have rolename "%s", cannot remove</font>' % (uname, rolename)
+    log.debug('rolename={0}'.format(rolename))
+    if add_or_remove == FORUM_ROLE_REMOVE:
+        if not alreadyexists:
+            msg ='<font color="red">Error: user "{0}" does not have rolename "{1}", cannot remove</font>'.format(uname, rolename)
         else: 
             user.roles.remove(role)
-            msg = '<font color="green">Removed "%s" from "%s" forum role = "%s"</font>' % (user, course.id, rolename)
+            msg = '<font color="green">Removed "{0}" from "{1}" forum role = "{2}"</font>'.format(user, course.id, rolename)
     else:
-        if (alreadyexists):
-            msg = '<font color="red">Error: user "%s" already has rolename "%s", cannot add</font>' % (uname, rolename)
+        if alreadyexists:
+            msg = '<font color="red">Error: user "{0}" already has rolename "{1}", cannot add</font>'.format(uname, rolename)
         else: 
             if (rolename == FORUM_ROLE_ADMINISTRATOR and not has_access(user, course, 'staff')):   
-                msg = '<font color="red">Error: user "%s" should first be added as staff before adding as a forum administrator, cannot add</font>' % uname
+                msg = '<font color="red">Error: user "{0}" should first be added as staff before adding as a forum administrator, cannot add</font>'.format(uname)
             else:
                 user.roles.add(role)
-                msg = '<font color="green">Added "%s" to "%s" forum role = "%s"</font>' % (user, course.id, rolename)
+                msg = '<font color="green">Added "{0}" to "{1}" forum role = "{2}"</font>'.format(user, course.id, rolename)
 
     return msg
     
@@ -385,7 +380,7 @@ def get_student_grade_summary_data(request, course, course_id, get_grades=True,
     if get_grades:
         # just to construct the header
         gradeset = grades.grade(enrolled_students[0], request, course, keep_raw_scores=get_raw_scores)
-        # log.debug('student %s gradeset %s' % (enrolled_students[0], gradeset))
+        # log.debug('student {0} gradeset {1}'.format(enrolled_students[0], gradeset))
         if get_raw_scores:
             header += [score.section for score in gradeset['raw_scores']]
         else:
@@ -403,7 +398,7 @@ def get_student_grade_summary_data(request, course, course_id, get_grades=True,
 
         if get_grades:
             gradeset = grades.grade(student, request, course, keep_raw_scores=get_raw_scores)
-            # log.debug('student=%s, gradeset=%s' % (student,gradeset))
+            # log.debug('student={0}, gradeset={1}'.format(student,gradeset))
             if get_raw_scores:
                 datarow += [score.earned for score in gradeset['raw_scores']]
             else: